Re: [PATCH v2 01/10] ref-filter: add %(refname:shortalign=X) option

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

Karthik Nayak <karthik.188@gmail.com> writes:

> Add support for %(refname:shortalign=X) where X is a number.
> This will print a shortened refname aligned to the left
> followed by spaces for a total length of X characters.
> If X is less than the shortened refname size, the entire
> shortened refname is printed.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  ref-filter.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)

This may be enough to support the various existing formats that are
offered by "git branch" and/or "git tag", but I do not think if this
is the right approach in the longer term, or if we are painting
ourselves in a corner we cannot cleanly get out of later [*1*].
Will the "refname" stay to be the only thing that may want alignment
padding appended in the future?  Will it stay true that we want to
align only to the left?  Etc., etc.

Cc'ed Duy as %< in the pretty-format was his invention at around
a5752342 (pretty: support padding placeholders, %< %> and %><,
2013-04-19).

> diff --git a/ref-filter.c b/ref-filter.c
> index dd0709d..3098497 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -10,6 +10,7 @@
>  #include "quote.h"
>  #include "ref-filter.h"
>  #include "revision.h"
> +#include "utf8.h"
>  
>  typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>  
> @@ -695,7 +696,23 @@ static void populate_value(struct ref_array_item *ref)
>  			int num_ours, num_theirs;
>  
>  			formatp++;
> -			if (!strcmp(formatp, "short"))
> +			if (starts_with(formatp, "shortalign=")) {

When adding a new thing to an existing list, we prefer to append it
at the end of the list, if there is no other reason not to do so
(e.g. "the existing list is sorted in this order, and the new
location was chosen to fit the new item to honor the existing
ordering rule" is a valid reason to put it at the beginning, if the
existing sorting rule dictates that the new thing must come at the
beginning).

> +				const char *valp, *short_refname = NULL;
> +				int val, len;
> +
> +				skip_prefix(formatp, "shortalign=", &valp);
> +				val = atoi(valp);

In newer code, we would want to avoid atoi() so that "foo:shortalign=1z"
that is a typo of "12" can be caught as an error.  Either strtol_i()
or strtoul_ui() may be better (we would need to adjust it further
when Michael decides to resurrect his numparse thing that has been
in the stalled bin for quite a while, though).

> +				refname = short_refname = shorten_unambiguous_ref(refname,
> +										  warn_ambiguous_refs);
> +				len = utf8_strwidth(refname);
> +				if (val > len) {
> +					struct strbuf buf = STRBUF_INIT;
> +					strbuf_addstr(&buf, refname);
> +					strbuf_addchars(&buf, ' ', val - len);
> +					free((char *)short_refname);
> +					refname = strbuf_detach(&buf, NULL);
> +				}

What should happen when the display column width of the string is
wider?  If a user wants to align the refs that are usually usually
short start the next thing at the 8th column, which should she use?

    "%(refname:shorta=7) %(next item)"
    "%(refname:shorta=8)%(next item)"

> +			} else if (!strcmp(formatp, "short"))
>  				refname = shorten_unambiguous_ref(refname,
>  						      warn_ambiguous_refs);
>  			else if (!strcmp(formatp, "track") &&