Re: [PATCH 01/17] mv: convert to using pathspec struct interface

[Brandon Williams <bmwill@google.com>]

On 12/08, Duy Nguyen wrote:
> On Thu, Dec 8, 2016 at 7:36 AM, Brandon Williams <bmwill@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