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

From: Clemens Buchacher <drizzd@aon.at>
Date: Mar 13, 2008, 12:52 UTC
Message-ID: <20080313125229.GA24758@localhost>
In-Reply-To: <alpine.LSU.1.00.0803081850470.3975@racer.site>

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:

Show 11 quoted lines

> 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