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

[Jeff King <peff@peff.net>]

On Wed, Aug 03, 2016 at 01:11:51PM -0700, Stefan Beller wrote:

> > 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";

OK by me, as long as the caller of submodule--helper handles the death
reasonably. Since it's an internal helper program, we really only have
to worry about what git-submodule.sh does with it, which is good.

> > 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.)

Yep, that makes sense to me, too.

-Peff