David Aguilar venit, vidit, dixit 23.05.2011 10:40:
> Added git@vger to the cc list. I sent y'all two copies of these patches because I forgot to cc the list the first time...
>
> On May 22, 2011, at 11:35 PM, Michael J Gruber <git@xxxxxxxxxxxxxxxxxxxx> wrote:
>
>> David Aguilar venit, vidit, dixit 22.05.2011 11:54:
>>> GIT_PREFIX was added in 7cf16a14f5c070f7b14cf28023769450133172ae so that
>>> aliases can know the directory from which a !alias was called.
>>>
>>> Knowing the prefix relative to the root is helpful in other programs
>>> so export it to built-ins as well.
>>>
>>> Signed-off-by: David Aguilar <davvid@xxxxxxxxx>
>>> ---
>>> setup.c | 6 ++++++
>>> t/t1020-subdirectory.sh | 16 ++++++++++++++++
>>> 2 files changed, 22 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/setup.c b/setup.c
>>> index b6e6b5a..fc169a4 100644
>>> --- a/setup.c
>>> +++ b/setup.c
>>> @@ -603,6 +603,12 @@ const char *setup_git_directory_gently(int *nongit_ok)
>>> const char *prefix;
>>>
>>> prefix = setup_git_directory_gently_1(nongit_ok);
>>> + /* Provide the prefix to all external processes and programs */
>>> + if (prefix)
>>> + setenv("GIT_PREFIX", prefix, 1);
>>> + else
>>> + unsetenv("GIT_PREFIX");
>>> +
>>
>> Do we really want to unset it? This is different from the existing
>> behaviour (but not more useful). But see also my comment on 3/3: We may
>> want to do something different which is also more useful.
>
> I thought the behavior was actually the same.
>
> The current alias code sets the value GIT_PREFIX= in the strbuf. When prefix is empty nothing else is added to the strbuf. The run_command function actually checks for FOO= with empty after the equals sign and translates it into unsetenv. That allows code to unset vars using that interface.
>
> If we can do something better that'd be good. Unsetting the variable also protects us from whatever randomness might be in there, which was my primary concern.
>
>>
>>> if (startup_info) {
>>> startup_info->have_repository = !nongit_ok || !*nongit_ok;
>>> startup_info->prefix = prefix;
>>> diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
>>> index ddc3921..a85b594 100755
>>> --- a/t/t1020-subdirectory.sh
>>> +++ b/t/t1020-subdirectory.sh
>>> @@ -139,6 +139,22 @@ test_expect_success 'GIT_PREFIX for !alias' '
>>> test_cmp expect actual
>>> '
>>>
>>> +test_expect_success 'GIT_PREFIX for built-ins' '
>>> + # Use GIT_EXTERNAL_DIFF to test that the "diff" built-in
>>> + # receives the GIT_PREFIX variable.
>>> + printf "dir/" >expect &&
>>> + printf "#!/bin/sh\n" >diff &&
>>> + printf "printf \"\$GIT_PREFIX\"\n" >>diff &&
>>> + chmod +x diff &&
>>> + (
>>> + cd dir &&
>>> + printf "change" >two &&
>>> + env GIT_EXTERNAL_DIFF=./diff git diff >../actual
>>
>> In passsing, this also tests the fact that GIT_EXTERNAL_DIFF is relative
>> to the repo root and not to cwd...
>
> That's true. Another thing is that this only affects built-ins. I wanted to set the variable for git-foo external programs too but that means adding a call to setup_git_directory_gently() which we currently do not do in that codepath. I guess external scripts can call rev-parse --show-prefix themselves? Or is this worth making consistent?
>
>>
>>> + git checkout -- two
>>> + ) &&
>>> + test_cmp expect actual
>>> +'
>>> +
>>> test_expect_success 'no file/rev ambiguity check inside .git' '
>>> git commit -a -m 1 &&
>>> (
>>
>> Overall I think it's a good change, btw. But it leaves it up to the
>> (script) user to know whether git has actually changed the cwd or not,
>> i.e.: Is $(pwd) where the user called us from or $(pwd)/$GIT_PREFIX?
>> That may may be a non-issue, though.
>>
>> Michael
>
> It's a non-issue for my use case but I can see it being confusing.
>
> For example, mergetool--lib's merge mode codepath can be run from subdirectories. The diff mode codepaths all assume that we are at the root (because git diff put us there).
>
> Thanks (and please let me know if my unsetenv analysis is incorrect),
Our run_command() would convert "GIT_PREFIX" into an unsetenv(), but it
leaves "GIT_PREFIX=" to be a putenv(). E.g., the alias
env = !sh -c 'set -u && echo $GIT_PREFIX'
gives an empty result but errors out when you misspell GIT_PREFIX
intentionally.
I don't mind either way. "set -u" is a good script checker, and I seem
to remember that on Windows, we do something extra do keep the
distinction between unset and null. Can't find it right now.
Michael
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html