Re: [PATCH 1/3] Add --gui option to mergetool

From: David Aguilar <davvid@gmail.com>
Date: Mar 3, 2017, 12:39 UTC
Message-ID: <20170303123934.4cau7ansprx2xpc7@gmail.com>
In-Reply-To: <20170303115738.GA13211@arch-attack.localdomain>

On Fri, Mar 03, 2017 at 03:57:38AM -0800, Denton Liu wrote:

Show 11 quoted lines

> 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@gmail.com>
> ---
>  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?

Show 17 quoted lines

> 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