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

Add post-checkout #1339

Merged
merged 1 commit into from Feb 23, 2020
Merged

Conversation

andrewhare
Copy link

@andrewhare andrewhare commented Feb 20, 2020

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

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.

will have to fix CI but this is a good start

parser.add_argument(
'--new-head-ref',
help='The ref of the new HEAD (which may or may not have changed).',
)
Copy link
Member

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?

Copy link
Author

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.

Copy link
Member

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

Copy link
Author

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.

with cwd(path):
cmd_output('git', 'add', '.')
git_commit()
yield path
Copy link
Member

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

Copy link
Author

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?

Copy link
Member

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

Copy link
Author

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.

@andrewhare
Copy link
Author

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

@andrewhare
Copy link
Author

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

pre_commit/commands/hook_impl.py Outdated Show resolved Hide resolved
pre_commit/constants.py Outdated Show resolved Hide resolved
pre_commit/main.py Outdated Show resolved Hide resolved
pre_commit/main.py Outdated Show resolved Hide resolved
tests/commands/install_uninstall_test.py Outdated Show resolved Hide resolved
tests/commands/run_test.py Outdated Show resolved Hide resolved
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.

thanks again for working on this!

@andrewhare
Copy link
Author

andrewhare commented Feb 23, 2020 via email

@asottile
Copy link
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 😆

@asottile asottile merged commit 566f1af into pre-commit:master Feb 23, 2020
@asottile
Copy link
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 👍

@asottile
Copy link
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]        

@asottile
Copy link
Member

#1344 for that one!

@andrewhare
Copy link
Author

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

#1344 looks like a great fix.

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

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

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.

Support post-checkout
2 participants