Re: [PATCHv4] clone --single: limit the fetch refspec to fetched branch

From: Ralf Thielow <ralf.thielow@gmail.com>
Date: Sep 17, 2012, 13:25 UTC
Message-ID: <CAN0XMOJrFOWR4_Cdx5fJz2zKCf=mSKBKV16A0dy1f5DptdOMVA@mail.gmail.com>
In-Reply-To: <CACsJy8Bsps_-RaXdBd9ipWPvJukjK7mRN_wG2ezEL5d4JLyx+g@mail.gmail.com>

On Mon, Sep 17, 2012 at 2:06 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:

Show 8 quoted lines

> On Sun, Sep 16, 2012 at 3:13 PM, Ralf Thielow <ralf.thielow@gmail.com> wrote:
>> +       if (option_mirror || !option_bare) {
>> +               strbuf_reset(&value);
>
> I think we should use a new strbuf local variable here to avoid
> resetting this. At least reviewers don't have to check if this
> statememt causes any effect later on because "value"'s value is gone.
>

It seems that we don't need this reset here because it's already done earlier in this function. The variable "key" is also used multiple times so I wouldn't use a new variable.

Show 20 quoted lines

>> +               if (option_single_branch) {
>> +                       if (option_branch)
>> +                               strbuf_addf(&value, "+%s%s:%s%s",
>> +                                               src_ref_prefix, option_branch,
>> +                                               branch_top.buf, option_branch);
>> +                       else if (remote_head_points_at)
>> +                               strbuf_addf(&value, "+%s:%s%s",
>> +                                               remote_head_points_at->name, branch_top.buf,
>> +                                               skip_prefix(remote_head_points_at->name, "refs/heads/"));
>> +                       /*
>> +                        * otherwise, the next "git fetch" will
>> +                        * simply fetch from HEAD without updating
>> +                        * any remote tracking branch, which is what
>> +                        * we want.
>> +                        */
>
> Maybe document updates too? Though if it's obvious that
> --single-branch should prepare refspec so that only one branch is
> fetched later on, then maybe not.
>

I think it's obvious.

Show 22 quoted lines

>> +               } else {
>> +                       strbuf_addf(&value, "+%s*:%s*", src_ref_prefix, branch_top.buf);
>> +               }
>
> --mirror --single-branch combination does not look right. The "heads/"
> part is missing..
>
> $ git branch
>   master * wildmatch
> $ LANG=C ./git clone --mirror --single-branch .git abc
> Cloning into bare repository 'abc'...
> done.
> $ grep fetch abc/config
>         fetch = +refs/heads/wildmatch:refs/wildmatch
> $ rm -rf abc
> $ LANG=C ./git clone --mirror --single-branch --branch=master .git abc
> Cloning into bare repository 'abc'...
> done.
> $ grep fetch abc/config
>         fetch = +refs/master:refs/master
> --
> Duy

Thanks, I'll check this later and send a new version.