Question about pre-merge and git merge octopus strategy

Question about pre-merge and git merge octopus strategy

[ZheNing Hu]

Hi,

I am thinking about if git can "pre-merge" multiple branches, which
can check if merge
will have conflict, but not to merge them actually, like a option `--intend`.

I find "git merge-tree" can output merge result in stdout, which meets
my needs, but it can only
support two branches' merge.

So I find git merge with more than two branches can use octopus strategy.
What about git merge --no-commit? Which will not commit automatically,
so we can check if they have
confilct, and abort merge.

I think it's not useful for git merge-octopus, because if we meet a
merge conflict, we can't find
MERGE_HEAD at all! How can we abort this conflict merge?

--
ZheNing Hu

Re: Question about pre-merge and git merge octopus strategy

[Christian Couder]

Hi,

On Fri, May 6, 2022 at 10:15 AM ZheNing Hu <adlternative@gmail.com> wrote:

> I am thinking about if git can "pre-merge" multiple branches, which
> can check if merge
> will have conflict, but not to merge them actually, like a option `--intend`.
>
> I find "git merge-tree" can output merge result in stdout, which meets
> my needs, but it can only
> support two branches' merge.

Elijah (added in Cc) has been working on "git merge-tree" improvements
based on the new "ort" merge he developed. It supports merging 2
branches, but maybe there are ways to make it support more than 2.

> So I find git merge with more than two branches can use octopus strategy.
> What about git merge --no-commit? Which will not commit automatically,
> so we can check if they have
> confilct, and abort merge.

Yeah, I think that's what you want.

> I think it's not useful for git merge-octopus, because if we meet a
> merge conflict, we can't find
> MERGE_HEAD at all! How can we abort this conflict merge?

I don't know octopus merges much, but I think you should be able to
abort using "git reset" (maybe with "--hard"). If the merge was
performed using --no-commit or if there was a conflict, then I think
it should be expected that there is no MERGE_HEAD as no commit would
be created so MERGE_HEAD would have nothing to point to.

Re: Question about pre-merge and git merge octopus strategy

[Elijah Newren]

On Fri, May 6, 2022 at 10:24 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> Hi,
>
> On Fri, May 6, 2022 at 10:15 AM ZheNing Hu <adlternative@gmail.com> wrote:
>
> > I am thinking about if git can "pre-merge" multiple branches, which
> > can check if merge
> > will have conflict, but not to merge them actually, like a option `--intend`.
> >
> > I find "git merge-tree" can output merge result in stdout, which meets
> > my needs, but it can only
> > support two branches' merge.
>
> Elijah (added in Cc) has been working on "git merge-tree" improvements
> based on the new "ort" merge he developed. It supports merging 2
> branches, but maybe there are ways to make it support more than 2.

The primary issue with in-core octopus merges is that there are lots
of questions about how to handle conflicts; possibly even more so than
git merge-tree --write-tree brings up, and that took us months of
discussion.  In particular, with octopii, do iterate merging one
branch in at a time and stop with any conflicts (thus potentially
stopping N-1 times when merging N branches), do you attempt to just
run all N-1 merges and have conflicts that can't readily be expressed
in the index (especially for non-textual multi-way conflicts, but even
nested conflict markers for text files could be painful), do you
attempt to handle the multi-way textual conflicts a new way with code
replacing xdiff/ in order to avoid nested conflicts?  I wasn't sure
what to do, so never implemented that in "ort".

Of course, people could roll their own by just serially merging pairs
of commits, and then rewriting the history to replace the string of
merges with an octopus.  Or perhaps just use "git merge --no-commit"
serially, though that only works if the branches touch disjoint files
(otherwise one of the merges will complain you have changes that could
be overwritten by the next merge).  And yes, if you don't want to mess
with the working tree/index, you could serially use the "git
merge-tree --write-tree" once I finish it, but it's not ready yet.
(Sorry about that; I've got a bunch of nearly complete changes from a
while ago but just didn't have much Git time.  I'll try to get to it
soon.)

However, I think Junio said that octopus merge only handles trivial
cases anyway, in which case an iterated "git merge --write-tree" would
actually be a sufficient solution here and we could sidestep the more
convoluted cases.  But, sadly for ZheNing, that option doesn't exist
yet -- It's still under development.

> > So I find git merge with more than two branches can use octopus strategy.
> > What about git merge --no-commit? Which will not commit automatically,
> > so we can check if they have
> > confilct, and abort merge.
>
> Yeah, I think that's what you want.
>
> > I think it's not useful for git merge-octopus, because if we meet a
> > merge conflict, we can't find
> > MERGE_HEAD at all! How can we abort this conflict merge?

MERGE_HEAD doesn't have anything to do with aborting the conflict
resolution step.  When you need to abort, the thing you want to go
back to is HEAD (which represents the commit you had checked out and
were merging the other stuff into), not MERGE_HEAD (which represents
the branch or branches you were merging into HEAD).

> I don't know octopus merges much, but I think you should be able to
> abort using "git reset" (maybe with "--hard").  If the merge was
> performed using --no-commit or if there was a conflict, then I think
> it should be expected that there is no MERGE_HEAD as no commit would
> be created so MERGE_HEAD would have nothing to point to.

MERGE_HEAD isn't something created during a merge, it's something that
existed before it -- namely, the tip of the other branch we are
merging.  For an octopus merge, you'd thus expect it to have N commits
rather than just 1.

AUTO_MERGE, new to ort, is something that is created during a merge
and when the merge interrupts due to conflicts to ask the user to
resolve the conflicts, AUTO_MERGE represents the tree checked out in
the working copy (thus it is a tree that likely has files with
conflict markers in them).

Re: Question about pre-merge and git merge octopus strategy

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> However, I think Junio said that octopus merge only handles trivial
> cases anyway,...

"git merge -s octopus A B C" deliberately made a design decision to
strongly discourage the users from creating an octopus that requires
non-trivial merges.  One of the reasons is because octopus merges
make the history less efficient to bisect, and they are meant as a
way to bind totally unrelated changes, whose order of merging into
the resulting history does not matter.

Yes, it is an opinionated design choice, but "git merge" is a
Porcelain that can afford to be opinionated.  It also means that
tools other than the git-merge--octopus backend does not have to
follow the design philosophy.

> ... in which case an iterated "git merge --write-tree" would
> actually be a sufficient solution here and we could sidestep the more
> convoluted cases.  But, sadly for ZheNing, that option doesn't exist
> yet -- It's still under development.

Re: Question about pre-merge and git merge octopus strategy

[ZheNing Hu]

Christian Couder <christian.couder@gmail.com> 于2022年5月7日周六 01:24写道：
>
> Hi,
>
> On Fri, May 6, 2022 at 10:15 AM ZheNing Hu <adlternative@gmail.com> wrote:
>
> > I am thinking about if git can "pre-merge" multiple branches, which
> > can check if merge
> > will have conflict, but not to merge them actually, like a option `--intend`.
> >
> > I find "git merge-tree" can output merge result in stdout, which meets
> > my needs, but it can only
> > support two branches' merge.
>
> Elijah (added in Cc) has been working on "git merge-tree" improvements
> based on the new "ort" merge he developed. It supports merging 2
> branches, but maybe there are ways to make it support more than 2.
>

OK. As he said, there is not such option :(

> > So I find git merge with more than two branches can use octopus strategy.
> > What about git merge --no-commit? Which will not commit automatically,
> > so we can check if they have
> > confilct, and abort merge.
>
> Yeah, I think that's what you want.
>
> > I think it's not useful for git merge-octopus, because if we meet a
> > merge conflict, we can't find
> > MERGE_HEAD at all! How can we abort this conflict merge?
>
> I don't know octopus merges much, but I think you should be able to
> abort using "git reset" (maybe with "--hard"). If the merge was
> performed using --no-commit or if there was a conflict, then I think
> it should be expected that there is no MERGE_HEAD as no commit would
> be created so MERGE_HEAD would have nothing to point to.

Yes, git reset --hard HEAD is the way get out of merging conflicts. But why
git merge --abort doesn't work? I think it may confuse users...

Re: Question about pre-merge and git merge octopus strategy

[ZheNing Hu]

Elijah Newren <newren@gmail.com> 于2022年5月7日周六 12:09写道：
>
> On Fri, May 6, 2022 at 10:24 AM Christian Couder
> <christian.couder@gmail.com> wrote:
> >
> > Hi,
> >
> > On Fri, May 6, 2022 at 10:15 AM ZheNing Hu <adlternative@gmail.com> wrote:
> >
> > > I am thinking about if git can "pre-merge" multiple branches, which
> > > can check if merge
> > > will have conflict, but not to merge them actually, like a option `--intend`.
> > >
> > > I find "git merge-tree" can output merge result in stdout, which meets
> > > my needs, but it can only
> > > support two branches' merge.
> >
> > Elijah (added in Cc) has been working on "git merge-tree" improvements
> > based on the new "ort" merge he developed. It supports merging 2
> > branches, but maybe there are ways to make it support more than 2.
>
> The primary issue with in-core octopus merges is that there are lots
> of questions about how to handle conflicts; possibly even more so than
> git merge-tree --write-tree brings up, and that took us months of
> discussion.  In particular, with octopii, do iterate merging one
> branch in at a time and stop with any conflicts (thus potentially
> stopping N-1 times when merging N branches), do you attempt to just
> run all N-1 merges and have conflicts that can't readily be expressed
> in the index (especially for non-textual multi-way conflicts, but even
> nested conflict markers for text files could be painful), do you
> attempt to handle the multi-way textual conflicts a new way with code
> replacing xdiff/ in order to avoid nested conflicts?  I wasn't sure
> what to do, so never implemented that in "ort".
>

I have see the code of git-merge-octopus.sh, it run merge-base,
read-tree, write-tree every loop.

Now I get you about it's hard to do multiple tree merge because it's hard to
express in index.

> Of course, people could roll their own by just serially merging pairs
> of commits, and then rewriting the history to replace the string of
> merges with an octopus.  Or perhaps just use "git merge --no-commit"
> serially, though that only works if the branches touch disjoint files
> (otherwise one of the merges will complain you have changes that could
> be overwritten by the next merge).  And yes, if you don't want to mess
> with the working tree/index, you could serially use the "git
> merge-tree --write-tree" once I finish it, but it's not ready yet.
> (Sorry about that; I've got a bunch of nearly complete changes from a
> while ago but just didn't have much Git time.  I'll try to get to it
> soon.)
>

I'll be looking forward to this feature!

> However, I think Junio said that octopus merge only handles trivial
> cases anyway, in which case an iterated "git merge --write-tree" would
> actually be a sufficient solution here and we could sidestep the more
> convoluted cases.  But, sadly for ZheNing, that option doesn't exist
> yet -- It's still under development.
>
> > > So I find git merge with more than two branches can use octopus strategy.
> > > What about git merge --no-commit? Which will not commit automatically,
> > > so we can check if they have
> > > confilct, and abort merge.
> >
> > Yeah, I think that's what you want.
> >
> > > I think it's not useful for git merge-octopus, because if we meet a
> > > merge conflict, we can't find
> > > MERGE_HEAD at all! How can we abort this conflict merge?
>
> MERGE_HEAD doesn't have anything to do with aborting the conflict
> resolution step.  When you need to abort, the thing you want to go
> back to is HEAD (which represents the commit you had checked out and
> were merging the other stuff into), not MERGE_HEAD (which represents
> the branch or branches you were merging into HEAD).
>

Thanks for clarifying. As I reply to Christian, when I just use "git
merge A B C" happily,
and there is a conflict, so I try "git merge --abort" as usual, but it
can not work... git tell me:

fatal: There is no merge to abort (MERGE_HEAD missing).

> > I don't know octopus merges much, but I think you should be able to
> > abort using "git reset" (maybe with "--hard").  If the merge was
> > performed using --no-commit or if there was a conflict, then I think
> > it should be expected that there is no MERGE_HEAD as no commit would
> > be created so MERGE_HEAD would have nothing to point to.
>
> MERGE_HEAD isn't something created during a merge, it's something that
> existed before it -- namely, the tip of the other branch we are
> merging.  For an octopus merge, you'd thus expect it to have N commits
> rather than just 1.
>
> AUTO_MERGE, new to ort, is something that is created during a merge
> and when the merge interrupts due to conflicts to ask the user to
> resolve the conflicts, AUTO_MERGE represents the tree checked out in
> the working copy (thus it is a tree that likely has files with
> conflict markers in them).

I think this AUTO_MERGE maybe a good thing for checking merge conflicts,
maybe it will help git merge-octopus.

Thanks.

Re: Question about pre-merge and git merge octopus strategy

[Carlo Marcelo Arenas Belón]

On Fri, May 06, 2022 at 04:14:56PM +0800, ZheNing Hu wrote:
> 
> I am thinking about if git can "pre-merge" multiple branches, which
> can check if merge
> will have conflict, but not to merge them actually, like a option `--intend`.

why not merge them and find that way if they have conflicts that are left
unresolved, or are you also interested in conflicts that were solved by the
merge?

if you only care about the unresolved conflicts then (unless this broke
recently with ort) the following (untested) should work:

  $ git switch target
  $ git merge topic && echo "SHIP IT!" || git reset --hard ORIG_HEAD

obviously will need to be changed if you want to merge more than two branches
but the principle is tme same.

Carlo

Re: Question about pre-merge and git merge octopus strategy

[Elijah Newren]

On Sun, May 8, 2022 at 7:44 AM ZheNing Hu <adlternative@gmail.com> wrote:
>
> Elijah Newren <newren@gmail.com> 于2022年5月7日周六 12:09写道：
> >
> > On Fri, May 6, 2022 at 10:24 AM Christian Couder
> > <christian.couder@gmail.com> wrote:
> > >
> > > On Fri, May 6, 2022 at 10:15 AM ZheNing Hu <adlternative@gmail.com> wrote:
[...]
> > > > I think it's not useful for git merge-octopus, because if we meet a
> > > > merge conflict, we can't find
> > > > MERGE_HEAD at all! How can we abort this conflict merge?
> >
> > MERGE_HEAD doesn't have anything to do with aborting the conflict
> > resolution step.  When you need to abort, the thing you want to go
> > back to is HEAD (which represents the commit you had checked out and
> > were merging the other stuff into), not MERGE_HEAD (which represents
> > the branch or branches you were merging into HEAD).
> >
>
> Thanks for clarifying. As I reply to Christian, when I just use "git
> merge A B C" happily,
> and there is a conflict, so I try "git merge --abort" as usual, but it
> can not work... git tell me:
>
> fatal: There is no merge to abort (MERGE_HEAD missing).

Sounds like a bug to me; .git/MERGE_HEAD should be written.  That file
is created for me when I set up a simple octopus merge that has
conflicts.  Do you have a set of steps others can use to reproduce the
problem you are seeing?

Re: Question about pre-merge and git merge octopus strategy

[ZheNing Hu]

Elijah Newren <newren@gmail.com> 于2022年5月10日周二 15:07写道：
>
> On Sun, May 8, 2022 at 7:44 AM ZheNing Hu <adlternative@gmail.com> wrote:
> >
> > Elijah Newren <newren@gmail.com> 于2022年5月7日周六 12:09写道：
> > >
> > > On Fri, May 6, 2022 at 10:24 AM Christian Couder
> > > <christian.couder@gmail.com> wrote:
> > > >
> > > > On Fri, May 6, 2022 at 10:15 AM ZheNing Hu <adlternative@gmail.com> wrote:
> [...]
> > > > > I think it's not useful for git merge-octopus, because if we meet a
> > > > > merge conflict, we can't find
> > > > > MERGE_HEAD at all! How can we abort this conflict merge?
> > >
> > > MERGE_HEAD doesn't have anything to do with aborting the conflict
> > > resolution step.  When you need to abort, the thing you want to go
> > > back to is HEAD (which represents the commit you had checked out and
> > > were merging the other stuff into), not MERGE_HEAD (which represents
> > > the branch or branches you were merging into HEAD).
> > >
> >
> > Thanks for clarifying. As I reply to Christian, when I just use "git
> > merge A B C" happily,
> > and there is a conflict, so I try "git merge --abort" as usual, but it
> > can not work... git tell me:
> >
> > fatal: There is no merge to abort (MERGE_HEAD missing).
>
> Sounds like a bug to me; .git/MERGE_HEAD should be written.  That file
> is created for me when I set up a simple octopus merge that has
> conflicts.  Do you have a set of steps others can use to reproduce the
> problem you are seeing?

Let me minimally reproduce this question (with git 2.33.0):

#!/bin/bash

rm -rf test-repo
git init test-repo
cd test-repo
git branch -m main
echo base > base
git add .
git commit -m "base"
git branch -c dev-1
git branch -c dev-2
echo main > main
git add .
git commit -m "main change"
git checkout dev-1
echo dev-1 >> base
git add .
git commit -m "dev-1 change"
git checkout dev-2
echo dev-2 >> base
git add .
git commit -m "dev-2 change"
git checkout main
echo main >> base
git add .
git commit -m "main change"
git merge dev-1 dev-2
file .git/MERGE_HEAD

which output:

.git/MERGE_HEAD: cannot open `.git/MERGE_HEAD' (No such file or directory)

Re: Question about pre-merge and git merge octopus strategy

[Elijah Newren]

On Wed, May 11, 2022 at 4:21 AM ZheNing Hu <adlternative@gmail.com> wrote:
>
> Elijah Newren <newren@gmail.com> 于2022年5月10日周二 15:07写道：
> >
> > On Sun, May 8, 2022 at 7:44 AM ZheNing Hu <adlternative@gmail.com> wrote:
[...]
> > > Thanks for clarifying. As I reply to Christian, when I just use "git
> > > merge A B C" happily,
> > > and there is a conflict, so I try "git merge --abort" as usual, but it
> > > can not work... git tell me:
> > >
> > > fatal: There is no merge to abort (MERGE_HEAD missing).
> >
> > Sounds like a bug to me; .git/MERGE_HEAD should be written.  That file
> > is created for me when I set up a simple octopus merge that has
> > conflicts.  Do you have a set of steps others can use to reproduce the
> > problem you are seeing?
>
> Let me minimally reproduce this question (with git 2.33.0):
>
> #!/bin/bash
>
> rm -rf test-repo
> git init test-repo
> cd test-repo
> git branch -m main
> echo base > base
> git add .
> git commit -m "base"
> git branch -c dev-1
> git branch -c dev-2
> echo main > main
> git add .
> git commit -m "main change"
> git checkout dev-1
> echo dev-1 >> base
> git add .
> git commit -m "dev-1 change"
> git checkout dev-2
> echo dev-2 >> base
> git add .
> git commit -m "dev-2 change"
> git checkout main
> echo main >> base
> git add .
> git commit -m "main change"
> git merge dev-1 dev-2
> file .git/MERGE_HEAD
>
> which output:
>
> .git/MERGE_HEAD: cannot open `.git/MERGE_HEAD' (No such file or directory)

Ah, thanks.  With this testcase, the output is:

    $ git merge dev-1 dev-2
    Trying simple merge with dev-1
    Simple merge did not work, trying automatic merge.
    Auto-merging base
    ERROR: content conflict in base
    fatal: merge program failed
    Automated merge did not work.
    Should not be doing an octopus.
    Merge with strategy octopus failed.

Also, if we check `git status`:

    $ git status
    On branch main
    Unmerged paths:
      (use "git restore --staged <file>..." to unstage)
      (use "git add <file>..." to mark resolution)
    both modified:   base

    no changes added to commit (use "git add" and/or "git commit -a")

And in git-merge-octopus.sh we see:

    case "$OCTOPUS_FAILURE" in
    1)
    # We allow only last one to have a hand-resolvable
    # conflicts.  Last round failed and we still had
    # a head to merge.
    gettextln "Automated merge did not work."
    gettextln "Should not be doing an octopus."
    exit 2
    esac

and in builtin/merge.c, we see:

    /*
     * The backend exits with 1 when conflicts are
     * left to be resolved, with 2 when it does not
     * handle the given merge at all.
     */

Which means git-merge-octopus.sh is claiming it can't handle this type
of merge, and some other merge strategy should be tried, and
implicitly that it didn't leave any conflicts to be resolved because
it can't handle this merge.  But it clearly decides to leave the
modifications it made to the index and working tree around, which just
seems wrong to me.  If it doesn't handle the merge and other merge
strategies (if available) should be tried, it should leave things the
way it found them.  So, perhaps a patch like the below would do that;
can you give it a try to see if it solves your cases (it works for
your simplified testcase, but I'm curious about your more involved
cases and if it works for them too)?


----- >8 -----

diff --git a/git-merge-octopus.sh b/git-merge-octopus.sh
index 7d19d37951..6ddbdc8f56 100755
--- a/git-merge-octopus.sh
+++ b/git-merge-octopus.sh
@@ -47,6 +47,13 @@ then
     git diff-index --cached --name-only HEAD -- | sed -e 's/^/    /'
     exit 2
 fi
+
+# If octopus is inapplicable, make sure we undo any changes we made first
+cannot_octopus() {
+       git reset --merge
+       exit 2
+}
+
 MRC=$(git rev-parse --verify -q $head)
 MRT=$(git write-tree)
 NON_FF_MERGE=0
@@ -60,7 +67,7 @@ do
                # a head to merge.
                gettextln "Automated merge did not work."
                gettextln "Should not be doing an octopus."
-               exit 2
+               cannot_octopus
        esac

        eval pretty_name=\${GITHEAD_$SHA1:-$SHA1}
@@ -95,7 +102,8 @@ do
        NON_FF_MERGE=1

        eval_gettextln "Trying simple merge with \$pretty_name"
-       git read-tree -u -m --aggressive  $common $MRT $SHA1 || exit 2
+       git read-tree -u -m --aggressive  $common $MRT $SHA1 || cannot_octopus
+
        next=$(git write-tree 2>/dev/null)
        if test $? -ne 0
        then

Re: Question about pre-merge and git merge octopus strategy

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>     Merge with strategy octopus failed.
>
> Also, if we check `git status`:
>
>     $ git status
>     On branch main
>     Unmerged paths:
>       (use "git restore --staged <file>..." to unstage)
>       (use "git add <file>..." to mark resolution)
>     both modified:   base
>
>     no changes added to commit (use "git add" and/or "git commit -a")
>
> And in git-merge-octopus.sh we see:
>
>     case "$OCTOPUS_FAILURE" in
>     1)
>     # We allow only last one to have a hand-resolvable
>     # conflicts.  Last round failed and we still had
>     # a head to merge.
>     gettextln "Automated merge did not work."
>     gettextln "Should not be doing an octopus."
>     exit 2
>     esac
>
> and in builtin/merge.c, we see:
>
>     /*
>      * The backend exits with 1 when conflicts are
>      * left to be resolved, with 2 when it does not
>      * handle the given merge at all.
>      */
>
> Which means git-merge-octopus.sh is claiming it can't handle this type
> of merge, and some other merge strategy should be tried, and
> implicitly that it didn't leave any conflicts to be resolved because
> it can't handle this merge.

Correct.  Near the beginning of the loop you found the above
comment, there is this code:

	if (use_strategies_nr == 1 ||
	    /*
	     * Stash away the local changes so that we can try more than one.
	     */
	    save_state(&stash))
		oidclr(&stash);

	for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {
		int ret, cnt;
		if (i) {
			printf(_("Rewinding the tree to pristine...\n"));
			restore_state(&head_commit->object.oid, &stash);
		}

but that save-then-restore triggers ONLY when there are multiple
strategies to try.  Unfortunately, octopus has no friends to fall
back on, so we do not do the save-restore dance on the calling side.

> But it clearly decides to leave the
> modifications it made to the index and working tree around, which just
> seems wrong to me.

If merge-recursive or merge-resolve is asked to merge a single
commit to the current branch without any other strategies to use as
a fallback, they leave the working tree and index into a state where
the end-user can conclude the conflict resolution and commit the
result.  In spirit, we are in the same situation, aren't we?

The user, if they want to proceed against octopus's opinion, would
resolve the current conflict, read-tree -m the next one, ..., to
conclude and commit the result. 

So I am not sure if it is a good idea to unconditionally "reset --merge"
in this situation.

> +# If octopus is inapplicable, make sure we undo any changes we made first
> +cannot_octopus() {
> +       git reset --merge
> +       exit 2
> +}

Re: Question about pre-merge and git merge octopus strategy

[Elijah Newren]

On Thu, May 12, 2022 at 8:39 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> >     Merge with strategy octopus failed.
> >
> > Also, if we check `git status`:
> >
> >     $ git status
> >     On branch main
> >     Unmerged paths:
> >       (use "git restore --staged <file>..." to unstage)
> >       (use "git add <file>..." to mark resolution)
> >     both modified:   base
> >
> >     no changes added to commit (use "git add" and/or "git commit -a")
> >
> > And in git-merge-octopus.sh we see:
> >
> >     case "$OCTOPUS_FAILURE" in
> >     1)
> >     # We allow only last one to have a hand-resolvable
> >     # conflicts.  Last round failed and we still had
> >     # a head to merge.
> >     gettextln "Automated merge did not work."
> >     gettextln "Should not be doing an octopus."
> >     exit 2
> >     esac
> >
> > and in builtin/merge.c, we see:
> >
> >     /*
> >      * The backend exits with 1 when conflicts are
> >      * left to be resolved, with 2 when it does not
> >      * handle the given merge at all.
> >      */
> >
> > Which means git-merge-octopus.sh is claiming it can't handle this type
> > of merge, and some other merge strategy should be tried, and
> > implicitly that it didn't leave any conflicts to be resolved because
> > it can't handle this merge.
>
> Correct.  Near the beginning of the loop you found the above
> comment, there is this code:
>
>         if (use_strategies_nr == 1 ||
>             /*
>              * Stash away the local changes so that we can try more than one.
>              */
>             save_state(&stash))
>                 oidclr(&stash);
>
>         for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {
>                 int ret, cnt;
>                 if (i) {
>                         printf(_("Rewinding the tree to pristine...\n"));
>                         restore_state(&head_commit->object.oid, &stash);
>                 }

Side-comment, which becomes important below: The save/restore code in
builtin/merge.c appears to be broken to me.  As noted in the code
above, stash will be set to null_oid() if save_state() returns
non-zero (which happens when "stash create" has no output, which
happens if there is _initially_ no state to save, i.e. if there are no
local changes before the merge started).  restore_state() is a no-op
whenever stash is the null_oid, meaning in that case it won't actually
rewind the tree to a pristine state to undo the changes of the
previous merge attempt.  So, if:

* The user had no local changes before starting the merge
* Multiple merge strategies are applicable
* The first merge strategy makes index/working-tree changes, but
returns with exit status 2

Then the restore_state() called before the second merge strategy will
do nothing, and the second merge strategy will be working on an index
and working tree with garbage leftover from the first merge strategy.
While this may have never been triggered (in what case do we have
multiple merge strategies that all return an exit status of 2?), I
suspect we want to fix this problem with something like this:

diff --git a/builtin/merge.c b/builtin/merge.c
index f178f5a3ee..7f3650fb09 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -378,11 +378,11 @@ static void restore_state(const struct object_id *head,
        struct strbuf sb = STRBUF_INIT;
        const char *args[] = { "stash", "apply", NULL, NULL };

+       reset_hard(head, 1);
+
        if (is_null_oid(stash))
                return;

-       reset_hard(head, 1);
-
        args[2] = oid_to_hex(stash);

        /*

> but that save-then-restore triggers ONLY when there are multiple
> strategies to try.  Unfortunately, octopus has no friends to fall
> back on, so we do not do the save-restore dance on the calling side.

If octopus did have a friend which also failed in the same way, the
restore_state() that you highlighted would only trigger for the first
such strategy and not the second.  However, restore_state() would
still be called after the second strategy, it would just come from a
later section of the code, highlighted here:

    /*
     * Pick the result from the best strategy and have the user fix
     * it up.
     */
    if (!best_strategy) {
        restore_state(&head_commit->object.oid, &stash);
        if (use_strategies_nr > 1)
            fprintf(stderr,
                    _("No merge strategy handled the merge.\n"));
        else
            fprintf(stderr, _("Merge with strategy %s failed.\n"),
                    use_strategies[0]->name);

Interestingly, the restore_state() call here does trigger even when we
only have octopus, but in that case it's a no-op because stash will
always be the null_oid because of the special case "use_strategies_nr
== 1" in the code you highlighted.

> > But it clearly decides to leave the
> > modifications it made to the index and working tree around, which just
> > seems wrong to me.
>
> If merge-recursive or merge-resolve is asked to merge a single
> commit to the current branch without any other strategies to use as
> a fallback, they leave the working tree and index into a state where
> the end-user can conclude the conflict resolution and commit the
> result.  In spirit, we are in the same situation, aren't we?

I don't think it's quite the same.  Those strategies return an exit
status of 1 on conflicts; if they returned a 2 (that is, if they all
returned a 2), meaning "I don't handle this", then in spirit we'd be
in the same situation.  If they all returned a 2, then best_strategy
would remain NULL and the code would do the restore_state() I
highlighted above in the (!best_stratgegy) block.  I believe the
intent of that restore_state() call was meant to make the working tree
and index be clean afterward (even if that's not its effect as I
mentioned in my side-note about its brokenness), and thus there would
be no conflict resolution for a user to perform.

However, some alternatives...

Perhaps you are arguing that git-merge-octopus.sh should return a 1
here instead of a 2, i.e. octopus handled the merge as far as it
could, but there are conflicts for the user to address?

Or are you saying that if all merge strategies return a 2, we just
treat the last one as good enough and consider the merge to be in
progress?  If that's your intent, we should probably remove the
restore_state() call in the "!best_strategy" block, and add a call to
write_merge_state(remoteheads) so that .git/MERGE_HEAD and friends get
written; something like this:

diff --git a/builtin/merge.c b/builtin/merge.c
index f178f5a3ee..397eb9c228 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1721,14 +1721,13 @@ int cmd_merge(int argc, const char **argv,
const char *prefix)
         * it up.
         */
        if (!best_strategy) {
-               restore_state(&head_commit->object.oid, &stash);
+               write_merge_state(remoteheads);
                if (use_strategies_nr > 1)
                        fprintf(stderr,
                                _("No merge strategy handled the merge.\n"));
                else
                        fprintf(stderr, _("Merge with strategy %s failed.\n"),
                                use_strategies[0]->name);
-               apply_autostash(git_path_merge_autostash(the_repository));
                ret = 2;
                goto done;
        } else if (best_strategy == wt_strategy)


> The user, if they want to proceed against octopus's opinion, would
> resolve the current conflict, read-tree -m the next one, ..., to
> conclude and commit the result.

That may be reasonable, but are there a few usability questions here?
In particular, would users know which trees have been merged and which
remain that they need to call "read-tree -m" on?  Would they even know
that repeated invocations of "read-tree -m" are in order?  Also, do
they just memorize "read-tree -m", or would they perhaps expect "git
merge --continue" to invoke it for them?

> So I am not sure if it is a good idea to unconditionally "reset --merge"
> in this situation.

Yeah, I think it should be handled by builtin/merge.c given how it's
apparently meant to be handling saving and restoring state.

Re: Question about pre-merge and git merge octopus strategy

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> If merge-recursive or merge-resolve is asked to merge a single
>> commit to the current branch without any other strategies to use as
>> a fallback, they leave the working tree and index into a state where
>> the end-user can conclude the conflict resolution and commit the
>> result.  In spirit, we are in the same situation, aren't we?
>
> I don't think it's quite the same....
> ...
> Or are you saying that if all merge strategies return a 2, we just
> treat the last one as good enough and consider the merge to be in
> progress?

No, I was just confusing the difference between the special return
value 2 and a normal failure value 1.

I have a feeling that it would be nice if we can restore to pristine
on the calling "git merge" side, rather than forcing individual
strategy backends to implement the restore-to-pristine correctly,
but in any case, as you said, we should behave as if a merge
strategy backend never run after it failed with exit value 2 by
restoring to pristine state.

Re: Question about pre-merge and git merge octopus strategy

[ZheNing Hu]

Elijah Newren <newren@gmail.com> 于2022年5月13日周五 13:16写道：
>
> On Thu, May 12, 2022 at 8:39 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Elijah Newren <newren@gmail.com> writes:
> >
> > >     Merge with strategy octopus failed.
> > >
> > > Also, if we check `git status`:
> > >
> > >     $ git status
> > >     On branch main
> > >     Unmerged paths:
> > >       (use "git restore --staged <file>..." to unstage)
> > >       (use "git add <file>..." to mark resolution)
> > >     both modified:   base
> > >
> > >     no changes added to commit (use "git add" and/or "git commit -a")
> > >
> > > And in git-merge-octopus.sh we see:
> > >
> > >     case "$OCTOPUS_FAILURE" in
> > >     1)
> > >     # We allow only last one to have a hand-resolvable
> > >     # conflicts.  Last round failed and we still had
> > >     # a head to merge.
> > >     gettextln "Automated merge did not work."
> > >     gettextln "Should not be doing an octopus."
> > >     exit 2
> > >     esac
> > >
> > > and in builtin/merge.c, we see:
> > >
> > >     /*
> > >      * The backend exits with 1 when conflicts are
> > >      * left to be resolved, with 2 when it does not
> > >      * handle the given merge at all.
> > >      */
> > >
> > > Which means git-merge-octopus.sh is claiming it can't handle this type
> > > of merge, and some other merge strategy should be tried, and
> > > implicitly that it didn't leave any conflicts to be resolved because
> > > it can't handle this merge.
> >
> > Correct.  Near the beginning of the loop you found the above
> > comment, there is this code:
> >
> >         if (use_strategies_nr == 1 ||
> >             /*
> >              * Stash away the local changes so that we can try more than one.
> >              */
> >             save_state(&stash))
> >                 oidclr(&stash);
> >
> >         for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {
> >                 int ret, cnt;
> >                 if (i) {
> >                         printf(_("Rewinding the tree to pristine...\n"));
> >                         restore_state(&head_commit->object.oid, &stash);
> >                 }
>
> Side-comment, which becomes important below: The save/restore code in
> builtin/merge.c appears to be broken to me.  As noted in the code
> above, stash will be set to null_oid() if save_state() returns
> non-zero (which happens when "stash create" has no output, which
> happens if there is _initially_ no state to save, i.e. if there are no
> local changes before the merge started).  restore_state() is a no-op
> whenever stash is the null_oid, meaning in that case it won't actually
> rewind the tree to a pristine state to undo the changes of the
> previous merge attempt.  So, if:
>
> * The user had no local changes before starting the merge
> * Multiple merge strategies are applicable
> * The first merge strategy makes index/working-tree changes, but
> returns with exit status 2
>
> Then the restore_state() called before the second merge strategy will
> do nothing, and the second merge strategy will be working on an index
> and working tree with garbage leftover from the first merge strategy.
> While this may have never been triggered (in what case do we have
> multiple merge strategies that all return an exit status of 2?), I
> suspect we want to fix this problem with something like this:
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index f178f5a3ee..7f3650fb09 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -378,11 +378,11 @@ static void restore_state(const struct object_id *head,
>         struct strbuf sb = STRBUF_INIT;
>         const char *args[] = { "stash", "apply", NULL, NULL };
>
> +       reset_hard(head, 1);
> +
>         if (is_null_oid(stash))
>                 return;
>
> -       reset_hard(head, 1);
> -
>         args[2] = oid_to_hex(stash);
>
>         /*
>

It looks like this code can go back to the old state now?

I think if we can handle it as special case when:

1. Use octopus merge without other strategies (no friend).
2. Merge failed, so we don't have the best strategy.

Then we force restore the original state?

Re: Question about pre-merge and git merge octopus strategy

[Elijah Newren]

On Thu, May 19, 2022 at 6:15 AM ZheNing Hu <adlternative@gmail.com> wrote:
>
> Elijah Newren <newren@gmail.com> 于2022年5月13日周五 13:16写道：
> >
> > On Thu, May 12, 2022 at 8:39 AM Junio C Hamano <gitster@pobox.com> wrote:
> > >
> > > Elijah Newren <newren@gmail.com> writes:
> > >
> > > >     Merge with strategy octopus failed.
> > > >
> > > > Also, if we check `git status`:
> > > >
> > > >     $ git status
> > > >     On branch main
> > > >     Unmerged paths:
> > > >       (use "git restore --staged <file>..." to unstage)
> > > >       (use "git add <file>..." to mark resolution)
> > > >     both modified:   base
> > > >
> > > >     no changes added to commit (use "git add" and/or "git commit -a")
> > > >
> > > > And in git-merge-octopus.sh we see:
> > > >
> > > >     case "$OCTOPUS_FAILURE" in
> > > >     1)
> > > >     # We allow only last one to have a hand-resolvable
> > > >     # conflicts.  Last round failed and we still had
> > > >     # a head to merge.
> > > >     gettextln "Automated merge did not work."
> > > >     gettextln "Should not be doing an octopus."
> > > >     exit 2
> > > >     esac
> > > >
> > > > and in builtin/merge.c, we see:
> > > >
> > > >     /*
> > > >      * The backend exits with 1 when conflicts are
> > > >      * left to be resolved, with 2 when it does not
> > > >      * handle the given merge at all.
> > > >      */
> > > >
> > > > Which means git-merge-octopus.sh is claiming it can't handle this type
> > > > of merge, and some other merge strategy should be tried, and
> > > > implicitly that it didn't leave any conflicts to be resolved because
> > > > it can't handle this merge.
> > >
> > > Correct.  Near the beginning of the loop you found the above
> > > comment, there is this code:
> > >
> > >         if (use_strategies_nr == 1 ||
> > >             /*
> > >              * Stash away the local changes so that we can try more than one.
> > >              */
> > >             save_state(&stash))
> > >                 oidclr(&stash);
> > >
> > >         for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {
> > >                 int ret, cnt;
> > >                 if (i) {
> > >                         printf(_("Rewinding the tree to pristine...\n"));
> > >                         restore_state(&head_commit->object.oid, &stash);
> > >                 }
> >
> > Side-comment, which becomes important below: The save/restore code in
> > builtin/merge.c appears to be broken to me.  As noted in the code
> > above, stash will be set to null_oid() if save_state() returns
> > non-zero (which happens when "stash create" has no output, which
> > happens if there is _initially_ no state to save, i.e. if there are no
> > local changes before the merge started).  restore_state() is a no-op
> > whenever stash is the null_oid, meaning in that case it won't actually
> > rewind the tree to a pristine state to undo the changes of the
> > previous merge attempt.  So, if:
> >
> > * The user had no local changes before starting the merge
> > * Multiple merge strategies are applicable
> > * The first merge strategy makes index/working-tree changes, but
> > returns with exit status 2
> >
> > Then the restore_state() called before the second merge strategy will
> > do nothing, and the second merge strategy will be working on an index
> > and working tree with garbage leftover from the first merge strategy.
> > While this may have never been triggered (in what case do we have
> > multiple merge strategies that all return an exit status of 2?), I
> > suspect we want to fix this problem with something like this:
> >
> > diff --git a/builtin/merge.c b/builtin/merge.c
> > index f178f5a3ee..7f3650fb09 100644
> > --- a/builtin/merge.c
> > +++ b/builtin/merge.c
> > @@ -378,11 +378,11 @@ static void restore_state(const struct object_id *head,
> >         struct strbuf sb = STRBUF_INIT;
> >         const char *args[] = { "stash", "apply", NULL, NULL };
> >
> > +       reset_hard(head, 1);
> > +
> >         if (is_null_oid(stash))
> >                 return;
> >
> > -       reset_hard(head, 1);
> > -
> >         args[2] = oid_to_hex(stash);
> >
> >         /*
> >
>
> It looks like this code can go back to the old state now?
>
> I think if we can handle it as special case when:
>
> 1. Use octopus merge without other strategies (no friend).
> 2. Merge failed, so we don't have the best strategy.
>
> Then we force restore the original state?

Yeah, I've got some patches ready; just didn't submit yet as I was
hoping to get time to update the in-core merge series.
https://github.com/gitgitgadget/git/pull/1231