Opened 4 weeks ago

Last modified 5 days ago

#30286 needs_review defect

pre-push git hook will warn about fixups for no reason

Reported by: asn Owned by: rl1987
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-ci, git-scripts, 041-should
Cc: rl1987 Actual Points:
Parent ID: Points: 0.2
Reviewer: asn Sponsor:

Description

$ git push github bug30076_conflict
Enter passphrase for key '/home/f/.ssh/id_rsa': 
Enter passphrase for key '/home/f/.ssh/id_rsa': 
Running pre-push hook
Found fixup! commit in refs/heads/bug30076_conflict, not pushing
If you really want to push this, use --no-verify.

My pre-push hook does not allow me to push a branch even tho it should. First of all, because the branch is going to github and not upstream, and most importantly because there is no fixup in my part of the branch. There might be fixups down in history but I'm not repsonsible for those and I cannot do anything about them/.

Child Tickets

Change History (11)

comment:1 Changed 4 weeks ago by asn

A workaround is to use HEAD instead of the branch name.

comment:2 Changed 4 weeks ago by rl1987

Owner: set to rl1987
Status: newaccepted

Will try to look into this soon.

comment:3 Changed 3 weeks ago by rl1987

Status: acceptedneeds_information

Regarding "Github but not upstream" part, we can refrain from checking commit titles when not pushing to upstream. See: https://github.com/rl1987/tor/commit/d91deeee45c3433cc7dcaf1e67a84692420db870

The second part is harder. To list commits that first appeared on branch being pushed one needs to find out parent branch, which seems to require some nasty looking code to make it work in general case. See:

Not sure the extra complexity cost makes it worthwhile?

comment:4 in reply to:  3 Changed 3 weeks ago by asn

Status: needs_informationnew

Replying to rl1987:

Regarding "Github but not upstream" part, we can refrain from checking commit titles when not pushing to upstream. See: https://github.com/rl1987/tor/commit/d91deeee45c3433cc7dcaf1e67a84692420db870

The second part is harder. To list commits that first appeared on branch being pushed one needs to find out parent branch, which seems to require some nasty looking code to make it work in general case. See:

Not sure the extra complexity cost makes it worthwhile?

I think we should at least do the github but not upstream part, because right now we can't push stuff without dirty workarounds.

Not sure if the second part is worth doing indeed. Let's start with the first and see how it goes?

comment:5 Changed 2 weeks ago by rl1987

From IRC: let's hardcode a default remote name of upstream and let it be overridden through environment variable (that we could set in e.g. .bashrc).

comment:6 Changed 11 days ago by rl1987

Status: newaccepted

comment:7 Changed 11 days ago by rl1987

Status: acceptedneeds_review

comment:8 Changed 9 days ago by dgoulet

Reviewer: asn

comment:9 Changed 8 days ago by asn

Status: needs_reviewneeds_revision

Looks good, but let's document the new env var in the top-level file comment, so that people dont have to read the source to discover this new feature.

Thanks!

comment:10 Changed 6 days ago by nickm

Keywords: 041-should added

comment:11 Changed 5 days ago by rl1987

Status: needs_revisionneeds_review
Note: See TracTickets for help on using tickets.