{"thread":{"id":"8921","subject":"[PATCH] path-list.c: always free strdup'ed paths","startedAt":"2007-07-07T19:39:10Z","lastAt":"2007-07-07T20:27:57Z","messageCount":4,"participants":["René Scharfe","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"46710","messageId":"Pine.LNX.4.64.0707072036530.4093@racer.site","threadId":"8921","inReplyTo":"468FEC54.307@lsrfire.ath.cx","subject":"Re: [PATCH] path-list.c: always free strdup'ed paths","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-07-07T19:39:10Z","receivedAt":"2007-07-07T19:39:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 7 Jul 2007, Ren? Scharfe wrote:\n\n> Always free .paths if .strdup_paths is set, no matter if the\n> parameter free_items is set or not, plugging a minor memory leak.\n> And to clarify the meaning of the flag, rename it to free_util,\n> since it now only affects the freeing of the .util field.\n\nThe rational was that it might very well be possible that the pointers you \nhand to the path_list are already strdup()ed. So you do not set \nstrdup_paths, but you want them free()d.\n\nThe .util field is in many cases something that is not trivially free()d, \nbut has to call a type-specific function, such as path_list_clear() \nitself.\n\nSo I'm mildly negative about your patch.\n\nCiao,\nDscho\n"},{"id":"46708","messageId":"468FEC54.307@lsrfire.ath.cx","threadId":"8921","inReplyTo":null,"subject":"[PATCH] path-list.c: always free strdup'ed paths","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-07-07T19:41:08Z","receivedAt":"2007-07-07T19:41:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Always free .paths if .strdup_paths is set, no matter if the\nparameter free_items is set or not, plugging a minor memory leak.\nAnd to clarify the meaning of the flag, rename it to free_util,\nsince it now only affects the freeing of the .util field.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n\n path-list.c |   14 ++++++++------\n path-list.h |    2 +-\n 2 files changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/path-list.c b/path-list.c\nindex dcb4b3a..3d83b7b 100644\n--- a/path-list.c\n+++ b/path-list.c\n@@ -76,16 +76,18 @@ struct path_list_item *path_list_lookup(const char *path, struct path_list *list\n \treturn list->items + i;\n }\n \n-void path_list_clear(struct path_list *list, int free_items)\n+void path_list_clear(struct path_list *list, int free_util)\n {\n \tif (list->items) {\n \t\tint i;\n-\t\tif (free_items)\n-\t\t\tfor (i = 0; i < list->nr; i++) {\n-\t\t\t\tif (list->strdup_paths)\n-\t\t\t\t\tfree(list->items[i].path);\n+\t\tif (list->strdup_paths) {\n+\t\t\tfor (i = 0; i < list->nr; i++)\n+\t\t\t\tfree(list->items[i].path);\n+\t\t}\n+\t\tif (free_util) {\n+\t\t\tfor (i = 0; i < list->nr; i++)\n \t\t\t\tfree(list->items[i].util);\n-\t\t\t}\n+\t\t}\n \t\tfree(list->items);\n \t}\n \tlist->items = NULL;\ndiff --git a/path-list.h b/path-list.h\nindex ce5ffab..5931e2c 100644\n--- a/path-list.h\n+++ b/path-list.h\n@@ -15,7 +15,7 @@ struct path_list\n void print_path_list(const char *text, const struct path_list *p);\n \n int path_list_has_path(const struct path_list *list, const char *path);\n-void path_list_clear(struct path_list *list, int free_items);\n+void path_list_clear(struct path_list *list, int free_util);\n struct path_list_item *path_list_insert(const char *path, struct path_list *list);\n struct path_list_item *path_list_lookup(const char *path, struct path_list *list);\n \n"},{"id":"46711","messageId":"468FF19E.8020406@lsrfire.ath.cx","threadId":"8921","inReplyTo":"Pine.LNX.4.64.0707072036530.4093@racer.site","subject":"Re: [PATCH] path-list.c: always free strdup'ed paths","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-07-07T20:03:42Z","receivedAt":"2007-07-07T20:03:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Johannes Schindelin schrieb:\n> Hi,\n> \n> On Sat, 7 Jul 2007, Ren? Scharfe wrote:\n> \n>> Always free .paths if .strdup_paths is set, no matter if the \n>> parameter free_items is set or not, plugging a minor memory leak. \n>> And to clarify the meaning of the flag, rename it to free_util, \n>> since it now only affects the freeing of the .util field.\n> \n> The rational was that it might very well be possible that the\n> pointers you hand to the path_list are already strdup()ed. So you do\n> not set strdup_paths, but you want them free()d.\n\nThe patch doesn't take that away, i.e. .path fields are not freed if\n.strdup_paths is not set, both before and after the patch.  And the\nworkaround used in builtin-shortlog.c, viz. setting .strdup_paths just\nbefore calling path_list_clear(), still works.\n\n> The .util field is in many cases something that is not trivially\n> free()d, but has to call a type-specific function, such as\n> path_list_clear() itself.\n\nIndeed; this is a good reason to separate freeing of .util and .path, no?\n\nRené\n"},{"id":"46713","messageId":"Pine.LNX.4.64.0707072124550.4093@racer.site","threadId":"8921","inReplyTo":"468FF19E.8020406@lsrfire.ath.cx","subject":"Re: [PATCH] path-list.c: always free strdup'ed paths","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-07-07T20:27:57Z","receivedAt":"2007-07-07T20:27:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 7 Jul 2007, Ren? Scharfe wrote:\n\n> Johannes Schindelin schrieb:\n> > \n> > On Sat, 7 Jul 2007, Ren? Scharfe wrote:\n> > \n> >> Always free .paths if .strdup_paths is set, no matter if the \n> >> parameter free_items is set or not, plugging a minor memory leak. \n> >> And to clarify the meaning of the flag, rename it to free_util, \n> >> since it now only affects the freeing of the .util field.\n> > \n> > The rational was that it might very well be possible that the\n> > pointers you hand to the path_list are already strdup()ed. So you do\n> > not set strdup_paths, but you want them free()d.\n> \n> The patch doesn't take that away, i.e. .path fields are not freed if\n> .strdup_paths is not set, both before and after the patch.\n\nYeah, the old code was buggy.\n\n> And the workaround used in builtin-shortlog.c, viz. setting \n> .strdup_paths just before calling path_list_clear(), still works.\n\nYeah, but it is ugly.\n\n> > The .util field is in many cases something that is not trivially\n> > free()d, but has to call a type-specific function, such as\n> > path_list_clear() itself.\n> \n> Indeed; this is a good reason to separate freeing of .util and .path, no?\n\nYes.\n\nCiao,\nDscho\n"}]}