ci(win+Meson): build in Release mode, avoiding t7001-mv hangs #1908
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.1908.git.1745593515875.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Since switching to `--vsenv`, the t7001-mv test consistently times out
> after six hours in the CI builds on GitHub. This kind of waste is
> inconsistent with my values.
With mine too and I would presume everybody else's. I've been
annoyed for a long time by one of those sharded Meson-Win test jobs
that hang around until timeout.
Thank you very much for addressing the issue.
> The reason for this timeout is the test case 'nonsense mv triggers
> assertion failure and partially updated index' in t7001-mv (which is
> not even a regression test, but instead merely demonstrates a bug that
> someone thought someone else should fix at some time). As the name
> suggests, it triggers an assertion. The problem with this is that an
> assertion on Windows, at least when run in Debug mode, will open a modal
> dialog that patiently awaits some buttons to be clicked. Which never
> happens in automated builds.
Interesting.
So another viable fix (no, I am not suggesting a counter-proposal,
but asking a pure question to see if I understand the issue
correctly) is to rewrite "assert(cond)" to "if (cond) BUG(...)"
or something like that, so that it truly fails?
> The solution is straight-forward: Just like the `win+VS` job already did
> in forever, build in Release mode (where that modal assertion dialog is
> never shown).
OK.
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> ci(win+Meson): build in Release mode, avoiding t7001-mv hangs
>
> I was surprised to find this issue today, and that this had not been
> addressed yet.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1908%2Fdscho%2Fdont-let-win%2BMeson-hang-in-t7001-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1908/dscho/dont-let-win+Meson-hang-in-t7001-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1908
>
> .github/workflows/main.yml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index 83ca8e4182b..275240be5dc 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -265,7 +265,7 @@ jobs:
> run: pip install meson ninja
> - name: Setup
> shell: pwsh
> - run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred
> + run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred
> - name: Compile
> shell: pwsh
> run: meson compile -C build
>
> base-commit: f65182a99e545d2f2bc22e6c1c2da192133b16a3 |
|
This patch series was integrated into seen via git@018e242. |
|
This patch series was integrated into seen via git@ebaf1e2. |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Fri, Apr 25, 2025 at 08:18:02AM -0700, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Since switching to `--vsenv`, the t7001-mv test consistently times out
> > after six hours in the CI builds on GitHub. This kind of waste is
> > inconsistent with my values.
>
> With mine too and I would presume everybody else's. I've been
> annoyed for a long time by one of those sharded Meson-Win test jobs
> that hang around until timeout.
>
> Thank you very much for addressing the issue.
Indeed, thanks for fixing the issue! I haven't noticed these failing
jobs yet on my end, probably because on GitLab we only execute the
Win+Meson changes manually. Which is not great given that it makes me
miss issues with Meson like this.
I'll send a patch to run these tests by default so that I can see the
pain myself and address any issues that come up before others are forced
to.
> > The reason for this timeout is the test case 'nonsense mv triggers
> > assertion failure and partially updated index' in t7001-mv (which is
> > not even a regression test, but instead merely demonstrates a bug that
> > someone thought someone else should fix at some time). As the name
> > suggests, it triggers an assertion. The problem with this is that an
> > assertion on Windows, at least when run in Debug mode, will open a modal
> > dialog that patiently awaits some buttons to be clicked. Which never
> > happens in automated builds.
>
> Interesting.
>
> So another viable fix (no, I am not suggesting a counter-proposal,
> but asking a pure question to see if I understand the issue
> correctly) is to rewrite "assert(cond)" to "if (cond) BUG(...)"
> or something like that, so that it truly fails?
On the surface this sounds like a reasonable thing to do, but I don't
have enough context to be really able to tell.
> > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> > index 83ca8e4182b..275240be5dc 100644
> > --- a/.github/workflows/main.yml
> > +++ b/.github/workflows/main.yml
> > @@ -265,7 +265,7 @@ jobs:
> > run: pip install meson ninja
> > - name: Setup
> > shell: pwsh
> > - run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred
> > + run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred
> > - name: Compile
> > shell: pwsh
> > run: meson compile -C build
So I'm fine with this trivial change as a band aid for now. The diff
looks obviously good to me.
Patrick |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Patrick Steinhardt <ps@pks.im> writes:
>> > The reason for this timeout is the test case 'nonsense mv triggers
>> > assertion failure and partially updated index' in t7001-mv (which is
>> > not even a regression test, but instead merely demonstrates a bug that
>> > someone thought someone else should fix at some time). As the name
>> > suggests, it triggers an assertion. The problem with this is that an
>> > assertion on Windows, at least when run in Debug mode, will open a modal
>> > dialog that patiently awaits some buttons to be clicked. Which never
>> > happens in automated builds.
>>
>> Interesting.
>>
>> So another viable fix (no, I am not suggesting a counter-proposal,
>> but asking a pure question to see if I understand the issue
>> correctly) is to rewrite "assert(cond)" to "if (cond) BUG(...)"
>> or something like that, so that it truly fails?
>
> On the surface this sounds like a reasonable thing to do, but I don't
> have enough context to be really able to tell.
Interesting again ;-) I didn't realize that it was a fairly recent
development. 0fcd473f (t7001: add failure test which triggers
assertion, 2024-10-22) is what adds the questionable test.
And I do agree with Dscho's assessment that this is "show a bug
without bothering to fix it", which is not what we usually take
without first exploring how involved the necessary fix would be.
I wonder in what bad status would a production build that simply
disabled the assert() is leaving the resulting repository.
Quoting from the last part of my response [*] to the initial report
that eventually turned into the test after 9 months:
[*] https://lore.kernel.org/git/xmqqil47obnw.fsf@gitster.g/
---- snip snap ----
Thanks for reporting, Kristoffer.
Any takers?
$ git shortlog --since=3.years -s -n -e --no-merges v2.43.0 builtin/mv.c
15 Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
10 Elijah Newren <newren@gmail.com>
5 Ævar Arnfjörð Bjarmason <avarab@gmail.com>
2 Junio C Hamano <gitster@pobox.com>
1 Andrzej Hunt <ajrhunt@google.com>
1 Calvin Wan <calvinwan@google.com>
1 Derrick Stolee <stolee@gmail.com>
1 Sebastian Thiel <sebastian.thiel@icloud.com>
1 Torsten Bögershausen <tboegi@web.de> |
|
This branch is now known as |
|
This patch series was integrated into seen via git@28131fe. |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Mon, Apr 28, 2025 at 10:01:34AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> > The reason for this timeout is the test case 'nonsense mv triggers
> >> > assertion failure and partially updated index' in t7001-mv (which is
> >> > not even a regression test, but instead merely demonstrates a bug that
> >> > someone thought someone else should fix at some time). As the name
> >> > suggests, it triggers an assertion. The problem with this is that an
> >> > assertion on Windows, at least when run in Debug mode, will open a modal
> >> > dialog that patiently awaits some buttons to be clicked. Which never
> >> > happens in automated builds.
> >>
> >> Interesting.
> >>
> >> So another viable fix (no, I am not suggesting a counter-proposal,
> >> but asking a pure question to see if I understand the issue
> >> correctly) is to rewrite "assert(cond)" to "if (cond) BUG(...)"
> >> or something like that, so that it truly fails?
> >
> > On the surface this sounds like a reasonable thing to do, but I don't
> > have enough context to be really able to tell.
>
> Interesting again ;-) I didn't realize that it was a fairly recent
> development. 0fcd473f (t7001: add failure test which triggers
> assertion, 2024-10-22) is what adds the questionable test.
>
> And I do agree with Dscho's assessment that this is "show a bug
> without bothering to fix it", which is not what we usually take
> without first exploring how involved the necessary fix would be.
>
> I wonder in what bad status would a production build that simply
> disabled the assert() is leaving the resulting repository.
>
> Quoting from the last part of my response [*] to the initial report
> that eventually turned into the test after 9 months:
>
> [*] https://lore.kernel.org/git/xmqqil47obnw.fsf@gitster.g/
Hm. So the issue here is that we're trying to move both a parent
directory as well as a child thereof at the same point in time? I guess
that'd need something like the below patch to restrict such requests.
This leads to:
$ ./git mv ../builtin/add.c ../builtin ../t
fatal: cannot move both 'builtin/add.c' and its parent directory 'builtin'
The idea of the patch is that we're tracking all parent directories in a
hashmap. After we have processed all arguments, we then perform a second
pass to check that none of the source files have any of their parent
directories moved at the same time.
I only had a brief look, so this assessment may very well be completely
wrong, and the change doesn't even pass all tests right now. But if this
is indeed the issue then I'm happy to polish this into a proper patch.
Patrick
-- >8 --
diff --git a/builtin/mv.c b/builtin/mv.c
index 54b323fff72..10b2cb56ec7 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -183,6 +183,21 @@ static void remove_empty_src_dirs(const char **src_dir, size_t src_dir_nr)
strbuf_release(&a_src_dir);
}
+struct pathmap_entry {
+ struct hashmap_entry ent;
+ const char *path;
+};
+
+static int pathmap_cmp(const void *cmp_data UNUSED,
+ const struct hashmap_entry *a,
+ const struct hashmap_entry *b,
+ const void *key UNUSED)
+{
+ const struct pathmap_entry *e1 = container_of(a, struct pathmap_entry, ent);
+ const struct pathmap_entry *e2 = container_of(b, struct pathmap_entry, ent);
+ return fspathcmp(e1->path, e2->path);
+}
+
int cmd_mv(int argc,
const char **argv,
const char *prefix,
@@ -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);
+
if (first >= 0) {
const char *path = submodule_gitfile_path(src, first);
if (path != SUBMODULE_WITH_GITDIR)
@@ -465,6 +488,40 @@ int cmd_mv(int argc,
}
}
+ for (i = 0; i < argc; i++) {
+ const char *slash_pos;
+
+ strbuf_addstr(&pathbuf, sources.v[i]);
+
+ slash_pos = strrchr(pathbuf.buf, '/');
+ 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));
+
+ if (!hashmap_get_entry(&moved_dirs, &needle, ent, NULL))
+ continue;
+
+ if (!ignore_errors)
+ die(_("cannot move both parent directory '%s' and its child '%s'"),
+ pathbuf.buf, sources.v[i]);
+
+ 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;
+ }
+ }
+ }
+
if (only_match_skip_worktree.nr) {
advise_on_updating_sparse_paths(&only_match_skip_worktree);
if (!ignore_errors) {
@@ -589,6 +646,8 @@ int cmd_mv(int argc,
strvec_clear(&dest_paths);
strvec_clear(&destinations);
strvec_clear(&submodule_gitfiles_to_free);
+ hashmap_clear(&moved_dirs);
+ strbuf_release(&pathbuf);
free(submodule_gitfiles);
free(modes);
return ret;
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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. |
|
On the Git mailing list, "Kristoffer Haugsbakk" wrote (reply to this): On Fri, Apr 25, 2025, at 17:05, Johannes Schindelin via GitGitGadget wrote:
> The reason for this timeout is the test case 'nonsense mv triggers
> assertion failure and partially updated index' in t7001-mv (which is
> not even a regression test, but instead merely demonstrates a bug that
> someone thought someone else should fix at some time). As the name
> suggests, it triggers an assertion. The problem with this is that an
> assertion on Windows, at least when run in Debug mode, will open a modal
> dialog that patiently awaits some buttons to be clicked. Which never
> happens in automated builds.
Sorry for the trouble. |
|
User |
|
This patch series was integrated into seen via git@41cc85b. |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Tue, Apr 29, 2025 at 01:48:18PM -0700, Junio C Hamano wrote:
> 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?
Yup, indeed.
> > + 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.
Ah, there's another gotcha here: when moving a directory, we also add
all of its children to `argc`. So this would now always fail when we
move directories around.
I guess we can handle this by introducing another `MOVE_VIA_PARENT_DIR`
mode -- we'd then skip the verification for any entry marked like this.
> > + 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.
Fully agreed. I was quite surprised to see that git-mv(1) already
behaves like this with a couple of other error conditions. So I simply
continued to build on top of this behaviour, but I'm not a fan of it at
all.
Note that this behaviour doesn't trigger by default though. So your
above command would cause us to die without doing any change at all. You
explicitly have to `git mv -k` (whatever 'k' is supposed to mean --
maybe "keep going"?) to opt into this weird behaviour. Which makes this
overall a bit less awful.
> 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.
Fair. I guess the intent of '-k' is about handling the case where a
subset of files might be missing, not the case where the original
request didn't make any sense at all. I certainly wouldn't mind to
tighten this code.
> 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.
Yeah. From my point of view this isn't the only confusing part about
this code.
Patrick |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Wed, Apr 30, 2025 at 10:58:24AM +0200, Patrick Steinhardt wrote:
> On Tue, Apr 29, 2025 at 01:48:18PM -0700, Junio C Hamano wrote:
> > 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?
>
> Yup, indeed.
>
> > > + 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.
>
> Ah, there's another gotcha here: when moving a directory, we also add
> all of its children to `argc`. So this would now always fail when we
> move directories around.
>
> I guess we can handle this by introducing another `MOVE_VIA_PARENT_DIR`
> mode -- we'd then skip the verification for any entry marked like this.
>
> > > + 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.
>
> Fully agreed. I was quite surprised to see that git-mv(1) already
> behaves like this with a couple of other error conditions. So I simply
> continued to build on top of this behaviour, but I'm not a fan of it at
> all.
>
> Note that this behaviour doesn't trigger by default though. So your
> above command would cause us to die without doing any change at all. You
> explicitly have to `git mv -k` (whatever 'k' is supposed to mean --
> maybe "keep going"?) to opt into this weird behaviour. Which makes this
> overall a bit less awful.
>
> > 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.
>
> Fair. I guess the intent of '-k' is about handling the case where a
> subset of files might be missing, not the case where the original
> request didn't make any sense at all. I certainly wouldn't mind to
> tighten this code.
>
> > 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.
>
> Yeah. From my point of view this isn't the only confusing part about
> this code.
>
> Patrick
>
I have polished this patch a bit now and sent it via [1]. Thanks!
Patrick
[1]: <20250430-pks-mv-parent-child-conflict-v1-0-11a87c55ffb9@pks.im> |
|
This patch series was integrated into seen via git@04579be. |
|
There was a status update in the "Cooking" section about the branch win+Meson CI pipeline, unlike other pipelines for Windows, used to build artifacts in develper mode, which has been changed to build them in release mode for consistency. Expecting a reroll, stating an updated rationale for the change. cf. <xmqqjz6zuf80.fsf@gitster.g> source: <pull.1908.git.1745593515875.gitgitgadget@gmail.com> |
When the `win+Meson` job was added to Git's CI, modeled after the `win+vs` job, it overlooked that the latter built the Git artifacts in release mode. The reason for this is that there is code in `compat/mingw.c` that turns on the modal assertion dialogs in debug mode, which are very useful when debugging interactively (as they offer to attach Visual Studio's debugger), but they are scarcely useful in CI builds (where that modal dialog would sit around, waiting for a human being to see and deal with it, which obviously won't ever happen). This problem was not realized immediately because of a separate bug: the `win+Meson` job erroneously built using the `gcc` that is in the `PATH` by default on hosted GitHub Actions runners. Since that bug was fixed by switching to `--vsenv`, though, the t7001-mv test consistently timed out after six hours in the CI builds on GitHub, quite often, and wasting build minutes without any benefit in return. The reason for this timeout was a symptom of aforementioned debug mode problem, where the test case 'nonsense mv triggers assertion failure and partially updated index' in t7001-mv triggered an assertion. I originally proposed this here patch to address the timeouts in CI builds. The Git project decided to address this timeout differently, though: by fixing the bug that the t7001-mv test case demonstrated. This does not address the debug mode problem, though, as an `assert()` call could be triggered in other ways in CI, and it should still not cause the CI build to hang but should cause Git to error out instead. To avoid having to accept this here patch, it was then proposed to replace all `assert()` calls in Git's code base by `BUG()` calls. This might be reasonable for independent reasons, but it obviously still does not address the debug mode problem, as `assert()` calls could be easily re-introduced by mistake, and besides, Git has a couple of dependencies that all may have their own `assert()` calls (which are then safely outside the control of the Git project to remove), therefore this here patch is still needed. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
dont-let-win+Meson-hang-in-t7001
branch
from
18d1d18 to
c04b6c9
Compare
|
/submit |
|
Submitted as pull.1908.v2.git.1746282346370.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Sat, May 03, 2025 at 02:25:46PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> Changes since v1:
>
> * Rewrote the commit message to reflect that this patch is still
> needed, even if the symptom that originally motivated the patch was
> addressed in a different manner, because it was merely a symptom of
> the underlying root cause that CI builds should not let Visual C
> build Git in debug mode.
Ok, makes sense. I think we should ideally address this issue
strategically, e.g. by getting rid of asserts completely in our
codebase. But for now I agree that we should just build Git in release
mode on Windows.
Thanks!
Patrick |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Patrick,
On Mon, 5 May 2025, Patrick Steinhardt wrote:
> On Sat, May 03, 2025 at 02:25:46PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > Changes since v1:
> >
> > * Rewrote the commit message to reflect that this patch is still
> > needed, even if the symptom that originally motivated the patch was
> > addressed in a different manner, because it was merely a symptom of
> > the underlying root cause that CI builds should not let Visual C
> > build Git in debug mode.
>
> Ok, makes sense. I think we should ideally address this issue
> strategically, e.g. by getting rid of asserts completely in our
> codebase. But for now I agree that we should just build Git in release
> mode on Windows.
I am afraid that getting rid of asserts in Git's codebase won't ever be
able to address the challenge that Git -- despite much reluctance --
relies on a couple of external dependencies that might at any point in
time cause `assert()` to be called, e.g. due to unexpected changes in the
CI runner images.
In essence, we will need both: CI builds in release mode _and_ converting
`assert()`s to `BUG()` calls (the latter, however, for different reasons
than to proactively address CI hangs: Git can convey better information in
`BUG()` calls than in `assert()` calls).
There are more reasons to use release mode builds in CI, too, but we
already have a compelling reason to do so, therefore there is no need for
me to spend more time assembling an exhaustive list.
Ciao,
Johannes |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Mon, May 05, 2025 at 09:27:10AM +0200, Johannes Schindelin wrote:
> Hi Patrick,
>
> On Mon, 5 May 2025, Patrick Steinhardt wrote:
>
> > On Sat, May 03, 2025 at 02:25:46PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > > Changes since v1:
> > >
> > > * Rewrote the commit message to reflect that this patch is still
> > > needed, even if the symptom that originally motivated the patch was
> > > addressed in a different manner, because it was merely a symptom of
> > > the underlying root cause that CI builds should not let Visual C
> > > build Git in debug mode.
> >
> > Ok, makes sense. I think we should ideally address this issue
> > strategically, e.g. by getting rid of asserts completely in our
> > codebase. But for now I agree that we should just build Git in release
> > mode on Windows.
>
> I am afraid that getting rid of asserts in Git's codebase won't ever be
> able to address the challenge that Git -- despite much reluctance --
> relies on a couple of external dependencies that might at any point in
> time cause `assert()` to be called, e.g. due to unexpected changes in the
> CI runner images.
Good point indeed, I haven't considered this.
> In essence, we will need both: CI builds in release mode _and_ converting
> `assert()`s to `BUG()` calls (the latter, however, for different reasons
> than to proactively address CI hangs: Git can convey better information in
> `BUG()` calls than in `assert()` calls).
>
> There are more reasons to use release mode builds in CI, too, but we
> already have a compelling reason to do so, therefore there is no need for
> me to spend more time assembling an exhaustive list.
Yup.
Patrick |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Patrick Steinhardt <ps@pks.im> writes:
>> I am afraid that getting rid of asserts in Git's codebase won't ever be
>> able to address the challenge that Git -- despite much reluctance --
>> relies on a couple of external dependencies that might at any point in
>> time cause `assert()` to be called, e.g. due to unexpected changes in the
>> CI runner images.
>
> Good point indeed, I haven't considered this.
Thanks both for a discussion. Let's replace and queue this, and
fast track it down to 'maint'.
Here is a range-diff for my tentative rebasing the patch on 'maint';
I'll make sure merging it up to 'master' would match exactly the
result of applying the original patch directly to 'master' before
queuing.
Thanks!
1: f3ae94b175 ! 1: 184abdcf05 ci(win+Meson): build in Release mode
@@ Commit message
patch is still needed.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
+ Acked-by: Patrick Steinhardt <ps@pks.im>
+ [jc: rebased on 'maint' to enable fast-tracking the change down]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
## .github/workflows/main.yml ##
@@ .github/workflows/main.yml: jobs:
run: pip install meson ninja
- name: Setup
shell: pwsh
-- run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred
-+ run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred
+- run: meson setup build -Dperl=disabled -Dcredential_helpers=wincred
++ run: meson setup build -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred
- name: Compile
shell: pwsh
run: meson compile -C build
|
|
This patch series was integrated into seen via git@f04d074. |
|
There was a status update in the "Graduated to 'master'" section about the branch win+Meson CI pipeline, unlike other pipelines for Windows, used to build artifacts in develper mode, which has been changed to build them in release mode for consistency. Expecting a reroll, stating an updated rationale for the change. cf. <xmqqjz6zuf80.fsf@gitster.g> source: <pull.1908.git.1745593515875.gitgitgadget@gmail.com> |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Mon, May 05, 2025 at 08:54:23AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> I am afraid that getting rid of asserts in Git's codebase won't ever be
> >> able to address the challenge that Git -- despite much reluctance --
> >> relies on a couple of external dependencies that might at any point in
> >> time cause `assert()` to be called, e.g. due to unexpected changes in the
> >> CI runner images.
> >
> > Good point indeed, I haven't considered this.
>
> Thanks both for a discussion. Let's replace and queue this, and
> fast track it down to 'maint'.
>
> Here is a range-diff for my tentative rebasing the patch on 'maint';
> I'll make sure merging it up to 'master' would match exactly the
> result of applying the original patch directly to 'master' before
> queuing.
>
> Thanks!
>
>
> 1: f3ae94b175 ! 1: 184abdcf05 ci(win+Meson): build in Release mode
> @@ Commit message
> patch is still needed.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> + Acked-by: Patrick Steinhardt <ps@pks.im>
> + [jc: rebased on 'maint' to enable fast-tracking the change down]
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> ## .github/workflows/main.yml ##
> @@ .github/workflows/main.yml: jobs:
> run: pip install meson ninja
> - name: Setup
> shell: pwsh
> -- run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred
> -+ run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred
> +- run: meson setup build -Dperl=disabled -Dcredential_helpers=wincred
> ++ run: meson setup build -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred
> - name: Compile
> shell: pwsh
> run: meson compile -C build
Am I reading this diff correctly that we drop the `--vsenv` flag? I
think we should keep it around as it was a bug in the first place that
we didn't have it.
I guess this is done because we didn't have the flag in "maint" yet. But
I'm not even sure whether the fix would be needed in case we don't build
with Visual Studio, as the MinGW toolchain probably doesn't have the
same behaviour with asserts (but please correct me if I'm wrong, Dscho).
So maybe we should also cherry-pick 85e1d6819fb (ci: use Visual Studio
for win+meson job on GitHub Workflows, 2025-03-31) at the same time.
Patrick |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Patrick,
On Tue, 6 May 2025, Patrick Steinhardt wrote:
> On Mon, May 05, 2025 at 08:54:23AM -0700, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> > >> I am afraid that getting rid of asserts in Git's codebase won't ever be
> > >> able to address the challenge that Git -- despite much reluctance --
> > >> relies on a couple of external dependencies that might at any point in
> > >> time cause `assert()` to be called, e.g. due to unexpected changes in the
> > >> CI runner images.
> > >
> > > Good point indeed, I haven't considered this.
> >
> > Thanks both for a discussion. Let's replace and queue this, and
> > fast track it down to 'maint'.
> >
> > Here is a range-diff for my tentative rebasing the patch on 'maint';
> > I'll make sure merging it up to 'master' would match exactly the
> > result of applying the original patch directly to 'master' before
> > queuing.
> >
> > Thanks!
> >
> >
> > 1: f3ae94b175 ! 1: 184abdcf05 ci(win+Meson): build in Release mode
> > @@ Commit message
> > patch is still needed.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > + Acked-by: Patrick Steinhardt <ps@pks.im>
> > + [jc: rebased on 'maint' to enable fast-tracking the change down]
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> >
> > ## .github/workflows/main.yml ##
> > @@ .github/workflows/main.yml: jobs:
> > run: pip install meson ninja
> > - name: Setup
> > shell: pwsh
> > -- run: meson setup build --vsenv -Dperl=disabled -Dcredential_helpers=wincred
> > -+ run: meson setup build --vsenv -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred
> > +- run: meson setup build -Dperl=disabled -Dcredential_helpers=wincred
> > ++ run: meson setup build -Dbuildtype=release -Dperl=disabled -Dcredential_helpers=wincred
> > - name: Compile
> > shell: pwsh
> > run: meson compile -C build
>
> Am I reading this diff correctly that we drop the `--vsenv` flag?
That's similar to my reading as well, but...
> I think we should keep it around as it was a bug in the first place that
> we didn't have it.
Correct.
> I guess this is done because we didn't have the flag in "maint" yet.
... I think this is the reason. It would probably make more sense to
backport the `--vsenv` patch first, seeing as having `win+Meson` without
that flag defeats the purpose of `win+Meson`.
> But I'm not even sure whether the fix would be needed in case we don't
> build with Visual Studio, as the MinGW toolchain probably doesn't have
> the same behaviour with asserts (but please correct me if I'm wrong,
> Dscho). So maybe we should also cherry-pick 85e1d6819fb (ci: use Visual
> Studio for win+meson job on GitHub Workflows, 2025-03-31) at the same
> time.
Indeed. If we keep both bugs unfixed in `maint` (the bug that `win+Meson`
does not build with Visual Studio, plus the bug that `win+Meson` builds in
Debug mode), the CI will continue to pass... just not for the intended
reasons.
Ciao,
Johannes |
|
This branch is now known as |
|
This patch series was integrated into seen via git@11201ca. |
|
There was a status update in the "Cooking" section about the branch win+Meson CI pipeline, unlike other pipelines for Windows, used to build artifacts in develper mode, which has been changed to build them in release mode for consistency. WIll merge to 'next'. source: <pull.1908.v2.git.1746282346370.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@d72d55f. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Indeed. If we keep both bugs unfixed in `maint` (the bug that `win+Meson`
> does not build with Visual Studio, plus the bug that `win+Meson` builds in
> Debug mode), the CI will continue to pass... just not for the intended
> reasons.
That's fun ;-) |
|
This patch series was integrated into seen via git@fa626d8. |
|
This patch series was integrated into seen via git@6f4d284. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was surprised to find this issue today, and that this had not been addressed yet.
Changes since v1:
Cc: Patrick Steinhardt ps@pks.im
cc: "Kristoffer Haugsbakk" code@khaugsbakk.name