Hi,
There's something in Gerrit that i still don't get, even after trying to ask this several times on mediawiki.org [1] and on IRC. I sincerely thank everyone who took the time to reply, but either i still misunderstand the replies or i do understand them and I Just Don't Like It [2].
Let's consider a patch set i submitted: https://gerrit.wikimedia.org/r/#change,3361 .
The patch set as i submitted it was not perfect and Aaron marked it "-1". So far, so good. Then Hashar amended the patch set and Nikerabbit amended it some more. Now here's what i don't like: The diffs of their changes show all the changes instead of just showing Hashar's and Nikerabbit's changes. This is somewhat similar to how we review patches by new volunteer developers - if the patch attached to a Bugzilla is not perfect we ask to write a whole new one. That's the custom, and maybe it's considered educational, but actually its usefulness is doubtful. Despite this, it is now applied to all the commits.
Now honestly, the result in the story of this particular patch set is OK. The resulting patch is better than the first one and the collaboration worked well. But does this scale? This patch set changes only a few lines of code; what will happen with patch sets in which dozens of lines are changed? These happen very often. The reviewer will have to re-read all the changed lines for every amend and this looks like a huge waste of time.
It would make a lot more sense to me to see these three changes as a branch with three commits - the first one is mine, the second one is Hashar's fix and the third one is Nikerabbit's fix. When there's agreement that the tip of the branch is good, it's merged to the master branch. If a reviewer wants to see the whole end-to-end diff, it's not hard. Mind you, this has nothing to do with SVN - in SVN branches are used rarely. This, to the best of my understanding, is the Git way. Git makes this sensible scenario easy, and it is used successfully in Github - but in the Gerrit workflow this scenario is not used. I tried reading the Gerrit manual [3] and it didn't make it any clearer. Yet again, i may be misunderstanding something, so correct me if i'm wrong; It's really weird that a tool that makes code review so central makes it a lot harder, too. But that's the feeling that i get.
Am i just wrong in my understanding of Gerrit? Am i the only one who doesn't like it? Is it too late to complain?
[1] https://www.mediawiki.org/wiki/Talk:Git/Workflow [2] As in https://en.wikipedia.org/wiki/Wikipedia:IDONTLIKEIT . [3] http://gerrit-documentation.googlecode.com/svn/Documentation/2.2.2/index.htm...
-- Amir Elisha Aharoni · אָמִיר אֱלִישָׁע אַהֲרוֹנִי http://aharoni.wordpress.com “We're living in pieces, I want to live in peace.” – T. Moore
2012/3/25 Amir E. Aharoni amir.aharoni@mail.huji.ac.il:
Hi,
There's something in Gerrit that i still don't get, even after trying to ask this several times on mediawiki.org [1] and on IRC. I sincerely thank everyone who took the time to reply, but either i still misunderstand the replies or i do understand them and I Just Don't Like It [2].
Let's consider a patch set i submitted: https://gerrit.wikimedia.org/r/#change,3361 .
The patch set as i submitted it was not perfect and Aaron marked it "-1". So far, so good. Then Hashar amended the patch set and Nikerabbit amended it some more.
... Now that i look at that patch again, i realize that there were no code changes by Nikerabbit - only inline code comments. That confusion was probably caused by Gerrit's UI, with which many people already expressed dissatisfaction, but that's a separate issue.
My main claim still applies - it would make more sense to see every stage in the development of this patch as a separate commit. I tried to play with the "Old Version History" dropdown, which looked like it can help making it easier, but it only added more confusion.
-- Amir
There is a dropdown called "Old Version History" from which you can diff from a previous patchset instead of viewing the whole patch (yes, I had to be told about it). It's not perfect though, and for instance the commit message is always compared as if it were new.
You can also compare it by downloading with git review, but the only way I found requires you to copy the sha1 ids, which makes a horrible usability.
I like your proposal of doing patchsets as a branch. Sometimes the new patchset are better viewed as a completely different work, in which case it could be based from the parent, but most subsequent patchsets fit better in the branch model. Branched patchsets could then be moved to master as a merge (which is not only better from a history POV, but also in tracking the authorship) or as a cherry-pick (eg. for a typo fix)
2012/3/25 Platonides Platonides@gmail.com:
There is a dropdown called "Old Version History" from which you can diff from a previous patchset instead of viewing the whole patch (yes, I had to be told about it).
I actually saw it myself and i even mentioned it.
If i try to use it, the file "includes/filerepo/backend/FileBackendStore.php" appears in the list of modified files, even though it has nothing to do with any of these commits. It has "+0, -54" in the "size" column, which is supposed to mean that 54 lines were removed, and when i actually click it, then the diff is empty.
Is that supposed to happen?
-- Amir Elisha Aharoni · אָמִיר אֱלִישָׁע אַהֲרוֹנִי http://aharoni.wordpress.com “We're living in pieces, I want to live in peace.” – T. Moore
On 25/03/12 22:20, Amir E. Aharoni wrote:
2012/3/25 Platonides Platonides@gmail.com:
There is a dropdown called "Old Version History" from which you can diff from a previous patchset instead of viewing the whole patch (yes, I had to be told about it).
I actually saw it myself and i even mentioned it.
If i try to use it, the file "includes/filerepo/backend/FileBackendStore.php" appears in the list of modified files, even though it has nothing to do with any of these commits. It has "+0, -54" in the "size" column, which is supposed to mean that 54 lines were removed, and when i actually click it, then the diff is empty.
Yes, it seems that is still refers to the parent revision, even though it is now compared to another changeset. Although if you go to the diff, it shows empty for example (comparing to the other changeset).
Is that supposed to happen?
I find that a confusing interface. I have no idea if it's really intended or not.
wikitech-l@lists.wikimedia.org