pretty: add %r format specifier for showing refs

Message ID	20230712110732.8274-1-andy.koppe@gmail.com (mailing list archive)
State	Superseded
Headers	show Return-Path: <git-owner@vger.kernel.org> From: Andy Koppe <andy.koppe@gmail.com> To: git@vger.kernel.org Cc: Andy Koppe <andy.koppe@gmail.com> Subject: [PATCH] pretty: add %r format specifier for showing refs Date: Wed, 12 Jul 2023 12:07:32 +0100 Message-ID: <20230712110732.8274-1-andy.koppe@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	pretty: add %r format specifier for showing refs \| expand pretty: add %r format specifier for showing refs

Message ID

20230712110732.8274-1-andy.koppe@gmail.com (mailing list archive)

State

Superseded

Headers

From: Andy Koppe <andy.koppe@gmail.com>
To: git@vger.kernel.org
Cc: Andy Koppe <andy.koppe@gmail.com>
Subject: [PATCH] pretty: add %r format specifier for showing refs
Date: Wed, 12 Jul 2023 12:07:32 +0100
Message-ID: <20230712110732.8274-1-andy.koppe@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Series

pretty: add %r format specifier for showing refs | expand

Commit Message

Andy Koppe July 12, 2023, 11:07 a.m. UTC

This lists refs similarly to the %D decoration format, but separates
the refs with spaces only and omits "tag:" annotations. It's intended
primarily for color output, where tags are already distinguished by
color.

Refactor format_decorations() to take an enum decoration_format argument
that determines the prefix, separator and suffix as well as the tag
annotation.

For %d and %D, wrap the "tag:" annotation and the actual tag in separate
color controls, because otherwise the tag ends up uncolored when %w
width formatting breaks a line between the annotation and tag.

Amend t4207-log-decoration-colors.sh to reflect the added color
controls, and t4202-log.sh to test the %r format.
---
 Documentation/pretty-formats.txt |  3 +-
 log-tree.c                       | 52 +++++++++++++++++++++-----------
 log-tree.h                       | 15 ++++-----
 pretty.c                         |  7 +++--
 t/t4202-log.sh                   |  7 ++++-
 t/t4207-log-decoration-colors.sh | 22 +++++++-------
 6 files changed, 67 insertions(+), 39 deletions(-)

Comments

Eric Sunshine July 12, 2023, 4:56 p.m. UTC | #1

Not a proper review... just running my eye quickly over the patch...

On Wed, Jul 12, 2023 at 7:17 AM Andy Koppe <andy.koppe@gmail.com> wrote:
> This lists refs similarly to the %D decoration format, but separates
> the refs with spaces only and omits "tag:" annotations. It's intended
> primarily for color output, where tags are already distinguished by
> color.
>
> Refactor format_decorations() to take an enum decoration_format argument
> that determines the prefix, separator and suffix as well as the tag
> annotation.
>
> For %d and %D, wrap the "tag:" annotation and the actual tag in separate
> color controls, because otherwise the tag ends up uncolored when %w
> width formatting breaks a line between the annotation and tag.
>
> Amend t4207-log-decoration-colors.sh to reflect the added color
> controls, and t4202-log.sh to test the %r format.
> ---

Missing sign-off.

> diff --git a/log-tree.h b/log-tree.h
> @@ -13,17 +13,18 @@ struct decoration_filter {
> +enum decoration_format {
> +  DECO_FMT_BARE = 0,
> +  DECO_FMT_UNWRAPPED,
> +  DECO_FMT_WRAPPED,
> +};

Indent with TAB, not spaces.

Is this enum name a bit too generic for a public header? A quick scan
of other enums in the project shows that they usually incorporate the
"subsystem" into their names somehow (often as a prefix); for
instance, "enum apply_ws_ignore", "enum bisect_error".

Junio C Hamano July 12, 2023, 6:19 p.m. UTC | #2

Eric Sunshine <sunshine@sunshineco.com> writes:

> Not a proper review... just running my eye quickly over the patch...
> ...
>> Amend t4207-log-decoration-colors.sh to reflect the added color
>> controls, and t4202-log.sh to test the %r format.
>> ---
>
> Missing sign-off.
>
>> diff --git a/log-tree.h b/log-tree.h
>> @@ -13,17 +13,18 @@ struct decoration_filter {
>> +enum decoration_format {
>> +  DECO_FMT_BARE = 0,
>> +  DECO_FMT_UNWRAPPED,
>> +  DECO_FMT_WRAPPED,
>> +};
>
> Indent with TAB, not spaces.
>
> Is this enum name a bit too generic for a public header? A quick scan
> of other enums in the project shows that they usually incorporate the
> "subsystem" into their names somehow (often as a prefix); for
> instance, "enum apply_ws_ignore", "enum bisect_error".

Everything you said makes sense.

But more importantly, I doubt the wisdom of adding any more %<single
letter> placeholders to the vocabulary.  Even though I personally do
not see any need for variants other than just the plain "%d" to show
the "decorate" information (if you want anything else, just
post-process the output), if we really want to, the way we should
extend the format placeholders is to add %(decorate:<options>) that
is extensible enough that it can produce the identical output as
existing "%d" and "%D" placeholders do, and add new ones as a new
option to %(decorate).

Thanks.

Andy Koppe July 12, 2023, 8:47 p.m. UTC | #3

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> Not a proper review... just running my eye quickly over the patch...
>> ...
>> Missing sign-off.
>>
>> Indent with TAB, not spaces.

Thanks for the check, and apologies for those avoidable mistakes. Must
remember to run checkpatch …

I'll send a corrected patch, if only for completeness.

>>> +enum decoration_format {
>> Is this enum name a bit too generic for a public header? A quick scan
>> of other enums in the project shows that they usually incorporate the
>> "subsystem" into their names somehow (often as a prefix); for
>> instance, "enum apply_ws_ignore", "enum bisect_error".

I took existing decoration-related types as precedent, in particular
enum decoration_type and structs decoration_entry and
decoration_filter, whereby the latter is in the same header.

Junio C Hamano <gitster@pobox.com> writes:
> But more importantly, I doubt the wisdom of adding any more %<single
> letter> placeholders to the vocabulary.  Even though I personally do
> not see any need for variants other than just the plain "%d" to show
> the "decorate" information (if you want anything else, just
> post-process the output)

The proposed %r placeholder basically is the minimised version of %d,
which could save space in one-line logs and generally reduce visual
noise in custom log formats. Post-processing is rather more difficult
and error-prone than a built-in feature.

> if we really want to, the way we should
> extend the format placeholders is to add %(decorate:<options>) that
> is extensible enough that it can produce the identical output as
> existing "%d" and "%D" placeholders do, and add new ones as a new
> option to %(decorate).

I'd be happy to look into that.

What have you got in mind for the <options>?

Something like:
  %(decorate) for %d
  %(decorate:unwrapped) for %D
  %(decorate:bare) instead of the proposed %r

Or something with separate options for each element, similar to the
separator option of %(trailers)?

%r might look as follows, with a space for the separator and empty
strings for the other elements:

  %(decorate:prefix=,separator= ,suffix=,tag=)

(Each option would default to its %d value if not specified.)

Thanks,
Andy

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 3b71334459..d2ae898c79 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -221,7 +221,8 @@  The placeholders are:
 '%ch':: committer date, human style (like the `--date=human` option of
 	linkgit:git-rev-list[1])
 '%d':: ref names, like the --decorate option of linkgit:git-log[1]
-'%D':: ref names without the " (", ")" wrapping.
+'%D':: ref names without the " (", ")" wrapping
+'%r':: ref names only, separated by spaces
 '%(describe[:options])':: human-readable name, like
 			  linkgit:git-describe[1]; empty string for
 			  undescribable commits.  The `describe` string
diff --git a/log-tree.c b/log-tree.c
index f4b22a60cc..80850b3a03 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -301,14 +301,12 @@  static void show_name(struct strbuf *sb, const struct name_decoration *decoratio
 
 /*
  * The caller makes sure there is no funny color before calling.
- * format_decorations_extended makes sure the same after return.
+ * format_decorations makes sure the same after return.
  */
-void format_decorations_extended(struct strbuf *sb,
+void format_decorations(struct strbuf *sb,
 			const struct commit *commit,
 			int use_color,
-			const char *prefix,
-			const char *separator,
-			const char *suffix)
+			enum decoration_format format)
 {
 	const struct name_decoration *decoration;
 	const struct name_decoration *current_and_HEAD;
@@ -316,11 +314,18 @@  void format_decorations_extended(struct strbuf *sb,
 		diff_get_color(use_color, DIFF_COMMIT);
 	const char *color_reset =
 		decorate_get_color(use_color, DECORATION_NONE);
+	int first = 1;
 
 	decoration = get_name_decoration(&commit->object);
 	if (!decoration)
 		return;
 
+	if (format == DECO_FMT_WRAPPED) {
+		strbuf_addstr(sb, color_commit);
+		strbuf_addstr(sb, " (");
+		strbuf_addstr(sb, color_reset);
+	}
+
 	current_and_HEAD = current_pointed_by_HEAD(decoration);
 	while (decoration) {
 		/*
@@ -329,13 +334,25 @@  void format_decorations_extended(struct strbuf *sb,
 		 * appeared, skipping the entry for current.
 		 */
 		if (decoration != current_and_HEAD) {
-			strbuf_addstr(sb, color_commit);
-			strbuf_addstr(sb, prefix);
-			strbuf_addstr(sb, color_reset);
-			strbuf_addstr(sb, decorate_get_color(use_color, decoration->type));
-			if (decoration->type == DECORATION_REF_TAG)
-				strbuf_addstr(sb, "tag: ");
+			const char *color = decorate_get_color(use_color, decoration->type);
+			if (!first) {
+				if (format == DECO_FMT_BARE)
+					strbuf_addstr(sb, " ");
+				else {
+					strbuf_addstr(sb, color_commit);
+					strbuf_addstr(sb, ", ");
+					strbuf_addstr(sb, color_reset);
+				}
+			}
+			first = 0;
 
+			if (format != DECO_FMT_BARE &&
+			    decoration->type == DECORATION_REF_TAG) {
+				strbuf_addstr(sb, color);
+				strbuf_addstr(sb, "tag: ");
+				strbuf_addstr(sb, color_reset);
+			}
+			strbuf_addstr(sb, color);
 			show_name(sb, decoration);
 
 			if (current_and_HEAD &&
@@ -346,14 +363,15 @@  void format_decorations_extended(struct strbuf *sb,
 				show_name(sb, current_and_HEAD);
 			}
 			strbuf_addstr(sb, color_reset);
-
-			prefix = separator;
 		}
 		decoration = decoration->next;
 	}
-	strbuf_addstr(sb, color_commit);
-	strbuf_addstr(sb, suffix);
-	strbuf_addstr(sb, color_reset);
+
+	if (format == DECO_FMT_WRAPPED) {
+		strbuf_addstr(sb, color_commit);
+		strbuf_addstr(sb, ")");
+		strbuf_addstr(sb, color_reset);
+	}
 }
 
 void show_decorations(struct rev_info *opt, struct commit *commit)
@@ -368,7 +386,7 @@  void show_decorations(struct rev_info *opt, struct commit *commit)
 	}
 	if (!opt->show_decorations)
 		return;
-	format_decorations(&sb, commit, opt->diffopt.use_color);
+	format_decorations(&sb, commit, opt->diffopt.use_color, DECO_FMT_WRAPPED);
 	fputs(sb.buf, opt->diffopt.file);
 	strbuf_release(&sb);
 }
diff --git a/log-tree.h b/log-tree.h
index e7e4641cf8..5aa8908c65 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -13,17 +13,18 @@  struct decoration_filter {
 	struct string_list *exclude_ref_config_pattern;
 };
 
+enum decoration_format {
+  DECO_FMT_BARE = 0,
+  DECO_FMT_UNWRAPPED,
+  DECO_FMT_WRAPPED,
+};
+
 int parse_decorate_color_config(const char *var, const char *slot_name, const char *value);
 int log_tree_diff_flush(struct rev_info *);
 int log_tree_commit(struct rev_info *, struct commit *);
 void show_log(struct rev_info *opt);
-void format_decorations_extended(struct strbuf *sb, const struct commit *commit,
-			     int use_color,
-			     const char *prefix,
-			     const char *separator,
-			     const char *suffix);
-#define format_decorations(strbuf, commit, color) \
-			     format_decorations_extended((strbuf), (commit), (color), " (", ", ", ")")
+void format_decorations(struct strbuf *sb, const struct commit *commit,
+			int use_color, enum decoration_format format);
 void show_decorations(struct rev_info *opt, struct commit *commit);
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     const char **extra_headers_p,
diff --git a/pretty.c b/pretty.c
index 0bb938021b..88b041df85 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1526,10 +1526,13 @@  static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		strbuf_addstr(sb, get_revision_mark(NULL, commit));
 		return 1;
 	case 'd':
-		format_decorations(sb, commit, c->auto_color);
+		format_decorations(sb, commit, c->auto_color, DECO_FMT_WRAPPED);
 		return 1;
 	case 'D':
-		format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
+		format_decorations(sb, commit, c->auto_color, DECO_FMT_UNWRAPPED);
+		return 1;
+	case 'r':
+		format_decorations(sb, commit, c->auto_color, DECO_FMT_BARE);
 		return 1;
 	case 'S':		/* tag/branch like --source */
 		if (!(c->pretty_ctx->rev && c->pretty_ctx->rev->sources))
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index ae73aef922..9e0871df24 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -2325,7 +2325,12 @@  test_expect_success 'log --decorate includes all levels of tag annotated tags' '
 	HEAD -> branch, tag: lightweight, tag: double-1, tag: double-0, tag: annotated
 	EOF
 	git log -1 --format="%D" >actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	cat >expect2 <<-\EOF &&
+	HEAD -> branch lightweight double-1 double-0 annotated
+	EOF
+	git log -1 --format="%r" >actual2 &&
+	test_cmp expect2 actual2
 '
 
 test_expect_success 'log --decorate does not include things outside filter' '
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index ded33a82e2..7effc0813f 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -55,13 +55,13 @@  test_expect_success 'commit decorations colored correctly' '
 	cat >expect <<-EOF &&
 	${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD -> \
 ${c_reset}${c_branch}main${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: v1.0${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: B${c_reset}${c_commit})${c_reset} B
-${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: A1${c_reset}${c_commit}, \
+${c_reset}${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_commit}, \
+${c_reset}${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_commit})${c_reset} B
+${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: ${c_reset}${c_tag}A1${c_reset}${c_commit}, \
 ${c_reset}${c_remoteBranch}other/main${c_reset}${c_commit})${c_reset} A1
 	${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_stash}refs/stash${c_reset}${c_commit})${c_reset} \
 On main: Changes to A.t
-	${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: A${c_reset}${c_commit})${c_reset} A
+	${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
 	EOF
 
 	git log --first-parent --no-abbrev --decorate --oneline --color=always --all >actual &&
@@ -78,10 +78,10 @@  test_expect_success 'test coloring with replace-objects' '
 	cat >expect <<-EOF &&
 	${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD -> \
 ${c_reset}${c_branch}main${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: D${c_reset}${c_commit})${c_reset} D
-	${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: C${c_reset}${c_commit}, \
+${c_reset}${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_commit})${c_reset} D
+	${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: ${c_reset}${c_tag}C${c_reset}${c_commit}, \
 ${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} B
-	${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: A${c_reset}${c_commit})${c_reset} A
+	${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
 EOF
 
 	git log --first-parent --no-abbrev --decorate --oneline --color=always HEAD >actual &&
@@ -102,11 +102,11 @@  test_expect_success 'test coloring with grafted commit' '
 	cat >expect <<-EOF &&
 	${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD -> \
 ${c_reset}${c_branch}main${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: D${c_reset}${c_commit}, \
+${c_reset}${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_commit}, \
 ${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} D
-	${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: v1.0${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: B${c_reset}${c_commit})${c_reset} B
-	${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: A${c_reset}${c_commit})${c_reset} A
+	${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_commit}, \
+${c_reset}${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_commit})${c_reset} B
+	${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
 	EOF
 
 	git log --first-parent --no-abbrev --decorate --oneline --color=always HEAD >actual &&

pretty: add %r format specifier for showing refs

Commit Message

Comments

Patch