Excited to try this out.

Would be good to say "You can create a personal configuration file (e.g. in :file:$HOME/.reviewboardrc) with the following ...)
That helps if the reader is not familiar with our per-user config and they can easily find more info about it if needed. Especially for Windows users who don't have a home directory.

Typo: thtrough

I think there's an extra : here?

This reads kinda weird, the "... working about branches ..." part.
Might also benefit from splitting it up into two sentences: .. merge commits is ugly. When we're working ..."

Does rbt land support the --push option with jujutsu? If so we should mention that here too.

This method doesn't seem to ever return None, we always return 1 if p is None.

You can consolidate this to return super()...

Missing the Raises section in the docstring.

Missing the Raises section in the docstring.

We should wrap this in a try/except and raise an SCMError if we encounter a RunProcessError, like we do with the other methods that use run_process

This docstring should have a Raises section for the possible rbtools.utils.process.RunProcessError

Add , optional

Missing a Raises section in the docstring.

Add , optional

This docstring should have a Raises section for the possible rbtools.utils.process.RunProcessError

Here too.

Here too. And its missing Args and Returns.

I think its worth it to link to the per-user config page here https://www.reviewboard.org/docs/rbtools/latest/rbt/configuration/users/. You can use "personal configuration" as the link text.

The type needs to be updated here as well (get rid of Optional)

Ship It!

This is the only workflow guide that isn't organized into steps. It comes across more as general documentation for Jujutsu. While that's useful and can certainly fit into this guide, we should make sure this guide is Step-based.

I feel like this initial comment isn't going to age well. Eventually it won't be new, or it'll have been popular for a while, or it'll be defunct. Maybe describe the tool independent of recent history.

Two blank lines between content and reference definitions.

Typo: "want ask" -> "want to ask".

Almost commented that there's something missing here, until I read further. I think this means pxolvpnn from above? Can we be explicit about this somehow? Coming from someone unfamiliar with Jujutsu, this was confusing.

Given this logic, it seems we can avoid the second call if the first passes.

We shouldn't re-type any of these. The parent owns the type.

_local_path sorts after _has_multiple_remotes.

Here and below, we aren't caching this result. Should we be? Otherwise repeated calls would repeat these checks.

Are we guaranteed utf-8 from this file?
Also, for consistency, context variable should be fp.

Just in case, we might want to safeguard against a line with too many spaces crashing RBTools. Trust but verify.

Missing a period. Might also be nice to include more context in this error.

We're missing an explicit return None if this check fails.

Can we briefly document what this is for?

We should be asserting instead of casting. Removes an assumption and verifies.

Can we pass as keyword-arguments?

We should assert here too.

Same as above.

We have SCMClientCommitHistoryItem we can use. That also documents the keys.

We can use the type here too.

It'd be nice to end multi-line strings like this with the ) on the following line, matching multi-line lists and dictionaries. Gives more room for text and minimizes syntactical changes when modifying text.

Typo: entriely -> entirely

Can we do:

if not message:
    # comment
    message = 'No description set'

entry['commit_message'] = message

We should probably provide a message here. We have too many error messages from commands that are empty or confusing with little-to-no context today.
Also, these should be passed a str(e), I think, instead of e.
Same below.

Is this always true? I could see it if we were using a Jujutsu backend in RB, since any version supporting that would support empty files, but if using new RBTools with older Review Board, this would be incorrect.

We should assert instead of cast.

Same styling request as above re: multi-line strings.
Same below.

Here and elsewhere, the module path is now rbtools.diffs.patches.PatchAuthor.

In the vast majority of cases (all the cases, I'm pretty sure), we use %s formatting within localized txt. We should stay consistent. Here and below.

We should document both the True and the False. Especially since in this case, it's always False.
The comment has some important information here, and at least some of it (or the implications) should live in the docs for the function.

Let's make this keyword-only. Too many diff-related functions have had to be changed in important ways over the years, and positional arguments make that annoying.

These should take Sequence[str], since we're not modifying the lists.
They're also documented as optional, but require a value.

Description needs to be indented a level.
Should also use :command: for the jj.

Are we guaranteed utf-8 results?

Just a suggestion. This could be simplified to:

return (... or
        ... or
        self._get_remote_bookmark(tip))

Description needs to be indented a level.

Do we want to log any of this? What are the conditions in which it fails but we're okay with it? Can we document that here?

Same question here.

Same note as above re: multi-line strings.

Description needs to be indented a level.

This can use the standard run_process chain format.

We may want to return a Sequence here, unless we want this to be mutable.

Description needs to be indented a level.

Same as above re: Sequence.

Description needs to be indented a level.

Description needs to be indented a level.
Not going to flood the review with these, and I missed them above. Can you go through all Raises blocks and check?

As above (earlier in the review), it'd be nice to provide some more explicit errors with context in the error message, in case we get something useless here.

rb should be br.
It may not matter so much anymore, but it used to in earlier versions of Python. I remember hitting this all the time because I kept wanting to type rb. We should be consistent either way.

This is defined in the context, but used outside of it. Let's move it down to when we need it.
All of these should probably be moved out, really.

No need for the outer parens.

We've always been explicit about the utf-8. I don't mind changing away from it, but we should decide if we're doing that going forward for all new code. Might be worth a discussion in Slack to get on the same page.

Can this be one write?

We should use "Git" and not "git" when referring to the SCM instead of the command.

Same comment re: multi-line strings.
Also let's list the SHA we're missing.

No need for the parens.

checks sorts before filesystem.

We shouldn't redefine types in subclasses. This is owned by the parent.

"Jujutsu"

Can we make these keyword-only?

Given the alignment, this reads pretty poorly, here and below. The pattern we use everywhere else is:

message = ...

with self.assertRaisesMessage(exception, message):
    ...