On Fri, Mar 03, 2017 at 03:57:38AM -0800, Denton Liu wrote: > This fixes the discrepancy between difftool and mergetool where the > former has the --gui flag and the latter does not by adding the > functionality to mergetool. > > Signed-off-by: Denton Liu <liu.denton@xxxxxxxxx> > --- > Documentation/git-mergetool.txt | 8 +++++++- > contrib/completion/git-completion.bash | 3 ++- > git-mergetool.sh | 5 ++++- > t/t7610-mergetool.sh | 28 +++++++++++++++++++++++++++- > 4 files changed, 40 insertions(+), 4 deletions(-) Would you mind splitting up this patch so that the completion part is done separately? > diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh > index 381b7df45..5683907ab 100755 > --- a/t/t7610-mergetool.sh > +++ b/t/t7610-mergetool.sh > @@ -145,6 +147,30 @@ test_expect_success 'custom mergetool' ' > git commit -m "branch1 resolved with mergetool" > ' > > +test_expect_success 'gui mergetool' ' > + test_when_finished "git reset --hard" && > + test_when_finished "git config merge.tool mytool" && > + test_when_finished "git config --unset merge.guitool" && > + git config merge.tool badtool && > + git config merge.guitool mytool && > + git checkout -b test$test_count branch1 && > + git submodule update -N && > + test_must_fail git merge master >/dev/null 2>&1 && It'd probably be better to use test_expect_code instead of test_must_fail here: test_expect_code 1 git merge master ... > + ( yes "" | git mergetool -g both >/dev/null 2>&1 ) && > + ( yes "" | git mergetool -g file1 file1 ) && > + ( yes "" | git mergetool --gui file2 "spaced name" >/dev/null 2>&1 ) && > + ( yes "" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) && I realize that this test is based on an existing one, but I'm curious.. is "yes" used above because it's prompting, and using -y or --no-prompt here would eliminate the need for the 'yes ""' parts? Looks good otherwise. Thanks, -- David