Re: [PATCH v2 6/6] t5620: test backfill's unknown argument handling

[Derrick Stolee <stolee@gmail.com>]

On 3/23/2026 11:29 AM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <stolee@gmail.com>
>>
>> Before the recent changes to parse rev-list arguments inside of 'git
>> backfill', the builtin would take arbitrary arguments without complaint (and
>> ignore them). This was noticed and a patch was sent [1] which motivates this
>> change to encode this behavior in test.
>>
>> [1] https://lore.kernel.org/git/20260321031643.5185-1-r.siddharth.shrimali@gmail.com/
>>
>> Reported-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
>> Signed-off-by: Derrick Stolee <stolee@gmail.com>
>> ---
>>  t/t5620-backfill.sh | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/t/t5620-backfill.sh b/t/t5620-backfill.sh
>> index c6f54ee91c..85740f1f13 100755
>> --- a/t/t5620-backfill.sh
>> +++ b/t/t5620-backfill.sh
>> @@ -7,6 +7,14 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>>  
>>  . ./test-lib.sh
>>  
>> +test_expect_success 'backfill rejects unexpected arguments' '
>> +	test_must_fail git backfill unexpected-arg 2>err &&
>> +	test_grep "ambiguous argument .*unexpected-arg" err &&
>> +
>> +	test_must_fail git backfill --all --firt-parent unexpected-arg 2>err &&
>> +	test_grep "ambiguous argument .*unexpected-arg" err
>> +'
> 
> Hmph, I would have expected that an earlier --firt-parent on the
> command line would trigger "unknown option" instead.

Interesting that my mistype has demonstrated an interesting
behavior here. It turns out that random options starting with
'--' are accepted here, including --unexpected-arg.

This means that we actually have room here for some improvement!

I'll see what can be done to make even these arguments be seen
as failures.

> Having said that, if the code lets the setup_revisions() parse the
> command line, the usual "unless disambiguated with a double-dash
> '--', stop at the first non-revision and take everything as paths
> but for safety all of them must refer to an existing path in the
> working tree" behaviour should trigger, and it is not specific to
> "backfill", and may already be tested centrally (if not, I do not
> object to such a new set of tests).

Thanks,
-Stolee