Re: [PATCH 1/3] setup: Provide GIT_PREFIX to built-ins

[Michael J Gruber <git@drmicha.warpmail.net>]

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@drmicha.warpmail.net> 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@gmail.com>
>>> ---
>>> 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