Comparing rebased patchsets

[Gustavo Leite de Mendonça Chaves]

I was comparing two patchsets unaware of the fact that the newer one had been rebased. Hence, it took me a while to understand why I was seeing some changes that could not possibly have been introduced in the newer patchset.

BTW, this is the problem already reported in https://code.google.com/p/gerrit/issues/detail?id=551 and which got some more attention in https://code.google.com/p/gerrit/issues/detail?id=217. However, it seems that it hasn't been solved yet.

To me, the root of the problem is that Gerrit's interface doesn't make it clear when rebases have occurred during the patchset history. The "Base" link, preceeding the numeric links to each patchset, is different from them because it isn't a fixed link as I thought it was. It really points to the parent of the currently selected patchset commit, right? I mean, for instance, if there is a rebase between patchsets 1 and 2, when you select patchset 1 the base link points to #1's parent and when you select patchset 2 the base link changes to point to #2's parent.

I agree that we must have a link pointing to the currently selected patchset's parent commit. However, it's confusing to have a single link that changes where it points to depending on the context. It's very subtle and can lead to misunderstandings.

I think that perhaps we need more "base" links: one for the first patchset and an extra one for each rebase. I'm thinking of something like this. Suppose we have a series of 6 patchsets and that we had a rebase before #3 and another one before #5. In this case the patchset links in the diff interface could be something like this:

    Base 1 2 Base 3 4 Base 5 6

Or, perhaps, using some vertical spacing to enhance the visualization, something like this:

         1 2      3 4      5 6

    Base     Base     Base

The first Base link points the the parent commit of patchsets 1 and 2. The second Base points to the parent commit of patchsets 3 and 4. The third Base points to the parent commit of patchsets 5 and 6. This makes it clear where rebases occurred and avoids a source of misunderstandings. It also allows one to compare different bases, something I don't think is possible in the current interface, right?

The default diff endpoints should remain the same, i.e., the last patchset against it's own Base (the last one).

What do you think?

--

Gustavo

[Saša Živkov]

Yes this is a well known problem.

I agree that the UI should provide more details wrt different bases and I like your proposal :-)

 

--

Gustavo

--
--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en
 
---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[apeksha...@gmail.com]

 Hi Gustavo,

 I am currently working on issue 217 and I have read your suggestion of introducing multiple "Base" links.
 My understanding is :
 If there is a rebase between PatchSet1 and PatchSet2, then dropdown options would be
 'Base'   1     'Base'     2
 and selection of the second 'Base' link would show only rebase differences, and selection of '1' would show only patchset differences.

 Can you please confirm if my understanding is correct.

 Thanks,
 Apeksha

[Gustavo Leite de Mendonça Chaves]

Replying to the list this time...

2014-06-17 7:37 GMT-03:00 <apeksha...@gmail.com>:

 Hi Gustavo,

 I am currently working on issue 217 and I have read your suggestion of introducing multiple "Base" links.
 My understanding is :
 If there is a rebase between PatchSet1 and PatchSet2, then dropdown options would be
 'Base'   1     'Base'     2
 and selection of the second 'Base' link would show only rebase differences, and selection of '1' would show only patchset differences.

 Can you please confirm if my understanding is correct.

Dropdown? You should be refering to Gerrit's New Screen but I'm still using the Old Screen of Gerrit 2.8.4 and I'm not used to the new interface.

I've just configured it to try to understand what you mean. I see two dropdown menus there: the "Revisions" menu at the top-right corner and the "Diff against" menu at the center of the screen. I suppose you refer to the second one, right?

In this case, I think the options should be presented like this:

2: SHA1

Base: SHA1 (default)

1: SHA1

Base: SHA1

The default selection should be the second Base (couting from bottom to top), because it is the current patchset's base.

The effect of selecting any option should be to show exactly what the command "git diff <selected SHA1> <2: SHA1>" would.

I don't think in terms of "only rebase" or "only patchset" differences, though. Gerrit shouldn't try to innovate here but offer to show exactly what "git diff" would show when specifying two SHA1s.

Does this make sense to you?

--

Gustavo Leite de Mendonça Chaves

CPqD - Gerência de Tecnologia da Informação e Comunicação

Tel.: +55 19 3705-7003

gus...@cpqd.com.br

www.cpqd.com.br

[apeksha...@gmail.com]

 Hi Gustavo,

 Thanks for your response. It definitely makes sense to me.
 However, I found a change [1] which has been merged in gerrit recently.
 Could you please have a look at it and let us know if it is in line with your suggestion ?

 [1] https://gerrit-review.googlesource.com/#/c/57086/

 I dont know if this would be useful to you, but just mentioning here what I tested in my local environment (which is a copy of the latest master, so it has the above change)
 1. Upload a change with 8-10 files, say changeA
 2. Upload another change, say changeB with same files as in changeA, but just modified first 3 files.
 3. Merge changeA (which is successful)
 4. While trying to rebase , got merge conflicts for first 3 files. Resolved the conflicts.
 5. Pushed another patchSet for changeB.
 6. Compare patchset2 with patchset1. Output shows only first 3 files which were modified.

 Thanks,
 Apeksha

[Gustavo Leite de Mendonça Chaves]

2014-06-19 10:22 GMT-03:00 <apeksha...@gmail.com>:

 Hi Gustavo,

 Thanks for your response. It definitely makes sense to me.
 However, I found a change [1] which has been merged in gerrit recently.
 Could you please have a look at it and let us know if it is in line with your suggestion ?

 [1] https://gerrit-review.googlesource.com/#/c/57086/

Yes, I've seen this change when it was mentioned in a comment on issue 217 a few days ago. I haven't fully grasped it, so I don't have a definite opinion on it. For what it's worth, I'll try to reason about it below.

As I understand it, since the Change Screen by default shows the diff between the current patchset and its own Base, this change doesn't change anything in the default case. The difference only affects diffs from the current patchset to other patchsets explicitly selected from the dropdown menu.

The second most common case is to select the previous patchset to diff against the current one. If there is no rebase in between them this change also doesn't change anything, since only files touched by the current patchset during the amendment of the previous one would show any difference, anyway.

If there was a rebase, though, between the current patchset and the previous one, then this change would omit from the diff file list any files not explicitly touched by the new patchset. This is the main purpose of the change, to help the user focus the review on the files that matter. This seems to be a good thing, but since it works only at the file level, there can be diff hunks in the touched files that were introduced by the rebase and not by the amendment, which can be confusing, as Bruce Zu pointed out in the change discussion. I can see the benefit of this change in a situation where the rebase introduces many other files, but since it's not a complete solution I think I'll have to try it out a bit to make my own opinion about the tradeoffs therein.

I'm not sure what would happen if we select an yet older patchset to diff against the current one. Imagine there are three patchsets: A touching 10 files, B touching 5 of those 10 files, and C touching just one of those 5 files. There are no rebases among them. What if we select to diff A against C? As I understand the change, I'm guessing it would show only the one file touched by C and I don't think this helps. In this case, I think it would be best to show exactly the changes between A and C as "git diff A C" would show, with all five files differences, or the user wouldn't have a complete picture of the difference that matters. (Can you confirm that this is what would happen?)

In the end, rebases are the real cuprits here, and I'm afraid that this change may be introducing a partial solution to them that can have confusing collateral effects... That's why I feel that Gerrit should not "innovate" here and use the original Git diff semantics by default, all over. Perhaps, instead of hiding the untouched files the touched ones could be marked differently (e.g., by a different font, or color) to aid the user but not to omit any information that can mislead her.

I still think that showing fixed links to all Bases in the dropdown menu (the original one and the new ones introduced by rebases) would give a better picture of the change history, from which users could have more control about what they are diffing the current patchset against.

The problem with rebases occur when the user rebase the patchset and amend it in a single step, producing a single new patchset. Then it's difficult to separate what changes (hunks) were due to the rebase and what changes were due to the amendment. If users adhere to the "best practice" of first rebase and then amend (or vice-versa) producing two patchsets, the problem vanishes, since the different changes happen in different steps, and we have more commits to diff against.

However, Gerrit cannot impose this "best practice", as it cannot know what the user did to come up with a new patchset. It just occurs to me that the only way to show only the amendment hunks would be for Gerrit to simulate the rebase of the previous patchset in a temporary commit and diff the resultant commit against the new patchset. This would in a slngle pass "hide" all files and hunks introduced by the rebase itself. But, of course, this could only work if the rebase didn't produce any conflicts, in which case Gerrit could simply give up and alert the user about its limitation.

Just my 2 cents, if it's worth that much... :-)