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

[Duy Nguyen <pclouds@gmail.com>]

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.

> I agree that this code should probably be rewritten and made a
> bit cleaner, I don't know if that fits in the scope of this series or
> should be done as a followup patch.  If you think it fits here then I
> can try and find some time to do the rework.
-- 
Duy