* [PATCH 1/2] log: handle --decorate-refs with userformat "%d"
2021-12-02 5:30 ` [PATCH 0/2] log: handle --decorate-ref without explicit --decorate Jeff King
@ 2021-12-02 5:35 ` Jeff King
2021-12-02 5:37 ` [PATCH 2/2] log: load decorations with --simplify-by-decoration Jeff King
1 sibling, 0 replies; 4+ messages in thread
From: Jeff King @ 2021-12-02 5:35 UTC (permalink / raw)
To: Josh Rampersad; +Cc: git
In order to show ref decorations, we first have to load them. If you
run:
git log --decorate
then git-log will recognize the option and load them up front via
cmd_log_init(). Likewise if log.decorate is set.
If you don't say --decorate explicitly, but do mention "%d" or "%D" in
the output format, like so:
git log --format=%d
then this also works, because we lazy-load the ref decorations. This has
been true since 3b3d443feb (add '%d' pretty format specifier to show
decoration, 2008-09-04), though the lazy-load was later moved into
log-tree.c.
But there's one problem: that lazy-load just uses the defaults; it
doesn't take into account any --decorate-refs options (or its exclude
variant, or their config). So this does not work:
git log --decorate-refs=whatever --format=%d
It will decorate using all refs, not just the specified ones. This has
been true since --decorate-refs was added in 65516f586b (log: add option
to choose which refs to decorate, 2017-11-21). Adding further confusion
is that it _may_ work because of the auto-decoration feature. If that's
in use (and it often is, as it's the default), then if the output is
going to stdout, we do enable decorations early (and so load them up
front, respecting the extra options). But otherwise we do not. So:
git log --decorate-refs=whatever --format=%d >some-file
would typically behave differently than it does when the output goes to
the pager or terminal!
The solution is simple: we should recognize in cmd_log_init() that we're
going to show decorations, and make sure we load them there. We already
check userformat_find_requirements(), so we can couple this with our
existing code there.
There are two new tests. The first shows off the actual fix. The second
makes sure that our fix doesn't cause us to stomp on an existing
--decorate option (see the new comment in the code, as well).
Reported-by: Josh Rampersad <josh.rampersad@voiceflow.com>
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/log.c | 18 ++++++++++++++++--
t/t4202-log.sh | 22 ++++++++++++++++++++++
2 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index f75d87e8d7..a924f56299 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -245,8 +245,22 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
rev->abbrev_commit = 0;
}
- if (rev->commit_format == CMIT_FMT_USERFORMAT && !w.decorate)
- decoration_style = 0;
+ if (rev->commit_format == CMIT_FMT_USERFORMAT) {
+ if (!w.decorate) {
+ /*
+ * Disable decoration loading if the format will not
+ * show them anyway.
+ */
+ decoration_style = 0;
+ } else if (!decoration_style) {
+ /*
+ * If we are going to show them, make sure we do load
+ * them here, but taking care not to override a
+ * specific style set by config or --decorate.
+ */
+ decoration_style = DECORATE_SHORT_REFS;
+ }
+ }
if (decoration_style) {
const struct string_list *config_exclude =
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 7884e3d46b..5f0ae7a785 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -952,6 +952,28 @@ test_expect_success 'decorate-refs-exclude and simplify-by-decoration' '
test_cmp expect.decorate actual
'
+test_expect_success 'decorate-refs with implied decorate from format' '
+ cat >expect <<-\EOF &&
+ side-2 (tag: side-2)
+ side-1
+ EOF
+ git log --no-walk --format="%s%d" \
+ --decorate-refs="*side-2" side-1 side-2 \
+ >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'implied decorate does not override option' '
+ cat >expect <<-\EOF &&
+ side-2 (tag: refs/tags/side-2, refs/heads/side)
+ side-1 (tag: refs/tags/side-1)
+ EOF
+ git log --no-walk --format="%s%d" \
+ --decorate=full side-1 side-2 \
+ >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'log.decorate config parsing' '
git log --oneline --decorate=full >expect.full &&
git log --oneline --decorate=short >expect.short &&
--
2.34.1.328.g8a543840e4
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 2/2] log: load decorations with --simplify-by-decoration
2021-12-02 5:30 ` [PATCH 0/2] log: handle --decorate-ref without explicit --decorate Jeff King
2021-12-02 5:35 ` [PATCH 1/2] log: handle --decorate-refs with userformat "%d" Jeff King
@ 2021-12-02 5:37 ` Jeff King
1 sibling, 0 replies; 4+ messages in thread
From: Jeff King @ 2021-12-02 5:37 UTC (permalink / raw)
To: Josh Rampersad; +Cc: git
It's possible to specify --simplify-by-decoration but not --decorate. In
this case we do respect the simplification, but we don't actually show
any decorations. However, it works by lazy-loading the decorations when
needed; this is discussed in more detail in 0cc7380d88 (log-tree: call
load_ref_decorations() in get_name_decoration(), 2019-09-08).
This works for basic cases, but will fail to respect any --decorate-refs
option (or its variants). Those are handled only when cmd_log_init()
loads the ref decorations up front, which is only when --decorate is
specified explicitly (or as of the previous commit, when the userformat
asks for %d or similar).
We can solve this by making sure to load the decorations if we're going
to simplify using them but they're not otherwise going to be displayed.
The new test shows a simple case that fails without this patch. Note
that we expect two commits in the output: the one we asked for by
--decorate-refs, and the initial commit. The latter is just a quirk of
how --simplify-by-decoration works. Arguably it may be a bug, but it's
unrelated to this patch (which is just about the loading of the
decorations; you get the same behavior before this patch with an
explicit --decorate).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/log.c | 5 +++--
t/t4202-log.sh | 15 +++++++++++++++
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index a924f56299..93ace0dde7 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -262,7 +262,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
}
}
- if (decoration_style) {
+ if (decoration_style || rev->simplify_by_decoration) {
const struct string_list *config_exclude =
repo_config_get_value_multi(the_repository,
"log.excludeDecoration");
@@ -274,7 +274,8 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
item->string);
}
- rev->show_decorations = 1;
+ if (decoration_style)
+ rev->show_decorations = 1;
load_ref_decorations(&decoration_filter, decoration_style);
}
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 5f0ae7a785..7a725b9a23 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -974,6 +974,21 @@ test_expect_success 'implied decorate does not override option' '
test_cmp expect actual
'
+test_expect_success 'decorate-refs and simplify-by-decoration without output' '
+ cat >expect <<-\EOF &&
+ side-2
+ initial
+ EOF
+ # Do not just use a --format without %d here; we want to
+ # make sure that we did not accidentally turn on displaying
+ # the decorations, too. And that requires one of the regular
+ # formats.
+ git log --decorate-refs="*side-2" --oneline \
+ --simplify-by-decoration >actual.raw &&
+ sed "s/^[0-9a-f]* //" <actual.raw >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'log.decorate config parsing' '
git log --oneline --decorate=full >expect.full &&
git log --oneline --decorate=short >expect.short &&
--
2.34.1.328.g8a543840e4
^ permalink raw reply related [flat|nested] 4+ messages in thread