Re: git: regression with mergetool and answering "n" (backport fix / add tests)

[David Aguilar <davvid@gmail.com>]

On Tue, Dec 23, 2014 at 08:08:34PM +0100, Daniel Hahler wrote:
> Hi,
> 
> this is in reply to the commits from David:
> 
>     commit 0ddedd4d6b9b3e8eb3557d8ed28e1a0b354a25f8
>     Refs: v2.2.0-60-g0ddedd4
>     Merge: e886efd 1e86d5b
>     Author:     Junio C Hamano <gitster@pobox.com>
>     AuthorDate: Fri Dec 12 14:31:39 2014 -0800
>     Commit:     Junio C Hamano <gitster@pobox.com>
>     CommitDate: Fri Dec 12 14:31:39 2014 -0800
> 
>         Merge branch 'da/difftool-mergetool-simplify-reporting-status'
> 
>         Code simplification.
> 
>         * da/difftool-mergetool-simplify-reporting-status:
>           mergetools: stop setting $status in merge_cmd()
>           mergetool: simplify conditionals
>           difftool--helper: add explicit exit statement
>           mergetool--lib: remove use of $status global
>           mergetool--lib: remove no-op assignment to $status from setup_user_tool
> 
> I've ran into a problem, where "git mergetool" (using vimdiff) would add
> the changes to the index, although you'd answered "n" after not changing/saving
> the merged file.

Thanks for the heads-up.

Do you perhaps have mergetool.vimdiff.trustExitCode defined, or
a similar setting?

If you saw the prompt then it should have aborted right after
you answered "n".

The very last thing merge_cmd() for vimdiff does is call
check_unchanged().  We'll come back to check_unchanged() later.

I tried to reproduce this issue.  Here's a transcript:

....
$ git status -s
UU file.txt

$ git mergetool -t vimdiff file.txt
Merging:
file.txt

Normal merge conflict for 'file.txt':
  {local}: modified file
  {remote}: modified file
4 files to edit
#### Enter :qall inside vim
file.txt seems unchanged.
Was the merge successful? [y/n] n
merge of file.txt failed
Continue merging other unresolved paths (y/n) ? n

$ git status -s
UU file.txt
....

That seemed to work fine.  Any clues?
More notes below...

> This regression has been introduced in:
> 
>     commit 99474b6340dbcbe58f6c256fdee231cbadb060f4
>     Author: David Aguilar <davvid@gmail.com>
>     Date:   Fri Nov 14 13:33:55 2014 -0800
> 
>         difftool: honor --trust-exit-code for builtin tools
>         
>         run_merge_tool() was not setting $status, which prevented the
>         exit code for builtin tools from being forwarded to the caller.
>         
>         Capture the exit status and add a test to guarantee the behavior.
>         
>         Reported-by: Adria Farres <14farresa@gmail.com>
>         Signed-off-by: David Aguilar <davvid@gmail.com>
>         Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
>     diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
>     index c45a020..cce4f8c 100644
>     --- a/git-mergetool--lib.sh
>     +++ b/git-mergetool--lib.sh
>     @@ -221,6 +221,7 @@ run_merge_tool () {
>             else
>                     run_diff_cmd "$1"
>             fi
>     +       status=$?
>             return $status
>      }
> 
> 
> My fix has been the following, but I agree that the changes from David
> are much better in general.
> 
>     diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
>     index cce4f8c..fa9acb1 100644
>     --- a/git-mergetool--lib.sh
>     +++ b/git-mergetool--lib.sh
>     @@ -105,6 +105,7 @@ check_unchanged () {
>                             esac
>                     done
>             fi
>     +       return $status
>      }

I don't think this fix does anything.
Here is all of check_unchanged() for context:

check_unchanged () {
	if test "$MERGED" -nt "$BACKUP"
	then
		return 0
	else
		while true
		do
			echo "$MERGED seems unchanged."
			printf "Was the merge successful? [y/n] "
			read answer || return 1
			case "$answer" in
			y*|Y*) return 0 ;;
			n*|N*) return 1 ;;
			esac
		done
	fi
}

The addition of "return $status" after the "fi" in the above fix
won't do anything because that code is unreachable.
We either return 0 or 1.

> I haven't verified if it really fixes the regression, but if it does it
> should get backported into the branches where the regression is present.

Also, the $status variable doesn't even exist anymore, so the
fix is suspect.

What platform are you on?

> Also, there should be some tests for this.

I don't disagree with that ;-)

Let me know if you have any clues.  I don't see anything obvious.

cheers,
-- 
David