Re: Destructive pre-commit behaviour and "--all"

From: Jan Rüegg <rggjan@gmail.com>
Date: Mar 28, 2023, 09:33 UTC
Message-ID: <CAMbUV8Eoi2fkuG5jZhjCSaB51Dry+cEbSXfotxhVGPaxA7BB_A@mail.gmail.com>
In-Reply-To: <xmqqfs9wcush.fsf@gitster.g>

Thanks Junio, Chris for the detailed explanations

> FYI, we tried not to do the extra re-reading, because pre-commit

hook was designed to be a mechanism to allow users to validate, but not correct, what gets committed.

I see, and wasn't quite aware of this. The issue I see is that, in contrast to what it was designed for, pre-commit is in practice used to change files.

The pre-commit project, for example (https://pre-commit.com/) has many hooks (as seen here: https://pre-commit.com/#4-optional-run-against-all-the-files) has many useful file-changing hooks. Small cleanup tasks like running code formatters, fixing end of line spaces etc. are very useful to run while committing. As this has become more common, also more dangerous hooks are run as part of pre-commit, like ruff (https://github.com/charliermarsh/ruff-pre-commit) which has pretty heavy auto-fixes like automatically removing unused imports.

This is usually not a problem. When you add all your changes to the staging area ("git add --all") and then commit, you will see what additional changes happened in the auto-fixes during the pre-commit. With a "git commit -a", however, this can potentially break your files without you knowing what exactly was changed in your code. Since that code was not committed or added to an index beforehand, it's impossible to find out what the hook changed.

As a developer working on a project using "destructive" pre-commit hooks, I was just wondering what can be done to make this "safer". Looking at the issue and comments in https://github.com/pre-commit/pre-commit/issues/1499 it seems the pre-commit hooks themselves don't have a way to do that. So it seems the only option to do this would be to add it somehow to git itself.

I was thinking of something like a setting "git config commit.add_before_precommit_runs true" which would emulate the behaviour of "git add --all && git commit". Or is there some other way (in git, the pre-commit hook, or anywhere else) to make this existing workflow safer without resorting to bash aliases and stuff like that?

Jan

On Wed, 22 Mar 2023 at 22:58, Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

>
> Chris Torek <chris.torek@gmail.com> writes:
>
> > It would in theory be possible for Git to load *the* index twice,
> > once before and once after the hook, and compare them to see what
> > changed, then perhaps try to use that change to update the
> > additional indices.  That would be a pretty big change, but if it
> > were done right, it might get what you want.
>
> FYI, we tried not to do the extra re-reading, because pre-commit
> hook was designed to be a mechanism to allow users to validate, but
> not correct, what gets committed.  As the system originally was
> designed, users who correctly use Git would *not* be modifying the
> index.  Because it is an error to modify the index in the hook, (1)
> re-reading the index just in case the user commits such a mistake is
> waste of resources, and (2) checking the index to make sure it did
> not change before and after invoking the hook, again, is also waste
> of resources.
>
> It may have been a mistake that we re-read the index in some case,
> which adds to the confusion, but not others.