[PATCH 1/1] Make sure the empty tree exists when needed in merge-recursive.

[PATCH 1/1] Make sure the empty tree exists when needed in merge-recursive.

[Shawn O. Pearce]

There are some baseless merge cases where git-merge-recursive will
try to compare one of the branches against the empty tree.  However
most projects won't have the empty tree object in their object database
as Git does not normally create empty tree objects.  If the empty tree
object is missing then the merge process will die, as it cannot load the
object from the database.  The error message may make the user think that
their database is corrupt when its actually not.

So instead we should just create the empty tree object whenever it is
needed.  If the object already exists as a loose object then no harm
done.  Otherwise that loose object will be pruned away later by either
git-prune or git-prune-packed.

Thanks goes to Junio for suggesting this fix.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 merge-recursive.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index cd2cc77..32e186c 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1238,7 +1238,7 @@ static int merge(struct commit *h1,
 
 		tree->object.parsed = 1;
 		tree->object.type = OBJ_TREE;
-		hash_sha1_file(NULL, 0, tree_type, tree->object.sha1);
+		write_sha1_file(NULL, 0, tree_type, tree->object.sha1);
 		merged_common_ancestors = make_virtual_commit(tree, "ancestor");
 	}
 
-- 

[PATCH 1/3] diff_tree_sha1(): avoid rereading trees if possible

[Johannes Schindelin]

If the tree has already been read, no need to read it into memory
again.

This also helps when this function is called on temporary trees;
these no longer have to be written to disk.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 tree-diff.c |   33 ++++++++++++++++++++++-----------
 1 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/tree-diff.c b/tree-diff.c
index 9d80dfb..54a6b44 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -195,23 +195,34 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2, const char *base, stru
 	return 0;
 }
 
+static int get_tree_desc_from_sha1(const unsigned char *sha1,
+		struct tree_desc *t)
+{
+	struct object *o;
+
+	o = lookup_object(sha1);
+	if (o && o->type == OBJ_TREE && o->parsed) {
+		struct tree *tree = (struct tree *)o;
+		t->size = tree->size;
+		t->buf = xmalloc(t->size);
+		memcpy(t->buf, tree->buffer, t->size);
+	} else {
+		t->buf = read_object_with_reference(sha1,
+				tree_type, &t->size, NULL);
+		if (!t->buf)
+			die("unable to read source tree (%s)",
+					sha1_to_hex(sha1));
+	}
+}
+
 int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const char *base, struct diff_options *opt)
 {
-	void *tree1, *tree2;
 	struct tree_desc t1, t2;
 	int retval;
 
-	tree1 = read_object_with_reference(old, tree_type, &t1.size, NULL);
-	if (!tree1)
-		die("unable to read source tree (%s)", sha1_to_hex(old));
-	tree2 = read_object_with_reference(new, tree_type, &t2.size, NULL);
-	if (!tree2)
-		die("unable to read destination tree (%s)", sha1_to_hex(new));
-	t1.buf = tree1;
-	t2.buf = tree2;
+	get_tree_desc_from_sha1(old, &t1);
+	get_tree_desc_from_sha1(new, &t2);
 	retval = diff_tree(&t1, &t2, base, opt);
-	free(tree1);
-	free(tree2);
 	return retval;
 }
 
-- 
1.4.4.2.g0f32-dirty

Re: [PATCH 1/3] diff_tree_sha1(): avoid rereading trees if possible

[Junio C Hamano]

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

> If the tree has already been read, no need to read it into memory
> again.

Hmm...

> This also helps when this function is called on temporary trees;
> these no longer have to be written to disk.

To generate tree-diff, probably yes, but I am not sure that
allows you to use hash_sha1_file() everywhere in merge_recursive
instead of write_sha1_file(), if that is what you are getting
at.

> +static int get_tree_desc_from_sha1(const unsigned char *sha1,
> +		struct tree_desc *t)
> +{
> +	struct object *o;
> +
> +	o = lookup_object(sha1);
> +	if (o && o->type == OBJ_TREE && o->parsed) {
> +		struct tree *tree = (struct tree *)o;
> +		t->size = tree->size;
> +		t->buf = xmalloc(t->size);
> +		memcpy(t->buf, tree->buffer, t->size);
> +	} else {
> +		t->buf = read_object_with_reference(sha1,
> +				tree_type, &t->size, NULL);
> +		if (!t->buf)
> +			die("unable to read source tree (%s)",
> +					sha1_to_hex(sha1));
> +	}
> +}

Are you absolutely sure that all users of "struct tree" retains
the tree->buffer for a parsed tree?  If nobody does
"free(tree->buffer)" without "tree->buffer = NULL", then the
situation is still salvageable (you need a bit more code above),
though.

I think this is overkill that only helps a very narrow "empty
tree" special case that [PATCH 2/3] addresses, and can be easily
and incorrectly abused.  We do not want people to expect that
reading many trees from different revisions as "struct tree"
objects and keeping all of them in memory would magically speed
up diff-tree, for example.

I'd prefer write_sha1_file() approach in Shawn's patch for its
simplicity at least for now.

I suspect gitlink/subproject people might want to modify in-core
representation of a tree to graft in subproject directory
somewhere in the superproject, just like the history traversal
code modifies in-core representation of a commit to simplify
parents.  Your approach might turn out to be the right thing to
for that application --- populate tree objects in the in-core
obj_hash[], muck with its entries and then have everybody else
go through get_tree_desc_from_sha1() interface to pretend as if
the superproject has everything contained in the subproject
tree.  I dunno.

Re: [PATCH 1/3] diff_tree_sha1(): avoid rereading trees if possible

[Johannes Schindelin]

Hi,

On Sat, 9 Dec 2006, Junio C Hamano wrote:

> I think this is overkill that only helps a very narrow "empty tree" 
> special case that [PATCH 2/3] addresses, and can be easily and 
> incorrectly abused.  We do not want people to expect that reading many 
> trees from different revisions as "struct tree" objects and keeping all 
> of them in memory would magically speed up diff-tree, for example.
> 
> I'd prefer write_sha1_file() approach in Shawn's patch for its 
> simplicity at least for now.

Okay, after thinking about it, I agree. merge-recursive is really the only 
user for such a diff. So, I do not think EMPTY_TREE would be useful.

Ciao,
Dscho

[PATCH 2/3] merge-recursive: make empty tree a known object

[Johannes Schindelin]

To use it in diff_tree_sha1(), a tree has to be hashed in the
global object collection. This actually moves the empty tree (if
it is needed) into the global object hash.

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

diff --git a/merge-recursive.c b/merge-recursive.c
index 6e13b8e..280f23c 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1220,9 +1220,10 @@ static int merge(struct commit *h1,
 		/* if there is no common ancestor, make an empty tree */
 		struct tree *tree = xcalloc(1, sizeof(struct tree));
 
+		hash_sha1_file(NULL, 0, tree_type, tree->object.sha1);
+		created_object(tree->object.sha1, &tree->object);
 		tree->object.parsed = 1;
 		tree->object.type = OBJ_TREE;
-		hash_sha1_file(NULL, 0, tree_type, tree->object.sha1);
 		merged_common_ancestors = make_virtual_commit(tree, "ancestor");
 	}
 
-- 
1.4.4.2.g0f32-dirty

Re: [PATCH 2/3] merge-recursive: make empty tree a known object

[Linus Torvalds]

On Sun, 10 Dec 2006, Johannes Schindelin wrote:
> 
> To use it in diff_tree_sha1(), a tree has to be hashed in the
> global object collection. This actually moves the empty tree (if
> it is needed) into the global object hash.

I think you should do this more generically.

If we create this kind of "default fake object" that we know about whether 
the object _really_ exists or not (and I agree it makes sense for the 
empty tree), we should probably use it for "diff_root_tree_sha1()" too, 
rather than the special case we have now.

In other words, right now we have that very special 
"diff_root_tree_sha1()" function, but if you create a generic fake "empty 
tree" SHA1 that git knows about implicitly, we could entirely replace it 
with just using

	diff_tree_sha1(EMPTY_TREE_SHA1, tree, base, opt);

instead, and get rid of that special case code (which is efficient, but we 
don't really _need_ the efficiency).

So you could make "read_sha1_file()" just have a special case for known 
objects at the end. If the pack entry fails, the loose file case fails, 
then rather than returning NULL at the end, you could have a list of known 
fixed objects..

Hmm?

Re: [PATCH 2/3] merge-recursive: make empty tree a known object

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> So you could make "read_sha1_file()" just have a special case for known 
> objects at the end. If the pack entry fails, the loose file case fails, 
> then rather than returning NULL at the end, you could have a list of known 
> fixed objects..

That is fine by me.  We would benefit from an empty blob and an
empty tree.

Re: [PATCH 2/3] merge-recursive: make empty tree a known object

[Linus Torvalds]

On Sun, 10 Dec 2006, Junio C Hamano wrote:
> 
> That is fine by me.  We would benefit from an empty blob and an
> empty tree.

I was wondering if we ever had any special case where we wanted the empty 
blob, and couldn't come up with any. Unlike the "tree diff" case, a "blob 
diff" will not actually do a "real diff" with a non-existing object, it 
special-cases it (and, for performance reasons, I think it really should). 

And all the blob-diff routines generally want to be able to take explicit 
data anyway, not just diffing two SHA1's (since we often don't have a SHA1 
anyway - the working tree case).

So while I think it would make sense to have both the "empty tree" and the 
"empty blob" as special cases, off-hand I can't actually see where we'd 
ever use the empty blob SHA1.

In contrast, the empty tree clearly ends up being an interesting special 
case that actually gets used occasionally, ie here we had two independent 
uses for the same thing..

But maybe somebody can point to a case where we actually would want to 
internally have an easy representation of "empty blob".

Re: [PATCH 2/3] merge-recursive: make empty tree a known object

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Sun, 10 Dec 2006, Junio C Hamano wrote:
>> 
>> That is fine by me.  We would benefit from an empty blob and an
>> empty tree.
>
> I was wondering if we ever had any special case where we wanted the empty 
> blob, and couldn't come up with any.

After saying that I was thinking about the same.  Empty blob is
not all that useful (and let's not bring up the "empty commit"
in this discussion pretty please).

> In contrast, the empty tree clearly ends up being an interesting special 
> case that actually gets used occasionally, ie here we had two independent 
> uses for the same thing..

Two?

I sent out a patch with comment about an issue I worry about.

I do not think the empty-tree case matters in practice because
we do not allow an empty tree to be committed, but if we add
empty blob to the set of "well known objects", the backward
compatibility issue becomes real.

Re: [PATCH 2/3] merge-recursive: make empty tree a known object

[Linus Torvalds]

On Sun, 10 Dec 2006, Junio C Hamano wrote:

> > In contrast, the empty tree clearly ends up being an interesting special 
> > case that actually gets used occasionally, ie here we had two independent 
> > uses for the same thing..
> 
> Two?

merge-recursive and "diff against root"

Re: [PATCH 2/3] merge-recursive: make empty tree a known object

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Linus Torvalds <torvalds@osdl.org> writes:
>
>> So you could make "read_sha1_file()" just have a special case for known 
>> objects at the end. If the pack entry fails, the loose file case fails, 
>> then rather than returning NULL at the end, you could have a list of known 
>> fixed objects..
>
> That is fine by me.  We would benefit from an empty blob and an
> empty tree.

I think this might be fragile in the presense of older
implementation of git.

A new git may operate fine without having "well known" objects
in the repository, and would happily write result that points at
them; an older git would find the resulting repository corrupt.

We somehow need to force "well known" objects to be instantiated
in the object database when something else points at them.  I am
not absolutely sure if keeping has_sha1_file() unaware of this
magic is enough.

-- >8 --
Implement "well known" built-in objects.

Make read_sha1_file() aware of a handful "well known" objects
and use built-in copy when they are not found in the object
database.

Note that we do not hook this into has_sha1_file(), so it is
possible that has_sha1_file() says you do not have an empty tree
object in your repository but read_sha1_file() successfully
gives you an empty tree object back.

Otherwise write_sha1_file() would not create "well known"
objects in the repository, leaving the repository corrupt for
older implementations of git.

---

diff --git a/cache.h b/cache.h
index f2ec5c8..7ee40e8 100644
--- a/cache.h
+++ b/cache.h
@@ -241,6 +241,10 @@ int adjust_shared_perm(const char *path);
 int safe_create_leading_directories(char *path);
 char *enter_repo(char *path, int strict);
 
+/* Well known object names */
+extern unsigned const char EMPTY_TREE_NAME[];
+extern unsigned const char EMPTY_BLOB_NAME[];
+
 /* Read and unpack a sha1 file into memory, write memory to a sha1 file */
 extern int sha1_object_info(const unsigned char *, char *, unsigned long *);
 extern void * unpack_sha1_file(void *map, unsigned long mapsize, char *type, unsigned long *size);
diff --git a/merge-recursive.c b/merge-recursive.c
index 32e186c..1c56fd3 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1234,11 +1234,8 @@ static int merge(struct commit *h1,
 	merged_common_ancestors = pop_commit(&ca);
 	if (merged_common_ancestors == NULL) {
 		/* if there is no common ancestor, make an empty tree */
-		struct tree *tree = xcalloc(1, sizeof(struct tree));
-
-		tree->object.parsed = 1;
-		tree->object.type = OBJ_TREE;
-		write_sha1_file(NULL, 0, tree_type, tree->object.sha1);
+		struct tree *tree = lookup_tree(EMPTY_TREE_NAME);
+		parse_tree(tree);
 		merged_common_ancestors = make_virtual_commit(tree, "ancestor");
 	}
 
diff --git a/sha1_file.c b/sha1_file.c
index 63f416b..c6efe88 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1315,11 +1315,32 @@ static void *read_packed_sha1(const unsigned char *sha1, char *type, unsigned lo
 	return unpack_entry(&e, type, size);
 }
 
-void * read_sha1_file(const unsigned char *sha1, char *type, unsigned long *size)
+unsigned const char EMPTY_TREE_NAME[20] = {
+	0x4b, 0x82, 0x5d, 0xc6, 0x42, 0xcb, 0x6e, 0xb9,	0xa0, 0x60,
+	0xe5, 0x4b, 0xf8, 0xd6, 0x92, 0x88, 0xfb, 0xee, 0x49, 0x04,
+};
+
+unsigned const char EMPTY_BLOB_NAME[20] = {
+	0xe6, 0x9d, 0xe2, 0x9b, 0xb2, 0xd1, 0xd6, 0x43, 0x4b, 0x8b,
+	0x29, 0xae, 0x77, 0x5a, 0xd8, 0xc2, 0xe4, 0x8c, 0x53, 0x91,
+};
+
+static struct well_known_objects {
+	char *type;
+	unsigned const char *sha1;
+	unsigned long size;
+	const char *data;
+} well_known_objects[] = {
+	{ "tree", EMPTY_TREE_NAME, 0UL, "" },
+	{ "blob", EMPTY_BLOB_NAME, 0UL, "" },
+};
+
+void *read_sha1_file(const unsigned char *sha1, char *type, unsigned long *size)
 {
 	unsigned long mapsize;
 	void *map, *buf;
 	struct pack_entry e;
+	int i;
 
 	if (find_pack_entry(sha1, &e, NULL))
 		return read_packed_sha1(sha1, type, size);
@@ -1332,6 +1353,17 @@ void * read_sha1_file(const unsigned char *sha1, char *type, unsigned long *size
 	reprepare_packed_git();
 	if (find_pack_entry(sha1, &e, NULL))
 		return read_packed_sha1(sha1, type, size);
+
+	/* "well known" objects */
+	for (i = 0; i < ARRAY_SIZE(well_known_objects); i++) {
+		if (!hashcmp(well_known_objects[i].sha1, sha1)) {
+			*size = well_known_objects[i].size;
+			buf = xmalloc(*size);
+			strcpy(type, well_known_objects[i].type);
+			memcpy(buf, well_known_objects[i].data, *size);
+			return buf;
+		}
+	}
 	return NULL;
 }
 

Re: [PATCH 2/3] merge-recursive: make empty tree a known object

[Johannes Schindelin]

Hi,

On Sun, 10 Dec 2006, Junio C Hamano wrote:

> Implement "well known" built-in objects.

Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Ciao,

[PATCH 3/3] add test case for recursive merge

[Johannes Schindelin]

This test case is based on the bug report by Shawn Pearce.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 t/t6024-recursive-merge.sh |   68 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh
new file mode 100644
index 0000000..5f821fb
--- /dev/null
+++ b/t/t6024-recursive-merge.sh
@@ -0,0 +1,68 @@
+#!/bin/sh
+
+test_description='Test merge without common ancestors'
+. ./test-lib.sh
+
+# This scenario is based on a real-world repository of Shawn Pearce.
+
+# 1 - A - D - F
+#   \   X   /
+#     B   X
+#       X   \
+# 2 - C - E - G
+
+echo 1 > a1
+git add a1
+git commit -m 1 a1
+
+git checkout -b A master
+echo A > a1
+git commit -m A a1
+
+git checkout -b B master
+echo B > a1
+git commit -m B a1
+
+git checkout -b D A
+git-rev-parse B > .git/MERGE_HEAD
+echo D > a1
+git update-index a1
+git commit -m D
+
+git symbolic-ref HEAD refs/heads/other
+echo 2 > a1
+git commit -m 2 a1
+
+git checkout -b C
+echo C > a1
+git commit -m C a1
+
+git checkout -b E C
+git-rev-parse B > .git/MERGE_HEAD
+echo E > a1
+git update-index a1
+git commit -m E
+
+git checkout -b G E
+git-rev-parse A > .git/MERGE_HEAD
+echo G > a1
+git update-index a1
+git commit -m G
+
+git checkout -b F D
+git-rev-parse C > .git/MERGE_HEAD
+echo F > a1
+git update-index a1
+git commit -m F
+
+test_expect_failure "combined merge conflicts" "git merge -m final G"
+
+git ls-files --stage > out
+cat > expect << EOF
+100644 cf84443e49e1b366fac938711ddf4be2d4d1d9e9 2	a1
+100644 fd7923529855d0b274795ae3349c5e0438333979 3	a1
+EOF
+
+test_expect_success "virtual trees were processed" "diff -u expect out"
+
+test_done
-- 
1.4.4.2.g0f32-dirty

Re: [PATCH 3/3] add test case for recursive merge

[Johannes Schindelin]

Hi,

this test succeeds consistently on the machine where I tested it 
originally, but fails on another of my machines, but only when run without 
"-v". Very annoying. I will not have time to investigate until Monday, 
though.

Ciao,
Dscho

Re: [PATCH 3/3] add test case for recursive merge

[Junio C Hamano]

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

> this test succeeds consistently on the machine where I tested it 
> originally, but fails on another of my machines, but only when run without 
> "-v". Very annoying. I will not have time to investigate until Monday, 
> though.

There seem to be cases where stage #1 contains blob 'B' or 'A'
or nothing depending on something totally random.  Ring a bell?

Re: [PATCH 3/3] add test case for recursive merge

[Johannes Schindelin]

Hi,

On Sat, 9 Dec 2006, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > this test succeeds consistently on the machine where I tested it 
> > originally, but fails on another of my machines, but only when run without 
> > "-v". Very annoying. I will not have time to investigate until Monday, 
> > though.
> 
> There seem to be cases where stage #1 contains blob 'B' or 'A'
> or nothing depending on something totally random.  Ring a bell?

Not at all.

It seems that one merge is conflicting on "a1", and not storing _anything_ 
in the resulting (unclean) tree. So, there is no stage 1.

However, I also saw that something is left in stage 1 on the box where the 
problem arises. Tomorrow.

Ciao,
Dscho

[PATCH] t6024: fix timing problem

[Johannes Schindelin]

This script tests a complicated merge, where _all_ files conflict. In
these circumstances, the ordering of the commits -- which is affected
not by the timestamps in the commit message -- becomes a deciding factor
of the merge result.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---

	On Sat, 9 Dec 2006, Junio C Hamano wrote:

	> There seem to be cases where stage #1 contains blob 'B' or 'A'
	> or nothing depending on something totally random.  Ring a bell?

	This patch fixes it..

	Depending on the order, an add/add conflict _removes_ the file
	from the index, and as a consequence stage 1 is missing in the
	final merge (which still fails, because the return value of the
	previous merge is remembered).

	How about this: if there is an add/add conflict, we treat it as
	if there _was_ an empty file, and we let the shiny new xdl_merge()
	find the _true_ conflicts, _instead of_ removing the file from
	the index, adding both files with different "~blabla" markers
	appended to their file names to the working directory.

	Hmm?

 t/t6024-recursive-merge.sh |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh
index 5f821fb..9416c27 100644
--- a/t/t6024-recursive-merge.sh
+++ b/t/t6024-recursive-merge.sh
@@ -11,54 +11,56 @@ test_description='Test merge without common ancestors'
 #       X   \
 # 2 - C - E - G
 
+export GIT_COMMITTER_DATE="2006-12-12 23:28:00 +0100"
 echo 1 > a1
 git add a1
-git commit -m 1 a1
+GIT_AUTHOR_DATE="2006-12-12 23:00:00" git commit -m 1 a1
 
 git checkout -b A master
 echo A > a1
-git commit -m A a1
+GIT_AUTHOR_DATE="2006-12-12 23:00:01" git commit -m A a1
 
 git checkout -b B master
 echo B > a1
-git commit -m B a1
+GIT_AUTHOR_DATE="2006-12-12 23:00:02" git commit -m B a1
 
 git checkout -b D A
 git-rev-parse B > .git/MERGE_HEAD
 echo D > a1
 git update-index a1
-git commit -m D
+GIT_AUTHOR_DATE="2006-12-12 23:00:03" git commit -m D
 
 git symbolic-ref HEAD refs/heads/other
 echo 2 > a1
-git commit -m 2 a1
+GIT_AUTHOR_DATE="2006-12-12 23:00:04" git commit -m 2 a1
 
 git checkout -b C
 echo C > a1
-git commit -m C a1
+GIT_AUTHOR_DATE="2006-12-12 23:00:05" git commit -m C a1
 
 git checkout -b E C
 git-rev-parse B > .git/MERGE_HEAD
 echo E > a1
 git update-index a1
-git commit -m E
+GIT_AUTHOR_DATE="2006-12-12 23:00:06" git commit -m E
 
 git checkout -b G E
 git-rev-parse A > .git/MERGE_HEAD
 echo G > a1
 git update-index a1
-git commit -m G
+GIT_AUTHOR_DATE="2006-12-12 23:00:07" git commit -m G
 
 git checkout -b F D
 git-rev-parse C > .git/MERGE_HEAD
 echo F > a1
 git update-index a1
-git commit -m F
+GIT_AUTHOR_DATE="2006-12-12 23:00:08" git commit -m F
 
 test_expect_failure "combined merge conflicts" "git merge -m final G"
 
 git ls-files --stage > out
 cat > expect << EOF
+100644 f70f10e4db19068f79bc43844b49f3eece45c4e8 1	a1
 100644 cf84443e49e1b366fac938711ddf4be2d4d1d9e9 2	a1
 100644 fd7923529855d0b274795ae3349c5e0438333979 3	a1
 EOF
-- 
1.4.4.2.g473fa0-dirty

Re: [PATCH] t6024: fix timing problem

[Junio C Hamano]

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

> This script tests a complicated merge, where _all_ files conflict. In
> these circumstances, the ordering of the commits -- which is affected
> not by the timestamps in the commit message -- becomes a deciding factor
> of the merge result.

"not by the timestamps", or "by the timestamps"?  I am
confused...

Do you mean the commit timestamps affect which merge base commit
becomes ours and theirs during the computation of the virtual
merge base commit?  That certainly explains the problem.

> 	How about this: if there is an add/add conflict, we treat it as
> 	if there _was_ an empty file, and we let the shiny new xdl_merge()
> 	find the _true_ conflicts, _instead of_ removing the file from
> 	the index, adding both files with different "~blabla" markers
> 	appended to their file names to the working directory.

I was not thinking about this t6024 test failure problem but was
wondering about doing exactly that in merge-recursive to match
the "two file merge" magic we have in git-merge-one-file.sh ---
I guess great minds do think alike ;-).

Re: [PATCH] t6024: fix timing problem

[Johannes Schindelin]

hI,

On Tue, 12 Dec 2006, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > This script tests a complicated merge, where _all_ files conflict. In
> > these circumstances, the ordering of the commits -- which is affected
> > not by the timestamps in the commit message -- becomes a deciding factor
> > of the merge result.
> 
> "not by the timestamps", or "by the timestamps"?  I am confused...

I deleted the "only", but not the "not" in front of it. Should have read 
my mail before sending... Sorry.

> Do you mean the commit timestamps affect which merge base commit becomes 
> ours and theirs during the computation of the virtual merge base commit?  
> That certainly explains the problem.

It affects in which order the merge bases are merged. I remember that I 
made a case for the oldest merge base to go first. If two of them (or all 
three!) have the same timestamp, I _think_ they are ordered by SHA1...

Now, the problem here is that two of the merge bases have a common merge 
base, but the third has a completely different root. So, depending on 
which merge base goes first, the add/add conflict can remove the file from 
the index early, in which case the next merge does not find a stage 1.

> > 	How about this: if there is an add/add conflict, we treat it as
> > 	if there _was_ an empty file, and we let the shiny new xdl_merge()
> > 	find the _true_ conflicts, _instead of_ removing the file from
> > 	the index, adding both files with different "~blabla" markers
> > 	appended to their file names to the working directory.
> 
> I was not thinking about this t6024 test failure problem but was
> wondering about doing exactly that in merge-recursive to match
> the "two file merge" magic we have in git-merge-one-file.sh ---
> I guess great minds do think alike ;-).

*beams* Gee, thanks!

Ciao,
Dscho

[PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Johannes Schindelin]

Unify the handling for cases C (add/add) and D (modify/modify).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Tue, 12 Dec 2006, Junio C Hamano wrote:

	> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
	> 
	> > How about this: if there is an add/add conflict, we treat it 
	> > as if there _was_ an empty file, and we let the shiny new 
	> > xdl_merge() find the _true_ conflicts, _instead of_ removing 
	> > the file from the index, adding both files with different 
	> > "~blabla" markers appended to their file names to the working 
	> > directory.
	> 
	> I was not thinking about this t6024 test failure problem but was
	> wondering about doing exactly that in merge-recursive to match
	> the "two file merge" magic we have in git-merge-one-file.sh

	As can be seen with the test case, the result is more pleasing.

 merge-recursive.c          |   44 +++++++++++++++-----------------------------
 t/t6024-recursive-merge.sh |   12 +++++++++++-
 2 files changed, 26 insertions(+), 30 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 7d203a6..5bec599 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -610,6 +610,12 @@ static void fill_mm(const unsigned char *sha1, mmfile_t *mm)
 	unsigned long size;
 	char type[20];
 
+	if (!hashcmp(sha1, null_sha1)) {
+		mm->ptr = xstrdup("");
+		mm->size = 0;
+		return;
+	}
+
 	mm->ptr = read_sha1_file(sha1, type, &size);
 	if (!mm->ptr || strcmp(type, blob_type))
 		die("unable to read blob object %s", sha1_to_hex(sha1));
@@ -1045,38 +1051,17 @@ static int process_entry(const char *path, struct stage_data *entry,
 			output("Adding %s", path);
 			update_file(1, sha, mode, path);
 		}
-	} else if (!o_sha && a_sha && b_sha) {
-		/* Case C: Added in both (check for same permissions). */
-		if (sha_eq(a_sha, b_sha)) {
-			if (a_mode != b_mode) {
-				clean_merge = 0;
-				output("CONFLICT: File %s added identically in both branches, "
-				       "but permissions conflict %06o->%06o",
-				       path, a_mode, b_mode);
-				output("CONFLICT: adding with permission: %06o", a_mode);
-				update_file(0, a_sha, a_mode, path);
-			} else {
-				/* This case is handled by git-read-tree */
-				assert(0 && "This case must be handled by git-read-tree");
-			}
-		} else {
-			const char *new_path1, *new_path2;
-			clean_merge = 0;
-			new_path1 = unique_path(path, branch1);
-			new_path2 = unique_path(path, branch2);
-			output("CONFLICT (add/add): File %s added non-identically "
-			       "in both branches. Adding as %s and %s instead.",
-			       path, new_path1, new_path2);
-			remove_file(0, path, 0);
-			update_file(0, a_sha, a_mode, new_path1);
-			update_file(0, b_sha, b_mode, new_path2);
-		}
-
-	} else if (o_sha && a_sha && b_sha) {
+	} else if (a_sha && b_sha) {
+		/* Case C: Added in both (check for same permissions) and */
 		/* case D: Modified in both, but differently. */
+		const char *reason = "content";
 		struct merge_file_info mfi;
 		struct diff_filespec o, a, b;
 
+		if (!o_sha) {
+			reason = "add/add";
+			o_sha = (unsigned char *)null_sha1;
+		}
 		output("Auto-merging %s", path);
 		o.path = a.path = b.path = (char *)path;
 		hashcpy(o.sha1, o_sha);
@@ -1093,7 +1078,8 @@ static int process_entry(const char *path, struct stage_data *entry,
 			update_file(1, mfi.sha, mfi.mode, path);
 		else {
 			clean_merge = 0;
-			output("CONFLICT (content): Merge conflict in %s", path);
+			output("CONFLICT (%s): Merge conflict in %s",
+					reason, path);
 
 			if (index_only)
 				update_file(0, mfi.sha, mfi.mode, path);
diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh
index 9416c27..964010e 100644
--- a/t/t6024-recursive-merge.sh
+++ b/t/t6024-recursive-merge.sh
@@ -58,9 +58,19 @@ GIT_AUTHOR_DATE="2006-12-12 23:00:08" git commit -m F
 
 test_expect_failure "combined merge conflicts" "git merge -m final G"
 
+cat > expect << EOF
+<<<<<<< HEAD/a1
+F
+=======
+G
+>>>>>>> 26f86b677eb03d4d956dbe108b29cb77061c1e73/a1
+EOF
+
+test_expect_success "result contains a conflict" "diff -u expect a1"
+
 git ls-files --stage > out
 cat > expect << EOF
-100644 f70f10e4db19068f79bc43844b49f3eece45c4e8 1	a1
+100644 f16f906ab60483c100d1241dfc39868de9ec9fcb 1	a1
 100644 cf84443e49e1b366fac938711ddf4be2d4d1d9e9 2	a1
 100644 fd7923529855d0b274795ae3349c5e0438333979 3	a1
 EOF
-- 

Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Junio C Hamano]

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

> Unify the handling for cases C (add/add) and D (modify/modify).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> 	On Tue, 12 Dec 2006, Junio C Hamano wrote:
>
> 	> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 	> 
> 	> > How about this: if there is an add/add conflict, we treat it 
> 	> > as if there _was_ an empty file, and we let the shiny new 
> 	> > xdl_merge() find the _true_ conflicts, _instead of_ removing 
> 	> > the file from the index, adding both files with different 
> 	> > "~blabla" markers appended to their file names to the working 
> 	> > directory.
> 	> 
> 	> I was not thinking about this t6024 test failure problem but was
> 	> wondering about doing exactly that in merge-recursive to match
> 	> the "two file merge" magic we have in git-merge-one-file.sh
>
> 	As can be seen with the test case, the result is more pleasing.

This fixes the behaviour in "both branches add the path
differently" case.  Previously merge-recursive did not create
the working tree file, but now it does just like merge-resolve.

Although I would feel very happy about this change, Catalin
might want to be informed about potential interaction this
change might have with his commit 8d41555 in StGIT.

StGit repo & gitweb, was Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Johannes Schindelin]

Hi,

On Tue, 12 Dec 2006, Junio C Hamano wrote:

> Although I would feel very happy about this change, Catalin
> might want to be informed about potential interaction this
> change might have with his commit 8d41555 in StGIT.

Indeed. Catalin, do you have any suggestion how to proceed? Do you want to 
introduce a check if the file exists prior to re-generating it? Or do you 
need some version check?

BTW why is StGit not on kernel.org?

Not that it matters: repo.or.cz has a nice mirror. Pasky, how powerful is 
that machine? I am a happy user of the gitweb interface on that box...

Yet another question: On repo.or.cz, I searched for the "commit" "8d41555" 
(upper right side, very nice, although the search button is lacking, which 
is needed for some browsers like w3m). But I got _two_ results, the one I 
expected, and its child (probably because it contains the sha1 as part of 
the parent tag). Is this intended behaviour? After all, I search for the 
commit...

Ciao,
Dscho

Re: StGit repo & gitweb, was Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Jakub Narebski]

Johannes Schindelin wrote:

> Yet another question: On repo.or.cz, I searched for the "commit" "8d41555" 
> (upper right side, very nice, although the search button is lacking, which 
> is needed for some browsers like w3m). 

The problem is where to put it. The previous version didn't even had the
"select type" but used [undocumented] search operators 'author:',
'committer:' and 'pickaxe:'.

>                                        But I got _two_ results, the one I  
> expected, and its child (probably because it contains the sha1 as part of 
> the parent tag). Is this intended behaviour? After all, I search for the 
> commit...

Actually, "commit" is a bit of misnomer, as it searches commits contents
(commit body). You can go to given commit easier by hand editing URL,
editing "a=commit;h=8d41555".

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: StGit repo & gitweb, was Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Catalin Marinas]

On 13/12/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Tue, 12 Dec 2006, Junio C Hamano wrote:
>
> > Although I would feel very happy about this change, Catalin
> > might want to be informed about potential interaction this
> > change might have with his commit 8d41555 in StGIT.
>
> Indeed. Catalin, do you have any suggestion how to proceed? Do you want to
> introduce a check if the file exists prior to re-generating it? Or do you
> need some version check?

It currently checks whether the file exists and, if it doesn't, it is
re-generated. I think the patch is good idea.

> BTW why is StGit not on kernel.org?

Why would it be? Unless you know who to talk to for this :-), it's not
really a derivative of the Linux kernel.

> Not that it matters: repo.or.cz has a nice mirror. Pasky, how powerful is
> that machine? I am a happy user of the gitweb interface on that box...

Hopefully, when I get a bit of time, I'll try to give up the
http-hosted repository and use Pasky's one exclusively.

BTW, how can I notify people that only pull from the http repository
that it will no longer be updated (rather than them thinking the
development stopped)? One solution would be to create a file with a
meaningful name in the top dir and hope people will notice it.

-- 

Re: StGit repo & gitweb, was Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Robin Rosenberg]

onsdag 13 december 2006 23:09 skrev Catalin Marinas:
> BTW, how can I notify people that only pull from the http repository
> that it will no longer be updated (rather than them thinking the
> development stopped)? One solution would be to create a file with a
> meaningful name in the top dir and hope people will notice it.

If you also remove all other files, people will notice. 

Re: StGit repo & gitweb, was Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Johannes Schindelin]

Hi,

On Wed, 13 Dec 2006, Catalin Marinas wrote:

> On 13/12/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > On Tue, 12 Dec 2006, Junio C Hamano wrote:
> > 
> > > Although I would feel very happy about this change, Catalin
> > > might want to be informed about potential interaction this
> > > change might have with his commit 8d41555 in StGIT.
> > 
> > Indeed. Catalin, do you have any suggestion how to proceed? Do you want to
> > introduce a check if the file exists prior to re-generating it? Or do you
> > need some version check?
> 
> It currently checks whether the file exists and, if it doesn't, it is
> re-generated. I think the patch is good idea.

Thanks.

> > BTW why is StGit not on kernel.org?
> 
> Why would it be? Unless you know who to talk to for this :-), it's not 
> really a derivative of the Linux kernel.

There is plenty of stuff on kernel.org which is only related to the Linux 
kernel. And StGit (even if I did not try it yet), seems to be very 
valuable for working on the kernel.

> > Not that it matters: repo.or.cz has a nice mirror. Pasky, how powerful is
> > that machine? I am a happy user of the gitweb interface on that box...
> 
> Hopefully, when I get a bit of time, I'll try to give up the
> http-hosted repository and use Pasky's one exclusively.
> 
> BTW, how can I notify people that only pull from the http repository 
> that it will no longer be updated (rather than them thinking the 
> development stopped)? One solution would be to create a file with a 
> meaningful name in the top dir and hope people will notice it.

IMHO removing it altogether would be a good idea. The website should carry 
the news of the new repository site.

Ciao,
Dscho

Re: StGit repo & gitweb, was Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Jakub Narebski]

Johannes Schindelin wrote:

>> BTW, how can I notify people that only pull from the http repository 
>> that it will no longer be updated (rather than them thinking the 
>> development stopped)? One solution would be to create a file with a 
>> meaningful name in the top dir and hope people will notice it.
> 
> IMHO removing it altogether would be a good idea. The website should carry 
> the news of the new repository site.

By the way, would setting http-alternates help? I guess not, not without
having refs copied from the other place...
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: StGit repo & gitweb, was Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Petr Baudis]

On Wed, Dec 13, 2006 at 12:46:56PM CET, Johannes Schindelin wrote:
> Not that it matters: repo.or.cz has a nice mirror. Pasky, how powerful is 
> that machine? I am a happy user of the gitweb interface on that box...

See http://repo.or.cz/about.html - two-processor P3 800MHz with 2G RAM,
and it does quite some work besides doing repo.or.cz as well - there are
no load problems so far, though. The gitweb load in particular is pretty
low, though - I think it's about 350 requests per day; the periodical
mirroring is far more intensive, but the intervals for this could be
reduced greatly if that would become a problem.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
The meaning of Stonehenge in Traflamadorian, when viewed from above, is:
"Replacement part being rushed with all possible speed."

Re: StGit repo & gitweb, was Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Jakub Narebski]

Petr Baudis wrote:

> On Wed, Dec 13, 2006 at 12:46:56PM CET, Johannes Schindelin wrote:
>> Not that it matters: repo.or.cz has a nice mirror. Pasky, how powerful is 
>> that machine? I am a happy user of the gitweb interface on that box...
> 
> See http://repo.or.cz/about.html - two-processor P3 800MHz with 2G RAM,
> and it does quite some work besides doing repo.or.cz as well - there are
> no load problems so far, though. The gitweb load in particular is pretty
> low, though - I think it's about 350 requests per day; the periodical
> mirroring is far more intensive, but the intervals for this could be
> reduced greatly if that would become a problem.

Do you do "git repack -a -d" (and "git prune-packed"), or do you use
some kept packs?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Catalin Marinas]

On 13/12/06, Junio C Hamano <junkio@cox.net> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Unify the handling for cases C (add/add) and D (modify/modify).
> >
> >       On Tue, 12 Dec 2006, Junio C Hamano wrote:
> >
> >       > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >       >
> >       > > How about this: if there is an add/add conflict, we treat it
> >       > > as if there _was_ an empty file, and we let the shiny new
> >       > > xdl_merge() find the _true_ conflicts, _instead of_ removing
> >       > > the file from the index, adding both files with different
> >       > > "~blabla" markers appended to their file names to the working
> >       > > directory.

What is this new xdl_merge()? Is it a better replacement for diff3? In
this situation diff3 would actually show two confict parts, each of
them being the full file, with an empty ancestor.

> This fixes the behaviour in "both branches add the path
> differently" case.  Previously merge-recursive did not create
> the working tree file, but now it does just like merge-resolve.
>
> Although I would feel very happy about this change, Catalin
> might want to be informed about potential interaction this
> change might have with his commit 8d41555 in StGIT.

I don't think it affects StGIT. Previously, "git-read-tree -m" left a
file in the tree in this conflict situation. When I switched to
git-merge-recursive (to handle renames better), I noticed that the
file was no longer there and my merge algorithm failed. It now checks
whether the file is missing and it generates one.

The way StGIT handle any conflicts is not to leave the index in a
state with multiple stages per file. When I push a patch that is
causing an add/add situation, I want a version of the file to be added
to the index (usually the one already in the tree, not in the patch
being pushed) so that a "stg status" won't show like the patch is
removing that file.

-- 

Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Junio C Hamano]

"Catalin Marinas" <catalin.marinas@gmail.com> writes:

> On 13/12/06, Junio C Hamano <junkio@cox.net> wrote:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
> What is this new xdl_merge()? Is it a better replacement for diff3? In
> this situation diff3 would actually show two confict parts, each of
> them being the full file, with an empty ancestor.

The calls to xdl_merge() from merge-recursive replace
invocations to external "merge" from RCS suite.

An older merge-recursive had a bug (as you noticed and adjusted
StGIT with the commit 8d41555) in that it did not leave anything
in the working tree in add/add situation, while merge-resolve
would have left its best attempt of ancestor-less two file merge
(which is not necessarily the straight diff3 "no common section,
full copies from both" result).  The change in question corrects
that problem and merge-recursive would create a file in the
working tree just like merge-resolve would.

You were CC'ed just in case this change in behaviour might
interact with the abovementioned change in StGIT, but as you say
the change would not break StGIT, we would all be happy ;-).

Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Johannes Schindelin]

Hi,

On Wed, 13 Dec 2006, Catalin Marinas wrote:

> On 13/12/06, Junio C Hamano <junkio@cox.net> wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> > > Unify the handling for cases C (add/add) and D (modify/modify).
> > >
> > >       On Tue, 12 Dec 2006, Junio C Hamano wrote:
> > >
> > >       > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > >       >
> > >       > > How about this: if there is an add/add conflict, we treat it
> > >       > > as if there _was_ an empty file, and we let the shiny new
> > >       > > xdl_merge() find the _true_ conflicts, _instead of_ removing
> > >       > > the file from the index, adding both files with different
> > >       > > "~blabla" markers appended to their file names to the working
> > >       > > directory.
> 
> What is this new xdl_merge()? Is it a better replacement for diff3? In 
> this situation diff3 would actually show two confict parts, each of them 
> being the full file, with an empty ancestor.

xdl_merge(), as Git uses it, tries harder to find the true conflicts. So, 
if the files actually differ in only one line, just this line will be 
shown as conflict.

There is a flag passed to xdl_merge(), which tells it how hard it should 
try to make sense of the conflict. We use the "zealous" option, which is 
most accurate, but also slowest (although I have no numbers).

> > This fixes the behaviour in "both branches add the path
> > differently" case.  Previously merge-recursive did not create
> > the working tree file, but now it does just like merge-resolve.
> > 
> > Although I would feel very happy about this change, Catalin
> > might want to be informed about potential interaction this
> > change might have with his commit 8d41555 in StGIT.
> 
> I don't think it affects StGIT. Previously, "git-read-tree -m" left a
> file in the tree in this conflict situation. When I switched to
> git-merge-recursive (to handle renames better), I noticed that the
> file was no longer there and my merge algorithm failed. It now checks
> whether the file is missing and it generates one.

Great!

Ciao,
Dscho

Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Catalin Marinas]

On 13/12/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> xdl_merge(), as Git uses it, tries harder to find the true conflicts. So,
> if the files actually differ in only one line, just this line will be
> shown as conflict.

I gave the latest GIT a try and it works OK with StGIT.

This new merge looks much better than diff3 (or rcs merge) because it
only shows the true conflicts.

What it the relation between git-merge-recursive and "git-read-tree
-m" (if any)? I currently still use "git-read-tree -m" for some merges
because of the speed gain due to the --agressive option (really
noticeable when picking a patch from an older branch). Probably
git-merge-recursive cannot implement this since it needs to track
deletion/additions for rename detection.

Are there any other things to be aware if I completely replace the
"git-read-tree + diff3" with git-merge-recursive?

One nice addition to git-merge-recursive (probably only useful to
StGIT) would be more meaningful labeling of the conflict regions,
passed via a command line similar to the "diff3 -L" option. StGIT
generates "patched", "current" and "ancestor" labels with diff3.

Yet another nice feature would be the ancestor region (which diff3
doesn't add either but it gets added by emacs'
ediff-merge-files-with-ancestor function if you use the interactive
merge with StGIT).

-- 

Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Shawn Pearce]

Catalin Marinas <catalin.marinas@gmail.com> wrote:
> One nice addition to git-merge-recursive (probably only useful to
> StGIT) would be more meaningful labeling of the conflict regions,
> passed via a command line similar to the "diff3 -L" option. StGIT
> generates "patched", "current" and "ancestor" labels with diff3.

Indeed.  Getting a SHA1 hash of the branch that I merged in by
name with "git merge foof" is horrible UI.  I want to see "foof"
in the conflict.  Its in my list of things I'd really like to fix,
but is currently low priority compared to the issues I've run into
this past week with git-merge-recursive's basic functions.

On the other hand Git's open source strategy is really paying off on
resolving those problems, typically the issue is fixed and accepted
by Junio into 'next' in under a day of identification.  :-)

-- 

Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Shawn Pearce]

Catalin Marinas <catalin.marinas@gmail.com> wrote:
> What it the relation between git-merge-recursive and "git-read-tree
> -m" (if any)? I currently still use "git-read-tree -m" for some merges
> because of the speed gain due to the --agressive option (really
> noticeable when picking a patch from an older branch). Probably
> git-merge-recursive cannot implement this since it needs to track
> deletion/additions for rename detection.

There is a difference; always has been, probably will be for a
long time.

read-tree -m performs some trivial merges in the index.  Its manual
page explains it in gory detail, but its the really trivial, basic
three way merge rules: Given two trees X and Y and some so-called
base B:

 * If X == Y == B for that file, take any of the three.
 * If X == Y, but not B, take X or Y.
 * If X changes a file, but Y == B for that file, take X.
 * If Y changes a file, but X == B for that file, take Y.

I believe that the --agressive option has added some additional
rules about trivial file deletes.  However notice the all important
rule is not handled by read-tree -m:

 * If X changes a file, Y also changes file, merge them to create Z.

This is where read-tree -m punts and hands things off to
merge-recursive, which needs to invoke diff3 (or now the internal
xdl_merge).  read-tree -m also currently does not handle file
additions, mode changes, or renames/copies.  All of which are a
lot more expensive to compute and are slightly less common.

So Git gets decent performance by going through the rather cheap
read-tree -m, then falling back into the slower merge-recursive
when read-tree -m punted.  Given that I see about 50% of my merges
succeed with just read-tree -m and the other half punt over to
merge-recursive it just about balances out over several merges.

> Are there any other things to be aware if I completely replace the
> "git-read-tree + diff3" with git-merge-recursive?

From what I understand git-merge-recursive will do everything that
git-read-tree -m will do, except its going to be slower doing the
really common, stupid cases that git-read-tree -m can handle on
its own.

-- 

Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Johannes Schindelin]

Hi,

On Thu, 14 Dec 2006, Catalin Marinas wrote:

> On 13/12/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > xdl_merge(), as Git uses it, tries harder to find the true conflicts. So,
> > if the files actually differ in only one line, just this line will be
> > shown as conflict.
> 
> I gave the latest GIT a try and it works OK with StGIT.

Great!

> What it the relation between git-merge-recursive and "git-read-tree
> -m" (if any)?

git-merge-recursive internally calls the equivalent of 'git-read-tree -m' 
(it does not fork() and exec(), but rather calls the C functions 
directly) and does nothing more if the 'git-write-tree' succeeds. At least 
that's the theory...

If the git-write-tree does not succeed, it is because there are unmerged 
entries, and only then, merge-recursive does the rename detection and the 
file level merges.

> One nice addition to git-merge-recursive (probably only useful to
> StGIT) would be more meaningful labeling of the conflict regions,
> passed via a command line similar to the "diff3 -L" option. StGIT
> generates "patched", "current" and "ancestor" labels with diff3.

This is possible. However, it is not _that_ horrible to see "HEAD" and 
some SHA1 which is obviously non-HEAD. Added to that, a quite common case 
are the intermediate merges which make merge-recursive so powerful, and 
they are rightly called "Temporary branch 1" and "... 2".

> Yet another nice feature would be the ancestor region (which diff3
> doesn't add either but it gets added by emacs'
> ediff-merge-files-with-ancestor function if you use the interactive
> merge with StGIT).

Feasible, yes. But the code was written without that idea, so it would 
involve changes in the xdmerge_t structure (add i0, chg0), recording that 
in xdl_append_merge(), and passing also the base to 
xdl_fill_merge_buffer().

Is this really that nice? I never needed it... Besides, it can get really 
crappy when the conflicting regions are too large.

Ciao,
Dscho

Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base

[Catalin Marinas]

On 14/12/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Thu, 14 Dec 2006, Catalin Marinas wrote:
> > What it the relation between git-merge-recursive and "git-read-tree
> > -m" (if any)?
>
> git-merge-recursive internally calls the equivalent of 'git-read-tree -m'
> (it does not fork() and exec(), but rather calls the C functions
> directly) and does nothing more if the 'git-write-tree' succeeds. At least
> that's the theory...

OK, thanks (I imagined it might work like this).

> > One nice addition to git-merge-recursive (probably only useful to
> > StGIT) would be more meaningful labeling of the conflict regions,
> > passed via a command line similar to the "diff3 -L" option. StGIT
> > generates "patched", "current" and "ancestor" labels with diff3.
>
> This is possible. However, it is not _that_ horrible to see "HEAD" and
> some SHA1 which is obviously non-HEAD. Added to that, a quite common case
> are the intermediate merges which make merge-recursive so powerful, and
> they are rightly called "Temporary branch 1" and "... 2".

From the StGIT perspective, it only does a three-way merge and passes
commit id to git-merge-recursive, hence the complicated naming.
However, it wouldn't be hard to modify StGIT to actually replace the
hashes in the file with meaningful names.

> > Yet another nice feature would be the ancestor region (which diff3
> > doesn't add either but it gets added by emacs'
> > ediff-merge-files-with-ancestor function if you use the interactive
> > merge with StGIT).
>
> Is this really that nice? I never needed it... Besides, it can get really
> crappy when the conflicting regions are too large.

I think I only used the ancestor region for a mental representation of
what xdl_merge already produces :-) since emacs (nor diff3) wasn't
able to show the real differences. Probably no longer needed now.

Thanks for the explanations.

--