The linked page is: https://moz-conduit.readthedocs.io/en/latest/arcanist-user.html#series-of-commits
Which did link to another now gone blog post but I found a copy on Internet Archive: https://web.archive.org/web/20180907120112/https://www.smacleod.ca/posts/commit-series-with-phabricator/
I could rewrite the linked Mozilla page so that we don't have to worry about it moving but I thought it would be a shame since it's quite well laid out.
I added the "recommended" (at least by me, please disagree) workflow paragraph to summarise what the now missing second blog post talked about.
Since this is in response to a question on llvm-dev I will post a link to this back there and hopefully the author can truly say whether this helps or not.
I'm not all that convinced about referring to the external arcanist reference here, since arcanist is only one way of uploading patches for review. I would be more inclined to just summarise the contents, e.g. the following two paragraphs:
"Creating a patch series requires some manual work. There are two methods to achieve this. The first approach is to create a patch using arc or the web interface for each individual commit, and then select the "Edit Related Revisions->Edit Parent Revisions" option in the Phabricator UI for each patch after the first in the series. This will open a dialog where you can enter the patch number (e.g. D12345) of the parent patch(es). Click "Save Parent Revisions" after entering them.
The second approach is to create the patches and simply add "Depends on DXXXXXX" to the patch description (where "DXXXXXX" is replaced by the parent's patch number) in the child patch, entirely on its own line, with a blank line before it."
You could still add a link, if you felt the concrete examples are helpful.
I'm not entirely convinced this paragraph is a) that useful, and b) all that understandable. A recommended workflow for what? Assuming "creating a commit series", in which case, what does "make the necessary changes" mean in this case?
@jhenderson , I really like your suggestion here!
External references for Arcanist/Phab are frequently requested and, AFAIK, this is the only thing that is available. It is very well written and goes over two approaches:
So it's not really Arcanist specific and I would definitely include it here. Perhaps you could add it as a "Useful reference with some examples". Btw, I know that both approaches require "some manual work", but perhaps it's worth emphasizing the difference between "fully manual" and "semi-automated".
I think that adding a paragraph like this is very helpful. People frequently ask: "how do it do this with arc?" and this paragraph provides an answer.
@DavidSpickett Perhaps just clarify that this is for rebasing/updating a series of patches on Phabricator with arc? Basically, the first paragraph in this patch is about the basics (i.e. how to create a series of patches) and this paragraph is about updating a series of patches, right?
Also, perhaps rephrase "A recommended" as "One frequently used"? Personally I don't mind too much, but some people might not like any Arcanist-related recommendations :)
You could use it for both. That's what I do personally but perhaps just because I could never figure out how to stop arc combining commits. Maybe once you've got a series setup, it knows not to because of the differential links?
Thanks for the update; it looks much clearer to me. In addition to my inline comments, is it worth adding a couple of other points?
- Should we say something about "before merging, go back and remove all "Depends on..." lines" to avoid cluttering the commit message with an irrelevant comment (irrelevant because once the patch has landed, there's an implicit dependency chain anyway, due to git.
- Maybe add a note saying that patch series don't need to be a strict linear history: a patch can have multiple parents and/or multiple children. Each additional parent dependency would require its own "Depends on" line, if I'm not mistaken.
I don't believe you need to use arc for this process. I've used the "Depends on" trick many times, and I don't use arc. You can do it in the patch description in the Web UI too.
Possible alternative phrasing in the inline edit, including a couple of other suggested tweaks ("is less" is subjective, whereas "may be less" will always be true (in that "not being less" is covered under "may").
Should this really be a double colon?
I think you want to put this in some kind of no-format/code markdown or similar.
This line will need modifying in a similar manner to the above comment, if you go with it.
This suggested workflow works in the same manner for the web UI.
arc diff --head <commit> where <commit> is the end of the commit range. I think.
That's true. In fact, there are at least 3 methods:
Originally David only documented 1. and 2. For me that's sufficient and it makes a lot of sense - there are 2 paragraphs and 2 "completely" different methods. IMO, adding 3. to the mix blurs the message a bit ("essentially, there are 2 ways to achieve this"). Perhaps "less is more" in this case?
This line will be added by Arcanist _after_ you run arc diff, right?
Rephrase so that method 1 is using the add revisions in the web interface and
method 2 is using "Depends on" with either arc or the web interface.
Add note that you can have multiple parent reviews. I tested that "and"
works to add more.
Implement phrasing comments.
Double backtick where showing a command so that dash dash isn't merged
visually into one character. Put the rebase command in its own box
since it didn't make sense on the end of a line.
Removed the "Differential review" line from the example commit since
that's only there once you upload it with arc.
On second thought there's not too much text here that it needs it.
On reflection I think the 2 routes are "Depends" with arc or web, and "Edit parent" with only the web. So I've rephrased it that way.
This makes the following text into a box.
Switched to plain text boxes or double backtick for all the commands.
Removed that line.
Mostly looks good. In case I don't get back to review this again (my last day before Christmas is tomorrow), looks good, once my comments have been addressed.
I think it's important to state "patch description" because the Phabricator description doesn't have to match the commit message the patch corresponds to.
(Also some minor grammar improvements)
I think the current form looks a little off to me (though I can't explain exactly why). I suggest adding "for example:" as per the inline edit.
Canonical spelling is "GitHub" (upper-case H).
It looks like after a few rounds of updates, two methods emerged:
- "Using the web UI"
- "Through commit messages/summaries"
Perhaps it's worth revisiting the idea of adding section titles? I think that it would help with the flow.
I think that "pushing to GitHub" is too vague. Also, this might suggest that you normally push multiple patches at once. But often you push one patch at a time, right?
It's not immediately clear what "this" refers to here. I would expand.
I would remove this last sentence. In this case, IMHO, less is more.
Basically, this is a bit complex and I think that focusing on arc diff + git rebase is sufficient. Yes, you can use the web UI too, but perhaps that's something that people can deduct here themselves?
Looking at this again, I think you should just put "When you push the patches, please ...". The fact that we push to GitHub is an implementation detail that might change, plus it could be misunderstood as something to do with GitHub PRs, I guess (i.e. pushing your private branch to the LLVM repo as a PR).
I wouldn't agree here (as someone who doesn't use arc at all, if I read this paragraph without the sentence, I'd just skip the rest of the example, as it "doesn't apply to me"), but on reread, this example really isn't all that applicable to the Web UI, since commit messages aren't directly linked to patch descriptions (i.e. modifying the local commit message has no impact on anything in the web UI).
Fair point. Maybe the first sentence should be rephrased to be a bit more general:
One frequently used workflow for creating a series of patches involves git's rebasing. These steps assume that you have a series of commits that you have not posted for review.
- ``arc diff`` to upload for review.`
- upload the current commit for a review (either with ``arc diff`` or via the we UI)
This way it should work for everyone (other steps are already general enough).
- Add section titles and used that to better signal what the rebasing suggestion refers to.
- Incorporated grammar changes.
- Removed full stops from end of lines that ended in a command. Since a monospace "." looks the same as the default style "." and could be seen as being part of the command.
- Changed a few instances of using "commit message" where I mean the patch summary. (which means it makes more sense if you're in the web UI)