[v2,0/6] Trailer readability cleanups

Message

Philippe Blain via GitGitGadget Sept. 9, 2023, 6:16 a.m. UTC

These patches were created while digging into the trailer code to better
understand how it works, in preparation for making the trailer.{c,h} files
as small as possible to make them available as a library for external users.
This series was originally created as part of [1], but are sent here
separately because the changes here are arguably more subjective in nature.
I think Patch 1 is the most important in this series. The others can wait,
if folks are opposed to adding them on their own merits at this point in
time.

These patches do not add or change any features. Instead, their goal is to
make the code easier to understand for new contributors (like myself), by
making various cleanups and improvements. Ultimately, my hope is that with
such cleanups, we are better positioned to make larger changes (especially
the broader libification effort, as in "Introduce Git Standard Library"
[2]).

Patch 1 was inspired by 576de3d956 (unpack_trees: start splitting internal
fields from public API, 2023-02-27) [3], and is in preparation for a
libification effort in the future around the trailer code. Independent of
libification, it still makes sense to discourage callers from peeking into
these trailer-internal fields.

Patches 2-3 aim to make some functions do a little less multitasking.

Patch 4 makes the find_patch_start function care about the "--no-divider"
option, because it that option matters for determining the start of the
"patch part" of the input.

Patch 5 is a renaming change to reduce overloaded language in the codebase.
It is inspired by 229d6ab6bf (doc: trailer: examples: avoid the word
"message" by itself, 2023-06-15) [4], which did a similar thing for the
interpret-trailers documentation.

Patch 6 makes trailer_info use offsets for trailer_start and trailer_end.


Updates in v2
=============

 * Patch 1: Drop the use of a #define. Instead just use an anonymous struct
   named internal.
 * Patch 2: Don't free info out parameter inside parse_trailers(). Instead
   free it from the caller, process_trailers(). Update comment in
   parse_trailers().
 * Patch 3: Reword commit message.
 * Patch 4: Mention be3d654343 (commit: pass --no-divider to
   interpret-trailers, 2023-06-17) in commit message.
 * Added Patch 6 to make trailer_info use offsets for trailer_start and
   trailer_end (thanks to Glen Choo for the suggestion).

[1]
https://lore.kernel.org/git/[e-mail]/T/#mb044012670663d8eb7a548924bbcc933bef116de
[2]
https://lore.kernel.org/git/[e-mail]/
[3]
https://lore.kernel.org/git/[e-mail]/
[4]
https://lore.kernel.org/git/6b4c[e-mail]/

Linus Arver (6):
  trailer: separate public from internal portion of trailer_iterator
  trailer: split process_input_file into separate pieces
  trailer: split process_command_line_args into separate functions
  trailer: teach find_patch_start about --no-divider
  trailer: rename *_DEFAULT enums to *_UNSPECIFIED
  trailer: use offsets for trailer_start/trailer_end

 trailer.c | 126 +++++++++++++++++++++++++++++-------------------------
 trailer.h |  19 ++++----
 2 files changed, 77 insertions(+), 68 deletions(-)


base-commit: 1b0a5129563ebe720330fdc8f5c6843d27641137
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1563%2Flistx%2Ftrailer-libification-prep-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1563/listx/trailer-libification-prep-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1563

Range-diff vs v1:

 1:  0bce4d4b0d5 ! 1:  4f116d2550f trailer: separate public from internal portion of trailer_iterator
     @@ Commit message
          trailer: separate public from internal portion of trailer_iterator
      
          The fields here are not meant to be used by downstream callers, so put
     -    them behind an anonymous struct named as
     -    "__private_to_trailer_c__do_not_use" to warn against their use.
     +    them behind an anonymous struct named as "internal" to warn against
     +    their use. This follows the pattern in 576de3d956 (unpack_trees: start
     +    splitting internal fields from public API, 2023-02-27).
      
     -    Internally, use a "#define" to keep the code tidy.
     -
     -    Helped-by: Junio C Hamano <[e-mail]>
          Signed-off-by: Linus Arver <[e-mail]>
      
       ## trailer.c ##
     -@@ trailer.c: void format_trailers_from_commit(struct strbuf *out, const char *msg,
     - 	trailer_info_release(&info);
     - }
     - 
     -+#define private __private_to_trailer_c__do_not_use
     -+
     - void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
     - {
     - 	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
     +@@ trailer.c: void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
       	strbuf_init(&iter->key, 0);
       	strbuf_init(&iter->val, 0);
       	opts.no_divider = 1;
      -	trailer_info_get(&iter->info, msg, &opts);
      -	iter->cur = 0;
     -+	trailer_info_get(&iter->private.info, msg, &opts);
     -+	iter->private.cur = 0;
     ++	trailer_info_get(&iter->internal.info, msg, &opts);
     ++	iter->internal.cur = 0;
       }
       
       int trailer_iterator_advance(struct trailer_iterator *iter)
       {
      -	while (iter->cur < iter->info.trailer_nr) {
      -		char *trailer = iter->info.trailers[iter->cur++];
     -+	while (iter->private.cur < iter->private.info.trailer_nr) {
     -+		char *trailer = iter->private.info.trailers[iter->private.cur++];
     ++	while (iter->internal.cur < iter->internal.info.trailer_nr) {
     ++		char *trailer = iter->internal.info.trailers[iter->internal.cur++];
       		int separator_pos = find_separator(trailer, separators);
       
       		if (separator_pos < 1)
     @@ trailer.c: int trailer_iterator_advance(struct trailer_iterator *iter)
       void trailer_iterator_release(struct trailer_iterator *iter)
       {
      -	trailer_info_release(&iter->info);
     -+	trailer_info_release(&iter->private.info);
     ++	trailer_info_release(&iter->internal.info);
       	strbuf_release(&iter->val);
       	strbuf_release(&iter->key);
       }
     @@ trailer.h: struct trailer_iterator {
      +	struct {
      +		struct trailer_info info;
      +		size_t cur;
     -+	} __private_to_trailer_c__do_not_use;
     ++	} internal;
       };
       
       /*
 2:  d023c297dca ! 2:  c00f4623d0b trailer: split process_input_file into separate pieces
     @@ trailer.c: static void unfold_value(struct strbuf *val)
      -				 struct list_head *head,
      -				 const struct process_trailer_options *opts)
      +/*
     -+ * Parse trailers in "str" and populate the "head" linked list structure.
     ++ * Parse trailers in "str", populating the trailer info and "head"
     ++ * linked list structure.
      + */
      +static void parse_trailers(struct trailer_info *info,
      +			     const char *str,
     @@ trailer.c: static void unfold_value(struct strbuf *val)
       			continue;
       		separator_pos = find_separator(trailer, separators);
      @@ trailer.c: static size_t process_input_file(FILE *outfile,
     + 					 strbuf_detach(&val, NULL));
       		}
       	}
     - 
     +-
      -	trailer_info_release(&info);
      -
      -	return info.trailer_end - str;
     -+	trailer_info_release(info);
       }
       
       static void free_all(struct list_head *head)
     @@ trailer.c: void process_trailers(const char *file,
       
       	if (!opts->only_input) {
       		LIST_HEAD(arg_head);
     +@@ trailer.c: void process_trailers(const char *file,
     + 	print_all(outfile, &head, opts);
     + 
     + 	free_all(&head);
     ++	trailer_info_release(&info);
     + 
     + 	/* Print the lines after the trailers as is */
     + 	if (!opts->only_trailers)
 3:  c8bb0136621 ! 3:  f78c2345fad trailer: split process_command_line_args into separate functions
     @@ Commit message
              (1) parse trailers from the configuration, and
              (2) parse trailers defined on the command line.
      
     -    Separate these concerns into parse_trailers_from_config and
     -    parse_trailers_from_command_line_args, respectively. Remove (now
     -    redundant) process_command_line_args.
     +    Separate (1) outside to a new function, parse_trailers_from_config.
     +    Rename the remaining logic to parse_trailers_from_command_line_args.
      
          Signed-off-by: Linus Arver <[e-mail]>
      
 4:  1fc060041db ! 4:  f5f507c4c6c trailer: teach find_patch_start about --no-divider
     @@ Commit message
      
          Instead, make find_patch_start aware of "--no-divider" and make it
          handle that case as well. This means we no longer need to call strlen at
     -    all and can just rely on the existing code in find_patch_start.
     +    all and can just rely on the existing code in find_patch_start. By
     +    forcing callers to consider this important option, we avoid the kind of
     +    mistake described in be3d654343 (commit: pass --no-divider to
     +    interpret-trailers, 2023-06-17).
      
          This patch will make unit testing a bit more pleasant in this area in
          the future when we adopt a unit testing framework, because we would not
 5:  7c9b63c2616 ! 5:  52958c3557c trailer: rename *_DEFAULT enums to *_UNSPECIFIED
     @@ Commit message
          (2) "Default" can also mean the "trailer.*" configurations themselves,
              because these configurations are used by "default" (ahead of the
              hardcoded defaults in (1)) if no command line arguments are
     -        provided.
     +        provided. This concept of defaulting back to the configurations was
     +        introduced in 0ea5292e6b (interpret-trailers: add options for
     +        actions, 2017-08-01).
      
          In addition, the corresponding *_DEFAULT values are chosen when the user
          provides the "--no-where", "--no-if-exists", or "--no-if-missing" flags
 -:  ----------- > 6:  0463066ebe0 trailer: use offsets for trailer_start/trailer_end