git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] log: add log.excludeDecoration config option

From
Taylor Blau <me@ttaylorr.com>
Date
Apr 13, 2020, 15:49 UTC
Message-ID
<20200413154945.GA59601@syl.local>
In-Reply-To
<pull.610.git.1586791720114.gitgitgadget@gmail.com>
Hi Stolee,
On Mon, Apr 13, 2020 at 03:28:39PM +0000, Derrick Stolee via GitGitGadget wrote:
Show 66 quoted lines
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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/
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.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(+)
>
> diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
> index e9e1e397f3f..1a158324f79 100644
> --- a/Documentation/config/log.txt
> +++ b/Documentation/config/log.txt
> @@ -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.

Show 20 quoted lines
>  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`,
> diff --git a/builtin/log.c b/builtin/log.c
> index 83a4a6188e2..d7d1d5b7143 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -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++)

Any reason not to use the 'for_each_string_list_item' macro in 'string-list.h' for this?

> +				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.

Show 66 quoted lines
>  		rev->show_decorations = 1;
> +
>  		load_ref_decorations(&decoration_filter, decoration_style);
>  	}
>
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 0f766ba65f5..b5de449e510 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -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
>  '
>

Thanks for the tests, they look good. I like your strategy of repeating the test with and without the new config settings.

>
> base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925
> --
> gitgitgadget

Thanks, Taylor

Previous: Derrick Stolee via GitGitGadgetNext: Derrick Stolee
Message 2 of 22 in “log: add log.excludeDecoration config option”
  1. log: add log.excludeDecoration config optionDerrick Stolee via GitGitGadget, Apr 13, 2020
  2. Taylor BlauApr 13, 2020
  3. Derrick StoleeApr 14, 2020
  4. Taylor BlauApr 14, 2020
  5. Derrick StoleeApr 14, 2020
  6. Junio C HamanoApr 14, 2020
  7. Derrick StoleeApr 14, 2020
  8. Junio C HamanoApr 14, 2020
  9. Derrick StoleeApr 15, 2020
  10. log: add log.excludeDecoration config optionDerrick Stolee via GitGitGadget, Apr 15, 2020
  11. Taylor BlauApr 15, 2020
  12. Junio C HamanoApr 15, 2020
  13. Junio C HamanoApr 15, 2020
  14. Derrick StoleeApr 16, 2020
  15. Derrick StoleeApr 16, 2020
  16. 0/2 log: add log.excludeDecoration config optionDerrick Stolee via GitGitGadget, Apr 16, 2020
  17. 2/2 log: add log.excludeDecoration config optionDerrick Stolee via GitGitGadget, Apr 16, 2020
  18. Junio C HamanoApr 16, 2020
  19. Junio C HamanoApr 16, 2020
  20. Derrick StoleeApr 17, 2020
  21. Junio C HamanoApr 17, 2020
  22. 1/2 log-tree: make ref_filter_match() a helper methodDerrick Stolee via GitGitGadget, Apr 16, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.