Subject: Re: [PATCH v3 4/4] trace.c: be smart about what env to print in trace_run_command()

From: Junio C Hamano <[email-masked]>
Date: Fri, 12 Jan 2018 10:24:28 -0800

Jeff King <[email-masked]> writes:

>> +		/*
>> +		 * Do we have a sequence of "unset GIT_DIR; GIT_DIR=foo"?
>> +		 * Then don't bother with the unset thing.
>> +		 */
>> +		if (i + 1 < envs.nr &&
>> +		    !strcmp(env, envs.items[i + 1].string))
>>  			continue;
>
> Are we guaranteed that "FOO" and "FOO=bar" appear next to each other in the
> sorted list? I think "FOO123=bar" could come between.

At this point, envs is a string list whose key is FOO for both "FOO"
(unset) and "FOO=bar" (set); "FOO123=bar" would sort after these two.

But I did not see anything that attempts to guarantee that "FOO"
sorts before "FOO=bar" with string_list_sort().  If the sort used in
the function is stable, and if the case we care about is unset
followed by set, then the above would catch the case, but even if
that were the case, it is unclear what we want to do when a set of
FOO is followed by an unset of FOO.

And if the sort is not stable, then the above code is just simply
broken.

> I also think this is a special case of a more general problem. FOO could
> appear any number of times in the "env" array, as a deletion or with
> multiple values. Our prep_childenv() would treat that as "last one
> wins", I think. Could we just do the same here?

Yeah, if the last one is to set FOO=bar, then it feels sufficient to
just check if FOO is set to bar in the original and deciding to show
or hide; similarly if the last one is to unset FOO, it does not matter
if the caller sets it to some other value before unsetting, so it
feels sufficient to just check if FOO is set to anything in the
original environment.