{"thread":{"id":"4084","subject":"[RFC] get_pathspec(): free() old buffer if rewriting","startedAt":"2006-05-06T22:04:25Z","lastAt":"2006-05-07T04:50:45Z","messageCount":5,"participants":["Johannes Schindelin","Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"19623","messageId":"Pine.LNX.4.63.0605070003430.6357@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"4084","inReplyTo":null,"subject":"[RFC] get_pathspec(): free() old buffer if rewriting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-05-06T22:04:25Z","receivedAt":"2006-05-06T22:04:25Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nThis might be the wrong way to do it, but as it is without this patch,\nget_pathspec() is leaking memory.\n\nHowever, it is by no means guaranteed that the input is malloc()ed. The\ntests run through without problems, but you never know...\n\nSigned-off-by: Johannes Schindelin \n\n---\n\n setup.c | 5 +++++\n 1 files changed, 5 insertions(+), 0 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex fe7f884..9c39d6e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -126,6 +126,11 @@ const char **get_pathspec(const char *pr\n \tprefixlen = prefix ? strlen(prefix) : 0;\n \tdo {\n \t\t*p = prefix_path(prefix, prefixlen, entry);\n+\t\tif (*p != entry) {\n+\t\t\tif (*p > entry && *p < entry + strlen(entry))\n+\t\t\t\t*p = strdup(*p);\n+\t\t\tfree((char*)entry);\n+\t\t}\n \t} while ((entry = *++p) != NULL);\n \treturn (const char **) pathspec;\n }\n-- \n1.3.2.g9ba6-dirty\n"},{"id":"19625","messageId":"Pine.LNX.4.64.0605061532190.16343@g5.osdl.org","threadId":"4084","inReplyTo":"Pine.LNX.4.63.0605070003430.6357@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [RFC] get_pathspec(): free() old buffer if rewriting","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-05-06T22:37:19Z","receivedAt":"2006-05-06T22:37:19Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 7 May 2006, Johannes Schindelin wrote:\n> \n> This might be the wrong way to do it, but as it is without this patch,\n> get_pathspec() is leaking memory.\n\nI'm not at all convinced we want to do somethng like this.\n\nget_pathspec() is a one-time event. It doesn't \"leak\" memory. To me, \n\"leaking\" is when you continually lose a bit of memory, and you eventually \nrun out. I don't see that happening here.\n\nIt's like saying that calling an initialization routine \"leaks\" memory, \nbecause it doesn't free the memory that contains the function that is now \nno longer ever used. It's clearly leaving that memory unused, \"dangling\".\n\nBut the fact is, the OS will free the memory for us when we exit, and \nsimplicity is often much more powerful than trying to be overly careful.\n\nSo there's a difference between \"don't care\" and \"leak memory\". It sounds \nlike you may be using some automated tool that warns because it simply \ndoesn't understand that difference.\n\n\t\t\tLinus\n"},{"id":"19626","messageId":"Pine.LNX.4.63.0605070041540.26058@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"4084","inReplyTo":"Pine.LNX.4.64.0605061532190.16343@g5.osdl.org","subject":"Re: [RFC] get_pathspec(): free() old buffer if rewriting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-05-06T22:44:31Z","receivedAt":"2006-05-06T22:44:31Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 6 May 2006, Linus Torvalds wrote:\n\n> On Sun, 7 May 2006, Johannes Schindelin wrote:\n> > \n> > This might be the wrong way to do it, but as it is without this patch,\n> > get_pathspec() is leaking memory.\n> \n> I'm not at all convinced we want to do somethng like this.\n> \n> get_pathspec() is a one-time event. It doesn't \"leak\" memory. To me, \n> \"leaking\" is when you continually lose a bit of memory, and you eventually \n> run out. I don't see that happening here.\n\nI see your point. That was exactly why I put \"RFC\" and not \"PATCH\" on the \nsubject.\n\n> So there's a difference between \"don't care\" and \"leak memory\". It sounds \n> like you may be using some automated tool that warns because it simply \n> doesn't understand that difference.\n\nNope. No automated tool. Just my brain which wanted to fix _all_ \noccurrences of that prefix_path() usage bug.\n\nBut in case anybody uses Valgrind et al. on git, how about at least a \ncomment telling the casual observer why we don't free() the buffers?\n\nCiao,\nDscho\n"},{"id":"19636","messageId":"7viroipijf.fsf@assigned-by-dhcp.cox.net","threadId":"4084","inReplyTo":"Pine.LNX.4.64.0605061532190.16343@g5.osdl.org","subject":"Re: [RFC] get_pathspec(): free() old buffer if rewriting","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-07T04:26:12Z","receivedAt":"2006-05-07T04:26:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Sun, 7 May 2006, Johannes Schindelin wrote:\n>> \n>> This might be the wrong way to do it, but as it is without this patch,\n>> get_pathspec() is leaking memory.\n>\n> I'm not at all convinced we want to do somethng like this.\n>\n> get_pathspec() is a one-time event. It doesn't \"leak\" memory. To me, \n> \"leaking\" is when you continually lose a bit of memory, and you eventually \n> run out. I don't see that happening here.\n\nI agree with that, except that blame does use it for each\ncommit and loses memory with deep history.\n\nI have been contemplating to revamp pathspec stuff to deal with\nnot just an array pointers to strings. tree-diff already uses a\nparallel array of ints (pathlens) to optimize away the repeated\nuse of strlen() for each pathspec elements, and I think we would\nbe better off using something like that everywhere. The API I\nhave in mind goes like this:\n\nstruct pathspec; /* opaque */ \n\n\tA type opaque to the caller.\n\nstruct pathspec **get_pathspec(const char *prefix,\n\t\t\t const char **pathspec,\n\t\t\t int wildcard);\n\n\tThe caller gives the prefix (return value from\n\tsetup_git_directory(), the user supplied pathspec list,\n\tand if it wants to use ls-files style wildcard or\n\ttree-diff style directory prefix bahaviour. A newly\n\tallocated array is returned and the caller can free() it\n\twhen done.\n\nint match_pathspec(const char *path, int len,\n\t struct pathspec **pathspec);\n\n\tSee if the path (with length) matches the given spec.\n\tpath[len-1] == '/' signals that the caller is traversing\n\ta tree and checking if it is worth descending into the\n\ttree. In that case, original spec string \"foo/bar/baz\"\n\tmatches path = \"foo/\" (with len = 4). Otherwise the full\n\tpath is checked so that original spec string would match\n\tpath = \"foo/bar/baz\" (with len = 11), but not \"foo\"\n\t(with len = 3). If the get_pathspec() was called with\n\twildcard support, spec string \"foo/bar*\" matches these:\n\n \"foo/\" (i.e. should descend into it),\n \"foo/barboz/\" (ditto)\n \"foo/bar.txt\" (matches)\n\n but not these:\n\n\t\t\"fob/\" (no point descending into it)\n \"foo/bax\" (does not match)\n\nA wildcard aware diff-tree, when invoked like this:\n\n\tcd Documentation\n git-diff-tree -r A B -- 'ho*'\n\nmight do:\n\n\tstruct pathspec **spec;\n const char *(paths[2]);\n\tpaths[0] = \"how*\"; paths[1] = NULL;\n\tspec = get_pathspec(\"Documentation/\", argv, 1);\n\nand when traversing the tree for A and B, upon seeing \"Documentation\"\nentry in the topleve tree object buffers, call:\n\n\tpath_match(\"Documentation\", 14, 1, spec)\n\nwhich would return true (worth descending into the directory).\nThe recursive call would, upon seeing \"hooks.txt\" entry, call:\n\n\tpath_match(\"Documentation/hooks.txt\", 23, 0, spec)\n\nwhich would say true and compares the entry from two trees.\nAlso, the same recursive invocation would call\n\n\tpath_match(\"Documentation/howto\", 19, 1, spec)\n\nwhich would return true to cause it further recurse into it.\n"},{"id":"19637","messageId":"7vejz6phei.fsf@assigned-by-dhcp.cox.net","threadId":"4084","inReplyTo":"7viroipijf.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC] get_pathspec(): free() old buffer if rewriting","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-07T04:50:45Z","receivedAt":"2006-05-07T04:50:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> int match_pathspec(const char *path, int len,\n> \t struct pathspec **pathspec);\n>\n> \tSee if the path (with length) matches the given spec.\n> \tpath[len-1] == '/' signals that the caller is traversing\n> \ta tree and checking if it is worth descending into the\n> \ttree.\n> ...\n> and when traversing the tree for A and B, upon seeing \"Documentation\"\n> entry in the topleve tree object buffers, call:\n>\n> \tpath_match(\"Documentation\", 14, 1, spec)\n\nOops, match_pathspec(\"Documentation/\", 15, spec) is what I meant\nhere. Likewise in the later examples.\n"}]}