Re: [PATCH 2/5] cat-file: mention --unordered along with --batch-all-objects

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 2/5] cat-file: mention --unordered along with --batch-all-objects
Date: Fri, 08 Oct 2021 09:54:45 +0200	[thread overview]
Message-ID: <87lf34ey5a.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <YV+tXVWVuy/rZn/l@coredump.intra.peff.net>

On Thu, Oct 07 2021, Jeff King wrote:

> On Thu, Oct 07, 2021 at 12:18:45PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> >> The usage of OPT_CMDMODE() in "cat-file"[1] was added in parallel with
>> >> the development of[3] the --batch-all-objects option[4], so we've
>> >> since grown[5] checks that it can't be combined with other command
>> >> modes, when it should just be made a top-level command-mode
>> >> instead. It doesn't combine with --filters, --textconv etc.
>> >
>> > This is not right. --batch-all-objects does not provide a mode exclusive
>> > with "-t", etc, by itself.
>> 
>> Yes it does. See the "if (opt) {" branch on master. We just don't
>> implement it via a cmdmode, but --batch-all-objects can definitely be a
>> CMDMODE (I see you found that out below...)
>
> I agree that if you make it a CMDMODE it does not introduce any bugs.
> But it is semantically confusing. You would not make, say, --buffer a
> CMDMODE option. It is a flag which only takes effect under certain
> modes. And the same is true of --batch-all-objects, which modifies the
> batch cmd modes.
>
> In fact, it _would_ be a bug to make it a CMDMODE if --batch were
> correctly marked as one (but it is not sufficient to reason the other
> way; --batch without --batch-all-objects is still mutually exclusive
> with -t, etc).
>
 > What really makes things confusing, IMHO, is the --textconv and --filter
> options. They are marked as CMDMODEs, and they are indeed mutually
> exclusive with -t, etc. But they also work with --batch, which is itself
> a different mode.
>
> So I don't think OPT_CMDMODE could ever present this complete set of
> rules, because they are not all mutually exclusive with each other. But
> I think calling "--batch-all-objects" a mode is just muddying the waters
> even further.

I think we've got some different understanding of what a CMDMODE
means. --batch-all-objects should be a cmdmode, but --batch, --buffer
etc. can't be. Similarly it's not a bug that --filters and --textconv
are cmdmodes, but you think that's bad.

I think it's fair to say that you think this because "the batch mode" is
a ... mode, that cat-file operates in, therefore it's weird that we
don't call it a "command", but that when you're doing "cat-file --batch
--textconv[...]" you're in "textconv mode" or whatever.

I agree that it's a bit weird. But OPT_CMDMODE() under the hood is just
a way to label N options as being mutually exclusive with each other,
and not needing to follow-up parse_options() with a bunch of "die() if x
&& y" etc.

So both in terms of code clarity and to enable later clever use of
parse_options() I think just squinting a bit and reading it as
s/OPT_CMDMODE/OPT_MUTUALLY_EXCLUSIVE_WITH_OTHER_SUCH/ makes the most
sense.

E.g. for any command-mode we should be able to teach the completion that:

    git cat-file --batch-all-objects --<tab>

Should only complete those options that go with it. We don't have that
full picture yet (since we just have cmdmode, but no way to say A goes
with B and C, but not D), but a rough working start at that is to
exclude all the other cmdmode options.