Page MenuHomePhabricator

better support for "raw" patches, in particular corresponding to uncommitted changes?
Closed, DuplicatePublic

Description

The context: KDE is moving from using ReviewBoard to using Phabricator, and I am working on a Purpose/Phabricator plugin so that developers can upload patches for review from KDevelop.

With ReviewBoard it was possible to upload a raw patch, either on the website or via a programmatic API, and in most cases it would be able to infer all required context from the corresponding git repository, exactly as one would expect when applying the patch in a local working copy of that repository. (It would refuse patches where this failed.)

This allowed a workflow that fits in very well with an IDE and is (IMHO) much more suited to small changes, esp. when you have multiple independent local changes. In KDevelop one could select a file or subdirectory, activate the "Show Differences" patch review mode, and upload the patch for review without having to worry about commits, branches, rebasing, merging etc, and regardless of the number of uncommitted other changes or untracked files. This is also a workflow that is particularly well suited for distribution/packaging maintainers who often also maintain a series of patches some of which they may want to submit upstream with minimal effort.

The Root Problem is that it doesn't appear to be possible currently to upload raw patches that reflect UNcommitted changes in a working copy.

I am not claiming either approach is better, I think both have their advantages and disadvantages as well as their intended audience and use.

Is this a feature that has ever been considered? Is it something that would require invasive changes to Phabricator itself or is it something that could be implemented largely in Arcanist?

Event Timeline

RJVB created this task.Mar 8 2017, 2:12 PM
chad added a subscriber: chad.Mar 8 2017, 2:50 PM

Please use Ponder or Conpherence if you have general questions about using Phabricator. If this is a feature request, it needs to state a root problem in order to be accepted. See Contributing Feature Requests and Describing Root Problems.

RJVB renamed this task from better support for "raw" patches? to better support for "raw" patches, in particular corresponding to uncommitted changes?.Mar 8 2017, 3:47 PM
RJVB updated the task description. (Show Details)

Sorry, the Root Problem was clear enough in my head but apparently not explicit enough in the OP. I hope to have addressed that now.

chad added a comment.Mar 8 2017, 4:09 PM

We want to know the problem you feel that feature solves specifically, not just that you want it. You can run patches manually or run arc diff --preview to just preview changes. But offhand I don't see any workflow benefit to reviewing non committed changes and don't recall any requests for such a feature.

chad added a comment.Mar 8 2017, 4:30 PM

I guess this is pre pre-commit review?

RJVB added a comment.Mar 8 2017, 5:12 PM

...

The problem to be solved is that workflows as I've tried to describe aren't possible. Workflows where there is no official policy that changes first have to be committed, signed off or whatever before they can be considered for inclusion upstream. Or if you want, workflows in a more relaxed community with smaller and larger contributions coming in from a wide variety of users and developers not all of whom are experts in using git, svn or what have you.

As I said in the OP, both approaches have their advantages and disadvantages as well as their audiences. I for instance do not see any workflow benefit to having to commit changes locally before I can present them for review, it usually only adds overhead and hassle, and begs confusion when you start talking about committed versions. I don't have the impression that the KDE community as a whole has an opposite viewpoint; the change from ReviewBoard to Phabricator is unrelated to this aspect AFAIK.

The KDE revision workflow as I know it is

  • present a patch. When working from KDevelop this patch can be created by doing "Show differences" on a file or subdirectory so that only outstanding changes in that or those items are considered. But one can also create a more all-encompassing patch and then prune hunks by hand that shouldn't be included.
  • act on reviews
  • once accepted, commit the patch to the (up-to-date) target branch and push to the official repository
  • when commits are mentioned they only refer to commits to the official repositories.

The whole revision history of the patch is not expected to be added to the official history. Making it obligatory to create such a history just so the Phab web interface can show context is thus an unnecessary overhead you can only really get rid off by uploading each diff from a dedicated working copy that you can trash once the change has been committed.

That overhead only becomes bigger if it means you need to commit all other unrelated changes too. Or rather, you'd probably have to revert those changes because otherwise they're likely to show up in the diff between the remote branch master and your feature commit.

I'm not sure what you mean with pre pre-commit preview. It's preview before upstreaming something, committing it to the official repository. What people do in their personal working copies or forks is their business and shouldn't affect anyone or anything beyond that.

Maybe I can formulate the underlying problem this way:
It shouldn't matter for the review process exactly how a patch was created, as long as it applies to the indicated repository/branch. When that is the case, all context reviewers might require should be available. It would be when they download and apply the patch in a local working copy so there is no reason why this couldn't be the case online.
Supporting this mode of operation should make Phabricator more appealing to a wider audience (and not less so to users who swear only by the current approach).

How do you make guarantee that unrelated, simultaneous, uncommitted changes in the same working copy do not interfere with each other?

That is, when you generate a diff of subdirectory_a/, why are you confident that simultaneous changes present in subdiretory_b/ are not related, and that claims you make about the behavior of the changes you are presenting are accurate (i.e., you claim that the changes you are presenting compile successful, but you've never actually compiled them on their own)?

chad added a comment.EditedMar 8 2017, 5:47 PM

Maybe a better way to explain Phabricator's philosophy is that it's build on code quality. That is, if we have to equally choose between developer efficiency and code quality, code quality will win because we feel code quality leads to greater long-term efficiency. It means less likelihood of wasting reviewers time, or chasing bugs, or missing issues because of easily catchable programmatic mistakes. Is it a great workflow if you're just fixing a typo in a document file? Not at all. But 99% of revisions are not typo fixes. With a commit, we can run builds, show reviewers full context diffs, allow for arc to patch diffs, allow for chain of custody and so on.

RJVB added a comment.Mar 8 2017, 6:26 PM

How do you make guarantee that unrelated, simultaneous, uncommitted changes in the same working copy do not interfere with each other?

I don't think you can one way or another, not if you accept the fact that multiple developers can be working on the same code base from any number of different locations. Or a single contributor who presents multiple overlapping diffs at the same time from different working copies.

Reviewing locally committed changes is undoubtedly a great tool that has certain advantages if you accept the overhead, but I think it's more adapted to development within a smaller community, like in a company.

Code quality is a lofty goal, but ultimately that boils down to the individuals working together, and in case of code review on the reviewers.

Ultimately it is already possible to upload and create reviews from raw patches. The only thing I am suggesting is to provide context from such patches as that should be just as possible as when those patches were committed locally before uploading.

We could probably upload a raw patch with an n=N context that's so large it covers even the largest file in the patch set (git diff -U99999999999) but I'm guessing that the remote Phab software also has access to the official repositories for all the other features it supports, and in that case those raw patches could just as well be committed on the server if they really have to be committed before they're really committed where they're intended to be committed (sic).

I'm going to merge this into T5029. This is an extremely low priority for this upstream because this workflow is virtually unheard of among full-time software development professionals (see "Target Audience" in Contributing Feature Requests) because it is inherently error-prone.

In the future, please coordinate with @bcooksley to submit requests on behalf of KDE. We've already had 5 or 6 different people submit requests from KDE, and we can not prioritize requests relative to one another if no one on KDE's side is coordinating things.

@epriestley my apologies for this. I had sent out a memo previously asking for everyone to file tasks on our Phabricator so that I could co-ordinate things from there. It seems some folks have missed that, so i've now resent it to a wider audience.