| Message ID | pull.901.v3.git.1615726978059.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,GSOC] commit: add --trailer option | expand |
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > 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); > + 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 :). > /* > * 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); 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", ..... > 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" && 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 <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' ' Hope these comments are useful.
Rafael Silva <rafaeloliveira.cs@gmail.com> 于2021年3月14日周日 下午9:46写道： > > > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > 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); > > + > > 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 :). Thank you, I didn't notice the problem before :). > > > /* > > * 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); > > Style: The "--in-place" part should be aligned with the parentheses > much the like the following line with the "argc = parse_and_validate_options....". > Well, the CodingGuidelines say it's both vaild. But you are right, try to keep the same format as nearby code will be better. > For example: > > strvec_pushl(&run_trailer.args, "interpret-trailers", > "--in-place", "--where=end", ..... > > > 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" && > > 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 <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' ' > > > Hope these comments are useful. > -- > Thanks > Rafael I am gratitude to you for helping me. -- Thanks ZheNing Hu
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 &&