{"thread":{"id":"7605","subject":"Rebase, please help","startedAt":"2007-04-11T01:52:00Z","lastAt":"2007-04-12T21:22:58Z","messageCount":8,"participants":["Alexander Litvinov","Junio C Hamano","Alex Riesen","Andy Parkins","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"39083","messageId":"200704110852.00540.litvinov2004@gmail.com","threadId":"7605","inReplyTo":null,"subject":"Rebase, please help","fromName":"Alexander Litvinov","fromEmail":"litvinov2004@gmail.com","sentAt":"2007-04-11T01:52:00Z","receivedAt":"2007-04-11T01:52:00Z","isPatch":false,"sender":{"key":"litvinov2004@gmail.com","avatar":null},"body":"Hello list.\n\nI have found that rebase have (new) option : --merge\nLooking at the code show me that regular rebase is a simply format-patch and \nam but --merge (or -s) use some merge stratyegy to merge changes between two \ncommits into current head.\n\nWhat is --merge for ? Will the result be the same ?\n"},{"id":"39091","messageId":"7v8xczqs1q.fsf@assigned-by-dhcp.cox.net","threadId":"7605","inReplyTo":"200704110852.00540.litvinov2004@gmail.com","subject":"Re: Rebase, please help","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-11T07:38:41Z","receivedAt":"2007-04-11T07:38:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Litvinov writes:\n\n> I have found that rebase have (new) option : --merge\n> Looking at the code show me that regular rebase is a simply format-patch and \n> am but --merge (or -s) use some merge stratyegy to merge changes between two \n> commits into current head.\n>\n> What is --merge for ? Will the result be the same ?\n\nRegular \"rebase\" uses \"format-patch\" piped to \"am -3\", so if you\ndo not have renames the file-level patch conflict can be\nresolved using the 3-way merge logic. However, because we do\nnot give -M to format-patch, it does not deal with case where\nyou have renames in the series of commits you are rebasing, nor\nwhere you have renames between the current base commit and the\ncommit you are rebasing onto (the latter won't be solved with\ngiving -M to format-patch anyway, so we do not even try).\n\nIn cases involving such renames, giving --merge option would\nprobably be nicer to work with. It invokes merge-recursive\nlogic to deal with the renames.\n\nI find that the regular rebase without --merge is faster (at\nleast it feels to me that it is, and I kind of understand why;\npatch application to write out a tree is optimized to take\nadvantage of cache-tree extension, as opposed to merging three\ntrees which clobbers it), when there is no patch conflict.\nSince most rebases do not involve patch conflict for me and\nseldom involve rebases, I almost never use --merge myself, but\nthis would depend highly on personal taste and project.\n"},{"id":"39092","messageId":"81b0412b0704110048j30193650r6a7e7417a9afeaf8@mail.gmail.com","threadId":"7605","inReplyTo":"200704110852.00540.litvinov2004@gmail.com","subject":"Re: Rebase, please help","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-11T07:48:01Z","receivedAt":"2007-04-11T07:48:01Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 4/11/07, Alexander Litvinov wrote:\n>\n> What is --merge for ? Will the result be the same ?\n\nMaybe, maybe not. It uses merge strategies instead of git-am\nand has advantages over blindly applying the patches (it can\nknow how a change got in, and it uses resolved conflict cache).\n"},{"id":"39105","messageId":"7vr6qrnszb.fsf@assigned-by-dhcp.cox.net","threadId":"7605","inReplyTo":"81b0412b0704110048j30193650r6a7e7417a9afeaf8@mail.gmail.com","subject":"Re: Rebase, please help","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-11T09:46:48Z","receivedAt":"2007-04-11T09:46:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" writes:\n\n> On 4/11/07, Alexander Litvinov wrote:\n>>\n>> What is --merge for ? Will the result be the same ?\n>\n> Maybe, maybe not. It uses merge strategies instead of git-am\n> and has advantages over blindly applying the patches (it can\n> know how a change got in, and it uses resolved conflict cache).\n\nI think \"blindly applying the patches\" is a gross overstatement,\nas merge-recursive will get confused the same way as git-apply,\nwhen a difference that comes from the two commits can be applied\nto two places. When the forward-ported change gets conflict,\n3-way merge logic in \"git-am -3\" kicks in and does a fall back\nto merge-recursive on a reconstructed tree that has only the\npaths relevant to the case.\n\nWith \"format-patch piped to am-3\", we could give the -M option\nto \"format-patch\" to deal with renames that happen in the series\nyou are rebasing, but renames between the bases (the original\nbase commit for the series and the new \"onto\" commit) is not\nsomething it can handle sensibly.\n\nThat is the true advantage --merge has over \"format-patch piped\nto am-3\", as it always drives merge-recursive and it can notice\nrenames between the two bases.\n\nBut always driving merge-recursive is also its weakness. When\nthe series being rebased is simple and long, especially on a big\ntree, applying many patches without conflicts tends to\noutperform running the same number of merges, as the patch\napplication is tuned to take advantage of cache-tree while\nread-tree based merge essentially trashes cache-tree, and has to\npay the full cost of write-tree for every commit it makes.\n\nAlso there is that small D/F conflict problem merge-recursive\nhas that I told you about, which does not exist in git-apply ;-)\nDid you have a chance to take a look at it yet?\n"},{"id":"39109","messageId":"200704111110.18461.andyparkins@gmail.com","threadId":"7605","inReplyTo":"7v8xczqs1q.fsf@assigned-by-dhcp.cox.net","subject":"Re: Rebase, please help","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-04-11T10:10:14Z","receivedAt":"2007-04-11T10:10:14Z","isPatch":false,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Wednesday 2007 April 11 08:38, Junio C Hamano wrote:\n\n> I find that the regular rebase without --merge is faster (at\n> least it feels to me that it is, and I kind of understand why;\n\nThis is interesting and brings to mind a difficult I've had. I had problems \nwith rebase when rebasing chains with a file that was self-similar. Indulge \nme for a while with this example (forgive the C++, but that's where I had \nthis problem):\n\nclass A : public C\n{\n // ...\n\n int someVirtualOverride(n) { return ArrayA[n]; }\n\n // ...\n}\n\nclass B : public C\n{\n // ...\n\n int someVirtualOverride(n) { return ArrayB[n]; }\n\n // ...\n}\n\nOne patch changed \"ArrayX[n]\" to \"Array.at(n)\" and another inserted more \nsimilar classes around these two.\n\nWhen I was rebasing, some strange things happened (without any conflict \nwarnings):\n\nclass D : public C\n{\n int someVirtualOverride(n) { return ArrayA.at(n); }\n}\n\nclass A : public C\n{\n int someVirtualOverride(n) { return ArrayB.at(n); }\n}\n\nclass B : public C\n{\n int someVirtualOverride(n) { return ArrayB[n]; }\n}\n\nNotice that the arrays don't match up with the classes. By some crazy \ncoincidence and the strong similarity between localities within the file, the \npatch successfully applied in the wrong place. The fix was easy enough to do \nmanually, but it needed a bit of untangling as this was in a longish chain of \nrevisions that I was rebasing.\n\nI didn't mind much, and hence didn't report it as a bug as I guessed it was to \ndo with git-rebase using git-am. The annoying part was actually that there \nwas no conflict warning and hence the rest of the chain applied, making it \nall the more difficult to untangle.\n\nMy question then is this: given that I don't care about speed of rebase, is it \nsafe to permanently use --merge with rebase, and would that have caught the \nerror in the above case?\n\n\n\nAndy\n\n-- \nDr Andy Parkins, M Eng (hons), MIET\nandyparkins@gmail.com\n"},{"id":"39113","messageId":"81b0412b0704110432o3f861d4aha68df22f88e59da3@mail.gmail.com","threadId":"7605","inReplyTo":"7vr6qrnszb.fsf@assigned-by-dhcp.cox.net","subject":"Re: Rebase, please help","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-11T11:32:08Z","receivedAt":"2007-04-11T11:32:08Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 4/11/07, Junio C Hamano wrote:\n> Also there is that small D/F conflict problem merge-recursive\n> has that I told you about, which does not exist in git-apply ;-)\n> Did you have a chance to take a look at it yet?\n\nnot yet. I was greatly distracted by subprojects\n"},{"id":"39229","messageId":"7v7ishjpm9.fsf@assigned-by-dhcp.cox.net","threadId":"7605","inReplyTo":"200704111110.18461.andyparkins@gmail.com","subject":"Re: Rebase, please help","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-12T20:37:34Z","receivedAt":"2007-04-12T20:37:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins writes:\n\n> This is interesting and brings to mind a difficult I've had.\n> I had problems with rebase when rebasing chains with a file\n> that was self-similar. Indulge me for a while with this\n> example (forgive the C++, but that's where I had this\n> problem):\n>\n> class A : public C\n> {\n> // ...\n>\n> int someVirtualOverride(n) { return ArrayA[n]; }\n>\n> // ...\n> }\n>\n> class B : public C\n> {\n> // ...\n>\n> int someVirtualOverride(n) { return ArrayB[n]; }\n>\n> // ...\n> }\n>\n> One patch changed \"ArrayX[n]\" to \"Array.at(n)\" and another inserted more \n> similar classes around these two.\n>\n> When I was rebasing, some strange things happened (without any conflict \n> warnings):\n>\n> class D : public C\n> {\n> int someVirtualOverride(n) { return ArrayA.at(n); }\n> }\n>\n> class A : public C\n> {\n> int someVirtualOverride(n) { return ArrayB.at(n); }\n> }\n>\n> class B : public C\n> {\n> int someVirtualOverride(n) { return ArrayB[n]; }\n> }\n>\n> Notice that the arrays don't match up with the classes. By\n> some crazy coincidence and the strong similarity between\n> localities within the file, the patch successfully applied in\n> the wrong place.\n\nA patch that can ambiguously apply to multiple places is indeed\na problem, and in such situations merge based rebase is probably\nsafer as it can take advantage of the whole file as the context.\n\nBut it brings up another interesting point. The ambiguous patch\nis a problem even more so outside the context of rebasing, for\nanother reason. When rebasing, you are dealing with your own\nchanges and you know what and how you want each of them to\nchange the tree state, as opposed to applying somebody else's\npatch outside the context of rebasing.\n\nWhen we only have the patch text (i.e. applymbox), there is no\n\"merge to use the whole file as the context\" fallback. I wonder\nif this is a common enough problem that we would want to make it\nsafer somehow...\n\n[jc: Since I happen to know somebody who applies more patches in\n one week than anybody else would ever apply in the lifetime\n ;-), I am CC'ing that person]\n\nI can see two possible improvements.\n\n - On the diff generation side, we could notice that the hunk\n we are going to output can be applied to more than one\n location, and automatically widen the context for it.\n\n This is only a half-solution, as many patches do not even\n come from git.\n\n - Inside git-apply, apply_one_fragment(), ask find_offset() if\n the hunk can match more than one location, and exit with an\n error status (still writing out the patch results if it\n otherwise applies cleanly) so that the user can manually\n inspect and confirm.\n"},{"id":"39230","messageId":"Pine.LNX.4.64.0704121402530.4061@woody.linux-foundation.org","threadId":"7605","inReplyTo":"7v7ishjpm9.fsf@assigned-by-dhcp.cox.net","subject":"Re: Rebase, please help","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-12T21:22:58Z","receivedAt":"2007-04-12T21:22:58Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 12 Apr 2007, Junio C Hamano wrote:\n> Andy Parkins writes:\n> >\n> > When I was rebasing, some strange things happened (without any conflict \n> > warnings):\n> \n> A patch that can ambiguously apply to multiple places is indeed\n> a problem, and in such situations merge based rebase is probably\n> safer as it can take advantage of the whole file as the context.\n\nIndeed. It's one of the reasons I think the default \"patch\" behaviour of \nallowing some fuzz in the patch is totally broken, and why \"git apply\" \ndefaults to a much stricter \"no context differences allowed\" behaviour.\n\nThat still doesn't entirely get *rid* of the problem, but it at least \nmakes it slightly less common. You can still get a patch that applies \ncleanly if you have certain multi-line patterns that are very common, and \nthat end up causing patch to find the wrong place to apply a patch, and \nstill make it look right enough.\n\nThis is just one reason why patch-based systems are totally and utterly \nbroken, and why I detested the original cogito patch-based merging.\n\nThe three-way merge behaviour (\"git rebase --merge\") is a lot safer, but \nit's obviously more expensive too.\n\nAlso, the reason a lot of people like patch-based setups is not only is it \nefficient, but it turns out too many people prefer \"clean merges\" even \n*despite* the dangers. See the recent patches that were floating around on \n\"stgit\" that actually make applying patches default to the same \n*broken*defaults* as standard \"patch\" does (see the emails with subject \nline\n\n\t\"Pass -C1 to git-apply in StGIT ...\"\n\nfor more on that).\n\nSo a lot of people prefer the less strict patch application rules, because \n*most* of the time it actually does the right thing. Never mind that it \nmakes a fundamental problem with patches even worse.\n\nMe, I'd prefer to have the patch fail early. But even failing early does \nnot _guarantee_ that it applies in the right place. \n\n> But it brings up another interesting point. The ambiguous patch\n> is a problem even more so outside the context of rebasing, for\n> another reason. When rebasing, you are dealing with your own\n> changes and you know what and how you want each of them to\n> change the tree state, as opposed to applying somebody else's\n> patch outside the context of rebasing.\n> \n> When we only have the patch text (i.e. applymbox), there is no\n> \"merge to use the whole file as the context\" fallback. I wonder\n> if this is a common enough problem that we would want to make it\n> safer somehow...\n\nWe could make \"git apply\" also refuse to move more than a few lines by \ndefault (ie not only does the context have to be exact, it has to actually \nshow up where the patch claims it should be!)\n\ngit-apply still allows arbitrary line offset differences (see \n\"find_offset()\").\n\n> I can see two possible improvements.\n> \n> - On the diff generation side, we could notice that the hunk\n> we are going to output can be applied to more than one\n> location, and automatically widen the context for it.\n> \n> This is only a half-solution, as many patches do not even\n> come from git.\n\nIt's not even a solution _within_ git. Since a patch will always apply at \nthe right place if no changes have been done (because we always start \nlooking at the line number where the patch fragment _claims_ it should \ngo), the problem only occurs when independent changes have been done to \nthe target.\n\nAnd those independent changes may obviously be the ones that *introduce* \nthe new location that the patch can (incorrectly) apply to.\n\n> - Inside git-apply, apply_one_fragment(), ask find_offset() if\n> the hunk can match more than one location, and exit with an\n> error status (still writing out the patch results if it\n> otherwise applies cleanly) so that the user can manually\n> inspect and confirm.\n\nYes, that might be a good idea, but is going to be pretty expensive. \n\"find_offset()\" wasn't exactly written to be a model of efficiency. See \nthe comment about me not being one of the \"smart and beautiful\" people.\n\nSo you'd want to have some smarter method of finding potential places to \napply the fragment if you want to do those kinds of things. Like creating \nhashes of the line contents in order to not have to compare the whole \nfragment..\n\nAnd EVEN THEN it wouldn't actually solve the problem. The most common case \nis simply:\n - somebody *already* fixed the same bug by a very similar patch (or an \n identical one), and thus the patch obviously won't apply, since the \n place it *should* apply to got changed.\n - so git-apply will look for another place to apply it, and it's quite \n possible that there is just one such place - even though it's the exact \n wrong place!\n\nSo it really does boil down to: patches will sometimes falsely apply at \nthe wrong place. That's just in the fundamental nature of patches. The \nsame way a three-way merge can sometimes generate total crap, even when it \nmerged totally cleanly.\n\nIt's unusual enough that most of the time it doesn't happen (and I think \nit happens less with three-way merges than with patches), and hopefully \nmost of the time the error will be obvious enough that it gets noticed \nquickly (ie the badly patched sources simply won't build any more!).\n\nBut there is no practical way to guarantee it cannot happen. The only way \nto guarantee that patches apply correctly is to make sure the source file \nmatches *exactly*. But that kind of defeats the whole point of sending \npatches around in the first place.\n\n\t\t\tLinus\n"}]}