git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: pudinha <rogi@skylittlesystem.org>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 1/2] Refactor vimdiff and bc merge tool variants
Date: Mon, 20 Jul 2020 11:51:03 -0700	[thread overview]
Message-ID: <xmqqd04q9g14.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200719042335.3913-2-rogi@skylittlesystem.org> (pudinha's message of "Sun, 19 Jul 2020 05:23:37 +0100")

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.

> 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.

>  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...

>  		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.

  reply	other threads:[~2020-07-20 18:51 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-18 19:20 [PATCH] Support nvim as mergetool pudinha
2020-07-18 20:30 ` Junio C Hamano
2020-07-19  4:23 ` [PATCH v2 0/2] Support nvim as merge tool pudinha
2020-07-29 21:31   ` [PATCH v3 0/2] mergetools: add support for nvimdiff (neovim) family pudinha
2020-07-29 21:31     ` [PATCH v3 1/2] mergetool--lib: improve support for vimdiff-style tool variants pudinha
2020-07-29 21:31     ` [PATCH v3 2/2] mergetools: add support for nvimdiff (neovim) family pudinha
2020-07-19  4:23 ` [PATCH v2 1/2] Refactor vimdiff and bc merge tool variants pudinha
2020-07-20 18:51   ` Junio C Hamano [this message]
2020-07-19  4:23 ` [PATCH v2 2/2] Support nvim as merge tool pudinha
2020-07-20 18:52   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqd04q9g14.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=rogi@skylittlesystem.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).