[v3,1/3] builtin/commit.c: refactor --trailer logic

Commit Message

John Passaro April 29, 2024, 6:54 p.m. UTC

From: John Passaro <[e-mail]>

git-commit adds user trailers to the commit message by passing its
`--trailer` arguments to a child process running `git-interpret-trailers
--in-place`. This logic is broadly useful, not just for git-commit but
for other commands constructing message bodies (e.g. git-tag).

Let's move this logic from git-commit to a new function in the trailer
API, so that it can be re-used in other commands.

Additionally, replace git-commit's bespoke callback for --trailer with
the standard OPT_PASSTHRU_ARGV macro. This bespoke callback was only
adding its values to a strvec and sanity-checking that `unset` is always
false; both of these are already implemented in the parse-option API.

Signed-off-by: John Passaro <[e-mail]>
Helped-by: Patrick Steinhardt <[e-mail]>
Helped-by: Junio C Hamano <[e-mail]>
---
 builtin/commit.c | 20 +++-----------------
 trailer.c        | 12 ++++++++++++
 trailer.h        |  8 ++++++++
 3 files changed, 23 insertions(+), 17 deletions(-)

Comments

Patrick Steinhardt April 30, 2024, 5:54 a.m. UTC | #1

On Mon, Apr 29, 2024 at 06:54:21PM +0000, John Passaro via GitGitGadget wrote:
> From: John Passaro <[e-mail]>
> 
> git-commit adds user trailers to the commit message by passing its
> `--trailer` arguments to a child process running `git-interpret-trailers
> --in-place`. This logic is broadly useful, not just for git-commit but
> for other commands constructing message bodies (e.g. git-tag).
> 
> Let's move this logic from git-commit to a new function in the trailer
> API, so that it can be re-used in other commands.
> 
> Additionally, replace git-commit's bespoke callback for --trailer with
> the standard OPT_PASSTHRU_ARGV macro. This bespoke callback was only
> adding its values to a strvec and sanity-checking that `unset` is always
> false; both of these are already implemented in the parse-option API.
> 
> Signed-off-by: John Passaro <[e-mail]>
> Helped-by: Patrick Steinhardt <[e-mail]>
> Helped-by: Junio C Hamano <[e-mail]>

Your signed-off-by should always go last.

> ---
>  builtin/commit.c | 20 +++-----------------
>  trailer.c        | 12 ++++++++++++
>  trailer.h        |  8 ++++++++
>  3 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 6e1484446b0..63cd090b6f2 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -38,6 +38,7 @@
>  #include "commit-reach.h"
>  #include "commit-graph.h"
>  #include "pretty.h"
> +#include "trailer.h"
>  
>  static const char * const builtin_commit_usage[] = {
>  	N_("git commit [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]\n"
> @@ -142,14 +143,6 @@ 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(opt->value, "--trailer", arg, NULL);
> -	return 0;
> -}
> -

Nice to see this gone. I would have moved this refactoring into a
separate commit because it is completely unrelated to the new trailer
function that you're introducing.

>  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;
> @@ -1038,14 +1031,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  	fclose(s->fp);
>  
>  	if (trailer_args.nr) {
> -		struct child_process run_trailer = CHILD_PROCESS_INIT;
> -
> -		strvec_pushl(&run_trailer.args, "interpret-trailers",
> -			     "--in-place", "--no-divider",
> -			     git_path_commit_editmsg(), NULL);
> -		strvec_pushv(&run_trailer.args, trailer_args.v);
> -		run_trailer.git_cmd = 1;
> -		if (run_command(&run_trailer))
> +		if (amend_file_with_trailers(git_path_commit_editmsg(), &trailer_args))
>  			die(_("unable to pass trailers to --trailers"));
>  		strvec_clear(&trailer_args);
>  	}
> @@ -1673,7 +1659,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword 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", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, opt_pass_trailer),
> +		OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),
>  		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")),
> diff --git a/trailer.c b/trailer.c
> index c72ae687099..843c378199e 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -7,6 +7,7 @@
>  #include "commit.h"
>  #include "trailer.h"
>  #include "list.h"
> +#include "run-command.h"
>  /*
>   * Copyright (c) 2013, 2014 Christian Couder <[e-mail]>
>   */
> @@ -1170,3 +1171,14 @@ void trailer_iterator_release(struct trailer_iterator *iter)
>  	strbuf_release(&iter->val);
>  	strbuf_release(&iter->key);
>  }
> +
> +int amend_file_with_trailers(const char *path, struct strvec const* trailer_args) {

I would have called this `amend_trailers_to_file()`, which feels a bit
easier to understand. But I don't mind this much, your version should be
okay, too.

In any case, the second argument should be `const struct strvec *`. For
one, the `const` should come first. Second, the `*` always sticks to the
variable name in our codebase.

> +	struct child_process run_trailer = CHILD_PROCESS_INIT;
> +
> +	run_trailer.git_cmd = 1;
> +	strvec_pushl(&run_trailer.args, "interpret-trailers",
> +		     "--in-place", "--no-divider",
> +		     path, NULL);
> +	strvec_pushv(&run_trailer.args, trailer_args->v);
> +	return run_command(&run_trailer);
> +}
> diff --git a/trailer.h b/trailer.h
> index 9f42aa75994..55f85b008ee 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -3,6 +3,7 @@
>  
>  #include "list.h"
>  #include "strbuf.h"
> +#include "strvec.h"

Arguably you don't have to include "strvec.h" here, but can instead add
a simple forward declaration `struct strvec`.

>  enum trailer_where {
>  	WHERE_DEFAULT,
> @@ -158,4 +159,11 @@ int trailer_iterator_advance(struct trailer_iterator *iter);
>   */
>  void trailer_iterator_release(struct trailer_iterator *iter);
>  
> +/*
> + * Augment a file to add trailers to it by running git-interpret-trailers.
> + * This calls run_command() and its return value is the same (i.e. 0 for
> + * success, various non-zero for other errors). See run-command.h.
> + */
> +int amend_file_with_trailers(const char *path, struct strvec const* trailer_args);

Same comments here regarding the second argument.

Patrick

Junio C Hamano April 30, 2024, 4:38 p.m. UTC | #2

Patrick Steinhardt <[e-mail]> writes:

>> +int amend_file_with_trailers(const char *path, struct strvec const* trailer_args) {
>
> I would have called this `amend_trailers_to_file()`, which feels a bit
> easier to understand. But I don't mind this much, your version should be
> okay, too.

I had the same reaction, but we are editing trailers in the file
(without touching other things in the same file), so I would have
suggested "in" instead of "to" here.

I agree with everything else you said you your review, and I do not
think I have anything to add to this step.

> Arguably you don't have to include "strvec.h" here, but can instead add
> a simple forward declaration `struct strvec`.

Sensible.

Thanks.

Patch

diff --git a/builtin/commit.c b/builtin/commit.c
index 6e1484446b0..63cd090b6f2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -38,6 +38,7 @@ 
 #include "commit-reach.h"
 #include "commit-graph.h"
 #include "pretty.h"
+#include "trailer.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]\n"
@@ -142,14 +143,6 @@  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(opt->value, "--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;
@@ -1038,14 +1031,7 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 	fclose(s->fp);
 
 	if (trailer_args.nr) {
-		struct child_process run_trailer = CHILD_PROCESS_INIT;
-
-		strvec_pushl(&run_trailer.args, "interpret-trailers",
-			     "--in-place", "--no-divider",
-			     git_path_commit_editmsg(), NULL);
-		strvec_pushv(&run_trailer.args, trailer_args.v);
-		run_trailer.git_cmd = 1;
-		if (run_command(&run_trailer))
+		if (amend_file_with_trailers(git_path_commit_editmsg(), &trailer_args))
 			die(_("unable to pass trailers to --trailers"));
 		strvec_clear(&trailer_args);
 	}
@@ -1673,7 +1659,7 @@  int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword 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", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, opt_pass_trailer),
+		OPT_PASSTHRU_ARGV(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG),
 		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")),
diff --git a/trailer.c b/trailer.c
index c72ae687099..843c378199e 100644
--- a/trailer.c
+++ b/trailer.c
@@ -7,6 +7,7 @@ 
 #include "commit.h"
 #include "trailer.h"
 #include "list.h"
+#include "run-command.h"
 /*
  * Copyright (c) 2013, 2014 Christian Couder <[e-mail]>
  */
@@ -1170,3 +1171,14 @@  void trailer_iterator_release(struct trailer_iterator *iter)
 	strbuf_release(&iter->val);
 	strbuf_release(&iter->key);
 }
+
+int amend_file_with_trailers(const char *path, struct strvec const* trailer_args) {
+	struct child_process run_trailer = CHILD_PROCESS_INIT;
+
+	run_trailer.git_cmd = 1;
+	strvec_pushl(&run_trailer.args, "interpret-trailers",
+		     "--in-place", "--no-divider",
+		     path, NULL);
+	strvec_pushv(&run_trailer.args, trailer_args->v);
+	return run_command(&run_trailer);
+}
diff --git a/trailer.h b/trailer.h
index 9f42aa75994..55f85b008ee 100644
--- a/trailer.h
+++ b/trailer.h
@@ -3,6 +3,7 @@ 
 
 #include "list.h"
 #include "strbuf.h"
+#include "strvec.h"
 
 enum trailer_where {
 	WHERE_DEFAULT,
@@ -158,4 +159,11 @@  int trailer_iterator_advance(struct trailer_iterator *iter);
  */
 void trailer_iterator_release(struct trailer_iterator *iter);
 
+/*
+ * Augment a file to add trailers to it by running git-interpret-trailers.
+ * This calls run_command() and its return value is the same (i.e. 0 for
+ * success, various non-zero for other errors). See run-command.h.
+ */
+int amend_file_with_trailers(const char *path, struct strvec const* trailer_args);
+
 #endif /* TRAILER_H */