Sometimes unable to lock the index during pre-commit hook

Sometimes unable to lock the index during pre-commit hook

[Réda Housni Alaoui]

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

Re: Sometimes unable to lock the index during pre-commit hook

[Junio C Hamano]

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

> Are pre-commit hooks expected to be able to manipulate the index?

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.

Re: Sometimes unable to lock the index during pre-commit hook

[Réda Housni Alaoui]

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 :
>
> Réda Housni Alaoui <reda.housnialaoui@gmail.com> writes:
>
> > Are pre-commit hooks expected to be able to manipulate the index?
>
> 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.

Re: Sometimes unable to lock the index during pre-commit hook

[Junio C Hamano]

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

> Réda Housni Alaoui <reda.housnialaoui@gmail.com> writes:
>
>> Are pre-commit hooks expected to be able to manipulate the index?
>
> 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.

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.

Re: Sometimes unable to lock the index during pre-commit hook

[Réda Housni Alaoui]

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 :
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Réda Housni Alaoui <reda.housnialaoui@gmail.com> writes:
> >
> >> Are pre-commit hooks expected to be able to manipulate the index?
> >
> > 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.
>
> 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.