Re: [PATCH v5 1/3] sumodule--helper: fix submodule--helper clone usage and check argc count

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 29, 2016, 19:44 UTC
Message-ID: <xmqqegbv9u9a.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CA+P7+xryNo=n0fFn9HNrj78z4XtWwoF=j67+Qy8TbO7reBTO-Q@mail.gmail.com>

Jacob Keller <jacob.keller@gmail.com> writes:

Show 6 quoted lines

>> This does make sense but it is an unrelated fix.  Perhaps split this
>> patch into two?
>
> That actually is required because otherwise adding a check for argc
> would break the things. I could split them and do this first and then
> check for argc if you really prefer?

It is not "check for argc breaks", it is already broken and by checking for argc you are exposing the breakage, no?

So I'd say fix that first and then fix the clone subcommand?

Show 14 quoted lines

>
>>> +     if (argc)
>>> +             usage(*git_submodule_helper_usage);
>>> +
>>
>> That asterisk looks very unusual and wanting to be future-proofed
>> (i.e. who says that only the first entry matters?).  Should't this
>> be calling usage_with_options()?
>>
>
> I... didn't know usage_with_options was a thing! Hah. I can fix these up.
>
> Thanks,
> Jake\