GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Save diff between hook executions by jhenkens · Pull Request #1566 · pre-commit/pre-commit

Conversation

@jhenkens

@jhenkens jhenkens commented Aug 20, 2020

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jhenkens
jhenkens force-pushed the master branch from 4e074e0 to 370642b Compare August 20, 2020 18:37
asottile
asottile reviewed Aug 20, 2020
View reviewed changes
Comment thread pre_commit/commands/run.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pre_commit/commands/run.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pre_commit/commands/run.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pre_commit/commands/run.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@jhenkens
jhenkens requested a review from asottile August 20, 2020 21:32
@asottile

asottile commented Aug 22, 2020

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jhenkens

jhenkens commented Aug 22, 2020

Copy link
Copy Markdown
Author

I am looking at this doc: https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork but I do not have that checkbox.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@asottile

asottile commented Aug 22, 2020

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jhenkens

jhenkens commented Aug 22, 2020

Copy link
Copy Markdown
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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@asottile

asottile commented Aug 22, 2020

Copy link
Copy Markdown
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:
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@asottile

asottile commented Aug 22, 2020

Copy link
Copy Markdown
Member

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

jhenkens reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@asottile asottile mentioned this pull request Aug 22, 2020
Add advice to "Files were modified…" error #1031
Closed
1 task
Johan Henkens
Save diff between hook executions
4063730
@jhenkens
jhenkens force-pushed the master branch from ecd57a5 to 4063730 Compare August 22, 2020 03:41
asottile
asottile approved these changes Aug 22, 2020
View reviewed changes

@asottile asottile left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@asottile
asottile merged commit 9a5461d into pre-commit:master Aug 22, 2020
@jhenkens jhenkens mentioned this pull request Aug 25, 2020
Idea: Improve hook speed by skipping before/after diff via readonly flag #1564
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asottile asottile asottile approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@jhenkens @asottile
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.