Sometimes unable to lock the index during pre-commit hook

Hi everyone,

I am the maintainer of https://github.com/Cosium/maven-git-code-format/. maven-git-code-format is a code formatter plugin triggered by git pre-commit hook. On pre-commit, the plugin make use of https://www.eclipse.org/jgit/ to alter the git index to format files.

When performing:
  git add . && git commit -m "Test"
The plugin works without issues

But when performing instead:
  git commit -am "Test"
The plugin fails because it can't lock the index file.

With the commit "all" command, it seems that the index is locked at https://github.com/git/git/blob/master/builtin/commit.c#L419 and kept locked during the pre commit hooks execution.

This difference of behaviour with or without "all" option leaves me puzzled. Are pre-commit hooks expected to be able to manipulate the index? If yes, can we call the current commit "all" behaviour a bug? If no, is there a document specifying what hooks are authorized to do?

Thanks in advance

Réda Housni Alaoui <reda.housnialaoui@gmail.com> writes:

Hooks are described in githooks(5) manual pages; we may want to clarify what is not allowed, but back when most of the entries were written, the stance was that anything that is not explicitly allowed there is forbidden.

In general, a pre-<something> hook is a way to inspect (i.e. look but not touch) what is proposed to be done and veto it by exiting with non-zero. It is not expected to change the state of the repository in any way.

The code does not necessarily enforce it, because it is costly to take a snapshot of everything (including the index, the working tree files, the files that are untracked, the objects in the object database, etc.) before calling a hook and ensure that the hook did not touch anything.

Thanks.

Thanks for your answer Junio.

As you probably know, many projects (not only mine) format the code during pre-commit hook.

Could git project consider this widespread usecase? I guess unlocking the index (after https://github.com/git/git/blob/master/builtin/commit.c#L419) before running the hooks is not an option here? Could a new kind of hook be added to cover this usecase? If I am not mistaken, this would be similar to prepare-commit-msg which allows to fail or edit the commit message in place.

Le dim. 10 nov. 2019 à 08:26, Junio C Hamano <gitster@pobox.com> a écrit :

Junio C Hamano <gitster@pobox.com> writes:

Actually, we do accomodate for the possibility that pre-commit hook may muck with the on-disk index there days, even though the original design intention was not to allow random changes there (see ll 960- in the same file).

So it seems that if we hold the lock necessary to refresh the index for too long, it may be an option to move the code that unlocks to somewhere earlier in the callchain. prepare_index() however returns different on-disk index file (the real thing when making an as-is commit, and a temporary one otherwise), and the unlocking rule may be different, so some restructuring of the code might become necessary before that can be done. I dunno.

Hi Junio,

Thanks again for your answers.

I kept looking at the code and found out that the index path (temporary or not) is passed as an environment variable named GIT_INDEX_FILE to pre-commit hooks (https://github.com/git/git/blob/v2.24.0/builtin/commit.c#L1472). Making jGit lock/alter the file pointed by GIT_INDEX_FILE (instead of always locking .git/index) work with or without git commit 'all' option.

I guess this is the workflow that the GIT team imagined to allow pre-commit hooks to alter the index.

Thanks a lot !

Le lun. 11 nov. 2019 à 03:23, Junio C Hamano <gitster@pobox.com> a écrit :