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

On Thu, Dec 8, 2016 at 7:36 AM, Brandon Williams [off-list ref] wrote:

quoted

quoted

@@ -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