Page MenuHomePhabricator

Plans: 2018 Week 19 - Week 22 Bonus Content
Closed, ResolvedPublic

Description

See PHI616. When comparing diff 1 to diff 2, hide all files which have not changed [and have no {old/right/undone} inline comments].

See PHI251. Although "Ignore Generated Changes" technically made it into the last release, it's still pretty rough and could use some cleanup (e.g.: documentation, checkboxes control).

See PHI624, an issue with breadcrumbs/hamburgers in Instances.

See PHI609. An install would like a way to filter commits by a particular branch, plus normal filter criteria. This is somewhat difficult in the general case (see the Maniphest fulltext search join issue from prior to Ferret: it's hard because we can't apply the branch constraint in MySQL, so we have to intersect "stuff on the branch" with "stuff matching the other conditions" in the application) but desirable, and likely tractable in reasonable cases.

See PHI652. An install reported an issue with echo x | arc paste ending up with a nonsense language setting.

See PHI637. See T1246.

See PHI592. A diff with 600MB of videos required more than two hours to turn into an email patch, which exceeded the task lease duration and cascaded into various other kinds of problems. This is somewhat adjacent to T11767. See prior discussion and context in T13130, which made some headway here.

Event Timeline

epriestley triaged this task as Normal priority.May 6 2018, 1:45 PM
epriestley created this task.

An install would like a way to filter commits by a particular branch, plus normal filter criteria.

I think the strategy we can take here is:

  • Run a history query on the branch, limited to (some multiple of?) the number of results the query would take to overheat. If we get back fewer results, implement the constraint as a commitIdentifier IN (%Ls) constraint. This handles cases where the branch has a fairly small total number of commits.
  • If we get back more results than the overheat limit, continue normally.
  • In didFilterPage(), filter with a new "does branch X contain commits A, B, C?" call. This can use the GraphStream classes to be reasonably efficient. This call can have a timeout (cumulative across all pages, I guess?) and trigger an overheat if it times out (slightly wacky?). We'll also overheat normally sooner or later but it might be a while if we don't have a separate timeout. On the other hand, you have to explicitly enable this mode, so maybe just letting it spin is fine for v1.

That should work well if either set is small.

It won't work well if you want, e.g., open audits on develop, and both develop and master are a million commits long, but develop has like 4 open audits and master has 850,000. But we have no capacity to execute this query efficiently today so we're just going to have to accept that it will overheat.

epriestley renamed this task from Plans: 2018 Week 19 Bonus Content to Plans: 2018 Week 19 / Week 20 Bonus Content.May 14 2018, 3:56 PM

When comparing diff 1 to diff 2, hide all files which have not changed [and have no {old/right/undone} inline comments].

Currently, when you compare "Diff 1" to "Diff 2" on a revision, you tend to get a lot of changesets which look like this:

Screen Shot 2018-05-16 at 5.35.41 AM.png (115×1 px, 22 KB)

This means "the author changed this file in Diff 1, but didn't make any adjustments to it between Diffs 1 and 2". There's some value in showing this as a general piece of context, and if you've only changed a handful of files and many of them get hidden like this it isn't really much of an issue.

However, for larger changes, these are less useful and largely just clutter. Although some large changes are "probably bad" (i.e., should be broken into a sequence of smaller changes in a perfect world), stuff like refactoring may affect a large number of files and have only a tiny set of relevant changes between diffs (e.g., you changed 113 callsites, and 109 are obvious/trivial while the other 4 merit some sort of discussion), and this sort of change isn't "probably bad".

The discussion on PHI616 covers a few things, but one change I'm planning to make to improve this case is to get rid of these "contents not changed" diffs when viewing diff-of-diffs, and probably just showing a "34 other files did not change between Diff 1 and Diff 2" element in the table of contents instead. For now, the rules will be:

  • if things changed, show the diff;
  • if either side has inline comments, show the diff.

The inline rule could possibly be refined in the future, but I think a simple rule will get us most of the way there.

epriestley renamed this task from Plans: 2018 Week 19 / Week 20 Bonus Content to Plans: 2018 Week 19 - Week 22 Bonus Content.Jun 5 2018, 9:08 PM
epriestley updated the task description. (Show Details)

Some of this moved forward into T13151.