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
F14074958: D20428.id48730.diff
Thu, Nov 21, 9:22 AM
Unknown Object (File)
Mon, Nov 18, 6:36 PM
Unknown Object (File)
Fri, Nov 15, 11:08 AM
Unknown Object (File)
Sun, Nov 10, 6:49 PM
Unknown Object (File)
Thu, Nov 7, 12:13 AM
Unknown Object (File)
Tue, Oct 29, 5:25 PM
Unknown Object (File)
Oct 21 2024, 5:44 PM
Unknown Object (File)
Oct 21 2024, 5:38 PM
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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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