On Fri, 27 Jul 2012 01:10:52 -0700, Antoine Musso hashar+wmf@free.fr wrote:
Daniel Friesen write:
I want whatever review system we use to support real review branches;
- Review sets are heads with multiple commits, not single commits.
- -no-ff merge commits are used so that only the final commit code makes
it directly into master.
We have disallowed branch creation and merging for now, that can definitely be opened up. Chad recently opened the sandbox reference and we have a few of them:
remotes/gerrit/sandbox/apramana/gsoc remotes/gerrit/sandbox/demon/foo-bar remotes/gerrit/sandbox/devunt/oauth remotes/gerrit/sandbox/ori/hacks
They are actual branch we can most definitely merge -no-ff back in master. http://www.mediawiki.org/wiki/Gerrit/personal_sandbox
- We don't amend commits, we make new ones to the head of the review
branch.
You will be able to do that. I am afraid the end result is not going to look nice history wise with ton of followups to fix previous commits. That will definitely make finding the root cause of bug and its context harder to find out. Think about a commit that introduce a bug for which the commit message is:
Ahh typo in 3124a09 fix it
It is not going to be helpful :-D
Scrap all the other complaints. This pattern of amending commits is by far the worst thing about our current workflow.
I firmly disagree with you there. It let someone send some code then enhance it without cluttering everyone else code. That let you work in a collaborative way, finding out what the other person has done patch after patch. We eventually end up with a patchset that only got published (merged) to everyone whenever it is close to perfect. That makes our code history nicer and push the review stress to the submitters (lot of people) instead of toward the reviewers (few people).
I do agree though that the system can be confusing and is poorly toolized. git-review is a step to abstract the whole patchset system, but I am sure we can make it a better/easier tool to use.
Development history and minor contributions that never make it into the repo.
I think we do not care about the history of a change. The typo / comment update / logic errors, I am sure we do not care about. If someone has a change that requires to be split in several changes, that should easier be a branch (see sandbox above) or a chain of changes made interdependants (which is really just a local branch).
How would you do that? Well lets say I want to update our documentation to several area. I would first create a local branch:
git checkout -b doc -t origin/master <do my doc updates, commit as needed>
I then push that local branch to Gerrit (git push doc:refs/for/master), that craft a change per commit. Whenever one of the commit need to be amended I edit my local branch commits with git rebase --interactive. Once happy I repush the whole branch and Gerrit magically update all the patchsets.
The drawback is that editing just a commit out of several one will still trigger a new patchset to all the child commits although there is no code change per see (the parent got changed). That could be detected in Gerrit I guess. We might have Gerrit to automatically rebase children whenever a parent is modified. Gerrit could also use a feature to prevent the chain of commits to be submitted until they have all been reviewed. Once the last change is submitted, that will trigger a git merge --no-ff. We currently have to handle that manually by submitting the first commit last.
"Committer" going to the person who fixes a typo rather than the person who writes the code.
The Author field is kept around (unless someone amend it). See as an example https://gerrit.wikimedia.org/r/#/c/5550/, I am the patch author, Saper sent patchset 3 but I am still mentioned as the author.
git does not support multiple authors as far as I know, but one could add himself in the commit message by adding a new field such as:
Co-authored-by: John Doh johndoe@example.org
Confusion over whether someone has rebased or let unrelated changes leak into their patchset.
That is more a matter of educating our developers. Whenever I submit a new patchset, I add a cover message in Gerrit to introduce what that change did even if it is "just a rebase".
Huge development effort by multiple people potentially turning into a single commit with a single committer. ...
As I said previously, huge effort involving multiple persons could be made branch.
At this point I think the real question we have is this: "Can we reasonably get Gerrit to support real review branches?"
The answer is most definitely yes. We would need to open up branches creations to everyone.
If that answer to that question is no. Or "not without very significant development". Then the only option we have is to track down the system that we can get to support that with the most reasonable amount of development effort.
I tend to agree on that :-)
Most of your reply appears to be based on an assumption based around real branches in Gerrit rather than the ephemeral-branch-as-review/commit-as-patchset pattern discussed in the list earlier. Naturally the way you're suggesting to configure Gerrit is not the way I'm suggesting it work.
Review pages for what we see now as individual commits would be an ephemeral branch. Really just a moving head. When it gets accepted there wouldn't really be any head left in refs/heads/ as a branch. On the review pages the spot were we see "patchsets" be individual commits in that faux branch. Instead of being completely separate commits these would be actual commits building on each other.
Though rather than taking that Gerrit oriented description. Why not just read the previous emails. This one is my explanation of a --no-ff workflow: http://thread.gmane.org/gmane.science.linguistics.wikipedia.technical/62461/... Shortly after I posted that description Platonides independently ended up describing basically the same thing I was suggesting: http://thread.gmane.org/gmane.science.linguistics.wikipedia.technical/62461/...
You also seem to be making a false assumption about history based around limitations in linear history like in svn that don't exist in a non-linear --no-ff merge commit environment.
When you merge sets of changes into the master using --no-ff (Gerrit, whatever would do this automatically) the individual commits never really become part of master. They are there, they are in the history. But they are on a side of the branch you don't use unless you really mean to. When you are walking through the history of master (bisecting) you only want to walk one side of master. There is no point in splitting in two there as the commits that never got directly committed into master can never be the source of a bug. Because these sides are separate commits like "Ahh typo in 3124a09 fix it" will NEVER get in the way of fixing something. In fact, once you do find the commit that introduced a bug you can go into the other side of the branch and drill down into the individual commits and discover whether this bug introduced was something fundamental to the addition to core or was a last minute mistake caused after someone told the committer to fix something else and ended up creating a bug while fixing one.