git rev-parse fails in the pre-receive hook using git >= 2.11.0

[Zitrax]

Hi Jens,

this is probably an issue on our side since it worked earlier, but maybe you have some pointers ?

After a server restart critic started to raise exceptions on new pushes, and it fails in processCommits() in index.py in the first line that calls git rev-parse --verify --quiet SHA1+^{commit}. The error message is: "fatal: Needed a single revision".

So when I tried to debug this a bit I can see that the same git rev-parse command works if I add it directly in the pre-receive hook. But putting the same command early in githook.py fails.

Further I can see that if I print the environment variables in pre-receive I can see the variable GIT_OBJECT_DIRECTORY that points to a temporary incoming folder for the new objects. I assume that the git rev-parse command uses this to find the new commit even though they are not yet in the main objects directory. But this environment variable does not seem to be present in githook.py or index.py after the data is sent over the socket. So it seemed to me that this is a problem. But I don't have the full overview - how will the critic hooks know about the new object if they don't have this env variable set ? Should it be set - or is there something else that should handle this ?

[jensl]

This sounds like a new feature in git that Critic needs to be taught to deal with. Did you by any chance upgrade to a new git version around the time of the restart?

[Zitrax]

That is likely, and I found this in the 2.11.0 git release notes:

In order for the receiving end of "git push" to inspect the
received history and decide to reject the push, the objects sent
from the sending end need to be made available to the hook and
the mechanism for the connectivity check, and this was done
traditionally by storing the objects in the receiving repository
and letting "git gc" expire them. Instead, store the newly
received objects in a temporary area, and make them available by
reusing the alternate object store mechanism to them only while we
decide if we accept the check, and once we decide, either migrate
them to the repository or purge them immediately.

Not at the machine now but will verify this tomorrow.

[Zitrax]

Indeed, reverting to git v2.10.2 makes it work again so the mentioned change in v2.11.0 seem to break critic. For me it's fine to revert git version now, but obviously critic will need a fix at some point.

[jensl]

Critic needs fixing indeed. Thank you for reporting the issue! I will look into fixing it as soon as I can.