Re: [PATCH v2] [GSOC] commit: add trailer command

[ZheNing Hu <adlternative@gmail.com>]

Christian Couder <christian.couder@gmail.com> 于2021年3月14日周日 下午12:19写道：
>
> On Fri, Mar 12, 2021 at 4:59 PM ZheNing Hu via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > Historically, Git has supported the 'Signed-off-by' commit trailer
> > using the '--signoff' and the '-s' option from the command line.
> > But users may need to provide richer trailer information from the
> > command line such as "Helped-by", "Reported-by", "Mentored-by",
>
> Nit: not sure that "richer" is the proper word here. I would just use
> "other" instead.
>

OK.

> > Now use `--trailer <token>[(=|:)<value>]` pass the trailers to
> > `interpret-trailers` and generate trailers in commit messages.
>
> The subject says "add trailer command" while here you say "use". So
> which one is it? Does "--trailer" already exist, and we are just going
> to use it? Or will this patch series actually "add" it?
>
> Looking at the existing options and the code of this patch series, the
> patch series actually adds the "--trailer" (not "trailer") option, so
> "add" or "implement" would be clearer than "use".
>

You're right. "add" will be more accurate in this situation.

> So maybe something like the following might be better:
>
> "Now implement a new `--trailer <token>[(=|:)<value>]` option to pass
> other trailers to `interpret-trailers` and insert them into commit
> messages."
>
> Also something like "--trailer" is usually called an option (or
> sometimes a flag), not a command (especially not when the word is not
> a verb, and when the new feature isn't a new exclusive mode of
> operation). So something like "commit: add --trailer option" might be
> a better subject.
>
> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> > ---
> >     [GSOC] commit: provides multiple signatures from command line
>
> It looks like this is using the subject of a patch that previously
> attempted to add features with a similar purpose. I don't think you
> need to put it there, or if you want to refer to it, I think it might
> be better to be a bit more explicit, for example like:
>
> "This patch replaces my previous attempt to provide similar features
> in a patch called: [GSOC] commit: provides multiple signatures from
> command line."
>

I may have thought that the effect of the two patch was closer so did not
change it.

> >     Now maintainers or developers can also use commit
> >     --trailer="Signed-off-by:commiter<email>" from the command line to
> >     provide trailers to commit messages. This solution may be more
> >     generalized than v1.
>
> Ok, I agree that it's a good idea to have a good generic solution
> first, before having specialized options for specific trailers.
>

Thanks. :)

> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-901%2Fadlternative%2Fcommit-with-multiple-signatures-v2
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-901/adlternative/commit-with-multiple-signatures-v2
> > Pull-Request: https://github.com/gitgitgadget/git/pull/901
> >
> > Range-diff vs v1:
>
> If this patch series has very few code and commit messages in common
> with a previous attempt at implementing similar features, it might be
> better to make it a new patch series rather than a v2. This could
> avoid sending range-diffs that are mostly useless.

Thank you for these pertinent suggestions. I will pay more attention.