Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save diff between hook executions #1566

Merged
merged 1 commit into from Aug 22, 2020
Merged

Conversation

jhenkens
Copy link
Contributor

This reduces the number of calls to git diff by about half, as discussed in #1564

Validated locally on a large repository.

Overall duration before: 7.93s
Overall duration after: 5.01s

pre_commit/commands/run.py Outdated Show resolved Hide resolved
pre_commit/commands/run.py Outdated Show resolved Hide resolved
pre_commit/commands/run.py Outdated Show resolved Hide resolved
pre_commit/commands/run.py Outdated Show resolved Hide resolved
@jhenkens jhenkens requested a review from asottile August 20, 2020 21:32
@asottile
Copy link
Member

could you please allow edits on your PR so I can fix some nitpicky stuff

@jhenkens
Copy link
Contributor Author

@asottile
Copy link
Member

hmmm maybe it's because the PR is created from the master branch -- can you push to another branch and change the target?

@jhenkens
Copy link
Contributor Author

Even when looking at the dialog to create a new PR from a different branch, I do not see that checkbox. Perhaps due to the company account?

@asottile
Copy link
Member

alternatively, if you apply this and squash the commits that'll be equivalent to what I was going to do:

(copy, git apply, paste, ^D)

diff --git a/pre_commit/commands/run.py b/pre_commit/commands/run.py
index 170d43a..1f28c8c 100644
--- a/pre_commit/commands/run.py
+++ b/pre_commit/commands/run.py
@@ -152,7 +152,7 @@ def _run_single_hook(
         )
         duration = None
         retcode = 0
-        ret_diff = diff_before
+        diff_after = diff_before
         files_modified = False
         out = b''
     elif not filenames and not hook.always_run:
@@ -168,7 +168,7 @@ def _run_single_hook(
         )
         duration = None
         retcode = 0
-        ret_diff = diff_before
+        diff_after = diff_before
         files_modified = False
         out = b''
     else:
@@ -181,10 +181,10 @@ def _run_single_hook(
         language = languages[hook.language]
         retcode, out = language.run_hook(hook, filenames, use_color)
         duration = round(time.time() - time_before, 2) or 0
-        ret_diff = _get_diff()
+        diff_after = _get_diff()
 
         # if the hook makes changes, fail the commit
-        files_modified = diff_before != ret_diff
+        files_modified = diff_before != diff_after
 
         if retcode or files_modified:
             print_color = color.RED
@@ -213,7 +213,7 @@ def _run_single_hook(
             output.write_line_b(out.strip(), logfile_name=hook.log_file)
             output.write_line()
 
-    return files_modified or bool(retcode), ret_diff
+    return files_modified or bool(retcode), diff_after
 
 
 def _compute_cols(hooks: Sequence[Hook]) -> int:
@@ -250,9 +250,8 @@ def _all_filenames(args: argparse.Namespace) -> Collection[str]:
 
 
 def _get_diff() -> bytes:
-    diff_cmd = ('git', 'diff', '--no-ext-diff')
-    _, diff_bytes, _ = cmd_output_b(*diff_cmd, retcode=None)
-    return diff_bytes
+    _, out, _ = cmd_output_b('git', 'diff', '--no-ext-diff', retcode=None)
+    return out
 
 
 def _run_hooks(
@@ -270,12 +269,11 @@ def _run_hooks(
     retval = 0
     prior_diff = _get_diff()
     for hook in hooks:
-        current_retval, current_diff = _run_single_hook(
+        current_retval, prior_diff = _run_single_hook(
             classifier, hook, skips, cols, prior_diff,
             verbose=args.verbose, use_color=args.color,
         )
         retval |= current_retval
-        prior_diff = current_diff
         if retval and config['fail_fast']:
             break
     if retval and args.show_diff_on_failure and prior_diff:

@asottile
Copy link
Member

hmm yeah maybe the company account has something to do with it, weird

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants