"Close Revision" actions are executed by the Message worker, and do not execute if a previously discovered commit becomes reachable from a permanent branch
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	epriestley
	Jul 9 2020, 10:13 PM

Description

See PHI1804. Previously, see T13284, which has a more detailed discussion of the general case of this behavior. Broadly, the setup here is:

commit C is pushed to non-permanent branch "epriestley-tmp-123";
later, commit C is pushed to permanent branch "master".

After T13284, we correctly reprocess "C" when it becomes reachable. However, the reprocessing effectively skips the "Message" step, which has some side effects beyond parsing the message -- notably, closing related revisions and tasks.

This means that revisions related to C are not closed.

The cleanest fix is to move these actions to "Publish", since they're really publish actions anyway and other publishing stuff already happens there.

The major issue I see with this is that these actions currently depend on a DiffusionCommitRef object, which is available in the "Message" step but not in the "Publish" step. These actions also have some side effects that impact commit data and are not "pure" publish actions.

Revisions and Commits

rP Phabricator
		D21469	rP58d3f6145a85 Fix an issue where known Subversion commits are incorrectly shown as…
	Audit Required	D21468	rPf21a00a3159e Fix an out-of-order issue in the new update-during-publish behavior
		D21459	rP737e7c85412c When an in-process worker subtask fails permanently, don't fatal the whole…
		D21460	rP6f78e2a91c88 When a commit is marked "closeable", clear the "published" flag
		D21458	rP93ef902ffa3c Fix a view fatal in CommitGraphView when commits are undiscovered
		D21450	rPa39c59044235 Move task and revision closure to the "publishing" step of the commit import…
		D21449	rPcebde34425c2 Make "CommitData" wrap and persist a "CommitRef" record
		D21448	rPe454c3dafe99 Wrap all direct access to author/committer properties on "CommitData"
		D21446	rP3a80efa440f0 Build "DiffusionCommitRef" objects from "internal.commit.search", not…
		D21447	rP7d6874d9f099 Turn "bypassCache" into a no-op in "diffusion.querycommits"
		D21444	rPf6238f9d9bd3 Remove "bin/repository lookup-users" workflow
		D21443	rPa9506097ea04 Add "internal.commit.search" to replace the cache bypass mode of "diffusion.
		D21442	rPa745055813e4 Lift Diffusion Conduit call proxying to the root level of Conduit
		D21441	rP367cd289270d Delete some commit dead parsing code
		D21440	rP429543b6376f Fix some content/background overflow issues with commit graph lists
		D21431	rP0b64092d25d0 Improve handle/status list display on devices in commit graph lists
		D21430	rP49af92e90325 Improve commit action item layout on mobile
		D21418	rP57f9450bcfc0 Improve desktop and mobile layouts for new "CommitGridView"
		D21417	rP8aec3f916b39 Unify more build, property, auditor, and status information into…
		D21416	rP36dac46ff263 Clean up some minor commit list CSS
		D21415	rP57ee6649aa3f Remove "PhabricatorAuditListView"
		D21414	rP2b0632b442a2 Remove "DiffusionHistoryTableView" and "DiffusionHistoryView"
		D21413	rP7087c0439a7f Move the view of merged changes to "DiffusionCommitGraphView"
		D21412	rPcd09ba5e19d7 Replace "DiffusionCommitListView" with "DiffusionCommitGraphView"
		D21411	rP9fa25253844e Improve rendering of history graph in "CommitGraphView"
		D21410	rP46695c76ebfe Introduce "DiffusionCommitGraphView", which unifies "HistoryListView" and…
		D21409	rPc6de7c66a3c6 Remove the "Graph" view as a dedicated repository view
		D21408	rP9afc5c628757 Remove "Recent Commits" from repository landing page
		D21407	rPc8a279957dc1 Remove "DiffusionTagTableView"
		D21406	rP60e9f641907a Remove the "authored" subheader from commits
		D21405	rP7fd6bf26a949 Modernize "Author" and "Committer" rendering for commits
		D21404	rP81e4e5b7f9c9 Remove construction of "author" information from "LastModified" payload in…

Related Objects

Mentioned In: T13290: Clean up remaining "Autoclose" behaviors related to "One Revision, Many Commits"
D20880: Force `PhabricatorRepositoryGitCommitMessageParserWorker` to be executed when changing commit from non permanent to permanent which ensures that corresponding Differential Revision is closed
T13580: Make "fully imported" a separate commit bit or standalone flag, not a complex bitmask operation
Mentioned Here: T13645: In Diffusion "Tags" view, improve behavior for tags that point at tag objects
T13337: Resolve structural problems with Conduit API methods related to large input/output sizes and binary data
T13284: When a previously non-permanent ref is marked permanent, unpublished commits reachable from that ref are not published

Event Timeline

epriestley triaged this task as Normal priority.Jul 9 2020, 10:13 PM

epriestley created this task.

Herald added a subscriber: amckinley. · View Herald TranscriptJul 9 2020, 10:13 PM

A second issue is that some of the "publish" behavior depends on the commit/revision relationship. This can still be sequenced correctly entirely in the publish daemon, but "close related revisions" and "publish the commit" are not entirely independent.

epriestley added a revision: D21404: Remove construction of "author" information from "LastModified" payload in Diffusion.Jul 10 2020, 4:05 PM

The broader structural issue here is:

Only the Message daemon currently builds a DiffusionCommitRef. This object is expensive to build (requires a low-level repository call) and it's desirable to build it only once (and not rebuild it in the Publish daemon).
The DiffusionCommitRef currently has unique information which is not preserved on the commit object or commit data object, notably the commit hashes. In Git, this is significant because it is the only source of the tree hash.
The code which finds a revision for a given commit currently depends on being passed a DiffusionCommitRef object (which it uses to do hash queries).

So the "find a corresponding revision" code currently must live in the Message daemon.

I'd like to generally make this adjustment:

In the Message daemon, store the DiffusionCommitRef on the commit data object.
Later, reconstruct it from the commit data.

This is largely simple enough, but the CommitData object is a huge mess right now. A naive implementation of "save/load the CommitRef" would possibly put the 5th or 6th copy of some pieces of data onto the Commit/CommitData pair. I'd like to walk this object back into the realm of reason before making it more complex.

A simpler approach would be to keep the "find a revision" code in the Message daemon and only move the "close the revision" code to the Publish daemon. However, "find a revision" is really a publishing action (commits that are unpublished do not associated with a revision) so I'm less fond of this approach.

A cross-cutting issue here is that none of these objects support non-UTF8 data. For now, I'm not going to try to deal with that, and continue forcing everything to UTF-8. The original CommitRef is built over Conduit anyway so this has a complex dependency on T13337.

epriestley added a revision: D21405: Modernize "Author" and "Committer" rendering for commits.Jul 10 2020, 4:16 PM

epriestley added a revision: D21406: Remove the "authored" subheader from commits.Jul 10 2020, 4:20 PM

epriestley added a revision: D21407: Remove "DiffusionTagTableView".Jul 10 2020, 4:25 PM

epriestley added a revision: D21408: Remove "Recent Commits" from repository landing page.Jul 10 2020, 5:48 PM

epriestley added a revision: D21409: Remove the "Graph" view as a dedicated repository view.Jul 10 2020, 5:56 PM

epriestley added a revision: D21410: Introduce "DiffusionCommitGraphView", which unifies "HistoryListView" and "HistoryTableView".Jul 10 2020, 6:20 PM

epriestley added a revision: D21411: Improve rendering of history graph in "CommitGraphView".Jul 10 2020, 6:44 PM

epriestley added a revision: D21412: Replace "DiffusionCommitListView" with "DiffusionCommitGraphView".Jul 12 2020, 3:41 PM

epriestley added a revision: D21413: Move the view of merged changes to "DiffusionCommitGraphView".Jul 12 2020, 4:16 PM

epriestley added a revision: D21414: Remove "DiffusionHistoryTableView" and "DiffusionHistoryView".Jul 12 2020, 4:25 PM

epriestley added a revision: D21415: Remove "PhabricatorAuditListView".Jul 12 2020, 5:44 PM

epriestley added a revision: D21416: Clean up some minor commit list CSS.Jul 12 2020, 6:03 PM