blog/2021/phabricator-etiquette-part-2-the-author/

[utterances-bot]

Phabricator Etiquette Part 2: The Author | Hunting the Shmoo

https://ahal.ca/blog/2021/phabricator-etiquette-part-2-the-author/

[squelart]

Thank you for this follow-up, very nice! Strong-agree with your readable-revisions checklist. 👍

Personally I can’t think of any scenario to request a non-blocking review over a blocking one.

I think I just had one such scenario recently:
I had a stack of patches with the first half to be reviewed by the appropriate domain expert, and the second half by another one. But I wanted to invite the 2nd reviewer to have a look at the first set, because: The 2nd set used stuff (structs, functions) introduced in the 1st set; the 2nd reviewer may still have insights and useful comments about the 1st set, even if not in their preferred domain.
I did that with "r?expert!,invited" (so, only blocking on the expert). Though the invited reviewer contacted me to clarify this -- they understood my intent but were not 100% certain, so it's not an obvious pattern! (Yet?)
An alternative would have been to add a comment on Phabricator in the first set, such as "@invitee, this patch contains stuff you need to know about, and feel free to comment", but it would have been a bit more initial work for me, and could easily be missed in the flurry of Phabricator emails.
What do you think, any way to handle this situation better?

[ahal]

I think that's a good use case for it (me not being able to think of a reason points more to a lack of creativity on my part than anything else)!

I also see why it wouldn't be obvious what you intend there.. maybe if we had more consistent usage of the review flags this would come? I'd probably do the same thing you did, maybe also add a comment clarifying that @invitee's review is optional.

[staktrace]

Something else I try to pay attention to as an author (because I find it useful as a reviewer) is ensuring interdiffs are readable too. This involves a few things:
(a) whenever pushing up new versions of revisions with moz-phab, use the -m flag to include a message describing what changed. This can be something like "address review comments" or "add extra refactoring patch" or "rebase to tip"
(b) never mix rebase-to-tip updates with "address review comments" updates. Always do them in two separate steps, so that the reviewer has a clean interdiff to look at for the "address review comments" updates
(c) when pushing changes, try to only push the modified patches. i.e. if you have patches A, B, C and you make a change to B, git/hg will rebase C on top of the new B2 and give you C2. Many times you can just moz-phab submit -s B2 to upload the new version of B and not the new version of C. This reduces spam to the reviewer and lando should still be able to rebase C on top of B2 when landing.

[ahal]

I never considered that, but good points! I'll try and adopt A and B in my workflow. C makes sense too, but I use hg absorb a lot and often fail to remember which revisions need updating. I've been backed out over forgetting to submit modified revisions more times than I'd care to admit :). For that reason I've gotten in the habit of always submitting everything.. but maybe there's a balance I could strike.

[squelart]

(c) when pushing changes, try to only push the modified patches.

Like @ahal, I too am scared of missing something so I always push the full stack.
Really, Phabricator should be able to work out that nothing at all changed between two revisions of a patch, and not show it in the history 🤷‍♂️ "Never assign a human to do machine's job"