Re: [PATCH v9] [GSOC] commit: add --trailer option

[Shourya Shukla <periperidip@gmail.com>]

On 16/03 10:39, ZheNing Hu via GitGitGadget wrote:

Hey ZheNing!

> 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 other trailer information from the
> command line such as "Helped-by", "Reported-by", "Mentored-by",
> 
> Now implement a new `--trailer <token>[(=|:)<value>]` option to pass
> other trailers to `interpret-trailers` and insert them into commit
> messages.
> 
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>

I have been away for a while and directly seeing a V9 of this patch
feels great! Its good that you have worked upon the patch. The above
approach seems good to me!

>  	/*
>  	 * Reject an attempt to record a non-merge empty commit without
>  	 * explicit --allow-empty. In the cherry-pick case, it may be
> @@ -1507,6 +1528,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		OPT_STRING(0, "fixup", &fixup_message, N_("commit"), N_("use autosquash formatted message to fixup specified commit")),
>  		OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
>  		OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
> +		OPT_CALLBACK_F(0, "trailer", NULL, N_("trailer"), N_("trailer(s) to add"), PARSE_OPT_NONEG, opt_pass_trailer),

I feel that a better option description could be offered? Maybe
something like: 'add custom trailer(s)'.


>  		OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),
>  		OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
>  		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),

I have not yet gone through the the V2-V8s but I have a comment not
associated with the contents of the patch. I feel that you should wait a
little before posting a new version of the patch. I see that V4-V8 are
put up in almost 3 hour gaps. This isn't technically wrong or prohibited
by the communication rules of the List but I feel that posting a patch
in such short intervals makes it hard to review and unnecessarily
increases the versions of the patch. The reviewer lags behind the patch
series in fact.

What you could do instead is post one patch per day instead of 3-4 in
one single day so that your patches get thorough reviews. This way, you
won't create 3-4 new versions of the patch containing not-so-many
significant changes. You get me?

Also, in your reply on the V1 here:
https://lore.kernel.org/git/CAOLTT8SpAOj51jqYUYqYwXaVKSn1fANvetauaG0z4etiBMzVEw@mail.gmail.com/

I read:

> It's exactly what you said.
> My lack of English sometimes limits my expression.

It is okay please do not worry. Neither do we have English as our first
language nor have we ever communicated this much with an English
speaking audience online. I struggled initially too especially with many
American terms used here. You will get the hang of it soon.

Keep contributing!

Regards,
Shourya Shukla