Trailer readability cleanups #1563
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There are issues in commit 13eb6df: |
listx
force-pushed
the
trailer-libification-prep
branch
3 times, most recently
from
92f742f to
26acd10
Compare
|
There are issues in commit 84ff89c: |
1 similar comment
|
There are issues in commit 84ff89c: |
listx
force-pushed
the
trailer-libification-prep
branch
from
26acd10 to
6e6b64c
Compare
|
There are issues in commit 84ff89c: |
listx
force-pushed
the
trailer-libification-prep
branch
from
6e6b64c to
7c9b63c
Compare
|
/submit |
|
Submitted as pull.1563.git.1691211879.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This branch is now known as |
|
This patch series was integrated into seen via git@2d857d1. |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -1214,20 +1214,22 @@ void format_trailers_from_commit(struct strbuf *out, const char *msg, | |||
| trailer_info_release(&info); | |||
There was a problem hiding this comment.
On the Git mailing list, Glen Choo wrote (reply to this):
As someone who isn't that familiar with trailer code, and will have less
time for the ML soon, this is more of a quick drive-by..
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +#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;
> 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;
> }
> --- a/trailer.h
> +++ b/trailer.h
> @@ -119,8 +119,10 @@ struct trailer_iterator {
> struct strbuf val;
>...
> /* private */
> - struct trailer_info info;
> - size_t cur;
> + struct {
> + struct trailer_info info;
> + size_t cur;
> + } __private_to_trailer_c__do_not_use;
> };
Interesting approach to "private members". I like that it's fairly
lightweight and clear. On the other hand, I think this will fail to
autocomplete on most people's development setups, and I don't think this
is worth the tradeoff.
This is the first instance of this I could find in the codebase. I'm not
really opposed to having a new way of doing things, but it would be nice
for us to be consistent with how we handle private members. Other
approaches I've seen are:
- Using a "larger" struct to hold private members and "downcasting" for
public users (struct dir_iterator and struct dir_iterator_int). I
dislike this because I think this enables 'wrong' memory access too
easily.
(As an aside, if we really wanted to 'strictly' enforce privateness in
this patch, shouldn't we move the "#define private" into the .c file,
the way dir_iterator_int is in the .c file?)
- Prefixing private members with "__" (khash.h and other header-only
libraries use this at least, not sure if we have this in the 'main
tree'). I think this works pretty well most of the time.
- Just marking private members with a comment. IMO this is good enough
the vast majority of the time - if something is private for a good
reason, it's unlikely to get used accidentally anyway. But properly
enforcing "privateness" is worthy goal anyway.
Personally, I think a decent tradeoff between enforcement and ergonomics
would be to use an inner struct like you do here, but name it something
autocomplete-friendly and obviously private, like "private" or
"_private". I suspect self-regulation and code review should be enough
to catch nearly all accidental uses of private members.There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
On 07/08/2023 22:16, Glen Choo wrote:
> As someone who isn't that familiar with trailer code, and will have less
> time for the ML soon, this is more of a quick drive-by..
This is a bit of a drive-by comment as well ...
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >> +#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;
>> 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;
>> }
>> --- a/trailer.h
>> +++ b/trailer.h
>> @@ -119,8 +119,10 @@ struct trailer_iterator {
>> struct strbuf val;
>> ...
>> /* private */
>> - struct trailer_info info;
>> - size_t cur;
>> + struct {
>> + struct trailer_info info;
>> + size_t cur;
>> + } __private_to_trailer_c__do_not_use;
>> };
> > Interesting approach to "private members". I like that it's fairly
> lightweight and clear. On the other hand, I think this will fail to
> autocomplete on most people's development setups, and I don't think this
> is worth the tradeoff.
> > This is the first instance of this I could find in the codebase. We have something similar in unpack_trees.h see 576de3d9560 (unpack_trees: start splitting internal fields from public API, 2023-02-27). That adds an "internal" member to "sturct unpack_trees" of type "struct unpack_trees_internal which seems to be a easier naming scheme.
> I'm not
> really opposed to having a new way of doing things, but it would be nice
> for us to be consistent with how we handle private members. Other
> approaches I've seen are:
> > - Using a "larger" struct to hold private members and "downcasting" for
> public users (struct dir_iterator and struct dir_iterator_int). I
> dislike this because I think this enables 'wrong' memory access too
> easily.
> > (As an aside, if we really wanted to 'strictly' enforce privateness in
> this patch, shouldn't we move the "#define private" into the .c file,
> the way dir_iterator_int is in the .c file?)
That #define is pretty ugly
Another common scheme is to have an opaque pointer to the private struct in the public struct (aka pimpl idiom). The merge machinery uses this - see merge-recursive.h. (I'm working on something similar for the sequencer so we can change the internals without having to re-compile everything that includes "sequencer.h")
> - Prefixing private members with "__" (khash.h and other header-only
> libraries use this at least, not sure if we have this in the 'main
> tree'). I think this works pretty well most of the time.
It is common but I think the C standard reserves names beginning with "__"
> - Just marking private members with a comment. IMO this is good enough
> the vast majority of the time - if something is private for a good
> reason, it's unlikely to get used accidentally anyway. But properly
> enforcing "privateness" is worthy goal anyway.
>
> Personally, I think a decent tradeoff between enforcement and ergonomics
> would be to use an inner struct like you do here, but name it something
> autocomplete-friendly and obviously private, like "private" or
> "_private".
I agree, something like that would match the unpack_trees example
> I suspect self-regulation and code review should be enough
> to catch nearly all accidental uses of private members.
Agreed
Best Wishes
Phillip
There was a problem hiding this comment.
On the Git mailing list, Linus Arver wrote (reply to this):
Glen Choo <chooglen@google.com> writes:
> As someone who isn't that familiar with trailer code, and will have less
> time for the ML soon, this is more of a quick drive-by..
Aren't you also going on vacation soon? ;-)
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> +#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;
>> 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;
>> }
>> --- a/trailer.h
>> +++ b/trailer.h
>> @@ -119,8 +119,10 @@ struct trailer_iterator {
>> struct strbuf val;
>>...
>> /* private */
>> - struct trailer_info info;
>> - size_t cur;
>> + struct {
>> + struct trailer_info info;
>> + size_t cur;
>> + } __private_to_trailer_c__do_not_use;
>> };
>
> [...]
>
> This is the first instance of this I could find in the codebase. I'm not
> really opposed to having a new way of doing things, but it would be nice
> for us to be consistent with how we handle private members. Other
> approaches I've seen are:
>
> - Using a "larger" struct to hold private members and "downcasting" for
> public users (struct dir_iterator and struct dir_iterator_int). I
> dislike this because I think this enables 'wrong' memory access too
> easily.
> [...]
> - Prefixing private members with "__" (khash.h and other header-only
> libraries use this at least, not sure if we have this in the 'main
> tree'). I think this works pretty well most of the time.
> - Just marking private members with a comment. IMO this is good enough
> the vast majority of the time - if something is private for a good
> reason, it's unlikely to get used accidentally anyway. But properly
> enforcing "privateness" is worthy goal anyway.
Thanks for documenting these other approaches.
I prefer the "larger" struct to hold private members pattern. More
specifically I like the container_of approach pointed out by Jacob [2],
because it is an established pattern in the Linux Kernel and because we
already sort of use the same idea in the list_head type we imported from
the Kernel in 94e99012fc (http-walker: reduce O(n) ops with
doubly-linked list, 2016-07-11). That is, for example for the
new_trailer_item struct we have
struct new_trailer_item {
struct list_head list;
<list item stuff>
};
and to me this is symmetric to the container_of pattern described by Jacob:
struct dir_entry_private {
struct dir_entry entry;
<private stuff>
};
Accordingly, we are already doing the "structure pointer math" (which
Jacob described in [2]) for list_head in list.h:
/* Get typed element from list at a given position. */
#define list_entry(ptr, type, member) \
((type *) ((char *) (ptr) - offsetof(type, member)))
In this patch series though, I decided to just stick with giving the
struct a private-sounding name, because I don't think we reached
consensus on what the preferred approach is for separating
public/private fields.
> (As an aside, if we really wanted to 'strictly' enforce privateness in
> this patch, shouldn't we move the "#define private" into the .c file,
> the way dir_iterator_int is in the .c file?)
I think you meant moving the struct into the .c file (the "#define" is
already in the .c file).
> Personally, I think a decent tradeoff between enforcement and ergonomics
> would be to use an inner struct like you do here, but name it something
> autocomplete-friendly and obviously private, like "private" or
> "_private".
SGTM. I think I'll go with "internal" though, to align with 576de3d956
(unpack_trees: start splitting internal fields from public API,
2023-02-27) which Phillip pointed out. Will reroll.
> I suspect self-regulation and code review should be enough
> to catch nearly all accidental uses of private members.
Ack. In the future, if and when we want compiler-level guarantees to
make it impossible (this was the discussion at [1]), we can revisit this
area.
[1] https://lore.kernel.org/git/16ff5069-0408-21cd-995c-8b47afb9810d@github.com/
[2] https://lore.kernel.org/git/CA+P7+xo02dGkjb5DwJ1Af_hoQ5HiuxASheZxoFz+r6B-6cQMug@mail.gmail.com/There was a problem hiding this comment.
On the Git mailing list, Linus Arver wrote (reply to this):
Phillip Wood <phillip.wood123@gmail.com> writes:
> We have something similar in unpack_trees.h see 576de3d9560
> (unpack_trees: start splitting internal fields from public API,
> 2023-02-27). That adds an "internal" member to "sturct unpack_trees" of
> type "struct unpack_trees_internal which seems to be a easier naming scheme.
Ack, I will use "internal" as the member name in the next reroll.
>>> +#define private __private_to_trailer_c__do_not_use
> [...]
> That #define is pretty ugly
Haha, indeed. But I think that's the point though (i.e., the degree of
ugliness matches the strength of our codebase's posture to discourage
its use by external users).
I will drop the #define in the next reroll though, so, I guess it's a
moot point anyway.
> Another common scheme is to have an opaque pointer to the private struct
> in the public struct (aka pimpl idiom). The merge machinery uses this
> - see merge-recursive.h. (I'm working on something similar for the
> sequencer so we can change the internals without having to re-compile
> everything that includes "sequencer.h")
Very interesting! I look forward to seeing your work. :)
>> - Prefixing private members with "__" (khash.h and other header-only
>> libraries use this at least, not sure if we have this in the 'main
>> tree'). I think this works pretty well most of the time.
>
> It is common but I think the C standard reserves names beginning with "__"
Indeed (see [1]).
[1] https://devblogs.microsoft.com/oldnewthing/20230109-00/?p=107685
|
User |
|
This patch series was integrated into seen via git@1e74597. |
trailer.c
Outdated
| @@ -961,28 +961,23 @@ static void unfold_value(struct strbuf *val) | |||
| strbuf_release(&out); | |||
There was a problem hiding this comment.
On the Git mailing list, Glen Choo wrote (reply to this):
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Currently, process_input_file does three things:
>
> (1) parse the input string for trailers,
> (2) print text before the trailers, and
> (3) calculate the position of the input where the trailers end.
>
> Rename this function to parse_trailers(), and make it only do
> (1).
Okay, process_input_file() is a very unhelpful name (What does it mean
to "process a file"?). In contrast, parse_trailers() is more
self-descriptive (It parses trailers into some appropriate format, so it
shouldn't do things like print.) Makes sense.
Is there some additional, unstated purpose behind this change besides
"move things around for readability"? E.g. do you intend to move
parse_trailers() to a future trailer parsing library? If so, that would
be useful context to evaluate the goodness of this split.
> The caller of this function, process_trailers, becomes responsible
> for (2) and (3). These items belong inside process_trailers because they
> are both concerned with printing the surrounding text around
> trailers (which is already one of the immediate concerns of
> process_trailers).
I agree that (2) doesn't belong in parse_trailers(). OTOH, (3) sounds
like something that belongs in parse_trailers() - you have to parse
trailers in order to tell where the trailers start and end, so it makes
sense for the parsing function to give those values.
> diff --git a/trailer.c b/trailer.c
> index dff3fafe865..16fbba03d07 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -961,28 +961,23 @@ static void unfold_value(struct strbuf *val)
> strbuf_release(&out);
> }
>
> -static size_t process_input_file(FILE *outfile,
> - const char *str,
> - struct list_head *head,
> - const struct process_trailer_options *opts)
> +/*
> + * Parse trailers in "str" and populate the "head" linked list structure.
> + */
> +static void parse_trailers(struct trailer_info *info,
"info" is an out parameter, and IIRC we typically put out parameters
towards the end. I didn't find a callout in CodingGuidelines, though, so
idk if this is an ironclad rule or not.
> + const char *str,
> + struct list_head *head,
> + const struct process_trailer_options *opts)
> {
> - struct trailer_info info;
> struct strbuf tok = STRBUF_INIT;
> struct strbuf val = STRBUF_INIT;
> size_t i;
>
> - trailer_info_get(&info, str, opts);
> -
> - /* Print lines before the trailers as is */
> - if (!opts->only_trailers)
> - fwrite(str, 1, info.trailer_start - str, outfile);
We no longer fwrite the contents before the trailer, okay.
> + trailer_info_get(info, str, opts);
This is where we actually get the start and end of trailers, and each
trailer string. This is parsing out the trailers from a string, so what
other parsing is left? Reading ahead shows that we're actually parsing
the trailer string into a "struct trailer_item". Okay, so this function
is basically a wrapper around trailer_info_get() that also "returns" the
parsed trailer_items.
> - if (!opts->only_trailers && !info.blank_line_before_trailer)
> - fprintf(outfile, "\n");
> -
So we don't print the trailing line. Also makes sense.
> @@ -1003,9 +998,7 @@ static size_t process_input_file(FILE *outfile,
> }
> }
>
> - trailer_info_release(&info);
> -
> - return info.trailer_end - str;
> + trailer_info_release(info);
> }
>
Even though "info" is a pointer passed into this function, we are
_release-ing it. This is not an umabiguously good change, IMO. Before,
"info" was never used outside of this function, so we should obviously
release it before returning. However, now that "info" is an out
parameter, we should be more careful about releasing it. I don't think
it's obvious that the caller will see the right values for
info.trailer_end and info.trailer_start, but free()-d values for
info.trailers, and a meaningless value for info.trailer_nr (since the
items were free()-d).
I think it might be better to update the comment on parse_trailers()
like so:
/*
* Parse trailers in "str", populating the trailer info and "head"
* linked list structure.
*/
and make it the caller's responsibility to call trailer_info_release().
We could move this call to where we "free_all(head)".
> static void free_all(struct list_head *head)
> @@ -1054,6 +1047,7 @@ void process_trailers(const char *file,
> {
> LIST_HEAD(head);
> struct strbuf sb = STRBUF_INIT;
> + struct trailer_info info;
> size_t trailer_end;
> FILE *outfile = stdout;
>
> @@ -1064,8 +1058,16 @@ void process_trailers(const char *file,
> if (opts->in_place)
> outfile = create_in_place_tempfile(file);
Thinking out loud, should we move the creation of outfile next to where
we first use it?
> + parse_trailers(&info, sb.buf, &head, opts);
> + trailer_end = info.trailer_end - sb.buf;
> +
> /* Print the lines before the trailers */
> - trailer_end = process_input_file(outfile, sb.buf, &head, opts);
> + if (!opts->only_trailers)
> + fwrite(sb.buf, 1, info.trailer_start - sb.buf, outfile);
I'm not sure if it is an unambiguously good change for the caller to
learn how to compute the start and end of the trailer sections by doing
pointer arithmetic, but I guess format_trailer_info() does this anyway,
so your proposal to move (3) outside of the parse_trailers() makes
sense.
It feels a bit non-obvious that trailer_start and trailer_end are
pointing inside the input string. I wonder if we should just return the
_start and _end offsets directly instead of returning pointers. I.e.:
struct trailer_info {
int blank_line_before_trailer;
- /*
- * Pointers to the start and end of the trailer block found. If there
- * is no trailer block found, these 2 pointers point to the end of the
- * input string.
- */
- const char *trailer_start, *trailer_end;
+ /* Offsets to the trailer block start and end in the input string */
+ size_t *trailer_start, *trailer_end;
Which makes their intended use fairly unambiguous. A quick grep suggests
that in trailer.c, we're roughly as likely to use the pointer directly
vs using it to do pointer arithmetic, so converging on one use might be
a win for readability. The only other user outside of trailer.c is
sequencer.c, which doesn't care about the return type - it only checks
if there are trailers.There was a problem hiding this comment.
On the Git mailing list, Linus Arver wrote (reply to this):
Glen Choo <chooglen@google.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> Currently, process_input_file does three things:
>>
>> (1) parse the input string for trailers,
>> (2) print text before the trailers, and
>> (3) calculate the position of the input where the trailers end.
>>
>> Rename this function to parse_trailers(), and make it only do
>> (1).
>
> [...]
>
> Is there some additional, unstated purpose behind this change besides
> "move things around for readability"? E.g. do you intend to move
> parse_trailers() to a future trailer parsing library? If so, that would
> be useful context to evaluate the goodness of this split.
I think it's still too early to say whether certain functions will make
it (unmodified) into the public, libified API. So currently "move things
around for readability" is the most concrete reason.
>> The caller of this function, process_trailers, becomes responsible
>> for (2) and (3). These items belong inside process_trailers because they
>> are both concerned with printing the surrounding text around
>> trailers (which is already one of the immediate concerns of
>> process_trailers).
>
> I agree that (2) doesn't belong in parse_trailers(). OTOH, (3) sounds
> like something that belongs in parse_trailers() - you have to parse
> trailers in order to tell where the trailers start and end, so it makes
> sense for the parsing function to give those values.
I don't think (3) should belong in parse_trailers, mainly because the
"info" struct we pass into it gets this information populated by
parse_trailers already. Which is why we can do (3) in the caller with
parse_trailers(&info, sb.buf, &head, opts);
trailer_end = info.trailer_end - sb.buf;
to get the same information. Also, the endpoint of the trailers is no
more inherently special than, for example, the following other possible
return values:
- the number of trailers that were recognized and parsed
- whether we encountered any trailers or not
- the start position of when we first encountered a trailer in the input
which makes me want to avoid returning this "trailer_end" value from
parse_trailers.
One more thing: we already have a function named "find_trailer_end"
which is supposed to do this already. But it uses "ignore_non_trailer"
from commit.c (that function should probably use the trailer API later
on to figure this out...). I wanted to clean that part up in the future
as part of libifcation.
>> -static size_t process_input_file(FILE *outfile,
>> - const char *str,
>> - struct list_head *head,
>> - const struct process_trailer_options *opts)
>> +/*
>> + * Parse trailers in "str" and populate the "head" linked list structure.
>> + */
>> +static void parse_trailers(struct trailer_info *info,
>
> "info" is an out parameter, and IIRC we typically put out parameters
> towards the end. I didn't find a callout in CodingGuidelines, though, so
> idk if this is an ironclad rule or not.
I wanted to minimize churn as much as possible (hence my hesitation with
changing around the order of these parameters). But also,
trailer_info_get uses "info" as the first parameter, so I wanted to
align with that usage.
>> @@ -1003,9 +998,7 @@ static size_t process_input_file(FILE *outfile,
>> }
>> }
>>
>> - trailer_info_release(&info);
>> -
>> - return info.trailer_end - str;
>> + trailer_info_release(info);
>> }
>>
>
> Even though "info" is a pointer passed into this function, we are
> _release-ing it. This is not an umabiguously good change, IMO. Before,
> "info" was never used outside of this function, so we should obviously
> release it before returning. However, now that "info" is an out
> parameter, we should be more careful about releasing it.
Hmm, agreed.
> I don't think
> it's obvious that the caller will see the right values for
> info.trailer_end and info.trailer_start, but free()-d values for
> info.trailers, and a meaningless value for info.trailer_nr (since the
> items were free()-d).
Agreed. Will update to avoid calling trailer_info_release() inside
parse_trailers() because the caller might still need that information. I
think the fix is to move the trailer_info_get outside to the caller,
much like how format_trailers_from_commit() does it.
> I think it might be better to update the comment on parse_trailers()
> like so:
>
> /*
> * Parse trailers in "str", populating the trailer info and "head"
> * linked list structure.
> */
>
> and make it the caller's responsibility to call trailer_info_release().
> We could move this call to where we "free_all(head)".
SGTM. (I regret not reading this text before drafting my response above.)
>> static void free_all(struct list_head *head)
>> @@ -1054,6 +1047,7 @@ void process_trailers(const char *file,
>> {
>> LIST_HEAD(head);
>> struct strbuf sb = STRBUF_INIT;
>> + struct trailer_info info;
>> size_t trailer_end;
>> FILE *outfile = stdout;
>>
>> @@ -1064,8 +1058,16 @@ void process_trailers(const char *file,
>> if (opts->in_place)
>> outfile = create_in_place_tempfile(file);
>
> Thinking out loud, should we move the creation of outfile next to where
> we first use it?
Not sure what you mean here. Can you clarify?
>> + parse_trailers(&info, sb.buf, &head, opts);
>> + trailer_end = info.trailer_end - sb.buf;
>> +
>> /* Print the lines before the trailers */
>> - trailer_end = process_input_file(outfile, sb.buf, &head, opts);
>> + if (!opts->only_trailers)
>> + fwrite(sb.buf, 1, info.trailer_start - sb.buf, outfile);
>
> I'm not sure if it is an unambiguously good change for the caller to
> learn how to compute the start and end of the trailer sections by doing
> pointer arithmetic,
I think a future cleanup (in a follow-up series) involving
find_trailer_end should simplify this area and avoid the need for
pointer arithmetic in the caller.
> It feels a bit non-obvious that trailer_start and trailer_end are
> pointing inside the input string. I wonder if we should just return the
> _start and _end offsets directly instead of returning pointers. I.e.:
>
> struct trailer_info {
> int blank_line_before_trailer;
> - /*
> - * Pointers to the start and end of the trailer block found. If there
> - * is no trailer block found, these 2 pointers point to the end of the
> - * input string.
> - */
> - const char *trailer_start, *trailer_end;
> + /* Offsets to the trailer block start and end in the input string */
> + size_t *trailer_start, *trailer_end;
>
> Which makes their intended use fairly unambiguous. A quick grep suggests
> that in trailer.c, we're roughly as likely to use the pointer directly
> vs using it to do pointer arithmetic, so converging on one use might be
> a win for readability.
Agreed! I would prefer to use offsets everywhere, as I think that is
more direct (because we are concerned with locations in the input).
trailer.c
Outdated
| @@ -711,30 +711,35 @@ static void add_arg_item(struct list_head *arg_head, char *tok, char *val, | |||
| list_add_tail(&new_item->list, arg_head); | |||
There was a problem hiding this comment.
On the Git mailing list, Glen Choo wrote (reply to this):
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Previously, process_command_line_args did two things:
>
> (1) parse trailers from the configuration, and
> (2) parse trailers defined on the command line.
It parses trailers from two places, but it still only does "one thing",
in that it only parses trailers.
> @@ -711,30 +711,35 @@ static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
> list_add_tail(&new_item->list, arg_head);
> }
>
> -static void process_command_line_args(struct list_head *arg_head,
> - struct list_head *new_trailer_head)
> +static void parse_trailers_from_config(struct list_head *config_head)
> {
> struct arg_item *item;
> - struct strbuf tok = STRBUF_INIT;
> - struct strbuf val = STRBUF_INIT;
> - const struct conf_info *conf;
> struct list_head *pos;
>
> - /*
> - * In command-line arguments, '=' is accepted (in addition to the
> - * separators that are defined).
> - */
> - char *cl_separators = xstrfmt("=%s", separators);
> -
> /* Add an arg item for each configured trailer with a command */
> list_for_each(pos, &conf_head) {
> item = list_entry(pos, struct arg_item, list);
> if (item->conf.command)
> - add_arg_item(arg_head,
> + add_arg_item(config_head,
> xstrdup(token_from_item(item, NULL)),
> xstrdup(""),
> &item->conf, NULL);
> }
> +}
> +
> +static void parse_trailers_from_command_line_args(struct list_head *arg_head,
> + struct list_head *new_trailer_head)
> +{
> + struct strbuf tok = STRBUF_INIT;
> + struct strbuf val = STRBUF_INIT;
> + const struct conf_info *conf;
> + struct list_head *pos;
> +
> + /*
> + * In command-line arguments, '=' is accepted (in addition to the
> + * separators that are defined).
> + */
> + char *cl_separators = xstrfmt("=%s", separators);
>
> /* Add an arg item for each trailer on the command line */
> list_for_each(pos, new_trailer_head) {
I find this equally readable as the preimage, which IMO is adequately
scoped and commented.
> @@ -1070,8 +1075,11 @@ void process_trailers(const char *file,
>
>
> if (!opts->only_input) {
> + LIST_HEAD(config_head);
> LIST_HEAD(arg_head);
> - process_command_line_args(&arg_head, new_trailer_head);
> + parse_trailers_from_config(&config_head);
> + parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
> + list_splice(&config_head, &arg_head);
> process_trailers_lists(&head, &arg_head);
> }
But now, we have to remember to call two functions instead of just one.
This, and the slight additional churn makes me lean negative on this
change. I would be really happy if we had a use case where we only
wanted to call one function but not the other, but it seems like this
isn't the case.There was a problem hiding this comment.
On the Git mailing list, Linus Arver wrote (reply to this):
Glen Choo <chooglen@google.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> Previously, process_command_line_args did two things:
>>
>> (1) parse trailers from the configuration, and
>> (2) parse trailers defined on the command line.
>
> It parses trailers from two places, but it still only does "one thing",
> in that it only parses trailers.
More precisely, it parses trailers from the command line by first
parsing trailers from the configuration. In other words, parsing
trailers from the configuration (independent of the input string!) is a
required dependency for parsing trailers coming from the command line.
If we take a step back, we need to do 3 things:
(1) parse trailers from the configuration
(2) parse trailers from the command line
(3) parse trailers from the input
I think these three should be separated into separate functions. I think
no one wants to combine all three into one function. And I can't think
of a good enough reason to combine (1) and (2) together either. Hence
this patch.
> I find this equally readable as the preimage, which IMO is adequately
> scoped and commented.
Aside: is "preimage" the status quo before applying the patch?
>> @@ -1070,8 +1075,11 @@ void process_trailers(const char *file,
>>
>>
>> if (!opts->only_input) {
>> + LIST_HEAD(config_head);
>> LIST_HEAD(arg_head);
>> - process_command_line_args(&arg_head, new_trailer_head);
>> + parse_trailers_from_config(&config_head);
>> + parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
>> + list_splice(&config_head, &arg_head);
>> process_trailers_lists(&head, &arg_head);
>> }
>
> But now, we have to remember to call two functions instead of just one.
But only inside interpret-trailers.c, right?There was a problem hiding this comment.
On the Git mailing list, Linus Arver wrote (reply to this):
Linus Arver <linusa@google.com> writes:
>>
>> But now, we have to remember to call two functions instead of just one.
>
> But only inside interpret-trailers.c, right?
Oops, I meant trailer.c.
In the future I expect to move this to interpret-trailers.c.There was a problem hiding this comment.
On the Git mailing list, Glen Choo wrote (reply to this):
Linus Arver <linusa@google.com> writes:
>> I find this equally readable as the preimage, which IMO is adequately
>> scoped and commented.
>
> Aside: is "preimage" the status quo before applying the patch?
Yup.
trailer.c
Outdated
| @@ -807,14 +812,14 @@ static ssize_t last_line(const char *buf, size_t len) | |||
| * Return the position of the start of the patch or the length of str if there | |||
There was a problem hiding this comment.
On the Git mailing list, Glen Choo wrote (reply to this):
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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
> have to test multiple functions to check how finding the start of a
> patch part works (we would only need to test find_patch_start).
Unit tests typically only test external-facing interfaces, not
implementatino details, so without seeing the unit tests or library
boundary, it's hard to tell whether find_patch_start() is something we
want to unit test or not. I would have assumed it's not, given that it's
tiny and only has a single caller, so I'm hesitant to say that we should
definitely handle no_divider inside find_patch_start().
> @@ -812,14 +812,14 @@ static ssize_t last_line(const char *buf, size_t len)
> * Return the position of the start of the patch or the length of str if there
> * is no patch in the message.
> */
> -static size_t find_patch_start(const char *str)
> +static size_t find_patch_start(const char *str, int no_divider)
> {
> const char *s;
>
> for (s = str; *s; s = next_line(s)) {
> const char *v;
>
> - if (skip_prefix(s, "---", &v) && isspace(*v))
> + if (!no_divider && skip_prefix(s, "---", &v) && isspace(*v))
> return s - str;
> }
Assuming we wanted to make this unit-testable anyway, could we just move
the strlen() call into this function? Performance aside (I wouldn't be
surprised if a smart enough compiler could optimize away the noops), I
don't find this easier to understand. Now the reader needs to read the
code to see "if no_divider is given, noop until the end of the string,
at which point str will point to the end, and s - str will give us the
length of str", as opposed to "there are no dividers, so just return
strlen(str)".There was a problem hiding this comment.
On the Git mailing list, Linus Arver wrote (reply to this):
Glen Choo <chooglen@google.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> @@ -812,14 +812,14 @@ static ssize_t last_line(const char *buf, size_t len)
>> * Return the position of the start of the patch or the length of str if there
>> * is no patch in the message.
>> */
>> -static size_t find_patch_start(const char *str)
>> +static size_t find_patch_start(const char *str, int no_divider)
>> {
>> const char *s;
>>
>> for (s = str; *s; s = next_line(s)) {
>> const char *v;
>>
>> - if (skip_prefix(s, "---", &v) && isspace(*v))
>> + if (!no_divider && skip_prefix(s, "---", &v) && isspace(*v))
>> return s - str;
>> }
>
> Assuming we wanted to make this unit-testable anyway, could we just move
> the strlen() call into this function?
I don't see why we should preserve the if-statement and associated
strlen() call if we can just get rid of it.
> [...] I
> don't find this easier to understand. Now the reader needs to read the
> code to see "if no_divider is given, noop until the end of the string,
> at which point str will point to the end, and s - str will give us the
> length of str", as opposed to "there are no dividers, so just return
> strlen(str)".
The main idea behind this patch is to make find_patch_start() return the
correct answer. Currently it does not in all cases (whether --no-divider
is provided), and so the caller has to calculate the
start of the patch with strlen manually. By moving the --no-divider flag
into this function, we force all callers to consider this important
option.
For additional context we recently had to fix a bug where we weren't
passing in this flag to the interpret-trailers builtin. See be3d654343
(commit: pass --no-divider to interpret-trailers, 2023-06-17). There we
acknowledged that some callers forgot to pass in --no-divider to
interpret-trailers (such as the caller that was fixed up in that
commit).
I mention the above example because although it's not the exact same
thing as here, I think the scenario of "sometimes callers can forget
about --no-divider" is an important one to prevent wherever possible.
That's why I like this patch (in addition to the reasons cited in the
commit message).There was a problem hiding this comment.
On the Git mailing list, Glen Choo wrote (reply to this):
Linus Arver <linusa@google.com> writes:
> I don't see why we should preserve the if-statement and associated
> strlen() call if we can just get rid of it.
Here are some reasons:
- Without compiler optimizations, it is faster.
- Subjectively, I find the early return easier to understand.
I don't think I need to nitpick over such a tiny issue though, so I'm
okay either way.
trailer.c
Outdated
| @@ -388,7 +388,7 @@ static void process_trailers_lists(struct list_head *head, | |||
| int trailer_set_where(enum trailer_where *item, const char *value) | |||
There was a problem hiding this comment.
On the Git mailing list, Glen Choo wrote (reply to this):
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> (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.
Interesting, I would have never thought of config as 'default'. In fact,
I would have thought that this de facto behavior (which you also
clarified in [1]) is a bug if not for the fact that in an internal
version of this series, you cited a commit message that describes this
as expected behavior. That context would be very welcome in the ML, I
think.
[1] https://lore.kernel.org/git/6b427b4b1e82b1f01640f1f49fe8d1c2fd02111e.1691210737.git.gitgitgadget@gmail.com
> In addition, the corresponding *_DEFAULT values are chosen when the user
> provides the "--no-where", "--no-if-exists", or "--no-if-missing" flags
> on the command line. These "--no-*" flags are used to clear previously
> provided flags of the form "--where", "--if-exists", and "--if-missing".
> Using these "--no-*" flags undoes the specifying of these flags (if
> any), so using the word "UNSPECIFIED" is more natural here.
>
> So instead of using "*_DEFAULT", use "*_UNSPECIFIED" because this
> signals to the reader that the *_UNSPECIFIED value by itself carries no
> meaning (it's a zero value and by itself does not "default" to anything,
> necessitating the need to have some other way of getting to a useful
> value).
Makse sense. This seems like a good change.
> @@ -586,7 +586,10 @@ static void ensure_configured(void)
> if (configured)
> return;
>
> - /* Default config must be setup first */
> + /*
> + * Default config must be setup first. These defaults are used if there
> + * are no "trailer.*" or "trailer.<token>.*" options configured.
> + */
> default_conf_info.where = WHERE_END;
> default_conf_info.if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
> default_conf_info.if_missing = MISSING_ADD;
As mentioned earlier, I find it a bit odd that we're calling config
'default' (and also that we're calling CLI args config), but
renaming default_conf_info to config_conf_info sounds worse, so let's
leave it as-is.There was a problem hiding this comment.
On the Git mailing list, Linus Arver wrote (reply to this):
Glen Choo <chooglen@google.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> (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.
>
> Interesting, I would have never thought of config as 'default'. In fact,
> I would have thought that this de facto behavior (which you also
> clarified in [1]) is a bug if not for the fact that in an internal
> version of this series, you cited a commit message that describes this
> as expected behavior. That context would be very welcome in the ML, I
> think.
>
> [1] https://lore.kernel.org/git/6b427b4b1e82b1f01640f1f49fe8d1c2fd02111e.1691210737.git.gitgitgadget@gmail.com
I forget the internal version/discussion, but I assume you're thinking
of 0ea5292e6b (interpret-trailers: add options for actions, 2017-08-01).
I will reroll and mention it to the commit message.
>> In addition, the corresponding *_DEFAULT values are chosen when the user
>> provides the "--no-where", "--no-if-exists", or "--no-if-missing" flags
>> on the command line. These "--no-*" flags are used to clear previously
>> provided flags of the form "--where", "--if-exists", and "--if-missing".
>> Using these "--no-*" flags undoes the specifying of these flags (if
>> any), so using the word "UNSPECIFIED" is more natural here.
>>
>> So instead of using "*_DEFAULT", use "*_UNSPECIFIED" because this
>> signals to the reader that the *_UNSPECIFIED value by itself carries no
>> meaning (it's a zero value and by itself does not "default" to anything,
>> necessitating the need to have some other way of getting to a useful
>> value).
>
> Makse sense. This seems like a good change.
>
>> @@ -586,7 +586,10 @@ static void ensure_configured(void)
>> if (configured)
>> return;
>>
>> - /* Default config must be setup first */
>> + /*
>> + * Default config must be setup first. These defaults are used if there
>> + * are no "trailer.*" or "trailer.<token>.*" options configured.
>> + */
>> default_conf_info.where = WHERE_END;
>> default_conf_info.if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
>> default_conf_info.if_missing = MISSING_ADD;
>
> As mentioned earlier, I find it a bit odd that we're calling config
> 'default' (and also that we're calling CLI args config), but
> renaming default_conf_info to config_conf_info sounds worse, so let's
> leave it as-is.
SGTM. Although, we could also just rename it to "conf_info" (same name
as the struct name). Unless such same-variable-name-as-the-struct is
discouraged in the codebase.
|
User |
|
This patch series was integrated into seen via git@4144f29. |
|
There was a status update in the "New Topics" section about the branch Code clean-up. Needs review. source: <pull.1563.git.1691211879.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@0fac635. |
|
This patch series was integrated into seen via git@cb6a4d0. |
|
This patch series was integrated into seen via git@9c5a4dd. |
|
There was a status update in the "Cooking" section about the branch Code clean-up. Needs review. source: <pull.1563.git.1691211879.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@3d709b4. |
|
This patch series was integrated into seen via git@2a85e89. |
|
There was a status update in the "Cooking" section about the branch Code clean-up. Comments? source: <pull.1563.v5.git.1697828495.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@0662143. |
|
There was a status update in the "Cooking" section about the branch Code clean-up. Comments? source: <pull.1563.v5.git.1697828495.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch Code clean-up. Comments? source: <pull.1563.v5.git.1697828495.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@7e4d013. |
|
There was a status update in the "Cooking" section about the branch Code clean-up. Comments? source: <pull.1563.v5.git.1697828495.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@19620bf. |
|
This patch series was integrated into seen via git@8d9e5cf. |
|
There was a status update in the "Cooking" section about the branch Code clean-up. Comments? source: <pull.1563.v5.git.1697828495.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@b87396f. |
|
This patch series was integrated into seen via git@ff8c098. |
|
This patch series was integrated into seen via git@4f2af08. |
|
This patch series was integrated into seen via git@059d58c. |
|
This patch series was integrated into seen via git@293c408. |
|
There was a status update in the "Cooking" section about the branch Code clean-up. Comments? source: <pull.1563.v5.git.1697828495.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@18fc1ce. |
|
This patch series was integrated into seen via git@88d5c66. |
|
There was a status update in the "Cooking" section about the branch Code clean-up. Will merge to 'next'. source: <pull.1563.v5.git.1697828495.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@b927d10. |
|
This patch series was integrated into next via git@e26ede5. |
|
This patch series was integrated into seen via git@a990e32. |
|
There was a status update in the "Cooking" section about the branch Code clean-up. Will merge to 'master'. source: <pull.1563.v5.git.1697828495.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@eb9f735. |
|
This patch series was integrated into seen via git@59a29e1. |
|
This patch series was integrated into master via git@59a29e1. |
|
This patch series was integrated into next via git@59a29e1. |
|
Closed via 59a29e1. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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]).
Updates in v5
Updates in v4
Updates in v3
Updates in v2
#define. Instead just use an anonymous struct namedinternal.infoout parameter inside parse_trailers(). Instead free it from the caller, process_trailers(). Update comment in parse_trailers().[1] https://lore.kernel.org/git/pull.1564.git.1691210737.gitgitgadget@gmail.com/T/#mb044012670663d8eb7a548924bbcc933bef116de
[2] https://lore.kernel.org/git/20230627195251.1973421-1-calvinwan@google.com/
[3] https://lore.kernel.org/git/pull.1149.git.1677143700.gitgitgadget@gmail.com/
[4] https://lore.kernel.org/git/6b4cb31b17077181a311ca87e82464a1e2ad67dd.1686797630.git.gitgitgadget@gmail.com/
[5] https://lore.kernel.org/git/pull.1563.git.1691211879.gitgitgadget@gmail.com/T/#m0131f9829c35d8e0103ffa88f07d8e0e43dd732c
cc: Glen Choo glencbz@gmail.com
cc: Christian Couder chriscool@tuxfamily.org
cc: Phillip Wood phillip.wood123@gmail.com
cc: Jonathan Tan jonathantanmy@google.com