Page MenuHomePhabricator

add option to arc diff to include entire commit history for review
Closed, DuplicatePublic

Description

I use git, and I like to make a nice clean commit history that shows a logical progression of thought, especially with complicated changes. I make heavy use of rebasing to accomplish this. I'd like to be able to tell arcanist to include each one of my commits as a diff in my differential revision, so that my colleagues can review each commit in turn.

Currently, the only way to do this is to arc diff after every commit, and only add reviewers after I"ve finished. This is tedious and error-prone; if I forget to arc diff between two commits, I have to git reset --hard HEAD^, arc diff, and then git reset --hard <tip with second commit> and finally arc diff again. I have to hope I don't mess it up and lose HEAD.

Event Timeline

lexelby created this task.Jul 16 2014, 1:38 PM
lexelby raised the priority of this task from to Normal.
lexelby updated the task description. (Show Details)
lexelby added a project: Arcanist.
lexelby added a subscriber: lexelby.

As a workaround, you can use git rebase -i with "edit" to step through the commits one at a time.

You can also use git reflog to recover HEAD, reducing the risk of either of these strategies.

Ooh, hadn't thought of that trick with rebase. Thanks!

And sure, it's hard to actually really lose HEAD (especially if you just create a backup branch first), but it just sort of "feels" inherently risky nonetheless :)

avivey added a subscriber: avivey.Jul 16 2014, 4:43 PM

an even more convoluted hack I used to use, is to rebase -i, and add "exec arc diff" after each commit...

Oh, neat idea. I've automated interactive rebases in the past by providing a program for the GIT_SEQUENCE_EDITOR environment variable. I could probably hack something like that up to automatically stick the exec arc diff in there.

avivey added a comment.Oct 3 2014, 4:52 AM

Since this task was last updated, arc diff learned about --head, which lets you diff from any historical point (But doesn't run lint/unit).

ezyang added a comment.Oct 6 2014, 4:14 PM

I'd also like to chime in that it is frustrating to (1) make sure all of patches in the series have correct dependency information (T6232), and (2) that the Phabricator web interface doesn't have any affordances for handling patch series like this (e.g. collecting all the patches in a logical series into a single summary page.)

When we interact with users in the GHC community who are being asked to use Phabricator, there is a persistent misunderstanding that Arcanist must squash commits together (as opposed to the CR system they are more familiar with, GitHub PRs, which don't squash by default.) I do think that squashing by default is not the right thing to do, but let's take this one step at a time...

chad added a subscriber: chad.Oct 6 2014, 4:43 PM

@ezyang your arguments are certainly valid, but keep in mind the development team here is both very small and unlikely to spend time on features that only serve a small number of developers or installs. T4778 covers a bit how we decide to spend development time.

https://secure.phabricator.com/book/phabricator/article/arcanist_new_project/#history-mutability covers the topic of squashing. We believe we have chosen the correct defaults for vanilla Phabricator based on time and interaction with the people who use and maintain Phabricator at hundreds of installs as well as our own seasoned and documented development process.

Phabricator may do more than you currently realize or that we have spent time documenting. Spending time in IRC might be a good option at understanding the community and options you have more.

ezyang added a comment.Oct 6 2014, 5:41 PM

Thank you for the pointer on history mutability!

To be clear, I am definitely not demanding these features be implemented; mostly, I'm hoping to help contribute some feedback, and get some ideas for what the devs might be interested in, if I wanted to roll up my sleeves and contribute a patch. (For example, I think teaching arc diff to be interactive here would not be too difficult, and I'd be willing to write this patch the next time I need to submit another large patchset.) I'll try to spend more time on IRC.

ibotty added a subscriber: ibotty.Oct 19 2014, 11:43 AM
ianh added a subscriber: ianh.Feb 10 2016, 6:07 AM
kaya added a subscriber: kaya.Mar 18 2016, 5:42 PM

As requested, this is the same as T1508, "when I run arc diff, each commit in the range should be preserved and visible in the web UI".

Note that this is different from "when I run arc diff, I want it to create multiple revisions: one revision per commit in the range".