Re: [PATCH] clone: do not segfault when specifying a nonexistent branch

From: Ralf Thielow <ralf.thielow@gmail.com>
Date: Oct 9, 2013, 16:38 UTC
Message-ID: <CAN0XMOLRt=kJPNhNDSfY_oMV90Xp=_wY=yf0OhiAcaShhWLCnA@mail.gmail.com>
In-Reply-To: <CACsJy8BX_fWdsCGa4jnh4CbkSMxp7btOFjwzB9K0eRtjUR_F-Q@mail.gmail.com>

On Sat, Oct 5, 2013 at 1:55 AM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 31 quoted lines

> On Fri, Oct 4, 2013 at 9:20 PM, Stefan Beller
> <stefanbeller@googlemail.com> wrote:
>> I think we should emit a warning additionally?
>>
>> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
>
> I think it's nice to credit Robert for reporting the fault in the
> commit message (something like "reported-by:" or "noticed-by:"...)
>
>> ---
>>  builtin/clone.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index 0aff974..b764ad0 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -688,7 +688,7 @@ static void write_refspec_config(const char* src_ref_prefix,
>>
>>         if (option_mirror || !option_bare) {
>>                 if (option_single_branch && !option_mirror) {
>> -                       if (option_branch) {
>> +                       if (option_branch && our_head_points_at) {
>>                                 if (strstr(our_head_points_at->name, "refs/tags/"))
>>                                         strbuf_addf(&value, "+%s:%s", our_head_points_at->name,
>>                                                 our_head_points_at->name);
>
> This prevents the segfault, but what about remote.*.fetch? Should we
> setup standard refspec for fetch or..?
> --
> Duy

This segfault only happens when cloning an empty repository and only with option "--single-branch". Or do I miss something?

If we call "git clone" for a non-empty repository with a non-existing branch using "[--single-branch] --branch foo" then Git will abort with a message that the branch doesn't exist in upstream.

In an empty upstream repo the branch doesn't exist, either. So why not abort with the same message? That would be consistent. Otherwise I'd just override the options "--single-branch" and "--branch" to "not set".

Ralf