[PATCH] merge-tree: fix "same file added in subdir"

[PATCH] merge-tree: fix "same file added in subdir"

[John Keeping]

When the same file is added with identical content at the top level,
git-merge-tree prints "added in both" with the details.  But if the file
is added in an existing subdirectory, threeway_callback() bails out early
because the two trees have been modified identically.

In order to detect this, we need to fall through and recurse into the
subtree in this case.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 builtin/merge-tree.c  |  9 +++++++--
 t/t4300-merge-tree.sh | 17 +++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index e0d0b7d..ca97fbd 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -298,12 +298,17 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s
 {
 	/* Same in both? */
 	if (same_entry(entry+1, entry+2)) {
-		if (entry[0].sha1) {
+		if (entry[0].sha1 && !S_ISDIR(entry[0].mode)) {
 			/* Modified identically */
 			resolve(info, NULL, entry+1);
 			return mask;
 		}
-		/* "Both added the same" is left unresolved */
+		/*
+		 * "Both added the same" is left unresolved.  We also leave
+		 * "Both directories modified identically" unresolved in
+		 * order to catch changes where the same file (with the same
+		 * content) has been added to both directories.
+		 */
 	}
 
 	if (same_entry(entry+0, entry+1)) {
diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
index d0b2a45..be0737e 100755
--- a/t/t4300-merge-tree.sh
+++ b/t/t4300-merge-tree.sh
@@ -298,4 +298,21 @@ test_expect_success 'turn tree to file' '
 	test_cmp expect actual
 '
 
+test_expect_success 'add identical files to subdir' '
+	cat >expected <<\EXPECTED &&
+added in both
+  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d sub/ONE
+  their  100644 43d5a8ed6ef6c00ff775008633f95787d088285d sub/ONE
+EXPECTED
+
+	git reset --hard initial &&
+	mkdir sub &&
+	test_commit "sub-initial" "sub/initial" "initial" &&
+	test_commit "sub-add-a-b-same-A" "sub/ONE" "AAA" &&
+	git reset --hard sub-initial &&
+	test_commit "sub-add-a-b-same-B" "sub/ONE" "AAA" &&
+	git merge-tree sub-initial sub-add-a-b-same-A sub-add-a-b-same-B >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
1.8.2.411.g65a544e

Re: [PATCH] merge-tree: fix "same file added in subdir"

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> When the same file is added with identical content at the top level,
> git-merge-tree prints "added in both" with the details.  But if the file
> is added in an existing subdirectory, threeway_callback() bails out early
> because the two trees have been modified identically.
>
> In order to detect this, we need to fall through and recurse into the
> subtree in this case.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>

The rationale the above description gives is internally consistent,
but it is rather sad to see this optimization go.  The primary
motivation behind this program, which does not use the usual
unpack-trees machinery, is to allow us to cull the identical result
at a shallow level of the traversal when the both sides changed (not
added) a file deep in a subdirectory hierarchy.

The patch makes me wonder if we should go the other way around,
resolving the "both added identically" case at the top cleanly
without complaint.

>  builtin/merge-tree.c  |  9 +++++++--
>  t/t4300-merge-tree.sh | 17 +++++++++++++++++
>  2 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index e0d0b7d..ca97fbd 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -298,12 +298,17 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s
>  {
>  	/* Same in both? */
>  	if (same_entry(entry+1, entry+2)) {
> -		if (entry[0].sha1) {
> +		if (entry[0].sha1 && !S_ISDIR(entry[0].mode)) {
>  			/* Modified identically */
>  			resolve(info, NULL, entry+1);
>  			return mask;
>  		}
> -		/* "Both added the same" is left unresolved */
> +		/*
> +		 * "Both added the same" is left unresolved.  We also leave
> +		 * "Both directories modified identically" unresolved in
> +		 * order to catch changes where the same file (with the same
> +		 * content) has been added to both directories.
> +		 */
>  	}
>  
>  	if (same_entry(entry+0, entry+1)) {
> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
> index d0b2a45..be0737e 100755
> --- a/t/t4300-merge-tree.sh
> +++ b/t/t4300-merge-tree.sh
> @@ -298,4 +298,21 @@ test_expect_success 'turn tree to file' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'add identical files to subdir' '
> +	cat >expected <<\EXPECTED &&
> +added in both
> +  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d sub/ONE
> +  their  100644 43d5a8ed6ef6c00ff775008633f95787d088285d sub/ONE
> +EXPECTED
> +
> +	git reset --hard initial &&
> +	mkdir sub &&
> +	test_commit "sub-initial" "sub/initial" "initial" &&
> +	test_commit "sub-add-a-b-same-A" "sub/ONE" "AAA" &&
> +	git reset --hard sub-initial &&
> +	test_commit "sub-add-a-b-same-B" "sub/ONE" "AAA" &&
> +	git merge-tree sub-initial sub-add-a-b-same-A sub-add-a-b-same-B >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_done

Re: [PATCH] merge-tree: fix "same file added in subdir"

[John Keeping]

On Wed, Mar 27, 2013 at 03:42:40PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > When the same file is added with identical content at the top level,
> > git-merge-tree prints "added in both" with the details.  But if the file
> > is added in an existing subdirectory, threeway_callback() bails out early
> > because the two trees have been modified identically.
> >
> > In order to detect this, we need to fall through and recurse into the
> > subtree in this case.
> >
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> 
> The rationale the above description gives is internally consistent,
> but it is rather sad to see this optimization go.  The primary
> motivation behind this program, which does not use the usual
> unpack-trees machinery, is to allow us to cull the identical result
> at a shallow level of the traversal when the both sides changed (not
> added) a file deep in a subdirectory hierarchy.
> 
> The patch makes me wonder if we should go the other way around,
> resolving the "both added identically" case at the top cleanly
> without complaint.

I don't use merge-tree so I have no opinion on this, just wanted to fix
an inconsistency :-)

I'll try to have a look at doing the other change tomorrow if no one
else gets there first.

> >  builtin/merge-tree.c  |  9 +++++++--
> >  t/t4300-merge-tree.sh | 17 +++++++++++++++++
> >  2 files changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> > index e0d0b7d..ca97fbd 100644
> > --- a/builtin/merge-tree.c
> > +++ b/builtin/merge-tree.c
> > @@ -298,12 +298,17 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s
> >  {
> >  	/* Same in both? */
> >  	if (same_entry(entry+1, entry+2)) {
> > -		if (entry[0].sha1) {
> > +		if (entry[0].sha1 && !S_ISDIR(entry[0].mode)) {
> >  			/* Modified identically */
> >  			resolve(info, NULL, entry+1);
> >  			return mask;
> >  		}
> > -		/* "Both added the same" is left unresolved */
> > +		/*
> > +		 * "Both added the same" is left unresolved.  We also leave
> > +		 * "Both directories modified identically" unresolved in
> > +		 * order to catch changes where the same file (with the same
> > +		 * content) has been added to both directories.
> > +		 */
> >  	}
> >  
> >  	if (same_entry(entry+0, entry+1)) {
> > diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
> > index d0b2a45..be0737e 100755
> > --- a/t/t4300-merge-tree.sh
> > +++ b/t/t4300-merge-tree.sh
> > @@ -298,4 +298,21 @@ test_expect_success 'turn tree to file' '
> >  	test_cmp expect actual
> >  '
> >  
> > +test_expect_success 'add identical files to subdir' '
> > +	cat >expected <<\EXPECTED &&
> > +added in both
> > +  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d sub/ONE
> > +  their  100644 43d5a8ed6ef6c00ff775008633f95787d088285d sub/ONE
> > +EXPECTED
> > +
> > +	git reset --hard initial &&
> > +	mkdir sub &&
> > +	test_commit "sub-initial" "sub/initial" "initial" &&
> > +	test_commit "sub-add-a-b-same-A" "sub/ONE" "AAA" &&
> > +	git reset --hard sub-initial &&
> > +	test_commit "sub-add-a-b-same-B" "sub/ONE" "AAA" &&
> > +	git merge-tree sub-initial sub-add-a-b-same-A sub-add-a-b-same-B >actual &&
> > +	test_cmp expected actual
> > +'
> > +
> >  test_done

Re: [PATCH] merge-tree: fix "same file added in subdir"

[John Keeping]

On Wed, Mar 27, 2013 at 10:57:39PM +0000, John Keeping wrote:
> On Wed, Mar 27, 2013 at 03:42:40PM -0700, Junio C Hamano wrote:
> > John Keeping <john@keeping.me.uk> writes:
> > 
> > > When the same file is added with identical content at the top level,
> > > git-merge-tree prints "added in both" with the details.  But if the file
> > > is added in an existing subdirectory, threeway_callback() bails out early
> > > because the two trees have been modified identically.
> > >
> > > In order to detect this, we need to fall through and recurse into the
> > > subtree in this case.
> > >
> > > Signed-off-by: John Keeping <john@keeping.me.uk>
> > 
> > The rationale the above description gives is internally consistent,
> > but it is rather sad to see this optimization go.  The primary
> > motivation behind this program, which does not use the usual
> > unpack-trees machinery, is to allow us to cull the identical result
> > at a shallow level of the traversal when the both sides changed (not
> > added) a file deep in a subdirectory hierarchy.
> > 
> > The patch makes me wonder if we should go the other way around,
> > resolving the "both added identically" case at the top cleanly
> > without complaint.
> 
> I don't use merge-tree so I have no opinion on this, just wanted to fix
> an inconsistency :-)

Having re-read the manpage, I think you're right that we should just
resolve the "both added identically" case cleanly, but I wonder whether
some of the other cases should also be resolved cleanly.

git-merge-tree(1) says:

    the output from the command omits entries that match the <branch1>
    tree.

so you could argue that "added in branch1", "changed in branch1,
unmodified in branch2" and "removed in branch1, unchanged in branch2"
should also print no output.

But as I said above I don't use git-merge-tree so perhaps people who do
would like to explain what they expect in these cases.

Re: [PATCH] merge-tree: fix "same file added in subdir"

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> Having re-read the manpage, I think you're right that we should just
> resolve the "both added identically" case cleanly, but I wonder whether
> some of the other cases should also be resolved cleanly.
>
> git-merge-tree(1) says:
>
>     the output from the command omits entries that match the <branch1>
>     tree.
>
> so you could argue that "added in branch1", "changed in branch1,
> unmodified in branch2" and "removed in branch1, unchanged in branch2"
> should also print no output.

Yes, that description explains what we wanted to achieve with
merge-tree that is still an unfinished experiment and fixing these
cases you identified above to match the stated goal is a good thing
to do.  I've been meaning to find enough time to do a new merge
strategy backend based on the logic of this program, and the few
patches I sent out on it recently were fallout from preparatory
steps for it.

[PATCH v2] merge-tree: don't print entries that match "local"

[John Keeping]

The documentation says:

	the output from the command omits entries that match the
	<branch1> tree.

But currently "added in branch1" and "removed in branch1" (both while
unchanged in branch2) do print output.  Change this so that the
behaviour matches the documentation.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 builtin/merge-tree.c  | 26 +++++++++++++-------------
 t/t4300-merge-tree.sh | 10 ----------
 2 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index bc912e3..ed25d81 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -155,6 +155,11 @@ static int same_entry(struct name_entry *a, struct name_entry *b)
 		a->mode == b->mode;
 }
 
+static int both_empty(struct name_entry *a, struct name_entry *b)
+{
+	return !(a->sha1 || b->sha1);
+}
+
 static struct merge_list *create_entry(unsigned stage, unsigned mode, const unsigned char *sha1, const char *path)
 {
 	struct merge_list *res = xcalloc(1, sizeof(*res));
@@ -297,13 +302,10 @@ static void unresolved(const struct traverse_info *info, struct name_entry n[3])
 static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *entry, struct traverse_info *info)
 {
 	/* Same in both? */
-	if (same_entry(entry+1, entry+2)) {
-		if (entry[0].sha1) {
-			/* Modified identically */
-			resolve(info, NULL, entry+1);
-			return mask;
-		}
-		/* "Both added the same" is left unresolved */
+	if (same_entry(entry+1, entry+2) || both_empty(entry+0, entry+2)) {
+		/* Modified, added or removed identically */
+		resolve(info, NULL, entry+1);
+		return mask;
 	}
 
 	if (same_entry(entry+0, entry+1)) {
@@ -319,12 +321,10 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s
 		 */
 	}
 
-	if (same_entry(entry+0, entry+2)) {
-		if (entry[1].sha1 && !S_ISDIR(entry[1].mode)) {
-			/* We modified, they did not touch -- take ours */
-			resolve(info, NULL, entry+1);
-			return mask;
-		}
+	if (same_entry(entry+0, entry+2) || both_empty(entry+0, entry+2)) {
+		/* We added, modified or removed, they did not touch -- take ours */
+		resolve(info, NULL, entry+1);
+		return mask;
 	}
 
 	unresolved(info, entry);
diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
index d0b2a45..bd43b3d 100755
--- a/t/t4300-merge-tree.sh
+++ b/t/t4300-merge-tree.sh
@@ -26,8 +26,6 @@ EXPECTED
 
 test_expect_success 'file add !A, B' '
 	cat >expected <<\EXPECTED &&
-added in local
-  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
 EXPECTED
 
 	git reset --hard initial &&
@@ -38,9 +36,6 @@ EXPECTED
 
 test_expect_success 'file add A, B (same)' '
 	cat >expected <<\EXPECTED &&
-added in both
-  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
-  their  100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
 EXPECTED
 
 	git reset --hard initial &&
@@ -181,9 +176,6 @@ AAA" &&
 
 test_expect_success 'file remove A, !B' '
 	cat >expected <<\EXPECTED &&
-removed in local
-  base   100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
-  their  100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
 EXPECTED
 
 	git reset --hard initial &&
@@ -283,8 +275,6 @@ test_expect_success 'turn tree to file' '
 	test_commit "make-file" "dir" "CCC" &&
 	git merge-tree add-tree add-another-tree make-file >actual &&
 	cat >expect <<-\EOF &&
-	added in local
-	  our    100644 ba629238ca89489f2b350e196ca445e09d8bb834 dir/another
 	removed in remote
 	  base   100644 43d5a8ed6ef6c00ff775008633f95787d088285d dir/path
 	  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d dir/path
-- 
1.8.2.694.ga76e9c3.dirty

Re: [PATCH v2] merge-tree: don't print entries that match "local"

[René Scharfe]

Am 07.04.2013 23:07, schrieb John Keeping:
> The documentation says:
>
> 	the output from the command omits entries that match the
> 	<branch1> tree.
>
> But currently "added in branch1" and "removed in branch1" (both while
> unchanged in branch2) do print output.  Change this so that the
> behaviour matches the documentation.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>   builtin/merge-tree.c  | 26 +++++++++++++-------------
>   t/t4300-merge-tree.sh | 10 ----------
>   2 files changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index bc912e3..ed25d81 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -155,6 +155,11 @@ static int same_entry(struct name_entry *a, struct name_entry *b)
>   		a->mode == b->mode;
>   }
>
> +static int both_empty(struct name_entry *a, struct name_entry *b)
> +{
> +	return !(a->sha1 || b->sha1);
> +}
> +
>   static struct merge_list *create_entry(unsigned stage, unsigned mode, const unsigned char *sha1, const char *path)
>   {
>   	struct merge_list *res = xcalloc(1, sizeof(*res));
> @@ -297,13 +302,10 @@ static void unresolved(const struct traverse_info *info, struct name_entry n[3])
>   static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *entry, struct traverse_info *info)
>   {
>   	/* Same in both? */
> -	if (same_entry(entry+1, entry+2)) {
> -		if (entry[0].sha1) {
> -			/* Modified identically */
> -			resolve(info, NULL, entry+1);
> -			return mask;
> -		}
> -		/* "Both added the same" is left unresolved */
> +	if (same_entry(entry+1, entry+2) || both_empty(entry+0, entry+2)) {

Shouldn't this zero be a one instead?

> +		/* Modified, added or removed identically */
> +		resolve(info, NULL, entry+1);
> +		return mask;
>   	}
>
>   	if (same_entry(entry+0, entry+1)) {
> @@ -319,12 +321,10 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s
>   		 */
>   	}
>
> -	if (same_entry(entry+0, entry+2)) {
> -		if (entry[1].sha1 && !S_ISDIR(entry[1].mode)) {
> -			/* We modified, they did not touch -- take ours */
> -			resolve(info, NULL, entry+1);
> -			return mask;
> -		}
> +	if (same_entry(entry+0, entry+2) || both_empty(entry+0, entry+2)) {

Otherwise the both_empty check here can be removed because we'd have 
already returned above if it was true.

But do we actually want to resolve the removal of a file on both sides 
silently?  The added comment above says so, but the commit message 
doesn't mention it.

> +		/* We added, modified or removed, they did not touch -- take ours */
> +		resolve(info, NULL, entry+1);
> +		return mask;
>   	}
>
>   	unresolved(info, entry);
> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
> index d0b2a45..bd43b3d 100755
> --- a/t/t4300-merge-tree.sh
> +++ b/t/t4300-merge-tree.sh
> @@ -26,8 +26,6 @@ EXPECTED
>
>   test_expect_success 'file add !A, B' '
>   	cat >expected <<\EXPECTED &&
> -added in local
> -  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
>   EXPECTED
>
>   	git reset --hard initial &&
> @@ -38,9 +36,6 @@ EXPECTED
>
>   test_expect_success 'file add A, B (same)' '
>   	cat >expected <<\EXPECTED &&
> -added in both
> -  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
> -  their  100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
>   EXPECTED
>
>   	git reset --hard initial &&
> @@ -181,9 +176,6 @@ AAA" &&
>
>   test_expect_success 'file remove A, !B' '
>   	cat >expected <<\EXPECTED &&
> -removed in local
> -  base   100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
> -  their  100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
>   EXPECTED
>
>   	git reset --hard initial &&
> @@ -283,8 +275,6 @@ test_expect_success 'turn tree to file' '
>   	test_commit "make-file" "dir" "CCC" &&
>   	git merge-tree add-tree add-another-tree make-file >actual &&
>   	cat >expect <<-\EOF &&
> -	added in local
> -	  our    100644 ba629238ca89489f2b350e196ca445e09d8bb834 dir/another
>   	removed in remote
>   	  base   100644 43d5a8ed6ef6c00ff775008633f95787d088285d dir/path
>   	  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d dir/path
>

Re: [PATCH v2] merge-tree: don't print entries that match "local"

[John Keeping]

On Sat, Apr 27, 2013 at 01:35:57PM +0200, René Scharfe wrote:
> Am 07.04.2013 23:07, schrieb John Keeping:
> > The documentation says:
> >
> > 	the output from the command omits entries that match the
> > 	<branch1> tree.
> >
> > But currently "added in branch1" and "removed in branch1" (both while
> > unchanged in branch2) do print output.  Change this so that the
> > behaviour matches the documentation.
> >
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> > ---
> >   builtin/merge-tree.c  | 26 +++++++++++++-------------
> >   t/t4300-merge-tree.sh | 10 ----------
> >   2 files changed, 13 insertions(+), 23 deletions(-)
> >
> > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> > index bc912e3..ed25d81 100644
> > --- a/builtin/merge-tree.c
> > +++ b/builtin/merge-tree.c
> > @@ -155,6 +155,11 @@ static int same_entry(struct name_entry *a, struct name_entry *b)
> >   		a->mode == b->mode;
> >   }
> >
> > +static int both_empty(struct name_entry *a, struct name_entry *b)
> > +{
> > +	return !(a->sha1 || b->sha1);
> > +}
> > +
> >   static struct merge_list *create_entry(unsigned stage, unsigned mode, const unsigned char *sha1, const char *path)
> >   {
> >   	struct merge_list *res = xcalloc(1, sizeof(*res));
> > @@ -297,13 +302,10 @@ static void unresolved(const struct traverse_info *info, struct name_entry n[3])
> >   static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *entry, struct traverse_info *info)
> >   {
> >   	/* Same in both? */
> > -	if (same_entry(entry+1, entry+2)) {
> > -		if (entry[0].sha1) {
> > -			/* Modified identically */
> > -			resolve(info, NULL, entry+1);
> > -			return mask;
> > -		}
> > -		/* "Both added the same" is left unresolved */
> > +	if (same_entry(entry+1, entry+2) || both_empty(entry+0, entry+2)) {
> 
> Shouldn't this zero be a one instead?

Yes, that's a typo.

> > +		/* Modified, added or removed identically */
> > +		resolve(info, NULL, entry+1);
> > +		return mask;
> >   	}
> >
> >   	if (same_entry(entry+0, entry+1)) {
> > @@ -319,12 +321,10 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s
> >   		 */
> >   	}
> >
> > -	if (same_entry(entry+0, entry+2)) {
> > -		if (entry[1].sha1 && !S_ISDIR(entry[1].mode)) {
> > -			/* We modified, they did not touch -- take ours */
> > -			resolve(info, NULL, entry+1);
> > -			return mask;
> > -		}
> > +	if (same_entry(entry+0, entry+2) || both_empty(entry+0, entry+2)) {
> 
> Otherwise the both_empty check here can be removed because we'd have 
> already returned above if it was true.
> 
> But do we actually want to resolve the removal of a file on both sides 
> silently?  The added comment above says so, but the commit message 
> doesn't mention it.

I think so.  The rule is that we omit things that match the branch1
tree.  So if it is removed in branch1 and there isn't a conflict with
branch2 then we shouldn't be printing any output.

> > +		/* We added, modified or removed, they did not touch -- take ours */
> > +		resolve(info, NULL, entry+1);
> > +		return mask;
> >   	}
> >
> >   	unresolved(info, entry);
> > diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
> > index d0b2a45..bd43b3d 100755
> > --- a/t/t4300-merge-tree.sh
> > +++ b/t/t4300-merge-tree.sh
> > @@ -26,8 +26,6 @@ EXPECTED
> >
> >   test_expect_success 'file add !A, B' '
> >   	cat >expected <<\EXPECTED &&
> > -added in local
> > -  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
> >   EXPECTED
> >
> >   	git reset --hard initial &&
> > @@ -38,9 +36,6 @@ EXPECTED
> >
> >   test_expect_success 'file add A, B (same)' '
> >   	cat >expected <<\EXPECTED &&
> > -added in both
> > -  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
> > -  their  100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
> >   EXPECTED
> >
> >   	git reset --hard initial &&
> > @@ -181,9 +176,6 @@ AAA" &&
> >
> >   test_expect_success 'file remove A, !B' '
> >   	cat >expected <<\EXPECTED &&
> > -removed in local
> > -  base   100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
> > -  their  100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE
> >   EXPECTED
> >
> >   	git reset --hard initial &&
> > @@ -283,8 +275,6 @@ test_expect_success 'turn tree to file' '
> >   	test_commit "make-file" "dir" "CCC" &&
> >   	git merge-tree add-tree add-another-tree make-file >actual &&
> >   	cat >expect <<-\EOF &&
> > -	added in local
> > -	  our    100644 ba629238ca89489f2b350e196ca445e09d8bb834 dir/another
> >   	removed in remote
> >   	  base   100644 43d5a8ed6ef6c00ff775008633f95787d088285d dir/path
> >   	  our    100644 43d5a8ed6ef6c00ff775008633f95787d088285d dir/path
> >
>