From: Junio C Hamano <hidden> Date: 2021-10-08 22:04:33
Jeff King [off-list ref] writes:
Yes, it would definitely need that extension. But it's also weirder than
that. --textconv is an OPT_CMDMODE(), because it is mutually exclusive
with "-t", etc.
Yeah, in hindsight, we should have made "--textconv" a modifier for
"-p", because it is not a true cmdmode. It is much easier to
understand if you imagine "--textconv", without a command mode,
implies the "-p" mode, but when a command mode like "--batch" is
given, that would apply. And it is job of other individual command
modes to notice that "--textconv" modifier does not make sense in
their context and issue a warning.
The current code uses OPT_CMDMODE() for (1) and (2), and then manually
enforces the exclusion between (1) and (3). But IMHO it is (2) that is
the odd-man out, in that it can be its own mode or a modifier, and it
probably should not be OPT_CMDMODE() (but from the end-user perspective,
that is OK, though it may influence how we document or group things).
I guess we are exactly on the same page (see "'textconv' is a
modifier, which implies '-p' command mode unless otherwise
specified" above).
This series of patches to cat-file significantly improves the UX of
the -h output, see 08/10.
This is something I hacked up a month or so ago but didn't send after
the discussion thread about whether --batch-all-objects should be a
cmdmode[1].
This series marks it as such, as the square peg of wanting to have
mutually exclusive options best fits into the "cmdmode" round hole in
parse-options.c :)
I'm submitting this now because John Cai has a proposed change to
cat-file[2] which I proposed an alternate direction to on top of this
series.
I think as should become clear when reading this series a careful look
at any change to cat-file's interface would come up against the
missing assertions of what options are compatible with what other
options etc., which is hopefully all fixed and tested for in this
series.
1. https://lore.kernel.org/git/87tuhuikhf.fsf@evledraar.gmail.com/
2. https://lore.kernel.org/git/pull.1124.git.git.1636149400.gitgitgadget@gmail.com/#t
Ævar Arnfjörð Bjarmason (10):
cat-file tests: test bad usage
cat-file tests: test messaging on bad objects/paths
parse-options API: add a usage_msg_optf()
cat-file docs: fix SYNOPSIS and "-h" output
cat-file: move "usage" variable to cmd_cat_file()
cat-file: make --batch-all-objects a CMDMODE
cat-file: fix remaining usage bugs
cat-file: correct and improve usage information
object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY
cat-file: improve --(textconv|filters) disambiguation
Documentation/git-cat-file.txt | 10 +-
builtin/cat-file.c | 181 ++++++++++++++++++++-------------
builtin/stash.c | 4 +-
cache.h | 1 +
object-name.c | 11 +-
parse-options.c | 13 +++
parse-options.h | 10 ++
t/t1006-cat-file.sh | 88 ++++++++++++++++
t/t8007-cat-file-textconv.sh | 26 +++++
9 files changed, 263 insertions(+), 81 deletions(-)
--
2.34.0.rc1.741.gab7bfd97031
Stress test the usage emitted when options are combined in ways that
isn't supported. Let's test various option combinations, some of these
we buggily allow right now.
E.g. this reveals a bug in 321459439e1 (cat-file: support
--textconv/--filters in batch mode, 2016-09-09) that we'll fix in a
subsequent commit. We're supposed to be emitting a relevant message
when --batch-all-objects is combined with --textconv or --filters, but
we don't.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t1006-cat-file.sh | 90 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 90 insertions(+)
Add tests for the output that's emitted when we disambiguate
<obj>:<path> in cat-file. This gives us a baseline for improving these
messages.
For e.g. "git blame" we'll emit:
$ git blame HEAD:foo
fatal: no such path 'HEAD:foo' in HEAD
But cat-file doesn't disambiguate these two cases, and just gives the
rather unhelpful:
$ git cat-file --textconv HEAD:foo
fatal: Not a valid object name HEAD:foo
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t8007-cat-file-textconv.sh | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
Add a usage_msg_optf() as a shorthand for the sort of
usage_msg_opt(xstrfmt(...)) used in builtin/stash.c. I'll make more
use of this function in builtin/cat-file.c shortly.
The disconnect between the "..." and "fmt" is a bit unusual, but it
works just fine and this keeps it consistent with usage_msg_opt(),
i.e. a caller of it can be moved to usage_msg_optf() and not have to
have its arguments re-arranged.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/stash.c | 4 ++--
parse-options.c | 13 +++++++++++++
parse-options.h | 10 ++++++++++
3 files changed, 25 insertions(+), 2 deletions(-)
There were various inaccuracies in the previous SYNOPSIS output,
e.g. "--path" is not something that can optionally go with any options
except --textconv or --filters, as the output implied.
The opening line of the DESCRIPTION section is also "In its first
form[...]", which refers to "git cat-file <type> <object>", but the
SYNOPSIS section wasn't showing that as the first form!
That part of the documentation made sense in
d83a42f34a6 (Documentation: minor grammatical fixes in
git-cat-file.txt, 2009-03-22) when it was introduced, but since then
various options that were added have made that intro make no sense in
the context it was in. Now the two will match again.
The usage output here is not properly aligned on "master" currently,
but will be with my in-flight 4631cfc20bd (parse-options: properly
align continued usage output, 2021-09-21), so let's indent things
correctly in the C code in anticipation of that.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Documentation/git-cat-file.txt | 10 ++++++++--
builtin/cat-file.c | 10 ++++++++--
2 files changed, 16 insertions(+), 4 deletions(-)
There's no benefit to defining this at a distance, and it makes the
code harder to read as you've got to scroll up to see the usage that
corresponds to the options.
In subsequent commits I'll make use of usage_msg_opt(), which will be
quite noisy if I have to use the long "cat_file_usage" variable,
there's no other command being defined in this file, so let's rename
it to just "usage".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 37 ++++++++++++++++++-------------------
1 file changed, 18 insertions(+), 19 deletions(-)
@@ -708,35 +707,35 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)elseif(argc==1)obj_name=argv[0];else-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(!opt&&!batch.enabled){if(argc==2){exp_type=argv[0];obj_name=argv[1];}else-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(batch.enabled){if(batch.cmdmode!=opt||argc)-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);if(batch.cmdmode&&batch.all_objects)die("--batch-all-objects cannot be combined with ""--textconv nor with --filters");}if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(force_path&&opt!='c'&&opt!='w'){error("--path=<path> needs --textconv or --filters");-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(force_path&&batch.enabled){error("--path=<path> incompatible with --batch");-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(batch.buffer_output<0)
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.
By giving parse_options() information about what options are mutually
exclusive with one another we can get the die9) message being removed
here for free, we didn't even use that removed message in some cases,
e.g. for both of:
--batch-all-objects --textconv
--batch-all-objects --filters
We'd take the "goto usage" in the "if (opt)" branch, and never reach
the previous message. Now we'll emit e.g.:
$ git cat-file --batch-all-objects --filters
error: option `filters' is incompatible with --batch-all-objects
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 <redacted>
---
builtin/cat-file.c | 25 +++++++++++--------------
t/t1006-cat-file.sh | 7 ++-----
2 files changed, 13 insertions(+), 19 deletions(-)
@@ -674,6 +674,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)N_("for blob objects, run textconv on object's content"),'c'),OPT_CMDMODE(0,"filters",&opt,N_("for blob objects, run filters on object's content"),'w'),+OPT_CMDMODE(0,"batch-all-objects",&opt,+N_("show all objects with --batch or --batch-check"),'b'),OPT_STRING(0,"path",&force_path,N_("blob"),N_("use a specific path for --textconv/--filters")),OPT_BOOL(0,"allow-unknown-type",&unknown_type,
@@ -689,8 +691,6 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)batch_option_callback),OPT_BOOL(0,"follow-symlinks",&batch.follow_symlinks,N_("follow in-tree symlinks (used with --batch or --batch-check)")),-OPT_BOOL(0,"batch-all-objects",&batch.all_objects,-N_("show all objects with --batch or --batch-check")),OPT_BOOL(0,"unordered",&batch.unordered,N_("do not order --batch-all-objects output")),OPT_END()
@@ -699,30 +699,27 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)git_config(git_cat_file_config,NULL);batch.buffer_output=-1;-argc=parse_options(argc,argv,prefix,options,usage,0);-if(opt){+argc=parse_options(argc,argv,prefix,options,usage,0);+if(argc&&batch.enabled)+usage_with_options(usage,options);+if(opt=='b'){+batch.all_objects=1;+}elseif(opt){if(batch.enabled&&(opt=='c'||opt=='w'))batch.cmdmode=opt;elseif(argc==1)obj_name=argv[0];elseusage_with_options(usage,options);-}-if(!opt&&!batch.enabled){+}elseif(!opt&&!batch.enabled){if(argc==2){exp_type=argv[0];obj_name=argv[1];}elseusage_with_options(usage,options);-}-if(batch.enabled){-if(batch.cmdmode!=opt||argc)-usage_with_options(usage,options);-if(batch.cmdmode&&batch.all_objects)-die("--batch-all-objects cannot be combined with "-"--textconv nor with --filters");-}+}elseif(batch.enabled&&batch.cmdmode!=opt)+usage_with_options(usage,options);if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){usage_with_options(usage,options);
With the migration of --batch-all-objects to OPT_CMDMODE() in the
preceding commit one bug with combining it and other OPT_CMDMODE()
options was solved, but we were still left with e.g. --buffer silently
being discarded when not in batch mode.
Fix all those bugs, and in addition emit errors telling the user
specifically what options can't be combined with what other options,
before this we'd usually just emit the cryptic usage text and leave
the users to work it out by themselves.
This change is rather large, because to do so we need to untangle the
options processing so that we can not only error out, but emit
sensible errors, and e.g. emit errors about options before errors
about stray argc elements (as they might become valid if the option
were removed).
Some of the output changes ("error:" to "fatal:" with
usage_msg_opt[f]()), but none of the exit codes change, except in
those cases where we silently accepted bad option combinations before,
now we'll error out.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 95 ++++++++++++++++++++++++++++++---------------
t/t1006-cat-file.sh | 41 +++++++++----------
2 files changed, 84 insertions(+), 52 deletions(-)
@@ -648,6 +648,8 @@ static int batch_option_callback(const struct option *opt,intcmd_cat_file(intargc,constchar**argv,constchar*prefix){intopt=0;+intopt_cw=0;+intopt_epts=0;constchar*exp_type=NULL,*obj_name=NULL;structbatch_optionsbatch={0};intunknown_type=0;
@@ -701,45 +703,74 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)batch.buffer_output=-1;argc=parse_options(argc,argv,prefix,options,usage,0);-if(argc&&batch.enabled)-usage_with_options(usage,options);-if(opt=='b'){-batch.all_objects=1;-}elseif(opt){-if(batch.enabled&&(opt=='c'||opt=='w'))-batch.cmdmode=opt;-elseif(argc==1)-obj_name=argv[0];-else-usage_with_options(usage,options);-}elseif(!opt&&!batch.enabled){-if(argc==2){-exp_type=argv[0];-obj_name=argv[1];-}else-usage_with_options(usage,options);-}elseif(batch.enabled&&batch.cmdmode!=opt)-usage_with_options(usage,options);+opt_cw=(opt=='c'||opt=='w');+opt_epts=(opt=='e'||opt=='p'||opt=='t'||opt=='s');-if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){-usage_with_options(usage,options);-}--if(force_path&&opt!='c'&&opt!='w'){-error("--path=<path> needs --textconv or --filters");-usage_with_options(usage,options);-}+/* --batch-all-objects? */+if(opt=='b')+batch.all_objects=1;-if(force_path&&batch.enabled){-error("--path=<path> incompatible with --batch");-usage_with_options(usage,options);-}+/* Option compatibility */+if(force_path&&!opt_cw)+usage_msg_optf(_("'%s=<%s> needs '%s' or '%s'"),+usage,options,+"--path",_("path|tree-ish"),"--filters",+"--textconv");+/* Option compatibility with batch mode */+if(batch.enabled)+;+elseif(batch.follow_symlinks)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--follow_symlinks");+elseif(batch.buffer_output>=0)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--buffer");+elseif(batch.all_objects)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--batch-all_objects");++/* Batch defaults */if(batch.buffer_output<0)batch.buffer_output=batch.all_objects;-if(batch.enabled)+/* Return early if we're in batch mode? */+if(batch.enabled){+if(opt_cw)+batch.cmdmode=opt;+elseif(opt&&opt!='b')+usage_msg_optf(_("'-%c' is incompatible with batch mode"),+usage,options,opt);+elseif(argc)+usage_msg_opt(_("batch modes take no arguments"),usage,+options);+returnbatch_objects(&batch);+}++if(opt){+if(!argc&&opt=='c')+usage_msg_optf(_("<rev> required with '%s'"),+usage,options,"--textconv");+elseif(!argc&&opt=='w')+usage_msg_optf(_("<rev> required with '%s'"),+usage,options,"--filters");+elseif(!argc&&opt_epts)+usage_msg_optf(_("<object> required with '-%c'"),+usage,options,opt);+elseif(argc==1)+obj_name=argv[0];+else+usage_msg_opt(_("too many arguments"),usage,options);+}elseif(!argc){+usage_with_options(usage,options);+}elseif(argc!=2){+usage_msg_optf(_("only two arguments allowed in <type> <object> mode, not %d"),+usage,options,argc);+}elseif(argc){+exp_type=argv[0];+obj_name=argv[1];+}if(unknown_type&&opt!='t'&&opt!='s')die("git cat-file --allow-unknown-type: use with -s or -t");
@@ -30,48 +30,54 @@ do'done+test_missing_usage(){+test_expect_code129"$@"2>err&&+grep-E"^fatal:.*required"err+}+short_modes="-e -p -t -s"cw_modes="--textconv --filters"foroptin$cw_modesdotest_expect_success"usage: $opt requires another option"'-test_expect_code129gitcat-file$opt+test_missing_usagegitcat-file$opt'doneforoptin$short_modesdotest_expect_success"usage: $opt requires another option"'-test_expect_code129gitcat-file$opt+test_missing_usagegitcat-file$opt'foropt2in--batch\--batch-check\---follow-symlinks+--follow-symlinks\+"--path=foo HEAD:some-path.txt"do-test_expect_failure"usage: incompatible options: $opt and $opt2"'+test_expect_success"usage: incompatible options: $opt and $opt2"'test_incompatible_usagegitcat-file$opt$opt2'done--opt2="--path=foo HEAD:some-path.txt"-test_expect_success"usage: incompatible options: $opt and $opt2"'-test_incompatible_usagegitcat-file$opt$opt2-'done+test_too_many_arguments(){+test_expect_code129"$@"2>err&&+grep-E"^fatal: too many arguments$"err+}+foroptin$short_modes$cw_modesdoargs="one two three"test_expect_success"usage: too many arguments: $opt$args"'-test_expect_code129gitcat-file$opt$args+test_too_many_argumentsgitcat-file$opt$args'foropt2in--buffer--follow-symlinksdotest_expect_success"usage: incompatible arguments: $opt with batch option $opt2"'-test_expect_code129gitcat-file$opt$opt2+test_incompatible_usagegitcat-file$opt$opt2'donedone
@@ -80,14 +86,9 @@ for opt in --buffer \--follow-symlinks\--batch-all-objectsdo-status=success-iftest$opt="--buffer"-then-status=failure-fi-test_expect_$status"usage: bad option combination: $opt without batch mode"'-test_expect_code129gitcat-file$opt&&-test_expect_code129gitcat-file$optcommitHEAD+test_expect_success"usage: bad option combination: $opt without batch mode"'+test_incompatible_usagegitcat-file$opt&&+test_incompatible_usagegitcat-file$optcommitHEAD'done
Change the usage output emitted on "git cat-file -h" to group related
options, making it clear to users which options go with which other
ones.
The new output is:
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
Batch objects requested on stdin (or --batch-all-objects)
--batch[=<format>] show full <object> or <rev> contents
--batch-check[=<format>]
like --batch, but don't emit <contents>
--batch-all-objects with --batch[-check]: ignores stdin, batches all known objects
Change or optimize batch output
--buffer buffer --batch output
--follow-symlinks follow in-tree symlinks
--unordered do not order objects before emitting them
Emit object (blob or tree) with conversion or filter (stand-alone, or with batch)
--textconv run textconv on object's content
--filters run filters on object's content
--path blob|tree use a <path> for (--textconv | --filters ); Not with 'batch'
The old usage was:
<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
--batch-all-objects show all objects with --batch or --batch-check
--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)
--unordered do not order --batch-all-objects output
While shorter, I think the new one is easier to understand, as
e.g. "--allow-unknown-type" is grouped with "-t" and "-s", as it can
only be combined with those options. The same goes for "--buffer",
"--unordered" etc.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 49 +++++++++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 20 deletions(-)
@@ -666,35 +666,44 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)NULL};conststructoptionoptions[]={-OPT_GROUP(N_("<type> can be one of: blob, tree, commit, tag")),-OPT_CMDMODE('t',NULL,&opt,N_("show object type"),'t'),-OPT_CMDMODE('s',NULL,&opt,N_("show object size"),'s'),+/* Simple queries */+OPT_GROUP(N_("Check object existence or emit object contents")),OPT_CMDMODE('e',NULL,&opt,-N_("exit with zero when there's no error"),'e'),-OPT_CMDMODE('p',NULL,&opt,N_("pretty-print object's content"),'p'),-OPT_CMDMODE(0,"textconv",&opt,-N_("for blob objects, run textconv on object's content"),'c'),-OPT_CMDMODE(0,"filters",&opt,-N_("for blob objects, run filters on object's content"),'w'),-OPT_CMDMODE(0,"batch-all-objects",&opt,-N_("show all objects with --batch or --batch-check"),'b'),-OPT_STRING(0,"path",&force_path,N_("blob"),-N_("use a specific path for --textconv/--filters")),+N_("check if <object> exists"),'e'),+OPT_CMDMODE('p',NULL,&opt,N_("pretty-print <object> content"),'p'),++OPT_GROUP(N_("Emit [broken] object attributes")),+OPT_CMDMODE('t',NULL,&opt,N_("show object type (one of 'blob', 'tree', 'commit', 'tag', ...)"),'t'),+OPT_CMDMODE('s',NULL,&opt,N_("show object size"),'s'),OPT_BOOL(0,"allow-unknown-type",&unknown_type,N_("allow -s and -t to work with broken/corrupt objects")),-OPT_BOOL(0,"buffer",&batch.buffer_output,N_("buffer --batch output")),-OPT_CALLBACK_F(0,"batch",&batch,"format",-N_("show info and content of objects fed from the standard input"),+/* Batch mode */+OPT_GROUP(N_("Batch objects requested on stdin (or --batch-all-objects)")),+OPT_CALLBACK_F(0,"batch",&batch,N_("format"),+N_("show full <object> or <rev> contents"),PARSE_OPT_OPTARG|PARSE_OPT_NONEG,batch_option_callback),-OPT_CALLBACK_F(0,"batch-check",&batch,"format",-N_("show info about objects fed from the standard input"),+OPT_CALLBACK_F(0,"batch-check",&batch,N_("format"),+N_("like --batch, but don't emit <contents>"),PARSE_OPT_OPTARG|PARSE_OPT_NONEG,batch_option_callback),+OPT_CMDMODE(0,"batch-all-objects",&opt,+N_("with --batch[-check]: ignores stdin, batches all known objects"),'b'),+/* Batch-specific options */+OPT_GROUP(N_("Change or optimize batch output")),+OPT_BOOL(0,"buffer",&batch.buffer_output,N_("buffer --batch output")),OPT_BOOL(0,"follow-symlinks",&batch.follow_symlinks,-N_("follow in-tree symlinks (used with --batch or --batch-check)")),+N_("follow in-tree symlinks")),OPT_BOOL(0,"unordered",&batch.unordered,-N_("do not order --batch-all-objects output")),+N_("do not order objects before emitting them")),+/* Textconv options, stand-ole*/+OPT_GROUP(N_("Emit object (blob or tree) with conversion or filter (stand-alone, or with batch)")),+OPT_CMDMODE(0,"textconv",&opt,+N_("run textconv on object's content"),'c'),+OPT_CMDMODE(0,"filters",&opt,+N_("run filters on object's content"),'w'),+OPT_STRING(0,"path",&force_path,N_("blob|tree"),+N_("use a <path> for (--textconv | --filters ); Not with 'batch'")),OPT_END()};
Stop having GET_OID_ONLY_TO_DIE imply GET_OID_QUIETLY in
get_oid_with_context_1().
The *_DIE flag was added in 33bd598c390 (sha1_name.c: teach lookup
context to get_sha1_with_context(), 2012-07-02), and then later
tweaked in 7243ffdd78d (get_sha1: avoid repeating ourselves via
ONLY_TO_DIE, 2016-09-26).
Everything in that commit makes sense, but only for callers that
expect to fail in an initial call to get_oid_with_context_1(), e.g. as
"git show 0017" does via handle_revision_arg(), and then would like to
call get_oid_with_context_1() again via this
maybe_die_on_misspelt_object_name() function.
In the subsequent commit we'll add a new caller that excepts to call
this only once, but who would still like to have all the error
messaging that GET_OID_ONLY_TO_DIE gives it, in addition to any
regular errors.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
object-name.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
Improve the errors emitted when an invalid <object> and/or <path> is
provided with either the --path option, or as an argument. We now use
the same logic in get_oid_with_context_1() that "git show" et al use.
To replace the "cat-file" use-case we need to introduce a new
"GET_OID_REQUIRE_PATH" flag, otherwise it would exit early as soon as
a valid "HEAD" was resolved, but in the "cat-file" case being changed
we always need a valid revision and path.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 15 +++++----------
cache.h | 1 +
object-name.c | 6 +++++-
t/t8007-cat-file-textconv.sh | 6 +++---
4 files changed, 14 insertions(+), 14 deletions(-)
@@ -27,19 +27,19 @@ test_expect_success 'usage' 'test_cmpexpectactual&&cat>expect<<-\EOF&&-fatal:NotavalidobjectnameHEAD2:two.bin+fatal:invalidobjectname'\''HEAD2'\''.EOFtest_must_failgitcat-file--textconvHEAD2:two.bin2>actual&&test_cmpexpectactual&&cat>expect<<-\EOF&&-fatal:gitcat-file--textconvHEAD:<object>mustbe<sha1:path>+fatal:<object>:<path>required,only<object>'\''HEAD'\''givenEOFtest_must_failgitcat-file--textconvHEAD2>actual&&test_cmpexpectactual&&cat>expect<<-\EOF&&-fatal:NotavalidobjectnameHEAD:two.bin+fatal:path'\''two.bin'\'' does not exist in '\''HEAD'\''EOFtest_must_failgitcat-file--textconvHEAD:two.bin2>actual&&test_cmpexpectactual
From: Eric Sunshine <hidden> Date: 2021-11-07 01:07:33
On Sat, Nov 6, 2021 at 5:47 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted hunk
Stress test the usage emitted when options are combined in ways that
isn't supported. Let's test various option combinations, some of these
we buggily allow right now.
E.g. this reveals a bug in 321459439e1 (cat-file: support
--textconv/--filters in batch mode, 2016-09-09) that we'll fix in a
subsequent commit. We're supposed to be emitting a relevant message
when --batch-all-objects is combined with --textconv or --filters, but
we don't.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
A minor observation: I usually avoid combining tests into a
conglomerate since it makes it harder to discover at a glance
the problematic test if one does start failing. I'd probably have used a
separate test_expect_success() invocation for each allowed switch
combination (in other words, five distinct tests instead of all five
cases stuffed into a single test). Not a big deal.
So, the only reason the final `opt2` is not part of the for-loop:
for opt2 in --batch \
--batch-check \
--follow-symlinks \
"--path=foo HEAD:some-path.txt"
is that it succeeds but the others fail?
+for opt in --buffer \
+ --follow-symlinks \
+ --batch-all-objects
+do
+ status=success
+ if test $opt = "--buffer"
+ then
+ status=failure
+ fi
+ test_expect_$status "usage: bad option combination: $opt without batch mode" '
+ test_expect_code 129 git cat-file $opt &&
+ test_expect_code 129 git cat-file $opt commit HEAD
+ '
+done
In this case, `status` differentiates between success and failure...
From: Eric Sunshine <hidden> Date: 2021-11-07 03:06:00
On Sat, Nov 6, 2021 at 5:47 PM Ævar Arnfjörð Bjarmason [off-list ref] 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.
By giving parse_options() information about what options are mutually
exclusive with one another we can get the die9) message being removed
here for free, we didn't even use that removed message in some cases,
e.g. for both of:
s/die9)/die()/
--batch-all-objects --textconv
--batch-all-objects --filters
We'd take the "goto usage" in the "if (opt)" branch, and never reach
the previous message. Now we'll emit e.g.:
$ git cat-file --batch-all-objects --filters
error: option `filters' is incompatible with --batch-all-objects
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 <redacted>
From: Eric Sunshine <hidden> Date: 2021-11-07 03:06:00
On Sat, Nov 6, 2021 at 5:47 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
Stop having GET_OID_ONLY_TO_DIE imply GET_OID_QUIETLY in
get_oid_with_context_1().
The *_DIE flag was added in 33bd598c390 (sha1_name.c: teach lookup
context to get_sha1_with_context(), 2012-07-02), and then later
tweaked in 7243ffdd78d (get_sha1: avoid repeating ourselves via
ONLY_TO_DIE, 2016-09-26).
Everything in that commit makes sense, but only for callers that
expect to fail in an initial call to get_oid_with_context_1(), e.g. as
"git show 0017" does via handle_revision_arg(), and then would like to
call get_oid_with_context_1() again via this
maybe_die_on_misspelt_object_name() function.
In the subsequent commit we'll add a new caller that excepts to call
this only once, but who would still like to have all the error
messaging that GET_OID_ONLY_TO_DIE gives it, in addition to any
regular errors.
This series of patches to cat-file significantly improves the UX of
the -h output, see 08/10. For the v1 see[1]
This update addresses comments Eric Sunshine had on test patterns &
typos. I've taken (or attemted to address) all his comments
here. There was a leftover unused $switch variable from development,
and some other minor test & typo nits.
Junio: John Cai expressed (off-list) interest in building on top of
this for the cat-file "new stdin mode"+fflush() batch command. You
seemed to think the direction of my WIP patch in that direction was
good, whic his much easier to implement, test & be sure about the
correctness off after this series.
So it would be great to have this tightening of the options behavior &
UX improvement picked up.
1. https://lore.kernel.org/git/cover-00.10-00000000000-20211106T214259Z-avarab@gmail.com/
2. https://lore.kernel.org/git/xmqqk0hitnkc.fsf@gitster.g/
3. https://lore.kernel.org/git/xmqqk0hitnkc.fsf@gitster.g/
Ævar Arnfjörð Bjarmason (10):
cat-file tests: test bad usage
cat-file tests: test messaging on bad objects/paths
parse-options API: add a usage_msg_optf()
cat-file docs: fix SYNOPSIS and "-h" output
cat-file: move "usage" variable to cmd_cat_file()
cat-file: make --batch-all-objects a CMDMODE
cat-file: fix remaining usage bugs
cat-file: correct and improve usage information
object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY
cat-file: improve --(textconv|filters) disambiguation
Documentation/git-cat-file.txt | 10 +-
builtin/cat-file.c | 181 ++++++++++++++++++++-------------
builtin/stash.c | 4 +-
cache.h | 1 +
object-name.c | 11 +-
parse-options.c | 13 +++
parse-options.h | 10 ++
t/t1006-cat-file.sh | 92 +++++++++++++++++
t/t8007-cat-file-textconv.sh | 26 +++++
9 files changed, 267 insertions(+), 81 deletions(-)
Range-diff against v1:
1: c8040da8e55 ! 1: 3a0d2923cfa cat-file tests: test bad usage
@@ Commit message
when --batch-all-objects is combined with --textconv or --filters, but
we don't.
+ The cases of needing to assign to opt=2 in the "opt" loop are because
+ on those we do the right thing already, in subsequent commits the
+ "test_expect_failure" cases will be fixed, and the for-loops unified.
+
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
## t/t1006-cat-file.sh ##
@@ t/t1006-cat-file.sh: test_description='git cat-file'
. ./test-lib.sh
-+test_cmdmode_usage() {
++test_cmdmode_usage () {
+ test_expect_code 129 "$@" 2>err &&
+ grep "^error:.*is incompatible with" err
+}
+
-+test_expect_success 'usage: cmdmode' '
-+ test_cmdmode_usage git cat-file -e -p &&
-+ test_cmdmode_usage git cat-file -p -t &&
-+ test_cmdmode_usage git cat-file -t -s &&
-+ test_cmdmode_usage git cat-file -s --textconv &&
-+ test_cmdmode_usage git cat-file --textconv --filters
-+'
++for switches in \
++ '-e -p' \
++ '-p -t' \
++ '-t -s' \
++ '-s --textconv' \
++ '--textconv --filters'
++do
++ test_expect_success "usage: cmdmode $switches" '
++ test_cmdmode_usage git cat-file $switches
++ '
++done
+
-+test_incompatible_usage() {
++test_incompatible_usage () {
+ test_expect_code 129 "$@" 2>err &&
-+ grep -E "^error:.*$switch.*needs" err
++ grep -E "^error:.**needs" err
+}
+
+for opt in --batch --batch-check
2: a473185eb97 = 2: fc8d5e60682 cat-file tests: test messaging on bad objects/paths
3: 5d87897f49c = 3: 0e2e5ab9d2d parse-options API: add a usage_msg_optf()
4: 29b67330a48 = 4: b9c935b95b7 cat-file docs: fix SYNOPSIS and "-h" output
5: 1974136d483 = 5: 664c5db634e cat-file: move "usage" variable to cmd_cat_file()
6: ee49e586483 ! 6: d945fc94774 cat-file: make --batch-all-objects a CMDMODE
@@ Commit message
instead. It doesn't combine with --filters, --textconv etc.
By giving parse_options() information about what options are mutually
- exclusive with one another we can get the die9) message being removed
+ exclusive with one another we can get the die() message being removed
here for free, we didn't even use that removed message in some cases,
e.g. for both of:
@@ builtin/cat-file.c: int cmd_cat_file(int argc, const char **argv, const char *pr
usage_with_options(usage, options);
## t/t1006-cat-file.sh ##
-@@ t/t1006-cat-file.sh: test_expect_success 'usage: cmdmode' '
- test_cmdmode_usage git cat-file -p -t &&
- test_cmdmode_usage git cat-file -t -s &&
- test_cmdmode_usage git cat-file -s --textconv &&
-- test_cmdmode_usage git cat-file --textconv --filters
-+ test_cmdmode_usage git cat-file --textconv --filters &&
-+ test_cmdmode_usage git cat-file --batch-all-objects -e
- '
-
- test_incompatible_usage() {
+@@ t/t1006-cat-file.sh: for switches in \
+ '-p -t' \
+ '-t -s' \
+ '-s --textconv' \
+- '--textconv --filters'
++ '--textconv --filters' \
++ '--batch-all-objects -e'
+ do
+ test_expect_success "usage: cmdmode $switches" '
+ test_cmdmode_usage git cat-file $switches
@@ t/t1006-cat-file.sh: do
test_expect_success "usage: $opt requires another option" '
test_expect_code 129 git cat-file $opt
7: 9e1dcd6b824 ! 7: 22f55e1fb6b cat-file: fix remaining usage bugs
@@ builtin/cat-file.c: int cmd_cat_file(int argc, const char **argv, const char *pr
die("git cat-file --allow-unknown-type: use with -s or -t");
## t/t1006-cat-file.sh ##
-@@ t/t1006-cat-file.sh: test_expect_success 'usage: cmdmode' '
+@@ t/t1006-cat-file.sh: done
- test_incompatible_usage() {
+ test_incompatible_usage () {
test_expect_code 129 "$@" 2>err &&
-- grep -E "^error:.*$switch.*needs" err
-+ grep -E "^(fatal|error):.*$switch.*(requires|incompatible with|needs)" err
+- grep -E "^error:.**needs" err
++ grep -E "^(fatal|error):.*(requires|incompatible with|needs)" err
}
for opt in --batch --batch-check
8: 951fea02b83 = 8: 0842df64695 cat-file: correct and improve usage information
9: 49f9e30792b ! 9: 6642b57c6fe object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY
@@ Commit message
call get_oid_with_context_1() again via this
maybe_die_on_misspelt_object_name() function.
- In the subsequent commit we'll add a new caller that excepts to call
+ In the subsequent commit we'll add a new caller that expects to call
this only once, but who would still like to have all the error
messaging that GET_OID_ONLY_TO_DIE gives it, in addition to any
regular errors.
10: 3d61399aa78 = 10: 177f16ba856 cat-file: improve --(textconv|filters) disambiguation
--
2.34.0.rc2.795.g926201d1cc8
Stress test the usage emitted when options are combined in ways that
isn't supported. Let's test various option combinations, some of these
we buggily allow right now.
E.g. this reveals a bug in 321459439e1 (cat-file: support
--textconv/--filters in batch mode, 2016-09-09) that we'll fix in a
subsequent commit. We're supposed to be emitting a relevant message
when --batch-all-objects is combined with --textconv or --filters, but
we don't.
The cases of needing to assign to opt=2 in the "opt" loop are because
on those we do the right thing already, in subsequent commits the
"test_expect_failure" cases will be fixed, and the for-loops unified.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t1006-cat-file.sh | 94 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
There were various inaccuracies in the previous SYNOPSIS output,
e.g. "--path" is not something that can optionally go with any options
except --textconv or --filters, as the output implied.
The opening line of the DESCRIPTION section is also "In its first
form[...]", which refers to "git cat-file <type> <object>", but the
SYNOPSIS section wasn't showing that as the first form!
That part of the documentation made sense in
d83a42f34a6 (Documentation: minor grammatical fixes in
git-cat-file.txt, 2009-03-22) when it was introduced, but since then
various options that were added have made that intro make no sense in
the context it was in. Now the two will match again.
The usage output here is not properly aligned on "master" currently,
but will be with my in-flight 4631cfc20bd (parse-options: properly
align continued usage output, 2021-09-21), so let's indent things
correctly in the C code in anticipation of that.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Documentation/git-cat-file.txt | 10 ++++++++--
builtin/cat-file.c | 10 ++++++++--
2 files changed, 16 insertions(+), 4 deletions(-)
Add a usage_msg_optf() as a shorthand for the sort of
usage_msg_opt(xstrfmt(...)) used in builtin/stash.c. I'll make more
use of this function in builtin/cat-file.c shortly.
The disconnect between the "..." and "fmt" is a bit unusual, but it
works just fine and this keeps it consistent with usage_msg_opt(),
i.e. a caller of it can be moved to usage_msg_optf() and not have to
have its arguments re-arranged.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/stash.c | 4 ++--
parse-options.c | 13 +++++++++++++
parse-options.h | 10 ++++++++++
3 files changed, 25 insertions(+), 2 deletions(-)
Add tests for the output that's emitted when we disambiguate
<obj>:<path> in cat-file. This gives us a baseline for improving these
messages.
For e.g. "git blame" we'll emit:
$ git blame HEAD:foo
fatal: no such path 'HEAD:foo' in HEAD
But cat-file doesn't disambiguate these two cases, and just gives the
rather unhelpful:
$ git cat-file --textconv HEAD:foo
fatal: Not a valid object name HEAD:foo
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t8007-cat-file-textconv.sh | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
There's no benefit to defining this at a distance, and it makes the
code harder to read as you've got to scroll up to see the usage that
corresponds to the options.
In subsequent commits I'll make use of usage_msg_opt(), which will be
quite noisy if I have to use the long "cat_file_usage" variable,
there's no other command being defined in this file, so let's rename
it to just "usage".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 37 ++++++++++++++++++-------------------
1 file changed, 18 insertions(+), 19 deletions(-)
@@ -708,35 +707,35 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)elseif(argc==1)obj_name=argv[0];else-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(!opt&&!batch.enabled){if(argc==2){exp_type=argv[0];obj_name=argv[1];}else-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(batch.enabled){if(batch.cmdmode!=opt||argc)-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);if(batch.cmdmode&&batch.all_objects)die("--batch-all-objects cannot be combined with ""--textconv nor with --filters");}if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(force_path&&opt!='c'&&opt!='w'){error("--path=<path> needs --textconv or --filters");-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(force_path&&batch.enabled){error("--path=<path> incompatible with --batch");-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(batch.buffer_output<0)
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.
By giving parse_options() information about what options are mutually
exclusive with one another we can get the die() message being removed
here for free, we didn't even use that removed message in some cases,
e.g. for both of:
--batch-all-objects --textconv
--batch-all-objects --filters
We'd take the "goto usage" in the "if (opt)" branch, and never reach
the previous message. Now we'll emit e.g.:
$ git cat-file --batch-all-objects --filters
error: option `filters' is incompatible with --batch-all-objects
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 <redacted>
---
builtin/cat-file.c | 25 +++++++++++--------------
t/t1006-cat-file.sh | 7 ++-----
2 files changed, 13 insertions(+), 19 deletions(-)
@@ -674,6 +674,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)N_("for blob objects, run textconv on object's content"),'c'),OPT_CMDMODE(0,"filters",&opt,N_("for blob objects, run filters on object's content"),'w'),+OPT_CMDMODE(0,"batch-all-objects",&opt,+N_("show all objects with --batch or --batch-check"),'b'),OPT_STRING(0,"path",&force_path,N_("blob"),N_("use a specific path for --textconv/--filters")),OPT_BOOL(0,"allow-unknown-type",&unknown_type,
@@ -689,8 +691,6 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)batch_option_callback),OPT_BOOL(0,"follow-symlinks",&batch.follow_symlinks,N_("follow in-tree symlinks (used with --batch or --batch-check)")),-OPT_BOOL(0,"batch-all-objects",&batch.all_objects,-N_("show all objects with --batch or --batch-check")),OPT_BOOL(0,"unordered",&batch.unordered,N_("do not order --batch-all-objects output")),OPT_END()
@@ -699,30 +699,27 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)git_config(git_cat_file_config,NULL);batch.buffer_output=-1;-argc=parse_options(argc,argv,prefix,options,usage,0);-if(opt){+argc=parse_options(argc,argv,prefix,options,usage,0);+if(argc&&batch.enabled)+usage_with_options(usage,options);+if(opt=='b'){+batch.all_objects=1;+}elseif(opt){if(batch.enabled&&(opt=='c'||opt=='w'))batch.cmdmode=opt;elseif(argc==1)obj_name=argv[0];elseusage_with_options(usage,options);-}-if(!opt&&!batch.enabled){+}elseif(!opt&&!batch.enabled){if(argc==2){exp_type=argv[0];obj_name=argv[1];}elseusage_with_options(usage,options);-}-if(batch.enabled){-if(batch.cmdmode!=opt||argc)-usage_with_options(usage,options);-if(batch.cmdmode&&batch.all_objects)-die("--batch-all-objects cannot be combined with "-"--textconv nor with --filters");-}+}elseif(batch.enabled&&batch.cmdmode!=opt)+usage_with_options(usage,options);if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){usage_with_options(usage,options);
With the migration of --batch-all-objects to OPT_CMDMODE() in the
preceding commit one bug with combining it and other OPT_CMDMODE()
options was solved, but we were still left with e.g. --buffer silently
being discarded when not in batch mode.
Fix all those bugs, and in addition emit errors telling the user
specifically what options can't be combined with what other options,
before this we'd usually just emit the cryptic usage text and leave
the users to work it out by themselves.
This change is rather large, because to do so we need to untangle the
options processing so that we can not only error out, but emit
sensible errors, and e.g. emit errors about options before errors
about stray argc elements (as they might become valid if the option
were removed).
Some of the output changes ("error:" to "fatal:" with
usage_msg_opt[f]()), but none of the exit codes change, except in
those cases where we silently accepted bad option combinations before,
now we'll error out.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 95 ++++++++++++++++++++++++++++++---------------
t/t1006-cat-file.sh | 41 +++++++++----------
2 files changed, 84 insertions(+), 52 deletions(-)
@@ -648,6 +648,8 @@ static int batch_option_callback(const struct option *opt,intcmd_cat_file(intargc,constchar**argv,constchar*prefix){intopt=0;+intopt_cw=0;+intopt_epts=0;constchar*exp_type=NULL,*obj_name=NULL;structbatch_optionsbatch={0};intunknown_type=0;
@@ -701,45 +703,74 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)batch.buffer_output=-1;argc=parse_options(argc,argv,prefix,options,usage,0);-if(argc&&batch.enabled)-usage_with_options(usage,options);-if(opt=='b'){-batch.all_objects=1;-}elseif(opt){-if(batch.enabled&&(opt=='c'||opt=='w'))-batch.cmdmode=opt;-elseif(argc==1)-obj_name=argv[0];-else-usage_with_options(usage,options);-}elseif(!opt&&!batch.enabled){-if(argc==2){-exp_type=argv[0];-obj_name=argv[1];-}else-usage_with_options(usage,options);-}elseif(batch.enabled&&batch.cmdmode!=opt)-usage_with_options(usage,options);+opt_cw=(opt=='c'||opt=='w');+opt_epts=(opt=='e'||opt=='p'||opt=='t'||opt=='s');-if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){-usage_with_options(usage,options);-}--if(force_path&&opt!='c'&&opt!='w'){-error("--path=<path> needs --textconv or --filters");-usage_with_options(usage,options);-}+/* --batch-all-objects? */+if(opt=='b')+batch.all_objects=1;-if(force_path&&batch.enabled){-error("--path=<path> incompatible with --batch");-usage_with_options(usage,options);-}+/* Option compatibility */+if(force_path&&!opt_cw)+usage_msg_optf(_("'%s=<%s> needs '%s' or '%s'"),+usage,options,+"--path",_("path|tree-ish"),"--filters",+"--textconv");+/* Option compatibility with batch mode */+if(batch.enabled)+;+elseif(batch.follow_symlinks)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--follow_symlinks");+elseif(batch.buffer_output>=0)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--buffer");+elseif(batch.all_objects)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--batch-all_objects");++/* Batch defaults */if(batch.buffer_output<0)batch.buffer_output=batch.all_objects;-if(batch.enabled)+/* Return early if we're in batch mode? */+if(batch.enabled){+if(opt_cw)+batch.cmdmode=opt;+elseif(opt&&opt!='b')+usage_msg_optf(_("'-%c' is incompatible with batch mode"),+usage,options,opt);+elseif(argc)+usage_msg_opt(_("batch modes take no arguments"),usage,+options);+returnbatch_objects(&batch);+}++if(opt){+if(!argc&&opt=='c')+usage_msg_optf(_("<rev> required with '%s'"),+usage,options,"--textconv");+elseif(!argc&&opt=='w')+usage_msg_optf(_("<rev> required with '%s'"),+usage,options,"--filters");+elseif(!argc&&opt_epts)+usage_msg_optf(_("<object> required with '-%c'"),+usage,options,opt);+elseif(argc==1)+obj_name=argv[0];+else+usage_msg_opt(_("too many arguments"),usage,options);+}elseif(!argc){+usage_with_options(usage,options);+}elseif(argc!=2){+usage_msg_optf(_("only two arguments allowed in <type> <object> mode, not %d"),+usage,options,argc);+}elseif(argc){+exp_type=argv[0];+obj_name=argv[1];+}if(unknown_type&&opt!='t'&&opt!='s')die("git cat-file --allow-unknown-type: use with -s or -t");
@@ -34,48 +34,54 @@ do'done+test_missing_usage(){+test_expect_code129"$@"2>err&&+grep-E"^fatal:.*required"err+}+short_modes="-e -p -t -s"cw_modes="--textconv --filters"foroptin$cw_modesdotest_expect_success"usage: $opt requires another option"'-test_expect_code129gitcat-file$opt+test_missing_usagegitcat-file$opt'doneforoptin$short_modesdotest_expect_success"usage: $opt requires another option"'-test_expect_code129gitcat-file$opt+test_missing_usagegitcat-file$opt'foropt2in--batch\--batch-check\---follow-symlinks+--follow-symlinks\+"--path=foo HEAD:some-path.txt"do-test_expect_failure"usage: incompatible options: $opt and $opt2"'+test_expect_success"usage: incompatible options: $opt and $opt2"'test_incompatible_usagegitcat-file$opt$opt2'done--opt2="--path=foo HEAD:some-path.txt"-test_expect_success"usage: incompatible options: $opt and $opt2"'-test_incompatible_usagegitcat-file$opt$opt2-'done+test_too_many_arguments(){+test_expect_code129"$@"2>err&&+grep-E"^fatal: too many arguments$"err+}+foroptin$short_modes$cw_modesdoargs="one two three"test_expect_success"usage: too many arguments: $opt$args"'-test_expect_code129gitcat-file$opt$args+test_too_many_argumentsgitcat-file$opt$args'foropt2in--buffer--follow-symlinksdotest_expect_success"usage: incompatible arguments: $opt with batch option $opt2"'-test_expect_code129gitcat-file$opt$opt2+test_incompatible_usagegitcat-file$opt$opt2'donedone
@@ -84,14 +90,9 @@ for opt in --buffer \--follow-symlinks\--batch-all-objectsdo-status=success-iftest$opt="--buffer"-then-status=failure-fi-test_expect_$status"usage: bad option combination: $opt without batch mode"'-test_expect_code129gitcat-file$opt&&-test_expect_code129gitcat-file$optcommitHEAD+test_expect_success"usage: bad option combination: $opt without batch mode"'+test_incompatible_usagegitcat-file$opt&&+test_incompatible_usagegitcat-file$optcommitHEAD'done
Change the usage output emitted on "git cat-file -h" to group related
options, making it clear to users which options go with which other
ones.
The new output is:
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
Batch objects requested on stdin (or --batch-all-objects)
--batch[=<format>] show full <object> or <rev> contents
--batch-check[=<format>]
like --batch, but don't emit <contents>
--batch-all-objects with --batch[-check]: ignores stdin, batches all known objects
Change or optimize batch output
--buffer buffer --batch output
--follow-symlinks follow in-tree symlinks
--unordered do not order objects before emitting them
Emit object (blob or tree) with conversion or filter (stand-alone, or with batch)
--textconv run textconv on object's content
--filters run filters on object's content
--path blob|tree use a <path> for (--textconv | --filters ); Not with 'batch'
The old usage was:
<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
--batch-all-objects show all objects with --batch or --batch-check
--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)
--unordered do not order --batch-all-objects output
While shorter, I think the new one is easier to understand, as
e.g. "--allow-unknown-type" is grouped with "-t" and "-s", as it can
only be combined with those options. The same goes for "--buffer",
"--unordered" etc.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 49 +++++++++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 20 deletions(-)
@@ -666,35 +666,44 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)NULL};conststructoptionoptions[]={-OPT_GROUP(N_("<type> can be one of: blob, tree, commit, tag")),-OPT_CMDMODE('t',NULL,&opt,N_("show object type"),'t'),-OPT_CMDMODE('s',NULL,&opt,N_("show object size"),'s'),+/* Simple queries */+OPT_GROUP(N_("Check object existence or emit object contents")),OPT_CMDMODE('e',NULL,&opt,-N_("exit with zero when there's no error"),'e'),-OPT_CMDMODE('p',NULL,&opt,N_("pretty-print object's content"),'p'),-OPT_CMDMODE(0,"textconv",&opt,-N_("for blob objects, run textconv on object's content"),'c'),-OPT_CMDMODE(0,"filters",&opt,-N_("for blob objects, run filters on object's content"),'w'),-OPT_CMDMODE(0,"batch-all-objects",&opt,-N_("show all objects with --batch or --batch-check"),'b'),-OPT_STRING(0,"path",&force_path,N_("blob"),-N_("use a specific path for --textconv/--filters")),+N_("check if <object> exists"),'e'),+OPT_CMDMODE('p',NULL,&opt,N_("pretty-print <object> content"),'p'),++OPT_GROUP(N_("Emit [broken] object attributes")),+OPT_CMDMODE('t',NULL,&opt,N_("show object type (one of 'blob', 'tree', 'commit', 'tag', ...)"),'t'),+OPT_CMDMODE('s',NULL,&opt,N_("show object size"),'s'),OPT_BOOL(0,"allow-unknown-type",&unknown_type,N_("allow -s and -t to work with broken/corrupt objects")),-OPT_BOOL(0,"buffer",&batch.buffer_output,N_("buffer --batch output")),-OPT_CALLBACK_F(0,"batch",&batch,"format",-N_("show info and content of objects fed from the standard input"),+/* Batch mode */+OPT_GROUP(N_("Batch objects requested on stdin (or --batch-all-objects)")),+OPT_CALLBACK_F(0,"batch",&batch,N_("format"),+N_("show full <object> or <rev> contents"),PARSE_OPT_OPTARG|PARSE_OPT_NONEG,batch_option_callback),-OPT_CALLBACK_F(0,"batch-check",&batch,"format",-N_("show info about objects fed from the standard input"),+OPT_CALLBACK_F(0,"batch-check",&batch,N_("format"),+N_("like --batch, but don't emit <contents>"),PARSE_OPT_OPTARG|PARSE_OPT_NONEG,batch_option_callback),+OPT_CMDMODE(0,"batch-all-objects",&opt,+N_("with --batch[-check]: ignores stdin, batches all known objects"),'b'),+/* Batch-specific options */+OPT_GROUP(N_("Change or optimize batch output")),+OPT_BOOL(0,"buffer",&batch.buffer_output,N_("buffer --batch output")),OPT_BOOL(0,"follow-symlinks",&batch.follow_symlinks,-N_("follow in-tree symlinks (used with --batch or --batch-check)")),+N_("follow in-tree symlinks")),OPT_BOOL(0,"unordered",&batch.unordered,-N_("do not order --batch-all-objects output")),+N_("do not order objects before emitting them")),+/* Textconv options, stand-ole*/+OPT_GROUP(N_("Emit object (blob or tree) with conversion or filter (stand-alone, or with batch)")),+OPT_CMDMODE(0,"textconv",&opt,+N_("run textconv on object's content"),'c'),+OPT_CMDMODE(0,"filters",&opt,+N_("run filters on object's content"),'w'),+OPT_STRING(0,"path",&force_path,N_("blob|tree"),+N_("use a <path> for (--textconv | --filters ); Not with 'batch'")),OPT_END()};
Stop having GET_OID_ONLY_TO_DIE imply GET_OID_QUIETLY in
get_oid_with_context_1().
The *_DIE flag was added in 33bd598c390 (sha1_name.c: teach lookup
context to get_sha1_with_context(), 2012-07-02), and then later
tweaked in 7243ffdd78d (get_sha1: avoid repeating ourselves via
ONLY_TO_DIE, 2016-09-26).
Everything in that commit makes sense, but only for callers that
expect to fail in an initial call to get_oid_with_context_1(), e.g. as
"git show 0017" does via handle_revision_arg(), and then would like to
call get_oid_with_context_1() again via this
maybe_die_on_misspelt_object_name() function.
In the subsequent commit we'll add a new caller that expects to call
this only once, but who would still like to have all the error
messaging that GET_OID_ONLY_TO_DIE gives it, in addition to any
regular errors.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
object-name.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
Improve the errors emitted when an invalid <object> and/or <path> is
provided with either the --path option, or as an argument. We now use
the same logic in get_oid_with_context_1() that "git show" et al use.
To replace the "cat-file" use-case we need to introduce a new
"GET_OID_REQUIRE_PATH" flag, otherwise it would exit early as soon as
a valid "HEAD" was resolved, but in the "cat-file" case being changed
we always need a valid revision and path.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 15 +++++----------
cache.h | 1 +
object-name.c | 6 +++++-
t/t8007-cat-file-textconv.sh | 6 +++---
4 files changed, 14 insertions(+), 14 deletions(-)
@@ -27,19 +27,19 @@ test_expect_success 'usage' 'test_cmpexpectactual&&cat>expect<<-\EOF&&-fatal:NotavalidobjectnameHEAD2:two.bin+fatal:invalidobjectname'\''HEAD2'\''.EOFtest_must_failgitcat-file--textconvHEAD2:two.bin2>actual&&test_cmpexpectactual&&cat>expect<<-\EOF&&-fatal:gitcat-file--textconvHEAD:<object>mustbe<sha1:path>+fatal:<object>:<path>required,only<object>'\''HEAD'\''givenEOFtest_must_failgitcat-file--textconvHEAD2>actual&&test_cmpexpectactual&&cat>expect<<-\EOF&&-fatal:NotavalidobjectnameHEAD:two.bin+fatal:path'\''two.bin'\'' does not exist in '\''HEAD'\''EOFtest_must_failgitcat-file--textconvHEAD:two.bin2>actual&&test_cmpexpectactual
This series of patches to cat-file significantly improves the UX of
the -h output, see 08/10. For the v2 see[1].
This is mainly a re-submission for a series that got lost in the
shuffle around the last release, but in going through it again I made
some minor updates, see the below range-diff.
John Cai (CC'd) expressed interest in reviewing this & perhaps running
with the WIP patch I noted in [2] for extending "cat-file --batch" to
accept named commands. This series should help that along, i.e. it
eliminates the confusion about what does and doesn't combine with the
batch mode.
1. https://lore.kernel.org/git/cover-v2-00.10-00000000000-20211112T221506Z-avarab@gmail.com/
2. https://lore.kernel.org/git/211106.86k0hmgc8q.gmgdl@evledraar.gmail.com/
Ævar Arnfjörð Bjarmason (10):
cat-file tests: test bad usage
cat-file tests: test messaging on bad objects/paths
parse-options API: add a usage_msg_optf()
cat-file docs: fix SYNOPSIS and "-h" output
cat-file: move "usage" variable to cmd_cat_file()
cat-file: make --batch-all-objects a CMDMODE
cat-file: fix remaining usage bugs
cat-file: correct and improve usage information
object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY
cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters)
Documentation/git-cat-file.txt | 10 +-
builtin/cat-file.c | 181 ++++++++++++++++++++-------------
builtin/stash.c | 4 +-
cache.h | 1 +
object-name.c | 11 +-
parse-options.c | 13 +++
parse-options.h | 10 ++
t/t1006-cat-file.sh | 92 +++++++++++++++++
t/t8007-cat-file-textconv.sh | 42 ++++++++
9 files changed, 283 insertions(+), 81 deletions(-)
Range-diff against v2:
1: 3a0d2923cfa ! 1: d77771e3ea0 cat-file tests: test bad usage
@@ t/t1006-cat-file.sh: test_description='git cat-file'
+}
+
+for switches in \
-+ '-e -p' \
-+ '-p -t' \
-+ '-t -s' \
-+ '-s --textconv' \
-+ '--textconv --filters'
++ '-e -p' \
++ '-p -t' \
++ '-t -s' \
++ '-s --textconv' \
++ '--textconv --filters'
+do
+ test_expect_success "usage: cmdmode $switches" '
+ test_cmdmode_usage git cat-file $switches
2: fc8d5e60682 ! 2: ab21a69864f cat-file tests: test messaging on bad objects/paths
@@ t/t8007-cat-file-textconv.sh: test_expect_success 'setup ' '
GIT_AUTHOR_NAME=Number2 git commit -a -m Second --date="2010-01-01 20:00:00"
'
-+test_expect_success 'usage' '
++test_expect_success 'usage: <bad rev>' '
+ cat >expect <<-\EOF &&
+ fatal: Not a valid object name HEAD2
+ EOF
+ test_must_fail git cat-file --textconv HEAD2 2>actual &&
-+ test_cmp expect actual &&
++ test_cmp expect actual
++'
+
++test_expect_success 'usage: <bad rev>:<bad path>' '
+ cat >expect <<-\EOF &&
+ fatal: Not a valid object name HEAD2:two.bin
+ EOF
+ test_must_fail git cat-file --textconv HEAD2:two.bin 2>actual &&
-+ test_cmp expect actual &&
++ test_cmp expect actual
++'
+
++test_expect_success 'usage: <rev>:<bad path>' '
++ cat >expect <<-\EOF &&
++ fatal: Not a valid object name HEAD:two.bin
++ EOF
++ test_must_fail git cat-file --textconv HEAD:two.bin 2>actual &&
++ test_cmp expect actual
++'
++
++
++test_expect_success 'usage: <rev> with no <path>' '
+ cat >expect <<-\EOF &&
+ fatal: git cat-file --textconv HEAD: <object> must be <sha1:path>
+ EOF
+ test_must_fail git cat-file --textconv HEAD 2>actual &&
-+ test_cmp expect actual &&
++ test_cmp expect actual
++'
++
+
++test_expect_success 'usage: <bad rev>:<good (in HEAD) path>' '
+ cat >expect <<-\EOF &&
-+ fatal: Not a valid object name HEAD:two.bin
++ fatal: Not a valid object name HEAD2:one.bin
+ EOF
-+ test_must_fail git cat-file --textconv HEAD:two.bin 2>actual &&
++ test_must_fail git cat-file --textconv HEAD2:one.bin 2>actual &&
+ test_cmp expect actual
+'
+
3: 0e2e5ab9d2d = 3: 69ef1ae48c3 parse-options API: add a usage_msg_optf()
4: b9c935b95b7 = 4: 597bb97b90a cat-file docs: fix SYNOPSIS and "-h" output
5: 664c5db634e = 5: a9ea4c52222 cat-file: move "usage" variable to cmd_cat_file()
6: d945fc94774 ! 6: fcb8331f091 cat-file: make --batch-all-objects a CMDMODE
@@ builtin/cat-file.c: int cmd_cat_file(int argc, const char **argv, const char *pr
## t/t1006-cat-file.sh ##
@@ t/t1006-cat-file.sh: for switches in \
- '-p -t' \
- '-t -s' \
- '-s --textconv' \
-- '--textconv --filters'
-+ '--textconv --filters' \
-+ '--batch-all-objects -e'
+ '-p -t' \
+ '-t -s' \
+ '-s --textconv' \
+- '--textconv --filters'
++ '--textconv --filters' \
++ '--batch-all-objects -e'
do
test_expect_success "usage: cmdmode $switches" '
test_cmdmode_usage git cat-file $switches
7: 22f55e1fb6b = 7: ad79e2afc89 cat-file: fix remaining usage bugs
8: 0842df64695 = 8: a378dd30dd0 cat-file: correct and improve usage information
9: 6642b57c6fe = 9: 145c00db08c object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY
10: 177f16ba856 ! 10: 45a24f97c88 cat-file: improve --(textconv|filters) disambiguation
@@ Metadata
Author: Ævar Arnfjörð Bjarmason [off-list ref]
## Commit message ##
- cat-file: improve --(textconv|filters) disambiguation
+ cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters)
- Improve the errors emitted when an invalid <object> and/or <path> is
- provided with either the --path option, or as an argument. We now use
- the same logic in get_oid_with_context_1() that "git show" et al use.
+ Change the cat_one_file() logic that calls get_oid_with_context()
+ under --textconv and --filters to use the GET_OID_ONLY_TO_DIE flag,
+ thus improving the error messaging emitted when e.g. <path> is missing
+ but <rev> is not.
- To replace the "cat-file" use-case we need to introduce a new
+ To service the "cat-file" use-case we need to introduce a new
"GET_OID_REQUIRE_PATH" flag, otherwise it would exit early as soon as
a valid "HEAD" was resolved, but in the "cat-file" case being changed
we always need a valid revision and path.
+ This arguably makes the "<bad rev>:<bad path>" and "<bad
+ rev>:<good (in HEAD) path>" use cases worse, as we won't quote the
+ <path> component at the user anymore, but let's just use the existing
+ logic "git log" et al use for now. We can improve the messaging for
+ those cases as a follow-up for all callers.
+
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
## builtin/cat-file.c ##
@@ object-name.c: static enum get_oid_result get_oid_with_context_1(struct reposito
* :path -> object name of absolute path in index
## t/t8007-cat-file-textconv.sh ##
-@@ t/t8007-cat-file-textconv.sh: test_expect_success 'usage' '
- test_cmp expect actual &&
+@@ t/t8007-cat-file-textconv.sh: test_expect_success 'usage: <bad rev>' '
+ test_expect_success 'usage: <bad rev>:<bad path>' '
cat >expect <<-\EOF &&
- fatal: Not a valid object name HEAD2:two.bin
+ fatal: invalid object name '\''HEAD2'\''.
EOF
test_must_fail git cat-file --textconv HEAD2:two.bin 2>actual &&
- test_cmp expect actual &&
+ test_cmp expect actual
+@@ t/t8007-cat-file-textconv.sh: test_expect_success 'usage: <bad rev>:<bad path>' '
+
+ test_expect_success 'usage: <rev>:<bad path>' '
+ cat >expect <<-\EOF &&
+- fatal: Not a valid object name HEAD:two.bin
++ fatal: path '\''two.bin'\'' does not exist in '\''HEAD'\''
+ EOF
+ test_must_fail git cat-file --textconv HEAD:two.bin 2>actual &&
+ test_cmp expect actual
+@@ t/t8007-cat-file-textconv.sh: test_expect_success 'usage: <rev>:<bad path>' '
+ test_expect_success 'usage: <rev> with no <path>' '
cat >expect <<-\EOF &&
- fatal: git cat-file --textconv HEAD: <object> must be <sha1:path>
+ fatal: <object>:<path> required, only <object> '\''HEAD'\'' given
EOF
test_must_fail git cat-file --textconv HEAD 2>actual &&
- test_cmp expect actual &&
+ test_cmp expect actual
+@@ t/t8007-cat-file-textconv.sh: test_expect_success 'usage: <rev> with no <path>' '
+ test_expect_success 'usage: <bad rev>:<good (in HEAD) path>' '
cat >expect <<-\EOF &&
-- fatal: Not a valid object name HEAD:two.bin
-+ fatal: path '\''two.bin'\'' does not exist in '\''HEAD'\''
+- fatal: Not a valid object name HEAD2:one.bin
++ fatal: invalid object name '\''HEAD2'\''.
EOF
- test_must_fail git cat-file --textconv HEAD:two.bin 2>actual &&
+ test_must_fail git cat-file --textconv HEAD2:one.bin 2>actual &&
test_cmp expect actual
--
2.34.1.841.gf15fb7e6f34
Add tests for the output that's emitted when we disambiguate
<obj>:<path> in cat-file. This gives us a baseline for improving these
messages.
For e.g. "git blame" we'll emit:
$ git blame HEAD:foo
fatal: no such path 'HEAD:foo' in HEAD
But cat-file doesn't disambiguate these two cases, and just gives the
rather unhelpful:
$ git cat-file --textconv HEAD:foo
fatal: Not a valid object name HEAD:foo
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t8007-cat-file-textconv.sh | 42 ++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
Stress test the usage emitted when options are combined in ways that
isn't supported. Let's test various option combinations, some of these
we buggily allow right now.
E.g. this reveals a bug in 321459439e1 (cat-file: support
--textconv/--filters in batch mode, 2016-09-09) that we'll fix in a
subsequent commit. We're supposed to be emitting a relevant message
when --batch-all-objects is combined with --textconv or --filters, but
we don't.
The cases of needing to assign to opt=2 in the "opt" loop are because
on those we do the right thing already, in subsequent commits the
"test_expect_failure" cases will be fixed, and the for-loops unified.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t1006-cat-file.sh | 94 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
Add a usage_msg_optf() as a shorthand for the sort of
usage_msg_opt(xstrfmt(...)) used in builtin/stash.c. I'll make more
use of this function in builtin/cat-file.c shortly.
The disconnect between the "..." and "fmt" is a bit unusual, but it
works just fine and this keeps it consistent with usage_msg_opt(),
i.e. a caller of it can be moved to usage_msg_optf() and not have to
have its arguments re-arranged.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/stash.c | 4 ++--
parse-options.c | 13 +++++++++++++
parse-options.h | 10 ++++++++++
3 files changed, 25 insertions(+), 2 deletions(-)
There were various inaccuracies in the previous SYNOPSIS output,
e.g. "--path" is not something that can optionally go with any options
except --textconv or --filters, as the output implied.
The opening line of the DESCRIPTION section is also "In its first
form[...]", which refers to "git cat-file <type> <object>", but the
SYNOPSIS section wasn't showing that as the first form!
That part of the documentation made sense in
d83a42f34a6 (Documentation: minor grammatical fixes in
git-cat-file.txt, 2009-03-22) when it was introduced, but since then
various options that were added have made that intro make no sense in
the context it was in. Now the two will match again.
The usage output here is not properly aligned on "master" currently,
but will be with my in-flight 4631cfc20bd (parse-options: properly
align continued usage output, 2021-09-21), so let's indent things
correctly in the C code in anticipation of that.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Documentation/git-cat-file.txt | 10 ++++++++--
builtin/cat-file.c | 10 ++++++++--
2 files changed, 16 insertions(+), 4 deletions(-)
There's no benefit to defining this at a distance, and it makes the
code harder to read as you've got to scroll up to see the usage that
corresponds to the options.
In subsequent commits I'll make use of usage_msg_opt(), which will be
quite noisy if I have to use the long "cat_file_usage" variable,
there's no other command being defined in this file, so let's rename
it to just "usage".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 37 ++++++++++++++++++-------------------
1 file changed, 18 insertions(+), 19 deletions(-)
@@ -708,35 +707,35 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)elseif(argc==1)obj_name=argv[0];else-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(!opt&&!batch.enabled){if(argc==2){exp_type=argv[0];obj_name=argv[1];}else-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(batch.enabled){if(batch.cmdmode!=opt||argc)-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);if(batch.cmdmode&&batch.all_objects)die("--batch-all-objects cannot be combined with ""--textconv nor with --filters");}if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(force_path&&opt!='c'&&opt!='w'){error("--path=<path> needs --textconv or --filters");-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(force_path&&batch.enabled){error("--path=<path> incompatible with --batch");-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(batch.buffer_output<0)
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.
By giving parse_options() information about what options are mutually
exclusive with one another we can get the die() message being removed
here for free, we didn't even use that removed message in some cases,
e.g. for both of:
--batch-all-objects --textconv
--batch-all-objects --filters
We'd take the "goto usage" in the "if (opt)" branch, and never reach
the previous message. Now we'll emit e.g.:
$ git cat-file --batch-all-objects --filters
error: option `filters' is incompatible with --batch-all-objects
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 <redacted>
---
builtin/cat-file.c | 25 +++++++++++--------------
t/t1006-cat-file.sh | 7 ++-----
2 files changed, 13 insertions(+), 19 deletions(-)
@@ -674,6 +674,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)N_("for blob objects, run textconv on object's content"),'c'),OPT_CMDMODE(0,"filters",&opt,N_("for blob objects, run filters on object's content"),'w'),+OPT_CMDMODE(0,"batch-all-objects",&opt,+N_("show all objects with --batch or --batch-check"),'b'),OPT_STRING(0,"path",&force_path,N_("blob"),N_("use a specific path for --textconv/--filters")),OPT_BOOL(0,"allow-unknown-type",&unknown_type,
@@ -689,8 +691,6 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)batch_option_callback),OPT_BOOL(0,"follow-symlinks",&batch.follow_symlinks,N_("follow in-tree symlinks (used with --batch or --batch-check)")),-OPT_BOOL(0,"batch-all-objects",&batch.all_objects,-N_("show all objects with --batch or --batch-check")),OPT_BOOL(0,"unordered",&batch.unordered,N_("do not order --batch-all-objects output")),OPT_END()
@@ -699,30 +699,27 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)git_config(git_cat_file_config,NULL);batch.buffer_output=-1;-argc=parse_options(argc,argv,prefix,options,usage,0);-if(opt){+argc=parse_options(argc,argv,prefix,options,usage,0);+if(argc&&batch.enabled)+usage_with_options(usage,options);+if(opt=='b'){+batch.all_objects=1;+}elseif(opt){if(batch.enabled&&(opt=='c'||opt=='w'))batch.cmdmode=opt;elseif(argc==1)obj_name=argv[0];elseusage_with_options(usage,options);-}-if(!opt&&!batch.enabled){+}elseif(!opt&&!batch.enabled){if(argc==2){exp_type=argv[0];obj_name=argv[1];}elseusage_with_options(usage,options);-}-if(batch.enabled){-if(batch.cmdmode!=opt||argc)-usage_with_options(usage,options);-if(batch.cmdmode&&batch.all_objects)-die("--batch-all-objects cannot be combined with "-"--textconv nor with --filters");-}+}elseif(batch.enabled&&batch.cmdmode!=opt)+usage_with_options(usage,options);if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){usage_with_options(usage,options);
With the migration of --batch-all-objects to OPT_CMDMODE() in the
preceding commit one bug with combining it and other OPT_CMDMODE()
options was solved, but we were still left with e.g. --buffer silently
being discarded when not in batch mode.
Fix all those bugs, and in addition emit errors telling the user
specifically what options can't be combined with what other options,
before this we'd usually just emit the cryptic usage text and leave
the users to work it out by themselves.
This change is rather large, because to do so we need to untangle the
options processing so that we can not only error out, but emit
sensible errors, and e.g. emit errors about options before errors
about stray argc elements (as they might become valid if the option
were removed).
Some of the output changes ("error:" to "fatal:" with
usage_msg_opt[f]()), but none of the exit codes change, except in
those cases where we silently accepted bad option combinations before,
now we'll error out.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 95 ++++++++++++++++++++++++++++++---------------
t/t1006-cat-file.sh | 41 +++++++++----------
2 files changed, 84 insertions(+), 52 deletions(-)
@@ -648,6 +648,8 @@ static int batch_option_callback(const struct option *opt,intcmd_cat_file(intargc,constchar**argv,constchar*prefix){intopt=0;+intopt_cw=0;+intopt_epts=0;constchar*exp_type=NULL,*obj_name=NULL;structbatch_optionsbatch={0};intunknown_type=0;
@@ -701,45 +703,74 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)batch.buffer_output=-1;argc=parse_options(argc,argv,prefix,options,usage,0);-if(argc&&batch.enabled)-usage_with_options(usage,options);-if(opt=='b'){-batch.all_objects=1;-}elseif(opt){-if(batch.enabled&&(opt=='c'||opt=='w'))-batch.cmdmode=opt;-elseif(argc==1)-obj_name=argv[0];-else-usage_with_options(usage,options);-}elseif(!opt&&!batch.enabled){-if(argc==2){-exp_type=argv[0];-obj_name=argv[1];-}else-usage_with_options(usage,options);-}elseif(batch.enabled&&batch.cmdmode!=opt)-usage_with_options(usage,options);+opt_cw=(opt=='c'||opt=='w');+opt_epts=(opt=='e'||opt=='p'||opt=='t'||opt=='s');-if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){-usage_with_options(usage,options);-}--if(force_path&&opt!='c'&&opt!='w'){-error("--path=<path> needs --textconv or --filters");-usage_with_options(usage,options);-}+/* --batch-all-objects? */+if(opt=='b')+batch.all_objects=1;-if(force_path&&batch.enabled){-error("--path=<path> incompatible with --batch");-usage_with_options(usage,options);-}+/* Option compatibility */+if(force_path&&!opt_cw)+usage_msg_optf(_("'%s=<%s> needs '%s' or '%s'"),+usage,options,+"--path",_("path|tree-ish"),"--filters",+"--textconv");+/* Option compatibility with batch mode */+if(batch.enabled)+;+elseif(batch.follow_symlinks)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--follow_symlinks");+elseif(batch.buffer_output>=0)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--buffer");+elseif(batch.all_objects)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--batch-all_objects");++/* Batch defaults */if(batch.buffer_output<0)batch.buffer_output=batch.all_objects;-if(batch.enabled)+/* Return early if we're in batch mode? */+if(batch.enabled){+if(opt_cw)+batch.cmdmode=opt;+elseif(opt&&opt!='b')+usage_msg_optf(_("'-%c' is incompatible with batch mode"),+usage,options,opt);+elseif(argc)+usage_msg_opt(_("batch modes take no arguments"),usage,+options);+returnbatch_objects(&batch);+}++if(opt){+if(!argc&&opt=='c')+usage_msg_optf(_("<rev> required with '%s'"),+usage,options,"--textconv");+elseif(!argc&&opt=='w')+usage_msg_optf(_("<rev> required with '%s'"),+usage,options,"--filters");+elseif(!argc&&opt_epts)+usage_msg_optf(_("<object> required with '-%c'"),+usage,options,opt);+elseif(argc==1)+obj_name=argv[0];+else+usage_msg_opt(_("too many arguments"),usage,options);+}elseif(!argc){+usage_with_options(usage,options);+}elseif(argc!=2){+usage_msg_optf(_("only two arguments allowed in <type> <object> mode, not %d"),+usage,options,argc);+}elseif(argc){+exp_type=argv[0];+obj_name=argv[1];+}if(unknown_type&&opt!='t'&&opt!='s')die("git cat-file --allow-unknown-type: use with -s or -t");
@@ -34,48 +34,54 @@ do'done+test_missing_usage(){+test_expect_code129"$@"2>err&&+grep-E"^fatal:.*required"err+}+short_modes="-e -p -t -s"cw_modes="--textconv --filters"foroptin$cw_modesdotest_expect_success"usage: $opt requires another option"'-test_expect_code129gitcat-file$opt+test_missing_usagegitcat-file$opt'doneforoptin$short_modesdotest_expect_success"usage: $opt requires another option"'-test_expect_code129gitcat-file$opt+test_missing_usagegitcat-file$opt'foropt2in--batch\--batch-check\---follow-symlinks+--follow-symlinks\+"--path=foo HEAD:some-path.txt"do-test_expect_failure"usage: incompatible options: $opt and $opt2"'+test_expect_success"usage: incompatible options: $opt and $opt2"'test_incompatible_usagegitcat-file$opt$opt2'done--opt2="--path=foo HEAD:some-path.txt"-test_expect_success"usage: incompatible options: $opt and $opt2"'-test_incompatible_usagegitcat-file$opt$opt2-'done+test_too_many_arguments(){+test_expect_code129"$@"2>err&&+grep-E"^fatal: too many arguments$"err+}+foroptin$short_modes$cw_modesdoargs="one two three"test_expect_success"usage: too many arguments: $opt$args"'-test_expect_code129gitcat-file$opt$args+test_too_many_argumentsgitcat-file$opt$args'foropt2in--buffer--follow-symlinksdotest_expect_success"usage: incompatible arguments: $opt with batch option $opt2"'-test_expect_code129gitcat-file$opt$opt2+test_incompatible_usagegitcat-file$opt$opt2'donedone
@@ -84,14 +90,9 @@ for opt in --buffer \--follow-symlinks\--batch-all-objectsdo-status=success-iftest$opt="--buffer"-then-status=failure-fi-test_expect_$status"usage: bad option combination: $opt without batch mode"'-test_expect_code129gitcat-file$opt&&-test_expect_code129gitcat-file$optcommitHEAD+test_expect_success"usage: bad option combination: $opt without batch mode"'+test_incompatible_usagegitcat-file$opt&&+test_incompatible_usagegitcat-file$optcommitHEAD'done
Change the cat_one_file() logic that calls get_oid_with_context()
under --textconv and --filters to use the GET_OID_ONLY_TO_DIE flag,
thus improving the error messaging emitted when e.g. <path> is missing
but <rev> is not.
To service the "cat-file" use-case we need to introduce a new
"GET_OID_REQUIRE_PATH" flag, otherwise it would exit early as soon as
a valid "HEAD" was resolved, but in the "cat-file" case being changed
we always need a valid revision and path.
This arguably makes the "<bad rev>:<bad path>" and "<bad
rev>:<good (in HEAD) path>" use cases worse, as we won't quote the
<path> component at the user anymore, but let's just use the existing
logic "git log" et al use for now. We can improve the messaging for
those cases as a follow-up for all callers.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 15 +++++----------
cache.h | 1 +
object-name.c | 6 +++++-
t/t8007-cat-file-textconv.sh | 8 ++++----
4 files changed, 15 insertions(+), 15 deletions(-)
Stop having GET_OID_ONLY_TO_DIE imply GET_OID_QUIETLY in
get_oid_with_context_1().
The *_DIE flag was added in 33bd598c390 (sha1_name.c: teach lookup
context to get_sha1_with_context(), 2012-07-02), and then later
tweaked in 7243ffdd78d (get_sha1: avoid repeating ourselves via
ONLY_TO_DIE, 2016-09-26).
Everything in that commit makes sense, but only for callers that
expect to fail in an initial call to get_oid_with_context_1(), e.g. as
"git show 0017" does via handle_revision_arg(), and then would like to
call get_oid_with_context_1() again via this
maybe_die_on_misspelt_object_name() function.
In the subsequent commit we'll add a new caller that expects to call
this only once, but who would still like to have all the error
messaging that GET_OID_ONLY_TO_DIE gives it, in addition to any
regular errors.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
object-name.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
Change the usage output emitted on "git cat-file -h" to group related
options, making it clear to users which options go with which other
ones.
The new output is:
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
Batch objects requested on stdin (or --batch-all-objects)
--batch[=<format>] show full <object> or <rev> contents
--batch-check[=<format>]
like --batch, but don't emit <contents>
--batch-all-objects with --batch[-check]: ignores stdin, batches all known objects
Change or optimize batch output
--buffer buffer --batch output
--follow-symlinks follow in-tree symlinks
--unordered do not order objects before emitting them
Emit object (blob or tree) with conversion or filter (stand-alone, or with batch)
--textconv run textconv on object's content
--filters run filters on object's content
--path blob|tree use a <path> for (--textconv | --filters ); Not with 'batch'
The old usage was:
<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
--batch-all-objects show all objects with --batch or --batch-check
--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)
--unordered do not order --batch-all-objects output
While shorter, I think the new one is easier to understand, as
e.g. "--allow-unknown-type" is grouped with "-t" and "-s", as it can
only be combined with those options. The same goes for "--buffer",
"--unordered" etc.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 49 +++++++++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 20 deletions(-)
@@ -666,35 +666,44 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)NULL};conststructoptionoptions[]={-OPT_GROUP(N_("<type> can be one of: blob, tree, commit, tag")),-OPT_CMDMODE('t',NULL,&opt,N_("show object type"),'t'),-OPT_CMDMODE('s',NULL,&opt,N_("show object size"),'s'),+/* Simple queries */+OPT_GROUP(N_("Check object existence or emit object contents")),OPT_CMDMODE('e',NULL,&opt,-N_("exit with zero when there's no error"),'e'),-OPT_CMDMODE('p',NULL,&opt,N_("pretty-print object's content"),'p'),-OPT_CMDMODE(0,"textconv",&opt,-N_("for blob objects, run textconv on object's content"),'c'),-OPT_CMDMODE(0,"filters",&opt,-N_("for blob objects, run filters on object's content"),'w'),-OPT_CMDMODE(0,"batch-all-objects",&opt,-N_("show all objects with --batch or --batch-check"),'b'),-OPT_STRING(0,"path",&force_path,N_("blob"),-N_("use a specific path for --textconv/--filters")),+N_("check if <object> exists"),'e'),+OPT_CMDMODE('p',NULL,&opt,N_("pretty-print <object> content"),'p'),++OPT_GROUP(N_("Emit [broken] object attributes")),+OPT_CMDMODE('t',NULL,&opt,N_("show object type (one of 'blob', 'tree', 'commit', 'tag', ...)"),'t'),+OPT_CMDMODE('s',NULL,&opt,N_("show object size"),'s'),OPT_BOOL(0,"allow-unknown-type",&unknown_type,N_("allow -s and -t to work with broken/corrupt objects")),-OPT_BOOL(0,"buffer",&batch.buffer_output,N_("buffer --batch output")),-OPT_CALLBACK_F(0,"batch",&batch,"format",-N_("show info and content of objects fed from the standard input"),+/* Batch mode */+OPT_GROUP(N_("Batch objects requested on stdin (or --batch-all-objects)")),+OPT_CALLBACK_F(0,"batch",&batch,N_("format"),+N_("show full <object> or <rev> contents"),PARSE_OPT_OPTARG|PARSE_OPT_NONEG,batch_option_callback),-OPT_CALLBACK_F(0,"batch-check",&batch,"format",-N_("show info about objects fed from the standard input"),+OPT_CALLBACK_F(0,"batch-check",&batch,N_("format"),+N_("like --batch, but don't emit <contents>"),PARSE_OPT_OPTARG|PARSE_OPT_NONEG,batch_option_callback),+OPT_CMDMODE(0,"batch-all-objects",&opt,+N_("with --batch[-check]: ignores stdin, batches all known objects"),'b'),+/* Batch-specific options */+OPT_GROUP(N_("Change or optimize batch output")),+OPT_BOOL(0,"buffer",&batch.buffer_output,N_("buffer --batch output")),OPT_BOOL(0,"follow-symlinks",&batch.follow_symlinks,-N_("follow in-tree symlinks (used with --batch or --batch-check)")),+N_("follow in-tree symlinks")),OPT_BOOL(0,"unordered",&batch.unordered,-N_("do not order --batch-all-objects output")),+N_("do not order objects before emitting them")),+/* Textconv options, stand-ole*/+OPT_GROUP(N_("Emit object (blob or tree) with conversion or filter (stand-alone, or with batch)")),+OPT_CMDMODE(0,"textconv",&opt,+N_("run textconv on object's content"),'c'),+OPT_CMDMODE(0,"filters",&opt,+N_("run filters on object's content"),'w'),+OPT_STRING(0,"path",&force_path,N_("blob|tree"),+N_("use a <path> for (--textconv | --filters ); Not with 'batch'")),OPT_END()};
Stress test the usage emitted when options are combined in ways that
isn't supported. Let's test various option combinations, some of these
we buggily allow right now.
E.g. this reveals a bug in 321459439e1 (cat-file: support
--textconv/--filters in batch mode, 2016-09-09) that we'll fix in a
subsequent commit. We're supposed to be emitting a relevant message
when --batch-all-objects is combined with --textconv or --filters, but
we don't.
The cases of needing to assign to opt=2 in the "opt" loop are because
on those we do the right thing already, in subsequent commits the
"test_expect_failure" cases will be fixed, and the for-loops unified.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t1006-cat-file.sh | 94 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
Add tests for the output that's emitted when we disambiguate
<obj>:<path> in cat-file. This gives us a baseline for improving these
messages.
For e.g. "git blame" we'll emit:
$ git blame HEAD:foo
fatal: no such path 'HEAD:foo' in HEAD
But cat-file doesn't disambiguate these two cases, and just gives the
rather unhelpful:
$ git cat-file --textconv HEAD:foo
fatal: Not a valid object name HEAD:foo
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t8007-cat-file-textconv.sh | 42 ++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
There were various inaccuracies in the previous SYNOPSIS output,
e.g. "--path" is not something that can optionally go with any options
except --textconv or --filters, as the output implied.
The opening line of the DESCRIPTION section is also "In its first
form[...]", which refers to "git cat-file <type> <object>", but the
SYNOPSIS section wasn't showing that as the first form!
That part of the documentation made sense in
d83a42f34a6 (Documentation: minor grammatical fixes in
git-cat-file.txt, 2009-03-22) when it was introduced, but since then
various options that were added have made that intro make no sense in
the context it was in. Now the two will match again.
The usage output here is not properly aligned on "master" currently,
but will be with my in-flight 4631cfc20bd (parse-options: properly
align continued usage output, 2021-09-21), so let's indent things
correctly in the C code in anticipation of that.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Documentation/git-cat-file.txt | 10 ++++++++--
builtin/cat-file.c | 10 ++++++++--
2 files changed, 16 insertions(+), 4 deletions(-)
Add a usage_msg_optf() as a shorthand for the sort of
usage_msg_opt(xstrfmt(...)) used in builtin/stash.c. I'll make more
use of this function in builtin/cat-file.c shortly.
The disconnect between the "..." and "fmt" is a bit unusual, but it
works just fine and this keeps it consistent with usage_msg_opt(),
i.e. a caller of it can be moved to usage_msg_optf() and not have to
have its arguments re-arranged.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/stash.c | 4 ++--
parse-options.c | 13 +++++++++++++
parse-options.h | 10 ++++++++++
3 files changed, 25 insertions(+), 2 deletions(-)
There's no benefit to defining this at a distance, and it makes the
code harder to read as you've got to scroll up to see the usage that
corresponds to the options.
In subsequent commits I'll make use of usage_msg_opt(), which will be
quite noisy if I have to use the long "cat_file_usage" variable,
there's no other command being defined in this file, so let's rename
it to just "usage".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 37 ++++++++++++++++++-------------------
1 file changed, 18 insertions(+), 19 deletions(-)
@@ -708,35 +707,35 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)elseif(argc==1)obj_name=argv[0];else-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(!opt&&!batch.enabled){if(argc==2){exp_type=argv[0];obj_name=argv[1];}else-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(batch.enabled){if(batch.cmdmode!=opt||argc)-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);if(batch.cmdmode&&batch.all_objects)die("--batch-all-objects cannot be combined with ""--textconv nor with --filters");}if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(force_path&&opt!='c'&&opt!='w'){error("--path=<path> needs --textconv or --filters");-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(force_path&&batch.enabled){error("--path=<path> incompatible with --batch");-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(batch.buffer_output<0)
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.
By giving parse_options() information about what options are mutually
exclusive with one another we can get the die() message being removed
here for free, we didn't even use that removed message in some cases,
e.g. for both of:
--batch-all-objects --textconv
--batch-all-objects --filters
We'd take the "goto usage" in the "if (opt)" branch, and never reach
the previous message. Now we'll emit e.g.:
$ git cat-file --batch-all-objects --filters
error: option `filters' is incompatible with --batch-all-objects
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 <redacted>
---
builtin/cat-file.c | 25 +++++++++++--------------
t/t1006-cat-file.sh | 7 ++-----
2 files changed, 13 insertions(+), 19 deletions(-)
@@ -674,6 +674,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)N_("for blob objects, run textconv on object's content"),'c'),OPT_CMDMODE(0,"filters",&opt,N_("for blob objects, run filters on object's content"),'w'),+OPT_CMDMODE(0,"batch-all-objects",&opt,+N_("show all objects with --batch or --batch-check"),'b'),OPT_STRING(0,"path",&force_path,N_("blob"),N_("use a specific path for --textconv/--filters")),OPT_BOOL(0,"allow-unknown-type",&unknown_type,
@@ -689,8 +691,6 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)batch_option_callback),OPT_BOOL(0,"follow-symlinks",&batch.follow_symlinks,N_("follow in-tree symlinks (used with --batch or --batch-check)")),-OPT_BOOL(0,"batch-all-objects",&batch.all_objects,-N_("show all objects with --batch or --batch-check")),OPT_BOOL(0,"unordered",&batch.unordered,N_("do not order --batch-all-objects output")),OPT_END()
@@ -699,30 +699,27 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)git_config(git_cat_file_config,NULL);batch.buffer_output=-1;-argc=parse_options(argc,argv,prefix,options,usage,0);-if(opt){+argc=parse_options(argc,argv,prefix,options,usage,0);+if(argc&&batch.enabled)+usage_with_options(usage,options);+if(opt=='b'){+batch.all_objects=1;+}elseif(opt){if(batch.enabled&&(opt=='c'||opt=='w'))batch.cmdmode=opt;elseif(argc==1)obj_name=argv[0];elseusage_with_options(usage,options);-}-if(!opt&&!batch.enabled){+}elseif(!opt&&!batch.enabled){if(argc==2){exp_type=argv[0];obj_name=argv[1];}elseusage_with_options(usage,options);-}-if(batch.enabled){-if(batch.cmdmode!=opt||argc)-usage_with_options(usage,options);-if(batch.cmdmode&&batch.all_objects)-die("--batch-all-objects cannot be combined with "-"--textconv nor with --filters");-}+}elseif(batch.enabled&&batch.cmdmode!=opt)+usage_with_options(usage,options);if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){usage_with_options(usage,options);
With the migration of --batch-all-objects to OPT_CMDMODE() in the
preceding commit one bug with combining it and other OPT_CMDMODE()
options was solved, but we were still left with e.g. --buffer silently
being discarded when not in batch mode.
Fix all those bugs, and in addition emit errors telling the user
specifically what options can't be combined with what other options,
before this we'd usually just emit the cryptic usage text and leave
the users to work it out by themselves.
This change is rather large, because to do so we need to untangle the
options processing so that we can not only error out, but emit
sensible errors, and e.g. emit errors about options before errors
about stray argc elements (as they might become valid if the option
were removed).
Some of the output changes ("error:" to "fatal:" with
usage_msg_opt[f]()), but none of the exit codes change, except in
those cases where we silently accepted bad option combinations before,
now we'll error out.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 95 ++++++++++++++++++++++++++++++---------------
t/t1006-cat-file.sh | 41 +++++++++----------
2 files changed, 84 insertions(+), 52 deletions(-)
@@ -648,6 +648,8 @@ static int batch_option_callback(const struct option *opt,intcmd_cat_file(intargc,constchar**argv,constchar*prefix){intopt=0;+intopt_cw=0;+intopt_epts=0;constchar*exp_type=NULL,*obj_name=NULL;structbatch_optionsbatch={0};intunknown_type=0;
@@ -701,45 +703,74 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)batch.buffer_output=-1;argc=parse_options(argc,argv,prefix,options,usage,0);-if(argc&&batch.enabled)-usage_with_options(usage,options);-if(opt=='b'){-batch.all_objects=1;-}elseif(opt){-if(batch.enabled&&(opt=='c'||opt=='w'))-batch.cmdmode=opt;-elseif(argc==1)-obj_name=argv[0];-else-usage_with_options(usage,options);-}elseif(!opt&&!batch.enabled){-if(argc==2){-exp_type=argv[0];-obj_name=argv[1];-}else-usage_with_options(usage,options);-}elseif(batch.enabled&&batch.cmdmode!=opt)-usage_with_options(usage,options);+opt_cw=(opt=='c'||opt=='w');+opt_epts=(opt=='e'||opt=='p'||opt=='t'||opt=='s');-if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){-usage_with_options(usage,options);-}--if(force_path&&opt!='c'&&opt!='w'){-error("--path=<path> needs --textconv or --filters");-usage_with_options(usage,options);-}+/* --batch-all-objects? */+if(opt=='b')+batch.all_objects=1;-if(force_path&&batch.enabled){-error("--path=<path> incompatible with --batch");-usage_with_options(usage,options);-}+/* Option compatibility */+if(force_path&&!opt_cw)+usage_msg_optf(_("'%s=<%s>' needs '%s' or '%s'"),+usage,options,+"--path",_("path|tree-ish"),"--filters",+"--textconv");+/* Option compatibility with batch mode */+if(batch.enabled)+;+elseif(batch.follow_symlinks)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--follow_symlinks");+elseif(batch.buffer_output>=0)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--buffer");+elseif(batch.all_objects)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--batch-all_objects");++/* Batch defaults */if(batch.buffer_output<0)batch.buffer_output=batch.all_objects;-if(batch.enabled)+/* Return early if we're in batch mode? */+if(batch.enabled){+if(opt_cw)+batch.cmdmode=opt;+elseif(opt&&opt!='b')+usage_msg_optf(_("'-%c' is incompatible with batch mode"),+usage,options,opt);+elseif(argc)+usage_msg_opt(_("batch modes take no arguments"),usage,+options);+returnbatch_objects(&batch);+}++if(opt){+if(!argc&&opt=='c')+usage_msg_optf(_("<rev> required with '%s'"),+usage,options,"--textconv");+elseif(!argc&&opt=='w')+usage_msg_optf(_("<rev> required with '%s'"),+usage,options,"--filters");+elseif(!argc&&opt_epts)+usage_msg_optf(_("<object> required with '-%c'"),+usage,options,opt);+elseif(argc==1)+obj_name=argv[0];+else+usage_msg_opt(_("too many arguments"),usage,options);+}elseif(!argc){+usage_with_options(usage,options);+}elseif(argc!=2){+usage_msg_optf(_("only two arguments allowed in <type> <object> mode, not %d"),+usage,options,argc);+}elseif(argc){+exp_type=argv[0];+obj_name=argv[1];+}if(unknown_type&&opt!='t'&&opt!='s')die("git cat-file --allow-unknown-type: use with -s or -t");
@@ -34,48 +34,54 @@ do'done+test_missing_usage(){+test_expect_code129"$@"2>err&&+grep-E"^fatal:.*required"err+}+short_modes="-e -p -t -s"cw_modes="--textconv --filters"foroptin$cw_modesdotest_expect_success"usage: $opt requires another option"'-test_expect_code129gitcat-file$opt+test_missing_usagegitcat-file$opt'doneforoptin$short_modesdotest_expect_success"usage: $opt requires another option"'-test_expect_code129gitcat-file$opt+test_missing_usagegitcat-file$opt'foropt2in--batch\--batch-check\---follow-symlinks+--follow-symlinks\+"--path=foo HEAD:some-path.txt"do-test_expect_failure"usage: incompatible options: $opt and $opt2"'+test_expect_success"usage: incompatible options: $opt and $opt2"'test_incompatible_usagegitcat-file$opt$opt2'done--opt2="--path=foo HEAD:some-path.txt"-test_expect_success"usage: incompatible options: $opt and $opt2"'-test_incompatible_usagegitcat-file$opt$opt2-'done+test_too_many_arguments(){+test_expect_code129"$@"2>err&&+grep-E"^fatal: too many arguments$"err+}+foroptin$short_modes$cw_modesdoargs="one two three"test_expect_success"usage: too many arguments: $opt$args"'-test_expect_code129gitcat-file$opt$args+test_too_many_argumentsgitcat-file$opt$args'foropt2in--buffer--follow-symlinksdotest_expect_success"usage: incompatible arguments: $opt with batch option $opt2"'-test_expect_code129gitcat-file$opt$opt2+test_incompatible_usagegitcat-file$opt$opt2'donedone
@@ -84,14 +90,9 @@ for opt in --buffer \--follow-symlinks\--batch-all-objectsdo-status=success-iftest$opt="--buffer"-then-status=failure-fi-test_expect_$status"usage: bad option combination: $opt without batch mode"'-test_expect_code129gitcat-file$opt&&-test_expect_code129gitcat-file$optcommitHEAD+test_expect_success"usage: bad option combination: $opt without batch mode"'+test_incompatible_usagegitcat-file$opt&&+test_incompatible_usagegitcat-file$optcommitHEAD'done
Change the usage output emitted on "git cat-file -h" to group related
options, making it clear to users which options go with which other
ones.
The new output is:
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
Batch objects requested on stdin (or --batch-all-objects)
--batch[=<format>] show full <object> or <rev> contents
--batch-check[=<format>]
like --batch, but don't emit <contents>
--batch-all-objects with --batch[-check]: ignores stdin, batches all known objects
Change or optimize batch output
--buffer buffer --batch output
--follow-symlinks follow in-tree symlinks
--unordered do not order objects before emitting them
Emit object (blob or tree) with conversion or filter (stand-alone, or with batch)
--textconv run textconv on object's content
--filters run filters on object's content
--path blob|tree use a <path> for (--textconv | --filters ); Not with 'batch'
The old usage was:
<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
--batch-all-objects show all objects with --batch or --batch-check
--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)
--unordered do not order --batch-all-objects output
While shorter, I think the new one is easier to understand, as
e.g. "--allow-unknown-type" is grouped with "-t" and "-s", as it can
only be combined with those options. The same goes for "--buffer",
"--unordered" etc.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 49 +++++++++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 20 deletions(-)
@@ -666,35 +666,44 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)NULL};conststructoptionoptions[]={-OPT_GROUP(N_("<type> can be one of: blob, tree, commit, tag")),-OPT_CMDMODE('t',NULL,&opt,N_("show object type"),'t'),-OPT_CMDMODE('s',NULL,&opt,N_("show object size"),'s'),+/* Simple queries */+OPT_GROUP(N_("Check object existence or emit object contents")),OPT_CMDMODE('e',NULL,&opt,-N_("exit with zero when there's no error"),'e'),-OPT_CMDMODE('p',NULL,&opt,N_("pretty-print object's content"),'p'),-OPT_CMDMODE(0,"textconv",&opt,-N_("for blob objects, run textconv on object's content"),'c'),-OPT_CMDMODE(0,"filters",&opt,-N_("for blob objects, run filters on object's content"),'w'),-OPT_CMDMODE(0,"batch-all-objects",&opt,-N_("show all objects with --batch or --batch-check"),'b'),-OPT_STRING(0,"path",&force_path,N_("blob"),-N_("use a specific path for --textconv/--filters")),+N_("check if <object> exists"),'e'),+OPT_CMDMODE('p',NULL,&opt,N_("pretty-print <object> content"),'p'),++OPT_GROUP(N_("Emit [broken] object attributes")),+OPT_CMDMODE('t',NULL,&opt,N_("show object type (one of 'blob', 'tree', 'commit', 'tag', ...)"),'t'),+OPT_CMDMODE('s',NULL,&opt,N_("show object size"),'s'),OPT_BOOL(0,"allow-unknown-type",&unknown_type,N_("allow -s and -t to work with broken/corrupt objects")),-OPT_BOOL(0,"buffer",&batch.buffer_output,N_("buffer --batch output")),-OPT_CALLBACK_F(0,"batch",&batch,"format",-N_("show info and content of objects fed from the standard input"),+/* Batch mode */+OPT_GROUP(N_("Batch objects requested on stdin (or --batch-all-objects)")),+OPT_CALLBACK_F(0,"batch",&batch,N_("format"),+N_("show full <object> or <rev> contents"),PARSE_OPT_OPTARG|PARSE_OPT_NONEG,batch_option_callback),-OPT_CALLBACK_F(0,"batch-check",&batch,"format",-N_("show info about objects fed from the standard input"),+OPT_CALLBACK_F(0,"batch-check",&batch,N_("format"),+N_("like --batch, but don't emit <contents>"),PARSE_OPT_OPTARG|PARSE_OPT_NONEG,batch_option_callback),+OPT_CMDMODE(0,"batch-all-objects",&opt,+N_("with --batch[-check]: ignores stdin, batches all known objects"),'b'),+/* Batch-specific options */+OPT_GROUP(N_("Change or optimize batch output")),+OPT_BOOL(0,"buffer",&batch.buffer_output,N_("buffer --batch output")),OPT_BOOL(0,"follow-symlinks",&batch.follow_symlinks,-N_("follow in-tree symlinks (used with --batch or --batch-check)")),+N_("follow in-tree symlinks")),OPT_BOOL(0,"unordered",&batch.unordered,-N_("do not order --batch-all-objects output")),+N_("do not order objects before emitting them")),+/* Textconv options, stand-ole*/+OPT_GROUP(N_("Emit object (blob or tree) with conversion or filter (stand-alone, or with batch)")),+OPT_CMDMODE(0,"textconv",&opt,+N_("run textconv on object's content"),'c'),+OPT_CMDMODE(0,"filters",&opt,+N_("run filters on object's content"),'w'),+OPT_STRING(0,"path",&force_path,N_("blob|tree"),+N_("use a <path> for (--textconv | --filters ); Not with 'batch'")),OPT_END()};
Stop having GET_OID_ONLY_TO_DIE imply GET_OID_QUIETLY in
get_oid_with_context_1().
The *_DIE flag was added in 33bd598c390 (sha1_name.c: teach lookup
context to get_sha1_with_context(), 2012-07-02), and then later
tweaked in 7243ffdd78d (get_sha1: avoid repeating ourselves via
ONLY_TO_DIE, 2016-09-26).
Everything in that commit makes sense, but only for callers that
expect to fail in an initial call to get_oid_with_context_1(), e.g. as
"git show 0017" does via handle_revision_arg(), and then would like to
call get_oid_with_context_1() again via this
maybe_die_on_misspelt_object_name() function.
In the subsequent commit we'll add a new caller that expects to call
this only once, but who would still like to have all the error
messaging that GET_OID_ONLY_TO_DIE gives it, in addition to any
regular errors.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
object-name.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
Change the cat_one_file() logic that calls get_oid_with_context()
under --textconv and --filters to use the GET_OID_ONLY_TO_DIE flag,
thus improving the error messaging emitted when e.g. <path> is missing
but <rev> is not.
To service the "cat-file" use-case we need to introduce a new
"GET_OID_REQUIRE_PATH" flag, otherwise it would exit early as soon as
a valid "HEAD" was resolved, but in the "cat-file" case being changed
we always need a valid revision and path.
This arguably makes the "<bad rev>:<bad path>" and "<bad
rev>:<good (in HEAD) path>" use cases worse, as we won't quote the
<path> component at the user anymore, but let's just use the existing
logic "git log" et al use for now. We can improve the messaging for
those cases as a follow-up for all callers.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 16 ++++++----------
cache.h | 1 +
object-name.c | 3 +++
t/t8007-cat-file-textconv.sh | 8 ++++----
4 files changed, 14 insertions(+), 14 deletions(-)
On Dec 8, 2021, at 7:34 AM, Ævar Arnfjörð Bjarmason [off-list ref] wrote:
With the migration of --batch-all-objects to OPT_CMDMODE() in the
preceding commit one bug with combining it and other OPT_CMDMODE()
options was solved, but we were still left with e.g. --buffer silently
being discarded when not in batch mode.
Fix all those bugs, and in addition emit errors telling the user
specifically what options can't be combined with what other options,
before this we'd usually just emit the cryptic usage text and leave
the users to work it out by themselves.
This change is rather large, because to do so we need to untangle the
options processing so that we can not only error out, but emit
sensible errors, and e.g. emit errors about options before errors
about stray argc elements (as they might become valid if the option
were removed).
Some of the output changes ("error:" to "fatal:" with
usage_msg_opt[f]()), but none of the exit codes change, except in
those cases where we silently accepted bad option combinations before,
now we'll error out.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 95 ++++++++++++++++++++++++++++++---------------
t/t1006-cat-file.sh | 41 +++++++++----------
2 files changed, 84 insertions(+), 52 deletions(-)
This series of patches to cat-file significantly improves the UX of
the -h output, see 08/10. For the v4 see[1], for the new usage output
see [2].
This re-roll addresses a minor s/_/-/ in option name typo out by John
Cai in his review of the series.
1. https://lore.kernel.org/git/cover-v4-00.10-00000000000-20211208T123151Z-avarab@gmail.com/
2. https://lore.kernel.org/git/patch-v5-08.10-16b6bb8aaf2-20211222T041050Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (10):
cat-file tests: test bad usage
cat-file tests: test messaging on bad objects/paths
parse-options API: add a usage_msg_optf()
cat-file docs: fix SYNOPSIS and "-h" output
cat-file: move "usage" variable to cmd_cat_file()
cat-file: make --batch-all-objects a CMDMODE
cat-file: fix remaining usage bugs
cat-file: correct and improve usage information
object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY
cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters)
Documentation/git-cat-file.txt | 10 +-
builtin/cat-file.c | 182 ++++++++++++++++++++-------------
builtin/stash.c | 4 +-
cache.h | 1 +
object-name.c | 8 +-
parse-options.c | 13 +++
parse-options.h | 10 ++
t/t1006-cat-file.sh | 92 +++++++++++++++++
t/t8007-cat-file-textconv.sh | 42 ++++++++
9 files changed, 282 insertions(+), 80 deletions(-)
Range-diff against v4:
1: b3d8ec1697f = 1: e771bd38792 cat-file tests: test bad usage
2: eb6fa584287 = 2: 291312e2fb5 cat-file tests: test messaging on bad objects/paths
3: 01de6e4305f = 3: 0689dbb248c parse-options API: add a usage_msg_optf()
4: aa384803fef = 4: 2a28b39430e cat-file docs: fix SYNOPSIS and "-h" output
5: 32365ff569b = 5: 2d90c12fe7b cat-file: move "usage" variable to cmd_cat_file()
6: 473ea3b0394 = 6: 227805d1804 cat-file: make --batch-all-objects a CMDMODE
7: 878d9052bfb ! 7: e6ea403efe0 cat-file: fix remaining usage bugs
@@ builtin/cat-file.c: int cmd_cat_file(int argc, const char **argv, const char *pr
+ "--buffer");
+ else if (batch.all_objects)
+ usage_msg_optf(_("'%s' requires a batch mode"), usage, options,
-+ "--batch-all_objects");
++ "--batch-all-objects");
+
+ /* Batch defaults */
if (batch.buffer_output < 0)
8: ebc8dd0a22e = 8: 16b6bb8aaf2 cat-file: correct and improve usage information
9: a7447510e4b = 9: 47543c57135 object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY
10: a658099e3e1 = 10: 63920969ca8 cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters)
--
2.34.1.1146.gb52885e7c44
Add tests for the output that's emitted when we disambiguate
<obj>:<path> in cat-file. This gives us a baseline for improving these
messages.
For e.g. "git blame" we'll emit:
$ git blame HEAD:foo
fatal: no such path 'HEAD:foo' in HEAD
But cat-file doesn't disambiguate these two cases, and just gives the
rather unhelpful:
$ git cat-file --textconv HEAD:foo
fatal: Not a valid object name HEAD:foo
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t8007-cat-file-textconv.sh | 42 ++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
Stress test the usage emitted when options are combined in ways that
isn't supported. Let's test various option combinations, some of these
we buggily allow right now.
E.g. this reveals a bug in 321459439e1 (cat-file: support
--textconv/--filters in batch mode, 2016-09-09) that we'll fix in a
subsequent commit. We're supposed to be emitting a relevant message
when --batch-all-objects is combined with --textconv or --filters, but
we don't.
The cases of needing to assign to opt=2 in the "opt" loop are because
on those we do the right thing already, in subsequent commits the
"test_expect_failure" cases will be fixed, and the for-loops unified.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t1006-cat-file.sh | 94 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
Add a usage_msg_optf() as a shorthand for the sort of
usage_msg_opt(xstrfmt(...)) used in builtin/stash.c. I'll make more
use of this function in builtin/cat-file.c shortly.
The disconnect between the "..." and "fmt" is a bit unusual, but it
works just fine and this keeps it consistent with usage_msg_opt(),
i.e. a caller of it can be moved to usage_msg_optf() and not have to
have its arguments re-arranged.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/stash.c | 4 ++--
parse-options.c | 13 +++++++++++++
parse-options.h | 10 ++++++++++
3 files changed, 25 insertions(+), 2 deletions(-)
There were various inaccuracies in the previous SYNOPSIS output,
e.g. "--path" is not something that can optionally go with any options
except --textconv or --filters, as the output implied.
The opening line of the DESCRIPTION section is also "In its first
form[...]", which refers to "git cat-file <type> <object>", but the
SYNOPSIS section wasn't showing that as the first form!
That part of the documentation made sense in
d83a42f34a6 (Documentation: minor grammatical fixes in
git-cat-file.txt, 2009-03-22) when it was introduced, but since then
various options that were added have made that intro make no sense in
the context it was in. Now the two will match again.
The usage output here is not properly aligned on "master" currently,
but will be with my in-flight 4631cfc20bd (parse-options: properly
align continued usage output, 2021-09-21), so let's indent things
correctly in the C code in anticipation of that.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Documentation/git-cat-file.txt | 10 ++++++++--
builtin/cat-file.c | 10 ++++++++--
2 files changed, 16 insertions(+), 4 deletions(-)
There's no benefit to defining this at a distance, and it makes the
code harder to read as you've got to scroll up to see the usage that
corresponds to the options.
In subsequent commits I'll make use of usage_msg_opt(), which will be
quite noisy if I have to use the long "cat_file_usage" variable,
there's no other command being defined in this file, so let's rename
it to just "usage".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 37 ++++++++++++++++++-------------------
1 file changed, 18 insertions(+), 19 deletions(-)
@@ -708,35 +707,35 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)elseif(argc==1)obj_name=argv[0];else-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(!opt&&!batch.enabled){if(argc==2){exp_type=argv[0];obj_name=argv[1];}else-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(batch.enabled){if(batch.cmdmode!=opt||argc)-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);if(batch.cmdmode&&batch.all_objects)die("--batch-all-objects cannot be combined with ""--textconv nor with --filters");}if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(force_path&&opt!='c'&&opt!='w'){error("--path=<path> needs --textconv or --filters");-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(force_path&&batch.enabled){error("--path=<path> incompatible with --batch");-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(batch.buffer_output<0)
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.
By giving parse_options() information about what options are mutually
exclusive with one another we can get the die() message being removed
here for free, we didn't even use that removed message in some cases,
e.g. for both of:
--batch-all-objects --textconv
--batch-all-objects --filters
We'd take the "goto usage" in the "if (opt)" branch, and never reach
the previous message. Now we'll emit e.g.:
$ git cat-file --batch-all-objects --filters
error: option `filters' is incompatible with --batch-all-objects
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 <redacted>
---
builtin/cat-file.c | 25 +++++++++++--------------
t/t1006-cat-file.sh | 7 ++-----
2 files changed, 13 insertions(+), 19 deletions(-)
@@ -674,6 +674,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)N_("for blob objects, run textconv on object's content"),'c'),OPT_CMDMODE(0,"filters",&opt,N_("for blob objects, run filters on object's content"),'w'),+OPT_CMDMODE(0,"batch-all-objects",&opt,+N_("show all objects with --batch or --batch-check"),'b'),OPT_STRING(0,"path",&force_path,N_("blob"),N_("use a specific path for --textconv/--filters")),OPT_BOOL(0,"allow-unknown-type",&unknown_type,
@@ -689,8 +691,6 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)batch_option_callback),OPT_BOOL(0,"follow-symlinks",&batch.follow_symlinks,N_("follow in-tree symlinks (used with --batch or --batch-check)")),-OPT_BOOL(0,"batch-all-objects",&batch.all_objects,-N_("show all objects with --batch or --batch-check")),OPT_BOOL(0,"unordered",&batch.unordered,N_("do not order --batch-all-objects output")),OPT_END()
@@ -699,30 +699,27 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)git_config(git_cat_file_config,NULL);batch.buffer_output=-1;-argc=parse_options(argc,argv,prefix,options,usage,0);-if(opt){+argc=parse_options(argc,argv,prefix,options,usage,0);+if(argc&&batch.enabled)+usage_with_options(usage,options);+if(opt=='b'){+batch.all_objects=1;+}elseif(opt){if(batch.enabled&&(opt=='c'||opt=='w'))batch.cmdmode=opt;elseif(argc==1)obj_name=argv[0];elseusage_with_options(usage,options);-}-if(!opt&&!batch.enabled){+}elseif(!opt&&!batch.enabled){if(argc==2){exp_type=argv[0];obj_name=argv[1];}elseusage_with_options(usage,options);-}-if(batch.enabled){-if(batch.cmdmode!=opt||argc)-usage_with_options(usage,options);-if(batch.cmdmode&&batch.all_objects)-die("--batch-all-objects cannot be combined with "-"--textconv nor with --filters");-}+}elseif(batch.enabled&&batch.cmdmode!=opt)+usage_with_options(usage,options);if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){usage_with_options(usage,options);
With the migration of --batch-all-objects to OPT_CMDMODE() in the
preceding commit one bug with combining it and other OPT_CMDMODE()
options was solved, but we were still left with e.g. --buffer silently
being discarded when not in batch mode.
Fix all those bugs, and in addition emit errors telling the user
specifically what options can't be combined with what other options,
before this we'd usually just emit the cryptic usage text and leave
the users to work it out by themselves.
This change is rather large, because to do so we need to untangle the
options processing so that we can not only error out, but emit
sensible errors, and e.g. emit errors about options before errors
about stray argc elements (as they might become valid if the option
were removed).
Some of the output changes ("error:" to "fatal:" with
usage_msg_opt[f]()), but none of the exit codes change, except in
those cases where we silently accepted bad option combinations before,
now we'll error out.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 95 ++++++++++++++++++++++++++++++---------------
t/t1006-cat-file.sh | 41 +++++++++----------
2 files changed, 84 insertions(+), 52 deletions(-)
@@ -648,6 +648,8 @@ static int batch_option_callback(const struct option *opt,intcmd_cat_file(intargc,constchar**argv,constchar*prefix){intopt=0;+intopt_cw=0;+intopt_epts=0;constchar*exp_type=NULL,*obj_name=NULL;structbatch_optionsbatch={0};intunknown_type=0;
@@ -701,45 +703,74 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)batch.buffer_output=-1;argc=parse_options(argc,argv,prefix,options,usage,0);-if(argc&&batch.enabled)-usage_with_options(usage,options);-if(opt=='b'){-batch.all_objects=1;-}elseif(opt){-if(batch.enabled&&(opt=='c'||opt=='w'))-batch.cmdmode=opt;-elseif(argc==1)-obj_name=argv[0];-else-usage_with_options(usage,options);-}elseif(!opt&&!batch.enabled){-if(argc==2){-exp_type=argv[0];-obj_name=argv[1];-}else-usage_with_options(usage,options);-}elseif(batch.enabled&&batch.cmdmode!=opt)-usage_with_options(usage,options);+opt_cw=(opt=='c'||opt=='w');+opt_epts=(opt=='e'||opt=='p'||opt=='t'||opt=='s');-if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){-usage_with_options(usage,options);-}--if(force_path&&opt!='c'&&opt!='w'){-error("--path=<path> needs --textconv or --filters");-usage_with_options(usage,options);-}+/* --batch-all-objects? */+if(opt=='b')+batch.all_objects=1;-if(force_path&&batch.enabled){-error("--path=<path> incompatible with --batch");-usage_with_options(usage,options);-}+/* Option compatibility */+if(force_path&&!opt_cw)+usage_msg_optf(_("'%s=<%s>' needs '%s' or '%s'"),+usage,options,+"--path",_("path|tree-ish"),"--filters",+"--textconv");+/* Option compatibility with batch mode */+if(batch.enabled)+;+elseif(batch.follow_symlinks)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--follow_symlinks");+elseif(batch.buffer_output>=0)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--buffer");+elseif(batch.all_objects)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--batch-all-objects");++/* Batch defaults */if(batch.buffer_output<0)batch.buffer_output=batch.all_objects;-if(batch.enabled)+/* Return early if we're in batch mode? */+if(batch.enabled){+if(opt_cw)+batch.cmdmode=opt;+elseif(opt&&opt!='b')+usage_msg_optf(_("'-%c' is incompatible with batch mode"),+usage,options,opt);+elseif(argc)+usage_msg_opt(_("batch modes take no arguments"),usage,+options);+returnbatch_objects(&batch);+}++if(opt){+if(!argc&&opt=='c')+usage_msg_optf(_("<rev> required with '%s'"),+usage,options,"--textconv");+elseif(!argc&&opt=='w')+usage_msg_optf(_("<rev> required with '%s'"),+usage,options,"--filters");+elseif(!argc&&opt_epts)+usage_msg_optf(_("<object> required with '-%c'"),+usage,options,opt);+elseif(argc==1)+obj_name=argv[0];+else+usage_msg_opt(_("too many arguments"),usage,options);+}elseif(!argc){+usage_with_options(usage,options);+}elseif(argc!=2){+usage_msg_optf(_("only two arguments allowed in <type> <object> mode, not %d"),+usage,options,argc);+}elseif(argc){+exp_type=argv[0];+obj_name=argv[1];+}if(unknown_type&&opt!='t'&&opt!='s')die("git cat-file --allow-unknown-type: use with -s or -t");
@@ -34,48 +34,54 @@ do'done+test_missing_usage(){+test_expect_code129"$@"2>err&&+grep-E"^fatal:.*required"err+}+short_modes="-e -p -t -s"cw_modes="--textconv --filters"foroptin$cw_modesdotest_expect_success"usage: $opt requires another option"'-test_expect_code129gitcat-file$opt+test_missing_usagegitcat-file$opt'doneforoptin$short_modesdotest_expect_success"usage: $opt requires another option"'-test_expect_code129gitcat-file$opt+test_missing_usagegitcat-file$opt'foropt2in--batch\--batch-check\---follow-symlinks+--follow-symlinks\+"--path=foo HEAD:some-path.txt"do-test_expect_failure"usage: incompatible options: $opt and $opt2"'+test_expect_success"usage: incompatible options: $opt and $opt2"'test_incompatible_usagegitcat-file$opt$opt2'done--opt2="--path=foo HEAD:some-path.txt"-test_expect_success"usage: incompatible options: $opt and $opt2"'-test_incompatible_usagegitcat-file$opt$opt2-'done+test_too_many_arguments(){+test_expect_code129"$@"2>err&&+grep-E"^fatal: too many arguments$"err+}+foroptin$short_modes$cw_modesdoargs="one two three"test_expect_success"usage: too many arguments: $opt$args"'-test_expect_code129gitcat-file$opt$args+test_too_many_argumentsgitcat-file$opt$args'foropt2in--buffer--follow-symlinksdotest_expect_success"usage: incompatible arguments: $opt with batch option $opt2"'-test_expect_code129gitcat-file$opt$opt2+test_incompatible_usagegitcat-file$opt$opt2'donedone
@@ -84,14 +90,9 @@ for opt in --buffer \--follow-symlinks\--batch-all-objectsdo-status=success-iftest$opt="--buffer"-then-status=failure-fi-test_expect_$status"usage: bad option combination: $opt without batch mode"'-test_expect_code129gitcat-file$opt&&-test_expect_code129gitcat-file$optcommitHEAD+test_expect_success"usage: bad option combination: $opt without batch mode"'+test_incompatible_usagegitcat-file$opt&&+test_incompatible_usagegitcat-file$optcommitHEAD'done
Change the cat_one_file() logic that calls get_oid_with_context()
under --textconv and --filters to use the GET_OID_ONLY_TO_DIE flag,
thus improving the error messaging emitted when e.g. <path> is missing
but <rev> is not.
To service the "cat-file" use-case we need to introduce a new
"GET_OID_REQUIRE_PATH" flag, otherwise it would exit early as soon as
a valid "HEAD" was resolved, but in the "cat-file" case being changed
we always need a valid revision and path.
This arguably makes the "<bad rev>:<bad path>" and "<bad
rev>:<good (in HEAD) path>" use cases worse, as we won't quote the
<path> component at the user anymore, but let's just use the existing
logic "git log" et al use for now. We can improve the messaging for
those cases as a follow-up for all callers.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 16 ++++++----------
cache.h | 1 +
object-name.c | 3 +++
t/t8007-cat-file-textconv.sh | 8 ++++----
4 files changed, 14 insertions(+), 14 deletions(-)
Change the usage output emitted on "git cat-file -h" to group related
options, making it clear to users which options go with which other
ones.
The new output is:
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
Batch objects requested on stdin (or --batch-all-objects)
--batch[=<format>] show full <object> or <rev> contents
--batch-check[=<format>]
like --batch, but don't emit <contents>
--batch-all-objects with --batch[-check]: ignores stdin, batches all known objects
Change or optimize batch output
--buffer buffer --batch output
--follow-symlinks follow in-tree symlinks
--unordered do not order objects before emitting them
Emit object (blob or tree) with conversion or filter (stand-alone, or with batch)
--textconv run textconv on object's content
--filters run filters on object's content
--path blob|tree use a <path> for (--textconv | --filters ); Not with 'batch'
The old usage was:
<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
--batch-all-objects show all objects with --batch or --batch-check
--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)
--unordered do not order --batch-all-objects output
While shorter, I think the new one is easier to understand, as
e.g. "--allow-unknown-type" is grouped with "-t" and "-s", as it can
only be combined with those options. The same goes for "--buffer",
"--unordered" etc.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 49 +++++++++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 20 deletions(-)
@@ -666,35 +666,44 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)NULL};conststructoptionoptions[]={-OPT_GROUP(N_("<type> can be one of: blob, tree, commit, tag")),-OPT_CMDMODE('t',NULL,&opt,N_("show object type"),'t'),-OPT_CMDMODE('s',NULL,&opt,N_("show object size"),'s'),+/* Simple queries */+OPT_GROUP(N_("Check object existence or emit object contents")),OPT_CMDMODE('e',NULL,&opt,-N_("exit with zero when there's no error"),'e'),-OPT_CMDMODE('p',NULL,&opt,N_("pretty-print object's content"),'p'),-OPT_CMDMODE(0,"textconv",&opt,-N_("for blob objects, run textconv on object's content"),'c'),-OPT_CMDMODE(0,"filters",&opt,-N_("for blob objects, run filters on object's content"),'w'),-OPT_CMDMODE(0,"batch-all-objects",&opt,-N_("show all objects with --batch or --batch-check"),'b'),-OPT_STRING(0,"path",&force_path,N_("blob"),-N_("use a specific path for --textconv/--filters")),+N_("check if <object> exists"),'e'),+OPT_CMDMODE('p',NULL,&opt,N_("pretty-print <object> content"),'p'),++OPT_GROUP(N_("Emit [broken] object attributes")),+OPT_CMDMODE('t',NULL,&opt,N_("show object type (one of 'blob', 'tree', 'commit', 'tag', ...)"),'t'),+OPT_CMDMODE('s',NULL,&opt,N_("show object size"),'s'),OPT_BOOL(0,"allow-unknown-type",&unknown_type,N_("allow -s and -t to work with broken/corrupt objects")),-OPT_BOOL(0,"buffer",&batch.buffer_output,N_("buffer --batch output")),-OPT_CALLBACK_F(0,"batch",&batch,"format",-N_("show info and content of objects fed from the standard input"),+/* Batch mode */+OPT_GROUP(N_("Batch objects requested on stdin (or --batch-all-objects)")),+OPT_CALLBACK_F(0,"batch",&batch,N_("format"),+N_("show full <object> or <rev> contents"),PARSE_OPT_OPTARG|PARSE_OPT_NONEG,batch_option_callback),-OPT_CALLBACK_F(0,"batch-check",&batch,"format",-N_("show info about objects fed from the standard input"),+OPT_CALLBACK_F(0,"batch-check",&batch,N_("format"),+N_("like --batch, but don't emit <contents>"),PARSE_OPT_OPTARG|PARSE_OPT_NONEG,batch_option_callback),+OPT_CMDMODE(0,"batch-all-objects",&opt,+N_("with --batch[-check]: ignores stdin, batches all known objects"),'b'),+/* Batch-specific options */+OPT_GROUP(N_("Change or optimize batch output")),+OPT_BOOL(0,"buffer",&batch.buffer_output,N_("buffer --batch output")),OPT_BOOL(0,"follow-symlinks",&batch.follow_symlinks,-N_("follow in-tree symlinks (used with --batch or --batch-check)")),+N_("follow in-tree symlinks")),OPT_BOOL(0,"unordered",&batch.unordered,-N_("do not order --batch-all-objects output")),+N_("do not order objects before emitting them")),+/* Textconv options, stand-ole*/+OPT_GROUP(N_("Emit object (blob or tree) with conversion or filter (stand-alone, or with batch)")),+OPT_CMDMODE(0,"textconv",&opt,+N_("run textconv on object's content"),'c'),+OPT_CMDMODE(0,"filters",&opt,+N_("run filters on object's content"),'w'),+OPT_STRING(0,"path",&force_path,N_("blob|tree"),+N_("use a <path> for (--textconv | --filters ); Not with 'batch'")),OPT_END()};
Stop having GET_OID_ONLY_TO_DIE imply GET_OID_QUIETLY in
get_oid_with_context_1().
The *_DIE flag was added in 33bd598c390 (sha1_name.c: teach lookup
context to get_sha1_with_context(), 2012-07-02), and then later
tweaked in 7243ffdd78d (get_sha1: avoid repeating ourselves via
ONLY_TO_DIE, 2016-09-26).
Everything in that commit makes sense, but only for callers that
expect to fail in an initial call to get_oid_with_context_1(), e.g. as
"git show 0017" does via handle_revision_arg(), and then would like to
call get_oid_with_context_1() again via this
maybe_die_on_misspelt_object_name() function.
In the subsequent commit we'll add a new caller that expects to call
this only once, but who would still like to have all the error
messaging that GET_OID_ONLY_TO_DIE gives it, in addition to any
regular errors.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
object-name.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
Stress test the usage emitted when options are combined in ways that
isn't supported. Let's test various option combinations, some of these
we buggily allow right now.
E.g. this reveals a bug in 321459439e1 (cat-file: support
--textconv/--filters in batch mode, 2016-09-09) that we'll fix in a
subsequent commit. We're supposed to be emitting a relevant message
when --batch-all-objects is combined with --textconv or --filters, but
we don't.
The cases of needing to assign to opt=2 in the "opt" loop are because
on those we do the right thing already, in subsequent commits the
"test_expect_failure" cases will be fixed, and the for-loops unified.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t1006-cat-file.sh | 94 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
Add tests for the output that's emitted when we disambiguate
<obj>:<path> in cat-file. This gives us a baseline for improving these
messages.
For e.g. "git blame" we'll emit:
$ git blame HEAD:foo
fatal: no such path 'HEAD:foo' in HEAD
But cat-file doesn't disambiguate these two cases, and just gives the
rather unhelpful:
$ git cat-file --textconv HEAD:foo
fatal: Not a valid object name HEAD:foo
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t8007-cat-file-textconv.sh | 42 ++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
Add a usage_msg_optf() as a shorthand for the sort of
usage_msg_opt(xstrfmt(...)) used in builtin/stash.c. I'll make more
use of this function in builtin/cat-file.c shortly.
The disconnect between the "..." and "fmt" is a bit unusual, but it
works just fine and this keeps it consistent with usage_msg_opt(),
i.e. a caller of it can be moved to usage_msg_optf() and not have to
have its arguments re-arranged.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/stash.c | 4 ++--
parse-options.c | 13 +++++++++++++
parse-options.h | 10 ++++++++++
3 files changed, 25 insertions(+), 2 deletions(-)
There were various inaccuracies in the previous SYNOPSIS output,
e.g. "--path" is not something that can optionally go with any options
except --textconv or --filters, as the output implied.
The opening line of the DESCRIPTION section is also "In its first
form[...]", which refers to "git cat-file <type> <object>", but the
SYNOPSIS section wasn't showing that as the first form!
That part of the documentation made sense in
d83a42f34a6 (Documentation: minor grammatical fixes in
git-cat-file.txt, 2009-03-22) when it was introduced, but since then
various options that were added have made that intro make no sense in
the context it was in. Now the two will match again.
The usage output here is not properly aligned on "master" currently,
but will be with my in-flight 4631cfc20bd (parse-options: properly
align continued usage output, 2021-09-21), so let's indent things
correctly in the C code in anticipation of that.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Documentation/git-cat-file.txt | 10 ++++++++--
builtin/cat-file.c | 10 ++++++++--
2 files changed, 16 insertions(+), 4 deletions(-)
There's no benefit to defining this at a distance, and it makes the
code harder to read as you've got to scroll up to see the usage that
corresponds to the options.
In subsequent commits I'll make use of usage_msg_opt(), which will be
quite noisy if I have to use the long "cat_file_usage" variable,
there's no other command being defined in this file, so let's rename
it to just "usage".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 37 ++++++++++++++++++-------------------
1 file changed, 18 insertions(+), 19 deletions(-)
@@ -708,35 +707,35 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)elseif(argc==1)obj_name=argv[0];else-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(!opt&&!batch.enabled){if(argc==2){exp_type=argv[0];obj_name=argv[1];}else-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(batch.enabled){if(batch.cmdmode!=opt||argc)-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);if(batch.cmdmode&&batch.all_objects)die("--batch-all-objects cannot be combined with ""--textconv nor with --filters");}if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(force_path&&opt!='c'&&opt!='w'){error("--path=<path> needs --textconv or --filters");-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(force_path&&batch.enabled){error("--path=<path> incompatible with --batch");-usage_with_options(cat_file_usage,options);+usage_with_options(usage,options);}if(batch.buffer_output<0)
With the migration of --batch-all-objects to OPT_CMDMODE() in the
preceding commit one bug with combining it and other OPT_CMDMODE()
options was solved, but we were still left with e.g. --buffer silently
being discarded when not in batch mode.
Fix all those bugs, and in addition emit errors telling the user
specifically what options can't be combined with what other options,
before this we'd usually just emit the cryptic usage text and leave
the users to work it out by themselves.
This change is rather large, because to do so we need to untangle the
options processing so that we can not only error out, but emit
sensible errors, and e.g. emit errors about options before errors
about stray argc elements (as they might become valid if the option
were removed).
Some of the output changes ("error:" to "fatal:" with
usage_msg_opt[f]()), but none of the exit codes change, except in
those cases where we silently accepted bad option combinations before,
now we'll error out.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 95 ++++++++++++++++++++++++++++++---------------
t/t1006-cat-file.sh | 41 +++++++++----------
2 files changed, 84 insertions(+), 52 deletions(-)
@@ -648,6 +648,8 @@ static int batch_option_callback(const struct option *opt,intcmd_cat_file(intargc,constchar**argv,constchar*prefix){intopt=0;+intopt_cw=0;+intopt_epts=0;constchar*exp_type=NULL,*obj_name=NULL;structbatch_optionsbatch={0};intunknown_type=0;
@@ -701,45 +703,74 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)batch.buffer_output=-1;argc=parse_options(argc,argv,prefix,options,usage,0);-if(argc&&batch.enabled)-usage_with_options(usage,options);-if(opt=='b'){-batch.all_objects=1;-}elseif(opt){-if(batch.enabled&&(opt=='c'||opt=='w'))-batch.cmdmode=opt;-elseif(argc==1)-obj_name=argv[0];-else-usage_with_options(usage,options);-}elseif(!opt&&!batch.enabled){-if(argc==2){-exp_type=argv[0];-obj_name=argv[1];-}else-usage_with_options(usage,options);-}elseif(batch.enabled&&batch.cmdmode!=opt)-usage_with_options(usage,options);+opt_cw=(opt=='c'||opt=='w');+opt_epts=(opt=='e'||opt=='p'||opt=='t'||opt=='s');-if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){-usage_with_options(usage,options);-}--if(force_path&&opt!='c'&&opt!='w'){-error("--path=<path> needs --textconv or --filters");-usage_with_options(usage,options);-}+/* --batch-all-objects? */+if(opt=='b')+batch.all_objects=1;-if(force_path&&batch.enabled){-error("--path=<path> incompatible with --batch");-usage_with_options(usage,options);-}+/* Option compatibility */+if(force_path&&!opt_cw)+usage_msg_optf(_("'%s=<%s>' needs '%s' or '%s'"),+usage,options,+"--path",_("path|tree-ish"),"--filters",+"--textconv");+/* Option compatibility with batch mode */+if(batch.enabled)+;+elseif(batch.follow_symlinks)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--follow_symlinks");+elseif(batch.buffer_output>=0)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--buffer");+elseif(batch.all_objects)+usage_msg_optf(_("'%s' requires a batch mode"),usage,options,+"--batch-all-objects");++/* Batch defaults */if(batch.buffer_output<0)batch.buffer_output=batch.all_objects;-if(batch.enabled)+/* Return early if we're in batch mode? */+if(batch.enabled){+if(opt_cw)+batch.cmdmode=opt;+elseif(opt&&opt!='b')+usage_msg_optf(_("'-%c' is incompatible with batch mode"),+usage,options,opt);+elseif(argc)+usage_msg_opt(_("batch modes take no arguments"),usage,+options);+returnbatch_objects(&batch);+}++if(opt){+if(!argc&&opt=='c')+usage_msg_optf(_("<rev> required with '%s'"),+usage,options,"--textconv");+elseif(!argc&&opt=='w')+usage_msg_optf(_("<rev> required with '%s'"),+usage,options,"--filters");+elseif(!argc&&opt_epts)+usage_msg_optf(_("<object> required with '-%c'"),+usage,options,opt);+elseif(argc==1)+obj_name=argv[0];+else+usage_msg_opt(_("too many arguments"),usage,options);+}elseif(!argc){+usage_with_options(usage,options);+}elseif(argc!=2){+usage_msg_optf(_("only two arguments allowed in <type> <object> mode, not %d"),+usage,options,argc);+}elseif(argc){+exp_type=argv[0];+obj_name=argv[1];+}if(unknown_type&&opt!='t'&&opt!='s')die("git cat-file --allow-unknown-type: use with -s or -t");
@@ -34,48 +34,54 @@ do'done+test_missing_usage(){+test_expect_code129"$@"2>err&&+grep-E"^fatal:.*required"err+}+short_modes="-e -p -t -s"cw_modes="--textconv --filters"foroptin$cw_modesdotest_expect_success"usage: $opt requires another option"'-test_expect_code129gitcat-file$opt+test_missing_usagegitcat-file$opt'doneforoptin$short_modesdotest_expect_success"usage: $opt requires another option"'-test_expect_code129gitcat-file$opt+test_missing_usagegitcat-file$opt'foropt2in--batch\--batch-check\---follow-symlinks+--follow-symlinks\+"--path=foo HEAD:some-path.txt"do-test_expect_failure"usage: incompatible options: $opt and $opt2"'+test_expect_success"usage: incompatible options: $opt and $opt2"'test_incompatible_usagegitcat-file$opt$opt2'done--opt2="--path=foo HEAD:some-path.txt"-test_expect_success"usage: incompatible options: $opt and $opt2"'-test_incompatible_usagegitcat-file$opt$opt2-'done+test_too_many_arguments(){+test_expect_code129"$@"2>err&&+grep-E"^fatal: too many arguments$"err+}+foroptin$short_modes$cw_modesdoargs="one two three"test_expect_success"usage: too many arguments: $opt$args"'-test_expect_code129gitcat-file$opt$args+test_too_many_argumentsgitcat-file$opt$args'foropt2in--buffer--follow-symlinksdotest_expect_success"usage: incompatible arguments: $opt with batch option $opt2"'-test_expect_code129gitcat-file$opt$opt2+test_incompatible_usagegitcat-file$opt$opt2'donedone
@@ -84,14 +90,9 @@ for opt in --buffer \--follow-symlinks\--batch-all-objectsdo-status=success-iftest$opt="--buffer"-then-status=failure-fi-test_expect_$status"usage: bad option combination: $opt without batch mode"'-test_expect_code129gitcat-file$opt&&-test_expect_code129gitcat-file$optcommitHEAD+test_expect_success"usage: bad option combination: $opt without batch mode"'+test_incompatible_usagegitcat-file$opt&&+test_incompatible_usagegitcat-file$optcommitHEAD'done
Change the usage output emitted on "git cat-file -h" to group related
options, making it clear to users which options go with which other
ones.
The new output is:
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
Batch objects requested on stdin (or --batch-all-objects)
--batch[=<format>] show full <object> or <rev> contents
--batch-check[=<format>]
like --batch, but don't emit <contents>
--batch-all-objects with --batch[-check]: ignores stdin, batches all known objects
Change or optimize batch output
--buffer buffer --batch output
--follow-symlinks follow in-tree symlinks
--unordered do not order objects before emitting them
Emit object (blob or tree) with conversion or filter (stand-alone, or with batch)
--textconv run textconv on object's content
--filters run filters on object's content
--path blob|tree use a <path> for (--textconv | --filters ); Not with 'batch'
The old usage was:
<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
--batch-all-objects show all objects with --batch or --batch-check
--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)
--unordered do not order --batch-all-objects output
While shorter, I think the new one is easier to understand, as
e.g. "--allow-unknown-type" is grouped with "-t" and "-s", as it can
only be combined with those options. The same goes for "--buffer",
"--unordered" etc.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 49 +++++++++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 20 deletions(-)
@@ -666,35 +666,44 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)NULL};conststructoptionoptions[]={-OPT_GROUP(N_("<type> can be one of: blob, tree, commit, tag")),-OPT_CMDMODE('t',NULL,&opt,N_("show object type"),'t'),-OPT_CMDMODE('s',NULL,&opt,N_("show object size"),'s'),+/* Simple queries */+OPT_GROUP(N_("Check object existence or emit object contents")),OPT_CMDMODE('e',NULL,&opt,-N_("exit with zero when there's no error"),'e'),-OPT_CMDMODE('p',NULL,&opt,N_("pretty-print object's content"),'p'),-OPT_CMDMODE(0,"textconv",&opt,-N_("for blob objects, run textconv on object's content"),'c'),-OPT_CMDMODE(0,"filters",&opt,-N_("for blob objects, run filters on object's content"),'w'),-OPT_CMDMODE(0,"batch-all-objects",&opt,-N_("show all objects with --batch or --batch-check"),'b'),-OPT_STRING(0,"path",&force_path,N_("blob"),-N_("use a specific path for --textconv/--filters")),+N_("check if <object> exists"),'e'),+OPT_CMDMODE('p',NULL,&opt,N_("pretty-print <object> content"),'p'),++OPT_GROUP(N_("Emit [broken] object attributes")),+OPT_CMDMODE('t',NULL,&opt,N_("show object type (one of 'blob', 'tree', 'commit', 'tag', ...)"),'t'),+OPT_CMDMODE('s',NULL,&opt,N_("show object size"),'s'),OPT_BOOL(0,"allow-unknown-type",&unknown_type,N_("allow -s and -t to work with broken/corrupt objects")),-OPT_BOOL(0,"buffer",&batch.buffer_output,N_("buffer --batch output")),-OPT_CALLBACK_F(0,"batch",&batch,"format",-N_("show info and content of objects fed from the standard input"),+/* Batch mode */+OPT_GROUP(N_("Batch objects requested on stdin (or --batch-all-objects)")),+OPT_CALLBACK_F(0,"batch",&batch,N_("format"),+N_("show full <object> or <rev> contents"),PARSE_OPT_OPTARG|PARSE_OPT_NONEG,batch_option_callback),-OPT_CALLBACK_F(0,"batch-check",&batch,"format",-N_("show info about objects fed from the standard input"),+OPT_CALLBACK_F(0,"batch-check",&batch,N_("format"),+N_("like --batch, but don't emit <contents>"),PARSE_OPT_OPTARG|PARSE_OPT_NONEG,batch_option_callback),+OPT_CMDMODE(0,"batch-all-objects",&opt,+N_("with --batch[-check]: ignores stdin, batches all known objects"),'b'),+/* Batch-specific options */+OPT_GROUP(N_("Change or optimize batch output")),+OPT_BOOL(0,"buffer",&batch.buffer_output,N_("buffer --batch output")),OPT_BOOL(0,"follow-symlinks",&batch.follow_symlinks,-N_("follow in-tree symlinks (used with --batch or --batch-check)")),+N_("follow in-tree symlinks")),OPT_BOOL(0,"unordered",&batch.unordered,-N_("do not order --batch-all-objects output")),+N_("do not order objects before emitting them")),+/* Textconv options, stand-ole*/+OPT_GROUP(N_("Emit object (blob or tree) with conversion or filter (stand-alone, or with batch)")),+OPT_CMDMODE(0,"textconv",&opt,+N_("run textconv on object's content"),'c'),+OPT_CMDMODE(0,"filters",&opt,+N_("run filters on object's content"),'w'),+OPT_STRING(0,"path",&force_path,N_("blob|tree"),+N_("use a <path> for (--textconv | --filters ); Not with 'batch'")),OPT_END()};
Stop having GET_OID_ONLY_TO_DIE imply GET_OID_QUIETLY in
get_oid_with_context_1().
The *_DIE flag was added in 33bd598c390 (sha1_name.c: teach lookup
context to get_sha1_with_context(), 2012-07-02), and then later
tweaked in 7243ffdd78d (get_sha1: avoid repeating ourselves via
ONLY_TO_DIE, 2016-09-26).
Everything in that commit makes sense, but only for callers that
expect to fail in an initial call to get_oid_with_context_1(), e.g. as
"git show 0017" does via handle_revision_arg(), and then would like to
call get_oid_with_context_1() again via this
maybe_die_on_misspelt_object_name() function.
In the subsequent commit we'll add a new caller that expects to call
this only once, but who would still like to have all the error
messaging that GET_OID_ONLY_TO_DIE gives it, in addition to any
regular errors.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
object-name.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
Change the cat_one_file() logic that calls get_oid_with_context()
under --textconv and --filters to use the GET_OID_ONLY_TO_DIE flag,
thus improving the error messaging emitted when e.g. <path> is missing
but <rev> is not.
To service the "cat-file" use-case we need to introduce a new
"GET_OID_REQUIRE_PATH" flag, otherwise it would exit early as soon as
a valid "HEAD" was resolved, but in the "cat-file" case being changed
we always need a valid revision and path.
This arguably makes the "<bad rev>:<bad path>" and "<bad
rev>:<good (in HEAD) path>" use cases worse, as we won't quote the
<path> component at the user anymore, but let's just use the existing
logic "git log" et al use for now. We can improve the messaging for
those cases as a follow-up for all callers.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/cat-file.c | 16 ++++++----------
cache.h | 1 +
object-name.c | 3 +++
t/t8007-cat-file-textconv.sh | 8 ++++----
4 files changed, 14 insertions(+), 14 deletions(-)
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.
By giving parse_options() information about what options are mutually
exclusive with one another we can get the die() message being removed
here for free, we didn't even use that removed message in some cases,
e.g. for both of:
--batch-all-objects --textconv
--batch-all-objects --filters
We'd take the "goto usage" in the "if (opt)" branch, and never reach
the previous message. Now we'll emit e.g.:
$ git cat-file --batch-all-objects --filters
error: option `filters' is incompatible with --batch-all-objects
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 <redacted>
---
builtin/cat-file.c | 25 +++++++++++--------------
t/t1006-cat-file.sh | 7 ++-----
2 files changed, 13 insertions(+), 19 deletions(-)
@@ -674,6 +674,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)N_("for blob objects, run textconv on object's content"),'c'),OPT_CMDMODE(0,"filters",&opt,N_("for blob objects, run filters on object's content"),'w'),+OPT_CMDMODE(0,"batch-all-objects",&opt,+N_("show all objects with --batch or --batch-check"),'b'),OPT_STRING(0,"path",&force_path,N_("blob"),N_("use a specific path for --textconv/--filters")),OPT_BOOL(0,"allow-unknown-type",&unknown_type,
@@ -689,8 +691,6 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)batch_option_callback),OPT_BOOL(0,"follow-symlinks",&batch.follow_symlinks,N_("follow in-tree symlinks (used with --batch or --batch-check)")),-OPT_BOOL(0,"batch-all-objects",&batch.all_objects,-N_("show all objects with --batch or --batch-check")),OPT_BOOL(0,"unordered",&batch.unordered,N_("do not order --batch-all-objects output")),OPT_END()
@@ -699,30 +699,27 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)git_config(git_cat_file_config,NULL);batch.buffer_output=-1;-argc=parse_options(argc,argv,prefix,options,usage,0);-if(opt){+argc=parse_options(argc,argv,prefix,options,usage,0);+if(argc&&batch.enabled)+usage_with_options(usage,options);+if(opt=='b'){+batch.all_objects=1;+}elseif(opt){if(batch.enabled&&(opt=='c'||opt=='w'))batch.cmdmode=opt;elseif(argc==1)obj_name=argv[0];elseusage_with_options(usage,options);-}-if(!opt&&!batch.enabled){+}elseif(!opt&&!batch.enabled){if(argc==2){exp_type=argv[0];obj_name=argv[1];}elseusage_with_options(usage,options);-}-if(batch.enabled){-if(batch.cmdmode!=opt||argc)-usage_with_options(usage,options);-if(batch.cmdmode&&batch.all_objects)-die("--batch-all-objects cannot be combined with "-"--textconv nor with --filters");-}+}elseif(batch.enabled&&batch.cmdmode!=opt)+usage_with_options(usage,options);if((batch.follow_symlinks||batch.all_objects)&&!batch.enabled){usage_with_options(usage,options);
This series goes on top of ab/cat-file, which was merged to next in
e145efa6059 (Merge branch 'ab/cat-file' into next, 2022-01-05).
The first commit addresses an issue Jiang Xin raised in
https://lore.kernel.org/git/CANYiYbEYgSCx230S29zVhMKb8J8WQ1ScxVHn6fMvdhPOdjpBCg@mail.gmail.com/
The second fixes a typo of mine when referring to an option name that
I spotted while preparing this.
Ævar Arnfjörð Bjarmason (2):
cat-file: don't whitespace-pad "(...)" in SYNOPSIS and usage output
cat-file: s/_/-/ in typo'd usage_msg_optf() message
Documentation/git-cat-file.txt | 4 ++--
builtin/cat-file.c | 8 ++++----
2 files changed, 6 insertions(+), 6 deletions(-)
--
2.34.1.1373.g062f5534af2
Fix up whitespace issues around "(... | ...)" in the SYNOPSIS and
usage. These were introduced in ab/cat-file series. See
e145efa6059 (Merge branch 'ab/cat-file' into next, 2022-01-05). In
particular 57d6a1cf96, 5a40417876 and 97fe7250753 in that series.
We'll now correctly emit this usage output:
$ git cat-file -h
usage: git cat-file <type> <object>
or: git cat-file (-e | -p) <object>
or: git cat-file (-t | -s) [--allow-unknown-type] <object>
[...]
Before this the last line of that would be inconsistent with the
preceding "(-e | -p)":
or: git cat-file ( -t | -s ) [--allow-unknown-type] <object>
Reported-by: Jiang Xin <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Documentation/git-cat-file.txt | 4 ++--
builtin/cat-file.c | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
@@ -699,7 +699,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)OPT_CMDMODE(0,"filters",&opt,N_("run filters on object's content"),'w'),OPT_STRING(0,"path",&force_path,N_("blob|tree"),-N_("use a <path> for (--textconv | --filters ); Not with 'batch'")),+N_("use a <path> for (--textconv | --filters); Not with 'batch'")),OPT_END()};