Re: [PATCH v2 1/2] Refactor vimdiff and bc merge tool variants

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 20, 2020, 18:51 UTC
Message-ID: <xmqqd04q9g14.fsf@gitster.c.googlers.com>
In-Reply-To: <20200719042335.3913-2-rogi@skylittlesystem.org>

pudinha <rogi@skylittlesystem.org> writes:

> Subject: Re: [PATCH v2 1/2] Refactor vimdiff and bc merge tool variants

cf. https://git-scm.com/docs/SubmittingPatches#describe-changes

Two tricks to pick a good title are:

 - Read a pageful or two of "git shortlog --no-merges" output to get
   accustomed to the general pattern in the entire project.  It
   would become clear why titles with "area:" prefix help the
   patches with them easier to locate.

 - Read a pageful of "git shortlog --no-merges -- mergetools"
   (i.e. the same but limited to the files you are touching) to see
   how the changes that contributed over time to build the subsystem
   are called, so that the new patches can fit in the pattern.

Show 7 quoted lines

> The merge tools vimdiff2, vimdiff3, gvimdiff2, gvimdiff3 and bc3 are all
> variants of the main tools vimdiff and bc. They are implemented in the
> main and a one-liner script that just sources it exist for each.
>
> This patch allows variants ending in [0-9] to be correctly wired without
> the need for such one-liners, so instead of 5 scripts, only 1 (gvimdiff)
> is needed.

Well explained. The final paragraph could be made more in line with the existing commit log messages by writing in the imperative mood (e.g. "Introduce a new list_tool_variants method, and when a tool whose name ends with a digit is asked for, and if there is no such script in mergetools/ directoroy, ask the tool with the name without the final digit, if exists, if it knows how to handle the variant. That way, instead of 5 scriptss, ..."), but what is written above is already good enough.

> ---

cf. https://git-scm.com/docs/SubmittingPatches#sign-off

Thanks.

Show 59 quoted lines

>  git-mergetool--lib.sh | 28 +++++++++++++++++++++++-----
>  mergetools/bc         |  5 +++++
>  mergetools/bc3        |  1 -
>  mergetools/gvimdiff2  |  1 -
>  mergetools/gvimdiff3  |  1 -
>  mergetools/vimdiff    |  8 ++++++++
>  mergetools/vimdiff2   |  1 -
>  mergetools/vimdiff3   |  1 -
>  8 files changed, 36 insertions(+), 10 deletions(-)
>  delete mode 100644 mergetools/bc3
>  delete mode 100644 mergetools/gvimdiff2
>  delete mode 100644 mergetools/gvimdiff3
>  delete mode 100644 mergetools/vimdiff2
>  delete mode 100644 mergetools/vimdiff3
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 204a5acd66..29fecc340f 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -43,7 +43,14 @@ show_tool_names () {
>  
>  	shown_any=
>  	( cd "$MERGE_TOOLS_DIR" && ls ) | {
> -		while read toolname
> +		while read scriptname
> +		do
> +			setup_tool "$scriptname" 2>/dev/null
> +			variants="$variants$(list_tool_variants)\n"
> +		done
> +		variants="$(echo "$variants" | sort | uniq)"
> +
> +		for toolname in $variants
>  		do
>  			if setup_tool "$toolname" 2>/dev/null &&
>  				(eval "$condition" "$toolname")
> @@ -157,6 +164,10 @@ setup_tool () {
>  		echo "$1"
>  	}
>  
> +	list_tool_variants () {
> +		echo "$tool"
> +	}
> +
>  	# Most tools' exit codes cannot be trusted, so By default we ignore
>  	# their exit code and check the merged file's modification time in
>  	# check_unchanged() to determine whether or not the merge was
> @@ -178,19 +189,26 @@ setup_tool () {
>  		false
>  	}
>  
> -
> -	if ! test -f "$MERGE_TOOLS_DIR/$tool"
> +	if test -f "$MERGE_TOOLS_DIR/$tool"
>  	then
> +		. "$MERGE_TOOLS_DIR/$tool"
> +	elif test -f "$MERGE_TOOLS_DIR/${tool%[0-9]}"
> +	then
> +		. "$MERGE_TOOLS_DIR/${tool%[0-9]}"
> +	else

Nice---if the thing has a custom script in mergetools/ then we do not do anything special, but if a script is missing for a tool whose name ends with a digit, and a script exists for a tool with the same name without that digit, we try to use that instead, and we make sure it does support the named variant or bail out later...

Show 14 quoted lines

>  		setup_user_tool
>  		return $?
>  	fi
>  
> -	# Load the redefined functions
> -	. "$MERGE_TOOLS_DIR/$tool"
>  	# Now let the user override the default command for the tool.  If
>  	# they have not done so then this will return 1 which we ignore.
>  	setup_user_tool
>  
> +	if ! list_tool_variants | grep -q "^$tool$"
> +	then
> +		return 1
> +	fi

... like this.  Excellent.