Page MenuHomePhabricator

When a commit appears as an ancestor of a permanent ref for the first time, run all import steps
ClosedPublic

Authored by epriestley on Apr 15 2019, 5:04 PM.
Tags
None
Referenced Files
F14115904: D20428.id48730.diff
Thu, Nov 28, 1:05 PM
Unknown Object (File)
Wed, Nov 27, 5:53 AM
Unknown Object (File)
Tue, Nov 26, 10:42 AM
Unknown Object (File)
Sat, Nov 23, 4:35 AM
Unknown Object (File)
Thu, Nov 21, 10:59 PM
Unknown Object (File)
Thu, Nov 21, 9:22 AM
Unknown Object (File)
Mon, Nov 18, 6:36 PM
Unknown Object (File)
Fri, Nov 15, 11:08 AM
Subscribers

Details

Summary

Depends on D20427. Ref T13277. As an optimization, when we discover that a commit which was previously only on a non-permanent ref ("tmp-epriestley-123") is now reachable from a permanent ref ("master"), we currently queue only a new "message" parse step.

This is an optimization because these commits previously got the full treatment (feed, publish, audit, etc) as soon as they were discovered. Now, those steps only happen once a commit is reachable from a permanent ref, so we need to run everything.

Test Plan
  • Pushed local "tmp-123" branch to remote tag "tmp-123".
  • Updated repository with "bin/repository update", saw commit import as a "not on any permanent ref" commit, with no herald/audit/etc.
  • Merged "tmp-123" tag into "master".
  • Pushed new "master".
  • Updated repository with "bin/repository refs ... --trace --verbose", saw commit detected as now reachable from a permament ref.

Diff Detail

Repository
rP Phabricator
Branch
autoclose12
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 22600
Build 30965: Run Core Tests
Build 30964: arc lint + arc unit

Event Timeline

This probably needs a little more refinement in followups. In particular:

  • We don't need to re-run the change parsing step, but I think we currently will.
  • We may need more locking / race-avoidance in the Owners / Herald steps for cases where a commit is pushed to tmp-epriestley and then later (within a few seconds, but not in the same push) pushed to master.
This revision is now accepted and ready to land.Apr 17 2019, 8:09 PM
artms added inline comments.
src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
488

Removing this option might have introduced regression: see fix D20880 - where Differential Revision cannot be closed based on matching hashes