Page MenuHomePhabricator

Diffs in a task should be sorted by the dependency tree
Closed, InvalidPublic

Description

If the order of the commits changes, the alphabetical/chronological order of the diffs listed in a task's page does not make sense.

Event Timeline

aleb created this task.Dec 15 2015, 9:35 AM
aleb raised the priority of this task from to Needs Triage.
aleb updated the task description. (Show Details)
aleb changed the edit policy from "All Users" to "Custom Policy".
aleb added a project: Feature Request.
aleb added a subscriber: aleb.

I don't understand the request. In any case, see Describing Root Problems.

aleb added a comment.Dec 15 2015, 11:33 AM

For example here D506 should be between D560 and D561, not at the top:
https://phabricator.freedesktop.org/T3491

chad added a subscriber: chad.Dec 15 2015, 3:03 PM

Please read Describing Root Problems and Contributing Feature Requests. We unfortunately don't have the ability to prioritize feature requests that can't state a core problem. A core problem indicates to us what should be built, as in we may have a different solution or another means to solve your problem. Feature requests without core problems are problematic as they only serve the author's desires, not the entire projects need.

I don't believe this is a tree, that is a diff child can have 2 different parents.

aleb added a comment.Dec 15 2015, 3:29 PM

Sorry, those two pages are ridiculously long, I won't read them.

Not sure how to better explain the problem. If you look at the example it should be pretty obvious, see my previous comment.

In T9987#149252, @chad wrote:

Please read Describing Root Problems and Contributing Feature Requests. We unfortunately don't have the ability to prioritize feature requests that can't state a core problem. A core problem indicates to us what should be built, as in we may have a different solution or another means to solve your problem. Feature requests without core problems are problematic as they only serve the author's desires, not the entire projects need.

Sorry, those two pages are ridiculously long, I won't read them.

Not sure how to better explain the problem. If you look at the example it should be pretty obvious, see my previous comment.

I don't believe this is a tree, that is a diff child can have 2 different parents.

Yes, seems it can be a group of directed graphs with no cycles(?). Then it should be possible to traverse each graph starting from the diff which does not depend on any other diff in the same task. And the traversal would be a modified https://en.wikipedia.org/wiki/Graph_traversal#Breadth-first_search making sure visiting a node is performed only after all its parents have been visited.

chad closed this task as Invalid.Dec 15 2015, 3:43 PM
chad claimed this task.

Sorry, those two pages are ridiculously long, I won't read them.

Basically we're asking for 2 minutes of your time, which given you're asking for several hours of ours, seems reasonable. We have no other scalable means of handling the volume of feature requests that come in.

Calculated reading time of Describing Root Problems and Contributing Feature Requests is ~12min for very slow reader. Both of those documents are very helpful and universal because not only do they provide guidance for phabricator, but provide very general guidance, that can and will enhance Your input in any situation. Quite frankly, I translated and enforced those rules with my customers - it has improved both my life and my customers (after obvious "TL;DR" comments).

Now @aleb, as a developer not related to phabricator (only using it) I can tell you, that I don't understand Your request or report at all. I tried to follow Your description and links provided, yet seen no problem - surelly You do. So since You do understand problem, why won't you simply read Describing Root Problems and Contributing Feature Requests? It will take probably no longer than 12 minutes and will provide You with knowledge and tools necessary to describe problem fully in a way that everybody would be able to understand. This will ultimately lead to only possible conclusion: your root problem being solved!

chad added a comment.Dec 15 2015, 5:11 PM

I understand the basic request "I want my diffs sorted on the task by dependency, not alphabetical", I just don't understand they why, or more importantly, what the core problem is. The core problem is what's missing, and what means we can't move this forward. Why do you want this, what information is missing, how would you make different conclusions from having that information. If we provided it on a different view, would that suffice, and so on.

Fundamentally, the upstream is overwhelmed by support. We're two full time people managing a very large project. The documentation serves as a means to cut down on repetitive back and forths with requests by explaining our needs upfront. This means more time building new features and less time providing the same questions to each new user that shows up. It's good for everyone.

aleb added a comment.Dec 15 2015, 6:52 PM

When the reviewer looks at the task to review the diffs, he needs to start somewhere and end somewhere. This is where the diffs list help: the reviewer will start at the top and finish at the bottom. Currently the diffs are ordered by creation time. Normally this works ok because the newer diffs depend on the older diffs. But if a newer diff is made a dependency of an older diff, the order of the list is not relevant.

You might ask in which case a newer diff is made a dependency of an older diff. Probably rarely. For example when a task has many diffs and by the time the reviewer gets to review it, the task owner reorders them to make it a bit nicer.

A real example: https://phabricator.freedesktop.org/T3491, notice D506 is listed first, this is wrong. It should be listed after D560 because D506 depends on D560. The reordering has been done in this case to keep the ruler appearance changes together.

You might say a task should be split in smaller tasks, I say ok, but even then..

chad added a comment.Dec 15 2015, 8:57 PM

Based on https://phabricator.freedesktop.org/T3491 the root problem to me looks like "it should be easier to review large batchs of diffs". Right now you're using Maniphest as a stop-gap solution that should be correctly resolved Differential's Custom Query

chad added a comment.Dec 15 2015, 9:01 PM

T5815 and T9372 are all roughly along these lines to providing the experience you want. It'd also be good to know what's missing from Differential's QueryEngine that you're manually adding things to tasks.

aleb added a comment.EditedDec 15 2015, 9:45 PM

We use git-phab, a tool which creates a task per branch and keeps the commits in the branch and the diffs of the task in sync. https://phabricator.freedesktop.org/diffusion/GITPHAB/ What this means is that it allows us to change the order of the diffs more easily, so in our case it will happen more often!

Even if the reviewer would use the Differential's QueryEngine, that one only allows to order by "Created" or "Updated", so the problem still remains.

If you implement the deps-sorting which is useful when a reviewer has to look at some diffs, it could be useful in both the Task page for the diffs list and also as an "Order" option in the Differential's QueryEngine.

aleb added a comment.EditedJan 5 2016, 11:21 AM

Could you please reconsider this task considering what I said in the previous comment?

It would be very useful for people working on a project for a while, then sending a bunch of commits for review (grouped in a task) - git-phab makes this very easy.

What is the purpose of the list of diffs in a task, why does it exist? Why are the diffs sorted by creation time?

chad added a comment.Jan 5 2016, 3:10 PM

We don't make or support git-phab. If the only reason for needing a new feature is because you're using a new, unsupported 3rd party application, it's not something the upstream will have any ability to prioritize. We prioritize tasks by likely impact and adoption across all our installs.

See Describing Root Problems. We're happy to look at and recommend fixes to core problems that exist in workflows in Phabricator. We group common or similar issues and attack problems from one angle. If workflow problem only exists by use of a third party application, our only suggestion to you is to fork Phabricator and provide the functionality you need. Waiting for something like that to be resolved in the upstream would be 3-5 years away, at best.

aleb added a comment.EditedJan 23 2016, 6:15 PM

I think there is a misunderstanding. The problem is not caused by git-phab, it only becomes more visible with git-phab.

An example how new commits end up before older commits: Even when one is working on a large change, you find related small things to fix, or choose to make some smaller changes in preparation. But you still want your big commit to stay as small as possible so you keep them separate and move them *before* the main commit.

IIUC, the official arc tool squashes all the commits of the branch into a single diff. IMO there is a high risk this way that the diff gets larger and larger and thus more and more difficult to review. That's why some prefer to manage the commits/diffs themselves.

So ultimately it's about supporting a workflow in which commits are kept as small as possible and clean.

Is this a root-enough problem for reopening the task?

My main point is "organizing a bucket of diffs for review" is not something I would product plan for a feature in Maniphest. I only have a vague understanding of git-phab, but it seems like it's using tasks as milestones? We officially have milestone support coming very soon. Maybe @epriestley has other thoughts.

chad added a comment.Jan 23 2016, 6:51 PM

Basically if git-phab is resolving a deficiency in Phabricator workflows, it'd be helpful to understand why they are being used.

We have not seen any evidence that installs which do not use git-phab experience this problem, nor evidence that the upstream workflow is incompatible with small, clean commits.

In particular, we believe the upstream workflow is the best workflow available to support clean commits, which is why we use that workflow.

You're welcome to use a totally different workflow and use Phabricator as a general starting point for a custom flow, but should expect difficulty convincing us that issues you encounter are the most important things we could be spending our time on if you are the only user experiencing them and your root problem is so far afield from the problems faced by other installs that the builtin workflows are wholly unsuitable for it.

aleb added a comment.EditedMar 18 2016, 2:04 PM

We have not seen any evidence that installs which do not use git-phab experience this problem,

Create diff1, create diff 2, assign them to a task, change diff1 to depend on diff2, notice the task still displays them ordered by creation time (first diff1, second diff2), even though it makes no sense. I think you guys assume the users will use only one diff per task, while we prefer to keep diffs small and easy to review and understand, in which case it's very probable a task will have more than one diff. And believe it or not, it happens very often that diffs change dependency order. Please correct me if I'm wrong.

nor evidence that the upstream workflow is incompatible with small, clean commits.

Last time I checked arc was creating a single diff per branch and the commits are lost..

In particular, we believe the upstream workflow is the best workflow available to support clean commits, which is why we use that workflow.

You're welcome to use a totally different workflow and use Phabricator as a general starting point for a custom flow, but should expect difficulty convincing us that issues you encounter are the most important things we could be spending our time on if you are the only user experiencing them and your root problem is so far afield from the problems faced by other installs that the builtin workflows are wholly unsuitable for it.

I'm not alone. You can comment better on how others use it. I think most of the GStreamer contributors will start using git-phab once GStreamer moves to phabricator. And again.. what git-phab does is making this problem more visible, since it makes changing the diffs order much much easier. This task is not about git-phab.

Please tell me if you would merge this improvement if we implement it. Thanks!

Please tell me if you would merge this improvement if we implement it.

The upstream is not interested in this change.

aleb added a comment.Mar 20 2016, 10:05 PM

Why is the list of diffs in a task page ordered by creation time?

chad added a comment.Mar 20 2016, 10:18 PM
In T9987#166027, @aleb wrote:

Why is the list of diffs in a task page ordered by creation time?

I'm not sure what is unclear about how the upstream operates. We don't take feature requests like this. Please be considerate of the upstream. If you have needs we're not able to take, you're welcome to maintain a local patch, or if you need us to build that patch for you, pay our Consulting rates. (see https://secure.phabricator.com/w/consulting/).