1607549 - allow commit messsages that reference a non-public bug to be pushed if they are already in mozilla-central

Status: RESOLVED FIXED

(Developer Services :: Mercurial: hg.mozilla.org, defect, P2)

Description

[Zeid Zabaneh [:zeid]]

+++ This bug was initially created as a clone of Bug #1607003 +++

At the moment, Try pushes based on the tip of mozilla-central get rejected by the server:

remote: ************************************ ERROR ************************************
remote: Your commit message references bugs that are currently private. To avoid
remote: disclosing the nature of these bugs publicly, please remove the affected bug ID
remote: from the commit message.
remote:
remote:     Affected bug: 1513586
remote:
remote: Visit https://wiki.mozilla.org/Security/Bug_Approval_Process for more
remote: information.
remote:
remote:
remote: To push this commit anyway and ignore this warning, include SKIP_BMO_CHECK
remote: in your commit message.

Bug 1513586 is about a push to the nss repository: https://hg.mozilla.org/projects/nss/rev/993717228da05c6bf468ac861f01b6849a94eae3

It's referenced in the commit message of https://hg.mozilla.org/mozilla-central/rev/4de1557c27c6a6fc1b0b92dc3fdb507dd9015e50

That blocks pushes based on the tip of mozilla-central and everything pushed later while the push hook is active. The hook got added in bug 1420510.

Tweaking the repository history with setting the problematic changeset's state to draft and then amending the commit message allows one to push to Try but doesn't solve the issue in general.

Comment 1

[Tom Prince [:tomprince]]

I've also run into this pushing commits that have landed on mozilla-esr68.

Comment 2

[Andrew Halberstadt [:ahal]]

I've hit this twice in two days now (oddly hadn't seen it until yesterday). Is there a better workaround other than modifying the public commit to draft? I haven't been able to figure out how to push to try.

This seems like a fairly important bug to fix.

Comment 3

[Connor Sheehan [:sheehan]]

You should be able to add SKIP_BMO_CHECK to one of your draft changesets (ie the one you're pushing to try) to override the check.

If that isn't working, I'll disable the hook and we can debug this. Maybe we should disable this anyways, since it's hitting so many false positives.

Comment 4

[Bob Clary [:bc] (inactive)]

I'm reluctant to over-ride the block by substituting my judgement so I end up waiting for things to be merged to mozilla-central. I bet a number of people use autoland rather than mozilla-central or mozilla-unified and are hitting this. I vote for this to be disabled until we can eliminate the false positive issue.

Comment 5

[Andrew Halberstadt [:ahal]]

(In reply to Connor Sheehan [:sheehan] from comment #3)

You should be able to add SKIP_BMO_CHECK to one of your draft changesets (ie the one you're pushing to try) to override the check.

If that isn't working, I'll disable the hook and we can debug this. Maybe we should disable this anyways, since it's hitting so many false positives.

Ah I thought it would either need to be on the tip commit (which is auto-generated and not possible to edit) or on the one containing the security bug (which is public). If it can be on any artbirary commit, then that is at least a better workaround than messing with phases.

However, the UX here is still pretty bad. We're asking developers to:

1. Push to try and see a traceback (:zeid didn't mention it, but mach dumps a stack trace after the message in comment 0)
2. Understand why the problem is occurring (as :bc pointed out, the message is scary and it's unclear that overriding the check is acceptable)
3. Amend their commit message
4. Push to try again
5. Hopefully remember to undo the change to their commit message

Shouldn't a simple fix to this be to only check for security bugs in draft commits? Or is phase information not available on try because it's non-publishing?

Comment 6

[Andrew Halberstadt [:ahal]]

(In reply to Bob Clary [:bc] from comment #4)

I bet a number of people use autoland rather than mozilla-central or mozilla-unified and are hitting this.

Ahh, is this only showing up when rebasing on autoland? Or are people who rebase on autoland just more likely to be the first ones to push a commit to try?

I was actually rebased on autoland because I needed to resolve some conflicts, so that might explain why I only ran into this now.

Comment 7

[Connor Sheehan [:sheehan]]

(In reply to Andrew Halberstadt [:ahal] from comment #5)

Shouldn't a simple fix to this be to only check for security bugs in draft commits? Or is phase information not available on try because it's non-publishing?

We have a check for phase on the server side. However after a little digging, I think your theory about phase information not being available due to a non-publishing repo is correct.

I've disabled the hook in production and I'm working on a fix for this now.

Comment 8

[Connor Sheehan [:sheehan]]

Attachment: hghooks: change bug references check to a `pretxnclose` hook (Bug 1607549) r?zeid

After investigating several user reports of the bug references check
hook being too strict and rejecting too many pushes, it was determined
that the pretxnchangegroup hooks do not have access to the correct
changeset phase data when executing against pushes to non-publishing
repositories. This means that every changeset in a given push is being
inspected as though it was a draft changeset being pushed to try.
This bug went unnoticed in testing as the test repository is not set to
be non-publishing (as is the configuration for try).

This commit begins by setting the test try repository to publishing,
after which the test fails without any other modifications.

We then create a new check type for pretxnclose hooks and set them
to only execute when the txnname argument is "push". The remainder of
the code for the pretxnclose hook is copied from the pretxnchangegroup
hook, and the PreTxnCloseCheck class is also mostly copied.

We then switch the bug references hook to run as a pretxnclose hook, where
it will be able to access the correct changeset phase metadata for inspection.

After applying these changes, the test completes successfully with mostly no
other changes. We add some more verbose output, including the output of hg out
to show the phases of the changesets before pushing to the repo.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/3a29fda1c51d
ansible/hg-ssh: turn off bug references check on try

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/002cae39b87d
hghooks: change bug references check to a pretxnclose hook r=zeid

Comment 11

[Connor Sheehan [:sheehan]]

Attachment: ansible/hg-ssh: re-enable bug references check on try (Bug 1607549) r?zeid

This was never re-enabled after being fixed in 002cae39b87d.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/388ea44236d7
ansible/hg-ssh: re-enable bug references check on try r=zeid