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

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.

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
     }
     
     valid_tool () {

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, there should be some tests for this. I've came up with the following, which is not finished and left in a state of debugging why it did not trigger the bug.

In t/t7610-mergetool.sh:

  test_expect_success 'return code with untrusted mergetool' '
      # git config merge.tool untrusted &&
      # git config mergetool.untrusted.cmd "true" &&
      # git config mergetool.untrusted.trustExitCode false &&
          test_config mergetool.vimdiff.cmd "cat \"\$REMOTE\"" &&
      git config merge.tool vimdiff &&
      git config --get-regexp mergetool.
      git config --get-regexp merge\.tool.

      git checkout -b test_untrusted branch1 &&
      test_must_fail git merge master >/dev/null 2>&1 &&
      git status -s &&
      test "$(git status --short both)" = "AA both" &&
      ( echo "n" | test_must_fail git mergetool both ) &&
      git status -s &&

      test "$(git status --short both)" = "AA both" &&
      ( echo "y" | git mergetool both ) &&
      git status -s &&
      test "$(git status --short both)" = "M  both" &&
      git reset --hard &&
      git config merge.tool mytool

Cheers, Daniel.