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@xxxxxxxxx>
AuthorDate: Fri Dec 12 14:31:39 2014 -0800
Commit: Junio C Hamano <gitster@xxxxxxxxx>
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@xxxxxxxxx>
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@xxxxxxxxx>
Signed-off-by: David Aguilar <davvid@xxxxxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
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.
--
http://daniel.hahler.de/
Attachment:
signature.asc
Description: OpenPGP digital signature