Details
- Reviewers
epriestley - Maniphest Tasks
- T6293: Unable to mention differential revisions from commits
T4896: Move Audit to ApplicationTransactions - Commits
- Restricted Diffusion Commit
rP3dd92a78ac2a: Move audit to application transactions
made a commit and saw it get parsed.
made a commit with "Auditors: foo" field and saw audit made for foo
turned on inline patch and attach patch and saw the patches
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Question details are in the summary, but I added an inline on the part I don't get at the moment...
| src/applications/audit/editor/PhabricatorAuditEditor.php | ||
|---|---|---|
| 136–139 | when should I be writing this flag relative to the rest of the editor action? | |
This looks great, nice! Three thoughts:
- We should ideally backdate this transaction to the commit date (which may be arbitrarily far in the past, or even in the future). I think you can setDateCreated(...) on it to do this at top level without any of the underlying machinery mucking it up.
- On the editor, getRemarkupBlocksFromTransaction() should return the commit summary for these transactions, to activate @usernames, object mentions, and file embeds in commit messages (T6293 has a narrow discussion of this).
- TYPE_COMMIT or similar might be more appropriate than TYPE_PUSH, since it's possible we'll build a dedicated and distinct TYPE_PUSH eventually. In Git/Mercurial, the "commit" date and metadata are distinct from the "push" date and metadata, but we only know any push info if we host the repository, so support is a little uneven.
| src/applications/audit/editor/PhabricatorAuditEditor.php | ||
|---|---|---|
| 306–308 | This seems reasonable to me, per your inline. | |
| 359 | pht(), maybe -- or does that make this inconsistent? I think we store strings in the database right now, so maybe pht()ing is inconsistent. In some far-future world we should probably store constants (REQUESTED_BY_AUTHOR) and render them at display time, so not pht()'ing this might make that easier. | |
| src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php | ||
| 34 | I think it's possible that this isn't correct, if repository reparse is being run. I'm not sure if that affects anything. It might be cleaner to re-query the Commit with the appropriate need...() methods. | |
address feedback
only interesting bit was chose to consistently not pht the database-stored strings