[v5,3/9] trailer: prepare to expose functions as part of API

Commit Message

Linus Arver Feb. 16, 2024, 11:09 p.m. UTC

From: Linus Arver <[e-mail]>

In the next patch, we will move "process_trailers" from trailer.c to
builtin/interpret-trailers.c. That move will necessitate the growth of
the trailer.h API, forcing us to expose some additional functions in
trailer.h.

Rename relevant functions so that they include the term "trailer" in
their name, so that clients of the API will be able to easily identify
them by their "trailer" moniker, just like all the other functions
already exposed by trailer.h.

Take the opportunity to start putting trailer processing options (opts)
as the first parameter. This will be the pattern going forward in this
series.

Helped-by: Junio C Hamano <[e-mail]>
Signed-off-by: Linus Arver <[e-mail]>
---
 builtin/interpret-trailers.c |  4 ++--
 trailer.c                    | 26 +++++++++++++-------------
 trailer.h                    |  6 +++---
 3 files changed, 18 insertions(+), 18 deletions(-)

Comments

Christian Couder Feb. 19, 2024, 9:31 p.m. UTC | #1

On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget
<[e-mail]> wrote:
>
> From: Linus Arver <[e-mail]>
>
> In the next patch, we will move "process_trailers" from trailer.c to
> builtin/interpret-trailers.c. That move will necessitate the growth of
> the trailer.h API, forcing us to expose some additional functions in
> trailer.h.

Nit: actually this patch renames process_trailers() to
interpret_trailers() so the function that will be moved will be
interpret_trailers().

Nit: this patch and the next one will become commits, so perhaps:

s/In the next patch/In a following commit/

> Rename relevant functions so that they include the term "trailer" in
> their name, so that clients of the API will be able to easily identify
> them by their "trailer" moniker, just like all the other functions
> already exposed by trailer.h.

Except that "process_trailers()" already contains "trailer" but will
still be renamed by this patch to "interpret_trailers()". So I think
it might be nice to explain a bit why renaming process_trailers() to
interpret_trailers() makes sense too.

Also I think the subject, "trailer: prepare to expose functions as
part of API" could be more explicit about what the patch is actually
doing, like perhaps "trailer: rename functions to use 'trailer'".

In general, when there is a patch called "prepare to do X", then we
might expect a following patch called something like "actually do X".
But there isn't any patch in the series named like "trailer: expose
functions as part of API".

> Take the opportunity to start putting trailer processing options (opts)
> as the first parameter. This will be the pattern going forward in this
> series.

It's interesting to know that this will be the pattern going forward
in the series, but that doesn't quite tell why it's a good idea to do
it.

So I think it might be nice to repeat an explanation similar to the
one you give in "trailer: start preparing for formatting unification"
for format_trailers_from_commit():

"Reorder parameters for format_trailers_from_commit() to prefer

    const struct process_trailer_options *opts

as the first parameter, because these options are intimately tied to
formatting trailers."

And maybe also say that parameters like `FILE *outfile` should be last
because they are some kind of 'out' parameters.

> diff --git a/trailer.c b/trailer.c
> index f74915bd8cd..916175707d8 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -163,12 +163,12 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
>                 fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
>  }
>
> -static void print_all(FILE *outfile, struct list_head *head,
> -                     const struct process_trailer_options *opts)
> +static void format_trailers(const struct process_trailer_options *opts,
> +                           struct list_head *trailers, FILE *outfile)

This also renames `struct list_head *head` to `struct list_head
*trailers`. I think it would be nice if the commit message could talk
a bit about these renames too.

Linus Arver Feb. 29, 2024, 10:33 p.m. UTC | #2

Christian Couder <[e-mail]> writes:

> On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget
> <[e-mail]> wrote:
>>
>> From: Linus Arver <[e-mail]>
>>
>> In the next patch, we will move "process_trailers" from trailer.c to
>> builtin/interpret-trailers.c. That move will necessitate the growth of
>> the trailer.h API, forcing us to expose some additional functions in
>> trailer.h.
>
> Nit: actually this patch renames process_trailers() to
> interpret_trailers() so the function that will be moved will be
> interpret_trailers().

Oops, fixed locally.

> Nit: this patch and the next one will become commits, so perhaps:
>
> s/In the next patch/In a following commit/

TBH I've always wondered whether "patch" or "commit" matters --- I've
seen examples of patch series that referred to "commits" instead of
"patches", and vice versa. I was hoping to hear an opinion on this, so
I'm happy to see (and apply) your suggestion. Thanks.

>> Rename relevant functions so that they include the term "trailer" in
>> their name, so that clients of the API will be able to easily identify
>> them by their "trailer" moniker, just like all the other functions
>> already exposed by trailer.h.
>
> Except that "process_trailers()" already contains "trailer" but will
> still be renamed by this patch to "interpret_trailers()". So I think
> it might be nice to explain a bit why renaming process_trailers() to
> interpret_trailers() makes sense too.

I will add something like:

    Rename process_trailers() to interpret_trailers(), because it
    matches the name for the builtin command of the same name
    (git-interpret-trailers), which is the sole user of
    process_trailers().

> Also I think the subject, "trailer: prepare to expose functions as
> part of API" could be more explicit about what the patch is actually
> doing, like perhaps "trailer: rename functions to use 'trailer'".

Applied.

> In general, when there is a patch called "prepare to do X", then we
> might expect a following patch called something like "actually do X".
> But there isn't any patch in the series named like "trailer: expose
> functions as part of API".

Sounds like a very sensible rule. IOW, leave the detailed explanation to
the commit message and use the subject line only for the most obvious
explanation of what's going on in the patch. +1

>> Take the opportunity to start putting trailer processing options (opts)
>> as the first parameter. This will be the pattern going forward in this
>> series.
>
> It's interesting to know that this will be the pattern going forward
> in the series, but that doesn't quite tell why it's a good idea to do
> it.
>
> So I think it might be nice to repeat an explanation similar to the
> one you give in "trailer: start preparing for formatting unification"
> for format_trailers_from_commit():
>
> "Reorder parameters for format_trailers_from_commit() to prefer
>
>     const struct process_trailer_options *opts
>
> as the first parameter, because these options are intimately tied to
> formatting trailers."
>
> And maybe also say that parameters like `FILE *outfile` should be last
> because they are some kind of 'out' parameters.

SGTM, will do.

>> diff --git a/trailer.c b/trailer.c
>> index f74915bd8cd..916175707d8 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -163,12 +163,12 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
>>                 fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
>>  }
>>
>> -static void print_all(FILE *outfile, struct list_head *head,
>> -                     const struct process_trailer_options *opts)
>> +static void format_trailers(const struct process_trailer_options *opts,
>> +                           struct list_head *trailers, FILE *outfile)
>
> This also renames `struct list_head *head` to `struct list_head
> *trailers`. I think it would be nice if the commit message could talk
> a bit about these renames too.

Ah nice catch. Will do.

Really appreciate the quality of your reviews, thanks so much! :)

Junio C Hamano Feb. 29, 2024, 11:21 p.m. UTC | #3

Linus Arver <[e-mail]> writes:

>> Nit: this patch and the next one will become commits, so perhaps:
>>
>> s/In the next patch/In a following commit/
>
> TBH I've always wondered whether "patch" or "commit" matters --- I've
> seen examples of patch series that referred to "commits" instead of
> "patches", and vice versa. I was hoping to hear an opinion on this, so
> I'm happy to see (and apply) your suggestion. Thanks.

I think it is just fine to use either; sticking to one you pick
consistently in the same series would have value.  If you prefer
commit, then fine.  If you like patch, that's fine too.

Linus Arver Feb. 29, 2024, 11:53 p.m. UTC | #4

Junio C Hamano <[e-mail]> writes:

> Linus Arver <[e-mail]> writes:
>
>>> Nit: this patch and the next one will become commits, so perhaps:
>>>
>>> s/In the next patch/In a following commit/
>>
>> TBH I've always wondered whether "patch" or "commit" matters --- I've
>> seen examples of patch series that referred to "commits" instead of
>> "patches", and vice versa. I was hoping to hear an opinion on this, so
>> I'm happy to see (and apply) your suggestion. Thanks.
>
> I think it is just fine to use either; sticking to one you pick
> consistently in the same series would have value.  If you prefer
> commit, then fine.  If you like patch, that's fine too.

Makes sense, thanks.

Patch

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 033bd1556cf..85a3413baf5 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -132,11 +132,11 @@  int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
-			process_trailers(argv[i], &opts, &trailers);
+			interpret_trailers(&opts, &trailers, argv[i]);
 	} else {
 		if (opts.in_place)
 			die(_("no input file given for in-place editing"));
-		process_trailers(NULL, &opts, &trailers);
+		interpret_trailers(&opts, &trailers, NULL);
 	}
 
 	new_trailers_clear(&trailers);
diff --git a/trailer.c b/trailer.c
index f74915bd8cd..916175707d8 100644
--- a/trailer.c
+++ b/trailer.c
@@ -163,12 +163,12 @@  static void print_tok_val(FILE *outfile, const char *tok, const char *val)
 		fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
 }
 
-static void print_all(FILE *outfile, struct list_head *head,
-		      const struct process_trailer_options *opts)
+static void format_trailers(const struct process_trailer_options *opts,
+			    struct list_head *trailers, FILE *outfile)
 {
 	struct list_head *pos;
 	struct trailer_item *item;
-	list_for_each(pos, head) {
+	list_for_each(pos, trailers) {
 		item = list_entry(pos, struct trailer_item, list);
 		if ((!opts->trim_empty || strlen(item->value) > 0) &&
 		    (!opts->only_trailers || item->token))
@@ -589,7 +589,7 @@  static int git_trailer_config(const char *conf_key, const char *value,
 	return 0;
 }
 
-static void ensure_configured(void)
+static void trailer_config_init(void)
 {
 	if (configured)
 		return;
@@ -1035,10 +1035,10 @@  static void parse_trailers(struct trailer_info *info,
 	}
 }
 
-static void free_all(struct list_head *head)
+static void free_trailers(struct list_head *trailers)
 {
 	struct list_head *pos, *p;
-	list_for_each_safe(pos, p, head) {
+	list_for_each_safe(pos, p, trailers) {
 		list_del(pos);
 		free_trailer_item(list_entry(pos, struct trailer_item, list));
 	}
@@ -1075,16 +1075,16 @@  static FILE *create_in_place_tempfile(const char *file)
 	return outfile;
 }
 
-void process_trailers(const char *file,
-		      const struct process_trailer_options *opts,
-		      struct list_head *new_trailer_head)
+void interpret_trailers(const struct process_trailer_options *opts,
+			struct list_head *new_trailer_head,
+			const char *file)
 {
 	LIST_HEAD(head);
 	struct strbuf sb = STRBUF_INIT;
 	struct trailer_info info;
 	FILE *outfile = stdout;
 
-	ensure_configured();
+	trailer_config_init();
 
 	read_input_file(&sb, file);
 
@@ -1110,8 +1110,8 @@  void process_trailers(const char *file,
 		process_trailers_lists(&head, &arg_head);
 	}
 
-	print_all(outfile, &head, opts);
-	free_all(&head);
+	format_trailers(opts, &head, outfile);
+	free_trailers(&head);
 
 	/* Print the lines after the trailers as is */
 	if (!opts->only_trailers)
@@ -1134,7 +1134,7 @@  void trailer_info_get(struct trailer_info *info, const char *str,
 	size_t nr = 0, alloc = 0;
 	char **last = NULL;
 
-	ensure_configured();
+	trailer_config_init();
 
 	end_of_log_message = find_end_of_log_message(str, opts->no_divider);
 	trailer_block_start = find_trailer_block_start(str, end_of_log_message);
diff --git a/trailer.h b/trailer.h
index 1644cd05f60..37033e631a1 100644
--- a/trailer.h
+++ b/trailer.h
@@ -81,9 +81,9 @@  struct process_trailer_options {
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
 
-void process_trailers(const char *file,
-		      const struct process_trailer_options *opts,
-		      struct list_head *new_trailer_head);
+void interpret_trailers(const struct process_trailer_options *opts,
+			struct list_head *new_trailer_head,
+			const char *file);
 
 void trailer_info_get(struct trailer_info *info, const char *str,
 		      const struct process_trailer_options *opts);