log: add log.excludeDecoration config option
PATCH 1/1 log: add log.excludeDecoration config option
From: Derrick Stolee <[email protected]>
In 'git log', the --decorate-refs-exclude option appends a pattern to a string_list. This list is used to prevent showing some refs in the decoration output, or even by --simplify-by-decoration.
Users may want to use their refs space to store utility refs that should not appear in the decoration output. For example, Scalar [1] runs a background fetch but places the "new" refs inside the refs/scalar/hidden/<remote>/* refspace instead of refs/<remote>/* to avoid updating remote refs when the user is not looking. However, these "hidden" refs appear during regular 'git log' queries.
A similar idea to use "hidden" refs is under consideration for core Git [2].
Add the 'log.excludeDecoration' config option so users can exclude some refs from decorations by default instead of needing to use --decorate-refs-exclude manually. The config value is multi-valued much like the command-line option.
There are several tests in t4202-log.sh that test the --decorate-refs-(include|exclude) options, so these are extended. Since the expected output is already stored as a file, simply add new calls that replace a "--decorate-refs-exclude" option with an in-line config setting.
[1] https://github.com/microsoft/scalar
[2] https://lore.kernel.org/git/77b1da5d3063a2404cd750adfe3bb8be9b6c497d.1585946894.git.gitgitgadget@gmail.com/
---
log: add log.excludeDecoration config option
This was something hinted at in the "fetch" step of the background maintenance RFC. Should be a relatively minor addition to our config options.
We definitely want this feature for microsoft/git (we would set log.excludeDecoration=refs/scalar/* in all Scalar repos), but we will wait for feedback from the community.
Thanks, -Stolee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-610%2Fderrickstolee%2Flog-exclude-decoration-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-610/derrickstolee/log-exclude-decoration-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/610
Documentation/config/log.txt | 5 +++++ builtin/log.c | 14 ++++++++++++++ t/t4202-log.sh | 22 ++++++++++++++++++++++ 3 files changed, 41 insertions(+)
Documentation/config/log.txt +5 -0
@@ -18,6 +18,11 @@ log.decorate:: names are shown. This is the same as the `--decorate` option of the `git log`. +log.excludeDecoration:: + Exclude the specified patterns from the log decorations. This multi- + valued config option is the same as the `--decorate-refs-exclude` + option of `git log`. + log.follow:: If `true`, `git log` will act as if the `--follow` option was used when a single <path> is given. This has the same limitations as `--follow`,
builtin/log.c +14 -0
@@ -236,7 +236,21 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, }if (decoration_style) { +const struct string_list *config_exclude = +repo_config_get_value_multi (the_repository, +"log.excludeDecoration" ); + +if (config_exclude) { +struct string_list_item *item; +for (item = config_exclude->items; + item && item < config_exclude->items + config_exclude->nr; + item++) +string_list_append (&decorate_refs_exclude, + item->string); + } + rev->show_decorations =1 ; +load_ref_decorations (&decoration_filter, decoration_style); }
t/t4202-log.sh +22 -0
@@ -787,6 +787,9 @@ test_expect_success 'decorate-refs-exclude with glob' ' EOF git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs-exclude="heads/octopus*" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="heads/octopus*" log \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && test_cmp expect.decorate actual '
@@ -801,6 +804,9 @@ test_expect_success 'decorate-refs-exclude without globs' ' EOF git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs-exclude="tags/reach" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="tags/reach" log \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && test_cmp expect.decorate actual '
@@ -816,6 +822,14 @@ test_expect_success 'multiple decorate-refs-exclude' ' git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs-exclude="heads/octopus*" \ --decorate-refs-exclude="tags/reach" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="heads/octopus*" \ + -c log.excludeDecoration="tags/reach" log \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="heads/octopus*" log \ + --decorate-refs-exclude="tags/reach" \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && test_cmp expect.decorate actual '
@@ -831,6 +845,10 @@ test_expect_success 'decorate-refs and decorate-refs-exclude' ' git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs="heads/*" \ --decorate-refs-exclude="heads/oc*" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="heads/oc*" log \ + --decorate-refs="heads/*" \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && test_cmp expect.decorate actual '
@@ -846,6 +864,10 @@ test_expect_success 'decorate-refs-exclude and simplify-by-decoration' ' git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs-exclude="*octopus*" \ --simplify-by-decoration >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="*octopus*" log \ + -n6 --decorate=short --pretty="tformat:%f%d" \ + --simplify-by-decoration >actual && test_cmp expect.decorate actual '
base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925
Signature · 1 line
gitgitgadget
Hi Stolee,
Quoted 58 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
In 'git log', the --decorate-refs-exclude option appends a pattern to a
string_list. This list is used to prevent showing some refs in the decoration output, or even by --simplify-by-decoration.Users may want to use their refs space to store utility refs that should not appear in the decoration output. For example, Scalar [1] runs a background fetch but places the "new" refs inside the refs/scalar/hidden/<remote>/* refspace instead of refs/<remote>/* to avoid updating remote refs when the user is not looking. However, these "hidden" refs appear during regular 'git log' queries.
A similar idea to use "hidden" refs is under consideration for core Git [2].
Add the 'log.excludeDecoration' config option so users can exclude some refs from decorations by default instead of needing to use --decorate-refs-exclude manually. The config value is multi-valued much like the command-line option.
There are several tests in
t4202-log.shthat test the --decorate-refs-(include|exclude) options, so these are extended. Since the expected output is already stored as a file, simply add new calls that replace a "--decorate-refs-exclude" option with an in-line config setting.[1] https://github.com/microsoft/scalar
[2] https://lore.kernel.org/git/77b1da5d3063a2404cd750adfe3bb8be9b6c497d.1585946894.git.gitgitgadget@gmail.com/Signed-off-by Derrick Stolee <[email protected]>---
log: add log.excludeDecoration config optionThis was something hinted at in the "fetch" step of the background maintenance RFC. Should be a relatively minor addition to our config options.
We definitely want this feature for microsoft/git (we would set log.excludeDecoration=refs/scalar/* in all Scalar repos), but we will wait for feedback from the community.
Thanks, -Stolee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-610%2Fderrickstolee%2Flog-exclude-decoration-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-610/derrickstolee/log-exclude-decoration-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/610Documentation/config/log.txt | 5 +++++ builtin/log.c | 14 ++++++++++++++ t/t4202-log.sh | 22 ++++++++++++++++++++++ 3 files changed, 41 insertions(+)Documentation/config/log.txt +0
-0
Documentation/config/log.txt log.decorate:: · line 25
Quoted 8 lines
Documentation/config/log.txt +5
-0@@ -18,6 +18,11 @@ log.decorate:: names are shown. This is the same as the `--decorate` option of the `git log`. +log.excludeDecoration:: + Exclude the specified patterns from the log decorations. This multi- + valued config option is the same as the `--decorate-refs-exclude` + option of `git log`. +
Thanks for this documentation update. Do you think that it would be worth updating the entry for '--decorate-refs-exclude', too? I figure that it would be good, since scripters who look at 'git log's man page before 'git config's would have a trail to follow in case they want a persistent '--decorate-refs-exclude' option.
Quoted 10 lines
Documentation/config/log.txt +0
-0log.follow:: If `true`, `git log` will act as if the `--follow` option was used when a single <path> is given. This has the same limitations as `--follow`,builtin/log.c +0
-0@@ -236,7 +236,21 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, }
builtin/log.c static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, · line 247
Quoted 10 lines
builtin/log.c +9
-0if (decoration_style) { +const struct string_list *config_exclude = +repo_config_get_value_multi (the_repository, +"log.excludeDecoration" ); + +if (config_exclude) { +struct string_list_item *item; +for (item = config_exclude->items; + item && item < config_exclude->items + config_exclude->nr; + item++)
Any reason not to use the 'for_each_string_list_item' macro in 'string-list.h' for this?
builtin/log.c static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, · line 251
Quoted 4 lines
builtin/log.c +4
-0+string_list_append (&decorate_refs_exclude, + item->string); + } +
On my first read, I thought that there might be some value in using a hash set here, since I (incorrectly) thought that only literal reference names were added here, in which case we'd benefit from a constant time lookup.
But, since we store patterns here, it's not helpful for us to have a constant time lookup, since we don't have anything to look up in the first place! I guess you could expand out all of these patterns ahead of time, but I don't think that this is universally a good thing to do. We may have a pattern like 'refs/heads/*', but we're only doing a 'git log' over a part of history that doesn't have many/any refs pointing at it, in which case expanding ahead of time was a bad thing to do.
Of course, 'string_list_append' is going to potentially introduce duplicate patterns, but I don't think that should be a huge problem, since 'ref_filter_match' stops as soon as it finds an exclude/include. I guess you could use 'string_list_insert', but it makes insertions more expensive without a clear benefit.
Quoted 56 lines
builtin/log.c +1
-0rev->show_decorations =1 ; +load_ref_decorations (&decoration_filter, decoration_style); }t/t4202-log.sh +18
-0@@ -787,6 +787,9 @@ test_expect_success 'decorate-refs-exclude with glob' ' EOF git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs-exclude="heads/octopus*" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="heads/octopus*" log \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && test_cmp expect.decorate actual '@@ -801,6 +804,9 @@ test_expect_success 'decorate-refs-exclude without globs' ' EOF git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs-exclude="tags/reach" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="tags/reach" log \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && test_cmp expect.decorate actual '@@ -816,6 +822,14 @@ test_expect_success 'multiple decorate-refs-exclude' ' git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs-exclude="heads/octopus*" \ --decorate-refs-exclude="tags/reach" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="heads/octopus*" \ + -c log.excludeDecoration="tags/reach" log \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="heads/octopus*" log \ + --decorate-refs-exclude="tags/reach" \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && test_cmp expect.decorate actual '@@ -831,6 +845,10 @@ test_expect_success 'decorate-refs and decorate-refs-exclude' ' git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs="heads/*" \ --decorate-refs-exclude="heads/oc*" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="heads/oc*" log \ + --decorate-refs="heads/*" \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && test_cmp expect.decorate actual '
t/t4202-log.sh test_expect_success 'decorate-refs-exclude and simplify-by-decoration' ' · line 873
Quoted 10 lines
t/t4202-log.sh +4
-0@@ -846,6 +864,10 @@ test_expect_success 'decorate-refs-exclude and simplify-by-decoration' ' git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs-exclude="*octopus*" \ --simplify-by-decoration >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="*octopus*" log \ + -n6 --decorate=short --pretty="tformat:%f%d" \ + --simplify-by-decoration >actual && test_cmp expect.decorate actual '
Thanks for the tests, they look good. I like your strategy of repeating the test with and without the new config settings.
Quoted 4 lines
base-commit:
274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925--
gitgitgadget
Thanks,
Taylor
Quoted 64 lines · Taylor Blau
Hi Stolee,
Quoted 61 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
In 'git log', the --decorate-refs-exclude option appends a pattern to a
string_list. This list is used to prevent showing some refs in the decoration output, or even by --simplify-by-decoration.Users may want to use their refs space to store utility refs that should not appear in the decoration output. For example, Scalar [1] runs a background fetch but places the "new" refs inside the refs/scalar/hidden/<remote>/* refspace instead of refs/<remote>/* to avoid updating remote refs when the user is not looking. However, these "hidden" refs appear during regular 'git log' queries.
A similar idea to use "hidden" refs is under consideration for core Git [2].
Add the 'log.excludeDecoration' config option so users can exclude some refs from decorations by default instead of needing to use --decorate-refs-exclude manually. The config value is multi-valued much like the command-line option.
There are several tests in
t4202-log.shthat test the --decorate-refs-(include|exclude) options, so these are extended. Since the expected output is already stored as a file, simply add new calls that replace a "--decorate-refs-exclude" option with an in-line config setting.[1] https://github.com/microsoft/scalar
[2] https://lore.kernel.org/git/77b1da5d3063a2404cd750adfe3bb8be9b6c497d.1585946894.git.gitgitgadget@gmail.com/Signed-off-by Derrick Stolee <[email protected]>---
log: add log.excludeDecoration config optionThis was something hinted at in the "fetch" step of the background maintenance RFC. Should be a relatively minor addition to our config options.
We definitely want this feature for microsoft/git (we would set log.excludeDecoration=refs/scalar/* in all Scalar repos), but we will wait for feedback from the community.
Thanks, -Stolee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-610%2Fderrickstolee%2Flog-exclude-decoration-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-610/derrickstolee/log-exclude-decoration-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/610Documentation/config/log.txt | 5 +++++ builtin/log.c | 14 ++++++++++++++ t/t4202-log.sh | 22 ++++++++++++++++++++++ 3 files changed, 41 insertions(+)Documentation/config/log.txt +0
-0@@ -18,6 +18,11 @@ log.decorate:: names are shown. This is the same as the `--decorate` option of the `git log`.
Quoted 11 lines
Documentation/config/log.txt log.decorate:: · line 25
Quoted 5 lines
Documentation/config/log.txt +5
-0+log.excludeDecoration:: + Exclude the specified patterns from the log decorations. This multi- + valued config option is the same as the `--decorate-refs-exclude` + option of `git log`. +Thanks for this documentation update. Do you think that it would be worth updating the entry for '--decorate-refs-exclude', too? I figure that it would be good, since scripters who look at 'git log's man page before 'git config's would have a trail to follow in case they want a persistent '--decorate-refs-exclude' option.
That seems like a good way to help users.
Quoted 10 lines
Quoted 10 lines
Documentation/config/log.txt +0
-0log.follow:: If `true`, `git log` will act as if the `--follow` option was used when a single <path> is given. This has the same limitations as `--follow`,builtin/log.c +0
-0@@ -236,7 +236,21 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, }
Quoted 13 lines
builtin/log.c static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, · line 247
Quoted 10 lines
builtin/log.c +9
-0if (decoration_style) { +const struct string_list *config_exclude = +repo_config_get_value_multi (the_repository, +"log.excludeDecoration" ); + +if (config_exclude) { +struct string_list_item *item; +for (item = config_exclude->items; + item && item < config_exclude->items + config_exclude->nr; + item++)Any reason not to use the '
for_each_string_list_item' macro in 'string-list.h' for this?
The reason is I forgot about it.
Thanks,
-Stolee
Quoted 24 lines · Derrick Stolee
Quoted 10 lines
Quoted 10 lines
log.follow::
Documentation/config/log.txt +0
-0If `true`, `git log` will act as if the `--follow` option was used when a single <path> is given. This has the same limitations as `--follow`,builtin/log.c +0
-0@@ -236,7 +236,21 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, }Quoted 13 lines
builtin/log.c static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, · line 247
Quoted 10 lines
builtin/log.c +9
-0if (decoration_style) { +const struct string_list *config_exclude = +repo_config_get_value_multi (the_repository, +"log.excludeDecoration" ); + +if (config_exclude) { +struct string_list_item *item; +for (item = config_exclude->items; + item && item < config_exclude->items + config_exclude->nr; + item++)Any reason not to use the '
for_each_string_list_item' macro in 'string-list.h' for this?
Quoted 1 line
The reason is I forgot about it.
Heh, in fairness I forgot about it, too :). I thought that this code looked familiar, but it was only luck that I had 'string-list.h' open at the time I was reading this.
I don't think that it really matters much each way, but if you're already re-rolling...
Quoted 2 lines
Thanks,
-Stolee
Thanks,
Taylor
Quoted 25 lines · Taylor Blau
Quoted 24 lines · Derrick Stolee
Quoted 23 lines
Quoted 10 lines
log.follow::
Documentation/config/log.txt +0
-0If `true`, `git log` will act as if the `--follow` option was used when a single <path> is given. This has the same limitations as `--follow`,builtin/log.c +0
-0@@ -236,7 +236,21 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, }builtin/log.c static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, · line 247
Quoted 10 lines
builtin/log.c +9
-0if (decoration_style) { +const struct string_list *config_exclude = +repo_config_get_value_multi (the_repository, +"log.excludeDecoration" ); + +if (config_exclude) { +struct string_list_item *item; +for (item = config_exclude->items; + item && item < config_exclude->items + config_exclude->nr; + item++)Any reason not to use the '
for_each_string_list_item' macro in 'string-list.h' for this?
Quoted 8 lines
Quoted 1 line
The reason is I forgot about it.
Heh, in fairness I forgot about it, too :). I thought that this code looked familiar, but it was only luck that I had '
string-list.h' open at the time I was reading this.I don't think that it really matters much each way, but if you're already re-rolling...
This is probably worth a re-roll on its own. I'll give the patch some extra time to simmer before pushing a v2, though.
-Stolee
Quoted 8 lines · Derrick Stolee via GitGitGadget
builtin/log.c +7
-0if (decoration_style) { +const struct string_list *config_exclude = +repo_config_get_value_multi (the_repository, +"log.excludeDecoration" ); + +if (config_exclude) { +struct string_list_item *item; + for (item = config_exclude->items;
builtin/log.c static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, · line 255
Quoted 10 lines
builtin/log.c +7
-0+ item && item < config_exclude->items + config_exclude->nr; + item++) +string_list_append (&decorate_refs_exclude, + item->string); + } + rev->show_decorations =1 ; +load_ref_decorations (&decoration_filter, decoration_style); }
A few random thoughts. Unlike my other usual reviews, please do not take "should we do X" as a suggestion (these are purely me wondering and nothing more at this point):
* Given that we have command line options to specify what patterns to include as well as to exclude, it feels somewhat asymmetric to have only the configuration to exclude. Should we also have a configuration for including?
* The new code only adds to decorate_refs_exclude, which has the patterns that were given with the "--decorate-refs-exclude" command line option. As refs.c:ref_filter_match() rejects anything that match an exclude pattern first before looking at the include patterns, there is no way to countermand what is configured to be excluded with the configuration from the command line, even with --decorate-refs" option. Should we have a new command line option to "clear" the exclude list read from the configuration? And if we add configuration for including for symmetry, should that be cleared as well?
* As this is a multi-valued configuration, there probably are cases where you have configured three patterns, and for this single invocation you would want to override only one of them. It might not be usable if the only way to override were to "clear" with a new option and then add two that you want from the command line.
What if we had (configured) exclusion for X, Y and Z, and then allowed the command line to say "include Y", that would result in the combination to specify exclusion of X and Z only? Can we get away by not having "include these" configuration at all, perhaps, because "if there is no inclusion pattern, anything that does not match exclusion patterns is included" is how the matcher works?
I guess the last one, despite what I said upfront, is the beginning of my suggestion. If we take the quoted change as-is, and then before load_ref_decorations() uses the decoration_filter, perhaps we can see for each pattern in the "exclude" list, if there is the same entry in the "include" list, and remove it from both lists. That way, when the users wonder why their "git log" does not use certain refs to decorate (let's say, you configured "refs/heads/*" in the exclusion list), they can countermand by giving "--decorate-refs" from the command line, perhaps? It is still unclear to me how well such a scheme works, e.g. how should patterns "refs/tags/*" and "refs/tags/*-rc*" interact when they are given as configs and options to include/exclude in various permutations, though.
Thanks.
Quoted 25 lines · Junio C Hamano
Quoted 8 lines · Derrick Stolee via GitGitGadget
builtin/log.c +7
-0if (decoration_style) { +const struct string_list *config_exclude = +repo_config_get_value_multi (the_repository, +"log.excludeDecoration" ); + +if (config_exclude) { +struct string_list_item *item; + for (item = config_exclude->items;builtin/log.c static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, · line 255
Quoted 10 lines
builtin/log.c +7
-0+ item && item < config_exclude->items + config_exclude->nr; + item++) +string_list_append (&decorate_refs_exclude, + item->string); + } + rev->show_decorations =1 ; +load_ref_decorations (&decoration_filter, decoration_style); }A few random thoughts. Unlike my other usual reviews, please do not take "should we do X" as a suggestion (these are purely me wondering and nothing more at this point):
Quoted 4 lines
* Given that we have command line options to specify what patterns to include as well as to exclude, it feels somewhat asymmetric to have only the configuration to exclude. Should we also have a configuration for including?
I left the other side out for simplicity and because I didn't know the use case. It seems all refs are included by default.
Quoted 17 lines
* The new code only adds to
decorate_refs_exclude, which has the patterns that were given with the "--decorate-refs-exclude" command line option. As refs.c:ref_filter_match() rejects anything that match an exclude pattern first before looking at the include patterns, there is no way to countermand what is configured to be excluded with the configuration from the command line, even with --decorate-refs" option. Should we have a new command line option to "clear" the exclude list read from the configuration? And if we add configuration for including for symmetry, should that be cleared as well?* As this is a multi-valued configuration, there probably are cases where you have configured three patterns, and for this single invocation you would want to override only one of them. It might not be usable if the only way to override were to "clear" with a new option and then add two that you want from the command line.
Quoted 6 lines
What if we had (configured) exclusion for X, Y and Z, and then allowed the command line to say "include Y", that would result in the combination to specify exclusion of X and Z only? Can we get away by not having "include these" configuration at all, perhaps, because "if there is no inclusion pattern, anything that does not match exclusion patterns is included" is how the matcher works?
This is a very good point. We should be able to use command-line options to override configured values. Something like this should show decorations for refs/hidden/origin/master:
git -c log.excludeDecoration=refs/hidden/* log --decorate-refs=refs/hidden/*
But, the current patch does not.
Quoted 4 lines
I guess the last one, despite what I said upfront, is the beginning of my suggestion. If we take the quoted change as-is, and then before
load_ref_decorations()uses thedecoration_filter, perhaps we can see for each pattern in the "exclude" list, if there is the same
Quoted 8 lines
entry in the "include" list, and remove it from both lists. That way, when the users wonder why their "git log" does not use certain refs to decorate (let's say, you configured "refs/heads/*" in the exclusion list), they can countermand by giving "--decorate-refs" from the command line, perhaps? It is still unclear to me how well such a scheme works, e.g. how should patterns "refs/tags/*" and "refs/tags/*-rc*" interact when they are given as configs and options to
include/excludein various permutations, though.
My next version will allow this "overwrite" of configured values. It seems like an important use case that I had missed.
Without getting into the code immediately, I predict the change will be to include a second pass of "configured patterns" after the command-line patterns. If the explicit command-line patterns have already included the ref, then the configured exclude patterns should not be tested.
Thanks!
-Stolee
Quoted 7 lines · Derrick Stolee
Quoted 4 lines
* Given that we have command line options to specify what patterns to include as well as to exclude, it feels somewhat asymmetric to have only the configuration to exclude. Should we also have a configuration for including?
I left the other side out for simplicity and because I didn't know the use case. It seems all refs are included by default.
It is a bit more subtle than that.
Once you have an inclusion pattern, nothing is included by default. Only the ones that match an inclusion pattern would be considered. When there is no inclusion pattern, we behave as if there is a match-all inclusion pattern.
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Derrick Stolee
Quoted 4 lines
* Given that we have command line options to specify what patterns to include as well as to exclude, it feels somewhat asymmetric to have only the configuration to exclude. Should we also have a configuration for including?
I left the other side out for simplicity and because I didn't know the use case. It seems all refs are included by default.
Quoted 6 lines
It is a bit more subtle than that.
Once you have an inclusion pattern, nothing is included by default. Only the ones that match an inclusion pattern would be considered. When there is no inclusion pattern, we behave as if there is a match-all inclusion pattern.
I did a quick check to verify how things currently work with this diff:
t/t4202-log.sh +16 -0
@@ -742,8 +742,24 @@ test_expect_success 'decorate-refs with glob' ' octopus-a (octopus-a) reach EOF + cat >expect.no-decorate <<-\EOF && + Merge-tag-reach + Merge-tags-octopus-a-and-octopus-b + seventh + octopus-b + octopus-a + reach + EOF git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs="heads/octopus*" >actual && + test_cmp expect.decorate actual && + git log -n6 --decorate=short --pretty="tformat:%f%d" \ + --decorate-refs-exclude="heads/octopus*" \ + --decorate-refs="heads/octopus*" >actual && + test_cmp expect.no-decorate actual && + git -c log.excludeDecoration="heads/octopus*" log \ + -n6 --decorate=short --pretty="tformat:%f%d" \ + --decorate-refs="heads/octopus*" >actual && test_cmp expect.decorate actual '
This test fails at the last test_cmp with the current patch.
Note that if we have both --decorate-refs-exclude=X and --decorate-refs=X, then the exclusion wins. This means that we will need to split the "configured" exclusions from the "command-line" exclusions and give them different priority.
But, I believe that if we can get this test to pass, then we will have the correct inclusion/exclusion logic. I will get started on this right now.
Thanks,
-Stolee
PATCH v2 1/1 log: add log.excludeDecoration config option
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
In 'git log', the --decorate-refs-exclude option appends a pattern to a string_list. This list is used to prevent showing some refs in the decoration output, or even by --simplify-by-decoration.
Users may want to use their refs space to store utility refs that should not appear in the decoration output. For example, Scalar [1] runs a background fetch but places the "new" refs inside the refs/scalar/hidden/<remote>/* refspace instead of refs/<remote>/* to avoid updating remote refs when the user is not looking. However, these "hidden" refs appear during regular 'git log' queries.
A similar idea to use "hidden" refs is under consideration for core Git [2].
Add the 'log.excludeDecoration' config option so users can exclude some refs from decorations by default instead of needing to use --decorate-refs-exclude manually. The config value is multi-valued much like the command-line option. The documentation is careful to point out that the config value can be overridden by the --decorate-refs option, even though --decorate-refs-exclude would always "win" over --decorate-refs.
Since the 'log.excludeDecoration' takes lower precedence to --decorate-refs, and --decorate-refs-exclude takes higher precedence, the struct decoration_filter needed another field. This led also to new logic in load_ref_decorations() and ref_filter_match().
There are several tests in t4202-log.sh that test the --decorate-refs-(include|exclude) options, so these are extended. Since the expected output is already stored as a file, most tests could simply replace a "--decorate-refs-exclude" option with an in-line config setting. Other tests involve the precedence of the config option compared to command-line options and needed more modification.
[1] https://github.com/microsoft/scalar
[2] https://lore.kernel.org/git/77b1da5d3063a2404cd750adfe3bb8be9b6c497d.1585946894.git.gitgitgadget@gmail.com/
---
log: add log.excludeDecoration config option
This was something hinted at in the "fetch" step of the background maintenance RFC. Should be a relatively minor addition to our config options.
We definitely want this feature for microsoft/git (we would set log.excludeDecoration=refs/scalar/* in all Scalar repos), but we will wait for feedback from the community.
Updates in v2:
* Use for_each_string_list_item()
* Update the matching logic to allow --decorate-refs to override the config option.
Thanks, -Stolee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-610%2Fderrickstolee%2Flog-exclude-decoration-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-610/derrickstolee/log-exclude-decoration-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/610
Range-diff vs v1:
1: 6aa81243c1f ! 1: cbdaef4a8e1 log: add log.excludeDecoration config option
@@ Commit message
Add the 'log.excludeDecoration' config option so users can exclude some refs from decorations by default instead of needing to use --decorate-refs-exclude manually. The config value is multi-valued
- much like the command-line option.
+ much like the command-line option. The documentation is careful to
+ point out that the config value can be overridden by the
+ --decorate-refs option, even though --decorate-refs-exclude would
+ always "win" over --decorate-refs.
+
+ Since the 'log.excludeDecoration' takes lower precedence to
+ --decorate-refs, and --decorate-refs-exclude takes higher
+ precedence, the struct decoration_filter needed another field.
+ This led also to new logic in load_ref_decorations() and
+ ref_filter_match().
There are several tests in t4202-log.sh that test the --decorate-refs-(include|exclude) options, so these are extended.
- Since the expected output is already stored as a file, simply add
- new calls that replace a "--decorate-refs-exclude" option with an
- in-line config setting.
+ Since the expected output is already stored as a file, most tests
+ could simply replace a "--decorate-refs-exclude" option with an
+ in-line config setting. Other tests involve the precedence of
+ the config option compared to command-line options and needed more
+ modification.
[1] https://github.com/microsoft/scalar
[2] https://lore.kernel.org/git/77b1da5d3063a2404cd750adfe3bb8be9b6c497d.1585946894.git.gitgitgadget@gmail.com/
@@ Documentation/config/log.txt: log.decorate::
If true, git log will act as if the --follow option was used when
a single <path> is given. This has the same limitations as --follow,
+ ## Documentation/git-log.txt ##
+@@ Documentation/git-log.txt: OPTIONS
+ If no --decorate-refs is given, pretend as if all refs were
+ included. For each candidate, do not use it for decoration if it
+ matches any patterns given to --decorate-refs-exclude or if it
+- doesn't match any of the patterns given to --decorate-refs.
++ doesn't match any of the patterns given to --decorate-refs. The
++ log.excludeDecoration config option allows excluding refs from
++ the decorations, but an explicit --decorate-refs pattern will
++ override a match in log.excludeDecoration.
+
+ --source::
+ Print out the ref name given on the command line by which each
+
## builtin/log.c ##
+@@ builtin/log.c: static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
+ int quiet = 0, source = 0, mailmap;
+ static struct line_opt_callback_data line_cb = {NULL, NULL, STRING_LIST_INIT_DUP};
+ static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
++ static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
+ static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
+ struct decoration_filter decoration_filter = {&decorate_refs_include,
+- &decorate_refs_exclude};
++ &decorate_refs_exclude,
++ &decorate_refs_exclude_config};
+ static struct revision_sources revision_sources;
+
+ const struct option builtin_log_options[] = {
@@ builtin/log.c: static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
}
@@ builtin/log.c: static void cmd_log_init_finish(int argc, const char **argv, cons
+
+ if (config_exclude) {
+ struct string_list_item *item;
-+ for (item = config_exclude->items;
-+ item && item < config_exclude->items + config_exclude->nr;
-+ item++)
-+ string_list_append(&decorate_refs_exclude,
-+ item->string);
++ for_each_string_list_item(item, config_exclude)
++ string_list_append(&decorate_refs_exclude_config,
++ item->string);
+ }
+
rev->show_decorations = 1;
@@ builtin/log.c: static void cmd_log_init_finish(int argc, const char **argv, cons
}
+ ## log-tree.c ##
+@@ log-tree.c: static int add_ref_decoration(const char *refname, const struct object_id *oid, +
+ if (filter && !ref_filter_match(refname,
+ filter->include_ref_pattern,
+- filter->exclude_ref_pattern))
++ filter->exclude_ref_pattern,
++ filter->exclude_ref_config_pattern))
+ return 0;
+
+ if (starts_with(refname, git_replace_ref_base)) {
+@@ log-tree.c: void load_ref_decorations(struct decoration_filter *filter, int flags)
+ for_each_string_list_item(item, filter->include_ref_pattern) {
+ normalize_glob_ref(item, NULL, item->string);
+ }
++ for_each_string_list_item(item, filter->exclude_ref_config_pattern) {
++ normalize_glob_ref(item, NULL, item->string);
++ }
+ }
+ decoration_loaded = 1;
+ decoration_flags = flags;
+
+ ## log-tree.h ##
+@@ log-tree.h: struct log_info {
+ };
+
+ struct decoration_filter {
+- struct string_list *include_ref_pattern, *exclude_ref_pattern;
++ struct string_list *include_ref_pattern;
++ struct string_list *exclude_ref_pattern;
++ struct string_list *exclude_ref_config_pattern;
+ };
+
+ int parse_decorate_color_config(const char *var, const char *slot_name, const char *value); +
+ ## refs.c ##
+@@ refs.c: static int match_ref_pattern(const char *refname,
+
+ int ref_filter_match(const char *refname,
+ const struct string_list *include_patterns,
+- const struct string_list *exclude_patterns)
++ const struct string_list *exclude_patterns,
++ const struct string_list *exclude_patterns_config)
+ {
+ struct string_list_item *item;
++ int found = 0;
+
+ if (exclude_patterns && exclude_patterns->nr) {
+ for_each_string_list_item(item, exclude_patterns) {
+@@ refs.c: int ref_filter_match(const char *refname,
+ }
+
+ if (include_patterns && include_patterns->nr) {
+- int found = 0;
+ for_each_string_list_item(item, include_patterns) {
+ if (match_ref_pattern(refname, item)) {
+ found = 1;
+@@ refs.c: int ref_filter_match(const char *refname,
+ if (!found)
+ return 0;
+ }
++
++ if (!found &&
++ exclude_patterns_config &&
++ exclude_patterns_config->nr) {
++ for_each_string_list_item(item, exclude_patterns_config) {
++ if (match_ref_pattern(refname, item))
++ return 0;
++ }
++ }
++
+ return 1;
+ }
+
+
+ ## refs.h ##
+@@ refs.h: void normalize_glob_ref(struct string_list_item *item, const char *prefix,
+ const char *pattern);
+
+ /*
+- * Returns 0 if refname matches any of the exclude_patterns, or if it doesn't +- * match any of the include_patterns. Returns 1 otherwise.
++ * Returns 0 if the refname matches any of the exclude_patterns.
++ *
++ * Returns 0 if include_patterns is non-empty but refname does not match
++ * any of those patterns.
++ *
++ * Returns 0 if refname matches a pattern in exclude_patterns_config but
++ * does not match any pattern in inclue_patterns.
++ *
++ * Otherwise, returns 1.
+ *
+- * If pattern list is NULL or empty, matching against that list is skipped.
+ * This has the effect of matching everything by default, unless the user
+ * specifies rules otherwise.
+ */
+ int ref_filter_match(const char *refname,
+ const struct string_list *include_patterns,
+- const struct string_list *exclude_patterns);
++ const struct string_list *exclude_patterns,
++ const struct string_list *exclude_patterns_config);
+
+ static inline const char *has_glob_specials(const char *pattern)
+ {
+
## t/t4202-log.sh ##
+@@ t/t4202-log.sh: test_expect_success 'decorate-refs with glob' '
+ octopus-a (octopus-a)
+ reach
+ EOF
++ cat >expect.no-decorate <<-\EOF &&
++ Merge-tag-reach
++ Merge-tags-octopus-a-and-octopus-b
++ seventh
++ octopus-b
++ octopus-a
++ reach
++ EOF
++ git log -n6 --decorate=short --pretty="tformat:%f%d" \
++ --decorate-refs="heads/octopus*" >actual &&
++ test_cmp expect.decorate actual &&
+ git log -n6 --decorate=short --pretty="tformat:%f%d" \
++ --decorate-refs-exclude="heads/octopus*" \
++ --decorate-refs="heads/octopus*" >actual &&
++ test_cmp expect.no-decorate actual &&
++ git -c log.excludeDecoration="heads/octopus*" log \
++ -n6 --decorate=short --pretty="tformat:%f%d" \
+ --decorate-refs="heads/octopus*" >actual &&
+ test_cmp expect.decorate actual
+ '
@@ t/t4202-log.sh: test_expect_success 'decorate-refs-exclude with glob' '
EOF
git log -n6 --decorate=short --pretty="tformat:%f%d" \
@@ t/t4202-log.sh: test_expect_success 'multiple decorate-refs-exclude' '
test_cmp expect.decorate actual
'
+ test_expect_success 'decorate-refs and decorate-refs-exclude' '
+- cat >expect.decorate <<-\EOF &&
++ cat >expect.no-decorate <<-\EOF &&
+ Merge-tag-reach (master)
+ Merge-tags-octopus-a-and-octopus-b
+ seventh
@@ t/t4202-log.sh: test_expect_success 'decorate-refs and decorate-refs-exclude' '
git log -n6 --decorate=short --pretty="tformat:%f%d" \
--decorate-refs="heads/*" \
--decorate-refs-exclude="heads/oc*" >actual &&
-+ test_cmp expect.decorate actual &&
++ test_cmp expect.no-decorate actual
++'
++
++test_expect_success 'deocrate-refs and log.excludeDecoration' '
++ cat >expect.decorate <<-\EOF &&
++ Merge-tag-reach (master)
++ Merge-tags-octopus-a-and-octopus-b
++ seventh
++ octopus-b (octopus-b)
++ octopus-a (octopus-a)
++ reach (reach)
++ EOF
+ git -c log.excludeDecoration="heads/oc*" log \
+ --decorate-refs="heads/*" \
+ -n6 --decorate=short --pretty="tformat:%f%d" >actual &&
Documentation/config/log.txt | 5 ++++ Documentation/git-log.txt | 5 +++- builtin/log.c | 16 ++++++++++- log-tree.c | 6 ++++- log-tree.h | 4 ++- refs.c | 15 +++++++++-- refs.h | 15 ++++++++--- t/t4202-log.sh | 51 +++++++++++++++++++++++++++++++++++- 8 files changed, 106 insertions(+), 11 deletions(-)
Documentation/config/log.txt +5 -0
@@ -18,6 +18,11 @@ log.decorate:: names are shown. This is the same as the `--decorate` option of the `git log`. +log.excludeDecoration:: + Exclude the specified patterns from the log decorations. This multi- + valued config option is the same as the `--decorate-refs-exclude` + option of `git log`. + log.follow:: If `true`, `git log` will act as if the `--follow` option was used when a single <path> is given. This has the same limitations as `--follow`,
Documentation/git-log.txt +4 -1
@@ -43,7 +43,10 @@ OPTIONS If no `--decorate-refs` is given, pretend as if all refs were included. For each candidate, do not use it for decoration if it matches any patterns given to `--decorate-refs-exclude` or if it - doesn't match any of the patterns given to `--decorate-refs`. + doesn't match any of the patterns given to `--decorate-refs`. The + `log.excludeDecoration` config option allows excluding refs from + the decorations, but an explicit `--decorate-refs` pattern will + override a match in `log.excludeDecoration`. --source:: Print out the ref name given on the command line by which each
builtin/log.c +15 -1
@@ -164,9 +164,11 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,int quiet =0 , source =0 , mailmap;static struct line_opt_callback_data line_cb = {NULL ,NULL ,STRING_LIST_INIT_DUP };static struct string_list decorate_refs_exclude =STRING_LIST_INIT_NODUP ; +static struct string_list decorate_refs_exclude_config =STRING_LIST_INIT_NODUP ;static struct string_list decorate_refs_include =STRING_LIST_INIT_NODUP ;struct decoration_filter decoration_filter = {&decorate_refs_include, - &decorate_refs_exclude}; + &decorate_refs_exclude, + &decorate_refs_exclude_config};static struct revision_sources revision_sources;const struct option builtin_log_options[] = {
@@ -236,7 +238,19 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, }if (decoration_style) { +const struct string_list *config_exclude = +repo_config_get_value_multi (the_repository, +"log.excludeDecoration" ); + +if (config_exclude) { +struct string_list_item *item; +for_each_string_list_item (item, config_exclude) +string_list_append (&decorate_refs_exclude_config, + item->string); + } + rev->show_decorations =1 ; +load_ref_decorations (&decoration_filter, decoration_style); }
log-tree.c +5 -1
@@ -90,7 +90,8 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,if (filter && !ref_filter_match (refname, filter->include_ref_pattern, - filter->exclude_ref_pattern)) + filter->exclude_ref_pattern, + filter->exclude_ref_config_pattern))return 0 ;if (starts_with (refname, git_replace_ref_base)) {
@@ -155,6 +156,9 @@ void load_ref_decorations(struct decoration_filter *filter, int flags)for_each_string_list_item (item, filter->include_ref_pattern) {normalize_glob_ref (item,NULL , item->string); } +for_each_string_list_item (item, filter->exclude_ref_config_pattern) { +normalize_glob_ref (item,NULL , item->string); + } } decoration_loaded =1 ; decoration_flags = flags;
log-tree.h +3 -1
@@ -8,7 +8,9 @@ struct log_info { };struct decoration_filter { -struct string_list *include_ref_pattern, *exclude_ref_pattern; +struct string_list *include_ref_pattern; +struct string_list *exclude_ref_pattern; +struct string_list *exclude_ref_config_pattern; };int parse_decorate_color_config (const char *var,const char *slot_name,const char *value);
refs.c +13 -2
@@ -339,9 +339,11 @@ static int match_ref_pattern(const char *refname,int ref_filter_match (const char *refname,const struct string_list *include_patterns, -const struct string_list *exclude_patterns) +const struct string_list *exclude_patterns, +const struct string_list *exclude_patterns_config) {struct string_list_item *item; +int found =0 ;if (exclude_patterns && exclude_patterns->nr) {for_each_string_list_item (item, exclude_patterns) {
@@ -351,7 +353,6 @@ int ref_filter_match(const char *refname, }if (include_patterns && include_patterns->nr) { -int found =0 ;for_each_string_list_item (item, include_patterns) {if (match_ref_pattern (refname, item)) { found =1 ;
@@ -362,6 +363,16 @@ int ref_filter_match(const char *refname,if (!found)return 0 ; } + +if (!found && + exclude_patterns_config && + exclude_patterns_config->nr) { +for_each_string_list_item (item, exclude_patterns_config) { +if (match_ref_pattern (refname, item)) +return 0 ; + } + } +return 1 ; }
refs.h +11 -4
@@ -362,16 +362,23 @@ void normalize_glob_ref(struct string_list_item *item, const char *prefix, const char *pattern);/* -* Returns 0 if refname matches any of the exclude_patterns, or if it doesn't -* match any of the include_patterns. Returns 1 otherwise. +* Returns 0 if the refname matches any of the exclude_patterns. +* +* Returns 0 if include_patterns is non-empty but refname does not match +* any of those patterns. +* +* Returns 0 if refname matches a pattern in exclude_patterns_config but +* does not match any pattern in inclue_patterns. +* +* Otherwise, returns 1. * -* If pattern list is NULL or empty, matching against that list is skipped. * This has the effect of matching everything by default, unless the user * specifies rules otherwise. */ int ref_filter_match (const char *refname,const struct string_list *include_patterns, -const struct string_list *exclude_patterns); +const struct string_list *exclude_patterns, +const struct string_list *exclude_patterns_config);static inline const char *has_glob_specials (const char *pattern) {
t/t4202-log.sh +50 -1
92 lines not on this page · raw message
base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925
Signature · 1 line
gitgitgadget
Hi Stolee,
Quoted 58 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
In 'git log', the --decorate-refs-exclude option appends a pattern to a
string_list. This list is used to prevent showing some refs in the decoration output, or even by --simplify-by-decoration.Users may want to use their refs space to store utility refs that should not appear in the decoration output. For example, Scalar [1] runs a background fetch but places the "new" refs inside the refs/scalar/hidden/<remote>/* refspace instead of refs/<remote>/* to avoid updating remote refs when the user is not looking. However, these "hidden" refs appear during regular 'git log' queries.
A similar idea to use "hidden" refs is under consideration for core Git [2].
Add the 'log.excludeDecoration' config option so users can exclude some refs from decorations by default instead of needing to use --decorate-refs-exclude manually. The config value is multi-valued much like the command-line option. The documentation is careful to point out that the config value can be overridden by the --decorate-refs option, even though --decorate-refs-exclude would always "win" over --decorate-refs.
Since the 'log.excludeDecoration' takes lower precedence to --decorate-refs, and --decorate-refs-exclude takes higher precedence, the struct
decoration_filterneeded another field. This led also to new logic inload_ref_decorations()andref_filter_match().There are several tests in
t4202-log.shthat test the --decorate-refs-(include|exclude) options, so these are extended. Since the expected output is already stored as a file, most tests could simply replace a "--decorate-refs-exclude" option with an in-line config setting. Other tests involve the precedence of the config option compared to command-line options and needed more modification.[1] https://github.com/microsoft/scalar
[2] https://lore.kernel.org/git/77b1da5d3063a2404cd750adfe3bb8be9b6c497d.1585946894.git.gitgitgadget@gmail.com/Signed-off-by Derrick Stolee <[email protected]>---
log: add log.excludeDecoration config optionThis was something hinted at in the "fetch" step of the background maintenance RFC. Should be a relatively minor addition to our config options.
We definitely want this feature for microsoft/git (we would set log.excludeDecoration=refs/scalar/* in all Scalar repos), but we will wait for feedback from the community.
Updates in v2:
* Use
for_each_string_list_item()
Quoted 6 lines
* Update the matching logic to allow --decorate-refs to override the config option.
Thanks, -Stolee
Thanks for a v2, Stolee.
Quoted 21 lines
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-610%2Fderrickstolee%2Flog-exclude-decoration-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-610/derrickstolee/log-exclude-decoration-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/610[ snip range-diff ... ]
Documentation/config/log.txt | 5 ++++ Documentation/git-log.txt | 5 +++- builtin/log.c | 16 ++++++++++- log-tree.c | 6 ++++- log-tree.h | 4 ++- refs.c | 15 +++++++++-- refs.h | 15 ++++++++--- t/t4202-log.sh | 51 +++++++++++++++++++++++++++++++++++- 8 files changed, 106 insertions(+), 11 deletions(-)Documentation/config/log.txt +0
-0
Documentation/config/log.txt log.decorate:: · line 25
Quoted 8 lines
Documentation/config/log.txt +5
-0@@ -18,6 +18,11 @@ log.decorate:: names are shown. This is the same as the `--decorate` option of the `git log`. +log.excludeDecoration:: + Exclude the specified patterns from the log decorations. This multi- + valued config option is the same as the `--decorate-refs-exclude` + option of `git log`. +
Good, I'm glad that you didn't change this from v1, since I figure the interaction with 'git log' should be explained in 'git log's man page, not this one. Nice.
Quoted 272 lines
Documentation/config/log.txt +0
-0log.follow:: If `true`, `git log` will act as if the `--follow` option was used when a single <path> is given. This has the same limitations as `--follow`,Documentation/git-log.txt +4
-1@@ -43,7 +43,10 @@ OPTIONS If no `--decorate-refs` is given, pretend as if all refs were included. For each candidate, do not use it for decoration if it matches any patterns given to `--decorate-refs-exclude` or if it - doesn't match any of the patterns given to `--decorate-refs`. + doesn't match any of the patterns given to `--decorate-refs`. The + `log.excludeDecoration` config option allows excluding refs from + the decorations, but an explicit `--decorate-refs` pattern will + override a match in `log.excludeDecoration`. --source:: Print out the ref name given on the command line by which eachbuiltin/log.c +15
-1@@ -164,9 +164,11 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,int quiet =0 , source =0 , mailmap;static struct line_opt_callback_data line_cb = {NULL ,NULL ,STRING_LIST_INIT_DUP };static struct string_list decorate_refs_exclude =STRING_LIST_INIT_NODUP ; +static struct string_list decorate_refs_exclude_config =STRING_LIST_INIT_NODUP ;static struct string_list decorate_refs_include =STRING_LIST_INIT_NODUP ;struct decoration_filter decoration_filter = {&decorate_refs_include, - &decorate_refs_exclude}; + &decorate_refs_exclude, + &decorate_refs_exclude_config};static struct revision_sources revision_sources;const struct option builtin_log_options[] = {@@ -236,7 +238,19 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, }if (decoration_style) { +const struct string_list *config_exclude = +repo_config_get_value_multi (the_repository, +"log.excludeDecoration" ); + +if (config_exclude) { +struct string_list_item *item; +for_each_string_list_item (item, config_exclude) +string_list_append (&decorate_refs_exclude_config, + item->string); + } + rev->show_decorations =1 ; +load_ref_decorations (&decoration_filter, decoration_style); }log-tree.c +5
-1@@ -90,7 +90,8 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,if (filter && !ref_filter_match (refname, filter->include_ref_pattern, - filter->exclude_ref_pattern)) + filter->exclude_ref_pattern, + filter->exclude_ref_config_pattern))return 0 ;if (starts_with (refname, git_replace_ref_base)) {@@ -155,6 +156,9 @@ void load_ref_decorations(struct decoration_filter *filter, int flags)for_each_string_list_item (item, filter->include_ref_pattern) {normalize_glob_ref (item,NULL , item->string); } +for_each_string_list_item (item, filter->exclude_ref_config_pattern) { +normalize_glob_ref (item,NULL , item->string); + } } decoration_loaded =1 ; decoration_flags = flags;log-tree.h +3
-1@@ -8,7 +8,9 @@ struct log_info { };struct decoration_filter { -struct string_list *include_ref_pattern, *exclude_ref_pattern; +struct string_list *include_ref_pattern; +struct string_list *exclude_ref_pattern; +struct string_list *exclude_ref_config_pattern; };int parse_decorate_color_config (const char *var,const char *slot_name,const char *value);refs.c +13
-2@@ -339,9 +339,11 @@ static int match_ref_pattern(const char *refname,int ref_filter_match (const char *refname,const struct string_list *include_patterns, -const struct string_list *exclude_patterns) +const struct string_list *exclude_patterns, +const struct string_list *exclude_patterns_config) {struct string_list_item *item; +int found =0 ;if (exclude_patterns && exclude_patterns->nr) {for_each_string_list_item (item, exclude_patterns) {@@ -351,7 +353,6 @@ int ref_filter_match(const char *refname, }if (include_patterns && include_patterns->nr) { -int found =0 ;for_each_string_list_item (item, include_patterns) {if (match_ref_pattern (refname, item)) { found =1 ;@@ -362,6 +363,16 @@ int ref_filter_match(const char *refname,if (!found)return 0 ; } + +if (!found && + exclude_patterns_config && + exclude_patterns_config->nr) { +for_each_string_list_item (item, exclude_patterns_config) { +if (match_ref_pattern (refname, item)) +return 0 ; + } + } +return 1 ; }refs.h +11
-4@@ -362,16 +362,23 @@ void normalize_glob_ref(struct string_list_item *item, const char *prefix, const char *pattern);/* -* Returns 0 if refname matches any of the exclude_patterns, or if it doesn't -* match any of the include_patterns. Returns 1 otherwise. +* Returns 0 if the refname matches any of the exclude_patterns. +* +* Returns 0 if include_patterns is non-empty but refname does not match +* any of those patterns. +* +* Returns 0 if refname matches a pattern in exclude_patterns_config but +* does not match any pattern in inclue_patterns. +* +* Otherwise, returns 1. * -* If pattern list is NULL or empty, matching against that list is skipped. * This has the effect of matching everything by default, unless the user * specifies rules otherwise. */ int ref_filter_match (const char *refname,const struct string_list *include_patterns, -const struct string_list *exclude_patterns); +const struct string_list *exclude_patterns, +const struct string_list *exclude_patterns_config);static inline const char *has_glob_specials (const char *pattern) {t/t4202-log.sh +50
-1@@ -742,7 +742,23 @@ test_expect_success 'decorate-refs with glob' ' octopus-a (octopus-a) reach EOF + cat >expect.no-decorate <<-\EOF && + Merge-tag-reach + Merge-tags-octopus-a-and-octopus-b + seventh + octopus-b + octopus-a + reach + EOF + git log -n6 --decorate=short --pretty="tformat:%f%d" \ + --decorate-refs="heads/octopus*" >actual && + test_cmp expect.decorate actual && git log -n6 --decorate=short --pretty="tformat:%f%d" \ + --decorate-refs-exclude="heads/octopus*" \ + --decorate-refs="heads/octopus*" >actual && + test_cmp expect.no-decorate actual && + git -c log.excludeDecoration="heads/octopus*" log \ + -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs="heads/octopus*" >actual && test_cmp expect.decorate actual '@@ -787,6 +803,9 @@ test_expect_success 'decorate-refs-exclude with glob' ' EOF git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs-exclude="heads/octopus*" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="heads/octopus*" log \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && test_cmp expect.decorate actual '@@ -801,6 +820,9 @@ test_expect_success 'decorate-refs-exclude without globs' ' EOF git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs-exclude="tags/reach" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="tags/reach" log \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && test_cmp expect.decorate actual '@@ -816,11 +838,19 @@ test_expect_success 'multiple decorate-refs-exclude' ' git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs-exclude="heads/octopus*" \ --decorate-refs-exclude="tags/reach" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="heads/octopus*" \ + -c log.excludeDecoration="tags/reach" log \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="heads/octopus*" log \ + --decorate-refs-exclude="tags/reach" \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && test_cmp expect.decorate actual ' test_expect_success 'decorate-refs and decorate-refs-exclude' ' - cat >expect.decorate <<-\EOF && + cat >expect.no-decorate <<-\EOF && Merge-tag-reach (master) Merge-tags-octopus-a-and-octopus-b seventh@@ -831,6 +861,21 @@ test_expect_success 'decorate-refs and decorate-refs-exclude' ' git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs="heads/*" \ --decorate-refs-exclude="heads/oc*" >actual && + test_cmp expect.no-decorate actual +' + +test_expect_success 'deocrate-refs and log.excludeDecoration' ' + cat >expect.decorate <<-\EOF && + Merge-tag-reach (master) + Merge-tags-octopus-a-and-octopus-b + seventh + octopus-b (octopus-b) + octopus-a (octopus-a) + reach (reach) + EOF + git -c log.excludeDecoration="heads/oc*" log \ + --decorate-refs="heads/*" \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && test_cmp expect.decorate actual '@@ -846,6 +891,10 @@ test_expect_success 'decorate-refs-exclude and simplify-by-decoration' ' git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs-exclude="*octopus*" \ --simplify-by-decoration >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="*octopus*" log \ + -n6 --decorate=short --pretty="tformat:%f%d" \ + --simplify-by-decoration >actual && test_cmp expect.decorate actual '
Quoted 2 lines
base-commit:
274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925--
This looks good to me, thanks.
Thanks,
Taylor
Quoted 8 lines · Derrick Stolee via GitGitGadget
Add the 'log.excludeDecoration' config option so users can exclude some refs from decorations by default instead of needing to use --decorate-refs-exclude manually. The config value is multi-valued much like the command-line option. The documentation is careful to point out that the config value can be overridden by the --decorate-refs option, even though --decorate-refs-exclude would always "win" over --decorate-refs.
Quoted 5 lines
Since the 'log.excludeDecoration' takes lower precedence to --decorate-refs, and --decorate-refs-exclude takes higher precedence, the struct
decoration_filterneeded another field. This led also to new logic inload_ref_decorations()andref_filter_match().
All of the above makes sense to me.
Documentation/config/log.txt log.decorate:: · line 24
Quoted 12 lines
Documentation/config/log.txt +4
-0@@ -18,6 +18,11 @@ log.decorate:: names are shown. This is the same as the `--decorate` option of the `git log`. +log.excludeDecoration:: + Exclude the specified patterns from the log decorations. This multi- + valued config option is the same as the `--decorate-refs-exclude` + option of `git log`.
Can the config still be "the same as" that option, though, with the new "unlike --decorate-refs-exclude that always wins, config is at the lowest precedence" rule?
Documentation/git-log.txt OPTIONS · line 49
Quoted 13 lines
Documentation/git-log.txt +4
-1@@ -43,7 +43,10 @@ OPTIONS If no `--decorate-refs` is given, pretend as if all refs were included. For each candidate, do not use it for decoration if it matches any patterns given to `--decorate-refs-exclude` or if it - doesn't match any of the patterns given to `--decorate-refs`. + doesn't match any of the patterns given to `--decorate-refs`. The + `log.excludeDecoration` config option allows excluding refs from + the decorations, but an explicit `--decorate-refs` pattern will + override a match in `log.excludeDecoration`.
This description does make sense.
log-tree.c static int add_ref_decoration(const char *refname, const struct object_id *oid, · line 95
Quoted 12 lines
log-tree.c +2
-1@@ -90,7 +90,8 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,if (filter && !ref_filter_match (refname, filter->include_ref_pattern, - filter->exclude_ref_pattern)) + filter->exclude_ref_pattern, + filter->exclude_ref_config_pattern))return 0 ;
As there is only one caller of the ref_filter_match() helper, I wonder if we want to
(1) move the helper to log-tree.c, make it static and remove its definition from refs.h, and optionally rename it so that it is clear that this is not part of the "ref_filter" API that drives "for-each-ref" and friends;
(2) instead of adding yet another pattern to the parameter list, make the helper take the whole "filter" instance as a single parameter.
as a clean-up.
Quoted 28 lines
refs.c +3
-2@@ -339,9 +339,11 @@ static int match_ref_pattern(const char *refname,int ref_filter_match (const char *refname,const struct string_list *include_patterns, -const struct string_list *exclude_patterns) +const struct string_list *exclude_patterns, +const struct string_list *exclude_patterns_config) {struct string_list_item *item; +int found =0 ;if (exclude_patterns && exclude_patterns->nr) {for_each_string_list_item (item, exclude_patterns) {@@ -351,7 +353,6 @@ int ref_filter_match(const char *refname, }if (include_patterns && include_patterns->nr) { -int found =0 ;for_each_string_list_item (item, include_patterns) {if (match_ref_pattern (refname, item)) { found =1 ;@@ -362,6 +363,16 @@ int ref_filter_match(const char *refname,if (!found)return 0 ;
refs.c int ref_filter_match(const char *refname, · line 374
Quoted 10 lines
refs.c +9
-0} + +if (!found && + exclude_patterns_config && + exclude_patterns_config->nr) { +for_each_string_list_item (item, exclude_patterns_config) { +if (match_ref_pattern (refname, item)) +return 0 ; + } + }
Hmph. Do we still need "found" here? If there are include patterns given explicitly from the command line, a ref MUST match one of them in order to be included, and a ref that matches one of them will be included no matter that exclude config says.
So shouldn't the updated logic for the include patterns part be more like
if (include_patterns is not empty) {
for_each_string_list_item(item, include_patterns) {
if (matches)
return 1; /* matches */
}
return 0; /* did not match */
}Then the new code to deal with "config", which will come after the "include" thing, will only have to deal with the case where there is no include patterns given from the command line. And when the control reaches there, we need to do exactly the same thing as we do to the commad line excludes, i.e. see if any of the config patterns match, and say "reject" if we find a match.
And after the loop for the exclude config fails to "return 0" early, we will hit the "return 1" at the end.
Had I butchered the logic somehow?
Quoted 4 lines
refs.c +3
-0/* +* Returns 0 if the refname matches any of the exclude_patterns. +* +* Returns 0 if include_patterns is non-empty but refname does not match
refs.c int ref_filter_match(const char *refname,
Quoted 10 lines
refs.c +6
-0+* any of those patterns. +* +* Returns 0 if refname matches a pattern in exclude_patterns_config but +* does not match any pattern in inclue_patterns. +* +* Otherwise, returns 1. * * This has the effect of matching everything by default, unless the user * specifies rules otherwise. */
The above is not wrong per-se, but feels somewhat roundabout way to define what it does, from the viewpoint of somebody who may want to call or understand it. "What matches one of the exclude patterns is excluded. If the include patterns is empty, what did not match exclude patterns is included unless it matches one of the exclude configs. But if the include patterns is not empty, what did not match exclude patterns is included only if it matches one of the include patterns."
Quoted 112 lines
int ref_filter_match(const char *refname,
refs.c +2
-1const struct string_list *include_patterns, - const struct string_list *exclude_patterns); + const struct string_list *exclude_patterns, + const struct string_list *exclude_patterns_config);static inline const char *has_glob_specials (const char *pattern) {t/t4202-log.sh +50
-1@@ -742,7 +742,23 @@ test_expect_success 'decorate-refs with glob' ' octopus-a (octopus-a) reach EOF + cat >expect.no-decorate <<-\EOF && + Merge-tag-reach + Merge-tags-octopus-a-and-octopus-b + seventh + octopus-b + octopus-a + reach + EOF + git log -n6 --decorate=short --pretty="tformat:%f%d" \ + --decorate-refs="heads/octopus*" >actual && + test_cmp expect.decorate actual && git log -n6 --decorate=short --pretty="tformat:%f%d" \ + --decorate-refs-exclude="heads/octopus*" \ + --decorate-refs="heads/octopus*" >actual && + test_cmp expect.no-decorate actual && + git -c log.excludeDecoration="heads/octopus*" log \ + -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs="heads/octopus*" >actual && test_cmp expect.decorate actual '@@ -787,6 +803,9 @@ test_expect_success 'decorate-refs-exclude with glob' ' EOF git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs-exclude="heads/octopus*" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="heads/octopus*" log \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && test_cmp expect.decorate actual '@@ -801,6 +820,9 @@ test_expect_success 'decorate-refs-exclude without globs' ' EOF git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs-exclude="tags/reach" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="tags/reach" log \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && test_cmp expect.decorate actual '@@ -816,11 +838,19 @@ test_expect_success 'multiple decorate-refs-exclude' ' git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs-exclude="heads/octopus*" \ --decorate-refs-exclude="tags/reach" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="heads/octopus*" \ + -c log.excludeDecoration="tags/reach" log \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="heads/octopus*" log \ + --decorate-refs-exclude="tags/reach" \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && test_cmp expect.decorate actual ' test_expect_success 'decorate-refs and decorate-refs-exclude' ' - cat >expect.decorate <<-\EOF && + cat >expect.no-decorate <<-\EOF && Merge-tag-reach (master) Merge-tags-octopus-a-and-octopus-b seventh@@ -831,6 +861,21 @@ test_expect_success 'decorate-refs and decorate-refs-exclude' ' git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs="heads/*" \ --decorate-refs-exclude="heads/oc*" >actual && + test_cmp expect.no-decorate actual +' + +test_expect_success 'deocrate-refs and log.excludeDecoration' ' + cat >expect.decorate <<-\EOF && + Merge-tag-reach (master) + Merge-tags-octopus-a-and-octopus-b + seventh + octopus-b (octopus-b) + octopus-a (octopus-a) + reach (reach) + EOF + git -c log.excludeDecoration="heads/oc*" log \ + --decorate-refs="heads/*" \ + -n6 --decorate=short --pretty="tformat:%f%d" >actual && test_cmp expect.decorate actual '@@ -846,6 +891,10 @@ test_expect_success 'decorate-refs-exclude and simplify-by-decoration' ' git log -n6 --decorate=short --pretty="tformat:%f%d" \ --decorate-refs-exclude="*octopus*" \ --simplify-by-decoration >actual && + test_cmp expect.decorate actual && + git -c log.excludeDecoration="*octopus*" log \ + -n6 --decorate=short --pretty="tformat:%f%d" \ + --simplify-by-decoration >actual && test_cmp expect.decorate actual 'base-commit:
274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925
Quoted 7 lines · Junio C Hamano
Hmph. Do we still need "found" here? If there are include patterns given explicitly from the command line, a ref MUST match one of them in order to be included, and a ref that matches one of them will be included no matter that exclude config says.
So shouldn't the updated logic for the include patterns part be more like ...
I still think that the two clean-ups I mentioned are both worth doing, but without them, but with the simplification of the code I suggested, the resulting helper becomes like this, which I think is quite easy to understand. It seems to pass t4202, which you updated, too.
int ref_filter_match(const char *refname,
const struct string_list *include_patterns, const struct string_list *exclude_patterns, const struct string_list *exclude_patterns_config)
{
struct string_list_item *item;
if (exclude_patterns && exclude_patterns->nr) {
for_each_string_list_item(item, exclude_patterns) {
if (match_ref_pattern(refname, item))
return 0;
}
}
if (include_patterns && include_patterns->nr) {
for_each_string_list_item(item, include_patterns) {
if (match_ref_pattern(refname, item)) {
return 1;
}
}
return 0;
}
if (exclude_patterns_config && exclude_patterns_config->nr) {
for_each_string_list_item(item, exclude_patterns_config) {
if (match_ref_pattern(refname, item))
return 0;
}
}
return 1;
}Quoted 42 lines · Junio C Hamano
Quoted 7 lines · Junio C Hamano
Hmph. Do we still need "found" here? If there are include patterns given explicitly from the command line, a ref MUST match one of them in order to be included, and a ref that matches one of them will be included no matter that exclude config says.
So shouldn't the updated logic for the include patterns part be more like ...
I still think that the two clean-ups I mentioned are both worth doing, but without them, but with the simplification of the code I suggested, the resulting helper becomes like this, which I think is quite easy to understand. It seems to pass t4202, which you updated, too.
int ref_filter_match(const char *refname,
const structstring_list*include_patterns, const structstring_list*exclude_patterns, const structstring_list*exclude_patterns_config){ struct string_list_item *item; if (exclude_patterns && exclude_patterns->nr) { for_each_string_list_item(item, exclude_patterns) { if (match_ref_pattern(refname, item)) return 0; } } if (include_patterns && include_patterns->nr) { for_each_string_list_item(item, include_patterns) { if (match_ref_pattern(refname, item)) { return 1; } } return 0; } if (exclude_patterns_config && exclude_patterns_config->nr) {
Quoted 8 lines
for_each_string_list_item(item, exclude_patterns_config) {
if (match_ref_pattern(refname, item))
return 0;
}
}
return 1;
}This reorganization of the code is much cleaner, and it can guide cleaning up the documentation comment in the header file.
I see you included it in a SQUASH??? commit, which I will squash into my branch for v3.
Thanks,
-Stolee
Quoted 26 lines · Junio C Hamano
Quoted 8 lines · Derrick Stolee via GitGitGadget
Add the 'log.excludeDecoration' config option so users can exclude some refs from decorations by default instead of needing to use --decorate-refs-exclude manually. The config value is multi-valued much like the command-line option. The documentation is careful to point out that the config value can be overridden by the --decorate-refs option, even though --decorate-refs-exclude would always "win" over --decorate-refs.
Quoted 5 lines
Since the 'log.excludeDecoration' takes lower precedence to --decorate-refs, and --decorate-refs-exclude takes higher precedence, the struct
decoration_filterneeded another field. This led also to new logic inload_ref_decorations()andref_filter_match().All of the above makes sense to me.
Quoted 8 lines
Documentation/config/log.txt +0
-0@@ -18,6 +18,11 @@ log.decorate:: names are shown. This is the same as the `--decorate` option of the `git log`.
Quoted 8 lines
Documentation/config/log.txt log.decorate:: · line 24
Quoted 4 lines
Documentation/config/log.txt +4
-0+log.excludeDecoration:: + Exclude the specified patterns from the log decorations. This multi- + valued config option is the same as the `--decorate-refs-exclude` + option of `git log`.Can the config still be "the same as" that option, though, with the new "unlike --decorate-refs-exclude that always wins, config is at the lowest precedence" rule?
I thought I had updated this to make that clearer, but it looks like I missed it when staging.
What I had meant to write was this:
log.excludeDecoration::
Exclude the specified patterns from the log decorations. This is similar to the --decorate-refs-exclude command-line option, but the config option can be overridden by the --decorate-refs option.
Quoted 21 lines
log-tree.c static int add_ref_decoration(const char *refname, const struct object_id *oid, · line 95
Quoted 12 lines
log-tree.c +2
-1@@ -90,7 +90,8 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,if (filter && !ref_filter_match (refname, filter->include_ref_pattern, - filter->exclude_ref_pattern)) + filter->exclude_ref_pattern, + filter->exclude_ref_config_pattern))return 0 ;As there is only one caller of the
ref_filter_match()helper, I wonder if we want to(1) move the helper to
log-tree.c, make it static and remove its definition fromrefs.h, and optionally rename it so that it is clear that this is not part of the "ref_filter" API that drives "for-each-ref" and friends;
Quoted 5 lines
(2) instead of adding yet another pattern to the parameter list, make the helper take the whole "filter" instance as a single parameter.
as a clean-up.
This is a good idea. I was thinking the code was "smelly" when I had to jump through so many hoops to get it working. I'll add a refactor patch in front of this one.
Quoted 32 lines
Quoted 32 lines
refs.c +6
-2@@ -339,9 +339,11 @@ static int match_ref_pattern(const char *refname,int ref_filter_match (const char *refname,const struct string_list *include_patterns, -const struct string_list *exclude_patterns) +const struct string_list *exclude_patterns, +const struct string_list *exclude_patterns_config) {struct string_list_item *item; +int found =0 ;if (exclude_patterns && exclude_patterns->nr) {for_each_string_list_item (item, exclude_patterns) {@@ -351,7 +353,6 @@ int ref_filter_match(const char *refname, }if (include_patterns && include_patterns->nr) { -int found =0 ;for_each_string_list_item (item, include_patterns) {if (match_ref_pattern (refname, item)) { found =1 ;@@ -362,6 +363,16 @@ int ref_filter_match(const char *refname,if (!found)return 0 ; } + + if (!found && + exclude_patterns_config &&
Quoted 8 lines
refs.c int ref_filter_match(const char *refname, · line 374
Quoted 6 lines
refs.c +6
-0+ exclude_patterns_config->nr) { +for_each_string_list_item (item, exclude_patterns_config) { +if (match_ref_pattern (refname, item)) +return 0 ; + } + }Hmph. Do we still need "found" here? ...
You included an excellent update in another response, which I have squashed locally.
Quoted 15 lines
Quoted 4 lines
refs.c +3
-0/* +* Returns 0 if the refname matches any of the exclude_patterns. +* +* Returns 0 if include_patterns is non-empty but refname does not match refs.c int ref_filter_match(const char *refname,
Quoted 10 lines
refs.c +6
-0+* any of those patterns. +* +* Returns 0 if refname matches a pattern in exclude_patterns_config but +* does not match any pattern in inclue_patterns. +* +* Otherwise, returns 1. * * This has the effect of matching everything by default, unless the user * specifies rules otherwise. */
Quoted 8 lines
The above is not wrong per-se, but feels somewhat roundabout way to define what it does, from the viewpoint of somebody who may want to call or understand it. "What matches one of the exclude patterns is excluded. If the include patterns is empty, what did not match exclude patterns is included unless it matches one of the exclude configs. But if the include patterns is not empty, what did not match exclude patterns is included only if it matches one of the include patterns."
Your new logic for the method makes this a bit simpler to write:
/*
* Returns 0 if the refname matches any of the exclude_patterns. *
* Otherwise, returns 1 if the refname matches any of the include_patterns. *
* Otherwise, returns 0 if include_patterns is non-empty.
*
* Otherwise, returns 0 if the refname matches any of the patterns
* in exclude_patterns_config.
*
* Finally, if none of the above apply, return 1.
*/
However, if I pull this method into a static helper method, then the documentation is unnecessary.
Thanks for the careful review!
-Stolee
v3 cover log: add log.excludeDecoration config option
In reply to Derrick Stolee via GitGitGadgetThis was something hinted at in the "fetch" step of the background maintenance RFC. Should be a relatively minor addition to our config options.
We definitely want this feature for microsoft/git (we would set log.excludeDecoration=refs/scalar/* in all Scalar repos), but we will wait for feedback from the community.
Updates in v2:
* Use for_each_string_list_item()
* Update the matching logic to allow --decorate-refs to override the config option.
Updates in v3:
* Moved and refactored the ref_filter_match() in a preparation patch.
* Used Junio's new logic in ref_filter_match()
* Updated the config documentation to be more clear.
Thanks, -Stolee
Derrick Stolee (2):
log-tree: make ref_filter_match() a helper method
log: add log.excludeDecoration config option
Documentation/config/log.txt | 6 ++++ Documentation/git-log.txt | 5 +++- builtin/log.c | 16 +++++++++- log-tree.c | 58 ++++++++++++++++++++++++++++++++++-- log-tree.h | 4 ++- refs.c | 44 --------------------------- refs.h | 12 -------- t/t4202-log.sh | 51 ++++++++++++++++++++++++++++++- 8 files changed, 133 insertions(+), 63 deletions(-)
base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-610%2Fderrickstolee%2Flog-exclude-decoration-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-610/derrickstolee/log-exclude-decoration-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/610
Range-diff vs v2:
-: ----------- > 1: 6840f8801e4 log-tree: make ref_filter_match() a helper method 1: cbdaef4a8e1 ! 2: 96c865e9214 log: add log.excludeDecoration config option
@@ Commit message
[1] https://github.com/microsoft/scalar
[2] https://lore.kernel.org/git/77b1da5d3063a2404cd750adfe3bb8be9b6c497d.1585946894.git.gitgitgadget@gmail.com/
+ Helped-by: Junio C Hamano <[email protected]>
## Documentation/config/log.txt ##
@@ Documentation/config/log.txt: log.decorate::
of the git log.
+log.excludeDecoration::
-+ Exclude the specified patterns from the log decorations. This multi-
-+ valued config option is the same as the --decorate-refs-exclude
-+ option of git log.
++ Exclude the specified patterns from the log decorations. This is
++ similar to the --decorate-refs-exclude command-line option, but
++ the config option can be overridden by the --decorate-refs
++ option.
+
log.follow::
If true, git log will act as if the --follow option was used when
@@ builtin/log.c: static void cmd_log_init_finish(int argc, const char **argv, cons
## log-tree.c ##
-@@ log-tree.c: static int add_ref_decoration(const char *refname, const struct object_id *oid, +@@ log-tree.c: static int ref_filter_match(const char *refname,
+ struct string_list_item *item;
+ const struct string_list *exclude_patterns = filter->exclude_ref_pattern;
+ const struct string_list *include_patterns = filter->include_ref_pattern;
++ const struct string_list *exclude_patterns_config =
++ filter->exclude_ref_config_pattern;
+
+ if (exclude_patterns && exclude_patterns->nr) {
+ for_each_string_list_item(item, exclude_patterns) {
+@@ log-tree.c: static int ref_filter_match(const char *refname,
+ }
+
+ if (include_patterns && include_patterns->nr) {
+- int found = 0;
+ for_each_string_list_item(item, include_patterns) {
+ if (match_ref_pattern(refname, item)) {
+- found = 1;
+- break;
++ return 1;
+ }
+ }
++ return 0;
++ }
- if (filter && !ref_filter_match(refname,
- filter->include_ref_pattern,
-- filter->exclude_ref_pattern))
-+ filter->exclude_ref_pattern,
-+ filter->exclude_ref_config_pattern))
- return 0;
+- if (!found)
+- return 0;
++ if (exclude_patterns_config && exclude_patterns_config->nr) {
++ for_each_string_list_item(item, exclude_patterns_config) {
++ if (match_ref_pattern(refname, item))
++ return 0;
++ }
+ }
++
+ return 1;
+ }
- if (starts_with(refname, git_replace_ref_base)) {
@@ log-tree.c: void load_ref_decorations(struct decoration_filter *filter, int flags)
for_each_string_list_item(item, filter->include_ref_pattern) {
normalize_glob_ref(item, NULL, item->string);
@@ log-tree.h: struct log_info {
int parse_decorate_color_config(const char *var, const char *slot_name, const char *value);
- ## refs.c ##
-@@ refs.c: static int match_ref_pattern(const char *refname,
-
- int ref_filter_match(const char *refname,
- const struct string_list *include_patterns,
-- const struct string_list *exclude_patterns)
-+ const struct string_list *exclude_patterns,
-+ const struct string_list *exclude_patterns_config)
- {
- struct string_list_item *item;
-+ int found = 0;
-
- if (exclude_patterns && exclude_patterns->nr) {
- for_each_string_list_item(item, exclude_patterns) {
-@@ refs.c: int ref_filter_match(const char *refname,
- }
-
- if (include_patterns && include_patterns->nr) {
-- int found = 0;
- for_each_string_list_item(item, include_patterns) {
- if (match_ref_pattern(refname, item)) {
- found = 1;
-@@ refs.c: int ref_filter_match(const char *refname,
- if (!found)
- return 0;
- }
-+
-+ if (!found &&
-+ exclude_patterns_config &&
-+ exclude_patterns_config->nr) {
-+ for_each_string_list_item(item, exclude_patterns_config) {
-+ if (match_ref_pattern(refname, item))
-+ return 0;
-+ }
-+ }
-+
- return 1;
- }
-
-
- ## refs.h ##
-@@ refs.h: void normalize_glob_ref(struct string_list_item *item, const char *prefix,
- const char *pattern);
-
- /*
-- * Returns 0 if refname matches any of the exclude_patterns, or if it doesn't -- * match any of the include_patterns. Returns 1 otherwise.
-+ * Returns 0 if the refname matches any of the exclude_patterns.
-+ *
-+ * Returns 0 if include_patterns is non-empty but refname does not match -+ * any of those patterns.
-+ *
-+ * Returns 0 if refname matches a pattern in exclude_patterns_config but -+ * does not match any pattern in inclue_patterns.
-+ *
-+ * Otherwise, returns 1.
- *
-- * If pattern list is NULL or empty, matching against that list is skipped.
- * This has the effect of matching everything by default, unless the user
- * specifies rules otherwise.
- */
- int ref_filter_match(const char *refname,
- const struct string_list *include_patterns,
-- const struct string_list *exclude_patterns);
-+ const struct string_list *exclude_patterns,
-+ const struct string_list *exclude_patterns_config);
-
- static inline const char *has_glob_specials(const char *pattern)
- {
-
## t/t4202-log.sh ##
@@ t/t4202-log.sh: test_expect_success 'decorate-refs with glob' '
octopus-a (octopus-a)
Signature · 1 line
gitgitgadget
PATCH v3 1/2 log-tree: make ref_filter_match() a helper method
From: Derrick Stolee <[email protected]>
The ref_filter_match() method is defined in refs.h and implemented in refs.c, but is only used by add_ref_decoration() in log-tree.c. Move it into that file as a static helper method. The match_ref_pattern() comes along for the ride.
While moving the code, also make a slight adjustment to have ref_filter_match() take a struct decoration_filter pointer instead of multiple string lists. This is non-functional, but will make a later change be much cleaner.
The diff is easier to parse when using the --color-moved option.
--- log-tree.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++--- refs.c | 44 -------------------------------------------- refs.h | 12 ------------ 3 files changed, 46 insertions(+), 59 deletions(-)
log-tree.c +46 -3
@@ -81,6 +81,51 @@ const struct name_decoration *get_name_decoration(const struct object *obj)return lookup_decoration (&name_decoration, obj); } +static int match_ref_pattern (const char *refname, +const struct string_list_item *item) +{ +int matched =0 ; +if (item->util ==NULL ) { +if (!wildmatch (item->string, refname,0 )) + matched =1 ; + }else { +const char *rest; +if (skip_prefix (refname, item->string, &rest) && + (!*rest || *rest =='/' )) + matched =1 ; + } +return matched; +} + +static int ref_filter_match (const char *refname, +const struct decoration_filter *filter) +{ +struct string_list_item *item; +const struct string_list *exclude_patterns = filter->exclude_ref_pattern; +const struct string_list *include_patterns = filter->include_ref_pattern; + +if (exclude_patterns && exclude_patterns->nr) { +for_each_string_list_item (item, exclude_patterns) { +if (match_ref_pattern (refname, item)) +return 0 ; + } + } + +if (include_patterns && include_patterns->nr) { +int found =0 ; +for_each_string_list_item (item, include_patterns) { +if (match_ref_pattern (refname, item)) { + found =1 ; +break ; + } + } + +if (!found) +return 0 ; + } +return 1 ; +} +static int add_ref_decoration (const char *refname,const struct object_id *oid,int flags,void *cb_data) {
@@ -88,9 +133,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,enum decoration_type type =DECORATION_NONE ;struct decoration_filter *filter = (struct decoration_filter *)cb_data; -if (filter && !ref_filter_match (refname, - filter->include_ref_pattern, - filter->exclude_ref_pattern)) +if (filter && !ref_filter_match (refname, filter))return 0 ;if (starts_with (refname, git_replace_ref_base)) {
refs.c +0 -44
50 lines not on this page · raw message
refs.h +0 -12
@@ -361,18 +361,6 @@ int for_each_rawref(each_ref_fn fn, void *cb_data);void normalize_glob_ref (struct string_list_item *item,const char *prefix,const char *pattern); -/* -* Returns 0 if refname matches any of the exclude_patterns, or if it doesn't -* match any of the include_patterns. Returns 1 otherwise. -* -* If pattern list is NULL or empty, matching against that list is skipped. -* This has the effect of matching everything by default, unless the user -* specifies rules otherwise. -*/ -int ref_filter_match (const char *refname, -const struct string_list *include_patterns, -const struct string_list *exclude_patterns); -static inline const char *has_glob_specials (const char *pattern) {return strpbrk (pattern,"?*[" );
Signature · 1 line
gitgitgadget
PATCH v3 2/2 log: add log.excludeDecoration config option
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
In 'git log', the --decorate-refs-exclude option appends a pattern to a string_list. This list is used to prevent showing some refs in the decoration output, or even by --simplify-by-decoration.
Users may want to use their refs space to store utility refs that should not appear in the decoration output. For example, Scalar [1] runs a background fetch but places the "new" refs inside the refs/scalar/hidden/<remote>/* refspace instead of refs/<remote>/* to avoid updating remote refs when the user is not looking. However, these "hidden" refs appear during regular 'git log' queries.
A similar idea to use "hidden" refs is under consideration for core Git [2].
Add the 'log.excludeDecoration' config option so users can exclude some refs from decorations by default instead of needing to use --decorate-refs-exclude manually. The config value is multi-valued much like the command-line option. The documentation is careful to point out that the config value can be overridden by the --decorate-refs option, even though --decorate-refs-exclude would always "win" over --decorate-refs.
Since the 'log.excludeDecoration' takes lower precedence to --decorate-refs, and --decorate-refs-exclude takes higher precedence, the struct decoration_filter needed another field. This led also to new logic in load_ref_decorations() and ref_filter_match().
There are several tests in t4202-log.sh that test the --decorate-refs-(include|exclude) options, so these are extended. Since the expected output is already stored as a file, most tests could simply replace a "--decorate-refs-exclude" option with an in-line config setting. Other tests involve the precedence of the config option compared to command-line options and needed more modification.
[1] https://github.com/microsoft/scalar
[2] https://lore.kernel.org/git/77b1da5d3063a2404cd750adfe3bb8be9b6c497d.1585946894.git.gitgitgadget@gmail.com/
--- Documentation/config/log.txt | 6 +++++ Documentation/git-log.txt | 5 +++- builtin/log.c | 16 ++++++++++- log-tree.c | 19 ++++++++++---- log-tree.h | 4 ++- t/t4202-log.sh | 51 +++++++++++++++++++++++++++++++++++- 6 files changed, 92 insertions(+), 9 deletions(-)
Documentation/config/log.txt +6 -0
@@ -18,6 +18,12 @@ log.decorate:: names are shown. This is the same as the `--decorate` option of the `git log`. +log.excludeDecoration:: + Exclude the specified patterns from the log decorations. This is + similar to the `--decorate-refs-exclude` command-line option, but + the config option can be overridden by the `--decorate-refs` + option. + log.follow:: If `true`, `git log` will act as if the `--follow` option was used when a single <path> is given. This has the same limitations as `--follow`,
Documentation/git-log.txt +4 -1
@@ -43,7 +43,10 @@ OPTIONS If no `--decorate-refs` is given, pretend as if all refs were included. For each candidate, do not use it for decoration if it matches any patterns given to `--decorate-refs-exclude` or if it - doesn't match any of the patterns given to `--decorate-refs`. + doesn't match any of the patterns given to `--decorate-refs`. The + `log.excludeDecoration` config option allows excluding refs from + the decorations, but an explicit `--decorate-refs` pattern will + override a match in `log.excludeDecoration`. --source:: Print out the ref name given on the command line by which each
builtin/log.c +15 -1
@@ -164,9 +164,11 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,int quiet =0 , source =0 , mailmap;static struct line_opt_callback_data line_cb = {NULL ,NULL ,STRING_LIST_INIT_DUP };static struct string_list decorate_refs_exclude =STRING_LIST_INIT_NODUP ; +static struct string_list decorate_refs_exclude_config =STRING_LIST_INIT_NODUP ;static struct string_list decorate_refs_include =STRING_LIST_INIT_NODUP ;struct decoration_filter decoration_filter = {&decorate_refs_include, - &decorate_refs_exclude}; + &decorate_refs_exclude, + &decorate_refs_exclude_config};static struct revision_sources revision_sources;const struct option builtin_log_options[] = {
@@ -236,7 +238,19 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, }if (decoration_style) { +const struct string_list *config_exclude = +repo_config_get_value_multi (the_repository, +"log.excludeDecoration" ); + +if (config_exclude) { +struct string_list_item *item; +for_each_string_list_item (item, config_exclude) +string_list_append (&decorate_refs_exclude_config, + item->string); + } + rev->show_decorations =1 ; +load_ref_decorations (&decoration_filter, decoration_style); }
log-tree.c +14 -5
@@ -103,6 +103,8 @@ static int ref_filter_match(const char *refname,struct string_list_item *item;const struct string_list *exclude_patterns = filter->exclude_ref_pattern;const struct string_list *include_patterns = filter->include_ref_pattern; +const struct string_list *exclude_patterns_config = + filter->exclude_ref_config_pattern;if (exclude_patterns && exclude_patterns->nr) {for_each_string_list_item (item, exclude_patterns) {
@@ -112,17 +114,21 @@ static int ref_filter_match(const char *refname, }if (include_patterns && include_patterns->nr) { -int found =0 ;for_each_string_list_item (item, include_patterns) {if (match_ref_pattern (refname, item)) { - found =1 ; -break ; +return 1 ; } } +return 0 ; + } -if (!found) -return 0 ; +if (exclude_patterns_config && exclude_patterns_config->nr) { +for_each_string_list_item (item, exclude_patterns_config) { +if (match_ref_pattern (refname, item)) +return 0 ; + } } +return 1 ; }
@@ -198,6 +204,9 @@ void load_ref_decorations(struct decoration_filter *filter, int flags)for_each_string_list_item (item, filter->include_ref_pattern) {normalize_glob_ref (item,NULL , item->string); } +for_each_string_list_item (item, filter->exclude_ref_config_pattern) { +normalize_glob_ref (item,NULL , item->string); + } } decoration_loaded =1 ; decoration_flags = flags;
log-tree.h +3 -1
@@ -8,7 +8,9 @@ struct log_info { };struct decoration_filter { -struct string_list *include_ref_pattern, *exclude_ref_pattern; +struct string_list *include_ref_pattern; +struct string_list *exclude_ref_pattern; +struct string_list *exclude_ref_config_pattern; };int parse_decorate_color_config (const char *var,const char *slot_name,const char *value);
t/t4202-log.sh +50 -1
92 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Quoted 14 lines · Derrick Stolee via GitGitGadget
log-tree.c +2
-0@@ -103,6 +103,8 @@ static int ref_filter_match(const char *refname,struct string_list_item *item;const struct string_list *exclude_patterns = filter->exclude_ref_pattern;const struct string_list *include_patterns = filter->include_ref_pattern; +const struct string_list *exclude_patterns_config = + filter->exclude_ref_config_pattern;if (exclude_patterns && exclude_patterns->nr) {for_each_string_list_item (item, exclude_patterns) {
log-tree.c static int ref_filter_match(const char *refname, · line 120
Quoted 10 lines
log-tree.c +1
-3@@ -112,17 +114,21 @@ static int ref_filter_match(const char *refname, }if (include_patterns && include_patterns->nr) { -int found =0 ;for_each_string_list_item (item, include_patterns) {if (match_ref_pattern (refname, item)) { - found =1 ; -break ; +return 1 ; }
Micronit.
Let's mimick the early return in the loop above (for command line excludes) and below (for configured excludes), each of which is just a single "return" statement in a block without {braces} around.
Other than that, looks nicely done.
The new tests are really appreciated, too.
Quoted 11 lines · Junio C Hamano
log-tree.c static int ref_filter_match(const char *refname, · line 120
Quoted 8 lines
log-tree.c +1
-3if (include_patterns && include_patterns->nr) { -int found =0 ;for_each_string_list_item (item, include_patterns) {if (match_ref_pattern (refname, item)) { - found =1 ; -break ; +return 1 ; }Micronit.
Quoted 3 lines
Let's mimick the early return in the loop above (for command line excludes) and below (for configured excludes), each of which is just a single "return" statement in a block without {braces} around.
... heh, it seems that the nit is mine in the suggested alternative upthread. Let me amend while queuing.
Thanks.
Quoted 13 lines · Junio C Hamano
Quoted 11 lines · Junio C Hamano
log-tree.c static int ref_filter_match(const char *refname, · line 120
Quoted 8 lines
log-tree.c +1
-3if (include_patterns && include_patterns->nr) { -int found =0 ;for_each_string_list_item (item, include_patterns) {if (match_ref_pattern (refname, item)) { - found =1 ; -break ; +return 1 ; }Micronit.
Quoted 6 lines
Quoted 3 lines
Let's mimick the early return in the loop above (for command line excludes) and below (for configured excludes), each of which is just a single "return" statement in a block without {braces} around.
... heh, it seems that the nit is mine in the suggested alternative upthread. Let me amend while queuing.
Shame on me for not noticing. Thanks for the close look!
-Stolee
Quoted 14 lines · Derrick Stolee
Quoted 13 lines · Junio C Hamano
Quoted 11 lines · Junio C Hamano
log-tree.c static int ref_filter_match(const char *refname, · line 120
Quoted 8 lines
log-tree.c +1
-3if (include_patterns && include_patterns->nr) { -int found =0 ;for_each_string_list_item (item, include_patterns) {if (match_ref_pattern (refname, item)) { - found =1 ; -break ; +return 1 ; }Micronit.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Let's mimick the early return in the loop above (for command line excludes) and below (for configured excludes), each of which is just a single "return" statement in a block without {braces} around.
... heh, it seems that the nit is mine in the suggested alternative upthread. Let me amend while queuing.
Shame on me for not noticing. Thanks for the close look!
Heh, shame is mine, too ;-)
Thanks.