* Re: [BUG] `git describe` doesn't traverse the graph in topological order
From: 'Ben Boeckel' @ 2023-09-22 17:43 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <8f034aa3-67ac-456c-a754-b0a86666bcdb@app.fastmail.com>
On Fri, Sep 22, 2023 at 19:35:01 +0200, Kristoffer Haugsbakk wrote:
> Looks related:
>
> Link: https://public-inbox.org/git/CABPp-BH2zuYe87xhjdp5v7M7i+EfEgLHAZgwfzJUAxGk1CFgfA@mail.gmail.com/
> Message-ID: CABPp-BH2zuYe87xhjdp5v7M7i+EfEgLHAZgwfzJUAxGk1CFgfA@mail.gmail.com
> Via: https://stackoverflow.com/questions/72886894/git-describe-is-not-returning-the-expected-tag
Thanks. It seems that these discussions previously determined the same
(painful) pill:
SZEDER Gábor at https://lore.kernel.org/git/20191008123156.GG11529@szeder.dev/:
I think the proper way to fix this issue would be to make 'git
describe' traverse the history in topographical order. Alas, I'm
afraid this would result in a noticable performance penalty on big
histories without a commit graph.
The `sleep 1` is probably the remedy we'll use if time to actually fix
this doesn't come up (or the "proper" fix is deemed as "too expensive").
Thanks for the links,
--Ben
^ permalink raw reply
* Re: [BUG] `git describe` doesn't traverse the graph in topological order
From: Junio C Hamano @ 2023-09-22 17:51 UTC (permalink / raw)
To: rsbecker; +Cc: 'Ben Boeckel', git
In-Reply-To: <02e701d9ed78$436b3c60$ca41b520$@nexbridge.com>
<rsbecker@nexbridge.com> writes:
> There appears to be a merge at 446120fd88 which brings v9.3.0.rc0 closer to HEAD than v9.3.0.rc1.
I didn't look at the actual graph but let me say I trust you ;-)
I wonder if there should be an obvious "explain why you gave this
name" mode added to the command, though. The command should be able
to say "The closest path from HEAD to any tag is via this, that, and
that commit, which is N hops to tag T0", and from there, the user
should be able to say "Oh, I thought T1 was closer, let me try again
to describe HEAD, limiting the candidate only to T1" and run the
command in that mode, which should be able to say "The closest path
from HEAD to any tag that is allowed as a candidate is via these
commits, which is M hops to tag T1". And if M is smaller than N,
then that may deserve to trigger a bug report (but as you said,
there are rules like preferring annotated over unannotated tags
involved, so it may not as straight-forward as comparing the two
integer hop counts).
Thanks for digging.
^ permalink raw reply
* Re: [PATCH] diff --stat: set the width defaults in a helper function
From: Dragan Simic @ 2023-09-22 17:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqjzsi2l7r.fsf@gitster.g>
On 2023-09-22 19:04, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>
>> Extract the commonly used initialization of the --stat-width=<width>,
>> --stat-name-width=<width> and --stat-graph-with=<width> parameters to
>> the
>> internal default values into a helper function, to avoid repeating the
>> same
>> initialization code in a few places.
>
> Thanks.
>
> If this is only about settings related to controlling widths of
> various elements on diffstat lines, isn't the "std" in name a bit
> too broad, though? init_diffstat_widths(&rev.diffopt) or something
> like that might be a better fit. I dunno if it is a huge deal,
> though.
Makes sense to me, init_diffstat_widths(&rev.diffopt) fits better and
it's more descriptive. I'm a big fan of using self-descriptive naming,
but originally I wanted to follow the already present naming scheme, so
I'm glad to see that you asked for a more descriptive function name.
>> Add a couple of tests to additionally cover existing configuration
>> options
>> diff.statNameWidth=<width> and diff.statGraphWidth=<width> when used
>> by
>> git-merge to generate --stat outputs. This closes the gap in the
>> tests that
>> existed previously for the --stat tests, reducing the chances for
>> having
>> any regressions introduced by this commit.
>
> Nice.
Thanks.
>> While there, perform a bunch of small wording improvements and some
>> minor
>> code cleanups in the improved unit test, as spotted, to make it a bit
>> neater
>> and to improve its test-level consistency.
>
> Alright. The last category of changes need somebody else to review
> them in addition to myself, as I expect that it would be somewhat
> subjective and I tend to be change-averse.
>
> The code changes all looked sensible.
>
>> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
>> index beb2ec2a55..aa947d93cf 100755
>> --- a/t/t4052-stat-output.sh
>> +++ b/t/t4052-stat-output.sh
>> @@ -12,32 +12,31 @@ TEST_PASSES_SANITIZE_LEAK=true
>> . ./test-lib.sh
>> . "$TEST_DIRECTORY"/lib-terminal.sh
>>
>> -# 120 character name
>> -name=aaaaaaaaaa
>> -name=$name$name$name$name$name$name$name$name$name$name$name$name
>> +# 120-character name
>> +printf -v name 'a%.0s' {1..120}
>
> This is a totally unnecessary and unacceptable change. "-v name"
> may be available in the built-in variant found in bash, but you
> would likely find that it is missing from other shells. {1..120} is
> also a bash-ism.
Makes sense to stick with the simpler variant, which won't cause
compatibility issues.
> And because we are still calling the result a "name" (not
> "filename") ...
>
>> cat >expect72 <<-'EOF'
>> ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1
>> +
>> EOF
>> -test_expect_success "format-patch: small change with long name gives
>> more space to the name" '
>> +test_expect_success "format-patch: small change with long filename
>> gives more space to the filename" '
>
> ... I do not see the point of this change (and similar ones in the
> rest of the patch). Even the configuration is called statNameWidth
> and not statFileNameWidth. In the context of the tests that check
> "stat-output" (that is in the filename of this script), we should be
> able to use "name" consistently without causing any confusion, as it
> is unlikely to be mistaken with other kinds of "name".
On second thought, I agree that sticking with just "name" is better in
this test.
>> -test_expect_success "format-patch --cover-letter ignores COLUMNS (big
>> change)" '
>> +test_expect_success "format-patch --cover-letter ignores COLUMNS
>> envvar with big change" '
>
> Not wrong per-se, but I wonder if it is necessary to stress that
> COLUMNS is an environment variable that tells the programs how wide
> a terminal they are showing their output. A usual shell variable
> would not affect the "git" process it runs, and COLUMNS without any
> dot in it cannot be our configuration variable, so even without deep
> knowledge of tradition, I thought it would be rather obvious.
On second thought, I agree that using just "COLUMN" suffices while also
being more compact.
> Same comment for "statNameWidth config"; with fewer number of bytes,
> it would be more descriptive to say "diff.statNameWidth".
Oh, nice catch! Makes sense.
>> - test_expect_success "$cmd --stat-graph-width with big change" '
>> + test_expect_success "$cmd --stat-graph-width=width with big change"
>> '
>> git $cmd $args --stat-graph-width=26 >output &&
>
> This may be a good change, especially if there are tests that feed
> different parameters and if it helps clarifying which variant is
> tested, e.g. "--stat=<width>,<name-width>" vs "--stat=<width>".
>
> Ah, wait, "--stat-graph-width" always takes a single value, so the
> above justification does not quite apply. But still, it is not
> making it worse, and because there is another test that is labeled
> with "--stat-width=width", being consistent with it has value.
Yes, I think that the improved consistency outweights the slight
redundancy.
> OK.
>
>> - test_expect_success "$cmd --stat-graph-width --graph with big
>> change" '
>> + test_expect_success "$cmd --stat-graph-width=width --graph with big
>> change" '
>
> Ditto.
>
> Thanks.
Thanks, I'll prepare and send v2 of the patch, based on your feedback.
^ permalink raw reply
* RE: [BUG] `git describe` doesn't traverse the graph in topological order
From: rsbecker @ 2023-09-22 18:12 UTC (permalink / raw)
To: 'Junio C Hamano'; +Cc: 'Ben Boeckel', git
In-Reply-To: <xmqqediq2j0g.fsf@gitster.g>
On Friday, September 22, 2023 1:52 PM, Junio C Hamano wrote:
><rsbecker@nexbridge.com> writes:
>
>> There appears to be a merge at 446120fd88 which brings v9.3.0.rc0 closer
to HEAD
>than v9.3.0.rc1.
>
>I didn't look at the actual graph but let me say I trust you ;-)
>
>I wonder if there should be an obvious "explain why you gave this name"
mode added
>to the command, though. The command should be able to say "The closest
path from
>HEAD to any tag is via this, that, and that commit, which is N hops to tag
T0", and
>from there, the user should be able to say "Oh, I thought T1 was closer,
let me try
>again to describe HEAD, limiting the candidate only to T1" and run the
command in
>that mode, which should be able to say "The closest path from HEAD to any
tag that
>is allowed as a candidate is via these commits, which is M hops to tag T1".
And if M
>is smaller than N, then that may deserve to trigger a bug report (but as
you said,
>there are rules like preferring annotated over unannotated tags involved,
so it may
>not as straight-forward as comparing the two integer hop counts).
>
>Thanks for digging.
I'm wondering whether we need something more general that --first-parent.
Perhaps something like
git describe commitish [ commitish ... ]
Where the traversal must cross the set of specified commitish points in
history in order to find the expected tag. In Ben's case, I do not think
that would help much, given the complexity of his history. Perhaps a
--verbose argument might display the analysis path done by git describe as
above. Sadly, I am not familiar with this code area.
What confuses me is how, in the other subthread, that adding sleep 1 to the
construction of history should make any difference. My understanding is that
the path to the tag is invariant of the commit-date.
^ permalink raw reply
* Re: [PATCH v2 5/6] trailer: rename *_DEFAULT enums to *_UNSPECIFIED
From: Linus Arver @ 2023-09-22 18:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Arver via GitGitGadget, git, Christian Couder, Phillip Wood
In-Reply-To: <xmqq1qf1la0q.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>>> It gets tempting to initialize a variable to the default and arrange
>>> the rest of the system so that the variable set to the default
>>> triggers the default activity. Such an obvious solution however
>>> cannot be used when (1) being left unspecified to use the default
>>> value and (2) explicitly set by the user to a value that happens to
>>> be the same as the default have to behave differently. I am not
>>> sure if that applies to the trailers system, though.
>>>
>>> Thanks.
>>
>> I get the feeling that you wrote the "Such an obvious ... differently"
>> sentence after writing the last sentence in that paragraph, because when
>> you say
>>
>> I am not
>> sure if that applies to the trailers system, though.
>>
>> I read the "that" (emphasis added) in there as referring to the solution
>> described in the first sentence, and not the conditions (1) and (2) you
>> enumerated. IOW, you are OK with this patch.
>
> "that" refers to "the reason not to use such an obvious solution".
> I do not know if trailer subsystem wants to treat "left unspecified"
> and "set to the value that happens to be the same as the default" in
> a different way. If it does want to do so, then I do not see a
> strong reason not to use the "obvious solution".
Currently we set the defaults (these take effect absent any
configuration or CLI options) in trailer.c like this:
static void ensure_configured(void)
{
if (configured)
return;
/* Default config must be setup first */
default_conf_info.where = WHERE_END;
default_conf_info.if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
default_conf_info.if_missing = MISSING_ADD;
git_config(git_trailer_default_config, NULL);
git_config(git_trailer_config, NULL);
configured = 1;
}
So technically we already sort of do the "obvious solution". And then
these get overriden by configuration (if any), and finally any CLI
options that are passed in (e.g., "--where after"). The reason why I
prefer the *_UNSPECIFIED style in this patch for these enums though is
because of this (and other similar functions) in trailer.c:
int trailer_set_where(enum trailer_where *item, const char *value)
{
if (!value)
*item = WHERE_DEFAULT;
else if (!strcasecmp("after", value))
*item = WHERE_AFTER;
else if (!strcasecmp("before", value))
*item = WHERE_BEFORE;
else if (!strcasecmp("end", value))
*item = WHERE_END;
else if (!strcasecmp("start", value))
*item = WHERE_START;
else
return -1;
return 0;
}
and this function is used as a callback to the "--where" flag, such that
the WHERE_DEFAULT gets chosen if "--no-where" is where. I prefer the
WHERE_UNSPECIFIED as in this patch because the WHERE_DEFAULT is
ambiguous on its own (i.e., WHERE_DEFAULT could mean that we either use
the default value WHERE_END in default_conf_info, or it could mean that
we fall back to the configuration variables (where it could be something
else)).
^ permalink raw reply
* Re: [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both
From: Linus Arver @ 2023-09-22 18:26 UTC (permalink / raw)
To: Junio C Hamano, Jonathan Tan
Cc: Linus Arver via GitGitGadget, git, Christian Couder
In-Reply-To: <xmqq8r90dh2v.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>> @@ -248,34 +258,40 @@ With `add`, a new trailer will be added.
>>> +
>>> With `doNothing`, nothing will be done.
>>>
>>> -trailer.<token>.key::
>>> - This `key` will be used instead of <token> in the trailer. At
>>> - the end of this key, a separator can appear and then some
>>> - space characters. By default the only valid separator is ':',
>>> - but this can be changed using the `trailer.separators` config
>>> - variable.
>>> +trailer.<keyAlias>.key::
>>> + Defines a <keyAlias> for the <key>. The <keyAlias> must be a
>>> + prefix (case does not matter) of the <key>. For example, in `git
>>> + config trailer.ack.key "Acked-by"` the "Acked-by" is the <key> and
>>> + the "ack" is the <keyAlias>. This configuration allows the shorter
>>> + `--trailer "ack:..."` invocation on the command line using the "ack"
>>> + <keyAlias> instead of the longer `--trailer "Acked-by:..."`.
>>> ++
>>> +At the end of the <key>, a separator can appear and then some
>>> +space characters. By default the only valid separator is ':',
>>> +but this can be changed using the `trailer.separators` config
>>> +variable.
>>
>> I think all the other patches will be a great help to the user, but I'm
>> on the fence about this one. Someone who knows these trailer components
>> by their old names might be confused upon seeing tne new ones, so I'm
>> inclined to minimize such changes. I do think that the new names make
>> more sense, though.
>
> As long as the new names describe the world order better than the
> old description, I do not mind rephrasing the documentation, and you
> seem to find the more descriptive <keyAlias> easier to understand
> compared to the non-descriptive <token>. Adding a concrete example
> (ack vs acked-by) is also a good change.
SGTM (i.e., I am OK to keep this patch as is). So I will not re-roll.
In the future I will be cleaning up more of the trailer system to make
use of the disinction between the key vs keyAlias where appropriate.
Thanks!
^ permalink raw reply
* Re: [PATCH] completion: loosen and document the requirement around completing alias
From: Linus Arver @ 2023-09-22 18:31 UTC (permalink / raw)
To: Junio C Hamano, Philippe Blain; +Cc: git
In-Reply-To: <xmqqr0ms8yxs.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> * We've discussed this when we reviewed the topic that just hit
>> 'master'. Before we forget...
>>
>> contrib/completion/git-completion.bash | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> And here is an obligatory "test" update I will squash into the main
> patch. The new ones copy a canonical ": git <cmd> ; ..." test and
> remove 'git' and also the space before the semicolon.
>
> diff --git c/t/t9902-completion.sh w/t/t9902-completion.sh
> index 47e20fb8b1..a7c3b4eb63 100755
> --- c/t/t9902-completion.sh
> +++ w/t/t9902-completion.sh
> @@ -2464,6 +2464,24 @@ test_expect_success 'completion used <cmd> completion for alias: !f() { : git <c
> EOF
> '
>
> +test_expect_success 'completion used <cmd> completion for alias: !f() { : <cmd> ; ... }' '
> + test_config alias.co "!f() { : checkout ; if ... } f" &&
> + test_completion "git co m" <<-\EOF
> + main Z
> + mybranch Z
> + mytag Z
> + EOF
> +'
> +
> +test_expect_success 'completion used <cmd> completion for alias: !f() { : <cmd>; ... }' '
> + test_config alias.co "!f() { : checkout; if ... } f" &&
> + test_completion "git co m" <<-\EOF
> + main Z
> + mybranch Z
> + mytag Z
> + EOF
> +'
> +
> test_expect_success 'completion without explicit _git_xxx function' '
> test_completion "git version --" <<-\EOF
> --build-options Z
LGTM, thanks!
^ permalink raw reply
* Re: [BUG] `git describe` doesn't traverse the graph in topological order
From: 'Ben Boeckel' @ 2023-09-22 18:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: rsbecker, git
In-Reply-To: <xmqqediq2j0g.fsf@gitster.g>
On Fri, Sep 22, 2023 at 10:51:59 -0700, Junio C Hamano wrote:
> <rsbecker@nexbridge.com> writes:
>
> > There appears to be a merge at 446120fd88 which brings v9.3.0.rc0 closer to HEAD than v9.3.0.rc1.
>
> I didn't look at the actual graph but let me say I trust you ;-)
>
> I wonder if there should be an obvious "explain why you gave this
> name" mode added to the command, though. The command should be able
> to say "The closest path from HEAD to any tag is via this, that, and
> that commit, which is N hops to tag T0", and from there, the user
> should be able to say "Oh, I thought T1 was closer, let me try again
> to describe HEAD, limiting the candidate only to T1" and run the
> command in that mode, which should be able to say "The closest path
> from HEAD to any tag that is allowed as a candidate is via these
> commits, which is M hops to tag T1". And if M is smaller than N,
> then that may deserve to trigger a bug report (but as you said,
> there are rules like preferring annotated over unannotated tags
> involved, so it may not as straight-forward as comparing the two
> integer hop counts).
The thing is that the count is what is wrong here, so the determination
of what is "closer" is wrong. Any explanation would say things like
"commit X~10 is not part of X".
--Ben
^ permalink raw reply
* Re: [BUG] `git describe` doesn't traverse the graph in topological order
From: 'Ben Boeckel' @ 2023-09-22 18:44 UTC (permalink / raw)
To: rsbecker; +Cc: 'Junio C Hamano', git
In-Reply-To: <032d01d9ed80$5e569670$1b03c350$@nexbridge.com>
On Fri, Sep 22, 2023 at 14:12:31 -0400, rsbecker@nexbridge.com wrote:
> What confuses me is how, in the other subthread, that adding sleep 1 to the
> construction of history should make any difference. My understanding is that
> the path to the tag is invariant of the commit-date.
Yes. It is explained that the commit date stored is only to 1 second
granularity. Since the commits are stored in commit-date, an equal
commit date ends up "twisting" the history and traversing some ancestors
of commits before the commits themsevles. This loses the "seen" bit
tracking that is done and ends up labeling way more commits as "not part
of" ancestors. By sleeping for a second, the commit dates can be totally
ordered reliably.
And this tracks with my and the other thread's result that the traversal
is not paying attention to the topological history properly.
--Ben
^ permalink raw reply
* RE: [BUG] `git describe` doesn't traverse the graph in topological order
From: rsbecker @ 2023-09-22 18:49 UTC (permalink / raw)
To: 'Ben Boeckel'; +Cc: 'Junio C Hamano', git
In-Reply-To: <ZQ3ggxA7KOysXrba@farprobe>
On Friday, September 22, 2023 2:44 PM, Ben Boeckel wrote:
>On Fri, Sep 22, 2023 at 14:12:31 -0400, rsbecker@nexbridge.com wrote:
>> What confuses me is how, in the other subthread, that adding sleep 1
>> to the construction of history should make any difference. My
>> understanding is that the path to the tag is invariant of the commit-date.
>
>Yes. It is explained that the commit date stored is only to 1 second granularity. Since
>the commits are stored in commit-date, an equal commit date ends up "twisting" the
>history and traversing some ancestors of commits before the commits themsevles.
>This loses the "seen" bit tracking that is done and ends up labeling way more
>commits as "not part of" ancestors. By sleeping for a second, the commit dates can
>be totally ordered reliably.
This is going to be awkward to resolve as time_t only resolves (portably) to 1 second intervals. I still would prefer the resolution to be path-based rather than time-based.
^ permalink raw reply
* Re: [BUG] `git describe` doesn't traverse the graph in topological order
From: 'Ben Boeckel' @ 2023-09-22 19:05 UTC (permalink / raw)
To: rsbecker; +Cc: 'Junio C Hamano', git
In-Reply-To: <033201d9ed85$991c6af0$cb5540d0$@nexbridge.com>
On Fri, Sep 22, 2023 at 14:49:58 -0400, rsbecker@nexbridge.com wrote:
> On Friday, September 22, 2023 2:44 PM, Ben Boeckel wrote:
> >Yes. It is explained that the commit date stored is only to 1 second granularity. Since
> >the commits are stored in commit-date, an equal commit date ends up "twisting" the
> >history and traversing some ancestors of commits before the commits themsevles.
> >This loses the "seen" bit tracking that is done and ends up labeling way more
> >commits as "not part of" ancestors. By sleeping for a second, the commit dates can
> >be totally ordered reliably.
>
> This is going to be awkward to resolve as time_t only resolves
> (portably) to 1 second intervals. I still would prefer the resolution
> to be path-based rather than time-based.
I certainly agree, but I'm not sure of the best way of doing that. Do we
create/load a commit graph and use that for resolving insertion order
into the commit heap?
--Ben
^ permalink raw reply
* RE: [BUG] `git describe` doesn't traverse the graph in topological order
From: rsbecker @ 2023-09-22 19:27 UTC (permalink / raw)
To: 'Ben Boeckel'; +Cc: 'Junio C Hamano', git
In-Reply-To: <ZQ3leoLhljc+P5wP@farprobe>
On Friday, September 22, 2023 3:06 PM, Ben Boeckel wrote:
>On Fri, Sep 22, 2023 at 14:49:58 -0400, rsbecker@nexbridge.com wrote:
>> On Friday, September 22, 2023 2:44 PM, Ben Boeckel wrote:
>> >Yes. It is explained that the commit date stored is only to 1 second
>> >granularity. Since the commits are stored in commit-date, an equal
>> >commit date ends up "twisting" the history and traversing some ancestors of
>commits before the commits themsevles.
>> >This loses the "seen" bit tracking that is done and ends up labeling
>> >way more commits as "not part of" ancestors. By sleeping for a
>> >second, the commit dates can be totally ordered reliably.
>>
>> This is going to be awkward to resolve as time_t only resolves
>> (portably) to 1 second intervals. I still would prefer the resolution
>> to be path-based rather than time-based.
>
>I certainly agree, but I'm not sure of the best way of doing that. Do we create/load a
>commit graph and use that for resolving insertion order into the commit heap?
I actually thought it worked that way. This may end up in a bigger change than fixing the issue because --first-parent does not appear to be sufficient to resolve the correct tag from your graph. My thought on using multiple commitish values to do that may help, but implementing that could lead to an O(n*m) scan (n=max commit tree width, m=depth to tag), plus a commitish hash lookup.
^ permalink raw reply
* Re: [PATCH v2 5/6] trailer: rename *_DEFAULT enums to *_UNSPECIFIED
From: Junio C Hamano @ 2023-09-22 19:48 UTC (permalink / raw)
To: Linus Arver
Cc: Linus Arver via GitGitGadget, git, Christian Couder, Phillip Wood
In-Reply-To: <owly8r8yt6cr.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> ... I prefer the
> WHERE_UNSPECIFIED as in this patch because the WHERE_DEFAULT is
> ambiguous on its own (i.e., WHERE_DEFAULT could mean that we either use
> the default value WHERE_END in default_conf_info, or it could mean that
> we fall back to the configuration variables (where it could be something
> else)).
Yup. "Turning something that is left UNSPECIFIED after command line
options and configuration files are processed into the hardcoded
DEFAULT" is one mental model that is easy to explain.
I however am not sure if it is easier than "Setting something to
hardcoded DEFAULT before command line options and configuration
files are allowed to tweak it, and if nobody touches it, then it
gets the hardcoded DEFAULT value in the end", which is another valid
mental model, though. If both can be used, I'd personally prefer
the latter, and reserve the "UNSPECIFIED to DEFAULT" pattern to
signal that we are dealing with a case where the simpler pattern
without UNSPECIFIED cannot solve.
The simpler pattern would not work, when the default is defined
depending on a larger context. Imagine we have two Boolean
variables, A and B, where A defaults to false, and B defaults to
some value derived from the value of A (say, opposite of A).
In the most natural implementation, you'd initialize A to false and
B to unspecified, let command line options and configuration
variables to set them to true or false, and after all that, you do
not have to tweak A's value (it will be left to false that is the
default unless the user or the configuration gave an explicit
value), but you need to check if B is left unspecified and tweak it
to true or false using the final value of A.
For a variable with such a need like B, we cannot avoid having
"unspecified". If you initialize it to false (or true), after the
command line and the configuration files are read and you find B is
set to false (or true), you cannot tell if the user or the
configuration explicitly set B to false (or true), in which case you
do not want to futz with its value based on what is in A, or it is
false (or true) only because nobody touched it, in which case you
need to compute its value based on what is in A.
And that is why I asked if we need to special case "the user did not
touch and the variable is left untouched" in the trailer subsystem.
Thanks.
^ permalink raw reply
* Re: [PATCH] completion: loosen and document the requirement around completing alias
From: Junio C Hamano @ 2023-09-22 19:49 UTC (permalink / raw)
To: Linus Arver; +Cc: Philippe Blain, git
In-Reply-To: <owly34z6t5zg.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> * We've discussed this when we reviewed the topic that just hit
>>> 'master'. Before we forget...
> ...
> LGTM, thanks!
Thanks for reading.
^ permalink raw reply
* [PATCH v3 1/9] trailer: separate public from internal portion of trailer_iterator
From: Linus Arver via GitGitGadget @ 2023-09-22 19:50 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Christian Couder, Phillip Wood, Linus Arver,
Linus Arver
In-Reply-To: <pull.1563.v3.git.1695412245.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
The fields here are not meant to be used by downstream callers, so put
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).
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 10 +++++-----
trailer.h | 6 ++++--
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/trailer.c b/trailer.c
index f408f9b058d..de4bdece847 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1220,14 +1220,14 @@ 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->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->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)
@@ -1245,7 +1245,7 @@ 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->internal.info);
strbuf_release(&iter->val);
strbuf_release(&iter->key);
}
diff --git a/trailer.h b/trailer.h
index 795d2fccfd9..ab2cd017567 100644
--- 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;
+ } internal;
};
/*
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 2/9] trailer: split process_input_file into separate pieces
From: Linus Arver via GitGitGadget @ 2023-09-22 19:50 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Christian Couder, Phillip Wood, Linus Arver,
Linus Arver
In-Reply-To: <pull.1563.v3.git.1695412245.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
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). 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).
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 42 ++++++++++++++++++++++--------------------
1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/trailer.c b/trailer.c
index de4bdece847..2c56cbc4a2e 100644
--- a/trailer.c
+++ b/trailer.c
@@ -961,28 +961,24 @@ 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", populating the trailer info and "head"
+ * linked list structure.
+ */
+static void parse_trailers(struct trailer_info *info,
+ 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);
+ trailer_info_get(info, str, opts);
- if (!opts->only_trailers && !info.blank_line_before_trailer)
- fprintf(outfile, "\n");
-
- for (i = 0; i < info.trailer_nr; i++) {
+ for (i = 0; i < info->trailer_nr; i++) {
int separator_pos;
- char *trailer = info.trailers[i];
+ char *trailer = info->trailers[i];
if (trailer[0] == comment_line_char)
continue;
separator_pos = find_separator(trailer, separators);
@@ -1002,10 +998,6 @@ static size_t process_input_file(FILE *outfile,
strbuf_detach(&val, NULL));
}
}
-
- trailer_info_release(&info);
-
- return info.trailer_end - str;
}
static void free_all(struct list_head *head)
@@ -1054,6 +1046,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 +1057,16 @@ void process_trailers(const char *file,
if (opts->in_place)
outfile = create_in_place_tempfile(file);
+ 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);
+
+ if (!opts->only_trailers && !info.blank_line_before_trailer)
+ fprintf(outfile, "\n");
+
if (!opts->only_input) {
LIST_HEAD(arg_head);
@@ -1076,6 +1077,7 @@ 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)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 0/9] Trailer readability cleanups
From: Linus Arver via GitGitGadget @ 2023-09-22 19:50 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Christian Couder, Phillip Wood, Linus Arver
In-Reply-To: <pull.1563.v2.git.1694240177.gitgitgadget@gmail.com>
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 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.
Patches 5-8 clean up the area around handling the trailer block start and
end of the input. In particular we rename find_patch_start() to
find_end_of_log_message(). These patches address the new approach I cited in
[5].
Updates in v3
=============
* Patches 4 and 6 (--no-divider and trailer block start/end cleanups) have
been reorganized to Patches 5-8. This ended up touching commit.c in a
minor way, but otherwise all of the changes here are cleanups and do not
change any behavior.
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/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
Linus Arver (9):
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: rename *_DEFAULT enums to *_UNSPECIFIED
commit: ignore_non_trailer computes number of bytes to ignore
trailer: find the end of the log message
trailer: use offsets for trailer_start/trailer_end
trailer: only use trailer_block_* variables if trailers were found
trailer: make stack variable names match field names
builtin/commit.c | 2 +-
builtin/merge.c | 2 +-
commit.c | 2 +-
commit.h | 4 +-
sequencer.c | 2 +-
trailer.c | 220 ++++++++++++++++++++++++++++-------------------
trailer.h | 27 +++---
7 files changed, 154 insertions(+), 105 deletions(-)
base-commit: 1b0a5129563ebe720330fdc8f5c6843d27641137
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1563%2Flistx%2Ftrailer-libification-prep-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1563/listx/trailer-libification-prep-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1563
Range-diff vs v2:
1: 4f116d2550f = 1: 4f116d2550f trailer: separate public from internal portion of trailer_iterator
2: c00f4623d0b = 2: c00f4623d0b trailer: split process_input_file into separate pieces
3: f78c2345fad = 3: f78c2345fad trailer: split process_command_line_args into separate functions
5: 52958c3557c = 4: 47186a09b24 trailer: rename *_DEFAULT enums to *_UNSPECIFIED
-: ----------- > 5: da52cec42e1 commit: ignore_non_trailer computes number of bytes to ignore
4: f5f507c4c6c ! 6: ab8a6ced143 trailer: teach find_patch_start about --no-divider
@@ Metadata
Author: Linus Arver <linusa@google.com>
## Commit message ##
- trailer: teach find_patch_start about --no-divider
+ trailer: find the end of the log message
- Currently, find_patch_start only finds the start of the patch part of
- the input (by looking at the "---" divider) for cases where the
- "--no-divider" flag has not been provided. If the user provides this
- flag, we do not rely on find_patch_start at all and just call strlen()
- directly on the input.
+ Previously, trailer_info_get() computed the trailer block end position
+ by
- 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. 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).
+ (1) checking for the opts->no_divider flag and optionally calling
+ find_patch_start() to find the "patch start" location (patch_start), and
+ (2) calling find_trailer_end() to find the end of the trailer block
+ using patch_start as a guide, saving the return value into
+ "trailer_end".
- 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).
+ The logic in (1) was awkward because the variable "patch_start" is
+ misleading if there is no patch in the input. The logic in (2) was
+ misleading because it could be the case that no trailers are in the
+ input (yet we are setting a "trailer_end" variable before even searching
+ for trailers, which happens later in find_trailer_start()). The name
+ "find_trailer_end" was misleading because that function did not look for
+ any trailer block itself --- instead it just computed the end position
+ of the log message in the input where the end of the trailer block (if
+ it exists) would be (because trailer blocks must always come after the
+ end of the log message).
+ Combine the logic in (1) and (2) together into find_patch_start() by
+ renaming it to find_end_of_log_message(). The end of the log message is
+ the starting point which find_trailer_start() needs to start searching
+ backward to parse individual trailers (if any).
+
+ Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Arver <linusa@google.com>
## trailer.c ##
@@ trailer.c: 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.
+ }
+
+ /*
+- * Return the position of the start of the patch or the length of str if there
+- * is no patch in the message.
++ * Find the end of the log message as an offset from the start of the input
++ * (where callers of this function are interested in looking for a trailers
++ * block in the same input). We have to consider two categories of content that
++ * can come at the end of the input which we want to ignore (because they don't
++ * belong in the log message):
++ *
++ * (1) the "patch part" which begins with a "---" divider and has patch
++ * information (like the output of git-format-patch), and
++ *
++ * (2) any trailing comment lines, blank lines like in the output of "git
++ * commit -v", or stuff below the "cut" (scissor) line.
++ *
++ * As a formula, the situation looks like this:
++ *
++ * INPUT = LOG MESSAGE + IGNORED
++ *
++ * where IGNORED can be either of the two categories described above. It may be
++ * that there is nothing to ignore. Now it may be the case that the LOG MESSAGE
++ * contains a trailer block, but that's not the concern of this function.
*/
-static size_t find_patch_start(const char *str)
-+static size_t find_patch_start(const char *str, int no_divider)
++static size_t find_end_of_log_message(const char *input, int no_divider)
{
++ size_t end;
++
const char *s;
- for (s = str; *s; s = next_line(s)) {
+- for (s = str; *s; s = next_line(s)) {
++ /* Assume the naive end of the input is already what we want. */
++ end = strlen(input);
++
++ /* Optionally skip over any patch part ("---" line and below). */
++ for (s = input; *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;
+- return s - str;
++ if (!no_divider && skip_prefix(s, "---", &v) && isspace(*v)) {
++ end = s - input;
++ break;
++ }
}
+- return s - str;
++ /* Skip over other ignorable bits. */
++ return end - ignored_log_message_bytes(input, end);
+ }
+
+ /*
+@@ trailer.c: continue_outer_loop:
+ return len;
+ }
+
+-/* Return the position of the end of the trailers. */
+-static size_t find_trailer_end(const char *buf, size_t len)
+-{
+- return len - ignored_log_message_bytes(buf, len);
+-}
+-
+ static int ends_with_blank_line(const char *buf, size_t len)
+ {
+ ssize_t ll = last_line(buf, len);
+@@ trailer.c: void process_trailers(const char *file,
+ void trailer_info_get(struct trailer_info *info, const char *str,
+ const struct process_trailer_options *opts)
+ {
+- int patch_start, trailer_end, trailer_start;
++ int end_of_log_message, trailer_start;
+ struct strbuf **trailer_lines, **ptr;
+ char **trailer_strings = NULL;
+ size_t nr = 0, alloc = 0;
@@ trailer.c: void trailer_info_get(struct trailer_info *info, const char *str,
ensure_configured();
@@ trailer.c: void trailer_info_get(struct trailer_info *info, const char *str,
- else
- patch_start = find_patch_start(str);
-
-+ patch_start = find_patch_start(str, opts->no_divider);
- trailer_end = find_trailer_end(str, patch_start);
- trailer_start = find_trailer_start(str, trailer_end);
+- trailer_end = find_trailer_end(str, patch_start);
+- trailer_start = find_trailer_start(str, trailer_end);
++ end_of_log_message = find_end_of_log_message(str, opts->no_divider);
++ trailer_start = find_trailer_start(str, end_of_log_message);
+ trailer_lines = strbuf_split_buf(str + trailer_start,
+- trailer_end - trailer_start,
++ end_of_log_message - trailer_start,
+ '\n',
+ 0);
+ for (ptr = trailer_lines; *ptr; ptr++) {
+@@ trailer.c: void trailer_info_get(struct trailer_info *info, const char *str,
+ info->blank_line_before_trailer = ends_with_blank_line(str,
+ trailer_start);
+ info->trailer_start = str + trailer_start;
+- info->trailer_end = str + trailer_end;
++ info->trailer_end = str + end_of_log_message;
+ info->trailers = trailer_strings;
+ info->trailer_nr = nr;
+ }
6: 0463066ebe0 ! 7: 091805eb7d9 trailer: use offsets for trailer_start/trailer_end
@@ Commit message
reference the input string in format_trailer_info(), so update that
function to take a pointer to the input.
+ While we're at it, rename trailer_start to trailer_block_start to be
+ more explicit about these offsets (that they are for the entire trailer
+ block including other trailers). Ditto for trailer_end.
+
Signed-off-by: Linus Arver <linusa@google.com>
+ ## sequencer.c ##
+@@ sequencer.c: static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
+ if (ignore_footer)
+ sb->buf[sb->len - ignore_footer] = saved_char;
+
+- if (info.trailer_start == info.trailer_end)
++ if (info.trailer_block_start == info.trailer_block_end)
+ return 0;
+
+ for (i = 0; i < info.trailer_nr; i++)
+
## trailer.c ##
+@@ trailer.c: static size_t find_end_of_log_message(const char *input, int no_divider)
+ * Return the position of the first trailer line or len if there are no
+ * trailers.
+ */
+-static size_t find_trailer_start(const char *buf, size_t len)
++static size_t find_trailer_block_start(const char *buf, size_t len)
+ {
+ const char *s;
+ ssize_t end_of_title, l;
@@ trailer.c: void process_trailers(const char *file,
LIST_HEAD(head);
struct strbuf sb = STRBUF_INIT;
@@ trailer.c: void process_trailers(const char *file,
/* Print the lines before the trailers */
if (!opts->only_trailers)
- fwrite(sb.buf, 1, info.trailer_start - sb.buf, outfile);
-+ fwrite(sb.buf, 1, info.trailer_start, outfile);
++ fwrite(sb.buf, 1, info.trailer_block_start, outfile);
if (!opts->only_trailers && !info.blank_line_before_trailer)
fprintf(outfile, "\n");
@@ trailer.c: void process_trailers(const char *file,
/* Print the lines after the trailers as is */
if (!opts->only_trailers)
- fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
-+ fwrite(sb.buf + info.trailer_end, 1, sb.len - info.trailer_end, outfile);
++ fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
if (opts->in_place)
if (rename_tempfile(&trailers_tempfile, file))
@@ trailer.c: void process_trailers(const char *file,
void trailer_info_get(struct trailer_info *info, const char *str,
const struct process_trailer_options *opts)
{
-- int patch_start, trailer_end, trailer_start;
-+ size_t patch_start, trailer_end = 0, trailer_start = 0;
+- int end_of_log_message, trailer_start;
++ size_t end_of_log_message = 0, trailer_block_start = 0;
struct strbuf **trailer_lines, **ptr;
char **trailer_strings = NULL;
size_t nr = 0, alloc = 0;
@@ trailer.c: void trailer_info_get(struct trailer_info *info, const char *str,
+ ensure_configured();
+
+ end_of_log_message = find_end_of_log_message(str, opts->no_divider);
+- trailer_start = find_trailer_start(str, end_of_log_message);
++ trailer_block_start = find_trailer_block_start(str, end_of_log_message);
+
+- trailer_lines = strbuf_split_buf(str + trailer_start,
+- end_of_log_message - trailer_start,
++ trailer_lines = strbuf_split_buf(str + trailer_block_start,
++ end_of_log_message - trailer_block_start,
+ '\n',
+ 0);
+ for (ptr = trailer_lines; *ptr; ptr++) {
+@@ trailer.c: void trailer_info_get(struct trailer_info *info, const char *str,
+ strbuf_list_free(trailer_lines);
info->blank_line_before_trailer = ends_with_blank_line(str,
- trailer_start);
+- trailer_start);
- info->trailer_start = str + trailer_start;
-- info->trailer_end = str + trailer_end;
-+ info->trailer_start = trailer_start;
-+ info->trailer_end = trailer_end;
+- info->trailer_end = str + end_of_log_message;
++ trailer_block_start);
++ info->trailer_block_start = trailer_block_start;
++ info->trailer_block_end = end_of_log_message;
info->trailers = trailer_strings;
info->trailer_nr = nr;
}
@@ trailer.c: static void format_trailer_info(struct strbuf *out,
!opts->separator && !opts->key_only && !opts->value_only &&
!opts->key_value_separator) {
- strbuf_add(out, info->trailer_start,
-+ strbuf_add(out, msg + info->trailer_start,
- info->trailer_end - info->trailer_start);
+- info->trailer_end - info->trailer_start);
++ strbuf_add(out, msg + info->trailer_block_start,
++ info->trailer_block_end - info->trailer_block_start);
return;
}
+
@@ trailer.c: void format_trailers_from_commit(struct strbuf *out, const char *msg,
struct trailer_info info;
@@ trailer.c: void format_trailers_from_commit(struct strbuf *out, const char *msg,
## trailer.h ##
-@@ trailer.h: struct trailer_info {
+@@ trailer.h: int trailer_set_if_missing(enum trailer_if_missing *item, const char *value);
+ struct trailer_info {
+ /*
+ * True if there is a blank line before the location pointed to by
+- * trailer_start.
++ * trailer_block_start.
+ */
int blank_line_before_trailer;
/*
@@ trailer.h: struct trailer_info {
- * is no trailer block found, these 2 pointers point to the end of the
- * input string.
+ * Offsets to the trailer block start and end positions in the input
-+ * string. If no trailer block is found, these are set to 0.
++ * string. If no trailer block is found, these are both set to the
++ * "true" end of the input, per find_true_end_of_input().
++ *
++ * NOTE: This will be changed so that these point to 0 in the next
++ * patch if no trailers are found.
*/
- const char *trailer_start, *trailer_end;
-+ size_t trailer_start, trailer_end;
++ size_t trailer_block_start, trailer_block_end;
/*
* Array of trailers found.
-: ----------- > 8: 1762f78a613 trailer: only use trailer_block_* variables if trailers were found
-: ----------- > 9: a784c45ed71 trailer: make stack variable names match field names
--
gitgitgadget
^ permalink raw reply
* [PATCH v3 3/9] trailer: split process_command_line_args into separate functions
From: Linus Arver via GitGitGadget @ 2023-09-22 19:50 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Christian Couder, Phillip Wood, Linus Arver,
Linus Arver
In-Reply-To: <pull.1563.v3.git.1695412245.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Previously, process_command_line_args did two things:
(1) parse trailers from the configuration, and
(2) parse trailers defined on the command line.
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 <linusa@google.com>
---
trailer.c | 34 +++++++++++++++++++++-------------
1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/trailer.c b/trailer.c
index 2c56cbc4a2e..b6de5d9cb2d 100644
--- a/trailer.c
+++ b/trailer.c
@@ -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) {
@@ -1069,8 +1074,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);
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 4/9] trailer: rename *_DEFAULT enums to *_UNSPECIFIED
From: Linus Arver via GitGitGadget @ 2023-09-22 19:50 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Christian Couder, Phillip Wood, Linus Arver,
Linus Arver
In-Reply-To: <pull.1563.v3.git.1695412245.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Do not use *_DEFAULT as a suffix to the enums, because the word
"default" is overloaded. The following are two examples of the ambiguity
of the word "default":
(1) "Default" can mean using the "default" values that are hardcoded
in trailer.c as
default_conf_info.where = WHERE_END;
default_conf_info.if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
default_conf_info.if_missing = MISSING_ADD;
in ensure_configured(). These values are referred to as "the
default" in the docs for interpret-trailers. These defaults are used
if no "trailer.*" configurations are defined.
(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. 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
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).
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 17 ++++++++++-------
trailer.h | 6 +++---
2 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/trailer.c b/trailer.c
index b6de5d9cb2d..0b66effceb5 100644
--- a/trailer.c
+++ b/trailer.c
@@ -388,7 +388,7 @@ static void process_trailers_lists(struct list_head *head,
int trailer_set_where(enum trailer_where *item, const char *value)
{
if (!value)
- *item = WHERE_DEFAULT;
+ *item = WHERE_UNSPECIFIED;
else if (!strcasecmp("after", value))
*item = WHERE_AFTER;
else if (!strcasecmp("before", value))
@@ -405,7 +405,7 @@ int trailer_set_where(enum trailer_where *item, const char *value)
int trailer_set_if_exists(enum trailer_if_exists *item, const char *value)
{
if (!value)
- *item = EXISTS_DEFAULT;
+ *item = EXISTS_UNSPECIFIED;
else if (!strcasecmp("addIfDifferent", value))
*item = EXISTS_ADD_IF_DIFFERENT;
else if (!strcasecmp("addIfDifferentNeighbor", value))
@@ -424,7 +424,7 @@ int trailer_set_if_exists(enum trailer_if_exists *item, const char *value)
int trailer_set_if_missing(enum trailer_if_missing *item, const char *value)
{
if (!value)
- *item = MISSING_DEFAULT;
+ *item = MISSING_UNSPECIFIED;
else if (!strcasecmp("doNothing", value))
*item = MISSING_DO_NOTHING;
else if (!strcasecmp("add", value))
@@ -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;
@@ -701,11 +704,11 @@ static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
new_item->value = val;
duplicate_conf(&new_item->conf, conf);
if (new_trailer_item) {
- if (new_trailer_item->where != WHERE_DEFAULT)
+ if (new_trailer_item->where != WHERE_UNSPECIFIED)
new_item->conf.where = new_trailer_item->where;
- if (new_trailer_item->if_exists != EXISTS_DEFAULT)
+ if (new_trailer_item->if_exists != EXISTS_UNSPECIFIED)
new_item->conf.if_exists = new_trailer_item->if_exists;
- if (new_trailer_item->if_missing != MISSING_DEFAULT)
+ if (new_trailer_item->if_missing != MISSING_UNSPECIFIED)
new_item->conf.if_missing = new_trailer_item->if_missing;
}
list_add_tail(&new_item->list, arg_head);
diff --git a/trailer.h b/trailer.h
index ab2cd017567..a689d768c79 100644
--- a/trailer.h
+++ b/trailer.h
@@ -5,14 +5,14 @@
#include "strbuf.h"
enum trailer_where {
- WHERE_DEFAULT,
+ WHERE_UNSPECIFIED,
WHERE_END,
WHERE_AFTER,
WHERE_BEFORE,
WHERE_START
};
enum trailer_if_exists {
- EXISTS_DEFAULT,
+ EXISTS_UNSPECIFIED,
EXISTS_ADD_IF_DIFFERENT_NEIGHBOR,
EXISTS_ADD_IF_DIFFERENT,
EXISTS_ADD,
@@ -20,7 +20,7 @@ enum trailer_if_exists {
EXISTS_DO_NOTHING
};
enum trailer_if_missing {
- MISSING_DEFAULT,
+ MISSING_UNSPECIFIED,
MISSING_ADD,
MISSING_DO_NOTHING
};
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 5/9] commit: ignore_non_trailer computes number of bytes to ignore
From: Linus Arver via GitGitGadget @ 2023-09-22 19:50 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Christian Couder, Phillip Wood, Linus Arver,
Linus Arver
In-Reply-To: <pull.1563.v3.git.1695412245.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
ignore_non_trailer() returns the _number of bytes_ that should be
ignored from the end of the log message. It does not by itself "ignore"
anything.
Rename this function to remove the leading "ignore" verb, to sound more
like a quantity than an action.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/commit.c | 2 +-
builtin/merge.c | 2 +-
commit.c | 2 +-
commit.h | 4 ++--
trailer.c | 2 +-
5 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 7da5f924484..d1785d32db1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -900,7 +900,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
strbuf_stripspace(&sb, '\0');
if (signoff)
- append_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0);
+ append_signoff(&sb, ignored_log_message_bytes(sb.buf, sb.len), 0);
if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
die_errno(_("could not write commit template"));
diff --git a/builtin/merge.c b/builtin/merge.c
index de68910177f..6cbbebca13d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -891,7 +891,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
_(no_scissors_editor_comment), comment_line_char);
}
if (signoff)
- append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
+ append_signoff(&msg, ignored_log_message_bytes(msg.buf, msg.len), 0);
write_merge_heads(remoteheads);
write_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);
if (run_commit_hook(0 < option_edit, get_index_file(), NULL,
diff --git a/commit.c b/commit.c
index b3223478bc2..4440fbabb83 100644
--- a/commit.c
+++ b/commit.c
@@ -1769,7 +1769,7 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len
* Returns the number of bytes from the tail to ignore, to be fed as
* the second parameter to append_signoff().
*/
-size_t ignore_non_trailer(const char *buf, size_t len)
+size_t ignored_log_message_bytes(const char *buf, size_t len)
{
size_t boc = 0;
size_t bol = 0;
diff --git a/commit.h b/commit.h
index 28928833c54..1cc872f225f 100644
--- a/commit.h
+++ b/commit.h
@@ -294,8 +294,8 @@ const char *find_header_mem(const char *msg, size_t len,
const char *find_commit_header(const char *msg, const char *key,
size_t *out_len);
-/* Find the end of the log message, the right place for a new trailer. */
-size_t ignore_non_trailer(const char *buf, size_t len);
+/* Find the number of bytes to ignore from the end of a log message. */
+size_t ignored_log_message_bytes(const char *buf, size_t len);
typedef int (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,
void *cb_data);
diff --git a/trailer.c b/trailer.c
index 0b66effceb5..185b3e2707f 100644
--- a/trailer.c
+++ b/trailer.c
@@ -931,7 +931,7 @@ continue_outer_loop:
/* Return the position of the end of the trailers. */
static size_t find_trailer_end(const char *buf, size_t len)
{
- return len - ignore_non_trailer(buf, len);
+ return len - ignored_log_message_bytes(buf, len);
}
static int ends_with_blank_line(const char *buf, size_t len)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 6/9] trailer: find the end of the log message
From: Linus Arver via GitGitGadget @ 2023-09-22 19:50 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Christian Couder, Phillip Wood, Linus Arver,
Linus Arver
In-Reply-To: <pull.1563.v3.git.1695412245.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Previously, trailer_info_get() computed the trailer block end position
by
(1) checking for the opts->no_divider flag and optionally calling
find_patch_start() to find the "patch start" location (patch_start), and
(2) calling find_trailer_end() to find the end of the trailer block
using patch_start as a guide, saving the return value into
"trailer_end".
The logic in (1) was awkward because the variable "patch_start" is
misleading if there is no patch in the input. The logic in (2) was
misleading because it could be the case that no trailers are in the
input (yet we are setting a "trailer_end" variable before even searching
for trailers, which happens later in find_trailer_start()). The name
"find_trailer_end" was misleading because that function did not look for
any trailer block itself --- instead it just computed the end position
of the log message in the input where the end of the trailer block (if
it exists) would be (because trailer blocks must always come after the
end of the log message).
Combine the logic in (1) and (2) together into find_patch_start() by
renaming it to find_end_of_log_message(). The end of the log message is
the starting point which find_trailer_start() needs to start searching
backward to parse individual trailers (if any).
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 61 ++++++++++++++++++++++++++++++++++---------------------
1 file changed, 38 insertions(+), 23 deletions(-)
diff --git a/trailer.c b/trailer.c
index 185b3e2707f..9da89df9d8a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -812,21 +812,47 @@ 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.
+ * Find the end of the log message as an offset from the start of the input
+ * (where callers of this function are interested in looking for a trailers
+ * block in the same input). We have to consider two categories of content that
+ * can come at the end of the input which we want to ignore (because they don't
+ * belong in the log message):
+ *
+ * (1) the "patch part" which begins with a "---" divider and has patch
+ * information (like the output of git-format-patch), and
+ *
+ * (2) any trailing comment lines, blank lines like in the output of "git
+ * commit -v", or stuff below the "cut" (scissor) line.
+ *
+ * As a formula, the situation looks like this:
+ *
+ * INPUT = LOG MESSAGE + IGNORED
+ *
+ * where IGNORED can be either of the two categories described above. It may be
+ * that there is nothing to ignore. Now it may be the case that the LOG MESSAGE
+ * contains a trailer block, but that's not the concern of this function.
*/
-static size_t find_patch_start(const char *str)
+static size_t find_end_of_log_message(const char *input, int no_divider)
{
+ size_t end;
+
const char *s;
- for (s = str; *s; s = next_line(s)) {
+ /* Assume the naive end of the input is already what we want. */
+ end = strlen(input);
+
+ /* Optionally skip over any patch part ("---" line and below). */
+ for (s = input; *s; s = next_line(s)) {
const char *v;
- if (skip_prefix(s, "---", &v) && isspace(*v))
- return s - str;
+ if (!no_divider && skip_prefix(s, "---", &v) && isspace(*v)) {
+ end = s - input;
+ break;
+ }
}
- return s - str;
+ /* Skip over other ignorable bits. */
+ return end - ignored_log_message_bytes(input, end);
}
/*
@@ -928,12 +954,6 @@ continue_outer_loop:
return len;
}
-/* Return the position of the end of the trailers. */
-static size_t find_trailer_end(const char *buf, size_t len)
-{
- return len - ignored_log_message_bytes(buf, len);
-}
-
static int ends_with_blank_line(const char *buf, size_t len)
{
ssize_t ll = last_line(buf, len);
@@ -1104,7 +1124,7 @@ void process_trailers(const char *file,
void trailer_info_get(struct trailer_info *info, const char *str,
const struct process_trailer_options *opts)
{
- int patch_start, trailer_end, trailer_start;
+ int end_of_log_message, trailer_start;
struct strbuf **trailer_lines, **ptr;
char **trailer_strings = NULL;
size_t nr = 0, alloc = 0;
@@ -1112,16 +1132,11 @@ void trailer_info_get(struct trailer_info *info, const char *str,
ensure_configured();
- if (opts->no_divider)
- patch_start = strlen(str);
- else
- patch_start = find_patch_start(str);
-
- trailer_end = find_trailer_end(str, patch_start);
- trailer_start = find_trailer_start(str, trailer_end);
+ end_of_log_message = find_end_of_log_message(str, opts->no_divider);
+ trailer_start = find_trailer_start(str, end_of_log_message);
trailer_lines = strbuf_split_buf(str + trailer_start,
- trailer_end - trailer_start,
+ end_of_log_message - trailer_start,
'\n',
0);
for (ptr = trailer_lines; *ptr; ptr++) {
@@ -1144,7 +1159,7 @@ void trailer_info_get(struct trailer_info *info, const char *str,
info->blank_line_before_trailer = ends_with_blank_line(str,
trailer_start);
info->trailer_start = str + trailer_start;
- info->trailer_end = str + trailer_end;
+ info->trailer_end = str + end_of_log_message;
info->trailers = trailer_strings;
info->trailer_nr = nr;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 9/9] trailer: make stack variable names match field names
From: Linus Arver via GitGitGadget @ 2023-09-22 19:50 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Christian Couder, Phillip Wood, Linus Arver,
Linus Arver
In-Reply-To: <pull.1563.v3.git.1695412245.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/trailer.c b/trailer.c
index 9a3837be770..739acafc4e9 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1134,8 +1134,8 @@ void trailer_info_get(struct trailer_info *info, const char *str,
{
size_t end_of_log_message = 0, trailer_block_start = 0;
struct strbuf **trailer_lines, **ptr;
- char **trailer_strings = NULL;
- size_t nr = 0, alloc = 0;
+ char **trailers = NULL;
+ size_t trailer_nr = 0, alloc = 0;
char **last = NULL;
ensure_configured();
@@ -1155,12 +1155,12 @@ void trailer_info_get(struct trailer_info *info, const char *str,
*last = strbuf_detach(&sb, NULL);
continue;
}
- ALLOC_GROW(trailer_strings, nr + 1, alloc);
- trailer_strings[nr] = strbuf_detach(*ptr, NULL);
- last = find_separator(trailer_strings[nr], separators) >= 1
- ? &trailer_strings[nr]
+ ALLOC_GROW(trailers, trailer_nr + 1, alloc);
+ trailers[trailer_nr] = strbuf_detach(*ptr, NULL);
+ last = find_separator(trailers[trailer_nr], separators) >= 1
+ ? &trailers[trailer_nr]
: NULL;
- nr++;
+ trailer_nr++;
}
strbuf_list_free(trailer_lines);
@@ -1168,13 +1168,13 @@ void trailer_info_get(struct trailer_info *info, const char *str,
trailer_block_start);
info->trailer_block_start = 0;
info->trailer_block_end = 0;
- if (nr) {
+ if (trailer_nr) {
info->trailer_block_start = trailer_block_start;
info->trailer_block_end = end_of_log_message;
}
info->end_of_log_message = end_of_log_message;
- info->trailers = trailer_strings;
- info->trailer_nr = nr;
+ info->trailers = trailers;
+ info->trailer_nr = trailer_nr;
}
void trailer_info_release(struct trailer_info *info)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 7/9] trailer: use offsets for trailer_start/trailer_end
From: Linus Arver via GitGitGadget @ 2023-09-22 19:50 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Christian Couder, Phillip Wood, Linus Arver,
Linus Arver
In-Reply-To: <pull.1563.v3.git.1695412245.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Previously these fields in the trailer_info struct were of type "const
char *" and pointed to positions in the input string directly (to the
start and end positions of the trailer block).
Use offsets to make the intended usage less ambiguous. We only need to
reference the input string in format_trailer_info(), so update that
function to take a pointer to the input.
While we're at it, rename trailer_start to trailer_block_start to be
more explicit about these offsets (that they are for the entire trailer
block including other trailers). Ditto for trailer_end.
Signed-off-by: Linus Arver <linusa@google.com>
---
sequencer.c | 2 +-
trailer.c | 29 ++++++++++++++---------------
trailer.h | 13 ++++++++-----
3 files changed, 23 insertions(+), 21 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index adc9cfb4df3..77362f5cd5d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -331,7 +331,7 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
if (ignore_footer)
sb->buf[sb->len - ignore_footer] = saved_char;
- if (info.trailer_start == info.trailer_end)
+ if (info.trailer_block_start == info.trailer_block_end)
return 0;
for (i = 0; i < info.trailer_nr; i++)
diff --git a/trailer.c b/trailer.c
index 9da89df9d8a..471c2722536 100644
--- a/trailer.c
+++ b/trailer.c
@@ -859,7 +859,7 @@ static size_t find_end_of_log_message(const char *input, int no_divider)
* Return the position of the first trailer line or len if there are no
* trailers.
*/
-static size_t find_trailer_start(const char *buf, size_t len)
+static size_t find_trailer_block_start(const char *buf, size_t len)
{
const char *s;
ssize_t end_of_title, l;
@@ -1075,7 +1075,6 @@ 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;
ensure_configured();
@@ -1086,11 +1085,10 @@ void process_trailers(const char *file,
outfile = create_in_place_tempfile(file);
parse_trailers(&info, sb.buf, &head, opts);
- trailer_end = info.trailer_end - sb.buf;
/* Print the lines before the trailers */
if (!opts->only_trailers)
- fwrite(sb.buf, 1, info.trailer_start - sb.buf, outfile);
+ fwrite(sb.buf, 1, info.trailer_block_start, outfile);
if (!opts->only_trailers && !info.blank_line_before_trailer)
fprintf(outfile, "\n");
@@ -1112,7 +1110,7 @@ void process_trailers(const char *file,
/* Print the lines after the trailers as is */
if (!opts->only_trailers)
- fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
+ fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
if (opts->in_place)
if (rename_tempfile(&trailers_tempfile, file))
@@ -1124,7 +1122,7 @@ void process_trailers(const char *file,
void trailer_info_get(struct trailer_info *info, const char *str,
const struct process_trailer_options *opts)
{
- int end_of_log_message, trailer_start;
+ size_t end_of_log_message = 0, trailer_block_start = 0;
struct strbuf **trailer_lines, **ptr;
char **trailer_strings = NULL;
size_t nr = 0, alloc = 0;
@@ -1133,10 +1131,10 @@ void trailer_info_get(struct trailer_info *info, const char *str,
ensure_configured();
end_of_log_message = find_end_of_log_message(str, opts->no_divider);
- trailer_start = find_trailer_start(str, end_of_log_message);
+ trailer_block_start = find_trailer_block_start(str, end_of_log_message);
- trailer_lines = strbuf_split_buf(str + trailer_start,
- end_of_log_message - trailer_start,
+ trailer_lines = strbuf_split_buf(str + trailer_block_start,
+ end_of_log_message - trailer_block_start,
'\n',
0);
for (ptr = trailer_lines; *ptr; ptr++) {
@@ -1157,9 +1155,9 @@ void trailer_info_get(struct trailer_info *info, const char *str,
strbuf_list_free(trailer_lines);
info->blank_line_before_trailer = ends_with_blank_line(str,
- trailer_start);
- info->trailer_start = str + trailer_start;
- info->trailer_end = str + end_of_log_message;
+ trailer_block_start);
+ info->trailer_block_start = trailer_block_start;
+ info->trailer_block_end = end_of_log_message;
info->trailers = trailer_strings;
info->trailer_nr = nr;
}
@@ -1174,6 +1172,7 @@ void trailer_info_release(struct trailer_info *info)
static void format_trailer_info(struct strbuf *out,
const struct trailer_info *info,
+ const char *msg,
const struct process_trailer_options *opts)
{
size_t origlen = out->len;
@@ -1183,8 +1182,8 @@ static void format_trailer_info(struct strbuf *out,
if (!opts->only_trailers && !opts->unfold && !opts->filter &&
!opts->separator && !opts->key_only && !opts->value_only &&
!opts->key_value_separator) {
- strbuf_add(out, info->trailer_start,
- info->trailer_end - info->trailer_start);
+ strbuf_add(out, msg + info->trailer_block_start,
+ info->trailer_block_end - info->trailer_block_start);
return;
}
@@ -1238,7 +1237,7 @@ void format_trailers_from_commit(struct strbuf *out, const char *msg,
struct trailer_info info;
trailer_info_get(&info, msg, opts);
- format_trailer_info(out, &info, opts);
+ format_trailer_info(out, &info, msg, opts);
trailer_info_release(&info);
}
diff --git a/trailer.h b/trailer.h
index a689d768c79..4dcb9080327 100644
--- a/trailer.h
+++ b/trailer.h
@@ -32,16 +32,19 @@ int trailer_set_if_missing(enum trailer_if_missing *item, const char *value);
struct trailer_info {
/*
* True if there is a blank line before the location pointed to by
- * trailer_start.
+ * trailer_block_start.
*/
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.
+ * Offsets to the trailer block start and end positions in the input
+ * string. If no trailer block is found, these are both set to the
+ * "true" end of the input, per find_true_end_of_input().
+ *
+ * NOTE: This will be changed so that these point to 0 in the next
+ * patch if no trailers are found.
*/
- const char *trailer_start, *trailer_end;
+ size_t trailer_block_start, trailer_block_end;
/*
* Array of trailers found.
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 8/9] trailer: only use trailer_block_* variables if trailers were found
From: Linus Arver via GitGitGadget @ 2023-09-22 19:50 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Christian Couder, Phillip Wood, Linus Arver,
Linus Arver
In-Reply-To: <pull.1563.v3.git.1695412245.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Previously, these variables were overloaded to act as the end of the log
message even if no trailers were found.
Remove the overloaded meaning by adding a new end_of_log_message field
to the trailer_info struct. The trailer_info struct consumers now only
refer to the trailer_block_start and trailer_block_end fields if
trailers were found (trailer_nr > 0), and otherwise refer to the
end_of_log_message.
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 31 +++++++++++++++++++++++--------
trailer.h | 12 +++++++-----
2 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/trailer.c b/trailer.c
index 471c2722536..9a3837be770 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1086,9 +1086,14 @@ void process_trailers(const char *file,
parse_trailers(&info, sb.buf, &head, opts);
- /* Print the lines before the trailers */
- if (!opts->only_trailers)
- fwrite(sb.buf, 1, info.trailer_block_start, outfile);
+ /* Print the lines before the trailers (if any) as is. */
+ if (!opts->only_trailers) {
+ if (info.trailer_nr) {
+ fwrite(sb.buf, 1, info.trailer_block_start, outfile);
+ } else {
+ fwrite(sb.buf, 1, info.end_of_log_message, outfile);
+ }
+ }
if (!opts->only_trailers && !info.blank_line_before_trailer)
fprintf(outfile, "\n");
@@ -1108,9 +1113,14 @@ void process_trailers(const char *file,
free_all(&head);
trailer_info_release(&info);
- /* Print the lines after the trailers as is */
- if (!opts->only_trailers)
- fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
+ /* Print the lines after the trailers (if any) as is. */
+ if (!opts->only_trailers) {
+ if (info.trailer_nr) {
+ fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
+ } else {
+ fwrite(sb.buf + info.end_of_log_message, 1, sb.len - info.end_of_log_message, outfile);
+ }
+ }
if (opts->in_place)
if (rename_tempfile(&trailers_tempfile, file))
@@ -1156,8 +1166,13 @@ void trailer_info_get(struct trailer_info *info, const char *str,
info->blank_line_before_trailer = ends_with_blank_line(str,
trailer_block_start);
- info->trailer_block_start = trailer_block_start;
- info->trailer_block_end = end_of_log_message;
+ info->trailer_block_start = 0;
+ info->trailer_block_end = 0;
+ if (nr) {
+ info->trailer_block_start = trailer_block_start;
+ info->trailer_block_end = end_of_log_message;
+ }
+ info->end_of_log_message = end_of_log_message;
info->trailers = trailer_strings;
info->trailer_nr = nr;
}
diff --git a/trailer.h b/trailer.h
index 4dcb9080327..5e2843d320a 100644
--- a/trailer.h
+++ b/trailer.h
@@ -38,14 +38,16 @@ struct trailer_info {
/*
* Offsets to the trailer block start and end positions in the input
- * string. If no trailer block is found, these are both set to the
- * "true" end of the input, per find_true_end_of_input().
- *
- * NOTE: This will be changed so that these point to 0 in the next
- * patch if no trailers are found.
+ * string. If no trailer block is found, these are set to 0.
*/
size_t trailer_block_start, trailer_block_end;
+ /*
+ * Offset to the end of the log message in the input (may not be the
+ * same as the end of the input).
+ */
+ size_t end_of_log_message;
+
/*
* Array of trailers found.
*/
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v7 2/3] unit tests: add TAP unit test framework
From: Junio C Hamano @ 2023-09-22 20:05 UTC (permalink / raw)
To: phillip.wood123; +Cc: Josh Steadmon, git, linusa, calvinwan, rsbecker
In-Reply-To: <xmqq350hw6n7.fsf@gitster.g>
It seems this got stuck during Josh's absense and I didn't ping it
further, but I should have noticed that you are the author of this
patch, and pinged you in the meantime.
Any thought on the "polarity" of the return values from the
assertion? I still find it confusing and hard to follow.
Thanks.
> Josh Steadmon <steadmon@google.com> writes:
>
>> +test_expect_success 'TAP output from unit tests' '
>> + cat >expect <<-EOF &&
>> + ok 1 - passing test
>> + ok 2 - passing test and assertion return 0
>> + # check "1 == 2" failed at t/unit-tests/t-basic.c:76
>> + # left: 1
>> + # right: 2
>> + not ok 3 - failing test
>> + ok 4 - failing test and assertion return -1
>> + not ok 5 - passing TEST_TODO() # TODO
>> + ok 6 - passing TEST_TODO() returns 0
>> + # todo check ${SQ}check(x)${SQ} succeeded at t/unit-tests/t-basic.c:25
>> + not ok 7 - failing TEST_TODO()
>> + ok 8 - failing TEST_TODO() returns -1
>> + # check "0" failed at t/unit-tests/t-basic.c:30
>> + # skipping test - missing prerequisite
>> + # skipping check ${SQ}1${SQ} at t/unit-tests/t-basic.c:32
>> + ok 9 - test_skip() # SKIP
>> + ok 10 - skipped test returns 0
>> + # skipping test - missing prerequisite
>> + ok 11 - test_skip() inside TEST_TODO() # SKIP
>> + ok 12 - test_skip() inside TEST_TODO() returns 0
>> + # check "0" failed at t/unit-tests/t-basic.c:48
>> + not ok 13 - TEST_TODO() after failing check
>> + ok 14 - TEST_TODO() after failing check returns -1
>> + # check "0" failed at t/unit-tests/t-basic.c:56
>> + not ok 15 - failing check after TEST_TODO()
>> + ok 16 - failing check after TEST_TODO() returns -1
>> + # check "!strcmp("\thello\\\\", "there\"\n")" failed at t/unit-tests/t-basic.c:61
>> + # left: "\011hello\\\\"
>> + # right: "there\"\012"
>> + # check "!strcmp("NULL", NULL)" failed at t/unit-tests/t-basic.c:62
>> + # left: "NULL"
>> + # right: NULL
>> + # check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/unit-tests/t-basic.c:63
>> + # left: ${SQ}a${SQ}
>> + # right: ${SQ}\012${SQ}
>> + # check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/unit-tests/t-basic.c:64
>> + # left: ${SQ}\\\\${SQ}
>> + # right: ${SQ}\\${SQ}${SQ}
>> + not ok 17 - messages from failing string and char comparison
>> + # BUG: test has no checks at t/unit-tests/t-basic.c:91
>> + not ok 18 - test with no checks
>> + ok 19 - test with no checks returns -1
>> + 1..19
>> + EOF
>
> Presumably t-basic will serve as a catalog of check_* functions and
> the test binary, together with this test piece, will keep growing as
> we gain features in the unit tests infrastructure. I wonder how
> maintainable the above is, though. When we acquire new test, we
> would need to renumber. What if multiple developers add new
> features to the catalog at the same time?
>
>> diff --git a/t/unit-tests/.gitignore b/t/unit-tests/.gitignore
>> new file mode 100644
>> index 0000000000..e292d58348
>> --- /dev/null
>> +++ b/t/unit-tests/.gitignore
>> @@ -0,0 +1,2 @@
>> +/t-basic
>> +/t-strbuf
>
> Also, can we come up with some naming convention so that we do not
> have to keep adding to this file every time we add a new test
> script?
>
>> diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c
>> new file mode 100644
>> index 0000000000..561611e242
>> --- /dev/null
>> +++ b/t/unit-tests/t-strbuf.c
>> @@ -0,0 +1,75 @@
>> +#include "test-lib.h"
>> +#include "strbuf.h"
>> +
>> +/* wrapper that supplies tests with an initialized strbuf */
>> +static void setup(void (*f)(struct strbuf*, void*), void *data)
>> +{
>> + struct strbuf buf = STRBUF_INIT;
>> +
>> + f(&buf, data);
>> + strbuf_release(&buf);
>> + check_uint(buf.len, ==, 0);
>> + check_uint(buf.alloc, ==, 0);
>> + check(buf.buf == strbuf_slopbuf);
>> + check_char(buf.buf[0], ==, '\0');
>> +}
>
> What I am going to utter from here on are not complaints but purely
> meant as questions.
>
> Would the resulting output and maintainability of the tests change
> (improve, or worsen) if we introduce
>
> static void assert_empty_strbuf(struct strbuf *buf)
> {
> check_uint(buf->len, ==, 0);
> check_uint(buf->alloc, ==, 0);
> check(buf.buf == strbuf_slopbuf);
> check_char(buf.buf[0], ==, '\0');
> }
>
> and call it from the setup() function to ensure that
> strbuf_release(&buf) it calls after running customer test f() brings
> the buffer in a reasonably initialized state? The t_static_init()
> test should be able to say
>
> static void t_static_init(void)
> {
> struct strbuf buf = STRBUF_INIT;
> assert_empty_strbuf(&buf);
> }
>
> if we did so, but is that a good thing or a bad thing (e.g. it may
> make it harder to figure out where the real error came from, because
> of the "line number" thing will not easily capture the caller of the
> caller, perhaps)?
>
>> +static void t_static_init(void)
>> +{
>> + struct strbuf buf = STRBUF_INIT;
>> +
>> + check_uint(buf.len, ==, 0);
>> + check_uint(buf.alloc, ==, 0);
>> + if (check(buf.buf == strbuf_slopbuf))
>> + return; /* avoid de-referencing buf.buf */
>
> strbuf_slopbuf[0] is designed to be readable. Do check() assertions
> return their parameter negated?
>
> In other words, if "we expect buf.buf to point at the slopbuf, but
> if that expectation does not hold, check() returns true and we
> refrain from doing check_char() on the next line because we cannot
> trust what buf.buf points at" is what is going on here, I find it
> very confusing. Perhaps my intuition is failing me, but somehow I
> would have expected that passing check_foo() would return true while
> failing ones would return false.
>
> IOW I would expect
>
> if (check(buf.buf == strbuf_slopbuf))
> return;
>
> to work very similarly to
>
> if (buf.buf == strbuf_slopbuf)
> return;
>
> in expressing the control flow, simply because they are visually
> similar. But of course, if we early-return because buf.buf that
> does not point at strbuf_slopbuf is a sign of trouble, then the
> control flow we want is
>
> if (buf.buf != strbuf_slopbuf)
> return;
>
> or
>
> if (!(buf.buf == strbuf_slopbuf))
> return;
>
> The latter is easier to translate to check_foo(), because what is
> inside the inner parentheses is the condition we expect, and we
> would like check_foo() to complain when the condition does not hold.
>
> For the "check_foo()" thing to work in a similar way, while having
> the side effect of reporting any failed expectations, we would want
> to write
>
> if (!check(buf.buf == strbuf_slopbuf))
> return;
>
> And for that similarity to work, check_foo() must return false when
> its expectation fails, and return true when its expectation holds.
>
> I think that is where my "I find it very confusing" comes from.
>
>> + check_char(buf.buf[0], ==, '\0');
>> +}
>
>> +static void t_dynamic_init(void)
>> +{
>> + struct strbuf buf;
>> +
>> + strbuf_init(&buf, 1024);
>> + check_uint(buf.len, ==, 0);
>> + check_uint(buf.alloc, >=, 1024);
>> + check_char(buf.buf[0], ==, '\0');
>
> Is it sensible to check buf.buf is not slopbuf at this point, or
> does it make the test TOO intimate with the current implementation
> detail?
>
>> + strbuf_release(&buf);
>> +}
>> +
>> +static void t_addch(struct strbuf *buf, void *data)
>> +{
>> + const char *p_ch = data;
>> + const char ch = *p_ch;
>> +
>> + strbuf_addch(buf, ch);
>> + if (check_uint(buf->len, ==, 1) ||
>> + check_uint(buf->alloc, >, 1))
>> + return; /* avoid de-referencing buf->buf */
>
> Again, I find the return values from these check_uint() calls highly
> confusing, if this is saying "if len is 1 and alloc is more than 1,
> then we are in an expected state and can further validate that buf[0]
> is ch and buf[1] is NULL, but otherwise we should punt". The polarity
> looks screwy. Perhaps it is just me?
>
>> + check_char(buf->buf[0], ==, ch);
>> + check_char(buf->buf[1], ==, '\0');
>> +}
>
> In any case, this t_addch() REQUIRES that incoming buf is empty,
> doesn't it? I do not think it is sensible. I would have expected
> that it would be more like
>
> t_addch(struct strbuf *buf, void *data)
> {
> char ch = *(char *)data;
> size_t orig_alloc = buf->alloc;
> size_t orig_len = buf->len;
>
> if (!assert_sane_strbuf(buf))
> return;
> strbuf_addch(buf, ch);
> if (!assert_sane_strbuf(buf))
> return;
> check_uint(buf->len, ==, orig_len + 1);
> check_uint(buf->alloc, >=, orig_alloc);
> check_char(buf->buf[buf->len - 1], ==, ch);
> check_char(buf->buf[buf->len], ==, '\0');
> }
>
> to ensure that we can add a ch to a strbuf with any existing
> contents and get a one-byte longer contents than before, with the
> last byte of the buffer becoming 'ch' and still NUL terminated.
>
> And we protect ourselves with a helper that checks if the given
> strbuf looks *sane*.
>
> static int assert_sane_strbuf(struct strbuf *buf)
> {
> /* can use slopbuf only when the length is 0 */
> if (buf->buf == strbuf_slopbuf)
> return (buf->len == 0);
> /* everybody else must have non-NULL buffer */
> if (buf->buf == NULL)
> return 0;
> /*
> * alloc must be at least 1 byte larger than len
> * for the terminating NUL at the end.
> */
> return ((buf->len + 1 <= buf->alloc) &&
> (buf->buf[buf->len] == '\0'));
> }
>
> You can obviously use your check_foo() for the individual checks
> done in this function to get a more detailed diagnosis, but because
> I have confused myself enough by thinking about their polarity, I
> wrote this in barebones comparison instead.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox