Hi Junio,
Le 2022-03-29 à 12:35, Junio C Hamano a écrit :
> Philippe Blain <levraiphilippeblain@xxxxxxxxx> 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@xxxxxx>
>>> Signed-off-by: Philippe Blain <levraiphilippeblain@xxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxxxxx/
[2] https://docs.asciidoctor.org/asciidoc/latest/lists/description/
