Re-using CLs when re-landing a reverted change, 2015 edition

[Lei Zhang]

It's been a while since our last discussion on this topic, [1] but I'm
in the do not reuse CLs camp. I've seen some recent changes that have
re-used a ChangeList twice, or even thrice. If your CL gets reverted,
please start a new CL rather than reusing the old reverted CL.

Downsides to reusing the CL:
- Commit messages are confusing. e.g. [2]
- The discussion on the CL tends to drag on and become hard to follow.

Upsides to reusing the CL:
- All the LGTMs are already in the CL. Make some more changes and
commit-bot will put it in the CQ with no more approvals needed.

We don't have a shortage of CL numbers. Please use a new one. In the new CL:
- Revert the revert and upload as patch set 1.
- Edit the CL description to mention this is attempt #2 and point to
the old CL for reference.
- Make your fixes and upload as patch set 2.
- Ask your original reviewers to review again based on the diff from
patch set 1 to patch set 2. Hopefully the changes are small and this
review will be quick.

[1] https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/pUCEqaugA9M
[2] https://crrev.com/346281

[Ian Clelland]

I was actually just about to ask about this situation, so thanks for the timely advice!

One question -- what if I want to re-land the original CL with no changes at all? In the situation I'm currently dealing with, the reverted CL was in Chromium, but the changes that needed to be made were to layout tests in Blink. Now that the changes have been committed, can I re-commit the original CL again, or should I still make a new one (and get new LGTMs)?

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

[Scott Graham]

I prefer reuse because there's not necessarily a reference from the 1st to the 2nd, so it's not clear if/when it relanded. It also keeps the discussion/attempts together which is sometimes nicer (but I guess my "togetherness" is your "dragging on").

Maybe there's something we could do to make the commit messages more readable?

(But I don't care that much, so if there's otherwise consensus, I'm fine switching. It seems like it happens less frequently these days with trybots more closely approximating waterfall.)

[Ben Goodger (Google)]

I don't think there's consensus here so I'd stop short of asking everyone to change their workflow.

I prefer to re-use since it it's way easier, e.g. the following things are carried forward:

- existing description

- existing reviewers & comments

Also it's convenient to have a single CL identifier track the entire lifetime of a change.

Maybe the tooling could be better allowing creation of a derived CL, but at that point why not just reuse the existing one? Or why not improve Rietveld to collapse patchsets/comments prior to commit?

-Ben

On Tue, Sep 1, 2015 at 12:29 PM, Lei Zhang <the...@chromium.org> wrote:

[John Abd-El-Malek]

+1 to Scott & Ben's. Without repeating everything they said, this has come up many times. Some feel it's much better to create a new one, and others (me included) feel it's much better to reuse.

On Tue, Sep 1, 2015 at 12:54 PM, Ben Goodger (Google) <b...@chromium.org> wrote:

I don't think there's consensus here so I'd stop short of asking everyone to change their workflow.

I prefer to re-use since it it's way easier, e.g. the following things are carried forward:

- existing description

- existing reviewers & comments

Also it's convenient to have a single CL identifier track the entire lifetime of a change.

Maybe the tooling could be better allowing creation of a derived CL, but at that point why not just reuse the existing one? Or why not improve Rietveld to collapse patchsets/comments prior to commit?

-Ben

On Tue, Sep 1, 2015 at 12:29 PM, Lei Zhang <the...@chromium.org> wrote:

It's been a while since our last discussion on this topic, [1] but I'm
in the do not reuse CLs camp. I've seen some recent changes that have
re-used a ChangeList twice, or even thrice. If your CL gets reverted,
please start a new CL rather than reusing the old reverted CL.

Downsides to reusing the CL:
- Commit messages are confusing. e.g. [2]

I actually find this an upside :) Seeing in the commit message each time it landed (and also in the same rietveld issue, when it was reverted) is convenient when trying to figure out if the revert/unrevert of a change fixed or didn't fix something. It also helps one figure out if a change is in a specific revision (i.e. branchpoint).

 

- The discussion on the CL tends to drag on and become hard to follow.

Again, this is an upside for me. Losing that conversation about the design in the committed reland is not always good. I've found those discussion threads, and diffing patchsets in a cl to see why something changed, very valuable when looking at old changes.

 

Upsides to reusing the CL:
- All the LGTMs are already in the CL. Make some more changes and
commit-bot will put it in the CQ with no more approvals needed.

We don't have a shortage of CL numbers.

This is a strawman :) No one reuses because of a shortage of IDs.

[Lei Zhang]

Ok, if everyone feels the other way or are ambivalent, then ignore my
ramblings and carry on.

[Peter Kasting]

On Tue, Sep 1, 2015 at 6:18 PM, Lei Zhang <the...@chromium.org> wrote:

Ok, if everyone feels the other way or are ambivalent, then ignore my
ramblings and carry on.

No, I feel strongly as you do.  The arguments for reuse focus on the convenience of the change author, and for that person reusing the change is certainly easier.  But as someone who's had to spelunk through various different reverted commits, git blame, etc., reused CLs are a nightmare to try and understand, especially if what landed the first time differs from what landed the second time.

I don't think we should be optimizing for change author convenience; I would prefer to optimize for the benefit of others trying to figure out what happened later.  So I think reusing CLs is a Bad Move.

PK 

[Ben Goodger (Google)]

commit-bot adds a comment to the CL saying "Patchset # landed as .." which lets you know what CL was landed.

The UI issues you raise are valid, and IMO could be best addressed with improvements to the Rietveld frontend.

-Ben