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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jul 12, 2015, 19:56 UTC
Message-ID: <CAOLa=ZThDcmM79hL0TUh6+8=NkQBWOWK=Ti+-v+2PCysquw-1w@mail.gmail.com>
In-Reply-To: <CACsJy8BL0ccxNYLE0gqBx5KTpqTOt=Ut6-k3fauxXNt2wpxSEw@mail.gmail.com>

On Sun, Jul 12, 2015 at 7:17 AM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 8 quoted lines

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

I was thinking of something on the lines of having a function which right before printing checks if any "align" option is given to the end of a given item and aligns it accordingly, this ensures that any item which needs to have such an option can easily do so.

https://github.com/KarthikNayak/git/commit/0284320483d6442a6425fc665e740f9f975654a1

This is what I came up with, you could have a look and let me know if you have any suggestions.

Show 8 quoted lines

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

This was cleared out in your other reply.

Show 5 quoted lines

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

Yes, sure, maybe a cleanup patch at the end to do all of cleanup any such use cases :)

Thanks for all the suggestions.

-- 
Regards,
Karthik Nayak