Error with pre-commit: Error when running on git HEAD commit

[jabesq]

Versions:

• darker: 1.2.4
• pre-commit: 2.11.1:

pre-commit configuration:

default_stages: [commit]
repos:
  - repo: https://github.com/akaihola/darker
    rev: 1.2.4
    hooks:
      - id: darker
        args: [ --line-length, "120", --isort]
        additional_dependencies:
          - black~=21.7b0
          - isort~=5.0

Step to reproduce:
Run pre-commit on git HEAD that contains black or isort errors with the following command:

$> pre-commit run  -s HEAD^ -o HEAD

Error:
pre-commit fails at the darker step with the following error message:

Traceback (most recent call last):
  File "/Users/user/.cache/pre-commit/repokk6vqznj/py_env-python3/bin/darker", line 8, in <module>
    sys.exit(main())
  File "/Users/user/.cache/pre-commit/repokk6vqznj/py_env-python3/lib/python3.8/site-packages/darker/__main__.py", line 234, in main
    raise ArgumentError(
argparse.ArgumentError: argument -r/--revision: Can't write reformatted files for revision 'HEAD'. Either --diff or --check must be used.

[akaihola]

Thanks @jabesq for the report!

Have you had a chance to look at Darker's code to see what could be the reason for this?

[akaihola]

This is actually where I don't understand how pre-commit is supposed to work. If I'm comparing two old revisions in the repository, should the linter still overwrite the corrected file in the working tree even though it might not be identical to the -o revision?

Consider the following chain of events:

• create src.py, commit and tag v1
• modify src.py, commit and tag v2
• modify src.py
• run pre-commit run -s v1 -o v2

So if Darker now detects that some lines modified between v1 and v2 need to be reformatted, writing the result out would cancel any changes currently in the working directory for src.py, wouldn't it? Is that still what pre-commit expects it to do?

There was a previous discussion about this in Darker #2. Could you review that and see if it clarifies the current problem?

[akaihola]

@Pacu2, @CorreyL and @sherbie were involved when pre-commit support was originally added. Any insight into this issue from you?

[akaihola]

A-ha, pre-commit documentation on pre-commit during commits says:

Running hooks on unstaged changes can lead to both false-positives and false-negatives during committing. pre-commit only runs on the staged contents of files by temporarily saving the contents of your files at commit time and stashing the unstaged changes while running hooks.

So if I'm not mistaken, this means that a hook (like Darker) can safely modify the files on disk in whatever way, since pre-commit will stash and unstash the current working tree anyway.

In other words, if pre-commit invokes Darker with PRE_COMMIT_FROM_REF=v1 and PRE_COMMIT_TO_REF=v2, and the working tree is already at v3 plus some unstaged changes, Darker can write out reformatted v2 files even if they had been modified since.

Could someone please verify that this is a correct interpretation?

[akaihola]

Ok so I tried this out.

pre-commit run -s <some-old-commit> -o HEAD will

• keep modifications done by linters
• merge unstaged changes which were in the working tree with those modifications
• revert any staged changes.

pre-commit run -s <rev1> -o <rev2> (where <rev2> is older than HEAD) will

• overwrite the working tree with files from <rev2>
• keep modifications done by linters to those old versions
• try to merge unstaged changes which were in the working tree
• revert any staged changes.

So the behavior of pre-commit is not at all safe if

• -o points to a revision older than HEAD
• there are staged changes, or unstaged changes which conflict with linter modifications

I wonder if I'm getting this right. I'm also curious to know when pre-commit is called with the -s and -o options. Do IDEs do that in some specific situation?

[akaihola]

I'm thinking Darker could allow a non---diff/--check run to modify files, but only with --revision=:PRE-COMMIT: and $PRE_COMMIT_TO_REF=HEAD, since in that specific situation we know that

• Darker was called by pre-commit (--revision=:PRE-COMMIT: is otherwise undocumented and not intended for interactive use), and
• no changes in the worktree are lost by pre-commit.

I'm hesitant to allow this behavior for other values of $PRE_COMMIT_TO_REF due to the possibility of information loss, and I'd hate for the tool to cause that for an unsuspecting end user.