{"thread":{"id":"52228","subject":"Sometimes unable to lock the index during pre-commit hook","startedAt":"2019-11-09T21:39:18Z","lastAt":"2019-11-11T18:37:18Z","messageCount":5,"participants":["Réda Housni Alaoui","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"385835","messageId":"CAE0unxyojHVWP4aM3C+DdMYORmcoCuq-0wwLGjE8ok6dLj_E8w@mail.gmail.com","threadId":"52228","inReplyTo":null,"subject":"Sometimes unable to lock the index during pre-commit hook","fromName":"Réda Housni Alaoui","fromEmail":"reda.housnialaoui@gmail.com","sentAt":"2019-11-09T21:39:04Z","receivedAt":"2019-11-09T21:39:18Z","isPatch":false,"sender":{"key":"reda.housnialaoui@gmail.com","avatar":"https://gravatar.com/avatar/6f2435d81f1871a5434cdf4a8ceda818ec3c2adb6aa9aed66e4d747a06c2b3bb?d=mp&s=160"},"body":"Hi everyone,\n\nI am the maintainer of https://github.com/Cosium/maven-git-code-format/.\nmaven-git-code-format is a code formatter plugin triggered by git\npre-commit hook.\nOn pre-commit, the plugin make use of https://www.eclipse.org/jgit/ to\nalter the git index to format files.\n\nWhen performing:\n git add . && git commit -m \"Test\"\nThe plugin works without issues\n\nBut when performing instead:\n git commit -am \"Test\"\nThe plugin fails because it can't lock the index file.\n\nWith the commit \"all\" command, it seems that the index is locked at\nhttps://github.com/git/git/blob/master/builtin/commit.c#L419 and kept\nlocked during the pre commit hooks execution.\n\nThis difference of behaviour with or without \"all\" option leaves me puzzled.\nAre pre-commit hooks expected to be able to manipulate the index?\nIf yes, can we call the current commit \"all\" behaviour a bug?\nIf no, is there a document specifying what hooks are authorized to do?\n\nThanks in advance\n"},{"id":"385844","messageId":"xmqqpni0jioq.fsf@gitster-ct.c.googlers.com","threadId":"52228","inReplyTo":"CAE0unxyojHVWP4aM3C+DdMYORmcoCuq-0wwLGjE8ok6dLj_E8w@mail.gmail.com","subject":"Re: Sometimes unable to lock the index during pre-commit hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-10T07:26:29Z","receivedAt":"2019-11-10T07:26:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Réda Housni Alaoui writes:\n\n> Are pre-commit hooks expected to be able to manipulate the index?\n\nHooks are described in githooks(5) manual pages; we may want to\nclarify what is not allowed, but back when most of the entries were\nwritten, the stance was that anything that is not explicitly allowed\nthere is forbidden.\n\nIn general, a pre- hook is a way to inspect (i.e. look\nbut not touch) what is proposed to be done and veto it by exiting\nwith non-zero. It is not expected to change the state of the\nrepository in any way.\n\nThe code does not necessarily enforce it, because it is costly to\ntake a snapshot of everything (including the index, the working tree\nfiles, the files that are untracked, the objects in the object\ndatabase, etc.) before calling a hook and ensure that the hook did\nnot touch anything.\n\nThanks.\n"},{"id":"385845","messageId":"CAE0unxxzBpTr1afmKfQm_nJYo5oFOftih+vE+wQbFW809rXdaA@mail.gmail.com","threadId":"52228","inReplyTo":"xmqqpni0jioq.fsf@gitster-ct.c.googlers.com","subject":"Re: Sometimes unable to lock the index during pre-commit hook","fromName":"Réda Housni Alaoui","fromEmail":"reda.housnialaoui@gmail.com","sentAt":"2019-11-10T09:10:57Z","receivedAt":"2019-11-10T09:11:13Z","isPatch":false,"sender":{"key":"reda.housnialaoui@gmail.com","avatar":"https://gravatar.com/avatar/6f2435d81f1871a5434cdf4a8ceda818ec3c2adb6aa9aed66e4d747a06c2b3bb?d=mp&s=160"},"body":"Thanks for your answer Junio.\n\nAs you probably know, many projects (not only mine) format the code\nduring pre-commit hook.\n\nCould git project consider this widespread usecase?\nI guess unlocking the index (after\nhttps://github.com/git/git/blob/master/builtin/commit.c#L419) before\nrunning the hooks is not an option here?\nCould a new kind of hook be added to cover this usecase? If I am not\nmistaken, this would be similar to prepare-commit-msg which allows to\nfail or edit the commit message in place.\n\nLe dim. 10 nov. 2019 à 08:26, Junio C Hamano a écrit :\n>\n> Réda Housni Alaoui writes:\n>\n> > Are pre-commit hooks expected to be able to manipulate the index?\n>\n> Hooks are described in githooks(5) manual pages; we may want to\n> clarify what is not allowed, but back when most of the entries were\n> written, the stance was that anything that is not explicitly allowed\n> there is forbidden.\n>\n> In general, a pre- hook is a way to inspect (i.e. look\n> but not touch) what is proposed to be done and veto it by exiting\n> with non-zero. It is not expected to change the state of the\n> repository in any way.\n>\n> The code does not necessarily enforce it, because it is costly to\n> take a snapshot of everything (including the index, the working tree\n> files, the files that are untracked, the objects in the object\n> database, etc.) before calling a hook and ensure that the hook did\n> not touch anything.\n>\n> Thanks.\n"},{"id":"385861","messageId":"xmqqtv7bi22j.fsf@gitster-ct.c.googlers.com","threadId":"52228","inReplyTo":"xmqqpni0jioq.fsf@gitster-ct.c.googlers.com","subject":"Re: Sometimes unable to lock the index during pre-commit hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-11T02:23:00Z","receivedAt":"2019-11-11T02:23:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Réda Housni Alaoui writes:\n>\n>> Are pre-commit hooks expected to be able to manipulate the index?\n>\n> Hooks are described in githooks(5) manual pages; we may want to\n> clarify what is not allowed, but back when most of the entries were\n> written, the stance was that anything that is not explicitly allowed\n> there is forbidden.\n>\n> In general, a pre- hook is a way to inspect (i.e. look\n> but not touch) what is proposed to be done and veto it by exiting\n> with non-zero. It is not expected to change the state of the\n> repository in any way.\n>\n> The code does not necessarily enforce it, because it is costly to\n> take a snapshot of everything (including the index, the working tree\n> files, the files that are untracked, the objects in the object\n> database, etc.) before calling a hook and ensure that the hook did\n> not touch anything.\n\nActually, we do accomodate for the possibility that pre-commit hook\nmay muck with the on-disk index there days, even though the original\ndesign intention was not to allow random changes there (see ll 960-\nin the same file).\n\nSo it seems that if we hold the lock necessary to refresh the index\nfor too long, it may be an option to move the code that unlocks to\nsomewhere earlier in the callchain. prepare_index() however returns\ndifferent on-disk index file (the real thing when making an as-is\ncommit, and a temporary one otherwise), and the unlocking rule may\nbe different, so some restructuring of the code might become\nnecessary before that can be done. I dunno.\n"},{"id":"385920","messageId":"CAE0unxzP07XsF7e3ZPZacBaotkzocc1mr6_2HyDfGT6s8VJaMg@mail.gmail.com","threadId":"52228","inReplyTo":"xmqqtv7bi22j.fsf@gitster-ct.c.googlers.com","subject":"Re: Sometimes unable to lock the index during pre-commit hook","fromName":"Réda Housni Alaoui","fromEmail":"reda.housnialaoui@gmail.com","sentAt":"2019-11-11T18:37:04Z","receivedAt":"2019-11-11T18:37:18Z","isPatch":false,"sender":{"key":"reda.housnialaoui@gmail.com","avatar":"https://gravatar.com/avatar/6f2435d81f1871a5434cdf4a8ceda818ec3c2adb6aa9aed66e4d747a06c2b3bb?d=mp&s=160"},"body":"Hi Junio,\n\nThanks again for your answers.\n\nI kept looking at the code and found out that the index path\n(temporary or not) is passed as an environment variable named\nGIT_INDEX_FILE to pre-commit hooks\n(https://github.com/git/git/blob/v2.24.0/builtin/commit.c#L1472).\nMaking jGit lock/alter the file pointed by GIT_INDEX_FILE (instead of\nalways locking .git/index) work with or without git commit 'all'\noption.\n\nI guess this is the workflow that the GIT team imagined to allow\npre-commit hooks to alter the index.\n\nThanks a lot !\n\nLe lun. 11 nov. 2019 à 03:23, Junio C Hamano a écrit :\n>\n> Junio C Hamano writes:\n>\n> > Réda Housni Alaoui writes:\n> >\n> >> Are pre-commit hooks expected to be able to manipulate the index?\n> >\n> > Hooks are described in githooks(5) manual pages; we may want to\n> > clarify what is not allowed, but back when most of the entries were\n> > written, the stance was that anything that is not explicitly allowed\n> > there is forbidden.\n> >\n> > In general, a pre- hook is a way to inspect (i.e. look\n> > but not touch) what is proposed to be done and veto it by exiting\n> > with non-zero. It is not expected to change the state of the\n> > repository in any way.\n> >\n> > The code does not necessarily enforce it, because it is costly to\n> > take a snapshot of everything (including the index, the working tree\n> > files, the files that are untracked, the objects in the object\n> > database, etc.) before calling a hook and ensure that the hook did\n> > not touch anything.\n>\n> Actually, we do accomodate for the possibility that pre-commit hook\n> may muck with the on-disk index there days, even though the original\n> design intention was not to allow random changes there (see ll 960-\n> in the same file).\n>\n> So it seems that if we hold the lock necessary to refresh the index\n> for too long, it may be an option to move the code that unlocks to\n> somewhere earlier in the callchain. prepare_index() however returns\n> different on-disk index file (the real thing when making an as-is\n> commit, and a temporary one otherwise), and the unlocking rule may\n> be different, so some restructuring of the code might become\n> necessary before that can be done. I dunno.\n"}]}