Git Hook - Feedback to Pusher

[John Crygier]

Hi All-

I'm working on implementing some of our hooks that we will need using the groovy hooks, and running into some issues.  In my groovy pre-receive hook, I have simply the following lines:

commands.each { ReceiveCommand command ->

    command.setResult(Result.REJECTED_OTHER_REASON, "You are not permitted to write to ${repository.name}");

}

return false;

The push gets rejected as expected, but the user doesn't see "You are not permitted to write to myRepo" as I was expecting.  Has anyone seen this, and know a workaround?

Thanks

--John

[James Moger]

Hi John,

I was hoping that a JGit developer would chime in - there are a few
that patrol this group - but I know several were involved with
EclipseCon so they may be busy or traveling.

I do not know why the Git client does not receive your message. I
have confirmed your experience with command-line git on my system.
When you are setting that message, you are setting it directly into a
JGit ReceiveCommand and the SmartHTTP GitServlet _should_ relay that
to the client. I know I've read that not all clients support return
messages, but I think that may be old information and that all current
clients do support return messages.

If we don't get a response here by Monday I'll bring it up on the JGit
developer list. Maybe I need to make an adjustment to Gitblit to
ensure the message is returned to the client.

-J

[John Crygier]

Hi-

Thanks for the feedback, I really just wanted to ensure I was not going insane and making a stupid mistake.  I actually ended up walking through a lot of the JGit code, but stopped after seeing ReceivePack.sendStatusReport is in fact writing the string "ng refs/heads/master You are not permitted..." to pckOut.  I figured if it looked good that far in the chain, something else was wrong.

Maybe I'll do some more research today if I get some free time.

Thanks again for the verification I'm not crazy!

--John

[John Crygier]

Okay, I'm close to figuring this out.  I went ahead and threw up a temporary apache server that has the git-http-backend on it so I could use git "out-of-the-box" smart http.  At the same time, I threw an HTTP proxy in the middle so I could sniff what was going back and forth when there are messages printed in shell pre-receive script.

Turns out that it's still sending something like "0033ng refs/heads/master pre-receive hook declined" but that looks like it's just info to the git client, and it NEVER prints to the end user either.  However, it's printing a whole bunch more stuff well before the actual rejection.  So it looks like that's the proper approach.

Looking through JGit ReceivePack, there is a method "sendMessage" that I think is intended to be used by the hooks.  So now it's just a trick of getting access to the ReceivePack instance, and I think I'm good.

--John

[James Moger]

Hmm. You could be correct, but I think we'll need clarification from
the JGit team.

The following JavaDoc snippet is from the ReceivePack.sendError(String what)

* {@link PreReceiveHook}s should always try to use
* {@link ReceiveCommand#setResult(Result, String)} with a result status of
* {@link Result#REJECTED_OTHER_REASON} to indicate any reasons for
* rejecting an update. Messages attached to a command are much more likely
* to be returned to the client.

[James Moger]

Hi John,

I've gotten a reply from the JGit team. This may be a bug in the git
client you are using. I'm running older clients on my machines than
the version Dave references. I have not verified/re-tried this with a
newer client. What version are you using?

-J

On Fri, Mar 30, 2012 at 09:37, <james...@gitblit.com> wrote:

Hi JGit Team,

PreReceive rejection.

receiveCommand.setResult(Result.REJECTED_OTHER_REASON, "this git server
does not like you")

I expected that the reason would be displayed client-side. This does
not appear to be the case. The JavaDoc for sendError suggests that
setResult(result, reason) is the appropriate technique for reporting a
rejection error message to the client.

Yes, that's correct.

Is Gitblit doing something
incorrect here? Should the Gitblit receive hook call sendError manually
if a prereceive command result is one of the REJECTED enum values?

You may choose to call sendError independently, but those error
messages are displayed before the per-branch error messages.

Until recently there was a bug where the C git client would fail to
display per-ref status messages under some circumstances. What git
client version are you using? The bug I'm thinking of was fixed in
5238cbf65638d7a097bdb5ca8226f5acbe31f143, included in v1.7.9.2.

[John Crygier]

Okay, I've verified that ReceivePack.sendMessage does in fact get the message back to the client as expected.  So if you wanted to enhance the Gitblit side, I see two options:

1. Pass ReceivePack in as a groovy binding.  I would actually wrap it in a "ClientLogger" class or something so that not all of the functionality is exposed to the hooks.  This doesn't seem too difficult, as you already have the ReceivePack in GitServlet.onPreReceive.
   
2. When looping through the ReceiveCommand objects after "runGroovy" in the onPreReceive, you can call "rp.sendMessage(cmd.getMessage());".  This would only be necessary if the JGit team intentionally does not want this message to be displayed on the client.

I think there would still be use in #1, even if JGit does change code to do this automatically.  This would allow for hooks to send info or warning messages instead of just error messages.

FYI, I'm using git version 1.7.6.msysgit.0 on the client.

Also, if you want me to go ahead and implement #1, I can do that with no problem.

--John

[John Crygier]

Yep, I upgraded to 1.7.9-preview20120201 and sure enough I'm getting ![remote rejected] <message> at the end of my push now, so it looks like that was a bug on the client side.

However, I still think there are some benefits to exposing a "Client Logger" to the hooks.  Thoughts?

Thanks for all the help

--John

[James Moger]

You beat me. I just updated, retested, and confirm your finding.

Yeah, I can see the utility of something like that..
If you are already hacking away at that why don't you whip up a proposal.

-J

[John Crygier]

Okay, I have it implemented.  I updated GroovyTestScript, although I'm not going through the effort of testing that ReceivePack is getting hit from the test case.  That would have required the test to have a real repository and runs the risk of it failing from time-to-time (if not run as part of the suite or if it runs early in the suite).

I have it pushed up to jcrygier/gitblit, but since I have the LDAP pull request pending, I can't start a new one (sorry, I'm no expert with GitHub).  I can tack it onto that pull request, but that doesn't quite seem right.

Ideas?

--John

[James Moger]

I'm not a GitHub guru either. :) But GitHub does allow me to
cherry-pick from your fork. Agreed, it should not be part of the LDAP
pull request. I'll take a peek this weekend.

Thanks for the contributions!

-J

[John Crygier]

Sounds like a plan.

No problem on the contributions, I'm having a blast!  Plan on seeing more from me, I think this project is already incredible, and has the potential to do even more!

--John