Re: [PATCH] ci(win+Meson): build in Release mode, avoiding t7001-mv hangs

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> 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.