Jeff King <p...@peff.net> 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.
