| Message ID | pull.901.v2.git.1615564478029.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,GSOC] commit: add trailer command | expand |
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. > 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. > 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." > 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. > 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.
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.
Christian Couder <christian.couder@gmail.com> writes: > Ok, I agree that it's a good idea to have a good generic solution > first, before having specialized options for specific trailers. > >> 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. Thanks for all the good suggestions.
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt index 17150fa7eabe..764513a3f287 100644 --- a/Documentation/git-commit.txt +++ b/Documentation/git-commit.txt @@ -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,12 @@ 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 <committer@example.com>" \ + --trailer "Helped-by:C O Mitter <committer@example.com>"`will add the "Signed-off" trailer + and the "Helped-by" trailer in the commit message.) + -n:: --no-verify:: This option bypasses the pre-commit and commit-msg hooks. diff --git a/builtin/commit.c b/builtin/commit.c index 739110c5a7f6..abbd136b27f0 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -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; @@ -958,6 +971,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix, fclose(s->fp); + run_command(&run_trailer); + /* * 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) 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); argc = parse_and_validate_options(argc, argv, builtin_commit_options, builtin_commit_usage, prefix, current_head, &s); diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh index 6396897cc818..4b9ac4587d17 100755 --- a/t/t7502-commit-porcelain.sh +++ b/t/t7502-commit-porcelain.sh @@ -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 <committer1@example.com>" \ + --trailer "Helped-by:C O Mitter2 <committer2@example.com>" \ + --trailer "Reported-by:C O Mitter3 <committer3@example.com>" \ + --trailer "Mentored-by:C O Mitter4 <committer4@example.com>" \ + -m "hello" && + git cat-file commit HEAD >commit.msg && + sed -e "1,7d" commit.msg >actual && + cat >expected <<-\EOF && + Signed-off-by: C O Mitter <committer@example.com> + Signed-off-by: C O Mitter1 <committer1@example.com> + Helped-by: C O Mitter2 <committer2@example.com> + Reported-by: C O Mitter3 <committer3@example.com> + Mentored-by: C O Mitter4 <committer4@example.com> + EOF + test_cmp expected actual +' + test_expect_success 'multiple -m' ' >negative &&