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 — Add post-checkout by andrewhare · Pull Request #1339 · pre-commit/pre-commit

Conversation

@andrewhare

@andrewhare andrewhare commented Feb 20, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

This adds support for the post-checkout hook.

Fixes: #1120
Documentation PR: pre-commit/pre-commit.com#307

One thing I would still like to do, if possible, would be to force hooks like this to always have always_run be set since that is somewhat implied by the nature of the hook (since it doesn't operate on files).

Sorry, something went wrong.

Uh oh!

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

casperdcl, jorgeorpinel, pared, dmpetrov, and Suor reacted with thumbs up emoji efiop, shcheklein, elleobrien, and dmpetrov reacted with hooray emoji efiop, shcheklein, and elleobrien reacted with heart emoji efiop, shcheklein, elleobrien, and jorgeorpinel reacted with rocket emoji
All reactions
  • 👍 5 reactions
  • 🎉 4 reactions
  • ❤️ 3 reactions
  • 🚀 4 reactions
@andrewhare
andrewhare requested a review from asottile February 20, 2020 20:30
@andrewhare andrewhare mentioned this pull request Feb 20, 2020
install: support pre-commit util treeverse/dvc#3271
Closed
asottile
asottile reviewed Feb 20, 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.

will have to fix CI but this is a good start

Sorry, something went wrong.

Uh oh!

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

efiop and elleobrien reacted with hooray emoji
All reactions
  • 🎉 2 reactions
Comment thread pre_commit/main.py Outdated
parser.add_argument(
'--new-head-ref',
help='The ref of the new HEAD (which may or may not have changed).',
)

@asottile asottile Feb 20, 2020

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.

I think we could potentially reuse --source / --origin for these arguments?

Sorry, something went wrong.

Uh oh!

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

All reactions

@andrewhare andrewhare Feb 21, 2020

Copy link
Copy Markdown
Author

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.

I can do that if you want to keep the arg count down. However, I don't think that source/origin maps directly to new-head-ref/prev-head-ref so it might create some confusion. I'll leave that decision up to you.

Sorry, something went wrong.

Uh oh!

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

All reactions

@asottile asottile Feb 21, 2020

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.

source / origin is a "from ref" -> "to ref" relation, so I think it fits well enough

Sorry, something went wrong.

Uh oh!

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

All reactions

@andrewhare andrewhare Feb 21, 2020

Copy link
Copy Markdown
Author

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.

Ok cool, I will do that.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread tests/conftest.py Outdated
with cwd(path):
cmd_output('git', 'add', '.')
git_commit()
yield path

@asottile asottile Feb 20, 2020

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.

since these are only used once, it probably makes sense to inline them adjacent to the test itself

Sorry, something went wrong.

Uh oh!

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

All reactions

@andrewhare andrewhare Feb 21, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Author

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.

Do you want me to inline the entire fixture into the test or just the git add / git commit part?

Sorry, something went wrong.

Uh oh!

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

All reactions

@asottile asottile Feb 21, 2020

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.

ah mostly that there isn't a reason to put it in conftest.py unless it is being shared across multiple tests

Sorry, something went wrong.

Uh oh!

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

All reactions

@andrewhare andrewhare Feb 21, 2020

Copy link
Copy Markdown
Author

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.

Sounds good to me, I will move it.

Sorry, something went wrong.

Uh oh!

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

All reactions
@andrewhare

andrewhare commented Feb 21, 2020

Copy link
Copy Markdown
Author

Thanks for the feedback, I will fix the CI issues later today.

All reactions

Sorry, something went wrong.

Uh oh!

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

@andrewhare

andrewhare commented Feb 22, 2020

Copy link
Copy Markdown
Author

@asottile I made the changes you requested and CI is now green as well, please take another look when you can.

All reactions

Sorry, something went wrong.

Uh oh!

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

asottile
asottile reviewed Feb 22, 2020
View reviewed changes
Comment thread pre_commit/commands/hook_impl.py Resolved

Uh oh!

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

Comment thread pre_commit/constants.py Resolved

Uh oh!

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

Comment thread pre_commit/main.py Resolved

Uh oh!

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

Comment thread pre_commit/main.py Resolved

Uh oh!

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

Comment thread tests/commands/install_uninstall_test.py Resolved

Uh oh!

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

Comment thread tests/commands/run_test.py Resolved

Uh oh!

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

Andrew Hare @asottile
Add post-checkout
18fa004
@asottile
asottile force-pushed the andrewhare/post-checkout branch from 8247369 to 18fa004 Compare February 23, 2020 18:23
asottile
asottile approved these changes Feb 23, 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.

thanks again for working on this!

Sorry, something went wrong.

Uh oh!

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

efiop reacted with hooray emoji efiop reacted with heart emoji
All reactions
  • 🎉 1 reaction
  • ❤️ 1 reaction
@andrewhare

andrewhare commented Feb 23, 2020 via email

Copy link
Copy Markdown
Author
Yeah, your welcome! It’s was fun, you’ve got a great project. Thanks for all your input!
…
On Sun, Feb 23, 2020 at 10:24 AM Anthony Sottile ***@***.***> wrote: ***@***.**** approved this pull request. thanks again for working on this! <https://camo.githubusercontent.com/da817a3f4b192dfb496051a9aba8111fb546ef29/68747470733a2f2f692e666c756666792e63632f673654523052564a4d4a3466356e577244523339744e7264646333674e53646d2e676966> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1339?email_source=notifications&email_token=AAAQKANQQH36CAF2RZ2YQF3REK5L7A5CNFSM4KYWLCFKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCWSKWZA#pullrequestreview-363113316>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAQKALKSGBO55A3EQFZUXDREK5L7ANCNFSM4KYWLCFA> .
efiop 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 commented Feb 23, 2020

Copy link
Copy Markdown
Member

I'm also going to follow up and add aliases for --source and --origin, probably --from-ref and --to-ref since the current names are definitely confusing 😆

efiop and marcjay reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

@asottile
asottile merged commit 566f1af into pre-commit:master Feb 23, 2020
@asottile

asottile commented Feb 23, 2020

Copy link
Copy Markdown
Member

just played with this a bit, not sure if it's intentional but the post-checkout hooks are currently getting the list of files different between the two branches

I think they're not supposed to get any files at all so I'm making a change to fix that 👍

All reactions

Sorry, something went wrong.

Uh oh!

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

@asottile

asottile commented Feb 23, 2020

Copy link
Copy Markdown
Member
$ ~/workspace/pre-commit/venv/bin/pre-commit install -t post-checkout
pre-commit installed at .git/hooks/post-checkout
$ git add .
$ git commit -m 'add config'
[master (root-commit) d15280a] add config
 1 file changed, 5 insertions(+)
 create mode 100644 .pre-commit-config.yaml
$ git status
On branch master
nothing to commit, working tree clean
$ git status
On branch master
nothing to commit, working tree clean
$ git checkout master -b wat
Switched to a new branch 'wat'
identity.............................................(no files to check)Skipped
- hook id: identity
$ touch foo.py
$ git status
On branch wat
Untracked files:
	foo.py

nothing added to commit but untracked files present
$ git add .
$ git commit -m "add foo"
[wat 1ca3ef0] add foo
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 foo.py
$ git checkout master 
Switched to branch 'master'
identity.............................................(no files to check)Skipped
- hook id: identity
$ git checkout wat
Switched to branch 'wat'
identity.................................................................Passed
- hook id: identity
- duration: 0.04s

foo.py

$ cat .pre-commit-config.yaml 
repos:
-   repo: meta
    hooks:
    -   id: identity
        stages: [post-checkout]        
All reactions

Sorry, something went wrong.

Uh oh!

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

@asottile

asottile commented Feb 23, 2020

Copy link
Copy Markdown
Member

#1344 for that one!

andrewhare 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.

@andrewhare

andrewhare commented Feb 24, 2020

Copy link
Copy Markdown
Author

@asottile Nice catch on not needing the files, I missed that!

#1344 looks like a great fix.

efiop 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.

@andrewhare
andrewhare deleted the andrewhare/post-checkout branch February 25, 2020 06:02
@asottile

asottile commented Mar 14, 2020

Copy link
Copy Markdown
Member

oh right -- this got released as part of 2.2.0 -- thanks again for the patch!

efiop 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.

@domodwyer domodwyer mentioned this pull request Apr 28, 2020
post-checkout hook prevents pre-commit hooks from working with unstaged files #1418
Closed
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.

Support post-checkout

Uh oh!

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

2 participants

@andrewhare @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.