Re: [PATCH 1/3] setup: Provide GIT_PREFIX to built-ins
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:
Show 31 quoted lines
> 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.
Show 26 quoted lines
>
>> 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?
Show 16 quoted lines
>
>> + 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),
--
DavidDavid Aguilar <davvid@gmail.com>