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

[John Keeping <john@keeping.me.uk>]

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