From: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
Date: Mon, 23 Mar 2026 06:17:52 GMT
Subject: Re: [PATCH v3] backfill: handle unexpected arguments
Message-ID: <CAGWgyh8E-=A+NKEAOz3xHPq96rf3j0tt7++3xvg7NoMCKq_Www@mail.gmail.com>
In-Reply-To: <6460601f-ff72-4683-abd1-2ae4c8352a27@gmail.com>

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