The right way to cherry-pick?

[Hung-Te Lin]

Hi CrOS-dev,

  Currently there are several ways to cherry-pick a CL into other branches:

    1. Click the 'cherry-pick' button on Gerrit web 

    2. In command line, do "git cherry-pick".

    3. In command line, do "repo cherry-pick".

  So I have a question: what's the "recommended approach"?

  The story behind that is I want to know how to deal with the Change-Id when cherry-picking.

  Gerrit web won't change Change-Id, but it'll add a "(cherry pick from commit $hash).

  git cherry-pick won't change Change-Id, nor adding messages.

  repo cherr-pick will replace the Change-Id by a "(cherry pick from commit $hash)" message. Also on the wiki page it said"make sure to delete the old ChangeID line so that a new one will automatically be generated for you."

  From what I understand, Change-Id has a special meaning that Gerrit use it to track/track "a change set". Previously we have some guideline with something like "when cherry-picking, change Reviewed-* to Original-Reviewed-* and remove ChangeId to get new one"; seems like that's removed from wiki pages so I think we only care about Change-Id now.

  I've seen bugs like "if we cherry-pick to same branch multiple times using same Change Id, merge one and abandon others, there's a chance Gerrit may reject further new changes based on change since it'll think the parent is abandoned" so I think ChangeId should be unique for cherry-pick CLs;

  However, people using Gerrit Web are usually not aware of the Change-Id stuff and simply use same Change-Id; and in fact that also helps them to track "what branches have I cherry-picked this change to".

  Example:

   https://chromium-review.googlesource.com/#/c/338092/   # this is a change on master/ToT.

   https://chromium-review.googlesource.com/#/c/400980/   # a cherry-pick change I've made by clicking the gerrit web button and keep original change id. You can find a "Cherry-Picks (2)" list on Gerrit interface.

   https://chromium-review.googlesource.com/#/c/400979/   # a cherry-pick change I've made by clicking the gerrit web button but deleted the original change id, to have a new id. You won't see Cherry-Picks list on Gerrit interface. You also can't find a link to this on the original change.

  So that makes me confused: people doing command line (repo cherry-pick) seem to use different change-id by default, but people using we interface are encouraged to use same change ID.

  Is there some new policy / guideline for what we should do?

[Mike Frysinger]

we provided cros_merge_to_branch to people to make it easy to DTRT.  the things it does are our recommendations.

  https://www.chromium.org/chromium-os/how-tos-and-troubleshooting/working-on-a-branch#TOC-cros_merge_to_branch

the other methods are technically possible and people are free to do what they're comfortable with.

when i use the gerrit cherry-pick button, i trim the tags like you referenced.  not sure it's possible to customize gerrit to do this automatically.

-mike

--
--
Chromium OS Developers mailing list: chromiu...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-os-dev?hl=en

[Daniel Kurtz]

Usually I use cros_merge_to_branch, one of the ways recommended in the working on a branch doc.

Here is an example cros_merge_to_branch CL:

https://chromium-review.googlesource.com/#/c/400122/

this generates a commit message with the same Change-Id, and a link to the original gerrit review, like this:

Change-Id: Iaf6f7b06e7355826a7540f823d89e5a0f9af82f8

Previous-Reviewed-on: https://chromium-review.googlesource.com/392789

(cherry picked from commit b9e382afa7e410745ac96b12b49d5a941070db1e)

Reviewed-on: https://chromium-review.googlesource.com/400122

Reviewed-by: Daniel Kurtz <djk...@chromium.org>

Commit-Queue: Daniel Kurtz <djk...@chromium.org>

Tested-by: Daniel Kurtz <djk...@chromium.org>

Trybot-Ready: Daniel Kurtz <djk...@chromium.org>

--

[Hung-Te Lin]

Oh right - I missed the cros_merge_to_branch.

 cros_merge_to_branch works like Gerrit button (keeping Change-Id), but it also changed Reviewed-On to Previous-Reviewed-On, with extra message (cherry picked from ...)

But Mike, do you keep Change-Id unchanged when doing cros_merge_to_branch ?

[Brian Norris]

I've found cros_merge_to_branch to be *extremely* slow. Is it doing an entire clone? That can be expensive with a kernel repo, but it feels like it's even slower than I'd expect from that. Maybe it's doing something even more.

[Bernie Thompson]

Forwarding from the proper email address...

On Wed, Oct 19, 2016 at 9:37 PM, Bernie Thompson <bhtho...@google.com> wrote:

If the patch is the same, I personally prefer when we keep the Change-Id, assuming the cherry-pick is really intended to be the same patch. When patches share a Change-Id it is much easier to track them across Gerrit, e.g. you can search for a particular Change-Id to find all the branches something has been merged to.

-Bernie

[Mike Frysinger]

yes, it does a full clone, but it does so re-using your local git repo.  you can run it with --debug to see what it does exactly.

00:46:53: INFO: RunCommand: git clone https://chromium-review.googlesource.com/chromiumos/chromite /tmp/cros_merge_to_branchhvr6BR/chromiumos/chromite --reference /usr/local/src/chromiumos/chromite in /tmp/cros_merge_to_branchhvr6BR/chromiumos/chromite

we might be able to add some logic to try and re-use your local checkout (assuming it's clean).  i don't think you can cherry pick w/out the tree actually being checked out to the branch you're targetting.

-mike

[Mike Frysinger]

yes, the Change-Id is preserved by design.  Change-Id's are unique to branches.  the gerrit flow makes it easy to keep track of a unique change even as you cherry pick it across diff branches (which would generate different git sha1's).  for example:

  https://chromium-review.googlesource.com/#/q/I27255aed54d14db7753617fa4939cec135e9d325

-mike

[Hung-Te Lin]

Thanks. Sounds like the wiki page should be changed

https://www.chromium.org/chromium-os/how-tos-and-troubleshooting/working-on-a-branch#TOC-Make-your-changes

# When the editor pops up, make sure to delete the old ChangeID line
# so that a new one will automatically be generated for you.
git cherry-pick -x -e <SHA1>

Should we remove the comment of deleting old ChangeID?

[Bernie Thompson]

I think so.

IIRC there was a point in time when having the same Change-Id was bad for Gerrit which may have driven that step of removing the Change-Id, but this was years ago.

-Bernie

[Doug Anderson]

Hi,

On Thu, Oct 20, 2016 at 8:41 AM, Bernie Thompson <bhtho...@chromium.org> wrote:

I think so.

IIRC there was a point in time when having the same Change-Id was bad for Gerrit which may have driven that step of removing the Change-Id, but this was years ago.

-Bernie

On Wed, Oct 19, 2016 at 10:48 PM, Hung-Te Lin <hun...@chromium.org> wrote:

Thanks. Sounds like the wiki page should be changed

https://www.chromium.org/chromium-os/how-tos-and-troubleshooting/working-on-a-branch#TOC-Make-your-changes

# When the editor pops up, make sure to delete the old ChangeID line
# so that a new one will automatically be generated for you.
git cherry-pick -x -e <SHA1>

Should we remove the comment of deleting old ChangeID?

Yes please.  Really gerrit wouldn't allow the same change ID across branches.  New gerrit does and it's actually useful.

[Doug Anderson]

Hi,

On Wed, Oct 19, 2016 at 8:08 PM, Daniel Kurtz <djk...@chromium.org> wrote:

Usually I use cros_merge_to_branch, one of the ways recommended in the working on a branch doc.

Here is an example cros_merge_to_branch CL:

https://chromium-review.googlesource.com/#/c/400122/

this generates a commit message with the same Change-Id, and a link to the original gerrit review, like this:

Change-Id: Iaf6f7b06e7355826a7540f823d89e5a0f9af82f8

Previous-Reviewed-on: https://chromium-review.googlesource.com/392789

(cherry picked from commit b9e382afa7e410745ac96b12b49d5a941070db1e)

Reviewed-on: https://chromium-review.googlesource.com/400122

Reviewed-by: Daniel Kurtz <djk...@chromium.org>

Commit-Queue: Daniel Kurtz <djk...@chromium.org>

Tested-by: Daniel Kurtz <djk...@chromium.org>

Trybot-Ready: Daniel Kurtz <djk...@chromium.org>

I'm not sure it's worth stressing over, but I've also noticed that gerrit (and possibly cros_merge_to_branch) violates our own upload hooks which doesn't like the (cherry picked from commit zzzzz) being stuffed in with the tags.  I've always just ignored that upload hook, but it's been in the back of my mind to figure out exactly why it was added...

[Dmitry Torokhov]

That was bugging me as well. https://chromium-review.googlesource.com/401102

[Mike Frysinger]

On Thu, Oct 20, 2016 at 8:49 AM, Doug Anderson <dian...@chromium.org> wrote:

you're misreading gerrit.  that is *not* how cros_merge_to_branch uploaded things.  that's what gerrit rewrote the commit message to be as it cherry-picked the change in.

you need to look at PS1 to see the original message:

https://chromium-review.googlesource.com/#/c/400122/1

-mike

[Dmitry Torokhov]

Here is an example of cherry-pick done via Gerrit UI (https://chromium-review.googlesource.com/#/c/393791/):

CHROMIUM: Input: atmel_mxt_ts - put into deep sleep at shutdown on Celes

Celes does not cut power to the touch controller when system is shut

down, causing issues when we try to reinitialize the controller when

system is booted up again.

To work around this issue we put the touch controller into deep sleep at

shutdown. Ideally firmware would do that for as (because kernel's

expectations is that peripherals are powered off when we shut the system

down, but at this time kernel workaround is easiest).

BUG=chrome-os-partner:57704

TEST=build kernel.

     check if atmel ic goes to deep sleep mode at power off.

     Power On, check if touch screen/touchpad work at power on.

Change-Id: I45de8f981482d560860d448b2c7f440fad477e19

Signed-off-by: Jongpil Jung <jongpil...@samsung.com>

Reviewed-on: https://chromium-review.googlesource.com/390211

Reviewed-by: Dmitry Torokhov <dt...@chromium.org>

(cherry picked from commit 93f67859ca5937896c9edcec2306515674903869)

As you can see it does not move/replace Change-ID, but simply adds "(cherry picked from commit " at the bottom. Unfortunately if I were to check out this CL (because maybe I wanted to add some more CLs on top), I would not be able to re-upload without --no-verify.

I really want to make sure that our per-upload hooks are compatible with the rest of infrastructure. For that we need to allow at least the following tags to be present after Change-Id:

Signed-off-by:

Reviewed-on:

Reviewed-by:

Tested-by:

Commit-Ready:

(cherry picked from commit 

Thanks,

Dmitry