Simplify-by-decoration with decorate-refs-exclude

Simplify-by-decoration with decorate-refs-exclude

[Étienne SERVAIS]

Hi there,

I've asked this question yesterday on stackoverflow: 
https://stackoverflow.com/q/57305719/2622010
And have been confirmed it does look like a bug. 
Unfortunately I'm unable to update to latest git revision but a search through the release notes didn't show anything related to this problem. 

I'm working in a git (v2.19.1) repo with lots of tags and branches. To get a glance of the git tree, I'd like to use the `--simplify-by-decoration` option while excluding some of the tags with `--decorate-refs-exclude=<pattern>` of `git log`, but, as per [the documentation](https://git-scm.com/docs/git-log/2.22.0#Documentation/git-log.txt---simplify-by-decoration):

> --simplify-by-decoration
> 
>     Commits that are referred by some branch or tag are selected.

Thus every tags are selected even those that are excluded from the decoration by the pattern.

Thus, when I enter

```
git log --oneline --graph  --decorate=full --decorate-refs-exclude='refs/tags/<pattern>'
```
The selected tags are properly excluded but once I add the `simplify-by-decoration` option

```
git log --oneline --graph  --decorate=full --decorate-refs-exclude='refs/tags/<pattern>' --simplify-by-decoration
```
The excluded tags pop back again.

Is there a workaround?

Étienne
--
+33 6 45 98 22 65
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
--
+33 6 45 98 22 65
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.

Re: Simplify-by-decoration with decorate-refs-exclude

[René Scharfe]

Am 02.08.19 um 10:47 schrieb Étienne SERVAIS:
> Thus, when I enter
>
> ```
> git log --oneline --graph  --decorate=full --decorate-refs-exclude='refs/tags/<pattern>'
> ```
> The selected tags are properly excluded but once I add the
> `simplify-by-decoration` option
>
> ```
> git log --oneline --graph  --decorate=full --decorate-refs-exclude='refs/tags/<pattern>' --simplify-by-decoration
> ```
> The excluded tags pop back again.

Does this help?

-- >8 --
Subject: [PATCH] revision: load decorations lazily for --simplify-by-decoration

Let setup_revisions() and friends respect a filtered set of decoration
refs loaded by callers by postponing its own load_ref_decorations() call
to just before decorations are used to simplify history.  That function
only does any actual work the first time it is called.

This allows using the revision option --simplify-by-decoration together
with the log option --decorate-refs-exclude and having it simplify over
the restricted set of refs.

Reported-by: Étienne SERVAIS <etienne.servais@voucoux.fr>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 revision.c     |  8 +++++++-
 t/t4202-log.sh | 15 +++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 07412297f0..d3456c959b 100644
--- a/revision.c
+++ b/revision.c
@@ -633,6 +633,13 @@ static int rev_compare_tree(struct rev_info *revs,
 		return REV_TREE_OLD;

 	if (revs->simplify_by_decoration) {
+		/*
+		 * Load decorations lazily; later calls have no effect.
+		 * This gives callers a chance to load a restricted set
+		 * beforehand.
+		 */
+		load_ref_decorations(NULL, DECORATE_SHORT_REFS);
+
 		/*
 		 * If we are simplifying by decoration, then the commit
 		 * is worth showing if it has a tag pointing at it.
@@ -2063,7 +2070,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->simplify_by_decoration = 1;
 		revs->limited = 1;
 		revs->prune = 1;
-		load_ref_decorations(NULL, DECORATE_SHORT_REFS);
 	} else if (!strcmp(arg, "--date-order")) {
 		revs->sort_order = REV_SORT_BY_COMMIT_DATE;
 		revs->topo_order = 1;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index c20209324c..bb66d1d93c 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -837,6 +837,21 @@ test_expect_success 'decorate-refs and decorate-refs-exclude' '
 	test_cmp expect.decorate actual
 '

+test_expect_success 'decorate-refs-exclude and simplify-by-decoration' '
+	cat >expect.decorate <<-\EOF &&
+	Merge-tag-reach (HEAD -> master)
+	reach (tag: reach, reach)
+	seventh (tag: seventh)
+	Merge-branch-tangle
+	Merge-branch-side-early-part-into-tangle (tangle)
+	tangle-a (tag: tangle-a)
+	EOF
+	git log -n6 --decorate=short --pretty="tformat:%f%d" \
+		--decorate-refs-exclude="*octopus*" \
+		--simplify-by-decoration >actual &&
+	test_cmp expect.decorate actual
+'
+
 test_expect_success 'log.decorate config parsing' '
 	git log --oneline --decorate=full >expect.full &&
 	git log --oneline --decorate=short >expect.short &&
--
2.22.0

Re: Simplify-by-decoration with decorate-refs-exclude

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 02.08.19 um 10:47 schrieb Étienne SERVAIS:
>> Thus, when I enter
>>
>> ```
>> git log --oneline --graph  --decorate=full --decorate-refs-exclude='refs/tags/<pattern>'
>> ```
>> The selected tags are properly excluded but once I add the
>> `simplify-by-decoration` option
>>
>> ```
>> git log --oneline --graph  --decorate=full --decorate-refs-exclude='refs/tags/<pattern>' --simplify-by-decoration
>> ```
>> The excluded tags pop back again.
>
> Does this help?

I can see how this would help, but it somehow feels a bit brittle
to rely on where the decorations get loaded.

I wonder if it would help to move the ability to handle decoration
filter down from the log layer to revisions.c API layer.

It looks to me that this caller of setup_revisions() can prepare
decoration_filter before it calls setup_revisions(); we can let the
revisions.c layer call load_ref_decorations() in setup_revisions()
if that is the case, no?

Other two callers of load_ref_decorations() are deep inside pretty.c
but I wonder in the longer term if we would want to turn them into
an "a lot higher level should have already loaded decorations"
assert.

Thanks.

> -- >8 --
> Subject: [PATCH] revision: load decorations lazily for --simplify-by-decoration
>
> Let setup_revisions() and friends respect a filtered set of decoration
> refs loaded by callers by postponing its own load_ref_decorations() call
> to just before decorations are used to simplify history.  That function
> only does any actual work the first time it is called.
>
> This allows using the revision option --simplify-by-decoration together
> with the log option --decorate-refs-exclude and having it simplify over
> the restricted set of refs.
>
> Reported-by: Étienne SERVAIS <etienne.servais@voucoux.fr>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  revision.c     |  8 +++++++-
>  t/t4202-log.sh | 15 +++++++++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/revision.c b/revision.c
> index 07412297f0..d3456c959b 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -633,6 +633,13 @@ static int rev_compare_tree(struct rev_info *revs,
>  		return REV_TREE_OLD;
>
>  	if (revs->simplify_by_decoration) {
> +		/*
> +		 * Load decorations lazily; later calls have no effect.
> +		 * This gives callers a chance to load a restricted set
> +		 * beforehand.
> +		 */
> +		load_ref_decorations(NULL, DECORATE_SHORT_REFS);
> +
>  		/*
>  		 * If we are simplifying by decoration, then the commit
>  		 * is worth showing if it has a tag pointing at it.
> @@ -2063,7 +2070,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  		revs->simplify_by_decoration = 1;
>  		revs->limited = 1;
>  		revs->prune = 1;
> -		load_ref_decorations(NULL, DECORATE_SHORT_REFS);
>  	} else if (!strcmp(arg, "--date-order")) {
>  		revs->sort_order = REV_SORT_BY_COMMIT_DATE;
>  		revs->topo_order = 1;
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index c20209324c..bb66d1d93c 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -837,6 +837,21 @@ test_expect_success 'decorate-refs and decorate-refs-exclude' '
>  	test_cmp expect.decorate actual
>  '
>
> +test_expect_success 'decorate-refs-exclude and simplify-by-decoration' '
> +	cat >expect.decorate <<-\EOF &&
> +	Merge-tag-reach (HEAD -> master)
> +	reach (tag: reach, reach)
> +	seventh (tag: seventh)
> +	Merge-branch-tangle
> +	Merge-branch-side-early-part-into-tangle (tangle)
> +	tangle-a (tag: tangle-a)
> +	EOF
> +	git log -n6 --decorate=short --pretty="tformat:%f%d" \
> +		--decorate-refs-exclude="*octopus*" \
> +		--simplify-by-decoration >actual &&
> +	test_cmp expect.decorate actual
> +'
> +
>  test_expect_success 'log.decorate config parsing' '
>  	git log --oneline --decorate=full >expect.full &&
>  	git log --oneline --decorate=short >expect.short &&
> --
> 2.22.0

Re: Simplify-by-decoration with decorate-refs-exclude

[René Scharfe]

Am 02.08.19 um 21:14 schrieb Junio C Hamano:
> I can see how this would help, but it somehow feels a bit brittle
> to rely on where the decorations get loaded.

Right.

> I wonder if it would help to move the ability to handle decoration
> filter down from the log layer to revisions.c API layer.
>
> It looks to me that this caller of setup_revisions() can prepare
> decoration_filter before it calls setup_revisions(); we can let the
> revisions.c layer call load_ref_decorations() in setup_revisions()
> if that is the case, no?

Having cmd_log_init_finish() call load_ref_decorations() before
setup_revisions() would indeed solve the issue as well.  But we need
to call the latter to check if --pretty=raw was given and avoid loading
decorations in that case, don't we?

> Other two callers of load_ref_decorations() are deep inside pretty.c
> but I wonder in the longer term if we would want to turn them into
> an "a lot higher level should have already loaded decorations"
> assert.

This would require that higher level to parse the user format to check
if %d or %D is present before formatting the first item.  Hmm.

René

Re: Simplify-by-decoration with decorate-refs-exclude

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Having cmd_log_init_finish() call load_ref_decorations() before
> setup_revisions() would indeed solve the issue as well.  But we need
> to call the latter to check if --pretty=raw was given and avoid loading
> decorations in that case, don't we?

I was thinking about giving an instance of the decoration_filter to
either rev_info or setup_revision_opt, and moving the call to
load_ref_decorations() and the decision to make that call from
cmd_log_init_finish() to setup_revisions().

>> Other two callers of load_ref_decorations() are deep inside pretty.c
>> but I wonder in the longer term if we would want to turn them into
>> an "a lot higher level should have already loaded decorations"
>> assert.
>
> This would require that higher level to parse the user format to check
> if %d or %D is present before formatting the first item.  Hmm.

Yes.  Don't we pre-scan what kind of formatting primitives are used
in the end-user supplied string already to optimize loading of notes
and source information?

Re: Simplify-by-decoration with decorate-refs-exclude

[Jeff King]

On Fri, Aug 02, 2019 at 02:20:31PM -0700, Junio C Hamano wrote:

> > This would require that higher level to parse the user format to check
> > if %d or %D is present before formatting the first item.  Hmm.
> 
> Yes.  Don't we pre-scan what kind of formatting primitives are used
> in the end-user supplied string already to optimize loading of notes
> and source information?

I think userformat_find_requirements() is what you're looking for.

I do think it might be tricky to find all of the callers who need to use
it, though. Some of them are not necessarily users of the traversal
machinery, but just have a one-off pretty_print_context.

-Peff

Re: Simplify-by-decoration with decorate-refs-exclude

[René Scharfe]

Am 02.08.19 um 23:20 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Having cmd_log_init_finish() call load_ref_decorations() before
>> setup_revisions() would indeed solve the issue as well.  But we need
>> to call the latter to check if --pretty=raw was given and avoid loading
>> decorations in that case, don't we?
>
> I was thinking about giving an instance of the decoration_filter to
> either rev_info or setup_revision_opt, and moving the call to
> load_ref_decorations() and the decision to make that call from
> cmd_log_init_finish() to setup_revisions().

Sure, but we'd need to move the code to handle the raw format as well, no?
Perhaps like this?  It depends on callers of parse_revision_opt() calling
setup_revisions() before using decorations.  And it may have other side
effects; I'm not comfortable with this change.

---
 builtin/log.c  | 10 +++++-----
 revision.c     | 17 ++++++++++++++++-
 revision.h     |  4 ++++
 t/t4202-log.sh | 15 +++++++++++++++
 4 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 1cf9e37736..5a1544276f 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -203,6 +203,10 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
 			     PARSE_OPT_KEEP_DASHDASH);

+	rev->decoration_filter = &decoration_filter;
+	rev->decoration_given = decoration_given;
+	rev->decoration_style = decoration_style;
+
 	if (quiet)
 		rev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;
 	argc = setup_revisions(argc, argv, rev, opt);
@@ -245,16 +249,12 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 		 * "log --pretty=raw" is special; ignore UI oriented
 		 * configuration variables such as decoration.
 		 */
-		if (!decoration_given)
-			decoration_style = 0;
 		if (!rev->abbrev_commit_given)
 			rev->abbrev_commit = 0;
 	}

-	if (decoration_style) {
+	if (rev->decoration_style)
 		rev->show_decorations = 1;
-		load_ref_decorations(&decoration_filter, decoration_style);
-	}

 	if (rev->line_level_traverse)
 		line_log_init(rev, line_cb.prefix, &line_cb.args);
diff --git a/revision.c b/revision.c
index 07412297f0..709d6a273c 100644
--- a/revision.c
+++ b/revision.c
@@ -2063,7 +2063,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->simplify_by_decoration = 1;
 		revs->limited = 1;
 		revs->prune = 1;
-		load_ref_decorations(NULL, DECORATE_SHORT_REFS);
 	} else if (!strcmp(arg, "--date-order")) {
 		revs->sort_order = REV_SORT_BY_COMMIT_DATE;
 		revs->topo_order = 1;
@@ -2716,6 +2715,22 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (revs->expand_tabs_in_log < 0)
 		revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;

+	if (revs->pretty_given && revs->commit_format == CMIT_FMT_RAW) {
+		/*
+		 * "log --pretty=raw" is special; ignore UI oriented
+		 * configuration variables such as decoration.
+		 */
+		if (!revs->decoration_given)
+			revs->decoration_style = 0;
+	}
+
+	if (revs->decoration_style)
+		load_ref_decorations(revs->decoration_filter,
+				     revs->decoration_style);
+	else if (revs->simplify_by_decoration)
+		load_ref_decorations(revs->decoration_filter,
+				     DECORATE_SHORT_REFS);
+
 	return left;
 }

diff --git a/revision.h b/revision.h
index 4134dc6029..67ffe095a9 100644
--- a/revision.h
+++ b/revision.h
@@ -186,6 +186,7 @@ struct rev_info {
 			pretty_given:1,
 			abbrev_commit:1,
 			abbrev_commit_given:1,
+			decoration_given:1,
 			zero_commit:1,
 			use_terminator:1,
 			missing_newline:1,
@@ -269,6 +270,9 @@ struct rev_info {
 	/* line level range that we are chasing */
 	struct decoration line_log_data;

+	struct decoration_filter *decoration_filter;
+	int decoration_style;
+
 	/* copies of the parent lists, for --full-diff display */
 	struct saved_parents *saved_parents_slab;

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index c20209324c..bb66d1d93c 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -837,6 +837,21 @@ test_expect_success 'decorate-refs and decorate-refs-exclude' '
 	test_cmp expect.decorate actual
 '

+test_expect_success 'decorate-refs-exclude and simplify-by-decoration' '
+	cat >expect.decorate <<-\EOF &&
+	Merge-tag-reach (HEAD -> master)
+	reach (tag: reach, reach)
+	seventh (tag: seventh)
+	Merge-branch-tangle
+	Merge-branch-side-early-part-into-tangle (tangle)
+	tangle-a (tag: tangle-a)
+	EOF
+	git log -n6 --decorate=short --pretty="tformat:%f%d" \
+		--decorate-refs-exclude="*octopus*" \
+		--simplify-by-decoration >actual &&
+	test_cmp expect.decorate actual
+'
+
 test_expect_success 'log.decorate config parsing' '
 	git log --oneline --decorate=full >expect.full &&
 	git log --oneline --decorate=short >expect.short &&
--
2.22.0

Re: Simplify-by-decoration with decorate-refs-exclude

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Sure, but we'd need to move the code to handle the raw format as well, no?
> Perhaps like this?  It depends on callers of parse_revision_opt() calling
> setup_revisions() before using decorations.  And it may have other side
> effects; I'm not comfortable with this change.

Hmph, fair enough.  I missed the fact that we do want to keep
"--pretty=raw" being "special" but only in "log"; a move in this
direction teaches the revision walk API layer about it, which is
probably not a good idea.

Thanks for sanity checking.

[PATCH 1/2] log: test --decorate-refs-exclude with --simplify-by-decoration

[René Scharfe]

Demonstrate that a decoration filter given with --decorate-refs-exclude
is inadvertently overruled by --simplify-by-decoration.

Reported-by: Étienne SERVAIS <etienne.servais@voucoux.fr>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 t/t4202-log.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index c20209324c..01c95d1375 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -837,6 +837,21 @@ test_expect_success 'decorate-refs and decorate-refs-exclude' '
 	test_cmp expect.decorate actual
 '

+test_expect_failure 'decorate-refs-exclude and simplify-by-decoration' '
+	cat >expect.decorate <<-\EOF &&
+	Merge-tag-reach (HEAD -> master)
+	reach (tag: reach, reach)
+	seventh (tag: seventh)
+	Merge-branch-tangle
+	Merge-branch-side-early-part-into-tangle (tangle)
+	tangle-a (tag: tangle-a)
+	EOF
+	git log -n6 --decorate=short --pretty="tformat:%f%d" \
+		--decorate-refs-exclude="*octopus*" \
+		--simplify-by-decoration >actual &&
+	test_cmp expect.decorate actual
+'
+
 test_expect_success 'log.decorate config parsing' '
 	git log --oneline --decorate=full >expect.full &&
 	git log --oneline --decorate=short >expect.short &&
--
2.23.0

[PATCH 2/2] log-tree: call load_ref_decorations() in get_name_decoration()

[René Scharfe]

Load a default set of ref name decorations at the first lookup.  This
frees direct and indirect callers from doing so.  They can still do it
if they want to use a filter or are interested in full decorations
instead of the default short ones -- the first load_ref_decorations()
call wins.

This means that the load in builtin/log.c::cmd_log_init_finish() is
respected even if --simplify-by-decoration is given, as the previously
dominating earlier load in handle_revision_opt() is gone.  So a filter
given with --decorate-refs-exclude is used for simplification in that
case, as expected.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 log-tree.c     | 1 +
 pretty.c       | 2 --
 revision.c     | 1 -
 t/t4202-log.sh | 2 +-
 4 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 1e56df62a7..2d5710e707 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -77,6 +77,7 @@ void add_name_decoration(enum decoration_type type, const char *name, struct obj

 const struct name_decoration *get_name_decoration(const struct object *obj)
 {
+	load_ref_decorations(NULL, DECORATE_SHORT_REFS);
 	return lookup_decoration(&name_decoration, obj);
 }

diff --git a/pretty.c b/pretty.c
index e4ed14effe..b32f036953 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1239,11 +1239,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		strbuf_addstr(sb, get_revision_mark(NULL, commit));
 		return 1;
 	case 'd':
-		load_ref_decorations(NULL, DECORATE_SHORT_REFS);
 		format_decorations(sb, commit, c->auto_color);
 		return 1;
 	case 'D':
-		load_ref_decorations(NULL, DECORATE_SHORT_REFS);
 		format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
 		return 1;
 	case 'S':		/* tag/branch like --source */
diff --git a/revision.c b/revision.c
index 07412297f0..1df3061e95 100644
--- a/revision.c
+++ b/revision.c
@@ -2063,7 +2063,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->simplify_by_decoration = 1;
 		revs->limited = 1;
 		revs->prune = 1;
-		load_ref_decorations(NULL, DECORATE_SHORT_REFS);
 	} else if (!strcmp(arg, "--date-order")) {
 		revs->sort_order = REV_SORT_BY_COMMIT_DATE;
 		revs->topo_order = 1;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 01c95d1375..bb66d1d93c 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -837,7 +837,7 @@ test_expect_success 'decorate-refs and decorate-refs-exclude' '
 	test_cmp expect.decorate actual
 '

-test_expect_failure 'decorate-refs-exclude and simplify-by-decoration' '
+test_expect_success 'decorate-refs-exclude and simplify-by-decoration' '
 	cat >expect.decorate <<-\EOF &&
 	Merge-tag-reach (HEAD -> master)
 	reach (tag: reach, reach)
--
2.23.0

Re: [PATCH 1/2] log: test --decorate-refs-exclude with --simplify-by-decoration

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Demonstrate that a decoration filter given with --decorate-refs-exclude
> is inadvertently overruled by --simplify-by-decoration.
>
> Reported-by: Étienne SERVAIS <etienne.servais@voucoux.fr>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  t/t4202-log.sh | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)

Looks vaguely familiar ;-)  Thanks for resurrecting it.