Re: [PATCH v4 15/16] branch: use ref-filter printing APIs

From: Jeff King <peff@peff.net>
Date: Apr 15, 2016, 21:09 UTC
Message-ID: <20160415210947.GA21678@sigill.intra.peff.net>
In-Reply-To: <CAOLa=ZSdnDFR52BJB1dm-D=gyUo=oB0aoh5pKAvwyNG5+u0P_A@mail.gmail.com>

On Sat, Apr 16, 2016 at 01:57:23AM +0530, Karthik Nayak wrote:

> I had a look at your patch and even tested it, seems solid, I like how you
> integrated all these atoms together under refname_atom_parser_internal().
> I'm squashing this in, for my re-roll. Thanks.

Great, thanks for picking it up.

Show 12 quoted lines

> > So actually, we _do_ accept "%(upstream:short,track)" with your
> > patch, which is somewhat nonsensical. It just ignores "short" and
> > takes whatever option came last. Which is reasonable, though
> > flagging an error would also be reasonable (and I think is what
> > existing git does). I don't think it matters much either way.
> >
> 
> I think it was decided a while ago that it'd be best to ignore all and
> accept the last argument without barfing, I couldn't find the exact
> link. But I'm open to both.  But if you look at the %(align) atom,
> even that accepts mutually exclusive arguments and accepts the last
> argument without reporting an error.

Makes sense, and I'm fine with how you have it (and my patch tried to retain that property). I just wasn't sure if it was intentional, as I did a bad job of paying attention to earlier rounds of the series. Thank you for keeping at it.

-Peff