* [PATCH v3 01/10] cat-file tests: test bad usage
2021-11-29 19:57 ` [PATCH v3 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
@ 2021-11-29 19:57 ` Ævar Arnfjörð Bjarmason
2021-11-29 19:57 ` [PATCH v3 02/10] cat-file tests: test messaging on bad objects/paths Ævar Arnfjörð Bjarmason
` (9 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-29 19:57 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
t/t1006-cat-file.sh | 94 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 658628375c8..fc9191c1b94 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -4,6 +4,100 @@ test_description='git cat-file'
. ./test-lib.sh
+test_cmdmode_usage () {
+ test_expect_code 129 "$@" 2>err &&
+ grep "^error:.*is incompatible with" err
+}
+
+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_expect_code 129 "$@" 2>err &&
+ grep -E "^error:.**needs" err
+}
+
+for opt in --batch --batch-check
+do
+ test_expect_success "usage: incompatible options: --path with $opt" '
+ test_incompatible_usage git cat-file --path=foo $opt
+ '
+done
+
+short_modes="-e -p -t -s"
+cw_modes="--textconv --filters"
+
+for opt in $cw_modes
+do
+ test_expect_success "usage: $opt requires another option" '
+ test_expect_code 129 git cat-file $opt
+ '
+
+ test_expect_failure "usage: incompatible options: --batch-all-objects with $opt" '
+ test_incompatible_usage git cat-file --batch-all-objects $opt
+ '
+done
+
+for opt in $short_modes
+do
+ test_expect_success "usage: $opt requires another option" '
+ test_expect_code 129 git cat-file $opt
+ '
+
+ for opt2 in --batch \
+ --batch-check \
+ --follow-symlinks
+ do
+ test_expect_failure "usage: incompatible options: $opt and $opt2" '
+ test_incompatible_usage git cat-file $opt $opt2
+ '
+ done
+
+ opt2="--path=foo HEAD:some-path.txt"
+ test_expect_success "usage: incompatible options: $opt and $opt2" '
+ test_incompatible_usage git cat-file $opt $opt2
+ '
+done
+
+for opt in $short_modes $cw_modes
+do
+ args="one two three"
+ test_expect_success "usage: too many arguments: $opt $args" '
+ test_expect_code 129 git cat-file $opt $args
+ '
+
+ for opt2 in --buffer --follow-symlinks
+ do
+ test_expect_success "usage: incompatible arguments: $opt with batch option $opt2" '
+ test_expect_code 129 git cat-file $opt $opt2
+ '
+ done
+done
+
+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
+
echo_without_newline () {
printf '%s' "$*"
}
--
2.34.1.841.gf15fb7e6f34
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v3 02/10] cat-file tests: test messaging on bad objects/paths
2021-11-29 19:57 ` [PATCH v3 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
2021-11-29 19:57 ` [PATCH v3 01/10] cat-file tests: test bad usage Ævar Arnfjörð Bjarmason
@ 2021-11-29 19:57 ` Ævar Arnfjörð Bjarmason
2021-11-29 19:57 ` [PATCH v3 03/10] parse-options API: add a usage_msg_optf() Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-29 19:57 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
t/t8007-cat-file-textconv.sh | 42 ++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index eacd49ade63..71ea2ac987e 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -19,6 +19,48 @@ test_expect_success 'setup ' '
GIT_AUTHOR_NAME=Number2 git commit -a -m Second --date="2010-01-01 20:00:00"
'
+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_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_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_expect_success 'usage: <bad rev>:<good (in HEAD) path>' '
+ cat >expect <<-\EOF &&
+ fatal: Not a valid object name HEAD2:one.bin
+ EOF
+ test_must_fail git cat-file --textconv HEAD2:one.bin 2>actual &&
+ test_cmp expect actual
+'
+
cat >expected <<EOF
bin: test version 2
EOF
--
2.34.1.841.gf15fb7e6f34
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v3 03/10] parse-options API: add a usage_msg_optf()
2021-11-29 19:57 ` [PATCH v3 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
2021-11-29 19:57 ` [PATCH v3 01/10] cat-file tests: test bad usage Ævar Arnfjörð Bjarmason
2021-11-29 19:57 ` [PATCH v3 02/10] cat-file tests: test messaging on bad objects/paths Ævar Arnfjörð Bjarmason
@ 2021-11-29 19:57 ` Ævar Arnfjörð Bjarmason
2021-11-29 19:57 ` [PATCH v3 04/10] cat-file docs: fix SYNOPSIS and "-h" output Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-29 19:57 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/stash.c | 4 ++--
parse-options.c | 13 +++++++++++++
parse-options.h | 10 ++++++++++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/builtin/stash.c b/builtin/stash.c
index a0ccc8654df..e27ade3e821 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1749,8 +1749,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
else if (!strcmp(argv[0], "save"))
return !!save_stash(argc, argv, prefix);
else if (*argv[0] != '-')
- usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
- git_stash_usage, options);
+ usage_msg_optf(_("unknown subcommand: %s"),
+ git_stash_usage, options, argv[0]);
/* Assume 'stash push' */
strvec_push(&args, "push");
diff --git a/parse-options.c b/parse-options.c
index fc5b43ff0b2..5a319dda7d5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1078,3 +1078,16 @@ void NORETURN usage_msg_opt(const char *msg,
fprintf(stderr, "fatal: %s\n\n", msg);
usage_with_options(usagestr, options);
}
+
+void NORETURN usage_msg_optf(const char * const fmt,
+ const char * const *usagestr,
+ const struct option *options, ...)
+{
+ struct strbuf msg = STRBUF_INIT;
+ va_list ap;
+ va_start(ap, options);
+ strbuf_vaddf(&msg, fmt, ap);
+ va_end(ap);
+
+ usage_msg_opt(msg.buf, usagestr, options);
+}
diff --git a/parse-options.h b/parse-options.h
index 275fb440818..4a9fa8a84d7 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -225,6 +225,16 @@ NORETURN void usage_msg_opt(const char *msg,
const char * const *usagestr,
const struct option *options);
+/**
+ * usage_msg_optf() is like usage_msg_opt() except that the first
+ * argument is a format string, and optional format arguments follow
+ * after the 3rd option.
+ */
+__attribute__((format (printf,1,4)))
+void NORETURN usage_msg_optf(const char *fmt,
+ const char * const *usagestr,
+ const struct option *options, ...);
+
/*
* Use these assertions for callbacks that expect to be called with NONEG and
* NOARG respectively, and do not otherwise handle the "unset" and "arg"
--
2.34.1.841.gf15fb7e6f34
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v3 04/10] cat-file docs: fix SYNOPSIS and "-h" output
2021-11-29 19:57 ` [PATCH v3 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-11-29 19:57 ` [PATCH v3 03/10] parse-options API: add a usage_msg_optf() Ævar Arnfjörð Bjarmason
@ 2021-11-29 19:57 ` Ævar Arnfjörð Bjarmason
2021-11-29 19:57 ` [PATCH v3 05/10] cat-file: move "usage" variable to cmd_cat_file() Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-29 19:57 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
Documentation/git-cat-file.txt | 10 ++++++++--
builtin/cat-file.c | 10 ++++++++--
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 27b27e2b300..73ebbc70ee2 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -9,8 +9,14 @@ git-cat-file - Provide content or type and size information for repository objec
SYNOPSIS
--------
[verse]
-'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) [--path=<path>] <object>
-'git cat-file' (--batch[=<format>] | --batch-check[=<format>]) [ --textconv | --filters ] [--follow-symlinks]
+'git cat-file' <type> <object>
+'git cat-file' (-e | -p) <object>
+'git cat-file' ( -t | -s ) [--allow-unknown-type] <object>
+'git cat-file' (--batch | --batch-check) [--batch-all-objects]
+ [--buffer] [--follow-symlinks] [--unordered]
+ [--textconv | --filters]
+'git cat-file' (--textconv | --filters )
+ [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]
DESCRIPTION
-----------
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 86fc03242b8..1df7f797cb6 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -619,8 +619,14 @@ static int batch_objects(struct batch_options *opt)
}
static const char * const cat_file_usage[] = {
- N_("git cat-file (-t [--allow-unknown-type] | -s [--allow-unknown-type] | -e | -p | <type> | --textconv | --filters) [--path=<path>] <object>"),
- N_("git cat-file (--batch[=<format>] | --batch-check[=<format>]) [--follow-symlinks] [--textconv | --filters]"),
+ N_("git cat-file <type> <object>"),
+ N_("git cat-file (-e | -p) <object>"),
+ N_("git cat-file ( -t | -s ) [--allow-unknown-type] <object>"),
+ N_("git cat-file (--batch | --batch-check) [--batch-all-objects]\n"
+ " [--buffer] [--follow-symlinks] [--unordered]\n"
+ " [--textconv | --filters]"),
+ N_("git cat-file (--textconv | --filters )\n"
+ " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"),
NULL
};
--
2.34.1.841.gf15fb7e6f34
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v3 05/10] cat-file: move "usage" variable to cmd_cat_file()
2021-11-29 19:57 ` [PATCH v3 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-11-29 19:57 ` [PATCH v3 04/10] cat-file docs: fix SYNOPSIS and "-h" output Ævar Arnfjörð Bjarmason
@ 2021-11-29 19:57 ` Ævar Arnfjörð Bjarmason
2021-11-29 19:57 ` [PATCH v3 06/10] cat-file: make --batch-all-objects a CMDMODE Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-29 19:57 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/cat-file.c | 37 ++++++++++++++++++-------------------
1 file changed, 18 insertions(+), 19 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1df7f797cb6..6d0f645301b 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -618,18 +618,6 @@ static int batch_objects(struct batch_options *opt)
return retval;
}
-static const char * const cat_file_usage[] = {
- N_("git cat-file <type> <object>"),
- N_("git cat-file (-e | -p) <object>"),
- N_("git cat-file ( -t | -s ) [--allow-unknown-type] <object>"),
- N_("git cat-file (--batch | --batch-check) [--batch-all-objects]\n"
- " [--buffer] [--follow-symlinks] [--unordered]\n"
- " [--textconv | --filters]"),
- N_("git cat-file (--textconv | --filters )\n"
- " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"),
- NULL
-};
-
static int git_cat_file_config(const char *var, const char *value, void *cb)
{
if (userdiff_config(var, value) < 0)
@@ -664,6 +652,17 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
struct batch_options batch = {0};
int unknown_type = 0;
+ const char * const usage[] = {
+ N_("git cat-file <type> <object>"),
+ N_("git cat-file (-e | -p) <object>"),
+ N_("git cat-file ( -t | -s ) [--allow-unknown-type] <object>"),
+ N_("git cat-file (--batch | --batch-check) [--batch-all-objects]\n"
+ " [--buffer] [--follow-symlinks] [--unordered]\n"
+ " [--textconv | --filters]"),
+ N_("git cat-file (--textconv | --filters )\n"
+ " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"),
+ NULL
+ };
const struct option options[] = {
OPT_GROUP(N_("<type> can be one of: blob, tree, commit, tag")),
OPT_CMDMODE('t', NULL, &opt, N_("show object type"), 't'),
@@ -700,7 +699,7 @@ 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, cat_file_usage, 0);
+ argc = parse_options(argc, argv, prefix, options, usage, 0);
if (opt) {
if (batch.enabled && (opt == 'c' || opt == 'w'))
@@ -708,35 +707,35 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
else if (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)
--
2.34.1.841.gf15fb7e6f34
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v3 06/10] cat-file: make --batch-all-objects a CMDMODE
2021-11-29 19:57 ` [PATCH v3 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-11-29 19:57 ` [PATCH v3 05/10] cat-file: move "usage" variable to cmd_cat_file() Ævar Arnfjörð Bjarmason
@ 2021-11-29 19:57 ` Ævar Arnfjörð Bjarmason
2021-11-29 19:57 ` [PATCH v3 07/10] cat-file: fix remaining usage bugs Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-29 19:57 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/cat-file.c | 25 +++++++++++--------------
t/t1006-cat-file.sh | 7 ++-----
2 files changed, 13 insertions(+), 19 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 6d0f645301b..87356208134 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -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;
+ } else if (opt) {
if (batch.enabled && (opt == 'c' || opt == 'w'))
batch.cmdmode = opt;
else if (argc == 1)
obj_name = argv[0];
else
usage_with_options(usage, options);
- }
- if (!opt && !batch.enabled) {
+ } else if (!opt && !batch.enabled) {
if (argc == 2) {
exp_type = argv[0];
obj_name = argv[1];
} else
usage_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");
- }
+ } else if (batch.enabled && batch.cmdmode != opt)
+ usage_with_options(usage, options);
if ((batch.follow_symlinks || batch.all_objects) && !batch.enabled) {
usage_with_options(usage, options);
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index fc9191c1b94..ebec2061d25 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -14,7 +14,8 @@ 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
@@ -41,10 +42,6 @@ do
test_expect_success "usage: $opt requires another option" '
test_expect_code 129 git cat-file $opt
'
-
- test_expect_failure "usage: incompatible options: --batch-all-objects with $opt" '
- test_incompatible_usage git cat-file --batch-all-objects $opt
- '
done
for opt in $short_modes
--
2.34.1.841.gf15fb7e6f34
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v3 07/10] cat-file: fix remaining usage bugs
2021-11-29 19:57 ` [PATCH v3 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-11-29 19:57 ` [PATCH v3 06/10] cat-file: make --batch-all-objects a CMDMODE Ævar Arnfjörð Bjarmason
@ 2021-11-29 19:57 ` Ævar Arnfjörð Bjarmason
2021-12-06 1:19 ` Jiang Xin
2021-11-29 19:57 ` [PATCH v3 08/10] cat-file: correct and improve usage information Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
10 siblings, 1 reply; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-29 19:57 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/cat-file.c | 95 ++++++++++++++++++++++++++++++---------------
t/t1006-cat-file.sh | 41 +++++++++----------
2 files changed, 84 insertions(+), 52 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 87356208134..f507e3ae46c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -648,6 +648,8 @@ static int batch_option_callback(const struct option *opt,
int cmd_cat_file(int argc, const char **argv, const char *prefix)
{
int opt = 0;
+ int opt_cw = 0;
+ int opt_epts = 0;
const char *exp_type = NULL, *obj_name = NULL;
struct batch_options batch = {0};
int unknown_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;
- } else if (opt) {
- if (batch.enabled && (opt == 'c' || opt == 'w'))
- batch.cmdmode = opt;
- else if (argc == 1)
- obj_name = argv[0];
- else
- usage_with_options(usage, options);
- } else if (!opt && !batch.enabled) {
- if (argc == 2) {
- exp_type = argv[0];
- obj_name = argv[1];
- } else
- usage_with_options(usage, options);
- } else if (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)
+ ;
+ else if (batch.follow_symlinks)
+ usage_msg_optf(_("'%s' requires a batch mode"), usage, options,
+ "--follow_symlinks");
+ else if (batch.buffer_output >= 0)
+ usage_msg_optf(_("'%s' requires a batch mode"), usage, options,
+ "--buffer");
+ else if (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;
+ else if (opt && opt != 'b')
+ usage_msg_optf(_("'-%c' is incompatible with batch mode"),
+ usage, options, opt);
+ else if (argc)
+ usage_msg_opt(_("batch modes take no arguments"), usage,
+ options);
+
return batch_objects(&batch);
+ }
+
+ if (opt) {
+ if (!argc && opt == 'c')
+ usage_msg_optf(_("<rev> required with '%s'"),
+ usage, options, "--textconv");
+ else if (!argc && opt == 'w')
+ usage_msg_optf(_("<rev> required with '%s'"),
+ usage, options, "--filters");
+ else if (!argc && opt_epts)
+ usage_msg_optf(_("<object> required with '-%c'"),
+ usage, options, opt);
+ else if (argc == 1)
+ obj_name = argv[0];
+ else
+ usage_msg_opt(_("too many arguments"), usage, options);
+ } else if (!argc) {
+ usage_with_options(usage, options);
+ } else if (argc != 2) {
+ usage_msg_optf(_("only two arguments allowed in <type> <object> mode, not %d"),
+ usage, options, argc);
+ } else if (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");
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index ebec2061d25..123801cfe2a 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -24,7 +24,7 @@ done
test_incompatible_usage () {
test_expect_code 129 "$@" 2>err &&
- grep -E "^error:.**needs" err
+ grep -E "^(fatal|error):.*(requires|incompatible with|needs)" err
}
for opt in --batch --batch-check
@@ -34,48 +34,54 @@ do
'
done
+test_missing_usage() {
+ test_expect_code 129 "$@" 2>err &&
+ grep -E "^fatal:.*required" err
+}
+
short_modes="-e -p -t -s"
cw_modes="--textconv --filters"
for opt in $cw_modes
do
test_expect_success "usage: $opt requires another option" '
- test_expect_code 129 git cat-file $opt
+ test_missing_usage git cat-file $opt
'
done
for opt in $short_modes
do
test_expect_success "usage: $opt requires another option" '
- test_expect_code 129 git cat-file $opt
+ test_missing_usage git cat-file $opt
'
for opt2 in --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_usage git cat-file $opt $opt2
'
done
-
- opt2="--path=foo HEAD:some-path.txt"
- test_expect_success "usage: incompatible options: $opt and $opt2" '
- test_incompatible_usage git cat-file $opt $opt2
- '
done
+test_too_many_arguments() {
+ test_expect_code 129 "$@" 2>err &&
+ grep -E "^fatal: too many arguments$" err
+}
+
for opt in $short_modes $cw_modes
do
args="one two three"
test_expect_success "usage: too many arguments: $opt $args" '
- test_expect_code 129 git cat-file $opt $args
+ test_too_many_arguments git cat-file $opt $args
'
for opt2 in --buffer --follow-symlinks
do
test_expect_success "usage: incompatible arguments: $opt with batch option $opt2" '
- test_expect_code 129 git cat-file $opt $opt2
+ test_incompatible_usage git cat-file $opt $opt2
'
done
done
@@ -84,14 +90,9 @@ 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
+ test_expect_success "usage: bad option combination: $opt without batch mode" '
+ test_incompatible_usage git cat-file $opt &&
+ test_incompatible_usage git cat-file $opt commit HEAD
'
done
--
2.34.1.841.gf15fb7e6f34
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v3 08/10] cat-file: correct and improve usage information
2021-11-29 19:57 ` [PATCH v3 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-11-29 19:57 ` [PATCH v3 07/10] cat-file: fix remaining usage bugs Ævar Arnfjörð Bjarmason
@ 2021-11-29 19:57 ` Ævar Arnfjörð Bjarmason
2021-11-29 19:57 ` [PATCH v3 09/10] object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-29 19:57 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/cat-file.c | 49 +++++++++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 20 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f507e3ae46c..1d7f79184f0 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -666,35 +666,44 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
NULL
};
const struct option options[] = {
- 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()
};
--
2.34.1.841.gf15fb7e6f34
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v3 09/10] object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY
2021-11-29 19:57 ` [PATCH v3 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2021-11-29 19:57 ` [PATCH v3 08/10] cat-file: correct and improve usage information Ævar Arnfjörð Bjarmason
@ 2021-11-29 19:57 ` Ævar Arnfjörð Bjarmason
2021-11-29 19:57 ` [PATCH v3 10/10] cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters) Ævar Arnfjörð Bjarmason
2021-12-08 12:34 ` [PATCH v4 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-29 19:57 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
object-name.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/object-name.c b/object-name.c
index fdff4601b2c..d44a8f3a7ca 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1795,9 +1795,6 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
const char *cp;
int only_to_die = flags & GET_OID_ONLY_TO_DIE;
- if (only_to_die)
- flags |= GET_OID_QUIETLY;
-
memset(oc, 0, sizeof(*oc));
oc->mode = S_IFINVALID;
strbuf_init(&oc->symlink_path, 0);
@@ -1932,7 +1929,7 @@ void maybe_die_on_misspelt_object_name(struct repository *r,
{
struct object_context oc;
struct object_id oid;
- get_oid_with_context_1(r, name, GET_OID_ONLY_TO_DIE,
+ get_oid_with_context_1(r, name, GET_OID_ONLY_TO_DIE | GET_OID_QUIETLY,
prefix, &oid, &oc);
}
--
2.34.1.841.gf15fb7e6f34
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v3 10/10] cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters)
2021-11-29 19:57 ` [PATCH v3 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2021-11-29 19:57 ` [PATCH v3 09/10] object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY Ævar Arnfjörð Bjarmason
@ 2021-11-29 19:57 ` Ævar Arnfjörð Bjarmason
2021-12-08 12:34 ` [PATCH v4 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-29 19:57 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1d7f79184f0..b76f2a00046 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -73,14 +73,16 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
struct object_info oi = OBJECT_INFO_INIT;
struct strbuf sb = STRBUF_INIT;
unsigned flags = OBJECT_INFO_LOOKUP_REPLACE;
+ unsigned get_oid_flags = GET_OID_RECORD_PATH | GET_OID_ONLY_TO_DIE;
const char *path = force_path;
+ if (!path && (opt == 'w' || opt == 'c'))
+ get_oid_flags |= GET_OID_REQUIRE_PATH;
if (unknown_type)
flags |= OBJECT_INFO_ALLOW_UNKNOWN_TYPE;
- if (get_oid_with_context(the_repository, obj_name,
- GET_OID_RECORD_PATH,
- &oid, &obj_context))
+ if (get_oid_with_context(the_repository, obj_name, get_oid_flags, &oid,
+ &obj_context))
die("Not a valid object name %s", obj_name);
if (!path)
@@ -112,9 +114,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
return !has_object_file(&oid);
case 'w':
- if (!path)
- die("git cat-file --filters %s: <object> must be "
- "<sha1:path>", obj_name);
if (filter_object(path, obj_context.mode,
&oid, &buf, &size))
@@ -122,10 +121,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
break;
case 'c':
- if (!path)
- die("git cat-file --textconv %s: <object> must be <sha1:path>",
- obj_name);
-
if (textconv_object(the_repository, path, obj_context.mode,
&oid, 1, &buf, &size))
break;
diff --git a/cache.h b/cache.h
index eba12487b99..788127a9869 100644
--- a/cache.h
+++ b/cache.h
@@ -1366,6 +1366,7 @@ struct object_context {
#define GET_OID_FOLLOW_SYMLINKS 0100
#define GET_OID_RECORD_PATH 0200
#define GET_OID_ONLY_TO_DIE 04000
+#define GET_OID_REQUIRE_PATH 010000
#define GET_OID_DISAMBIGUATORS \
(GET_OID_COMMIT | GET_OID_COMMITTISH | \
diff --git a/object-name.c b/object-name.c
index d44a8f3a7ca..e94ced3f170 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1799,8 +1799,12 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
oc->mode = S_IFINVALID;
strbuf_init(&oc->symlink_path, 0);
ret = get_oid_1(repo, name, namelen, oid, flags);
- if (!ret)
+ if (!ret) {
+ if (flags & GET_OID_REQUIRE_PATH)
+ die(_("<object>:<path> required, only <object> '%s' given"), name);
return ret;
+ }
+
/*
* tree:path --> object name of path in tree
* :path -> object name of absolute path in index
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index 71ea2ac987e..b067983ba1c 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -29,7 +29,7 @@ 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
@@ -37,7 +37,7 @@ 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
@@ -46,7 +46,7 @@ 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
@@ -55,7 +55,7 @@ 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 HEAD2:one.bin
+ fatal: invalid object name '\''HEAD2'\''.
EOF
test_must_fail git cat-file --textconv HEAD2:one.bin 2>actual &&
test_cmp expect actual
--
2.34.1.841.gf15fb7e6f34
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v4 00/10] cat-file: better usage UX & error messages
2021-11-29 19:57 ` [PATCH v3 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (9 preceding siblings ...)
2021-11-29 19:57 ` [PATCH v3 10/10] cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters) Ævar Arnfjörð Bjarmason
@ 2021-12-08 12:34 ` Ævar Arnfjörð Bjarmason
2021-12-08 12:34 ` [PATCH v4 01/10] cat-file tests: test bad usage Ævar Arnfjörð Bjarmason
` (10 more replies)
10 siblings, 11 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-08 12:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
This series of patches to cat-file significantly improves the UX of
the -h output, see 08/10. For the v3 see[1], for the new usage output
see [2].
This re-roll addresses a minor formatting issue pointed out by Jiang
Xin in 7/10. I then updated 10/10 a bit to get rid of some repetition
and to reduce the diff size to the pre-image.
1. https://lore.kernel.org/git/cover-v3-00.10-00000000000-20211129T195357Z-avarab@gmail.com/
2. https://lore.kernel.org/git/patch-v4-08.10-ebc8dd0a22e-20211208T123151Z-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 v3:
1: d77771e3ea0 = 1: b3d8ec1697f cat-file tests: test bad usage
2: ab21a69864f = 2: eb6fa584287 cat-file tests: test messaging on bad objects/paths
3: 69ef1ae48c3 = 3: 01de6e4305f parse-options API: add a usage_msg_optf()
4: 597bb97b90a = 4: aa384803fef cat-file docs: fix SYNOPSIS and "-h" output
5: a9ea4c52222 = 5: 32365ff569b cat-file: move "usage" variable to cmd_cat_file()
6: fcb8331f091 = 6: 473ea3b0394 cat-file: make --batch-all-objects a CMDMODE
7: ad79e2afc89 ! 7: 878d9052bfb cat-file: fix remaining usage bugs
@@ builtin/cat-file.c: int cmd_cat_file(int argc, const char **argv, const char *pr
- }
+ /* Option compatibility */
+ if (force_path && !opt_cw)
-+ usage_msg_optf(_("'%s=<%s> needs '%s' or '%s'"),
++ usage_msg_optf(_("'%s=<%s>' needs '%s' or '%s'"),
+ usage, options,
+ "--path", _("path|tree-ish"), "--filters",
+ "--textconv");
8: a378dd30dd0 = 8: ebc8dd0a22e cat-file: correct and improve usage information
9: 145c00db08c = 9: a7447510e4b object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY
10: 45a24f97c88 ! 10: a658099e3e1 cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters)
@@ builtin/cat-file.c: static int cat_one_file(int opt, const char *exp_type, const
unsigned flags = OBJECT_INFO_LOOKUP_REPLACE;
+ unsigned get_oid_flags = GET_OID_RECORD_PATH | GET_OID_ONLY_TO_DIE;
const char *path = force_path;
-+ if (!path && (opt == 'w' || opt == 'c'))
++ const int opt_cw = (opt == 'c' || opt == 'w');
++ if (!path && opt_cw)
+ get_oid_flags |= GET_OID_REQUIRE_PATH;
if (unknown_type)
@@ object-name.c: static enum get_oid_result get_oid_with_context_1(struct reposito
oc->mode = S_IFINVALID;
strbuf_init(&oc->symlink_path, 0);
ret = get_oid_1(repo, name, namelen, oid, flags);
-- if (!ret)
-+ if (!ret) {
-+ if (flags & GET_OID_REQUIRE_PATH)
-+ die(_("<object>:<path> required, only <object> '%s' given"), name);
++ if (!ret && flags & GET_OID_REQUIRE_PATH)
++ die(_("<object>:<path> required, only <object> '%s' given"),
++ name);
+ if (!ret)
return ret;
-+ }
-+
/*
- * tree:path --> object name of path in tree
- * :path -> object name of absolute path in index
## t/t8007-cat-file-textconv.sh ##
@@ t/t8007-cat-file-textconv.sh: test_expect_success 'usage: <bad rev>' '
--
2.34.1.926.g895e15e0c0c
^ permalink raw reply [flat|nested] 101+ messages in thread
* [PATCH v4 01/10] cat-file tests: test bad usage
2021-12-08 12:34 ` [PATCH v4 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
@ 2021-12-08 12:34 ` Ævar Arnfjörð Bjarmason
2021-12-08 12:34 ` [PATCH v4 02/10] cat-file tests: test messaging on bad objects/paths Ævar Arnfjörð Bjarmason
` (9 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-08 12:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
t/t1006-cat-file.sh | 94 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 658628375c8..fc9191c1b94 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -4,6 +4,100 @@ test_description='git cat-file'
. ./test-lib.sh
+test_cmdmode_usage () {
+ test_expect_code 129 "$@" 2>err &&
+ grep "^error:.*is incompatible with" err
+}
+
+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_expect_code 129 "$@" 2>err &&
+ grep -E "^error:.**needs" err
+}
+
+for opt in --batch --batch-check
+do
+ test_expect_success "usage: incompatible options: --path with $opt" '
+ test_incompatible_usage git cat-file --path=foo $opt
+ '
+done
+
+short_modes="-e -p -t -s"
+cw_modes="--textconv --filters"
+
+for opt in $cw_modes
+do
+ test_expect_success "usage: $opt requires another option" '
+ test_expect_code 129 git cat-file $opt
+ '
+
+ test_expect_failure "usage: incompatible options: --batch-all-objects with $opt" '
+ test_incompatible_usage git cat-file --batch-all-objects $opt
+ '
+done
+
+for opt in $short_modes
+do
+ test_expect_success "usage: $opt requires another option" '
+ test_expect_code 129 git cat-file $opt
+ '
+
+ for opt2 in --batch \
+ --batch-check \
+ --follow-symlinks
+ do
+ test_expect_failure "usage: incompatible options: $opt and $opt2" '
+ test_incompatible_usage git cat-file $opt $opt2
+ '
+ done
+
+ opt2="--path=foo HEAD:some-path.txt"
+ test_expect_success "usage: incompatible options: $opt and $opt2" '
+ test_incompatible_usage git cat-file $opt $opt2
+ '
+done
+
+for opt in $short_modes $cw_modes
+do
+ args="one two three"
+ test_expect_success "usage: too many arguments: $opt $args" '
+ test_expect_code 129 git cat-file $opt $args
+ '
+
+ for opt2 in --buffer --follow-symlinks
+ do
+ test_expect_success "usage: incompatible arguments: $opt with batch option $opt2" '
+ test_expect_code 129 git cat-file $opt $opt2
+ '
+ done
+done
+
+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
+
echo_without_newline () {
printf '%s' "$*"
}
--
2.34.1.926.g895e15e0c0c
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v4 02/10] cat-file tests: test messaging on bad objects/paths
2021-12-08 12:34 ` [PATCH v4 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
2021-12-08 12:34 ` [PATCH v4 01/10] cat-file tests: test bad usage Ævar Arnfjörð Bjarmason
@ 2021-12-08 12:34 ` Ævar Arnfjörð Bjarmason
2021-12-08 12:34 ` [PATCH v4 03/10] parse-options API: add a usage_msg_optf() Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-08 12:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
t/t8007-cat-file-textconv.sh | 42 ++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index eacd49ade63..71ea2ac987e 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -19,6 +19,48 @@ test_expect_success 'setup ' '
GIT_AUTHOR_NAME=Number2 git commit -a -m Second --date="2010-01-01 20:00:00"
'
+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_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_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_expect_success 'usage: <bad rev>:<good (in HEAD) path>' '
+ cat >expect <<-\EOF &&
+ fatal: Not a valid object name HEAD2:one.bin
+ EOF
+ test_must_fail git cat-file --textconv HEAD2:one.bin 2>actual &&
+ test_cmp expect actual
+'
+
cat >expected <<EOF
bin: test version 2
EOF
--
2.34.1.926.g895e15e0c0c
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v4 03/10] parse-options API: add a usage_msg_optf()
2021-12-08 12:34 ` [PATCH v4 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
2021-12-08 12:34 ` [PATCH v4 01/10] cat-file tests: test bad usage Ævar Arnfjörð Bjarmason
2021-12-08 12:34 ` [PATCH v4 02/10] cat-file tests: test messaging on bad objects/paths Ævar Arnfjörð Bjarmason
@ 2021-12-08 12:34 ` Ævar Arnfjörð Bjarmason
2021-12-08 12:34 ` [PATCH v4 04/10] cat-file docs: fix SYNOPSIS and "-h" output Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-08 12:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/stash.c | 4 ++--
parse-options.c | 13 +++++++++++++
parse-options.h | 10 ++++++++++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/builtin/stash.c b/builtin/stash.c
index 18c812bbe03..c9a09047a6e 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1811,8 +1811,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
else if (!strcmp(argv[0], "save"))
return !!save_stash(argc, argv, prefix);
else if (*argv[0] != '-')
- usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
- git_stash_usage, options);
+ usage_msg_optf(_("unknown subcommand: %s"),
+ git_stash_usage, options, argv[0]);
/* Assume 'stash push' */
strvec_push(&args, "push");
diff --git a/parse-options.c b/parse-options.c
index fc5b43ff0b2..5a319dda7d5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1078,3 +1078,16 @@ void NORETURN usage_msg_opt(const char *msg,
fprintf(stderr, "fatal: %s\n\n", msg);
usage_with_options(usagestr, options);
}
+
+void NORETURN usage_msg_optf(const char * const fmt,
+ const char * const *usagestr,
+ const struct option *options, ...)
+{
+ struct strbuf msg = STRBUF_INIT;
+ va_list ap;
+ va_start(ap, options);
+ strbuf_vaddf(&msg, fmt, ap);
+ va_end(ap);
+
+ usage_msg_opt(msg.buf, usagestr, options);
+}
diff --git a/parse-options.h b/parse-options.h
index 275fb440818..4a9fa8a84d7 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -225,6 +225,16 @@ NORETURN void usage_msg_opt(const char *msg,
const char * const *usagestr,
const struct option *options);
+/**
+ * usage_msg_optf() is like usage_msg_opt() except that the first
+ * argument is a format string, and optional format arguments follow
+ * after the 3rd option.
+ */
+__attribute__((format (printf,1,4)))
+void NORETURN usage_msg_optf(const char *fmt,
+ const char * const *usagestr,
+ const struct option *options, ...);
+
/*
* Use these assertions for callbacks that expect to be called with NONEG and
* NOARG respectively, and do not otherwise handle the "unset" and "arg"
--
2.34.1.926.g895e15e0c0c
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v4 04/10] cat-file docs: fix SYNOPSIS and "-h" output
2021-12-08 12:34 ` [PATCH v4 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-12-08 12:34 ` [PATCH v4 03/10] parse-options API: add a usage_msg_optf() Ævar Arnfjörð Bjarmason
@ 2021-12-08 12:34 ` Ævar Arnfjörð Bjarmason
2021-12-08 12:34 ` [PATCH v4 05/10] cat-file: move "usage" variable to cmd_cat_file() Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-08 12:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
Documentation/git-cat-file.txt | 10 ++++++++--
builtin/cat-file.c | 10 ++++++++--
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 27b27e2b300..73ebbc70ee2 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -9,8 +9,14 @@ git-cat-file - Provide content or type and size information for repository objec
SYNOPSIS
--------
[verse]
-'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) [--path=<path>] <object>
-'git cat-file' (--batch[=<format>] | --batch-check[=<format>]) [ --textconv | --filters ] [--follow-symlinks]
+'git cat-file' <type> <object>
+'git cat-file' (-e | -p) <object>
+'git cat-file' ( -t | -s ) [--allow-unknown-type] <object>
+'git cat-file' (--batch | --batch-check) [--batch-all-objects]
+ [--buffer] [--follow-symlinks] [--unordered]
+ [--textconv | --filters]
+'git cat-file' (--textconv | --filters )
+ [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]
DESCRIPTION
-----------
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 86fc03242b8..1df7f797cb6 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -619,8 +619,14 @@ static int batch_objects(struct batch_options *opt)
}
static const char * const cat_file_usage[] = {
- N_("git cat-file (-t [--allow-unknown-type] | -s [--allow-unknown-type] | -e | -p | <type> | --textconv | --filters) [--path=<path>] <object>"),
- N_("git cat-file (--batch[=<format>] | --batch-check[=<format>]) [--follow-symlinks] [--textconv | --filters]"),
+ N_("git cat-file <type> <object>"),
+ N_("git cat-file (-e | -p) <object>"),
+ N_("git cat-file ( -t | -s ) [--allow-unknown-type] <object>"),
+ N_("git cat-file (--batch | --batch-check) [--batch-all-objects]\n"
+ " [--buffer] [--follow-symlinks] [--unordered]\n"
+ " [--textconv | --filters]"),
+ N_("git cat-file (--textconv | --filters )\n"
+ " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"),
NULL
};
--
2.34.1.926.g895e15e0c0c
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v4 05/10] cat-file: move "usage" variable to cmd_cat_file()
2021-12-08 12:34 ` [PATCH v4 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-12-08 12:34 ` [PATCH v4 04/10] cat-file docs: fix SYNOPSIS and "-h" output Ævar Arnfjörð Bjarmason
@ 2021-12-08 12:34 ` Ævar Arnfjörð Bjarmason
2021-12-08 12:34 ` [PATCH v4 06/10] cat-file: make --batch-all-objects a CMDMODE Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-08 12:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/cat-file.c | 37 ++++++++++++++++++-------------------
1 file changed, 18 insertions(+), 19 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1df7f797cb6..6d0f645301b 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -618,18 +618,6 @@ static int batch_objects(struct batch_options *opt)
return retval;
}
-static const char * const cat_file_usage[] = {
- N_("git cat-file <type> <object>"),
- N_("git cat-file (-e | -p) <object>"),
- N_("git cat-file ( -t | -s ) [--allow-unknown-type] <object>"),
- N_("git cat-file (--batch | --batch-check) [--batch-all-objects]\n"
- " [--buffer] [--follow-symlinks] [--unordered]\n"
- " [--textconv | --filters]"),
- N_("git cat-file (--textconv | --filters )\n"
- " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"),
- NULL
-};
-
static int git_cat_file_config(const char *var, const char *value, void *cb)
{
if (userdiff_config(var, value) < 0)
@@ -664,6 +652,17 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
struct batch_options batch = {0};
int unknown_type = 0;
+ const char * const usage[] = {
+ N_("git cat-file <type> <object>"),
+ N_("git cat-file (-e | -p) <object>"),
+ N_("git cat-file ( -t | -s ) [--allow-unknown-type] <object>"),
+ N_("git cat-file (--batch | --batch-check) [--batch-all-objects]\n"
+ " [--buffer] [--follow-symlinks] [--unordered]\n"
+ " [--textconv | --filters]"),
+ N_("git cat-file (--textconv | --filters )\n"
+ " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"),
+ NULL
+ };
const struct option options[] = {
OPT_GROUP(N_("<type> can be one of: blob, tree, commit, tag")),
OPT_CMDMODE('t', NULL, &opt, N_("show object type"), 't'),
@@ -700,7 +699,7 @@ 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, cat_file_usage, 0);
+ argc = parse_options(argc, argv, prefix, options, usage, 0);
if (opt) {
if (batch.enabled && (opt == 'c' || opt == 'w'))
@@ -708,35 +707,35 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
else if (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)
--
2.34.1.926.g895e15e0c0c
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v4 06/10] cat-file: make --batch-all-objects a CMDMODE
2021-12-08 12:34 ` [PATCH v4 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-12-08 12:34 ` [PATCH v4 05/10] cat-file: move "usage" variable to cmd_cat_file() Ævar Arnfjörð Bjarmason
@ 2021-12-08 12:34 ` Ævar Arnfjörð Bjarmason
2021-12-08 12:34 ` [PATCH v4 07/10] cat-file: fix remaining usage bugs Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-08 12:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/cat-file.c | 25 +++++++++++--------------
t/t1006-cat-file.sh | 7 ++-----
2 files changed, 13 insertions(+), 19 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 6d0f645301b..87356208134 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -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;
+ } else if (opt) {
if (batch.enabled && (opt == 'c' || opt == 'w'))
batch.cmdmode = opt;
else if (argc == 1)
obj_name = argv[0];
else
usage_with_options(usage, options);
- }
- if (!opt && !batch.enabled) {
+ } else if (!opt && !batch.enabled) {
if (argc == 2) {
exp_type = argv[0];
obj_name = argv[1];
} else
usage_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");
- }
+ } else if (batch.enabled && batch.cmdmode != opt)
+ usage_with_options(usage, options);
if ((batch.follow_symlinks || batch.all_objects) && !batch.enabled) {
usage_with_options(usage, options);
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index fc9191c1b94..ebec2061d25 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -14,7 +14,8 @@ 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
@@ -41,10 +42,6 @@ do
test_expect_success "usage: $opt requires another option" '
test_expect_code 129 git cat-file $opt
'
-
- test_expect_failure "usage: incompatible options: --batch-all-objects with $opt" '
- test_incompatible_usage git cat-file --batch-all-objects $opt
- '
done
for opt in $short_modes
--
2.34.1.926.g895e15e0c0c
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v4 07/10] cat-file: fix remaining usage bugs
2021-12-08 12:34 ` [PATCH v4 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-12-08 12:34 ` [PATCH v4 06/10] cat-file: make --batch-all-objects a CMDMODE Ævar Arnfjörð Bjarmason
@ 2021-12-08 12:34 ` Ævar Arnfjörð Bjarmason
2021-12-20 16:00 ` John Cai
2021-12-08 12:34 ` [PATCH v4 08/10] cat-file: correct and improve usage information Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
10 siblings, 1 reply; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-08 12:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/cat-file.c | 95 ++++++++++++++++++++++++++++++---------------
t/t1006-cat-file.sh | 41 +++++++++----------
2 files changed, 84 insertions(+), 52 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 87356208134..1087f0f4a85 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -648,6 +648,8 @@ static int batch_option_callback(const struct option *opt,
int cmd_cat_file(int argc, const char **argv, const char *prefix)
{
int opt = 0;
+ int opt_cw = 0;
+ int opt_epts = 0;
const char *exp_type = NULL, *obj_name = NULL;
struct batch_options batch = {0};
int unknown_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;
- } else if (opt) {
- if (batch.enabled && (opt == 'c' || opt == 'w'))
- batch.cmdmode = opt;
- else if (argc == 1)
- obj_name = argv[0];
- else
- usage_with_options(usage, options);
- } else if (!opt && !batch.enabled) {
- if (argc == 2) {
- exp_type = argv[0];
- obj_name = argv[1];
- } else
- usage_with_options(usage, options);
- } else if (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)
+ ;
+ else if (batch.follow_symlinks)
+ usage_msg_optf(_("'%s' requires a batch mode"), usage, options,
+ "--follow_symlinks");
+ else if (batch.buffer_output >= 0)
+ usage_msg_optf(_("'%s' requires a batch mode"), usage, options,
+ "--buffer");
+ else if (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;
+ else if (opt && opt != 'b')
+ usage_msg_optf(_("'-%c' is incompatible with batch mode"),
+ usage, options, opt);
+ else if (argc)
+ usage_msg_opt(_("batch modes take no arguments"), usage,
+ options);
+
return batch_objects(&batch);
+ }
+
+ if (opt) {
+ if (!argc && opt == 'c')
+ usage_msg_optf(_("<rev> required with '%s'"),
+ usage, options, "--textconv");
+ else if (!argc && opt == 'w')
+ usage_msg_optf(_("<rev> required with '%s'"),
+ usage, options, "--filters");
+ else if (!argc && opt_epts)
+ usage_msg_optf(_("<object> required with '-%c'"),
+ usage, options, opt);
+ else if (argc == 1)
+ obj_name = argv[0];
+ else
+ usage_msg_opt(_("too many arguments"), usage, options);
+ } else if (!argc) {
+ usage_with_options(usage, options);
+ } else if (argc != 2) {
+ usage_msg_optf(_("only two arguments allowed in <type> <object> mode, not %d"),
+ usage, options, argc);
+ } else if (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");
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index ebec2061d25..123801cfe2a 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -24,7 +24,7 @@ done
test_incompatible_usage () {
test_expect_code 129 "$@" 2>err &&
- grep -E "^error:.**needs" err
+ grep -E "^(fatal|error):.*(requires|incompatible with|needs)" err
}
for opt in --batch --batch-check
@@ -34,48 +34,54 @@ do
'
done
+test_missing_usage() {
+ test_expect_code 129 "$@" 2>err &&
+ grep -E "^fatal:.*required" err
+}
+
short_modes="-e -p -t -s"
cw_modes="--textconv --filters"
for opt in $cw_modes
do
test_expect_success "usage: $opt requires another option" '
- test_expect_code 129 git cat-file $opt
+ test_missing_usage git cat-file $opt
'
done
for opt in $short_modes
do
test_expect_success "usage: $opt requires another option" '
- test_expect_code 129 git cat-file $opt
+ test_missing_usage git cat-file $opt
'
for opt2 in --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_usage git cat-file $opt $opt2
'
done
-
- opt2="--path=foo HEAD:some-path.txt"
- test_expect_success "usage: incompatible options: $opt and $opt2" '
- test_incompatible_usage git cat-file $opt $opt2
- '
done
+test_too_many_arguments() {
+ test_expect_code 129 "$@" 2>err &&
+ grep -E "^fatal: too many arguments$" err
+}
+
for opt in $short_modes $cw_modes
do
args="one two three"
test_expect_success "usage: too many arguments: $opt $args" '
- test_expect_code 129 git cat-file $opt $args
+ test_too_many_arguments git cat-file $opt $args
'
for opt2 in --buffer --follow-symlinks
do
test_expect_success "usage: incompatible arguments: $opt with batch option $opt2" '
- test_expect_code 129 git cat-file $opt $opt2
+ test_incompatible_usage git cat-file $opt $opt2
'
done
done
@@ -84,14 +90,9 @@ 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
+ test_expect_success "usage: bad option combination: $opt without batch mode" '
+ test_incompatible_usage git cat-file $opt &&
+ test_incompatible_usage git cat-file $opt commit HEAD
'
done
--
2.34.1.926.g895e15e0c0c
^ permalink raw reply related [flat|nested] 101+ messages in thread
* Re: [PATCH v4 07/10] cat-file: fix remaining usage bugs
2021-12-08 12:34 ` [PATCH v4 07/10] cat-file: fix remaining usage bugs Ævar Arnfjörð Bjarmason
@ 2021-12-20 16:00 ` John Cai
0 siblings, 0 replies; 101+ messages in thread
From: John Cai @ 2021-12-20 16:00 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin
> On Dec 8, 2021, at 7:34 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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 <avarab@gmail.com>
> ---
> builtin/cat-file.c | 95 ++++++++++++++++++++++++++++++---------------
> t/t1006-cat-file.sh | 41 +++++++++----------
> 2 files changed, 84 insertions(+), 52 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 87356208134..1087f0f4a85 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -648,6 +648,8 @@ static int batch_option_callback(const struct option *opt,
> int cmd_cat_file(int argc, const char **argv, const char *prefix)
> {
> int opt = 0;
> + int opt_cw = 0;
> + int opt_epts = 0;
> const char *exp_type = NULL, *obj_name = NULL;
> struct batch_options batch = {0};
> int unknown_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;
> - } else if (opt) {
> - if (batch.enabled && (opt == 'c' || opt == 'w'))
> - batch.cmdmode = opt;
> - else if (argc == 1)
> - obj_name = argv[0];
> - else
> - usage_with_options(usage, options);
> - } else if (!opt && !batch.enabled) {
> - if (argc == 2) {
> - exp_type = argv[0];
> - obj_name = argv[1];
> - } else
> - usage_with_options(usage, options);
> - } else if (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)
> + ;
> + else if (batch.follow_symlinks)
> + usage_msg_optf(_("'%s' requires a batch mode"), usage, options,
> + "--follow_symlinks");
> + else if (batch.buffer_output >= 0)
> + usage_msg_optf(_("'%s' requires a batch mode"), usage, options,
> + "--buffer");
> + else if (batch.all_objects)
> + usage_msg_optf(_("'%s' requires a batch mode"), usage, options,
> + "--batch-all_objects");
s/_/- for the last argument in the above usage_msg_optf calls
> +
> + /* 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;
> + else if (opt && opt != 'b')
> + usage_msg_optf(_("'-%c' is incompatible with batch mode"),
> + usage, options, opt);
> + else if (argc)
> + usage_msg_opt(_("batch modes take no arguments"), usage,
> + options);
> +
> return batch_objects(&batch);
> + }
> +
> + if (opt) {
> + if (!argc && opt == 'c')
> + usage_msg_optf(_("<rev> required with '%s'"),
> + usage, options, "--textconv");
> + else if (!argc && opt == 'w')
> + usage_msg_optf(_("<rev> required with '%s'"),
> + usage, options, "--filters");
> + else if (!argc && opt_epts)
> + usage_msg_optf(_("<object> required with '-%c'"),
> + usage, options, opt);
> + else if (argc == 1)
> + obj_name = argv[0];
> + else
> + usage_msg_opt(_("too many arguments"), usage, options);
> + } else if (!argc) {
> + usage_with_options(usage, options);
> + } else if (argc != 2) {
> + usage_msg_optf(_("only two arguments allowed in <type> <object> mode, not %d"),
> + usage, options, argc);
> + } else if (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");
> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> index ebec2061d25..123801cfe2a 100755
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -24,7 +24,7 @@ done
>
> test_incompatible_usage () {
> test_expect_code 129 "$@" 2>err &&
> - grep -E "^error:.**needs" err
> + grep -E "^(fatal|error):.*(requires|incompatible with|needs)" err
> }
>
> for opt in --batch --batch-check
> @@ -34,48 +34,54 @@ do
> '
> done
>
> +test_missing_usage() {
> + test_expect_code 129 "$@" 2>err &&
> + grep -E "^fatal:.*required" err
> +}
> +
> short_modes="-e -p -t -s"
> cw_modes="--textconv --filters"
>
> for opt in $cw_modes
> do
> test_expect_success "usage: $opt requires another option" '
> - test_expect_code 129 git cat-file $opt
> + test_missing_usage git cat-file $opt
> '
> done
>
> for opt in $short_modes
> do
> test_expect_success "usage: $opt requires another option" '
> - test_expect_code 129 git cat-file $opt
> + test_missing_usage git cat-file $opt
> '
>
> for opt2 in --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_usage git cat-file $opt $opt2
> '
> done
> -
> - opt2="--path=foo HEAD:some-path.txt"
> - test_expect_success "usage: incompatible options: $opt and $opt2" '
> - test_incompatible_usage git cat-file $opt $opt2
> - '
> done
>
> +test_too_many_arguments() {
> + test_expect_code 129 "$@" 2>err &&
> + grep -E "^fatal: too many arguments$" err
> +}
> +
> for opt in $short_modes $cw_modes
> do
> args="one two three"
> test_expect_success "usage: too many arguments: $opt $args" '
> - test_expect_code 129 git cat-file $opt $args
> + test_too_many_arguments git cat-file $opt $args
> '
>
> for opt2 in --buffer --follow-symlinks
> do
> test_expect_success "usage: incompatible arguments: $opt with batch option $opt2" '
> - test_expect_code 129 git cat-file $opt $opt2
> + test_incompatible_usage git cat-file $opt $opt2
> '
> done
> done
> @@ -84,14 +90,9 @@ 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
> + test_expect_success "usage: bad option combination: $opt without batch mode" '
> + test_incompatible_usage git cat-file $opt &&
> + test_incompatible_usage git cat-file $opt commit HEAD
> '
> done
>
> --
> 2.34.1.926.g895e15e0c0c
>
^ permalink raw reply [flat|nested] 101+ messages in thread
* [PATCH v4 08/10] cat-file: correct and improve usage information
2021-12-08 12:34 ` [PATCH v4 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-12-08 12:34 ` [PATCH v4 07/10] cat-file: fix remaining usage bugs Ævar Arnfjörð Bjarmason
@ 2021-12-08 12:34 ` Ævar Arnfjörð Bjarmason
2021-12-08 12:34 ` [PATCH v4 09/10] object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-08 12:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/cat-file.c | 49 +++++++++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 20 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1087f0f4a85..e71519739a4 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -666,35 +666,44 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
NULL
};
const struct option options[] = {
- 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()
};
--
2.34.1.926.g895e15e0c0c
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v4 09/10] object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY
2021-12-08 12:34 ` [PATCH v4 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2021-12-08 12:34 ` [PATCH v4 08/10] cat-file: correct and improve usage information Ævar Arnfjörð Bjarmason
@ 2021-12-08 12:34 ` Ævar Arnfjörð Bjarmason
2021-12-08 12:34 ` [PATCH v4 10/10] cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters) Ævar Arnfjörð Bjarmason
2021-12-22 4:12 ` [PATCH v5 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-08 12:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
object-name.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/object-name.c b/object-name.c
index fdff4601b2c..d44a8f3a7ca 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1795,9 +1795,6 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
const char *cp;
int only_to_die = flags & GET_OID_ONLY_TO_DIE;
- if (only_to_die)
- flags |= GET_OID_QUIETLY;
-
memset(oc, 0, sizeof(*oc));
oc->mode = S_IFINVALID;
strbuf_init(&oc->symlink_path, 0);
@@ -1932,7 +1929,7 @@ void maybe_die_on_misspelt_object_name(struct repository *r,
{
struct object_context oc;
struct object_id oid;
- get_oid_with_context_1(r, name, GET_OID_ONLY_TO_DIE,
+ get_oid_with_context_1(r, name, GET_OID_ONLY_TO_DIE | GET_OID_QUIETLY,
prefix, &oid, &oc);
}
--
2.34.1.926.g895e15e0c0c
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v4 10/10] cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters)
2021-12-08 12:34 ` [PATCH v4 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2021-12-08 12:34 ` [PATCH v4 09/10] object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY Ævar Arnfjörð Bjarmason
@ 2021-12-08 12:34 ` Ævar Arnfjörð Bjarmason
2021-12-22 4:12 ` [PATCH v5 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-08 12:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index e71519739a4..f5437c2d045 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -73,14 +73,17 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
struct object_info oi = OBJECT_INFO_INIT;
struct strbuf sb = STRBUF_INIT;
unsigned flags = OBJECT_INFO_LOOKUP_REPLACE;
+ unsigned get_oid_flags = GET_OID_RECORD_PATH | GET_OID_ONLY_TO_DIE;
const char *path = force_path;
+ const int opt_cw = (opt == 'c' || opt == 'w');
+ if (!path && opt_cw)
+ get_oid_flags |= GET_OID_REQUIRE_PATH;
if (unknown_type)
flags |= OBJECT_INFO_ALLOW_UNKNOWN_TYPE;
- if (get_oid_with_context(the_repository, obj_name,
- GET_OID_RECORD_PATH,
- &oid, &obj_context))
+ if (get_oid_with_context(the_repository, obj_name, get_oid_flags, &oid,
+ &obj_context))
die("Not a valid object name %s", obj_name);
if (!path)
@@ -112,9 +115,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
return !has_object_file(&oid);
case 'w':
- if (!path)
- die("git cat-file --filters %s: <object> must be "
- "<sha1:path>", obj_name);
if (filter_object(path, obj_context.mode,
&oid, &buf, &size))
@@ -122,10 +122,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
break;
case 'c':
- if (!path)
- die("git cat-file --textconv %s: <object> must be <sha1:path>",
- obj_name);
-
if (textconv_object(the_repository, path, obj_context.mode,
&oid, 1, &buf, &size))
break;
diff --git a/cache.h b/cache.h
index eba12487b99..788127a9869 100644
--- a/cache.h
+++ b/cache.h
@@ -1366,6 +1366,7 @@ struct object_context {
#define GET_OID_FOLLOW_SYMLINKS 0100
#define GET_OID_RECORD_PATH 0200
#define GET_OID_ONLY_TO_DIE 04000
+#define GET_OID_REQUIRE_PATH 010000
#define GET_OID_DISAMBIGUATORS \
(GET_OID_COMMIT | GET_OID_COMMITTISH | \
diff --git a/object-name.c b/object-name.c
index d44a8f3a7ca..92862eeb1ac 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1799,6 +1799,9 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
oc->mode = S_IFINVALID;
strbuf_init(&oc->symlink_path, 0);
ret = get_oid_1(repo, name, namelen, oid, flags);
+ if (!ret && flags & GET_OID_REQUIRE_PATH)
+ die(_("<object>:<path> required, only <object> '%s' given"),
+ name);
if (!ret)
return ret;
/*
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index 71ea2ac987e..b067983ba1c 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -29,7 +29,7 @@ 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
@@ -37,7 +37,7 @@ 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
@@ -46,7 +46,7 @@ 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
@@ -55,7 +55,7 @@ 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 HEAD2:one.bin
+ fatal: invalid object name '\''HEAD2'\''.
EOF
test_must_fail git cat-file --textconv HEAD2:one.bin 2>actual &&
test_cmp expect actual
--
2.34.1.926.g895e15e0c0c
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v5 00/10] cat-file: better usage UX & error messages
2021-12-08 12:34 ` [PATCH v4 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (9 preceding siblings ...)
2021-12-08 12:34 ` [PATCH v4 10/10] cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters) Ævar Arnfjörð Bjarmason
@ 2021-12-22 4:12 ` Ævar Arnfjörð Bjarmason
2021-12-22 4:12 ` [PATCH v5 01/10] cat-file tests: test bad usage Ævar Arnfjörð Bjarmason
` (10 more replies)
10 siblings, 11 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-22 4:12 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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
^ permalink raw reply [flat|nested] 101+ messages in thread
* [PATCH v5 01/10] cat-file tests: test bad usage
2021-12-22 4:12 ` [PATCH v5 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
@ 2021-12-22 4:12 ` Ævar Arnfjörð Bjarmason
2021-12-22 4:12 ` [PATCH v5 02/10] cat-file tests: test messaging on bad objects/paths Ævar Arnfjörð Bjarmason
` (9 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-22 4:12 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
t/t1006-cat-file.sh | 94 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 0d4c55f74ec..8a29f96809d 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -4,6 +4,100 @@ test_description='git cat-file'
. ./test-lib.sh
+test_cmdmode_usage () {
+ test_expect_code 129 "$@" 2>err &&
+ grep "^error:.*is incompatible with" err
+}
+
+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_expect_code 129 "$@" 2>err &&
+ grep -E "^error:.**needs" err
+}
+
+for opt in --batch --batch-check
+do
+ test_expect_success "usage: incompatible options: --path with $opt" '
+ test_incompatible_usage git cat-file --path=foo $opt
+ '
+done
+
+short_modes="-e -p -t -s"
+cw_modes="--textconv --filters"
+
+for opt in $cw_modes
+do
+ test_expect_success "usage: $opt requires another option" '
+ test_expect_code 129 git cat-file $opt
+ '
+
+ test_expect_failure "usage: incompatible options: --batch-all-objects with $opt" '
+ test_incompatible_usage git cat-file --batch-all-objects $opt
+ '
+done
+
+for opt in $short_modes
+do
+ test_expect_success "usage: $opt requires another option" '
+ test_expect_code 129 git cat-file $opt
+ '
+
+ for opt2 in --batch \
+ --batch-check \
+ --follow-symlinks
+ do
+ test_expect_failure "usage: incompatible options: $opt and $opt2" '
+ test_incompatible_usage git cat-file $opt $opt2
+ '
+ done
+
+ opt2="--path=foo HEAD:some-path.txt"
+ test_expect_success "usage: incompatible options: $opt and $opt2" '
+ test_incompatible_usage git cat-file $opt $opt2
+ '
+done
+
+for opt in $short_modes $cw_modes
+do
+ args="one two three"
+ test_expect_success "usage: too many arguments: $opt $args" '
+ test_expect_code 129 git cat-file $opt $args
+ '
+
+ for opt2 in --buffer --follow-symlinks
+ do
+ test_expect_success "usage: incompatible arguments: $opt with batch option $opt2" '
+ test_expect_code 129 git cat-file $opt $opt2
+ '
+ done
+done
+
+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
+
echo_without_newline () {
printf '%s' "$*"
}
--
2.34.1.1146.gb52885e7c44
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v5 02/10] cat-file tests: test messaging on bad objects/paths
2021-12-22 4:12 ` [PATCH v5 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
2021-12-22 4:12 ` [PATCH v5 01/10] cat-file tests: test bad usage Ævar Arnfjörð Bjarmason
@ 2021-12-22 4:12 ` Ævar Arnfjörð Bjarmason
2021-12-22 4:12 ` [PATCH v5 03/10] parse-options API: add a usage_msg_optf() Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-22 4:12 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
t/t8007-cat-file-textconv.sh | 42 ++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index eacd49ade63..71ea2ac987e 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -19,6 +19,48 @@ test_expect_success 'setup ' '
GIT_AUTHOR_NAME=Number2 git commit -a -m Second --date="2010-01-01 20:00:00"
'
+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_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_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_expect_success 'usage: <bad rev>:<good (in HEAD) path>' '
+ cat >expect <<-\EOF &&
+ fatal: Not a valid object name HEAD2:one.bin
+ EOF
+ test_must_fail git cat-file --textconv HEAD2:one.bin 2>actual &&
+ test_cmp expect actual
+'
+
cat >expected <<EOF
bin: test version 2
EOF
--
2.34.1.1146.gb52885e7c44
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v5 03/10] parse-options API: add a usage_msg_optf()
2021-12-22 4:12 ` [PATCH v5 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
2021-12-22 4:12 ` [PATCH v5 01/10] cat-file tests: test bad usage Ævar Arnfjörð Bjarmason
2021-12-22 4:12 ` [PATCH v5 02/10] cat-file tests: test messaging on bad objects/paths Ævar Arnfjörð Bjarmason
@ 2021-12-22 4:12 ` Ævar Arnfjörð Bjarmason
2021-12-22 4:12 ` [PATCH v5 04/10] cat-file docs: fix SYNOPSIS and "-h" output Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-22 4:12 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/stash.c | 4 ++--
parse-options.c | 13 +++++++++++++
parse-options.h | 10 ++++++++++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/builtin/stash.c b/builtin/stash.c
index 18c812bbe03..c9a09047a6e 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1811,8 +1811,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
else if (!strcmp(argv[0], "save"))
return !!save_stash(argc, argv, prefix);
else if (*argv[0] != '-')
- usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
- git_stash_usage, options);
+ usage_msg_optf(_("unknown subcommand: %s"),
+ git_stash_usage, options, argv[0]);
/* Assume 'stash push' */
strvec_push(&args, "push");
diff --git a/parse-options.c b/parse-options.c
index 629e7963497..c01f0040368 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1079,3 +1079,16 @@ void NORETURN usage_msg_opt(const char *msg,
fprintf(stderr, "fatal: %s\n\n", msg);
usage_with_options(usagestr, options);
}
+
+void NORETURN usage_msg_optf(const char * const fmt,
+ const char * const *usagestr,
+ const struct option *options, ...)
+{
+ struct strbuf msg = STRBUF_INIT;
+ va_list ap;
+ va_start(ap, options);
+ strbuf_vaddf(&msg, fmt, ap);
+ va_end(ap);
+
+ usage_msg_opt(msg.buf, usagestr, options);
+}
diff --git a/parse-options.h b/parse-options.h
index 275fb440818..4a9fa8a84d7 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -225,6 +225,16 @@ NORETURN void usage_msg_opt(const char *msg,
const char * const *usagestr,
const struct option *options);
+/**
+ * usage_msg_optf() is like usage_msg_opt() except that the first
+ * argument is a format string, and optional format arguments follow
+ * after the 3rd option.
+ */
+__attribute__((format (printf,1,4)))
+void NORETURN usage_msg_optf(const char *fmt,
+ const char * const *usagestr,
+ const struct option *options, ...);
+
/*
* Use these assertions for callbacks that expect to be called with NONEG and
* NOARG respectively, and do not otherwise handle the "unset" and "arg"
--
2.34.1.1146.gb52885e7c44
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v5 04/10] cat-file docs: fix SYNOPSIS and "-h" output
2021-12-22 4:12 ` [PATCH v5 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-12-22 4:12 ` [PATCH v5 03/10] parse-options API: add a usage_msg_optf() Ævar Arnfjörð Bjarmason
@ 2021-12-22 4:12 ` Ævar Arnfjörð Bjarmason
2021-12-22 4:12 ` [PATCH v5 05/10] cat-file: move "usage" variable to cmd_cat_file() Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-22 4:12 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
Documentation/git-cat-file.txt | 10 ++++++++--
builtin/cat-file.c | 10 ++++++++--
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 27b27e2b300..73ebbc70ee2 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -9,8 +9,14 @@ git-cat-file - Provide content or type and size information for repository objec
SYNOPSIS
--------
[verse]
-'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) [--path=<path>] <object>
-'git cat-file' (--batch[=<format>] | --batch-check[=<format>]) [ --textconv | --filters ] [--follow-symlinks]
+'git cat-file' <type> <object>
+'git cat-file' (-e | -p) <object>
+'git cat-file' ( -t | -s ) [--allow-unknown-type] <object>
+'git cat-file' (--batch | --batch-check) [--batch-all-objects]
+ [--buffer] [--follow-symlinks] [--unordered]
+ [--textconv | --filters]
+'git cat-file' (--textconv | --filters )
+ [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]
DESCRIPTION
-----------
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 86fc03242b8..1df7f797cb6 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -619,8 +619,14 @@ static int batch_objects(struct batch_options *opt)
}
static const char * const cat_file_usage[] = {
- N_("git cat-file (-t [--allow-unknown-type] | -s [--allow-unknown-type] | -e | -p | <type> | --textconv | --filters) [--path=<path>] <object>"),
- N_("git cat-file (--batch[=<format>] | --batch-check[=<format>]) [--follow-symlinks] [--textconv | --filters]"),
+ N_("git cat-file <type> <object>"),
+ N_("git cat-file (-e | -p) <object>"),
+ N_("git cat-file ( -t | -s ) [--allow-unknown-type] <object>"),
+ N_("git cat-file (--batch | --batch-check) [--batch-all-objects]\n"
+ " [--buffer] [--follow-symlinks] [--unordered]\n"
+ " [--textconv | --filters]"),
+ N_("git cat-file (--textconv | --filters )\n"
+ " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"),
NULL
};
--
2.34.1.1146.gb52885e7c44
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v5 05/10] cat-file: move "usage" variable to cmd_cat_file()
2021-12-22 4:12 ` [PATCH v5 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-12-22 4:12 ` [PATCH v5 04/10] cat-file docs: fix SYNOPSIS and "-h" output Ævar Arnfjörð Bjarmason
@ 2021-12-22 4:12 ` Ævar Arnfjörð Bjarmason
2021-12-22 4:12 ` [PATCH v5 06/10] cat-file: make --batch-all-objects a CMDMODE Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-22 4:12 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/cat-file.c | 37 ++++++++++++++++++-------------------
1 file changed, 18 insertions(+), 19 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1df7f797cb6..6d0f645301b 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -618,18 +618,6 @@ static int batch_objects(struct batch_options *opt)
return retval;
}
-static const char * const cat_file_usage[] = {
- N_("git cat-file <type> <object>"),
- N_("git cat-file (-e | -p) <object>"),
- N_("git cat-file ( -t | -s ) [--allow-unknown-type] <object>"),
- N_("git cat-file (--batch | --batch-check) [--batch-all-objects]\n"
- " [--buffer] [--follow-symlinks] [--unordered]\n"
- " [--textconv | --filters]"),
- N_("git cat-file (--textconv | --filters )\n"
- " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"),
- NULL
-};
-
static int git_cat_file_config(const char *var, const char *value, void *cb)
{
if (userdiff_config(var, value) < 0)
@@ -664,6 +652,17 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
struct batch_options batch = {0};
int unknown_type = 0;
+ const char * const usage[] = {
+ N_("git cat-file <type> <object>"),
+ N_("git cat-file (-e | -p) <object>"),
+ N_("git cat-file ( -t | -s ) [--allow-unknown-type] <object>"),
+ N_("git cat-file (--batch | --batch-check) [--batch-all-objects]\n"
+ " [--buffer] [--follow-symlinks] [--unordered]\n"
+ " [--textconv | --filters]"),
+ N_("git cat-file (--textconv | --filters )\n"
+ " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"),
+ NULL
+ };
const struct option options[] = {
OPT_GROUP(N_("<type> can be one of: blob, tree, commit, tag")),
OPT_CMDMODE('t', NULL, &opt, N_("show object type"), 't'),
@@ -700,7 +699,7 @@ 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, cat_file_usage, 0);
+ argc = parse_options(argc, argv, prefix, options, usage, 0);
if (opt) {
if (batch.enabled && (opt == 'c' || opt == 'w'))
@@ -708,35 +707,35 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
else if (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)
--
2.34.1.1146.gb52885e7c44
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v5 06/10] cat-file: make --batch-all-objects a CMDMODE
2021-12-22 4:12 ` [PATCH v5 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-12-22 4:12 ` [PATCH v5 05/10] cat-file: move "usage" variable to cmd_cat_file() Ævar Arnfjörð Bjarmason
@ 2021-12-22 4:12 ` Ævar Arnfjörð Bjarmason
2021-12-22 4:13 ` [PATCH v5 07/10] cat-file: fix remaining usage bugs Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-22 4:12 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/cat-file.c | 25 +++++++++++--------------
t/t1006-cat-file.sh | 7 ++-----
2 files changed, 13 insertions(+), 19 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 6d0f645301b..87356208134 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -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;
+ } else if (opt) {
if (batch.enabled && (opt == 'c' || opt == 'w'))
batch.cmdmode = opt;
else if (argc == 1)
obj_name = argv[0];
else
usage_with_options(usage, options);
- }
- if (!opt && !batch.enabled) {
+ } else if (!opt && !batch.enabled) {
if (argc == 2) {
exp_type = argv[0];
obj_name = argv[1];
} else
usage_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");
- }
+ } else if (batch.enabled && batch.cmdmode != opt)
+ usage_with_options(usage, options);
if ((batch.follow_symlinks || batch.all_objects) && !batch.enabled) {
usage_with_options(usage, options);
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 8a29f96809d..2ce5c8b1824 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -14,7 +14,8 @@ 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
@@ -41,10 +42,6 @@ do
test_expect_success "usage: $opt requires another option" '
test_expect_code 129 git cat-file $opt
'
-
- test_expect_failure "usage: incompatible options: --batch-all-objects with $opt" '
- test_incompatible_usage git cat-file --batch-all-objects $opt
- '
done
for opt in $short_modes
--
2.34.1.1146.gb52885e7c44
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v5 07/10] cat-file: fix remaining usage bugs
2021-12-22 4:12 ` [PATCH v5 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-12-22 4:12 ` [PATCH v5 06/10] cat-file: make --batch-all-objects a CMDMODE Ævar Arnfjörð Bjarmason
@ 2021-12-22 4:13 ` Ævar Arnfjörð Bjarmason
2021-12-26 0:31 ` Junio C Hamano
2021-12-22 4:13 ` [PATCH v5 08/10] cat-file: correct and improve usage information Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
10 siblings, 1 reply; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-22 4:13 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/cat-file.c | 95 ++++++++++++++++++++++++++++++---------------
t/t1006-cat-file.sh | 41 +++++++++----------
2 files changed, 84 insertions(+), 52 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 87356208134..895292074ae 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -648,6 +648,8 @@ static int batch_option_callback(const struct option *opt,
int cmd_cat_file(int argc, const char **argv, const char *prefix)
{
int opt = 0;
+ int opt_cw = 0;
+ int opt_epts = 0;
const char *exp_type = NULL, *obj_name = NULL;
struct batch_options batch = {0};
int unknown_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;
- } else if (opt) {
- if (batch.enabled && (opt == 'c' || opt == 'w'))
- batch.cmdmode = opt;
- else if (argc == 1)
- obj_name = argv[0];
- else
- usage_with_options(usage, options);
- } else if (!opt && !batch.enabled) {
- if (argc == 2) {
- exp_type = argv[0];
- obj_name = argv[1];
- } else
- usage_with_options(usage, options);
- } else if (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)
+ ;
+ else if (batch.follow_symlinks)
+ usage_msg_optf(_("'%s' requires a batch mode"), usage, options,
+ "--follow_symlinks");
+ else if (batch.buffer_output >= 0)
+ usage_msg_optf(_("'%s' requires a batch mode"), usage, options,
+ "--buffer");
+ else if (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;
+ else if (opt && opt != 'b')
+ usage_msg_optf(_("'-%c' is incompatible with batch mode"),
+ usage, options, opt);
+ else if (argc)
+ usage_msg_opt(_("batch modes take no arguments"), usage,
+ options);
+
return batch_objects(&batch);
+ }
+
+ if (opt) {
+ if (!argc && opt == 'c')
+ usage_msg_optf(_("<rev> required with '%s'"),
+ usage, options, "--textconv");
+ else if (!argc && opt == 'w')
+ usage_msg_optf(_("<rev> required with '%s'"),
+ usage, options, "--filters");
+ else if (!argc && opt_epts)
+ usage_msg_optf(_("<object> required with '-%c'"),
+ usage, options, opt);
+ else if (argc == 1)
+ obj_name = argv[0];
+ else
+ usage_msg_opt(_("too many arguments"), usage, options);
+ } else if (!argc) {
+ usage_with_options(usage, options);
+ } else if (argc != 2) {
+ usage_msg_optf(_("only two arguments allowed in <type> <object> mode, not %d"),
+ usage, options, argc);
+ } else if (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");
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 2ce5c8b1824..fd872bce016 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -24,7 +24,7 @@ done
test_incompatible_usage () {
test_expect_code 129 "$@" 2>err &&
- grep -E "^error:.**needs" err
+ grep -E "^(fatal|error):.*(requires|incompatible with|needs)" err
}
for opt in --batch --batch-check
@@ -34,48 +34,54 @@ do
'
done
+test_missing_usage() {
+ test_expect_code 129 "$@" 2>err &&
+ grep -E "^fatal:.*required" err
+}
+
short_modes="-e -p -t -s"
cw_modes="--textconv --filters"
for opt in $cw_modes
do
test_expect_success "usage: $opt requires another option" '
- test_expect_code 129 git cat-file $opt
+ test_missing_usage git cat-file $opt
'
done
for opt in $short_modes
do
test_expect_success "usage: $opt requires another option" '
- test_expect_code 129 git cat-file $opt
+ test_missing_usage git cat-file $opt
'
for opt2 in --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_usage git cat-file $opt $opt2
'
done
-
- opt2="--path=foo HEAD:some-path.txt"
- test_expect_success "usage: incompatible options: $opt and $opt2" '
- test_incompatible_usage git cat-file $opt $opt2
- '
done
+test_too_many_arguments() {
+ test_expect_code 129 "$@" 2>err &&
+ grep -E "^fatal: too many arguments$" err
+}
+
for opt in $short_modes $cw_modes
do
args="one two three"
test_expect_success "usage: too many arguments: $opt $args" '
- test_expect_code 129 git cat-file $opt $args
+ test_too_many_arguments git cat-file $opt $args
'
for opt2 in --buffer --follow-symlinks
do
test_expect_success "usage: incompatible arguments: $opt with batch option $opt2" '
- test_expect_code 129 git cat-file $opt $opt2
+ test_incompatible_usage git cat-file $opt $opt2
'
done
done
@@ -84,14 +90,9 @@ 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
+ test_expect_success "usage: bad option combination: $opt without batch mode" '
+ test_incompatible_usage git cat-file $opt &&
+ test_incompatible_usage git cat-file $opt commit HEAD
'
done
--
2.34.1.1146.gb52885e7c44
^ permalink raw reply related [flat|nested] 101+ messages in thread
* Re: [PATCH v5 07/10] cat-file: fix remaining usage bugs
2021-12-22 4:13 ` [PATCH v5 07/10] cat-file: fix remaining usage bugs Ævar Arnfjörð Bjarmason
@ 2021-12-26 0:31 ` Junio C Hamano
0 siblings, 0 replies; 101+ messages in thread
From: Junio C Hamano @ 2021-12-26 0:31 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, John Cai, Sergey Organov, Jiang Xin
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 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.
> ...
I've read the patches again myself, as I didn't see much comments to
the recent rounds of the series on the list. Here are a few fix-ups
to style violations that made my reading hiccup while doing so,
introduced by this step.
----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] fixup! cat-file: fix remaining usage bugs
Style fix to have SP on both sides of
shell_function () {
... body of the function ...
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t1006-cat-file.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 123801cfe2..aa859271d6 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -34,7 +34,7 @@ do
'
done
-test_missing_usage() {
+test_missing_usage () {
test_expect_code 129 "$@" 2>err &&
grep -E "^fatal:.*required" err
}
@@ -66,7 +66,7 @@ do
done
done
-test_too_many_arguments() {
+test_too_many_arguments () {
test_expect_code 129 "$@" 2>err &&
grep -E "^fatal: too many arguments$" err
}
--
2.34.1-568-g69e9fd72b5
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v5 08/10] cat-file: correct and improve usage information
2021-12-22 4:12 ` [PATCH v5 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-12-22 4:13 ` [PATCH v5 07/10] cat-file: fix remaining usage bugs Ævar Arnfjörð Bjarmason
@ 2021-12-22 4:13 ` Ævar Arnfjörð Bjarmason
2021-12-22 4:13 ` [PATCH v5 09/10] object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-22 4:13 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/cat-file.c | 49 +++++++++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 20 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 895292074ae..b5b130d79c1 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -666,35 +666,44 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
NULL
};
const struct option options[] = {
- 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()
};
--
2.34.1.1146.gb52885e7c44
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v5 09/10] object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY
2021-12-22 4:12 ` [PATCH v5 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2021-12-22 4:13 ` [PATCH v5 08/10] cat-file: correct and improve usage information Ævar Arnfjörð Bjarmason
@ 2021-12-22 4:13 ` Ævar Arnfjörð Bjarmason
2021-12-22 4:13 ` [PATCH v5 10/10] cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters) Ævar Arnfjörð Bjarmason
2021-12-28 13:28 ` [PATCH v6 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-22 4:13 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
object-name.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/object-name.c b/object-name.c
index fdff4601b2c..d44a8f3a7ca 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1795,9 +1795,6 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
const char *cp;
int only_to_die = flags & GET_OID_ONLY_TO_DIE;
- if (only_to_die)
- flags |= GET_OID_QUIETLY;
-
memset(oc, 0, sizeof(*oc));
oc->mode = S_IFINVALID;
strbuf_init(&oc->symlink_path, 0);
@@ -1932,7 +1929,7 @@ void maybe_die_on_misspelt_object_name(struct repository *r,
{
struct object_context oc;
struct object_id oid;
- get_oid_with_context_1(r, name, GET_OID_ONLY_TO_DIE,
+ get_oid_with_context_1(r, name, GET_OID_ONLY_TO_DIE | GET_OID_QUIETLY,
prefix, &oid, &oc);
}
--
2.34.1.1146.gb52885e7c44
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v5 10/10] cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters)
2021-12-22 4:12 ` [PATCH v5 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2021-12-22 4:13 ` [PATCH v5 09/10] object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY Ævar Arnfjörð Bjarmason
@ 2021-12-22 4:13 ` Ævar Arnfjörð Bjarmason
2021-12-28 13:28 ` [PATCH v6 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
10 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-22 4:13 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index b5b130d79c1..ad9b3eef4f4 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -73,14 +73,17 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
struct object_info oi = OBJECT_INFO_INIT;
struct strbuf sb = STRBUF_INIT;
unsigned flags = OBJECT_INFO_LOOKUP_REPLACE;
+ unsigned get_oid_flags = GET_OID_RECORD_PATH | GET_OID_ONLY_TO_DIE;
const char *path = force_path;
+ const int opt_cw = (opt == 'c' || opt == 'w');
+ if (!path && opt_cw)
+ get_oid_flags |= GET_OID_REQUIRE_PATH;
if (unknown_type)
flags |= OBJECT_INFO_ALLOW_UNKNOWN_TYPE;
- if (get_oid_with_context(the_repository, obj_name,
- GET_OID_RECORD_PATH,
- &oid, &obj_context))
+ if (get_oid_with_context(the_repository, obj_name, get_oid_flags, &oid,
+ &obj_context))
die("Not a valid object name %s", obj_name);
if (!path)
@@ -112,9 +115,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
return !has_object_file(&oid);
case 'w':
- if (!path)
- die("git cat-file --filters %s: <object> must be "
- "<sha1:path>", obj_name);
if (filter_object(path, obj_context.mode,
&oid, &buf, &size))
@@ -122,10 +122,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
break;
case 'c':
- if (!path)
- die("git cat-file --textconv %s: <object> must be <sha1:path>",
- obj_name);
-
if (textconv_object(the_repository, path, obj_context.mode,
&oid, 1, &buf, &size))
break;
diff --git a/cache.h b/cache.h
index cfba463aa97..fae55cfcb33 100644
--- a/cache.h
+++ b/cache.h
@@ -1377,6 +1377,7 @@ struct object_context {
#define GET_OID_FOLLOW_SYMLINKS 0100
#define GET_OID_RECORD_PATH 0200
#define GET_OID_ONLY_TO_DIE 04000
+#define GET_OID_REQUIRE_PATH 010000
#define GET_OID_DISAMBIGUATORS \
(GET_OID_COMMIT | GET_OID_COMMITTISH | \
diff --git a/object-name.c b/object-name.c
index d44a8f3a7ca..92862eeb1ac 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1799,6 +1799,9 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
oc->mode = S_IFINVALID;
strbuf_init(&oc->symlink_path, 0);
ret = get_oid_1(repo, name, namelen, oid, flags);
+ if (!ret && flags & GET_OID_REQUIRE_PATH)
+ die(_("<object>:<path> required, only <object> '%s' given"),
+ name);
if (!ret)
return ret;
/*
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index 71ea2ac987e..b067983ba1c 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -29,7 +29,7 @@ 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
@@ -37,7 +37,7 @@ 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
@@ -46,7 +46,7 @@ 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
@@ -55,7 +55,7 @@ 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 HEAD2:one.bin
+ fatal: invalid object name '\''HEAD2'\''.
EOF
test_must_fail git cat-file --textconv HEAD2:one.bin 2>actual &&
test_cmp expect actual
--
2.34.1.1146.gb52885e7c44
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v6 00/10] cat-file: better usage UX & error messages
2021-12-22 4:12 ` [PATCH v5 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (9 preceding siblings ...)
2021-12-22 4:13 ` [PATCH v5 10/10] cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters) Ævar Arnfjörð Bjarmason
@ 2021-12-28 13:28 ` Ævar Arnfjörð Bjarmason
2021-12-28 13:28 ` [PATCH v6 01/10] cat-file tests: test bad usage Ævar Arnfjörð Bjarmason
` (9 more replies)
10 siblings, 10 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 13:28 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
This series of patches to cat-file significantly improves the UX of
the -h output, see 08/10. For the v5 see[1], for the new usage output
see [2].
1. https://lore.kernel.org/git/cover-v5-00.10-00000000000-20211222T041050Z-avarab@gmail.com/
2. https://lore.kernel.org/git/patch-v6-08.10-af22a4cb134-20211228T132637Z-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 v5:
1: e771bd38792 = 1: e52834a343f cat-file tests: test bad usage
2: 291312e2fb5 = 2: 02622592803 cat-file tests: test messaging on bad objects/paths
3: 0689dbb248c = 3: ff717088a28 parse-options API: add a usage_msg_optf()
4: 2a28b39430e = 4: c4078ce9222 cat-file docs: fix SYNOPSIS and "-h" output
5: 2d90c12fe7b = 5: 9573437374a cat-file: move "usage" variable to cmd_cat_file()
6: 227805d1804 = 6: 30ed6617de8 cat-file: make --batch-all-objects a CMDMODE
7: e6ea403efe0 ! 7: bf24dd063c9 cat-file: fix remaining usage bugs
@@ t/t1006-cat-file.sh: do
'
done
-+test_missing_usage() {
++test_missing_usage () {
+ test_expect_code 129 "$@" 2>err &&
+ grep -E "^fatal:.*required" err
+}
@@ t/t1006-cat-file.sh: do
- '
done
-+test_too_many_arguments() {
++test_too_many_arguments () {
+ test_expect_code 129 "$@" 2>err &&
+ grep -E "^fatal: too many arguments$" err
+}
8: 16b6bb8aaf2 = 8: af22a4cb134 cat-file: correct and improve usage information
9: 47543c57135 = 9: 7bf5654e8f7 object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY
10: 63920969ca8 = 10: 56826ac73e6 cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters)
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply [flat|nested] 101+ messages in thread
* [PATCH v6 01/10] cat-file tests: test bad usage
2021-12-28 13:28 ` [PATCH v6 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
@ 2021-12-28 13:28 ` Ævar Arnfjörð Bjarmason
2021-12-28 13:28 ` [PATCH v6 02/10] cat-file tests: test messaging on bad objects/paths Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 13:28 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
t/t1006-cat-file.sh | 94 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 0d4c55f74ec..8a29f96809d 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -4,6 +4,100 @@ test_description='git cat-file'
. ./test-lib.sh
+test_cmdmode_usage () {
+ test_expect_code 129 "$@" 2>err &&
+ grep "^error:.*is incompatible with" err
+}
+
+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_expect_code 129 "$@" 2>err &&
+ grep -E "^error:.**needs" err
+}
+
+for opt in --batch --batch-check
+do
+ test_expect_success "usage: incompatible options: --path with $opt" '
+ test_incompatible_usage git cat-file --path=foo $opt
+ '
+done
+
+short_modes="-e -p -t -s"
+cw_modes="--textconv --filters"
+
+for opt in $cw_modes
+do
+ test_expect_success "usage: $opt requires another option" '
+ test_expect_code 129 git cat-file $opt
+ '
+
+ test_expect_failure "usage: incompatible options: --batch-all-objects with $opt" '
+ test_incompatible_usage git cat-file --batch-all-objects $opt
+ '
+done
+
+for opt in $short_modes
+do
+ test_expect_success "usage: $opt requires another option" '
+ test_expect_code 129 git cat-file $opt
+ '
+
+ for opt2 in --batch \
+ --batch-check \
+ --follow-symlinks
+ do
+ test_expect_failure "usage: incompatible options: $opt and $opt2" '
+ test_incompatible_usage git cat-file $opt $opt2
+ '
+ done
+
+ opt2="--path=foo HEAD:some-path.txt"
+ test_expect_success "usage: incompatible options: $opt and $opt2" '
+ test_incompatible_usage git cat-file $opt $opt2
+ '
+done
+
+for opt in $short_modes $cw_modes
+do
+ args="one two three"
+ test_expect_success "usage: too many arguments: $opt $args" '
+ test_expect_code 129 git cat-file $opt $args
+ '
+
+ for opt2 in --buffer --follow-symlinks
+ do
+ test_expect_success "usage: incompatible arguments: $opt with batch option $opt2" '
+ test_expect_code 129 git cat-file $opt $opt2
+ '
+ done
+done
+
+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
+
echo_without_newline () {
printf '%s' "$*"
}
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v6 02/10] cat-file tests: test messaging on bad objects/paths
2021-12-28 13:28 ` [PATCH v6 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
2021-12-28 13:28 ` [PATCH v6 01/10] cat-file tests: test bad usage Ævar Arnfjörð Bjarmason
@ 2021-12-28 13:28 ` Ævar Arnfjörð Bjarmason
2021-12-28 13:28 ` [PATCH v6 03/10] parse-options API: add a usage_msg_optf() Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 13:28 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
t/t8007-cat-file-textconv.sh | 42 ++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index eacd49ade63..71ea2ac987e 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -19,6 +19,48 @@ test_expect_success 'setup ' '
GIT_AUTHOR_NAME=Number2 git commit -a -m Second --date="2010-01-01 20:00:00"
'
+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_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_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_expect_success 'usage: <bad rev>:<good (in HEAD) path>' '
+ cat >expect <<-\EOF &&
+ fatal: Not a valid object name HEAD2:one.bin
+ EOF
+ test_must_fail git cat-file --textconv HEAD2:one.bin 2>actual &&
+ test_cmp expect actual
+'
+
cat >expected <<EOF
bin: test version 2
EOF
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v6 03/10] parse-options API: add a usage_msg_optf()
2021-12-28 13:28 ` [PATCH v6 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
2021-12-28 13:28 ` [PATCH v6 01/10] cat-file tests: test bad usage Ævar Arnfjörð Bjarmason
2021-12-28 13:28 ` [PATCH v6 02/10] cat-file tests: test messaging on bad objects/paths Ævar Arnfjörð Bjarmason
@ 2021-12-28 13:28 ` Ævar Arnfjörð Bjarmason
2021-12-28 13:28 ` [PATCH v6 04/10] cat-file docs: fix SYNOPSIS and "-h" output Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 13:28 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/stash.c | 4 ++--
parse-options.c | 13 +++++++++++++
parse-options.h | 10 ++++++++++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/builtin/stash.c b/builtin/stash.c
index 18c812bbe03..c9a09047a6e 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1811,8 +1811,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
else if (!strcmp(argv[0], "save"))
return !!save_stash(argc, argv, prefix);
else if (*argv[0] != '-')
- usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
- git_stash_usage, options);
+ usage_msg_optf(_("unknown subcommand: %s"),
+ git_stash_usage, options, argv[0]);
/* Assume 'stash push' */
strvec_push(&args, "push");
diff --git a/parse-options.c b/parse-options.c
index 629e7963497..c01f0040368 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1079,3 +1079,16 @@ void NORETURN usage_msg_opt(const char *msg,
fprintf(stderr, "fatal: %s\n\n", msg);
usage_with_options(usagestr, options);
}
+
+void NORETURN usage_msg_optf(const char * const fmt,
+ const char * const *usagestr,
+ const struct option *options, ...)
+{
+ struct strbuf msg = STRBUF_INIT;
+ va_list ap;
+ va_start(ap, options);
+ strbuf_vaddf(&msg, fmt, ap);
+ va_end(ap);
+
+ usage_msg_opt(msg.buf, usagestr, options);
+}
diff --git a/parse-options.h b/parse-options.h
index 275fb440818..4a9fa8a84d7 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -225,6 +225,16 @@ NORETURN void usage_msg_opt(const char *msg,
const char * const *usagestr,
const struct option *options);
+/**
+ * usage_msg_optf() is like usage_msg_opt() except that the first
+ * argument is a format string, and optional format arguments follow
+ * after the 3rd option.
+ */
+__attribute__((format (printf,1,4)))
+void NORETURN usage_msg_optf(const char *fmt,
+ const char * const *usagestr,
+ const struct option *options, ...);
+
/*
* Use these assertions for callbacks that expect to be called with NONEG and
* NOARG respectively, and do not otherwise handle the "unset" and "arg"
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v6 04/10] cat-file docs: fix SYNOPSIS and "-h" output
2021-12-28 13:28 ` [PATCH v6 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-12-28 13:28 ` [PATCH v6 03/10] parse-options API: add a usage_msg_optf() Ævar Arnfjörð Bjarmason
@ 2021-12-28 13:28 ` Ævar Arnfjörð Bjarmason
2021-12-28 13:28 ` [PATCH v6 05/10] cat-file: move "usage" variable to cmd_cat_file() Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 13:28 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
Documentation/git-cat-file.txt | 10 ++++++++--
builtin/cat-file.c | 10 ++++++++--
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 27b27e2b300..73ebbc70ee2 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -9,8 +9,14 @@ git-cat-file - Provide content or type and size information for repository objec
SYNOPSIS
--------
[verse]
-'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) [--path=<path>] <object>
-'git cat-file' (--batch[=<format>] | --batch-check[=<format>]) [ --textconv | --filters ] [--follow-symlinks]
+'git cat-file' <type> <object>
+'git cat-file' (-e | -p) <object>
+'git cat-file' ( -t | -s ) [--allow-unknown-type] <object>
+'git cat-file' (--batch | --batch-check) [--batch-all-objects]
+ [--buffer] [--follow-symlinks] [--unordered]
+ [--textconv | --filters]
+'git cat-file' (--textconv | --filters )
+ [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]
DESCRIPTION
-----------
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 86fc03242b8..1df7f797cb6 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -619,8 +619,14 @@ static int batch_objects(struct batch_options *opt)
}
static const char * const cat_file_usage[] = {
- N_("git cat-file (-t [--allow-unknown-type] | -s [--allow-unknown-type] | -e | -p | <type> | --textconv | --filters) [--path=<path>] <object>"),
- N_("git cat-file (--batch[=<format>] | --batch-check[=<format>]) [--follow-symlinks] [--textconv | --filters]"),
+ N_("git cat-file <type> <object>"),
+ N_("git cat-file (-e | -p) <object>"),
+ N_("git cat-file ( -t | -s ) [--allow-unknown-type] <object>"),
+ N_("git cat-file (--batch | --batch-check) [--batch-all-objects]\n"
+ " [--buffer] [--follow-symlinks] [--unordered]\n"
+ " [--textconv | --filters]"),
+ N_("git cat-file (--textconv | --filters )\n"
+ " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"),
NULL
};
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v6 05/10] cat-file: move "usage" variable to cmd_cat_file()
2021-12-28 13:28 ` [PATCH v6 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-12-28 13:28 ` [PATCH v6 04/10] cat-file docs: fix SYNOPSIS and "-h" output Ævar Arnfjörð Bjarmason
@ 2021-12-28 13:28 ` Ævar Arnfjörð Bjarmason
2021-12-28 13:28 ` [PATCH v6 06/10] cat-file: make --batch-all-objects a CMDMODE Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 13:28 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/cat-file.c | 37 ++++++++++++++++++-------------------
1 file changed, 18 insertions(+), 19 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1df7f797cb6..6d0f645301b 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -618,18 +618,6 @@ static int batch_objects(struct batch_options *opt)
return retval;
}
-static const char * const cat_file_usage[] = {
- N_("git cat-file <type> <object>"),
- N_("git cat-file (-e | -p) <object>"),
- N_("git cat-file ( -t | -s ) [--allow-unknown-type] <object>"),
- N_("git cat-file (--batch | --batch-check) [--batch-all-objects]\n"
- " [--buffer] [--follow-symlinks] [--unordered]\n"
- " [--textconv | --filters]"),
- N_("git cat-file (--textconv | --filters )\n"
- " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"),
- NULL
-};
-
static int git_cat_file_config(const char *var, const char *value, void *cb)
{
if (userdiff_config(var, value) < 0)
@@ -664,6 +652,17 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
struct batch_options batch = {0};
int unknown_type = 0;
+ const char * const usage[] = {
+ N_("git cat-file <type> <object>"),
+ N_("git cat-file (-e | -p) <object>"),
+ N_("git cat-file ( -t | -s ) [--allow-unknown-type] <object>"),
+ N_("git cat-file (--batch | --batch-check) [--batch-all-objects]\n"
+ " [--buffer] [--follow-symlinks] [--unordered]\n"
+ " [--textconv | --filters]"),
+ N_("git cat-file (--textconv | --filters )\n"
+ " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"),
+ NULL
+ };
const struct option options[] = {
OPT_GROUP(N_("<type> can be one of: blob, tree, commit, tag")),
OPT_CMDMODE('t', NULL, &opt, N_("show object type"), 't'),
@@ -700,7 +699,7 @@ 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, cat_file_usage, 0);
+ argc = parse_options(argc, argv, prefix, options, usage, 0);
if (opt) {
if (batch.enabled && (opt == 'c' || opt == 'w'))
@@ -708,35 +707,35 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
else if (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)
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v6 06/10] cat-file: make --batch-all-objects a CMDMODE
2021-12-28 13:28 ` [PATCH v6 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-12-28 13:28 ` [PATCH v6 05/10] cat-file: move "usage" variable to cmd_cat_file() Ævar Arnfjörð Bjarmason
@ 2021-12-28 13:28 ` Ævar Arnfjörð Bjarmason
2021-12-28 13:28 ` [PATCH v6 07/10] cat-file: fix remaining usage bugs Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 13:28 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/cat-file.c | 25 +++++++++++--------------
t/t1006-cat-file.sh | 7 ++-----
2 files changed, 13 insertions(+), 19 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 6d0f645301b..87356208134 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -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;
+ } else if (opt) {
if (batch.enabled && (opt == 'c' || opt == 'w'))
batch.cmdmode = opt;
else if (argc == 1)
obj_name = argv[0];
else
usage_with_options(usage, options);
- }
- if (!opt && !batch.enabled) {
+ } else if (!opt && !batch.enabled) {
if (argc == 2) {
exp_type = argv[0];
obj_name = argv[1];
} else
usage_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");
- }
+ } else if (batch.enabled && batch.cmdmode != opt)
+ usage_with_options(usage, options);
if ((batch.follow_symlinks || batch.all_objects) && !batch.enabled) {
usage_with_options(usage, options);
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 8a29f96809d..2ce5c8b1824 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -14,7 +14,8 @@ 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
@@ -41,10 +42,6 @@ do
test_expect_success "usage: $opt requires another option" '
test_expect_code 129 git cat-file $opt
'
-
- test_expect_failure "usage: incompatible options: --batch-all-objects with $opt" '
- test_incompatible_usage git cat-file --batch-all-objects $opt
- '
done
for opt in $short_modes
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v6 07/10] cat-file: fix remaining usage bugs
2021-12-28 13:28 ` [PATCH v6 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-12-28 13:28 ` [PATCH v6 06/10] cat-file: make --batch-all-objects a CMDMODE Ævar Arnfjörð Bjarmason
@ 2021-12-28 13:28 ` Ævar Arnfjörð Bjarmason
2021-12-28 13:28 ` [PATCH v6 08/10] cat-file: correct and improve usage information Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 13:28 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/cat-file.c | 95 ++++++++++++++++++++++++++++++---------------
t/t1006-cat-file.sh | 41 +++++++++----------
2 files changed, 84 insertions(+), 52 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 87356208134..895292074ae 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -648,6 +648,8 @@ static int batch_option_callback(const struct option *opt,
int cmd_cat_file(int argc, const char **argv, const char *prefix)
{
int opt = 0;
+ int opt_cw = 0;
+ int opt_epts = 0;
const char *exp_type = NULL, *obj_name = NULL;
struct batch_options batch = {0};
int unknown_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;
- } else if (opt) {
- if (batch.enabled && (opt == 'c' || opt == 'w'))
- batch.cmdmode = opt;
- else if (argc == 1)
- obj_name = argv[0];
- else
- usage_with_options(usage, options);
- } else if (!opt && !batch.enabled) {
- if (argc == 2) {
- exp_type = argv[0];
- obj_name = argv[1];
- } else
- usage_with_options(usage, options);
- } else if (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)
+ ;
+ else if (batch.follow_symlinks)
+ usage_msg_optf(_("'%s' requires a batch mode"), usage, options,
+ "--follow_symlinks");
+ else if (batch.buffer_output >= 0)
+ usage_msg_optf(_("'%s' requires a batch mode"), usage, options,
+ "--buffer");
+ else if (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;
+ else if (opt && opt != 'b')
+ usage_msg_optf(_("'-%c' is incompatible with batch mode"),
+ usage, options, opt);
+ else if (argc)
+ usage_msg_opt(_("batch modes take no arguments"), usage,
+ options);
+
return batch_objects(&batch);
+ }
+
+ if (opt) {
+ if (!argc && opt == 'c')
+ usage_msg_optf(_("<rev> required with '%s'"),
+ usage, options, "--textconv");
+ else if (!argc && opt == 'w')
+ usage_msg_optf(_("<rev> required with '%s'"),
+ usage, options, "--filters");
+ else if (!argc && opt_epts)
+ usage_msg_optf(_("<object> required with '-%c'"),
+ usage, options, opt);
+ else if (argc == 1)
+ obj_name = argv[0];
+ else
+ usage_msg_opt(_("too many arguments"), usage, options);
+ } else if (!argc) {
+ usage_with_options(usage, options);
+ } else if (argc != 2) {
+ usage_msg_optf(_("only two arguments allowed in <type> <object> mode, not %d"),
+ usage, options, argc);
+ } else if (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");
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 2ce5c8b1824..964b3bc0b40 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -24,7 +24,7 @@ done
test_incompatible_usage () {
test_expect_code 129 "$@" 2>err &&
- grep -E "^error:.**needs" err
+ grep -E "^(fatal|error):.*(requires|incompatible with|needs)" err
}
for opt in --batch --batch-check
@@ -34,48 +34,54 @@ do
'
done
+test_missing_usage () {
+ test_expect_code 129 "$@" 2>err &&
+ grep -E "^fatal:.*required" err
+}
+
short_modes="-e -p -t -s"
cw_modes="--textconv --filters"
for opt in $cw_modes
do
test_expect_success "usage: $opt requires another option" '
- test_expect_code 129 git cat-file $opt
+ test_missing_usage git cat-file $opt
'
done
for opt in $short_modes
do
test_expect_success "usage: $opt requires another option" '
- test_expect_code 129 git cat-file $opt
+ test_missing_usage git cat-file $opt
'
for opt2 in --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_usage git cat-file $opt $opt2
'
done
-
- opt2="--path=foo HEAD:some-path.txt"
- test_expect_success "usage: incompatible options: $opt and $opt2" '
- test_incompatible_usage git cat-file $opt $opt2
- '
done
+test_too_many_arguments () {
+ test_expect_code 129 "$@" 2>err &&
+ grep -E "^fatal: too many arguments$" err
+}
+
for opt in $short_modes $cw_modes
do
args="one two three"
test_expect_success "usage: too many arguments: $opt $args" '
- test_expect_code 129 git cat-file $opt $args
+ test_too_many_arguments git cat-file $opt $args
'
for opt2 in --buffer --follow-symlinks
do
test_expect_success "usage: incompatible arguments: $opt with batch option $opt2" '
- test_expect_code 129 git cat-file $opt $opt2
+ test_incompatible_usage git cat-file $opt $opt2
'
done
done
@@ -84,14 +90,9 @@ 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
+ test_expect_success "usage: bad option combination: $opt without batch mode" '
+ test_incompatible_usage git cat-file $opt &&
+ test_incompatible_usage git cat-file $opt commit HEAD
'
done
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v6 08/10] cat-file: correct and improve usage information
2021-12-28 13:28 ` [PATCH v6 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-12-28 13:28 ` [PATCH v6 07/10] cat-file: fix remaining usage bugs Ævar Arnfjörð Bjarmason
@ 2021-12-28 13:28 ` Ævar Arnfjörð Bjarmason
2022-01-08 2:58 ` Jiang Xin
2021-12-28 13:28 ` [PATCH v6 09/10] object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY Ævar Arnfjörð Bjarmason
2021-12-28 13:28 ` [PATCH v6 10/10] cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters) Ævar Arnfjörð Bjarmason
9 siblings, 1 reply; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 13:28 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/cat-file.c | 49 +++++++++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 20 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 895292074ae..b5b130d79c1 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -666,35 +666,44 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
NULL
};
const struct option options[] = {
- 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()
};
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 101+ messages in thread
* Re: [PATCH v6 08/10] cat-file: correct and improve usage information
2021-12-28 13:28 ` [PATCH v6 08/10] cat-file: correct and improve usage information Ævar Arnfjörð Bjarmason
@ 2022-01-08 2:58 ` Jiang Xin
2022-01-10 22:08 ` [PATCH 0/2] fixups for issues in next-merged ab/cat-file Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 101+ messages in thread
From: Jiang Xin @ 2022-01-08 2:58 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Jeff King, John Cai, Sergey Organov
On Tue, Dec 28, 2021 at 9:28 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> + 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'")),
Found unnecessary space between "--filters" and ")" in the new entries
of "po/git.pot", see:
* https://github.com/git-l10n/git-po/blob/pot/next/2022-01-05.diff#L200
* https://github.com/git-l10n/git-po/blob/pot/next/2022-01-05.diff#L319
They were introduced in the following commits:
* 57d6a1cf96 (cat-file: correct and improve usage information, 2021-12-28)
* 5a40417876 (cat-file: move "usage" variable to cmd_cat_file(), 2021-12-28)
--
Jiang Xin
^ permalink raw reply [flat|nested] 101+ messages in thread
* [PATCH 0/2] fixups for issues in next-merged ab/cat-file
2022-01-08 2:58 ` Jiang Xin
@ 2022-01-10 22:08 ` Ævar Arnfjörð Bjarmason
2022-01-10 22:08 ` [PATCH 1/2] cat-file: don't whitespace-pad "(...)" in SYNOPSIS and usage output Ævar Arnfjörð Bjarmason
` (3 more replies)
0 siblings, 4 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-10 22:08 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jiang Xin, John Cai, Sergey Organov,
Ævar Arnfjörð Bjarmason
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
^ permalink raw reply [flat|nested] 101+ messages in thread
* [PATCH 1/2] cat-file: don't whitespace-pad "(...)" in SYNOPSIS and usage output
2022-01-10 22:08 ` [PATCH 0/2] fixups for issues in next-merged ab/cat-file Ævar Arnfjörð Bjarmason
@ 2022-01-10 22:08 ` Ævar Arnfjörð Bjarmason
2022-01-10 22:08 ` [PATCH 2/2] cat-file: s/_/-/ in typo'd usage_msg_optf() message Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
3 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-10 22:08 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jiang Xin, John Cai, Sergey Organov,
Ævar Arnfjörð Bjarmason
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 <worldhello.net@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/git-cat-file.txt | 4 ++--
builtin/cat-file.c | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 73ebbc70ee2..bef76f4dd06 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -11,11 +11,11 @@ SYNOPSIS
[verse]
'git cat-file' <type> <object>
'git cat-file' (-e | -p) <object>
-'git cat-file' ( -t | -s ) [--allow-unknown-type] <object>
+'git cat-file' (-t | -s) [--allow-unknown-type] <object>
'git cat-file' (--batch | --batch-check) [--batch-all-objects]
[--buffer] [--follow-symlinks] [--unordered]
[--textconv | --filters]
-'git cat-file' (--textconv | --filters )
+'git cat-file' (--textconv | --filters)
[<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]
DESCRIPTION
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index ad9b3eef4f4..e36492235ba 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -653,11 +653,11 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
const char * const usage[] = {
N_("git cat-file <type> <object>"),
N_("git cat-file (-e | -p) <object>"),
- N_("git cat-file ( -t | -s ) [--allow-unknown-type] <object>"),
+ N_("git cat-file (-t | -s) [--allow-unknown-type] <object>"),
N_("git cat-file (--batch | --batch-check) [--batch-all-objects]\n"
" [--buffer] [--follow-symlinks] [--unordered]\n"
" [--textconv | --filters]"),
- N_("git cat-file (--textconv | --filters )\n"
+ N_("git cat-file (--textconv | --filters)\n"
" [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"),
NULL
};
@@ -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()
};
--
2.34.1.1373.g062f5534af2
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH 2/2] cat-file: s/_/-/ in typo'd usage_msg_optf() message
2022-01-10 22:08 ` [PATCH 0/2] fixups for issues in next-merged ab/cat-file Ævar Arnfjörð Bjarmason
2022-01-10 22:08 ` [PATCH 1/2] cat-file: don't whitespace-pad "(...)" in SYNOPSIS and usage output Ævar Arnfjörð Bjarmason
@ 2022-01-10 22:08 ` Ævar Arnfjörð Bjarmason
2022-01-10 22:20 ` [PATCH 0/2] fixups for issues in next-merged ab/cat-file Junio C Hamano
2022-01-11 15:48 ` Taylor Blau
3 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-10 22:08 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jiang Xin, John Cai, Sergey Organov,
Ævar Arnfjörð Bjarmason
Fix a typo in my recent 03dc51fe849 (cat-file: fix remaining usage
bugs, 2021-10-09).
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/cat-file.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index e36492235ba..7b3f42950ec 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -727,7 +727,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
;
else if (batch.follow_symlinks)
usage_msg_optf(_("'%s' requires a batch mode"), usage, options,
- "--follow_symlinks");
+ "--follow-symlinks");
else if (batch.buffer_output >= 0)
usage_msg_optf(_("'%s' requires a batch mode"), usage, options,
"--buffer");
--
2.34.1.1373.g062f5534af2
^ permalink raw reply related [flat|nested] 101+ messages in thread
* Re: [PATCH 0/2] fixups for issues in next-merged ab/cat-file
2022-01-10 22:08 ` [PATCH 0/2] fixups for issues in next-merged ab/cat-file Ævar Arnfjörð Bjarmason
2022-01-10 22:08 ` [PATCH 1/2] cat-file: don't whitespace-pad "(...)" in SYNOPSIS and usage output Ævar Arnfjörð Bjarmason
2022-01-10 22:08 ` [PATCH 2/2] cat-file: s/_/-/ in typo'd usage_msg_optf() message Ævar Arnfjörð Bjarmason
@ 2022-01-10 22:20 ` Junio C Hamano
2022-01-11 15:48 ` Taylor Blau
3 siblings, 0 replies; 101+ messages in thread
From: Junio C Hamano @ 2022-01-10 22:20 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jiang Xin, John Cai, Sergey Organov
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 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).
Both patches look good. Will queue.
^ permalink raw reply [flat|nested] 101+ messages in thread
* Re: [PATCH 0/2] fixups for issues in next-merged ab/cat-file
2022-01-10 22:08 ` [PATCH 0/2] fixups for issues in next-merged ab/cat-file Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2022-01-10 22:20 ` [PATCH 0/2] fixups for issues in next-merged ab/cat-file Junio C Hamano
@ 2022-01-11 15:48 ` Taylor Blau
2022-01-12 18:11 ` Junio C Hamano
3 siblings, 1 reply; 101+ messages in thread
From: Taylor Blau @ 2022-01-11 15:48 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Jiang Xin, John Cai, Sergey Organov
On Mon, Jan 10, 2022 at 11:08:44PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 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.
Thanks; both of these small fixes look fine to me.
Just ACKing in case this topic happens to graduate to master even though
we are already past -rc0.
Taylor
^ permalink raw reply [flat|nested] 101+ messages in thread
* Re: [PATCH 0/2] fixups for issues in next-merged ab/cat-file
2022-01-11 15:48 ` Taylor Blau
@ 2022-01-12 18:11 ` Junio C Hamano
0 siblings, 0 replies; 101+ messages in thread
From: Junio C Hamano @ 2022-01-12 18:11 UTC (permalink / raw)
To: Taylor Blau
Cc: Ævar Arnfjörð Bjarmason, git, Jiang Xin, John Cai,
Sergey Organov
Taylor Blau <me@ttaylorr.com> writes:
> On Mon, Jan 10, 2022 at 11:08:44PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> 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.
>
> Thanks; both of these small fixes look fine to me.
>
> Just ACKing in case this topic happens to graduate to master even though
> we are already past -rc0.
Thanks. I do think these fix-ups should be at the same integration
stage as the earlier patches, but I do not know if the topic itself
should be in the upcoming release.
Let's merge these two to 'next', too.
^ permalink raw reply [flat|nested] 101+ messages in thread
* [PATCH v6 09/10] object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY
2021-12-28 13:28 ` [PATCH v6 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2021-12-28 13:28 ` [PATCH v6 08/10] cat-file: correct and improve usage information Ævar Arnfjörð Bjarmason
@ 2021-12-28 13:28 ` Ævar Arnfjörð Bjarmason
2021-12-28 13:28 ` [PATCH v6 10/10] cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters) Ævar Arnfjörð Bjarmason
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 13:28 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
object-name.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/object-name.c b/object-name.c
index fdff4601b2c..d44a8f3a7ca 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1795,9 +1795,6 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
const char *cp;
int only_to_die = flags & GET_OID_ONLY_TO_DIE;
- if (only_to_die)
- flags |= GET_OID_QUIETLY;
-
memset(oc, 0, sizeof(*oc));
oc->mode = S_IFINVALID;
strbuf_init(&oc->symlink_path, 0);
@@ -1932,7 +1929,7 @@ void maybe_die_on_misspelt_object_name(struct repository *r,
{
struct object_context oc;
struct object_id oid;
- get_oid_with_context_1(r, name, GET_OID_ONLY_TO_DIE,
+ get_oid_with_context_1(r, name, GET_OID_ONLY_TO_DIE | GET_OID_QUIETLY,
prefix, &oid, &oc);
}
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 101+ messages in thread
* [PATCH v6 10/10] cat-file: use GET_OID_ONLY_TO_DIE in --(textconv|filters)
2021-12-28 13:28 ` [PATCH v6 00/10] cat-file: better usage UX & error messages Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2021-12-28 13:28 ` [PATCH v6 09/10] object-name.c: don't have GET_OID_ONLY_TO_DIE imply *_QUIETLY Ævar Arnfjörð Bjarmason
@ 2021-12-28 13:28 ` Ævar Arnfjörð Bjarmason
9 siblings, 0 replies; 101+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 13:28 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, John Cai, Sergey Organov, Jiang Xin,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index b5b130d79c1..ad9b3eef4f4 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -73,14 +73,17 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
struct object_info oi = OBJECT_INFO_INIT;
struct strbuf sb = STRBUF_INIT;
unsigned flags = OBJECT_INFO_LOOKUP_REPLACE;
+ unsigned get_oid_flags = GET_OID_RECORD_PATH | GET_OID_ONLY_TO_DIE;
const char *path = force_path;
+ const int opt_cw = (opt == 'c' || opt == 'w');
+ if (!path && opt_cw)
+ get_oid_flags |= GET_OID_REQUIRE_PATH;
if (unknown_type)
flags |= OBJECT_INFO_ALLOW_UNKNOWN_TYPE;
- if (get_oid_with_context(the_repository, obj_name,
- GET_OID_RECORD_PATH,
- &oid, &obj_context))
+ if (get_oid_with_context(the_repository, obj_name, get_oid_flags, &oid,
+ &obj_context))
die("Not a valid object name %s", obj_name);
if (!path)
@@ -112,9 +115,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
return !has_object_file(&oid);
case 'w':
- if (!path)
- die("git cat-file --filters %s: <object> must be "
- "<sha1:path>", obj_name);
if (filter_object(path, obj_context.mode,
&oid, &buf, &size))
@@ -122,10 +122,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
break;
case 'c':
- if (!path)
- die("git cat-file --textconv %s: <object> must be <sha1:path>",
- obj_name);
-
if (textconv_object(the_repository, path, obj_context.mode,
&oid, 1, &buf, &size))
break;
diff --git a/cache.h b/cache.h
index cfba463aa97..fae55cfcb33 100644
--- a/cache.h
+++ b/cache.h
@@ -1377,6 +1377,7 @@ struct object_context {
#define GET_OID_FOLLOW_SYMLINKS 0100
#define GET_OID_RECORD_PATH 0200
#define GET_OID_ONLY_TO_DIE 04000
+#define GET_OID_REQUIRE_PATH 010000
#define GET_OID_DISAMBIGUATORS \
(GET_OID_COMMIT | GET_OID_COMMITTISH | \
diff --git a/object-name.c b/object-name.c
index d44a8f3a7ca..92862eeb1ac 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1799,6 +1799,9 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
oc->mode = S_IFINVALID;
strbuf_init(&oc->symlink_path, 0);
ret = get_oid_1(repo, name, namelen, oid, flags);
+ if (!ret && flags & GET_OID_REQUIRE_PATH)
+ die(_("<object>:<path> required, only <object> '%s' given"),
+ name);
if (!ret)
return ret;
/*
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index 71ea2ac987e..b067983ba1c 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -29,7 +29,7 @@ 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
@@ -37,7 +37,7 @@ 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
@@ -46,7 +46,7 @@ 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
@@ -55,7 +55,7 @@ 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 HEAD2:one.bin
+ fatal: invalid object name '\''HEAD2'\''.
EOF
test_must_fail git cat-file --textconv HEAD2:one.bin 2>actual &&
test_cmp expect actual
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 101+ messages in thread