ref-update hook called when review is pushed

[JT Olds]

Hey folks!

I am playing around with the synchronous ref-update hook. I was surprised by the combination of two behaviors that either one might have been unexpected.

First, ref-update is called when reviews are opened. Even though no branch or tag or usual git ref has been updated, ref-update is called.

Second, ref-update is called for reviews with "refs/heads/<branchname>" instead of "refs/for/<branchname>" or something to distinguish that it's a review.

Is there a way to either not have ref-update get called for reviews, or to be able to distinguish between review-creation-triggered ref-update calls and normal git-level ref updates?

Thanks!

[JT Olds]

For what it's worth, the --oldrev value isn't always "0" * 40 or something on review pushes and looks like it's set to what the branch actually was, so that doesn't help distinguish.

[JT Olds]

I don't know who would be most interested in this review, but here's a possible solution:

https://gerrit-review.googlesource.com/#/c/97420/

[JT Olds]

Alright, sorry for the spam, but this hook isn't even called when a review is merged. How hard would that be to change? 

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

---
You received this message because you are subscribed to a topic in the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/repo-discuss/ix72NtTcZsU/unsubscribe.
To unsubscribe from this group and all its topics, send an email to repo-discuss...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[JT Olds]

I'm being so annoying today. The 'ref-update' hook uses the CommitValidationListener, which is super useful sounding and good for validating 

Looks like there's a CommitValidationListener, a RefOperationValidationListener, 

[JT Olds]

I'm being so annoying today. The 'ref-update' hook uses the CommitValidationListener, which is super useful sounding and good for validating commits. It makes totally sense why you'd want to run that before a review is opened and when anyone pushes. Seems like there should be a 'commit-validate' hook.

There's also a RefOperationValidationListener. It sounds like what I would want to implement a new plugin to use if I wanted to implement the 'ref-update' hook how it sounds like it should work. Unfortunately, the RefOperationValidationListener only receives RefReceivedEvents, and I'm pretty sure the 'ref-update' hook should get RefUpdateEvents.

Before I run off and try and add new validation points for RefUpdateEvents or something crazy, am I off in the weeds? Is there a better way to have a script get to say yay or nay before a ref is updated, either via pushes or review merges?

[David Pursehouse]

On Thu, Feb 16, 2017 at 6:25 AM JT Olds <jto...@gmail.com> wrote:

Alright, sorry for the spam, but this hook isn't even called when a review is merged. How hard would that be to change? 

The ref-update hook is called synchronously when a commit is received.

If you want a hook to run after a change is merged, that's the change-merged and/or the ref-updated hook.  Both of those are run asynchronously.

 

You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.

[David Pursehouse]

On Thu, Feb 16, 2017 at 6:04 AM JT Olds <jto...@gmail.com> wrote:

I don't know who would be most interested in this review, but here's a possible solution:

https://gerrit-review.googlesource.com/#/c/97420/

Thanks.  I'll have a look into that later.  Can you confirm which version of Gerrit you've found this problem in?  And is it a regression from an earlier version?

Specifically I want to find out if it's caused by either:

- Refactoring of the events handling/dispatching

- Splitting the hooks implementation out to a plugin

both of which were done in 2.13.

 

On Wednesday, February 15, 2017 at 12:21:14 PM UTC-7, JT Olds wrote:

For what it's worth, the --oldrev value isn't always "0" * 40 or something on review pushes and looks like it's set to what the branch actually was, so that doesn't help distinguish.

On Wednesday, February 15, 2017 at 12:09:52 PM UTC-7, JT Olds wrote:

Hey folks!

I am playing around with the synchronous ref-update hook. I was surprised by the combination of two behaviors that either one might have been unexpected.

First, ref-update is called when reviews are opened. Even though no branch or tag or usual git ref has been updated, ref-update is called.

Second, ref-update is called for reviews with "refs/heads/<branchname>" instead of "refs/for/<branchname>" or something to distinguish that it's a review.

Is there a way to either not have ref-update get called for reviews, or to be able to distinguish between review-creation-triggered ref-update calls and normal git-level ref updates?

Thanks!

--

[JT Olds]

Thanks.  I'll have a look into that later.  Can you confirm which version of Gerrit you've found this problem in?  And is it a regression from an earlier version?

This is in tip from Git, and I really can't believe this is a regression from an earlier version, but I suppose it's not impossible. This seems like an architectural issue.

The main problem is that what I want is a synchronous script hook that allows me to approve or deny RefUpdateEvents. Unfortunately RefUpdateEvents don't have a ValidationListener (RefReceivedEvents do), so I can't easily write a plugin.

On top of that, the existing "ref-update" synchronous script should really be named "commit-validate", since it's using the CommitReceivedValidationListener, and does not run when refs are updated.

[JT Olds]

On Wednesday, February 15, 2017 at 5:09:56 PM UTC-7, JT Olds wrote:

Thanks.  I'll have a look into that later.  Can you confirm which version of Gerrit you've found this problem in?  And is it a regression from an earlier version?

This is in tip from Git, and I really can't believe this is a regression from an earlier version, but I suppose it's not impossible. This seems like an architectural issue.

Oh, I'm sorry, you were responding to the Gerrit review I pushed for determining if a pushed changeset was for review. You're right, I don't know when that broke but it does look like it was intended to work and then at some point got refactored to where it didn't. Tip only checks for refs/changes/ instead of refs/for/, but yeah, I didn't use this hook before so I don't know when it broke.