On Sun, Jul 12, 2015 at 7:17 AM, Duy Nguyen <pclouds@xxxxxxxxx> wrote: > > 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. > 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. > > 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 -- 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