* Unexpected git merge exit code when killing merge driver during ancestor merge
@ 2024-04-04 16:16 Matt Cree
2024-04-05 20:04 ` brian m. carlson
0 siblings, 1 reply; 6+ messages in thread
From: Matt Cree @ 2024-04-04 16:16 UTC (permalink / raw
To: git
Hello all. I have observed some strange behaviour when exiting a custom merge driver that I was wondering if there’s any reason for — I think it may be a bug but I’ll leave that to you to decide.
I’m configuring that merge driver to exit during a merge at the first sign of conflicts — the exact nature of the rules for the decision to exit early isn’t too important I think though so given it’s ‘work stuff’ I’ll leave some details out.
Here is my current understanding of how the ort strategy will deal with this.
- Ort runs the merge driver with the parameters for the current file to be merged
- When the driver returns exit code 0 is returned it is treated as having no conflicts
- When the driver returns exit code 1-128 is returned it is treated as having conflicts
- When the driver returns exit code 129+ is returned it is treated as some kind of error scenario
Then subsequently
- If all files returned exit code 0 during the merge git will return exit code 0 i.e. no conflicts
- If any file returned exit code 1-128 during the merge git will return exit code 1 i.e. conflicts
- At any time if the driver returns 129+, git will stop merging and return exit code 2 i.e. error?
However, when setting up a criss-cross merge scenario and ‘short circuiting’ the merge during an ancestor merge, I get exit code 134
Here’s a couple of quick scripts that help recreate the situation https://gist.github.com/mattcree/c6d8cc95f41e30b5d7467e9d2b01cd3d
The logs also show
```
Assertion failed: (opt->priv == NULL), function merge_switch_to_result, file merge-ort.c, line 4661. ./run-recursive-merge.sh: line 162: 78797 Abort trap: 6 git merge $featureC --no-ff --no-commit
```
I thought it might be worth raising as a bug here but I’m not too sure really — I suppose the short circuiting logic I have introduced may not be a desirable use case from the git elders point of view, but I reckon the difference in behaviour depending on whether it’s an ancestor merge or a final merge seems to indicate to me that this is not intentional.
Hopefully someone knows a bit more about this.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Unexpected git merge exit code when killing merge driver during ancestor merge
2024-04-04 16:16 Unexpected git merge exit code when killing merge driver during ancestor merge Matt Cree
@ 2024-04-05 20:04 ` brian m. carlson
2024-05-09 5:03 ` Elijah Newren
0 siblings, 1 reply; 6+ messages in thread
From: brian m. carlson @ 2024-04-05 20:04 UTC (permalink / raw
To: Matt Cree; +Cc: git, Elijah Newren
[-- Attachment #1: Type: text/plain, Size: 3897 bytes --]
On 2024-04-04 at 16:16:05, Matt Cree wrote:
> Hello all. I have observed some strange behaviour when exiting a custom merge driver that I was wondering if there’s any reason for — I think it may be a bug but I’ll leave that to you to decide.
>
> I’m configuring that merge driver to exit during a merge at the first sign of conflicts — the exact nature of the rules for the decision to exit early isn’t too important I think though so given it’s ‘work stuff’ I’ll leave some details out.
>
> Here is my current understanding of how the ort strategy will deal with this.
>
> - Ort runs the merge driver with the parameters for the current file to be merged
> - When the driver returns exit code 0 is returned it is treated as having no conflicts
> - When the driver returns exit code 1-128 is returned it is treated as having conflicts
> - When the driver returns exit code 129+ is returned it is treated as some kind of error scenario
>
>
> Then subsequently
> - If all files returned exit code 0 during the merge git will return exit code 0 i.e. no conflicts
> - If any file returned exit code 1-128 during the merge git will return exit code 1 i.e. conflicts
> - At any time if the driver returns 129+, git will stop merging and return exit code 2 i.e. error?
>
> However, when setting up a criss-cross merge scenario and ‘short circuiting’ the merge during an ancestor merge, I get exit code 134
>
> Here’s a couple of quick scripts that help recreate the situation https://gist.github.com/mattcree/c6d8cc95f41e30b5d7467e9d2b01cd3d
Thanks for the repro steps. I'm on Debian, which uses dash as /bin/sh,
and I also use a different default branch (dev), so I was able to
reproduce with the following patch applied:
----
diff --git a/init-repo.sh b/init-repo.sh
old mode 100644
new mode 100755
index e0f42a4..25d7f25
--- a/init-repo.sh
+++ b/init-repo.sh
@@ -1,5 +1,5 @@
rm -rf merge-driver-test
mkdir merge-driver-test
cd merge-driver-test
-git init .
+git init -b master .
git commit --allow-empty -m "Initial"
\ No newline at end of file
diff --git a/run-merge.sh b/run-merge.sh
old mode 100644
new mode 100755
diff --git a/run-recursive-merge.sh b/run-recursive-merge.sh
old mode 100644
new mode 100755
index 6920720..c63d652
--- a/run-recursive-merge.sh
+++ b/run-recursive-merge.sh
@@ -1,3 +1,5 @@
+#!/bin/sh
+
cd merge-driver-test
current_time=$(date "+%Y%m%d-%H%M%S");
@@ -12,7 +14,7 @@ featureA="$current_time-feature-a";
featureB="$current_time-feature-b";
featureC="$current_time-feature-c";
-function writeFiles() {
+writeFiles() {
cat > $xmlFileName << EOM
<?xml version="1.0" encoding="UTF-8"?>
<CustomLabels xmlns="http://soap.sforce.com/2006/04/metadata">
----
I take it from the "Abort trap" message below, you're on macOS, but I
don't think that's relevant to reproduction.
> The logs also show
>
> ```
> Assertion failed: (opt->priv == NULL), function merge_switch_to_result, file merge-ort.c, line 4661. ./run-recursive-merge.sh: line 162: 78797 Abort trap: 6 git merge $featureC --no-ff --no-commit
> ```
This is definitely a bug because we triggered an assertion. The
assertion asserts that that case will never happen, so if it does, we've
made a mistake in our code.
This also explains the 134 exit status, because on most Unix systems,
`SIGABRT` is signal 6, and when a program exits with a signal, the shell
returns an exit status of 128 plus the signal number. Because a failed
assertion calls `abort`, which raises `SIGABRT`, that would lead to an
exit status in the shell of 134.
I've CC'd Elijah Newren, who's the author of merge-ort and who wrote the
code. I'm not familiar at all with merge-ort, so I can't speak to what
might be going wrong here.
--
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Unexpected git merge exit code when killing merge driver during ancestor merge
@ 2024-05-08 18:14 Matt Cree
0 siblings, 0 replies; 6+ messages in thread
From: Matt Cree @ 2024-05-08 18:14 UTC (permalink / raw
To: sandals; +Cc: git, matt.cree, newren
As time has gone on, I’ve discovered when we do have this crash during an ancestor merge, somewhat unsurprisingly there is an index.lock file left over in the .git directory.
I’ve taken a look at the code and I believe I’ve spotted the issue — I have v2.44.0 checked out atm.
In the function `merge_ort_internal` function the ancestor merges occur using recursion. If there’s an error during this process, a negative number must be set as the `result->clean` value and this is used to terminate the recursion early https://github.com/git/git/blob/184c3b4c735f1c1f0f811fddcf3c2676e7ad613f/merge-ort.c#L5052
In terminating here, it means the code does not continue on to the final merge which uses the merge_ort_nonrecursive_internal which would have been called in the non recursive case anyway https://github.com/git/git/blob/184c3b4c735f1c1f0f811fddcf3c2676e7ad613f/merge-ort.c#L5068
From what I can see, that function will correctly reinitialise/reset the opt data structure if the call depth is 0? I am guessing that is an indicator that the merge is supposed to be finished? If we did not clear it here, I’d guess we’d run into the same bug I identified during a non-recursive merge. Therefore, back at the point we first found a negative `result->clean` value, I think we probably want to do a similar reinitialisation of the opts i.e.
> if (result->clean < 0)
> /*
> * When there are ancestor merge failures,
> * the merge_ort_nonrecursive_internal() cleans these up
> * so we must do it here too
> */
> result->priv = opt->priv;
> result->_properly_initialized = RESULT_INITIALIZED;
> opt->priv = NULL;
> return;
I’m really not familiar with C or the git source code so this might be wrong, but it would be useful to have this case handled by git rather than myself as the time between an ancestor merge and a final merge can vary wildly and any kind of ‘short circuit’ logic I write would prefer heavily to use discover of conflicts during an ancestor merge.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Unexpected git merge exit code when killing merge driver during ancestor merge
2024-04-05 20:04 ` brian m. carlson
@ 2024-05-09 5:03 ` Elijah Newren
2024-06-13 20:50 ` Elijah Newren
0 siblings, 1 reply; 6+ messages in thread
From: Elijah Newren @ 2024-05-09 5:03 UTC (permalink / raw
To: brian m. carlson, Matt Cree, git, Elijah Newren
On Fri, Apr 5, 2024 at 1:04 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2024-04-04 at 16:16:05, Matt Cree wrote:
> > Hello all. I have observed some strange behaviour when exiting a custom merge driver that I was wondering if there’s any reason for — I think it may be a bug but I’ll leave that to you to decide.
> >
> > I’m configuring that merge driver to exit during a merge at the first sign of conflicts — the exact nature of the rules for the decision to exit early isn’t too important I think though so given it’s ‘work stuff’ I’ll leave some details out.
> >
> > Here is my current understanding of how the ort strategy will deal with this.
> >
> > - Ort runs the merge driver with the parameters for the current file to be merged
> > - When the driver returns exit code 0 is returned it is treated as having no conflicts
> > - When the driver returns exit code 1-128 is returned it is treated as having conflicts
> > - When the driver returns exit code 129+ is returned it is treated as some kind of error scenario
> >
> >
> > Then subsequently
> > - If all files returned exit code 0 during the merge git will return exit code 0 i.e. no conflicts
> > - If any file returned exit code 1-128 during the merge git will return exit code 1 i.e. conflicts
> > - At any time if the driver returns 129+, git will stop merging and return exit code 2 i.e. error?
> >
> > However, when setting up a criss-cross merge scenario and ‘short circuiting’ the merge during an ancestor merge, I get exit code 134
> >
> > Here’s a couple of quick scripts that help recreate the situation https://gist.github.com/mattcree/c6d8cc95f41e30b5d7467e9d2b01cd3d
>
> Thanks for the repro steps. I'm on Debian, which uses dash as /bin/sh,
> and I also use a different default branch (dev), so I was able to
> reproduce with the following patch applied:
>
> ----
> diff --git a/init-repo.sh b/init-repo.sh
> old mode 100644
> new mode 100755
> index e0f42a4..25d7f25
> --- a/init-repo.sh
> +++ b/init-repo.sh
> @@ -1,5 +1,5 @@
> rm -rf merge-driver-test
> mkdir merge-driver-test
> cd merge-driver-test
> -git init .
> +git init -b master .
> git commit --allow-empty -m "Initial"
> \ No newline at end of file
> diff --git a/run-merge.sh b/run-merge.sh
> old mode 100644
> new mode 100755
> diff --git a/run-recursive-merge.sh b/run-recursive-merge.sh
> old mode 100644
> new mode 100755
> index 6920720..c63d652
> --- a/run-recursive-merge.sh
> +++ b/run-recursive-merge.sh
> @@ -1,3 +1,5 @@
> +#!/bin/sh
> +
> cd merge-driver-test
>
> current_time=$(date "+%Y%m%d-%H%M%S");
> @@ -12,7 +14,7 @@ featureA="$current_time-feature-a";
> featureB="$current_time-feature-b";
> featureC="$current_time-feature-c";
>
> -function writeFiles() {
> +writeFiles() {
> cat > $xmlFileName << EOM
> <?xml version="1.0" encoding="UTF-8"?>
> <CustomLabels xmlns="http://soap.sforce.com/2006/04/metadata">
> ----
>
> I take it from the "Abort trap" message below, you're on macOS, but I
> don't think that's relevant to reproduction.
>
> > The logs also show
> >
> > ```
> > Assertion failed: (opt->priv == NULL), function merge_switch_to_result, file merge-ort.c, line 4661. ./run-recursive-merge.sh: line 162: 78797 Abort trap: 6 git merge $featureC --no-ff --no-commit
> > ```
>
> This is definitely a bug because we triggered an assertion. The
> assertion asserts that that case will never happen, so if it does, we've
> made a mistake in our code.
>
> This also explains the 134 exit status, because on most Unix systems,
> `SIGABRT` is signal 6, and when a program exits with a signal, the shell
> returns an exit status of 128 plus the signal number. Because a failed
> assertion calls `abort`, which raises `SIGABRT`, that would lead to an
> exit status in the shell of 134.
>
> I've CC'd Elijah Newren, who's the author of merge-ort and who wrote the
> code. I'm not familiar at all with merge-ort, so I can't speak to what
> might be going wrong here.
brian: Thanks for tagging me and expounding on the testcase.
Matt: sorry for taking so long to respond.
This is just a quick note to say I'm aware of the bug and will respond
(I think there might be a simple fix here), but for various reasons
it's going to be a couple more weeks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Unexpected git merge exit code when killing merge driver during ancestor merge
2024-05-09 5:03 ` Elijah Newren
@ 2024-06-13 20:50 ` Elijah Newren
2024-06-13 22:22 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Elijah Newren @ 2024-06-13 20:50 UTC (permalink / raw
To: brian m. carlson, Matt Cree, git, Elijah Newren
On Wed, May 8, 2024 at 10:03 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Fri, Apr 5, 2024 at 1:04 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> >
> > On 2024-04-04 at 16:16:05, Matt Cree wrote:
> > > Hello all. I have observed some strange behaviour when exiting a custom merge driver that I was wondering if there’s any reason for — I think it may be a bug but I’ll leave that to you to decide.
> > >
[...]
> > This is definitely a bug because we triggered an assertion. The
> > assertion asserts that that case will never happen, so if it does, we've
> > made a mistake in our code.
[...]
> > I've CC'd Elijah Newren, who's the author of merge-ort and who wrote the
> > code. I'm not familiar at all with merge-ort, so I can't speak to what
> > might be going wrong here.
>
> brian: Thanks for tagging me and expounding on the testcase.
> Matt: sorry for taking so long to respond.
>
> This is just a quick note to say I'm aware of the bug and will respond
> (I think there might be a simple fix here), but for various reasons
> it's going to be a couple more weeks.
Again, sorry for the delay. For those reading through the archives, I
posted a fix to this and a few other issues found while investigating
this problem over here:
https://lore.kernel.org/git/pull.1748.git.1718310307.gitgitgadget@gmail.com/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Unexpected git merge exit code when killing merge driver during ancestor merge
2024-06-13 20:50 ` Elijah Newren
@ 2024-06-13 22:22 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2024-06-13 22:22 UTC (permalink / raw
To: Elijah Newren; +Cc: brian m. carlson, Matt Cree, git
Elijah Newren <newren@gmail.com> writes:
> Again, sorry for the delay. For those reading through the archives, I
> posted a fix to this and a few other issues found while investigating
> this problem over here:
> https://lore.kernel.org/git/pull.1748.git.1718310307.gitgitgadget@gmail.com/
Thanks for a pleasant read. It mostly looked fine. Queued.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-06-13 22:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-04 16:16 Unexpected git merge exit code when killing merge driver during ancestor merge Matt Cree
2024-04-05 20:04 ` brian m. carlson
2024-05-09 5:03 ` Elijah Newren
2024-06-13 20:50 ` Elijah Newren
2024-06-13 22:22 ` Junio C Hamano
-- strict thread matches above, loose matches on Subject: below --
2024-05-08 18:14 Matt Cree
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).