From: Philippe Blain <levraiphilippeblain@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Fernando Ramos <greenfoo@u92.eu>,
git@vger.kernel.org, davvid@gmail.com, sunshine@sunshineco.com,
seth@eseth.com, rogi@skylittlesystem.org, bagasdotme@gmail.com
Subject: Re: [PATCH v7 3/4] vimdiff: add tool documentation
Date: Tue, 29 Mar 2022 13:08:30 -0400 [thread overview]
Message-ID: <d0a0d00b-5c1a-4a0c-a91c-b03403578f80@gmail.com> (raw)
In-Reply-To: <xmqqmth8wwcf.fsf@gitster.g>
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/
next prev parent reply other threads:[~2022-03-29 17:08 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-28 22:30 [PATCH v7 0/4] vimdiff: new implementation with layout support Fernando Ramos
2022-03-28 22:30 ` [PATCH v7 1/4] " Fernando Ramos
2022-03-28 22:30 ` [PATCH v7 2/4] vimdiff: integrate layout tests in the unit tests framework ('t' folder) Fernando Ramos
2022-03-28 22:30 ` [PATCH v7 3/4] vimdiff: add tool documentation Fernando Ramos
2022-03-28 22:39 ` Fernando Ramos
2022-03-29 1:01 ` Junio C Hamano
2022-03-29 12:54 ` [PATCH] fixup! " Philippe Blain
2022-03-29 16:03 ` Junio C Hamano
2022-03-29 14:07 ` [PATCH v7 3/4] " Philippe Blain
2022-03-29 16:35 ` Junio C Hamano
2022-03-29 17:08 ` Philippe Blain [this message]
2022-03-29 17:29 ` Philippe Blain
2022-03-28 22:30 ` [PATCH v7 4/4] vimdiff: add description to already existing diff/merge tools Fernando Ramos
2022-03-29 16:38 ` Junio C Hamano
2022-03-29 17:24 ` Philippe Blain
2022-03-29 18:50 ` Junio C Hamano
2022-03-29 22:39 ` Fernando Ramos
2022-03-29 14:10 ` [PATCH v7 0/4] vimdiff: new implementation with layout support Philippe Blain
2022-03-29 21:45 ` Fernando Ramos
2022-03-29 22:44 ` [PATCH v8 0/5] " Fernando Ramos
2022-03-29 22:44 ` [PATCH v8 1/5] " Fernando Ramos
2022-03-29 22:44 ` [PATCH v8 2/5] vimdiff: integrate layout tests in the unit tests framework ('t' folder) Fernando Ramos
2022-03-29 22:44 ` [PATCH v8 3/5] vimdiff: add tool documentation Fernando Ramos
2022-03-29 22:44 ` [PATCH v8 4/5] mergetools: add description to all diff/merge tools Fernando Ramos
2022-03-29 22:44 ` [PATCH v8 5/5] mergetools: add tools description to `git help config` Fernando Ramos
2022-03-30 12:43 ` Philippe Blain
2022-03-30 18:33 ` Fernando Ramos
2022-03-30 18:45 ` Philippe Blain
2022-03-30 19:19 ` [PATCH v9 0/4] vimdiff: new implementation with layout support Fernando Ramos
2022-03-30 19:19 ` [PATCH v9 1/4] " Fernando Ramos
2022-03-30 19:19 ` [PATCH v9 2/4] vimdiff: integrate layout tests in the unit tests framework ('t' folder) Fernando Ramos
2022-03-30 19:19 ` [PATCH v9 3/4] vimdiff: add tool documentation Fernando Ramos
2022-04-03 20:02 ` Philippe Blain
2022-04-03 22:44 ` Junio C Hamano
2022-04-04 20:00 ` Fernando Ramos
2022-04-04 21:38 ` Junio C Hamano
2022-03-30 19:19 ` [PATCH v9 4/4] mergetools: add description to all diff/merge tools Fernando Ramos
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d0a0d00b-5c1a-4a0c-a91c-b03403578f80@gmail.com \
--to=levraiphilippeblain@gmail.com \
--cc=bagasdotme@gmail.com \
--cc=davvid@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=greenfoo@u92.eu \
--cc=rogi@skylittlesystem.org \
--cc=seth@eseth.com \
--cc=sunshine@sunshineco.com \
/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.
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 NNTP newsgroup(s).