Re: [PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 15, 2016, 05:52 UTC
Message-ID: <CAPig+cR01WCgyJQuDcq-j5Z6u3S-LO5kUVuT+g-jdu-hoH-5yw@mail.gmail.com>
In-Reply-To: <CACsJy8B=p0frmU8ahc9bnk-uoDPNUT_6UB0MVRPiLc9DqNz3vQ@mail.gmail.com>

On Sun, Feb 14, 2016 at 10:53 PM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 31 quoted lines

> On Fri, Feb 5, 2016 at 12:26 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>> ---
>>> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
>>> @@ -109,6 +109,16 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>>> +               if (skip_prefix(arg, "--shallow-exclude=", &value)) {
>>> +                       static struct string_list *deepen_not;
>>> +                       if (!deepen_not) {
>>> +                               deepen_not = xmalloc(sizeof(*deepen_not));
>>> +                               string_list_init(deepen_not, 1);
>>> +                               args.deepen_not = deepen_not;
>>> +                       }
>>> +                       string_list_append(deepen_not, value);
>>> +                       continue;
>>> +               }
>>
>> Hmm, can't this be simplified to:
>>
>>     if (skip_prefix(arg, "--shallow-exclude=", &value)) {
>>         if (!args.deepen_not) {
>>             args.deepen_not = xmalloc(sizeof(*args.deepen_not));
>>             string_list_init(args.deepen_not, 1);
>>         }
>>         string_list_append(args.deepen_not, value);
>>         continue;
>>     }
>
> args.deepen_not is const, so no, the compiler will complain at
> string_list_init and string_list_append. Dropping "const" is one
> option, if you prefer.

Yes, dropping 'const' was implied. I didn't examine it too deeply, but it did not appear as if there would be any major fallout from dropping 'const'. It feels a bit cleaner to keep it all self-contained than to have that somewhat oddball static string_list*, but it's not such a big deal that I'd insist upon a rewrite.

Show 12 quoted lines

>> Or, perhaps even better, declare it as plain 'struct string_list
>> deepen_not' in struct fetch_pack_args, rather than as a pointer, and
>> then in cmd_fetch_pack():
>>
>>     memset(&args, 0, sizeof(args));
>>     args.uploadpack = "git-upload-pack";
>>     string_list_init(&args.deepen_not, 1);
>
> There's another place fetch_pack_args variable is declared, in
> fetch_refs_via_pack(), and we would need to string_list_copy() from
> transport->data->options.deepen_not and then free it afterward. So I
> think it's not really worth it.

Okay.

>>     if (skip_prefix(arg, "--shallow-exclude=", &value)) {
>>         string_list_append(args.deepen_not, value);
>>         continue;
>>     }