Page MenuHomePhabricator

Allow Differential changesets to be marked with various attributes
Open, NormalPublic

Description

This task originally focused narrowly on generated code.

  • See also T13130 for broader discussion and rationale.
  • See PHI64 for some plans.

Recently, changesets in Differential have "attributes". There is a large set of behaviors which attributes should expand to handle. Generally, "attributes" mean that arc can specify these behaviors per-changeset with client-side rules, and Differential/Diffusion can optionally also specify these behaviors with rules.

  • Generated Code: See PHI64. arc should be able to mark arbitrary changesets as "generated" so they are folded at display time. See PHI1112 for a Go-specific convention.
  • Third-Party Code: See PHI1007. This is exactly the same as "generated" code, it just should say "This is third party code..." instead of "This is generated code...".
  • Context: See PHI675. By default, arc uploads files with full context. There are several levels of complexity here, but one desirable outcome is likely to let arc selectively send large files with less context (i.e., produce a low-context diff if a change includes a couple of lines in a 10MB text file).
  • Tab Widths: See T2495. arc could have rules to say "this file uses 17-space tabstops".
  • Syntax Highlighting: arc could have rules to say "by default, highlight this file as though it were python source code".
  • Encoding: arc could have rules to say "by default, render this file as encoded with Shift-JIS".
  • "Don't Hide this Deleted File": See PHI985, where an install would like Differential to show deleted files by default. Although I don't think this use case is very marginal, it might be reasonable to have an "always show this changeset" flag that's stronger than other behaviors like the "generated" attribute and the default behavior for deleted files. You could then configure arc to set this flag on every changeset if you want "always show deleted files" as a behavior.

Obsolete/questionable:

  • Whitespace: After changes connected to T13161, specifying whitespace behavior no longer makes much sense because whitespace options have been removed.
  • Bypass Owners: PHI1061 wants to be able to specify a rule that changes to a specific file by a specific user can bypass audits triggered by Owners. I think the best clean/general way to accomplish this is to provide "Owners: Skip Audit" and "Owners: Skip Review" attributes, which cause tagged files to be ignored when compiling package lists. I'm less thrilled about this approach after trying to actually do this. Among other reasons, we don't have Changeset objects in PhabricatorRepositoryCommitOwnersWorker. My next-best approach is to try to come at this via T731.

Related Objects

Event Timeline

epriestley triaged this task as Normal priority.Jan 18 2012, 7:40 PM
epriestley added projects: Differential, Arcanist.
epriestley added subscribers: epriestley, davidreuss.
epriestley changed file(s), attached 0: ; detached 0: .Jan 19 2012, 6:13 PM

D1455 is a partial approach. In thinking about that patch, it occurs to me that Diffusion would also benefit from being able to run these rules, so maybe the best place to put it is Phabricator, just pulled out of the hideous mess that is DifferentialChangesetParser so it can access the repository/project objects and have more information available to make decisions.

It would be nice to list the generated files in .arcproject, using some syntax similar to .gitignore

See also discussion in {D5826}.

The @generated tag work nicely for generators which you can force this string to appear. But you can't always do that. Here is a typical generated file header generated by Visual Studio Entity Framework:

//------------------------------------------------------------------------------
// <auto-generated>
//     This code was generated from a template.
//
//     Manual changes to this file may cause unexpected behavior in your application.
//     Manual changes to this file will be overwritten if the code is regenerated.
// </auto-generated>
//------------------------------------------------------------------------------

I can't exclude generated files because they just look like regular sources. It would be nice to add, say, <auto-generated> to the list of keywords to mark a file as generated.

I'm in a similar scenario, t4 templates for codegen in visual studio often have <auto-generated /> somewhere in them. It'd be nice to have this string configurable so we don't need to have comments like this everywhere:

// <auto-generated />
// @generated

just to followup: differential.generated-paths works great.

It'd be nice if this could a be .arcconfig value like differential.generated-marker: [regex to match in files] that defaults to @generated or similar so this could live with the project code.

chad changed the visibility from "All Users" to "Public (No Login Required)".Jul 3 2015, 4:35 AM
eadler added a project: Restricted Project.Sep 15 2016, 6:13 PM
gregprice added a subscriber: gregprice.

Having a generated-paths variable in .arcconfig with a similar semantics to the differential.generated-paths server configuration -- something like D11537 a couple of years ago was intended to do -- would be really useful!

A user on our large install asked for help getting some files ignored, where the software that generates them doesn't have a knob to insert extra text like the @generated keyword. We were briefly happy to find D11538 and its reference to an .arcconfig variable, before realizing that that was referring to another abandoned revision. In the end we added a rule to the server differential.generated-paths value, but putting something in .arcconfig would be much cleaner -- it'd be discoverable by non-admins, self-serve for users of any repo to modify, and have a changelog with all the nice review features provided by this code-review system we use :).

Whats the status of this? Also is there any actual documentation for the existing @generated and differential-generated-paths functionality?

See Planning for help on status and timelines.

epriestley renamed this task from Add more support for folding/ignoring/categorizing changesets from "arc" to Allow Differential changesets to be marked with various attributes.Jun 22 2018, 3:37 PM
epriestley updated the task description. (Show Details)
Pawka added a project: Restricted Project.Dec 4 2018, 12:35 PM

Is this functionality blocked on something else?

Mostly, there's just relatively little interest from paying customers right now. PHI64, PHI675, PHI723 and PHI985 are all somewhat interested, but they're generally low-priority / "nice-to-have" sorts of requests, or this is a tangential or supplemental approach that might help with some other problem the request focuses on. Since there's limited customer interest, this isn't a very high priority right now.

(This is somewhat entangled with the larger iterations in T13098 and T13161, but not really blocked by either one.)