{"thread":{"id":"54497","subject":"Please explain why this merge ignored changes from one branch in favor of another rather than showing a conflict","startedAt":"2020-10-24T21:07:07Z","lastAt":"2020-10-25T00:09:14Z","messageCount":2,"participants":["Johnathon Wright","Elijah Newren"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"408298","messageId":"CAD6JFnNX03MLBSUjdTjvK1TsBR-sntqgXMDKR9z2OjdUKBERKQ@mail.gmail.com","threadId":"54497","inReplyTo":null,"subject":"Please explain why this merge ignored changes from one branch in favor of another rather than showing a conflict","fromName":"Johnathon Wright","fromEmail":"jw@mustmodify.com","sentAt":"2020-10-24T21:06:40Z","receivedAt":"2020-10-24T21:07:07Z","isPatch":false,"sender":{"key":"jw@mustmodify.com","avatar":null},"body":"Your Excellencies:\n\nI'm a new tech lead trying to figure out what caused a significant\nproduction issue. I want to know why the merge described below\ndiscards the content of a certain file from one branch and uses\ncontent from another branch, while saying that the file was \"added by\nthem\". More importantly, I want to know how to prevent this in the\nfuture.\n\nDETAILS\n\nA dev wrote a feature across many files, including\napp/events/checkout_event.rb. Once merged and deployed, it was\ndiscovered that his changes to that file didn't make it to\nproduction... but many other changes from the same commit(s) did make\nit.\n\nAfter much research, we have isolated the issue to a specific ref -- a\nmerge. It appears that, during the merge where both branches had\nchanged that file, git decided to throw out changes to a from one\nbranch and to use the other branch.\n\nI created three tags so my small brain didn't have to remember refs:\n* \"working\" (aa29d0f4)\n* other_parent (7f9ebfa5).\n* broken (ccbeb88)\n\n----\njw@logopolis:/projects/unity/atlantis$ git show broken\n\ncommit cbbeb883428a468e9c906c646becdf751ecff99a (broken)\nMerge: 7f9ebfa5 aa29d0f4\nAuthor: Johnathon Wright <jw@mustmodify.com>\nDate:   Fri Oct 16 13:17:43 2020 -0400\n\n    merge\n\n...\n----\n\n'git show broken' doesn't show any changes to the file in question.\n\nHere, you can see that the files were different:\n\n----\njw@logopolis:/projects/unity/atlantis$ git ls-tree working\napp/events/checkout_event.rb\n\n100644 blob 077013dc52f03b336143b892c1468516cae29dce\napp/events/checkout_event.rb\n----\n\nand\n\n----\njw@logopolis:/projects/unity/atlantis$ git ls-tree other_parent\napp/events/checkout_event.rb\n\n100644 blob 21b3d4e9b5ac4903c2aadbed28ab5b60eb77e8ce\napp/events/checkout_event.rb\n-----\n\nThen I tried to reproduce the merge:\n\n----\njw@logopolis:/projects/unity/atlantis$ git checkout working\n\nHEAD is now at aa29d0f4 Track instances of price changing\n\njw@logopolis:/projects/unity/atlantis$ git merge other_parent\n\nCONFLICT (rename/delete): spec/events/single_payment_event_spec.rb\ndeleted in HEAD and renamed to spec/events/checkout_event_spec.rb in\nother_parent. Version other_parent of\nspec/events/checkout_event_spec.rb left in tree.\nRemoving spec/commands/generate_ticket_and_send_command_spec.rb\nAuto-merging db/schema.rb\nCONFLICT (content): Merge conflict in db/schema.rb\nAuto-merging config/routes.rb\nAuto-merging app/models/user.rb\nCONFLICT (rename/delete): app/events/single_payment_event.rb deleted\nin HEAD and renamed to app/events/checkout_event.rb in other_parent.\nVersion other_parent of app/events/checkout_event.rb left in tree.\nAuto-merging app/controllers/products_controller.rb\nAuto-merging app/controllers/priceline/price_points_controller.rb\nAutomatic merge failed; fix conflicts and then commit the result.\n\n\njw@logopolis:/projects/unity/atlantis$ git status\n\nHEAD detached at working\nYou have unmerged paths.\n  (fix conflicts and run \"git commit\")\n  (use \"git merge --abort\" to abort the merge)\n\nChanges to be committed:\n\n        new file:   app/controllers/criteria_sets_controller.rb\n        modified:   app/controllers/priceline/price_points_controller.rb\n        modified:   app/controllers/products_controller.rb\n        modified:   app/events/search_hotels_event.rb\n        modified:   app/forms/search_hotels_form.rb\n        modified:   app/interfaces/priceline_api.rb\n        modified:   app/models/priceline/criteria_set.rb\n        modified:   app/models/user.rb\n        modified:   app/queries/hotels_filter_query.rb\n        new file:   app/views/products/waiting.html.erb\n        modified:   app/workers/generate_booking_ticket_worker.rb\n        modified:   app/workers/update_priceline_hotels_worker.rb\n        modified:   config/routes.rb\n        new file:\ndb/migrate/20201009200507_add_timestamps_to_pl_criteria_sets.rb\n        new file:\ndb/migrate/20201009235952_add_completion_flag_to_criteria_sets.rb\n        deleted:    spec/commands/generate_ticket_and_send_command_spec.rb\n        modified:   spec/forms/search_hotels_form_spec.rb\n\nUnmerged paths:\n  (use \"git add <file>...\" to mark resolution)\n\n        added by them:   app/events/checkout_event.rb\n        both modified:   db/schema.rb\n        added by them:   spec/events/checkout_event_spec.rb\n----\n\nnote that even though app/events/checkout_event.rb exists in both the\n\"working\" and \"other_parent\" branches, git here says \"added by them\".\n\nNote that \"working\" and \"other_parent\" both have parent nodes which\nrename the file in question from something else TO\napp/events/checkout_event.rb (same for the spec.) The two ancestors\nhave different refs but the same datetime and commit message.\n\nThis might also be of interest, or perhaps not.\n\n----\njw@logopolis:/projects/unity/atlantis$ git diff working..broken --\napp/events/checkout_event.rb | grep \"^-\" | wc -l\n\n158\n\njw@logopolis:/projects/unity/atlantis$ git diff working..broken --\napp/events/checkout_event.rb | grep \"^+\" | wc -l\n\n51\n----\n\nFinal bit of information:\n\n----\njw@logopolis:/projects/unity/atlantis$ git --version\n\ngit version 2.17.1\n----\n\nSo, why does git say that this file was \"added by them\" instead of\nsaying it had a conflict that needed merging?\n\nIt has been suggested that this might have something to do with some\nteam members using merge and others using rebase. Or possibly rebasing\nshared history. I don't know how to determine whether that's the case\nor whether that might have caused this.\n\nIf so, I would appreciate seeing a set of instructions from a blank\nrepo that cause this same thing to happen, so that I can help my team\nto understand the situation and how it can be avoided.\n\nIf it is not related to rebasing and then merging, any help would be\nappreciated in diagnosing this issue.\n\nThanks,\n\njw\n"},{"id":"408304","messageId":"CABPp-BF80aMFWgNdr_seQkj+UvZ3=hPoT3cv5bxNhE1APCmCkQ@mail.gmail.com","threadId":"54497","inReplyTo":"CAD6JFnNX03MLBSUjdTjvK1TsBR-sntqgXMDKR9z2OjdUKBERKQ@mail.gmail.com","subject":"Re: Please explain why this merge ignored changes from one branch in favor of another rather than showing a conflict","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-10-25T00:05:52Z","receivedAt":"2020-10-25T00:09:14Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Sat, Oct 24, 2020 at 2:39 PM Johnathon Wright <jw@mustmodify.com> wrote:\n>\n> Your Excellencies:\n\nI'll respond despite not being one of the ones to whom your email was\naddressed...  ;-)\n\n> I'm a new tech lead trying to figure out what caused a significant\n> production issue. I want to know why the merge described below\n> discards the content of a certain file from one branch and uses\n> content from another branch, while saying that the file was \"added by\n> them\". More importantly, I want to know how to prevent this in the\n> future.\n\nWell, I've got bad news and good news for you, but mostly bad news...\n\n> DETAILS\n>\n> A dev wrote a feature across many files, including\n> app/events/checkout_event.rb. Once merged and deployed, it was\n> discovered that his changes to that file didn't make it to\n> production... but many other changes from the same commit(s) did make\n> it.\n>\n> After much research, we have isolated the issue to a specific ref -- a\n> merge. It appears that, during the merge where both branches had\n> changed that file, git decided to throw out changes to a from one\n> branch and to use the other branch.\n\nThis doesn't quite match what you show below.  git _did_ report a\nconflict for this path, and left the user to resolve.  Granted, there\nwas content on the other side that git should have notified the user\nabout with the conflict to make the conflict easier to resolve, but\ngit did not \"decide\" how this path should be resolved and it\nexplicitly marked it as conflicted.\n\nAll that said, I do lay the blame mostly at git's feet and not to your\ndevelopers here.  Being unable to correctly detect and report a\nrename/add/delete conflict and misreporting it as a rename/delete,\nwith only one side's content shown does seem likely to lead to this\nkind of error.  Someone would have to be very cautious in inspecting\nthe conflicts to resolve this correctly.\n\n> I created three tags so my small brain didn't have to remember refs:\n> * \"working\" (aa29d0f4)\n> * other_parent (7f9ebfa5).\n> * broken (ccbeb88)\n>\n> ----\n> jw@logopolis:/projects/unity/atlantis$ git show broken\n>\n> commit cbbeb883428a468e9c906c646becdf751ecff99a (broken)\n> Merge: 7f9ebfa5 aa29d0f4\n> Author: Johnathon Wright <jw@mustmodify.com>\n> Date:   Fri Oct 16 13:17:43 2020 -0400\n>\n>     merge\n>\n> ...\n> ----\n>\n> 'git show broken' doesn't show any changes to the file in question.\n\nThat's because git doesn't show file changes within a merge regardless\nof whether someone tweaked the merge commit by modifying every single\nfile.  You would need to add --cc or something to see some of the\ndifferences.  Or, when --remerge-diff is complete then you'd be able\nto use it, but it's still only half-baked.\n\n> Here, you can see that the files were different:\n>\n> ----\n> jw@logopolis:/projects/unity/atlantis$ git ls-tree working\n> app/events/checkout_event.rb\n>\n> 100644 blob 077013dc52f03b336143b892c1468516cae29dce\n> app/events/checkout_event.rb\n> ----\n>\n> and\n>\n> ----\n> jw@logopolis:/projects/unity/atlantis$ git ls-tree other_parent\n> app/events/checkout_event.rb\n>\n> 100644 blob 21b3d4e9b5ac4903c2aadbed28ab5b60eb77e8ce\n> app/events/checkout_event.rb\n> -----\n\nThis doesn't state what version of the file existed in the merge base,\nor merge bases.  How many merge bases where there between these two\ncommits?  What version(s) of the file existed in that commit (or those\ncommits)?  (I think I can deduce the answer from the conflicts below,\nthough, so don't worry too much about answering this.)\n\n> Then I tried to reproduce the merge:\n>\n> ----\n> jw@logopolis:/projects/unity/atlantis$ git checkout working\n>\n> HEAD is now at aa29d0f4 Track instances of price changing\n>\n> jw@logopolis:/projects/unity/atlantis$ git merge other_parent\n...\n> CONFLICT (rename/delete): app/events/single_payment_event.rb deleted\n> in HEAD and renamed to app/events/checkout_event.rb in other_parent.\n> Version other_parent of app/events/checkout_event.rb left in tree.\n...\n\n\n> jw@logopolis:/projects/unity/atlantis$ git status\n...\n>         added by them:   app/events/checkout_event.rb\n...\n>\n> note that even though app/events/checkout_event.rb exists in both the\n> \"working\" and \"other_parent\" branches, git here says \"added by them\".\n>\n> Note that \"working\" and \"other_parent\" both have parent nodes which\n> rename the file in question from something else TO\n> app/events/checkout_event.rb (same for the spec.) The two ancestors\n> have different refs but the same datetime and commit message.\n\nYes, but in particular I think that git won't detect the rename on one\nof the two sides of history because the file has been modified too\nmuch.\n\n> This might also be of interest, or perhaps not.\n>\n> ----\n> jw@logopolis:/projects/unity/atlantis$ git diff working..broken --\n> app/events/checkout_event.rb | grep \"^-\" | wc -l\n>\n> 158\n>\n> jw@logopolis:/projects/unity/atlantis$ git diff working..broken --\n> app/events/checkout_event.rb | grep \"^+\" | wc -l\n>\n> 51\n\nIt'd be more helpful to see the output of both (note: three dots, not two)\n   git diff --name-status working...broken | grep -e checkout_event.rb\n-e single_payment_event.rb\n   git diff --name-status broken...working | grep -e checkout_event.rb\n-e single_payment_event.rb\n\nI suspect the output looks like\n    R071    single_payment_event.rb   checkout_event.rb\nfor one of those two commands (with the number 071 likely changing),\nand for the other it looks like\n    A       checkout_event.rb\n    D       single_payment_event.rb\n\nIn other words, the files were similar enough on one branch to detect\nthe rename, but not on the other.  You could tweak this with git diff\n--find-renames=<percentage> ..., and tweak it for the merge with git\nmerge -Xfind-renames=<percentage>, but that's just attempting to work\naround the problem.\n\n> ----\n>\n> Final bit of information:\n>\n> ----\n> jw@logopolis:/projects/unity/atlantis$ git --version\n>\n> git version 2.17.1\n> ----\n\nThis is pretty old version of git so I'd like to encourage you to\nupgrade...but it won't help with this bug.  This bug is present in all\nreleased git versions; it's test t6422.17 in our testsuite, marked as\ntest_expect_failure.\n\n> So, why does git say that this file was \"added by them\" instead of\n> saying it had a conflict that needed merging?\n\nBug.\n\n> It has been suggested that this might have something to do with some\n> team members using merge and others using rebase. Or possibly rebasing\n> shared history. I don't know how to determine whether that's the case\n> or whether that might have caused this.\n>\n> If so, I would appreciate seeing a set of instructions from a blank\n> repo that cause this same thing to happen, so that I can help my team\n> to understand the situation and how it can be avoided.\n\nNope, git bug.  You can create a simple repository and trigger it\npretty easily, as follows:\n\n# Create testcase in new temporary repo\ngit init stupid\ncd stupid/\nseq 10 20 >original.file\ngit add original.file\ngit commit -m Initial\ngit branch A\ngit branch B\ngit checkout A\ngit mv original.file new.file\nseq 5 21 >new.file\ngit add new.file\ngit commit -m \"Change on A\"\ngit checkout B\ngit mv original.file new.file\nseq 10 50 >new.file\ngit add new.file\ngit commit -m \"Change on B\"\n\n# Switch to branch A and merge B in\ngit checkout A\ngit merge B\n\n> If it is not related to rebasing and then merging, any help would be\n> appreciated in diagnosing this issue.\n\nOkay, so I said I had bad news and good news.  The good news: this\ntestcase passes if you use a special version of git -- the 'ort'\nbranch of my fork of git at https://github.com/newren/git.  I am\nworking on pushing this upstream; see e.g.\nhttps://lore.kernel.org/git/a8d4825a323d5c1e7b2dc1edc8621c51c030ae1e.1603468885.git.gitgitgadget@gmail.com/\nwhich marks t6422.17 as passing under the new ort merge backend.  But\nunless you want to download someone's randomly tweaked git version\nwith hundreds of patches, and build and use it, you might want to wait\nfor it to go through the review process first.  But, at least a\nsolution is on it's way...\n\nHope that helps,\nElijah\n"}]}