| Message ID | bec532fb8c63b3ae784d442f438687a4f0bbad37.1659122979.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | log: create tighter default decoration filter | expand |
On Fri, Jul 29 2022, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <derrickstolee@github.com> Thanks for the re-roll in general, there's a lot of good stuff here & I hope I find more time to comment on it in more detail, but just focusing on things that would be hard to back out of once changed: > [...] > Another alternative would be to exclude the known namespaces that are > not intended to be shown. This would reduce the visible effect of the > change for expert users who use their own custom ref namespaces. The > implementation change would be very simple to swap due to our use of > ref_namespaces: > > int i; > struct string_list *exclude = decoration_filter->exclude_ref_pattern; > > /* > * No command-line or config options were given, so > * populate with sensible defaults. > */ > for (i = 0; i < NAMESPACE__COUNT; i++) { > if (ref_namespaces[i].decoration) > continue; > > string_list_append(exclude, ref_namespaces[i].ref); > } > > The main downside of this approach is that we expect to add new hidden > namespaces in the future, and that means that Git versions will be less > stable in how they behave as those namespaces are added. I see that as a feature, and not a downside. If we simply explain this in the documentation as e.g.: When adding decorations git will by default exclude certain "internal" ref namespaces that it treats specially, such as refs/magical-1/*, refs/magical-2/* (or whatever). Other such special namespaces may be reserved in the future. There's no lack of "stability", because the ref hiding only act on what's known to be something we can ignore, because our git version knows about it. If git v2.40 knows about refs/magical-1/* but not refs/magical-2/*, but git v2.50 knows about both it's not a lack of stability that v2.40 shows one decorated by default, but v2.50 shows neither. To v2.40 one of them isn't a magical "I know what this is, it's internal & I can hide it" ref. I'm aware that we disagree, and some of this was discussed in v1. I'm not intending to just repeat what I said before. But it's not just that I disagree, I genuinely don't get your POV here. We: * Know that we have (admittedly probably rare) in-the-wild use of non-standard and custom namespaces, and that this series would change long-standing log output. If I could see a good reason to change the existing "log" behavior here I'd probably be sold on it. We try not to change existing output in general, but this part isn't "plumbing", and arguably not a very "stable" part of the log output either (decorations being optional etc). But it does rub me the wrong way to sell a change in the name of "stability" when it's from the outset doing the exact opposite, to.... * ... are willing to make that one-time change so that we can have stability for users that are relying on "decorate" working consistently across versions once we're in the new world order, because we *might* add new magical refs. Since the latter group of users don't exist today by definition (this having not been integrated) why isn't it a better win-win solution to give those users some --decorate-only-known-refs option/config? They'd get their "stability" going forward, and without the needless breaking of existing behavior, no? > +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 && > + > + for ref in $reflist > + do > + ! grep $ref/fake actual || return 1 > + done I haven't tested, but isn't that last for-loop replacable with: ! grep /fake actual ? Or do we have other "/fake" refs we want to include?
On 8/4/2022 3:08 AM, Ævar Arnfjörð Bjarmason wrote: > There's no lack of "stability", because the ref hiding only act on > what's known to be something we can ignore, because our git version > knows about it. > > If git v2.40 knows about refs/magical-1/* but not refs/magical-2/*, but > git v2.50 knows about both it's not a lack of stability that v2.40 shows > one decorated by default, but v2.50 shows neither. You are describing how the behavior changes between these versions on the same repository. That's what I mean by lack of stability. > But it's not just that I disagree, I genuinely don't get your POV > here. I'm optimizing for non-experts who never need any refs outside of the standard set. Now that this version removed the notes ref from the decoration, the stance for inclusion is simple: If Git offers to color the namespace with color.decoration.<slot>, then Git decorates with that namespace by default. >> +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 && >> + >> + for ref in $reflist >> + do >> + ! grep $ref/fake actual || return 1 >> + done > > I haven't tested, but isn't that last for-loop replacable with: > > ! grep /fake actual > > ? > > Or do we have other "/fake" refs we want to include? This is a nice efficient replacement. Thanks. -Stolee
On Fri, Aug 05 2022, Derrick Stolee wrote: > On 8/4/2022 3:08 AM, Ævar Arnfjörð Bjarmason wrote: > >> There's no lack of "stability", because the ref hiding only act on >> what's known to be something we can ignore, because our git version >> knows about it. >> >> If git v2.40 knows about refs/magical-1/* but not refs/magical-2/*, but >> git v2.50 knows about both it's not a lack of stability that v2.40 shows >> one decorated by default, but v2.50 shows neither. > > You are describing how the behavior changes between these versions on > the same repository. That's what I mean by lack of stability. If you want that forward-stability wouldn't another way to accomplish this to put all of these new magical refs in the same refs/git-internal/ namespace, e.g. refs/git-internal/foo/, refs/git-internal/bar/? >> But it's not just that I disagree, I genuinely don't get your POV >> here. > > I'm optimizing for non-experts who never need any refs outside of the > standard set. Clearly, I'm wondering if we can find a way forward that doesn't change existing long-statnding behavior and caters to the "stability" of future inter-version behavior with this new class of refs. > Now that this version removed the notes ref from the > decoration, the stance for inclusion is simple: > > If Git offers to color the namespace with color.decoration.<slot>, > then Git decorates with that namespace by default. I'm a bit confused, sorry. So aside from "notes", if we have a color.decoration.<slot> applying to a ref now, it's a bug in your series if it's not showing up anymore? I still find that inclusion criteria to be a bit odd, we've usually considered colors optional sugar. Just because nobody bothered to add a coloring config for it doesn't seem like a good reason to not show something at all.
On 8/5/2022 10:50 AM, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Aug 05 2022, Derrick Stolee wrote: >> Now that this version removed the notes ref from the >> decoration, the stance for inclusion is simple: >> >> If Git offers to color the namespace with color.decoration.<slot>, >> then Git decorates with that namespace by default. > > I'm a bit confused, sorry. > > So aside from "notes", if we have a color.decoration.<slot> applying to > a ref now, it's a bug in your series if it's not showing up anymore? The possible slots are: * branch (refs/heads/) * remoteBranch (refs/remotes/) * tag (refs/tags/) * stash (refs/stash) * HEAD (HEAD) * grafted (refs/replace/ or GIT_REPLACE_REF_BASE) These are exactly the namespaces that are now shown by default in this series. If someone adds a new color slot, then that would need to be justified with a reason why that slot is important. We've already had discussions as to why showing a decoration for notes is not valuable to an end user. The stability of this config option (the last addition of 'grafted' was in 2011) is good evidence that this core set of namespaces is all users will need. By contrast, the use of "hidden" namespaces is relatively new and is expected to increase in the near future. -Stolee
On Fri, Aug 05 2022, Derrick Stolee wrote: > On 8/5/2022 10:50 AM, Ævar Arnfjörð Bjarmason wrote: >> >> On Fri, Aug 05 2022, Derrick Stolee wrote: > >>> Now that this version removed the notes ref from the >>> decoration, the stance for inclusion is simple: >>> >>> If Git offers to color the namespace with color.decoration.<slot>, >>> then Git decorates with that namespace by default. >> >> I'm a bit confused, sorry. >> >> So aside from "notes", if we have a color.decoration.<slot> applying to >> a ref now, it's a bug in your series if it's not showing up anymore? > > The possible slots are: > > * branch (refs/heads/) > * remoteBranch (refs/remotes/) > * tag (refs/tags/) > * stash (refs/stash) > * HEAD (HEAD) > * grafted (refs/replace/ or GIT_REPLACE_REF_BASE) > > These are exactly the namespaces that are now shown by default in > this series. No, e.g. "tag" doesn't mean "refs/tags/*", it means *a tag object*. Try this on master: git update-ref refs/archived-tags/v2.36.0 refs/tags/v2.36.0 Then on master: ./git -P -c color.decorate.tag="bold blue" log --oneline -1 v2.36.0 6cd33dceed6 (tag: v2.36.0, gitster/yw/cmake-use-pcre2, gitgitgadget/yw/cmake-use-pcre2, tag: refs/archived-tags/v2.36.0) Git 2.36 But on "seen" currently: $ ./git -P -c color.decorate.tag="bold blue" log --oneline -1 v2.36.0 6cd33dceed6 (tag: v2.36.0, gitster/yw/cmake-use-pcre2, gitgitgadget/yw/cmake-use-pcre2) Git 2.36 Before that "bold blue" applied to *tag objects*, but your series has made it apply to the refs/tags/* namespace. I noted this (indirectly) before in https://lore.kernel.org/git/220726.86tu73ncf8.gmgdl@evledraar.gmail.com/; I.e. that I have a "refs/built-tags/" namespace. So that specifically seems like a regression by the criteria you've established for inclusion. I.e. we have objects that are impacted by existing coloring config now that your series is hiding, seemingly because you've conflated "tag object" with "a name in in refs/tags/". I *also* think it's overzelous to hide *uknown* things by default because we think we might add more *known* internal things in the future, but that's a distinct topic from this more narrow case, which seems to be a clear regression by criteria you're establishing & advocating for. > If someone adds a new color slot, then that would need to be > justified with a reason why that slot is important. We've already > had discussions as to why showing a decoration for notes is not > valuable to an end user. The stability of this config option (the > last addition of 'grafted' was in 2011) is good evidence that this > core set of namespaces is all users will need. > > By contrast, the use of "hidden" namespaces is relatively new and > is expected to increase in the near future. I really don't see how you're making the leap that because nobody's bothered to customize the coloring for things in custom namespaces that it's OK to hide them entirely. I just leave everything at the default color settings, aside from (after checking my ~/.gitconfig) one bit of diff coloring default that I found annoying.
On 8/11/2022 3:30 PM, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Aug 05 2022, Derrick Stolee wrote: > >> On 8/5/2022 10:50 AM, Ævar Arnfjörð Bjarmason wrote: >>> >>> On Fri, Aug 05 2022, Derrick Stolee wrote: >> >>>> Now that this version removed the notes ref from the >>>> decoration, the stance for inclusion is simple: >>>> >>>> If Git offers to color the namespace with color.decoration.<slot>, >>>> then Git decorates with that namespace by default. >>> >>> I'm a bit confused, sorry. >>> >>> So aside from "notes", if we have a color.decoration.<slot> applying to >>> a ref now, it's a bug in your series if it's not showing up anymore? >> >> The possible slots are: >> >> * branch (refs/heads/) >> * remoteBranch (refs/remotes/) >> * tag (refs/tags/) >> * stash (refs/stash) >> * HEAD (HEAD) >> * grafted (refs/replace/ or GIT_REPLACE_REF_BASE) >> >> These are exactly the namespaces that are now shown by default in >> this series. > > No, e.g. "tag" doesn't mean "refs/tags/*", it means *a tag object*. You are half right. It means "refs/tags/*" _or_ "a tag object". A lightweight tag is still counted based only on its ref namespace. > Try > this on master: > > git update-ref refs/archived-tags/v2.36.0 refs/tags/v2.36.0 > > Then on master: > > ./git -P -c color.decorate.tag="bold blue" log --oneline -1 v2.36.0 > 6cd33dceed6 (tag: v2.36.0, gitster/yw/cmake-use-pcre2, gitgitgadget/yw/cmake-use-pcre2, tag: refs/archived-tags/v2.36.0) Git 2.36 > > But on "seen" currently: > > $ ./git -P -c color.decorate.tag="bold blue" log --oneline -1 v2.36.0 > 6cd33dceed6 (tag: v2.36.0, gitster/yw/cmake-use-pcre2, gitgitgadget/yw/cmake-use-pcre2) Git 2.36 You initially made me think there was a bug that was not covered by our existing test infrastructure. We may still be missing a test about this "annotated tag outside of refs/tags/*" behavior, but it is not broken with this series. The change of behavior here is that refs/archived-tags is excluded from the list of decorations. If you applied --clear-decorations to your log command, then it would appear. It would also receive the bold blue coloring (and the "tag:" prefix) since it points to an annotated tag object. I also tested non-annotated tags in the refs/tags/* space and the coloring continues to work correctly before and after this change. Thanks, -Stolee
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..0b5efc9434c 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 < NAMESPACE__COUNT; i++) { + if (!ref_namespaces[i].decoration) + continue; + + string_list_append(include, ref_namespaces[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..9ea9e835d43 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,22 @@ 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 && + + for ref in $reflist + do + ! grep $ref/fake actual || return 1 + done +' + test_expect_success 'log --end-of-options' ' git update-ref refs/heads/--source HEAD && git log --end-of-options --source >actual &&