"No New Changes" getting in the way

[Jason Ganetsky]

I occasionally get into circumstances where a 'No New Changes' error arises. This has been discussed previously at: https://groups.google.com/forum/?fromgroups=#!topic/repo-discuss/NeuQmv4LdOc

I appreciate that Gerrit permits multiple changes per ChangeId, provided they are on distinct branches. This facilitates cherry-picking. The problem is, they must also have distinct commit hashes. In other words, I can't push the same exact commit to multiple branches for review. It's a strange use case, but it's the one that I'm stuck with right now. sop@, in the aforementioned thread, claims "No. Because Gerrit is looking only for commits it has never seen before." Is there a reason for this constraint? Can it be relaxed?

--

Jason Ganetsky | Software Engineer | gane...@google.com | US-NYC-9TH 5F117E

[Mihai Rusu]

It seems to require some big changes in Gerrit. Right now Gerrit only
considers the commits transferred over the network as negotiated by the
Git network protocol. That usually means objects that the server doesn't
yet know about (but the negotiation is simple and limited, upon
connection the server sends to the client a limited list of its refs and
SHA1s similar to what "git ls-remote" does and the client, if it
knows those SHA1s, can determine what to send and not send, of course it
easily can fail and send too much).

So, if your commit object is not sent over the network Gerrit doesn't
see it.

There is a related issue already open:
http://code.google.com/p/gerrit/issues/detail?id=465

Changing Gerrit to process all the history between the pushed commit and
the current commit on the destination branch is a large change in
Gerrit's behavior that may never happen. It would make it somewhat
more consistent (the number of changes created/updated by a push will
dpened strictly of the history between the pushed commit and the
destination branch and not on random things such as some other,
unrelated branch, pointing to some of the SHA1s being pushed affecting
the number of commits transferred over the network and thus the list of
commits processed by Gerrit).

It would also plug a security hole (IMO) in the Gerrit review model, it's
possible right now to easly "sneak in" a reviewed branch code changes
and commit history without code review. First push all that history to
another branch (in the same repository) without code review and then make
an "innocent" commit on top of it that would pass review and send it for
review to the branch requiring reviews. The reviewers will look at the Gerrit
diff of that single commit without realizing that the parent commit is not the
current tip of the branch requiring review but some other SHA1 and once this
is approved and submitted the whole history and code changes brought in by
that parent become part of the history of the reviewed branch.

However, there are likely valid use cases that would be broken by this
change in behavior.

--
Mihai Rusu

[Martin Fick]

On Tuesday, February 19, 2013 01:41:14 pm Mihai Rusu wrote:
> On Tue, Feb 19, 2013 at 06:26:26PM +0000, Jason Ganetsky
wrote:

> It would also plug a security hole (IMO) in the Gerrit
> review model, it's possible right now to easly "sneak
> in" a reviewed branch code changes and commit history
> without code review. First push all that history to
> another branch (in the same repository) without code
> review and then make an "innocent" commit on top of it
> that would pass review and send it for review to the
> branch requiring reviews. The reviewers will look at the
> Gerrit diff of that single commit without realizing that
> the parent commit is not the current tip of the branch
> requiring review but some other SHA1 and once this is
> approved and submitted the whole history and code
> changes brought in by that parent become part of the
> history of the reviewed branch.

I agree that it is a security hole. I believe that only
merge commits should be allowed to bring in history that is
not "in the current change". Our CI system has to add a
specific check to avoid allowing these types of changes in,
they have burned us badly before. We are working on getting
Gerrit to do this check.

> However, there are likely valid use cases that would be
> broken by this change in behavior.

Perhaps, but I would be willing to break them. Anyone else
have a problem with that?

-Martin

[Matthias Sohn]

2013/2/19 Martin Fick <mf...@codeaurora.org>

On Tuesday, February 19, 2013 01:41:14 pm Mihai Rusu wrote:
> On Tue, Feb 19, 2013 at 06:26:26PM +0000, Jason Ganetsky
wrote:

> It would also plug a security hole (IMO) in the Gerrit
> review model, it's possible right now to easly "sneak
> in" a reviewed branch code changes and commit history
> without code review. First push all that history to
> another branch (in the same repository) without code
> review and then make an "innocent" commit on top of it
> that would pass review and send it for review to the
> branch requiring reviews. The reviewers will look at the
> Gerrit diff of that single commit without realizing that
> the parent commit is not the current tip of the branch
> requiring review but some other SHA1 and once this is
> approved and submitted the whole history and code
> changes brought in by that parent become part of the
> history of the reviewed branch.

I agree that it is a security hole.  I believe that only
merge commits should be allowed to bring in history that is
not "in the current change".  Our CI system has to add a
specific check to avoid allowing these types of changes in,
they have burned us badly before.  We are working on getting
Gerrit to do this check.

I agree, this should be fixed

--

Matthias

[Jason Ganetsky]

It seems to me that creating a review and "git push" are conflated together. If I know the server already has all the relevant objects, I wouldn't mind using some other command, or even the Gerrit UI, to create a change.

[Jason Ganetsky]

Posted a feature request: https://code.google.com/p/gerrit/issues/detail?id=1815