threads / discuss / 29684

how do you review auto-resolved files

Subject: how do you review auto-resolved files

## tl;dr

6 messages between Feb 21, 2012 and Feb 22, 2012.

replies: 5people: 5as markdown or json

Neal Kreitzinger· Feb 21, 2012, 20:41 UTC · lore

When git does a merges (merge/rebase/cherry-pick) it auto-resolves same-file changes that do not conflict on the same line(s).

Technical Question: What are the recommended commands for reviewing the files that auto-resolved after a "merge"?

It seems like the commands might be different depending on the type of merge: git-merge, git-rebase, git-cherry-pick. I imagine there are three steps to the "review auto-resolutions" procedure:

(1) Determine the list of files that were changed on both sides (same-file edits) and which of those were auto-resolved during the merge. (Preferably excluding those files that merge-conflicted since you already know how you manually resolved those.)

(2) Review the auto-resolved files in full context to verify whether the auto-resolutions are desirable.

(3) Manually remediate the merge-result (auto-resolution) or redo the merge-of-that-file for any files with undesirable auto-resolutions. Perhaps an edit of the auto-resolved file is sufficient for simple remediations, but for more challenging remediations a manual redo of the merge-of-that-file would be desired.

Please advise on the proven (tried and tested) ways that others are using to verify/ensure that their auto-resolve results are correct.

Procedural/Philosophical Question: What are the pros and cons of auto-resolved files?

Currently, we address the problem up-front instead of after-the-fact by enforcing merge-conflicts on every same-file edit by means of a "user-date-stamp" on "line 1" of every source file changed by performing keyword expansion (# $User$ $Date$) in our pre-commit hook. I don't think keyword expansion or forcing merge-conflicts for every same-file edit is a common practice among git users. Therefore, this seems like somewhat of a kludgey hack. Furthermore, I assume that all git users are somehow reviewing their auto-resolutions. (There is no way I would assume that git merged my same-file edits correctly. It's great that git does-the-right-thing most-of-the-time, but that doesn't change the fact that I still have to review everything for undesirable resolutions.)

In light of this, it seems that there is no advantage to letting git auto-resolve same-file changes because the review process after-the-fact would actually be more error-prone and tedious than just manually-merging same-file edits up-front. If I force you to resolve merge-conflicts up-front then I'm ensuring the merge-resolution is deliberate (and hopefully intelligent). If I expect/assume you are going to review the auto-resolutions after-the-fact then you can neglect this because you:

  - have become complacent that git usually does-what-you-want so "you don't 
really need to do it",
  - are lazy and do it half-way,
  - forget to do it,
  - think "git magically does your work for you",
  - don't know how to do it,
  - don't even realize that anything auto-resolved or what auto-resolved,
  - decide you don't have to do it because that is what testing if for,
  - you think that your time is so valuable that an ounce-of-prevention on 
your part is not worth a pound-of-cure on the part of others.

Please comment on the pros and cons of "manual-merge up-front for same-file edits" vs. "review-and-remediate after-the-fact for auto-resolutions of same-file edits".

Thanks in advance for your replies!

v/r, neal

Junio C Hamano· Feb 21, 2012, 21:19 UTC · re: Neal Kreitzinger · lore

Re: how do you review auto-resolved files

"Neal Kreitzinger" <neal@rsss.com> writes:
Show 5 quoted lines
> When git does a merges (merge/rebase/cherry-pick) it auto-resolves same-file 
> changes that do not conflict on the same line(s).
>
> Technical Question:  What are the recommended commands for reviewing the 
> files that auto-resolved after a "merge"?

Imagine that you are the maintainer of the mainline and are reviewing the work made on a side branch that you just merged, but pretend that the contribution came as a patch instead. How would you assess the damage to your mainline?

You would use "git show --first-parent $commit" for that.
And then look at what the sideline wanted to do to the old baseline:
	git log -p $commit^..$commit

which would, unless the person who worked on the side branch did a shoddy job describing his work, explain what the side branch wanted to achieve and also _how_ it wanted to achieve it.

And then re-read the first "git show" output with that knowledge, together with the knowledge you have on your mainline codebase, and decide if the solution used by the side branch is still valid. If it makes sense, you are done. If the advance in your mainline since the side branch forked invalidated some assumption the side branch made (e.g. a helper function the side branch used has changed its meaning, a helper function the side branch changed its meaning gained more callsite on the mainline, etc.), you have a semantic conflict that you would need to address.

It is unclear what exactly you consider "auto-resolve" in your message, so I'd refrain from commenting on the "Philosophical" part, at least for now.

Neal Kreitzinger· Feb 21, 2012, 23:22 UTC · re: Junio C Hamano · lore

Re: how do you review auto-resolved files

On 2/21/2012 3:19 PM, Junio C Hamano wrote:
Show 34 quoted lines
> "Neal Kreitzinger"<neal@rsss.com>  writes:
>
>> When git does a merges (merge/rebase/cherry-pick) it auto-resolves same-file
>> changes that do not conflict on the same line(s).
>>
>> Technical Question:  What are the recommended commands for reviewing the
>> files that auto-resolved after a "merge"?
>
> Imagine that you are the maintainer of the mainline and are reviewing the
> work made on a side branch that you just merged, but pretend that the
> contribution came as a patch instead.  How would you assess the damage to
> your mainline?
>
> You would use "git show --first-parent $commit" for that.
>
> And then look at what the sideline wanted to do to the old baseline:
>
> 	git log -p $commit^..$commit
>
> which would, unless the person who worked on the side branch did a shoddy
> job describing his work, explain what the side branch wanted to achieve
> and also _how_ it wanted to achieve it.
>
> And then re-read the first "git show" output with that knowledge, together
> with the knowledge you have on your mainline codebase, and decide if the
> solution used by the side branch is still valid.  If it makes sense, you
> are done.  If the advance in your mainline since the side branch forked
> invalidated some assumption the side branch made (e.g. a helper function
> the side branch used has changed its meaning, a helper function the side
> branch changed its meaning gained more callsite on the mainline, etc.),
> you have a semantic conflict that you would need to address.
>
> It is unclear what exactly you consider "auto-resolve" in your message, so
> I'd refrain from commenting on the "Philosophical" part, at least for now.
Context: (git-merge manpage definition of merge-conflict) "During a 
merge, the working tree files are updated to reflect the result of the 
merge... When both sides made changes to the same area, however, git 
cannot randomly pick one side over the other, and asks you to resolve it 
by leaving what both sides did to that area."

My definition for "auto-resolve": "During a merge, the working tree files are updated to reflect the result of the merge... When both sides made changes to different areas of the same file, git picks both sides automatically, and leaves its up to you to make sure you review those merge results for correctness after git has made the merge commit."

IOW, an "auto-resolve" specifically means that both sides (ours and theirs) made changes to file(a) since the common-ancestor version of fila(a), and git picked both sides without raising a merge-conflict. (The reason I came up with the term "auto-resolve" is because in the git-merge output the term "Auto-merging" can also indicate that only one side (theirs) changed file(a) since the common-ancestor and that git is just "fast-forwarding" theirs file(a) on top of common-ancestor file(a).)

v/r, neal

Jeff King· Feb 22, 2012, 07:28 UTC · re: Neal Kreitzinger · lore

Re: how do you review auto-resolved files

On Tue, Feb 21, 2012 at 05:22:09PM -0600, Neal Kreitzinger wrote:
Show 6 quoted lines
> My definition for "auto-resolve": "During a merge, the working tree
> files are updated to reflect the result of the merge... When both
> sides made changes to different areas of the same file, git picks
> both sides automatically, and leaves its up to you to make sure you
> review those merge results for correctness after git has made the
> merge commit."
Once the merge commit is made, you can review these with:
  $ git show --raw

which will give you the list of paths that were touched on both sides, and then you can examine them manually.

You can also use:
  $ git show -c

to get the combined diff, showing hunks that were changed on both sides (but only in files that would have been listed above). Annoyingly, I don't think there is a way to get the same multi-way diff information before the commit is created (i.e., when you still have some conflicts in the index and working tree left to resolve).

But even both of those are not sufficient to find merge errors. Even though there is no textual conflict, there may be semantic conflicts that cross file boundaries (e.g., function foo() changes in foo.c, but a caller in bar.c is introduced on a side branch). There is no replacement for actually looking at the full result (though for the lazy, compiling and running the test suite can often catch the low-hanging fruit).

-Peff
Zbigniew Jędrzejewski-Szmek· Feb 22, 2012, 15:24 UTC · re: Junio C Hamano · lore

Re: how do you review auto-resolved files

On 02/21/2012 10:19 PM, Junio C Hamano wrote:
Show 6 quoted lines
> Imagine that you are the maintainer of the mainline and are reviewing the
> work made on a side branch that you just merged, but pretend that the
> contribution came as a patch instead.  How would you assess the damage to
> your mainline?
>
> You would use "git show --first-parent $commit" for that.
Hi,
it seems that git show --first-parent is not documented in the man page.
This option is only documented for rev-list and log. I think that
- this example should land in Examples in git-show.txt
- --first-parent should be documented in git-show.txt because it (at 
least to me, but I guess that for other people also) it isn't 
immediately obvious that it means to _diff_ with the first parent.

-- Zbyszek

Junio C Hamano· Feb 21, 2012, 23:52 UTC · re: Neal Kreitzinger · lore

Re: how do you review auto-resolved files

"Neal Kreitzinger" <neal@rsss.com> writes:
Show 13 quoted lines
> If I expect/assume you are going to review the 
> auto-resolutions after-the-fact then you can neglect this because you:
>
>   - have become complacent that git usually does-what-you-want so "you don't 
>     really need to do it",
>   - are lazy and do it half-way,
>   - forget to do it,
>   - think "git magically does your work for you",
>   - don't know how to do it,
>   - don't even realize that anything auto-resolved or what auto-resolved,
>   - decide you don't have to do it because that is what testing if for,
>   - you think that your time is so valuable that an ounce-of-prevention on 
>     your part is not worth a pound-of-cure on the part of others.

A couple more bullet points I can think of off the top of my head, after making sure that you do not count what "rerere" does as part of the "auto-resolution", to add to the above list are:

 - know git is stupid and errs on the safe side, punting anything remotely
   complex;
 - know that textual non-conflicts that occur in the same file have the
   same risk of having semantic conflict across different files, so
   singling out "touched the same file but did not conflict" any special
   is pointless, but in either case, the chance of having such a conflict
   is small enough that completing the merge (and other merges) first and
   then checking the overall result is more efficient use of your time,
   because you have to eyeball the result at least once anyway before
   pushing it out.

← back to recent threads