Re: [PATCH] merge-recursive: cause a conflict if file mode does not match
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Mar 13, 2008, 21:17 UTC
- Message-ID
- <alpine.LSU.1.00.0803132216580.4174@racer.site>
- In-Reply-To
- <20080313192246.GA30361@localhost>
Hi,
On Thu, 13 Mar 2008, Clemens Buchacher wrote:
Show 14 quoted lines
> Previously, mismatching file modes would be auto-merged by picking the > mode in the remote tree. > > This also fixes a bug which caused merge-recursive to fail if the merged > files were empty. > > Signed-off-by: Clemens Buchacher <drizzd@aon.at> > --- > > Hi Dscho, > > Your patch certainly fixes a bug in git-merge-file. It does not fix the > bug in git-merge-recursive, however. The test script also fails with > your patch.
Now, _that_ is funny. I tested before sending, and my test suit runs just fine.
Show 10 quoted lines
> On Thu, Mar 13, 2008 at 04:19:35PM +0100, Johannes Schindelin wrote: > > On Sat, Mar 08, 2008 at 06:17:26PM +0100, Clemens Buchacher wrote: > > > One could argue that it would be better to mark the mismatching > > > permissions as a conflict. > > > > Right you are. Your whole "it still is xdl_merge()s fault" point was > > just contradicted by your own analysis. Calling xdl_merge() when the > > sha1 does _not_ differ is _a mistake_. _That_ is the bug. > > Alright, fixed in the appended patch.
I have to admit that I wanted you to fix that patch, instead of me, because you were already researching the issue.
> merge-recursive.c | 9 +++++++-- > t/t6031-merge-recursive.sh | 23 +++++++++++++++++++++++ > 2 files changed, 30 insertions(+), 2 deletions(-) > create mode 100755 t/t6031-merge-recursive.sh
Looks much better.
Show 15 quoted lines
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 34e3167..01918a7 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1028,9 +1028,14 @@ static struct merge_file_info merge_file(struct diff_filespec *o,
> if (!sha_eq(a->sha1, o->sha1) && !sha_eq(b->sha1, o->sha1))
> result.merge = 1;
>
> - result.mode = a->mode == o->mode ? b->mode: a->mode;
> + if (!o->mode) {
> + if (a->mode != b->mode)
> + result.clean = 0;
> + result.mode = b->mode;
> + } else
> + result.mode = a->mode == o->mode ? b->mode: a->mode;So you only set clean = 0 if o->mode == 0, i.e. the file did not exist? That was not what I had in mind. I would have expected that "if (a->mode != b->mode)" to come _before_ the assignment to result.mode, which should have been left alone.
The rationale would have been this:
If the modes are different, the merge is not clean.
If the SHA-1s differ, the merge is not clean, and xld_merge() should be called.
> - if (sha_eq(a->sha1, o->sha1)) > + if (sha_eq(a->sha1, b->sha1) || sha_eq(a->sha1, o->sha1))
Why do you still compare to o->sha1?
/me goes looking in the original source, since the issue and the fix does not become apparent from your patch, including the commit message.
Oh, okay.
You are reusing a _different_ case, which just happens to have the same outcome.
In a perfect world, this would have a one-line comment above to explain issues.
Show 12 quoted lines
> diff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh > new file mode 100755 > index 0000000..7ea371e > --- /dev/null > +++ b/t/t6031-merge-recursive.sh > @@ -0,0 +1,23 @@ > +#!/bin/sh > + > +test_description='merge-recursive corner cases' > +. ./test-lib.sh > + > +test_expect_success 'merge empty files with different permission flags' '
The point is not that they are empty. Maybe you want to fix that message.
Show 20 quoted lines
> + : >dummy && > + git add dummy && > + git commit -m "initial commit" && > + git checkout -b a master && > + : >a && > + git add a && > + git commit -m "branch a" && > + git checkout -b b master && > + : >a && > + chmod +x a && > + git add a && > + git commit -m "branch b" && > + git checkout master && > + ! (git merge-recursive master -- a b || test $? -ne 1) > +' > + > +test_done > -- > 1.5.4.4.2.gd2fe >
Thanks, Dscho