Re: [PATCH] fix recursive-merge of empty files with different permissions

[Clemens Buchacher <drizzd@aon.at>]

Hi,

On Sat, Mar 08, 2008 at 06:17:26PM +0100, Clemens Buchacher wrote:
> If git-merge-recursive attempts to merge two empty new files with
> different executable flags, eventually xdl_merge() is called and produces
> empty diffs for both files and therefore does not choose either file as
> successor. Make xdl_merge() choose one of the files instead.

On Sat, Mar 08, 2008 at 06:51:48PM +0100, Johannes Schindelin wrote:
> On Sat, 8 Mar 2008, Clemens Buchacher wrote:
> > I do not understand why, but this does not happen if the file 
> > permissions are the same.
> 
> I think this is the biggest problem.
> 
> >  t/t6031-merge-recursive.sh |   23 +++++++++++++++++++++++
> >  xdiff/xmerge.c             |   30 ++++++++++++++----------------
> 
> ... because xdiff/xmerge.c is definitely the wrong place to "fix" this 
> issue.  xdl_merge() does not even _know_ about permissions.

After analyzing the problem in greater detail, I have to disagree. It is true,
of course, that xdl_merge() does not and should not know about permissions at
all. However, the bug is still in xdl_merge(). Different permissions are only
the trigger of the problem, because only then will xdl_merge() be called at
all.

What happens is this. Before looking at the file contents directly
merge_trees() attempts to resolve the merge trivially. If both sha1 and mode of
the head and remote entries match, the merge will be resolved as per case #5ALT
(see Documentation/trivial-merge.txt), i.e. head is chosen as the merge result.

If either sha1 _or_ mode differ between the head and remote entries, however,
merge_trees() will use xdl_merge() to merge the file content and the remote
entry's mode will be chosen as result mode.

One could argue that it would be better to mark the mismatching permissions as
a conflict. However, this is how the merge currently silently succeeds _unless_
both files are empty. If they are, xdl_merge() will effectively exit with an
error status and git-merge-recursive will fail with an internal error (as shown
in the testcase).

In any case, I think it is reasonable to expect xdl_merge() to work with empty
files. Whether or not the current "mode merging" behavior is desired is a
different matter.

Regards,
Clemens