From: Stefan Beller <sbeller@google.com>
Date: Wed, 03 Aug 2016 20:11:51 GMT
Subject: Re: [PATCHv3 6/7] submodule--helper: add remote-branch helper
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:
> 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.


>
>> +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