Hi,
[Junio, there is a bugfix at the end of this mail.]
On Thu, 13 Mar 2008, Clemens Buchacher wrote:
> 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.
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. Not xdl_merge()
returning 0 (because there were 0 conflicts).
> 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.
I just tested. xdl_merge() is _just fine_ with empty files. However,
git-merge-file was not. Fixed by this:
-- snipsnap --
[PATCH] merge-file: handle empty files gracefully
Earlier, it would error out while trying to read and/or writing them.
Now, calling merge-file with empty files is neither interesting nor
useful, but it is a bug that needed fixing.
Noticed by Clemens Buchacher.
Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
---
builtin-merge-file.c | 3 ++-
xdiff-interface.c | 4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin-merge-file.c b/builtin-merge-file.c
index adce6d4..cde4b2c 100644
--- a/builtin-merge-file.c
+++ b/builtin-merge-file.c
@@ -57,7 +57,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
if (!f)
ret = error("Could not open %s for writing", filename);
- else if (fwrite(result.ptr, result.size, 1, f) != 1)
+ else if (result.size &&
+ fwrite(result.ptr, result.size, 1, f) != 1)
ret = error("Could not write to %s", filename);
else if (fclose(f))
ret = error("Could not close %s", filename);
diff --git a/xdiff-interface.c b/xdiff-interface.c
index bba2364..61dc5c5 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -152,8 +152,8 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
if ((f = fopen(filename, "rb")) == NULL)
return error("Could not open %s", filename);
sz = xsize_t(st.st_size);
- ptr->ptr = xmalloc(sz);
- if (fread(ptr->ptr, sz, 1, f) != 1)
+ ptr->ptr = xmalloc(sz ? sz : 1);
+ if (sz && fread(ptr->ptr, sz, 1, f) != 1)
return error("Could not read %s", filename);
fclose(f);
ptr->size = sz;
--
1.5.4.4.706.ga822
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
