Page MenuHomePhabricator

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

Authored by epriestley on Apr 15 2019, 5:04 PM.



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

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Apr 15 2019, 5:04 PM
epriestley requested review of this revision.Apr 15 2019, 5:05 PM
epriestley added a comment.EditedApr 15 2019, 5:07 PM

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.
amckinley accepted this revision.Apr 17 2019, 8:09 PM
This revision is now accepted and ready to land.Apr 17 2019, 8:09 PM
This revision was automatically updated to reflect the committed changes.
artms added a subscriber: artms.Oct 31 2019, 3:05 PM
artms added inline comments.

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