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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Jul 11, 2015 at 7:05 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
> On Fri, Jul 10, 2015 at 9:50 PM, Junio C Hamano <gitster@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]