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

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

From
Glen Choo <chooglen@google.com>
Date
Jul 19, 2023, 18:16 UTC
Message-ID
<kl6l351j22dr.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To
<20230715160730.4046-1-andy.koppe@gmail.com>
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:
Show 14 quoted lines
> 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.

Show 16 quoted lines
> 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.

Show 33 quoted lines
>  /*
>   * 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.

Show 5 quoted lines
> +	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"...
Show 21 quoted lines
> +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.
Show 12 quoted lines
> +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.

Show 14 quoted lines
>  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.

Show 21 quoted lines
> +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!
Previous: Andy KoppeNext: Phillip Wood
Message 6 of 59 in “pretty: add %(decorate[:<options>]) format”
  1. pretty: add %(decorate[:<options>]) formatAndy Koppe, Jul 15, 2023
  2. pretty: add %(decorate[:<options>]) formatAndy Koppe, Jul 15, 2023
  3. Junio C HamanoJul 17, 2023
  4. Junio C HamanoJul 18, 2023
  5. Andy KoppeAug 11, 2023
  6. Glen ChooJul 19, 2023
  7. Phillip WoodJul 23, 2023
  8. Andy KoppeAug 11, 2023
  9. Junio C HamanoAug 11, 2023
  10. Andy KoppeAug 11, 2023
  11. Junio C HamanoAug 12, 2023
  12. Andy KoppeAug 11, 2023
  13. Junio C HamanoAug 15, 2023
  14. Andy KoppeAug 15, 2023
  15. Junio C HamanoAug 15, 2023
  16. main != master at github.com/git/gitAndy Koppe, Aug 15, 2023
  17. Taylor BlauAug 15, 2023
  18. Jeff KingAug 16, 2023
  19. rsbecker@nexbridge.comAug 16, 2023
  20. Junio C HamanoAug 18, 2023
  21. Johannes SchindelinAug 21, 2023
  22. Junio C HamanoAug 21, 2023
  23. ci: avoid building from the same commit in parallelJunio C Hamano, Aug 22, 2023
  24. Junio C HamanoAug 22, 2023
  25. Johannes SchindelinAug 22, 2023
  26. Junio C HamanoAug 22, 2023
  27. Johannes SchindelinAug 23, 2023
  28. Junio C HamanoAug 23, 2023
  29. Junio C HamanoAug 23, 2023
  30. Johannes SchindelinAug 25, 2023
  31. 1/7 pretty-formats: define "literal formatting code"Andy Koppe, Aug 10, 2023
  32. 2/7 pretty-formats: enclose options in angle bracketsAndy Koppe, Aug 10, 2023
  33. 4/7 decorate: avoid some unnecessary color overheadAndy Koppe, Aug 10, 2023
  34. 3/7 decorate: refactor format_decorations()Andy Koppe, Aug 10, 2023
  35. 6/7 pretty: add %(decorate[:<options>]) formatAndy Koppe, Aug 10, 2023
  36. 5/7 decorate: color each token separatelyAndy Koppe, Aug 10, 2023
  37. 7/7 pretty: add pointer and tag options to %(decorate)Andy Koppe, Aug 10, 2023
  38. Junio C HamanoAug 16, 2023
  39. 0/8 pretty: add %(decorate[:<options>]) formatAndy Koppe, Aug 20, 2023
  40. 1/8 pretty-formats: define "literal formatting code"Andy Koppe, Aug 20, 2023
  41. 2/8 pretty-formats: enclose options in angle bracketsAndy Koppe, Aug 20, 2023
  42. 5/8 decorate: color each token separatelyAndy Koppe, Aug 20, 2023
  43. 4/8 decorate: avoid some unnecessary color overheadAndy Koppe, Aug 20, 2023
  44. 3/8 decorate: refactor format_decorations()Andy Koppe, Aug 20, 2023
  45. 6/8 pretty: add %(decorate[:<options>]) formatAndy Koppe, Aug 20, 2023
  46. 7/8 pretty: add pointer and tag options to %(decorate)Andy Koppe, Aug 20, 2023
  47. 8/8 decorate: use commit color for HEAD arrowAndy Koppe, Aug 20, 2023
  48. 0/8 pretty: add %(decorate[:<options>]) formatAndy Koppe, Aug 20, 2023
  49. 1/8 pretty-formats: define "literal formatting code"Andy Koppe, Aug 20, 2023
  50. 2/8 pretty-formats: enclose options in angle bracketsAndy Koppe, Aug 20, 2023
  51. 4/8 decorate: avoid some unnecessary color overheadAndy Koppe, Aug 20, 2023
  52. 5/8 decorate: color each token separatelyAndy Koppe, Aug 20, 2023
  53. 3/8 decorate: refactor format_decorations()Andy Koppe, Aug 20, 2023
  54. 6/8 pretty: add %(decorate[:<options>]) formatAndy Koppe, Aug 20, 2023
  55. 8/8 decorate: use commit color for HEAD arrowAndy Koppe, Aug 20, 2023
  56. 7/8 pretty: add pointer and tag options to %(decorate)Andy Koppe, Aug 20, 2023
  57. Junio C HamanoAug 29, 2023
  58. Andy KoppeSep 1, 2023
  59. Junio C HamanoAug 21, 2023

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

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