{"thread":{"id":"48587","subject":"RFC: Merge-related plans","startedAt":"2018-05-28T20:48:54Z","lastAt":"2018-05-30T04:21:49Z","messageCount":5,"participants":["Elijah Newren","Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"348683","messageId":"CABPp-BFQJZHfCJZ1qvhvVcMd-_sOfi0Fkm5PexEwzzN+Zw2akw@mail.gmail.com","threadId":"48587","inReplyTo":null,"subject":"RFC: Merge-related plans","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-28T20:48:50Z","receivedAt":"2018-05-28T20:48:54Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi everyone,\n\nI have some merge-related plans (and work in progress) that I'd like\nto get some feedback on in order to find what order would be best to\naddress things in, if there are special steps I should take while\napproaching some of the bigger items, and even if folks disagree with\nany of the plans.\n\n\nCurrently, I would like to:\n\nA) Fix cases where directory rename detection does not work with\n   rebase/am due to how they call merge-recursive.\n\n   Notes: Could just wait for D & E to land before fixing.\n   Alternatively, email RFC to list now explaining issues and how the\n   fix has performance implications; poll for opinions on whether to\n   fix before or after D.\n\nB) Implement a remerge-diff ability (compare a merge commit to what an\n   \"automatic merge\" would look like)[1].\n\n   Notes: Possibly for cherry-picks and reverts too.  Depends on C &\n   E.\n\nC) Modify/extend how path-based and mode-based conflicts are\n   communicated to the user.\n\n   Notes: Particularly important as a mechanism for handling\n   challenges[2] with implementing the remerge-diff ability.  Need to\n   send RFC to list with ideas to get feedback.\n\nD) Improve merge performance.\n\n   Notes: Includes 4-5 specific optimizations[5], some of which I\n   expect to be somewhat invasive and thus may make more sense to just\n   make part of the new merge strategy implemented in E.  Biggest\n   optimization depends on F.\n\nE) Write a new merge strategy meant to replace merge-recursive.\n\n   Notes: Suggested by Junio[3][4].  Depends on F & G.\n\nF) Make file collision conflict types more consistent.\n\n   Notes: Specifically, make rename/rename(2to1) & rename/add\n   conflicts behave more like add/add[6][7].  Depends on part of G.\n   Would prefer H to be accepted first.\n\nG) Improve merge-related portion of testsuite.\n\n   Notes: Intended to help test new merge strategy with more\n   confidence.  Will include approximately a dozen edge and corner\n   cases where merge-recursive currently falls short.  Started at [8];\n   see also [9].\n\nH) Miscellaneous code cleanups irritating me while working on other\n   changes[10].\n\n\nMy current plan was to work roughly in reverse, from H to A.  Some questions:\n\n  * Does any of this look objectionable?\n  * Should I post RFC questions on A and C earlier?\n  * Should I split D and G?  (Current plan was to keep D together, but\n    split G into five short slightly inter-dependent topics)\n  * E is kind of big.  Are there any specific things folks would like to see\n    with how that is handled?\n\n\nThanks,\nElijah\n\n\n[1] https://bugs.chromium.org/p/git/issues/detail?id=12 [remerge-diff]\n[2] https://public-inbox.org/git/CABPp-BEsTOZ-tCvG1y5a0qPB8xJLLa0obyTU===mBgXC1jXgFA@mail.gmail.com/\n    [remerge-diff challenges]\n[3] https://public-inbox.org/git/xmqqd147kpdm.fsf@gitster.mtv.corp.google.com/\n    [Ideal world merge strategy]\n[4] https://public-inbox.org/git/xmqqk1ydkbx0.fsf@gitster.mtv.corp.google.com/\n    [New strategy]\n[5] Some of which are included in\n    https://public-inbox.org/git/20171120221944.15431-1-newren@gmail.com/\n    [perf improvements]\n[6] https://public-inbox.org/git/CAPc5daVu8vv9RdGON8JiXEO3ycDVqQ38ySzZc-cpo+AQcAKXjA@mail.gmail.com/\n    [discussion of add/add conflict resolution]\n[7] https://public-inbox.org/git/20180305171125.22331-1-newren@gmail.com/\n    [file collision consistency RFC series]\n[8] https://public-inbox.org/git/20180524070439.6367-1-newren@gmail.com/\n    [testcase cleanup]\n[9] https://public-inbox.org/git/CABPp-BFc1OLYKzS5rauOehvEugPc0oGMJp-NMEAmVMW7QR=4Eg@mail.gmail.com/\n    [weird corner cases]\n[10] https://public-inbox.org/git/20180522004327.13085-1-newren@gmail.com/\n     [code cleanups]\n"},{"id":"348711","messageId":"CAGZ79ka39xbbm_koyFeEizggoDBqKuwgmGwRBZJnBc13aOWZag@mail.gmail.com","threadId":"48587","inReplyTo":"CABPp-BFQJZHfCJZ1qvhvVcMd-_sOfi0Fkm5PexEwzzN+Zw2akw@mail.gmail.com","subject":"Re: RFC: Merge-related plans","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-29T18:19:07Z","receivedAt":"2018-05-29T18:20:13Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 28, 2018 at 1:48 PM, Elijah Newren <newren@gmail.com> wrote:\n> Hi everyone,\n>\n> I have some merge-related plans (and work in progress) that I'd like\n> to get some feedback on in order to find what order would be best to\n> address things in, if there are special steps I should take while\n> approaching some of the bigger items, and even if folks disagree with\n> any of the plans.\n>\n>\n> Currently, I would like to:\n>\n> A) Fix cases where directory rename detection does not work with\n>    rebase/am due to how they call merge-recursive.\n>\n>    Notes: Could just wait for D & E to land before fixing.\n>    Alternatively, email RFC to list now explaining issues and how the\n>    fix has performance implications; poll for opinions on whether to\n>    fix before or after D.\n>\n> B) Implement a remerge-diff ability (compare a merge commit to what an\n>    \"automatic merge\" would look like)[1].\n>\n>    Notes: Possibly for cherry-picks and reverts too.  Depends on C &\n>    E.\n>\n> C) Modify/extend how path-based and mode-based conflicts are\n>    communicated to the user.\n>\n>    Notes: Particularly important as a mechanism for handling\n>    challenges[2] with implementing the remerge-diff ability.  Need to\n>    send RFC to list with ideas to get feedback.\n>\n> D) Improve merge performance.\n>\n>    Notes: Includes 4-5 specific optimizations[5], some of which I\n>    expect to be somewhat invasive and thus may make more sense to just\n>    make part of the new merge strategy implemented in E.  Biggest\n>    optimization depends on F.\n>\n> E) Write a new merge strategy meant to replace merge-recursive.\n>\n>    Notes: Suggested by Junio[3][4].  Depends on F & G.\n>\n> F) Make file collision conflict types more consistent.\n>\n>    Notes: Specifically, make rename/rename(2to1) & rename/add\n>    conflicts behave more like add/add[6][7].  Depends on part of G.\n>    Would prefer H to be accepted first.\n>\n> G) Improve merge-related portion of testsuite.\n>\n>    Notes: Intended to help test new merge strategy with more\n>    confidence.  Will include approximately a dozen edge and corner\n>    cases where merge-recursive currently falls short.  Started at [8];\n>    see also [9].\n\nMost items forward-reference \"Depends on (<later letter>) up to here;\n(H) seems independent, but might be a good first start.\n(G) [8] is queued as origin/en/merge-recursive-tests, or do you mean\nto expand (G) into a mini-framework of merge-testing? i.e. run the\nmini test framework multiple times, each using a different\nmerge strategy, similar to submodule tests, e.g. see\nt/lib-submodule-update.sh and one of its users, t1013.\n\n\n> H) Miscellaneous code cleanups irritating me while working on other\n>    changes[10].\n>\n>\n> My current plan was to work roughly in reverse, from H to A.  Some questions:\n>\n>   * Does any of this look objectionable?\n\nGoing in order A-H seems slightly out-of-order to me, I'd think (H) and (G)\nwould go first;\n\n(B) sounds like an independent feature, which could go in parallel?\n\n>   * Should I post RFC questions on A and C earlier?\n\nI would think so, it is easier to give feedback on code, I would think.\n\n>   * Should I split D and G?  (Current plan was to keep D together, but\n>     split G into five short slightly inter-dependent topics)\n\nI would have expected to have tests (G) as a companion of (A) or (C)\nrather than (D), as performance improvements would keep the test suite\nunchanged?\n\n>   * E is kind of big.  Are there any specific things folks would like to see\n>     with how that is handled?\n\nHow much abstraction can be done ahead of time such that there is an\ninterface/API where you just plug in a new merge strategy and do not\nneed to duplicate a lot of code/tests?\n\nStefan\n"},{"id":"348720","messageId":"CABPp-BH1BBH7aBpoSaJE5J7okdfrDzpbu5rH-w-FSG2RM0hU1g@mail.gmail.com","threadId":"48587","inReplyTo":"CAGZ79ka39xbbm_koyFeEizggoDBqKuwgmGwRBZJnBc13aOWZag@mail.gmail.com","subject":"Re: RFC: Merge-related plans","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-29T21:03:27Z","receivedAt":"2018-05-29T21:03:34Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Stefan,\n\nOn Tue, May 29, 2018 at 11:19 AM, Stefan Beller <sbeller@google.com> wrote:\n> On Mon, May 28, 2018 at 1:48 PM, Elijah Newren <newren@gmail.com> wrote:\n>> Currently, I would like to:\n>>\n>> A) Fix cases where directory rename detection does not work with\n>>    rebase/am due to how they call merge-recursive.\n>>\n>>    Notes: Could just wait for D & E to land before fixing.\n>>    Alternatively, email RFC to list now explaining issues and how the\n>>    fix has performance implications; poll for opinions on whether to\n>>    fix before or after D.\n>>\n>> B) Implement a remerge-diff ability (compare a merge commit to what an\n>>    \"automatic merge\" would look like)[1].\n>>\n>>    Notes: Possibly for cherry-picks and reverts too.  Depends on C &\n>>    E.\n>>\n>> C) Modify/extend how path-based and mode-based conflicts are\n>>    communicated to the user.\n>>\n>>    Notes: Particularly important as a mechanism for handling\n>>    challenges[2] with implementing the remerge-diff ability.  Need to\n>>    send RFC to list with ideas to get feedback.\n>>\n>> D) Improve merge performance.\n>>\n>>    Notes: Includes 4-5 specific optimizations[5], some of which I\n>>    expect to be somewhat invasive and thus may make more sense to just\n>>    make part of the new merge strategy implemented in E.  Biggest\n>>    optimization depends on F.\n>>\n>> E) Write a new merge strategy meant to replace merge-recursive.\n>>\n>>    Notes: Suggested by Junio[3][4].  Depends on F & G.\n>>\n>> F) Make file collision conflict types more consistent.\n>>\n>>    Notes: Specifically, make rename/rename(2to1) & rename/add\n>>    conflicts behave more like add/add[6][7].  Depends on part of G.\n>>    Would prefer H to be accepted first.\n>>\n>> G) Improve merge-related portion of testsuite.\n>>\n>>    Notes: Intended to help test new merge strategy with more\n>>    confidence.  Will include approximately a dozen edge and corner\n>>    cases where merge-recursive currently falls short.  Started at [8];\n>>    see also [9].\n>\n> Most items forward-reference \"Depends on (<later letter>) up to here;\n> (H) seems independent, but might be a good first start.\n> (G) [8] is queued as origin/en/merge-recursive-tests, or do you mean\n> to expand (G) into a mini-framework of merge-testing? i.e. run the\n> mini test framework multiple times, each using a different\n> merge strategy, similar to submodule tests, e.g. see\n> t/lib-submodule-update.sh and one of its users, t1013.\n\nSorry, I should have been more clear about G.\nen/merge-recursive-tests is not G, it's just a preparation topic for\nit (or, alternatively, the first of a few topics for G).  In short,\nthe idea for G was missing coverage of existing functionality for\nmerge-recursive.\n\nIn more detail...\n\nThe idea for G was that, if possible, I wanted to avoid a repeat of\nthe problems directory rename detection caused Junio where he had to\nrevert it from master and worried about mis-merges.  That could have\nbeen avoided if we had additional tests in the testsuite covering the\nalready-up-to-date check, making me wonder if there were other\nimportant cases where merge-recursive lacked coverage.  Rather than\nfinding untested cases which merge-recursive handles correctly, my\ninvestigation mostly yielded additional testcases where\nmerge-recursive falls down, including:\n  * about 3 different rename cases (non-recursive), one of which was\nreported previously but isn't in the testsuite\n  * about 4 different issues with submodules (non-recursive) that I\ndon't think are represented in the testsuite (a couple of which I\nmentioned in an email to you on the list last year but never created\ntestcases for)\n  * about 6 different issues with special types and/or mode conflicts\n(recursive only)\n  * a few additional tests, commentary, and an alternate idea for\nimproving directory/file conflict handling\n\n>> H) Miscellaneous code cleanups irritating me while working on other\n>>    changes[10].\n>>\n>>\n>> My current plan was to work roughly in reverse, from H to A.  Some questions:\n>>\n>>   * Does any of this look objectionable?\n>\n> Going in order A-H seems slightly out-of-order to me, I'd think (H) and (G)\n> would go first;\n>\n> (B) sounds like an independent feature, which could go in parallel?\n\nB may sound like an independent feature, but it needs a merge\nalgorithm that doesn't mess with the working tree so it depends pretty\nstrongly on E.\n\n>>   * Should I post RFC questions on A and C earlier?\n>\n> I would think so, it is easier to give feedback on code, I would think.\n\nIf the idea is to give feedback on *code* rather than just\nideas/tradeoffs/pinpointing-buggy-lines, then it sounds like you're\nactually suggesting posting the RFC later rather than earlier?\n\nAlso, the bigger question for me wasn't so much \"should I ask the list\nabout these changes?\" before making them, but rather: Do folks want me\nto bring these things up before I work on D & E -- even if I end up\nnot getting back to incorporating their answers for months until D & E\nare completed and merged?\n\n>>   * Should I split D and G?  (Current plan was to keep D together, but\n>>     split G into five short slightly inter-dependent topics)\n>\n> I would have expected to have tests (G) as a companion of (A) or (C)\n> rather than (D), as performance improvements would keep the test suite\n> unchanged?\n\nLet me re-phrase: D and G are completely independent series, both of\nwhich happen to be divisible.  Should either of them be split?\n\nMore background: D is only a handful of commits, so far; the main\nreason to split it is to allow some of it to go first (maybe even\nbefore G or H).  The downside is introducing extra churn and risk in\nmerge-recursive, when I'm planning to rewrite it soon anyway.  I was\ntrying to minimize merge-recursive changes, other than trying to make\nsure that the new merge strategy and merge-recursive will give\nidentical results (modulo maybe fixing a few extra corner cases and\nrunning faster).  Basically, I wanted it to be really easy to compare\nold and new strategies, but otherwise wanted to leave merge-recursive\nmostly alone.  It's not entirely clear how quickly I'll find time to\nwork on all of this, though, so maybe just-wait-for-the-rewrite is not\nthe right prioritization.\n\n(And yes, A & C deserve tests, but they're not of the\nmissing-coverage-for-merge-recursive variety that I was thinking of\nfor G.)\n\n>>   * E is kind of big.  Are there any specific things folks would like to see\n>>     with how that is handled?\n>\n> How much abstraction can be done ahead of time such that there is an\n> interface/API where you just plug in a new merge strategy and do not\n> need to duplicate a lot of code/tests?\n\nFor avoiding duplicate tests, my plan was to use a variable (like\nGIT_TEST_SPLIT_INDEX or GIT_FSMONITOR_TEST), which, when set, would\nchange the default merge strategy from \"recursive\" to the new one, and\nalso replace explicit requests for the \"recursive\" merge strategy with\nthe new one.\n\nFor avoiding duplicate code...well, Junio's suggestion was \"[to\nrewrite] without using much from the existing code in\nmerge-recursive.c at all\" because he has \"written off that code as\nmostly unsalvageable\"[4].\n\nThere are some functions that I think are worth leaving intact (e.g.\nhelpers like output(), the directory rename detection stuff, or the\nmerge_submodule code that you recently moved).  That stuff I was\nplanning to put in a \"/* COPIED FROM merge-recursive.c VERBATIM */\"\nsection.  And eventually, the idea would be to delete the old\nmerge-recursive and just use the replacement, making the duplication\nconcerns eventually go away.\n\n...or is your question more about how to abstract things so that\nothers can also write new merge strategies in the future and allow as\nmuch code and test re-use as possible?\n\n\n>> [4] https://public-inbox.org/git/xmqqk1ydkbx0.fsf@gitster.mtv.corp.google.com/\n>>    [New strategy]\n\n\nThanks for taking a look!\n\nElijah\n"},{"id":"348731","messageId":"CAGZ79kYGTaFg7NnfxzdXQYTNddvqbUD3hfWEiq1RBETjYk=3gQ@mail.gmail.com","threadId":"48587","inReplyTo":"CABPp-BH1BBH7aBpoSaJE5J7okdfrDzpbu5rH-w-FSG2RM0hU1g@mail.gmail.com","subject":"Re: RFC: Merge-related plans","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-29T22:12:58Z","receivedAt":"2018-05-29T22:13:03Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"context: https://public-inbox.org/git/CABPp-BFQJZHfCJZ1qvhvVcMd-_sOfi0Fkm5PexEwzzN+Zw2akw@mail.gmail.com/\n\nHi Elijah,\n\n>> Most items forward-reference \"Depends on (<later letter>) up to here;\n>> (H) seems independent, but might be a good first start.\n>> (G) [8] is queued as origin/en/merge-recursive-tests, or do you mean\n>> to expand (G) into a mini-framework of merge-testing? i.e. run the\n>> mini test framework multiple times, each using a different\n>> merge strategy, similar to submodule tests, e.g. see\n>> t/lib-submodule-update.sh and one of its users, t1013.\n>\n> Sorry, I should have been more clear about G.\n>\n> In more detail...\n>\n> [...]\n\nYeah that is what I had in mind when talking about a mini test frame work\nabove, such that you can run the same test on any merge strategy easily.\nSorry for being confusing there.\n\n>> (B) sounds like an independent feature, which could go in parallel?\n>\n> B may sound like an independent feature, but it needs a merge\n> algorithm that doesn't mess with the working tree\n\nI agree on that,\n\n> so it depends pretty strongly on E.\n\n.. but not quite on the conclusion:\nYou could also make the current merge algorithm working\ntree independent. Thomas Rast (cc'd) did so IIUC in\nhttps://public-inbox.org/git/cover.1409860234.git.tr@thomasrast.ch/\n(search the archive for \"--remerge\" if interested in this prior work)\nwhich did not land upstream.\n\n>>>   * Should I post RFC questions on A and C earlier?\n>>\n>> I would think so, it is easier to give feedback on code, I would think.\n>\n> If the idea is to give feedback on *code* rather than just\n> ideas/tradeoffs/pinpointing-buggy-lines, then it sounds like you're\n> actually suggesting posting the RFC later rather than earlier?\n\nSome people have complained that they don't get feedback on\nideas/tradeoffs/pinpointing-buggy-lines, but did so after sending\nRFC code. So I'd think a sure way to get feedback is to send actual\ncode as an RFC even if it misses some parts.\n\n> Also, the bigger question for me wasn't so much \"should I ask the list\n> about these changes?\" before making them, but rather: Do folks want me\n> to bring these things up before I work on D & E -- even if I end up\n> not getting back to incorporating their answers for months until D & E\n> are completed and merged?\n\nI'd think A + C are worth asking early nevertheless, even if D & E are\naway for some month; having these niche cases covered (in code and\ntests) as well as a new UI/UX for user communication sound exciting\n(The latter could result in some bike shedding, and then having enough\ntime before you spend time to do it one way or another in D&E sounds\nfavorable)\n\n>>>   * Should I split D and G?  (Current plan was to keep D together, but\n>>>     split G into five short slightly inter-dependent topics)\n>>\n>> I would have expected to have tests (G) as a companion of (A) or (C)\n>> rather than (D), as performance improvements would keep the test suite\n>> unchanged?\n>\n> Let me re-phrase: D and G are completely independent series, both of\n> which happen to be divisible.  Should either of them be split?\n\nSorry for my confusion. This clears it up\n\n>\n> More background: D is only a handful of commits, so far; the main\n> reason to split it is to allow some of it to go first (maybe even\n> before G or H).  The downside is introducing extra churn and risk in\n> merge-recursive, when I'm planning to rewrite it soon anyway.  I was\n> trying to minimize merge-recursive changes, other than trying to make\n> sure that the new merge strategy and merge-recursive will give\n> identical results (modulo maybe fixing a few extra corner cases and\n> running faster).  Basically, I wanted it to be really easy to compare\n> old and new strategies, but otherwise wanted to leave merge-recursive\n> mostly alone.  It's not entirely clear how quickly I'll find time to\n> work on all of this, though, so maybe just-wait-for-the-rewrite is not\n> the right prioritization.\n\nMy experience is that smaller patch series are reviewed faster,\nso if the cost of splitting them up is not prohibitive, I'd strongly\nconsider doing that.\n\n>>>   * E is kind of big.  Are there any specific things folks would like to see\n>>>     with how that is handled?\n>>\n>> How much abstraction can be done ahead of time such that there is an\n>> interface/API where you just plug in a new merge strategy and do not\n>> need to duplicate a lot of code/tests?\n>\n> For avoiding duplicate tests, my plan was to use a variable (like\n> GIT_TEST_SPLIT_INDEX or GIT_FSMONITOR_TEST), which, when set, would\n> change the default merge strategy from \"recursive\" to the new one, and\n> also replace explicit requests for the \"recursive\" merge strategy with\n> the new one.\n\nSo your long term plan is to *replace* the whole merge recursive strategy\ngiving the same results [analogous to implementing refstable as a new refs\nback end, that behaves the same except faster and easier code for corner\ncases] and not offer a *new* strategy [analog here: different (merge/)diff\nstrategies, such as Myers vs minimal vs patience] that yields user visible\ndifference and it is unclear whether the user prefers one or the other?\nThat wasn't clear to me, thanks for clarifying.\n\nMy original reply was assuming a new \"mode\" of merge strategy which\nwould be a user visible knob to turn. The environment variable you mentioned\nsounds like that is an implementation detail, that is just for a\nsmooth transition\nperiod, but the current code would be deprecated and deleted eventually.\n\n> For avoiding duplicate code...well, Junio's suggestion was \"[to\n> rewrite] without using much from the existing code in\n> merge-recursive.c at all\" because he has \"written off that code as\n> mostly unsalvageable\"[4].\n>\n> There are some functions that I think are worth leaving intact (e.g.\n> helpers like output(), the directory rename detection stuff, or the\n> merge_submodule code that you recently moved).  That stuff I was\n> planning to put in a \"/* COPIED FROM merge-recursive.c VERBATIM */\"\n> section.  And eventually, the idea would be to delete the old\n> merge-recursive and just use the replacement, making the duplication\n> concerns eventually go away.\n\nThis clears up even more, thanks for clarifying!\n\n> ...or is your question more about how to abstract things so that\n> others can also write new merge strategies in the future and allow as\n> much code and test re-use as possible?\n\nThat was my original question as I thought an API would help with\ndoing that as it would help to not fiddle around with the rest of the\ncode base.\n\n>>> [4] https://public-inbox.org/git/xmqqk1ydkbx0.fsf@gitster.mtv.corp.google.com/\n\nDid you mean:\nhttps://public-inbox.org/git/xmqqd147kpdm.fsf@gitster.mtv.corp.google.com/\n\nSo in that case my reply above (\"..so it depends pretty strongly on E..\"\nI have a different conclusion) is void, as that *is* the new strategy?\n\nThanks,\nStefan\n"},{"id":"348782","messageId":"CABPp-BG2fZHm3s-yrzxyGj3Eh+O7_LHLz5pgstHhG2drigSyRA@mail.gmail.com","threadId":"48587","inReplyTo":"CAGZ79kYGTaFg7NnfxzdXQYTNddvqbUD3hfWEiq1RBETjYk=3gQ@mail.gmail.com","subject":"Re: RFC: Merge-related plans","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-30T04:21:42Z","receivedAt":"2018-05-30T04:21:49Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Stefan,\n\nOn Tue, May 29, 2018 at 3:12 PM, Stefan Beller <sbeller@google.com> wrote:\n>>> (B) sounds like an independent feature, which could go in parallel?\n>>\n>> B may sound like an independent feature, but it needs a merge\n>> algorithm that doesn't mess with the working tree\n>\n> I agree on that,\n>\n>> so it depends pretty strongly on E.\n>\n> .. but not quite on the conclusion:\n> You could also make the current merge algorithm working\n> tree independent. Thomas Rast (cc'd) did so IIUC in\n> https://public-inbox.org/git/cover.1409860234.git.tr@thomasrast.ch/\n> (search the archive for \"--remerge\" if interested in this prior work)\n> which did not land upstream.\n\nI'm aware of Thomas' remerge-diff series (it's where I got the name\nfrom, and I linked to it in\nhttps://bugs.chromium.org/p/git/issues/detail?id=12).  It's some\npretty cool work.  I also submitted a previous series to make\nmerge-recursive not touch the working tree (for different reasons),\nand then when folks pointed out the similarity between my series and\nThomas' in this area, I compared our approaches:\n\nhttps://public-inbox.org/git/CABPp-BG49Gr3Kf8Q3E6Vc=GF9MG+m10HkhkwbaOBfzs1cFcgVw@mail.gmail.com/\n\nI still think my approach to the do-not-touch-worktree aspect was\ncleaner, but both complicate the already excessively complex\nmerge-recursive codebase, and just dance around the fundamental design\nflaw in merge-recursive (discussed more at the end of the email via\nlinks to Junio's thoughts on the topic).  At this point, I'd rather\njust fix the design flaw rather than complicate the code further.\n\n>> If the idea is to give feedback on *code* rather than just\n>> ideas/tradeoffs/pinpointing-buggy-lines, then it sounds like you're\n>> actually suggesting posting the RFC later rather than earlier?\n>\n> Some people have complained that they don't get feedback on\n> ideas/tradeoffs/pinpointing-buggy-lines, but did so after sending\n> RFC code. So I'd think a sure way to get feedback is to send actual\n> code as an RFC even if it misses some parts.\n\nMakes sense.\n\n>> Also, the bigger question for me wasn't so much \"should I ask the list\n>> about these changes?\" before making them, but rather: Do folks want me\n>> to bring these things up before I work on D & E -- even if I end up\n>> not getting back to incorporating their answers for months until D & E\n>> are completed and merged?\n>\n> I'd think A + C are worth asking early nevertheless, even if D & E are\n> away for some month; having these niche cases covered (in code and\n> tests) as well as a new UI/UX for user communication sound exciting\n> (The latter could result in some bike shedding, and then having enough\n> time before you spend time to do it one way or another in D&E sounds\n> favorable)\n\nOkay, sounds like I need to put a little code behind my ideas, at\nleast enough for some RFCs.\n\n> My experience is that smaller patch series are reviewed faster,\n> so if the cost of splitting them up is not prohibitive, I'd strongly\n> consider doing that.\n\nWill do.\n\n> So your long term plan is to *replace* the whole merge recursive strategy\n> giving the same results.\n\nYes, precisely.  :-)\n\n>> For avoiding duplicate code...well, Junio's suggestion was \"[to\n>> rewrite] without using much from the existing code in\n>> merge-recursive.c at all\" because he has \"written off that code as\n>> mostly unsalvageable\"[4].\n>\n>>>> [4] https://public-inbox.org/git/xmqqk1ydkbx0.fsf@gitster.mtv.corp.google.com/\n>\n> Did you mean:\n> https://public-inbox.org/git/xmqqd147kpdm.fsf@gitster.mtv.corp.google.com/\n\nThe email you link describes the new algorithm behind the replacement\nstrategy.  That's a useful email, but I was specifically trying to\nreference where I was quoting Junio from, which was the email where\nJunio suggested implementing the new algorithm as a replacement\nstrategy rather than re-factoring merge-recursive.c; those quotes came\nfrom the email link I gave.\n\n> So in that case my reply above (\"..so it depends pretty strongly on E..\"\n> I have a different conclusion) is void, as that *is* the new strategy?\n\nYes, I think Junio's suggestion is a _much_ cleaner way of getting a\ndo-not-touch-the-working-tree merge than either Thomas' or my previous\nsubmissions. It's a much, much bigger chunk of work to do it that way,\nbut will fix multiple other problems that the current design make hard\nor impossible to fix.\n\n\nThanks for the feedback!\nElijah\n"}]}