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

[Junio C Hamano <gitster@pobox.com>]

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