[PATCH 1/2] mergetool: Add a test for running mergetool in a sub-directory

[PATCH 1/2] mergetool: Add a test for running mergetool in a sub-directory

[Charles Bailey]

Signed-off-by: Charles Bailey <charles@hashpling.org>
---
 t/t7610-mergetool.sh |   70 ++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 51 insertions(+), 19 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index edb6a57..ee8589e 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -9,49 +9,81 @@ Testing basic merge tool invocation'
 
 . ./test-lib.sh
 
+# All the mergetool test work by checking out a temporary branch based
+# off 'branch1' and then merging in master and checking the results of
+# running mergetool
+
 test_expect_success 'setup' '
     echo master >file1 &&
-    git add file1 &&
+    mkdir subdir &&
+    echo master sub >subdir/file3 &&
+    git add file1 subdir/file3 &&
     git commit -m "added file1" &&
+
     git checkout -b branch1 master &&
     echo branch1 change >file1 &&
     echo branch1 newfile >file2 &&
-    git add file1 file2 &&
+    echo branch1 sub >subdir/file3 &&
+    git add file1 file2 subdir/file3 &&
     git commit -m "branch1 changes" &&
-    git checkout -b branch2 master &&
-    echo branch2 change >file1 &&
-    echo branch2 newfile >file2 &&
-    git add file1 file2 &&
-    git commit -m "branch2 changes" &&
+
     git checkout master &&
     echo master updated >file1 &&
     echo master new >file2 &&
-    git add file1 file2 &&
-    git commit -m "master updates"
-'
+    echo master new sub >subdir/file3 &&
+    git add file1 file2 subdir/file3 &&
+    git commit -m "master updates" &&
 
-test_expect_success 'custom mergetool' '
     git config merge.tool mytool &&
     git config mergetool.mytool.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
-    git config mergetool.mytool.trustExitCode true &&
-    git checkout branch1 &&
+    git config mergetool.mytool.trustExitCode true
+'
+
+test_expect_success 'custom mergetool' '
+    git checkout -b test1 branch1 &&
     test_must_fail git merge master >/dev/null 2>&1 &&
-    ( yes "" | git mergetool file1>/dev/null 2>&1 ) &&
-    ( yes "" | git mergetool file2>/dev/null 2>&1 ) &&
+    ( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
+    ( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
+    ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
     test "$(cat file1)" = "master updated" &&
     test "$(cat file2)" = "master new" &&
+    test "$(cat subdir/file3)" = "master new sub" &&
     git commit -m "branch1 resolved with mergetool"
 '
 
 test_expect_success 'mergetool crlf' '
     git config core.autocrlf true &&
-    git reset --hard HEAD^
+    git checkout -b test2 branch1
     test_must_fail git merge master >/dev/null 2>&1 &&
-    ( yes "" | git mergetool file1>/dev/null 2>&1 ) &&
-    ( yes "" | git mergetool file2>/dev/null 2>&1 ) &&
+    ( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
+    ( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
+    ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
     test "$(printf x | cat file1 -)" = "$(printf "master updated\r\nx")" &&
     test "$(printf x | cat file2 -)" = "$(printf "master new\r\nx")" &&
-    git commit -m "branch1 resolved with mergetool - autocrlf"
+    test "$(printf x | cat subdir/file3 -)" = "$(printf "master new sub\r\nx")" &&
+    git commit -m "branch1 resolved with mergetool - autocrlf" &&
+    git config core.autocrlf false &&
+    git reset --hard
+'
+
+test_expect_failure 'mergetool in subdir' '
+    git checkout -b test3 branch1
+    cd subdir && (
+    test_must_fail git merge master >/dev/null 2>&1 &&
+    ( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
+    test "$(cat file3)" = "master new sub" )
 '
 
+# We can't merge files from parent directories when running mergetool
+# from a subdir. Is this a bug?
+#
+#test_expect_failure 'mergetool in subdir' '
+#    cd subdir && (
+#    ( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
+#    ( yes "" | git mergetool ../file2 >/dev/null 2>&1 ) &&
+#    test "$(cat ../file1)" = "master updated" &&
+#    test "$(cat ../file2)" = "master new" &&
+#    git commit -m "branch1 resolved with mergetool - subdir" )
+#'
+
 test_done
-- 
1.6.1.235.gc9d403

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

[Charles Bailey]

The previous fix to mergetool to use checkout-index instead of cat-file
broke running mergetool anywhere except the root of the repository.

This fixes it by using the correct relative paths for temporary files
and index paths.

Signed-off-by: Charles Bailey <charles@hashpling.org>
---
 git-mergetool.sh     |    9 ++++-----
 t/t7610-mergetool.sh |    2 +-
 2 files changed, 5 insertions(+), 6 deletions(-)

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
 }
 
@@ -161,9 +160,9 @@ merge_file () {
     local_mode=`git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}'`
     remote_mode=`git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $1;}'`
 
-    base_present   && checkout_staged_file 1 "$prefix$MERGED" "$BASE"
-    local_present  && checkout_staged_file 2 "$prefix$MERGED" "$LOCAL"
-    remote_present && checkout_staged_file 3 "$prefix$MERGED" "$REMOTE"
+    base_present   && checkout_staged_file 1 "$MERGED" "$BASE"
+    local_present  && checkout_staged_file 2 "$MERGED" "$LOCAL"
+    remote_present && checkout_staged_file 3 "$MERGED" "$REMOTE"
 
     if test -z "$local_mode" -o -z "$remote_mode"; then
 	echo "Deleted merge conflict for '$MERGED':"
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index ee8589e..e768c3e 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -66,7 +66,7 @@ test_expect_success 'mergetool crlf' '
     git reset --hard
 '
 
-test_expect_failure 'mergetool in subdir' '
+test_expect_success 'mergetool in subdir' '
     git checkout -b test3 branch1
     cd subdir && (
     test_must_fail git merge master >/dev/null 2>&1 &&
-- 
1.6.1.235.gc9d403

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

[Junio C Hamano]

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?
> +#
> +#test_expect_failure 'mergetool in subdir' '
> +#    cd subdir && (
> +#    ( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
> +#    ( yes "" | git mergetool ../file2 >/dev/null 2>&1 ) &&
> +#    test "$(cat ../file1)" = "master updated" &&
> +#    test "$(cat ../file2)" = "master new" &&
> +#    git commit -m "branch1 resolved with mergetool - subdir" )
> +#'

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.

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

[Charles Bailey]

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.

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

[Junio C Hamano]

Charles Bailey <charles@hashpling.org> writes:

> Junio C Hamano wrote:
>> 
>> I wonder if it would be cleaner to keep $prefix and instead cd_to_toplevel
>> at the beginning.
>> ...
> 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?).

A very sensible consideration that I agree with.  I am happy that you
were the one who volunteered to take this script over.