[PATCH] backfill: handle unexpected arguments

[PATCH] backfill: handle unexpected arguments

[Siddharth Shrimali]

git backfill takes no non-option arguments. However, if extra
arguments are passed with git backfill, parse_options() leaves
them in argc and the command currently ignores them silently,
giving the user no indication that something is wrong.

Add a check after parse_options() to call usage_with_options()
if any unexpected arguments remain. This prints the correct usage
and exits with an error, consistent with how other Git commands
such as git-gc and git-repack handle this situation.

Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 builtin/backfill.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/backfill.c b/builtin/backfill.c
index e9a33e81be..0eb171478a 100644
--- a/builtin/backfill.c
+++ b/builtin/backfill.c
@@ -135,6 +135,9 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
 
 	argc = parse_options(argc, argv, prefix, options, builtin_backfill_usage,
 			     0);
+	
+	if (argc)
+		usage_with_options(builtin_backfill_usage, options);
 
 	repo_config(repo, git_default_config, NULL);
 
-- 
2.51.2

Re: [PATCH] backfill: handle unexpected arguments

[Junio C Hamano]

Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:

> git backfill takes no non-option arguments. However, if extra
> arguments are passed with git backfill, parse_options() leaves
> them in argc and the command currently ignores them silently,
> giving the user no indication that something is wrong.

Well written.  If you drop unnecessary "currently", it would be
perfect ;-).

> Add a check after parse_options() to call usage_with_options()
> if any unexpected arguments remain. This prints the correct usage
> and exits with an error, consistent with how other Git commands
> such as git-gc and git-repack handle this situation.

I am not sure if this is a good idea.

When parse_options() finds an unrecognised option, you would get
usage-with-options help, so without explicitly telling the user
"Hey, you have an extra argument that I do not expect at the end of
the command line" and giving only the same usage-with-options help,
the user would not know why they are seeing the help message, as it
is totally unclear what mistake they made in their command line.

"git bugreport" is also a command that does not take any positional
arguments on its command line.  Study how it complains about an
unwanted argument, and follow its example, perhaps?

Thanks.

> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
> ---
>  builtin/backfill.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/builtin/backfill.c b/builtin/backfill.c
> index e9a33e81be..0eb171478a 100644
> --- a/builtin/backfill.c
> +++ b/builtin/backfill.c
> @@ -135,6 +135,9 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
>  
>  	argc = parse_options(argc, argv, prefix, options, builtin_backfill_usage,
>  			     0);
> +	
> +	if (argc)
> +		usage_with_options(builtin_backfill_usage, options);
>  
>  	repo_config(repo, git_default_config, NULL);

Re: [PATCH] backfill: handle unexpected arguments

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I am not sure if this is a good idea.
>
> When parse_options() finds an unrecognised option, you would get
> usage-with-options help, so without explicitly telling the user
> "Hey, you have an extra argument that I do not expect at the end of
> the command line" and giving only the same usage-with-options help,
> the user would not know why they are seeing the help message, as it
> is totally unclear what mistake they made in their command line.
>
> "git bugreport" is also a command that does not take any positional
> arguments on its command line.  Study how it complains about an
> unwanted argument, and follow its example, perhaps?
>
> Thanks.
>
>> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
>> ---
>>  builtin/backfill.c | 3 +++
>>  1 file changed, 3 insertions(+)

One thing I forgot.  You may want to add a test for this.

[PATCH v2] backfill: handle unexpected arguments

[Siddharth Shrimali]

git backfill takes no non-option arguments. However, if extra
arguments are passed with git backfill, parse_options() leaves
them in argc and the command ignores them silently, giving the
user no indication that something is wrong.

Add a check after parse_options() to call usage_with_options()
if any unexpected arguments remain. To ensure the user understands
why the command failed, print an error message specifying the unknown
argument before showing the usage string. This is consistent with how
other Git commands such as git-bugreport handle this situation.

Also, add a test in t5620 to ensure the unexpected arguments are
rejected with the correct error message.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
Changes in v2:
- Dropped the word "currently" from the commit message as per 
  Junio's feedback.
- Added an `error()` call before `usage_with_options()` to state which 
  argument was unknown, following the pattern in `git bugreport`.
- Added a test case in `t5620-backfill.sh` to verify the new error output.

 builtin/backfill.c  | 5 +++++
 t/t5620-backfill.sh | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/builtin/backfill.c b/builtin/backfill.c
index e9a33e81be..5a333afde0 100644
--- a/builtin/backfill.c
+++ b/builtin/backfill.c
@@ -135,6 +135,11 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
 
 	argc = parse_options(argc, argv, prefix, options, builtin_backfill_usage,
 			     0);
+	
+	if (argc) {
+		error(_("unknown argument `%s'"), argv[0]);
+		usage_with_options(builtin_backfill_usage, options);
+	}
 
 	repo_config(repo, git_default_config, NULL);
 
diff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh
index 58c81556e7..d74e1be74b 100755
--- a/t/t5620-backfill.sh
+++ b/t/t5620-backfill.sh
@@ -176,6 +176,11 @@ test_expect_success 'backfill --sparse without cone mode (negative)' '
 	test_line_count = 12 missing
 '
 
+test_expect_success 'backfill rejects unexpected arguments' '
+	test_must_fail git -C backfill1 backfill unexpected-arg 2>err &&
+	grep "unknown argument .*unexpected-arg" err
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
-- 
2.51.2

Re: [PATCH v2] backfill: handle unexpected arguments

[Junio C Hamano]

Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:

> git backfill takes no non-option arguments. However, if extra
> arguments are passed with git backfill, parse_options() leaves
> them in argc and the command ignores them silently, giving the
> user no indication that something is wrong.
>
> Add a check after parse_options() to call usage_with_options()
> if any unexpected arguments remain. To ensure the user understands
> why the command failed, print an error message specifying the unknown
> argument before showing the usage string. This is consistent with how
> other Git commands such as git-bugreport handle this situation.

While it is a thing to aim for, I doubt the updated behaviour is
consistent with how bugreport behaves.

	$ git bugreport extra-arg
        error: unknown argument `extra-arg'
        usage: git bugreport [(-o | --output-directory) <path>]
                      [(-s | --suffix) <format> | --no-suffix]
                      [--diagnose[=<mode>]]

I.e. the command does not show the list of options with descriptions
to the user, who did not make any mistake in specifying any dashed
option.  On the other hand,

	$ git bugreport --mistyped-option

would give usage_with_options(), because unlike the case where the
user gave only an extra argument, it is clear in this case that the
user failed to choose the right option, which might be helped if we
tell them what each option does.  Also with

	$ git bugreport --suffix

the user will get a more targetted help that is specific to the
option given incorrectly.

Compared to that, the way the updated code would react to

	$ git backfill extra-arg

cannot be called "consistent with how" bugreport handles this
situation, I would have to say.  I didn't check how the command
behaves in the other two cases (i.e., option that does not exist,
and an option that needs a value but the user forgot to give one)


> diff --git a/builtin/backfill.c b/builtin/backfill.c
> index e9a33e81be..5a333afde0 100644
> --- a/builtin/backfill.c
> +++ b/builtin/backfill.c
> @@ -135,6 +135,11 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
>  
>  	argc = parse_options(argc, argv, prefix, options, builtin_backfill_usage,
>  			     0);
> +	
> +	if (argc) {
> +		error(_("unknown argument `%s'"), argv[0]);

Yikes.  Please do not add more uses of cute quotes.

Perhaps we found a microproject target for the next intern season?
Let's not spread cute quotes any more than what we already have, and
clean existing up ones before it becomes too late.

    $ git grep -e "'%s'" \*.c | wc -l
    2012
    $ git grep -e "\`%s'" \*.c | wc -l
    16

> +		usage_with_options(builtin_backfill_usage, options);
> +	}
>  
>  	repo_config(repo, git_default_config, NULL);
>  
> diff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh
> index 58c81556e7..d74e1be74b 100755
> --- a/t/t5620-backfill.sh
> +++ b/t/t5620-backfill.sh
> @@ -176,6 +176,11 @@ test_expect_success 'backfill --sparse without cone mode (negative)' '
>  	test_line_count = 12 missing
>  '
>  
> +test_expect_success 'backfill rejects unexpected arguments' '
> +	test_must_fail git -C backfill1 backfill unexpected-arg 2>err &&
> +	grep "unknown argument .*unexpected-arg" err

The test is a bit underspecified, isn't it?

This test will still pass if the code called usage_with_options() to
show the list of option descriptions after the short usage, instead
of showing just the short usage by calling usage().

As with any test, it is important to test that the output has what
we expect to see, but at the same time we need to ensure that the
output does not have what we do not want to see.

> +'
> +
>  . "$TEST_DIRECTORY"/lib-httpd.sh
>  start_httpd

[PATCH v3] backfill: handle unexpected arguments

[Siddharth Shrimali]

git backfill takes no non-option arguments. However, if extra
arguments are passed with git backfill, parse_options() leaves
them in argc and the command ignores them silently, giving the
user no indication that something is wrong.

Add a check after parse_options() to report an error if any unexpected
arguments remain. To ensure the user understands why the command
failed, print an error message specifying the unknown argument
followed by the short usage string. This matches the behavior of
other Git commands such as git bugreport.

Also, add a test in t5620 to ensure the unexpected arguments are
rejected with the correct error message and that the full option
descriptions are not printed.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
Changes since v2:
- Replaced the backtick (`%s') with standard single quotes ('%s').
- Swapped `usage_with_options()` for `usage(builtin_backfill_usage[0])` 
  so the user only sees the short usage string instead of the full 
  option descriptions.
- Updated the test to also verify that the full option descriptions 
  are not printed.

 builtin/backfill.c  | 5 +++++
 t/t5620-backfill.sh | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/builtin/backfill.c b/builtin/backfill.c
index e9a33e81be..6d90db3da0 100644
--- a/builtin/backfill.c
+++ b/builtin/backfill.c
@@ -135,6 +135,11 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
 
 	argc = parse_options(argc, argv, prefix, options, builtin_backfill_usage,
 			     0);
+	
+	if (argc) {
+		error(_("unknown argument '%s'"), argv[0]);
+		usage(builtin_backfill_usage[0]);
+	}
 
 	repo_config(repo, git_default_config, NULL);
 
diff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh
index 58c81556e7..3f1eeb67e8 100755
--- a/t/t5620-backfill.sh
+++ b/t/t5620-backfill.sh
@@ -176,6 +176,12 @@ test_expect_success 'backfill --sparse without cone mode (negative)' '
 	test_line_count = 12 missing
 '
 
+test_expect_success 'backfill rejects unexpected arguments' '
+	test_must_fail git -C backfill1 backfill unexpected-arg >err 2>&1 &&
+	grep "unknown argument .*unexpected-arg" err &&
+	! grep "Minimum number of objects" err
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
-- 
2.51.2

Re: [PATCH v3] backfill: handle unexpected arguments

[Phillip Wood]

On 22/03/2026 05:32, Siddharth Shrimali wrote:
> git backfill takes no non-option arguments. However, if extra
> arguments are passed with git backfill, parse_options() leaves
> them in argc and the command ignores them silently, giving the
> user no indication that something is wrong.
> 
> Add a check after parse_options() to report an error if any unexpected
> arguments remain. To ensure the user understands why the command
> failed, print an error message specifying the unknown argument
> followed by the short usage string. This matches the behavior of
> other Git commands such as git bugreport.
> 
> Also, add a test in t5620 to ensure the unexpected arguments are
> rejected with the correct error message and that the full option
> descriptions are not printed.

Nicely explained

> diff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh
> index 58c81556e7..3f1eeb67e8 100755
> --- a/t/t5620-backfill.sh
> +++ b/t/t5620-backfill.sh
> @@ -176,6 +176,12 @@ test_expect_success 'backfill --sparse without cone mode (negative)' '
>   	test_line_count = 12 missing
>   '
>   
> +test_expect_success 'backfill rejects unexpected arguments' '
> +	test_must_fail git -C backfill1 backfill unexpected-arg >err 2>&1 &&
> +	grep "unknown argument .*unexpected-arg" err &&
> +	! grep "Minimum number of objects" err

Using test_grep would make test failures easier to debug as it prints a 
diagnostic message if it fails. Note that "! grep" should become 
"test_grep !" to ensure the diagnostic message is printed when the 
expression matches.

Thanks

Phillip


> +'
> +
>   . "$TEST_DIRECTORY"/lib-httpd.sh
>   start_httpd
>   

Re: [PATCH v3] backfill: handle unexpected arguments

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>> +test_expect_success 'backfill rejects unexpected arguments' '
>> +	test_must_fail git -C backfill1 backfill unexpected-arg >err 2>&1 &&
>> +	grep "unknown argument .*unexpected-arg" err &&
>> +	! grep "Minimum number of objects" err
>
> Using test_grep would make test failures easier to debug as it prints a 
> diagnostic message if it fails. Note that "! grep" should become 
> "test_grep !" to ensure the diagnostic message is printed when the 
> expression matches.
>
> Thanks

Great suggestion.  Thanks.

Re: [PATCH v3] backfill: handle unexpected arguments

[Derrick Stolee]

On 3/22/26 1:32 AM, Siddharth Shrimali wrote:
> git backfill takes no non-option arguments. However, if extra
> arguments are passed with git backfill, parse_options() leaves
> them in argc and the command ignores them silently, giving the
> user no indication that something is wrong.

> @@ -135,6 +135,11 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
>   
>   	argc = parse_options(argc, argv, prefix, options, builtin_backfill_usage,
>   			     0);
> +	
> +	if (argc) {
> +		error(_("unknown argument '%s'"), argv[0]);
> +		usage(builtin_backfill_usage[0]);
> +	}

Before we get too far into this: How does this interact with
the ongoing change to introduce revision arguments to 'git
backfill' [1]? I suppose that the important bit would be that
we still parse arguments using the revision walk machinery at
some point, but it would be difficult to guarantee that when
working on a patch disconnected from that series.

[1] https://lore.kernel.org/git/pull.2070.git.1773707361.gitgitgadget@gmail.com/

Thanks,
-Stolee

Re: [PATCH v3] backfill: handle unexpected arguments

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

>> +	if (argc) {
>> +		error(_("unknown argument '%s'"), argv[0]);
>> +		usage(builtin_backfill_usage[0]);
>> +	}
>
> Before we get too far into this: How does this interact with
> the ongoing change to introduce revision arguments to 'git
> backfill' [1]?

Ahh, that one completely slipped my mind.

Thanks for a doze of sanity.  This patch becomes completely
irrelevant if we are taking command line arguments.

It will become the responsibility of the other topic to detect and
complain about excess command line parameters (unless the feature it
adds absorbs all of them, which may be the case).

> [1] https://lore.kernel.org/git/pull.2070.git.1773707361.gitgitgadget@gmail.com/

Re: [PATCH v3] backfill: handle unexpected arguments

[Derrick Stolee]

On 3/22/26 9:01 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>>> +	if (argc) {
>>> +		error(_("unknown argument '%s'"), argv[0]);
>>> +		usage(builtin_backfill_usage[0]);
>>> +	}
>>
>> Before we get too far into this: How does this interact with
>> the ongoing change to introduce revision arguments to 'git
>> backfill' [1]?
> 
> Ahh, that one completely slipped my mind.
> 
> Thanks for a doze of sanity.  This patch becomes completely
> irrelevant if we are taking command line arguments.
> 
> It will become the responsibility of the other topic to detect and
> complain about excess command line parameters (unless the feature it
> adds absorbs all of them, which may be the case).

At the end of my series, the error output for an unknown argument now
looks like this:

   fatal: ambiguous argument 'unexpected-arg': unknown revision or
   path not in the working tree.

I'm not sure it's worth updating this, but I can incorporate a test
that shows that this is handled.

Thanks,
-Stolee

Re: [PATCH v3] backfill: handle unexpected arguments

[Siddharth Shrimali]

On Mon, 23 Mar 2026 at 07:12, Derrick Stolee <stolee@gmail.com> wrote:
>
> On 3/22/26 9:01 PM, Junio C Hamano wrote:
> > Derrick Stolee <stolee@gmail.com> writes:
> >
> >>> +   if (argc) {
> >>> +           error(_("unknown argument '%s'"), argv[0]);
> >>> +           usage(builtin_backfill_usage[0]);
> >>> +   }
> >>
> >> Before we get too far into this: How does this interact with
> >> the ongoing change to introduce revision arguments to 'git
> >> backfill' [1]?
> >
> > Ahh, that one completely slipped my mind.
> >
> > Thanks for a doze of sanity.  This patch becomes completely
> > irrelevant if we are taking command line arguments.
> >
> > It will become the responsibility of the other topic to detect and
> > complain about excess command line parameters (unless the feature it
> > adds absorbs all of them, which may be the case).
>

Thank you for pointing this out, and apologies for missing the in-flight series.
I agree that this patch becomes irrelevant given the revision arguments work,
and it should be dropped.

> At the end of my series, the error output for an unknown argument now
> looks like this:
>
>    fatal: ambiguous argument 'unexpected-arg': unknown revision or
>    path not in the working tree.
>
That makes sense. Since backfill will now be passing arguments down to
the revision walking machinery, falling back to the standard revision parsing
error is exactly the right behavior.

> I'm not sure it's worth updating this,

I will drop this patch so it does not get in the way of your work.

> but I can incorporate a test
> that shows that this is handled.
>
> Thanks,
> -Stolee

Regarding the test, I think it would be worth adding one to your series to
explicitly verify the behavior for unexpected arguments, since the error message
from the revision walk machinery is less obvious to users than a direct
"unknown argument" message. But I will leave that decision to you.


Thanks also to Phillip Wood for the test_grep suggestion, and to Junio for the
guidance throughout this thread.

Thanks,
Siddharth