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

[Karthik Nayak <karthik.188@gmail.com>]

On Thu, Jul 9, 2015 at 6:28 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> 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.
>

Thanks, I was just manually clipping it.

>> 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=...'.
>

I didn't want to include documentation as this is mostly for internal use,
but will add with tests.

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

Oh yeah! will do.

-- 
Regards,
Karthik Nayak