Get git history with 'arc patch'

Hello,

if I select a task in Differential, I can see the git history (clicking on Commits).

However when I get the patch with ‘arc patch D5’ I only get it as one big diff!

I want the full git history downloaded. Differential knows about it but how do I get it?

Thanks,

Andreas

1 Like

It’s not supported, but there’s an open ticket for this - https://secure.phabricator.com/T1508.

Differential doesn’t currently have all the commits, only their hashes and messages.

It is bad that it is destructive and removes information which are normally important. There is a reason why people create a lot of small commits but then arc just creates one big patch :frowning:

1 Like

The expectation in Phabricator is one idea = one commit. Then that commit is reviewed. It is not expected that someone build 10 commits, and ships one enormous patch. Diffs should be reviewed at the smallest possible level. We understand your workflow is desirable in some cases, but it isn’t really normal in Phabricator. Smaller patches mean faster review, easier reverts, cleaner code.

Well, if I create a branch and have 10 commits each of them important. Especially each patch is a step explaining and leading to the end result, then I would expect that ‘arc diff’ creates a Differential task for each commit and not create one huge patch for it!

1 Like

And I’d guess 99.999% of developers don’t work that way.

Specifically if every single patch you create is perfect and reviewable - hats off to you. The vast majority of the rest of us leave typos, lint fixes, and other bugs in our commits.

Yeah, what Chad said. Basically, A “Revision” is the base unit of review in our workflows (Which translates to a single commit); If you want each commit to be reviewed separately, then you should create a separate Revision from each one (arc diff HEAD~).

The guiding principle should be something like “Does this change makes sense on its own?” or maybe “year from now, when someone a blames a bug to a commit - will that commit/revision useful?”.

You can create a set of dependent revisions, it’s just a little more work. When I need to do that, I usually first squash the changes to one-commit-per-review, and then run something like git rebase -x arc diff HEAD~ or something do that extent. I think the biggest set I’ve ever had was maybe 4 revisions deep though.

I think my normal patchset is around 10 commits but can easily go up to 50. And none of there patchsets should be merged into one commit because each patch is made in a way it is easy to review.

Current example:
https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/master-smbpasswd

1 Like

Yeah, those look fine to me.

Since you expect each commit to be reviewed separately, you should create a separate review object - Revision - using a separate arc diff invocation. Even when we’ll keep the small commits that make up a Revision, it would be shown as a single item to review.

You can easily script something around git rebase -x for this, or maybe even arc diff --head (But that loses all lint/test capability, which is a shame).

Completely agree with @cryptomilk.
I’m new to Phabricator, and my first “revision” from 6 commits appeared as just a mess: https://phabricator.kde.org/D17662.
There is no such information how to do tricks with git rebase -x in Quick Start Guide.
I had to dig into all those internet forums just to learn such basic thing as how to put the existing commits for review individually…

Hi @bam,

The reason we don’t have that thing written down anywhere is that nobody could spare the time to write it down. We do have some deficiencies in the area of onboarding, because the assumption of the upstream is that teams will onboard their own staff, with 1-2 people in every team who know and can onboard a new user.
This assumption works well in the enterprise world, where Phabricator was born; It’s works less well in the open-source world.

The best approach to solve this is for you to ask the KDE team to write some onboarding docs to help new members, and add them to the Community Resources’ Training section, so that other teams can share and expand on it.

There’s a bunch or writeups around this topic - in https://secure.phabricator.com/book/phabricator/, in https://secure.phabricator.com/phame/post/view/766/write_review_merge_publish_phabricator_review_workflow/ and in https://secure.phabricator.com/book/phabflavor/, but none of it is exactly tailored to a new user (coming from an unknown background with unknown practices).

Thanks @avivey for the explanation.

The issues in question are dated 2012 or earlier. Through that time, I think documentation on such important aspects should be improved, even with deficiencies you mentioned.

KDE already has Phabricator onboarding docs: https://community.kde.org/Infrastructure/Phabricator, but it doesn’t mention git rebase -x trick also. And I doubt if they have to.
Working with patch-sets is too common technique to be ignored.
Even with git rebase -x hack, it’s clear that Phabricator was not designed with this technique in mind - after arc diff, it still needs a much hand-work afterwards (to stick the revisions together in stack, etc.).

That should be improved. Quick Start Guide seems good point to start with.

This kind of discussion isn’t constructive.