{"thread":{"id":"47207","subject":"submodules and merging (Was: Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue)","startedAt":"2017-11-14T17:17:36Z","lastAt":"2017-12-04T20:17:00Z","messageCount":7,"participants":["Elijah Newren","Stefan Beller","Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":30},"messages":[{"id":"332555","messageId":"CABPp-BHDrw_dAESic3xK7kC3jMgKeNQuPQF69OpbVYhRkbhJsw@mail.gmail.com","threadId":"47207","inReplyTo":null,"subject":"submodules and merging (Was: Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-14T17:17:21Z","receivedAt":"2017-11-14T17:17:36Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Nov 13, 2017 at 3:46 PM, Stefan Beller wrote:\n> On Mon, Nov 13, 2017 at 3:39 PM, Elijah Newren wrote:\n>> On Mon, Nov 13, 2017 at 2:12 PM, Stefan Beller wrote:\n>>> I wanted to debug a very similar issue today just after reviewing this\n>>> series, see\n>>> https://public-inbox.org/git/743acc29-85bb-3773-b6a0-68d4a0b8fd63@ispras.ru/\n>>\n>> Oh, bleh. That's not a D/F conflict at all, it's the code assuming\n>> there's a D/F conflict because the entry it is processing (\"sub\") is a\n>> submodule rather than a file, and it panics when it sees \"a directory\n>> in the way\" -- a directory that just so happens to be named \"sub\" and\n>> which is in fact the desired submodule, meaning that the working\n>> directory is already good and needs no changes.\n>\n> yup, I came to find the same snippet of code to be the offender,\n> I just haven't figured out how to fix this bug.\n>\n> Thanks for taking a look!\n>\n> As you have a lot of fresh knowledge in the merge-recursive case\n> currently, how would we approach the fix here?\n\nsubmodules and merging looks pretty broken, to me. Backing off from\nthe current bug and just looking at merging with submodules in\ngeneral, makes me a little uneasy with what I see. I mean, just look\nat update_file_flags, when it wants the working directory updated, the\ncode for a submodule is the following:\n\n if (update_wd) {\n\n\n if (S_ISGITLINK(mode)) {\n /*\n * We may later decide to recursively descend into\n * the submodule directory and update its index\n * and/or work tree, but we do not do that now.\n */\n update_wd = 0;\n goto update_index;\n }\n\nSo, it just doesn't put anything there, so the working directory is\nmade out-of-date immediately. Users are happy with that? Maybe it is\nwhat makes sense, but it surprised me.\n\nFrom there, we can start stacking on the weird. For example let's say\nwe have this setup:\nO (merge base): path/contents containing \"whatever\\n\"\nA (ours): path is a submodule\nB (theirs): path/contents containing \"whatever\\nworks\\n\"\n\nMerging A into B results in\n\nCONFLICT (modify/delete): path/contents deleted in A and modified in\nHEAD. Version HEAD of path/contents left in tree.\nCONFLICT (directory/file): There is a directory with name path in\nHEAD. Adding path as path~A\nAutomatic merge failed; fix conflicts and then commit the result.\n\nbut path~A is never created, because of the update_file_flags code\nsnippet above. If the user looks at the path directory, it doesn't\nhave any submodule info either. It has just \"disappeared\", despite\nthe claim made in the conflict notice. Maybe okay, but slightly\nconfusing.\n\nNow let's say the user just wants to back out of this merge. What if\nthey run 'git merge --abort'? Well, they're greeted with:\n\nerror: 'path/contents' appears as both a file and as a directory\nerror: path/contents: cannot drop to stage #0\nerror: Untracked working tree file 'path/contents' would be\noverwritten by merge.\nfatal: Could not reset index file to revision 'HEAD'.\n\n\"Would be overwritten by merge\"? We're unmerging, and I agree it\nshouldn't be overwritten, but the fact that it thinks it needs to try\nis worrisome. And I don't like the fact that it just outright failed.\nOkay, what about 'git reset --hard' at this point:\n\nerror: 'path/contents' appears as both a file and as a directory\nerror: path/contents: cannot drop to stage #0\nwarning: unable to rmdir path: Directory not empty\nHEAD is now at 3e098a0 B\n\nCannot drop to stage #0? Oh, boy. Things must be really messed up.\nExcept they're not; it did drop path/contents to stage #0 despite the\nerror, and git status is clean again.\n\nNow, let's switch tracks and look at things from the other side. What\nif the user had been on A and tried to merge B into A?\n\n$ git checkout A\nSwitched to branch 'A'\n$ git merge B\nCONFLICT (modify/delete): path/contents deleted in HEAD and modified\nin B. Version B of path/contents left in tree.\nAdding path\nAutomatic merge failed; fix conflicts and then commit the result.\n\npath/contents is left in the tree? But there's a submodule in the\nway! Did it really put it there...? Yep:\n\n$ ls -al path/\ntotal 16\ndrwxrwxr-x 3 newren newren 4096 Nov 14 08:40 .\ndrwxrwxr-x 4 newren newren 4096 Nov 14 08:38 ..\n-rw-rw-r-- 1 newren newren 15 Nov 14 08:40 contents\n-rw-rw-r-- 1 newren newren 0 Nov 14 08:39 foo\ndrwxrwxr-x 8 newren newren 4096 Nov 14 08:40 .git\n\nAt least git add and git rm on that location, from the supermodule, do\nthe right thing.\n\n$ git add path/contents\nfatal: Pathspec 'path/contents' is in submodule 'path'\n$ git rm path/contents\npath/contents: needs merge\nrm 'path/contents'\n\nBut the fact that merge-recursive wrote unmerged entries back to the\nworking tree within the submodule feels pretty weird and dirty to me.\n(Luckily, if the locations conflict with something in the submodule,\nit'll write things out to an alternate path, such as path/contents~B,\nso it merely muddies the submodule rather than destroying stuff within\nit).\n\n\nAlso, now I'm worried that instead of just D/F (directory/file)\nconflicts, we now have two new classes that need to be checked\neverywhere throughout the code: submodule/file and submodule/directory\nconflicts. No idea how pervasive of a problem that is. Maybe there's\na clever way to handle them all with the current D/F conflict code (it\ncertainly didn't take submodules into consideration when it was\nwritten), but someone would have to take a careful look. There are\nalready many codepaths due to many different conflict types, and folks\nalready have to keep several things in their head: renames, directory\nrenames, D/F conflicts, recursive cases, dirty files, untracked\nfiles/directories, and now submodules. It would be nice if we could\nreduce the number of things folks have to focus on all at once while\nreading merge-recursive.c. I'm really starting to think we should at\nleast change how unpack_trees() and merge-recursive interoperate,\nbecause at least the dirty files & untracked files/directories should\nbe able to be split off, so folks can focus on just the index for most\nthe code paths, and everything working tree related could be handled\nseparately. But that's a topic for a different thread.\n\nAnyway, circling back the problem you brought up, there is actually a\nfairly easy fix if you want _just_ this issue cleaned up. I'll post a\nsimple patch, but it doesn't address the above problems so it feels a\nbit like a band-aid to me.\n"},{"id":"332558","messageId":"CAGZ79kbRY1gY8L361o568391Efoa1epZ1dFQZcTqJiOE7EAJfw@mail.gmail.com","threadId":"47207","inReplyTo":"CABPp-BHDrw_dAESic3xK7kC3jMgKeNQuPQF69OpbVYhRkbhJsw@mail.gmail.com","subject":"Re: submodules and merging (Was: Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-14T18:13:28Z","receivedAt":"2017-11-14T18:13:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Thanks for your reply!\n\nOn Tue, Nov 14, 2017 at 9:17 AM, Elijah Newren wrote:\n> On Mon, Nov 13, 2017 at 3:46 PM, Stefan Beller wrote:\n>> On Mon, Nov 13, 2017 at 3:39 PM, Elijah Newren wrote:\n>>> On Mon, Nov 13, 2017 at 2:12 PM, Stefan Beller wrote:\n>>>> I wanted to debug a very similar issue today just after reviewing this\n>>>> series, see\n>>>> https://public-inbox.org/git/743acc29-85bb-3773-b6a0-68d4a0b8fd63@ispras.ru/\n>>>\n>>> Oh, bleh. That's not a D/F conflict at all, it's the code assuming\n>>> there's a D/F conflict because the entry it is processing (\"sub\") is a\n>>> submodule rather than a file, and it panics when it sees \"a directory\n>>> in the way\" -- a directory that just so happens to be named \"sub\" and\n>>> which is in fact the desired submodule, meaning that the working\n>>> directory is already good and needs no changes.\n>>\n>> yup, I came to find the same snippet of code to be the offender,\n>> I just haven't figured out how to fix this bug.\n>>\n>> Thanks for taking a look!\n>>\n>> As you have a lot of fresh knowledge in the merge-recursive case\n>> currently, how would we approach the fix here?\n>\n> submodules and merging looks pretty broken, to me. Backing off from\n> the current bug and just looking at merging with submodules in\n> general, makes me a little uneasy with what I see. I mean, just look\n> at update_file_flags, when it wants the working directory updated, the\n> code for a submodule is the following:\n>\n> if (update_wd) {\n> \n>\n> if (S_ISGITLINK(mode)) {\n> /*\n> * We may later decide to recursively descend into\n> * the submodule directory and update its index\n> * and/or work tree, but we do not do that now.\n> */\n> update_wd = 0;\n> goto update_index;\n> }\n>\n> So, it just doesn't put anything there, so the working directory is\n> made out-of-date immediately. Users are happy with that? Maybe it is\n> what makes sense, but it surprised me.\n\nSubmodules are traditionally not touched by git commands and we are slowly\ntrying to get that changed. Some commands have a --recurse-submodules\nflag now, including checkout, reset; merge is missing this flag as the semantics\nare hard to define sensibly, yet.\n\n> From there, we can start stacking on the weird. For example let's say\n> we have this setup:\n> O (merge base): path/contents containing \"whatever\\n\"\n> A (ours): path is a submodule\n> B (theirs): path/contents containing \"whatever\\nworks\\n\"\n>\n> Merging A into B results in\n>\n> CONFLICT (modify/delete): path/contents deleted in A and modified in\n> HEAD. Version HEAD of path/contents left in tree.\n> CONFLICT (directory/file): There is a directory with name path in\n> HEAD. Adding path as path~A\n> Automatic merge failed; fix conflicts and then commit the result.\n>\n> but path~A is never created, because of the update_file_flags code\n> snippet above. If the user looks at the path directory, it doesn't\n> have any submodule info either. It has just \"disappeared\", despite\n> the claim made in the conflict notice. Maybe okay, but slightly\n> confusing.\n>\n> Now let's say the user just wants to back out of this merge. What if\n> they run 'git merge --abort'? Well, they're greeted with:\n>\n> error: 'path/contents' appears as both a file and as a directory\n> error: path/contents: cannot drop to stage #0\n> error: Untracked working tree file 'path/contents' would be\n> overwritten by merge.\n> fatal: Could not reset index file to revision 'HEAD'.\n>\n> \"Would be overwritten by merge\"? We're unmerging, and I agree it\n> shouldn't be overwritten, but the fact that it thinks it needs to try\n> is worrisome. And I don't like the fact that it just outright failed.\n> Okay, what about 'git reset --hard' at this point:\n>\n> error: 'path/contents' appears as both a file and as a directory\n> error: path/contents: cannot drop to stage #0\n> warning: unable to rmdir path: Directory not empty\n> HEAD is now at 3e098a0 B\n>\n> Cannot drop to stage #0? Oh, boy. Things must be really messed up.\n> Except they're not; it did drop path/contents to stage #0 despite the\n> error, and git status is clean again.\n>\n> Now, let's switch tracks and look at things from the other side. What\n> if the user had been on A and tried to merge B into A?\n>\n> $ git checkout A\n> Switched to branch 'A'\n> $ git merge B\n> CONFLICT (modify/delete): path/contents deleted in HEAD and modified\n> in B. Version B of path/contents left in tree.\n> Adding path\n> Automatic merge failed; fix conflicts and then commit the result.\n>\n> path/contents is left in the tree? But there's a submodule in the\n> way! Did it really put it there...? Yep:\n>\n> $ ls -al path/\n> total 16\n> drwxrwxr-x 3 newren newren 4096 Nov 14 08:40 .\n> drwxrwxr-x 4 newren newren 4096 Nov 14 08:38 ..\n> -rw-rw-r-- 1 newren newren 15 Nov 14 08:40 contents\n> -rw-rw-r-- 1 newren newren 0 Nov 14 08:39 foo\n> drwxrwxr-x 8 newren newren 4096 Nov 14 08:40 .git\n>\n> At least git add and git rm on that location, from the supermodule, do\n> the right thing.\n>\n> $ git add path/contents\n> fatal: Pathspec 'path/contents' is in submodule 'path'\n> $ git rm path/contents\n> path/contents: needs merge\n> rm 'path/contents'\n>\n> But the fact that merge-recursive wrote unmerged entries back to the\n> working tree within the submodule feels pretty weird and dirty to me.\n> (Luckily, if the locations conflict with something in the submodule,\n> it'll write things out to an alternate path, such as path/contents~B,\n> so it merely muddies the submodule rather than destroying stuff within\n> it).\n>\n>\n> Also, now I'm worried that instead of just D/F (directory/file)\n> conflicts, we now have two new classes that need to be checked\n> everywhere throughout the code: submodule/file and submodule/directory\n> conflicts. No idea how pervasive of a problem that is. Maybe there's\n> a clever way to handle them all with the current D/F conflict code (it\n> certainly didn't take submodules into consideration when it was\n> written), but someone would have to take a careful look. There are\n> already many codepaths due to many different conflict types, and folks\n> already have to keep several things in their head: renames, directory\n> renames, D/F conflicts, recursive cases, dirty files, untracked\n> files/directories, and now submodules. It would be nice if we could\n> reduce the number of things folks have to focus on all at once while\n> reading merge-recursive.c. I'm really starting to think we should at\n> least change how unpack_trees() and merge-recursive interoperate,\n> because at least the dirty files & untracked files/directories should\n> be able to be split off, so folks can focus on just the index for most\n> the code paths, and everything working tree related could be handled\n> separately. But that's a topic for a different thread.\n>\n> Anyway, circling back the problem you brought up, there is actually a\n> fairly easy fix if you want _just_ this issue cleaned up. I'll post a\n> simple patch, but it doesn't address the above problems so it feels a\n> bit like a band-aid to me.\n\nYes, introducing submodules into the mix is a big mess, because\nin the tree it is recorded as if it is a file (only the top level\nentry at path/)\nbut on the file system a submodule is represented as a directory\nwith contents, so the conflict detection is harder, too.\n\nI had the idea of introducing a command that can \"internalize\"\na submodule. This would take the tree recorded in the submodule\ncommit and put it where the submodule was mounted,\nThe opposite is \"externalizing\" a submodule, which would turn\na tree into a submodule. (One of the many question is if it will take\nthe whole history or start with a new fresh initial commit).\n\nBut this line of though might be distracting from your original point,\nwhich was that we have so much to keep in mind when doing tree\noperations (flags, D/F conflicts, now submodules too). I wonder how\na sensible refactoring would look like to detangle all these aspects,\nbut still keeping Git fast and not overengineered.\n\nThanks,\nStefan\n"},{"id":"332609","messageId":"CA+P7+xorpJGrn=5zTrAPVDwHLkU5vCphOrebkH_d+MxV1k1oRg@mail.gmail.com","threadId":"47207","inReplyTo":"CAGZ79kbRY1gY8L361o568391Efoa1epZ1dFQZcTqJiOE7EAJfw@mail.gmail.com","subject":"Re: submodules and merging (Was: Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue)","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-11-15T17:13:55Z","receivedAt":"2017-11-15T17:14:51Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Nov 14, 2017 at 10:13 AM, Stefan Beller wrote:\n> Thanks for your reply!\n>\n> On Tue, Nov 14, 2017 at 9:17 AM, Elijah Newren wrote:\n>> On Mon, Nov 13, 2017 at 3:46 PM, Stefan Beller wrote:\n>>> On Mon, Nov 13, 2017 at 3:39 PM, Elijah Newren wrote:\n>>>> On Mon, Nov 13, 2017 at 2:12 PM, Stefan Beller wrote:\n>>>>> I wanted to debug a very similar issue today just after reviewing this\n>>>>> series, see\n>>>>> https://public-inbox.org/git/743acc29-85bb-3773-b6a0-68d4a0b8fd63@ispras.ru/\n>>>>\n>>>> Oh, bleh. That's not a D/F conflict at all, it's the code assuming\n>>>> there's a D/F conflict because the entry it is processing (\"sub\") is a\n>>>> submodule rather than a file, and it panics when it sees \"a directory\n>>>> in the way\" -- a directory that just so happens to be named \"sub\" and\n>>>> which is in fact the desired submodule, meaning that the working\n>>>> directory is already good and needs no changes.\n>>>\n>>> yup, I came to find the same snippet of code to be the offender,\n>>> I just haven't figured out how to fix this bug.\n>>>\n>>> Thanks for taking a look!\n>>>\n>>> As you have a lot of fresh knowledge in the merge-recursive case\n>>> currently, how would we approach the fix here?\n>>\n>> submodules and merging looks pretty broken, to me. Backing off from\n>> the current bug and just looking at merging with submodules in\n>> general, makes me a little uneasy with what I see. I mean, just look\n>> at update_file_flags, when it wants the working directory updated, the\n>> code for a submodule is the following:\n>>\n>> if (update_wd) {\n>> \n>>\n>> if (S_ISGITLINK(mode)) {\n>> /*\n>> * We may later decide to recursively descend into\n>> * the submodule directory and update its index\n>> * and/or work tree, but we do not do that now.\n>> */\n>> update_wd = 0;\n>> goto update_index;\n>> }\n>>\n>> So, it just doesn't put anything there, so the working directory is\n>> made out-of-date immediately. Users are happy with that? Maybe it is\n>> what makes sense, but it surprised me.\n>\n> Submodules are traditionally not touched by git commands and we are slowly\n> trying to get that changed. Some commands have a --recurse-submodules\n> flag now, including checkout, reset; merge is missing this flag as the semantics\n> are hard to define sensibly, yet.\n>\n\nYea, figuring out how to represent a conflict with submodules is\npretty challenging, especially in cases where one side has a submodule\nand the other side does not.\n\n> Yes, introducing submodules into the mix is a big mess, because\n> in the tree it is recorded as if it is a file (only the top level\n> entry at path/)\n> but on the file system a submodule is represented as a directory\n> with contents, so the conflict detection is harder, too.\n>\n> I had the idea of introducing a command that can \"internalize\"\n> a submodule. This would take the tree recorded in the submodule\n> commit and put it where the submodule was mounted,\n> The opposite is \"externalizing\" a submodule, which would turn\n> a tree into a submodule. (One of the many question is if it will take\n> the whole history or start with a new fresh initial commit).\n>\n> But this line of though might be distracting from your original point,\n> which was that we have so much to keep in mind when doing tree\n> operations (flags, D/F conflicts, now submodules too). I wonder how\n> a sensible refactoring would look like to detangle all these aspects,\n> but still keeping Git fast and not overengineered.\n\nI think given how complex a lot of these code paths are, that an\nattempt to refactor it a bit to detangle some of the mess would be\nwell worth the time. I'd suspect it might make handling the more\ncomplex task of actually resolving conflicts to be easier, so the\neffort to clean up the code here should be worth it.\n\nThanks,\nJake\n\n>\n> Thanks,\n> Stefan\n"},{"id":"333493","messageId":"CABPp-BFdJr8AL3hJSSLiqwPJMt6LZcLOEcTtxz1vohEuw==wKg@mail.gmail.com","threadId":"47207","inReplyTo":"CA+P7+xorpJGrn=5zTrAPVDwHLkU5vCphOrebkH_d+MxV1k1oRg@mail.gmail.com","subject":"Re: submodules and merging (Was: Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-25T22:37:29Z","receivedAt":"2017-11-25T22:37:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Nov 15, 2017 at 9:13 AM, Jacob Keller wrote:\n> On Tue, Nov 14, 2017 at 10:13 AM, Stefan Beller wrote:\n\n>> But this line of though might be distracting from your original point,\n>> which was that we have so much to keep in mind when doing tree\n>> operations (flags, D/F conflicts, now submodules too). I wonder how\n>> a sensible refactoring would look like to detangle all these aspects,\n>> but still keeping Git fast and not overengineered.\n>\n> I think given how complex a lot of these code paths are, that an\n> attempt to refactor it a bit to detangle some of the mess would be\n> well worth the time. I'd suspect it might make handling the more\n> complex task of actually resolving conflicts to be easier, so the\n> effort to clean up the code here should be worth it.\n\nI think changing from a 4-way merge to a 3-way merge would make things\nmuch better, as Junio outlined here:\n\nhttps://public-inbox.org/git/xmqqd147kpdm.fsf@gitster.mtv.corp.google.com/\n\nI don't know of any way to detangle the other aspects, yet.\n"},{"id":"333520","messageId":"CA+P7+xonU838dOAzsUY4KmweYyM-NLFQeKVYD9jYM7i5EuWXcw@mail.gmail.com","threadId":"47207","inReplyTo":"CABPp-BFdJr8AL3hJSSLiqwPJMt6LZcLOEcTtxz1vohEuw==wKg@mail.gmail.com","subject":"Re: submodules and merging (Was: Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue)","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-11-26T05:59:04Z","receivedAt":"2017-11-26T05:59:31Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sat, Nov 25, 2017 at 2:37 PM, Elijah Newren wrote:\n> On Wed, Nov 15, 2017 at 9:13 AM, Jacob Keller wrote:\n>> On Tue, Nov 14, 2017 at 10:13 AM, Stefan Beller wrote:\n>\n>>> But this line of though might be distracting from your original point,\n>>> which was that we have so much to keep in mind when doing tree\n>>> operations (flags, D/F conflicts, now submodules too). I wonder how\n>>> a sensible refactoring would look like to detangle all these aspects,\n>>> but still keeping Git fast and not overengineered.\n>>\n>> I think given how complex a lot of these code paths are, that an\n>> attempt to refactor it a bit to detangle some of the mess would be\n>> well worth the time. I'd suspect it might make handling the more\n>> complex task of actually resolving conflicts to be easier, so the\n>> effort to clean up the code here should be worth it.\n>\n> I think changing from a 4-way merge to a 3-way merge would make things\n> much better, as Junio outlined here:\n>\n> https://public-inbox.org/git/xmqqd147kpdm.fsf@gitster.mtv.corp.google.com/\n>\n> I don't know of any way to detangle the other aspects, yet.\n\nI agree, that is absolutely a (big) step in the right direction.\n\nThanks,\nJake\n"},{"id":"334080","messageId":"CAGZ79kYbhTyHRMRvZaq3SC7MpxdruXsr+4nKGo-4RojjymwYVA@mail.gmail.com","threadId":"47207","inReplyTo":"CA+P7+xonU838dOAzsUY4KmweYyM-NLFQeKVYD9jYM7i5EuWXcw@mail.gmail.com","subject":"Re: submodules and merging (Was: Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-04T19:33:28Z","receivedAt":"2017-12-04T19:33:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Nov 25, 2017 at 9:59 PM, Jacob Keller wrote:\n> On Sat, Nov 25, 2017 at 2:37 PM, Elijah Newren wrote:\n>> On Wed, Nov 15, 2017 at 9:13 AM, Jacob Keller wrote:\n>>> On Tue, Nov 14, 2017 at 10:13 AM, Stefan Beller wrote:\n>>\n>>>> But this line of though might be distracting from your original point,\n>>>> which was that we have so much to keep in mind when doing tree\n>>>> operations (flags, D/F conflicts, now submodules too). I wonder how\n>>>> a sensible refactoring would look like to detangle all these aspects,\n>>>> but still keeping Git fast and not overengineered.\n>>>\n>>> I think given how complex a lot of these code paths are, that an\n>>> attempt to refactor it a bit to detangle some of the mess would be\n>>> well worth the time. I'd suspect it might make handling the more\n>>> complex task of actually resolving conflicts to be easier, so the\n>>> effort to clean up the code here should be worth it.\n>>\n>> I think changing from a 4-way merge to a 3-way merge would make things\n>> much better, as Junio outlined here:\n>>\n>> https://public-inbox.org/git/xmqqd147kpdm.fsf@gitster.mtv.corp.google.com/\n>>\n>> I don't know of any way to detangle the other aspects, yet.\n\nJonathan Nieder and me tried some pair programming some time ago[1]\nplumbing the repository object through most of the low level internals, which\nwould help in detangling submodule merges as then these merges could\nbe done in-core, just as Junio laid out.\n\n[1] https://github.com/stefanbeller/git/tree/object-store-jrn-rebased\n\n> I agree, that is absolutely a (big) step in the right direction.\n\n\nI agree as well; A better (abstracted) merge backend would be huge for\nthe future of Git.\n\nThanks,\nStefan\n"},{"id":"334085","messageId":"CA+P7+xrTZZn0r8JBG+=tjEN__jVsLZrNxbA9B_m0PNu2+c0hGg@mail.gmail.com","threadId":"47207","inReplyTo":"CAGZ79kYbhTyHRMRvZaq3SC7MpxdruXsr+4nKGo-4RojjymwYVA@mail.gmail.com","subject":"Re: submodules and merging (Was: Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue)","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-12-04T20:16:33Z","receivedAt":"2017-12-04T20:17:00Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Dec 4, 2017 at 11:33 AM, Stefan Beller wrote:\n> On Sat, Nov 25, 2017 at 9:59 PM, Jacob Keller wrote:\n>> On Sat, Nov 25, 2017 at 2:37 PM, Elijah Newren wrote:\n>>> On Wed, Nov 15, 2017 at 9:13 AM, Jacob Keller wrote:\n>>>> On Tue, Nov 14, 2017 at 10:13 AM, Stefan Beller wrote:\n>>>\n>>>>> But this line of though might be distracting from your original point,\n>>>>> which was that we have so much to keep in mind when doing tree\n>>>>> operations (flags, D/F conflicts, now submodules too). I wonder how\n>>>>> a sensible refactoring would look like to detangle all these aspects,\n>>>>> but still keeping Git fast and not overengineered.\n>>>>\n>>>> I think given how complex a lot of these code paths are, that an\n>>>> attempt to refactor it a bit to detangle some of the mess would be\n>>>> well worth the time. I'd suspect it might make handling the more\n>>>> complex task of actually resolving conflicts to be easier, so the\n>>>> effort to clean up the code here should be worth it.\n>>>\n>>> I think changing from a 4-way merge to a 3-way merge would make things\n>>> much better, as Junio outlined here:\n>>>\n>>> https://public-inbox.org/git/xmqqd147kpdm.fsf@gitster.mtv.corp.google.com/\n>>>\n>>> I don't know of any way to detangle the other aspects, yet.\n>\n> Jonathan Nieder and me tried some pair programming some time ago[1]\n> plumbing the repository object through most of the low level internals, which\n> would help in detangling submodule merges as then these merges could\n> be done in-core, just as Junio laid out.\n>\n> [1] https://github.com/stefanbeller/git/tree/object-store-jrn-rebased\n>\n\nThis sounds promising to me.\n\nThanks,\nJake\n\n>> I agree, that is absolutely a (big) step in the right direction.\n>\n>\n> I agree as well; A better (abstracted) merge backend would be huge for\n> the future of Git.\n>\n> Thanks,\n> Stefan\n"}]}