Page MenuHomePhabricator

Support meta-audit of arbitrary baskets of nonadjacent commits
Closed, ResolvedPublic

Assigned To
Authored By
michas2
Jul 28 2014, 10:52 AM
Referenced Files
F2559018: Screen Shot 2017-02-01 at 5.08.24 PM.png
Feb 2 2017, 1:10 AM
Tokens
"Heartbreak" token, awarded by cspeckmim."Like" token, awarded by monsdar."Like" token, awarded by matelich."Like" token, awarded by mim.

Description

We are working in a scrum like process. All tasks have numbers and svn commits usually contain the number of the corresponding task.
Now we are thinking of using the phabricator audit tool for "code reviews".

We used to use Crucible before, which has the nice feature of grouping commits with the same task number into a single review for the whole task.

Is something like this possible in phabricator, too? (I only found a filter for the committer.)

If this is currently not possible, I would really love to see a feature like this in future versions.

Event Timeline

michas2 raised the priority of this task from to Normal.
michas2 updated the task description. (Show Details)
michas2 added a project: Audit.
michas2 added a subscriber: michas2.

Sorry if the question is too obvious, but why not using Differential for code review, which is the right tool for that job? You can relate multiple differentials (patches) to a single Phabricator task.

The old workflow used Jira for managing tasks and Crucible for reviewing changes after they were committed.
Pre-commit reviews are definitively better, but convincing the whole team to change their dear habits is not so easy.
Therefore I was hoping to start with the well known post-commit reviews and try to change to pre-commit later.

Regarding pre-commit reviews: I used gerrit with git before. But I'm not sure how pre-commit reviews work with svn, which does not allow local commits.
Does pre-commit on svn mean, that you cannot work on your code as long as your change is under review?

And how does differential make sure that the review changeset is the same as the later manually committed changeset?

epriestley lowered the priority of this task from Normal to Wishlist.Jul 28 2014, 1:23 PM
epriestley added a subscriber: epriestley.

This isn't currently supported, and we don't have immediate plans to support it. It would be very complicated to build, create a lot of UI/UX challenges, and we haven't seen much demand for it.

I think we'll probably build it eventually (and possibly meta-reviews for Differential, too), and may even build it relatively soon to support things like branch review for Releeph pull requests, but it's not on the immediate horizon. We also only need range review (i.e., all commits adjacent) for other purposes, and that might be much easier than "arbitrary basket of tagged commits which may be nonadjacent (and may not be in the same repository?)" review.

After T4896 and T4036 there may be better tools in the UI for finding all commits linked to a task, but those won't help with showing a single cohesive view across multiple disjoint commits.

Two strategies for SVN pre-commit review without blocking are: using multiple working copies like local branches; or saving your work to a .diff and using patch to restore it later (or using arc patch, if using Differential).

These are definitely both more cumbersome than Git.

epriestley renamed this task from Support for ticket numbers in commits? to Support meta-audit of arbitrary baskets of nonadjacent commits.Jul 28 2014, 1:27 PM

I wanted to add some additional information for my situataion. In my group we are also trying to move away from post-commit reviews and perform mostly pre-commit reviews. However, part of our regulatory system requires certain types of reviews of specific features throughout the year. These reviews are primarily focused on the current state of some feature, and is not easily put together by selecting commits to review. Having the ability to select any files at a given revision to review in an audit would be excellent feature. We have looked at several repository management suites along with Phabricator such as GitHub, GitLab, ReviewBoard, Gerrit, and Kallithea, and none of them appear to support this feature.

In addition to regulatory purposes, this immensely benefits our design process - when we need to review existing features to analyze the quality and gain a more technical review of how to update the system for new requirements.

@cspeckmim, T4348 is probably closer to the workflow you're after.

Our existing process also works with post-commit reviews instead of pre-commit. We used Upsource before, which handled the feature of having multiple commits for a specific audit quite elegant. Would love to see such a feature in Phabricator too.

epriestley claimed this task.

I'm going to close this as tentatively resolved since it's fairly old and things have changed a fair bit since it was filed. I believe we may now handle this use case reasonably well. If you still have use cases for this that aren't addressed at HEAD, feel free to file a new issue describing what problems you currently face after upgrading (following Contributing Feature Requests).

In particular:

  • T4348 is a better task for "audit a subset of a codebase", e.g., auditing all files in a codebase or auditing all files in a subdirectory. I think this is a different problem than auditing a subset of commits.
  • The original use case can now be satisfied by using Ref Txxx on commits, then reviewing them from Maniphest, because Maniphest now shows audit status for linked commits. This screenshot has revisions, but the "" audit status icon should be present even if you do not use pre-publish review:

Screen Shot 2017-02-01 at 5.08.24 PM.png (530×881 px, 220 KB)