On Thu, Dec 8, 2016 at 7:36 AM, Brandon Williams <bmwill@xxxxxxxxxx> 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