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(+)

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

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

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.

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

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

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(+)

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

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)

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

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.

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(+)

On 22/03/2026 05:32, Siddharth Shrimali wrote:

Nicely explained

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

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

Great suggestion.  Thanks.

On 3/22/26 1:32 AM, Siddharth Shrimali wrote:

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

Derrick Stolee <stolee@gmail.com> writes:

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).

On 3/22/26 9:01 PM, Junio C Hamano wrote:

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

On Mon, 23 Mar 2026 at 07:12, Derrick Stolee <stolee@gmail.com> wrote:

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.

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 will drop this patch so it does not get in the way of your work.

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