Re: [PATCH v2] remote: add get-url subcommand

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 4, 2015, 00:45 UTC
Message-ID: <CAPig+cSNNtNP93U6jy3X424FV3SRvoLr3PZaYe3m+auOM+gWcA@mail.gmail.com>
In-Reply-To: <20150804001631.GA10416@megas.kitware.com>

On Mon, Aug 3, 2015 at 8:16 PM, Ben Boeckel <mathstuf@gmail.com> wrote:

Show 22 quoted lines

> On Mon, Aug 03, 2015 at 19:38:15 -0400, Eric Sunshine wrote:
>> On Mon, Aug 3, 2015 at 5:00 PM, Ben Boeckel <mathstuf@gmail.com> wrote:
>> > +       argc = parse_options(argc, argv, NULL, options, builtin_remote_geturl_usage,
>> > +                            PARSE_OPT_KEEP_ARGV0);
>>
>> What is the reason for PARSE_OPT_KEEP_ARGV0 in this case?
>
> Copied from get-url; I presume for more natural argv[] usage within the
> function.
>
>> > +       if (argc < 1 || argc > 2)
>> > +               usage_with_options(builtin_remote_geturl_usage, options);
>>
>> So,  'argc' must be 1 or 2, which in 'argv' terms is argv[0] and argv[1]).
>>
>> > +       remotename = argv[1];
>>
>> But here, argv[1] is accessed unconditionally, even though 'argc' may
>> have been 1, thus out of bounds.
>
> Yep, should be (argc < 2 || argc > 2) (or 1 if PARSE_OPT_KEEP_ARGV0 is
> removed). Off-by-one when converting from get-url.

Or, expressed more naturally:

    if (argc != 1)
        usage_with_options(...);

assuming the unnecessary PARSE_OPT_KEEP_ARGV0 is dropped.

> I'll reroll tomorrow morning in case there are more comments until then
> (particularly about PARSE_OPT_KEEP_ARGV0).

This new code doesn't take advantage of it, and it's very rarely used in Git itself, thus its use here is a potential source of confusion, so it's probably best to drop it. (The same could be said for set_url(), but that's a separate topic.)