Re: [PATCH v14 04/13] ref-filter: implement an `align` atom

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 1, 2015, 13:05 UTC
Message-ID: <CAOLa=ZQV63ZETPswEPYjSMrXcZAK+W-Jek+Psq7wtmSsQHtb3g@mail.gmail.com>
In-Reply-To: <CAPig+cRig+zk=D1SDF7wBHuQgcQqb-4cHkmeVgLs5BaWzk9WRg@mail.gmail.com>

On Mon, Aug 31, 2015 at 11:32 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 28 quoted lines

> On Mon, Aug 31, 2015 at 1:28 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>>
>>> Warning about unrecognized atoms may indeed be a good idea, however,
>>> the current behavior isn't a huge problem since user discovers the
>>> error when the output fails to match his expectation.
>>
>> It's a bit worse than it seems: without this change, using --format
>> '%(align)%(end)' results in Git complaining about %(end) without
>> matching atom, which is really confusing since you do have a %(align) (I
>> got it for real while testing a preliminary version).
>>
>>> This behavior of ignoring unrecognized atoms predates your work,
>>> doesn't it? If so, it's probably not something you need to address in
>>> this series.
>>
>> I wouldn't insist in having it in the series, but now that it's here, I
>> think we can keep it (if only to shorten the interdiff for the next
>> iteration).
>
> The unstated subtext in my original question is that the approach
> taken by this patch of warning only about unrecognized %(align) is too
> special-case; if it's going to warn at all, it should do so
> generically for any unrecognized %(atom). Special-casing the warning
> about malformed %(align) sets a poor precedent; it's code which will
> eventually need to be removed anyhow when the generic warning
> mechanism is eventually implemented.

Probably, I could just have a check within the align block and maybe build a general case later.

Like this:

 else if (skip_prefix(name, "align", &valp)) {
            struct align *align = &v->u.align;
            struct strbuf **s;

            if (valp[0] != ':')
                die(_("format: usage %%(align:<width>,<position>)"));
            else
                valp++;
            ......
            ........
}

-- 
Regards,
Karthik Nayak