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

[Philippe Blain <levraiphilippeblain@gmail.com>]

Hi Junio,

Le 2022-03-29 à 12:35, Junio C Hamano a écrit :
> Philippe Blain <levraiphilippeblain@gmail.com> writes:
> 
>>> 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 :) 
> 
> If co-authors closely worked together (possibly but not necessarily
> outside the public view), exchanging drafts and agreeing on the
> final version before sending it to the list, by one approving the
> other's final draft, Co-authored-by may be appropriate.
> 
> I am not sure if that is what happened.
> 
> I would prefer to see more use of Helped-by when suggestions for
> improvements were made, possibly but not necessarily in a concrete
> "squashable patch" form, the original author accepted before sending
> the new version out, and the party who made suggestions saw the
> updated version at the same time as the general public.
> 
> In addition, especially if it was not co-authored, the chain of
> sign-off should mirror how the patches flowed.  If philippe helped
> to improve Fernando's original idea, and Fernando assembled this
> version before sending it out to the list, then 
> 
>     Helped-by: Philippe
>     Signed-off-by: Philippe
>     Signed-off-by: Fernando
> 
> Philippe's sign-off would help when his contribution is so big that
> it by itself makes a copyrightable work, which may be the case.  If
> not (e.g. when pointing out trivial typo or grammo), it is simpler
> to omit it.
> 
> If this were truly co-authored, then replace "Helped-by" in the
> above sequence with "Co-authored-by".

Thanks for clarifying all that (whatever happened to the "Mailing
list etiquette series [1]... it would be nice to revive this:).

I did not mean to "claim" co-authorship by the way, I agree that 
"Helped-by" is more appropriate in this case. I included my sign-off
mostly by accident as my 'ci' Git alias in this project is 'commit --sign-off' ;)

> 
>>> +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.
> 
> I agree on both counts.
> 
>>>  				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!
> 
> You mean that the output on a single line is better than multiple lines?

Yes.

> 
>> 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 && \
> 
> If you are piping the output into a sed command, then you discard
> the exit status of the upstream of the pipe, so " || :" at the end
> of the shell command can be discarded, no?

Yes, you are right.

> 
>>  	$(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 >$@
> 
> Ditto.
> 
> In any case, the raw output from the command are of the form
> 
>     * <name of the tool> <explanation>
> 
> Is the idea that you want to enclose the <name of the tool> part
> inside `literal`?

Yes, exactly, and add ': ' after the name of the tool. This makes the
HTML doc for 'git-config' more nicely formatted.

> 
> I do not know if the inclusion of <explanation> part in the output
> is sensible in the first place (without this series, we only showed
> the possible values, right), 

Right. I think it is a very nice addition, as several mergetool "names"
are not the same as the program name or the executable name, so I think
it's nice for the user to spell out exactly which software we are refering
to :) 

> but if we wanted to do this, shouldn't
> we be doing, instead of doing
> 
>     * araxis	Use Araxis Merge
> 
> more like
> 
>     araxis;;
> 		Use Araxis
> 
> I wonder.  The logical structure of each line is unclear with the
> current output (the asterisk is meaningful in that it tells that the
> line is an item in a bulleted list, but among the words on the rest
> of the line, the first line is special only by convention, so in a
> manpage for example, it looks like a gramatically correct sentence
> "Use Araxis Merge" is somehow prefixed by a stray word that does not
> begin with uppercase).
> 

OK, I see you mean using an Asciidoc "description list" [2]. I agree 
with you that it would be more semantically correct. So, let's use this
instead:

diff --git a/Documentation/Makefile b/Documentation/Makefile
index ed656db2ae..de0b5fed42 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))

[1] https://lore.kernel.org/git/20210512233412.10737-1-dwh@linuxprogrammer.org/
[2] https://docs.asciidoctor.org/asciidoc/latest/lists/description/