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

From: Duy Nguyen <pclouds@gmail.com>
Date: Jul 12, 2015, 01:47 UTC
Message-ID: <CACsJy8BsLnkPFX04WujMdnBiRGmXacF1QSQudb3T8nxGrZ_1Fg@mail.gmail.com>
In-Reply-To: <CAOLa=ZTNGV=NAXDPB3HX=fanPMQaQQc4ZpFdDFeg+LJ8SFuVOQ@mail.gmail.com>

On Sat, Jul 11, 2015 at 7:05 PM, Karthik Nayak <karthik.188@gmail.com> wrote:

Show 19 quoted lines

> On Fri, Jul 10, 2015 at 9:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> 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).
>>
>
> I kinda had the same though, my only justification was that it was only being
> internally used. I'll have another look if as to see if I can make it
> universal somehow.
> Let's see what Duy has to suggest.

I guess if you can have multiple arguments after ':' in an atom, then you have wiggle room for future. But it looks like you only accept one argument after ':'.. (I only checked the version on 'pu'). Having an "alignment atom" to augment the real one (like %< changes the behavior of the next placeholder), could also work, but it adds dependency between atoms, something I don't think ref-filter.c is ready for.

Another thing, the atom value is also used for sorting. When used for sorting, I think these padding spaces should not be generated or it may confuse the sort algorithm. Left alignment may be ok, right or center alignment (in future?), not so much. Perhaps we should do the padding in a separate phase, outside populate_value(). If you go this route, having separate atoms for alignment works better: you don't have to parse them in populate_value() which is for actual values, and you can handle dependency easily (I think).

By the way, please consider adding _() back to translatable strings, usually those die() or warn(), or "[ahead %s]"... In the last case, because you don't really know how long the string is after translation, avoid hard coding buffer size (to 40).

-- 
Duy