Patrick Steinhardt <ps@xxxxxx> writes:
> @@ -213,6 +228,8 @@ int cmd_mv(int argc,
> struct cache_entry *ce;
> struct string_list only_match_skip_worktree = STRING_LIST_INIT_DUP;
> struct string_list dirty_paths = STRING_LIST_INIT_DUP;
> + struct hashmap moved_dirs = HASHMAP_INIT(pathmap_cmp, NULL);
> + struct strbuf pathbuf = STRBUF_INIT;
> int ret;
>
> git_config(git_default_config, NULL);
> @@ -331,11 +348,17 @@ int cmd_mv(int argc,
>
> dir_check:
> if (S_ISDIR(st.st_mode)) {
> + struct pathmap_entry *entry;
> char *dst_with_slash;
> size_t dst_with_slash_len;
> int j, n;
> int first = index_name_pos(the_repository->index, src, length), last;
>
> + entry = xmalloc(sizeof(*entry));
> + entry->path = src;
> + hashmap_entry_init(&entry->ent, fspathhash(src));
> + hashmap_add(&moved_dirs, &entry->ent);
> +
OK, this collects in moved_dirs the directories that will get moved.
And then a separate loop, ...
> + for (i = 0; i < argc; i++) {
> + const char *slash_pos;
> +
> + strbuf_addstr(&pathbuf, sources.v[i]);
Shouldn't there be a call to strbuf_reset(&pathbuf) before doing
this?
> + slash_pos = strrchr(pathbuf.buf, '/');
And start from the deepest directory, going one level up per
iteration, ...
> + while (slash_pos > pathbuf.buf) {
> + struct pathmap_entry needle;
> +
> + strbuf_setlen(&pathbuf, slash_pos - pathbuf.buf);
> +
> + needle.path = pathbuf.buf;
> + hashmap_entry_init(&needle.ent, fspathhash(pathbuf.buf));
... see if the path being moved falls within that subdirectory.
> + if (!hashmap_get_entry(&moved_dirs, &needle, ent, NULL))
> + continue;
If there is no overlap, we need to do anything special.
> + if (!ignore_errors)
> + die(_("cannot move both parent directory '%s' and its child '%s'"),
> + pathbuf.buf, sources.v[i]);
Otherwise we are in trouble.
> + if (--argc > 0) {
> + int n = argc - i;
> + strvec_remove(&sources, i);
> + strvec_remove(&destinations, i);
> + MOVE_ARRAY(modes + i, modes + i + 1, n);
> + MOVE_ARRAY(submodule_gitfiles + i,
> + submodule_gitfiles + i + 1, n);
> + i--;
> + break;
> + }
So with
$ git mv a/ a/b x y z/
then a/ is left in the argv[]/sources[]/destinations[] arrays, and
upon inspecting a/b, we come here and in order to ignore a/b, we
shift it out; the resulting arrays would have a/, x, and y being
moved to z/.
It somehow feels troubling that it would lead to a different result
if I give a morally equivalent arguments, i.e.
$ git mv a/b a/ x y z/
where a/b survives and a/ gets omitted.
One thing that came to my mind (without concrete "here is the right
way to solve it" that I am myself convinced) is this.
* Should this code path even have its own ignore-errors handling?
"git mv a b z/", when 'a' does not exist, may ignore 'a' and move
only 'b', which may make sense. But the original command line in
that case is a plausibly correct one if there weren't missing or
unmovable paths. The command line "git mv a/ a/b z/" seems to
fall into a different category (aka "total nonsense"); no matter
how you fix the items in your working tree files, you cannot make
it plausibly correct.
a totally unrelated tangent that made me scratch my head while
reading the original ocde is the dest_paths variable. It is never
used as a collection to hold potentially multiple paths; it is a
strvec only to be able to call internel_prefix_pathspec() with, and
used only once with only one element in the vector. At least it
should lose the plural 's' suffix to unconfuse its readers, I would
think.
