{"thread":{"id":"29684","subject":"how do you review auto-resolved files","startedAt":"2012-02-21T20:41:57Z","lastAt":"2012-02-22T15:24:58Z","messageCount":6,"participants":["Neal Kreitzinger","Junio C Hamano","Jeff King","Zbigniew Jędrzejewski-Szmek"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"185099","messageId":"ji0vik$e48$1@dough.gmane.org","threadId":"29684","inReplyTo":null,"subject":"how do you review auto-resolved files","fromName":"Neal Kreitzinger","fromEmail":"neal@rsss.com","sentAt":"2012-02-21T20:41:57Z","receivedAt":"2012-02-21T20:41:57Z","isPatch":false,"sender":{"key":"neal@rsss.com","avatar":null},"body":"When git does a merges (merge/rebase/cherry-pick) it auto-resolves same-file \nchanges that do not conflict on the same line(s).\n\nTechnical Question:  What are the recommended commands for reviewing the \nfiles that auto-resolved after a \"merge\"?\n\nIt seems like the commands might be different depending on the type of \nmerge: git-merge, git-rebase, git-cherry-pick.  I imagine there are three \nsteps to the \"review auto-resolutions\" procedure:\n\n(1)  Determine the list of files that were changed on both sides (same-file \nedits) and which of those were auto-resolved during the merge.  (Preferably \nexcluding those files that merge-conflicted since you already know how you \nmanually resolved those.)\n\n(2)  Review the auto-resolved files in full context to verify whether the \nauto-resolutions are desirable.\n\n(3)  Manually remediate the merge-result (auto-resolution) or redo the \nmerge-of-that-file for any files with undesirable auto-resolutions.  Perhaps \nan edit of the auto-resolved file is sufficient for simple remediations, but \nfor more challenging remediations a manual redo of the merge-of-that-file \nwould be desired.\n\nPlease advise on the proven (tried and tested) ways that others are using to \nverify/ensure that their auto-resolve results are correct.\n\n\nProcedural/Philosophical Question:  What are the pros and cons of \nauto-resolved files?\n\nCurrently, we address the problem up-front instead of after-the-fact by \nenforcing merge-conflicts on every same-file edit by means of a \n\"user-date-stamp\" on \"line 1\" of every source file changed by performing \nkeyword expansion (# $User$ $Date$) in our pre-commit hook.  I don't think \nkeyword expansion or forcing merge-conflicts for every same-file edit is a \ncommon practice among git users.  Therefore, this seems like somewhat of a \nkludgey hack.  Furthermore, I assume that all git users are somehow \nreviewing their auto-resolutions.  (There is no way I would assume that git \nmerged my same-file edits correctly.  It's great that git \ndoes-the-right-thing most-of-the-time, but that doesn't change the fact that \nI still have to review everything for undesirable resolutions.)\n\nIn light of this, it seems that there is no advantage to letting git \nauto-resolve same-file changes because the review process after-the-fact \nwould actually be more error-prone and tedious than just manually-merging \nsame-file edits up-front.  If I force you to resolve merge-conflicts \nup-front then I'm ensuring the merge-resolution is deliberate (and hopefully \nintelligent).  If I expect/assume you are going to review the \nauto-resolutions after-the-fact then you can neglect this because you:\n\n  - have become complacent that git usually does-what-you-want so \"you don't \nreally need to do it\",\n  - are lazy and do it half-way,\n  - forget to do it,\n  - think \"git magically does your work for you\",\n  - don't know how to do it,\n  - don't even realize that anything auto-resolved or what auto-resolved,\n  - decide you don't have to do it because that is what testing if for,\n  - you think that your time is so valuable that an ounce-of-prevention on \nyour part is not worth a pound-of-cure on the part of others.\n\nPlease comment on the pros and cons of \"manual-merge up-front for same-file \nedits\" vs. \"review-and-remediate after-the-fact for auto-resolutions of \nsame-file edits\".\n\nThanks in advance for your replies!\n\nv/r,\nneal \n"},{"id":"185104","messageId":"7vhayjga0a.fsf@alter.siamese.dyndns.org","threadId":"29684","inReplyTo":"ji0vik$e48$1@dough.gmane.org","subject":"Re: how do you review auto-resolved files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-21T21:19:17Z","receivedAt":"2012-02-21T21:19:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Neal Kreitzinger\" <neal@rsss.com> writes:\n\n> When git does a merges (merge/rebase/cherry-pick) it auto-resolves same-file \n> changes that do not conflict on the same line(s).\n>\n> Technical Question:  What are the recommended commands for reviewing the \n> files that auto-resolved after a \"merge\"?\n\nImagine that you are the maintainer of the mainline and are reviewing the\nwork made on a side branch that you just merged, but pretend that the\ncontribution came as a patch instead.  How would you assess the damage to\nyour mainline?\n\nYou would use \"git show --first-parent $commit\" for that.\n\nAnd then look at what the sideline wanted to do to the old baseline:\n\n\tgit log -p $commit^..$commit\n\nwhich would, unless the person who worked on the side branch did a shoddy\njob describing his work, explain what the side branch wanted to achieve\nand also _how_ it wanted to achieve it.\n\nAnd then re-read the first \"git show\" output with that knowledge, together\nwith the knowledge you have on your mainline codebase, and decide if the\nsolution used by the side branch is still valid.  If it makes sense, you\nare done.  If the advance in your mainline since the side branch forked\ninvalidated some assumption the side branch made (e.g. a helper function\nthe side branch used has changed its meaning, a helper function the side\nbranch changed its meaning gained more callsite on the mainline, etc.),\nyou have a semantic conflict that you would need to address.\n\nIt is unclear what exactly you consider \"auto-resolve\" in your message, so\nI'd refrain from commenting on the \"Philosophical\" part, at least for now.\n"},{"id":"185114","messageId":"4F442721.4080107@gmail.com","threadId":"29684","inReplyTo":"7vhayjga0a.fsf@alter.siamese.dyndns.org","subject":"Re: how do you review auto-resolved files","fromName":"Neal Kreitzinger","fromEmail":"nkreitzinger@gmail.com","sentAt":"2012-02-21T23:22:09Z","receivedAt":"2012-02-21T23:22:09Z","isPatch":false,"sender":{"key":"nkreitzinger@gmail.com","avatar":null},"body":"On 2/21/2012 3:19 PM, Junio C Hamano wrote:\n> \"Neal Kreitzinger\"<neal@rsss.com>  writes:\n>\n>> When git does a merges (merge/rebase/cherry-pick) it auto-resolves same-file\n>> changes that do not conflict on the same line(s).\n>>\n>> Technical Question:  What are the recommended commands for reviewing the\n>> files that auto-resolved after a \"merge\"?\n>\n> Imagine that you are the maintainer of the mainline and are reviewing the\n> work made on a side branch that you just merged, but pretend that the\n> contribution came as a patch instead.  How would you assess the damage to\n> your mainline?\n>\n> You would use \"git show --first-parent $commit\" for that.\n>\n> And then look at what the sideline wanted to do to the old baseline:\n>\n> \tgit log -p $commit^..$commit\n>\n> which would, unless the person who worked on the side branch did a shoddy\n> job describing his work, explain what the side branch wanted to achieve\n> and also _how_ it wanted to achieve it.\n>\n> And then re-read the first \"git show\" output with that knowledge, together\n> with the knowledge you have on your mainline codebase, and decide if the\n> solution used by the side branch is still valid.  If it makes sense, you\n> are done.  If the advance in your mainline since the side branch forked\n> invalidated some assumption the side branch made (e.g. a helper function\n> the side branch used has changed its meaning, a helper function the side\n> branch changed its meaning gained more callsite on the mainline, etc.),\n> you have a semantic conflict that you would need to address.\n>\n> It is unclear what exactly you consider \"auto-resolve\" in your message, so\n> I'd refrain from commenting on the \"Philosophical\" part, at least for now.\n\nContext: (git-merge manpage definition of merge-conflict) \"During a \nmerge, the working tree files are updated to reflect the result of the \nmerge... When both sides made changes to the same area, however, git \ncannot randomly pick one side over the other, and asks you to resolve it \nby leaving what both sides did to that area.\"\n\nMy definition for \"auto-resolve\": \"During a merge, the working tree \nfiles are updated to reflect the result of the merge... When both sides \nmade changes to different areas of the same file, git picks both sides \nautomatically, and leaves its up to you to make sure you review those \nmerge results for correctness after git has made the merge commit.\"\n\nIOW, an \"auto-resolve\" specifically means that both sides (ours and \ntheirs) made changes to file(a) since the common-ancestor version of \nfila(a), and git picked both sides without raising a merge-conflict. \n(The reason I came up with the term \"auto-resolve\" is because in the \ngit-merge output the term \"Auto-merging\" can also indicate that only one \nside (theirs) changed file(a) since the common-ancestor and that git is \njust \"fast-forwarding\" theirs file(a) on top of common-ancestor file(a).)\n\nv/r,\nneal\n"},{"id":"185117","messageId":"7vobsreocm.fsf@alter.siamese.dyndns.org","threadId":"29684","inReplyTo":"ji0vik$e48$1@dough.gmane.org","subject":"Re: how do you review auto-resolved files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-21T23:52:25Z","receivedAt":"2012-02-21T23:52:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Neal Kreitzinger\" <neal@rsss.com> writes:\n\n> If I expect/assume you are going to review the \n> auto-resolutions after-the-fact then you can neglect this because you:\n>\n>   - have become complacent that git usually does-what-you-want so \"you don't \n>     really need to do it\",\n>   - are lazy and do it half-way,\n>   - forget to do it,\n>   - think \"git magically does your work for you\",\n>   - don't know how to do it,\n>   - don't even realize that anything auto-resolved or what auto-resolved,\n>   - decide you don't have to do it because that is what testing if for,\n>   - you think that your time is so valuable that an ounce-of-prevention on \n>     your part is not worth a pound-of-cure on the part of others.\n\nA couple more bullet points I can think of off the top of my head, after\nmaking sure that you do not count what \"rerere\" does as part of the\n\"auto-resolution\", to add to the above list are:\n\n - know git is stupid and errs on the safe side, punting anything remotely\n   complex;\n\n - know that textual non-conflicts that occur in the same file have the\n   same risk of having semantic conflict across different files, so\n   singling out \"touched the same file but did not conflict\" any special\n   is pointless, but in either case, the chance of having such a conflict\n   is small enough that completing the merge (and other merges) first and\n   then checking the overall result is more efficient use of your time,\n   because you have to eyeball the result at least once anyway before\n   pushing it out.\n"},{"id":"185152","messageId":"20120222072848.GC17015@sigill.intra.peff.net","threadId":"29684","inReplyTo":"4F442721.4080107@gmail.com","subject":"Re: how do you review auto-resolved files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-22T07:28:48Z","receivedAt":"2012-02-22T07:28:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 21, 2012 at 05:22:09PM -0600, Neal Kreitzinger wrote:\n\n> My definition for \"auto-resolve\": \"During a merge, the working tree\n> files are updated to reflect the result of the merge... When both\n> sides made changes to different areas of the same file, git picks\n> both sides automatically, and leaves its up to you to make sure you\n> review those merge results for correctness after git has made the\n> merge commit.\"\n\nOnce the merge commit is made, you can review these with:\n\n  $ git show --raw\n\nwhich will give you the list of paths that were touched on both sides,\nand then you can examine them manually.\n\nYou can also use:\n\n  $ git show -c\n\nto get the combined diff, showing hunks that were changed on both sides\n(but only in files that would have been listed above). Annoyingly, I\ndon't think there is a way to get the same multi-way diff information\nbefore the commit is created (i.e., when you still have some conflicts\nin the index and working tree left to resolve).\n\nBut even both of those are not sufficient to find merge errors. Even\nthough there is no textual conflict, there may be semantic conflicts\nthat cross file boundaries (e.g., function foo() changes in foo.c, but a\ncaller in bar.c is introduced on a side branch). There is no replacement\nfor actually looking at the full result (though for the lazy, compiling\nand running the test suite can often catch the low-hanging fruit).\n\n-Peff\n"},{"id":"185178","messageId":"4F4508CA.9050707@in.waw.pl","threadId":"29684","inReplyTo":"7vhayjga0a.fsf@alter.siamese.dyndns.org","subject":"Re: how do you review auto-resolved files","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-02-22T15:24:58Z","receivedAt":"2012-02-22T15:24:58Z","isPatch":false,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"On 02/21/2012 10:19 PM, Junio C Hamano wrote:\n> Imagine that you are the maintainer of the mainline and are reviewing the\n> work made on a side branch that you just merged, but pretend that the\n> contribution came as a patch instead.  How would you assess the damage to\n> your mainline?\n>\n> You would use \"git show --first-parent $commit\" for that.\nHi,\nit seems that git show --first-parent is not documented in the man page.\nThis option is only documented for rev-list and log. I think that\n- this example should land in Examples in git-show.txt\n- --first-parent should be documented in git-show.txt because it (at \nleast to me, but I guess that for other people also) it isn't \nimmediately obvious that it means to _diff_ with the first parent.\n\n--\nZbyszek\n"}]}