Re: [PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude
On Mon, Feb 15, 2016 at 3:15 AM, Duy Nguyen wrote: > On Mon, Feb 15, 2016 at 12:52:26AM -0500, Eric Sunshine wrote: >> 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. > > Dropping 'const' is not a big deal. But before we do that, how about > this instead? I think the code looks better, and the compiler can > still catch invalid updates to deepen_not. I like this better, too. > diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c > index 0402e27..07570be 100644 > --- a/builtin/fetch-pack.c > +++ b/builtin/fetch-pack.c > @@ -50,6 +50,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char > *prefix) > struct child_process *conn; > struct fetch_pack_args args; > struct sha1_array shallow = SHA1_ARRAY_INIT; > + struct string_list deepen_not = STRING_LIST_INIT_DUP; > > packet_trace_identity("fetch-pack"); > > @@ -108,6 +109,10 @@ int cmd_fetch_pack(int argc, const char **argv, const > char *prefix) > args.deepen_since = xstrdup(arg); > continue; > } > + if (skip_prefix(arg, "--shallow-exclude=", &arg)) { > + string_list_append(&deepen_not, arg); > + continue; > + } > if (!strcmp("--no-progress", arg)) { > args.no_progress = 1; > continue; > @@ -135,6 +140,8 @@ int cmd_fetch_pack(int argc, const char **argv, const > char *prefix) > } > usage(fetch_pack_usage); > } > + if (deepen_not.nr) > + args.deepen_not = &deepen_not; > > if (i < argc) > dest = argv[i++]; > -- > Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude
On Mon, Feb 15, 2016 at 12:52:26AM -0500, Eric Sunshine wrote: > 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. Dropping 'const' is not a big deal. But before we do that, how about this instead? I think the code looks better, and the compiler can still catch invalid updates to deepen_not. diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 0402e27..07570be 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -50,6 +50,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) struct child_process *conn; struct fetch_pack_args args; struct sha1_array shallow = SHA1_ARRAY_INIT; + struct string_list deepen_not = STRING_LIST_INIT_DUP; packet_trace_identity("fetch-pack"); @@ -108,6 +109,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) args.deepen_since = xstrdup(arg); continue; } + if (skip_prefix(arg, "--shallow-exclude=", &arg)) { + string_list_append(&deepen_not, arg); + continue; + } if (!strcmp("--no-progress", arg)) { args.no_progress = 1; continue; @@ -135,6 +140,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) } usage(fetch_pack_usage); } + if (deepen_not.nr) + args.deepen_not = &deepen_not; if (i < argc) dest = argv[i++]; -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude
On Mon, Feb 15, 2016 at 12:52 AM, Eric Sunshine wrote: > On Sun, Feb 14, 2016 at 10:53 PM, Duy Nguyen wrote: >> On Fri, Feb 5, 2016 at 12:26 PM, Eric Sunshine >> wrote: >>> 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. > >>> 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. Upon re-reading, I suppose this also is an argument for keeping the static string_list* rather than dropping the 'const'...(?) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude
On Sun, Feb 14, 2016 at 10:53 PM, Duy Nguyen wrote: > On Fri, Feb 5, 2016 at 12:26 PM, Eric Sunshine > wrote: >> On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy >> wrote: >>> Signed-off-by: Nguyễn Thái Ngọc Duy >>> --- >>> 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. >> 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; >> } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude
On Fri, Feb 5, 2016 at 12:26 PM, Eric Sunshine wrote: > On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy > wrote: >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- >> 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. > 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. > if (skip_prefix(arg, "--shallow-exclude=", &value)) { > string_list_append(args.deepen_not, value); > continue; > } -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude
On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy wrote: > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > 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; } 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); ... if (skip_prefix(arg, "--shallow-exclude=", &value)) { string_list_append(args.deepen_not, value); continue; } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html