Opened 4 months ago

Closed 7 weeks ago

#30979 closed defect (fixed)

pre-push hook runs practracker unconditionally

Reported by: nickm Owned by: teor
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version: Tor: 0.4.1.1-alpha
Severity: Normal Keywords: git-scripts nickm-merge asn-merge
Cc: Actual Points: 0.1
Parent ID: Points: 0.1
Reviewer: dgoulet Sponsor:

Description

Our pre-push hook script runs practracker whenever it exists. But we only want to run practracker on branches that are targeted for master.

I think perhaps we should change it to try making check-local? Or perhaps it could check for the existence of some other file that we could use to indicate whether we want practracker to run.

Child Tickets

TicketStatusOwnerSummaryComponent
#31462closedteorRemove duplicate call to practracker in pre-commit hookCore Tor/Tor

Change History (11)

comment:1 in reply to:  description Changed 4 months ago by teor

Replying to nickm:

Our pre-push hook script runs practracker whenever it exists. But we only want to run practracker on branches that are targeted for master.

I think perhaps we should change it to try making check-local? Or perhaps it could check for the existence of some other file that we could use to indicate whether we want practracker to run.

Checking another file would be nice: I don't run configure to generate a Makefile in my upstream push directories, but I still want practracker to run on them.

comment:2 Changed 2 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:3 Changed 2 months ago by nickm

Actual Points: 0
Status: acceptedneeds_review

I ran ahead and did this, since I'm getting annoyed about seeing practracker failures whenever I push an older branch.

See my branch ticket30979 with PR at https://github.com/torproject/tor/pull/1220 .

comment:4 Changed 2 months ago by asn

Reviewer: dgoulet

comment:5 Changed 2 months ago by dgoulet

Keywords: asn-merge added
Status: needs_reviewmerge_ready

LGTM

comment:6 Changed 2 months ago by asn

Resolution: fixed
Status: merge_readyclosed

Merged!

comment:7 Changed 2 months ago by teor

Resolution: fixed
Status: closedreopened

This ticket does not fix this issue:

  • the pre-commit hook still calls practracker unconditionally
  • the pre-push hook should never have added a duplicate call to practracker in #30051, because it already calls the pre-commit hook, which calls practracker

I'll create another PR that fixes these bugs.

comment:8 Changed 2 months ago by teor

Actual Points: 00.1
Keywords: asn-merge removed
Owner: changed from nickm to teor
Points: 0.1
Status: reopenedassigned
Version: Tor: 0.4.1.1-alpha

See my PR which fixes this issue, I opened #31462 to get a different bug number, but updated both changes files:

comment:9 Changed 2 months ago by teor

Status: assignedneeds_review

comment:10 Changed 7 weeks ago by dgoulet

Keywords: nickm-merge asn-merge added
Status: needs_reviewmerge_ready

lgtm;

comment:11 Changed 7 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

merged!

Note: See TracTickets for help on using tickets.