[teampractices] Code review social norms

[Antoine Musso]

Le 18/03/2016 01:47, Gergo Tisza a écrit :
> 
> Nontrivial changes in a merge commit is a bad practice, as the changes
> are invisible to git diff / git log -p unless you explicitly tell git to
> diff to the non-default parent, so they inevitably cause confusion.

Hello,

It is slightly more complicated.  Different git commands have different
behaviour when it comes to display the diff of a merge commit. See man
git-diff section "COMBINED DIFF FORMAT". In short when acting on a merge
commit:


git show : yields a combined diff
git log -p : skip the merge diff and output the commits diff.
git diff : diff against the first parent (ie include the conflict fix)

So yeah I agree it can cause confusion. In Gerrit, if a merge is needed
it has to be trivial effectively forcing devs to amend a patch. That is
less confusing indeed.



For the record, history, training purpose:

Combined diff can be forced via '--combined', '-c' or '--cc'.

Lets journey into git. Given an empty README.txt where developers fill
in a note about the feature they are adding.  Each comes with respectivly:

* feature1: compile!   commit

And

* feature1: send mails!


The 'compile' one is merged first, then one goes to merge 'send mails!'
feature. The conflict is fixed in the merge commit by renumbering the
feature #2 and the end result is:

 * feature1: compile!
 * feature2: send mails!


`git show` yields the combined diff:

  Acme Software

 -* feature1: send mails!
 +* feature1: compile!
++* feature2: send mails!


`git log -p` gives the original patches:

Merge branch 'feature1: send mails' as feature2

feature1: compile!
+* feature1: compile!

feature1: send mails
+* feature1: mails


`git diff HEAD^` the actual change:

* feature1: compile!
+* feature2: send mails!



Instructing git log -p to show the combined diff can be done with '-c':

`git log --oneline -p -c -n1`

Merge branch 'feature1: send mails' as feature2
 -* feature1: send mails!
 +* feature1: compile!
++* feature2: send mails!



-- 
Antoine "hashar" Musso