Re: [PATCH] ci(win+Meson): build in Release mode, avoiding t7001-mv hangs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 29, 2025, 20:48 UTC
- Message-ID
- <xmqqplgu4ru5.fsf@gitster.g>
- In-Reply-To
- <aBDD-NeN2YoQbU9S@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 24 quoted lines
> @@ -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, ...
Show 7 quoted lines
> + 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.
Show 10 quoted lines
> + 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.