[v2,00/10] log: create tighter default decoration filter

Message

Philippe Blain via GitGitGadget July 29, 2022, 7:29 p.m. UTC

When running 'git log', the default is to report any refs within refs/* as
decorations. This series reduces the default set to be more specific to a
set of known-useful namespaces.

This was previously reduced by adding the log.excludeDecoration config
option and modifying that config in git maintenance's prefetch task (to hide
refs/prefetch/*). I then followed that pattern again for the bundle URI
feature [1], but this caught some reviewers by surprise as an unfortunate
side-effect. This series is a way to roll back the previous decision to use
log.excludeDecoration and instead use tighter filters by default.

[1]
https://lore.kernel.org/git/a217[e-mail]/

The current design ignores the new filters if there are any
previously-specified filters. This includes the
log.excludeDecorations=refs/prefetch/ set by the git maintenance command.
This means that users who ran that command in their repo will not get the
benefits of the more strict filters. While we stop writing
log.excludeDecorations, we don't remove existing instances of it. This
should be fine at least for the refs/bundles/ namespace in the future, since
those refs will typically be created for new clones that have not yet had
the log.excludeDecoration setting.

To make it easy for users to opt-in to the previous behavior, add a new git
log --decorate-all option and log.decorateFilter=all config option.


Updates in v2
=============

 * As discussed in the previous version, there are a lot of places that care
   about different subsets of the special ref namespaces. This version
   creates a new ref_namespaces array that centralizes and describes how
   these known ref namespaces interact with other Git features.

 * Some patches are added to help centralize previously-unrelated features
   onto the ref_namespaces array.

 * One thing that I noticed while doing this work was that notes come from a
   commit history that is pointed by a single ref, not multiple refs in a
   namespace. Also, that ref can change based on config and environment
   variables. This version updates that knowledge to allow our
   ref_namespaces[NAMESPACE_NOTES] value change with these settings. Since
   there is only one ref, I also chose to remove it from the default
   decorations. As Junio mentioned, this should not make any difference
   since a user won't see it unless they purposefully start git log from the
   notes ref.

 * Despite feedback to the opposite, I maintain my opinion that this default
   filter should be a small list of known-useful namespaces. For expert
   users who use other namespaces, I added patches that make it easier to
   show custom namespaces (specifically --decorate-all and
   log.decorateFilter=all). The logic for creating the default filter now
   uses the ref_namespaces array in such a way that it would be simple to
   reverse this decision, and I include that alternate implementation in a
   commit message.

 * This version includes several code and test style improvements.

 * The final patch demonstrates how we can remove some duplicate string
   literals for these namespaces. It is focused on the prefetch namespace
   (and an adjacent use of the tags namespace) because of how this series
   changes our handling of that namespace in particular. This makes it
   easier for a future change to modify the namespace via config or
   environment variables. There are opportunities to do this kind of update
   more widely, but I don't want to set that as a critical refactor to do
   right now.

Thanks, -Stolee

Derrick Stolee (10):
  refs: allow "HEAD" as decoration filter
  t4207: test coloring of grafted decorations
  refs: add array of ref namespaces
  refs: use ref_namespaces for replace refs base
  log-tree: use ref_namespaces instead of if/else-if
  log: add default decoration filter
  log: add --decorate-all option
  log: create log.decorateFilter=all
  maintenance: stop writing log.excludeDecoration
  fetch: use ref_namespaces during prefetch

 Documentation/config/log.txt                  |   5 +
 Documentation/git-log.txt                     |  14 +-
 builtin/fetch.c                               |   6 +-
 builtin/gc.c                                  |   6 -
 builtin/log.c                                 |  87 +++++++++---
 builtin/replace.c                             |   3 +
 cache.h                                       |   1 -
 environment.c                                 |   5 +-
 log-tree.c                                    |  28 ++--
 notes.c                                       |   1 +
 refs.c                                        |  88 +++++++++++-
 refs.h                                        |  46 ++++++
 t/t4013-diff-various.sh                       |   2 +
 t/t4013/diff.log_--decorate=full_--all        |   2 +-
 ...f.log_--decorate=full_--decorate-all_--all |  61 ++++++++
 t/t4013/diff.log_--decorate_--all             |   2 +-
 .../diff.log_--decorate_--decorate-all_--all  |  61 ++++++++
 t/t4202-log.sh                                | 134 +++++++++++++++++-
 t/t4207-log-decoration-colors.sh              |  59 ++++++++
 t/t7900-maintenance.sh                        |  21 ---
 t/t9902-completion.sh                         |   3 +
 21 files changed, 568 insertions(+), 67 deletions(-)
 create mode 100644 t/t4013/diff.log_--decorate=full_--decorate-all_--all
 create mode 100644 t/t4013/diff.log_--decorate_--decorate-all_--all


base-commit: 6a475b71f8c4ce708d69fdc9317aefbde3769e25
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1301%2Fderrickstolee%2Fdecorations-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1301/derrickstolee/decorations-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1301

Range-diff vs v1:

  1:  c2e5a0b3a50 =  1:  c2e5a0b3a50 refs: allow "HEAD" as decoration filter
  -:  ----------- >  2:  b5eb110958b t4207: test coloring of grafted decorations
  -:  ----------- >  3:  d7486390d57 refs: add array of ref namespaces
  -:  ----------- >  4:  8da0b0a3181 refs: use ref_namespaces for replace refs base
  -:  ----------- >  5:  53b15a0b793 log-tree: use ref_namespaces instead of if/else-if
  2:  6b40b84773c !  6:  bec532fb8c6 log: add default decoration filter
     @@ Commit message
      
          * The HEAD ref
          * Branches (refs/heads/)
     -    * Notes (refs/notes/)
     -    * Stashes (refs/stash/)
     +    * 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.
     +
     +    While one test checks for a decoration from notes, it only applies to
     +    the tip of refs/notes/commit (or its configured ref name). Notes form
     +    their own kind of decoration instead. Modify the expected output for the
     +    tests in t4013 that expect this note decoration.  There are several
     +    tests throughout the codebase that verify that --decorate-refs,
     +    --decorate-refs-exclude, and log.excludeDecoration work as designed and
     +    the tests continue to pass without intervention.
      
          However, there are other refs that are less helpful to show as
          decoration:
     @@ Commit message
          * Rebase refs (refs/rebase-merge/ and refs/rebase-apply/)
          * Bundle refs (refs/bundle/) [!]
      
     +    [!] The bundle refs are part of a parallel series that bootstraps a repo
     +        from a bundle file, storing the bundle's refs into the repo's
     +        refs/bundle/ namespace.
     +
          In the case of prefetch refs, 96eaffebbf3d0 (maintenance: set
          log.excludeDecoration durin prefetch, 2021-01-19) added logic to add
          refs/prefetch/ to the log.excludeDecoration config option. Additional
     @@ Commit message
          from log.excludeDecoration, then checks if the list of pattern
          modifications are empty. If none are specified, then the default set is
          restricted to the set of inclusions mentioned earlier (HEAD, branches,
     -    etc.).
     +    etc.).  A previous change introduced the ref_namespaces array, which
     +    includes all of these currently-used namespaces. The 'decoration' value
     +    is non-zero when that namespace is associated with a special coloring
     +    and fits into the list of "expected" decorations as described above,
     +    which makes the implementation of this filter very simple.
      
          Note that the logic in ref_filter_match() in log-tree.c follows this
          matching pattern:
     @@ Commit message
          refs outside of the defaults while also excluding some using
          log.excludeDecoration.
      
     +    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.
     +
     +    It is critical that we provide ways for expert users to disable this
     +    behavior change via command-line options and config keys. These changes
     +    will be implemented in a future change.
     +
          Add a test that checks that the defaults are not added when
          --decorate-refs is specified. We verify this by showing that HEAD is not
          included as it normally would.  Also add a test that shows that the
     @@ Commit message
          refs/rebase-merge,
          and refs/bundle.
      
     -    There are several tests throughout the codebase that verify that
     -    --decorate-refs, --decorate-refs-exclude, and log.excludeDecoration work
     -    as designed and the tests continue to pass without intervention.
     -
     -    [!] The bundle refs are part of a parallel series that bootstraps a repo
     -    from a bundle file, storing the bundle's refs into the repo's
     -    refs/bundle/ namespace.
     -
          Signed-off-by: Derrick Stolee <[e-mail]>
      
       ## Documentation/git-log.txt ##
     @@ Documentation/git-log.txt: OPTIONS
       	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/notes/`,
     -+`refs/remotes/`, `refs/stash/`, or `refs/tags/`.
     ++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
     @@ builtin/log.c: static void cmd_log_init_defaults(struct rev_info *rev)
       
      +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 =
     -+		repo_config_get_value_multi(the_repository, "log.excludeDecoration");
     ++			git_config_get_value_multi("log.excludeDecoration");
      +
      +	if (config_exclude) {
      +		struct string_list_item *item;
     @@ builtin/log.c: static void cmd_log_init_defaults(struct rev_info *rev)
      +					   item->string);
      +	}
      +
     -+	if (!decoration_filter->exclude_ref_pattern->nr &&
     -+	    !decoration_filter->include_ref_pattern->nr &&
     -+	    !decoration_filter->exclude_ref_config_pattern->nr) {
     -+		/*
     -+		 * No command-line or config options were given, so
     -+		 * populate with sensible defaults.
     -+		 */
     -+		string_list_append(decoration_filter->include_ref_pattern,
     -+				   "refs/heads/");
     -+		string_list_append(decoration_filter->include_ref_pattern,
     -+				   "refs/notes/");
     -+		string_list_append(decoration_filter->include_ref_pattern,
     -+				   "refs/stash/");
     -+		string_list_append(decoration_filter->include_ref_pattern,
     -+				   "refs/tags/");
     -+		string_list_append(decoration_filter->include_ref_pattern,
     -+				   "refs/remotes/");
     -+		string_list_append(decoration_filter->include_ref_pattern,
     -+				   "HEAD");
     ++	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);
      +	}
      +}
      +
     @@ builtin/log.c: static void cmd_log_init_finish(int argc, const char **argv, cons
       		if (decoration_style)
       			rev->show_decorations = 1;
      
     + ## t/t4013/diff.log_--decorate=full_--all ##
     +@@ t/t4013/diff.log_--decorate=full_--all: 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 <[e-mail]>
     + Date:   Mon Jun 26 00:06:00 2006 +0000
     + 
     +
     + ## t/t4013/diff.log_--decorate_--all ##
     +@@ t/t4013/diff.log_--decorate_--all: 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 <[e-mail]>
     + Date:   Mon Jun 26 00:06:00 2006 +0000
     + 
     +
       ## t/t4202-log.sh ##
      @@ t/t4202-log.sh: test_expect_success 'decorate-refs-exclude HEAD' '
       	! grep HEAD actual
     @@ t/t4202-log.sh: test_expect_success 'log --decorate includes all levels of tag a
       '
       
      +test_expect_success 'log --decorate does not include things outside filter' '
     -+	reflist="refs/prefetch/ refs/rebase-merge/ refs/bundle/" &&
     ++	reflist="refs/prefetch refs/rebase-merge refs/bundle" &&
      +
      +	for ref in $reflist
      +	do
     -+		mkdir -p .git/$ref &&
     -+		echo $(git rev-parse HEAD) >.git/$ref/fake || return 1
     ++		git update-ref $ref/fake HEAD || return 1
      +	done &&
      +
      +	git log --decorate=full --oneline >actual &&
  -:  ----------- >  7:  64ee889369d log: add --decorate-all option
  -:  ----------- >  8:  8142b32f023 log: create log.decorateFilter=all
  3:  84fbf16613d =  9:  318269dfe27 maintenance: stop writing log.excludeDecoration
  -:  ----------- > 10:  8599bb55045 fetch: use ref_namespaces during prefetch