[PATCH] pretty: add %(decorate[:<options>]) format

[PATCH] pretty: add %(decorate[:<options>]) format

[Andy Koppe]

This lists ref names in the same way as the %d decoration format, but
allows all the otherwise fixed strings printed around the ref names to
be customized, namely prefix, suffix, separator, the "tag:" annotation
and the arrow used to show where HEAD points.

Examples:
- %decorate(prefix=,suffix=) removes the enclosing parentheses, like %D.
- %decorate(prefix=,suffix=,separator=,tag=,arrow=->) produces a
  space-separated list without wrapping, tag annotations or spaces
  around the arrow.
- %(decorate:prefix=[,suffix=],separator=%x2C,arrow=%x2C,tag=) produces
  a comma-separated list enclosed in square brackets where the arrow is
  replaced by a comma as well.

Add functions parse_decoration_option(), parse_decoration_options() and
free_decoration_options() to help implement the format. Test it in
t4205-log-pretty-formats.sh and document it in pretty-formats.txt.

Refactor format_decorations() to take a struct decoration_options
argument specifying those strings, whereby NULL entries select the
default. Avoid emitting color sequences for empty strings.

Wrap tag annotations in separate color sequences from tag names, because
otherwise tag names can end up uncolored when %w width formatting breaks
lines between annotation and name. Amend t4207-log-decoration-colors.sh
accordingly.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
CI: https://github.com/ak2/git/actions/runs/5561647197

 Documentation/pretty-formats.txt | 19 ++++++++-
 log-tree.c                       | 69 ++++++++++++++++++++------------
 log-tree.h                       | 17 ++++----
 pretty.c                         | 62 +++++++++++++++++++++++++++-
 t/t4205-log-pretty-formats.sh    | 21 ++++++++++
 t/t4207-log-decoration-colors.sh | 32 +++++++++------
 6 files changed, 171 insertions(+), 49 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 3b71334459..c08aba15af 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -222,7 +222,22 @@ The placeholders are:
 	linkgit:git-rev-list[1])
 '%d':: ref names, like the --decorate option of linkgit:git-log[1]
 '%D':: ref names without the " (", ")" wrapping.
-'%(describe[:options])':: human-readable name, like
+'%(decorate[:<options>])':: ref names with custom decorations.
+			  The `decorate` string may be followed by a colon
+			  and zero or more comma-separated options.
+			  Option values may contain literal formatting codes.
+			  These must be used for commas (`%x2C`) and closing
+			  parentheses (`%x29`), due to their role in the option
+			  syntax.
++
+** 'prefix=<value>': Shown before the list of ref names.  Defaults to " (".
+** 'suffix=<value>': Shown after the list of ref names.  Defaults to ")".
+** 'separator=<value>': Shown between ref names.  Defaults to ", ".
+** 'arrow=<value>': Shown between HEAD and the branch it points to, if any.
+		    Defaults to " \-> ".
+** 'tag=<value>': Shown before tag names. Defaults to "tag: ".
+
+'%(describe[:<options>])':: human-readable name, like
 			  linkgit:git-describe[1]; empty string for
 			  undescribable commits.  The `describe` string
 			  may be followed by a colon and zero or more
@@ -281,7 +296,7 @@ endif::git-rev-list[]
 '%gE':: reflog identity email (respecting .mailmap, see
 	linkgit:git-shortlog[1] or linkgit:git-blame[1])
 '%gs':: reflog subject
-'%(trailers[:options])':: display the trailers of the body as
+'%(trailers[:<options>])':: display the trailers of the body as
 			  interpreted by
 			  linkgit:git-interpret-trailers[1]. The
 			  `trailers` string may be followed by a colon
diff --git a/log-tree.c b/log-tree.c
index f4b22a60cc..4b46884ef6 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -301,27 +301,34 @@ 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 ensures 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)
+			const struct decoration_options *opts)
 {
-	const struct name_decoration *decoration;
-	const struct name_decoration *current_and_HEAD;
-	const char *color_commit =
-		diff_get_color(use_color, DIFF_COMMIT);
-	const char *color_reset =
-		decorate_get_color(use_color, DECORATION_NONE);
+	const char *color_commit, *color_reset;
+	const char *prefix, *suffix, *separator, *arrow, *tag;
+
+	const struct name_decoration *current_and_HEAD;
+	const struct name_decoration *decoration =
+		get_name_decoration(&commit->object);
 
-	decoration = get_name_decoration(&commit->object);
 	if (!decoration)
 		return;
 
+	color_commit = diff_get_color(use_color, DIFF_COMMIT);
+	color_reset = decorate_get_color(use_color, DECORATION_NONE);
+
+	prefix = (opts && opts->prefix) ? opts->prefix : " (";
+	suffix = (opts && opts->suffix) ? opts->suffix : ")";
+	separator = (opts && opts->separator) ? opts->separator : ", ";
+	arrow = (opts && opts->arrow) ? opts->arrow : " -> ";
+	tag = (opts && opts->tag) ? opts->tag : "tag: ";
+
 	current_and_HEAD = current_pointed_by_HEAD(decoration);
+
 	while (decoration) {
 		/*
 		 * When both current and HEAD are there, only
@@ -329,20 +336,29 @@ 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 (*prefix) {
+				strbuf_addstr(sb, color_commit);
+				strbuf_addstr(sb, prefix);
+				strbuf_addstr(sb, color_reset);
+			}
+
+			if (*tag && 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 &&
+			if (*arrow && current_and_HEAD &&
 			    decoration->type == DECORATION_REF_HEAD) {
-				strbuf_addstr(sb, " -> ");
+				strbuf_addstr(sb, arrow);
 				strbuf_addstr(sb, color_reset);
-				strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
+				strbuf_addstr(sb, decorate_get_color(
+					use_color, current_and_HEAD->type));
 				show_name(sb, current_and_HEAD);
 			}
 			strbuf_addstr(sb, color_reset);
@@ -351,9 +367,12 @@ void format_decorations_extended(struct strbuf *sb,
 		}
 		decoration = decoration->next;
 	}
-	strbuf_addstr(sb, color_commit);
-	strbuf_addstr(sb, suffix);
-	strbuf_addstr(sb, color_reset);
+
+	if (*suffix) {
+		strbuf_addstr(sb, color_commit);
+		strbuf_addstr(sb, suffix);
+		strbuf_addstr(sb, color_reset);
+	}
 }
 
 void show_decorations(struct rev_info *opt, struct commit *commit)
@@ -368,7 +387,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, NULL);
 	fputs(sb.buf, opt->diffopt.file);
 	strbuf_release(&sb);
 }
diff --git a/log-tree.h b/log-tree.h
index e7e4641cf8..39ab06a3ca 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -13,17 +13,20 @@ struct decoration_filter {
 	struct string_list *exclude_ref_config_pattern;
 };
 
+struct decoration_options {
+	char *prefix;
+	char *suffix;
+	char *separator;
+	char *arrow;
+	char *tag;
+};
+
 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, const struct decoration_options *opts);
 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..a59b7f0dbc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1373,6 +1373,46 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 	return arg - start;
 }
 
+
+static int parse_decoration_option(const char **arg,
+				   const char *name,
+				   char **opt)
+{
+	const char *argval;
+	size_t arglen;
+
+	if (match_placeholder_arg_value(*arg, name, arg, &argval, &arglen)) {
+		char *val = xstrndup(argval, arglen);
+		struct strbuf sb = STRBUF_INIT;
+
+		strbuf_expand(&sb, val, strbuf_expand_literal_cb, NULL);
+		free(val);
+		*opt = strbuf_detach(&sb, NULL);
+		return 1;
+	}
+	return 0;
+}
+
+static void parse_decoration_options(const char **arg,
+				     struct decoration_options *opts)
+{
+	while (parse_decoration_option(arg, "prefix", &opts->prefix) ||
+	       parse_decoration_option(arg, "suffix", &opts->suffix) ||
+	       parse_decoration_option(arg, "separator", &opts->separator) ||
+	       parse_decoration_option(arg, "arrow", &opts->arrow) ||
+	       parse_decoration_option(arg, "tag", &opts->tag))
+		;
+}
+
+static void free_decoration_options(const struct decoration_options *opts)
+{
+	free(opts->prefix);
+	free(opts->suffix);
+	free(opts->separator);
+	free(opts->arrow);
+	free(opts->tag);
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1526,10 +1566,11 @@ 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, NULL);
 		return 1;
 	case 'D':
-		format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
+		format_decorations(sb, commit, c->auto_color,
+				   &(struct decoration_options){"", ""});
 		return 1;
 	case 'S':		/* tag/branch like --source */
 		if (!(c->pretty_ctx->rev && c->pretty_ctx->rev->sources))
@@ -1627,6 +1668,23 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		return 2;
 	}
 
+	if (skip_prefix(placeholder, "(decorate", &arg)) {
+		struct decoration_options opts = { NULL };
+		size_t ret = 0;
+
+		if (*arg == ':') {
+			arg++;
+			parse_decoration_options(&arg, &opts);
+		}
+		if (*arg == ')') {
+			format_decorations(sb, commit, c->auto_color, &opts);
+			ret = arg - placeholder + 1;
+		}
+
+		free_decoration_options(&opts);
+		return ret;
+	}
+
 	/* For the rest we have to parse the commit header. */
 	if (!c->commit_header_parsed) {
 		msg = c->message =
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 4cf8a77667..5ea937648a 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -576,6 +576,27 @@ test_expect_success 'clean log decoration' '
 	test_cmp expected actual1
 '
 
+test_expect_success 'pretty format %decorate' '
+	git checkout -b foo &&
+	git commit --allow-empty -m "new commit" &&
+	git tag bar &&
+	git branch qux &&
+	echo " (HEAD -> foo, tag: bar, qux)" >expect1 &&
+	git log --format="%(decorate)" -1 >actual1 &&
+	test_cmp expect1 actual1 &&
+	echo "HEAD -> foo, tag: bar, qux" >expect2 &&
+	git log --format="%(decorate:prefix=,suffix=)" -1 >actual2 &&
+	test_cmp expect2 actual2 &&
+	echo "HEAD->foo bar qux" >expect3 &&
+	git log --format="%(decorate:prefix=,suffix=,separator= ,arrow=->,tag=)" \
+		-1 >actual3 &&
+	test_cmp expect3 actual3 &&
+	echo "[HEAD,foo,bar,qux]" >expect4 &&
+	git log --format="%(decorate:prefix=[,suffix=],separator=%x2C,arrow=%x2C,tag=)" \
+		-1 >actual4 &&
+	test_cmp expect4 actual4
+'
+
 cat >trailers <<EOF
 Signed-off-by: A U Thor <author@example.com>
 Acked-by: A U Thor <author@example.com>
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index ded33a82e2..3a4eedc494 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -55,13 +55,15 @@ 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_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: ${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 +80,12 @@ 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 +106,13 @@ 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 &&
-- 
2.41.0

[PATCH v2] pretty: add %(decorate[:<options>]) format

[Andy Koppe]

This lists ref names in the same way as the %d decoration format, but
allows all the otherwise fixed strings printed around the ref names to
be customized, namely prefix, suffix, separator, the "tag:" annotation
and the arrow used to show where HEAD points.

Examples:
- %(decorate) is equivalent to %d.
- %(decorate:prefix=,suffix=) is equivalent to %D.
- %(decorate:prefix=,suffix=,separator= ,tag=,arrow=->) produces a
  space-separated list without wrapping, tag annotations or spaces
  around the arrow.
- %(decorate:prefix=[,suffix=],separator=%x2C,arrow=%x2C,tag=) produces
  a comma-separated list enclosed in square brackets where the arrow is
  replaced by a comma as well.

Add functions parse_decoration_option(), parse_decoration_options() and
free_decoration_options() to help implement the format. Test it in
t4205-log-pretty-formats.sh and document it in pretty-formats.txt.

Refactor format_decorations() to take a struct decoration_options
argument specifying those strings, whereby NULL entries select the
default. Avoid emitting color sequences for empty strings.

Wrap tag annotations in separate color sequences from tag names, because
otherwise tag names can end up uncolored when %w width formatting breaks
lines between annotation and name. Amend t4207-log-decoration-colors.sh
accordingly.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Corrected mistakes in commit description.

 Documentation/pretty-formats.txt | 19 ++++++++-
 log-tree.c                       | 69 ++++++++++++++++++++------------
 log-tree.h                       | 17 ++++----
 pretty.c                         | 62 +++++++++++++++++++++++++++-
 t/t4205-log-pretty-formats.sh    | 21 ++++++++++
 t/t4207-log-decoration-colors.sh | 32 +++++++++------
 6 files changed, 171 insertions(+), 49 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 3b71334459..c08aba15af 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -222,7 +222,22 @@ The placeholders are:
 	linkgit:git-rev-list[1])
 '%d':: ref names, like the --decorate option of linkgit:git-log[1]
 '%D':: ref names without the " (", ")" wrapping.
-'%(describe[:options])':: human-readable name, like
+'%(decorate[:<options>])':: ref names with custom decorations.
+			  The `decorate` string may be followed by a colon
+			  and zero or more comma-separated options.
+			  Option values may contain literal formatting codes.
+			  These must be used for commas (`%x2C`) and closing
+			  parentheses (`%x29`), due to their role in the option
+			  syntax.
++
+** 'prefix=<value>': Shown before the list of ref names.  Defaults to " (".
+** 'suffix=<value>': Shown after the list of ref names.  Defaults to ")".
+** 'separator=<value>': Shown between ref names.  Defaults to ", ".
+** 'arrow=<value>': Shown between HEAD and the branch it points to, if any.
+		    Defaults to " \-> ".
+** 'tag=<value>': Shown before tag names. Defaults to "tag: ".
+
+'%(describe[:<options>])':: human-readable name, like
 			  linkgit:git-describe[1]; empty string for
 			  undescribable commits.  The `describe` string
 			  may be followed by a colon and zero or more
@@ -281,7 +296,7 @@ endif::git-rev-list[]
 '%gE':: reflog identity email (respecting .mailmap, see
 	linkgit:git-shortlog[1] or linkgit:git-blame[1])
 '%gs':: reflog subject
-'%(trailers[:options])':: display the trailers of the body as
+'%(trailers[:<options>])':: display the trailers of the body as
 			  interpreted by
 			  linkgit:git-interpret-trailers[1]. The
 			  `trailers` string may be followed by a colon
diff --git a/log-tree.c b/log-tree.c
index f4b22a60cc..4b46884ef6 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -301,27 +301,34 @@ 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 ensures 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)
+			const struct decoration_options *opts)
 {
-	const struct name_decoration *decoration;
-	const struct name_decoration *current_and_HEAD;
-	const char *color_commit =
-		diff_get_color(use_color, DIFF_COMMIT);
-	const char *color_reset =
-		decorate_get_color(use_color, DECORATION_NONE);
+	const char *color_commit, *color_reset;
+	const char *prefix, *suffix, *separator, *arrow, *tag;
+
+	const struct name_decoration *current_and_HEAD;
+	const struct name_decoration *decoration =
+		get_name_decoration(&commit->object);
 
-	decoration = get_name_decoration(&commit->object);
 	if (!decoration)
 		return;
 
+	color_commit = diff_get_color(use_color, DIFF_COMMIT);
+	color_reset = decorate_get_color(use_color, DECORATION_NONE);
+
+	prefix = (opts && opts->prefix) ? opts->prefix : " (";
+	suffix = (opts && opts->suffix) ? opts->suffix : ")";
+	separator = (opts && opts->separator) ? opts->separator : ", ";
+	arrow = (opts && opts->arrow) ? opts->arrow : " -> ";
+	tag = (opts && opts->tag) ? opts->tag : "tag: ";
+
 	current_and_HEAD = current_pointed_by_HEAD(decoration);
+
 	while (decoration) {
 		/*
 		 * When both current and HEAD are there, only
@@ -329,20 +336,29 @@ 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 (*prefix) {
+				strbuf_addstr(sb, color_commit);
+				strbuf_addstr(sb, prefix);
+				strbuf_addstr(sb, color_reset);
+			}
+
+			if (*tag && 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 &&
+			if (*arrow && current_and_HEAD &&
 			    decoration->type == DECORATION_REF_HEAD) {
-				strbuf_addstr(sb, " -> ");
+				strbuf_addstr(sb, arrow);
 				strbuf_addstr(sb, color_reset);
-				strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
+				strbuf_addstr(sb, decorate_get_color(
+					use_color, current_and_HEAD->type));
 				show_name(sb, current_and_HEAD);
 			}
 			strbuf_addstr(sb, color_reset);
@@ -351,9 +367,12 @@ void format_decorations_extended(struct strbuf *sb,
 		}
 		decoration = decoration->next;
 	}
-	strbuf_addstr(sb, color_commit);
-	strbuf_addstr(sb, suffix);
-	strbuf_addstr(sb, color_reset);
+
+	if (*suffix) {
+		strbuf_addstr(sb, color_commit);
+		strbuf_addstr(sb, suffix);
+		strbuf_addstr(sb, color_reset);
+	}
 }
 
 void show_decorations(struct rev_info *opt, struct commit *commit)
@@ -368,7 +387,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, NULL);
 	fputs(sb.buf, opt->diffopt.file);
 	strbuf_release(&sb);
 }
diff --git a/log-tree.h b/log-tree.h
index e7e4641cf8..39ab06a3ca 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -13,17 +13,20 @@ struct decoration_filter {
 	struct string_list *exclude_ref_config_pattern;
 };
 
+struct decoration_options {
+	char *prefix;
+	char *suffix;
+	char *separator;
+	char *arrow;
+	char *tag;
+};
+
 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, const struct decoration_options *opts);
 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..a59b7f0dbc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1373,6 +1373,46 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 	return arg - start;
 }
 
+
+static int parse_decoration_option(const char **arg,
+				   const char *name,
+				   char **opt)
+{
+	const char *argval;
+	size_t arglen;
+
+	if (match_placeholder_arg_value(*arg, name, arg, &argval, &arglen)) {
+		char *val = xstrndup(argval, arglen);
+		struct strbuf sb = STRBUF_INIT;
+
+		strbuf_expand(&sb, val, strbuf_expand_literal_cb, NULL);
+		free(val);
+		*opt = strbuf_detach(&sb, NULL);
+		return 1;
+	}
+	return 0;
+}
+
+static void parse_decoration_options(const char **arg,
+				     struct decoration_options *opts)
+{
+	while (parse_decoration_option(arg, "prefix", &opts->prefix) ||
+	       parse_decoration_option(arg, "suffix", &opts->suffix) ||
+	       parse_decoration_option(arg, "separator", &opts->separator) ||
+	       parse_decoration_option(arg, "arrow", &opts->arrow) ||
+	       parse_decoration_option(arg, "tag", &opts->tag))
+		;
+}
+
+static void free_decoration_options(const struct decoration_options *opts)
+{
+	free(opts->prefix);
+	free(opts->suffix);
+	free(opts->separator);
+	free(opts->arrow);
+	free(opts->tag);
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1526,10 +1566,11 @@ 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, NULL);
 		return 1;
 	case 'D':
-		format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
+		format_decorations(sb, commit, c->auto_color,
+				   &(struct decoration_options){"", ""});
 		return 1;
 	case 'S':		/* tag/branch like --source */
 		if (!(c->pretty_ctx->rev && c->pretty_ctx->rev->sources))
@@ -1627,6 +1668,23 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		return 2;
 	}
 
+	if (skip_prefix(placeholder, "(decorate", &arg)) {
+		struct decoration_options opts = { NULL };
+		size_t ret = 0;
+
+		if (*arg == ':') {
+			arg++;
+			parse_decoration_options(&arg, &opts);
+		}
+		if (*arg == ')') {
+			format_decorations(sb, commit, c->auto_color, &opts);
+			ret = arg - placeholder + 1;
+		}
+
+		free_decoration_options(&opts);
+		return ret;
+	}
+
 	/* For the rest we have to parse the commit header. */
 	if (!c->commit_header_parsed) {
 		msg = c->message =
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 4cf8a77667..5ea937648a 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -576,6 +576,27 @@ test_expect_success 'clean log decoration' '
 	test_cmp expected actual1
 '
 
+test_expect_success 'pretty format %decorate' '
+	git checkout -b foo &&
+	git commit --allow-empty -m "new commit" &&
+	git tag bar &&
+	git branch qux &&
+	echo " (HEAD -> foo, tag: bar, qux)" >expect1 &&
+	git log --format="%(decorate)" -1 >actual1 &&
+	test_cmp expect1 actual1 &&
+	echo "HEAD -> foo, tag: bar, qux" >expect2 &&
+	git log --format="%(decorate:prefix=,suffix=)" -1 >actual2 &&
+	test_cmp expect2 actual2 &&
+	echo "HEAD->foo bar qux" >expect3 &&
+	git log --format="%(decorate:prefix=,suffix=,separator= ,arrow=->,tag=)" \
+		-1 >actual3 &&
+	test_cmp expect3 actual3 &&
+	echo "[HEAD,foo,bar,qux]" >expect4 &&
+	git log --format="%(decorate:prefix=[,suffix=],separator=%x2C,arrow=%x2C,tag=)" \
+		-1 >actual4 &&
+	test_cmp expect4 actual4
+'
+
 cat >trailers <<EOF
 Signed-off-by: A U Thor <author@example.com>
 Acked-by: A U Thor <author@example.com>
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index ded33a82e2..3a4eedc494 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -55,13 +55,15 @@ 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_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: ${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 +80,12 @@ 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 +106,13 @@ 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 &&
-- 
2.41.0

Re: [PATCH v2] pretty: add %(decorate[:<options>]) format

[Junio C Hamano]

Andy Koppe <andy.koppe@gmail.com> writes:

> This lists ref names in the same way as the %d decoration format, but

Please replace "This" with "%(descorate[:<options>]", i.e. a more
concrete form, so that people do not have to go back to the title in
order to understand the body of the proposed log message.

> -'%(describe[:options])':: human-readable name, like
> +'%(describe[:<options>])':: human-readable name, like
> -'%(trailers[:options])':: display the trailers of the body as
> +'%(trailers[:<options>])':: display the trailers of the body as

It is a very good idea to signal that <options> is a placeholder by
enclosing it inside <angle bracket> like this patch wants to do with
%(decorate), and to make sure that other existing ones consistently
follow the same convention.

But the latter, being very small, can be buried in the noise.

It may be a good idea to have small "preliminary clean-up" patches
that do not add anything related to %(decorate) at the beginning of
the series.  [PATCH 1/3] can be %(token[:<options>]) clean-up,
[PATCH 2/3] can be "what is literal formatting code" clarification,
and [PATCH 3/3] can be the rest of this patch, for example.

> +'%(decorate[:<options>])':: ref names with custom decorations.
> +			  The `decorate` string may be followed by a colon
> +			  and zero or more comma-separated options.
> +			  Option values may contain literal formatting codes.
> +			  These must be used for commas (`%x2C`) and closing
> +			  parentheses (`%x29`), due to their role in the option
> +			  syntax.

OK.  I fear that "literal formatting codes" may not be understood by
readers without having any cross references here.  Perhaps something
like the patch attached at the end of this message would help.

> +** 'arrow=<value>': Shown between HEAD and the branch it points to, if any.
> +		    Defaults to " \-> ".

It feels a bit strange that this feature is limited only to "HEAD"
and other symbolic refs are not annotated with an arrow, but it is
not the fault of this patch.  We might want to see if it is worth to
extend this to other symbolic refs but not while this patch is being
discussed and polished.

> diff --git a/log-tree.c b/log-tree.c
> index f4b22a60cc..4b46884ef6 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -301,27 +301,34 @@ 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 ensures 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)
> +			const struct decoration_options *opts)

Hmph, presumably the idea is to collect these parameters in a struct
and pass it around, which would be easier to extend, and then teach
the hardcoded default to the callee, instead of the macro.  OK.

It may have made the change easier to review if such a change that
can cleanly separable into a single step into a single preliminary
clean-up patch before we start adding the customization, but let's
read on to see if I can keep everything in my head---I'll complain
at the end if I can't ;-).

>  {
> -	const struct name_decoration *decoration;
> -	const struct name_decoration *current_and_HEAD;
> -	const char *color_commit =
> -		diff_get_color(use_color, DIFF_COMMIT);
> -	const char *color_reset =
> -		decorate_get_color(use_color, DECORATION_NONE);

Getting rid of the computation of the initialization value from the
above decl block, ...


> +	const char *color_commit, *color_reset;
> +	const char *prefix, *suffix, *separator, *arrow, *tag;
> +
> +	const struct name_decoration *current_and_HEAD;

... like this, so we will return early without wasting extra cycles
whose result we will not use, is a very good idea.  But then, ...

> +	const struct name_decoration *decoration =
> +		get_name_decoration(&commit->object);
>  
> -	decoration = get_name_decoration(&commit->object);
>  	if (!decoration)
>  		return;

... I think the original is easier to follow than the updated form
for the "decoration" variable, simply because the declaration part
will become absolutely free, and it becomes easier to see that the
computation of "decoration" is the very first thing the cycles are
spent on.

> +	color_commit = diff_get_color(use_color, DIFF_COMMIT);
> +	color_reset = decorate_get_color(use_color, DECORATION_NONE);
> +
> +	prefix = (opts && opts->prefix) ? opts->prefix : " (";
> +	suffix = (opts && opts->suffix) ? opts->suffix : ")";
> +	separator = (opts && opts->separator) ? opts->separator : ", ";

Knowing these hardcoded values were the responsibility of the
format_decorations() C preprocessor macro; now it is written here.
It is a moral no-op change from the caller's point of view.

> +	arrow = (opts && opts->arrow) ? opts->arrow : " -> ";
> +	tag = (opts && opts->tag) ? opts->tag : "tag: ";

These two are new.  That is one thing why I wondered above if it is
a good idea to separate the "refactor to introduce
decoration_options structure that has three members and replace
three parameters to this function with it, so that we can get rid of
the format_decorations() macro" into a single preliminary step.
Then the reviewers can go on, after being convinced that such a
moral no-op refactoring is correct, to review the next step that
would presumably add these two members to the option struct and make
these customizable.

>  	current_and_HEAD = current_pointed_by_HEAD(decoration);
> +
>  	while (decoration) {
>  		/*
>  		 * When both current and HEAD are there, only

Unrelated noise.

> @@ -329,20 +336,29 @@ 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 (*prefix) {
> +				strbuf_addstr(sb, color_commit);
> +				strbuf_addstr(sb, prefix);
> +				strbuf_addstr(sb, color_reset);
> +			}
> +
> +			if (*tag && 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);

Again, this mixes adding new things (i.e. customizeable "tag:"
string, and "->" we see below) and improving existing things
(i.e. "<color><reset>" that is presumably pointless when prefix is
an empty string is shown as an empty string).  Ideally, the step to
move the three existing parameters to three members of the new
struct should be done first WITHOUT the empty-string improvement,
then another step should do the empty-string improvement (at which
time, presumably existing test script may have to be adjusted), and
then new features to costumize "tag:" and "->" should be added on
top.

> -			if (current_and_HEAD &&
> +			if (*arrow && current_and_HEAD &&
>  			    decoration->type == DECORATION_REF_HEAD) {

Because arrow is never allowed to be NULL, remove the above change,
and ...

> -				strbuf_addstr(sb, " -> ");
> +				strbuf_addstr(sb, arrow);

... let the program crash to catch a future bug at runtime.

> @@ -351,9 +367,12 @@ void format_decorations_extended(struct strbuf *sb,
>  		}
>  		decoration = decoration->next;
>  	}
> -	strbuf_addstr(sb, color_commit);
> -	strbuf_addstr(sb, suffix);
> -	strbuf_addstr(sb, color_reset);
> +
> +	if (*suffix) {
> +		strbuf_addstr(sb, color_commit);
> +		strbuf_addstr(sb, suffix);
> +		strbuf_addstr(sb, color_reset);
> +	}

Ditto about "improving the <color><reset> empty sequence is a
separate change from making various fields customizable".

I'll stop here. After skimming the changes to the test, I think this
single patch should be split into separate steps.  Perhaps the split
should go like this:

 * documentation clean-up %(token[:options]) -> %(token[:<options>])
   plus clarification of what a "literal formatting code" is.

 * introduction of "struct decoration_option",
   removing the format_decorations() macro,
   renaming format_decorations_extended() to format_decorations(),
   replacing the three parameters with a single struct pointer.

 * improving <color><reset> string when the meat of the string is
   empty.  This step will be the FIRST step that changes the
   externally visible behaviour, and presumably will have adjustment
   to existing tests.

 * making "tag:" and "->" customizable, if values are passed in the
   struct.  This step does not have UI changes, and the existing
   tests should serve as a safety net to catch mistakes in this
   step.

 * read the %(describe[:<option>]) and fill "struct describe_option".
   This will be accompanied by additional tests for the new feature.

Thanks.

---------------------- >8 ----------------------
Subject: pretty-formats: define "literal formatting code"

The description for %(trailer) option already uses this term without
having definition anywhere in the document, and we are about to add
another one %(decorate) that uses it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
diff --git c/Documentation/pretty-formats.txt w/Documentation/pretty-formats.txt
index c08aba15af..b7a3a150ae 100644
--- c/Documentation/pretty-formats.txt
+++ w/Documentation/pretty-formats.txt
@@ -122,7 +122,9 @@ The placeholders are:
 - Placeholders that expand to a single literal character:
 '%n':: newline
 '%%':: a raw '%'
-'%x00':: print a byte from a hex code
+'%x00':: '%x' followed by two hexadecimal digits is replaced with a
+	byte with the hexdecimal digits' value (we will call this
+	"literal formatting code" in the rest of this document).
 
 - Placeholders that affect formatting of later placeholders:
 '%Cred':: switch color to red

Re: [PATCH v2] pretty: add %(decorate[:<options>]) format

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I'll stop here. After skimming the changes to the test, I think this
> single patch should be split into separate steps.  Perhaps the split
> should go like this:
> ...
> Thanks.

Oh, sorry that I forgot to add one thing.

Overall, the patch seems to be done very well when viewed as a
whole.  Thanks for working on it.

It is just I cannot be as confident as I would like to be in my
review when the single patch does several different things at once.
If it were split in steps, each step focusing on doing a single
thing well and describing well what it does and why, reviewers can
be more confident that they did not miss something important in the
patch(es).

Re: [PATCH v2] pretty: add %(decorate[:<options>]) format

[Glen Choo]

Hi Andy!

We picked up this series at Review Club. Reviewers will leave their
thoughts on the mailing list, but if you like, you can find the notes
at:

  https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit

Andy Koppe <andy.koppe@gmail.com> writes:

> This lists ref names in the same way as the %d decoration format, but
> allows all the otherwise fixed strings printed around the ref names to
> be customized, namely prefix, suffix, separator, the "tag:" annotation
> and the arrow used to show where HEAD points.
>
> Examples:
> - %(decorate) is equivalent to %d.
> - %(decorate:prefix=,suffix=) is equivalent to %D.
> - %(decorate:prefix=,suffix=,separator= ,tag=,arrow=->) produces a
>   space-separated list without wrapping, tag annotations or spaces
>   around the arrow.
> - %(decorate:prefix=[,suffix=],separator=%x2C,arrow=%x2C,tag=) produces
>   a comma-separated list enclosed in square brackets where the arrow is
>   replaced by a comma as well.

I think giving the user this level of customization makes sense,
especially since we do this for other format options. Importantly, this
design also fits the existing conventions we have, so this looks like a
good proposal.

As a micro-nit: there's some useful context behind your chosen design in
[1]. It would have been useful to link to it in the `---` context, or
perhaps send this series as v3 and v4 to [1].

[1] https://lore.kernel.org/git/20230712110732.8274-1-andy.koppe@gmail.com/

> Add functions parse_decoration_option(), parse_decoration_options() and
> free_decoration_options() to help implement the format. Test it in
> t4205-log-pretty-formats.sh and document it in pretty-formats.txt.

This commit adds the new feature...

> Refactor format_decorations() to take a struct decoration_options
> argument specifying those strings, whereby NULL entries select the
> default. Avoid emitting color sequences for empty strings.

does some refactoring to support the new feature + existing use cases...

> Wrap tag annotations in separate color sequences from tag names, because
> otherwise tag names can end up uncolored when %w width formatting breaks
> lines between annotation and name. Amend t4207-log-decoration-colors.sh
> accordingly.

and fixes a bug with coloring that is easier to run into as a result of
the new feature.

As others have mentioned, I think this would be easier to follow as
separate commits. This commit isn't so big that the refactor needs to be
its own commit, though I don't feel strongly either way.

> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 3b71334459..c08aba15af 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -222,7 +222,22 @@ The placeholders are:
>  	linkgit:git-rev-list[1])
>  '%d':: ref names, like the --decorate option of linkgit:git-log[1]
>  '%D':: ref names without the " (", ")" wrapping.
> -'%(describe[:options])':: human-readable name, like
> +'%(decorate[:<options>])':: ref names with custom decorations.
> +			  The `decorate` string may be followed by a colon
> +			  and zero or more comma-separated options.
> +			  Option values may contain literal formatting codes.
> +			  These must be used for commas (`%x2C`) and closing
> +			  parentheses (`%x29`), due to their role in the option
> +			  syntax.

To make this easier to visualize, it would be useful to include the
examples from your commit message (%d, %D, etc.).

> +'%(describe[:<options>])':: human-readable name, like

Ah, adding the <> is a good fix. I think it doesn't warrant its own
patch, but it should be called out in the commit message.

>  /*
>   * The caller makes sure there is no funny color before calling.
> - * format_decorations_extended makes sure the same after return.
> + * format_decorations ensures 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)
> +			const struct decoration_options *opts)
>  {
> -	const struct name_decoration *decoration;
> -	const struct name_decoration *current_and_HEAD;
> -	const char *color_commit =
> -		diff_get_color(use_color, DIFF_COMMIT);
> -	const char *color_reset =
> -		decorate_get_color(use_color, DECORATION_NONE);
> +	const char *color_commit, *color_reset;
> +	const char *prefix, *suffix, *separator, *arrow, *tag;
> +
> +	const struct name_decoration *current_and_HEAD;
> +	const struct name_decoration *decoration =
> +		get_name_decoration(&commit->object);
>  
> -	decoration = get_name_decoration(&commit->object);
>  	if (!decoration)
>  		return;
>  
> +	color_commit = diff_get_color(use_color, DIFF_COMMIT);
> +	color_reset = decorate_get_color(use_color, DECORATION_NONE);

I'm guessing that you shuffled these lines to make use of an early
return? If so, both versions are not different enough to warrant the
churn IMO. It would be worth pointing out the reshuffling in the commit
message, especially if you had another rationale in mind.

> +	prefix = (opts && opts->prefix) ? opts->prefix : " (";
> +	suffix = (opts && opts->suffix) ? opts->suffix : ")";
> +	separator = (opts && opts->separator) ? opts->separator : ", ";
> +	arrow = (opts && opts->arrow) ? opts->arrow : " -> ";
> +	tag = (opts && opts->tag) ? opts->tag : "tag: ";

So NULL means "use the default"...

> +struct decoration_options {
> +	char *prefix;
> +	char *suffix;
> +	char *separator;
> +	char *arrow;
> +	char *tag;
> +};
> +
>  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, const struct decoration_options *opts);

Which lets us unify these two functions. Makes sense.

> +static int parse_decoration_option(const char **arg,
> +				   const char *name,
> +				   char **opt)
> +{
> +	const char *argval;
> +	size_t arglen;
> +
> +	if (match_placeholder_arg_value(*arg, name, arg, &argval, &arglen)) {
> +		char *val = xstrndup(argval, arglen);
> +		struct strbuf sb = STRBUF_INIT;
> +
> +		strbuf_expand(&sb, val, strbuf_expand_literal_cb, NULL);

strbuf_expand() got removed in 'master' recently, so this should be
rebased.

>  static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  				const char *placeholder,
>  				void *context)
> @@ -1526,10 +1566,11 @@ 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, NULL);
>  		return 1;
>  	case 'D':
> -		format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
> +		format_decorations(sb, commit, c->auto_color,
> +				   &(struct decoration_options){"", ""});

I don't remember if C99 lets you name .prefix and .suffix here, but if
so, it would be good to name them. Otherwise it's easy to get the order
wrong, e.g. if someone reorders the fields in struct decoration_options.

> +test_expect_success 'pretty format %decorate' '
> +	git checkout -b foo &&
> +	git commit --allow-empty -m "new commit" &&
> +	git tag bar &&
> +	git branch qux &&
> +	echo " (HEAD -> foo, tag: bar, qux)" >expect1 &&
> +	git log --format="%(decorate)" -1 >actual1 &&
> +	test_cmp expect1 actual1 &&
> +	echo "HEAD -> foo, tag: bar, qux" >expect2 &&
> +	git log --format="%(decorate:prefix=,suffix=)" -1 >actual2 &&
> +	test_cmp expect2 actual2 &&
> +	echo "HEAD->foo bar qux" >expect3 &&
> +	git log --format="%(decorate:prefix=,suffix=,separator= ,arrow=->,tag=)" \
> +		-1 >actual3 &&
> +	test_cmp expect3 actual3 &&
> +	echo "[HEAD,foo,bar,qux]" >expect4 &&
> +	git log --format="%(decorate:prefix=[,suffix=],separator=%x2C,arrow=%x2C,tag=)" \
> +		-1 >actual4 &&
> +	test_cmp expect4 actual4
> +'
> +

It would be useful to get some "bad" inputs to %(decorate:) to check
that we handle them correctly, especially since it's implemented with
while() loops.

Overall, I thought this patch looks really good. Thanks!

Re: [PATCH v2] pretty: add %(decorate[:<options>]) format

[Phillip Wood]

Hi Andy

On 19/07/2023 19:16, Glen Choo wrote:
>>   	case 'D':
>> -		format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
>> +		format_decorations(sb, commit, c->auto_color,
>> +				   &(struct decoration_options){"", ""});
> 
> I don't remember if C99 lets you name .prefix and .suffix here, but if
> so, it would be good to name them. Otherwise it's easy to get the order
> wrong, e.g. if someone reorders the fields in struct decoration_options.

That's a good suggestion. I think this would be the first use of a 
compound literal in the code base so it would be helpful to mention that 
in the commit message.

We've been depending on C99 for a while now so I'd support adding this 
compound literal as a test balloon for compiler support. Ævar reported a 
while back that they are supported by IBM xlc, Oracle SunCC and HP/UX's 
aCC[1] and back then I looked at NonStop which seemed to offer support 
with the right compiler flag.

Overall this is a well written, well motivated patch with a good commit 
message.

Best Wishes

Phillip

[1] https://lore.kernel.org/git/87h7e61duk.fsf@evledraar.gmail.com/

[PATCH v3 1/7] pretty-formats: define "literal formatting code"

[Andy Koppe]

From: Junio C Hamano <gitster@pobox.com>

The description for a %(trailer) option already uses this term without
having a definition anywhere in the document, and we are about to add
another one in %(decorate) that uses it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 Documentation/pretty-formats.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 3b71334459..5e1432951b 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -122,7 +122,9 @@ The placeholders are:
 - Placeholders that expand to a single literal character:
 '%n':: newline
 '%%':: a raw '%'
-'%x00':: print a byte from a hex code
+'%x00':: '%x' followed by two hexadecimal digits is replaced with a
+	 byte with the hexadecimal digits' value (we will call this
+	 "literal formatting code" in the rest of this document).
 
 - Placeholders that affect formatting of later placeholders:
 '%Cred':: switch color to red
-- 
2.42.0-rc1

[PATCH v3 2/7] pretty-formats: enclose options in angle brackets

[Andy Koppe]

Enclose the 'options' placeholders in the %(describe) and %(trailers)
format specifiers in angle brackets to clarify that they are
placeholders rather than keywords.

Also remove the indentation from their descriptions, instead of
increasing it to account for the extra two angle bracket in the
headings. The indentation isn't required by asciidoc, it doesn't reflect
how the output text is formatted, and it's inconsistent with the
following bullet points that are at the same level in the output.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 Documentation/pretty-formats.txt | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 5e1432951b..851a9878e6 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -224,13 +224,11 @@ The placeholders are:
 	linkgit:git-rev-list[1])
 '%d':: ref names, like the --decorate option of linkgit:git-log[1]
 '%D':: ref names without the " (", ")" wrapping.
-'%(describe[:options])':: human-readable name, like
-			  linkgit:git-describe[1]; empty string for
-			  undescribable commits.  The `describe` string
-			  may be followed by a colon and zero or more
-			  comma-separated options.  Descriptions can be
-			  inconsistent when tags are added or removed at
-			  the same time.
+'%(describe[:<options>])'::
+human-readable name, like linkgit:git-describe[1]; empty string for
+undescribable commits.  The `describe` string may be followed by a colon and
+zero or more comma-separated options.  Descriptions can be inconsistent when
+tags are added or removed at the same time.
 +
 ** 'tags[=<bool-value>]': Instead of only considering annotated tags,
    consider lightweight tags as well.
@@ -283,13 +281,11 @@ endif::git-rev-list[]
 '%gE':: reflog identity email (respecting .mailmap, see
 	linkgit:git-shortlog[1] or linkgit:git-blame[1])
 '%gs':: reflog subject
-'%(trailers[:options])':: display the trailers of the body as
-			  interpreted by
-			  linkgit:git-interpret-trailers[1]. The
-			  `trailers` string may be followed by a colon
-			  and zero or more comma-separated options.
-			  If any option is provided multiple times the
-			  last occurrence wins.
+'%(trailers[:<options>])'::
+display the trailers of the body as interpreted by
+linkgit:git-interpret-trailers[1]. The `trailers` string may be followed by
+a colon and zero or more comma-separated options. If any option is provided
+multiple times, the last occurrence wins.
 +
 ** 'key=<key>': only show trailers with specified <key>. Matching is done
    case-insensitively and trailing colon is optional. If option is
-- 
2.42.0-rc1

[PATCH v3 3/7] decorate: refactor format_decorations()

[Andy Koppe]

Rename the format_decorations_extended function to format_decorations
and drop the format_decorations wrapper macro. Pass the prefix, suffix
and separator strings as a single 'struct format_decorations' pointer
argument instead of separate arguments. Use default values defined in
the function when either the struct pointer or any of the struct fields
are NULL. This is to ease extension with additional options.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 log-tree.c | 23 +++++++++++++++++------
 log-tree.h | 15 ++++++++-------
 pretty.c   |  6 ++++--
 3 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 208c69cbb7..cd12c26c29 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -303,14 +303,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 ensures 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)
+			const struct decoration_options *opts)
 {
 	const struct name_decoration *decoration;
 	const struct name_decoration *current_and_HEAD;
@@ -319,10 +317,23 @@ void format_decorations_extended(struct strbuf *sb,
 	const char *color_reset =
 		decorate_get_color(use_color, DECORATION_NONE);
 
+	const char *prefix = " (";
+	const char *suffix = ")";
+	const char *separator = ", ";
+
 	decoration = get_name_decoration(&commit->object);
 	if (!decoration)
 		return;
 
+	if (opts) {
+		if (opts->prefix)
+			prefix = opts->prefix;
+		if (opts->suffix)
+			suffix = opts->suffix;
+		if (opts->separator)
+			separator = opts->separator;
+	}
+
 	current_and_HEAD = current_pointed_by_HEAD(decoration);
 	while (decoration) {
 		/*
@@ -370,7 +381,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, NULL);
 	fputs(sb.buf, opt->diffopt.file);
 	strbuf_release(&sb);
 }
diff --git a/log-tree.h b/log-tree.h
index bdb6432815..14898de8ac 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -13,17 +13,18 @@ struct decoration_filter {
 	struct string_list *exclude_ref_config_pattern;
 };
 
+struct decoration_options {
+	char *prefix;
+	char *suffix;
+	char *separator;
+};
+
 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, const struct decoration_options *opts);
 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 718530bbab..24fb82a5a2 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1537,10 +1537,12 @@ 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, NULL);
 		return 1;
 	case 'D':
-		format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
+		format_decorations(sb, commit, c->auto_color,
+			&(struct decoration_options){.prefix = "",
+						     .suffix = ""});
 		return 1;
 	case 'S':		/* tag/branch like --source */
 		if (!(c->pretty_ctx->rev && c->pretty_ctx->rev->sources))
-- 
2.42.0-rc1

[PATCH v3 4/7] decorate: avoid some unnecessary color overhead

[Andy Koppe]

In format_decorations(), don't obtain color sequences if there are no
decorations, and don't emit color sequences around empty strings.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 log-tree.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index cd12c26c29..7c6d3f1ac3 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -312,10 +312,7 @@ void format_decorations(struct strbuf *sb,
 {
 	const struct name_decoration *decoration;
 	const struct name_decoration *current_and_HEAD;
-	const char *color_commit =
-		diff_get_color(use_color, DIFF_COMMIT);
-	const char *color_reset =
-		decorate_get_color(use_color, DECORATION_NONE);
+	const char *color_commit, *color_reset;
 
 	const char *prefix = " (";
 	const char *suffix = ")";
@@ -334,6 +331,9 @@ void format_decorations(struct strbuf *sb,
 			separator = opts->separator;
 	}
 
+	color_commit = diff_get_color(use_color, DIFF_COMMIT);
+	color_reset = decorate_get_color(use_color, DECORATION_NONE);
+
 	current_and_HEAD = current_pointed_by_HEAD(decoration);
 	while (decoration) {
 		/*
@@ -342,9 +342,12 @@ void format_decorations(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);
+			if (*prefix) {
+				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: ");
@@ -364,9 +367,11 @@ void format_decorations(struct strbuf *sb,
 		}
 		decoration = decoration->next;
 	}
-	strbuf_addstr(sb, color_commit);
-	strbuf_addstr(sb, suffix);
-	strbuf_addstr(sb, color_reset);
+	if (*suffix) {
+		strbuf_addstr(sb, color_commit);
+		strbuf_addstr(sb, suffix);
+		strbuf_addstr(sb, color_reset);
+	}
 }
 
 void show_decorations(struct rev_info *opt, struct commit *commit)
-- 
2.42.0-rc1

[PATCH v3 5/7] decorate: color each token separately

[Andy Koppe]

Wrap "tag:" prefixes and the arrows in "HEAD -> branch" annotations in
their own color sequences, because otherwise tag names or arrows can end
up uncolored when %w width formatting breaks lines just before them.

Use the commit color for arrows, for visual consistency with the '(',
',' and ')' symbols used as prefix, separator and suffix, which are also
colored with the commit color.

Amend test t4207-log-decoration-colors.sh accordingly.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 log-tree.c                       | 14 +++++++---
 t/t4207-log-decoration-colors.sh | 44 ++++++++++++++++++--------------
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 7c6d3f1ac3..3b62dca048 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -342,26 +342,34 @@ void format_decorations(struct strbuf *sb,
 		 * appeared, skipping the entry for current.
 		 */
 		if (decoration != current_and_HEAD) {
+			const char *color =
+				decorate_get_color(use_color, decoration->type);
+
 			if (*prefix) {
 				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)
+			if (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);
+			strbuf_addstr(sb, color_reset);
 
 			if (current_and_HEAD &&
 			    decoration->type == DECORATION_REF_HEAD) {
+				strbuf_addstr(sb, color_commit);
 				strbuf_addstr(sb, " -> ");
 				strbuf_addstr(sb, color_reset);
 				strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
 				show_name(sb, current_and_HEAD);
+				strbuf_addstr(sb, color_reset);
 			}
-			strbuf_addstr(sb, color_reset);
 
 			prefix = separator;
 		}
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index ded33a82e2..21986a866d 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -53,15 +53,17 @@ cmp_filtered_decorations () {
 # to this test since it does not contain any decoration, hence --first-parent
 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_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_commit} -> ${c_reset}${c_branch}main${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_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: ${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 &&
@@ -76,12 +78,14 @@ test_expect_success 'test coloring with replace-objects' '
 	git replace HEAD~1 HEAD~2 &&
 
 	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_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_commit} -> ${c_reset}${c_branch}main${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 &&
@@ -100,13 +104,15 @@ test_expect_success 'test coloring with grafted commit' '
 	git replace --graft HEAD HEAD~2 &&
 
 	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_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_commit} -> ${c_reset}${c_branch}main${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 &&
-- 
2.42.0-rc1

[PATCH v3 6/7] pretty: add %(decorate[:<options>]) format

[Andy Koppe]

Add %(decorate[:<options>]) format that lists ref names similarly to the
%d format, but which allows the otherwise fixed prefix, suffix and
separator strings to be customized. Omitted options default to the
strings used in %d.

Rename expand_separator() function used to expand %x literal formatting
codes to expand_string_arg(), as it is now used on strings other than
separators.

Examples:
- %(decorate) is equivalent to %d.
- %(decorate:prefix=,suffix=) is equivalent to %D.
- %(decorate:prefix=[,suffix=],separator=%x3B) produces a list enclosed
in square brackets and separated by semicolons.

Test the format in t4205-log-pretty-formats.sh and document it in
pretty-formats.txt.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 Documentation/pretty-formats.txt | 10 ++++++
 pretty.c                         | 59 +++++++++++++++++++++++++++++---
 t/t4205-log-pretty-formats.sh    | 27 +++++++++++++++
 3 files changed, 92 insertions(+), 4 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 851a9878e6..709d85af21 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -224,6 +224,16 @@ The placeholders are:
 	linkgit:git-rev-list[1])
 '%d':: ref names, like the --decorate option of linkgit:git-log[1]
 '%D':: ref names without the " (", ")" wrapping.
+'%(decorate[:<options>])'::
+ref names with custom decorations. The `decorate` string may be followed by a
+colon and zero or more comma-separated options. Option values may contain
+literal formatting codes. These must be used for commas (`%x2C`) and closing
+parentheses (`%x29`), due to their role in the option syntax.
++
+** 'prefix=<value>': Shown before the list of ref names.  Defaults to "{nbsp}`(`".
+** 'suffix=<value>': Shown after the list of ref names.  Defaults to "`)`".
+** 'separator=<value>': Shown between ref names.  Defaults to "`,`{nbsp}".
+
 '%(describe[:<options>])'::
 human-readable name, like linkgit:git-describe[1]; empty string for
 undescribable commits.  The `describe` string may be followed by a colon and
diff --git a/pretty.c b/pretty.c
index 24fb82a5a2..d972051543 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1252,8 +1252,8 @@ static int format_trailer_match_cb(const struct strbuf *key, void *ud)
 	return 0;
 }
 
-static struct strbuf *expand_separator(struct strbuf *sb,
-				       const char *argval, size_t arglen)
+static struct strbuf *expand_string_arg(struct strbuf *sb,
+					const char *argval, size_t arglen)
 {
 	char *fmt = xstrndup(argval, arglen);
 	const char *format = fmt;
@@ -1301,9 +1301,9 @@ int format_set_trailers_options(struct process_trailer_options *opts,
 			opts->filter_data = filter_list;
 			opts->only_trailers = 1;
 		} else if (match_placeholder_arg_value(*arg, "separator", arg, &argval, &arglen)) {
-			opts->separator = expand_separator(sepbuf, argval, arglen);
+			opts->separator = expand_string_arg(sepbuf, argval, arglen);
 		} else if (match_placeholder_arg_value(*arg, "key_value_separator", arg, &argval, &arglen)) {
-			opts->key_value_separator = expand_separator(kvsepbuf, argval, arglen);
+			opts->key_value_separator = expand_string_arg(kvsepbuf, argval, arglen);
 		} else if (!match_placeholder_bool_arg(*arg, "only", arg, &opts->only_trailers) &&
 			   !match_placeholder_bool_arg(*arg, "unfold", arg, &opts->unfold) &&
 			   !match_placeholder_bool_arg(*arg, "keyonly", arg, &opts->key_only) &&
@@ -1384,6 +1384,40 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 	return arg - start;
 }
 
+
+static int parse_decoration_option(const char **arg,
+				   const char *name,
+				   char **opt)
+{
+	const char *argval;
+	size_t arglen;
+
+	if (match_placeholder_arg_value(*arg, name, arg, &argval, &arglen)) {
+		struct strbuf sb = STRBUF_INIT;
+
+		expand_string_arg(&sb, argval, arglen);
+		*opt = strbuf_detach(&sb, NULL);
+		return 1;
+	}
+	return 0;
+}
+
+static void parse_decoration_options(const char **arg,
+				     struct decoration_options *opts)
+{
+	while (parse_decoration_option(arg, "prefix", &opts->prefix) ||
+	       parse_decoration_option(arg, "suffix", &opts->suffix) ||
+	       parse_decoration_option(arg, "separator", &opts->separator))
+		;
+}
+
+static void free_decoration_options(const struct decoration_options *opts)
+{
+	free(opts->prefix);
+	free(opts->suffix);
+	free(opts->separator);
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1640,6 +1674,23 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		return 2;
 	}
 
+	if (skip_prefix(placeholder, "(decorate", &arg)) {
+		struct decoration_options opts = { NULL };
+		size_t ret = 0;
+
+		if (*arg == ':') {
+			arg++;
+			parse_decoration_options(&arg, &opts);
+		}
+		if (*arg == ')') {
+			format_decorations(sb, commit, c->auto_color, &opts);
+			ret = arg - placeholder + 1;
+		}
+
+		free_decoration_options(&opts);
+		return ret;
+	}
+
 	/* For the rest we have to parse the commit header. */
 	if (!c->commit_header_parsed) {
 		msg = c->message =
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index dd9035aa38..6ba399c5be 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -576,6 +576,33 @@ test_expect_success 'clean log decoration' '
 	test_cmp expected actual1
 '
 
+test_expect_success 'pretty format %decorate' '
+	git checkout -b foo &&
+	git commit --allow-empty -m "new commit" &&
+	git tag bar &&
+	git branch qux &&
+
+	echo " (HEAD -> foo, tag: bar, qux)" >expect1 &&
+	git log --format="%(decorate)" -1 >actual1 &&
+	test_cmp expect1 actual1 &&
+
+	echo "HEAD -> foo, tag: bar, qux" >expect2 &&
+	git log --format="%(decorate:prefix=,suffix=)" -1 >actual2 &&
+	test_cmp expect2 actual2 &&
+
+	echo "[ HEAD -> foo; tag: bar; qux ]" >expect3 &&
+	git log --format="%(decorate:prefix=[ ,suffix= ],separator=%x3B )" \
+		-1 >actual3 &&
+	test_cmp expect3 actual3 &&
+
+	# Try with a typo (in "separator"), in which case the placeholder should
+	# not be replaced.
+	echo "%(decorate:prefix=[ ,suffix= ],separater=; )" >expect4 &&
+	git log --format="%(decorate:prefix=[ ,suffix= ],separater=%x3B )" \
+		-1 >actual4 &&
+	test_cmp expect4 actual4
+'
+
 cat >trailers <<EOF
 Signed-off-by: A U Thor <author@example.com>
 Acked-by: A U Thor <author@example.com>
-- 
2.42.0-rc1

[PATCH v3 7/7] pretty: add pointer and tag options to %(decorate)

[Andy Koppe]

Add pointer and tag options to %(decorate) format, to allow to override
the " -> " string used to show where HEAD points and the "tag: " string
used to mark tags.

Document in pretty-formats.txt and test in t4205-log-pretty-formats.sh.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 Documentation/pretty-formats.txt |  9 +++++++++
 log-tree.c                       | 12 +++++++++---
 log-tree.h                       |  2 ++
 pretty.c                         |  6 +++++-
 t/t4205-log-pretty-formats.sh    |  7 ++++++-
 5 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 709d85af21..d38b4ab566 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -233,6 +233,15 @@ parentheses (`%x29`), due to their role in the option syntax.
 ** 'prefix=<value>': Shown before the list of ref names.  Defaults to "{nbsp}`(`".
 ** 'suffix=<value>': Shown after the list of ref names.  Defaults to "`)`".
 ** 'separator=<value>': Shown between ref names.  Defaults to "`,`{nbsp}".
+** 'pointer=<value>': Shown between HEAD and the branch it points to, if any.
+		      Defaults to "{nbsp}`->`{nbsp}".
+** 'tag=<value>': Shown before tag names. Defaults to "`tag:`{nbsp}".
+
++
+For example, to produce decorations with no wrapping
+or tag annotations, and spaces as separators:
++
+`%(decorate:prefix=,suffix=,tag=,separator= )`
 
 '%(describe[:<options>])'::
 human-readable name, like linkgit:git-describe[1]; empty string for
diff --git a/log-tree.c b/log-tree.c
index 3b62dca048..504da6b519 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -317,6 +317,8 @@ void format_decorations(struct strbuf *sb,
 	const char *prefix = " (";
 	const char *suffix = ")";
 	const char *separator = ", ";
+	const char *pointer = " -> ";
+	const char *tag = "tag: ";
 
 	decoration = get_name_decoration(&commit->object);
 	if (!decoration)
@@ -329,6 +331,10 @@ void format_decorations(struct strbuf *sb,
 			suffix = opts->suffix;
 		if (opts->separator)
 			separator = opts->separator;
+		if (opts->pointer)
+			pointer = opts->pointer;
+		if (opts->tag)
+			tag = opts->tag;
 	}
 
 	color_commit = diff_get_color(use_color, DIFF_COMMIT);
@@ -351,9 +357,9 @@ void format_decorations(struct strbuf *sb,
 				strbuf_addstr(sb, color_reset);
 			}
 
-			if (decoration->type == DECORATION_REF_TAG) {
+			if (*tag && decoration->type == DECORATION_REF_TAG) {
 				strbuf_addstr(sb, color);
-				strbuf_addstr(sb, "tag: ");
+				strbuf_addstr(sb, tag);
 				strbuf_addstr(sb, color_reset);
 			}
 
@@ -364,7 +370,7 @@ void format_decorations(struct strbuf *sb,
 			if (current_and_HEAD &&
 			    decoration->type == DECORATION_REF_HEAD) {
 				strbuf_addstr(sb, color_commit);
-				strbuf_addstr(sb, " -> ");
+				strbuf_addstr(sb, pointer);
 				strbuf_addstr(sb, color_reset);
 				strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
 				show_name(sb, current_and_HEAD);
diff --git a/log-tree.h b/log-tree.h
index 14898de8ac..41c776fea5 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -17,6 +17,8 @@ struct decoration_options {
 	char *prefix;
 	char *suffix;
 	char *separator;
+	char *pointer;
+	char *tag;
 };
 
 int parse_decorate_color_config(const char *var, const char *slot_name, const char *value);
diff --git a/pretty.c b/pretty.c
index d972051543..5effbe9ad9 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1407,7 +1407,9 @@ static void parse_decoration_options(const char **arg,
 {
 	while (parse_decoration_option(arg, "prefix", &opts->prefix) ||
 	       parse_decoration_option(arg, "suffix", &opts->suffix) ||
-	       parse_decoration_option(arg, "separator", &opts->separator))
+	       parse_decoration_option(arg, "separator", &opts->separator) ||
+	       parse_decoration_option(arg, "pointer", &opts->pointer) ||
+	       parse_decoration_option(arg, "tag", &opts->tag))
 		;
 }
 
@@ -1416,6 +1418,8 @@ static void free_decoration_options(const struct decoration_options *opts)
 	free(opts->prefix);
 	free(opts->suffix);
 	free(opts->separator);
+	free(opts->pointer);
+	free(opts->tag);
 }
 
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 6ba399c5be..16626e4fe9 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -600,7 +600,12 @@ test_expect_success 'pretty format %decorate' '
 	echo "%(decorate:prefix=[ ,suffix= ],separater=; )" >expect4 &&
 	git log --format="%(decorate:prefix=[ ,suffix= ],separater=%x3B )" \
 		-1 >actual4 &&
-	test_cmp expect4 actual4
+	test_cmp expect4 actual4 &&
+
+	echo "HEAD->foo bar qux" >expect5 &&
+	git log --format="%(decorate:prefix=,suffix=,separator= ,tag=,pointer=->)" \
+		-1 >actual5 &&
+	test_cmp expect5 actual5
 '
 
 cat >trailers <<EOF
-- 
2.42.0-rc1

Re: [PATCH v2] pretty: add %(decorate[:<options>]) format

[Andy Koppe]

Junio C Hamano <gitster@pobox.com> wrote:
> Overall, the patch seems to be done very well when viewed as a
> whole.  Thanks for working on it.
>
> It is just I cannot be as confident as I would like to be in my
> review when the single patch does several different things at once.
> If it were split in steps, each step focusing on doing a single
> thing well and describing well what it does and why, reviewers can
> be more confident that they did not miss something important in the
> patch(es).

Thanks to Junio, Glen, Phil and the Review Club for the helpful
reviews, especially the guidance on commit granularity.

Sorry for not getting back to this sooner, but the v3 patch series
addressing the review comments is now here:
https://lore.kernel.org/git/20230715160730.4046-1-andy.koppe@gmail.com/T/#m46ad3ebbe3163821f649f7122edcabd619fc5837

Kind regards,
Andy

Re: [PATCH v2] pretty: add %(decorate[:<options>]) format

[Andy Koppe]

On Wed, 19 Jul 2023 at 19:16, Glen Choo  wrote:

> As a micro-nit: there's some useful context behind your chosen design in
> [1]. It would have been useful to link to it in the `---` context, or
> perhaps send this series as v3 and v4 to [1].
>
> [1] https://lore.kernel.org/git/20230712110732.8274-1-andy.koppe@gmail.com/

Point taken.

> > +             strbuf_expand(&sb, val, strbuf_expand_literal_cb, NULL);
>
> strbuf_expand() got removed in 'master' recently, so this should be
> rebased.

Done. I think I had started off main, wrongly assuming that it's the
same as master.

Thanks again,
Andy

Re: [PATCH v2] pretty: add %(decorate[:<options>]) format

[Andy Koppe]

On Sun, 23 Jul 2023 at 17:26, Phillip Wood  wrote:
>
> On 19/07/2023 19:16, Glen Choo wrote:
> >>      case 'D':
> >> -            format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
> >> +            format_decorations(sb, commit, c->auto_color,
> >> +                               &(struct decoration_options){"", ""});
> >
> > I don't remember if C99 lets you name .prefix and .suffix here, but if
> > so, it would be good to name them. Otherwise it's easy to get the order
> > wrong, e.g. if someone reorders the fields in struct decoration_options.
>
> That's a good suggestion. I think this would be the first use of a
> compound literal in the code base so it would be helpful to mention that
> in the commit message.

I've taken the suggestion, but then forgot to mention it in the commit
message. Will do in the next round.

> We've been depending on C99 for a while now so I'd support adding this
> compound literal as a test balloon for compiler support. Ævar reported a
> while back that they are supported by IBM xlc, Oracle SunCC and HP/UX's
> aCC[1] and back then I looked at NonStop which seemed to offer support
> with the right compiler flag.

There are a number of uses of designated initializers already, so
hopefully compound literals aren't too much of an extra challenge.

Thanks,
Andy

Re: [PATCH v2] pretty: add %(decorate[:<options>]) format

[Junio C Hamano]

Andy Koppe <andy.koppe@gmail.com> writes:

> There are a number of uses of designated initializers already, so
> hopefully compound literals aren't too much of an extra challenge.

I do not see how one leads to the other here.  I'd prefer not to see
use of a new construct we do not currently use mixed in a new code,
even if it is mentioned in the proposed log message.

If we want to use compound literals in our codebase in the longer
term, we should first add a weatherballoon use to a very stable part
of the codebase that rarely changes, in a single patch that is
trivial to revert when a platform that matters is found to have
problem with the language construct, just like what we did when we
adopted the use of designated initializers.

Thanks.

Re: [PATCH v2] pretty: add %(decorate[:<options>]) format

[Andy Koppe]

On 11/08/2023 21:38, Junio C Hamano wrote:
 > Andy Koppe <andy.koppe@gmail.com> writes:
 >
 >> There are a number of uses of designated initializers already, so
 >> hopefully compound literals aren't too much of an extra challenge.
 >
 > I do not see how one leads to the other here.  I'd prefer not to see
 > use of a new construct we do not currently use mixed in a new code,
 > even if it is mentioned in the proposed log message.

Okay.

Would this style be acceptable to fulfil Glen's request to name the
fields?

	case 'D':
		{
			const struct decoration_options opts = {
				.prefix = "",
				.suffix = ""
			};

			format_decorations(sb, commit, c->auto_color, &opts);
		}
		return 1;

Andy

Re: [PATCH v2] pretty: add %(decorate[:<options>]) format

[Junio C Hamano]

Andy Koppe <andy.koppe@gmail.com> writes:

> On 11/08/2023 21:38, Junio C Hamano wrote:
>> Andy Koppe <andy.koppe@gmail.com> writes:
>>
>>> There are a number of uses of designated initializers already, so
>>> hopefully compound literals aren't too much of an extra challenge.
>>
>> I do not see how one leads to the other here.  I'd prefer not to see
>> use of a new construct we do not currently use mixed in a new code,
>> even if it is mentioned in the proposed log message.
>
> Okay.
>
> Would this style be acceptable to fulfil Glen's request to name the
> fields?
>
> 	case 'D':
> 		{
> 			const struct decoration_options opts = {
> 				.prefix = "",
> 				.suffix = ""
> 			};
>
> 			format_decorations(sb, commit, c->auto_color, &opts);
> 		}
> 		return 1;
>
> Andy

Sounds good to me.

Re: [PATCH v2] pretty: add %(decorate[:<options>]) format

[Junio C Hamano]

Andy Koppe <andy.koppe@gmail.com> writes:

> Done. I think I had started off main, wrongly assuming that it's the
> same as master.

If there is 'main' that is different from 'master', that sounds like
a problem to me.  This project predates the newer convention that
allows the primary branch to be named 'main', but many new folks of
course expect to see 'main', so while my primary working areas all
call the primary branch 'master', it is pushed out to both names.

Or at least I thought I arranged that to happen.

Thanks.

Re: [PATCH v2] pretty: add %(decorate[:<options>]) format

[Andy Koppe]

On 15/08/2023 19:13, Junio C Hamano wrote:
> If there is 'main' that is different from 'master', that sounds like
> a problem to me.  This project predates the newer convention that
> allows the primary branch to be named 'main', but many new folks of
> course expect to see 'main', so while my primary working areas all
> call the primary branch 'master', it is pushed out to both names.
> 
> Or at least I thought I arranged that to happen.

See [1], where main currently is at v2.41.0.

Regards,
Andy

[1] https://github.com/git/git/tree/main

Re: [PATCH v2] pretty: add %(decorate[:<options>]) format

[Junio C Hamano]

Andy Koppe <andy.koppe@gmail.com> writes:

> On 15/08/2023 19:13, Junio C Hamano wrote:
>> If there is 'main' that is different from 'master', that sounds like
>> a problem to me.  This project predates the newer convention that
>> allows the primary branch to be named 'main', but many new folks of
>> course expect to see 'main', so while my primary working areas all
>> call the primary branch 'master', it is pushed out to both names.
>> Or at least I thought I arranged that to happen.
>
> See [1], where main currently is at v2.41.0.
>
> Regards,
> Andy
>
> [1] https://github.com/git/git/tree/main

Ah, that one.  The CI job is unfortunately attached to that tree and
updating 'master' and 'main' with the same commit at the same time
wastes CI cycles, so I had to tentatively stop updating it.

It used to be that 'main' was set to lag behind 'master' by 24 hours
or so to prevent the problem---CI notices that the commit updated
'main' has been already dealt with 24 hours ago at 'master' and
refrains from wasting time on it.  But resurrecting it would still
make folks confused about how 'main' is different from 'master'.
Perhaps it is a good time to remove stale 'main' and keep only
'master' there?

main != master at github.com/git/git

[Andy Koppe]

On 15/08/2023 20:01, Junio C Hamano wrote:
>> See [1], where main currently is at v2.41.0.
>>
>> [1] https://github.com/git/git/tree/main
> 
> Ah, that one.  The CI job is unfortunately attached to that tree and
> updating 'master' and 'main' with the same commit at the same time
> wastes CI cycles, so I had to tentatively stop updating it.
> 
> It used to be that 'main' was set to lag behind 'master' by 24 hours
> or so to prevent the problem---CI notices that the commit updated
> 'main' has been already dealt with 24 hours ago at 'master' and
> refrains from wasting time on it.  But resurrecting it would still
> make folks confused about how 'main' is different from 'master'.
> Perhaps it is a good time to remove stale 'main' and keep only
> 'master' there?

An alternative might be to exclude one of the branches in the workflow 
file, as per [1].

Andy

[1] 
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-excluding-branches

Re: main != master at github.com/git/git

[Taylor Blau]

On Tue, Aug 15, 2023 at 08:29:43PM +0100, Andy Koppe wrote:
> On 15/08/2023 20:01, Junio C Hamano wrote:
> > > See [1], where main currently is at v2.41.0.
> > >
> > > [1] https://github.com/git/git/tree/main
> >
> > Ah, that one.  The CI job is unfortunately attached to that tree and
> > updating 'master' and 'main' with the same commit at the same time
> > wastes CI cycles, so I had to tentatively stop updating it.
> >
> > It used to be that 'main' was set to lag behind 'master' by 24 hours
> > or so to prevent the problem---CI notices that the commit updated
> > 'main' has been already dealt with 24 hours ago at 'master' and
> > refrains from wasting time on it.  But resurrecting it would still
> > make folks confused about how 'main' is different from 'master'.
> > Perhaps it is a good time to remove stale 'main' and keep only
> > 'master' there?
>
> An alternative might be to exclude one of the branches in the workflow file,
> as per [1].

I think that this should be relatively straightforward to do, and would
be preferable to dropping 'main'.

Here's an (untested) patch that should do the trick:

--- >8 ---
diff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml
index a58e2dc8ad..764f46b21f 100644
--- a/.github/workflows/check-whitespace.yml
+++ b/.github/workflows/check-whitespace.yml
@@ -8,6 +8,8 @@ name: check-whitespace
 on:
   pull_request:
     types: [opened, synchronize]
+    branches_ignore:
+      - main

 # Avoid unnecessary builds. Unlike the main CI jobs, these are not
 # ci-configurable (but could be).
diff --git a/.github/workflows/l10n.yml b/.github/workflows/l10n.yml
index 6c3849658a..f6767a73d2 100644
--- a/.github/workflows/l10n.yml
+++ b/.github/workflows/l10n.yml
@@ -1,6 +1,12 @@
 name: git-l10n

-on: [push, pull_request_target]
+on:
+  push:
+    branches_ignore:
+      - main
+  pull_request_target:
+    branches_ignore:
+      - main

 # Avoid unnecessary builds. Unlike the main CI jobs, these are not
 # ci-configurable (but could be).
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 079645b776..eaaf6a9151 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -1,6 +1,12 @@
 name: CI

-on: [push, pull_request]
+on:
+  push:
+    branches-ignore:
+      - main
+  pull_request:
+    branches-ignore:
+      - main

 env:
   DEVELOPER: 1
--- 8< ---

> [1] https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-excluding-branches

Thanks,
Taylor

Re: main != master at github.com/git/git

[Jeff King]

On Tue, Aug 15, 2023 at 06:16:29PM -0400, Taylor Blau wrote:

> > An alternative might be to exclude one of the branches in the workflow file,
> > as per [1].
> 
> I think that this should be relatively straightforward to do, and would
> be preferable to dropping 'main'.

That was my inclination, too, though I wonder if that might cause
hassles for Git for Windows:

  $ git ls-remote --symref https://github.com/git-for-windows/git HEAD
  ref: refs/heads/main	HEAD
  a67b85bf88ddbccae96714edb64d741ddfc3a1c9	HEAD

I'm not sure how big a deal it would be in practice. Obviously they
carry patches that are not in upstream git and could adjust the file
themselves that way. But it might introduce extra friction, and in my
experience changes to "meta" files like this can be a hassle, because
you often want them independently on every branch (though in theory this
one only matters for the "main" branch itself).

So I won't say it's obviously a bad idea, but it might bear some
thinking on what the ramifications would be for downstream.

-Peff

Re: [PATCH v3 1/7] pretty-formats: define "literal formatting code"

[Junio C Hamano]

The v3 iteration of the series that begins at

  https://lore.kernel.org/git/20230810211619.19055-1-andy.koppe@gmail.com/

08-10 ` [PATCH v3 1/7] pretty-formats: define "literal formatting code"
08-10   ` [PATCH v3 2/7] pretty-formats: enclose options in angle brackets
08-10   ` [PATCH v3 3/7] decorate: refactor format_decorations()
08-10   ` [PATCH v3 4/7] decorate: avoid some unnecessary color overhead
08-10   ` [PATCH v3 5/7] decorate: color each token separately
08-10   ` [PATCH v3 6/7] pretty: add %(decorate[:<options>]) format
08-10   ` [PATCH v3 7/7] pretty: add pointer and tag options to %(decorate)

unfortunately did not see any discussion.  Any comments?

RE: main != master at github.com/git/git

[rsbecker]

On Tuesday, August 15, 2023 10:24 PM, Jeff King wrote:
>On Tue, Aug 15, 2023 at 06:16:29PM -0400, Taylor Blau wrote:
>
>> > An alternative might be to exclude one of the branches in the
>> > workflow file, as per [1].
>>
>> I think that this should be relatively straightforward to do, and
>> would be preferable to dropping 'main'.
>
>That was my inclination, too, though I wonder if that might cause hassles for Git for
>Windows:
>
>  $ git ls-remote --symref https://github.com/git-for-windows/git HEAD
>  ref: refs/heads/main	HEAD
>  a67b85bf88ddbccae96714edb64d741ddfc3a1c9	HEAD
>
>I'm not sure how big a deal it would be in practice. Obviously they carry patches that
>are not in upstream git and could adjust the file themselves that way. But it might
>introduce extra friction, and in my experience changes to "meta" files like this can be
>a hassle, because you often want them independently on every branch (though in
>theory this one only matters for the "main" branch itself).
>
>So I won't say it's obviously a bad idea, but it might bear some thinking on what the
>ramifications would be for downstream.

Would it not be more convenient just to add a GitHub action that set main = master for each push?

Re: main != master at github.com/git/git

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

> Would it not be more convenient just to add a GitHub action that
> set main = master for each push?

If "my private working area calls the primary integration branch
'master', but for publishing repositories, I have to push it twice,
once to 'master' and then to 'main'" were the problem, the solution
I would rather want to see implemented is to an ability for the
repository owners to set a symref that makes 'main' refer to
'master', so that I do not have to worry about the aliasing.  But it
is not a problem (the push refspec can be set up to send the same
commit to two different branches just fine).

In any case, I am not sure if it would solve the problem being
discussed: when CI runner sees branches updated to commit that
hasn't been worked on, a new job is created to work on that commit,
and updating two branches with the same commit at the same time
unfortunately means two independent CI jobs work on the same commit
in parallel.  The 'lagging behind by 24 hours' hack I mentioned
earlier was one way to work it around, but it would confuse folks.

I'd really prefer not to special case 'main' (or 'master' for that
matter), primarily because some downstreams rely more heavily on
'main' as Peff pointed out, but also because the problem is not
'master' vs 'main'.  If 'next' happens to become empty soon after a
new cycle starts and points at the same commit as 'master', we will
see the same waste of cycles between 'master' and 'next'.

Thanks.

[PATCH v4 0/8] pretty: add %(decorate[:<options>]) format

[Andy Koppe]

Compared to v3, this avoids introducing a compound literal, and splits
part of patch 5 into an additional patch 8.

Andy Koppe (7):
  pretty-formats: enclose options in angle brackets
  decorate: refactor format_decorations()
  decorate: avoid some unnecessary color overhead
  decorate: color each token separately
  pretty: add %(decorate[:<options>]) format
  pretty: add pointer and tag options to %(decorate)
  decorate: use commit color for HEAD arrow

Junio C Hamano (1):
  pretty-formats: define "literal formatting code"

 Documentation/pretty-formats.txt | 47 +++++++++++++-------
 log-tree.c                       | 72 +++++++++++++++++++++---------
 log-tree.h                       | 17 ++++---
 pretty.c                         | 76 +++++++++++++++++++++++++++++---
 t/t4205-log-pretty-formats.sh    | 32 ++++++++++++++
 t/t4207-log-decoration-colors.sh | 44 ++++++++++--------
 6 files changed, 219 insertions(+), 69 deletions(-)

-- 
2.42.0-rc2

[PATCH v4 1/8] pretty-formats: define "literal formatting code"

[Andy Koppe]

From: Junio C Hamano <gitster@pobox.com>

The description for a %(trailer) option already uses this term without
having a definition anywhere in the document, and we are about to add
another one in %(decorate) that uses it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 Documentation/pretty-formats.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 3b71334459..5e1432951b 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -122,7 +122,9 @@ The placeholders are:
 - Placeholders that expand to a single literal character:
 '%n':: newline
 '%%':: a raw '%'
-'%x00':: print a byte from a hex code
+'%x00':: '%x' followed by two hexadecimal digits is replaced with a
+	 byte with the hexadecimal digits' value (we will call this
+	 "literal formatting code" in the rest of this document).
 
 - Placeholders that affect formatting of later placeholders:
 '%Cred':: switch color to red
-- 
2.42.0-rc2

[PATCH v4 2/8] pretty-formats: enclose options in angle brackets

[Andy Koppe]

Enclose the 'options' placeholders in the %(describe) and %(trailers)
format specifiers in angle brackets to clarify that they are
placeholders rather than keywords.

Also remove the indentation from their descriptions, instead of
increasing it to account for the extra two angle bracket in the
headings. The indentation isn't required by asciidoc, it doesn't reflect
how the output text is formatted, and it's inconsistent with the
following bullet points that are at the same level in the output.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 Documentation/pretty-formats.txt | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 5e1432951b..851a9878e6 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -224,13 +224,11 @@ The placeholders are:
 	linkgit:git-rev-list[1])
 '%d':: ref names, like the --decorate option of linkgit:git-log[1]
 '%D':: ref names without the " (", ")" wrapping.
-'%(describe[:options])':: human-readable name, like
-			  linkgit:git-describe[1]; empty string for
-			  undescribable commits.  The `describe` string
-			  may be followed by a colon and zero or more
-			  comma-separated options.  Descriptions can be
-			  inconsistent when tags are added or removed at
-			  the same time.
+'%(describe[:<options>])'::
+human-readable name, like linkgit:git-describe[1]; empty string for
+undescribable commits.  The `describe` string may be followed by a colon and
+zero or more comma-separated options.  Descriptions can be inconsistent when
+tags are added or removed at the same time.
 +
 ** 'tags[=<bool-value>]': Instead of only considering annotated tags,
    consider lightweight tags as well.
@@ -283,13 +281,11 @@ endif::git-rev-list[]
 '%gE':: reflog identity email (respecting .mailmap, see
 	linkgit:git-shortlog[1] or linkgit:git-blame[1])
 '%gs':: reflog subject
-'%(trailers[:options])':: display the trailers of the body as
-			  interpreted by
-			  linkgit:git-interpret-trailers[1]. The
-			  `trailers` string may be followed by a colon
-			  and zero or more comma-separated options.
-			  If any option is provided multiple times the
-			  last occurrence wins.
+'%(trailers[:<options>])'::
+display the trailers of the body as interpreted by
+linkgit:git-interpret-trailers[1]. The `trailers` string may be followed by
+a colon and zero or more comma-separated options. If any option is provided
+multiple times, the last occurrence wins.
 +
 ** 'key=<key>': only show trailers with specified <key>. Matching is done
    case-insensitively and trailing colon is optional. If option is
-- 
2.42.0-rc2

[PATCH v4 3/8] decorate: refactor format_decorations()

[Andy Koppe]

Rename the format_decorations_extended function to format_decorations
and drop the format_decorations wrapper macro. Pass the prefix, suffix
and separator strings as a single 'struct format_decorations' pointer
argument instead of separate arguments. Use default values defined in
the function when either the struct pointer or any of the struct fields
are NULL. This is to ease extension with additional options.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 log-tree.c | 23 +++++++++++++++++------
 log-tree.h | 15 ++++++++-------
 pretty.c   |  6 ++++--
 3 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 208c69cbb7..cd12c26c29 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -303,14 +303,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 ensures 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)
+			const struct decoration_options *opts)
 {
 	const struct name_decoration *decoration;
 	const struct name_decoration *current_and_HEAD;
@@ -319,10 +317,23 @@ void format_decorations_extended(struct strbuf *sb,
 	const char *color_reset =
 		decorate_get_color(use_color, DECORATION_NONE);
 
+	const char *prefix = " (";
+	const char *suffix = ")";
+	const char *separator = ", ";
+
 	decoration = get_name_decoration(&commit->object);
 	if (!decoration)
 		return;
 
+	if (opts) {
+		if (opts->prefix)
+			prefix = opts->prefix;
+		if (opts->suffix)
+			suffix = opts->suffix;
+		if (opts->separator)
+			separator = opts->separator;
+	}
+
 	current_and_HEAD = current_pointed_by_HEAD(decoration);
 	while (decoration) {
 		/*
@@ -370,7 +381,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, NULL);
 	fputs(sb.buf, opt->diffopt.file);
 	strbuf_release(&sb);
 }
diff --git a/log-tree.h b/log-tree.h
index bdb6432815..14898de8ac 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -13,17 +13,18 @@ struct decoration_filter {
 	struct string_list *exclude_ref_config_pattern;
 };
 
+struct decoration_options {
+	char *prefix;
+	char *suffix;
+	char *separator;
+};
+
 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, const struct decoration_options *opts);
 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 718530bbab..24fb82a5a2 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1537,10 +1537,12 @@ 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, NULL);
 		return 1;
 	case 'D':
-		format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
+		format_decorations(sb, commit, c->auto_color,
+			&(struct decoration_options){.prefix = "",
+						     .suffix = ""});
 		return 1;
 	case 'S':		/* tag/branch like --source */
 		if (!(c->pretty_ctx->rev && c->pretty_ctx->rev->sources))
-- 
2.42.0-rc2

[PATCH v4 4/8] decorate: avoid some unnecessary color overhead

[Andy Koppe]

In format_decorations(), don't obtain color sequences if there are no
decorations, and don't emit color sequences around empty strings.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 log-tree.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index cd12c26c29..7c6d3f1ac3 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -312,10 +312,7 @@ void format_decorations(struct strbuf *sb,
 {
 	const struct name_decoration *decoration;
 	const struct name_decoration *current_and_HEAD;
-	const char *color_commit =
-		diff_get_color(use_color, DIFF_COMMIT);
-	const char *color_reset =
-		decorate_get_color(use_color, DECORATION_NONE);
+	const char *color_commit, *color_reset;
 
 	const char *prefix = " (";
 	const char *suffix = ")";
@@ -334,6 +331,9 @@ void format_decorations(struct strbuf *sb,
 			separator = opts->separator;
 	}
 
+	color_commit = diff_get_color(use_color, DIFF_COMMIT);
+	color_reset = decorate_get_color(use_color, DECORATION_NONE);
+
 	current_and_HEAD = current_pointed_by_HEAD(decoration);
 	while (decoration) {
 		/*
@@ -342,9 +342,12 @@ void format_decorations(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);
+			if (*prefix) {
+				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: ");
@@ -364,9 +367,11 @@ void format_decorations(struct strbuf *sb,
 		}
 		decoration = decoration->next;
 	}
-	strbuf_addstr(sb, color_commit);
-	strbuf_addstr(sb, suffix);
-	strbuf_addstr(sb, color_reset);
+	if (*suffix) {
+		strbuf_addstr(sb, color_commit);
+		strbuf_addstr(sb, suffix);
+		strbuf_addstr(sb, color_reset);
+	}
 }
 
 void show_decorations(struct rev_info *opt, struct commit *commit)
-- 
2.42.0-rc2

[PATCH v4 5/8] decorate: color each token separately

[Andy Koppe]

Wrap "tag:" prefixes and the arrows in "HEAD -> branch" decorations in
their own color sequences. Otherwise, if --graph is used, tag names or
arrows can end up uncolored when %w width formatting breaks a line just
before them. This is because --graph resets the color after doing its
drawing at the start of a line.

Amend test t4207-log-decoration-colors.sh accordingly.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 log-tree.c                       | 14 +++++++---
 t/t4207-log-decoration-colors.sh | 44 ++++++++++++++++++--------------
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 7c6d3f1ac3..44f4693567 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -342,26 +342,34 @@ void format_decorations(struct strbuf *sb,
 		 * appeared, skipping the entry for current.
 		 */
 		if (decoration != current_and_HEAD) {
+			const char *color =
+				decorate_get_color(use_color, decoration->type);
+
 			if (*prefix) {
 				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)
+			if (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);
+			strbuf_addstr(sb, color_reset);
 
 			if (current_and_HEAD &&
 			    decoration->type == DECORATION_REF_HEAD) {
+				strbuf_addstr(sb, color);
 				strbuf_addstr(sb, " -> ");
 				strbuf_addstr(sb, color_reset);
 				strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
 				show_name(sb, current_and_HEAD);
+				strbuf_addstr(sb, color_reset);
 			}
-			strbuf_addstr(sb, color_reset);
 
 			prefix = separator;
 		}
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index ded33a82e2..df804f38e2 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -53,15 +53,17 @@ cmp_filtered_decorations () {
 # to this test since it does not contain any decoration, hence --first-parent
 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_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_HEAD} -> ${c_reset}${c_branch}main${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_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: ${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 &&
@@ -76,12 +78,14 @@ test_expect_success 'test coloring with replace-objects' '
 	git replace HEAD~1 HEAD~2 &&
 
 	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_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_HEAD} -> ${c_reset}${c_branch}main${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 &&
@@ -100,13 +104,15 @@ test_expect_success 'test coloring with grafted commit' '
 	git replace --graft HEAD HEAD~2 &&
 
 	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_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_HEAD} -> ${c_reset}${c_branch}main${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 &&
-- 
2.42.0-rc2

[PATCH v4 6/8] pretty: add %(decorate[:<options>]) format

[Andy Koppe]

Add %(decorate[:<options>]) format that lists ref names similarly to the
%d format, but which allows the otherwise fixed prefix, suffix and
separator strings to be customized. Omitted options default to the
strings used in %d.

Rename expand_separator() function used to expand %x literal formatting
codes to expand_string_arg(), as it is now used on strings other than
separators.

Examples:
- %(decorate) is equivalent to %d.
- %(decorate:prefix=,suffix=) is equivalent to %D.
- %(decorate:prefix=[,suffix=],separator=%x3B) produces a list enclosed
in square brackets and separated by semicolons.

Test the format in t4205-log-pretty-formats.sh and document it in
pretty-formats.txt.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 Documentation/pretty-formats.txt | 10 +++++
 pretty.c                         | 72 ++++++++++++++++++++++++++++----
 t/t4205-log-pretty-formats.sh    | 27 ++++++++++++
 3 files changed, 101 insertions(+), 8 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 851a9878e6..709d85af21 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -224,6 +224,16 @@ The placeholders are:
 	linkgit:git-rev-list[1])
 '%d':: ref names, like the --decorate option of linkgit:git-log[1]
 '%D':: ref names without the " (", ")" wrapping.
+'%(decorate[:<options>])'::
+ref names with custom decorations. The `decorate` string may be followed by a
+colon and zero or more comma-separated options. Option values may contain
+literal formatting codes. These must be used for commas (`%x2C`) and closing
+parentheses (`%x29`), due to their role in the option syntax.
++
+** 'prefix=<value>': Shown before the list of ref names.  Defaults to "{nbsp}`(`".
+** 'suffix=<value>': Shown after the list of ref names.  Defaults to "`)`".
+** 'separator=<value>': Shown between ref names.  Defaults to "`,`{nbsp}".
+
 '%(describe[:<options>])'::
 human-readable name, like linkgit:git-describe[1]; empty string for
 undescribable commits.  The `describe` string may be followed by a colon and
diff --git a/pretty.c b/pretty.c
index 24fb82a5a2..1639efe2f8 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1252,8 +1252,8 @@ static int format_trailer_match_cb(const struct strbuf *key, void *ud)
 	return 0;
 }
 
-static struct strbuf *expand_separator(struct strbuf *sb,
-				       const char *argval, size_t arglen)
+static struct strbuf *expand_string_arg(struct strbuf *sb,
+					const char *argval, size_t arglen)
 {
 	char *fmt = xstrndup(argval, arglen);
 	const char *format = fmt;
@@ -1301,9 +1301,9 @@ int format_set_trailers_options(struct process_trailer_options *opts,
 			opts->filter_data = filter_list;
 			opts->only_trailers = 1;
 		} else if (match_placeholder_arg_value(*arg, "separator", arg, &argval, &arglen)) {
-			opts->separator = expand_separator(sepbuf, argval, arglen);
+			opts->separator = expand_string_arg(sepbuf, argval, arglen);
 		} else if (match_placeholder_arg_value(*arg, "key_value_separator", arg, &argval, &arglen)) {
-			opts->key_value_separator = expand_separator(kvsepbuf, argval, arglen);
+			opts->key_value_separator = expand_string_arg(kvsepbuf, argval, arglen);
 		} else if (!match_placeholder_bool_arg(*arg, "only", arg, &opts->only_trailers) &&
 			   !match_placeholder_bool_arg(*arg, "unfold", arg, &opts->unfold) &&
 			   !match_placeholder_bool_arg(*arg, "keyonly", arg, &opts->key_only) &&
@@ -1384,6 +1384,40 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 	return arg - start;
 }
 
+
+static int parse_decoration_option(const char **arg,
+				   const char *name,
+				   char **opt)
+{
+	const char *argval;
+	size_t arglen;
+
+	if (match_placeholder_arg_value(*arg, name, arg, &argval, &arglen)) {
+		struct strbuf sb = STRBUF_INIT;
+
+		expand_string_arg(&sb, argval, arglen);
+		*opt = strbuf_detach(&sb, NULL);
+		return 1;
+	}
+	return 0;
+}
+
+static void parse_decoration_options(const char **arg,
+				     struct decoration_options *opts)
+{
+	while (parse_decoration_option(arg, "prefix", &opts->prefix) ||
+	       parse_decoration_option(arg, "suffix", &opts->suffix) ||
+	       parse_decoration_option(arg, "separator", &opts->separator))
+		;
+}
+
+static void free_decoration_options(const struct decoration_options *opts)
+{
+	free(opts->prefix);
+	free(opts->suffix);
+	free(opts->separator);
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1540,10 +1574,15 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		format_decorations(sb, commit, c->auto_color, NULL);
 		return 1;
 	case 'D':
-		format_decorations(sb, commit, c->auto_color,
-			&(struct decoration_options){.prefix = "",
-						     .suffix = ""});
-		return 1;
+		{
+			const struct decoration_options opts = {
+				.prefix = "",
+				.suffix = ""
+			};
+
+			format_decorations(sb, commit, c->auto_color, &opts);
+			return 1;
+		}
 	case 'S':		/* tag/branch like --source */
 		if (!(c->pretty_ctx->rev && c->pretty_ctx->rev->sources))
 			return 0;
@@ -1640,6 +1679,23 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		return 2;
 	}
 
+	if (skip_prefix(placeholder, "(decorate", &arg)) {
+		struct decoration_options opts = { NULL };
+		size_t ret = 0;
+
+		if (*arg == ':') {
+			arg++;
+			parse_decoration_options(&arg, &opts);
+		}
+		if (*arg == ')') {
+			format_decorations(sb, commit, c->auto_color, &opts);
+			ret = arg - placeholder + 1;
+		}
+
+		free_decoration_options(&opts);
+		return ret;
+	}
+
 	/* For the rest we have to parse the commit header. */
 	if (!c->commit_header_parsed) {
 		msg = c->message =
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index dd9035aa38..6ba399c5be 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -576,6 +576,33 @@ test_expect_success 'clean log decoration' '
 	test_cmp expected actual1
 '
 
+test_expect_success 'pretty format %decorate' '
+	git checkout -b foo &&
+	git commit --allow-empty -m "new commit" &&
+	git tag bar &&
+	git branch qux &&
+
+	echo " (HEAD -> foo, tag: bar, qux)" >expect1 &&
+	git log --format="%(decorate)" -1 >actual1 &&
+	test_cmp expect1 actual1 &&
+
+	echo "HEAD -> foo, tag: bar, qux" >expect2 &&
+	git log --format="%(decorate:prefix=,suffix=)" -1 >actual2 &&
+	test_cmp expect2 actual2 &&
+
+	echo "[ HEAD -> foo; tag: bar; qux ]" >expect3 &&
+	git log --format="%(decorate:prefix=[ ,suffix= ],separator=%x3B )" \
+		-1 >actual3 &&
+	test_cmp expect3 actual3 &&
+
+	# Try with a typo (in "separator"), in which case the placeholder should
+	# not be replaced.
+	echo "%(decorate:prefix=[ ,suffix= ],separater=; )" >expect4 &&
+	git log --format="%(decorate:prefix=[ ,suffix= ],separater=%x3B )" \
+		-1 >actual4 &&
+	test_cmp expect4 actual4
+'
+
 cat >trailers <<EOF
 Signed-off-by: A U Thor <author@example.com>
 Acked-by: A U Thor <author@example.com>
-- 
2.42.0-rc2

[PATCH v4 7/8] pretty: add pointer and tag options to %(decorate)

[Andy Koppe]

Add pointer and tag options to %(decorate) format, to allow to override
the " -> " string used to show where HEAD points and the "tag: " string
used to mark tags.

Document in pretty-formats.txt and test in t4205-log-pretty-formats.sh.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 Documentation/pretty-formats.txt |  9 +++++++++
 log-tree.c                       | 12 +++++++++---
 log-tree.h                       |  2 ++
 pretty.c                         |  6 +++++-
 t/t4205-log-pretty-formats.sh    |  7 ++++++-
 5 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 709d85af21..d38b4ab566 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -233,6 +233,15 @@ parentheses (`%x29`), due to their role in the option syntax.
 ** 'prefix=<value>': Shown before the list of ref names.  Defaults to "{nbsp}`(`".
 ** 'suffix=<value>': Shown after the list of ref names.  Defaults to "`)`".
 ** 'separator=<value>': Shown between ref names.  Defaults to "`,`{nbsp}".
+** 'pointer=<value>': Shown between HEAD and the branch it points to, if any.
+		      Defaults to "{nbsp}`->`{nbsp}".
+** 'tag=<value>': Shown before tag names. Defaults to "`tag:`{nbsp}".
+
++
+For example, to produce decorations with no wrapping
+or tag annotations, and spaces as separators:
++
+`%(decorate:prefix=,suffix=,tag=,separator= )`
 
 '%(describe[:<options>])'::
 human-readable name, like linkgit:git-describe[1]; empty string for
diff --git a/log-tree.c b/log-tree.c
index 44f4693567..50b4850eda 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -317,6 +317,8 @@ void format_decorations(struct strbuf *sb,
 	const char *prefix = " (";
 	const char *suffix = ")";
 	const char *separator = ", ";
+	const char *pointer = " -> ";
+	const char *tag = "tag: ";
 
 	decoration = get_name_decoration(&commit->object);
 	if (!decoration)
@@ -329,6 +331,10 @@ void format_decorations(struct strbuf *sb,
 			suffix = opts->suffix;
 		if (opts->separator)
 			separator = opts->separator;
+		if (opts->pointer)
+			pointer = opts->pointer;
+		if (opts->tag)
+			tag = opts->tag;
 	}
 
 	color_commit = diff_get_color(use_color, DIFF_COMMIT);
@@ -351,9 +357,9 @@ void format_decorations(struct strbuf *sb,
 				strbuf_addstr(sb, color_reset);
 			}
 
-			if (decoration->type == DECORATION_REF_TAG) {
+			if (*tag && decoration->type == DECORATION_REF_TAG) {
 				strbuf_addstr(sb, color);
-				strbuf_addstr(sb, "tag: ");
+				strbuf_addstr(sb, tag);
 				strbuf_addstr(sb, color_reset);
 			}
 
@@ -364,7 +370,7 @@ void format_decorations(struct strbuf *sb,
 			if (current_and_HEAD &&
 			    decoration->type == DECORATION_REF_HEAD) {
 				strbuf_addstr(sb, color);
-				strbuf_addstr(sb, " -> ");
+				strbuf_addstr(sb, pointer);
 				strbuf_addstr(sb, color_reset);
 				strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
 				show_name(sb, current_and_HEAD);
diff --git a/log-tree.h b/log-tree.h
index 14898de8ac..41c776fea5 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -17,6 +17,8 @@ struct decoration_options {
 	char *prefix;
 	char *suffix;
 	char *separator;
+	char *pointer;
+	char *tag;
 };
 
 int parse_decorate_color_config(const char *var, const char *slot_name, const char *value);
diff --git a/pretty.c b/pretty.c
index 1639efe2f8..7f3abb676c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1407,7 +1407,9 @@ static void parse_decoration_options(const char **arg,
 {
 	while (parse_decoration_option(arg, "prefix", &opts->prefix) ||
 	       parse_decoration_option(arg, "suffix", &opts->suffix) ||
-	       parse_decoration_option(arg, "separator", &opts->separator))
+	       parse_decoration_option(arg, "separator", &opts->separator) ||
+	       parse_decoration_option(arg, "pointer", &opts->pointer) ||
+	       parse_decoration_option(arg, "tag", &opts->tag))
 		;
 }
 
@@ -1416,6 +1418,8 @@ static void free_decoration_options(const struct decoration_options *opts)
 	free(opts->prefix);
 	free(opts->suffix);
 	free(opts->separator);
+	free(opts->pointer);
+	free(opts->tag);
 }
 
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 6ba399c5be..16626e4fe9 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -600,7 +600,12 @@ test_expect_success 'pretty format %decorate' '
 	echo "%(decorate:prefix=[ ,suffix= ],separater=; )" >expect4 &&
 	git log --format="%(decorate:prefix=[ ,suffix= ],separater=%x3B )" \
 		-1 >actual4 &&
-	test_cmp expect4 actual4
+	test_cmp expect4 actual4 &&
+
+	echo "HEAD->foo bar qux" >expect5 &&
+	git log --format="%(decorate:prefix=,suffix=,separator= ,tag=,pointer=->)" \
+		-1 >actual5 &&
+	test_cmp expect5 actual5
 '
 
 cat >trailers <<EOF
-- 
2.42.0-rc2

[PATCH v4 8/8] decorate: use commit color for HEAD arrow

[Andy Koppe]

Use the commit color instead of the HEAD color for the arrow or custom
symbol in "HEAD -> branch" decorations, for visual consistency with the
prefix, separator and suffix symbols, which are also colored with the
commit color.

This change was triggered by the possibility that one could choose to
use the same symbol for the pointer and the separator options in
%(decorate), in which case they ought to be the same color.

A related precedent is 'ls -l', where the arrow for symlinks gets the
default color rather than that of the symlink name.

Amend test t4207-log-decoration-colors.sh accordingly.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 log-tree.c                       | 2 +-
 t/t4207-log-decoration-colors.sh | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 50b4850eda..504da6b519 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -369,7 +369,7 @@ void format_decorations(struct strbuf *sb,
 
 			if (current_and_HEAD &&
 			    decoration->type == DECORATION_REF_HEAD) {
-				strbuf_addstr(sb, color);
+				strbuf_addstr(sb, color_commit);
 				strbuf_addstr(sb, pointer);
 				strbuf_addstr(sb, color_reset);
 				strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index df804f38e2..21986a866d 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -54,7 +54,7 @@ cmp_filtered_decorations () {
 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_HEAD} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
+${c_commit} -> ${c_reset}${c_branch}main${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}\
@@ -79,7 +79,7 @@ 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_HEAD} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
+${c_commit} -> ${c_reset}${c_branch}main${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}, \
@@ -105,7 +105,7 @@ 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_HEAD} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
+${c_commit} -> ${c_reset}${c_branch}main${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}\
-- 
2.42.0-rc2

[PATCH v5 0/8] pretty: add %(decorate[:<options>]) format

[Andy Koppe]

Apologies for sending another version so soon, but I realized that I
hadn't removed the use of a compound literal from the first commit where
I had added it, so it still appeared in the patches. The overall diff
for v5 is the same as for v4.

Andy Koppe (7):
  pretty-formats: enclose options in angle brackets
  decorate: refactor format_decorations()
  decorate: avoid some unnecessary color overhead
  decorate: color each token separately
  pretty: add %(decorate[:<options>]) format
  pretty: add pointer and tag options to %(decorate)
  decorate: use commit color for HEAD arrow

Junio C Hamano (1):
  pretty-formats: define "literal formatting code"

 Documentation/pretty-formats.txt | 47 +++++++++++++-------
 log-tree.c                       | 72 +++++++++++++++++++++---------
 log-tree.h                       | 17 ++++---
 pretty.c                         | 76 +++++++++++++++++++++++++++++---
 t/t4205-log-pretty-formats.sh    | 32 ++++++++++++++
 t/t4207-log-decoration-colors.sh | 44 ++++++++++--------
 6 files changed, 219 insertions(+), 69 deletions(-)

-- 
2.42.0-rc2

[PATCH v5 1/8] pretty-formats: define "literal formatting code"

[Andy Koppe]

From: Junio C Hamano <gitster@pobox.com>

The description for a %(trailer) option already uses this term without
having a definition anywhere in the document, and we are about to add
another one in %(decorate) that uses it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 Documentation/pretty-formats.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 3b71334459..5e1432951b 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -122,7 +122,9 @@ The placeholders are:
 - Placeholders that expand to a single literal character:
 '%n':: newline
 '%%':: a raw '%'
-'%x00':: print a byte from a hex code
+'%x00':: '%x' followed by two hexadecimal digits is replaced with a
+	 byte with the hexadecimal digits' value (we will call this
+	 "literal formatting code" in the rest of this document).
 
 - Placeholders that affect formatting of later placeholders:
 '%Cred':: switch color to red
-- 
2.42.0-rc2

[PATCH v5 2/8] pretty-formats: enclose options in angle brackets

[Andy Koppe]

Enclose the 'options' placeholders in the documentation of the
%(describe) and %(trailers) format specifiers in angle brackets to
clarify that they are placeholders rather than keywords.

Also remove the indentation from their descriptions, instead of
increasing it to account for the extra two angle brackets in the
headings. The indentation isn't required by asciidoc, it doesn't reflect
how the output text is formatted, and it's inconsistent with the
following bullet points that are at the same level in the output.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 Documentation/pretty-formats.txt | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 5e1432951b..851a9878e6 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -224,13 +224,11 @@ The placeholders are:
 	linkgit:git-rev-list[1])
 '%d':: ref names, like the --decorate option of linkgit:git-log[1]
 '%D':: ref names without the " (", ")" wrapping.
-'%(describe[:options])':: human-readable name, like
-			  linkgit:git-describe[1]; empty string for
-			  undescribable commits.  The `describe` string
-			  may be followed by a colon and zero or more
-			  comma-separated options.  Descriptions can be
-			  inconsistent when tags are added or removed at
-			  the same time.
+'%(describe[:<options>])'::
+human-readable name, like linkgit:git-describe[1]; empty string for
+undescribable commits.  The `describe` string may be followed by a colon and
+zero or more comma-separated options.  Descriptions can be inconsistent when
+tags are added or removed at the same time.
 +
 ** 'tags[=<bool-value>]': Instead of only considering annotated tags,
    consider lightweight tags as well.
@@ -283,13 +281,11 @@ endif::git-rev-list[]
 '%gE':: reflog identity email (respecting .mailmap, see
 	linkgit:git-shortlog[1] or linkgit:git-blame[1])
 '%gs':: reflog subject
-'%(trailers[:options])':: display the trailers of the body as
-			  interpreted by
-			  linkgit:git-interpret-trailers[1]. The
-			  `trailers` string may be followed by a colon
-			  and zero or more comma-separated options.
-			  If any option is provided multiple times the
-			  last occurrence wins.
+'%(trailers[:<options>])'::
+display the trailers of the body as interpreted by
+linkgit:git-interpret-trailers[1]. The `trailers` string may be followed by
+a colon and zero or more comma-separated options. If any option is provided
+multiple times, the last occurrence wins.
 +
 ** 'key=<key>': only show trailers with specified <key>. Matching is done
    case-insensitively and trailing colon is optional. If option is
-- 
2.42.0-rc2

[PATCH v5 3/8] decorate: refactor format_decorations()

[Andy Koppe]

Rename the format_decorations_extended function to format_decorations
and drop the format_decorations wrapper macro. Pass the prefix, suffix
and separator strings as a single 'struct format_decorations' pointer
argument instead of separate arguments. Use default values defined in
the function when either the struct pointer or any of the struct fields
are NULL. This is to ease extension with additional options.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 log-tree.c | 23 +++++++++++++++++------
 log-tree.h | 15 ++++++++-------
 pretty.c   | 13 ++++++++++---
 3 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 208c69cbb7..cd12c26c29 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -303,14 +303,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 ensures 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)
+			const struct decoration_options *opts)
 {
 	const struct name_decoration *decoration;
 	const struct name_decoration *current_and_HEAD;
@@ -319,10 +317,23 @@ void format_decorations_extended(struct strbuf *sb,
 	const char *color_reset =
 		decorate_get_color(use_color, DECORATION_NONE);
 
+	const char *prefix = " (";
+	const char *suffix = ")";
+	const char *separator = ", ";
+
 	decoration = get_name_decoration(&commit->object);
 	if (!decoration)
 		return;
 
+	if (opts) {
+		if (opts->prefix)
+			prefix = opts->prefix;
+		if (opts->suffix)
+			suffix = opts->suffix;
+		if (opts->separator)
+			separator = opts->separator;
+	}
+
 	current_and_HEAD = current_pointed_by_HEAD(decoration);
 	while (decoration) {
 		/*
@@ -370,7 +381,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, NULL);
 	fputs(sb.buf, opt->diffopt.file);
 	strbuf_release(&sb);
 }
diff --git a/log-tree.h b/log-tree.h
index bdb6432815..14898de8ac 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -13,17 +13,18 @@ struct decoration_filter {
 	struct string_list *exclude_ref_config_pattern;
 };
 
+struct decoration_options {
+	char *prefix;
+	char *suffix;
+	char *separator;
+};
+
 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, const struct decoration_options *opts);
 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 718530bbab..69b6db3340 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1537,11 +1537,18 @@ 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, NULL);
 		return 1;
 	case 'D':
-		format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
-		return 1;
+		{
+			const struct decoration_options opts = {
+				.prefix = "",
+				.suffix = ""
+			};
+
+			format_decorations(sb, commit, c->auto_color, &opts);
+			return 1;
+		}
 	case 'S':		/* tag/branch like --source */
 		if (!(c->pretty_ctx->rev && c->pretty_ctx->rev->sources))
 			return 0;
-- 
2.42.0-rc2

[PATCH v5 4/8] decorate: avoid some unnecessary color overhead

[Andy Koppe]

In format_decorations(), don't obtain color sequences if there are no
decorations, and don't emit color sequences around empty strings.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 log-tree.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index cd12c26c29..7c6d3f1ac3 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -312,10 +312,7 @@ void format_decorations(struct strbuf *sb,
 {
 	const struct name_decoration *decoration;
 	const struct name_decoration *current_and_HEAD;
-	const char *color_commit =
-		diff_get_color(use_color, DIFF_COMMIT);
-	const char *color_reset =
-		decorate_get_color(use_color, DECORATION_NONE);
+	const char *color_commit, *color_reset;
 
 	const char *prefix = " (";
 	const char *suffix = ")";
@@ -334,6 +331,9 @@ void format_decorations(struct strbuf *sb,
 			separator = opts->separator;
 	}
 
+	color_commit = diff_get_color(use_color, DIFF_COMMIT);
+	color_reset = decorate_get_color(use_color, DECORATION_NONE);
+
 	current_and_HEAD = current_pointed_by_HEAD(decoration);
 	while (decoration) {
 		/*
@@ -342,9 +342,12 @@ void format_decorations(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);
+			if (*prefix) {
+				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: ");
@@ -364,9 +367,11 @@ void format_decorations(struct strbuf *sb,
 		}
 		decoration = decoration->next;
 	}
-	strbuf_addstr(sb, color_commit);
-	strbuf_addstr(sb, suffix);
-	strbuf_addstr(sb, color_reset);
+	if (*suffix) {
+		strbuf_addstr(sb, color_commit);
+		strbuf_addstr(sb, suffix);
+		strbuf_addstr(sb, color_reset);
+	}
 }
 
 void show_decorations(struct rev_info *opt, struct commit *commit)
-- 
2.42.0-rc2

[PATCH v5 5/8] decorate: color each token separately

[Andy Koppe]

Wrap "tag:" prefixes and the arrows in "HEAD -> branch" decorations in
their own color sequences. Otherwise, if --graph is used, tag names or
arrows can end up uncolored when %w width formatting breaks a line just
before them. This is because --graph resets the color after doing its
drawing at the start of a line.

Amend test t4207-log-decoration-colors.sh accordingly.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 log-tree.c                       | 14 +++++++---
 t/t4207-log-decoration-colors.sh | 44 ++++++++++++++++++--------------
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 7c6d3f1ac3..44f4693567 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -342,26 +342,34 @@ void format_decorations(struct strbuf *sb,
 		 * appeared, skipping the entry for current.
 		 */
 		if (decoration != current_and_HEAD) {
+			const char *color =
+				decorate_get_color(use_color, decoration->type);
+
 			if (*prefix) {
 				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)
+			if (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);
+			strbuf_addstr(sb, color_reset);
 
 			if (current_and_HEAD &&
 			    decoration->type == DECORATION_REF_HEAD) {
+				strbuf_addstr(sb, color);
 				strbuf_addstr(sb, " -> ");
 				strbuf_addstr(sb, color_reset);
 				strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
 				show_name(sb, current_and_HEAD);
+				strbuf_addstr(sb, color_reset);
 			}
-			strbuf_addstr(sb, color_reset);
 
 			prefix = separator;
 		}
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index ded33a82e2..df804f38e2 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -53,15 +53,17 @@ cmp_filtered_decorations () {
 # to this test since it does not contain any decoration, hence --first-parent
 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_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_HEAD} -> ${c_reset}${c_branch}main${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_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: ${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 &&
@@ -76,12 +78,14 @@ test_expect_success 'test coloring with replace-objects' '
 	git replace HEAD~1 HEAD~2 &&
 
 	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_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_HEAD} -> ${c_reset}${c_branch}main${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 &&
@@ -100,13 +104,15 @@ test_expect_success 'test coloring with grafted commit' '
 	git replace --graft HEAD HEAD~2 &&
 
 	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_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_HEAD} -> ${c_reset}${c_branch}main${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 &&
-- 
2.42.0-rc2

[PATCH v5 6/8] pretty: add %(decorate[:<options>]) format

[Andy Koppe]

Add %(decorate[:<options>]) format that lists ref names similarly to the
%d format, but which allows the otherwise fixed prefix, suffix and
separator strings to be customized. Omitted options default to the
strings used in %d.

Rename expand_separator() function used to expand %x literal formatting
codes to expand_string_arg(), as it is now used on strings other than
separators.

Examples:
- %(decorate) is equivalent to %d.
- %(decorate:prefix=,suffix=) is equivalent to %D.
- %(decorate:prefix=[,suffix=],separator=%x3B) produces a list enclosed
in square brackets and separated by semicolons.

Test the format in t4205-log-pretty-formats.sh and document it in
pretty-formats.txt.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 Documentation/pretty-formats.txt | 10 ++++++
 pretty.c                         | 59 +++++++++++++++++++++++++++++---
 t/t4205-log-pretty-formats.sh    | 27 +++++++++++++++
 3 files changed, 92 insertions(+), 4 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 851a9878e6..709d85af21 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -224,6 +224,16 @@ The placeholders are:
 	linkgit:git-rev-list[1])
 '%d':: ref names, like the --decorate option of linkgit:git-log[1]
 '%D':: ref names without the " (", ")" wrapping.
+'%(decorate[:<options>])'::
+ref names with custom decorations. The `decorate` string may be followed by a
+colon and zero or more comma-separated options. Option values may contain
+literal formatting codes. These must be used for commas (`%x2C`) and closing
+parentheses (`%x29`), due to their role in the option syntax.
++
+** 'prefix=<value>': Shown before the list of ref names.  Defaults to "{nbsp}`(`".
+** 'suffix=<value>': Shown after the list of ref names.  Defaults to "`)`".
+** 'separator=<value>': Shown between ref names.  Defaults to "`,`{nbsp}".
+
 '%(describe[:<options>])'::
 human-readable name, like linkgit:git-describe[1]; empty string for
 undescribable commits.  The `describe` string may be followed by a colon and
diff --git a/pretty.c b/pretty.c
index 69b6db3340..1639efe2f8 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1252,8 +1252,8 @@ static int format_trailer_match_cb(const struct strbuf *key, void *ud)
 	return 0;
 }
 
-static struct strbuf *expand_separator(struct strbuf *sb,
-				       const char *argval, size_t arglen)
+static struct strbuf *expand_string_arg(struct strbuf *sb,
+					const char *argval, size_t arglen)
 {
 	char *fmt = xstrndup(argval, arglen);
 	const char *format = fmt;
@@ -1301,9 +1301,9 @@ int format_set_trailers_options(struct process_trailer_options *opts,
 			opts->filter_data = filter_list;
 			opts->only_trailers = 1;
 		} else if (match_placeholder_arg_value(*arg, "separator", arg, &argval, &arglen)) {
-			opts->separator = expand_separator(sepbuf, argval, arglen);
+			opts->separator = expand_string_arg(sepbuf, argval, arglen);
 		} else if (match_placeholder_arg_value(*arg, "key_value_separator", arg, &argval, &arglen)) {
-			opts->key_value_separator = expand_separator(kvsepbuf, argval, arglen);
+			opts->key_value_separator = expand_string_arg(kvsepbuf, argval, arglen);
 		} else if (!match_placeholder_bool_arg(*arg, "only", arg, &opts->only_trailers) &&
 			   !match_placeholder_bool_arg(*arg, "unfold", arg, &opts->unfold) &&
 			   !match_placeholder_bool_arg(*arg, "keyonly", arg, &opts->key_only) &&
@@ -1384,6 +1384,40 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 	return arg - start;
 }
 
+
+static int parse_decoration_option(const char **arg,
+				   const char *name,
+				   char **opt)
+{
+	const char *argval;
+	size_t arglen;
+
+	if (match_placeholder_arg_value(*arg, name, arg, &argval, &arglen)) {
+		struct strbuf sb = STRBUF_INIT;
+
+		expand_string_arg(&sb, argval, arglen);
+		*opt = strbuf_detach(&sb, NULL);
+		return 1;
+	}
+	return 0;
+}
+
+static void parse_decoration_options(const char **arg,
+				     struct decoration_options *opts)
+{
+	while (parse_decoration_option(arg, "prefix", &opts->prefix) ||
+	       parse_decoration_option(arg, "suffix", &opts->suffix) ||
+	       parse_decoration_option(arg, "separator", &opts->separator))
+		;
+}
+
+static void free_decoration_options(const struct decoration_options *opts)
+{
+	free(opts->prefix);
+	free(opts->suffix);
+	free(opts->separator);
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1645,6 +1679,23 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		return 2;
 	}
 
+	if (skip_prefix(placeholder, "(decorate", &arg)) {
+		struct decoration_options opts = { NULL };
+		size_t ret = 0;
+
+		if (*arg == ':') {
+			arg++;
+			parse_decoration_options(&arg, &opts);
+		}
+		if (*arg == ')') {
+			format_decorations(sb, commit, c->auto_color, &opts);
+			ret = arg - placeholder + 1;
+		}
+
+		free_decoration_options(&opts);
+		return ret;
+	}
+
 	/* For the rest we have to parse the commit header. */
 	if (!c->commit_header_parsed) {
 		msg = c->message =
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index dd9035aa38..6ba399c5be 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -576,6 +576,33 @@ test_expect_success 'clean log decoration' '
 	test_cmp expected actual1
 '
 
+test_expect_success 'pretty format %decorate' '
+	git checkout -b foo &&
+	git commit --allow-empty -m "new commit" &&
+	git tag bar &&
+	git branch qux &&
+
+	echo " (HEAD -> foo, tag: bar, qux)" >expect1 &&
+	git log --format="%(decorate)" -1 >actual1 &&
+	test_cmp expect1 actual1 &&
+
+	echo "HEAD -> foo, tag: bar, qux" >expect2 &&
+	git log --format="%(decorate:prefix=,suffix=)" -1 >actual2 &&
+	test_cmp expect2 actual2 &&
+
+	echo "[ HEAD -> foo; tag: bar; qux ]" >expect3 &&
+	git log --format="%(decorate:prefix=[ ,suffix= ],separator=%x3B )" \
+		-1 >actual3 &&
+	test_cmp expect3 actual3 &&
+
+	# Try with a typo (in "separator"), in which case the placeholder should
+	# not be replaced.
+	echo "%(decorate:prefix=[ ,suffix= ],separater=; )" >expect4 &&
+	git log --format="%(decorate:prefix=[ ,suffix= ],separater=%x3B )" \
+		-1 >actual4 &&
+	test_cmp expect4 actual4
+'
+
 cat >trailers <<EOF
 Signed-off-by: A U Thor <author@example.com>
 Acked-by: A U Thor <author@example.com>
-- 
2.42.0-rc2

[PATCH v5 7/8] pretty: add pointer and tag options to %(decorate)

[Andy Koppe]

Add pointer and tag options to %(decorate) format, to allow to override
the " -> " string used to show where HEAD points and the "tag: " string
used to mark tags.

Document in pretty-formats.txt and test in t4205-log-pretty-formats.sh.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 Documentation/pretty-formats.txt |  9 +++++++++
 log-tree.c                       | 12 +++++++++---
 log-tree.h                       |  2 ++
 pretty.c                         |  6 +++++-
 t/t4205-log-pretty-formats.sh    |  7 ++++++-
 5 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 709d85af21..d38b4ab566 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -233,6 +233,15 @@ parentheses (`%x29`), due to their role in the option syntax.
 ** 'prefix=<value>': Shown before the list of ref names.  Defaults to "{nbsp}`(`".
 ** 'suffix=<value>': Shown after the list of ref names.  Defaults to "`)`".
 ** 'separator=<value>': Shown between ref names.  Defaults to "`,`{nbsp}".
+** 'pointer=<value>': Shown between HEAD and the branch it points to, if any.
+		      Defaults to "{nbsp}`->`{nbsp}".
+** 'tag=<value>': Shown before tag names. Defaults to "`tag:`{nbsp}".
+
++
+For example, to produce decorations with no wrapping
+or tag annotations, and spaces as separators:
++
+`%(decorate:prefix=,suffix=,tag=,separator= )`
 
 '%(describe[:<options>])'::
 human-readable name, like linkgit:git-describe[1]; empty string for
diff --git a/log-tree.c b/log-tree.c
index 44f4693567..50b4850eda 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -317,6 +317,8 @@ void format_decorations(struct strbuf *sb,
 	const char *prefix = " (";
 	const char *suffix = ")";
 	const char *separator = ", ";
+	const char *pointer = " -> ";
+	const char *tag = "tag: ";
 
 	decoration = get_name_decoration(&commit->object);
 	if (!decoration)
@@ -329,6 +331,10 @@ void format_decorations(struct strbuf *sb,
 			suffix = opts->suffix;
 		if (opts->separator)
 			separator = opts->separator;
+		if (opts->pointer)
+			pointer = opts->pointer;
+		if (opts->tag)
+			tag = opts->tag;
 	}
 
 	color_commit = diff_get_color(use_color, DIFF_COMMIT);
@@ -351,9 +357,9 @@ void format_decorations(struct strbuf *sb,
 				strbuf_addstr(sb, color_reset);
 			}
 
-			if (decoration->type == DECORATION_REF_TAG) {
+			if (*tag && decoration->type == DECORATION_REF_TAG) {
 				strbuf_addstr(sb, color);
-				strbuf_addstr(sb, "tag: ");
+				strbuf_addstr(sb, tag);
 				strbuf_addstr(sb, color_reset);
 			}
 
@@ -364,7 +370,7 @@ void format_decorations(struct strbuf *sb,
 			if (current_and_HEAD &&
 			    decoration->type == DECORATION_REF_HEAD) {
 				strbuf_addstr(sb, color);
-				strbuf_addstr(sb, " -> ");
+				strbuf_addstr(sb, pointer);
 				strbuf_addstr(sb, color_reset);
 				strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
 				show_name(sb, current_and_HEAD);
diff --git a/log-tree.h b/log-tree.h
index 14898de8ac..41c776fea5 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -17,6 +17,8 @@ struct decoration_options {
 	char *prefix;
 	char *suffix;
 	char *separator;
+	char *pointer;
+	char *tag;
 };
 
 int parse_decorate_color_config(const char *var, const char *slot_name, const char *value);
diff --git a/pretty.c b/pretty.c
index 1639efe2f8..7f3abb676c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1407,7 +1407,9 @@ static void parse_decoration_options(const char **arg,
 {
 	while (parse_decoration_option(arg, "prefix", &opts->prefix) ||
 	       parse_decoration_option(arg, "suffix", &opts->suffix) ||
-	       parse_decoration_option(arg, "separator", &opts->separator))
+	       parse_decoration_option(arg, "separator", &opts->separator) ||
+	       parse_decoration_option(arg, "pointer", &opts->pointer) ||
+	       parse_decoration_option(arg, "tag", &opts->tag))
 		;
 }
 
@@ -1416,6 +1418,8 @@ static void free_decoration_options(const struct decoration_options *opts)
 	free(opts->prefix);
 	free(opts->suffix);
 	free(opts->separator);
+	free(opts->pointer);
+	free(opts->tag);
 }
 
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 6ba399c5be..16626e4fe9 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -600,7 +600,12 @@ test_expect_success 'pretty format %decorate' '
 	echo "%(decorate:prefix=[ ,suffix= ],separater=; )" >expect4 &&
 	git log --format="%(decorate:prefix=[ ,suffix= ],separater=%x3B )" \
 		-1 >actual4 &&
-	test_cmp expect4 actual4
+	test_cmp expect4 actual4 &&
+
+	echo "HEAD->foo bar qux" >expect5 &&
+	git log --format="%(decorate:prefix=,suffix=,separator= ,tag=,pointer=->)" \
+		-1 >actual5 &&
+	test_cmp expect5 actual5
 '
 
 cat >trailers <<EOF
-- 
2.42.0-rc2

[PATCH v5 8/8] decorate: use commit color for HEAD arrow

[Andy Koppe]

Use the commit color instead of the HEAD color for the arrow or custom
symbol in "HEAD -> branch" decorations, for visual consistency with the
prefix, separator and suffix symbols, which are also colored with the
commit color.

This change was triggered by the possibility that one could choose to
use the same symbol for the pointer and the separator options in
%(decorate), in which case they ought to be the same color.

A related precedent is 'ls -l', where the arrow for symlinks gets the
default color rather than that of the symlink name.

Amend test t4207-log-decoration-colors.sh accordingly.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 log-tree.c                       | 2 +-
 t/t4207-log-decoration-colors.sh | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 50b4850eda..504da6b519 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -369,7 +369,7 @@ void format_decorations(struct strbuf *sb,
 
 			if (current_and_HEAD &&
 			    decoration->type == DECORATION_REF_HEAD) {
-				strbuf_addstr(sb, color);
+				strbuf_addstr(sb, color_commit);
 				strbuf_addstr(sb, pointer);
 				strbuf_addstr(sb, color_reset);
 				strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index df804f38e2..21986a866d 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -54,7 +54,7 @@ cmp_filtered_decorations () {
 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_HEAD} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
+${c_commit} -> ${c_reset}${c_branch}main${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}\
@@ -79,7 +79,7 @@ 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_HEAD} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
+${c_commit} -> ${c_reset}${c_branch}main${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}, \
@@ -105,7 +105,7 @@ 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_HEAD} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
+${c_commit} -> ${c_reset}${c_branch}main${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}\
-- 
2.42.0-rc2

Re: main != master at github.com/git/git

[Johannes Schindelin]

Hi Junio,

On Thu, 17 Aug 2023, Junio C Hamano wrote:

> <rsbecker@nexbridge.com> writes:
>
> [...] when CI runner sees branches updated to commit that hasn't been
> worked on, a new job is created to work on that commit, and updating two
> branches with the same commit at the same time unfortunately means two
> independent CI jobs work on the same commit in parallel.

My understanding is that the recommended way to handle this via the
`concurrency` key [*1*]. That is, if we changed

    concurrency:
      group: windows-build-${{ github.ref }}
      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}

to

    concurrency:
      group: windows-build-${{ github.sha }}

then pushing both `master` and `next` pointing at the same commit would
start only one of the workflow runs immediately, keeping the second one
pending until the first run is done. If the first run succeeds, the second
run will pick up that status and avoid running everything all over again,
via `skip-if-redundant`.

Ciao,
Johannes

Footnote *1*:
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#concurrency

Re: main != master at github.com/git/git

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> My understanding is that the recommended way to handle this via the
> `concurrency` key [*1*]. That is, if we changed
>
>     concurrency:
>       group: windows-build-${{ github.ref }}
>       cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
>
> to
>
>     concurrency:
>       group: windows-build-${{ github.sha }}
>
> then pushing both `master` and `next` pointing at the same commit would
> start only one of the workflow runs immediately, keeping the second one
> pending until the first run is done.

Perfect.  It is much better than pushing 'master@{24.hours.ago} to
'main' which was what I used to do avoid the problem between these
two, which I stopped doing because it did not work well.

> If the first run succeeds, the second
> run will pick up that status and avoid running everything all over again,
> via `skip-if-redundant`.

Nice.  I understand that this would kick in regardless, but the
right use of the concurrency key would make it far more effective.

Very nice.

Re: [PATCH v4 0/8] pretty: add %(decorate[:<options>]) format

[Junio C Hamano]

Andy Koppe <andy.koppe@gmail.com> writes:

> Compared to v3, this avoids introducing a compound literal, and splits
> part of patch 5 into an additional patch 8.

Thanks, will requeue.

[PATCH] ci: avoid building from the same commit in parallel

[Junio C Hamano]

At times, we may need to push the same commit to multiple branches
in the same push.  Rewinding 'next' to rebuild on top of 'master'
soon after a release is such an occasion.  Making sure 'main' stays
in sync with 'master' to help those who expect that primary branch
of the project is named either of these is another.

We used to use the branch name as the "concurrency group" key, but
by switching to use the commit object name would make sure the
builds for the same commit would happen serially, and by the time
the second job becomes ready to run, the first job's outcome would
be available and mking it unnecessary to run the second job.

Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * There are tons of concurrency groups defined, but as a trial
   change, here is to cover the "regular" matrix that consumes the
   most resources (linux-asan-ubsan is the worst culprit, it seems).

 .github/workflows/main.yml | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 30492eacdd..27b151aadf 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -240,8 +240,7 @@ jobs:
     needs: ci-config
     if: needs.ci-config.outputs.enabled == 'yes'
     concurrency:
-      group: ${{ matrix.vector.jobname }}-${{ matrix.vector.pool }}-${{ github.ref }}
-      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
+      group: ${{ matrix.vector.jobname }}-${{ matrix.vector.pool }}-${{ github.sha }}
     strategy:
       fail-fast: false
       matrix:
-- 
2.42.0

Re: [PATCH] ci: avoid building from the same commit in parallel

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>  * There are tons of concurrency groups defined, but as a trial
>    change, here is to cover the "regular" matrix that consumes the
>    most resources (linux-asan-ubsan is the worst culprit, it seems).

Unfortunately, this did not work.

https://github.com/git/git/actions/runs/5933451874
https://github.com/git/git/actions/runs/5933451805

are trial runs that had the same commit with this patch pushed to
'seen' and 'pu'.  While one of them started the "regular" matrix,
the other one indeed went into paused state and waited.  But that is
way too late.  What happened was that their "config" (which everything
else depends on) started in parallel before the serialization at the
"regular" matrix kicked in.

So, one did wait before doing the "regular" matrix, until the other
one finished everything, and then kept going and did its own
"regular" matrix for the same commit.  It is because the avoidance
of "redundant build" was done at the "config" phase, which both of
them had already done X-<.

If we wanted to do this, I suspect that we need to serialize the
entire thing, not at the individual level where we currently define
the "concurrency" thing.

Re: [PATCH] ci: avoid building from the same commit in parallel

[Johannes Schindelin]

Hi Junio,

On Mon, 21 Aug 2023, Junio C Hamano wrote:

> If we wanted to do this, I suspect that we need to serialize the
> entire thing, not at the individual level where we currently define
> the "concurrency" thing.

Right, we'd need that `concurrency: ${{ github.sha }}` attribute on the
`config` job.

BTW there is another caveat. According to the documentation, if a job is
queued while another job is already queued, that other job is canceled in
favor of the latest one.

I have not verified this by testing it out, but have no reason to doubt
it.

If that's the case, pushing, say, `master`, `main` and `next` to the same
SHA will see one of them canceled. Which might be okay, if the cancelation
message on the canceled one is indicative enough.

This all depends on reducing the number of flakes (in particular the p4
tests in the ASAN job), but that's a story for another day.

Ciao,
Johannes

Re: [PATCH] ci: avoid building from the same commit in parallel

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Right, we'd need that `concurrency: ${{ github.sha }}` attribute on the
> `config` job.

That was my first thought, but I am not sure how it would work.

Doesn't skip-if-redundant grab the workflow runs that have succeeded
and then see if one for the same commit already exists?  If you used
concurrency on the 'config', what gets serialized between two jobs
for the same commit is only the 'config' phase, so 'master' may wait
starting (because 'config' is what everybody else 'needs' it) while
'config' phase of 'main' runs, and then when it gets to the turn of
'config' phase of 'master', it would not find the run for the same
commit being done for 'main' completed yet, would it?

> BTW there is another caveat. According to the documentation, if a job is
> queued while another job is already queued, that other job is canceled in
> favor of the latest one.

Yes, that was the impression I got; your second one will wait (so
you need a working skip-if-redundant to turn it into noop), but the
third and subsequent ones are discarded without starting, which
unfortunately is what we may want to see happen.

Hmph, from that point of view, would the best and simplest we can do
be to use the commit object name as the concurrency key for the
'config' phase, and use something similar to cancel-in-progress
(which kills the other one when the new one starts, but what we want
is what stops the new one to start when it notices there is already
one running)?

Re: [PATCH] ci: avoid building from the same commit in parallel

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 4624 bytes --]

Hi Junio,

On Tue, 22 Aug 2023, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Right, we'd need that `concurrency: ${{ github.sha }}` attribute on
> > the `config` job.
>
> That was my first thought, but I am not sure how it would work.
>
> Doesn't skip-if-redundant grab the workflow runs that have succeeded
> and then see if one for the same commit already exists?  If you used
> concurrency on the 'config', what gets serialized between two jobs
> for the same commit is only the 'config' phase, so 'master' may wait
> starting (because 'config' is what everybody else 'needs' it) while
> 'config' phase of 'main' runs, and then when it gets to the turn of
> 'config' phase of 'master', it would not find the run for the same
> commit being done for 'main' completed yet, would it?

Yes, that's true.

But there is a silver lining: the `concurrency` can not only be specified
on the job level, but also on the workflow run level.

I tested this, and present the corresponding patch at the end of this
mail.

> > BTW there is another caveat. According to the documentation, if a job
> > is queued while another job is already queued, that other job is
> > canceled in favor of the latest one.
>
> Yes, that was the impression I got; your second one will wait (so
> you need a working skip-if-redundant to turn it into noop), but the
> third and subsequent ones are discarded without starting, which
> unfortunately is what we may want to see happen.

It's actually the last one that is still pending, while the intermediate
ones will be canceled before they are started (see also the attached
screenshot). The message that is shown in the web UI reads like this:

 𐤈 CI: .github#L1
   Canceling since a higher priority waiting request for '<SHA>' exists

See https://github.com/dscho/git/actions/runs/5948890677 for an example.

Here is the patch:

-- snipsnap --
From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 21 Aug 2023 17:31:26 -0700
Subject: [PATCH] ci: avoid building from the same commit in parallel

At times, we may need to push the same commit to multiple branches
in the same push.  Rewinding 'next' to rebuild on top of 'master'
soon after a release is such an occasion.  Making sure 'main' stays
in sync with 'master' to help those who expect that primary branch
of the project is named either of these is another.

We already use the branch name as a "concurrency group" key, but
that does not address the situation illustrated above.

Let's introduce another `concurrency` attribute, using the commit
hash as the concurrency group key, on the workflow run level, to
address this. This will hold any workflow run in the queued state
when there is already a workflow run targeting the same commit,
until that latter run completed. The `skip-if-redundant` check of
the second run will then have a chance to see whether the first
run succeeded.

The only caveat with this strategy is that only one workflow run
will be kept in the queued state by the `concurrency` feature: if
another run targeting the same commit is triggered, the
previously-queued run will be canceled. Considering the benefit,
this seems the smaller price to pay than to overload Git's build
agent pool with undesired workflow runs.

Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 .github/workflows/main.yml | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 30492eacddf..1739a0278dc 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -5,6 +5,19 @@ on: [push, pull_request]
 env:
   DEVELOPER: 1

+# If more than one workflow run is triggered for the very same commit hash
+# (which happens when multiple branches pointing to the same commit), only
+# the first one is allowed to run, the second will be kept in the "queued"
+# state. This allows a successful completion of the first run to be reused
+# in the second run via the `skip-if-redundant` logic in the `config` job.
+#
+# The only caveat is that if a workflow run is triggered for the same commit
+# hash that another run is already being held, that latter run will be
+# canceled. For more details about the `concurrency` attribute, see:
+# https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#concurrency
+concurrency:
+  group: ${{ github.sha }}
+
 jobs:
   ci-config:
     name: config
--
2.42.0.rc2.windows.1


[-- Attachment #2: Type: image/png, Size: 43278 bytes --]

Re: [PATCH] ci: avoid building from the same commit in parallel

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Tue, 22 Aug 2023, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> > Right, we'd need that `concurrency: ${{ github.sha }}` attribute on
>> > the `config` job.
>>
>> That was my first thought, but I am not sure how it would work.
>>
>> Doesn't skip-if-redundant grab the workflow runs that have succeeded
>> and then see if one for the same commit already exists?  If you used
>> concurrency on the 'config', what gets serialized between two jobs
>> for the same commit is only the 'config' phase, so 'master' may wait
>> starting (because 'config' is what everybody else 'needs' it) while
>> 'config' phase of 'main' runs, and then when it gets to the turn of
>> 'config' phase of 'master', it would not find the run for the same
>> commit being done for 'main' completed yet, would it?
>
> Yes, that's true.
>
> But there is a silver lining: the `concurrency` can not only be specified
> on the job level, but also on the workflow run level.
> I tested this, and present the corresponding patch at the end of this
> mail.

Yeah, serializing the whole thing was the only way I thought that
would work, and I am glad you already tested that it works.

Thanks.

Re: [PATCH] ci: avoid building from the same commit in parallel

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Here is the patch:
>
> -- snipsnap --
> From: Junio C Hamano <gitster@pobox.com>
> Date: Mon, 21 Aug 2023 17:31:26 -0700
> Subject: [PATCH] ci: avoid building from the same commit in parallel

I forgot to say that I do not think I deserve the credit in the end
result, as you've done all the hard part.  

Mind taking the authorship, while demoting me to "Helped-by" status?

Thanks.

Re: [PATCH] ci: avoid building from the same commit in parallel

[Johannes Schindelin]

Hi Junio,

On Wed, 23 Aug 2023, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Here is the patch:
> >
> > -- snipsnap --
> > From: Junio C Hamano <gitster@pobox.com>
> > Date: Mon, 21 Aug 2023 17:31:26 -0700
> > Subject: [PATCH] ci: avoid building from the same commit in parallel
>
> I forgot to say that I do not think I deserve the credit in the end
> result, as you've done all the hard part.
>
> Mind taking the authorship, while demoting me to "Helped-by" status?

Sure. But I disagree that I did most of the work. You tried all the
avenues that I would have tried, saving me tons of time by spending yours.

Thank you,
Johannes

Re: [PATCH v5 0/8] pretty: add %(decorate[:<options>]) format

[Junio C Hamano]

Andy Koppe <andy.koppe@gmail.com> writes:

> Apologies for sending another version so soon, but I realized that I
> hadn't removed the use of a compound literal from the first commit where
> I had added it, so it still appeared in the patches. The overall diff
> for v5 is the same as for v4.

Sorry, but I lost track.  How does this latest round of the topic
look to folks?  Ready to go?

Thanks.

Re: [PATCH v5 0/8] pretty: add %(decorate[:<options>]) format

[Andy Koppe]

On 29/08/2023 22:59, Junio C Hamano wrote:
> Andy Koppe <andy.koppe@gmail.com> writes:
> 
>> Apologies for sending another version so soon, but I realized that I
>> hadn't removed the use of a compound literal from the first commit where
>> I had added it, so it still appeared in the patches. The overall diff
>> for v5 is the same as for v4.
> 
> Sorry, but I lost track.  How does this latest round of the topic
> look to folks?  Ready to go?

Unfortunately there haven't been any comments beyond v2, apart from your 
request not to introduce compound literals, which I've addressed.

I haven't got any changes to this pending.

Regards,
Andy