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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx> wrote:
>
> Chris Torek <chris.torek@xxxxxxxxx> 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.



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux