Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jul 29, 2015, 17:48 UTC
Message-ID: <CAOLa=ZTUJHnXWJyr0wp0CZgR9S1pBxCEsGRiDPsJdUyzTZaZUw@mail.gmail.com>
In-Reply-To: <xmqq8ua0f9lk.fsf@gitster.dls.corp.google.com>

On Tue, Jul 28, 2015 at 11:27 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> The 'ifexists' atom allows us to print a required format if the
>> preceeding atom has a value. If the preceeding atom has no value then
>> the format given is not printed. e.g. to print "[<refname>]" we can
>> now use the format "%(ifexists:[%s])%(refname)".
>
> A handful of "huh?" on the design.
>
>  - The atom says "if *exists*" and explanation says "has a value".
>    How are they related?  Does an atom whose value is an empty
>    string has a value?  Or is "ifexists" meant to be used only to
>    ignore meaningless atom, e.g. %(*objectname) applied to a ref that
>    refers to an object that is not an annotated tag?

It's meant to ignore meaningless atom. atom's whose values are empty strings are ignored.

>
>  - That %s looks ugly.  Are there cases where a user may want to say
>    %(ifexists:[%i]) or something other than 's' after that per-cent?
>

Couldn't think of a better replacer, any suggestions would be welcome :)

>    . Is it allowed to have more than one %s there?
>    . Is it allowed to have no %s there?
>

1. yes its allowed to have multiple %s
2. yes no %s is also allowed.

>  - The syntax makes the reader wonder if [] is part of the
>    construct, or just an example of any arbitrary string, i.e. is
>    "%(ifexists:the %s can be part of arbitrary string)" valid?
>

Its given as example, is that misleading?

>  - If an arbitrary string is allowed, is there any quoting mechanism
>    to allow ")" to be part of that arbitrary string?

Nope. Haven't done anything for that. I'll look into that :)

Show 6 quoted lines

>
>  - What, if anything, is allowed to come between %(ifexists...) and
>    the next atom like %(refname)?  For example, are these valid
>    constructs?
>
>     . %(ifexists...)%(padright:20)%(refname)

Doesn't work with padright, maybe we could eventually support that.

>     . %(ifexists...) %(refname) [%(subject)]
>

Not sure what this is.

>  - This syntax does not seem to allow switching on an attribute to
>    show or not to show another, e.g. "if %(*objectname) makes sense,
>    then show '%(padright:20)%(refname:short) %(*subject)' for it".

Yes this doesn't do that, I can say this is a pretty basic version, we could probably work on and implement more things? This is currently to support 'git branch -vv' where we have the upstream in square brackets.

-- 
Regards,
Karthik Nayak