On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@xxxxxxxxxx> wrote: > Convert the 'internal_copy_pathspec()' function to use the pathspec > struct interface from using the deprecated 'get_pathspec()' interface. > > In addition to this, fix a memory leak caused by only duplicating some > of the pathspec elements. Instead always duplicate all of the the > pathspec elements as an intermediate step (with modificationed based on > the passed in flags). This way the intermediate strings can then be > freed prior to duplicating the result of parse_pathspec (which contains > each of the elements with the prefix prepended). > > Signed-off-by: Brandon Williams <bmwill@xxxxxxxxxx> > --- > builtin/mv.c | 45 ++++++++++++++++++++++++++++++++------------- > 1 file changed, 32 insertions(+), 13 deletions(-) Stefan did something similar last year [1] but I couldn't find why it did not get merged. Not sure if the reasons are still relevant or not... [1] http://public-inbox.org/git/%3C1438885632-26470-1-git-send-email-sbeller@xxxxxxxxxx%3E/ > diff --git a/builtin/mv.c b/builtin/mv.c > index 2f43877..4df4a12 100644 > --- a/builtin/mv.c > +++ b/builtin/mv.c > @@ -4,6 +4,7 @@ > * Copyright (C) 2006 Johannes Schindelin > */ > #include "builtin.h" > +#include "pathspec.h" > #include "lockfile.h" > #include "dir.h" > #include "cache-tree.h" > @@ -25,25 +26,43 @@ static const char **internal_copy_pathspec(const char *prefix, > { > int i; > const char **result; > + struct pathspec ps; > ALLOC_ARRAY(result, count + 1); > - COPY_ARRAY(result, pathspec, count); > - result[count] = NULL; > + > + /* Create an intermediate copy of the pathspec based on the flags */ > for (i = 0; i < count; i++) { > - int length = strlen(result[i]); > + int length = strlen(pathspec[i]); > int to_copy = length; > + char *it; > while (!(flags & KEEP_TRAILING_SLASH) && > - to_copy > 0 && is_dir_sep(result[i][to_copy - 1])) > + to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1])) > to_copy--; > - if (to_copy != length || flags & DUP_BASENAME) { > - char *it = xmemdupz(result[i], to_copy); > - if (flags & DUP_BASENAME) { > - result[i] = xstrdup(basename(it)); > - free(it); > - } else > - result[i] = it; > - } > + > + it = xmemdupz(pathspec[i], to_copy); > + if (flags & DUP_BASENAME) { > + result[i] = xstrdup(basename(it)); > + free(it); > + } else > + result[i] = it; > + } > + result[count] = NULL; > + > + parse_pathspec(&ps, > + PATHSPEC_ALL_MAGIC & > + ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL), > + PATHSPEC_KEEP_ORDER | PATHSPEC_PREFER_CWD, > + prefix, result); > + assert(count == ps.nr); > + > + /* Copy the pathspec and free the old intermediate strings */ > + for (i = 0; i < count; i++) { > + const char *match = xstrdup(ps.items[i].match); > + free((char *) result[i]); > + result[i] = match; Sigh.. it looks so weird that we do all the parsing (in a _copy_ pathspec function) then remove struct pathspec and return the plain string. I guess we can't do anything more until we rework cmd_mv code to handle pathspec natively. At the least I think we should rename this function to something else. But if you have time I really wish we could kill this function. I haven't stared at cmd_mv() long and hard, but it looks to me that we combining two separate functionalities in the same function here. If "mv" takes n arguments, then the first <n-1> arguments may be pathspec, the last one is always a plain path. The "dest_path = internal_copy_pathspec..." could be as simple as "dest_path = prefix_path(argv[argc - 1])". the special treatment for this last argument [1] can live here. Then, we can do parse_pathspec for the <n-1> arguments in cmd_mv(). It's still far from perfect, because cmd_mv can't handle pathspec properly, but it reduces the messy mess in internal_copy_pathspec a bit, I hope. [1] c57f628 (mv: let 'git mv file no-such-dir/' error out - 2013-12-03) > } > - return get_pathspec(prefix, result); > + > + clear_pathspec(&ps); > + return result; > } > > static const char *add_slash(const char *path) > -- > 2.8.0.rc3.226.g39d4020 > -- Duy