git difftool does does not respect current working directory

git difftool does does not respect current working directory

[Frédéric Heitzmann]

Hello.

It is useful to compare the modified version of a file to HEAD's version, in
order to review changes before committing. gitk is fine for this but I often use
git difftool -t gvimdiff, so that I can rewrite code right into my diff editor.
Doing this, it is very likely that I open some more files (e.g. foo.h
corresponding to foo.c) in gvimdiff.
Unfortunately, 'git difftool' does not keep the current working directory while
launching gvimdiff.

=> Is it done on purpose ?
If not, it is probably a good idea to fix this.

I will be more than happy to contribute but I have some hard time to get
familiar with git source code.
Any help to locate the proper pieces of code would be realy appreciated.

--
Fred

Re: git difftool does does not respect current working directory

[Junio C Hamano]

Frédéric Heitzmann  <frederic.heitzmann@gmail.com> writes:

> Unfortunately, 'git difftool' does not keep the current working directory while
> launching gvimdiff.
>
> => Is it done on purpose ?
> If not, it is probably a good idea to fix this.

I will not comment on "on purpose?" part, as I do not use difftool myself.

But the right set of questions to ask is not the above, but these:

 - Is it on purpose that difftool runs its diff viewer from the top of the
   working tree?

 - Is there any existing user who depends on that current behaviour?  IOW,
   would anybody suffer if difftool suddenly starts to run the diff viewer
   from the subdirectory that the user started "git difftool" from?

If the answers to both of them are No, then it might be a good idea to
change the behaviour.

Re: git difftool does does not respect current working directory

[David Aguilar]

Hello,

On Sun, May 15, 2011 at 10:39:18PM -0700, Junio C Hamano wrote:
> Frédéric Heitzmann  <frederic.heitzmann@gmail.com> writes:
> 
> > Unfortunately, 'git difftool' does not keep the current working directory while
> > launching gvimdiff.
> >
> > => Is it done on purpose ?
> > If not, it is probably a good idea to fix this.
> 
> I will not comment on "on purpose?" part, as I do not use difftool myself.
> 
> But the right set of questions to ask is not the above, but these:
> 
>  - Is it on purpose that difftool runs its diff viewer from the top of the
>    working tree?
> 
>  - Is there any existing user who depends on that current behaviour?  IOW,
>    would anybody suffer if difftool suddenly starts to run the diff viewer
>    from the subdirectory that the user started "git difftool" from?
> 
> If the answers to both of them are No, then it might be a good idea to
> change the behaviour.

Another thing to consider is ensuring that there is a
consistency between 'diff' and 'difftool'.

When you run 'git diff' from a subdirectory git will show you
a diff against the entire tree.  In this respect, 'difftool' is
consistent.  In that sense, yes, this is very much "on purpose".

Is there something that isn't working because of the internal
chdir?  I'm not sure if you want to change the chdir behavior
for aesthetic purposes or if there's something it is
preventing you from doing.  If what you're trying to accomplish
is to have 'difftool' only show you changes within the current
directory then you can accomplish that today by passing ".",
e.g. "git difftool ."


Implementation details:

difftool is a thin wrapper around 'git diff'.  Specifically,
it is implemented as a $GIT_EXTERNAL_DIFF script which means
that it inherits most of its behavior from 'diff'.
The "chdir to root" behavior actually happens inside of
'git diff'.

Can we can change the behavior? Sure, anything is possible.
The question is *should* we change the behavior?
Even though I highly doubt there are any scripts relying on it,
I don't think we gain much by doing so.  The downside to
changing it is that we lose consistency, which is not so good.
I hope that's a compelling enough argument :-)

I hope the "." thing helps.

Cheers,
-- 
					David

Re: git difftool does does not respect current working directory

[David Aguilar]

On Thu, May 19, 2011 at 08:59:00PM -0700, David Aguilar wrote:
> 
> Hello,
> 
> On Sun, May 15, 2011 at 10:39:18PM -0700, Junio C Hamano wrote:
> > Frédéric Heitzmann  <frederic.heitzmann@gmail.com> writes:
> > 
> > > Unfortunately, 'git difftool' does not keep the current working directory while
> > > launching gvimdiff.
> > >
> > > => Is it done on purpose ?
> > > If not, it is probably a good idea to fix this.
> > 
> > [...snip...]
> > 
> > If the answers to both of them are No, then it might be a good idea to
> > change the behaviour.
> 
> Another thing to consider is ensuring that there is a
> consistency between 'diff' and 'difftool'.
> 
> When you run 'git diff' from a subdirectory git will show you
> a diff against the entire tree.  In this respect, 'difftool' is
> consistent.  In that sense, yes, this is very much "on purpose".
> 
> [...snip...]

I just realized that I was focusing on the "whole tree diff"
aspect which really is orthogonal to the "chdir to root"
done by 'git diff' when running $GIT_EXTERNAL_DIFF scripts.

I imagine you want to be able to use vimdiff while diffing
and open files, etc., from the current directory?

In that case, maybe we can have the best of both worlds --
whole-tree diff and keeping the current directory.
That would be kinda like what 'git status' does when it shows
you relative paths above and beside the current directory.

We would have to change the way $GIT_EXTERNAL_DIFF works so
that it preserves the current directory and constructs
paths relative to it.  Patches welcome :-)

I don't really know whether it would be a good thing to do,
though, since I have not dug too deep into how the extdiff
code is structured.
-- 
					David

Re: git difftool does does not respect current working directory

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> We would have to change the way $GIT_EXTERNAL_DIFF works so
> that it preserves the current directory and constructs
> paths relative to it.  Patches welcome :-)

I am afraild that would break a lot more than difftool.

If we really wanted to change the behaviour, the external diff interface
needs to export the value of prefix (i.e. what the original subdirectory
was), and the script that is spawned as $GIT_EXTERNAL_DIFF (optionally
optionally) take it into account, perhaps by cd'ing back to that
subdirectory and possibly moving or renaming the temporary files to suit
its needs (I think recently we also saw a request to rename the temporary
files).

Or something like that.

Re: git difftool does does not respect current working directory

[David Aguilar]

On Thu, May 19, 2011 at 09:31:54PM -0700, Junio C Hamano wrote:
> David Aguilar <davvid@gmail.com> writes:
> 
> > We would have to change the way $GIT_EXTERNAL_DIFF works so
> > that it preserves the current directory and constructs
> > paths relative to it.  Patches welcome :-)
> 
> I am afraild that would break a lot more than difftool.
> 
> If we really wanted to change the behaviour, the external diff interface
> needs to export the value of prefix (i.e. what the original subdirectory
> was), and the script that is spawned as $GIT_EXTERNAL_DIFF (optionally
> optionally) take it into account, perhaps by cd'ing back to that
> subdirectory and possibly moving or renaming the temporary files to suit
> its needs (I think recently we also saw a request to rename the temporary
> files).
> 
> Or something like that.

Yup, yup.  That's a lot of machinery for a relatively small
gain.  Simple is simple, simple is good.  Thanks for
outlining how someone could implement it, though.

I won't do it myself but if someone is motivated enough then
your email at least gives an idea about how to go about doing
it.  git-difftool--helper could chdir to $prefix and diff each
file with $(git rev-parse --show-cdup)/$path as the path since
it may no longer be at the root.

This seems very messy so I don't really want to sound too
encouraging about going down this route.  I probably
shouldn't have encouraged looking at the temporary files
thing in the other thread either.

Thanks,
-- 
					David

Re: git difftool does does not respect current working directory

[Frédéric Heitzmann]

Reading your replies, my understanding is :
- difftool is consistent with diff, and chdir to root directory. It is 
seems indeed very common to have diffs showing from the root directory.
- on the overhand, openning gvimdiff via difftool and having a new cwd 
is for sure not consistent with usual gvim text editing.

I am afraid I am going to need some gvim trick like :
$ git difftool -x "gvimdiff -f -d -c 'wincmd l' -c 'cd $PWD' " my_file

Not sure that it is less messy though ;-)
If there is no stronger need to adapt git-difftool, for gvimdiff or any 
other difftool, we could probably settle for it.

Thanks for you help.
--
Fred

Le 20/05/2011 06:48, David Aguilar a écrit :
> On Thu, May 19, 2011 at 09:31:54PM -0700, Junio C Hamano wrote:
>> David Aguilar<davvid@gmail.com>  writes:
>>
>>> We would have to change the way $GIT_EXTERNAL_DIFF works so
>>> that it preserves the current directory and constructs
>>> paths relative to it.  Patches welcome :-)
>> I am afraild that would break a lot more than difftool.
>>
>> If we really wanted to change the behaviour, the external diff interface
>> needs to export the value of prefix (i.e. what the original subdirectory
>> was), and the script that is spawned as $GIT_EXTERNAL_DIFF (optionally
>> optionally) take it into account, perhaps by cd'ing back to that
>> subdirectory and possibly moving or renaming the temporary files to suit
>> its needs (I think recently we also saw a request to rename the temporary
>> files).
>>
>> Or something like that.
> Yup, yup.  That's a lot of machinery for a relatively small
> gain.  Simple is simple, simple is good.  Thanks for
> outlining how someone could implement it, though.
>
> I won't do it myself but if someone is motivated enough then
> your email at least gives an idea about how to go about doing
> it.  git-difftool--helper could chdir to $prefix and diff each
> file with $(git rev-parse --show-cdup)/$path as the path since
> it may no longer be at the root.
>
> This seems very messy so I don't really want to sound too
> encouraging about going down this route.  I probably
> shouldn't have encouraged looking at the temporary files
> thing in the other thread either.
>
> Thanks,

Re: git difftool does does not respect current working directory

[David Aguilar]

On Sat, May 21, 2011 at 11:35:06AM +0200, Frédéric Heitzmann wrote:
> Reading your replies, my understanding is :
> - difftool is consistent with diff, and chdir to root directory. It
> is seems indeed very common to have diffs showing from the root
> directory.
> - on the overhand, openning gvimdiff via difftool and having a new
> cwd is for sure not consistent with usual gvim text editing.
> 
> I am afraid I am going to need some gvim trick like :
> $ git difftool -x "gvimdiff -f -d -c 'wincmd l' -c 'cd $PWD' " my_file
> 
> Not sure that it is less messy though ;-)
> If there is no stronger need to adapt git-difftool, for gvimdiff or
> any other difftool, we could probably settle for it.

I think updating git-difftool--helper.sh to pass a chdir to vim
might be just the thing to do.  git-difftool.perl can be
updated to set $GIT_DIFFTOOL_PWD so that the helper can use it
as -c 'cd $GIT_DIFFTOOL_PWD'.  I'll see if I can whip up a patch
in a lil bit.
-- 
					David

Re: git difftool does does not respect current working directory

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> I think updating git-difftool--helper.sh to pass a chdir to vim
> might be just the thing to do.  git-difftool.perl can be
> updated to set $GIT_DIFFTOOL_PWD so that the helper can use it
> as -c 'cd $GIT_DIFFTOOL_PWD'.  I'll see if I can whip up a patch
> in a lil bit.

Hmm, would this benefit from sharing some concepts with 7cf16a1
(handle_alias: provide GIT_PREFIX to !alias, 2011-04-27)?

If it helps, we might want to uniformly give this information to all
external processes and programs, including hooks.

Re: git difftool does does not respect current working directory

[David Aguilar]

On Sat, May 21, 2011 at 11:30:58PM -0700, Junio C Hamano wrote:
> David Aguilar <davvid@gmail.com> writes:
> 
> > I think updating git-difftool--helper.sh to pass a chdir to vim
> > might be just the thing to do.  git-difftool.perl can be
> > updated to set $GIT_DIFFTOOL_PWD so that the helper can use it
> > as -c 'cd $GIT_DIFFTOOL_PWD'.  I'll see if I can whip up a patch
> > in a lil bit.
> 
> Hmm, would this benefit from sharing some concepts with 7cf16a1
> (handle_alias: provide GIT_PREFIX to !alias, 2011-04-27)?
> 
> If it helps, we might want to uniformly give this information to all
> external processes and programs, including hooks.

Yes, this would be very helpful.
Without this my patch would have to touch git-difftool.perl,
git-mergetool.sh, and git-mergetool--helper.sh.

With $GIT_PREFIX it would touch git-mergetool--helper.sh only.
-- 
					David

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

[David Aguilar]

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");
+
 	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
+		git checkout -- two
+	) &&
+	test_cmp expect actual
+'
+
 test_expect_success 'no file/rev ambiguity check inside .git' '
 	git commit -a -m 1 &&
 	(
-- 
1.7.5.2.317.g391b14

[PATCH 2/3] git: Remove handling for GIT_PREFIX

[David Aguilar]

handle_alias() no longer needs to set GIT_PREFIX since it is defined
in setup_git_directory_gently().  Remove the duplicated effort and use
run_command_v_opt() since there is no need to setup the environment.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/git.c b/git.c
index a5ef3c6..60a9403 100644
--- a/git.c
+++ b/git.c
@@ -185,8 +185,6 @@ static int handle_alias(int *argcp, const char ***argv)
 		if (alias_string[0] == '!') {
 			const char **alias_argv;
 			int argc = *argcp, i;
-			struct strbuf sb = STRBUF_INIT;
-			const char *env[2];
 
 			commit_pager_choice();
 
@@ -197,13 +195,7 @@ static int handle_alias(int *argcp, const char ***argv)
 				alias_argv[i] = (*argv)[i];
 			alias_argv[argc] = NULL;
 
-			strbuf_addstr(&sb, "GIT_PREFIX=");
-			if (subdir)
-				strbuf_addstr(&sb, subdir);
-			env[0] = sb.buf;
-			env[1] = NULL;
-			ret = run_command_v_opt_cd_env(alias_argv, RUN_USING_SHELL, NULL, env);
-			strbuf_release(&sb);
+			ret = run_command_v_opt(alias_argv, RUN_USING_SHELL);
 			if (ret >= 0)   /* normal exit */
 				exit(ret);
 
-- 
1.7.5.2.317.g391b14

[PATCH 3/3] git-mergetool--lib: Make vimdiff retain the current directory

[David Aguilar]

When using difftool with vimdiff it can be unexpected that
the current directory changes to the root of the project.
Tell vim to chdir to the value of $GIT_PREFIX to fix this.

Care is taken to quote the variable so that vim expands it.
This avoids problems when directory names contain spaces.

Signed-off-by: David Aguilar <davvid@gmail.com>
Reported-by: Frédéric Heitzmann <frederic.heitzmann@gmail.com>
---
 git-mergetool--lib.sh |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 4db9212..ece6a08 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -187,7 +187,9 @@ run_merge_tool () {
 			fi
 			check_unchanged
 		else
+			resolve_git_prefix
 			"$merge_tool_path" -R -f -d -c "wincmd l" \
+				-c 'cd $GIT_PREFIX' \
 				"$LOCAL" "$REMOTE"
 		fi
 		;;
@@ -198,7 +200,9 @@ run_merge_tool () {
 				"$LOCAL" "$MERGED" "$REMOTE"
 			check_unchanged
 		else
+			resolve_git_prefix
 			"$merge_tool_path" -R -f -d -c "wincmd l" \
+				-c 'cd $GIT_PREFIX' \
 				"$LOCAL" "$REMOTE"
 		fi
 		;;
@@ -437,3 +441,12 @@ get_merge_tool () {
 	fi
 	echo "$merge_tool"
 }
+
+resolve_git_prefix() {
+	# If GIT_PREFIX is empty then we cannot use it in tools
+	# that expect to be able to chdir() to its value.
+	if test -z "$GIT_PREFIX"; then
+		GIT_PREFIX=.
+		export GIT_PREFIX
+	fi
+}
-- 
1.7.5.2.317.g391b14

Re: [PATCH 3/3] git-mergetool--lib: Make vimdiff retain the current directory

[Michael J Gruber]

David Aguilar venit, vidit, dixit 22.05.2011 11:57:
> When using difftool with vimdiff it can be unexpected that
> the current directory changes to the root of the project.
> Tell vim to chdir to the value of $GIT_PREFIX to fix this.
> 
> Care is taken to quote the variable so that vim expands it.
> This avoids problems when directory names contain spaces.
> 
> Signed-off-by: David Aguilar <davvid@gmail.com>
> Reported-by: Frédéric Heitzmann <frederic.heitzmann@gmail.com>
> ---
>  git-mergetool--lib.sh |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 4db9212..ece6a08 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -187,7 +187,9 @@ run_merge_tool () {
>  			fi
>  			check_unchanged
>  		else
> +			resolve_git_prefix
>  			"$merge_tool_path" -R -f -d -c "wincmd l" \
> +				-c 'cd $GIT_PREFIX' \
>  				"$LOCAL" "$REMOTE"
>  		fi
>  		;;
> @@ -198,7 +200,9 @@ run_merge_tool () {
>  				"$LOCAL" "$MERGED" "$REMOTE"
>  			check_unchanged
>  		else
> +			resolve_git_prefix
>  			"$merge_tool_path" -R -f -d -c "wincmd l" \
> +				-c 'cd $GIT_PREFIX' \
>  				"$LOCAL" "$REMOTE"
>  		fi
>  		;;
> @@ -437,3 +441,12 @@ get_merge_tool () {
>  	fi
>  	echo "$merge_tool"
>  }
> +
> +resolve_git_prefix() {
> +	# If GIT_PREFIX is empty then we cannot use it in tools
> +	# that expect to be able to chdir() to its value.
> +	if test -z "$GIT_PREFIX"; then
> +		GIT_PREFIX=.
> +		export GIT_PREFIX
> +	fi
> +}

Hmmm. Maybe we should export "." when there is no prefix? Maybe it's not
too late to change that aspect of GIT_PREFIX. We went through some
iteration back then for !alias.

Michael

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

[David Aguilar]

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),
-- 
                                        David

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

[Michael J Gruber]

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

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

[Ævar Arnfjörð Bjarmason]

On Sun, May 22, 2011 at 11:57, David Aguilar <davvid@gmail.com> wrote:
> +       printf "#!/bin/sh\n" >diff &&
> +       printf "printf \"\$GIT_PREFIX\"\n" >>diff &&

If you're going to use /bin/sh (which might be a non-POSIX shell) it's
probably better to use echo than rely on printf understanding \n.

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> I guess external scripts can call rev-parse --show-prefix themselves?

That has always been the case, I think, and it shouldn't be a problem.

The real reason you want the new GIT_PREFIX for alias/hooks is otherwise
they would not have a way to even say --show-prefix to figure it out
themselves.

>> 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?

As long as there is a way for a script to figure it out when it wants to
know, I think it should be Ok.

Isn't it just the matter of reading --show-prefix and comparing it with
what came in $GIT_PREFIX?

Re: [PATCH 3/3] git-mergetool--lib: Make vimdiff retain the current directory

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> +resolve_git_prefix() {
> +	# If GIT_PREFIX is empty then we cannot use it in tools
> +	# that expect to be able to chdir() to its value.
> +	if test -z "$GIT_PREFIX"; then
> +		GIT_PREFIX=.
> +		export GIT_PREFIX
> +	fi
> +}

Does this "export" have to be conditional?  Otherwise, it would be simpler
to do this upfront at the beginning:

    : GIT_PREFIX=${GIT_PREFIX:-.}

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 23.05.2011 18:43:
> David Aguilar <davvid@gmail.com> writes:
> 
>> I guess external scripts can call rev-parse --show-prefix themselves?
> 
> That has always been the case, I think, and it shouldn't be a problem.
> 
> The real reason you want the new GIT_PREFIX for alias/hooks is otherwise
> they would not have a way to even say --show-prefix to figure it out
> themselves.
> 
>>> 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?
> 
> As long as there is a way for a script to figure it out when it wants to
> know, I think it should be Ok.
> 
> Isn't it just the matter of reading --show-prefix and comparing it with
> what came in $GIT_PREFIX?

Yep, one is before and one is after any eventual cd'ing which git may
do. I just wanted to point out the difference. And the technical
difference (env var. vs. rev-parse option) is due to that difference
(and thus natural).

Michael

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

[David Aguilar]

On Mon, May 23, 2011 at 02:09:40PM +0200, Ævar Arnfjörð Bjarmason wrote:
> On Sun, May 22, 2011 at 11:57, David Aguilar <davvid@gmail.com> wrote:
> > +       printf "#!/bin/sh\n" >diff &&
> > +       printf "printf \"\$GIT_PREFIX\"\n" >>diff &&
> 
> If you're going to use /bin/sh (which might be a non-POSIX shell) it's
> probably better to use echo than rely on printf understanding \n.

I'll reroll a v2 of these patches using echo instead of printf.
The mergetool--lib patch will make the test -z "$GIT_PREFIX"
check happen unconditionally as you suggested, Junio.

Another thought was that I could have implemented the
mergetool--lib patch without $GIT_PREFIX at all and just called
rev-parse --show-prefix explicitly.  The change has merits,
though, and I'll consider mergetool--lib not using rev-parse
as an optimization.  Afterall, fork+exec is expensive on
Windows so doing without an additional call is nicer for our
msysgit brothers.

Thank you both.
-- 
					David