Issue #1650: Misleading text on merge commit display - pagure

pagure

#1650 Misleading text on merge commit display

Closed: Fixed Dec 13, 2016 Opened Dec 8, 2016 by mikem.

Merge commits display the overall diff, which is useful. However, the display suggests that the author of the merge commit is the author of all the commits, which is often incorrect. I had a contributor get a little irritated over this.

E.g. on https://pagure.io/koji/c/c4d027cf, the text says

2 files Authored and Committed by mikem 7 minutes ago

I guess it would be ideal to more clearly indicate that changes shown come from the commits that have been merged. Perhaps the page could show a list of authors for the merged commits.

mikem commented Dec 8, 2016

pingou commented Dec 8, 2016

We are being consistent with git itself there:

$ git show c4d027cf     
commit c4d027cfab399c0cca44824b2a1901ce2f4cc338
Merge: d7e8570 57d8729
Author: Mike McLean <mikem@redhat.com>
Date:   Thu Dec 8 09:40:13 2016 -0500
    Merge PR#208 kojiweb/cli: [rpminfo]rename "Payload Hash" to "SIGMD5"
    Merges #208
    https://pagure.io/koji/pull-request/208

From a git point of view, the only thing that distinguish the merge commit is that it has more than one parent, but then we would need a way to figure out which of the branches to consider.
I don't know if the pain is worth the gain :s

mikem commented Dec 8, 2016

Well, yes, I am certainly the author of the merge commit. The difference is that the git display does not imply that I am also the author of the changes in the merge. Pagure says "2 files Authored and Committed by mikem."

Of course this is a little nit-picky, low priority, and open to discussion.

With git itself, you have to go to a little trouble to see the diff from a merge commit. That pagure does this work for you is nice, but perhaps a little confusing to some users.

Github does similar, but the phrasing is a little different.
"mikem committed on Oct 26 ... 2 parents ...."
"Showing 2 changed files ...."

Again, not a big issue, but just filed because the situation came up and I wondered if a slight rewording might have avoided confusion.

mikem commented Dec 8, 2016

but then we would need a way to figure out which of the branches to consider.

You must already have done this to show the diff

pingou commented Dec 12, 2016

I wondered if a slight rewording might have avoided confusion.

I thought you had something different in mind, a rewording is definitely possible.

What do you think of this?

pingou commented Dec 12, 2016

Another alternative would be to not show the diff on merge commits. I am not entirely fan of the idea but if people think it's a good one, then it is doable (and is similar to what git does in git show).

mikem commented Dec 12, 2016

I like the text change.

I think showing the diff is useful. I wouldn't remove it, I just want to avoid folks getting confused by it.

pingou commented Dec 13, 2016

Proposed PR: https://pagure.io/pagure/pull-request/1660

pingou commented Dec 13, 2016

Commit 4d7c9617 fixes this issue

@pingou changed the status to Closed

Dec 13, 2016