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

On Tue, Oct 05 2021, Jeff King wrote:

Urgh.

This is the Nth time I've completely failed to mentally model how this command works, after having hacked on it extensively.

I thought that --batch-all-objects and --batch were mutually exclusive, but they're obviously not.

In my defense I think the help/code is very confusing. I hacked up some
WIP changes to change it from:
    
    $ git cat-file -h
    usage: git cat-file (-t [--allow-unknown-type] | -s [--allow-unknown-type] | -e | -p | <type> | --textconv | --filters) [--path=<path>] <object>
       or: git cat-file (--batch[=<format>] | --batch-check[=<format>]) [--follow-symlinks] [--textconv | --filters]
    
    <type> can be one of: blob, tree, commit, tag
        -t                    show object type
        -s                    show object size
        -e                    exit with zero when there's no error
        -p                    pretty-print object's content
        --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
        --allow-unknown-type  allow -s and -t to work with broken/corrupt objects
        --buffer              buffer --batch output
        --batch[=<format>]    show info and content of objects fed from the standard input
        --batch-check[=<format>]
                              show info about objects fed from the standard input
        --follow-symlinks     follow in-tree symlinks (used with --batch or --batch-check)
        --batch-all-objects   show all objects with --batch or --batch-check
        --unordered           do not order --batch-all-objects output
    
To:
    
    usage: git cat-file (-e | -p) <object>
       or: git cat-file ( -t | -s ) [--allow-unknown-type] <object>
       or: git cat-file [--batch-all-objects] [--batch | --batch-check] [--buffer] [--follow-symlinks] [--unordered]
       or: git cat-file [--textconv|--filters] [--path=<path|tree-ish> <rev> | <rev>:<path|tree-ish>]
    
    Check object existence or emit object contents
        -e                    check if <object> exists
        -p                    pretty-print <object> content
    
    Emit [broken] object attributes
        -t                    show object type (one of 'blob', 'tree', 'commit', 'tag', ...)
        -s                    show object size
        --allow-unknown-type  allow -s and -t to work with broken/corrupt objects
    
    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>]
                              show info about objects fed from the standard input
        --follow-symlinks     follow in-tree symlinks
        --unordered           do not order objects before emitting them

Which actually reflects reality, i.e. half of the options aren't accepted by the batch mode, so grouping them makes sense, and the current help gives the impression that e.g. --textconv can be used with --batch, but it's effectively a CMDMODE.

Then we don't document that [--textconv|--filters] will fallback to -p if given <tree-ish>, I'm not sure if that's intentional (but a "fallthrough" comment suggests so).

And we silentnly accept --buffer etc. without the batch mode, but it should die.

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.

1. b48158ac94c (cat-file: make the options mutually exclusive, 2015-05-03)
2. https://lore.kernel.org/git/xmqqtwspgusf.fsf@gitster.dls.corp.google.com/
3. https://lore.kernel.org/git/20150622104559.GG14475@peff.net/
4. 6a951937ae1 (cat-file: add --batch-all-objects option, 2015-06-22)
5. 321459439e1 (cat-file: support --textconv/--filters in batch mode, 2016-09-09)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/cat-file.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)