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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

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.

Not really an issue, but you're wrapping your text at ~60 characters.
The common use is to wrap around 70 to 80. Using Emacs, auto-fill-mode
or M-q does this automatically. If you use another text editor, it can
probably do that for you too.

> 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 ++++++++++++++++++-

I think this would deserve a test and documentation. Even though your
motivation is for an internal implementation, some users may want to use
the feature in 'git for-each-ref --format=...'.

>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> 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=")) {
> +				const char *valp, *short_refname = NULL;
> +				int val, len;
> +
> +				skip_prefix(formatp, "shortalign=", &valp);
> +				val = atoi(valp);

You're silently accepting %(refname:shortalign=foo) and
%(refname:shortalign=). I think it would be better to reject such cases
explicitly.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/