Gerrit and cherry-picking changes across branches

[Maria]

When an engineer makes a change on an older, more stable branch we
want them to cherry-pick the change to master and submit it through
Gerrit again.

However, if they cherry-pick and don't edit the commit message, Gerrit
interprets this commit as another patch set for the existing change.
Furthermore, if the original commit has already been submitted to the
branch, Gerrit won't let them push the cherry-picked commit, even
though it's on a different branch. (Error: ! [remote rejected] HEAD
-> refs/for/some_other_branch (no new changes) )

We don't have the resources for an engineer to spend most of their
time doing merge work and we definitely don't want everyone to have
Push Branch privileges.

Our best though so far is to require them to edit their commit message
and remove the original Change ID. This is non-ideal since we'd like
the process to be as automated and hassle-free as possible.

Has anyone else had to deal with this issue? Any suggestions?

Thanks in advance!

- Maria

--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

[Shawn Pearce]

Maria <maria.g...@gmail.com> wrote:
> When an engineer makes a change on an older, more stable branch we
> want them to cherry-pick the change to master and submit it through
> Gerrit again.

Why cherry-pick? Are there also changes in the stable branch that
you do *not* want in master?

The Android project merges the stable branch into the master branch,
rather than cherry-picking, so they don't run into this sort of
problem.

> However, if they cherry-pick and don't edit the commit message, Gerrit
> interprets this commit as another patch set for the existing change.

But yes, its, common problem.

> We don't have the resources for an engineer to spend most of their
> time doing merge work

I don't see how that is an issue. `git merge stable` is about as
fast to execute as `git cherry-pick MYCOMMIT`.

> and we definitely don't want everyone to have
> Push Branch privileges.

You can "code review" a merge commit just like any other commit.
Its file list will be empty if the merge was completely trivial.

> Our best though so far is to require them to edit their commit message
> and remove the original Change ID. This is non-ideal since we'd like
> the process to be as automated and hassle-free as possible.

I can't think of an easy way to automate that within git cherry-pick.
It fires the standard commit hooks, which can't really identify
that this is a cherry-pick vs. another type of commit.

You probably would need to write a script that does something like:

#!/bin/sh
c=$(git rev-parse --verify "$1")
git cherry-pick --no-commit $c
perl -pi -e 'next if /^Change-Id:/i' .git/MERGE_MSG
git commit -F .git/MERGE_MSG

and encourage your users to invoke that instead.

[Simon Wilkinson]

>
>> However, if they cherry-pick and don't edit the commit message, Gerrit
>> interprets this commit as another patch set for the existing change.
>
> But yes, its, common problem.

I hit this problem myself today. OpenAFS has a model where all new changes go to master first, and then once changes have baked on master for a sufficient amount of time, and are judge to be suitably low risk, they get pulled up to the stable branch.

In my case, I neglected to remove the ChangeIds for some pullups. The first time I submitted them, they went in fine, creating new changes (despite the fact that merged changes with those ids already existed on a different branch). However, when I revised them, and resubmitted, they created another set of new changes, despite still having the same ChangeId.

So, different symptoms, same problem. I wonder if it might be possible for a changeId/branch tuple to be considered the unique patch identifier, rather than just a changeId on its own. Doing this would have the added bonus that ChangeIds could then be used to unify cherry-picked patches across branches. (We currently use a combination of git patch-id and the cherry-picked from line for this task).

Cheers,

Simon.

[Shawn Pearce]

Simon Wilkinson <si...@sxw.org.uk> wrote:
> In my case, I neglected to remove the ChangeIds for some
> pullups. The first time I submitted them, they went in fine, creating
> new changes (despite the fact that merged changes with those ids
> already existed on a different branch). However, when I revised
> them, and resubmitted, they created another set of new changes,
> despite still having the same ChangeId.
>

> I wonder if it might be possible for a changeId/branch tuple
> to be considered the unique patch identifier, rather than just a
> changeId on its own.

Hmm. Didn't consider doing that. Its actually pretty logical.
And might not be that hard. Currently the change_id column is not
unique. Its just the matching code in ReceiveCommits if I recall.
So a minor change there to query on branch/changeId instead of
just changeId. Might be a fairly simple code change, and might
make a lot of folks happier.

> Doing this would have the added bonus that
> ChangeIds could then be used to unify cherry-picked patches across
> branches. (We currently use a combination of git patch-id and the
> cherry-picked from line for this task).

Yes.

[Ishaaq Chandy]

I have no more real input towards solving this problem, instead I just
wanted to chime in and observe that unlike the android team's way of
working there are other team's out there with differing release models
that would benefit from resolving this problem.

For e.g., at our work we typically have a branch per major release.
Once released each branch is supported for a period of time during
which we may issue (and tag) minor releases. However, all the main dev
work is on master. Sometimes we might have to patch one (or all) of
the release branches to solve some critical issue (typically a
non-functional, security patch). Careful cherry-picking is the only
viable approach (that I know of) to doing this - the branches should
not be contaminated with other unrelated changes made in the other
branches.

Ishaaq

[Nasser Grainawi]

I'll second Simon's suggestion. Sounds good to me.

[Jay Soffian]

On Thu, May 20, 2010 at 7:10 PM, Shawn Pearce <s...@google.com> wrote:
> Hmm.  Didn't consider doing that.  Its actually pretty logical.
> And might not be that hard.  Currently the change_id column is not
> unique.  Its just the matching code in ReceiveCommits if I recall.
> So a minor change there to query on branch/changeId instead of
> just changeId.  Might be a fairly simple code change, and might
> make a lot of folks happier.

Well, there's a UI component also - when a search on
#q,<change_id>,n,z returns multiple changes, you'd want to present all
of them in a table view I'd think.

j.

[Shawn Pearce]

Jay Soffian <jayso...@gmail.com> wrote:
> On Thu, May 20, 2010 at 7:10 PM, Shawn Pearce <s...@google.com> wrote:
> > Hmm.  Didn't consider doing that.  Its actually pretty logical.
> > And might not be that hard.  Currently the change_id column is not
> > unique.  Its just the matching code in ReceiveCommits if I recall.
> > So a minor change there to query on branch/changeId instead of
> > just changeId.  Might be a fairly simple code change, and might
> > make a lot of folks happier.
>
> Well, there's a UI component also - when a search on
> #q,<change_id>,n,z returns multiple changes, you'd want to present all
> of them in a table view I'd think.

That's exactly what it does now. That's why /r/I629f5439 does a
redirect to #q,I629f5439,n,z... which then does another redirect
to #change,14559 as there is exactly one matching record.

If there were multiple changes with the same Change-Id, the
#q,<id>,n,z page would instead show you a table with the changes
listed on it. Its the same table layout as All > Open. You just
don't see this come up because typically the Change-Id is unique.

[Jay Soffian]

On Mon, May 24, 2010 at 8:13 AM, Shawn Pearce <s...@google.com> wrote:
> That's exactly what it does now.  That's why /r/I629f5439 does a
> redirect to #q,I629f5439,n,z...

Was that added after 2.1.2.2? I get a 404 from Jetty on my install.
/r/<change_num> works but /r/<change_id> does not.

j

[Shawn Pearce]

On Mon, May 24, 2010 at 16:58, Jay Soffian <jayso...@gmail.com> wrote:
> On Mon, May 24, 2010 at 8:13 AM, Shawn Pearce <s...@google.com> wrote:
>> That's exactly what it does now.  That's why /r/I629f5439 does a
>> redirect to #q,I629f5439,n,z...
>
> Was that added after 2.1.2.2? I get a 404 from Jetty on my install.
> /r/<change_num> works but /r/<change_id> does not.

Uh, it was commit c23529ddf9b68e07c2a0f44262043321b7defe5e which is
part of 2.1.2 according to git. So it should be working in 2.1.2.2.
Are you sure you had the leading capital I in the Change-Id? IIRC the
regex used in the URL is /r/I and then at least some number of hex
digits.

[Jay Soffian]

On Mon, May 24, 2010 at 10:05 PM, Shawn Pearce <s...@google.com> wrote:
>> Was that added after 2.1.2.2? I get a 404 from Jetty on my install.
>> /r/<change_num> works but /r/<change_id> does not.
>
> Uh, it was commit c23529ddf9b68e07c2a0f44262043321b7defe5e which is
> part of 2.1.2 according to git.  So it should be working in 2.1.2.2.
> Are you sure you had the leading capital I in the Change-Id?  IIRC the
> regex used in the URL is /r/I and then at least some number of hex
> digits.

Definitely not working for me with 2.1.2.2:

"GET /r/I81d0d438 HTTP/1.1" 404 258

<body><h2>HTTP ERROR 404</h2>
<p>Problem accessing /r/I81d0d438. Reason:
<pre> Not Found</pre></p><hr /><i><small>Powered by
Jetty://</small></i><br/>

/r/#q,I81d0d438,n,z works fine.

But, now knowing it's supposed to work, I'll figure it out.

j.

[Derrick Brashear]

On May 20, 6:49 pm, Simon Wilkinson <si...@sxw.org.uk> wrote:
> >> However, if they cherry-pick and don't edit the commit message, Gerrit
> >> interprets this commit as another patch set for the existing change.
>
> > But yes, its, common problem.
>
> I hit this problem myself today. OpenAFS has a model where all new changes go to master first, and then once changes have baked on master for a sufficient amount of time, and are judge to be suitably low risk, they get pulled up to the stable branch.
>
> In my case, I neglected to remove the ChangeIds for some pullups. The first time I submitted them, they went in fine, creating new changes (despite the fact that merged changes with those ids already existed on a different branch). However, when I revised them, and resubmitted, they created another set of new changes, despite still having the sameChangeId.
>

> So, different symptoms, same problem. I wonder if it might be possible for achangeId/branch tuple to be considered the unique patch identifier, rather than just achangeIdon its own. Doing this would have the added bonus that ChangeIds could then be used to unify cherry-picked patches across branches. (We currently use a combination of git patch-id and the cherry-picked from line for this task).

The problem with this approach is that if this change is submitted to
another gerrit, the change id stays the same; the branch of course
does not. which change did i submit from the first tree?

I bring this up because I'm curious what the behavior should be; right
now, it seems that i can't submit a change with the same change id to
another branch (2.1.5); I get an error telling me no changes have been
made. If I nuke the change id, I can submit... but now I have a change
with no change id.

What *should* happen?