[PATCH 2/2] mergetool: Don't assume paths are unmerged

[PATCH 2/2] mergetool: Don't assume paths are unmerged

[Jonathon Mah]

Like commit, mergetool now treats its path arguments as restricting
operation to the listed paths. Running "git mergetool subdir" will
prompt to resolve all conflicted blobs under subdir.

Previously mergetool would assume each path was in an unresolved state,
and get confused when it couldn't check out their other stages.

Signed-off-by: Jonathon Mah <me@JonathonMah.com>
---
Apologies, having some issues with my mail agent. This should be in-reply-to: 734F376D-0CF5-4417-8DC2-8A46AA05D995@JonathonMah.com

 Documentation/git-mergetool.txt |    7 ++-
 git-mergetool.sh                |   83 +++++++++++++++++---------------------
 t/t7610-mergetool.sh            |   28 +++++++++++--
 3 files changed, 64 insertions(+), 54 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 8c79ae8..f1f4e7a 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -16,9 +16,10 @@ Use `git mergetool` to run one of several merge utilities to resolve
 merge conflicts.  It is typically run after 'git merge'.
 
 If one or more <file> parameters are given, the merge tool program will
-be run to resolve differences on each file.  If no <file> names are
-specified, 'git mergetool' will run the merge tool program on every file
-with merge conflicts.
+be run to resolve differences on each file (skipping those without
+conflicts).  Specifying a directory will include all unresolved files in
+that path.  If no <file> names are specified, 'git mergetool' will run
+the merge tool program on every file with merge conflicts.
 
 OPTIONS
 -------
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 3aab5aa..81cf2cb 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -342,64 +342,55 @@ merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo fa
 
 last_status=0
 rollup_status=0
-rerere=false
-
-files_to_merge() {
-    if test "$rerere" = true
-    then
-	git rerere remaining
-    else
-	git ls-files -u | sed -e 's/^[^	]*	//' | sort -u
-    fi
-}
-
+files=
 
 if test $# -eq 0 ; then
     cd_to_toplevel
 
     if test -e "$GIT_DIR/MERGE_RR"
     then
-	rerere=true
-    fi
-
-    files=$(files_to_merge)
-    if test -z "$files" ; then
-	echo "No files need merging"
-	exit 0
+	files=$(git rerere remaining)
+    else
+	files=$(git ls-files -u | sed -e 's/^[^	]*	//' | sort -u)
     fi
-
-    # Save original stdin
-    exec 3<&0
-
-    printf "Merging:\n"
-    printf "$files\n"
-
-    files_to_merge |
-    while IFS= read i
-    do
-	if test $last_status -ne 0; then
-	    prompt_after_failed_merge <&3 || exit 1
-	fi
-	printf "\n"
-	merge_file "$i" <&3
-	last_status=$?
-	if test $last_status -ne 0; then
-	    rollup_status=1
-	fi
-    done
 else
     while test $# -gt 0; do
-	if test $last_status -ne 0; then
-	    prompt_after_failed_merge || exit 1
-	fi
-	printf "\n"
-	merge_file "$1"
-	last_status=$?
-	if test $last_status -ne 0; then
-	    rollup_status=1
+	matches=$(git ls-files -u -- "$1" | sed -e 's/^[^	]*	//' | sort -u)
+	if test -n "$matches"; then
+	    if test -z "$files"; then
+		files=$matches
+	    else
+		files=$(printf "%s\n%s" "$files" "$matches")
+	    fi
 	fi
 	shift
     done
+    files=$(printf "%s" "$files" | sort -u)
 fi
 
+if test -z "$files" ; then
+    echo "No files need merging"
+    exit 0
+fi
+
+# Save original stdin
+exec 3<&0
+
+printf "Merging:\n"
+printf "$files\n"
+
+IFS='
+'; for i in $files
+do
+    if test $last_status -ne 0; then
+	prompt_after_failed_merge <&3 || exit 1
+    fi
+    printf "\n"
+    merge_file "$i" <&3
+    last_status=$?
+    if test $last_status -ne 0; then
+	rollup_status=1
+    fi
+done
+
 exit $rollup_status
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index f00caa3..4aab2a7 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -81,7 +81,7 @@ test_expect_success 'custom mergetool' '
     git checkout -b test1 branch1 &&
     git submodule update -N &&
     test_must_fail git merge master >/dev/null 2>&1 &&
-    ( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
+    ( yes "" | git mergetool file1 file1 ) &&
     ( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
     ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
     ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
@@ -184,6 +184,24 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '
     git reset --hard
 '
 
+test_expect_success 'mergetool takes partial path' '
+    git config rerere.enabled false &&
+    git checkout -b test12 branch1 &&
+    git submodule update -N &&
+    test_must_fail git merge master &&
+
+    #shouldnt need these lines
+    #( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+    #( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
+    #( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
+    #( yes "" | git mergetool file1 file2 >/dev/null 2>&1 ) &&
+
+    ( yes "" | git mergetool subdir ) &&
+
+    test "$(cat subdir/file3)" = "master new sub" &&
+    git reset --hard
+'
+
 test_expect_success 'deleted vs modified submodule' '
     git checkout -b test6 branch1 &&
     git submodule update -N &&
@@ -392,7 +410,7 @@ test_expect_success 'directory vs modified submodule' '
     test "$(cat submod/file16)" = "not a submodule" &&
     rm -rf submod.orig &&
 
-    git reset --hard &&
+    git reset --hard >/dev/null 2>&1 &&
     test_must_fail git merge master &&
     test -n "$(git ls-files -u)" &&
     test ! -e submod.orig &&
@@ -404,7 +422,7 @@ test_expect_success 'directory vs modified submodule' '
     ( cd submod && git clean -f && git reset --hard ) &&
     git submodule update -N &&
     test "$(cat submod/bar)" = "master submodule" &&
-    git reset --hard && rm -rf submod-movedaside &&
+    git reset --hard >/dev/null 2>&1 && rm -rf submod-movedaside &&
 
     git checkout -b test11.c master &&
     git submodule update -N &&
@@ -414,7 +432,7 @@ test_expect_success 'directory vs modified submodule' '
     git submodule update -N &&
     test "$(cat submod/bar)" = "master submodule" &&
 
-    git reset --hard &&
+    git reset --hard >/dev/null 2>&1 &&
     git submodule update -N &&
     test_must_fail git merge test11 &&
     test -n "$(git ls-files -u)" &&
@@ -422,7 +440,7 @@ test_expect_success 'directory vs modified submodule' '
     ( yes "r" | git mergetool submod ) &&
     test "$(cat submod/file16)" = "not a submodule" &&
 
-    git reset --hard master &&
+    git reset --hard master >/dev/null 2>&1 &&
     ( cd submod && git clean -f && git reset --hard ) &&
     git submodule update -N
 '
-- 
1.7.4.4

Re: [PATCH 2/2] mergetool: Don't assume paths are unmerged

[Junio C Hamano]

Jonathon Mah <me@JonathonMah.com> writes:

> Like commit, mergetool now treats its path arguments as restricting
> operation to the listed paths. Running "git mergetool subdir" will
> prompt to resolve all conflicted blobs under subdir.
>
> Previously mergetool would assume each path was in an unresolved state,
> and get confused when it couldn't check out their other stages.
>
> Signed-off-by: Jonathon Mah <me@JonathonMah.com>
> ---

This does two unrelated things, no?

 - Given the name of a file from the command line, current mergetool gets
   confused if the given file is not in a conflicted state (bugfix).

 - Instead of taking command line arguments as literal filenames, treat
   them as pathspec like any sane git subcommand does, allowing the users
   more flexibility (new feature).

And the title of the patch advertises only the first point.  Using the
command line argument as pathspec may make a lot of sense, and code
reduction in git-mergetool.sh with this patch may also be a good change.

> diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
> index 8c79ae8..f1f4e7a 100644
> --- a/Documentation/git-mergetool.txt
> +++ b/Documentation/git-mergetool.txt
> @@ -16,9 +16,10 @@ Use `git mergetool` to run one of several merge utilities to resolve
>  merge conflicts.  It is typically run after 'git merge'.
>  
>  If one or more <file> parameters are given, the merge tool program will
> -be run to resolve differences on each file.  If no <file> names are
> -specified, 'git mergetool' will run the merge tool program on every file
> -with merge conflicts.
> +be run to resolve differences on each file (skipping those without
> +conflicts).  Specifying a directory will include all unresolved files in
> +that path.  If no <file> names are specified, 'git mergetool' will run
> +the merge tool program on every file with merge conflicts.

The documentation may also have to reword <file> written back in the days
when they were literal filenames, not pathspecs.

> +	matches=$(git ls-files -u -- "$1" | sed -e 's/^[^	]*	//' | sort -u)

Would we want to catch and signal a typo like this?

	git mergetool Mkaefile
	git mergetool Documentaiton/

when there is a conflict in Makefile and some file in Documentation/
directory, and user obviously wanted to name them but botched typing? I
think you are just letting these typos silently go.

> @@ -392,7 +410,7 @@ test_expect_success 'directory vs modified submodule' '
>      test "$(cat submod/file16)" = "not a submodule" &&
>      rm -rf submod.orig &&
>  
> -    git reset --hard &&
> +    git reset --hard >/dev/null 2>&1 &&

What is the justification for this change?

Please do not make "sh txxx.sh -v" less useful.

Re: [PATCH 2/2] mergetool: Don't assume paths are unmerged

[Junio C Hamano]

Jonathon Mah <me@JonathonMah.com> writes:

>  if test $# -eq 0 ; then
>      cd_to_toplevel
>  
>      if test -e "$GIT_DIR/MERGE_RR"
>      then
> +	files=$(git rerere remaining)
> +    else
> +	files=$(git ls-files -u | sed -e 's/^[^	]*	//' | sort -u)
>      fi
>  else
>      while test $# -gt 0; do
> +	matches=$(git ls-files -u -- "$1" | sed -e 's/^[^	]*	//' | sort -u)
> +	if test -n "$matches"; then
> +	    if test -z "$files"; then
> +		files=$matches
> +	    else
> +		files=$(printf "%s\n%s" "$files" "$matches")
> +	    fi
>  	fi
>  	shift
>      done
> +    files=$(printf "%s" "$files" | sort -u)
>  fi

Why do you need a loop here in the else clause, instead of just a single:

	files=$(git ls-files -u -- "$@" |...)