git merge (resolve) _is_ stupid

git merge (resolve) _is_ stupid

[Junio C Hamano]

While dealing with Jakub's gitweb changes, I found that merge we
use _is_ very stupid.  Perhaps it is not a surprise...

My "next" was at 688a750, and "master" was at b63fafd.  I had
already merged gitweb changes from Luben to "master" at this
point, and "next" was in sync with "master" with respect to
gitweb/, i.e. this gave empty:

	$ git diff master next gitweb/

Jakub's gitweb changes were stored in jn/web branch which
contained 16 patches on top of "master".

However, pulling "jn/web" (594e212) into "next" using resolve
strategy produced interesting conflicts.  Essentially, it said
that some changes done by Luben (which has already merged to
"master" and precedes what Jakub did in "jn/web") conflict with
what "jn/web" does, which is quite bogus.

This merge has three merge-bases, but

	$ git log master..next -- gitweb/

shows only two merges, and that is not so surprising given that
at this point gitweb/ files are identical between master and
next.

More interestingly, if I merge "master" to "next" first and then
merge "jn/web" on top of the result, the resolve strategy does
the right thing.  This is understandable -- by merging "master"
into "next", merge base between "next" and "jn/web" becomes
"master" and nothing else.

Using recursive strategy resolved this merge correctly, taking
gitweb/ files from "jn/web" branch.

By the way, the "recur" strategy in "next" produces the correct
result, but it produces a funny error in the middle (that is why
Johannes is CC'ed).

	error: Could not read 0100000000000000000000000000000000000000

: gitster; git merge -s recur 'test merge' next jn/web
Merging next with 594e212bc849039a204deef1d16c2eddcc451532
Merging:
688a75071490101dbc660e3304aafb7a13e28807 Merge branch '__/setup-n-mv' into next
594e212bc849039a204deef1d16c2eddcc451532 gitweb: Ref refactoring - use git_get_referencing for marking tagged/head commits
found 3 common ancestor(s):
7061cf0f205e86613c3a3306fdfedf2a5dcc8a65 Merge branch 'lt/setup' into __/setup-n-mv
acb0f6f33760b43c1fc9617a45346ab3738f021a gitweb.cgi: git_blame2: slight optimization reading the blame lines
2d023581c9a0ae5efdebfd0084d54d09669a25d5 Set datarootdir in config.mak.in
  Merging:
  7061cf0f205e86613c3a3306fdfedf2a5dcc8a65 Merge branch 'lt/setup' into __/setup-n-mv
  acb0f6f33760b43c1fc9617a45346ab3738f021a gitweb.cgi: git_blame2: slight optimization reading the blame lines
  found 1 common ancestor(s):
  634331061599a82968daddae2d2c0896b6137d4c gitweb.cgi: Centralize printing of the page path
  Auto-merging gitweb/gitweb.cgi
  Merging:
  virtual merged tree
  2d023581c9a0ae5efdebfd0084d54d09669a25d5 Set datarootdir in config.mak.in
error: Could not read 0100000000000000000000000000000000000000
  found 1 common ancestor(s):
  7b8cf0cf2973cc8df3bdd36b9b36542b1f04d70a Rename man1 and man7 variables to man1dir and man7dir
  Auto-merging .gitignore
  Auto-merging INSTALL
  Auto-merging Makefile
Auto-merging Makefile
Merge made by recur.
 Documentation/git-tar-tree.txt |    5 
 Makefile                       |    3 
 git.c                          |   76 ++---
 gitweb/gitweb.cgi              |  609 +++++++++++++++++-----------------------
 setup.c                        |    4 
 5 files changed, 300 insertions(+), 397 deletions(-)

Re: git merge (resolve) _is_ stupid

[Johannes Schindelin]

Hi,

On Mon, 31 Jul 2006, Junio C Hamano wrote:

> By the way, the "recur" strategy in "next" produces the correct
> result, but it produces a funny error in the middle (that is why
> Johannes is CC'ed).
> 
> 	error: Could not read 0100000000000000000000000000000000000000

I get "0000000100..." ;-) (surprixse her wi1h a big-endian ;-)

The culprit is the call to parse_commit() in merge_bases(). How about 
this?

-- 8< --
[PATCH] merge-recur: virtual commits shall never be parsed

It would not make sense to parse a virtual commit, therefore set the
"parsed" flag to 1.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 merge-recursive.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 10bce70..74a329f 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -43,6 +43,8 @@ static struct commit *make_virtual_commi
 	commit->tree = tree;
 	commit->util = (void*)comment;
 	*(int*)commit->object.sha1 = virtual_id++;
+	/* avoid warnings */
+	commit->object.parsed = 1;
 	return commit;
 }
 
-- 
1.4.2.rc2.gfd00-dirty

Re: git merge (resolve) _is_ stupid

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The culprit is the call to parse_commit() in merge_bases(). How about 
> this?

Do you mean merge_bases() in commit.c which is called by
get_merge_bases()?  If so the patch feels like papering over a
more grave bug -- the result from make_virtual_commit does not
seem to have any proper parent information, so how is merge_bases()
expected to return anything sensible?

I am confused, but going to bed first.

Re: git merge (resolve) _is_ stupid

[Johannes Schindelin]

Hi,

On Mon, 31 Jul 2006, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > The culprit is the call to parse_commit() in merge_bases(). How about 
> > this?
> 
> Do you mean merge_bases() in commit.c which is called by
> get_merge_bases()?

Yes, the same.

> If so the patch feels like papering over a more grave bug -- the result 
> from make_virtual_commit does not seem to have any proper parent 
> information, so how is merge_bases() expected to return anything 
> sensible?

It is not a grave bug, but very much by design: remember, a recursive 
merge means that if you have more than one merge base, then the merge 
bases are merged first. And the result is -- tada -- a virtual commit.

It does have (virtual) parents, but we do _not_ want to traverse them in 
merge_bases().

However, they have (virtual) children, and _these_ relationships are 
important: in your case (I guess) that the merge_bases() finds the 
virtual commit, since (with the virtual history) the other commit 
is just a fast-forward.

So, a virtual commit is something like a merge base combining two or more 
merge bases.

> I am confused, but going to bed first.

I know the feeling ;-)

Ciao,
Dscho

Re: git merge (resolve) _is_ stupid

[Luben Tuikov]

I had seen something similar:

Merging linux-scsi into my own branches whereby both had a
particular patch (03aba2f79594ca94d159c8bab454de9bcc385b76),
git-merge complained that a resolve is needed.

After manually "resolving" the result was identical to
either branch (linux-scsi and my own), so in fact the
diff was 0.

I found this strange, but due to lack of time couldn't
pursue it further.

    Luben