{"thread":{"id":"31472","subject":"Binary file-friendly merge -Xours or -Xtheirs?","startedAt":"2012-09-07T14:48:48Z","lastAt":"2012-09-12T17:50:36Z","messageCount":12,"participants":["Stephen Bash","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"198526","messageId":"1265473388.275309.1347029328407.JavaMail.root@genarts.com","threadId":"31472","inReplyTo":"1799969825.275125.1347028392272.JavaMail.root@genarts.com","subject":"Binary file-friendly merge -Xours or -Xtheirs?","fromName":"Stephen Bash","fromEmail":"bash@genarts.com","sentAt":"2012-09-07T14:48:48Z","receivedAt":"2012-09-07T14:48:48Z","isPatch":false,"sender":{"key":"bash@genarts.com","avatar":null},"body":"Hi all-\n\nHelping a coworker resolve merge conflicts today I found I wanted a -Xtheirs that completely replaces conflicted binary files with the copy from the incoming branch.  In other words rather than doing\n\n  $ git merge maint\n  ... conflicts occur ...\n  $ git checkout --theirs -- path/to/binary-file\n  $ git add path/to/binary-file\n  $ git commit\n\nI'd like to do\n\n  $ git merge -Xbinary_theirs maint\n  ... binary conflicts are resolved automatically ...\n\n>From reading the docs it's obvious the current -Xours and -Xtheirs expect to work on hunks, so I (mostly) understand the current behavior, but as a user it feels like \"I'm telling you how to resolve conflicts, please do the same thing for binary files\".\n\nI am missing a solution that already exists?  Or is this perhaps an improvement that could be made?\n\nThanks,\nStephen\n"},{"id":"198556","messageId":"7v392twlnm.fsf@alter.siamese.dyndns.org","threadId":"31472","inReplyTo":"1265473388.275309.1347029328407.JavaMail.root@genarts.com","subject":"Re: Binary file-friendly merge -Xours or -Xtheirs?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-07T21:47:09Z","receivedAt":"2012-09-07T21:47:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Bash <bash@genarts.com> writes:\n\n> From reading the docs it's obvious the current -Xours and -Xtheirs\n> expect to work on hunks, so I (mostly) understand the current\n> behavior, but as a user it feels like \"I'm telling you how to\n> resolve conflicts, please do the same thing for binary files\".\n\nEven though merge-recursive accepts -Xours/-Xtheirs, I do not think\nthe low-level merge machinery for anything but the text merge is\naware of the option.\n\nIt may be just the matter of something like this, though (completely\nuntested).\n\n\n\n ll-merge.c | 25 ++++++++++++++++++++-----\n 1 file changed, 20 insertions(+), 5 deletions(-)\n\ndiff --git i/ll-merge.c w/ll-merge.c\nindex f3f7692..fee578f 100644\n--- i/ll-merge.c\n+++ w/ll-merge.c\n@@ -46,16 +46,31 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n \tassert(opts);\n \n \t/*\n-\t * The tentative merge result is \"ours\" for the final round,\n-\t * or common ancestor for an internal merge.  Still return\n-\t * \"conflicted merge\" status.\n+\t * The tentative merge result is the or common ancestor for an internal merge.\n \t */\n-\tstolen = opts->virtual_ancestor ? orig : src1;\n+\tif (opts->virtual_ancestor) {\n+\t\tstolen = orig;\n+\t} else {\n+\t\tswitch (opts->variant) {\n+\t\tdefault:\n+\t\tcase XDL_MERGE_FAVOR_OURS:\n+\t\t\tstolen = src1;\n+\t\t\tbreak;\n+\t\tcase XDL_MERGE_FAVOR_THEIRS:\n+\t\t\tstolen = src2;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n \n \tresult->ptr = stolen->ptr;\n \tresult->size = stolen->size;\n \tstolen->ptr = NULL;\n-\treturn 1;\n+\n+\t/*\n+\t * With -Xtheirs or -Xours, we have cleanly merged;\n+\t * otherwise we got a conflict.\n+\t */\n+\treturn (opts->variant ? 0 : 1);\n }\n \n static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n"},{"id":"198588","messageId":"1347165639-12149-1-git-send-email-gitster@pobox.com","threadId":"31472","inReplyTo":"7v392twlnm.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/2] Teaching -Xours/-Xtheirs to binary ll-merge driver","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-09T04:40:37Z","receivedAt":"2012-09-09T04:40:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The part that grants Stephen's wish is unchanged from the earlier\n\"perhaps like this\" patch, but this time with a bit of documentation\nand test.\n\nA more important change between the two actually is [PATCH 2/2].\nWhen a \"binary\" synthetic attribute is given to a path, we used to\n(1) disable textual diff and (2) disable CR/LF conversion, but it is\nalso sane to disable the textual merge for a path marked as\n\"binary\", and setting the \"-merge\" attribute to summon the \"binary\"\nll-merge driver is the way to do so.\n\nJunio C Hamano (2):\n  merge: teach -Xours/-Xtheirs to binary ll-merge driver\n  attr: \"binary\" attribute should choose built-in \"binary\" merge driver\n\n Documentation/gitattributes.txt    |  2 +-\n Documentation/merge-strategies.txt |  3 ++-\n attr.c                             |  2 +-\n ll-merge.c                         | 25 ++++++++++++++++++++-----\n t/t6037-merge-ours-theirs.sh       | 14 +++++++++++++-\n 5 files changed, 37 insertions(+), 9 deletions(-)\n\n-- \n1.7.12.322.g2c7d289\n"},{"id":"198589","messageId":"1347165639-12149-2-git-send-email-gitster@pobox.com","threadId":"31472","inReplyTo":"1347165639-12149-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/2] merge: teach -Xours/-Xtheirs to binary ll-merge driver","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-09T04:40:38Z","receivedAt":"2012-09-09T04:40:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The (discouraged) -Xours/-Xtheirs modes of merge are supposed to\ngive a quick and dirty way to come up with a random mixture of\ncleanly merged parts and punted conflict resolution to take contents\nfrom one side in conflicting parts.  These options however were only\npassed down to the low level merge driver for text.\n\nTeach the built-in binary merge driver to notice them as well.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/merge-strategies.txt |  3 ++-\n ll-merge.c                         | 25 ++++++++++++++++++++-----\n t/t6037-merge-ours-theirs.sh       | 14 +++++++++++++-\n 3 files changed, 35 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt\nindex 595a3cf..66db802 100644\n--- a/Documentation/merge-strategies.txt\n+++ b/Documentation/merge-strategies.txt\n@@ -32,13 +32,14 @@ ours;;\n \tThis option forces conflicting hunks to be auto-resolved cleanly by\n \tfavoring 'our' version.  Changes from the other tree that do not\n \tconflict with our side are reflected to the merge result.\n+\tFor a binary file, the entire contents are taken from our side.\n +\n This should not be confused with the 'ours' merge strategy, which does not\n even look at what the other tree contains at all.  It discards everything\n the other tree did, declaring 'our' history contains all that happened in it.\n \n theirs;;\n-\tThis is opposite of 'ours'.\n+\tThis is the opposite of 'ours'.\n \n patience;;\n \tWith this option, 'merge-recursive' spends a little extra time\ndiff --git a/ll-merge.c b/ll-merge.c\nindex da59738..8535e2d 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -46,16 +46,31 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n \tassert(opts);\n \n \t/*\n-\t * The tentative merge result is \"ours\" for the final round,\n-\t * or common ancestor for an internal merge.  Still return\n-\t * \"conflicted merge\" status.\n+\t * The tentative merge result is the or common ancestor for an internal merge.\n \t */\n-\tstolen = opts->virtual_ancestor ? orig : src1;\n+\tif (opts->virtual_ancestor) {\n+\t\tstolen = orig;\n+\t} else {\n+\t\tswitch (opts->variant) {\n+\t\tdefault:\n+\t\tcase XDL_MERGE_FAVOR_OURS:\n+\t\t\tstolen = src1;\n+\t\t\tbreak;\n+\t\tcase XDL_MERGE_FAVOR_THEIRS:\n+\t\t\tstolen = src2;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n \n \tresult->ptr = stolen->ptr;\n \tresult->size = stolen->size;\n \tstolen->ptr = NULL;\n-\treturn 1;\n+\n+\t/*\n+\t * With -Xtheirs or -Xours, we have cleanly merged;\n+\t * otherwise we got a conflict.\n+\t */\n+\treturn (opts->variant ? 0 : 1);\n }\n \n static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\ndiff --git a/t/t6037-merge-ours-theirs.sh b/t/t6037-merge-ours-theirs.sh\nindex 2cf42c7..8d05671 100755\n--- a/t/t6037-merge-ours-theirs.sh\n+++ b/t/t6037-merge-ours-theirs.sh\n@@ -53,7 +53,19 @@ test_expect_success 'recursive favouring ours' '\n \t! grep 1 file\n '\n \n-test_expect_success 'pull with -X' '\n+test_expect_success 'binary file with -Xours/-Xtheirs' '\n+\techo \"file -merge\" >.gitattributes &&\n+\n+\tgit reset --hard master &&\n+\tgit merge -s recursive -X theirs side &&\n+\tgit diff --exit-code side HEAD -- file &&\n+\n+\tgit reset --hard master &&\n+\tgit merge -s recursive -X ours side &&\n+\tgit diff --exit-code master HEAD -- file\n+'\n+\n+test_expect_success 'pull passes -X to underlying merge' '\n \tgit reset --hard master && git pull -s recursive -Xours . side &&\n \tgit reset --hard master && git pull -s recursive -X ours . side &&\n \tgit reset --hard master && git pull -s recursive -Xtheirs . side &&\n-- \n1.7.12.322.g2c7d289\n"},{"id":"198590","messageId":"1347165639-12149-3-git-send-email-gitster@pobox.com","threadId":"31472","inReplyTo":"1347165639-12149-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/2] attr: \"binary\" attribute should choose built-in \"binary\" merge driver","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-09T04:40:39Z","receivedAt":"2012-09-09T04:40:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The built-in \"binary\" attribute macro expands to \"-diff -text\", so\nthat textual diff is not produced, and the contents will not go\nthrough any CR/LF conversion ever.  During a merge, it should also\nchoose the \"binary\" low-level merge driver, but it didn't.\n\nMake it expand to \"-diff -merge -text\".\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/gitattributes.txt | 2 +-\n attr.c                          | 2 +-\n t/t6037-merge-ours-theirs.sh    | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex a85b187..ead7254 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -904,7 +904,7 @@ file at the toplevel (i.e. not in any subdirectory).  The built-in\n macro attribute \"binary\" is equivalent to:\n \n ------------\n-[attr]binary -diff -text\n+[attr]binary -diff -merge -text\n ------------\n \n \ndiff --git a/attr.c b/attr.c\nindex 303751f..3f581b3 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -306,7 +306,7 @@ static void free_attr_elem(struct attr_stack *e)\n }\n \n static const char *builtin_attr[] = {\n-\t\"[attr]binary -diff -text\",\n+\t\"[attr]binary -diff -merge -text\",\n \tNULL,\n };\n \ndiff --git a/t/t6037-merge-ours-theirs.sh b/t/t6037-merge-ours-theirs.sh\nindex 8d05671..3889eca 100755\n--- a/t/t6037-merge-ours-theirs.sh\n+++ b/t/t6037-merge-ours-theirs.sh\n@@ -54,7 +54,7 @@ test_expect_success 'recursive favouring ours' '\n '\n \n test_expect_success 'binary file with -Xours/-Xtheirs' '\n-\techo \"file -merge\" >.gitattributes &&\n+\techo file binary >.gitattributes &&\n \n \tgit reset --hard master &&\n \tgit merge -s recursive -X theirs side &&\n-- \n1.7.12.322.g2c7d289\n"},{"id":"198630","messageId":"545618054.286621.1347201018375.JavaMail.root@genarts.com","threadId":"31472","inReplyTo":"1347165639-12149-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 0/2] Teaching -Xours/-Xtheirs to binary ll-merge driver","fromName":"Stephen Bash","fromEmail":"bash@genarts.com","sentAt":"2012-09-09T14:30:18Z","receivedAt":"2012-09-09T14:30:18Z","isPatch":true,"sender":{"key":"bash@genarts.com","avatar":null},"body":"----- Original Message -----\n> From: \"Junio C Hamano\" <gitster@pobox.com>\n> Sent: Sunday, September 9, 2012 12:40:37 AM\n> Subject: [PATCH 0/2] Teaching -Xours/-Xtheirs to binary ll-merge driver\n> \n> The part that grants Stephen's wish is unchanged from the earlier\n> \"perhaps like this\" patch, but this time with a bit of documentation\n> and test.\n> \n> A more important change between the two actually is [PATCH 2/2].\n> When a \"binary\" synthetic attribute is given to a path, we used to\n> (1) disable textual diff and (2) disable CR/LF conversion, but it is\n> also sane to disable the textual merge for a path marked as\n> \"binary\", and setting the \"-merge\" attribute to summon the \"binary\"\n> ll-merge driver is the way to do so.\n> \n> Junio C Hamano (2):\n>   merge: teach -Xours/-Xtheirs to binary ll-merge driver\n>   attr: \"binary\" attribute should choose built-in \"binary\" merge\n>   driver\n> \n>  Documentation/gitattributes.txt    |  2 +-\n>  Documentation/merge-strategies.txt |  3 ++-\n>  attr.c                             |  2 +-\n>  ll-merge.c                         | 25 ++++++++++++++++++++-----\n>  t/t6037-merge-ours-theirs.sh       | 14 +++++++++++++-\n>  5 files changed, 37 insertions(+), 9 deletions(-)\n\nThanks for this Junio.  After figuring out how to make the corporate email server NOT munge the patches, I was able to apply these with no problem.  Then \n\n  $ git merge -Xtheirs maint\n\nmostly does what I want, but conflicting files deleted on the incoming branch are still marked unmerged/\"deleted by them\" (binary or not).  I'm not sure what the best (most common?) resolution is for that.  In my case I would be happy for the -Xtheirs to also delete the files, but that might not always be the right answer (but then again -Xtheirs is a pretty heavy hammer to begin with).\n\nThanks,\nStephen\n"},{"id":"198674","messageId":"20120910140317.GA7906@sigill.intra.peff.net","threadId":"31472","inReplyTo":"1347165639-12149-3-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 2/2] attr: \"binary\" attribute should choose built-in \"binary\" merge driver","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-10T14:03:17Z","receivedAt":"2012-09-10T14:03:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 08, 2012 at 09:40:39PM -0700, Junio C Hamano wrote:\n\n> The built-in \"binary\" attribute macro expands to \"-diff -text\", so\n> that textual diff is not produced, and the contents will not go\n> through any CR/LF conversion ever.  During a merge, it should also\n> choose the \"binary\" low-level merge driver, but it didn't.\n> \n> Make it expand to \"-diff -merge -text\".\n\nYeah, that seems like the obviously correct thing to do. In practice,\nmost files would end up in the first few lines of ll_xdl_merge checking\nbuffer_is_binary anyway, so I think this would really only make a\ndifference when our \"is it binary?\" heuristic guesses wrong.\n\n>  Documentation/gitattributes.txt | 2 +-\n>  attr.c                          | 2 +-\n>  t/t6037-merge-ours-theirs.sh    | 2 +-\n>  3 files changed, 3 insertions(+), 3 deletions(-)\n\nPatch itself looks good to me.\n\n-Peff\n"},{"id":"198817","messageId":"7vk3vzfwme.fsf@alter.siamese.dyndns.org","threadId":"31472","inReplyTo":"20120910140317.GA7906@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] attr: \"binary\" attribute should choose built-in \"binary\" merge driver","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-12T08:55:53Z","receivedAt":"2012-09-12T08:55:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Sep 08, 2012 at 09:40:39PM -0700, Junio C Hamano wrote:\n>\n>> The built-in \"binary\" attribute macro expands to \"-diff -text\", so\n>> that textual diff is not produced, and the contents will not go\n>> through any CR/LF conversion ever.  During a merge, it should also\n>> choose the \"binary\" low-level merge driver, but it didn't.\n>> \n>> Make it expand to \"-diff -merge -text\".\n>\n> Yeah, that seems like the obviously correct thing to do. In practice,\n> most files would end up in the first few lines of ll_xdl_merge checking\n> buffer_is_binary anyway, so I think this would really only make a\n> difference when our \"is it binary?\" heuristic guesses wrong.\n\nYou made me look at that part again and then made me notice\nsomething unrelated.\n\n\tif (buffer_is_binary(orig->ptr, orig->size) ||\n\t    buffer_is_binary(src1->ptr, src1->size) ||\n\t    buffer_is_binary(src2->ptr, src2->size)) {\n\t\twarning(\"Cannot merge binary files: %s (%s vs. %s)\",\n\t\t\tpath, name1, name2);\n\t\treturn ll_binary_merge(drv_unused, result,\n\t\t\t\t       path,\n\t\t\t\t       orig, orig_name,\n\t\t\t\t       src1, name1,\n\t\t\t\t       src2, name2,\n\t\t\t\t       opts, marker_size);\n\t}\n\nGiven that we now may know how to merge these things, the\nunconditional warning feels very wrong.\n\nPerhaps something like this makes it better.\n\nA path that is explicitly marked as binary did not get any such\nwarning, but it will start to get warned just like a path that was\nauto-detected to be a binary.\n\nIt is a behaviour change, but I think it is a good one that makes\ntwo cases more consistent.\n\nAnd we won't see the warning when -Xtheirs/-Xours large sledgehammer\nis in use, which tells us how to resolve these things \"cleanly\".\n\n ll-merge.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git i/ll-merge.c w/ll-merge.c\nindex 8535e2d..307315b 100644\n--- i/ll-merge.c\n+++ w/ll-merge.c\n@@ -35,7 +35,7 @@ struct ll_merge_driver {\n  */\n static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n \t\t\t   mmbuffer_t *result,\n-\t\t\t   const char *path_unused,\n+\t\t\t   const char *path,\n \t\t\t   mmfile_t *orig, const char *orig_name,\n \t\t\t   mmfile_t *src1, const char *name1,\n \t\t\t   mmfile_t *src2, const char *name2,\n@@ -53,6 +53,9 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n \t} else {\n \t\tswitch (opts->variant) {\n \t\tdefault:\n+\t\t\twarning(\"Cannot merge binary files: %s (%s vs. %s)\\n\",\n+\t\t\t\tpath, name1, name2);\n+\t\t\t/* fallthru */\n \t\tcase XDL_MERGE_FAVOR_OURS:\n \t\t\tstolen = src1;\n \t\t\tbreak;\n@@ -88,8 +91,6 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n \tif (buffer_is_binary(orig->ptr, orig->size) ||\n \t    buffer_is_binary(src1->ptr, src1->size) ||\n \t    buffer_is_binary(src2->ptr, src2->size)) {\n-\t\twarning(\"Cannot merge binary files: %s (%s vs. %s)\\n\",\n-\t\t\tpath, name1, name2);\n \t\treturn ll_binary_merge(drv_unused, result,\n \t\t\t\t       path,\n \t\t\t\t       orig, orig_name,\n"},{"id":"198831","messageId":"1734879571.321704.1347454681726.JavaMail.root@genarts.com","threadId":"31472","inReplyTo":"7vk3vzfwme.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] attr: \"binary\" attribute should choose built-in \"binary\" merge driver","fromName":"Stephen Bash","fromEmail":"bash@genarts.com","sentAt":"2012-09-12T12:58:01Z","receivedAt":"2012-09-12T12:58:01Z","isPatch":true,"sender":{"key":"bash@genarts.com","avatar":null},"body":"----- Original Message -----\n> From: \"Junio C Hamano\" <gitster@pobox.com>\n> Sent: Wednesday, September 12, 2012 4:55:53 AM\n> Subject: Re: [PATCH 2/2] attr: \"binary\" attribute should choose built-in \"binary\" merge driver\n> \n> Jeff King <peff@peff.net> writes:\n> \n> > On Sat, Sep 08, 2012 at 09:40:39PM -0700, Junio C Hamano wrote:\n> >\n> >> The built-in \"binary\" attribute macro expands to \"-diff -text\", so\n> >> that textual diff is not produced, and the contents will not go\n> >> through any CR/LF conversion ever.  During a merge, it should also\n> >> choose the \"binary\" low-level merge driver, but it didn't.\n> >> \n> >> Make it expand to \"-diff -merge -text\".\n> >\n> > Yeah, that seems like the obviously correct thing to do. In\n> > practice,\n> > most files would end up in the first few lines of ll_xdl_merge\n> > checking\n> > buffer_is_binary anyway, so I think this would really only make a\n> > difference when our \"is it binary?\" heuristic guesses wrong.\n> \n> You made me look at that part again and then made me notice\n> something unrelated.\n> \n> \tif (buffer_is_binary(orig->ptr, orig->size) ||\n> \t    buffer_is_binary(src1->ptr, src1->size) ||\n> \t    buffer_is_binary(src2->ptr, src2->size)) {\n> \t\twarning(\"Cannot merge binary files: %s (%s vs. %s)\",\n> \t\t\tpath, name1, name2);\n> \t\treturn ll_binary_merge(drv_unused, result,\n> \t\t\t\t       path,\n> \t\t\t\t       orig, orig_name,\n> \t\t\t\t       src1, name1,\n> \t\t\t\t       src2, name2,\n> \t\t\t\t       opts, marker_size);\n> \t}\n> \n> Given that we now may know how to merge these things, the\n> unconditional warning feels very wrong.\n> \n> Perhaps something like this makes it better.\n\nPatch didn't apply on top of the previous two for me, but after making the edits manually does what it claims to do (and makes the merge output much nicer to read, thanks!).  The only remaining question for me is should -Xtheirs resolve \"deleted by them\" conflicts?\n\nThanks,\nStephen\n"},{"id":"198833","messageId":"20120912131702.GA13710@sigill.intra.peff.net","threadId":"31472","inReplyTo":"7vk3vzfwme.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] attr: \"binary\" attribute should choose built-in \"binary\" merge driver","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-12T13:17:02Z","receivedAt":"2012-09-12T13:17:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 12, 2012 at 01:55:53AM -0700, Junio C Hamano wrote:\n\n> > Yeah, that seems like the obviously correct thing to do. In practice,\n> > most files would end up in the first few lines of ll_xdl_merge checking\n> > buffer_is_binary anyway, so I think this would really only make a\n> > difference when our \"is it binary?\" heuristic guesses wrong.\n> \n> You made me look at that part again and then made me notice\n> something unrelated.\n> \n> \tif (buffer_is_binary(orig->ptr, orig->size) ||\n> \t    buffer_is_binary(src1->ptr, src1->size) ||\n> \t    buffer_is_binary(src2->ptr, src2->size)) {\n> \t\twarning(\"Cannot merge binary files: %s (%s vs. %s)\",\n> \t\t\tpath, name1, name2);\n> \t\treturn ll_binary_merge(drv_unused, result,\n> \t\t\t\t       path,\n> \t\t\t\t       orig, orig_name,\n> \t\t\t\t       src1, name1,\n> \t\t\t\t       src2, name2,\n> \t\t\t\t       opts, marker_size);\n> \t}\n> \n> Given that we now may know how to merge these things, the\n> unconditional warning feels very wrong.\n> \n> Perhaps something like this makes it better.\n> \n> A path that is explicitly marked as binary did not get any such\n> warning, but it will start to get warned just like a path that was\n> auto-detected to be a binary.\n> \n> It is a behaviour change, but I think it is a good one that makes\n> two cases more consistent.\n> \n> And we won't see the warning when -Xtheirs/-Xours large sledgehammer\n> is in use, which tells us how to resolve these things \"cleanly\".\n\nYeah, I think it is the right thing to do. I noticed that the warning\nwould not trigger in the \"-merge\" case and wondered if it should, but\nfigured it was not a big deal either way.\n\nHowever, I agree it is very bad for it to trigger with -Xours/theirs,\nand that is worth fixing.  That it triggers in the \"-merge\" case\nafterwards is a slight bonus.\n\n-Peff\n"},{"id":"198855","messageId":"7vhar3dvkl.fsf@alter.siamese.dyndns.org","threadId":"31472","inReplyTo":"1734879571.321704.1347454681726.JavaMail.root@genarts.com","subject":"Re: [PATCH 2/2] attr: \"binary\" attribute should choose built-in \"binary\" merge driver","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-12T17:01:30Z","receivedAt":"2012-09-12T17:01:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Bash <bash@genarts.com> writes:\n\n>> Perhaps something like this makes it better.\n>\n> Patch didn't apply on top of the previous two for me,...\n\nLook at 'pu' and see how it applies.\n\n> ...  The only remaining\n> question for me is should -Xtheirs resolve \"deleted by them\"\n> conflicts?\n\nI do not know, and I do not care to worry about it too deeply, which\nmeans I would rather err on the safe side and have users inspect the\nsituation, make the decision when such a conflict happens and\nresolve them themselves, instead of claiming a clean resolution that\nis possibly wrong.\n"},{"id":"198859","messageId":"1041293807.327912.1347472236069.JavaMail.root@genarts.com","threadId":"31472","inReplyTo":"7vhar3dvkl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] attr: \"binary\" attribute should choose built-in \"binary\" merge driver","fromName":"Stephen Bash","fromEmail":"bash@genarts.com","sentAt":"2012-09-12T17:50:36Z","receivedAt":"2012-09-12T17:50:36Z","isPatch":true,"sender":{"key":"bash@genarts.com","avatar":null},"body":"----- Original Message -----\n> From: \"Junio C Hamano\" <gitster@pobox.com>\n> Sent: Wednesday, September 12, 2012 1:01:30 PM\n> Subject: Re: [PATCH 2/2] attr: \"binary\" attribute should choose built-in \"binary\" merge driver\n> \n> >> Perhaps something like this makes it better.\n> >\n> > Patch didn't apply on top of the previous two for me,...\n> \n> Look at 'pu' and see how it applies.\n\nAh, that seems to work better.\n \n> > ...  The only remaining\n> > question for me is should -Xtheirs resolve \"deleted by them\"\n> > conflicts?\n> \n> I do not know, and I do not care to worry about it too deeply, which\n> means I would rather err on the safe side and have users inspect the\n> situation, make the decision when such a conflict happens and\n> resolve them themselves, instead of claiming a clean resolution that\n> is possibly wrong.\n\nA reasonable approach.  Thanks for clarifying.\n\nStephen\n"}]}