auto merge bug

3 messages, 2 authors, 2016-06-15 · open the first message on its own page

auto merge bug

From: David Krmpotic <hidden>
Date: 2016-06-15 22:56:18

Hi!

We started working on a .NET app and the XML project file (.csproj)
got corrupted (a few closing tag missing).

79	     <Compile Include="SlovaricaForm.Designer.cs">
80	       <DependentUpon>SlovaricaForm.cs</DependentUpon>
81	+    <Compile Include="WebCamForm.cs">
82	+      <SubType>Form</SubType>
83	+    </Compile>
84	+    <Compile Include="WebCamForm.Designer.cs">
85	+      <DependentUpon>WebCamForm.cs</DependentUpon>
86	     </Compile>

between lines 80 and 81 there should be </Compile>

similarly:

121	     </EmbeddedResource>
122	     <EmbeddedResource Include="SlovaricaForm.resx">
123	       <DependentUpon>SlovaricaForm.cs</DependentUpon>
124	+    <EmbeddedResource Include="WebCamForm.resx">
125	+      <DependentUpon>WebCamForm.cs</DependentUpon>
126	     </EmbeddedResource>
127	     <EmbeddedResource Include="WordsSelectForm.resx">
128	       <DependentUpon>WordsSelectForm.cs</DependentUpon>

between 123 and 124 there is  </EmbeddedResource> missing.

The problematic commit is here:

https://github.com/davidhq/logo_x/commit/e3e5fa4b60b7939999b2a8c44330312755b72f93

it has two parents: ae2a364 and bd1a059

on both parents the project compiles in Visual Studio because
Logo.csproj is not corrupted.

How to reproduce and see that really there were no conflicts and the
file became corrupted:

C:\temp> git clone git@github.com:davidhq/logo_x.git
C:\temp\logo_x [master]> git checkout ae2a364
Note: checking out 'ae2a364'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b new_branch_name

HEAD is now at ae2a364... general handler for letters
C:\temp\logo_x [(ae2a364...)]> git merge bd1a059
Auto-merging Logo/Logo.csproj
Merge made by the 'recursive' strategy.
 Logo/Logo.csproj            |   7 ++
 Logo/WebCamForm.Designer.cs |  88 +++++++++++++++++++
 Logo/WebCamForm.cs          | 209 ++++++++++++++++++++++++++++++++++++++++++++
 Logo/WebCamForm.resx        | 120 +++++++++++++++++++++++++
 Logo/WordsForm.Designer.cs  |   1 +
 Logo/WordsForm.cs           |   7 ++
 6 files changed, 432 insertions(+)
 create mode 100644 Logo/WebCamForm.Designer.cs
 create mode 100644 Logo/WebCamForm.cs
 create mode 100644 Logo/WebCamForm.resx

Now check Logo.csproj and observe line 81 (it should read </Compile>

If I add both missing closing tags the project compiles again.

Please investigate and thank you!

PS: on Windows I have version 1.8.0.msysgit.0 of git and on Mac I'm
not sure now, it's a bit older, but the same problem happens.

David

Re: auto merge bug

From: Jeff King <hidden>
Date: 2016-06-15 22:56:18

On Mon, Mar 04, 2013 at 05:46:48PM +0100, David Krmpotic wrote:
We started working on a .NET app and the XML project file (.csproj)
got corrupted (a few closing tag missing).

79	     <Compile Include="SlovaricaForm.Designer.cs">
80	       <DependentUpon>SlovaricaForm.cs</DependentUpon>
81	+    <Compile Include="WebCamForm.cs">
82	+      <SubType>Form</SubType>
83	+    </Compile>
84	+    <Compile Include="WebCamForm.Designer.cs">
85	+      <DependentUpon>WebCamForm.cs</DependentUpon>
86	     </Compile>

between lines 80 and 81 there should be </Compile>

[...]

The problematic commit is here:

https://github.com/davidhq/logo_x/commit/e3e5fa4b60b7939999b2a8c44330312755b72f93
Thanks for an easy-to-reproduce report. The problem here is that your
.gitattributes file specifies the "union" merge driver for .csproj
(and other) files. From "git help attributes":

           union
               Run 3-way file level merge for text files, but take lines
               from both versions, instead of leaving conflict markers.
               This tends to leave the added lines in the resulting file
               in random order and the user should verify the result. Do
               not use this if you do not understand the implications.

Your <Compile> stanzas each end on an identical line. So it sees that
one side has:

  A
  B
  Z

and the other side has:

  C
  D
  Z

It realizes that the "Z" is common, so is not part of the conflict. But
in the normal 3-way merge case, the rest of it conflicts, so you get a
chance to inspect it. But with "union", it just silently concatenates
the conflicting bits.

I suspect you can run into other problems with "union" here, too,
because line order _does_ matter for you. It comes close to working if
both sides are just adding elements at the same level of the tree (as
you are here), but what about more complicated edits?

I think what you really want is an XML-aware merge tool that can see you
just added two independent <Compile>...</Compile> stanzas that can
co-exist (i.e., it could do a union, but at the level of XML tags, not
at the level of individual lines).  I do not know offhand of any such
tool (or for that matter, a good general XML-aware 3-way merge tool),
but if you had one, you could plug it in as a custom merge driver.

You might be able to get by with a version of the "union" driver that
asks the 3-way merge driver to be less aggressive about shrinking the
conflict blocks. For example, with this patch to git:
diff --git a/ll-merge.c b/ll-merge.c
index fb61ea6..61b1d4e 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -100,7 +100,6 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 	}
 
 	memset(&xmp, 0, sizeof(xmp));
-	xmp.level = XDL_MERGE_ZEALOUS;
 	xmp.favor = opts->variant;
 	xmp.xpp.flags = opts->xdl_opts;
 	if (git_xmerge_style >= 0)
I think the merge will produce the results you are looking for. This
would have to be configurable, though, as it is a regression for
existing users of "union", which would want the duplicate-line
suppression (or maybe not; it will only catch such duplicates at the
beginning and end of the conflict hunk, so maybe it is sane to always
ask "union" to keep all lines).

I'd still worry about more complicated edits fooling "union", but at
least the simple cases would work.

In the meantime, I think you are better to drop those "merge"
gitattributes, and just let the regular three way merge generate a
conflict which you can inspect. For the merge in question, it yields:

  diff --cc Logo/Logo.csproj
  index 4113434,c681862..0000000
  --- a/Logo/Logo.csproj
  +++ b/Logo/Logo.csproj
  @@@ -67,17 -67,11 +67,25 @@@
        <Reference Include="System.Xml" />
      </ItemGroup>
      <ItemGroup>
  ++<<<<<<< HEAD
   +    <Compile Include="MainForm.cs">
   +      <SubType>Form</SubType>
   +    </Compile>
   +    <Compile Include="MainForm.Designer.cs">
   +      <DependentUpon>MainForm.cs</DependentUpon>
   +    </Compile>
   +    <Compile Include="SlovaricaForm.cs">
   +      <SubType>Form</SubType>
   +    </Compile>
   +    <Compile Include="SlovaricaForm.Designer.cs">
   +      <DependentUpon>SlovaricaForm.cs</DependentUpon>
  ++=======
  +     <Compile Include="WebCamForm.cs">
  +       <SubType>Form</SubType>
  +     </Compile>
  +     <Compile Include="WebCamForm.Designer.cs">
  +       <DependentUpon>WebCamForm.cs</DependentUpon>
  ++>>>>>>> bd1a059
        </Compile>
        <Compile Include="WordsSelectForm.cs">
          <SubType>Form</SubType>

where you can see that it "shrinks" the conflict hunk to not include the
line added by both sides (the file "</Compile>" after the conflict). But
by triggering a conflict, you can actually look at and fix it. That's
more work, of course.

Another alternate is to keep the "union" driver and just do better
testing of merges. Even with the stock 3-way driver, a merge that
auto-resolves is not necessarily correct (e.g., even if there are not
textual conflicts, there may be semantic ones).

-Peff

Re: auto merge bug

From: Jeff King <hidden>
Date: 2016-06-15 22:56:18

On Tue, Mar 05, 2013 at 04:03:26AM -0500, Jeff King wrote:
quoted hunk
You might be able to get by with a version of the "union" driver that
asks the 3-way merge driver to be less aggressive about shrinking the
conflict blocks. For example, with this patch to git:
diff --git a/ll-merge.c b/ll-merge.c
index fb61ea6..61b1d4e 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -100,7 +100,6 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 	}
 
 	memset(&xmp, 0, sizeof(xmp));
-	xmp.level = XDL_MERGE_ZEALOUS;
 	xmp.favor = opts->variant;
 	xmp.xpp.flags = opts->xdl_opts;
 	if (git_xmerge_style >= 0)
I think the merge will produce the results you are looking for. This
would have to be configurable, though, as it is a regression for
existing users of "union", which would want the duplicate-line
suppression (or maybe not; it will only catch such duplicates at the
beginning and end of the conflict hunk, so maybe it is sane to always
ask "union" to keep all lines).
Here's what the patch would look like to make it non-configurable, but
to just trigger for the "union" case:
diff --git a/ll-merge.c b/ll-merge.c
index fb61ea6..fc33a23 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -83,7 +83,8 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 			mmfile_t *src1, const char *name1,
 			mmfile_t *src2, const char *name2,
 			const struct ll_merge_options *opts,
-			int marker_size)
+			int marker_size,
+			int level)
 {
 	xmparam_t xmp;
 	assert(opts);
@@ -100,7 +101,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 	}
 
 	memset(&xmp, 0, sizeof(xmp));
-	xmp.level = XDL_MERGE_ZEALOUS;
+	xmp.level = level;
 	xmp.favor = opts->variant;
 	xmp.xpp.flags = opts->xdl_opts;
 	if (git_xmerge_style >= 0)
@@ -129,7 +130,23 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused,
 	o.variant = XDL_MERGE_FAVOR_UNION;
 	return ll_xdl_merge(drv_unused, result, path_unused,
 			    orig, NULL, src1, NULL, src2, NULL,
-			    &o, marker_size);
+			    &o, marker_size, XDL_MERGE_MINIMAL);
+}
+
+static int ll_text_merge(const struct ll_merge_driver *drv,
+			 mmbuffer_t *result,
+			 const char *path,
+			 mmfile_t *orig, const char *orig_name,
+			 mmfile_t *src1, const char *name1,
+			 mmfile_t *src2, const char *name2,
+			 const struct ll_merge_options *opts,
+			 int marker_size)
+{
+	return ll_xdl_merge(drv, result, path,
+			    orig, orig_name,
+			    src1, name1,
+			    src2, name2,
+			    opts, marker_size, XDL_MERGE_ZEALOUS);
 }
 
 #define LL_BINARY_MERGE 0
@@ -137,7 +154,7 @@ static struct ll_merge_driver ll_merge_drv[] = {
 #define LL_UNION_MERGE 2
 static struct ll_merge_driver ll_merge_drv[] = {
 	{ "binary", "built-in binary merge", ll_binary_merge },
-	{ "text", "built-in 3-way text merge", ll_xdl_merge },
+	{ "text", "built-in 3-way text merge", ll_text_merge },
 	{ "union", "built-in union merge", ll_union_merge },
 };
 
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help