Add practracker as a pre-commit and pre-push git hook for frequent coders

• it might still be annoying, but it's less annoying than having CI fail
• we can fix issues for infrequent coders at the CI or merge stage
• reviewers can see the practracker diff and decide if it's reasonable

changed milestone to %Tor: 0.4.1.x-final

Trac:
Parent Ticket: #29792 (moved)

added component::core tor/tor git-scripts milestone::Tor: 0.4.1.x-final owner::rl1987 parent::29792 points::1 practracker priority::high resolution::implemented reviewer::asn severity::normal sponsor::31-can status::closed tech-debt tor-ci tor-ci-fail-sometimes type::enhancement labels

Trac:
Keywords: practracker tech-debt tor-ci tor-git-scripts deleted, practracker tech-debt tor-ci git-scripts added

Trac:
Status: new to accepted
Owner: N/A to rl1987

https://github.com/torproject/tor/pull/912

Trac:
Status: accepted to needs_review

Trac:
Reviewer: N/A to asn

This seems to work for me.

An important thing here is that this script won't abort the commit when practracker finds issues, which seems like a reasonable idea since maybe you want to fix all the practracker issues on a subsequence commit. However, this might be a problem for people using git wrappers (like me) that ignore git-commit output, since they might not notice the failure. Personally, I turned this into a pre-commit hook for this reason, but this might not work for other people.

Tim, Nick, any opinions here?

Marking as merge_ready. Also see comment above.

Trac:
Status: needs_review to merge_ready

Replying to asn:

An important thing here is that this script won't abort the commit when practracker finds issues, which seems like a reasonable idea since maybe you want to fix all the practracker issues on a subsequence commit. However, this might be a problem for people using git wrappers (like me) that ignore git-commit output, since they might not notice the failure. Personally, I turned this into a pre-commit hook for this reason, but this might not work for other people.

Tim, Nick, any opinions here?

If CI will fail when we push, git should also fail locally.

But I don't really mind when we fail.

Perhaps we should make it:

• a pre-commit hook, to encourage frequent coders to write better code
• a pre-push hook, so mergers don't push branches that will fail CI

I think some other tickets might do what I want here.

Trac:
Status: merge_ready to needs_revision

Trac:
Summary: Add practracker as a post-commit git hook for frequent coders to Add practracker as a pre-commit and pre-push git hook for frequent coders

CI sometimes fails because we don't check practracker before pushing.

Trac:
Priority: Medium to High
Sponsor: N/A to Sponsor31-must
Keywords: practracker tech-debt tor-ci git-scripts deleted, practracker tech-debt tor-ci git-scripts tor-ci-fail-sometimes added

https://github.com/torproject/tor/pull/967

Trac:
Status: needs_revision to needs_review

Replying to rl1987:

https://github.com/torproject/tor/pull/967

pretty cool! Almost there, but it seems like the pre-push hook (which is the one I'd like to use) does not block the push if practracker complains. It does throw the practracker error but does not abort the push (at least in my testing with a github branch).

Trac:
Status: needs_review to needs_revision

Trac:
Status: needs_revision to needs_review

Yes, great! Thanks!

Trac:
Status: needs_review to merge_ready

Squashed and merged!

Trac:
Status: merge_ready to closed
Resolution: N/A to implemented

These can be sponsor 31, if we want them to be.

Trac:
Sponsor: Sponsor31-must to Sponsor31-can

closed

changed time estimate to 8h

mentioned in issue #30979 (moved)

mentioned in issue #31462 (moved)

mentioned in issue #29792 (moved)

moved to tpo/core/tor#30051 (closed)

mentioned in issue tpo/core/tor#31462 (closed)