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

> 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,
>