Error with pre-commit: The environment variables PRE_COMMIT_FROM_REF and PRE_COMMIT_TO_REF must be defined

[rossbar]

Thanks for building darker, it seems to fill an obvious need and is a very nice tool!

Apologies in advance if I'm just misunderstanding something, but I'm having trouble getting the darker pre-commit hooks working out-of-the-box. I've followed the instructions in the README, but when I test it locally I'm seeing the following:

Trim Trailing Whitespace.................................................Passed
Fix End of Files.........................................................Passed
Check Yaml...........................................(no files to check)Skipped
Check for added large files..............................................Passed
darker...................................................................Failed
- hook id: darker
- exit code: 123

ERROR:darker.git:The environment variables PRE_COMMIT_FROM_REF and PRE_COMMIT_TO_REF must be defined when using -r / --revision :PRE-COMMIT:

I'm not sure I understand this error message - my understanding was that darker compares the current staged/working directory against HEAD by default. I tried to explicitly set PRE_COMMIT_FROM_REF=HEAD, but it doesn't seem possible to set the FROM envvar without also specifying TO.

Reproducing example

I tested things in a big, un-blackened code base, e.g. numpy (assuming a clean virtual env):

git clone https://github.com/numpy/numpy.git
cd numpy
pip install darker pre-commit
git checkout -b try-darker

Add some lines to test the formatter, e.g. I add the following to numpy/polynomial/chebyshev.py:

poorly_formatted_cruft = {'a' : 1,                                              
                          'b':2,}

Running darker directly, the result is as expected (note that the file has many non-black-formatted lines):

darker --diff numpy/polynomial/chebyshev.py
--- numpy/polynomial/chebyshev.py
+++ numpy/polynomial/chebyshev.py
@@ -123,8 +123,10 @@
     'chebgrid3d', 'chebvander2d', 'chebvander3d', 'chebcompanion',
     'chebgauss', 'chebweight', 'chebinterpolate']
 
-poorly_formatted_cruft = {'a' : 1,
-                          'b':2,}
+poorly_formatted_cruft = {
+    "a": 1,
+    "b": 2,
+}

Now test with pre-commit, first setting up .pre-commit-config.yaml as specified in the README, then:

pre-commit install
git commit -am "trying darker pre-commit hooks"
Trim Trailing Whitespace.................................................Passed
Fix End of Files.........................................................Passed
Check Yaml...........................................(no files to check)Skipped
Check for added large files..............................................Passed
darker...................................................................Failed
- hook id: darker
- exit code: 123

ERROR:darker.git:The environment variables PRE_COMMIT_FROM_REF and PRE_COMMIT_TO_REF must be defined when using -r / --revision :PRE-COMMIT:

Other attempts

I've also tried adding args: [--revision HEAD] to the pre-commit config, as is done in #108, but am still seeing the same error.

Version info

pre-commit: 2.10.1
darker: 1.2.2

[akaihola]

Hi @rossbar, thanks for the detailed bug report!

I haven't yet used Darker too much with pre-commit, so I'm not entirely sure what the problem could be. When adding #109 (which fixed #103 reported by @KangOl), I assumed that pre-commit will always provide values for PRE_COMMIT_FROM_REF and PRE_COMMIT_TO_REF. Thus these environment variables are required by Darker if the --revision=:PRE-COMMIT: command line argument is defined.

#109 documents:

When using the -r :PRE-COMMIT: / --revision=:PRE-COMMIT: parameter, Darker will expect to find the revision range from the PRE_COMMIT_FROM_REF and PRE_COMMIT_TO_REF environment variables as set by pre-commit.

Reading pre-commit's documentation, I see that the environment variables are only provided during pre-push hooks:

During a push, pre-commit will export the following environment variables:

• PRE_COMMIT_FROM_REF: the remote revision that is being pushed to.
• new in 2.2.0 prior to 2.2.0 the variable was PRE_COMMIT_SOURCE.
• PRE_COMMIT_TO_REF: the local revision that is being pushed to the remote.
• new in 2.2.0 prior to 2.2.0 the variable was PRE_COMMIT_ORIGIN.

Is it then so that from-ref and to-ref don't actually make sense during a pre-commit hook? So if you're using Darker in a commit hook, you shouldn't specify --revision=:PRE-COMMIT: but e.g. --revision=master... instead?

Maybe @KangOl, @sherbie or @samoylovfp could shed some light on this?

[tkolleh]

Perhaps this can be conditional, based on whether the environment variables have a value set or not. If set use them if not refer back to default behavior.

$ darker .

[akaihola]

@tkolleh but is that better than the current explicit --revision=:PRE-COMMIT: command line option? Isn't it a good feature to fail with an error if the environment variables are expected but not found?

[tkolleh]

I believe it is more consistent. The intuition is that pre-commit (by default) runs darker as if darker was ran from the terminal by a user upon first install. If the user wants to adjust how darker runs they can pass an option/argument to darker.

If using pre-commit the behavior is the same, by default run darker without options.

@akaihola Thank you for creating darker, its been a great help.

[akaihola]

@tkolleh, I'm not sure I follow what you'd see as the optimal behavior. Could you elaborate?

I'm also curious how you @rossbar see the points presented above?

[philipgian]

I've also stumbled upon this issue.

As far as I can tell, those variables are also set (besides the pre-push hooks) when running pre-commit with manual --from-ref,--to-ref arguments, to explicitly run the checks on a specific revision range.

Perhaps the setting --revision :PRE-COMMIT: could mean that darker is running in what could be called pre-commit mode. That means that darker will act upon a specific range provided by the aforementioned pre-commit args. If those are not set, it falls back to running against the changes to HEAD on the staged files. pre-commit should take care of stashing/unstashing all the non-staged changes.

Another relevant issue is that it is not possible to override the --revision :PRE-COMMIT: by setting the args entry in pre-commits configuration file (e.g. to --revision HEAD). One must manually override the entry key. This however is not an issue with the above proposal, where darker tries to be compatible with pre-commits execution context.

Side note: I've also found that pre-commit sets an environment flag which could be used to auto detect the pre-commit mode, albeit I didn't find a relevant reference on the documentation page.

Edit:
For example, this quick hack seams to do the trick:

diff --git a/src/darker/git.py b/src/darker/git.py
index 31c4b27..3ebc98f 100644
--- a/src/darker/git.py
+++ b/src/darker/git.py
@@ -105,12 +105,8 @@ class RevisionRange:
                     use_common_ancestor=True,
                 )
             except KeyError:
-                logger.error(
-                    "The environment variables PRE_COMMIT_FROM_REF and"
-                    " PRE_COMMIT_TO_REF must be defined when using -r / --revision"
-                    " :PRE-COMMIT:"
-                )
-                sys.exit(123)
+                # Fallback to running against HEAD
+                revision_range = "HEAD"
         match = COMMIT_RANGE_RE.match(revision_range)
         if match:
             rev1, range_dots, rev2 = match.groups()

[akaihola]

@rossbar, @tkolleh would @philipgian's suggestion above fix the issue for you, too, in an appropriate way? If so, @philipgian please go ahead with a pull request!

[rossbar]

I'm also curious how you @rossbar see the points presented above?

I think I agree with what @tkolleh is saying, though I'm not familiar enough with pre-commit to know if it's exactly correct. At the very least I think we agree on the desired default behavior.

would @philipgian's suggestion above fix the issue for you, too, in an appropriate way?

If I'm interpreting things correctly, I think this is consistent with @tkolleh 's intuition - i.e. run darker on the staged changes (against HEAD) unless the user explicitly asks for something different by passing in arguments or setting flags. I tested @philipgian 's suggested patch against the motivating case I set up in the OP and it does indeed behave the way that I was expecting/hoping, so +1 from me for that change if it doesn't break behavior for other users. Also thanks @philipgian for the detailed explanation, that was very informative!

[tkolleh]

Thanks @philipgian for the PR and the details provided above. The changes address my concerns.