Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Jul 20, 2015, 17:04 UTC
- Message-ID
- <CAOLa=ZT7szz=cwzEmVDPKucRbh2o_8mBhUY22=R8qCU34QPwGg@mail.gmail.com>
- In-Reply-To
- <xmqq1tg2zsxr.fsf@gitster.dls.corp.google.com>
Show 6 quoted lines
> > What is your plan for this function? Is it envisioned that this > will gain more variations of formatting options over time? > Otherwise it seems misnamed (it is not "assign formatting" but > merely "pad to the right"). >
I wanna include an 'ifexists' atom in the future where an user can specify a string format to be printed only if the preceding atom holds a value. e.g. '%(ifexists:refname is %s)%(refname)' So I see that going in here.
> I also doubt that the logic is sane. More specifically, what does > that "if (v->s[0])" buy you?
Yeah, that's wrong, like Eric pointed out. Should have been "if (v->s[0] == '\0')"
Show 9 quoted lines
> > Your caller is iterating over the elements in a format string, > e.g. 'A %(align:20)%(foo) B %(bar) C', and its caller is iterating > over a list of refs, e.g. 'maint', 'master' branches. With that > format string, as long as %(foo) does not expand to something that > exceeds 20 display places or so, I'd expect literal 'B' for all refs > to align, but I do not think this code gives me that; what happens > if '%(foo)' happens to be an empty string for 'maint' but is a > string, say 'x', for 'master'?
Oh yeah! That doesn't work, I have a fix in mind I'll reply to this mail with a fix.
-- Regards, Karthik Nayak