# [PATCH v2 1/1] Don't free remote->name after fetch 4 messages from 2016-06-16 to 2016-06-16. Participants: kmcguigan@twopensource.com, Junio C Hamano, Keith McGuigan. Thread: https://gitlist.dev/t/42610 ## kmcguigan@twopensource.com, 2016-06-14 18:28 Subject: [PATCH v2 1/1] Don't free remote->name after fetch Message-ID: <1465928936-68866-1-git-send-email-kmcguigan@twopensource.com> URL: https://gitlist.dev/e/1465928936-68866-1-git-send-email-kmcguigan%40twopensource.com ``` From: Keith McGuigan Make fetch's string_list of remote names owns all of its string items (strdup'ing when necessary) so that it can deallocate them safely when clearing. --- builtin/fetch.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 630ae6a1bb78..1b4e924bd222 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1071,7 +1071,7 @@ static int get_remote_group(const char *key, const char *value, void *priv) size_t wordlen = strcspn(value, " \t\n"); if (wordlen >= 1) - string_list_append(g->list, + string_list_append_nodup(g->list, xstrndup(value, wordlen)); value += wordlen + (value[wordlen] != '\0'); } @@ -1261,7 +1261,7 @@ done: int cmd_fetch(int argc, const char **argv, const char *prefix) { int i; - struct string_list list = STRING_LIST_INIT_NODUP; + struct string_list list = STRING_LIST_INIT_DUP; struct remote *remote; int result = 0; struct argv_array argv_gc_auto = ARGV_ARRAY_INIT; @@ -1347,8 +1347,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) argv_array_clear(&options); } - /* All names were strdup()ed or strndup()ed */ - list.strdup_strings = 1; string_list_clear(&list, 0); close_all_packs(); -- 2.8.0.rc3.94.g2dc3105.dirty ``` ## Junio C Hamano, 2016-06-14 18:49 Subject: Re: [PATCH v2 1/1] Don't free remote->name after fetch Message-ID: URL: https://gitlist.dev/e/xmqqvb1b8v2w.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1465928936-68866-1-git-send-email-kmcguigan@twopensource.com> ``` kmcguigan@twopensource.com writes: > From: Keith McGuigan > > Make fetch's string_list of remote names owns all of its string items > (strdup'ing when necessary) so that it can deallocate them safely when > clearing. > > --- OK. When I pointed out the call to string_list_append() in get_remote_group() as one example, I did not check if there are others that need similar adjustment. I just skimmed through the file again and it seems there is none, so this change looks good. I assume you meant to sign this off, too? > builtin/fetch.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index 630ae6a1bb78..1b4e924bd222 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1071,7 +1071,7 @@ static int get_remote_group(const char *key, const char *value, void *priv) > size_t wordlen = strcspn(value, " \t\n"); > > if (wordlen >= 1) > - string_list_append(g->list, > + string_list_append_nodup(g->list, > xstrndup(value, wordlen)); > value += wordlen + (value[wordlen] != '\0'); > } > @@ -1261,7 +1261,7 @@ done: > int cmd_fetch(int argc, const char **argv, const char *prefix) > { > int i; > - struct string_list list = STRING_LIST_INIT_NODUP; > + struct string_list list = STRING_LIST_INIT_DUP; > struct remote *remote; > int result = 0; > struct argv_array argv_gc_auto = ARGV_ARRAY_INIT; > @@ -1347,8 +1347,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) > argv_array_clear(&options); > } > > - /* All names were strdup()ed or strndup()ed */ > - list.strdup_strings = 1; > string_list_clear(&list, 0); > > close_all_packs(); ``` ## Keith McGuigan, 2016-06-14 18:54 Subject: Re: [PATCH v2 1/1] Don't free remote->name after fetch Message-ID: URL: https://gitlist.dev/e/CALnYDJObFtrKFBRbbaA2KV257j8L8gcKcGk-Pkhsyvbk7%2BoTjQ%40mail.gmail.com In-Reply-To: ``` Yeah that was the only place I found where it was doing the strdup already (and in that situation, it has to). All the other places just grabbed remote->name. Yes, sorry, I can sign off on this. Do you want me to resend with the header in place, or is this confirmation good enough? -- - Keith On Tue, Jun 14, 2016 at 2:49 PM, Junio C Hamano wrote: > kmcguigan@twopensource.com writes: > >> From: Keith McGuigan >> >> Make fetch's string_list of remote names owns all of its string items >> (strdup'ing when necessary) so that it can deallocate them safely when >> clearing. >> >> --- > > OK. > > When I pointed out the call to string_list_append() in > get_remote_group() as one example, I did not check if there are > others that need similar adjustment. I just skimmed through the > file again and it seems there is none, so this change looks good. > > I assume you meant to sign this off, too? > >> builtin/fetch.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/builtin/fetch.c b/builtin/fetch.c >> index 630ae6a1bb78..1b4e924bd222 100644 >> --- a/builtin/fetch.c >> +++ b/builtin/fetch.c >> @@ -1071,7 +1071,7 @@ static int get_remote_group(const char *key, const char *value, void *priv) >> size_t wordlen = strcspn(value, " \t\n"); >> >> if (wordlen >= 1) >> - string_list_append(g->list, >> + string_list_append_nodup(g->list, >> xstrndup(value, wordlen)); >> value += wordlen + (value[wordlen] != '\0'); >> } >> @@ -1261,7 +1261,7 @@ done: >> int cmd_fetch(int argc, const char **argv, const char *prefix) >> { >> int i; >> - struct string_list list = STRING_LIST_INIT_NODUP; >> + struct string_list list = STRING_LIST_INIT_DUP; >> struct remote *remote; >> int result = 0; >> struct argv_array argv_gc_auto = ARGV_ARRAY_INIT; >> @@ -1347,8 +1347,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) >> argv_array_clear(&options); >> } >> >> - /* All names were strdup()ed or strndup()ed */ >> - list.strdup_strings = 1; >> string_list_clear(&list, 0); >> >> close_all_packs(); ``` ## Junio C Hamano, 2016-06-14 18:57 Subject: Re: [PATCH v2 1/1] Don't free remote->name after fetch Message-ID: URL: https://gitlist.dev/e/xmqqr3bz8upo.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Keith McGuigan writes: > Yeah that was the only place I found where it was doing the strdup > already (and in that situation, it has to). All the other places just > grabbed remote->name. > > Yes, sorry, I can sign off on this. Do you want me to resend with the > header in place, or is this confirmation good enough? The latter ;-) Thanks. ```