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.