Re: [PATCH v7 3/4] vimdiff: add tool documentation

[Philippe Blain <levraiphilippeblain@gmail.com>]

Hi Fernando,

Le 2022-03-28 à 18:30, Fernando Ramos a écrit :
> Running 'git {merge,diff}tool --tool-help' now also prints usage
> information about the vimdiff tool (and its variantes) instead of just
> its name.
> 
> Two new functions ('diff_cmd_help()' and 'merge_cmd_help()') have been
> added to the set of functions that each merge tool (ie. scripts found
> inside "mergetools/") can overwrite to provided tool specific
> information.
> 
> Right now, only 'mergetools/vimdiff' implements these functions, but
> other tools are encouraged to do so in the future, specially if they
> take configuration options not explained anywhere else (as it is the
> case with the 'vimdiff' tool and the new 'layout' option)
> 
> In addition, a section has been added to
> "Documentation/git-mergetool.txt" to explain the new "layout"
> configuration option with examples.
> 
> Signed-off-by: Fernando Ramos <greenfoo@u92.eu>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>

Thanks :) I think the project convention is to also use the 
'Co-authored-by' trailer as well :) 

> ---
>  Documentation/config/mergetool.txt   |   9 ++
>  Documentation/git-mergetool.txt      |   8 ++
>  Documentation/mergetools/vimdiff.txt | 189 +++++++++++++++++++++++++++
>  git-mergetool--lib.sh                |  10 +-
>  mergetools/vimdiff                   |  53 ++++++++
>  5 files changed, 268 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/mergetools/vimdiff.txt
> 
> diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
> index cafbbef46a..13f1b234db 100644
> --- a/Documentation/config/mergetool.txt
> +++ b/Documentation/config/mergetool.txt
> @@ -45,6 +45,15 @@ mergetool.meld.useAutoMerge::
>  	value of `false` avoids using `--auto-merge` altogether, and is the
>  	default value.
>  
> +mergetool.vimdiff.layout::
> +	The vimdiff backend uses this variable to control how its split
> +	windows look like. Applies even if you are using Neovim (`nvim`) or
> +	gVim (`gvim`) as the merge tool. See BACKEND SPECIFIC HINTS section
> +ifndef::git-mergetool[]
> +	(on linkgit:git-mergetool[1])

small nit: "in linkgit:git-mergetool[1]" would read slightly better I think,
but that may be just me... and I think I would drop the parentheses.

> +endif::[]
> +	for details.
> +

> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 542a6a75eb..9f99201bcc 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -63,7 +63,7 @@ $(list_tool_variants)"
>  					preamble=
>  				fi
>  				shown_any=yes
> -				printf "%s%s\n" "$per_line_prefix" "$toolname"
> +				printf "%s%-15s  %s\n" "$per_line_prefix" "$toolname" $(diff_mode && diff_cmd_help "$toolname" || merge_cmd_help "$toolname")
>  			fi

I tried this and it looks much better on a single line, nice!

I also noticed that the list of available tools is embedded in 'git help config'
(see the rule for "mergetools-list.made" in Documentation/Makefile). I looked 
at the generated 'git-config.html' and it is not ideal; it would be better if 
the tool names would be enclosed in backticks. I tried the following tweak:

diff --git a/Documentation/Makefile b/Documentation/Makefile
index ed656db2ae..a2201680a2 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -333,10 +333,10 @@ mergetools-list.made: ../git-mergetool--lib.sh $(wildcard ../mergetools/*)
 	$(QUIET_GEN) \
 	$(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \
 		. ../git-mergetool--lib.sh && \
-		show_tool_names can_diff "* " || :' >mergetools-diff.txt && \
+		show_tool_names can_diff "* " || :' | sed -e "s/* \([a-z0-9]*\)/* \`\1\`:/" >mergetools-diff.txt && \
 	$(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \
 		. ../git-mergetool--lib.sh && \
-		show_tool_names can_merge "* " || :' >mergetools-merge.txt && \
+		show_tool_names can_merge "* " || :' | sed -e "s/* \([a-z0-9]*\)/* \`\1\`:/" >mergetools-merge.txt && \
 	date >$@
 
 TRACK_ASCIIDOCFLAGS = $(subst ','\'',$(ASCIIDOC_COMMON):$(ASCIIDOC_HTML):$(ASCIIDOC_DOCBOOK))


and it mostly works, though with the Asciidoc HTML backend, "emerge" is not correctly
formatted, I get:

‘emerge`: Use Emacs’ Emerge

and in the man page the backticks are kept litereally.It works correctly 
with Asciidoctor though, both in HTML and man page ...