Fixes to trailer test script, help text, and documentation #1564
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
listx
changed the title
|
/submit |
|
Submitted as pull.1564.git.1691210737.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -114,16 +114,20 @@ OPTIONS | |||
| Specify where all new trailers will be added. A setting | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -114,8 +114,10 @@ OPTIONS
> Specify where all new trailers will be added. A setting
> provided with '--where' overrides all configuration variables
Obviously this is not a new issue, but "all configuration variables"
is misleading (the same comment applies to the description of the
"--[no-]if-exists" and the "--[no-]if-missing" options).
If I am reading the code correctly, --where=value overrides the
trailer.where variable and nothing else, and --no-where stops the
overriding of the trailer.where variable. Ditto for the other two
with their relevant configuration variables.
There was a problem hiding this comment.
On the Git mailing list, Linus Arver wrote (reply to this):
Junio C Hamano <gitster@pobox.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> @@ -114,8 +114,10 @@ OPTIONS
>> Specify where all new trailers will be added. A setting
>> provided with '--where' overrides all configuration variables
>
> Obviously this is not a new issue, but "all configuration variables"
> is misleading (the same comment applies to the description of the
> "--[no-]if-exists" and the "--[no-]if-missing" options).
Agreed.
> If I am reading the code correctly, --where=value overrides the
> trailer.where variable and nothing else, and --no-where stops the
> overriding of the trailer.where variable. Ditto for the other two
> with their relevant configuration variables.
That is also my understanding. Will update to remove the "all" wording.
On a separate note, I've realized there are more fixes to be done in
this area (as I get more familiar with the codebase). For example, we
have the following language in builtin/interpret-trailers.c inside
cmd_interpret_trailers():
OPT_BOOL(0, "only-input", &opts.only_input, N_("do not apply config rules")),
which should be fixed in similar style to what you suggested above,
probably with:
OPT_BOOL(0, "only-input", &opts.only_input, N_("do not apply trailer.* configuration variables")),
When I reroll, I will include these additional fixes so expect the patch
series to grow (probably ~12 patches instead of the ~5).
One more thing. I think the documentation
(Documentation/git-interpret-trailers.txt) uses the word "<token>" in
two different ways. For example, if we have in the input
subject line
body text
Acked-by: Foo
the docs treat the word "Acked-by:" as the <token>. However, it defines
the relevant configuration section like this:
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.
+
If there is a separator, then the key will be used instead of both the
<token> and the default separator when adding the trailer.
So if I configure this like
git config trailer.ack.key "Acked-by" &&
the <token> is both the longer-form "Acked-by:" (per the meaning so far
in the doc) but also the shorter string "ack" per the
"trailer.<token>.key" configuration section syntax. This secondary
meaning is repeated again in the very start of the doc when we define
the --trailer option syntax as
SYNOPSIS
--------
[verse]
'git interpret-trailers' [--in-place] [--trim-empty]
[(--trailer <token>[(=|:)<value>])...]
[--parse] [<file>...]
because the <token> here could be (using the example above) either
"Acked-by" (as in "--trailer=Acked-by:...") if we did not configure
"trailer.ack.key", or just "ack" (as in "--trailer=ack:...") if we did
configure it. These two scenarios would give identical "Acked-by: ..."
output.
This is confusing and I don't like how we overload this "token" word
(not to mention we already have the word "key" which we don't really use
much in the docs).
I am inclined to replace most uses of the word "<token>" with "<key>"
while leaving the "trailer.<token>.key" configuration syntax intact.
This will result in a large diff but I think the removal of the double
meaning is worth it, and will include this fix also in the next reroll.
The main reason I bring this up is because this means also having to
update our funciton names like "token_len_without_separator" in
trailer.c, to be "key_len_without_separator" if we want the nomenclature
in the trailer.c internals to be consistent with the (updated)
user-facing docs. I am not sure whether we want to do this as part of
the same reroll, or if we should leave it as #leftoverbits for a future
series.There was a problem hiding this comment.
On the Git mailing list, Linus Arver wrote (reply to this):
Linus Arver <linusa@google.com> writes:
> So if I configure this like
>
> git config trailer.ack.key "Acked-by" &&
>
Oops, I meant just
git config trailer.ack.key "Acked-by"
without the trailing "&&".There was a problem hiding this comment.
On the Git mailing list, Linus Arver wrote (reply to this):
Linus Arver <linusa@google.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> @@ -114,8 +114,10 @@ OPTIONS
>>> Specify where all new trailers will be added. A setting
>>> provided with '--where' overrides all configuration variables
>>
>> Obviously this is not a new issue, but "all configuration variables"
>> is misleading (the same comment applies to the description of the
>> "--[no-]if-exists" and the "--[no-]if-missing" options).
>
> Agreed.
>
>> If I am reading the code correctly, --where=value overrides the
>> trailer.where variable and nothing else, and --no-where stops the
>> overriding of the trailer.where variable. Ditto for the other two
>> with their relevant configuration variables.
>
> That is also my understanding. Will update to remove the "all" wording.
Hmph, actually it also overrides any applicable "trailer.<token>.where"
configurations (these <token>-specific configurations override the
"trailer.where" configuration where applicable). Still, the "all
configuration variables" wording should be updated, probably like this:
› Specify where all new trailers will be added. A setting
› provided with '--where' overrides the `trailer.where` and any
› applicable `trailer.<token>.where` configuration variables
› and applies to all '--trailer' options until the next occurrence of
› '--where' or '--no-where'.There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Linus Arver <linusa@google.com> writes:
>>> If I am reading the code correctly, --where=value overrides the
>>> trailer.where variable and nothing else, and --no-where stops the
>>> overriding of the trailer.where variable. Ditto for the other two
>>> with their relevant configuration variables.
>>
>> That is also my understanding. Will update to remove the "all" wording.
>
> Hmph, actually it also overrides any applicable "trailer.<token>.where"
> configurations (these <token>-specific configurations override the
> "trailer.where" configuration where applicable). Still, the "all
> configuration variables" wording should be updated, probably like this:
>
> › Specify where all new trailers will be added. A setting
> › provided with '--where' overrides the `trailer.where` and any
> › applicable `trailer.<token>.where` configuration variables
> › and applies to all '--trailer' options until the next occurrence of
> › '--where' or '--no-where'.
Yup, that was what I meant. I found it troubling that the "all"
phrasing felt like it meant all trailer.* configurations, not just
the "where" thing. Your new description looks good.
Thanks.
|
This branch is now known as |
|
This patch series was integrated into seen via git@a0e7d72. |
| @@ -489,7 +489,7 @@ test_expect_success 'multiline field treated as atomic for neighbor check' ' | |||
| ' | |||
There was a problem hiding this comment.
On the Git mailing list, Linus Arver wrote (reply to this):
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
> ...
> @@ -1392,7 +1610,9 @@ test_expect_success 'with failing command using $ARG' '
> '
>
> test_expect_success 'with empty tokens' '
> - git config --unset trailer.fix.command &&
> + test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
> + test_config trailer.sign.key "Signed-off-by: " &&
> + test_config trailer.ifexists "addIfDifferent" &&
> cat >expected <<-EOF &&
>
> Signed-off-by: A U Thor <author@example.com>
In this test and some other places we get the chance to remove
invocations of "git config --unset ..." (because we don't leak config
state anymore). I will update the commit message accordingly in the next
reroll.
added 11 commits
August 6, 2023 23:03
By using "test_config" instead of "git config", we avoid leaking
configuration state across test cases. This in turn helps to make the
tests more self-contained, by explicitly capturing the configuration
setup. It then makes it easier to add tests anywhere in this 1500+ line
file, without worrying about what implicit state was set in some prior
test case defined earlier up in the script.
This commit was created mechanically as follows: we changed the first
occurrence of a particular "git config trailer.*" option, then ran the
tests repeatedly to see which ones broke, adding in the extra
"test_config" equivalents to make them pass again. In addition, in some
test cases we removed "git config --unset ..." lines because they were
no longer necessary (as the --unset was being used to clean up leaked
configuration state from earlier test cases).
The process described above was done repeatedly until there were no more
unbridled "git config" invocations. Some "git config" invocations still
do exist in the script, but they were already cleaned up properly with
test_when_finished "git config --remove-section ..."
so they were left alone.
Note that these cleanups result in generally longer test case setups
because the previously hidden state is now being exposed. Although we
could then clean up the test cases' "expected" values to be less
verbose (the verbosity arising from the use of implicit state), we
choose not to do so here, to make sure that this cleanup does not change
any meanings behind the test cases.
Signed-off-by: Linus Arver <linusa@google.com>
Signed-off-by: Linus Arver <linusa@google.com>
While the "--no-where" flag is tested, the "--no-if-exists" and "--no-if-missing" flags are not, so add tests for them. But also add tests for all "--no-*" flags to check their effects, both when (1) there are relevant configuration variables set, and (2) they are not set. Signed-off-by: Linus Arver <linusa@google.com>
The wording "all configuration variables" is misleading (the same could be said to the descriptions of the "--[no-]if-exists" and the "--[no-]if-missing" options). Specifying --where=value overrides only the trailer.where variable and applicable trailer.<token>.where variables, and --no-where stops the overriding of these variables. Ditto for the other two with their relevant configuration variables. Reported-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Linus Arver <linusa@google.com>
Fix the help text to say "placement" instead of "action" because the values are placements, not actions. While we're at it, tweak the documentation to say "placements" instead of "values", similar to how the existing language for "--if-exists" uses the word "action" to describe both the syntax (with the phrase "--if-exists <action>") and the possible values (with the phrase "possible actions"). Signed-off-by: Linus Arver <linusa@google.com>
It's unclear what treating something "specially" means. Signed-off-by: Linus Arver <linusa@google.com>
The existing description "set parsing options" is vague, because arguably _all_ of the options for interpret-trailers have to do with parsing to some degree. Explain what this flag does to match what is in the docs, namely how it is an alias for "--only-trailers --only-input --unfold". Signed-off-by: Linus Arver <linusa@google.com>
Use the phrase "configuration variables" instead of "rules" because
(1) we already say "configuration variables" in multiple
places in the docs (where the word "rules" is only used for describing
"--only-input" behavior and for an unrelated case of mentioning how
the trailers do not follow "rules for RFC 822 headers"), and
(2) this phrase is more specific than just "rules".
Signed-off-by: Linus Arver <linusa@google.com>
For users who are skimming the docs to go straight to the individual breakdown of each flag, it may not be clear why --parse is a convenience alias (without them also looking at the other options that --parse turns on). To save them the trouble of looking at the other options (and computing what that would mean), describe a summary of the overall effect. Similarly update the area when we first mention --parse near the top of the doc. Signed-off-by: Linus Arver <linusa@google.com>
The phrase "join whitespace-continued values" requires some additional
context. For example, "whitespace" means newlines (not just space
characters), and "join" means to join only the multiple lines together
for a single trailer (and not that we are joining multiple trailers
together). That is, "join" means to convert
token: This is a very long value, with spaces and
newlines in it.
to
token: This is a very long value, with spaces and newlines in it.
and does not mean to convert
token: value1
token: value2
to
token: value1 value2.
Update the help text to resolve the above ambiguity. While we're add it,
update the docs to use similar language as the change in the help text.
Signed-off-by: Linus Arver <linusa@google.com>
The sentence does not mention the effect of configuration variables at all, when they are actively used by default (unless --parse is specified) to potentially add new trailers, without the user having to always supply --trailer manually. Signed-off-by: Linus Arver <linusa@google.com>
|
This patch series was integrated into seen via git@5d50700. |
|
This patch series was integrated into seen via git@67ce935. |
|
There was a status update in the "New Topics" section about the branch Test coverage improvement for trailers. Needs review. source: <pull.1564.git.1691210737.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@46c99c2. |
|
This patch series was integrated into seen via git@4a1d9e4. |
Signed-off-by: Linus Arver <linusa@google.com>
listx
force-pushed
the
trailer-fixes
branch
2 times, most recently
from
416772b
to
7b66cf2
Compare
|
/submit |
|
Submitted as pull.1564.v2.git.1691702283.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
There was a status update in the "Cooking" section about the branch Test coverage for trailers has been improved. source: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@954a083. |
|
There was a status update in the "Cooking" section about the branch Test coverage for trailers has been improved. source: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@8c7ccd9. |
|
There was a status update in the "Cooking" section about the branch Test coverage for trailers has been improved. source: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@3d996f9. |
|
This patch series was integrated into seen via git@83fd24a. |
|
There was a status update in the "Cooking" section about the branch Test coverage for trailers has been improved. source: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@81d8c82. |
|
This patch series was integrated into seen via git@82f084a. |
|
There was a status update in the "Cooking" section about the branch Test coverage for trailers has been improved. source: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@1fc5981. |
|
This patch series was integrated into next via git@69fef35. |
|
There was a status update in the "Cooking" section about the branch Test coverage for trailers has been improved. Will merge to 'master'. source: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@652cd7c. |
|
This patch series was integrated into seen via git@4a1306b. |
|
This patch series was integrated into seen via git@ac01feb. |
|
There was a status update in the "Cooking" section about the branch Test coverage for trailers has been improved. Will merge to 'master'. source: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@2a461ae. |
|
This patch series was integrated into seen via git@36593fb. |
|
There was a status update in the "Cooking" section about the branch Test coverage for trailers has been improved. Will merge to 'master'. source: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@c75e914. |
|
This patch series was integrated into master via git@c75e914. |
|
This patch series was integrated into next via git@c75e914. |
|
Closed via c75e914. |
| @@ -9,7 +9,7 @@ SYNOPSIS | |||
| -------- | |||
There was a problem hiding this comment.
On the Git mailing list, Teng Long wrote (reply to this):
Linus Arver <linusa@google.com> writes:
> diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
> index 25433e1a1ff..418265f044d 100644
> --- a/Documentation/git-interpret-trailers.txt
> +++ b/Documentation/git-interpret-trailers.txt
> @@ -9,7 +9,7 @@ SYNOPSIS
> --------
> [verse]
> 'git interpret-trailers' [--in-place] [--trim-empty]
> - [(--trailer <token>[(=|:)<value>])...]
> + [(--trailer (<key>|<keyAlias>)[(=|:)<value>])...]
Maybe use 'key-alias' instead of 'keyAlias'? After checking the naming in some
other documents, it seems that they are basically in the former format, not
camel case format.
Thanks.
|
User |
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.
This series contains various fixes to the trailer code. They pertain to fixes to the test script, the command line help text for the interpret-trailers builtin, and the documentation.
Patch 1 is the most important as it does cleanups in the tests where we used 'git config' in a test case without cleaning up that state for the next test. This makes the tests self-contained, making it easier to add new tests anywhere along the script, without worrying about previously-set implicit state. These test cleanups exposed lots of cases where the test cases are mutating more configuration state than is necessary to test the specific behavior in the test; however such extraneous configurations were not cleaned up to make these patches easier to review (again, we are not changing any behavior and we are also not changing what the test cases themselves purport to do).
Note that Patch 1 was originally a 22-commit series, but was squashed together to make it easier to see the final diff for each test case. You can see the 22-commit breakdown at https://github.com/listx/git/tree/backup-trailer-22-commit-breakdown
Patch 3 adds some tests to check the behavior of '--no-if-exists' and '--no-if-missing', which weren't previously tested. It also adds similarly-themed test cases for '--no-where' which only had 1 test case for it.
The other patches aren't as important, but are included here because I think they are too small to include in a separate series.
Updates in v3
Updates in v2
<keyAlias>) is the most important as it resolves a longtime ambiguity about what a<token>can be.Cc: Christian Couder chriscool@tuxfamily.org
cc: Jonathan Tan jonathantanmy@google.com
cc: Teng Long dyroneteng@gmail.com