Re: git bugreport with invalid CLI argument does not report error

[Emily Shaffer <nasamuffin@google.com>]

On Thu, Oct 26, 2023 at 07:59:16AM +1100, Sheik wrote:
> 
> Hi Maintainers,
> 
> 
> Running git bugreport with an invalid CLI argument in a valid Git directory
> does not report error. Expected behaviour would be that it reports an error.
> 
> 
> #Example git commands which should have reported an error but continues to
> succeed
> 
> cd $ToAnyDirectory
> 
> git bugreport diagnose

It looks like parse-options.[ch] helps us here for misspelled dashed
options, like `--diaggnose`. But it doesn't complain when there are
unexpected positional arguments. I think we can just notice if there are
any argc left over, complain, and print usage.

I put together a quick patch; could be that we don't need to leave this
error about "positional arguments" and can leave it as an exercise to
the reader to compare their previous command to the usage text. I guess
we could also unroll remaining argv but it was just a hair more time
than I wanted to spend ;)

 - Emily

--- 8< ---

From 2031c7f55652559b8b4ec3c67ce4c4f94a355762 Mon Sep 17 00:00:00 2001
From: Emily Shaffer <nasamuffin@google.com>
Date: Wed, 25 Oct 2023 15:45:25 -0700
Subject: [PATCH] bugreport: reject positional arguments

git-bugreport already rejected unrecognized flag arguments, like
`--diaggnose`, but this doesn't help if the user's mistake was to forget
the `--` in front of the argument. This can result in a user's intended
argument not being parsed with no indication to the user that something
went wrong. Since git-bugreport presently doesn't take any positionals
at all, let's reject all positionals and give the user a usage hint.

Signed-off-by: Emily Shaffer <nasamuffin@google.com>
---
 builtin/bugreport.c  | 5 +++++
 t/t0091-bugreport.sh | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index d2ae5c305d..eb6234a50d 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -126,6 +126,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, bugreport_options,
 			     bugreport_usage, 0);
 
+	if (argc) {
+		error(_("git bugreport does not take positional arguments"));
+		usage(bugreport_usage[0]);
+	}
+
 	/* Prepare the path to put the result */
 	prefixed_filename = prefix_filename(prefix,
 					    option_output ? option_output : "");
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index f6998269be..2061d6f386 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -69,6 +69,12 @@ test_expect_success 'incorrect arguments abort with usage' '
 	test_path_is_missing git-bugreport-*
 '
 
+test_expect_success 'incorrect positional arguments abort with usage' '
+	test_must_fail git bugreport false 2>output &&
+	test_i18ngrep usage output &&
+	test_path_is_missing git-bugreport-*
+'
+
 test_expect_success 'runs outside of a git dir' '
 	test_when_finished rm non-repo/git-bugreport-* &&
 	nongit git bugreport
-- 
2.42.0.758.gaed0368e0e-goog