Classify changesets as "generated" at creation time, in addition to display time
ClosedPublic
Actions

Authored by epriestley on May 4 2018, 7:33 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T13130: Plans: 2018 Week 17/18 Bonus Content

Commits

rPaf295341c863: Classify changesets as "generated" at creation time, in addition to display time

Summary

Ref T13130. See PHI251. Currently, changesets are marked as "generated" (i.e., the file contains generated code and does not normally need to be reviewed) at display time.

An install would like support for having Owners rules ignore generated files. Additionally, future changes anticipate making "generated" and some other similar behaviors more flexible and more general.

To support these, move toward a world where:

Changesets have "attributes": today, generated. In the future, perhaps: third-party, highlight-as, encoding, enormous-text-file, etc.
Attributes are either "trusted" (usually: the server assigned the attribute) or "untrusted" (usually: the client assigned the attribute). For attributes like "highlight-as", this isn't relevant, but I'd like to provide tools so that you can't make arc mark every file as "generated" and sneak past review rules in the future.

Here, the differential.generated-paths config can mark a file as "generated" with a trusted attribute. The @generated-in-content rule can mark a file as "generated" with an untrusted attribute.

Putting these attributes on changesets at creation time instead of display time will let Owners interact with changesets cheaply: it won't have to render an entire changeset just to figure out if it's generated or not.

Test Plan

Created a revision touching several files, some generated and some not.
Saw the generated files get marked properly with attribute metadata in the database, and show/fold as "Generated" in the UI.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.May 4 2018, 7:33 PM

Harbormaster completed remote builds in B20276: Diff 46466.May 4 2018, 7:34 PM

epriestley requested review of this revision.May 4 2018, 7:34 PM

epriestley added inline comments.May 4 2018, 7:38 PM

src/applications/differential/storage/DifferentialDiff.php
826	This doesn't feel like a great place for this code, but I expect to move it somewhere better once I can combine this copy of it with the similar code in `DifferentialChangesetParser.php`, after this has been in production for a bit and/or we do a migration and/or provide some migration tools. If I just delete the display-time version of the code right now, existing changesets will lose their "generated" behavior.

epriestley added a child revision: D19426: Prepare to support an "Ignore generated files" flag in Owners.May 4 2018, 8:30 PM

amckinley accepted this revision.May 5 2018, 2:13 AM

amckinley added inline comments.

src/applications/differential/parser/DifferentialChangesetParser.php
544–550	$generated = ($event->getValue('is_generated') \|\| $this->changeset->isGeneratedChangeset()) ?
src/applications/differential/storage/DifferentialDiff.php
847	Shouldn't this be per-repo? I can imagine a scenario where a random repo actually ends up producing "generated" diffs because they happened to create a file matching `differential.generated-paths`.
857–868	On a related note, maybe the untrusted config could come in part from `.arcconfig`?

This revision is now accepted and ready to land.May 5 2018, 2:13 AM

epriestley added inline comments.May 5 2018, 3:20 PM

src/applications/differential/parser/DifferentialChangesetParser.php
544–550	I split these apart a little oddly since I expect to possibly delete the top half of this function later, and that future diff will be cleaner with the logic separated.
src/applications/differential/storage/DifferentialDiff.php
847	Yes, this setting is pretty limited. It was added a long time ago, and I hope to remove it once there are better tools for classifying stuff as generated. We just don't have a way to do per-repo settings yet. One problem this setting is better-than-nothing at addressing is stuff like `xcode.pbproject` or whatever the file is, which XCode makes in every XCode project, is a big XML file, changes frequently, is effectively just generated code (although: not exactly), and which users can't reasonably add `@generated` to. Patterns like `.*/xcode.pbproject$` are probably better fits for this setting than `/path/to/some/specific/file.json`, which will misfire across repositories.
857–868	Yeah. In the future, I expect `arc` to provide untrusted configuration in most cases (see PHI64 and connected stuff, I think). We might keep some untrusted rules on the server side, so that if you copy/paste a diff with `@generated` you still get the folding behavior, but I think there's at least an argument for getting rid of them entirely, eventually. If you're copy/pasting diffs, more sophisticated stuff like `@generated` is probably not terribly important.