From: Karthik Nayak Date: Mon, 27 Jul 2015 18:42:24 GMT Subject: Re: [PATCH v5 03/11] ref-filter: add option to pad atoms to the right Message-ID: In-Reply-To: On Mon, Jul 27, 2015 at 9:24 PM, Matthieu Moy wrote: > Junio C Hamano writes: > >> Matthieu Moy writes: >> >>> See my remark on previous patch: this test is not sufficient. You do >>> not only need to check that %(padright) is taken into account, but you >>> also need to check that it is taken into account for the right atom and >>> only this one. I'd suggest >>> >>> --format '%(refname)%(padright:25)|%(refname)|%(refname)|' >>> >>> Only the middle column should be padded. >> >> This actually raises an interesting point. It is equally valid to >> view that format string as requesting to pad the first "|" with 24 >> spaces; in other words, %(padright:N) would apply to the next span, >> be it a literal string up to the next %(atom), or the %(atom) that >> comes immediately after it. > > For an arbitrary %(modifier), I'd agree, but in the particular case of > %(padright), I think it only makes sense if applied to something of > variable length > I agree with Matthieu here, Other than atom values, any user defined string would be of known size, hence padding for such things would make no sense. >> The thing is, the above _might_ be an OK way to ask the middle >> refname to be padded, but I somehow find it very unnatural and >> unintuitive to expect that this: >> >> --format '%(padright:25)A Very Long Irrelevancy%(refname)' > > Yes, but on the other hand we already have: > > git log --format='%<|(50)A very long irrevlevancy|%an|' > > that pads/truncate %an. So, consistancy would dictate that Karthik's > version is the right one. Sorry but I didn't understand what you're trying to say here, Matthieu. > >> My preference between the three is "%(padright:4)", etc. to apply to >> the "next span", but I can live with "it is an error to pad-right >> a far-away %(atom)". > > I think there are valid argument for the 3 versions. I'm fine with any > of them, as long as there's a test that shows which one is picked. > Makes sense, thanks both of you :) -- Regards, Karthik Nayak