Re: [PATCH 2/2] mergetool: fix running mergetool in sub-directories

[Charles Bailey <charles@hashpling.org>]

Junio C Hamano wrote:
> Charles Bailey <charles@hashpling.org> writes:
> 
>> diff --git a/git-mergetool.sh b/git-mergetool.sh
>> index aefdca7..87fa88a 100755
>> --- a/git-mergetool.sh
>> +++ b/git-mergetool.sh
>> @@ -13,7 +13,6 @@ SUBDIRECTORY_OK=Yes
>>  OPTIONS_SPEC=
>>  . git-sh-setup
>>  require_work_tree
>> -prefix=$(git rev-parse --show-prefix)
>>  
>>  # Returns true if the mode reflects a symlink
>>  is_symlink () {
>> @@ -131,7 +130,7 @@ checkout_staged_file () {
>>      tmpfile=$(expr "$(git checkout-index --temp --stage="$1" "$2")" : '\([^	]*\)	')
>>  
>>      if test $? -eq 0 -a -n "$tmpfile" ; then
>> -	mv -- "$tmpfile" "$3"
>> +	mv -- "$(git rev-parse --show-cdup)$tmpfile" "$3"
>>      fi
>>  }
> 
> Looking at the above change together with this disabled patch in the previous,
> 
>> +# We can't merge files from parent directories when running mergetool
>> +# from a subdir. Is this a bug?
>> +#
> 
> I wonder if it would be cleaner to keep $prefix and instead cd_to_toplevel
> at the beginning.
> 
> There are two ways for Porcelain scripts to work from inside a
> subdirectory of a project.
> 
>  * Stay in the original subdirectory and use show-cdup to convert a path
>    that is relative to the repository root to a path relative to your
>    current subdirectory.
> 
>  * Go up to the root of the work tree, use prefix to convert a path that
>    is relative to the original subdirectory to a path relative to the root
>    of the work tree.
> 
> The former way is probably the clunkier one between the two.  I think the
> latter is how most of the Porcelain scripts work when they need to worry
> about the Plumbing commands that return/accept repository relative paths.
> Also, all the plumbing commands work in the latter way.

Possibly, and I certainly wouldn't object if mergetool moved to the
latter mode of operation in the medium term.

In general, though, mergetool is pretty happy running from the users
directory and there may be cases in which users expect the merge tool to
be running from the directory that they started in (saving extra files
during the merge process?).

At the moment it is really a 'bugette' in the eyes of mergetool that
checkout-index echoes the name of the temporary file relative to the
repository root even if you called it from somewhere else. Without this,
mergetool doesn't require any prefix wizardry at all.

When I said 'is this a bug?', the counter argument is that it might be
considered a more useful feature that cd'ing into a subdirectory and
running mergetool restricts the files merged to an expected subset.

I meant to put in a cover note about the test changes. The reason that I
commented out the test_expect_failure was that I thought it not
completely obvious that it even counted as a failure rather than a
feature - certainly not one worth adding one to git's overall 'broken'
count - but that there might be some alternative opinions.

The pu patch is certainly necessary on top of the cat-file ->
checkout-index change in next and it is better tested now. It's
certainly not the last word on mergetool.

Charles.