Page MenuHomePhabricator
Create Task
Maniphest T11023

Branch contains condition in Herald rule not behaving as expected
Closed, DuplicatePublic
Actions

Assigned To
None
Authored By
swisspol
May 24 2016, 6:14 AM
Tags
Referenced Files
F1657428: pasted_file
May 24 2016, 6:14 AM
F1657435: pasted_file
May 24 2016, 6:14 AM
F1657432: pasted_file
May 24 2016, 6:14 AM
Subscribers
epriestley
swisspol

Description

(Observed on Phacility earlier today)

We have repository object commit rules used to trigger Jenkins builds. Earlier today, because I needed to have OR and AND combination of conditions, I split them in 2.

The first rule simply checks if any files that are related to a given project have been modified like this (it uses OR):

pasted_file (401×1 px, 53 KB)

The second rule does an AND between the first rule and the branch on which the commit is like this:

pasted_file (391×1 px, 49 KB)

The intent of splitting the rules is to only trigger builds when committing on master and not on other branches. But the "Branch contains master" condition does not behave as expected:

  • Right now, there's only a single master branch in the repo and pushing a new commit does trigger the build correctly.
  • However, earlier today, there was another topic branch in the repo called mbedTLS:
    • A commit was pushed to that branch
    • Then the topic branch was fast-forwarded merged into master which was pushed
    • The commit page was showing "Branches: master, mbedTLS" (it's not showing anymore since mbedTLS has been deleted, so I don't know what the exact order was, but it was certainly showing both)
    • The second Herald rule failed even though the branches for the commit contain master
      pasted_file (108×356 px, 16 KB)

Related Objects

Event Timeline

swisspol created this task.May 24 2016, 6:14 AM
epriestley added a subscriber: epriestley.May 24 2016, 12:44 PM

Is this the same as T6522? I'm not sure where the last screenshot came from, I wouldn't expect an FF-merge + push to trigger another evaluation of Herald rules.

swisspol added a comment.May 24 2016, 2:29 PM

Sounds like a dup indeed. What's the work around?

  • Doing a rebase instead of FF-merge?
  • Deleting the topic branch in Diffusion first, then FF-merge and push?
epriestley added a comment.May 24 2016, 2:36 PM

Deleting the branch first won't have any effect, since the commit still exists.

Rebasing the commits will work, as will any other mutation (e.g., trivial amend to the message or timestamp change) which updates the commit hashes, creating new commits.

If you merge without fast-forwarding, the new merge commit will be subject to normal rule evaluation, although the commits it merges will not. I think this might be satisfactory given the current definition of rule H12, but would not work if you had a field check like "commit message contains: ..." because that would only run against the merge commit itself.

epriestley closed this task as a duplicate of T6522: "Commit's branches contains" condition is not re-evaluated for commits in multiple branches.May 24 2016, 2:36 PM
swisspol added a comment.May 24 2016, 2:37 PM

Deleting the branch first won't have any effect, since the commit still exists.

Even that topic branch is the only branch the commit was on? Commit objects are not GC'ed in Phabricator?

epriestley added a comment.May 24 2016, 2:38 PM

Commit objects are not GC'ed in Phabricator?

Right. If you --force push and delete master by accident, we do not destroy your five years of audit history.

Phabricator · Built by Phacility