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

From: Réda Housni Alaoui <reda.housnialaoui@gmail.com>
Date: Nov 11, 2019, 18:37 UTC
Message-ID: <CAE0unxzP07XsF7e3ZPZacBaotkzocc1mr6_2HyDfGT6s8VJaMg@mail.gmail.com>
In-Reply-To: <xmqqtv7bi22j.fsf@gitster-ct.c.googlers.com>

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 :

Show 35 quoted lines

>
> 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.