{"thread":{"id":"59436","subject":"Destructive pre-commit behaviour and \"--all\"","startedAt":"2023-03-22T10:34:19Z","lastAt":"2023-03-28T09:35:33Z","messageCount":5,"participants":["Jan Rüegg","Chris Torek","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"473907","messageId":"CAMbUV8F8y62n5ud3AoWQak8J==Zrn7kzwTE7jCveVJEpfcqncg@mail.gmail.com","threadId":"59436","inReplyTo":null,"subject":"Destructive pre-commit behaviour and \"--all\"","fromName":"Jan Rüegg","fromEmail":"rggjan@gmail.com","sentAt":"2023-03-22T10:34:00Z","receivedAt":"2023-03-22T10:34:19Z","isPatch":false,"sender":{"key":"rggjan@gmail.com","avatar":null},"body":"Why doesn't \"git commit --all\" keep the files added to the staging\narea, so \"git commit --all\" would have the same behaviour as \"git add\n--all && git commit\"?\n\nAs described here a pre-commit hook which changes files, when run with\n\"git commit --all\", will make it impossible to see what the hook\nchanged.\n\nMaking \"git commit --all\" and \"git add --all && git commit\" have the\nsame behaviour (or at least having that configurable) would allow\nlooking (and reverting if necessary) the changes generated by the\nhooks.\n\n- Jan\n"},{"id":"473956","messageId":"CAPx1GvftGM2kpVL9JWWXrjyTRWs+6OxteoJDq9dgeOjuidUGwg@mail.gmail.com","threadId":"59436","inReplyTo":"CAMbUV8F8y62n5ud3AoWQak8J==Zrn7kzwTE7jCveVJEpfcqncg@mail.gmail.com","subject":"Re: Destructive pre-commit behaviour and \"--all\"","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2023-03-22T20:00:40Z","receivedAt":"2023-03-22T20:06:48Z","isPatch":false,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Wed, Mar 22, 2023 at 3:35 AM Jan Rüegg wrote:\n> Why doesn't \"git commit --all\" keep the files added to the staging\n> area, so \"git commit --all\" would have the same behaviour as \"git add\n> --all && git commit\"?\n\nIt's not completely clear to me what behavior you *want* here,\nbut I can answer the \"why doesn't\" question.\n\nRemember that `git commit` can be aborted (in several ways). If\nit *is* aborted, whatever it did must be rolled back. This means\nthat `git commit -a`, which is otherwise a lot like `git add -u &&\ngit commit`, is fundamentally different from a successful `git add\n-u` followed by a `git commit`. In particular, if the `git\ncommit` *is* aborted, the `git commit -a` command *must not* leave\nthe updated files in the index. The `git commit` without `-a` did\nnot update any files in the index, so it does not have to \"roll\nback\" the index update.\n\nTo achieve this, `git commit -a` doesn't actually update *the*\nindex at all. Instead, it prepares a *new* index and updates\nthat new index. It then proceeds to do the committing, using\nthe new index, rather than the main index, as if it were *the*\nindex. If all goes well so that the commit is done, the code\nthen swaps in the *new* index for *the* index. If the commit\nis aborted, the code simply *deletes* the new index, leaving\n*the* index unchanged.\n\nThis is fundamentally different from \"update *the* index, then\nstart committing, then either roll back *the* index or leave it in\nplace\": there's a secondary temporary index involved here.\n\nThings get even more complicated if you use `git commit --only`\n(vs `git commit --include`) as in this case there are *three*\nindices simultaneously active (\"the\" index, \"proposed index for\ncommit\", and \"proposed index if commit succeeds\"). All three\nmust be managed carefully and correctly.\n\nIt is unwise to invoke `git add` in a pre-commit hook precisely\nbecause there may be two or three indices, and `git add` cannot\naffect all of them correctly.\n\nNote that Git could be designed differently (with real databases\nthat have roll-forward and roll-back options), but that's a much\nbigger change. The process described here is merely how Git works\nnow, which explains the constraints on using `git add` in pre-\ncommit hooks.\n\nChris\n"},{"id":"473964","messageId":"CAPx1GvfFrUQBDnJ9QYue91ScZb9ctB+tWv2S+pcprVC7dTgdxg@mail.gmail.com","threadId":"59436","inReplyTo":"CAPx1GvftGM2kpVL9JWWXrjyTRWs+6OxteoJDq9dgeOjuidUGwg@mail.gmail.com","subject":"Re: Destructive pre-commit behaviour and \"--all\"","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2023-03-22T21:26:12Z","receivedAt":"2023-03-22T21:26:28Z","isPatch":false,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Wed, Mar 22, 2023 at 1:00 PM Chris Torek wrote:\n[snip]\n> Note that Git could be designed differently (with real databases\n> that have roll-forward and roll-back options), but that's a much\n> bigger change. ...\n\nI realized on re-reading my reply here that there's an odd bit at\nthe end. It's because I decided to simplify something away, but\nthat left the oddness: bigger change than what? So I'll bring the\nextra part back in.\n\nFor a `git commit` *without* `-a`, Git has a special case: after\nit runs the pre-commit hook, it reloads *the* index, as there's\nonly the one index. This makes it possible to run `git add`\nsuccessfully in a pre-commit hook. But when using `--only`,\n`--include`, or `--all` / `-a`, there are multiple index files.\nGit therefore *doesn't* re-load \"the\" index.\n\nIt would in theory be possible for Git to load *the* index twice,\nonce before and once after the hook, and compare them to see what\nchanged, then perhaps try to use that change to update the\nadditional indices. That would be a pretty big change, but if it\nwere done right, it might get what you want.\n\nChris\n"},{"id":"473965","messageId":"xmqqfs9wcush.fsf@gitster.g","threadId":"59436","inReplyTo":"CAPx1GvfFrUQBDnJ9QYue91ScZb9ctB+tWv2S+pcprVC7dTgdxg@mail.gmail.com","subject":"Re: Destructive pre-commit behaviour and \"--all\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-22T21:58:54Z","receivedAt":"2023-03-22T21:59:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Torek writes:\n\n> It would in theory be possible for Git to load *the* index twice,\n> once before and once after the hook, and compare them to see what\n> changed, then perhaps try to use that change to update the\n> additional indices. That would be a pretty big change, but if it\n> were done right, it might get what you want.\n\nFYI, we tried not to do the extra re-reading, because pre-commit\nhook was designed to be a mechanism to allow users to validate, but\nnot correct, what gets committed. As the system originally was\ndesigned, users who correctly use Git would *not* be modifying the\nindex. Because it is an error to modify the index in the hook, (1)\nre-reading the index just in case the user commits such a mistake is\nwaste of resources, and (2) checking the index to make sure it did\nnot change before and after invoking the hook, again, is also waste\nof resources.\n\nIt may have been a mistake that we re-read the index in some case,\nwhich adds to the confusion, but not others.\n"},{"id":"474270","messageId":"CAMbUV8Eoi2fkuG5jZhjCSaB51Dry+cEbSXfotxhVGPaxA7BB_A@mail.gmail.com","threadId":"59436","inReplyTo":"xmqqfs9wcush.fsf@gitster.g","subject":"Re: Destructive pre-commit behaviour and \"--all\"","fromName":"Jan Rüegg","fromEmail":"rggjan@gmail.com","sentAt":"2023-03-28T09:33:55Z","receivedAt":"2023-03-28T09:35:33Z","isPatch":false,"sender":{"key":"rggjan@gmail.com","avatar":null},"body":"Thanks Junio, Chris for the detailed explanations\n\n> FYI, we tried not to do the extra re-reading, because pre-commit\nhook was designed to be a mechanism to allow users to validate, but\nnot correct, what gets committed.\n\nI see, and wasn't quite aware of this. The issue I see is that, in\ncontrast to what it was designed for, pre-commit is in practice used\nto change files.\n\nThe pre-commit project, for example (https://pre-commit.com/) has many\nhooks (as seen here:\nhttps://pre-commit.com/#4-optional-run-against-all-the-files) has many\nuseful file-changing hooks. Small cleanup tasks like running code\nformatters, fixing end of line spaces etc. are very useful to run\nwhile committing. As this has become more common, also more dangerous\nhooks are run as part of pre-commit, like ruff\n(https://github.com/charliermarsh/ruff-pre-commit) which has pretty\nheavy auto-fixes like automatically removing unused imports.\n\nThis is usually not a problem. When you add all your changes to the\nstaging area (\"git add --all\") and then commit, you will see what\nadditional changes happened in the auto-fixes during the pre-commit.\nWith a \"git commit -a\", however, this can potentially break your files\nwithout you knowing what exactly was changed in your code. Since that\ncode was not committed or added to an index beforehand, it's\nimpossible to find out what the hook changed.\n\nAs a developer working on a project using \"destructive\" pre-commit\nhooks, I was just wondering what can be done to make this \"safer\".\nLooking at the issue and comments in\nhttps://github.com/pre-commit/pre-commit/issues/1499 it seems the\npre-commit hooks themselves don't have a way to do that. So it seems\nthe only option to do this would be to add it somehow to git itself.\n\nI was thinking of something like a setting \"git config\ncommit.add_before_precommit_runs true\" which would emulate the\nbehaviour of \"git add --all && git commit\". Or is there some other way\n(in git, the pre-commit hook, or anywhere else) to make this existing\nworkflow safer without resorting to bash aliases and stuff like that?\n\nJan\n\n\nOn Wed, 22 Mar 2023 at 22:58, Junio C Hamano wrote:\n>\n> Chris Torek writes:\n>\n> > It would in theory be possible for Git to load *the* index twice,\n> > once before and once after the hook, and compare them to see what\n> > changed, then perhaps try to use that change to update the\n> > additional indices. That would be a pretty big change, but if it\n> > were done right, it might get what you want.\n>\n> FYI, we tried not to do the extra re-reading, because pre-commit\n> hook was designed to be a mechanism to allow users to validate, but\n> not correct, what gets committed. As the system originally was\n> designed, users who correctly use Git would *not* be modifying the\n> index. Because it is an error to modify the index in the hook, (1)\n> re-reading the index just in case the user commits such a mistake is\n> waste of resources, and (2) checking the index to make sure it did\n> not change before and after invoking the hook, again, is also waste\n> of resources.\n>\n> It may have been a mistake that we re-read the index in some case,\n> which adds to the confusion, but not others.\n"}]}