On 20/07/12 08:50, Antoine Musso wrote:
Platonides wrote:
What I have trouble with is in imagining the UI for reviewing a topic branch, with perhaps several commits per "step".
To me the concept of topic is to lamely tag a commit as being part of specific area of code such as API, Parser, tests. We could probably tweak Gerrit to automatically assign a set of reviewer just on the basis of the topic name.
I see a topic as a zone of the code, like 'parser'. If you added a pluggable bparser system where you add support for wikicreole that would indeed be a parser commit. But the glue should be more related than just all of them having the same topic, which is also shared by eg. some intermixed fixes to <li> output.
If someone is working on a big changes having several steps, I would prefer we use a branch (for now the sandbox system). Then each final commit will be one of the step and all the intermediate/fixing commits would be patchsets.
Ok, we could call them branches instead of "topic branches". We could rephrase it as "an UI for properly reviewing a short-lived branch".
One could also submit all their patchsets and have them submitted in reversed date order. The submission of the first patch that has an already merged parent will then trigger a merge commit of all the changes. An example with some doc rewrite I did which was merged by Sam in mediawiki/core.git:
$ git log --oneline --graph d11d328^..444ab87
- 444ab87 Merge changes
Icfd267a9,If956c0a1,I0e232273,Ia84494c8,Ib70e0046,If2c01a11,I40196371 |\ | * 4a69fce prettify documentation | * 12e608e converts '@fixme' to '@todo FIXME' | * d5737f8 update @param @return doc in several files | * bcf0010 prettify filerepo documentation | * 9aa825f prettify SkinTemplate:makeLink documentation | * 1dd38a7 move parameter doc under @param | * aab43dd escape tags and entity in doxygen comments
- | 1a9246e ChannelFeed now marked as an abstract class
- | f3f985c remove unneeded @abstract
|/
- d11d328 Merge "Fixed Language::isValidBuiltInCode exception."
- 75f5bda Fixed Language::isValidBuiltInCode exception.
$
I made my changed in sequence and based on d11d328. Submitted all of them then Sam submitted all my change submitting aab43dd as the last. Gerrit then happily merged them.
I got confused by that explanation of "submitted in reversed date order". Seems you just submited dependent patches. Does merge 4a69fce automatically merge all of them? What if 1dd38a7 had been marked as -2? Would it still be possible to merge 4a69fce? How does the reviewer know that it is part of a bigger group? If I go to https://gerrit.wikimedia.org/r/14915 I get no indication that it is part of a bigger set.
Always registering as a merge, has such as storing the merger name as the commiter, but I'd like keeping them as fest-forwards having for one-commit changes.
A strategy could be setup where we always merge as to know from git history who actually applied the change but do a fast forward when the submitter is the patch author. Still messy though, I can live without the merge commits.
I think the history would easily get hard to browse.
One can filter out merge commit by passing `--no-merges` to `git log`
But it sorts by date, not by when it entered master.
A recent example:
$ git log --no-merges --oneline ef95343 prevents some doxygen warnings 85c84c2 enhance DefaultSettings.php documentation 089c58d jshint: resources/jquery/* 87524b1 (bug 38376) call to undefined method ThumbnailImage::getPath() 22f10ef For consistency of the interface, don't do expensive checks on page view. 1aac87e Fix for: Fatal error: Call to undefined method ApiDelete::getErrorsArray() 02cde99 Update messages.inc and rebuild MessagesEn.php. 84ce626 Moved getDatabase() from contribs pager to Pager class. d6e31f6 Sort result of FileRepo::findBySha1 a16288e Increase length of edit summary by 5 bytes fb9cbb1 Fix casing of wgDBerrorLogInUtc fe6da52 memcached: better error messaging bc6d5fb dbErrorLog can now be forced to UTC 4b9bd6a Do not show empty categories with list=allcategories
I would expect
ef95343 prevents some doxygen warnings d80ebf9 In LinkHolderArray::doVariants(), redlinks need to be checked as well 6601c36 Retrieve rev_len for sizediff in usercontributions API 85c84c2 enhance DefaultSettings.php documentation 089c58d jshint: resources/jquery/* 87524b1 (bug 38376) call to undefined method ThumbnailImage::getPath()
I just found --topo-order while writing thie email, it does seem to provide it...
Also, I don't like how when browsing a merge (eg. gitk) you are not shown the actual changes to the repository (which for us is the left-parent). Anyone knows of a way to do so?
A merge commit does not really have any change by itself. I just do a diff against the merge parent such as:
git diff 444ab87^ 444ab87
The related changelist would be: git log --oneline 444ab87^..444ab87
That's an interesting way to solve. Although not always as useful as if you got the diff inline.