PSA: One commit per codereview

[Isaac Levy]

Chromies,

Tonight I added some presubmit checks to prevent reusing a codereview issue for multiple commits.  As a reminder, 

if you want to reland a patch, please upload to a new issue number, include a link to the original review and mark

the subject something like "Relanding YYY...".

If you have any questions about this, please let me know!

-Isaac

[John Abd-El-Malek]

I think not everyone does things this way; I don't think we should force everyone.

I find it helpful that relands are in the same issue. When looking through history for a commit, all the review comments are together in one issue where I can compare different patchsets.

The main reason I heard about this is for CQ. There are other ways to solve this; i.e. the CQ could have Rietveld mark existing lgtms as "expired" once it commits a change.

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Dirk Pranke]

Interesting. I would find it very confusing if a single rietveld issue
had multiple commits. I've always assumed that if you needed to tie N
commits together you'd do that using a bug.

-- Dirk

[Scott Hess]

The case where I've used this in the past is when a CL gets reverted
and needs one or two relatively trivial changes relative to the
overall CL and review. In that case spinning up an entire new issue
makes it hard to see the substance of the changes you've made to fix
the reason you were reverted.

Another option is to open a new CL with the pre-fixed version, then
upload the fixes, so that the reviewer can easily look at
just-the-fixes.

-scott

[Isaac Levy]

Our tools do not work well for multiple commits on a single review page.  Among other reasons:

- It generates confusing source history where the same description could get listed twice.

- It sets a precedent of LGTMing an idea instead of code.  Once an issue number is approved, it's approved.  Reusing the issue sidesteps our approval process.  I feel that if a patch got reverted, by default we should ask for a fresh lgtm before a reland (with lots of exceptions to this rule).

John, would Scott's suggestion of uploading the original CL before further hacking be sufficient for your workflow?

-Isaac

[Albert J. Wong (王重傑)]

I also fall in the camp of reusing the same issue for trivial changes when relanding. If you were to tell me to start doing trivial fixes in new issues, I would almost certainly TBR those changes or dcommit -f them since blocking on a LGTM feels too bureaucratic there. Not sure if that's actually an improvement.

What are we trying to gain by forcing a new issue for relands?

-Albert

On Tue, Dec 4, 2012 at 9:55 PM, Scott Hess <sh...@chromium.org> wrote:

[John Abd-El-Malek]

On Tue, Dec 4, 2012 at 9:33 PM, Dirk Pranke <dpr...@chromium.org> wrote:

Interesting. I would find it very confusing if a single rietveld issue
had multiple commits. I've always assumed that if you needed to tie N
commits together you'd do that using a bug.

To be clear, I'm not talking about different changes. I'm talking about the same change with fixes after a reland.

[John Abd-El-Malek]

On Tue, Dec 4, 2012 at 10:00 PM, Isaac Levy <il...@chromium.org> wrote:

Our tools do not work well for multiple commits on a single review page.  Among other reasons: 

- It generates confusing source history where the same description could get listed twice.

I'm not sure what you mean, which tool are you referring to? When this is done, I see one description only in the svn history. It will have multiple "Committed" lines, which I also find helpful.

- It sets a precedent of LGTMing an idea instead of code.

I think this, like "lgtm with nit", is a bit of a judgement call that we depend on developers. i.e. if a change gets reverted because it missed a function update on a config that didn't get tried, and the author fixes it, there's no need to review.

 

 Once an issue number is approved, it's approved.  Reusing the issue sidesteps our approval process.

Even ignoring whether there's divergence on this topic, there are other ways of accomplishing this like the suggestion I gave earlier (have lgtm's "expire" after the first commit).

 I feel that if a patch got reverted, by default we should ask for a fresh lgtm before a reland (with lots of exceptions to this rule).

John, would Scott's suggestion of uploading the original CL before further hacking be sufficient for your workflow?

no because of the reasons above.

[Darin Fisher]

What problem was this solving?  Based on the thread so far, it seems to just preclude a reasonable use case.  I'm not seeing the upside.

-Darin

On Tue, Dec 4, 2012 at 8:47 PM, Isaac Levy <il...@chromium.org> wrote:

--

[Nico Weber]

+1 to what John, Albert, and Darin said.

Nico

[Jochen Eisinger]

I can't comment on the reason for the presubmit check, however, a similar issue: In http://crbug.com/162012 I filed a request for disallowing uploading new patchsets to a closed review.

The reason for this is that our tools fail badly on closed issues (e.g. tryjobs don't start). I guess that's mainly because they don't scanned closed issues. Rather than silently failing, my reasoning is, we should bail out early.

You can work around this restriction by opening the issue before uploading. Then our tools also work again (somewhat, the CQ is still confused about LGTMs).

Coming back to the original topic of this thread (the presubmit check). I personally prefer using a fresh review. I realize that there are several short-comings, however, like not easily seeing the delta to the last committed version, or having to manually add the old reviewers.

Aside from that, I'd prefer a uniform way.

I wonder whether most of the concerns (workflows we got used to will break) could be adressed by adding something like git cl revert and git cl reland: They'd both take either a rietveld issue number of git hash, create a fresh branch with the revert or cl applied, do the necessary setup of the new issue (first upload the old version in case of the reland, add all reviewers, for a revert query a reason to add to the description, pick the right reviewer)?

best

-jochen

[Isaac Levy]

I didn't realize that some were using this as a regular workflow; apologies for not proposing this in advance.

Isaac

On Tue, Dec 4, 2012 at 10:42 PM, Darin Fisher <da...@chromium.org> wrote:

What problem was this solving?  Based on the thread so far, it seems to just preclude a reasonable use case.  I'm not seeing the upside.

1) To encourage developers to pick a title for each commit, even for a reland with minor changes.  I find multiple commits with the same description pretty confusing and I suspect I am not alone.

2) To make rietveld issues less confusing.

-- If I see 3 patchsets on a page, and it says committed at the top, is patchset 3 committed?  Or maybe patchset 2 was committed and the author uploaded patchset 3 in preparation for relanding?

-- The review url is included in commit #1 svn logs.  But the review might no longer be about commit #1.  In fact the description could have been edited in preparation for relanding the commit.

[Anthony Berent]

Isaac,

I am confused one of your points:

-- If I see 3 patchsets on a page, and it says committed at the top, is patchset 3 committed?  Or maybe patchset 2 was committed and the author uploaded patchset 3 in preparation for relanding?

 

I don't see how you can fix this unless you say "one patchset per code review", hence forbidding new patchsets fixing reviewer's comments. This would mean either bypassing the commit queue and using dcommit, or creating a new code review, to fix every nit.  I have so-far been using one "commit" per review, but have been answering comments with "git commit --amend" and "git cl upload", which creates a new patchset.

A better answer to this problem would be for the commit bot to record which patchset it is committing in the log.

- Anthony

 

 

[John Abd-El-Malek]

On Wed, Dec 5, 2012 at 12:11 AM, Jochen Eisinger <joc...@chromium.org> wrote:

I can't comment on the reason for the presubmit check, however, a similar issue: In http://crbug.com/162012 I filed a request for disallowing uploading new patchsets to a closed review.

The reason for this is that our tools fail badly on closed issues (e.g. tryjobs don't start).

That's not true, at least for me and the others who use this to reland.

I guess that's mainly because they don't scanned closed issues. Rather than silently failing, my reasoning is, we should bail out early.

If there's a certain combination of tools that doesn't work in this case, they can be fixed instead of breaking a workflow that's used by many people..

 

You can work around this restriction by opening the issue before uploading. Then our tools also work again (somewhat, the CQ is still confused about LGTMs).

Yes, this is known. but we shouldn't be adding roadblocks in developers' workflow. things should work automatically.

 

Coming back to the original topic of this thread (the presubmit check). I personally prefer using a fresh review. I realize that there are several short-comings, however, like not easily seeing the delta to the last committed version, or having to manually add the old reviewers.

What I tried to say earlier is that I understand some people prefer this. But many others don't. So we shouldn't unilaterally force everyone to conform by changing how something worked for the last ~4 years.

 

Aside from that, I'd prefer a uniform way.

I wonder whether most of the concerns (workflows we got used to will break) could be adressed by adding something like git cl revert and git cl reland: They'd both take either a rietveld issue number of git hash, create a fresh branch with the revert or cl applied, do the necessary setup of the new issue (first upload the old version in case of the reland, add all reviewers, for a revert query a reason to add to the description, pick the right reviewer)?

The people that are complaining that they want one issue for relands, by definition, wouldn't like this. 

[John Abd-El-Malek]

On Wed, Dec 5, 2012 at 1:47 AM, Isaac Levy <il...@chromium.org> wrote:

I didn't realize that some were using this as a regular workflow; apologies for not proposing this in advance.

Isaac

On Tue, Dec 4, 2012 at 10:42 PM, Darin Fisher <da...@chromium.org> wrote:

What problem was this solving?  Based on the thread so far, it seems to just preclude a reasonable use case.  I'm not seeing the upside.

1) To encourage developers to pick a title for each commit, even for a reland with minor changes.  I find multiple commits with the same description pretty confusing and I suspect I am not alone.

Developers can change the description after reuploading. Note that since uploading to the same issue is in the case of relands, having the same description _is_ the norm.

 

2) To make rietveld issues less confusing.

-- If I see 3 patchsets on a page, and it says committed at the top, is patchset 3 committed?  Or maybe patchset 2 was committed and the author uploaded patchset 3 in preparation for relanding?

Anthony's suggestion of listing the patchset that the commit queue used is another solution to this.

 

-- The review url is included in commit #1 svn logs.  But the review might no longer be about commit #1.

I don't understand this comment. A reland is the same patch! It might have an extra fix, say for a platform that wasn't in the tryjobs, but the comments in the review still apply. Disassociating them makes it harder when going back through svn history and trying to understand why a particular line was changed.

[Justin Schuh]

On Wed, Dec 5, 2012 at 7:47 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Wed, Dec 5, 2012 at 1:47 AM, Isaac Levy <il...@chromium.org> wrote:

I didn't realize that some were using this as a regular workflow; apologies for not proposing this in advance.

Isaac

On Tue, Dec 4, 2012 at 10:42 PM, Darin Fisher <da...@chromium.org> wrote:

What problem was this solving?  Based on the thread so far, it seems to just preclude a reasonable use case.  I'm not seeing the upside.

1) To encourage developers to pick a title for each commit, even for a reland with minor changes.  I find multiple commits with the same description pretty confusing and I suspect I am not alone.

Developers can change the description after reuploading. Note that since uploading to the same issue is in the case of relands, having the same description _is_ the norm.

 

2) To make rietveld issues less confusing.

-- If I see 3 patchsets on a page, and it says committed at the top, is patchset 3 committed?  Or maybe patchset 2 was committed and the author uploaded patchset 3 in preparation for relanding?

Anthony's suggestion of listing the patchset that the commit queue used is another solution to this.

 

-- The review url is included in commit #1 svn logs.  But the review might no longer be about commit #1.

I don't understand this comment. A reland is the same patch! It might have an extra fix, say for a platform that wasn't in the tryjobs, but the comments in the review still apply. Disassociating them makes it harder when going back through svn history and trying to understand why a particular line was changed.

+1. This is the logical way to do it. On the few occasions I've had to walk back the history where it was done differently I found it extremely confusing.

[Peter Mayo]

The key point that breaks the reland for me is the fact that the revert is "reviewed" as a separate issue.

The notes about the failure are critical to the fixing, as are the notes of why the code was submitted that way in the first place.  My suggestion to self has been for a while to do the equivalent of a git cl revert that works like commit only inverts the patch, and a mechanism that allows easy invocation of this from the UI to any committer(hence sheriff)  (109650)

This also pushes the model of reitveld around a little it seems, and gerrit more so.  I don't have answers for all of those yet.

FWIW,

Peter.

Peter Mayo |

Waterloo |

pete...@google.com |

519-880-3439

[Scott Hess]

On Wed, Dec 5, 2012 at 7:44 AM, John Abd-El-Malek <j...@chromium.org> wrote:
> On Wed, Dec 5, 2012 at 12:11 AM, Jochen Eisinger <joc...@chromium.org> wrote:
>> I can't comment on the reason for the presubmit check, however, a similar
>> issue: In http://crbug.com/162012 I filed a request for disallowing
>> uploading new patchsets to a closed review.
>>
>> The reason for this is that our tools fail badly on closed issues (e.g.
>> tryjobs don't start).
>
> That's not true, at least for me and the others who use this to reland.

At least for myself, when I re-open a CL to make a patch before
re-landing, well, I re-open the CL.

No matter what, changes which need to be landed twice are going to be
a problem. Either there are two trunk commits referring to the same
Rietveld issue, or there are two trunk commits referring to parallel
Rietveld issues. In the latter case, if you want to know the review
history you better hope that the second Rietveld issue mentions the
first one, because otherwise you're going to have to go poke around in
the SVN history looking for the original patch.

I can imagine a system which would resolve this by somehow fork'ing
the original CL. Such a system would probably involve a substantial
amount of effort for a fairly small gain. If you see people
inappropriately re-landing CLs, spank them. If we have people
routinely subverting the system inappropriately, we're already doomed.

-scott

[Jonathan Kliegman]

On Wed, Dec 5, 2012 at 11:32 AM, Justin Schuh <jsc...@chromium.org> wrote:

On Wed, Dec 5, 2012 at 7:47 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Wed, Dec 5, 2012 at 1:47 AM, Isaac Levy <il...@chromium.org> wrote:

I didn't realize that some were using this as a regular workflow; apologies for not proposing this in advance.

Isaac

On Tue, Dec 4, 2012 at 10:42 PM, Darin Fisher <da...@chromium.org> wrote:

What problem was this solving?  Based on the thread so far, it seems to just preclude a reasonable use case.  I'm not seeing the upside.

1) To encourage developers to pick a title for each commit, even for a reland with minor changes.  I find multiple commits with the same description pretty confusing and I suspect I am not alone.

Developers can change the description after reuploading. Note that since uploading to the same issue is in the case of relands, having the same description _is_ the norm.

 

2) To make rietveld issues less confusing.

-- If I see 3 patchsets on a page, and it says committed at the top, is patchset 3 committed?  Or maybe patchset 2 was committed and the author uploaded patchset 3 in preparation for relanding?

Anthony's suggestion of listing the patchset that the commit queue used is another solution to this.

 

-- The review url is included in commit #1 svn logs.  But the review might no longer be about commit #1.

I don't understand this comment. A reland is the same patch! It might have an extra fix, say for a platform that wasn't in the tryjobs, but the comments in the review still apply. Disassociating them makes it harder when going back through svn history and trying to understand why a particular line was changed.

+1. This is the logical way to do it. On the few occasions I've had to walk back the history where it was done differently I found it extremely confusing.

I've actually been confused the other way, but it may be because I'm a CrOS committer, not a chromium one.

Its been confusing tracking down through the tools / svn logs what the state of a commit that was reverted is in.  I saw a CL land, then a revert message, but still saw the same behavior.  I saw a commit message in the svn log but as it linked to the issue I'd seen reverted, wasn't aware that it was the second landing of that CL.  I'd thought the revert had just failed to apply (or get merged to the branch).

More familiarity with the tools, logs or commit messages might help prevent this in the future.  But having a new issue would've made this case much cleaner.

[Ben Goodger (Google)]

+2, this use case must be supported. In fact, I filed this feature request:

https://code.google.com/p/chromium/issues/detail?id=163250

One note: If we have toolchain support, whether or not the existing CL gets reused becomes an implementation detail if you like - the toolchain could automate the creation of a new CL that refers back to the old one and starts seeded with the last patchset of the old one (so we can see the fixes easily). I would be fine with that.

-Ben

[Ryan Hamilton]

FWIW, this is the workflow I use.

On Tue, Dec 4, 2012 at 9:55 PM, Scott Hess <sh...@chromium.org> wrote:

[Dominic Mazzoni]

On Wed, Dec 5, 2012 at 12:11 AM, Jochen Eisinger <joc...@chromium.org> wrote:

I wonder whether most of the concerns (workflows we got used to will break) could be adressed by adding something like git cl revert and git cl reland: They'd both take either a rietveld issue number of git hash, create a fresh branch with the revert or cl applied, do the necessary setup of the new issue (first upload the old version in case of the reland, add all reviewers, for a revert query a reason to add to the description, pick the right reviewer)?

I vote for having "git cl reland" either way - whether the same issue is used or not, it could populate the description with all of the useful metadata about the reland (the original issue number if applicable, the land revision, the revert revision, and a placeholder for the reason for revert and explanation of the fix to re-land.)

I added a note to the bug Ben filed.

- Dominic

[Marc-Antoine Ruel]

2012/12/5 Dominic Mazzoni <dmaz...@google.com>

On Wed, Dec 5, 2012 at 12:11 AM, Jochen Eisinger <joc...@chromium.org> wrote:

I wonder whether most of the concerns (workflows we got used to will break) could be adressed by adding something like git cl revert and git cl reland: They'd both take either a rietveld issue number of git hash, create a fresh branch with the revert or cl applied, do the necessary setup of the new issue (first upload the old version in case of the reland, add all reviewers, for a revert query a reason to add to the description, pick the right reviewer)?

I vote for having "git cl reland" either way - whether the same issue is used or not, it could populate the description with all of the useful metadata about the reland (the original issue number if applicable, the land revision, the revert revision, and a placeholder for the reason for revert and explanation of the fix to re-land.)

^^^ that's a nice idea. I like it. Along with a git cl revert, with both that would understand revision number.

M-A 

 

I added a note to the bug Ben filed.

- Dominic

[Dirk Pranke]

On Tue, Dec 4, 2012 at 10:31 PM, John Abd-El-Malek <j...@chromium.org> wrote:
>
>
> On Tue, Dec 4, 2012 at 9:33 PM, Dirk Pranke <dpr...@chromium.org> wrote:
>>
>> Interesting. I would find it very confusing if a single rietveld issue
>> had multiple commits. I've always assumed that if you needed to tie N
>> commits together you'd do that using a bug.
>
>
> To be clear, I'm not talking about different changes. I'm talking about the
> same change with fixes after a reland.
>

It's abundantly clear now, but I missed this last night. I'm fine w/
re-using the same issue to reland a patch w/ fixes. More importantly,
I don't think a new lgtm should always be required, new issue or no
(there are times when the change might justify it, of course).

-- Dirk

[Elliot Glaysher (Chromium)]

On Tue, Dec 4, 2012 at 11:31 PM, Nico Weber <tha...@chromium.org> wrote:

+1 to what John, Albert, and Darin said.

+1 to these plus ones.

[Wez]

+1

[Isaac Levy]

OK.

I've removed the "Committed" presubmit check announced at the start of this thread.

-Isaac

[Victor Khimenko]

On Wed, Dec 5, 2012 at 9:29 PM, Dirk Pranke <dpr...@chromium.org> wrote:

On Tue, Dec 4, 2012 at 10:31 PM, John Abd-El-Malek <j...@chromium.org> wrote:
>
>
> On Tue, Dec 4, 2012 at 9:33 PM, Dirk Pranke <dpr...@chromium.org> wrote:
>>
>> Interesting. I would find it very confusing if a single rietveld issue
>> had multiple commits. I've always assumed that if you needed to tie N
>> commits together you'd do that using a bug.
>
>
> To be clear, I'm not talking about different changes. I'm talking about the
> same change with fixes after a reland.
>

It's abundantly clear now, but I missed this last night. I'm fine w/
re-using the same issue to reland a patch w/ fixes. More importantly,
I don't think a new lgtm should always be required, new issue or no
(there are times when the change might justify it, of course).

It's the same as with the normal LGTM. If someone gave you an LGTM and then you go and redo the whole thing from scratch then you should ask for a new review in both cases: when change is not yet committed and when it's committed and then reverted. If change is tiny then in both cases it's probably not needed.

It's continuation of the very same process. The fact that you've committed the change and broke the tree (changes are rarely reverted if they don't break something) is unfortunate, but why this case should be different from the case where trybots or one of reviewers caught the problem?

Ideally revert should refer the same issue, but it's usually done by some other person (build sheriff) thus it's not trivial.