Re: [PATCH 1/2] mergetool: Accept -g/--[no-]gui as arguments

From: David Aguilar <davvid@gmail.com>
Date: Oct 24, 2018, 05:44 UTC
Message-ID: <20181024053656.GA2985@gmail.com>
In-Reply-To: <xmqqy3aond55.fsf@gitster-ct.c.googlers.com>

On Wed, Oct 24, 2018 at 02:10:14PM +0900, Junio C Hamano wrote:

Show 29 quoted lines

> Denton Liu <liu.denton@gmail.com> writes:
> 
> > Subject: Re: [PATCH 1/2] mergetool: Accept -g/--[no-]gui as arguments
> 
> Other people may point it out, but s/Accept/accept/.
> 
> > In line with how difftool accepts a -g/--[no-]gui option, make mergetool
> > accept the same option in order to use the `merge.guitool` variable to
> > find the default mergetool instead of `merge.tool`.
> > ...
> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> > index 9a8b97a2a..e317ae003 100644
> > --- a/git-mergetool--lib.sh
> > +++ b/git-mergetool--lib.sh
> > @@ -350,17 +350,23 @@ guess_merge_tool () {
> >  }
> >  
> >  get_configured_merge_tool () {
> > -	# Diff mode first tries diff.tool and falls back to merge.tool.
> > -	# Merge mode only checks merge.tool
> > +	# If first argument is true, find the guitool instead
> > +	if [ "$1" = true ]
> 
> Don't use [] in our shell script (see Documentation/CodingGuidelines);
> say
> 
> 	if "$1" = true
> 
> instead.

Perhaps a small typo dropped "test" -- I think it should be

	if test "$1" = true

Show 20 quoted lines

> > +	# Diff mode first tries diff.(gui)tool and falls back to merge.(gui)tool.
> > +	# Merge mode only checks merge.(gui)tool
> >  	if diff_mode
> >  	then
> > -		merge_tool=$(git config diff.tool || git config merge.tool)
> > +		merge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)
> >  	else
> > -		merge_tool=$(git config merge.tool)
> > +		merge_tool=$(git config merge.${gui_prefix}tool)
> >  	fi
> 
> In mode_ok shell function, we seem to be prepared to deal with a
> case where neither diff_mode nor merge_mode is true.  Should this
> codepath also be prepared to?  
> 
> This is not a comment to point out an issue with this patch.  It is
> a genuine question on the code after (or before for that matter)
> this patch is applied.
> 
> Thanks.

I think the code is okay. mode_ok is setup that way to allow filtering for the other mode's tools, but this code path is only concerned with getting the default merge tool, which should only ever happen in one of the two modes.

The bit about difftool falling back to mergetool's config is a convenience so it does make sense to keep that for guitool as well.

The code after this part should handle merge_tool being empty just fine, so once the `[ ... ]` vs `test` bit is updated, please feel free to add:

Acked-by: David Aguilar <davvid@gmail.com>

cheers,

-- 
David