Re: auto merge bug

2013-03-06 Thread Jeff King
On Tue, Mar 05, 2013 at 11:13:12PM +0100, David Krmpotic wrote:

 Hi guys! Thank you for responses.. I haven't suspected that repos
 created via GitHub windows app would have union set by default :( have
 to ask them about it.. it seems wrong to me… Here are the defaults for
 a windows repo created with GitHub for windows app:
 [...]
 # Custom for Visual Studio
 *.cs diff=csharp
 *.slnmerge=union
 *.csproj merge=union
 *.vbproj merge=union
 *.fsproj merge=union
 *.dbproj merge=union

Yeah, I think defaulting to merge=union there is questionable. In an
ideal world, the GitHub for Windows folks would ship a specialized merge
helper for handling VS project files. It can be open-source and
distributed separately for people who don't use GitHub, but they can
integrate it seamlessly into the GitHub client. So everybody wins.

I see you've already written to GitHub support; thanks. I'll make sure
your issue gets routed to the right people, and I'll see if I can
convince them to write the specialized tool. :)

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: auto merge bug

2013-03-05 Thread Jeff King
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   DependentUponSlovaricaForm.cs/DependentUpon
 81+Compile Include=WebCamForm.cs
 82+  SubTypeForm/SubType
 83+/Compile
 84+Compile Include=WebCamForm.Designer.cs
 85+  DependentUponWebCamForm.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/e3e5fa4b60b793b2a8c44330312755b72f93

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..000
  --- 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
   +  SubTypeForm/SubType
   +/Compile
   +Compile Include=MainForm.Designer.cs
   +  DependentUponMainForm.cs/DependentUpon
   +/Compile
   +Compile Include=SlovaricaForm.cs
   +  SubTypeForm/SubType
   +/Compile
   +Compile Include=SlovaricaForm.Designer.cs
   +  DependentUponSlovaricaForm.cs/DependentUpon
  ++===
  + Compile Include=WebCamForm.cs
  +   SubTypeForm/SubType
  + /Compile
  + Compile Include=WebCamForm.Designer.cs
  +   DependentUponWebCamForm.cs/DependentUpon
  ++ bd1a059
/Compile
Compile Include=WordsSelectForm.cs
  SubTypeForm/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
--
To unsubscribe from this list: send the line 

Re: auto merge bug

2013-03-05 Thread Jeff King
On Tue, Mar 05, 2013 at 04:03:26AM -0500, Jeff King wrote:

 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 },
 };
 
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: auto merge bug

2013-03-05 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 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).

The original use-case example of union was to merge two shopping
lists (e.g. I add bread and orange juice to remind me that we
need to buy these things, while my wife adds bread and butter).

We do not necessarily want to end up with a shopping list to buy two
loaves of bread.  When the user verifies and fixes up the result, we
can keep the current behaviour and those who want to re-dup can add
one back, or we can change the behaviour to leave the duplicates and
those who do not want to see duplicates can remove them manually.

Given that the caveat you quoted already tells the user to verify
the result and not to use it without understanding its implications,
I think it technically is fine either way (read: keeping duplicates
is not a clearly superiour solution). So let's leave it as-is.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: auto merge bug

2013-03-05 Thread Jeff King
On Tue, Mar 05, 2013 at 07:44:13AM -0800, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  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).
 
 The original use-case example of union was to merge two shopping
 lists (e.g. I add bread and orange juice to remind me that we
 need to buy these things, while my wife adds bread and butter).
 
 We do not necessarily want to end up with a shopping list to buy two
 loaves of bread.  When the user verifies and fixes up the result, we
 can keep the current behaviour and those who want to re-dup can add
 one back, or we can change the behaviour to leave the duplicates and
 those who do not want to see duplicates can remove them manually.
 
 Given that the caveat you quoted already tells the user to verify
 the result and not to use it without understanding its implications,
 I think it technically is fine either way (read: keeping duplicates
 is not a clearly superiour solution). So let's leave it as-is.

My problem with the current behavior is that it is not predictable
whether it will de-dup or not. If your shopping lists are:

  bread
  orange juice

  bread
  butter

it works; you get only one bread. If they are:

  milk
  bread
  orange juice

  beer
  bread
  butter

you get two. It depends on the exact behavior of the XDL_MERGE_ZEALOUS
flag. What I'd propose is two different drivers:

  1. Find conflicts via 3-way merge, and include both sides of the
 conflict verbatim. Do not use XDL_MERGE_ZEALOUS, as it is more
 important to retain items from both sides (in their original order)
 than it is to remove duplicates.

  2. A true line-based union, which should act like cat $ours $theirs |
 sort | uniq. That is what you want for the shopping list example,
 I think (you could also preserve existing ordering with a lookup
 table, though I prefer clobbering the ordering; the ordering of
 resolved conflicts will be arbitrary anyway, so it makes it clear
 from the outset that you should not use this driver if your content
 is not really a set (in the mathematical sense) of lines).

 You could also have sets of other objects (e.g., blank-line
 delimited paragraphs, changelog entries, etc). But you would need
 some way to specify the parsing then[1].

I'm not sure which should be called union. The first one would still
need careful examination of the result. The second one should always be
correct, but only because it is limited to a much more constrained
problem.

I'm also not sure how useful those really are in practice. I have not
used union myself ever. And in the example that started this thread, I
find the use of union slightly dubious. I do not even know how it
would react to a line _changing_, or other complicated edit. Short of a
specialized XML-aware merge driver, using XDL_MERGE_ZEALOUS and kicking
the result out to the user (i.e., what the default merge driver does)
seems like the only sane thing, even if it is more work at merge time.

-Peff

[1] Some of this is fairly easy to do with perl one-liners (e.g., perl
   -00 -ne 'print unless $h{$_}++ for paragraph mode), so maybe it is
   just an education/documentation issue. I dunno. I have always been
   happy enough with the stock merge.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: auto merge bug

2013-03-05 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 I'm also not sure how useful those really are in practice. I have not
 used union myself ever. And in the example that started this thread, I
 find the use of union slightly dubious.

Yeah, I do not think anybody sane used union outside toy examples.
IIRC, it was originally done as a if you want a GIGO, here it is,
go hang yourself. response to I am too lazy to resolve conflicts
myself, Git should let me take both sides blindly.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: auto merge bug

2013-03-05 Thread Andreas Ericsson
On 03/05/2013 07:47 PM, Junio C Hamano wrote:
 Jeff King p...@peff.net writes:
 
 I'm also not sure how useful those really are in practice. I have not
 used union myself ever. And in the example that started this thread, I
 find the use of union slightly dubious.
 
 Yeah, I do not think anybody sane used union outside toy examples.

I do, for lists used in tests or to generate perfect hashes from. It's
really quite handy for things like that but totally useless for any
type of multiline format, or even .ini style files unless you're very,
very careful with how you write them.

-- 
Andreas Ericsson   andreas.erics...@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html