commit: provides multiple common signatures

From: ZheNing Hu <[email protected]>

Similar to "Helped-by", "Reported-by", "Reviewed-by", "Mentored-by" these signatures are often seen in git commit messages. After

referring to the simple implementation of commit --signoff and send-email -cc=" commiter <email>", I am considering whether to provide multiple signature parameters from the command line. I think this might help maintainers and developers directly uses the shell to provide these signatures instead of multiple times repetitive writing those trailers each time.

To achieve this goal, i refactored the append_signoff design and provided append_message and append_message_string_list interfaces, providing new ways to generate those various signatures.

Users now can use commit -H "helper <eamil>" to generate "Helped-by" trailer, commit -R "reviewer <eamil>" to generate "Reviewed-by" trailer, commit -r "reporter <eamil> "to generate "Reported-by" trailer, commit -M "mentor <eamil>" to generate "Mentored-by" trailer.

Signed-off-by ZheNing Hu <[email protected]>

[GSOC] commit: provides multiple signatures from command line

I don’t know if my idea will satisfy everyone, I'm also thinking about whether we can provide a more generalized version (I think this idea can be extended: because users and developers have other signature methods that they want, such as "Based-on-patch-by") I hope someone can give me pointers (on the correctness of ideas or codes)

 Documentation/git-commit.txt |  24 +++++++-
 builtin/commit.c             |  63 +++++++++++++++++++++
 sequencer.c                  |  40 +++++++++----
 sequencer.h                  |   4 ++
 t/t7502-commit-porcelain.sh  | 106 +++++++++++++++++++++++++++++++++++
 5 files changed, 226 insertions(+), 11 deletions(-)

Documentation/git-commit.txt +12 -1

index 17150fa7eabe..e1b528d70c1a 100644
@@ -14,7 +14,9 @@ SYNOPSIS
 	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
 	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
 	   [-i | -o] [--pathspec-from-file=<file> [--pathspec-file-nul]]
-	   [-S[<keyid>]] [--] [<pathspec>...]
+	   [-S[<keyid>]] [--] [<pathspec>...] [(-H|--helped-by)=<address>...]
+	   [(-R|--reviewed-by)=<address>...] [(-r|--reported-by)=<address>...]
+	   [(-M|--mentored)=<address>...]
 
 DESCRIPTION
 -----------

@@ -166,6 +168,26 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.
 
 include::signoff-option.txt[]
 
+-H=<address>...::
+--helped-by=<address>...::
+	Add one or more `Helped-by` trailer by the committer at the end of the commit
+	log message.
+
+-R=<address>...::
+--reviewed-by=<address>...::
+	Add one or more `Reviewed-by` trailer by the committer at the end of the commit
+	log message.

Documentation/git-commit.txt +0 -0

 -n::
 --no-verify::
 	This option bypasses the pre-commit and commit-msg hooks.

builtin/commit.c +0 -0

index 739110c5a7f6..4b312af03181 100644

builtin/commit.c +0 -0

 /*
  * The default commit message cleanup mode will remove the lines

@@ -829,6 +833,20 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (signoff)

builtin/commit.c +40 -0

+	string_list_clear(&helped_by, 0);
+	string_list_clear(&reviewed_by, 0);
+	string_list_clear(&reported_by, 0);
+	string_list_clear(&mentored_by, 0);
+
 	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
 		die_errno(_("could not write commit template"));
 

@@ -1490,6 +1508,42 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 	return git_status_config(k, v, s);
 }
 
+static int help_callback(const struct option *opt, const char *arg, int unset)
+{
+	if (unset)
+		string_list_clear(&helped_by, 0);
+	else
+		string_list_append(&helped_by, arg);
+	return 0;
+}
+
+static int review_callback(const struct option *opt, const char *arg, int unset)
+{
+	if (unset)
+		string_list_clear(&reviewed_by, 0);
+	else
+		string_list_append(&reviewed_by, arg);
+	return 0;
+}
+
+static int report_callback(const struct option *opt, const char *arg, int unset)
+{
+	if (unset)
+		string_list_clear(&reported_by, 0);
+	else
+		string_list_append(&reported_by, arg);
+	return 0;
+}
+
+static int mentor_callback(const struct option *opt, const char *arg, int unset)
+{
+	if (unset)
+		string_list_clear(&mentored_by, 0);
+	else
+		string_list_append(&mentored_by, arg);
+	return 0;
+}
 int cmd_commit(int argc, const char **argv, const char *prefix)
 {
 	static struct wt_status s;

builtin/commit.c +5 -0

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

@@ -1561,6 +1619,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	struct commit_extra_header *extra = NULL;
 	struct strbuf err = STRBUF_INIT;
 
+	helped_by.strdup_strings = 1;
+	reviewed_by.strdup_strings = 1;
+	reported_by.strdup_strings = 1;
+	mentored_by.strdup_strings = 1;
+
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_commit_usage, builtin_commit_options);
 

sequencer.c +0 -0

index d2332d3e1787..528daf9df252 100644

sequencer.c +0 -5

 {
 	unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
-	struct strbuf sob = STRBUF_INIT;
 	int has_footer;
 
-	strbuf_addstr(&sob, sign_off_header);
-	strbuf_addstr(&sob, fmt_name(WANT_COMMITTER_IDENT));
-	strbuf_addch(&sob, '\n');
-
 	if (!ignore_footer)
 		strbuf_complete_line(msgbuf);
 

@@ -4685,11 +4681,11 @@ void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)
 	 * If the whole message buffer is equal to the sob, pretend that we

sequencer.c +1 -1

 	if (!has_footer) {
 		const char *append_newlines = NULL;

@@ -4723,8 +4719,32 @@ void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)
 
 	if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
 		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
-				sob.buf, sob.len);
+				sob->buf, sob->len);

sequencer.c +5 -0

+void append_message_string_list(struct strbuf *msgbuf, const char *header,
+		struct string_list *sobs, size_t ignore_footer, unsigned flag) {
+	int i;
+	struct strbuf sob = STRBUF_INIT;
+

sequencer.h +0 -0

index f8b2e4ab8527..b24e274f4c62 100644

t/t7502-commit-porcelain.sh +4 -0

+test_expect_success 'helped-by' '
+
+	>file1 &&
+	git add file1 &&

t/t7502-commit-porcelain.sh +6 -0

+'
+
+test_expect_success 'reported-by' '
+
+	>file2 &&
+	git add file2 &&

Hey!

The idea seems very useful to me though I am not sure what others feel about this and whether Git is ready for such a thing or not. Keeping this concern aside, I will still review the patch due to my own interest in it as well.

Quoted 4 lines

From: ZheNing Hu <[email protected]>

Similar to "Helped-by", "Reported-by", "Reviewed-by", "Mentored-by" these signatures are often seen in git commit messages. After

I think it will be better to rephrase the line so that it is easier to understand. It took me a couple of reads to figure out what you meant.

Something like:

Similar to "Signed-off-by", trailers such as "Reported-by", "Helped-by" and "Mentored-by" are also seen in Git commits.

Quoted 7 lines

referring to the simple implementation of commit --signoff and send-email -cc=" commiter <email>", I am considering whether to provide multiple signature parameters from the command line. I think this might help maintainers and developers directly uses the shell to provide these signatures instead of multiple times repetitive writing those trailers each time.

The above para is more appropriate for a cover letter than a commit message. Your thought process for the patch you sent is equally valuable but this does not belong in the commit message. Commit messages are more about what you did and any nuances that follow. You get me?

Use 'git format-patch --cover-letter <...>' to create a cover letter for your patch.

Quoted 3 lines

To achieve this goal, i refactored the append_signoff design and provided append_message and append_message_string_list interfaces, providing new ways to generate those various signatures.

s/i/I

Also, commit messages are generally written in the imperative tense when desciribing what you have done in the commit.

Quoted 4 lines

Users now can use commit -H "helper <eamil>" to generate "Helped-by" trailer, commit -R "reviewer <eamil>" to generate "Reviewed-by" trailer, commit -r "reporter <eamil> "to generate "Reported-by" trailer, commit -M "mentor <eamil>" to generate "Mentored-by" trailer.

Multiple typos.

Quoted 1 line

Signed-off-by ZheNing Hu <[email protected]>

So, an improved commit message could be:

-----8<-----
commit: support multiple trailers from the command line

Historically, Git has supported the 'Signed-off-by' commit trailer using the '--signoff' and the '-s' option from the command line. Extend this list to include other commonly used trailers viz., "Helped-by", "Reviewed-by", "Reported-by" and "Mentored-by". Introduce options '-H', '-R', '-r' and '-M' corresponding to the aforementioned trailers.

While at it, add tests in the script 't7502-commit-porcelain.sh' and add information regarding these options in the Documentation of 'git commit'.

----->8-----

Quoted 7 lines

[GSOC] commit: provides multiple signatures from command line

I don’t know if my idea will satisfy everyone, I'm also thinking about whether we can provide a more generalized version (I think this idea can be extended: because users and developers have other signature methods that they want, such as "Based-on-patch-by") I hope someone can give me pointers (on the correctness of ideas or codes)

It will be great if we let the user add customised options for the respective trailers but I think that trying to cater to such a large audience will only complicate the whole process and decrease the support avialable for the options/command. Apart from this, I think that the trailer "Reviewed-by" may not be that widely used and it would be better if we remove it from this patch.

Quoted 42 lines

 Documentation/git-commit.txt |  24 +++++++-
 builtin/commit.c             |  63 +++++++++++++++++++++
 sequencer.c                  |  40 +++++++++----
 sequencer.h                  |   4 ++
 t/t7502-commit-porcelain.sh  | 106 +++++++++++++++++++++++++++++++++++
 5 files changed, 226 insertions(+), 11 deletions(-)

Documentation/git-commit.txt +18 -1

index 17150fa7eabe..e1b528d70c1a 100644
@@ -14,7 +14,9 @@ SYNOPSIS
         [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
         [--date=<date>] [--cleanup=<mode>] [--[no-]status]
         [-i | -o] [--pathspec-from-file=<file> [--pathspec-file-nul]]
-        [-S[<keyid>]] [--] [<pathspec>...]
+        [-S[<keyid>]] [--] [<pathspec>...] [(-H|--helped-by)=<address>...]
+        [(-R|--reviewed-by)=<address>...] [(-r|--reported-by)=<address>...]
+        [(-M|--mentored)=<address>...]
 
 DESCRIPTION
 -----------

@@ -166,6 +168,26 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.
 
 include::signoff-option.txt[]
 
+-H=<address>...::
+--helped-by=<address>...::
+     Add one or more `Helped-by` trailer by the committer at the end of the commit
+     log message.
+
+-R=<address>...::
+--reviewed-by=<address>...::
+     Add one or more `Reviewed-by` trailer by the committer at the end of the commit
+     log message.
+
+-r=<address>...::
+--reported-by=<address>...::
+     Add one or more `Reported-by` trailer by the committer at the end of the commit
+     log message.
+

Documentation/git-commit.txt The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`. · line 189

Quoted 4 lines

Documentation/git-commit.txt +4 -0

+-M=<address>...::
+--mentored-by=<address>...::
+     Add one or more `Mentored-by` trailer by the committer at the end of the commit
+     log message.

Oh! I did not think you had added long forms of these options and was about to comment on that. Good that you added. Do talk about them as well in the commit message.

Quoted 8 lines

Documentation/git-commit.txt +0 -0

 -n::
 --no-verify::
      This option bypasses the pre-commit and commit-msg hooks.

builtin/commit.c +0 -0

index 739110c5a7f6..4b312af03181 100644

builtin/commit.c static int config_commit_verbose = -1; /* unspecified */ · line 119

Quoted 7 lines

builtin/commit.c +4 -0

@@ -113,6 +113,10 @@ static int config_commit_verbose = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message, pathspec_file_nul;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;
 static char *sign_commit, *pathspec_from_file;
+static struct string_list helped_by = STRING_LIST_INIT_NODUP;
+static struct string_list mentored_by = STRING_LIST_INIT_NODUP;
+static struct string_list reviewed_by = STRING_LIST_INIT_NODUP;
+static struct string_list reported_by = STRING_LIST_INIT_NODUP;

Good that you kept the variables as 'string_list's instead of a usual 'char*'.

Quoted 6 lines

builtin/commit.c +0 -0

 /*
  * The default commit message cleanup mode will remove the lines

@@ -829,6 +833,20 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
      if (signoff)
              append_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0);
 

builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 843

Quoted 8 lines

builtin/commit.c +8 -0

+     if(helped_by.items)
+             append_message_string_list(&sb, "Helped-by: ", &helped_by, ignore_non_trailer(sb.buf, sb.len), 0);
+     if(reviewed_by.items)
+             append_message_string_list(&sb, "Reviewed-by: ", &reviewed_by, ignore_non_trailer(sb.buf, sb.len), 0);
+     if(reported_by.items)
+             append_message_string_list(&sb, "Reported-by: ", &reported_by, ignore_non_trailer(sb.buf, sb.len), 0);
+     if(mentored_by.items)
+             append_message_string_list(&sb, "Mentored-by: ", &mentored_by, ignore_non_trailer(sb.buf, sb.len), 0);

I think this part will look prettier if you wrap around the text. For code segments, the wrap around limit is 80 chars. For commit messages its 72 chars. Wrap around means that as soon as you hit N number of characters, you proceed to the next(new) line.

Quoted 51 lines

builtin/commit.c +40 -0

+     string_list_clear(&helped_by, 0);
+     string_list_clear(&reviewed_by, 0);
+     string_list_clear(&reported_by, 0);
+     string_list_clear(&mentored_by, 0);
+
      if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
              die_errno(_("could not write commit template"));
 

@@ -1490,6 +1508,42 @@ static int git_commit_config(const char *k, const char *v, void *cb)
      return git_status_config(k, v, s);
 }
 
+static int help_callback(const struct option *opt, const char *arg, int unset)
+{
+     if (unset)
+             string_list_clear(&helped_by, 0);
+     else
+             string_list_append(&helped_by, arg);
+     return 0;
+}
+
+static int review_callback(const struct option *opt, const char *arg, int unset)
+{
+     if (unset)
+             string_list_clear(&reviewed_by, 0);
+     else
+             string_list_append(&reviewed_by, arg);
+     return 0;
+}
+
+static int report_callback(const struct option *opt, const char *arg, int unset)
+{
+     if (unset)
+             string_list_clear(&reported_by, 0);
+     else
+             string_list_append(&reported_by, arg);
+     return 0;
+}
+
+static int mentor_callback(const struct option *opt, const char *arg, int unset)
+{
+     if (unset)
+             string_list_clear(&mentored_by, 0);
+     else
+             string_list_append(&mentored_by, arg);
+     return 0;
+}
 int cmd_commit(int argc, const char **argv, const char *prefix)
 {
      static struct wt_status s;

builtin/commit.c int cmd_commit(int argc, const char **argv, const char *prefix) · line 1567

Quoted 7 lines

builtin/commit.c +4 -0

@@ -1507,6 +1561,10 @@ 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('H', "helped-by", NULL, N_("email"), N_("add a Helped-by trailer"), help_callback),
+             OPT_CALLBACK('r', "reported-by", NULL, N_("email"), N_("add a Reported-by trailer"), report_callback),
+             OPT_CALLBACK('R', "reviewed-by", NULL, N_("email"), N_("add a Reviewed-by trailer"), review_callback),
+             OPT_CALLBACK('M', "mentored-by", NULL, N_("email"), N_("add a Mentored-by trailer"), mentor_callback),

It would be lovely if you could line wrap these above additions too!

Quoted 20 lines

builtin/commit.c +5 -0

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

@@ -1561,6 +1619,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
      struct commit_extra_header *extra = NULL;
      struct strbuf err = STRBUF_INIT;
 
+     helped_by.strdup_strings = 1;
+     reviewed_by.strdup_strings = 1;
+     reported_by.strdup_strings = 1;
+     mentored_by.strdup_strings = 1;
+
      if (argc == 2 && !strcmp(argv[1], "-h"))
              usage_with_options(builtin_commit_usage, builtin_commit_options);
 

sequencer.c +0 -0

index d2332d3e1787..528daf9df252 100644

sequencer.c int sequencer_pick_revisions(struct repository *r, · line 4672

Quoted 6 lines

sequencer.c +2 -1

@@ -4668,16 +4668,12 @@ int sequencer_pick_revisions(struct repository *r,
      return res;
 }
 
-void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)
+void append_message(struct strbuf *msgbuf, struct strbuf *sob,
+                     size_t ignore_footer, unsigned flag)

Its nice to see that you generalised the pre-exisiting function instead of creating a new one(s) for the new trailers. Good.

It will be better to name 'struct strbuf *sob' to something more generic, maybe 'struct strbuf *trailer' since 'sob' stands for 'Signed-off-by' and since the function is becoming more generic, its contents should too.

Quoted 14 lines

sequencer.c +0 -5

 {
      unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
-     struct strbuf sob = STRBUF_INIT;
      int has_footer;
 
-     strbuf_addstr(&sob, sign_off_header);
-     strbuf_addstr(&sob, fmt_name(WANT_COMMITTER_IDENT));
-     strbuf_addch(&sob, '\n');
-
      if (!ignore_footer)
              strbuf_complete_line(msgbuf);
 

@@ -4685,11 +4681,11 @@ void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)
       * If the whole message buffer is equal to the sob, pretend that we

sequencer.c void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag) · line 4688

Quoted 10 lines

sequencer.c +3 -3

       * found a conforming footer with a matching sob
       */
-     if (msgbuf->len - ignore_footer == sob.len &&
-         !strncmp(msgbuf->buf, sob.buf, sob.len))
+     if (msgbuf->len - ignore_footer == sob->len &&
+         !strncmp(msgbuf->buf, sob->buf, sob->len))
              has_footer = 3;
      else
-             has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);
+             has_footer = has_conforming_footer(msgbuf, sob, ignore_footer);

Again, rename the variables.

Quoted 8 lines

sequencer.c +1 -1

      if (!has_footer) {
              const char *append_newlines = NULL;

@@ -4723,8 +4719,32 @@ void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)
 
      if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
              strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
-                             sob.buf, sob.len);
+                             sob->buf, sob->len);

sequencer.c void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag) · line 4732

Quoted 10 lines

sequencer.c +10 -0

+}
+void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)
+{
+     struct strbuf sob = STRBUF_INIT;
+     strbuf_addstr(&sob, sign_off_header);
+     strbuf_addstr(&sob, fmt_name(WANT_COMMITTER_IDENT));
+     strbuf_addch(&sob, '\n');
+     append_message(msgbuf, &sob, ignore_footer, flag);
+     strbuf_release(&sob);
+}

I think it will be nice if you could use a flag to denote whether the entity to be appended is a 'sign-off' or not. This way when say the variable 'int signoff' is 1, then use the above part in the 'append_message()' function otherwise go with the flow that exists.

Quoted 5 lines

sequencer.c +5 -0

+void append_message_string_list(struct strbuf *msgbuf, const char *header,
+             struct string_list *sobs, size_t ignore_footer, unsigned flag) {
+     int i;
+     struct strbuf sob = STRBUF_INIT;
+

sequencer.c void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag) · line 4749

Quoted 10 lines

sequencer.c +8 -0

+     for ( i = 0; i < sobs->nr; i++)
+     {
+             strbuf_addstr(&sob, header);
+             strbuf_addstr(&sob, sobs->items[i].string);
+             strbuf_addch(&sob, '\n');
+             append_message(msgbuf, &sob, ignore_footer, flag);
+             strbuf_reset(&sob);
+     }
      strbuf_release(&sob);
 }

Similarly, here, if 'signoff = 0' then the above part goes on. Or another alternative can be to create a 'append_message_helper()' and shift the current contents of 'append_message()' into that and use the above function to work as per the value of signoff.

So a possible flow can be:

void append_message_string_list(...params...) {

        if (signoff) {
                ..excecute the necessary segments and call the
                'append_message_helper()' function...
        } else {
                ..similarly here..
        }
}

This way we save ourselves some extra functions.

Quoted 6 lines

sequencer.h +0 -0

index f8b2e4ab8527..b24e274f4c62 100644

sequencer.h int todo_list_rearrange_squash(struct todo_list *todo_list); · line 183

Quoted 10 lines

sequencer.h +4 -0

@@ -174,6 +174,10 @@ int todo_list_rearrange_squash(struct todo_list *todo_list);
  * and the new signoff will be spliced into the buffer before those bytes.
  */
 void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag);
+void append_message(struct strbuf *msgbuf, struct strbuf *sob,
+             size_t ignore_footer, unsigned flag);
+void append_message_string_list(struct strbuf *msgbuf, const char*header,
+             struct string_list *sobs, size_t ignore_footer, unsigned flag);
 
 void append_conflicts_hint(struct index_state *istate,
              struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode);

Then, these would have to be corrected too.

t/t7502-commit-porcelain.sh test_expect_success 'sign off' ' · line 155

Quoted 7 lines

t/t7502-commit-porcelain.sh +0 -0

index 6396897cc818..40823152a51c 100755
@@ -154,6 +154,112 @@ test_expect_success 'sign off' '
 
 '

I feel that the tests deserve a commit of their own. A follow-up commit in this patch.

Quoted 12 lines

t/t7502-commit-porcelain.sh +12 -0

+test_expect_success 'helped-by' '
+
+     >file1 &&
+     git add file1 &&
+     git commit --helped-by="foo <bar@frotz>" \
+     --helped-by="foo2 <bar2@frotz>" -m "thank you" &&
+     git cat-file commit HEAD >commit.msg &&
+     sed -ne "s/Helped-by: //p" commit.msg >actual &&
+     cat >expected <<-\EOF &&
+     foo <bar@frotz>
+     foo2 <bar2@frotz>
+     EOF

t/t7502-commit-porcelain.sh test_expect_success 'sign off' ' · line 169

Quoted 2 lines

t/t7502-commit-porcelain.sh +2 -0

+     test_cmp expected actual
+

Was this extra line feed intentional? It looks odd here and other test (scripts) don't have this. Though, this test script seems to have many tests which have an extra line feed. My suggestion would be to eliminate it. Maybe this script is old and hasn't been reviewed for a cleanup in a long time.

Quoted 6 lines

t/t7502-commit-porcelain.sh +6 -0

+'
+
+test_expect_success 'reported-by' '
+
+     >file2 &&
+     git add file2 &&

t/t7502-commit-porcelain.sh test_expect_success 'sign off' ' · line 186

Quoted 10 lines

t/t7502-commit-porcelain.sh +10 -0

+     git commit --reported-by="foo <bar@frotz>" \
+     --reported-by="foo2 <bar2@frotz>" -m "thank you" &&
+     git cat-file commit HEAD >commit.msg &&
+     sed -ne "s/Reported-by: //p" commit.msg >actual &&
+     cat >expected <<-\EOF &&
+     foo <bar@frotz>
+     foo2 <bar2@frotz>
+     EOF
+     test_cmp expected actual
+

Similarly here and in the parts that follow.

<...>

This seems like a good patch to me but more experienced members will be able to comment better I think. Good job anyways.

Regards,
Shourya Shukla

From: ZheNing Hu <[email protected]>

Similar to "Helped-by", "Reported-by", "Reviewed-by", "Mentored-by" these signatures are often seen in git commit messages. After referring to the simple implementation of commit --signoff and send-email -cc=" commiter <email>", I am considering whether to provide multiple signature parameters from the command line.
+ ...

Firm NAK.

Especially, not in this form that squats on short-and-sweet single letter option names only to support the convention of this single project (namely, Git).

cf. https://lore.kernel.org/git/[email protected]/

Thanks.

Firm NAK.

Especially, not in this form that squats on short-and-sweet single letter option names only to support the convention of this single project (namely, Git).

cf. https://lore.kernel.org/git/[email protected]/

Thanks.

From: ZheNing Hu <[email protected]>

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

Now use --trailer <token>[(=|:)<value>] pass the trailers to interpret-trailers and generate trailers in commit messages.

Signed-off-by ZheNing Hu <[email protected]>

---
[GSOC] commit: provides multiple signatures from command line

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.

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:

Quoted 3 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

Quoted 4 lines

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.

Quoted 2 lines

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

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.

Quoted 3 lines

Signed-off-by ZheNing Hu <[email protected]>

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

Quoted 4 lines

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.

Quoted 5 lines

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.

Ok, I agree that it's a good idea to have a good generic solution first, before having specialized options for specific trailers.

Quoted 4 lines

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

Quoted 1 line

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.

builtin/commit.c +13 -0

index 739110c5a7f6..abbd136b27f0 100644
@@ -113,6 +113,8 @@ static int config_commit_verbose = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message, pathspec_file_nul;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;
 static char *sign_commit, *pathspec_from_file;
+struct child_process run_trailer = CHILD_PROCESS_INIT;
+static const char *trailer;
 
 /*
  * The default commit message cleanup mode will remove the lines

@@ -131,6 +133,17 @@ static struct strbuf message = STRBUF_INIT;
 
 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
 
+static int opt_pass_trailer(const struct option *opt, const char *arg, int unset)
+{
+	if (unset) {
+		strvec_clear(&run_trailer.args);
+		return -1;
+	}
+	run_trailer.git_cmd = 1;
+	strvec_pushl(&run_trailer.args, "--trailer", arg, NULL);
+	return 0;
+}
+
 static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset)
 {
 	enum wt_status_format *value = (enum wt_status_format *)opt->value;

builtin/commit.c +1 -0

 	/*
 	 * Reject an attempt to record a non-merge empty commit without
 	 * explicit --allow-empty. In the cherry-pick case, it may be

@@ -1507,6 +1522,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(0, "trailer", &trailer, N_("trailer"), N_("trailer(s) to add"), opt_pass_trailer),
 		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")),

builtin/commit.c +0 -0

 	argc = parse_and_validate_options(argc, argv, builtin_commit_options,
 					  builtin_commit_usage,
 					  prefix, current_head, &s);

t/t7502-commit-porcelain.sh +0 -0

index 6396897cc818..4b9ac4587d17 100755
@@ -154,6 +154,26 @@ test_expect_success 'sign off' '
 

Quoted 32 lines · ZheNing Hu via GitGitGadget

builtin/commit.c +13 -0

index 739110c5a7f6..abbd136b27f0 100644
@@ -113,6 +113,8 @@ static int config_commit_verbose = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message, pathspec_file_nul;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;
 static char *sign_commit, *pathspec_from_file;
+struct child_process run_trailer = CHILD_PROCESS_INIT;
+static const char *trailer;
 
 /*
  * The default commit message cleanup mode will remove the lines

@@ -131,6 +133,17 @@ static struct strbuf message = STRBUF_INIT;
 
 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
 
+static int opt_pass_trailer(const struct option *opt, const char *arg, int unset)
+{
+     if (unset) {
+             strvec_clear(&run_trailer.args);
+             return -1;
+     }
+     run_trailer.git_cmd = 1;
+     strvec_pushl(&run_trailer.args, "--trailer", arg, NULL);
+     return 0;
+}
+
 static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset)
 {
      enum wt_status_format *value = (enum wt_status_format *)opt->value;

builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 975

Quoted 5 lines

builtin/commit.c +2 -0

@@ -958,6 +971,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 
      fclose(s->fp);
 
+     run_command(&run_trailer);
+

There is slight problem with running the command unconditionally. If no --trailer is passed, then the opt_pass_trailer() backend is never called, which consequently will not set the trailer command ".git_cmd" option to 1.

This will lead the run_command() API to not interpret the command as git internal, and attempt to launch as an usual command "interpret-trailers" that will likely not exist or launch an unwanted command that is not part of the GIT suite.

This can be seen by running git commit without any options:

     $ ./bin-wrappers/git -C /tmp/test commit
     error: cannot run interpret-trailers: No such file or directory
     ...

The .git_cmd should be set to true before running the command.

(the above output is from a built version with v2.31.0-rc2 + this patch
for confirmation).

Furthermore, in my opinion, we shouldn't even bother to run the command if no --trailer is passed, otherwise, we always be paying the cost of launching an OS process regardless if the user doesn't want to add trailers in theirs projects.

With that said and based on this current implementation, maybe an improved version will look like:

        if (run_trailer.args.nr) {
                run_trailer.git_cmd = 1;
                run_command(&run_trailer);
        }

Naturally the git_cmd = 1 will be removed from opt_pass_trailer() function as it won't be necessary. As minor bonus, we don't end up setting the value for every new --trailer :).

Quoted 13 lines

builtin/commit.c +1 -0

      /*
       * Reject an attempt to record a non-merge empty commit without
       * explicit --allow-empty. In the cherry-pick case, it may be

@@ -1507,6 +1522,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(0, "trailer", &trailer, N_("trailer"), N_("trailer(s) to add"), opt_pass_trailer),
              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")),

@@ -1577,6 +1593,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)

builtin/commit.c int cmd_commit(int argc, const char **argv, const char *prefix) · line 1597

Quoted 5 lines

builtin/commit.c +2 -0

                      die(_("could not parse HEAD commit"));
      }
      verbose = -1; /* unspecified */
+     strvec_pushl(&run_trailer.args, "interpret-trailers",
+             "--in-place", "--where=end", git_path_commit_editmsg(), NULL);

Style: The "--in-place" part should be aligned with the parentheses much the like the following line with the "argc = parse_and_validate_options....".

For example:

        strvec_pushl(&run_trailer.args, "interpret-trailers",
                     "--in-place", "--where=end", .....

Quoted 9 lines

builtin/commit.c +0 -0

      argc = parse_and_validate_options(argc, argv, builtin_commit_options,
                                        builtin_commit_usage,
                                        prefix, current_head, &s);

t/t7502-commit-porcelain.sh +0 -0

index 6396897cc818..4b9ac4587d17 100755
@@ -154,6 +154,26 @@ test_expect_success 'sign off' '
 

t/t7502-commit-porcelain.sh test_expect_success 'sign off' ' · line 164

Quoted 10 lines

t/t7502-commit-porcelain.sh +8 -0

 '
 
+test_expect_success 'trailer' '
+     >file1 &&
+     git add file1 &&
+     git commit -s --trailer "Signed-off-by:C O Mitter1 <[email protected]>" \
+     --trailer "Helped-by:C O Mitter2 <[email protected]>"  \
+     --trailer "Reported-by:C O Mitter3 <[email protected]>" \
+     --trailer "Mentored-by:C O Mitter4 <[email protected]>" \
+     -m "hello" &&

Perhaps here, the --trailer lines and "-m hello" option should be indent in order to make it clear that these option are part of the "git commit" from the above line, something like this:

        git commit -s --trailer "Signed-off-by:C O Mitter1 <[email protected]>" \
                --trailer "Helped-by:C O Mitter2 <[email protected]>"  \
                --trailer "Reported-by:C O Mitter3 <[email protected]>" \
                --trailer "Mentored-by:C O Mitter4 <[email protected]>" \
                -m "hello" &&

t/t7502-commit-porcelain.sh test_expect_success 'sign off' ' · line 177

Quoted 13 lines

t/t7502-commit-porcelain.sh +12 -0

+     git cat-file commit HEAD >commit.msg &&
+     sed -e "1,7d" commit.msg >actual &&
+     cat >expected <<-\EOF &&
+     Signed-off-by: C O Mitter <[email protected]>
+     Signed-off-by: C O Mitter1 <[email protected]>
+     Helped-by: C O Mitter2 <[email protected]>
+     Reported-by: C O Mitter3 <[email protected]>
+     Mentored-by: C O Mitter4 <[email protected]>
+     EOF
+     test_cmp expected actual
+'
+
 test_expect_success 'multiple -m' '

Hope these comments are useful.
--
Thanks
Rafael

builtin/commit.c +1 -0

 	/*
 	 * Reject an attempt to record a non-merge empty commit without
 	 * explicit --allow-empty. In the cherry-pick case, it may be

@@ -1507,6 +1525,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(0, "trailer", &trailer, N_("trailer"), N_("trailer(s) to add"), opt_pass_trailer),
 		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")),

builtin/commit.c +0 -0

 	argc = parse_and_validate_options(argc, argv, builtin_commit_options,
 					  builtin_commit_usage,
 					  prefix, current_head, &s);

IOW this part would become ...

        if (trailer_args.nr) {
                strvec_pushl(&run_trailer.args, "interpret-trailers",
                             "--in-place", ...);
                strvec_pushv(&run_trailer.args, trailer_args.v);
                run_trailer.git_cmd = 1;
                run_command(&run_trailer);
        }

builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 977

Quoted 2 lines

builtin/commit.c +2 -0

+     } else
+             strvec_clear(&run_trailer.args);

... and there is no need to have "else" that won't need to do anything.

builtin/commit.c int cmd_commit(int argc, const char **argv, const char *prefix) · line 1600

Quoted 4 lines

      verbose = -1; /* unspecified */

builtin/commit.c +2 -0

+     strvec_pushl(&run_trailer.args, "interpret-trailers",
+                 "--in-place", "--where=end", git_path_commit_editmsg(), NULL);

And this line will be gone.

Quoted 10 lines · Junio C Hamano <[email protected]> 于2021年3月15日周一 上午7:52写道：

IOW this part would become ...

        if (trailer_args.nr) {
                strvec_pushl(&run_trailer.args, "interpret-trailers",
                             "--in-place", ...);
                strvec_pushv(&run_trailer.args, trailer_args.v);
                run_trailer.git_cmd = 1;
                run_command(&run_trailer);
        }

Quoted 5 lines

builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 977

Quoted 2 lines

builtin/commit.c +2 -0

+     } else
+             strvec_clear(&run_trailer.args);

... and there is no need to have "else" that won't need to do anything.

Yes, but we also should clear "trailer_args" in "else" here, and check the return value of the "run_command()" for clear "run_trailer.args".

Quoted 17 lines · ZheNing Hu

Quoted 15 lines · Junio C Hamano <[email protected]> 于2021年3月15日周一 上午7:52写道：

IOW this part would become ...

        if (trailer_args.nr) {
                strvec_pushl(&run_trailer.args, "interpret-trailers",
                             "--in-place", ...);
                strvec_pushv(&run_trailer.args, trailer_args.v);
                run_trailer.git_cmd = 1;
                run_command(&run_trailer);
        }

builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 977

Quoted 2 lines

builtin/commit.c +2 -0

+     } else
+             strvec_clear(&run_trailer.args);

... and there is no need to have "else" that won't need to do anything.

Quoted 2 lines

Yes, but we also should clear "trailer_args" in "else" here, and check the return value of the "run_command()" for clear "run_trailer.args".

No. If you introduce the separate strvec, the "else" clause runs only when trailer_args haven't got anything, so there is nothing to clear.

On Mon, Mar 15, 2021 at 4:24 AM ZheNing Hu via GitGitGadget <[email protected]> wrote:

builtin/commit.c static int config_commit_verbose = -1; /* unspecified */ · line 116

Quoted 9 lines

builtin/commit.c +1 -0

index 739110c5a7f6..24fccb561398 100644
@@ -113,6 +113,9 @@ static int config_commit_verbose = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message, pathspec_file_nul;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;
 static char *sign_commit, *pathspec_from_file;
+struct child_process run_trailer = CHILD_PROCESS_INIT;

It seems to me that run_trailer is used only in the if (trailer_args.nr) {...} block, so it could be declared there instead of as a global variable.

Quoted 1 line

builtin/commit.c +1 -0

+struct strvec trailer_args = STRVEC_INIT;

builtin/commit.c static int config_commit_verbose = -1; /* unspecified */ · line 116

Quoted 9 lines · ZheNing Hu via GitGitGadget

builtin/commit.c +1 -0

index 739110c5a7f6..24fccb561398 100644
@@ -113,6 +113,9 @@ static int config_commit_verbose = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message, pathspec_file_nul;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;
 static char *sign_commit, *pathspec_from_file;
+struct child_process run_trailer = CHILD_PROCESS_INIT;

It seems to me that run_trailer is used only in the if (trailer_args.nr) {...} block, so it could be declared there instead of as a global variable.

builtin/commit.c static int config_commit_verbose = -1; /* unspecified */ · line 118

Quoted 2 lines

builtin/commit.c +2 -0

+struct strvec trailer_args = STRVEC_INIT;
+static const char *trailer;

I am not sure that this trailervariable is really needed. It seems to be used only as the third argument to OPT_CALLBACK(), but there are other places in the code base where we pass NULL as the third argument.

From: ZheNing Hu <[email protected]>

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 <[email protected]>

---
[GSOC] commit: add --trailer option

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.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-901%2Fadlternative%2Fcommit-with-multiple-signatures-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-901/adlternative/commit-with-multiple-signatures-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/901

Range-diff vs v5:

1: ca91accb2852 ! 1: c99ce75da792 [GSOC] commit: add --trailer option

     @@ builtin/commit.c: static int config_commit_verbose = -1; /* unspecified */
       static int no_post_rewrite, allow_empty_message, pathspec_file_nul;
       static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;
       static char *sign_commit, *pathspec_from_file;
     -+struct child_process run_trailer = CHILD_PROCESS_INIT;
      +struct strvec trailer_args = STRVEC_INIT;

+ if (trailer_args.nr) {
++ static struct child_process run_trailer = CHILD_PROCESS_INIT;

++
+ strvec_pushl(&run_trailer.args, "interpret-trailers",

Quoted 27 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

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 <[email protected]>

---
[GSOC] commit: add --trailer option

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.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-901%2Fadlternative%2Fcommit-with-multiple-signatures-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-901/adlternative/commit-with-multiple-signatures-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/901

Range-diff vs v5:

Quoted 7 lines

1: ca91accb2852 ! 1: c99ce75da792 [GSOC] commit: add --trailer option

     @@ builtin/commit.c: static int config_commit_verbose = -1; /* unspecified */
       static int no_post_rewrite, allow_empty_message, pathspec_file_nul;
       static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;
       static char *sign_commit, *pathspec_from_file;
     -+struct child_process run_trailer = CHILD_PROCESS_INIT;
      +struct strvec trailer_args = STRVEC_INIT;

I suggested added "static" in front of the above line, like this:

static struct strvec trailer_args = STRVEC_INIT;

You can check with git grep '^static struct strvec' '*.c' and git grep '^struct strvec' '*.c' that we use "static" when declaring a 'struct strvec' globally.

[...]

Quoted 2 lines

+ if (trailer_args.nr) {
++ static struct child_process run_trailer = CHILD_PROCESS_INIT;

I didn't suggest using "static" here.

Quoted 2 lines

++
+ strvec_pushl(&run_trailer.args, "interpret-trailers",

Thanks for working on this!

+ if (trailer_args.nr) {
-+ static struct child_process run_trailer = CHILD_PROCESS_INIT;
++ struct child_process run_trailer = CHILD_PROCESS_INIT;
+
+ strvec_pushl(&run_trailer.args, "interpret-trailers",
+ "--in-place", "--where=end", git_path_commit_editmsg(), NULL);

Quoted 6 lines · ZheNing Hu via GitGitGadget

+ if (trailer_args.nr) {
-+ static struct child_process run_trailer = CHILD_PROCESS_INIT;
++ struct child_process run_trailer = CHILD_PROCESS_INIT;
+
+ strvec_pushl(&run_trailer.args, "interpret-trailers",
+ "--in-place", "--where=end", git_path_commit_editmsg(), NULL);

Actually I don't think "--where=end" should be used here. "end" is the default for the "trailer.where" config variable, so by default if nothing has been configured, it will work as if "--where=end" was passed above.

If a user has configured "trailer.where" or trailer.<token>.where, then this should be respected. And users should be able to override such config variable using for example:

git -c trailer.where=start commit --trailer "Signed-off-by:C O Mitter <[email protected]>"

Quoted 20 lines · Christian Couder <[email protected]> 于2021年3月15日周一 下午6:14写道：

Quoted 6 lines · ZheNing Hu via GitGitGadget

+ if (trailer_args.nr) {
-+ static struct child_process run_trailer = CHILD_PROCESS_INIT;
++ struct child_process run_trailer = CHILD_PROCESS_INIT;
+
+ strvec_pushl(&run_trailer.args, "interpret-trailers",
+ "--in-place", "--where=end", git_path_commit_editmsg(), NULL);

Actually I don't think "--where=end" should be used here. "end" is the default for the "trailer.where" config variable, so by default if nothing has been configured, it will work as if "--where=end" was passed above.

If a user has configured "trailer.where" or trailer.<token>.where, then this should be respected. And users should be able to override such config variable using for example:

Quoted 2 lines

git -c trailer.where=start commit --trailer "Signed-off-by:C O Mitter <[email protected]>"

Thanks for reminding, generally speaking, we will put the trailer at the end of the commit messages.Take trailers in start, this should be something I haven't considered.

I notice another question:
if we commit this again with same trailer (even same email or same commiter) --trailer will not work again, because in interpret_trailers, "if-exists" default
set to "addIfDifferentNeighbor", I addvice enforce use "if-exists="add".

Quoted 3 lines

Thanks for reminding, generally speaking, we will put the trailer at the end of the commit messages.Take trailers in start, this should be something I haven't considered.

In general what I want to say is that git interpret-trailers should be considered to have sensible defaults, that can possibly be overridden using a number of config variables (or the git -c ... mechanism) which is a good thing. If something in it doesn't work well, it's possible to improve it of course. Otherwise it's better to just fully take advantage of it.

Quoted 5 lines

I notice another question:
if we commit this again with same trailer (even same email or same commiter) --trailer will not work again, because in interpret_trailers, "if-exists" default
set to "addIfDifferentNeighbor", I addvice enforce use "if-exists="add".

I don't agree with using "--if-exists=add". I think the default to not add a trailer line if it would be just above or below the same line is better, as doing that wouldn't add much information. It's better to encourage people to use trailers in a meaningful way.

And again if we use "--if-exists=add", then people who would want to take advantage of git interpret-trailers to customize what happens when the trailer already exists would not be able to do it.

For example if we don't use "--if-exists=add", then:

- people who want to customize what happens when the trailer already exists can do it with for example:

git -c trailer.ifexists=addIfDifferent commit --trailer "Signed-off-by:C O Mitter <[email protected]>"

- which means that people who want the "--if-exists=add" behavior can still have it with:

git -c trailer.ifexists=add commit --trailer "Signed-off-by:C O Mitter <[email protected]>"

While if we use "--if-exists=add", then using git -c trailer.ifexists=... commit ... will not customize anything as the "--if-exists=add" command line option will override any config customization.

From: ZheNing Hu <[email protected]>

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 <[email protected]>

---
 Documentation/git-commit.txt |  9 ++++++++-
 builtin/commit.c             | 23 +++++++++++++++++++++++
 t/t7502-commit-porcelain.sh  | 20 ++++++++++++++++++++
 3 files changed, 51 insertions(+), 1 deletion(-)

Documentation/git-commit.txt +8 -1

index 17150fa7eabe..f74d320f0c96 100644
@@ -14,7 +14,7 @@ SYNOPSIS
 	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
 	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
 	   [-i | -o] [--pathspec-from-file=<file> [--pathspec-file-nul]]
-	   [-S[<keyid>]] [--] [<pathspec>...]
+	   [-S[<keyid>]] [--] [<pathspec>...] [(--trailer <token>[(=|:)<value>])...]
 
 DESCRIPTION
 -----------

@@ -166,6 +166,13 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.
 
 include::signoff-option.txt[]
 
+--trailer <token>[(=|:)<value>]::
+	Specify a (<token>, <value>) pair that should be applied as a
+	trailer. (e.g. `git commit --trailer "Signed-off-by:C O Mitter \
+	<[email protected]>" --trailer "Helped-by:C O Mitter \
+	<[email protected]>"` will add the "Signed-off-by" trailer
+	and the "Helped-by" trailer in the commit message.)
+	See linkgit:git-interpret-trailers[1] for details.
 -n::
 --no-verify::
 	This option bypasses the pre-commit and commit-msg hooks.

builtin/commit.c +9 -0

index 739110c5a7f6..250e93d977df 100644
@@ -113,6 +113,7 @@ static int config_commit_verbose = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message, pathspec_file_nul;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;
 static char *sign_commit, *pathspec_from_file;
+static struct strvec trailer_args = STRVEC_INIT;
 
 /*
  * The default commit message cleanup mode will remove the lines

@@ -131,6 +132,14 @@ static struct strbuf message = STRBUF_INIT;
 
 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
 
+static int opt_pass_trailer(const struct option *opt, const char *arg, int unset)
+{
+	BUG_ON_OPT_NEG(unset);
+
+	strvec_pushl(&trailer_args, "--trailer", arg, NULL);
+	return 0;
+}
+
 static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset)
 {
 	enum wt_status_format *value = (enum wt_status_format *)opt->value;

@@ -958,6 +967,19 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 
 	fclose(s->fp);
 

builtin/commit.c +0 -0

 		OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
 		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),

t/t7502-commit-porcelain.sh +10 -0

index 6396897cc818..0acf23799931 100755
@@ -154,6 +154,26 @@ test_expect_success 'sign off' '
 
 '
 
+test_expect_success 'trailer' '
+	>file1 &&
+	git add file1 &&
+	git commit -s --trailer "Signed-off-by:C O Mitter1 <[email protected]>" \
+		--trailer "Helped-by:C O Mitter2 <[email protected]>"  \
+		--trailer "Reported-by:C O Mitter3 <[email protected]>" \
+		--trailer "Mentored-by:C O Mitter4 <[email protected]>" \
+		-m "hello" &&
+	git cat-file commit HEAD >commit.msg &&
+	sed -e "1,7d" commit.msg >actual &&

builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 979

Quoted 2 lines

builtin/commit.c +2 -0

+             if (run_command(&run_trailer))
+                     strvec_clear(&run_trailer.args);

This is git-commit, shouldn't we die() here instead of ignoring errors in sub-processes?

builtin/commit.c int cmd_commit(int argc, const char **argv, const char *prefix) · line 1533

Quoted 13 lines

builtin/commit.c +4 -0

+             strvec_clear(&trailer_args);
+     }
+
      /*
       * Reject an attempt to record a non-merge empty commit without
       * explicit --allow-empty. In the cherry-pick case, it may be

@@ -1507,6 +1529,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),
              OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),

Not required for this change, but perhaps a change here to N_() (if we can get it to fit) + doc update saying that we prefer --trailer="Signed-Off-By: to --signoff"? More on that later.

Quoted 20 lines

builtin/commit.c +0 -0

              OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
              OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),

t/t7502-commit-porcelain.sh +10 -0

index 6396897cc818..0acf23799931 100755
@@ -154,6 +154,26 @@ test_expect_success 'sign off' '
 
 '
 
+test_expect_success 'trailer' '
+     >file1 &&
+     git add file1 &&
+     git commit -s --trailer "Signed-off-by:C O Mitter1 <[email protected]>" \
+             --trailer "Helped-by:C O Mitter2 <[email protected]>"  \
+             --trailer "Reported-by:C O Mitter3 <[email protected]>" \
+             --trailer "Mentored-by:C O Mitter4 <[email protected]>" \
+             -m "hello" &&
+     git cat-file commit HEAD >commit.msg &&
+     sed -e "1,7d" commit.msg >actual &&

t/t7502-commit-porcelain.sh test_expect_success 'sign off' ' · line 176

Quoted 10 lines

t/t7502-commit-porcelain.sh +10 -0

+     cat >expected <<-\EOF &&
+     Signed-off-by: C O Mitter <[email protected]>
+     Signed-off-by: C O Mitter1 <[email protected]>
+     Helped-by: C O Mitter2 <[email protected]>
+     Reported-by: C O Mitter3 <[email protected]>
+     Mentored-by: C O Mitter4 <[email protected]>
+     EOF
+     test_cmp expected actual
+'
+

How does this interact with cases where the user has configured "trailer.separators" to have a value that doesn't contain ":"? I haven't tested, but my reading of git-interpret-trailers(1) is that if you supplied "=" instead that case would just work:

By default only : is recognized as a trailer separator, except that = is always accepted on the command line for compatibility with other git commands.

I don't know if that does the right thing in the presence of --if-exists=add.

So it would be good to update these tests so you test:

* For the --if-exists=add case at all, there's no tests for it now. I.e. add some trailers manually to the commit (via -F or whatever) and then see if they get added to, replacet etc.

* Ditto but for the user having configured trailer.separators (see the test_config helper for how to set config in a test). I.e. if it's "=" does adding trailers work, how about if it's "=" on the CLI but the config/commit message has ";" instead of ":" or something?

* Hrm, actually I think tweaking "-c trailer.ifexists" won't work at all, since the CLI switch would override it. I honestly don't know, but why not not supply it and keep the addIfDifferentNeighbor default?

If it's essential that seems like a good test / documentation addition...

* For the above -c ... case I can't think of a good way to deal with it that doesn't involve pulling in git_trailer_config() into git_commit_config(), but perhaps the least nasty way is to just set a flag in git_commit_config() if we see a "trailer.ifexists" flag, and if so don't provide "--if-exists=add", if there's no config (this will include "git -c ... commit" we set provide "--if-exists=add" ) or as noted above, maybe we can skip the whole thing and use the addIfDifferentNeighbor default.

And, not needed for this patch but worth thinking about:

* We pass through --trailer to git-interpret-trailers, what should we do about the other options? Should git-commit eventually support --trailer-where and pass it along as --where to git-interpret-trailers, or is "git -c trailer.where=... commit" good enough?

* It would be good to test for and document if that "-c trailer.*" trick works (no reason it shouldn't). I.e. to add something like this after what you have (along with tests, and check if it's even true):

Only the --trailer argument to linkgit:git-interpret-trailers[1] is supported. Other pass-through switches may be added in the future, but currently you'll need to pass arguments to linkgit:git-interpret-trailers[1] along as config, e.g. git -c trailer.where=start commit [...] --trailer=[...].

* We have a longer-term goal of having the .mailmap apply to trailers, it would be nice if git-interpret-trailers had some fuzzy-matching to check if the RHS of a trailer is a name/E-Mail pair, and if so did stricter validation on it with the ident functions we use for fsck etc. (that's copied & subtly different in several different places in the codebase, unfortunately[1]).

More thoughts:

* Having written all the above I checked how --signoff is implemented.

It seems to me to be a good idea to (at least for testing) convert the --signoff trailer to your implementation. We have plenty of tests for it, does migrating it over pass or fail those?

* I also agree with Junio that we shouldn't have a --fixed-by or whatever and wouldn't add --signoff today, but it seems very useful to me to have a shortcut like:

--trailer "Signed-off-by"

I.e. omitting the value, or:

      --trailer "Signed-off-by="

Or some other thing we deem sufficiently useful/sane syntax/unambiguous.n

Then the value would be provided by fmt_name(WANT_COMMITTER_IDENT) just as we do in append_signoff() now. I think a *very common* case for this would be something like:

       git commit --amend -v --trailer "Reviewed-by"

And it would be useful to help that along and not have to do:

       git commit --amend -v --trailer "Reviewed-by=$(git config user.name) <$(git config user.email)>"

Or worse yet, manually typo your name/e-mail address, as I'm sure I and many others will inevitably do when using this option...

1. https://lore.kernel.org/git/[email protected]/

Quoted 3 lines · Ævar Arnfjörð Bjarmason <[email protected]> 于2021年3月16日周二 下午8:52写道：

Quoted 2 lines

builtin/commit.c +2 -0

+             if (run_command(&run_trailer))
+                     strvec_clear(&run_trailer.args);

Quoted 2 lines

This is git-commit, shouldn't we die() here instead of ignoring errors in sub-processes?

After thinking about it carefully, your opinion is more reasonable, because if the user uses the wrong --trailer and does not get the information he needs, I think he will have to use --amend to modify, and die() can exit this commit directly.

Quoted 54 lines

builtin/commit.c int cmd_commit(int argc, const char **argv, const char *prefix) · line 1533

Quoted 13 lines

builtin/commit.c +4 -0

+             strvec_clear(&trailer_args);
+     }
+
      /*
       * Reject an attempt to record a non-merge empty commit without
       * explicit --allow-empty. In the cherry-pick case, it may be

@@ -1507,6 +1529,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),
              OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),

Not required for this change, but perhaps a change here to N_() (if we can get it to fit) + doc update saying that we prefer --trailer="Signed-Off-By: to --signoff"? More on that later.

Quoted 20 lines

builtin/commit.c +0 -0

              OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
              OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),

t/t7502-commit-porcelain.sh +10 -0

index 6396897cc818..0acf23799931 100755
@@ -154,6 +154,26 @@ test_expect_success 'sign off' '
 
 '
 
+test_expect_success 'trailer' '
+     >file1 &&
+     git add file1 &&
+     git commit -s --trailer "Signed-off-by:C O Mitter1 <[email protected]>" \
+             --trailer "Helped-by:C O Mitter2 <[email protected]>"  \
+             --trailer "Reported-by:C O Mitter3 <[email protected]>" \
+             --trailer "Mentored-by:C O Mitter4 <[email protected]>" \
+             -m "hello" &&
+     git cat-file commit HEAD >commit.msg &&
+     sed -e "1,7d" commit.msg >actual &&

t/t7502-commit-porcelain.sh test_expect_success 'sign off' ' · line 176

Quoted 10 lines

t/t7502-commit-porcelain.sh +10 -0

+     cat >expected <<-\EOF &&
+     Signed-off-by: C O Mitter <[email protected]>
+     Signed-off-by: C O Mitter1 <[email protected]>
+     Helped-by: C O Mitter2 <[email protected]>
+     Reported-by: C O Mitter3 <[email protected]>
+     Mentored-by: C O Mitter4 <[email protected]>
+     EOF
+     test_cmp expected actual
+'
+

How does this interact with cases where the user has configured "trailer.separators" to have a value that doesn't contain ":"? I haven't tested, but my reading of git-interpret-trailers(1) is that if you supplied "=" instead that case would just work:

Quoted 4 lines

By default only : is recognized as a trailer separator, except that = is always accepted on the command line for compatibility with other git commands.

But interpret_trailers interface allow us use "=" instead of other separators.

I did a simple test and modified the configuration "trailer.separators" and it still works. Now things are good here:

$ git -c trailer.separators="@" commit --trailer="Signed-off-by=C O <email>"

or

$ git -c trailer.separators="@" commit --trailer="Signed-off-by@C O <email>"

Both can work normally,

--trailer="Signed-off-by@ C O <email>"

will output in the commit message.

Quoted 3 lines

I don't know if that does the right thing in the presence of --if-exists=add.

Yesterday, Christian Couder and I had already discussed this issue:
Your idea is correct, I should not add "--if-exists = add", this will destroy the user's rights to configure by using git -c trailer.if-exist.

Quoted 11 lines

So it would be good to update these tests so you test:

* For the --if-exists=add case at all, there's no tests for it now. I.e. add some trailers manually to the commit (via -F or whatever) and then see if they get added to, replacet etc.

* Ditto but for the user having configured trailer.separators (see the test_config helper for how to set config in a test). I.e. if it's "=" does adding trailers work, how about if it's "=" on the CLI but the config/commit message has ";" instead of ":" or something?

As mentioned above, it works normally.

Quoted 8 lines

* Hrm, actually I think tweaking "-c trailer.ifexists" won't work at all, since the CLI switch would override it. I honestly don't know, but why not not supply it and keep the addIfDifferentNeighbor default?

If it's essential that seems like a good test / documentation addition...

Quoted 9 lines

* For the above -c ... case I can't think of a good way to deal with it that doesn't involve pulling in git_trailer_config() into git_commit_config(), but perhaps the least nasty way is to just set a flag in git_commit_config() if we see a "trailer.ifexists" flag, and if so don't provide "--if-exists=add", if there's no config (this will include "git -c ... commit" we set provide "--if-exists=add" ) or as noted above, maybe we can skip the whole thing and use the addIfDifferentNeighbor default.

Has been restored to the default settings.

Quoted 8 lines

And, not needed for this patch but worth thinking about:

* We pass through --trailer to git-interpret-trailers, what should we do about the other options? Should git-commit eventually support --trailer-where and pass it along as --where to git-interpret-trailers, or is "git -c trailer.where=... commit" good enough?

Logically speaking, interpret_trailers should be dedicated to commit or other sub-commands that require trailers.

But I think that in the later stage, the parse_options of the cmd_commit can keep the unrecognized options, and then these choices can be directly passed to the interpret_trailers backend.

Quoted 4 lines

* It would be good to test for and document if that "-c trailer.*" trick works (no reason it shouldn't). I.e. to add something like this after what you have (along with tests, and check if it's even true):

I haven't tested them for the time being, but I will do it.

Quoted 7 lines

Only the --trailer argument to linkgit:git-interpret-trailers[1] is supported. Other pass-through switches may be added in the future, but currently you'll need to pass arguments to linkgit:git-interpret-trailers[1] along as config, e.g. git -c trailer.where=start commit [...] --trailer=[...].

I think this is worth writing in the documentation.

Quoted 7 lines

* We have a longer-term goal of having the .mailmap apply to trailers, it would be nice if git-interpret-trailers had some fuzzy-matching to check if the RHS of a trailer is a name/E-Mail pair, and if so did stricter validation on it with the ident functions we use for fsck etc. (that's copied & subtly different in several different places in the codebase, unfortunately[1]).

I may not know much about fuzzy-matching, which may be worth studying later.

Quoted 2 lines

More thoughts:

Quoted 6 lines

* Having written all the above I checked how --signoff is implemented.

It seems to me to be a good idea to (at least for testing) convert the --signoff trailer to your implementation. We have plenty of tests for it, does migrating it over pass or fail those?

I don’t know how to migrating yet, it may take a long time. Even I think I can leave it as #leftoverbit later.

Quoted 17 lines

* I also agree with Junio that we shouldn't have a --fixed-by or whatever and wouldn't add --signoff today, but it seems very useful to me to have a shortcut like:

--trailer "Signed-off-by"

I.e. omitting the value, or:

      --trailer "Signed-off-by="

Or some other thing we deem sufficiently useful/sane syntax/unambiguous.n

Then the value would be provided by fmt_name(WANT_COMMITTER_IDENT) just as we do in append_signoff() now. I think a *very common* case for this would be something like:

Quoted 9 lines

       git commit --amend -v --trailer "Reviewed-by"

And it would be useful to help that along and not have to do:

       git commit --amend -v --trailer "Reviewed-by=$(git config user.name) <$(git config user.email)>"

Or worse yet, manually typo your name/e-mail address, as I'm sure I and many others will inevitably do when using this option...

I think this idea is very good and easy to implement.
We only need to do a simple string match when we get the "trailer" string, If it can be completed, it can indeed bring great convenience to users.

Quoted 1 line

1. https://lore.kernel.org/git/[email protected]/

Thanks, Ævar Arnfjörð Bjarmason!

Quoted 6 lines

Logically speaking, interpret_trailers should be dedicated to commit or other sub-commands that require trailers.

But I think that in the later stage, the parse_options of the cmd_commit can keep the unrecognized options, and then these choices can be directly passed to the interpret_trailers backend.

We have this interaction with e.g. range-diff and "log", it's often surprising. You add an option to one command and it appears in the other.

Quoted 4 lines

Quoted 4 lines

It seems to me to be a good idea to (at least for testing) convert the --signoff trailer to your implementation. We have plenty of tests for it, does migrating it over pass or fail those?

Quoted 2 lines

I don’t know how to migrating yet, it may take a long time. Even I think I can leave it as #leftoverbit later.

Sure, I mean (having looked at it) that at least for your own local testing it would make sense to change it (even if just search-replacing the --signoff in the test suite) to see if it behaves as you expect. I.e. does the --trailer behavior mirror --signoff?

Quoted 26 lines

Quoted 26 lines

* I also agree with Junio that we shouldn't have a --fixed-by or whatever and wouldn't add --signoff today, but it seems very useful to me to have a shortcut like:

--trailer "Signed-off-by"

I.e. omitting the value, or:

      --trailer "Signed-off-by="

Or some other thing we deem sufficiently useful/sane syntax/unambiguous.n

Then the value would be provided by fmt_name(WANT_COMMITTER_IDENT) just as we do in append_signoff() now. I think a *very common* case for this would be something like:

       git commit --amend -v --trailer "Reviewed-by"

And it would be useful to help that along and not have to do:

       git commit --amend -v --trailer "Reviewed-by=$(git config user.name) <$(git config user.email)>"

Or worse yet, manually typo your name/e-mail address, as I'm sure I and many others will inevitably do when using this option...

From: ZheNing Hu <[email protected]>

When using interpret-trailers, if user accidentally fill in the wrong option values (e.g. --if-exists=addd or --where=begin or --if-missing=do-nothing), git will exit quietly, and the user may not even know what happened.

So lets provides warnings when git parsing this three options:
"unknown value '%s' for key ('where'|'--if-exist'|'--if-missing')". This will remind the user :"Oh, it was because of my spelling error." (or using a synonym for error).

I am not against this, but I just want to say that when I previously worked on interpret-trailers I think I implemented or suggested such warnings, but they were rejected.

I think the reason they were rejected was to improve compatibility with future versions of Git where more options would be implemented.

For example if in a few years someone implements --where=middle and some people use it in a script like this:

git interpret-trailers --where=middle --trailer foo=bar

Then when such a script would be used with a recent version of Git it would work well, while if it would be used with an old version of Git it would emit warnings. And these warnings might actually be more annoying than the fact that the trailer is not put in the middle.

I might be wrong and there might have been other reasons though. Also things might have changed since that time, as not many options if any have been added since then.