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

From: Jacob Keller <jacob.keller@gmail.com>
Date: Feb 29, 2016, 19:34 UTC
Message-ID: <CA+P7+xryNo=n0fFn9HNrj78z4XtWwoF=j67+Qy8TbO7reBTO-Q@mail.gmail.com>
In-Reply-To: <xmqqr3fvcsoy.fsf@gitster.mtv.corp.google.com>

On Mon, Feb 29, 2016 at 9:49 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

> Jacob Keller <jacob.e.keller@intel.com> writes:
>> Fix the usage description to match implementation. Add an argc check to
>> enforce no extra arguments.
>
> The above sounds very sensible.
>

Right.

Show 8 quoted lines

>> Fix a bug in the argument passing in
>> git-submodule.sh which would pass --reference and --depth as empty
>> strings when they were unused, resulting in extra argc after parsing
>> options.
>
> 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?

Show 8 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\