Re: Re* [PATCH 10/15] commit.c: fix a memory leak
On Tue, Mar 24, 2015 at 2:17 PM, Junio C Hamano wrote: > > Move it to dir.c where match_pathspec() is defined. > > Signed-off-by: Junio C Hamano Reviewed-by: Stefan Beller -- 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 10/15] commit.c: fix a memory leak
Duy Nguyen writes: > On Sat, Mar 21, 2015 at 10:59 AM, Junio C Hamano wrote: >> A further tangent (Duy Cc'ed for this point). We might want to >> rethink the interface to ce_path_match() and report_path_error() >> so that we do not have to do a separate allocation of "has this >> pathspec been used?" array. This was a remnant from the olden days >> back when pathspec were mere "const char **" where we did not have >> any room to add per-item bit---these days pathspec is repreasented >> as an array of "struct pathspec" and we can afford to add a bit >> to the structure---which will make this kind of leak much less >> likely to happen. > > I just want to say "noted" (and therefore in my backlog). But no > promise that it will happen any time soon. Low hanging fruit, perhaps > some people may be interested.. OK, the other one I just did so that we won't forget. Otherwise we will leave too many loose ends untied. -- >8 -- From: Junio C Hamano Date: Tue, 24 Mar 2015 14:12:10 -0700 Subject: [PATCH] report_path_error(): move to dir.c The expected call sequence is for the caller to use match_pathspec() repeatedly on a set of pathspecs, accumulating the "hits" in a separate array, and then call this function to diagnose a pathspec that never matched anything, as that can indicate a typo from the command line, e.g. "git commit Maekfile". Many builtin commands use this function from builtin/ls-files.c, which is not a very healthy arrangement. ls-files might have been the first command to feel the need for such a helper, but the need is shared by everybody who uses the "match and then report" pattern. Move it to dir.c where match_pathspec() is defined. Signed-off-by: Junio C Hamano --- builtin/ls-files.c | 43 --- cache.h| 1 - dir.c | 43 +++ dir.h | 1 + 4 files changed, 44 insertions(+), 44 deletions(-) diff --git a/builtin/ls-files.c b/builtin/ls-files.c index 47c3880..47d70b2 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -354,49 +354,6 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix) } } -int report_path_error(const char *ps_matched, - const struct pathspec *pathspec, - const char *prefix) -{ - /* -* Make sure all pathspec matched; otherwise it is an error. -*/ - struct strbuf sb = STRBUF_INIT; - int num, errors = 0; - for (num = 0; num < pathspec->nr; num++) { - int other, found_dup; - - if (ps_matched[num]) - continue; - /* -* The caller might have fed identical pathspec -* twice. Do not barf on such a mistake. -* FIXME: parse_pathspec should have eliminated -* duplicate pathspec. -*/ - for (found_dup = other = 0; -!found_dup && other < pathspec->nr; -other++) { - if (other == num || !ps_matched[other]) - continue; - if (!strcmp(pathspec->items[other].original, - pathspec->items[num].original)) - /* -* Ok, we have a match already. -*/ - found_dup = 1; - } - if (found_dup) - continue; - - error("pathspec '%s' did not match any file(s) known to git.", - pathspec->items[num].original); - errors++; - } - strbuf_release(&sb); - return errors; -} - static const char * const ls_files_usage[] = { N_("git ls-files [options] [...]"), NULL diff --git a/cache.h b/cache.h index f23fdbe..8ec0b65 100644 --- a/cache.h +++ b/cache.h @@ -1411,7 +1411,6 @@ extern int ws_blank_line(const char *line, int len, unsigned ws_rule); #define ws_tab_width(rule) ((rule) & WS_TAB_WIDTH_MASK) /* ls-files */ -int report_path_error(const char *ps_matched, const struct pathspec *pathspec, const char *prefix); void overlay_tree_on_cache(const char *tree_name, const char *prefix); char *alias_lookup(const char *alias); diff --git a/dir.c b/dir.c index 797805d..5d6e102 100644 --- a/dir.c +++ b/dir.c @@ -377,6 +377,49 @@ int match_pathspec(const struct pathspec *ps, return negative ? 0 : positive; } +int report_path_error(const char *ps_matched, + const struct pathspec *pathspec, + const char *prefix) +{ + /* +* Make sure all pathspec matched; otherwise it is an error. +*/ + struct strbuf sb = STRBUF_INIT; + int num, errors = 0; + for (num = 0; num < pathspec->nr; num++) { + int other, found_dup; + + if
Re: [PATCH 10/15] commit.c: fix a memory leak
On Sat, Mar 21, 2015 at 10:59 AM, Junio C Hamano wrote: > A further tangent (Duy Cc'ed for this point). We might want to > rethink the interface to ce_path_match() and report_path_error() > so that we do not have to do a separate allocation of "has this > pathspec been used?" array. This was a remnant from the olden days > back when pathspec were mere "const char **" where we did not have > any room to add per-item bit---these days pathspec is repreasented > as an array of "struct pathspec" and we can afford to add a bit > to the structure---which will make this kind of leak much less > likely to happen. I just want to say "noted" (and therefore in my backlog). But no promise that it will happen any time soon. Low hanging fruit, perhaps some people may be interested.. -- 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 10/15] commit.c: fix a memory leak
Stefan Beller writes: > Signed-off-by: Stefan Beller > --- > builtin/commit.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index 961e467..da79ac4 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -229,7 +229,7 @@ static int commit_index_files(void) > static int list_paths(struct string_list *list, const char *with_tree, > const char *prefix, const struct pathspec *pattern) > { > - int i; > + int i, ret; > char *m; > > if (!pattern->nr) > @@ -256,7 +256,9 @@ static int list_paths(struct string_list *list, const > char *with_tree, > item->util = item; /* better a valid pointer than a > fake one */ > } > > - return report_path_error(m, pattern, prefix); > + ret = report_path_error(m, pattern, prefix); > + free(m); > + return ret; > } Looks correct. A tangent. We may want to move report_path_error() to somewhere more "common"-ish, like dir.c which is where we have bulk of pathspec matching infrastructure. Seeing the function used from builtin/ls-files.c makes me feel dirty. A further tangent (Duy Cc'ed for this point). We might want to rethink the interface to ce_path_match() and report_path_error() so that we do not have to do a separate allocation of "has this pathspec been used?" array. This was a remnant from the olden days back when pathspec were mere "const char **" where we did not have any room to add per-item bit---these days pathspec is repreasented as an array of "struct pathspec" and we can afford to add a bit to the structure---which will make this kind of leak much less likely to happen. -- 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
[PATCH 10/15] commit.c: fix a memory leak
Signed-off-by: Stefan Beller --- builtin/commit.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 961e467..da79ac4 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -229,7 +229,7 @@ static int commit_index_files(void) static int list_paths(struct string_list *list, const char *with_tree, const char *prefix, const struct pathspec *pattern) { - int i; + int i, ret; char *m; if (!pattern->nr) @@ -256,7 +256,9 @@ static int list_paths(struct string_list *list, const char *with_tree, item->util = item; /* better a valid pointer than a fake one */ } - return report_path_error(m, pattern, prefix); + ret = report_path_error(m, pattern, prefix); + free(m); + return ret; } static void add_remove_files(struct string_list *list) -- 2.3.0.81.gc37f363 -- 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