{"thread":{"id":"55920","subject":"[PATCH 0/4] merge: cleanups and fix","startedAt":"2021-06-13T22:58:48Z","lastAt":"2021-06-17T22:35:07Z","messageCount":10,"participants":["Felipe Contreras","Elijah Newren","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"427248","messageId":"20210613225836.1009569-1-felipe.contreras@gmail.com","threadId":"55920","inReplyTo":null,"subject":"[PATCH 0/4] merge: cleanups and fix","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-13T22:58:32Z","receivedAt":"2021-06-13T22:58:48Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Investigating a crash with the proposed zdiff3 [1] I found some easy\nimprovements in the merge/xmerge code and one crash fix.\n\nThese should not cause any functional changes, even the fix doesn't\naffect any current code-paths.\n\n[1] https://lore.kernel.org/git/YMYnVWSEgxvKRU9j@coredump.intra.peff.net/\n\nFelipe Contreras (4):\n merge: simplify initialization\n merge: fix Yoda conditions\n xdiff/xmerge: simplify xdl_recs_copy_0\n xdiff/xmerge: fix chg0 initialization\n\n builtin/merge-file.c | 8 ++------\n xdiff/xmerge.c | 30 ++++++++++++++++--------------\n 2 files changed, 18 insertions(+), 20 deletions(-)\n\n-- \n2.32.0\n\n"},{"id":"427249","messageId":"20210613225836.1009569-5-felipe.contreras@gmail.com","threadId":"55920","inReplyTo":"20210613225836.1009569-1-felipe.contreras@gmail.com","subject":"[PATCH 4/4] xdiff/xmerge: fix chg0 initialization","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-13T22:58:36Z","receivedAt":"2021-06-13T22:59:01Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"chg0 is needed when style is XDL_MERGE_DIFF3, xdl_refine_conflicts()\ncurrently is only called when level >= XDL_MERGE_ZEALOUS, which cannot\nhappen since the level is capped to XDL_MERGE_EAGER.\n\nHowever, if at some point in the future we decide to not cap diff3, or\nintroduce a similar uncapped mode, an uninitialized chg0 would cause a\ncrash.\n\nLet's initialize it to be safe.\n\nCc: Jeff King \nSigned-off-by: Felipe Contreras \n---\n xdiff/xmerge.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex b5b3f56f92..d9b3a0dccd 100644\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 \t\tmmfile_t t1, t2;\n \t\txdfenv_t xe;\n \t\txdchange_t *xscr, *x;\n-\t\tint i1 = m->i1, i2 = m->i2;\n+\t\tint i0 = m->i0, i1 = m->i1, i2 = m->i2;\n \n \t\t/* let's handle just the conflicts */\n \t\tif (m->mode)\n@@ -384,6 +384,8 @@ static int xdl_refine_conflicts(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m,\n \t\t\tm->next = m2;\n \t\t\tm = m2;\n \t\t\tm->mode = 0;\n+\t\t\tm->i0 = i0;\n+\t\t\tm->chg0 = 0;\n \t\t\tm->i1 = xscr->i1 + i1;\n \t\t\tm->chg1 = xscr->chg1;\n \t\t\tm->i2 = xscr->i2 + i2;\n-- \n2.32.0\n\n"},{"id":"427250","messageId":"20210613225836.1009569-2-felipe.contreras@gmail.com","threadId":"55920","inReplyTo":"20210613225836.1009569-1-felipe.contreras@gmail.com","subject":"[PATCH 1/4] merge: simplify initialization","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-13T22:58:33Z","receivedAt":"2021-06-13T22:59:41Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n builtin/merge-file.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 06a2f90c48..0186f4156a 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -28,7 +28,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \tconst char *names[3] = { NULL, NULL, NULL };\n \tmmfile_t mmfs[3];\n \tmmbuffer_t result = {NULL, 0};\n-\txmparam_t xmp = {{0}};\n+\txmparam_t xmp = { .level = XDL_MERGE_ZEALOUS_ALNUM };\n \tint ret = 0, i = 0, to_stdout = 0;\n \tint quiet = 0;\n \tstruct option options[] = {\n@@ -48,10 +48,6 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \t\tOPT_END(),\n \t};\n \n-\txmp.level = XDL_MERGE_ZEALOUS_ALNUM;\n-\txmp.style = 0;\n-\txmp.favor = 0;\n-\n \tif (startup_info->have_repository) {\n \t\t/* Read the configuration file */\n \t\tgit_config(git_xmerge_config, NULL);\n-- \n2.32.0\n\n"},{"id":"427251","messageId":"20210613225836.1009569-3-felipe.contreras@gmail.com","threadId":"55920","inReplyTo":"20210613225836.1009569-1-felipe.contreras@gmail.com","subject":"[PATCH 2/4] merge: fix Yoda conditions","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-13T22:58:34Z","receivedAt":"2021-06-13T22:59:57Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n builtin/merge-file.c | 2 +-\n xdiff/xmerge.c | 6 +++---\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 0186f4156a..6affccaf19 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -51,7 +51,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \tif (startup_info->have_repository) {\n \t\t/* Read the configuration file */\n \t\tgit_config(git_xmerge_config, NULL);\n-\t\tif (0 <= git_xmerge_style)\n+\t\tif (git_xmerge_style >= 0)\n \t\t\txmp.style = git_xmerge_style;\n \t}\n \ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex 1659edb453..ab3448ca88 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -487,7 +487,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n \t\t * \"diff3 -m\" output does not make sense for anything\n \t\t * more aggressive than XDL_MERGE_EAGER.\n \t\t */\n-\t\tif (XDL_MERGE_EAGER < level)\n+\t\tif (level > XDL_MERGE_EAGER)\n \t\t\tlevel = XDL_MERGE_EAGER;\n \t}\n \n@@ -603,10 +603,10 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n \tif (!changes)\n \t\tchanges = c;\n \t/* refine conflicts */\n-\tif (XDL_MERGE_ZEALOUS <= level &&\n+\tif (level >= XDL_MERGE_ZEALOUS &&\n \t (xdl_refine_conflicts(xe1, xe2, changes, xpp) < 0 ||\n \t xdl_simplify_non_conflicts(xe1, changes,\n-\t\t\t\t\tXDL_MERGE_ZEALOUS < level) < 0)) {\n+\t\t\t\t\tlevel > XDL_MERGE_ZEALOUS) < 0)) {\n \t\txdl_cleanup_merge(changes);\n \t\treturn -1;\n \t}\n-- \n2.32.0\n\n"},{"id":"427252","messageId":"20210613225836.1009569-4-felipe.contreras@gmail.com","threadId":"55920","inReplyTo":"20210613225836.1009569-1-felipe.contreras@gmail.com","subject":"[PATCH 3/4] xdiff/xmerge: simplify xdl_recs_copy_0","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-13T22:58:35Z","receivedAt":"2021-06-13T22:59:59Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Remove one unnecessary indentation level.\n\nSigned-off-by: Felipe Contreras \n---\n xdiff/xmerge.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex ab3448ca88..b5b3f56f92 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -122,19 +122,19 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int nee\n \tfor (i = 0; i < count; size += recs[i++]->size)\n \t\tif (dest)\n \t\t\tmemcpy(dest + size, recs[i]->ptr, recs[i]->size);\n-\tif (add_nl) {\n-\t\ti = recs[count - 1]->size;\n-\t\tif (i == 0 || recs[count - 1]->ptr[i - 1] != '\\n') {\n-\t\t\tif (needs_cr) {\n-\t\t\t\tif (dest)\n-\t\t\t\t\tdest[size] = '\\r';\n-\t\t\t\tsize++;\n-\t\t\t}\n-\n+\tif (!add_nl)\n+\t\treturn size;\n+\ti = recs[count - 1]->size;\n+\tif (i == 0 || recs[count - 1]->ptr[i - 1] != '\\n') {\n+\t\tif (needs_cr) {\n \t\t\tif (dest)\n-\t\t\t\tdest[size] = '\\n';\n+\t\t\t\tdest[size] = '\\r';\n \t\t\tsize++;\n \t\t}\n+\n+\t\tif (dest)\n+\t\t\tdest[size] = '\\n';\n+\t\tsize++;\n \t}\n \treturn size;\n }\n-- \n2.32.0\n\n"},{"id":"427358","messageId":"CABPp-BGZ2H1MVgw9RvSdogLMdqsX3n89NkkDYDa2VM3TRHn7tg@mail.gmail.com","threadId":"55920","inReplyTo":"20210613225836.1009569-5-felipe.contreras@gmail.com","subject":"Re: [PATCH 4/4] xdiff/xmerge: fix chg0 initialization","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-14T15:34:52Z","receivedAt":"2021-06-14T15:35:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Jun 13, 2021 at 4:04 PM Felipe Contreras\n wrote:\n>\n> chg0 is needed when style is XDL_MERGE_DIFF3, xdl_refine_conflicts()\n> currently is only called when level >= XDL_MERGE_ZEALOUS, which cannot\n> happen since the level is capped to XDL_MERGE_EAGER.\n>\n> However, if at some point in the future we decide to not cap diff3, or\n> introduce a similar uncapped mode, an uninitialized chg0 would cause a\n> crash.\n\nIf this cannot happen now, and is needed by your other posted patch,\nwhy aren't these two patches either combined or posted as part of the\nsame series? (I think a separate submission _only_ makes sense if you\nexplicitly withdraw the other patch from consideration, otherwise it\nfeels dangerous to submit patches this way).\n\n> Let's initialize it to be safe.\n\nIs it being safe, or is it hacking around a hypothetical future\nsegfault? Are these values reasonable, or did you just initialize\nthem to known garbage rather than letting them be unknown garbage?\nAre there other values that need to be changed for the xdiff data\nstructures to be consistent? Will future code readers be helped by\nthis initialization, or will it introduce inconsistencies (albeit\ninitialized ones) in existing data structures that make it harder for\nfuture readers to reason about?\n\nI'm somewhat worried by the cavalier posting of the zdiff3 patch[1],\nand am worried you're continuing more of the same here, especially\nsince in your previous post[2] you said that you didn't know whether\nthis particular change made sense and haven't posted anything yet to\nstate why it does. Peff already pointed out that you reposted the\nzdiff3 patch that had knonw segfaults without even stating as much[3].\nThat's one thing that needs to be corrected, but I think there are\nmore that should be pointed out. Peff linked to the old thread which\nis how you found out about the patches, but the old thread also\nincluded things that Junio wanted to see if zdiff3 were to be added as\nan option[4], and discussed some concerns about the implementation\nthat would need to be addressed[5]. Given the fact that Peff liked\nthe original zdiff3 patch and tried it for over a month and took time\nto report on it and argue for such a feature, I would have expected\nthat when you reposted the zdiff3 patch that you would have provided\nsome more detailed explanation of how it works and why it's right (and\nincluded some fixes with it rather than separate), and that you would\nhave included multiple new testcases to the testsuite both to make\nsure we don't cause any obvious bugs and to provide some samples of\nhow the option functions, and also likely include in the cover letter\nsome kind of explanation of additional testing done to try to assure\nus that the new mode is safe to use (e.g. \"I ran this on hundreds of\nlinux kernel commits, with X% of them showing a difference. They\ntypically looked like \" or \"I've run with this for a month without\nissue, and occasionally have re-checked a merge afterwards and found\nthat the conflicts were much easier to view because of...\"). Short of\nall that, I would have at least expected the patches to be posted as\nRFC and stating any known shortcomings and additional work you planned\non doing after gathering some feedback.\n\nYou didn't do any of that, making me wonder whether this patch is a\nsolid fix, or whether it represents just hacking around the first edge\ncase you ran into and posting a shot in the dark. It could well be\neither; how are we to know whether we should trust these patches?\n\n(Having read the old zdiff3 thread after Peff pointed to it, I really\nlike the idea of such an option. I'd like to see it exist and I would\nuse it. But I think it needs to be introduced appropriately,\notherwise it makes it even less likely we'll end up with such a\nthing.)\n\n[1] https://lore.kernel.org/git/20210613143155.836591-1-felipe.contreras@gmail.com/\n[2] https://lore.kernel.org/git/60c677a2c2d24_f5651208cf@natae.notmuch/\n[3] https://lore.kernel.org/git/YMbexfeUG78yBix4@coredump.intra.peff.net/\n[4] https://lore.kernel.org/git/7vip42gfjc.fsf@alter.siamese.dyndns.org/\n[5] https://lore.kernel.org/git/20130307180157.GA6604@sigill.intra.peff.net/\n\n> Cc: Jeff King \n> Signed-off-by: Felipe Contreras \n> ---\n> xdiff/xmerge.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\n> index b5b3f56f92..d9b3a0dccd 100644\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> 2.32.0\n"},{"id":"427421","messageId":"60c85bfd112a9_e633208d5@natae.notmuch","threadId":"55920","inReplyTo":"CABPp-BGZ2H1MVgw9RvSdogLMdqsX3n89NkkDYDa2VM3TRHn7tg@mail.gmail.com","subject":"Re: [PATCH 4/4] xdiff/xmerge: fix chg0 initialization","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-15T07:51:25Z","receivedAt":"2021-06-15T07:51:30Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Elijah Newren wrote:\n> On Sun, Jun 13, 2021 at 4:04 PM Felipe Contreras\n> wrote:\n> >\n> > chg0 is needed when style is XDL_MERGE_DIFF3, xdl_refine_conflicts()\n> > currently is only called when level >= XDL_MERGE_ZEALOUS, which cannot\n> > happen since the level is capped to XDL_MERGE_EAGER.\n> >\n> > However, if at some point in the future we decide to not cap diff3, or\n> > introduce a similar uncapped mode, an uninitialized chg0 would cause a\n> > crash.\n> \n> If this cannot happen now, and is needed by your other posted patch,\n> why aren't these two patches either combined or posted as part of the\n> same series?\n\nBecause I found the fix *after* I sent the patch, and after Jeff posted\na way to reproduce the issue he experienced in the past.\n\nv2 of the series has the fix included, but I was waiting for feedback on\nv1.\n\n> (I think a separate submission _only_ makes sense if you\n> explicitly withdraw the other patch from consideration, otherwise it\n> feels dangerous to submit patches this way).\n\nMore than 50% of my patches get either completely ignored, or commented,\nand then ignored. The vast majority of them don't have a valid reason\nfor rejection.\n\nI thought this series had a better chance of being merged before the\nzdiff3 series, and the chg0 is orthogonal to the zdiff3 series anyway\n(in my view).\n\nPlus, I don't see the harm in sending the same patch in two different\nseries. Especially if the chances of both series being merged any time\nsoon is very low.\n\n> > Let's initialize it to be safe.\n> \n> Is it being safe, or is it hacking around a hypothetical future\n> segfault?\n\nIt's being safe.\n\n> Are these values reasonable,\n\nI spent about two hours of my own free time--with no corporation backing\nup my git work--to familiarize myself with the code and the values are\nas reasonable as I could make them.\n\nIf m->chg0 is 0, xdl_orig_copy will ignore the chunk, so m->i0 is\nirrelevant, but just to be consistent I copied the original i0.\n\nOnce again: the only value that matters is m->chg0, and it's completely\nsafe to set it to 0.\n\nMuch more reasonable than garbage, like\n1774234 which we currently have sometimes.\n\n> or did you just initialize them to known garbage rather than letting\n> them be unknown garbage?\n\nNo.\n\n> Are there other values that need to be changed for the xdiff data\n> structures to be consistent?\n\nNo.\n\n> Will future code readers be helped by this initialization,\n\nYes.\n\n> or will it introduce inconsistencies (albeit initialized ones) in\n> existing data structures that make it harder for future readers to\n> reason about?\n\nNo.\n\n> I'm somewhat worried by the cavalier posting of the zdiff3 patch[1],\n\nWhen Uwe sent the patch [1] nobody said it was a \"cavalier posting\".\nJeff King said \"I think the patch is correct\".\n\n> and am worried you're continuing more of the same here, especially\n> since in your previous post[2] you said that you didn't know whether\n> this particular change made sense and haven't posted anything yet to\n> state why it does.\n\nNobody pays me to work on git. I comment when I have time. Yesterday was\nmy birthday and I spent my free time doing other things.\n\nWhen I sent that mail it was \"Sun, 13 Jun 2021 16:24:50 -0500\", when I\nsent the patch it was \"Sun, 13 Jun 2021 17:58:36 -0500\". After spending\nmore than an hour and a half reading the code, and trying different\napproaches I became more confident that there was no better approach. At\nleast not one that was easily found.\n\nI am 99% certain that m->chg0 = 0 is safe, and produces a reasonable\noutput.\n\nWhat I didn't know at 16:24 is if there was something better we could\ndo. But by 17:58 I became much more certain that there wasn't, although\nI'm still not sure.\n\nDegrees of confidence vary with time.\n\n> Peff already pointed out that you reposted the zdiff3 patch that had\n> knonw segfaults without even stating as much[3].\n\nHe knew that. I didn't.\n\n> That's one thing that needs to be corrected, but I think there are\n> more that should be pointed out. Peff linked to the old thread which\n> is how you found out about the patches, but the old thread also\n> included things that Junio wanted to see if zdiff3 were to be added as\n> an option[4],\n\nThat thread included many things, including links to other threads.\n\n> and discussed some concerns about the implementation that would need\n> to be addressed[5].\n\nThat link is a mail from Jeff stating that he disagrees with Junio in at\nleast one point, and said:\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> Given the fact that Peff liked the original zdiff3 patch and tried it\n> for over a month and took time to report on it and argue for such a\n> feature, I would have expected that when you reposted the zdiff3 patch\n> that you would have provided some more detailed explanation of how it\n> works and why it's right (and included some fixes with it rather than\n> separate),\n\nI didn't want to override Uwe's wording, especially since both Junio and\nJeff often complain about my commit messages, and it isn't rare that\nthey end up rewritten.\n\n> and that you would have included multiple new testcases to the\n> testsuite both to make sure we don't cause any obvious bugs and to\n> provide some samples of how the option functions,\n\nIt is not uncommon for v1 of a patch series to be missing something.\nUwe's patch series [1] did not include tests either, and nobody focused\non that. It is completely reasonable to assume that a reboot of the\nseries wouldn't initially focus on that either.\n\nThat being said, I did test zdiff3 with the whole testing framework, and\nI did explain how.\n\n> and also likely include in the cover letter some kind of explanation\n> of additional testing done to try to assure us that the new mode is\n> safe to use (e.g. \"I ran this on hundreds of linux kernel commits,\n> with X% of them showing a difference. They typically looked like \"\n> or \"I've run with this for a month without issue, and occasionally\n> have re-checked a merge afterwards and found that the conflicts were\n> much easier to view because of...\").\n\nI typically don't send cover letters for single patches (just like Uwe\ndidn't [1]), but I did add an explanation of what tests I ran below the\ncommit message of the patch, as is customary.\n\n> Short of all that, I would have at least expected the patches to be\n> posted as RFC and stating any known shortcomings and additional work\n> you planned on doing after gathering some feedback.\n\nI had not planned to do any additional work, as I don't usually plan how\nI spend my free time. I happened to have free time when I saw Jeff mail\nabout a way to reproduce and I felt motivated to investigate further.\nOne thing lead to another and fortunately I found the fix quickly\nenough.\n\n> You didn't do any of that, making me wonder whether this patch is a\n> solid fix, or whether it represents just hacking around the first edge\n> case you ran into and posting a shot in the dark. It could well be\n> either; how are we to know whether we should trust these patches?\n\nBy assuming good faith [2], and simply asking questions. I don't think\nassuming the worst and calling into question the competence of a\ncontributor is a good approach.\n\n\nMorevoer, this is not *my* patch, this is a patch from the community, to\nthe community, and it's in the best interest of the community that it\ngets developed *collaboratively*.\n\nI took Uwe's patch and added my two cents, not for me, but for the\ncommunity. I don't need zdiff3, I'm perfectly fine with diff3, but I\nwould like a better default conflict style than merge, for the\ncommunity. That's why I spend a considerable amount of time reading the\nold threads, and familiarizing myself with the xdiff/xmerge code of\nwhich I basically knew nothing about.\n\nThis is something I wish more people did, and then perhaps we wouldn't\nneed to wait 8 years for a simple crash fix for a feature many people\nprobably would like, which again, I'm 99% certain is correct.\n\n\nTo be crystal clear: this is not *my itch*, I did the patch\naltrusticially for the community. The fix correct--at least to the best\nknowledge of everyone involved so far. If you want to deride the hours I\nspent working for the community for free which resulted in a patch that\nmost likely is a net positive, feel free, I think this doesn't help the\ncommunity, which needs more of this kind of work, not less.\n\nCheers.\n\n[1] https://lore.kernel.org/git/1362602202-29749-1-git-send-email-u.kleine-koenig@pengutronix.de/\n[2] https://en.wikipedia.org/wiki/Wikipedia:Assume_good_faith\n\n-- \nFelipe Contreras\n"},{"id":"427487","messageId":"CABPp-BEXtJtkkh9Diuo4e1K1ci=vggGxkLRDfkpOH12LM8TCfA@mail.gmail.com","threadId":"55920","inReplyTo":"60c85bfd112a9_e633208d5@natae.notmuch","subject":"Re: [PATCH 4/4] xdiff/xmerge: fix chg0 initialization","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-15T15:21:43Z","receivedAt":"2021-06-15T15:21:57Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Jun 15, 2021 at 12:51 AM Felipe Contreras\n wrote:\n>\n> Elijah Newren wrote:\n> > On Sun, Jun 13, 2021 at 4:04 PM Felipe Contreras\n> > wrote:\n> > >\n...\n> > I'm somewhat worried by the cavalier posting of the zdiff3 patch[1],\n>\n> When Uwe sent the patch [1] nobody said it was a \"cavalier posting\".\n> Jeff King said \"I think the patch is correct\".\n\nThe difference here is that Jeff reported segfaults after Uwe sent the\npatch. Uwe could not have read those reports before he posted the\npatch. You did read those emails before re-posting the patch.\n\n> > Peff already pointed out that you reposted the zdiff3 patch that had\n> > knonw segfaults without even stating as much[3].\n>\n> He knew that. I didn't.\n\nYou knew he had reported it as a possibility. That behooved you to\ntry to investigate, either by asking him for details to try to\nreproduce, or attempt it on a large range of merges, and then to\nreport the results when you reposted the patch. Or, at the absolute\nminimum, to at least report Peff's report along with your reposting of\nthe patch.\n\n> > and that you would have included multiple new testcases to the\n> > testsuite both to make sure we don't cause any obvious bugs and to\n> > provide some samples of how the option functions,\n>\n> It is not uncommon for v1 of a patch series to be missing something.\n> Uwe's patch series [1] did not include tests either, and nobody focused\n> on that. It is completely reasonable to assume that a reboot of the\n> series wouldn't initially focus on that either.\n\nNot when folks spent several emails in response to the original about\nthe finer points of how splitting should or should not work. Nor when\nfolks had reported segfaults with the original patch. With that\nbackground, I'd say it's completely unreasonable to assume that a\nreboot of the series comes without any tests unless marked as RFC.\n\n> > You didn't do any of that, making me wonder whether this patch is a\n> > solid fix, or whether it represents just hacking around the first edge\n> > case you ran into and posting a shot in the dark. It could well be\n> > either; how are we to know whether we should trust these patches?\n>\n> By assuming good faith [2], and simply asking questions. I don't think\n> assuming the worst and calling into question the competence of a\n> contributor is a good approach.\n\nI did not assume the worst with your patches. I assumed good faith\nuntil I was provided with strong counter-evidence as perhaps best\nsummarized at https://lore.kernel.org/git/YMbexfeUG78yBix4@coredump.intra.peff.net/.\nThat made it clear something was heavily problematic. If you hadn't\nreposted patches for which there were reported segfaults without\nsaying anything about those reports, I would have asked you none of\nthose pointed questions. In fact, I probably wouldn't have asked them\nif your original response to Peff were along the lines of \"sorry,\nwon't do that again\" rather than \"I don't know how I was supposed to\ninvestigate the few segfaults you mentioned. All you said is that you\nnever tracked the bug.\" It was precisely that cavalier attitude that\nmade me question this patch in this manner.\n\n> If you want to deride the hours I\n> spent working for the community for free which resulted in a patch that\n> most likely is a net positive, feel free, I think this doesn't help the\n> community, which needs more of this kind of work, not less.\n\nI was not deriding your work. I was asking deep and pointed questions\ndue to the cavalier manner in which you were posting, and which you\nseem to be doubling down on. I would say that after\nhttps://lore.kernel.org/git/YMhx2BFlwUxZ2aFJ@coredump.intra.peff.net/,\nI'd have to agree with Peff that you have displayed a level of\ncarelessness with which I am not comfortable for the project. That\nkind of carelessness leads to skepticism in others, moreso when you\ndouble down on it. It makes me think that if I review any of your\nfuture patches, I need to check them far more rigorously because you\nare willing to eschew what I view as even the most basic of\nresponsibilities in ensuring you are submitting valid patches, and\neven worse, you are unwilling to change how you approach the project\neven when those are pointed out to you.\n"},{"id":"427524","messageId":"87sg1iyl92.fsf@evledraar.gmail.com","threadId":"55920","inReplyTo":"20210613225836.1009569-2-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/4] merge: simplify initialization","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-15T19:27:56Z","receivedAt":"2021-06-15T19:35:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Jun 13 2021, Felipe Contreras wrote:\n\n> Signed-off-by: Felipe Contreras \n> ---\n> builtin/merge-file.c | 6 +-----\n> 1 file changed, 1 insertion(+), 5 deletions(-)\n>\n> diff --git a/builtin/merge-file.c b/builtin/merge-file.c\n> index 06a2f90c48..0186f4156a 100644\n> --- a/builtin/merge-file.c\n> +++ b/builtin/merge-file.c\n> @@ -28,7 +28,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n> \tconst char *names[3] = { NULL, NULL, NULL };\n> \tmmfile_t mmfs[3];\n> \tmmbuffer_t result = {NULL, 0};\n> -\txmparam_t xmp = {{0}};\n> +\txmparam_t xmp = { .level = XDL_MERGE_ZEALOUS_ALNUM };\n> \tint ret = 0, i = 0, to_stdout = 0;\n> \tint quiet = 0;\n> \tstruct option options[] = {\n> @@ -48,10 +48,6 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n> \t\tOPT_END(),\n> \t};\n> \n> -\txmp.level = XDL_MERGE_ZEALOUS_ALNUM;\n> -\txmp.style = 0;\n> -\txmp.favor = 0;\n> -\n> \tif (startup_info->have_repository) {\n> \t\t/* Read the configuration file */\n> \t\tgit_config(git_xmerge_config, NULL);\n\nLooks good, maybe we can fix the similar code in ll_xdl_merge() while\nwe're at it?\n\nAlso, not a problem in your commit, but we check for that constant in\nonly one place, as:\n\n XDL_MERGE_ZEALOUS < level\n\nUrgh, do you know if there's some reason we're not doing level ==\nXDL_MERGE_ZEALOUS_ALNUM, or at least level >= XDL_MERGE_ZEALOUS_ALNUM\nthere?\n"},{"id":"427796","messageId":"60cbce16708c1_37b9208b3@natae.notmuch","threadId":"55920","inReplyTo":"87sg1iyl92.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/4] merge: simplify initialization","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-17T22:35:02Z","receivedAt":"2021-06-17T22:35:07Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> \n> On Sun, Jun 13 2021, Felipe Contreras wrote:\n> \n> > Signed-off-by: Felipe Contreras \n> > ---\n> > builtin/merge-file.c | 6 +-----\n> > 1 file changed, 1 insertion(+), 5 deletions(-)\n> >\n> > diff --git a/builtin/merge-file.c b/builtin/merge-file.c\n> > index 06a2f90c48..0186f4156a 100644\n> > --- a/builtin/merge-file.c\n> > +++ b/builtin/merge-file.c\n> > @@ -28,7 +28,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n> > \tconst char *names[3] = { NULL, NULL, NULL };\n> > \tmmfile_t mmfs[3];\n> > \tmmbuffer_t result = {NULL, 0};\n> > -\txmparam_t xmp = {{0}};\n> > +\txmparam_t xmp = { .level = XDL_MERGE_ZEALOUS_ALNUM };\n> > \tint ret = 0, i = 0, to_stdout = 0;\n> > \tint quiet = 0;\n> > \tstruct option options[] = {\n> > @@ -48,10 +48,6 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n> > \t\tOPT_END(),\n> > \t};\n> > \n> > -\txmp.level = XDL_MERGE_ZEALOUS_ALNUM;\n> > -\txmp.style = 0;\n> > -\txmp.favor = 0;\n> > -\n> > \tif (startup_info->have_repository) {\n> > \t\t/* Read the configuration file */\n> > \t\tgit_config(git_xmerge_config, NULL);\n> \n> Looks good, maybe we can fix the similar code in ll_xdl_merge() while\n> we're at it?\n\nYeah, but since code cleanups for the sake of cleaning up are frowned\nupon [1] it's not clear how we would get these merged in, since Elijah\nhas gunned down the obvious fix in patch 4.\n\nNow the cleanups are orphaned.\n\n> Also, not a problem in your commit, but we check for that constant in\n> only one place, as:\n> \n> XDL_MERGE_ZEALOUS < level\n\nMy mind can't process that, so...\n\n level > XDL_MERGE_ZEALOUS\n\n> Urgh, do you know if there's some reason we're not doing level ==\n> XDL_MERGE_ZEALOUS_ALNUM, or at least level >= XDL_MERGE_ZEALOUS_ALNUM\n> there?\n\nNope. To the best of my knowledge that's what the code actually meant.\n\nIn general the code in xdiff/* seems to be following a very different\nstyle than the rest of git's core.\n\nCheers.\n\n[1] https://lore.kernel.org/git/xmqqbl87zyra.fsf@gitster.g/\n\n-- \nFelipe Contreras"}]}