cat-file: fix remaining usage bugs

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 8735620813460ac6f7e652e0f6a79755d29e8260..895292074aebc9098b5290316fdf329f969e4909 100644 (file)
--- 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 ebec2061d250ccac3c358b202e4aac347f887e10..aa859271d606afd2b05f2af13986fc63a19b6b20 100755 (executable)
--- 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