Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

From: Jeff King <peff@peff.net>
Date: Jan 26, 2017, 14:24 UTC
Message-ID: <20170126142423.6rrorahyzfo6z43q@sigill.intra.peff.net>
In-Reply-To: <CACsJy8ATM_kc5SPY0dqprUefRy3vtpKW-4QEyJFK54jw0QgeJA@mail.gmail.com>

On Thu, Jan 26, 2017 at 04:28:17PM +0700, Duy Nguyen wrote:

Show 29 quoted lines

> On Thu, Jan 26, 2017 at 3:57 AM, Jeff King <peff@peff.net> wrote:
> > I don't think it means either. It means to include remotes in the
> > selected revisions, but excluding the entries mentioned by --exclude.
> >
> > IOW:
> >
> >   --exclude=foo --remotes
> >         include all remotes except refs/remotes/foo
> >
> >   --exclude=foo --unrelated --remotes
> >         same
> >
> >   --exclude=foo --decorate-reflog --remotes
> >         decorate reflogs of all remotes except "foo". Do _not_ use them
> >         as traversal tips.
> >
> >   --decorate-reflog --exclude=foo --remotes
> >         same
> >
> > IOW, the ref-selector options build up until a group option is given,
> > which acts on the built-up options (over that group) and then resets the
> > built-up options. Doing "--unrelated" as above is orthogonal (though I
> > think in practice nobody would do that, because it's hard to read).
> 
> This is because it makes sense to combine --exclude and
> --decorate-reflog. But what about a new --something that conflicts
> with either --exclude or --decorate-reflog? Should we simply catch
> such combinations and error out (which may be a bit more complicated
> than this patch, or maybe not)?

I'd cross that bridge when we see what the option is. But my gut is that rules would be:

  - apply all non-conflicting relevant options. So:

      --exclude=foo/* --decorate-refs --decorate-reflog --remotes

    would presumably decorate both ref tips _and_ reflogs for all
    remotes (except ones in refs/remotes/foo/*)

  - for ones that are directly related and override each other,
    use the usual last-one-wins rule. So:

      --decorate-reflog --no-decorate-reflog --remotes

    would countermand the original --decorate-reflog.

  - for ones that really have complex interactions, notice and complain
    in handle_refs().

That just seems to me like it follows our usual option parsing procedure. The only difference here is that process and reset some subset of the flags when we hit a special marker option ("--remotes" in these examples) instead of doing it at the end.

-Peff