Opened 4 years ago

Closed 5 weeks ago

#3679 closed change (rejected)

Reject commit messages that aren't in the right format

Reported by: fhd Assignee:
Priority: P4 Milestone:
Module: Sitescripts Keywords:
Cc: greiner Blocked By: #3674
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by fhd)

Background

In general, all Adblock Plus commit messages should either reference an existing issue by number, or start with Noissue. So far we just hope everyone sticks to that, but it would be better to ensure that in a hook.

What to change

#3674 added a hook that parses the commit message, and since #3676 sets it up as a pretxnchangegroup hook, we can simply change that hook to reject commits with an invalid commit message.

Change History (8)

comment:1 Changed 4 years ago by fhd

  • Blocked By 3674 added

comment:2 Changed 4 years ago by fhd

  • Description modified (diff)

I think this is more complicated than it sounds.

The only hook we seem to be able to use for rejecting changesets is pretxnchangegroup. It works just like changegroup but allows us to reject all changesets. I haven't seen any hook that we can use to reject individual changesets, and I can imagine that that is simply not possible because of the way push works.

So we need to deal with the potential situation where just one changeset in the group was problematic. I suppose the best way of doing that is to check all the issues before attempting to post any comments, but that leaves us with a potential race condition if an issue is being deleted while the hook runs. In that case we would post comments to some of the issues, referencing changesets that don't exist.

I couldn't come up with a good solution for this yet. We may have to finish #3678 first, which should give us more flexibility.

comment:3 Changed 4 years ago by fhd

  • Description modified (diff)

comment:4 Changed 4 years ago by fhd

  • Ready set

comment:5 follow-up: Changed 4 years ago by greiner

  • Cc greiner added

It'd be great if we could also check whether the commit message of Noissue-commits contains a reference (i.e. a URL) to the code review.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 4 years ago by fhd

Removing the ready flag, I just realised a problem we have to deal with: hg's automatic tag commits. hg tags are actually tracked in a file under version control, so adding or changing a tag needs a commit. That happens automatically, the format is: Added tag $TAG_NAME for changeset $CHANGESET_HASH. I guess the easiest way is to accept that format as valid as well. Thoughts?

Replying to greiner:

It'd be great if we could also check whether the commit message of Noissue-commits contains a reference (i.e. a URL) to the code review.

Yeah, agreed.

comment:7 in reply to: ↑ 6 Changed 4 years ago by kvas

Replying to fhd:

Removing the ready flag, I just realised a problem we have to deal with: hg's automatic tag commits. hg tags are actually tracked in a file under version control, so adding or changing a tag needs a commit. That happens automatically, the format is: Added tag $TAG_NAME for changeset $CHANGESET_HASH. I guess the easiest way is to accept that format as valid as well. Thoughts?

Sounds good to me.

comment:8 Changed 5 weeks ago by kvas

  • Resolution set to rejected
  • Status changed from new to closed

I think at this point with most development having moved to GitLab and the remaining few repos to follow soon this ticket can be declared irrelevant. I'm closing it.

Note: See TracTickets for help on using tickets.