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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Oct 06 2021, Jeff King wrote:

> On Wed, Oct 06, 2021 at 11:02:59AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> I thought that --batch-all-objects and --batch were mutually exclusive,
>> but they're obviously not.
>
> Right. In fact, the former is useless without the latter (or
> --batch-check).
>
>> In my defense I think the help/code is very confusing. I hacked up some
>> WIP changes to change it from:
>
> That's fair, but...
>
>> Looking at the history it seems you added --batch-all-objects around the
>> same time as the OPT_CMDMODE() was used in the command, so we should
>> probably have something like this to start with:
>> 
>> -- >8 --
>> Subject: [PATCH] cat-file: make --batch-all-objects a CMDMODE
>> 
>> 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...)

> Those in theory should be OPT_CMDMODE, but I don't think they can be,
> because they also take an argument. So we'd need some OPT_CMDMODE_ARG()
> or something, but then it needs _two_ value fields. So I think it would
> require major surgery to parse-options.

Aside: I think it would be worth it to teach it a general concept that
option X is incompatible with option Y, or group X, Y, Z and declare
those as incompatible with another group.

The current CMDMODE check is rather cryptic seemingly because it's
trying to avoid re-looping over the list while it emits an error.

> Using --batch-all-objects without --batch or --batch-check would be an
> error, and we do flag it as such.

*nod*

> So you are not wrong that using --batch-all-objects with -t is nonsense,
> and we do indeed error on it currently. But it is not because the two
> are themselves exclusive, but because of the chaining of the two rules.

Isn't it nonsense? I think so. I suppose we could make it a a synonym of
some --batch=<fmt> in that context, but that just seems like complexity
for no good reason.

> The groupings you showed in your larger output mostly make sense, but...
>
>>     Run <rev>:<blobs|tree> via conversion or filter
>>         --textconv            for blob objects, run textconv on object's content
>>         --filters             for blob objects, run filters on object's content
>>         --path <blob>         use a specific path for --textconv/--filters
>>     
>>     Emit objects in batch via requests on STDIN, or --batch-all-objects
>>         --batch-all-objects   Emit all objects in the repository, instead of taking requests on STDIN
>>         --buffer              buffer --batch output
>>         --batch[=<format>]    show info and content of objects fed from the standard input
>>         --batch-check[=<format>]
>> [...]
>
> These groups aren't mutually exclusive. You can use --textconv in batch
> mode. Which further muddies the CMDMODE waters; --batch is a mode that
> overrides "-t", but _not_ "--textconv", where it is a modifier.

Indeed, I conflated --batch* there again, those two are mutually
exclusive with --batch-all-objects, but not the other two. Will update
if I get to submitting this...