On 12/08, Duy Nguyen wrote:
> On Thu, Dec 8, 2016 at 7:36 AM, Brandon Williams <bmw...@google.com> wrote:
> >> > @@ -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.
> >>
> >
> > Actually, after looking at this a bit more it seems like we could
> > technically use prefix_path for both source and dest (based on how the
> > current code is structured) since the source's provied must all exist (as
> > in no wildcards are allowed). We could drop using the pathspec struct
> > completely in addition to renaming the function (to what I'm still
> > unsure).
>
> Yeah that sounds good too (with a caveat: I'm not a heavy user of
> git-mv nor touching this code a lot, I might be missing something).
> It'll take some looong time before somebody starts converting it to
> use pathspec properly, I guess. prefix_path() would keep the code
> clean meanwhile.
K for now I'll switch to using prefix_path() and rename the function
`internal_prefix_pathspec()` as that is a bit more descriptive.
--
Brandon Williams
