Re: [PATCHv3 6/7] submodule--helper: add remote-branch helper

From: Stefan Beller <sbeller@google.com>
Date: Aug 3, 2016, 20:11 UTC
Message-ID: <CAGZ79kYW0dwUh=QpKL9W+cv8k_Z9Vc2wxoLMqud5brDtS9DYpA@mail.gmail.com>
In-Reply-To: <20160803162512.veyff5g52kaqwlgx@sigill.intra.peff.net>

On Wed, Aug 3, 2016 at 9:25 AM, Jeff King <peff@peff.net> wrote:

Show 22 quoted lines

> On Thu, Jul 28, 2016 at 05:44:08PM -0700, Stefan Beller wrote:
>
>> +static const char *remote_submodule_branch(const char *path)
>> +{
>> +     const struct submodule *sub;
>> +     gitmodules_config();
>> +     git_config(submodule_config, NULL);
>> +
>> +     sub = submodule_from_path(null_sha1, path);
>> +     if (!sub->branch)
>> +             return "master";
>> +
>> +     return sub->branch;
>> +}
>
> Coverity complains about "sub" being NULL here, and indeed, it seems
> like an easy segfault:
>
>   $ ./git submodule--helper remote-branch foo
>   Segmentation fault
>
> I guess this should return NULL in that case. But then this...

I thought about checking for (!sub && !sub->branch) to return "master"; However I disregarded that implementation as the submodule-config is reliable for any correct input. Though we need to handle wrong input as well. So how about:

  if (!sub)
    die(_("A submodule doesn't exist at path %s"), path);
  if (!sub->branch)
    return "master";

...

I think that explains best why we even check for !sub.

Show 16 quoted lines

>
>> +static int resolve_remote_submodule_branch(int argc, const char **argv,
>> +             const char *prefix)
>> +{
>> +     struct strbuf sb = STRBUF_INIT;
>> +     if (argc != 2)
>> +             die("submodule--helper remote-branch takes exactly one arguments, got %d", argc);
>> +
>> +     printf("%s", remote_submodule_branch(argv[1]));
>> +     strbuf_release(&sb);
>> +     return 0;
>> +}
>
> would need to handle the NULL return, as well. So maybe "master" or the
> empty string would be better. I haven't followed the topic closely
> enough to have an opinion.

Oh I see, this can turn into a discussion what the API for remote_submodule_branch is. I think handling the NULL here and dying makes more sense as then we can keep remote_submodule_branch pure to its intended use case. (If in the far future we have all the submodule stuff in C, we may want to call remote_submodule_branch when we already know that the path is valid, so no need to check it in there. Also not dying in there is more libified.)

Thanks for this review, Stefan

>
> -Peff