| Message ID | c249bface2a6dcd0355620f92579b42a6fa4ea58.1659722324.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 92156291ca82ae4f4ad09fde8181c5f2b7dba6ca |
| Headers | show |
| Series | log: create tighter default decoration filter | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Derrick Stolee <derrickstolee@github.com> > > When a user runs 'git log', they expect a certain set of helpful > decorations. This includes: > > * The HEAD ref > * Branches (refs/heads/) > * Stashes (refs/stash) > * Tags (refs/tags/) > * Remote branches (refs/remotes/) > * Replace refs (refs/replace/ or $GIT_REPLACE_REF_BASE) > > Each of these namespaces was selected due to existing test cases that > verify these namespaces appear in the decorations. In particular, > stashes and replace refs can have custom colors from the > color.decorate.<slot> config option. I _just_ noticed that refs/bisect/* isn't part of this list, but I'd presume that users want to see those decorations (or I do, at least). Was that an intentional omission?
On 9/8/2022 5:13 PM, Glen Choo wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Derrick Stolee <derrickstolee@github.com> >> >> When a user runs 'git log', they expect a certain set of helpful >> decorations. This includes: >> >> * The HEAD ref >> * Branches (refs/heads/) >> * Stashes (refs/stash) >> * Tags (refs/tags/) >> * Remote branches (refs/remotes/) >> * Replace refs (refs/replace/ or $GIT_REPLACE_REF_BASE) >> >> Each of these namespaces was selected due to existing test cases that >> verify these namespaces appear in the decorations. In particular, >> stashes and replace refs can have custom colors from the >> color.decorate.<slot> config option. > > I _just_ noticed that refs/bisect/* isn't part of this list, but I'd > presume that users want to see those decorations (or I do, at least). > Was that an intentional omission? It was an intentional omission because the refs/bisect/* references are not part of the color.decorate.<slot> category. Looking into it further, the bisect refs look pretty ugly (especially the ones like "refs/bisect/good-<hash>"). If you would like to include these in the default filter, then I would recommend also adding a color.decorate.<slot> category for them and possibly replace the "refs/bisect" with just "bisect". Alternatively, you could take a hint from replace objects and just use an indicator like "bisect good" or "bisect bad" instead of listing the full ref name. Thanks, -Stolee
Derrick Stolee <derrickstolee@github.com> writes: > It was an intentional omission because the refs/bisect/* references > are not part of the color.decorate.<slot> category. > > Looking into it further, the bisect refs look pretty ugly (especially > the ones like "refs/bisect/good-<hash>"). > > If you would like to include these in the default filter, then I > would recommend also adding a color.decorate.<slot> category for them > and possibly replace the "refs/bisect" with just "bisect". Alternatively, > you could take a hint from replace objects and just use an indicator > like "bisect good" or "bisect bad" instead of listing the full ref name. I personally do not think bisect/bad and bisect/good-* need to be part of the default set of refs to use for decoration. I suspect that the suggestion to use them for decoration is based on the gut feeling: "People during their bisect session would want to know which points they already examined and what their states are." But during bisection, there is a specific command to give them exactly that information: "bisect visualize". It is roughly equilvalent to: git log refs/bisect/bad \ $(git for-each-ref --format='^%(refname)' refs/bisect/good-\* i.e. show the history surrounded by all the known-to-be-good commits and the known-to-be-bad commit we currently are chasing. Bad and good commits are at the boundary you can tell them without decoration. But if we still want to see bisect/bad and bisect/good-* in a larger graph (i.e. showing descendants of bad and ancestors of good), then I do not think good-<object name> being a long label is something we should special case and shorten. Especially because the user is not using "bisect visualize", which is readable without decoration, they may be seeking more information in the names, perhaps cutting and pasting the object names to feed "git show" running on a separate terminal, or something.
On 9/9/2022 12:19 PM, Junio C Hamano wrote: > But during bisection, there is a specific command to give them > exactly that information: "bisect visualize". It is roughly > equilvalent to: > > git log refs/bisect/bad \ > $(git for-each-ref --format='^%(refname)' refs/bisect/good-\* > > i.e. show the history surrounded by all the known-to-be-good commits > and the known-to-be-bad commit we currently are chasing. Bad and good > commits are at the boundary you can tell them without decoration. Thank you for recommending this as the expected behavior. > But if we still want to see bisect/bad and bisect/good-* in a larger > graph (i.e. showing descendants of bad and ancestors of good), then > I do not think good-<object name> being a long label is something we > should special case and shorten. Especially because the user is not > using "bisect visualize", which is readable without decoration, they > may be seeking more information in the names, perhaps cutting and > pasting the object names to feed "git show" running on a separate > terminal, or something. The object names should be visible as part of the "git log" output (perhaps abbreviated) and that provides a way to get that information without needing an unabbreviated hash showing up in a decoration slot. Thanks, -Stolee
Junio C Hamano <gitster@pobox.com> writes: > Derrick Stolee <derrickstolee@github.com> writes: > >> It was an intentional omission because the refs/bisect/* references >> are not part of the color.decorate.<slot> category. >> >> Looking into it further, the bisect refs look pretty ugly (especially >> the ones like "refs/bisect/good-<hash>"). >> >> If you would like to include these in the default filter, then I >> would recommend also adding a color.decorate.<slot> category for them >> and possibly replace the "refs/bisect" with just "bisect". Alternatively, >> you could take a hint from replace objects and just use an indicator >> like "bisect good" or "bisect bad" instead of listing the full ref name. > > I suspect that the suggestion to use them for decoration is based on > the gut feeling: "People during their bisect session would want to > know which points they already examined and what their states are." > > But during bisection, there is a specific command to give them > exactly that information: "bisect visualize". It is roughly > equilvalent to: > > git log refs/bisect/bad \ > $(git for-each-ref --format='^%(refname)' refs/bisect/good-\* > > i.e. show the history surrounded by all the known-to-be-good commits > and the known-to-be-bad commit we currently are chasing. Bad and good > commits are at the boundary you can tell them without decoration. Thanks, both. I think "bisect visualize" covers the use case I was thinking of (seeing the bisect result in the commit history), so omitting refs/bisect/* sounds fine. And if users are unhappy about the change, it seems simple enough to add it to the list.
Derrick Stolee <derrickstolee@github.com> writes: > The object names should be visible as part of the "git log" output > (perhaps abbreviated) and that provides a way to get that information > without needing an unabbreviated hash showing up in a decoration slot. OK.
diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt index 20e87cecf49..b2ac89dfafc 100644 --- a/Documentation/git-log.txt +++ b/Documentation/git-log.txt @@ -45,13 +45,16 @@ OPTIONS --decorate-refs=<pattern>:: --decorate-refs-exclude=<pattern>:: - If no `--decorate-refs` is given, pretend as if all refs were - included. For each candidate, do not use it for decoration if it + For each candidate reference, 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`. The `log.excludeDecoration` config option allows excluding refs from the decorations, but an explicit `--decorate-refs` pattern will override a match in `log.excludeDecoration`. ++ +If none of these options or config settings are given, then references are +used as decoration if they match `HEAD`, `refs/heads/`, `refs/remotes/`, +`refs/stash/`, or `refs/tags/`. --source:: Print out the ref name given on the command line by which each diff --git a/builtin/log.c b/builtin/log.c index 88a5e98875a..6683e5ff134 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -162,6 +162,37 @@ static void cmd_log_init_defaults(struct rev_info *rev) parse_date_format(default_date_mode, &rev->date_mode); } +static void set_default_decoration_filter(struct decoration_filter *decoration_filter) +{ + int i; + struct string_list *include = decoration_filter->include_ref_pattern; + const struct string_list *config_exclude = + git_config_get_value_multi("log.excludeDecoration"); + + if (config_exclude) { + struct string_list_item *item; + for_each_string_list_item(item, config_exclude) + string_list_append(decoration_filter->exclude_ref_config_pattern, + item->string); + } + + if (decoration_filter->exclude_ref_pattern->nr || + decoration_filter->include_ref_pattern->nr || + decoration_filter->exclude_ref_config_pattern->nr) + return; + + /* + * No command-line or config options were given, so + * populate with sensible defaults. + */ + for (i = 0; i < ARRAY_SIZE(ref_namespace); i++) { + if (!ref_namespace[i].decoration) + continue; + + string_list_append(include, ref_namespace[i].ref); + } +} + static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, struct rev_info *rev, struct setup_revision_opt *opt) { @@ -171,9 +202,11 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, 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_config}; + struct decoration_filter decoration_filter = { + .exclude_ref_pattern = &decorate_refs_exclude, + .include_ref_pattern = &decorate_refs_include, + .exclude_ref_config_pattern = &decorate_refs_exclude_config, + }; static struct revision_sources revision_sources; const struct option builtin_log_options[] = { @@ -265,16 +298,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, } if (decoration_style || rev->simplify_by_decoration) { - 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); - } + set_default_decoration_filter(&decoration_filter); if (decoration_style) rev->show_decorations = 1; diff --git a/t/t4013/diff.log_--decorate=full_--all b/t/t4013/diff.log_--decorate=full_--all index 3f9b872eceb..6b0b334a5d6 100644 --- a/t/t4013/diff.log_--decorate=full_--all +++ b/t/t4013/diff.log_--decorate=full_--all @@ -20,7 +20,7 @@ Date: Mon Jun 26 00:06:00 2006 +0000 Rearranged lines in dir/sub -commit cbacedd14cb8b89255a2c02b59e77a2e9a8021a0 (refs/notes/commits) +commit cbacedd14cb8b89255a2c02b59e77a2e9a8021a0 Author: A U Thor <author@example.com> Date: Mon Jun 26 00:06:00 2006 +0000 diff --git a/t/t4013/diff.log_--decorate_--all b/t/t4013/diff.log_--decorate_--all index f5e20e1e14a..c7df1f58141 100644 --- a/t/t4013/diff.log_--decorate_--all +++ b/t/t4013/diff.log_--decorate_--all @@ -20,7 +20,7 @@ Date: Mon Jun 26 00:06:00 2006 +0000 Rearranged lines in dir/sub -commit cbacedd14cb8b89255a2c02b59e77a2e9a8021a0 (refs/notes/commits) +commit cbacedd14cb8b89255a2c02b59e77a2e9a8021a0 Author: A U Thor <author@example.com> Date: Mon Jun 26 00:06:00 2006 +0000 diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 6b7d8e269f7..4c3ce863074 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -1031,6 +1031,12 @@ test_expect_success 'decorate-refs-exclude HEAD' ' ! grep HEAD actual ' +test_expect_success 'decorate-refs focus from default' ' + git log --decorate=full --oneline \ + --decorate-refs="refs/heads" >actual && + ! grep HEAD actual +' + test_expect_success 'log.decorate config parsing' ' git log --oneline --decorate=full >expect.full && git log --oneline --decorate=short >expect.short && @@ -2198,6 +2204,20 @@ test_expect_success 'log --decorate includes all levels of tag annotated tags' ' test_cmp expect actual ' +test_expect_success 'log --decorate does not include things outside filter' ' + reflist="refs/prefetch refs/rebase-merge refs/bundle" && + + for ref in $reflist + do + git update-ref $ref/fake HEAD || return 1 + done && + + git log --decorate=full --oneline >actual && + + # None of the refs are visible: + ! grep /fake actual +' + test_expect_success 'log --end-of-options' ' git update-ref refs/heads/--source HEAD && git log --end-of-options --source >actual &&