#27993 closed defect (implemented)

craft a git pre push hook that declines to receive fixup! commits?

Reported by: arma Owned by: rl1987
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: technical-debt
Cc: catalyst Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

Every so often I see nickm push a commit to tor's main git that is a fixup! commit. It's clear that he meant to squash them before the push but didn't.

Is there some one-liner or something we can put in a git hook that notices these unintended commits and tells us to fix them before pushing?

Child Tickets

Change History (14)

comment:1 Changed 12 months ago by catalyst

Cc: catalyst added

+1 on this idea. It will help our history look tidier.

It seems likely that we'll need to involve the git-rw.tpo admins to install it, but I guess we might want to figure out details of what we want first?

comment:2 Changed 12 months ago by teor

Let's make sure the hook also includes squash! and the other directives.

comment:3 Changed 12 months ago by teor

Keywords: technical-debt added
Milestone: Tor: 0.3.6.x-final

Since this is a process improvement task, I am tentatively putting it in 0.3.6, marked technical-debt.

comment:4 Changed 12 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:5 Changed 11 months ago by rl1987

Status: newneeds_review

comment:6 Changed 11 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewmerge_ready

This works great for me.

After a discussion on IRC with rl1987, the changes were made that the git hook only considers master, release-* and maint-* branches. It won't apply to the other branches.

I'm also fine having it named pre-push in maint/ but there is also a possible argument to identify it has a git hook like pre-push.git-hook or something around those lines. I'll leave it to nickm to decide on that.

comment:7 Changed 11 months ago by nickm

I'd like the script to include directions on how to install it.

I like pre-push.git-hook as a name.

Question -- will this stop somebody from pushing the existing master branch (which has some errant fixup! commits) to a new repository?

comment:8 in reply to:  7 Changed 11 months ago by dgoulet

Replying to nickm:

I'd like the script to include directions on how to install it.

It should be very straight forward: "Copy file into .git/hooks/ and name it pre-push" :).

I like pre-push.git-hook as a name.

Question -- will this stop somebody from pushing the existing master branch (which has some errant fixup! commits) to a new repository?

Yes it will but for that you can use --no-verify switch. So you can push all branches _before_ installing the hook and be done with it :).

comment:9 Changed 11 months ago by nickm

Okay. Should we have the script tell you about --no-verify in that case? Let's assume that the user will periodically forget how to use git.

comment:10 in reply to:  9 Changed 11 months ago by dgoulet

Status: merge_readyneeds_revision

Replying to nickm:

Okay. Should we have the script tell you about --no-verify in that case? Let's assume that the user will periodically forget how to use git.

Yup agree it could output it in the error message. So if I summarize, we need three changes:

  1. Rename it to pre-push.git-hook in scripts/maint/
  2. Add an indication to the --no-verify option to the error.
  3. Comment at the start of the file that tells you how to install the hook.

comment:11 Changed 11 months ago by rl1987

Owner: set to rl1987
Status: needs_revisionaccepted

comment:12 Changed 11 months ago by rl1987

Status: acceptedneeds_review

comment:13 Changed 11 months ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;

comment:14 Changed 11 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged, installed, successfully pushed!

Note: See TracTickets for help on using tickets.