how do you review auto-resolved files

how do you review auto-resolved files

[Neal Kreitzinger]

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 

Re: how do you review auto-resolved files

[Junio C Hamano]

"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.

Re: how do you review auto-resolved files

[Neal Kreitzinger]

On 2/21/2012 3:19 PM, Junio C Hamano wrote:
> "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

Re: how do you review auto-resolved files

[Jeff King]

On Tue, Feb 21, 2012 at 05:22:09PM -0600, Neal Kreitzinger wrote:

> 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

Re: how do you review auto-resolved files

[Zbigniew Jędrzejewski-Szmek]

On 02/21/2012 10:19 PM, Junio C Hamano wrote:
> 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

Re: how do you review auto-resolved files

[Junio C Hamano]

"Neal Kreitzinger" <neal@rsss.com> writes:

> 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.