Re: [PATCH v5 03/11] ref-filter: add option to pad atoms to the right

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jul 27, 2015, 15:54 UTC
Message-ID: <vpqk2tl4mvx.fsf@anie.imag.fr>
In-Reply-To: <xmqqr3ntioyh.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 16 quoted lines

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> 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.

Show 5 quoted lines

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

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

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/