Page MenuHomePhabricator

In differential, it would be interesting to know which commit affected a given line
Closed, WontfixPublic

Description

Given a range of commits to be reviewed in differential, it would be useful to know which commit last affected a given line.

Let me know if I should make myself more clear :)

Event Timeline

MathieuDuponchelle raised the priority of this task from to Needs Triage.
MathieuDuponchelle updated the task description. (Show Details)
chad added a subscriber: chad.Feb 13 2015, 8:26 PM

Please read https://secure.phabricator.com/book/phabcontrib/article/feature_requests/ and let us know what the core problem you are having with Phabricator is.

Basically, we have a situation where we have a huge branch of unrelated commits that we want to review and merge. We wanted to use phabricator for this, but couldn't figure out a workflow we were happy with with phabricator, and use github for this instead (pull request is at https://github.com/pitivi/gst-devtools/pull/3).

Of course the situation we're in is pretty unusual, there aren't many people who can review this, I couldn't find time to make the review and the commits piled up, but this can happen.

We don't do merges and have a rebase-based workflow, and amend commits rather than pushing new commits on top in order to have a clean history, we also don't push new commits on master without review.

We can't find a solution to review that branch using fabricator, making one differential per commit seems impractical and reviewing the whole difference in one differential is impossible.

I would very much like to figure out a workflow that works for this case, because it's something that can and will happen again. Github is pretty much ideal for us with respect to pull requests and review, but phabricator has a ton of features we would like to use, we can use both but ideally would like to only use phabricator :)

avivey added a subscriber: avivey.Feb 13 2015, 11:36 PM

The most-common workflow is "one differential per commit", or, more precisely, "one commit per differential revision" (Creating a revision can be done with several commits, but they are expected to be squashed in the landing process).

Differential is usually used for pre-commit flow - a revision is a "pending commit". You can use that idea and review each commit before it is pushed to the feature branch (Your get_position_on_idle), and then merge that branch to master outside of phabricator.

For post-commit reviews, use Audit/Diffusion. It's a lot less powerful, because the commit is already pushed; Changes will make a new commit, and connecting the two commits is not natural (Maybe "x mentioned this commit in commit r123").

Yeah I understood the audit -> post-merge review ; differential -> pre-merge review, but neither of these applications really serve the use case at hand :) Github's workflow is useful in that case because we get all the contexts : it's possible to comment on the whole request, on the global diff and on each commit, and even better line comments are preserved even when the branch is rebased or commits amended.

chad added a comment.Feb 14 2015, 2:40 AM

We can't find a solution to review that branch using fabricator, making one differential per commit seems impractical and reviewing the whole difference in one differential is impossible.

The workflow for that is to review each commit to feature_branch in Differential, then when the branch is ready, it's simply merged over.

The problem I see with your GitHub model is you state the commits are unrelated. Though perhaps this is what you're trying to solve with Phabricator.

The other problem is we tout 'one idea = one commit' from an architecture point of view, that is pre-commit reviews are most helpful when they can significantly alter the commit from additional architectural scrutiny. My concern with unrelated commits and quick OKs is the code isn't actually being reviewed. Merely looked over.

chad added a comment.Feb 14 2015, 2:49 AM

T5722 and T4348 are the two tasks you'd most likely be interested in (most closely related).

epriestley closed this task as Wontfix.Sep 27 2015, 9:13 PM
epriestley claimed this task.
epriestley added a subscriber: epriestley.

We don't plan to accommodate this particular workflow in the upstream, per se. Beyond @chad's links, see also T1508.

The workflow we'd use for these commits in the upstream is to generate approximately one revision for each commit, although some are small and might be merged. I'd guess I'd end up with 40-50 revisions from the ~57 commits if I were making these changes.