Re: [PATCH] difftool: support repositories with .git-files

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 25, 2014, 18:02 UTC
Message-ID: <xmqq1tyr6pfw.fsf@gitster.dls.corp.google.com>
In-Reply-To: <530BB548.3050603@web.de>

Jens Lehmann <Jens.Lehmann@web.de> writes:

Show 45 quoted lines

> Am 24.02.2014 17:55, schrieb Junio C Hamano:
>> David Aguilar <davvid@gmail.com> writes:
>> 
>>> Modern versions of "git submodule" use .git-files to setup the
>>> submodule directory.  When run in a "git submodule"-created
>>> repository "git difftool --dir-diff" dies with the following
>>> error:
>>>
>>> 	$ git difftool -d HEAD~
>>> 	fatal: This operation must be run in a work tree
>>> 	diff --raw --no-abbrev -z HEAD~: command returned error: 128
>>>
>>> core.worktree is relative to the .git directory but the logic
>>> in find_worktree() does not account for it.
>>>
>>> Use `git rev-parse --show-toplevel` to find the worktree so that
>>> the dir-diff feature works inside a submodule.
>>>
>>> Reported-by: Gábor Lipták <gabor.liptak@gmail.com>
>>> Helped-by: Jens Lehmann <jens.lehmann@web.de>
>>> Helped-by: John Keeping <john@keeping.me.uk>
>>> Signed-off-by: David Aguilar <davvid@gmail.com>
>>> ---
>> 
>> Looks good; thanks.
>
>
> FWIW:
> Tested-by: Jens Lehmann <jens.lehmann@web.de>
>
> What about squashing this in to detect any future regressions?
>
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index 2418528..d86ad68 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -434,4 +434,12 @@ test_expect_success PERL 'difftool --no-symlinks detects conflict ' '
>  	)
>  '
>
> +test_expect_success PERL 'difftool properly honours gitlink and core.worktree' '
> +	git submodule add ./. submod/ule &&
> +	(
> +		cd submod/ule &&
> +		git difftool --tool=echo  --dir-diff --cached

In the context of this fix, finishing with 0 exit status may be all we care about, but do we also care about things like in what directory the tool is invoked in, what arguments and extra environment settings (if any) it is given, and stuff like that?

In fact, the "echo" in the above is very misleading. The test relies on the fact that immediately after the submod/ule is cloned, "diff --cached" does not have to call any tool backend---if you modify some tracked file in its working tree and dropped --cached on the command line, the command will fail with "Huh? I do not know what 'echo' diff/merge backend is", no?

> +	)
> +'
> +
>  test_done