checkin comments and r=reviewer
fantasai
to remind everyone that a checkin comment should include:
1. The bug number either as "Bug XXXX" or "b=XXXX" (the former seems preferred)
2. A summary of the change
3. The reviewers' approval given as one or more of the following
r=reviewer
sr=reviewer
r+sr=reviewer
(It seems 'reviewer' can be a comma-separated list.)
4. The patch author, given via the appropriate settings in Mercurial
or as either "patch by author" or "p=author" in CVS.
I'm calling out the syntax because I've seen e.g. r+cpearce in the logs,
which doesn't look valid to me. Using weird syntax makes it harder for us
to parse logs and extract data, so let's not do that.
~fantasai
Ehsan Akhgari
> 1. The bug number either as "Bug XXXX" or "b=XXXX" (the former seems
> preferred)
> 2. A summary of the change
> 3. The reviewers' approval given as one or more of the following
> r=reviewer
> sr=reviewer
> r+sr=reviewer
> (It seems 'reviewer' can be a comma-separated list.)
> 4. The patch author, given via the appropriate settings in Mercurial
> or as either "patch by author" or "p=author" in CVS.
>
It might be a good idea to update <
https://developer.mozilla.org/En/Mozilla-central#Pushing_changes_to_mozilla-central>
with these details if no one has any objections.
--
Ehsan
<http://ehsanakhgari.org/>
Mike Beltzner
> It might be a good idea to update <
> https://developer.mozilla.org/En/Mozilla-
> central#Pushing_changes_to_mozilla-central>
> with these details if no one has any objections.
Please do.
cheers,
mike
Ehsan Akhgari
Peter Weilbacher
> I just saw a few checkins today that didn't mark the reviewer, so I wanted
> to remind everyone that a checkin comment should include:
I think that could be a bug in qimportbz. I updated it this morning to
the latest version, tested it a bit and sometimes I get comments that
contain neither bug number nor review flags (like for bug 506869 or 500000).
Peter.
P.S.: This is probably not the right group for a discussion about that. But
not sure where else to follow-up.
Boris Zbarsky
> I think that could be a bug in qimportbz. I updated it this morning to
> the latest version, tested it a bit and sometimes I get comments that
> contain neither bug number nor review flags (like for bug 506869 or 500000).
qimportbz will use the checkin comment from the patch if the patch
contains one. The patches you mention do.
If people are posting patches with broken checkin comments, they need to
be told to stop doing that. It's a problem for any method of patch
import (hg import, hg qimport) not just qimportbz.
That said, people shouldn't be pushing without doing hg out and checking
the checkin comments...
-Boris
Christopher Aillon
Might be a good idea to enforce the syntax with a precommit hook?
Boris Zbarsky
> Might be a good idea to enforce the syntax with a precommit hook?
Some patches have no review (e.g. bustage fixes).
Though I have no problem with bringing back r=lumpy!
-Boris
Mike Connor
We can do something like the "CLOSED TREE" override we use for the
closed tree hook. "bustage fix" seems clear and unobstrusive enough.
-- Mike
Serge Gautherie
> <https://developer.mozilla.org/En/Mozilla-central#Pushing_changes_to_mozilla-central>
"r+sr=reviewer"
Iiuc, this is not allowed anymore (for new r/sr), per new sr policy.
We may want to remove it ...
"Bug 505691 - Remove unnecessary include of nsIPref.h. r=vlad"
... and extend the example.
*****
<https://developer.mozilla.org/en/Mercurial_FAQ#How_do_I_check_stuff_in.3F>
"How do I deal with "abort: push creates new remote heads!"?
Using hg merge
This is the simplest and safest thing to do. It leaves a merge commit in
the log, which some people find annoying. Nonetheless merging is the
recommended fix for this problem."
(I guess I am one of those annoyed by the uselessly added history.)
"Using hg rebase
As of Mercurial 1.1, rebasing (especially with mq patches) might lose
rename data; this is a known bug that has been fixed in Mercurial 1.3."
Shouldn't we rather recommend 'rebase' now?
Boris Zbarsky
> "r+sr=reviewer"
>
> Iiuc, this is not allowed anymore (for new r/sr), per new sr policy.
> We may want to remove it ...
"It depends".
For example, I just checked in a patch where reviewer A did the review,
reviewer B did the sr of the interfaces, and reviewer B did a second
review of some of the code that he was particularly familiar with.
I labeled it as "r=A, r+sr=B". How would you label it?
> "How do I deal with "abort: push creates new remote heads!"?
> Using hg merge
> This is the simplest and safest thing to do. It leaves a merge commit in
> the log, which some people find annoying. Nonetheless merging is the
> recommended fix for this problem."
>
> (I guess I am one of those annoyed by the uselessly added history.)
No, but this FAQ predates a sane rebase.
> Shouldn't we rather recommend 'rebase' now?
I think so (but isn't all that off-topic for this thread?)
-Boris
Zack Weinberg
>
> "How do I deal with "abort: push creates new remote heads!"?
> Using hg merge
> This is the simplest and safest thing to do. It leaves a merge commit
> in the log, which some people find annoying. Nonetheless merging is
> the recommended fix for this problem."
>
> (I guess I am one of those annoyed by the uselessly added history.)
It's not useless; it records what actually happened rather than
revising history to account for losing the race, and it means that you
can separate the effect of each individual change from the effect of the
merge, should this be necessary later.
I am a fan of bushy history.
zw
Ehsan Akhgari
> On 07/27/2009 10:03 PM, fantasai wrote:
>
> Might be a good idea to enforce the syntax with a precommit hook?
Filed this bug for getting the precommit hook:
https://bugzilla.mozilla.org/show_bug.cgi?id=506949
--
Ehsan
<http://ehsanakhgari.org/>
Ehsan Akhgari
> Serge Gautherie wrote:
>
>> "r+sr=reviewer"
>>
>> Iiuc, this is not allowed anymore (for new r/sr), per new sr policy.
>> We may want to remove it ...
>>
>
> "It depends".
>
> For example, I just checked in a patch where reviewer A did the review,
> reviewer B did the sr of the interfaces, and reviewer B did a second review
> of some of the code that he was particularly familiar with.
>
> I labeled it as "r=A, r+sr=B". How would you label it?
>
"r=A,B sr=B"?
--
Ehsan
<http://ehsanakhgari.org/>
Serge Gautherie
>> I labeled it as "r=A, r+sr=B". How would you label it?
>
> "r=A,B sr=B"?
(My own usual way would have been "r=(A + B) sr=B".)
But my main point would be: only one "r=", only one "sr=", ...
Serge Gautherie
> Serge Gautherie wrote:
>> "r+sr=reviewer"
>>
>> Iiuc, this is not allowed anymore (for new r/sr), per new sr policy.
>> We may want to remove it ...
>
> "It depends".
>
> For example, I just checked in a patch where reviewer A did the review,
> reviewer B did the sr of the interfaces, and reviewer B did a second
> review of some of the code that he was particularly familiar with.
>
> I labeled it as "r=A, r+sr=B". How would you label it?
(See following replies.)
My main point here would be to at least add a comment to remind that
having the same person do both (part of) r and sr is a special case.
*****
>> Shouldn't we rather recommend 'rebase' now?
>
> I think so (but isn't all that off-topic for this thread?)
Sure, I'll recreate a new thread...
David Mandelin
I also do unreviewed checkins occasionally to fix warnings. Maybe
r=bustage, r=warning, and so on?
Serge Gautherie
>> We can do something like the "CLOSED TREE" override we use for the
>> closed tree hook. "bustage fix" seems clear and unobstrusive enough.
>
> I also do unreviewed checkins occasionally to fix warnings. Maybe
> r=bustage, r=warning, and so on?
I wouldn't like r=anything_dummy.
(Or maybe "r=N/A"?)
Why not just "no review / not reviewed"? Or maybe "no_r=me"?!
(less restrictive than "bustage fix")
"bustage/fix/warning/..." should be part of the commit message itself.
Robert Kaiser
> Ehsan Akhgari wrote:
> "r+sr=reviewer"
>
> Iiuc, this is not allowed anymore (for new r/sr), per new sr policy.
> We may want to remove it ...
It's still allowed e.g. for SeaMonkey, possibly even for Thunderbird.
Robert Kaiser
Robert Kaiser
If only hgweb would display those merges, as well as copies and renames
in a better way, I think everyone would be happier.
Right now a merge is displayed as a diff of one random branch to the
other it's merged to, and that's a bit bogus.
Robert Kaiser
Robert Kaiser
> Peter Weilbacher wrote:
>> I think that could be a bug in qimportbz. I updated it this morning to
>> the latest version, tested it a bit and sometimes I get comments that
>> contain neither bug number nor review flags (like for bug 506869 or
>> 500000).
>
> qimportbz will use the checkin comment from the patch if the patch
> contains one. The patches you mention do.
It's either hard to do or cheating in most cases if you have a review in
the comment already when you attach the patch to Bugzilla ;-)
> That said, people shouldn't be pushing without doing hg out and checking
> the checkin comments...
True. Pushing without checking hg out before is always a bad idea.
Robert Kaiser
Robert Kaiser
> On Monday 2009-07-27 22:03 -0700, fantasai wrote:
>> 4. The patch author, given via the appropriate settings in Mercurial
>> or as either "patch by author" or "p=author" in CVS.
>
> should no longer be considered correct (since we switched from CVS
> to Mercurial); the patch author should *always* be indicated by the
> "From:" in the mercurial changeset.
True for Mercurial, but not every piece of actively maintained code is
in hg right now, some bits are still in CVS, and even 1.9.0 still get
maintained for security on CVS - for those bits the "p=author" is still
relevant.
Robert Kaiser
L. David Baron
> Right now a merge is displayed as a diff of one random branch to the
> other it's merged to, and that's a bit bogus.
It's not random -- it depends on which side was the first parent of
the merge, which depends in turn on which parent of the merge was
the parent at the time the merge was done.
In other words, the "right" way to do a merge is, I think, to update
to the tip of the place you're merging the thing into and then merge
in the changes. So, for example, if you wanted all tracemonkey +
mozilla-central merges to show the diffs as being the changes in JS
(which I think would be the "right" way, although others might
disagree), then the way to do those merges would be to have a tree
containing TM and M-C, update to the tip of M-C (not TM), and then
run "hg merge".
-David
--
L. David Baron http://dbaron.org/
Mozilla Corporation http://www.mozilla.com/
timeless
> It's either hard to do or cheating in most cases if you have a review in the
> comment already when you attach the patch to Bugzilla ;-)
actually, i've switched to this approach. and i have a couple of
patches i'll repost tomorrow with new r= lines.
I'll write r= the person i want to do the review and set r?. If I get
an r-, I won't push that change. There isn't much harm -- again,
people shouldn't be pushing random attachments from bugzilla (and I'd
hope qimportbz would generally complain about attachments w/ negative
flags).
The only place I had problems was where vlad stopped being the owner,
and as I said, I'll redo those patches soon.
IMO, having to post extra attachments with changes is not a problem,
so if I have to post a couple more attachments, so be it.
Cheating is getting the bug number right at the time of first post
with attachment. And well, for 500,000 I didn't try to do that :)
Robert Kaiser
> On Wednesday 2009-07-29 18:39 +0200, Robert Kaiser wrote:
>> Right now a merge is displayed as a diff of one random branch to the
>> other it's merged to, and that's a bit bogus.
>
> It's not random -- it depends on which side was the first parent of
> the merge, which depends in turn on which parent of the merge was
> the parent at the time the merge was done.
I was pretty sure that the code behind it doesn't randomize what to take
but it looks somewhat random to the casual user and has a tendency to
show exactly the thing you didn't want to see.
It would be helpful IMHO if by default it would only show that it's a
merge changeset and not show any changes that are in either of the
parents (can it contain anything else at all?) but give you links to
view any of the two possible diff views of the merge.
Robert Kaiser
Robert Kaiser
> On Wed, Jul 29, 2009 at 7:41 PM, Robert Kaiser<ka...@kairo.at> wrote:
>> It's either hard to do or cheating in most cases if you have a review in the
>> comment already when you attach the patch to Bugzilla ;-)
>
> actually, i've switched to this approach. and i have a couple of
> patches i'll repost tomorrow with new r= lines.
Still feels somewhat cheaty and possibly gives a wrong impression to
someone looking at the patch IMHO.
Because of that, I usually reside to posting a hg diff that doesn't
contain any checkin comment or user headers at all. I also don't enter
comments locally before it's ready for pushing (and keep the bug number
in the mq name). That way it's easy to see in hg out, bugzilla, etc.
that something is missing ;-)
> Cheating is getting the bug number right at the time of first post
> with attachment. And well, for 500,000 I didn't try to do that :)
True. I tend to not post patches right when filing a bug exactly for
this (and because filing early, even if the patch isn't ready, is
usually better for other following the same or similar things).
Robert Kaiser
Peter Weilbacher
> Peter Weilbacher wrote:
>> I think that could be a bug in qimportbz. I updated it this morning to
>> the latest version, tested it a bit and sometimes I get comments that
>> contain neither bug number nor review flags (like for bug 506869 or
>> 500000).
>
> qimportbz will use the checkin comment from the patch if the patch
> contains one. The patches you mention do.
Ah, thanks for the clarification. I didn't read the code to find that
out. I was just surprised that qimportbz didn't tell me anything about
that. Even with -v it doesn't give much useful output...
> If people are posting patches with broken checkin comments, they need to
> be told to stop doing that. It's a problem for any method of patch
> import (hg import, hg qimport) not just qimportbz.
I guess I was just testing the wrong bugs. If I attach a bug that I plan
to check in myself, then it's nice if it already contains part of the
ci comment (without the r/sr).
If someone else plans to use "checkin-needed" then this would not be
nice, I agree.
Cheers,
Peter.
Rob Arnold
<news...@weilbacher.org>wrote:
> On 28/07/09 15:30, Boris Zbarsky wrote:
>
>> Peter Weilbacher wrote:
>>
>>> I think that could be a bug in qimportbz. I updated it this morning to
>>> the latest version, tested it a bit and sometimes I get comments that
>>> contain neither bug number nor review flags (like for bug 506869 or
>>> 500000).
>>>
>>
>> qimportbz will use the checkin comment from the patch if the patch
>> contains one. The patches you mention do.
>>
>
> Ah, thanks for the clarification. I didn't read the code to find that
> out. I was just surprised that qimportbz didn't tell me anything about
> that. Even with -v it doesn't give much useful output...
Noted. I'll try to fix that in a future version. -v should be more verbose -
there's a fair amount of information I can give during the import process. I
can only fix bugs that people report to me (or that I discover myself, but
my usage isn't the same).
-Rob
Jeff Walden
> Though I have no problem with bringing back r=lumpy!
"bringing back"? I was never aware it went away! See bug 481436 comments 14[0],16[1].
Jeff
0. https://bugzilla.mozilla.org/show_bug.cgi?id=481436#c14
1. https://bugzilla.mozilla.org/show_bug.cgi?id=481436#c16