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

[Junio C Hamano <gitster@pobox.com>]

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

>> +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?

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

>  	$(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`?

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