{"thread":{"id":"55919","subject":"[PATCH] xdiff: implement a zealous diff3","startedAt":"2021-06-13T14:36:27Z","lastAt":"2021-06-15T16:59:19Z","messageCount":14,"participants":["Felipe Contreras","Jeff King","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"427236","messageId":"20210613143155.836591-1-felipe.contreras@gmail.com","threadId":"55919","inReplyTo":null,"subject":"[PATCH] xdiff: implement a zealous diff3","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-13T14:31:55Z","receivedAt":"2021-06-13T14:36:27Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"From: Uwe Kleine-König \n\n\"zdiff3\" is identical to ordinary diff3, only it allows more aggressive\ncompaction than diff3. This way the displayed base isn't necessary\ntechnically correct, but still this mode might help resolving merge\nconflicts between two near identical additions.\n\nSigned-off-by: Uwe Kleine-König \n---\n\nI'm re-sending this patch from 2013 because I do think it provides value\nand we might want to make it the default.\n\nI hardcoded diff3 to be zdiff3 and all the tests passed, so if our tests\ndon't care about the simplification level of diff3 perhaps many (or even\nmost) our users don't either.\n\nFTR: this patch applied cleanly.\n\n builtin/merge-file.c | 2 ++\n contrib/completion/git-completion.bash | 2 +-\n xdiff-interface.c | 2 ++\n xdiff/xdiff.h | 1 +\n xdiff/xmerge.c | 8 +++++++-\n 5 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 06a2f90c48..e695867ee5 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -34,6 +34,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_BOOL('p', \"stdout\", &to_stdout, N_(\"send results to standard output\")),\n \t\tOPT_SET_INT(0, \"diff3\", &xmp.style, N_(\"use a diff3 based merge\"), XDL_MERGE_DIFF3),\n+\t\tOPT_SET_INT(0, \"zdiff3\", &xmp.style, N_(\"use a zealous diff3 based merge\"),\n+\t\t\t\tXDL_MERGE_ZEALOUS_DIFF3),\n \t\tOPT_SET_INT(0, \"ours\", &xmp.favor, N_(\"for conflicts, use our version\"),\n \t\t\t XDL_MERGE_FAVOR_OURS),\n \t\tOPT_SET_INT(0, \"theirs\", &xmp.favor, N_(\"for conflicts, use their version\"),\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex b50c5d0ea3..8594559298 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1566,7 +1566,7 @@ _git_checkout ()\n \n \tcase \"$cur\" in\n \t--conflict=*)\n-\t\t__gitcomp \"diff3 merge\" \"\" \"${cur##--conflict=}\"\n+\t\t__gitcomp \"diff3 merge zdiff3\" \"\" \"${cur##--conflict=}\"\n \t\t;;\n \t--*)\n \t\t__gitcomp_builtin checkout\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 609615db2c..9977813a9d 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -308,6 +308,8 @@ int git_xmerge_config(const char *var, const char *value, void *cb)\n \t\t\tdie(\"'%s' is not a boolean\", var);\n \t\tif (!strcmp(value, \"diff3\"))\n \t\t\tgit_xmerge_style = XDL_MERGE_DIFF3;\n+\t\telse if (!strcmp(value, \"zdiff3\"))\n+\t\t\tgit_xmerge_style = XDL_MERGE_ZEALOUS_DIFF3;\n \t\telse if (!strcmp(value, \"merge\"))\n \t\t\tgit_xmerge_style = 0;\n \t\t/*\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 7a04605146..8629ae287c 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -65,6 +65,7 @@ extern \"C\" {\n \n /* merge output styles */\n #define XDL_MERGE_DIFF3 1\n+#define XDL_MERGE_ZEALOUS_DIFF3 2\n \n typedef struct s_mmfile {\n \tchar *ptr;\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex 1659edb453..95871a0b6e 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -230,7 +230,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tsize += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, 1,\n \t\t\t dest ? dest + size : NULL);\n \n-\tif (style == XDL_MERGE_DIFF3) {\n+\tif (style == XDL_MERGE_DIFF3 || style == XDL_MERGE_ZEALOUS_DIFF3) {\n \t\t/* Shared preimage */\n \t\tif (!dest) {\n \t\t\tsize += marker_size + 1 + needs_cr + marker3_size;\n@@ -482,6 +482,12 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n \tint style = xmp->style;\n \tint favor = xmp->favor;\n \n+\t/*\n+\t * This is the only change between XDL_MERGE_DIFF3 and\n+\t * XDL_MERGE_ZEALOUS_DIFF3. \"zdiff3\" isn't 100% technically correct (as\n+\t * the base might be considerably simplified), but still it might help\n+\t * interpreting conflicts between two big and near identical additions.\n+\t */\n \tif (style == XDL_MERGE_DIFF3) {\n \t\t/*\n \t\t * \"diff3 -m\" output does not make sense for anything\n-- \n2.32.0\n\n"},{"id":"427237","messageId":"YMYnVWSEgxvKRU9j@coredump.intra.peff.net","threadId":"55919","inReplyTo":"20210613143155.836591-1-felipe.contreras@gmail.com","subject":"Re: [PATCH] xdiff: implement a zealous diff3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-13T15:42:13Z","receivedAt":"2021-06-13T15:42:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 13, 2021 at 09:31:55AM -0500, Felipe Contreras wrote:\n\n> From: Uwe Kleine-König \n> \n> \"zdiff3\" is identical to ordinary diff3, only it allows more aggressive\n> compaction than diff3. This way the displayed base isn't necessary\n> technically correct, but still this mode might help resolving merge\n> conflicts between two near identical additions.\n> \n> Signed-off-by: Uwe Kleine-König \n> ---\n> \n> I'm re-sending this patch from 2013 because I do think it provides value\n> and we might want to make it the default.\n\nI take it you didn't investigate the segfault I mentioned.\n\nTry this:\n\n commit=a5170794372cf1325710a3419473c91ec4af53bf\n for style in merge diff3 zdiff3; do\n git reset --hard\n git checkout $commit^1\n git -c merge.conflictstyle=$style merge $commit^2\n done\n\nThe first two are fine; the zdiff3 one segfaults within the xmerge.c\ncode.\n\n-Peff\n"},{"id":"427244","messageId":"60c647c1d9b5c_41f452089@natae.notmuch","threadId":"55919","inReplyTo":"YMYnVWSEgxvKRU9j@coredump.intra.peff.net","subject":"Re: [PATCH] xdiff: implement a zealous diff3","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-13T18:00:33Z","receivedAt":"2021-06-13T18:03:34Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Sun, Jun 13, 2021 at 09:31:55AM -0500, Felipe Contreras wrote:\n> \n> > From: Uwe Kleine-König \n> > \n> > \"zdiff3\" is identical to ordinary diff3, only it allows more aggressive\n> > compaction than diff3. This way the displayed base isn't necessary\n> > technically correct, but still this mode might help resolving merge\n> > conflicts between two near identical additions.\n> > \n> > Signed-off-by: Uwe Kleine-König \n> > ---\n> > \n> > I'm re-sending this patch from 2013 because I do think it provides value\n> > and we might want to make it the default.\n> \n> I take it you didn't investigate the segfault I mentioned.\n\nI don't know how I was supposed to investigate the few segfaults you\nmentioned. All you said is that you never tracked the bug.\n\n> Try this:\n> \n> commit=a5170794372cf1325710a3419473c91ec4af53bf\n> for style in merge diff3 zdiff3; do\n> git reset --hard\n> git checkout $commit^1\n> git -c merge.conflictstyle=$style merge $commit^2\n> done\n> \n> The first two are fine; the zdiff3 one segfaults within the xmerge.c\n> code.\n\nI can reproduct the segfault, and here is a simpler way to reproduce it:\n\n(I have a hacked version of diff3 until merge-file learns how to use\nmerge.conflictstyle)\n\n cat >b <l <r < Jeff King wrote:\n> > Try this:\n> > \n> > commit=a5170794372cf1325710a3419473c91ec4af53bf\n> > for style in merge diff3 zdiff3; do\n> > git reset --hard\n> > git checkout $commit^1\n> > git -c merge.conflictstyle=$style merge $commit^2\n> > done\n> > \n> > The first two are fine; the zdiff3 one segfaults within the xmerge.c\n> > code.\n> \n> I can reproduct the segfault, and here is a simpler way to reproduce it:\n\nI found the problem, m->chg0 was not initialized in xdl_refine_conflicts.\n\nI'm not familiar with the area so I don't know if the following makes\nsense, but it fixes the crash:\n\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -333,7 +333,7 @@ static int xdl_refine_conflicts(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m,\n mmfile_t t1, t2;\n xdfenv_t xe;\n xdchange_t *xscr, *x;\n- int i1 = m->i1, i2 = m->i2;\n+ int i0 = m->i0, i1 = m->i1, i2 = m->i2;\n \n /* let's handle just the conflicts */\n if (m->mode)\n@@ -384,6 +384,8 @@ static int xdl_refine_conflicts(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m,\n m->next = m2;\n m = m2;\n m->mode = 0;\n+ m->i0 = i0;\n+ m->chg0 = 0;\n m->i1 = xscr->i1 + i1;\n m->chg1 = xscr->chg1;\n m->i2 = xscr->i2 + i2;\n\n\n-- \nFelipe Contreras\n"},{"id":"427269","messageId":"YMbexfeUG78yBix4@coredump.intra.peff.net","threadId":"55919","inReplyTo":"60c647c1d9b5c_41f452089@natae.notmuch","subject":"Re: [PATCH] xdiff: implement a zealous diff3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-14T04:44:53Z","receivedAt":"2021-06-14T04:47:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 13, 2021 at 01:00:33PM -0500, Felipe Contreras wrote:\n\n> > > I'm re-sending this patch from 2013 because I do think it provides value\n> > > and we might want to make it the default.\n> > \n> > I take it you didn't investigate the segfault I mentioned.\n> \n> I don't know how I was supposed to investigate the few segfaults you\n> mentioned. All you said is that you never tracked the bug.\n\nMy point is that if you are going to repost a patch that has known\nproblems, it is worth saying so to give reviewers and the maintainer a\nrealistic idea of how stable it is.\n\nI also didn't have a reproduction recipe. I found the commit I sent by\njust re-running every merge in git.git in a loop.\n\nIt sounds like you have a smaller reproduction and maybe a fix, which is\ngood.\n\n-Peff\n"},{"id":"427390","messageId":"xmqq7divzxrr.fsf@gitster.g","threadId":"55919","inReplyTo":"60c677a2c2d24_f5651208cf@natae.notmuch","subject":"Re: [PATCH] xdiff: implement a zealous diff3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-15T02:07:04Z","receivedAt":"2021-06-15T02:24:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> I found the problem, m->chg0 was not initialized in xdl_refine_conflicts.\n>\n> I'm not familiar with the area so I don't know if the following makes\n> sense, but it fixes the crash:\n\nUnlike the remainder of the xdiff/ directory, xdiff/xmerge.c was\nDscho's brainchild if I am not mistaken, so I'm CCing him for\ninput.\n\n\n\n> --- a/xdiff/xmerge.c\n> +++ b/xdiff/xmerge.c\n> @@ -333,7 +333,7 @@ static int xdl_refine_conflicts(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m,\n> mmfile_t t1, t2;\n> xdfenv_t xe;\n> xdchange_t *xscr, *x;\n> - int i1 = m->i1, i2 = m->i2;\n> + int i0 = m->i0, i1 = m->i1, i2 = m->i2;\n> \n> /* let's handle just the conflicts */\n> if (m->mode)\n> @@ -384,6 +384,8 @@ static int xdl_refine_conflicts(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m,\n> m->next = m2;\n> m = m2;\n> m->mode = 0;\n> + m->i0 = i0;\n> + m->chg0 = 0;\n> m->i1 = xscr->i1 + i1;\n> m->chg1 = xscr->chg1;\n> m->i2 = xscr->i2 + i2;\n"},{"id":"427407","messageId":"CABPp-BFY7uU5Ugypv4xCHq+XHTc3UROWPdV1v-JbN7xBycDZTA@mail.gmail.com","threadId":"55919","inReplyTo":"xmqq7divzxrr.fsf@gitster.g","subject":"Re: [PATCH] xdiff: implement a zealous diff3","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-15T03:43:54Z","receivedAt":"2021-06-15T03:44:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jun 14, 2021 at 7:07 PM Junio C Hamano wrote:\n>\n> Felipe Contreras writes:\n>\n> > I found the problem, m->chg0 was not initialized in xdl_refine_conflicts.\n> >\n> > I'm not familiar with the area so I don't know if the following makes\n> > sense, but it fixes the crash:\n>\n> Unlike the remainder of the xdiff/ directory, xdiff/xmerge.c was\n> Dscho's brainchild if I am not mistaken, so I'm CCing him for\n> input.\n\nThis is going to sound harsh, but people shouldn't waste (any more)\ntime reviewing the patches in this thread or the \"merge: cleanups and\nfix\" series submitted elsewhere. They should all just be rejected.\n\nI do not think it is reasonable to expect reviewers to spend time\nresponding to re-posted patches when:\n * no attempt was made to make sure they were up-to-date with current\ncode beyond compiling (see below)\n * no attempt was made to address missing items pointed out in\nresponse to the original submission[1]\n * no attempt was made to handle or even test particular cases\npointed out in response to the original submission (see [1] and below)\n * the patches were posted despite knowing they caused segfaults, and\nwithout even stating as much![2]\n * the segfault \"fixes\" are submitted as a separate series from the\npatch introducing the segfault[3], raising the risk that one gets\npicked up without the other.\n\nIn my opinion, these submissions were egregiously cavalier. I'll\nsubmit a patch (or perhaps a few) soon that has a functioning zdiff3.\n\nHowever, since I've already put in the time to understand it, let me\nexplain what is wrong with this patch. This particular change is in\nthe area of the code that splits conflict regions when there are\nportions of the sides (not the base) that match. Doing such splitting\nmakes sense with \"merge\" conflictStyle since the base is never shown;\nthis splitting can allow pulling the common lines out of the conflict\nregion. However, with diff3 or zdiff3, the original text does not\nmatch the sides and by splitting the conflict region, we are forced to\ndecide how or where to split the original text among the various\nconflict (and non-conflict?) regions. This is pretty haphazard, and\nthe effect of this patch is to assign all of the original text to the\nfirst conflict region in the split, and make all other regions have\nempty base text.\n\nThis exact scenario was discussed by you and Peff back when zdiff3 was\noriginally introduced in the thread where Felipe got the patch that he\nstarted this thread with. In that thread, Peff explained how zdiff3\nshould only try to move common lines at the beginning or end of the\nconflict hunk outside the conflict region, without doing any splitting\nof the conflict region (this particular issue took about 1/3 to 1/2 of\nthe original thread, but I think [4] has a good hilight).\nAdditionally, a quick grep through the code showed that there are\nadditional places in bash/zsh completion that need to be fixed to use\nthe new option besides the locations modified in the original zdiff3\npatch. See [1] and [2] for various other things overlooked.\n\n\n[1] https://lore.kernel.org/git/CABPp-BGZ2H1MVgw9RvSdogLMdqsX3n89NkkDYDa2VM3TRHn7tg@mail.gmail.com/\n[2] https://lore.kernel.org/git/YMbexfeUG78yBix4@coredump.intra.peff.net/\n[3] https://lore.kernel.org/git/20210613225836.1009569-5-felipe.contreras@gmail.com/\n[4] https://lore.kernel.org/git/20130307180157.GA6604@sigill.intra.peff.net/\n"},{"id":"427410","messageId":"xmqqr1h3wz8j.fsf@gitster.g","threadId":"55919","inReplyTo":"CABPp-BFY7uU5Ugypv4xCHq+XHTc3UROWPdV1v-JbN7xBycDZTA@mail.gmail.com","subject":"Re: [PATCH] xdiff: implement a zealous diff3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-15T04:03:40Z","receivedAt":"2021-06-15T04:03:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> This is going to sound harsh, but people shouldn't waste (any more)\n> time reviewing the patches in this thread or the \"merge: cleanups and\n> fix\" series submitted elsewhere. They should all just be rejected.\n>\n> I do not think it is reasonable to expect reviewers to spend time\n> responding to re-posted patches when:\n> * no attempt was made to make sure they were up-to-date with current\n> code beyond compiling (see below)\n> * no attempt was made to address missing items pointed out in\n> response to the original submission[1]\n> * no attempt was made to handle or even test particular cases\n> pointed out in response to the original submission (see [1] and below)\n> * the patches were posted despite knowing they caused segfaults, and\n> without even stating as much![2]\n> * the segfault \"fixes\" are submitted as a separate series from the\n> patch introducing the segfault[3], raising the risk that one gets\n> picked up without the other.\n\nFair enough. Thanks.\n"},{"id":"427411","messageId":"60c82a622ae66_e5292087f@natae.notmuch","threadId":"55919","inReplyTo":"YMbexfeUG78yBix4@coredump.intra.peff.net","subject":"Re: [PATCH] xdiff: implement a zealous diff3","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-15T04:19:46Z","receivedAt":"2021-06-15T04:19:51Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Sun, Jun 13, 2021 at 01:00:33PM -0500, Felipe Contreras wrote:\n> \n> > > > I'm re-sending this patch from 2013 because I do think it provides value\n> > > > and we might want to make it the default.\n> > > \n> > > I take it you didn't investigate the segfault I mentioned.\n> > \n> > I don't know how I was supposed to investigate the few segfaults you\n> > mentioned. All you said is that you never tracked the bug.\n> \n> My point is that if you are going to repost a patch that has known\n> problems,\n\nIt was not known that it had problems.\n\nThat fact that person X said patch Y had a problem doesn't necessarily\nmean that patch Y has a problem.\n\n 1. The problem in the past might not apply in the present\n 2. The problem X person had might be specific to his/her setup\n 3. The problem might be due a combination of patches, not the patch\n itself\n\nPlus many others.\n\nA logical person sees evidence for what it is, and the only thing that\nperson X saying patch Y had a problem means, is that person X said patch\nY had a problem.\n\n-- \nFelipe Contreras\n"},{"id":"427427","messageId":"60c86ff87d598_e6332085b@natae.notmuch","threadId":"55919","inReplyTo":"CABPp-BFY7uU5Ugypv4xCHq+XHTc3UROWPdV1v-JbN7xBycDZTA@mail.gmail.com","subject":"Re: [PATCH] xdiff: implement a zealous diff3","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-15T09:16:40Z","receivedAt":"2021-06-15T09:16:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Elijah Newren wrote:\n> On Mon, Jun 14, 2021 at 7:07 PM Junio C Hamano wrote:\n> >\n> > Felipe Contreras writes:\n> >\n> > > I found the problem, m->chg0 was not initialized in xdl_refine_conflicts.\n> > >\n> > > I'm not familiar with the area so I don't know if the following makes\n> > > sense, but it fixes the crash:\n> >\n> > Unlike the remainder of the xdiff/ directory, xdiff/xmerge.c was\n> > Dscho's brainchild if I am not mistaken, so I'm CCing him for\n> > input.\n> \n> This is going to sound harsh, but people shouldn't waste (any more)\n> time reviewing the patches in this thread or the \"merge: cleanups and\n> fix\" series submitted elsewhere. They should all just be rejected.\n> \n> I do not think it is reasonable to expect reviewers to spend time\n> responding to re-posted patches when:\n> * no attempt was made to make sure they were up-to-date with current\n> code beyond compiling (see below)\n\nWhat makes you think so?\n\n> * no attempt was made to address missing items pointed out in\n> response to the original submission[1]\n\nThe original submission caused a discussion with no resolution, and\nedned with Jeff saying he wanted to try real use-cases and that that he\nwanted to use it in practice for a while.\n\nThe purpose v1 of this series was to respark the discussion and see if\nany of the original parties had changed their minds.\n\nPeople do change their minds after 8 years.\n\n> * no attempt was made to handle or even test particular cases\n> pointed out in response to the original submission (see [1] and below)\n\nThose were sent *after* the series, except [4], which clearly states the\n*opposite* of there being a deal-breaker:\n\n But again, we don't do this splitting now. So I don't think it's\n something that should make or break a decision to have zdiff3. Without\n the splitting, I can see it being quite useful.\n\n> * the patches were posted despite knowing they caused segfaults, and\n> without even stating as much![2]\n\nWhomever *knew* that, it wasn't me.\n\n> * the segfault \"fixes\" are submitted as a separate series from the\n> patch introducing the segfault[3], raising the risk that one gets\n> picked up without the other.\n\nMy v2 includes the patch.\n\nJust because a patch is in one series that doesn't preclude it from\nbeing in another series. `git merge` and `git rebase` are smart enough\nto handle such cases.\n\nThere is no risk of that happening (unless there's plans of merging v1\nas-is).\n\n> In my opinion, these submissions were egregiously cavalier.\n\nIf you make unwarranted assumptions everything is possible.\n\n> I'll submit a patch (or perhaps a few) soon that has a functioning\n> zdiff3.\n\nI already have a functioning zdiff3.\n\n> However, since I've already put in the time to understand it, let me\n> explain what is wrong with this patch. This particular change is in\n> the area of the code that splits conflict regions when there are\n> portions of the sides (not the base) that match. Doing such splitting\n> makes sense with \"merge\" conflictStyle since the base is never shown;\n> this splitting can allow pulling the common lines out of the conflict\n> region. However, with diff3 or zdiff3, the original text does not\n> match the sides and by splitting the conflict region, we are forced to\n> decide how or where to split the original text among the various\n> conflict (and non-conflict?) regions. This is pretty haphazard, and\n> the effect of this patch is to assign all of the original text to the\n> first conflict region in the split, and make all other regions have\n> empty base text.\n\nYes, that is *one* opinion. The jury is still out on what is the best\napproach. Junio and Jeff did not agree on that.\n\n\nThe whole point of \"zdiff3\" was to have something closer to \"merge\",\neven if it wasn't 100% correct. Your approach maybe more correct, but\ncorrectness was never the point.\n\nEither way, I have more rewarding things to focus on, so good luck with\nthat.\n\n> [1] https://lore.kernel.org/git/CABPp-BGZ2H1MVgw9RvSdogLMdqsX3n89NkkDYDa2VM3TRHn7tg@mail.gmail.com/\n> [2] https://lore.kernel.org/git/YMbexfeUG78yBix4@coredump.intra.peff.net/\n> [3] https://lore.kernel.org/git/20210613225836.1009569-5-felipe.contreras@gmail.com/\n> [4] https://lore.kernel.org/git/20130307180157.GA6604@sigill.intra.peff.net/\n\n-- \nFelipe Contreras\n"},{"id":"427428","messageId":"60c870c4a1bc1_e6332088f@natae.notmuch","threadId":"55919","inReplyTo":"xmqqr1h3wz8j.fsf@gitster.g","subject":"Re: [PATCH] xdiff: implement a zealous diff3","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-15T09:20:04Z","receivedAt":"2021-06-15T09:20:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Elijah Newren writes:\n> \n> > This is going to sound harsh, but people shouldn't waste (any more)\n> > time reviewing the patches in this thread or the \"merge: cleanups and\n> > fix\" series submitted elsewhere. They should all just be rejected.\n> >\n> > I do not think it is reasonable to expect reviewers to spend time\n> > responding to re-posted patches when:\n> > * no attempt was made to make sure they were up-to-date with current\n> > code beyond compiling (see below)\n> > * no attempt was made to address missing items pointed out in\n> > response to the original submission[1]\n> > * no attempt was made to handle or even test particular cases\n> > pointed out in response to the original submission (see [1] and below)\n> > * the patches were posted despite knowing they caused segfaults, and\n> > without even stating as much![2]\n> > * the segfault \"fixes\" are submitted as a separate series from the\n> > patch introducing the segfault[3], raising the risk that one gets\n> > picked up without the other.\n> \n> Fair enough. Thanks.\n\nI didn't know some people's opinions on this mailing list were\nautomatically promoted to facts, but FWIW the vast majority of the\npoints stated above are simply not true.\n\n-- \nFelipe Contreras\n"},{"id":"427429","messageId":"YMhx2BFlwUxZ2aFJ@coredump.intra.peff.net","threadId":"55919","inReplyTo":"60c82a622ae66_e5292087f@natae.notmuch","subject":"Re: [PATCH] xdiff: implement a zealous diff3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-15T09:24:40Z","receivedAt":"2021-06-15T09:24:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 14, 2021 at 11:19:46PM -0500, Felipe Contreras wrote:\n\n> > My point is that if you are going to repost a patch that has known\n> > problems,\n> \n> It was not known that it had problems.\n> \n> That fact that person X said patch Y had a problem doesn't necessarily\n> mean that patch Y has a problem.\n> \n> 1. The problem in the past might not apply in the present\n> 2. The problem X person had might be specific to his/her setup\n> 3. The problem might be due a combination of patches, not the patch\n> itself\n> \n> Plus many others.\n> \n> A logical person sees evidence for what it is, and the only thing that\n> person X saying patch Y had a problem means, is that person X said patch\n> Y had a problem.\n\nWow.\n\nFor one thing, you could still relay the _report_ of a problem along\nwith the patch, which would be valuable information for reviewers.\n\nBut much more important, in my opinion: that you would dismiss without\nfurther investigation a report of a bug from the one person who actually\nhad experience running with the patch implies a level of carelessness\nthat I'm not comfortable with for the project.\n\nI had already given up on having substantive discussion with you, but I\nhad hoped I could help the project by pointing out relevant facts in\nareas that you were working in. But if a simple statement like \"this\nsegfaulted for me\" is not even useful, then I don't see much point in\ncommunicating with you at all.\n\n-Peff\n"},{"id":"427440","messageId":"60c87fc6a87ba_e6332084f@natae.notmuch","threadId":"55919","inReplyTo":"YMhx2BFlwUxZ2aFJ@coredump.intra.peff.net","subject":"Re: [PATCH] xdiff: implement a zealous diff3","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-15T10:24:06Z","receivedAt":"2021-06-15T10:24:11Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Mon, Jun 14, 2021 at 11:19:46PM -0500, Felipe Contreras wrote:\n> \n> > > My point is that if you are going to repost a patch that has known\n> > > problems,\n> > \n> > It was not known that it had problems.\n> > \n> > That fact that person X said patch Y had a problem doesn't necessarily\n> > mean that patch Y has a problem.\n> > \n> > 1. The problem in the past might not apply in the present\n> > 2. The problem X person had might be specific to his/her setup\n> > 3. The problem might be due a combination of patches, not the patch\n> > itself\n> > \n> > Plus many others.\n> > \n> > A logical person sees evidence for what it is, and the only thing that\n> > person X saying patch Y had a problem means, is that person X said patch\n> > Y had a problem.\n> \n> Wow.\n> \n> For one thing, you could still relay the _report_ of a problem along\n> with the patch, which would be valuable information for reviewers.\n\nYes I could have, and knowing what I know now I wouldn't even have even\nposted the patch (not without a proposed fix). Woulda, coulda, shoulda.\n\nBut that's not the point. The point is that I did not repost a patch with\nknown problems *today*. Nor did I know what kind of problems, or\nhow pervasive the issue was.\n\nPresumably you had to try at least 2,500 merges to find *one* issue.\n\nI ran all the tests for diff3 with zdiff3 and they passed without\nproblems.\n\n\nMerging this patch would have:\n\n 1. Not broken any tests\n 2. Not changed any behavior for any user\n 3. Not have caused any problem for the vast majority (> 99%) of\n people trying out zdiff3\n\nSo there was no carelessness here.\n\nMoreover, I provied the patch at 9:30, at 10:42 you commented about the\nsegfault, and 16:24 I had the fix. On a Sunday.\n\nIf this is not caring, I don't know what is.\n\n-- \nFelipe Contreras\n"},{"id":"427504","messageId":"CABPp-BGrKjpjb5epv1nXcvn4Z1OHP4Uf6G1f9FARmwUcFVa96Q@mail.gmail.com","threadId":"55919","inReplyTo":"60c86ff87d598_e6332085b@natae.notmuch","subject":"Re: [PATCH] xdiff: implement a zealous diff3","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-15T16:59:04Z","receivedAt":"2021-06-15T16:59:19Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Jun 15, 2021 at 2:16 AM Felipe Contreras\n wrote:\n>\n> Elijah Newren wrote:\n> > On Mon, Jun 14, 2021 at 7:07 PM Junio C Hamano wrote:\n> > >\n> > > Felipe Contreras writes:\n> > >\n> > > > I found the problem, m->chg0 was not initialized in xdl_refine_conflicts.\n> > > >\n> > > > I'm not familiar with the area so I don't know if the following makes\n> > > > sense, but it fixes the crash:\n> > >\n> > > Unlike the remainder of the xdiff/ directory, xdiff/xmerge.c was\n> > > Dscho's brainchild if I am not mistaken, so I'm CCing him for\n> > > input.\n> >\n> > This is going to sound harsh, but people shouldn't waste (any more)\n> > time reviewing the patches in this thread or the \"merge: cleanups and\n> > fix\" series submitted elsewhere. They should all just be rejected.\n> >\n> > I do not think it is reasonable to expect reviewers to spend time\n> > responding to re-posted patches when:\n> > * no attempt was made to make sure they were up-to-date with current\n> > code beyond compiling (see below)\n>\n> What makes you think so?\n\nI did a simple grep to see where \"diff3\" was mentioned in the codebase\nto see if any of those needed a \"zdiff3\". Among the things I found\nwas that although the original patch updated git-completion.bash,\nthere were additional locations within a current git-completion.bash\nthat referred to \"diff3\" that should also have a \"zdiff3\". I know you\nunderstand that part of the code.\n\n> > * no attempt was made to address missing items pointed out in\n> > response to the original submission[1]\n>\n> The original submission caused a discussion with no resolution\n\nThe discussion ended with no resolution in part because there were\nmultiple items discussed that would need to be addressed. Including\nthe one reiterated at the end of the discussion.\n\n>, and\n> edned with Jeff saying he wanted to try real use-cases and that that he\n> wanted to use it in practice for a while.\n\nThat wasn't the end of the discussion. The email you are referencing\noccurred here: https://lore.kernel.org/git/20130307185046.GA11622@sigill.intra.peff.net/.\nThe end of the discussion was Junio quoting himself in order to\nreiterate that \"As long as we clearly present the users what the\noption does and what its implications are, it is not bad to have such\nan option, I think.\" See\nhttps://lore.kernel.org/git/7vip42gfjc.fsf@alter.siamese.dyndns.org/\nand check the timestamps in the threadlist.\n\n> > * no attempt was made to handle or even test particular cases\n> > pointed out in response to the original submission (see [1] and below)\n>\n> Those were sent *after* the series, except [4], which clearly states the\n> *opposite* of there being a deal-breaker:\n>\n> But again, we don't do this splitting now. So I don't think it's\n> something that should make or break a decision to have zdiff3. Without\n> the splitting, I can see it being quite useful.\n\nThis statement from Peff was incorrect; the zdiff3 patches made the\ncode do splitting of conflict hunks. I would normally understand if\nperhaps you didn't know his statement was incorrect and wouldn't have\nhad a way to know, *except* for the fact that this exact patch we are\ncommenting on that you posted is modifying the code that does conflict\nhunk splitting.\n\nFurther, you stated at\nhttps://lore.kernel.org/git/60c8758c80e13_e633208f7@natae.notmuch/\nthat you wanted to see conflict hunk splitting in a zdiff3 mode and\nexpected it. So clearly conflict hunk splitting is relevant to you\neven if it wasn't to Peff.\n\nPeff and Junio spent several emails discussing conflict hunk splitting\nin the original thread (with Junio raising the question multiple times\nshowing it was a concern of his), and Peff spent several emails\ndiscussing that topic even assuming that code was never triggered. In\ncontrast to Peff, you know that conflict hunk splitting is relevant\nsince you wanted it to occur, you saw the old thread where they\ndiscussed that topic and length, and yet you made no attempt to\ninclude a testcase (perhaps even using the one they discussed) to show\nhow the splitting works? I find that negligent.\n\n> > * the patches were posted despite knowing they caused segfaults, and\n> > without even stating as much![2]\n>\n> Whomever *knew* that, it wasn't me.\n\nYou knew that Peff had reported they caused segfaults. He pointed it\nout after making you aware of the zdiff3 patch; see\nhttps://lore.kernel.org/git/YMI+R5LFTj7ezlZE@coredump.intra.peff.net/.\nYou also acknowledged having known of Peff's reports before reposting\nthe patches at https://lore.kernel.org/git/60c82a622ae66_e5292087f@natae.notmuch/\n\nYou may be correct to point out that you only knew Peff had reported\nsegfaults, rather than having verified for yourself that there were\nsegfaults. But the fact that you took no action on the knowledge you\ndid have, neither trying to verify, nor asking if the segfaults still\noccurred, nor even relaying those reports when reposting the patch, is\nexactly the problem at stake here. I find the lack of action with\nrespect to the segfault report to be reckless.\n\n> > * the segfault \"fixes\" are submitted as a separate series from the\n> > patch introducing the segfault[3], raising the risk that one gets\n> > picked up without the other.\n>\n> My v2 includes the patch.\n\nAh, so your plan was to post a v2 with the fix as well as *also* post\nthat fix elsewhere? Okay, that makes me feel better about this item,\nso I retract it.\n\n> > In my opinion, these submissions were egregiously cavalier.\n>\n> If you make unwarranted assumptions everything is possible.\n\nWhich assumptions? That you were splitting the segfault fixes into a\nseparate series and not also including them with the patch that\nintroduces the segfault? That does seem unusual and would have been\nnice if you had communicated your plans somewhere so others wouldn't\nhave to worry about that particular issue, but I agree that your\nexplanation does invalidate the fifth item from my list as a concern.\n\nUnfortunately, that still leaves the other four. It's unfair to\nreviewers to post patches if you have not done due diligence. I've\nread other patches of yours and commented that I thought they looked\ngood, so I'm not just trying to pick on you. You clearly have talent.\nWith regards to the zdiff3 patches, I've stated above why I think you\nhaven't done your due diligence. Sometimes people make mistakes;\nthat's something that can be corrected. What I find egregious here is\nthat even when Peff and I have pointed out how more due diligence is\nexpected and needed, you've dug in to explain why you think your\ncourse of action was reasonable (both here and in\nhttps://lore.kernel.org/git/60c82a622ae66_e5292087f@natae.notmuch/).\nThat in my mind raises your submissions from careless to glaringly\ncavalier. Further, it makes me suspect we may continue to see you\nrepeat such behavior. That worries me.\n"}]}