Gerrit weirdness when pushing a new branch

[Maria]

When I push directly to a branch, does Gerrit generate Change-Ids for
commits which don't have them, and then close out that change, even if
it already existed?

I'm seeing some very strange Gerrit behavior when I push a new branch
directly to refs/heads. Gerrit will decide that some of the commits
pushed correspond to Gerrit changes that they have nothing to do with.

I see a couple of things: some open changes will be closed out as
merged into the new branch and the notification emails about this will
have a commit message summary that has nothing to do with the Gerrit
change what was closed. Similarly, changes that are already merged or
abandoned show up as merged into the new branch, and the notification
email contains a commit message that has nothing to do with the Gerrit
change on our server that was supposedly merged.

I initially thought that this was happening because the pushed commits
has Change-Ids which collided with open changes on our server, but
I've done more investigating, and that's not true.

We've pushed a few new branches lately, and the same pushed commit
will reliably cause a comment on the same Gerrit change, so I do
suspect Gerrit is generating a Change-Id, even though we already have
an unrelated change using that Change-Id.

This is a real bummer for us every time we push a new branch. Does
anyone have any suggested workarounds?

Thanks!

[Mike Harris]

We have similar issues. We have 2 git/gerrit servers and it happens
on both. We're on 2.1.5.
Sometimes we get open change reviews being closed as merged in the new
branch but usually it's an abandoned change review that gets marked as
merged in the new branch.
Both cases are bogus though because the change isn't actually merged
in the new branch.
The email that gets sent out is not related to the change at all.

Mike

[Dmitry Fink]

I think you mentioned that problem during GitTogether dinner, is it
the same one? The one that made you think you have collisions
in the Change-Id space?

I have same issue on my repositories too, every time I branch and push
a new head to my repo,
I get emails about some old abandoned commits being pushed (I've never
saw the case of real open changes being closed as a result).
I happens with same abandoned commits every time though (I have one of
these in my one repo and 3 in another).
I've grepped the whole history of the branches I pushed and these
Change-Id's do not appear in any of the commit messages in the
repository,
so it must be something else. Change-Id collisions theory sounds very
unlikely, I have 4 of these already, sounds like you have
even more of these cases.

Good thing is it is very reproducible so it may be easy to catch and
root cause, I'll let you know if I find anything.

Dmitry

On Dec 14 2010, 4:32 pm, Maria <maria.gutow...@gmail.com> wrote:

[Maria]

Yep, this is exactly what I mentioned at the GitTogether. I hadn't
looked closely enough at the time; I'm now pretty sure we were never
having Change-Id collisions, but something else going on.

It sounds like both of you are having the same thing we are.

Anyone have any suggestions for how we can avoid this?

> > Thanks!- Hide quoted text -
>
> - Show quoted text -

[Dmitry Fink]

Some update:
I've analyzed all my commits that had a problem, although I did find a
bug related to abandoned changes that explains what I am seeing, I
don't think its the one you are talking about:

To answer your question first, if I understand the code correctly when
you push to refs/heads two things happen in postReceive that may
trigger autoclose SHA1 check and Change-ID check:
1. SHA1: For each commit you are pushing in this branch, gerrit will
take it's SHA1 and compare it to all refs/changes/* in that repo sets
to see if any of the patchsets there corresponds to this SHA1. If
there is a match it will try to autoclose the corresponding patchset.
2. Change-Id: If the commit you are pushing has a Change-Id in the
body of the commit message, gerrit will try to match it to any of the
open changes in the main gerrit database and autoclose the change if
it finds a match.

Steps to reproduce the problem:
1. Create commit locally
2. Push it for review to refs/for
3. Abandon the commit in gerrit
4. Push same HEAD to refs/heads/test_branch

Gerrit is detecting that commit corresponds to a change, tries to
close that change (adds a comment, send emails) however fails to set
the status to MERGED, my guess is the reason for failing to update is
some last minute check that verifies that the change is "open", and
ABANDONED is no "open", so update to MERGED fails.

I am guessing that exactly is what happened in my repos, someone
pushed commits for review, then abandonded them, then later these
commits got into repo by direct push after all. The fix is really
simple: restore commits, then push some fake branch, this will really
autoclose them this time (their status will change to MERGED and they
won't bother anyone). I won't be lying either, these commits are on
the tree after all, so it makes sense to have their status as "merged"
instead of "abandoned" in the database.

So all that makes me think that the problem you are seeing (auto
closing of unrelated OPEN changes) may be a different one after all.

Dmitry

[Erik LaBianca]

I sent a note to this effect a week or so ago, but apparently my google-groups fu is lacking. Trying again.

On Jan 11, 2011, at 4:14 AM, Dmitry Fink wrote:
>
> Steps to reproduce the problem:
> 1. Create commit locally
> 2. Push it for review to refs/for
> 3. Abandon the commit in gerrit
> 4. Push same HEAD to refs/heads/test_branch
>
> Gerrit is detecting that commit corresponds to a change, tries to
> close that change (adds a comment, send emails) however fails to set
> the status to MERGED, my guess is the reason for failing to update is
> some last minute check that verifies that the change is "open", and
> ABANDONED is no "open", so update to MERGED fails.
>

I've observed the following in Gerrit 2.1.6.1, I think it's the same root issue. We use shared feature branches for development occasionally, and rather than maintaining a second git "hub", I've set gerrit to allow developers to push to refs/heads/feature/*. We use a branch called "develop" instead of "master", and the gerrit host is set up as "origin".

Steps to reproduce:
1. git checkout develop
2. git checkout -b feature/bogus-feature
3. echo "bogus change" > test.txt && git add test.txt && git commit -m 'added bogus.txt , ignore this'
5. git log

commit c0a9b29d89faf1bea0791a6dff60618f9d326dc2
Author: Erik S. LaBianca <erik.l...@gmail.com>
Date: Tue Jan 11 09:48:24 2011 -0500

adding a bogus change, ignore it

Change-Id: Idfd820808ddb47c91e5c516dcc7f4dc4aa641028

4. git push origin HEAD:refs/for/develop
5. observe review is created successfully in gerrit
6. git push origin feature/bogus-feature
7. observe gerrit flipped the submitted changeset to "merged" and added a comment:

Change has been successfully pushed into branch feature/bogus-feature.

So, at least in my case, I think the bug here is pretty straightforward. Wherever in the code watches for inbound changes to refs/heads*, it is not sufficiently discriminating in matching branches and instead sees a forced push to a branch with the same sha of the changeset and assumes it must be a manual merge.

Hope this helps.

Thanks

--erik

[Luciano Carvalho]

In fact, it's not doing this because of the similar SHA1, but because the commit message has a Change-Id that matches an existing Change-Id in Gerrit.

If you do a 'git commit --amend' before you push it to the feature branch and remove the Change-Id line from the commit message, it won't transition your change to Merged, even though it is the same SHA1 you're pushing.

We use this procedure here, it works well.

Luciano.

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

[Erik LaBianca]

On Jan 11, 2011, at 10:58 AM, Luciano Carvalho wrote:

In fact, it's not doing this because of the similar SHA1, but because the commit message has a Change-Id that matches an existing Change-Id in Gerrit.

If you do a 'git commit --amend' before you push it to the feature branch and remove the Change-Id line from the commit message, it won't transition your change to Merged, even though it is the same SHA1 you're pushing.

We use this procedure here, it works well.

Ah ok, that's good to know. I'd still advocate that it's a bug, however, as the changeset was only submitted for develop... Or not?

I'm poking around in the source code now, maybe I'll be able to find the offending lines.

--erik

[Dmitry Fink]

Hi, Erik
Yeah, that one is a well known bug, gerrit ignores target branch information when it tries to uniquely identify a commit in the system, all it cares about is the Change-Id. Usually people complain about another manifestation of the same problem: when they cherry-pick some commit that was reviewed a different branch and try to push it for review again, gerrit will reject it since it thinks we are trying to push a change that already exists in the system.

[Dmitry Fink]

Luciano, I don't think your statement is correct, if I understand the code correctly it does look both for SHA1 matches and for Change-Id matches.

When you do git commit --amend though and modify a commit message you SHA1 is going to change and as well and as a result the new commit

won't have any relation to the review item in gerrit.

Dmitry

[Erik LaBianca]

Yep, thats it. In my case, the commit is truly identical (sha1 and changeid both), just in the wrong place. Is there a ticket open for it somewhere?

I can't promise anything, but since it bugs me and I'd like to help out I'd like to make a pass at resolving it.

--erik

[Maria]

What I'm seeing happen sounds most like what Dmitry is describing, but
it's a bit different.

When I push the kernel to a new branch or some of the Android repos to
a new branch (even if these changes already existed on our server on
another branch), gerrit will sometimes do two things - A) comment on
an abandoned change in another repository that the change was merged
or B) close out a change in another repository as merged. The changes
pushed don't have to have a Change-Id for this to happen (unlike what
I intially thought).

It seems like gerrit is checking for the SHA1 of the pushed commits
across all repostories, instead of just the project they were pushed
to. Is there any way it would do this?

On Jan 11, 9:35 am, Dmitry Fink <fin...@gmail.com> wrote:
> Hi, Erik
> Yeah, that one is a well known bug, gerrit ignores target branch information when it tries to uniquely identify a commit in the system, all it cares about is the Change-Id. Usually people complain about another manifestation of the same problem: when they cherry-pick some commit that was reviewed a different branch and try to push it for review again, gerrit will reject it since it thinks we are trying to push a change that already exists in the system.
>
> On Jan 11, 2011, at 6:55 AM, Erik LaBianca wrote:
>
>
>
> > I sent a note to this effect a week or so ago, but apparently my google-groups fu is lacking. Trying again.
>
> > On Jan 11, 2011, at 4:14 AM, Dmitry Fink wrote:
>
> >> Steps to reproduce the problem:
> >> 1. Create commit locally
> >> 2. Push it for review to refs/for
> >> 3. Abandon the commit in gerrit
> >> 4. Push same HEAD to refs/heads/test_branch
>
> >> Gerrit is detecting that commit corresponds to a change, tries to
> >> close that change (adds a comment, send emails) however fails to set
> >> the status to MERGED, my guess is the reason for failing to update is
> >> some last minute check that verifies that the change is "open", and
> >> ABANDONED is no "open", so update to MERGED fails.
>
> > I've observed the following in Gerrit 2.1.6.1, I think it's the same root issue. We use shared feature branches for development occasionally, and rather than maintaining a second git "hub", I've set gerrit to allow developers to push to refs/heads/feature/*. We use a branch called "develop" instead of "master", and the gerrit host is set up as "origin".
>
> > Steps to reproduce:
> >    1. git checkout develop
> >    2. git checkout -b feature/bogus-feature
> >    3. echo "bogus change" > test.txt && git add test.txt && git commit -m 'added bogus.txt , ignore this'
> >    5. git log
>
> > commit c0a9b29d89faf1bea0791a6dff60618f9d326dc2

> > Author: Erik S. LaBianca <erik.labia...@gmail.com>

> > Date:   Tue Jan 11 09:48:24 2011 -0500
>
> >    adding a bogus change, ignore it
>
> >    Change-Id: Idfd820808ddb47c91e5c516dcc7f4dc4aa641028
>
> >    4. git push origin HEAD:refs/for/develop
> >    5. observe review is created successfully in gerrit
> >    6. git push origin feature/bogus-feature
> >    7. observe gerrit flipped the submitted changeset to "merged" and added a comment:
>
> > Change has been successfully pushed into branch feature/bogus-feature.
>
> > So, at least in my case, I think the bug here is pretty straightforward. Wherever in the code watches for inbound changes to refs/heads*, it is not sufficiently discriminating in matching branches and instead sees a forced push to a branch with the same sha of the changeset and assumes it must be a manual merge.
>
> > Hope this helps.
>
> > Thanks
>

> > --erik- Hide quoted text -

[Luciano Carvalho]

Dmitry,
I don't think it matches it at all, because when people cherry-pick the SHA1 changes, and the only matching thing is the change-id line, the change is going to move to ready.

Another test you can do is to disable the commit-message hook and create a commit, push it to refs/for, (make sure it doesn't have the commit-id line), then push it directly to a different branch head. It won't move the change to Merged.

Regards,

Luciano

[Luciano Carvalho]

I meant: "move to Merged" in the first paragraph.

>>> > To unsubscribe, email repo-discuss...@googlegroups.com<repo-discuss%2Bunsu...@googlegroups.com>

[Dmitry Fink]

Another test you can do is to disable the commit-message hook and create a commit, push it to refs/for, (make sure it doesn't have the commit-id line), then push it directly to a different branch head. It won't move the change to Merged.

Yes it will. I tested it again to make sure.

And here is the code that does it I think:

  private void autoCloseChanges(final ReceiveCommand cmd) {

    final RevWalk rw = rp.getRevWalk();

    try {

      rw.reset();

      rw.markStart(rw.parseCommit(cmd.getNewId()));

      if (!ObjectId.zeroId().equals(cmd.getOldId())) {

        rw.markUninteresting(rw.parseCommit(cmd.getOldId()));

      }

      final Map<ObjectId, Ref> byCommit = changeRefsById();   <--------- this is the hash of all refs/changes in repo and their SHA1 numbers

      final Map<Change.Key, Change.Id> byKey = openChangesByKey(); <--------------- this is a list of all open changed by Change-Id

      final List<ReplaceRequest> toClose = new ArrayList<ReplaceRequest>();

      RevCommit c;

      while ((c = rw.next()) != null) {

        final Ref ref = byCommit.get(c.copy());  <---- match by SHA1

        if (ref != null) {

          rw.parseBody(c);

          closeChange(cmd, PatchSet.Id.fromRef(ref.getName()), c);

          continue;

        }

        rw.parseBody(c);

        for (final String changeId : c.getFooterLines(CHANGE_ID)) {

          final Change.Id onto = byKey.get(new Change.Key(changeId.trim()));  <---- match by Change-Id

          if (onto != null) {

            toClose.add(new ReplaceRequest(onto, c, cmd));

            break;

          }

        }

      }

      for (final ReplaceRequest req : toClose) {

        final PatchSet.Id psi = doReplace(req);

        if (psi != null) {

          closeChange(req.cmd, psi, req.newCommit);

        }

      }

    } catch (IOException e) {

      log.error("Can't scan for changes to close", e);

    } catch (OrmException e) {

      log.error("Can't scan for changes to close", e);

    }

  }

[Luciano Carvalho]

OK. Yeah, I agree it needs improvement.

It has to match the destination branch before it closes an open change.

But I also think it should continue to at least add a comment to the change when it's pushed to another branch.

Thanks!

Luciano

[Mike Harris]

I've found a workaround to our problem. The bogus change messagess/
emails were specifically in our u-boot and kernel gits. However, the
messages/emails were only generated when we pushed a new branch in the
platform/hardware/<vendor>/<board1> and platform/hardware/<vendor>/
<board2> gits. I determined this by creating a unique branch for
every project and checked the changes that had the change messages to
determine which branch caused the change message.

I wrote a script to be used in a repo forall -c command that checks
the remote and if it's not one of the 2 platform/hardware gits, it
pushes the branch, otherwise it skips pushing the branch. Then I use
the gerrit web interface to create the branches for the 2 platform/
hardware gits, which doesn't create the change messages/emails.

Hope this helps.

On Dec 14 2010, 4:32 pm, Maria <maria.gutow...@gmail.com> wrote: