{"thread":{"id":"42610","subject":"[PATCH v2 1/1] Don't free remote->name after fetch","startedAt":"2016-06-16T06:55:52Z","lastAt":"2016-06-16T06:55:52Z","messageCount":4,"participants":["kmcguigan@twopensource.com","Junio C Hamano","Keith McGuigan"],"isPatch":true,"patchVersion":2,"patchTotal":1},"messages":[{"id":"289238","messageId":"1465928936-68866-1-git-send-email-kmcguigan@twopensource.com","threadId":"42610","inReplyTo":null,"subject":"[PATCH v2 1/1] Don't free remote->name after fetch","fromName":"","fromEmail":"kmcguigan@twopensource.com","sentAt":"2016-06-14T18:28:56Z","receivedAt":"2016-06-16T06:55:52Z","isPatch":true,"sender":{"key":"kmcguigan@twopensource.com","avatar":null},"body":"From: Keith McGuigan \n\nMake fetch's string_list of remote names owns all of its string items\n(strdup'ing when necessary) so that it can deallocate them safely when\nclearing.\n\n---\n builtin/fetch.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 630ae6a1bb78..1b4e924bd222 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1071,7 +1071,7 @@ static int get_remote_group(const char *key, const char *value, void *priv)\n \t\t\tsize_t wordlen = strcspn(value, \" \\t\\n\");\n \n \t\t\tif (wordlen >= 1)\n-\t\t\t\tstring_list_append(g->list,\n+\t\t\t\tstring_list_append_nodup(g->list,\n \t\t\t\t\t\t xstrndup(value, wordlen));\n \t\t\tvalue += wordlen + (value[wordlen] != '\\0');\n \t\t}\n@@ -1261,7 +1261,7 @@ done:\n int cmd_fetch(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n-\tstruct string_list list = STRING_LIST_INIT_NODUP;\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n \tstruct remote *remote;\n \tint result = 0;\n \tstruct argv_array argv_gc_auto = ARGV_ARRAY_INIT;\n@@ -1347,8 +1347,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\targv_array_clear(&options);\n \t}\n \n-\t/* All names were strdup()ed or strndup()ed */\n-\tlist.strdup_strings = 1;\n \tstring_list_clear(&list, 0);\n \n \tclose_all_packs();\n-- \n2.8.0.rc3.94.g2dc3105.dirty\n"},{"id":"289240","messageId":"xmqqvb1b8v2w.fsf@gitster.mtv.corp.google.com","threadId":"42610","inReplyTo":"1465928936-68866-1-git-send-email-kmcguigan@twopensource.com","subject":"Re: [PATCH v2 1/1] Don't free remote->name after fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-14T18:49:11Z","receivedAt":"2016-06-16T06:55:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"kmcguigan@twopensource.com writes:\n\n> From: Keith McGuigan \n>\n> Make fetch's string_list of remote names owns all of its string items\n> (strdup'ing when necessary) so that it can deallocate them safely when\n> clearing.\n>\n> ---\n\nOK.\n\nWhen I pointed out the call to string_list_append() in\nget_remote_group() as one example, I did not check if there are\nothers that need similar adjustment. I just skimmed through the\nfile again and it seems there is none, so this change looks good.\n\nI assume you meant to sign this off, too?\n\n> builtin/fetch.c | 6 ++----\n> 1 file changed, 2 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 630ae6a1bb78..1b4e924bd222 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1071,7 +1071,7 @@ static int get_remote_group(const char *key, const char *value, void *priv)\n> \t\t\tsize_t wordlen = strcspn(value, \" \\t\\n\");\n> \n> \t\t\tif (wordlen >= 1)\n> -\t\t\t\tstring_list_append(g->list,\n> +\t\t\t\tstring_list_append_nodup(g->list,\n> \t\t\t\t\t\t xstrndup(value, wordlen));\n> \t\t\tvalue += wordlen + (value[wordlen] != '\\0');\n> \t\t}\n> @@ -1261,7 +1261,7 @@ done:\n> int cmd_fetch(int argc, const char **argv, const char *prefix)\n> {\n> \tint i;\n> -\tstruct string_list list = STRING_LIST_INIT_NODUP;\n> +\tstruct string_list list = STRING_LIST_INIT_DUP;\n> \tstruct remote *remote;\n> \tint result = 0;\n> \tstruct argv_array argv_gc_auto = ARGV_ARRAY_INIT;\n> @@ -1347,8 +1347,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \t\targv_array_clear(&options);\n> \t}\n> \n> -\t/* All names were strdup()ed or strndup()ed */\n> -\tlist.strdup_strings = 1;\n> \tstring_list_clear(&list, 0);\n> \n> \tclose_all_packs();\n"},{"id":"289241","messageId":"CALnYDJObFtrKFBRbbaA2KV257j8L8gcKcGk-Pkhsyvbk7+oTjQ@mail.gmail.com","threadId":"42610","inReplyTo":"xmqqvb1b8v2w.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/1] Don't free remote->name after fetch","fromName":"Keith McGuigan","fromEmail":"kmcguigan@twopensource.com","sentAt":"2016-06-14T18:54:38Z","receivedAt":"2016-06-16T06:55:52Z","isPatch":true,"sender":{"key":"kmcguigan@twopensource.com","avatar":null},"body":"Yeah that was the only place I found where it was doing the strdup\nalready (and in that situation, it has to). All the other places just\ngrabbed remote->name.\n\nYes, sorry, I can sign off on this. Do you want me to resend with the\nheader in place, or is this confirmation good enough?\n\n--\n- Keith\n\nOn Tue, Jun 14, 2016 at 2:49 PM, Junio C Hamano wrote:\n> kmcguigan@twopensource.com writes:\n>\n>> From: Keith McGuigan \n>>\n>> Make fetch's string_list of remote names owns all of its string items\n>> (strdup'ing when necessary) so that it can deallocate them safely when\n>> clearing.\n>>\n>> ---\n>\n> OK.\n>\n> When I pointed out the call to string_list_append() in\n> get_remote_group() as one example, I did not check if there are\n> others that need similar adjustment. I just skimmed through the\n> file again and it seems there is none, so this change looks good.\n>\n> I assume you meant to sign this off, too?\n>\n>> builtin/fetch.c | 6 ++----\n>> 1 file changed, 2 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index 630ae6a1bb78..1b4e924bd222 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -1071,7 +1071,7 @@ static int get_remote_group(const char *key, const char *value, void *priv)\n>> size_t wordlen = strcspn(value, \" \\t\\n\");\n>>\n>> if (wordlen >= 1)\n>> - string_list_append(g->list,\n>> + string_list_append_nodup(g->list,\n>> xstrndup(value, wordlen));\n>> value += wordlen + (value[wordlen] != '\\0');\n>> }\n>> @@ -1261,7 +1261,7 @@ done:\n>> int cmd_fetch(int argc, const char **argv, const char *prefix)\n>> {\n>> int i;\n>> - struct string_list list = STRING_LIST_INIT_NODUP;\n>> + struct string_list list = STRING_LIST_INIT_DUP;\n>> struct remote *remote;\n>> int result = 0;\n>> struct argv_array argv_gc_auto = ARGV_ARRAY_INIT;\n>> @@ -1347,8 +1347,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>> argv_array_clear(&options);\n>> }\n>>\n>> - /* All names were strdup()ed or strndup()ed */\n>> - list.strdup_strings = 1;\n>> string_list_clear(&list, 0);\n>>\n>> close_all_packs();\n"},{"id":"289242","messageId":"xmqqr3bz8upo.fsf@gitster.mtv.corp.google.com","threadId":"42610","inReplyTo":"CALnYDJObFtrKFBRbbaA2KV257j8L8gcKcGk-Pkhsyvbk7+oTjQ@mail.gmail.com","subject":"Re: [PATCH v2 1/1] Don't free remote->name after fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-14T18:57:07Z","receivedAt":"2016-06-16T06:55:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Keith McGuigan writes:\n\n> Yeah that was the only place I found where it was doing the strdup\n> already (and in that situation, it has to). All the other places just\n> grabbed remote->name.\n>\n> Yes, sorry, I can sign off on this. Do you want me to resend with the\n> header in place, or is this confirmation good enough?\n\nThe latter ;-) Thanks.\n"}]}