breaking(but): make name validation in but branch new strict

[slarse]

This solves several problems in but branch new:

1. The command would previously normalize branch names automatically, which is unexpected for a CLI. If you type but branch new 'my branch' and get a success back, you expect there to be a branch called 'my branch'. But the actual name of the new branch was normalized to 'my-branch' (although the output would lie and say it created my branch). The new behavior is to reject any non-normalized name and print hint with the normalized name instead.
2. Branch creation was idempotent for branches already applied in the workspace, which was very confusing as the CLI claimed to have successfully created a new branch while in fact it had done nothing. Such branch creation is now clearly rejected, which also falls in line with how Git handles such a situation.
3. Various obtuse error messages have been improved here. For example, when creating a new branch with a name that already existed and anchoring it to anywhere, you'd get an error like this::

Error: The reference "refs/heads/<name>" should have content b54c2567115498ffcb87b31a4f1f95f0bead2546, actual content was 6078e6bb61d3b1ac4f35f57f05e2cce3382359ff

The new error just says that the branch already exists.

The rest of this PR body is more of a writeup than anything else, but I've done a great deal of thinking about how to provide good user feedback over the past few days and I think this is a good context to share it in.

Better error messages side quest

Continuing down the path of better error messages to help users out, I've introduced the ability to specify argument name and argument value separately. This allows us to have a standardized way of saying "hey, you done goofed on this argument". I'm not yet convinced exactly when and how to use all this, but it's a start.

For example, when the input is malformed (i.e. the equivalent of a 400 Bad Request HTTP response), I think it's important to clearly connect the dots between the input argument and the badness and try to help the user (or agent) rectify the issue, like this:

Note: Whether its helpful to name the positional argument in the output is something I'm still having an internal debate about. It can certainly be helpful if there are multiple positionals, but for just one it's rather questionable.

The formatting here can probably also be improved, but it's a start, and the point is mostly to standardize what the errors look like at this point.

However, when the branch name is valid but the state of the repository doesn't allow it to be created (i.e. the equivalent of a 409 Conflict HTTP response), I think we can be much more concise with the amount of information in the output, like so:

Look before you leap

This error handling is LBYL-based, rather than "better to ask for forgiveness than permission" using the new PreconditionFailed code. To a large extent I think we'll need to do LBYL-checks for really solid error messages. Here are a few reasons.

Not enough information from the underlying implementation

We simply don't have enough error information from the internals to directly correlate the error to the user's input. Even if the response code is PreconditionFailed, we don't know what precondition. How can we tell if it's the name that's bad, or if it's the anchor?

PreconditionFailed does not imply user error

It implies caller error. A trivial example in this piece of code is that we create a canned branch name if the user doesn't pass a branch name. If that fails a precondition, it's still a system error, not a user error. While it's unlikely that we'd create an invalid branch name, it is logically possible and only meant to illustrate the fallacy of assuming that a failing precondition is the user's fault.

What's worse is that a PreconditionFailed code only directly implies caller error to the direct caller, as the function that emits the code can only know about its direct inputs. In fact, as long as it's possible that we generate or manipulate input that can cause a PreconditionFailed, the code itself doesn't really add much information to the application as a whole. A caller can never know if the code was emitted from the directly called function, or from a function further down where the inputs aren't necessarily directly controlled by the original caller.

To summarize, it is not sound to assume that PreconditionFailed implies user error, or even caller error more than one stack frame away.

PreconditionFailed is a moving target

Right now, PreconditionFailed can only be emitted when the branch name is bad. If you build the code around that assumption, adding another PreconditionFailed cause in the internals breaks that assumption without there being any mechanism to find out from the call site.

There are downsides to LBYL, too

This is not to say that LBYL error handling is perfect.

• It adds overhead - because the checks should absolutely be performed again by the internals.
• It may be too strict and disallow use that should be allowed
• It adds more code to maintain

But to provide that terrific user feedback, I firmly believe at this point that we need to do input validation immediately, despite these drawbacks. We should also not attempt to do perfect input validation - if we can provide great user feedback for most invalid inputs, and produce obtuse errors for some, that's still a win.

Further notes

• This replaces fix(but): print normalized name after creating branch #13159
• The same name validation should be applied to renaming branches with but reword, and will thereby replace fix(but): print normalized name after branch rename #13160
• There's still a quirk I don't like: but branch new refs/heads/my-branch creates the branch my-branch. This is inconsistent with Git, which would literally create the branch refs/heads/my-branch (i.e. with full name refs/heads/refs/heads/my-branch).

[Byron]

That's a great initiative and I think it will make the CLI much better. I wasn't aware of LBYL, everything seems to have cool acronyms :D.

PS: This message is ignorant of the PR content itself, what's below is just some things that stood out to me.

[Byron]

I am still choking on this a little, but wouldn't if it was something like this:

Suggested change

	return BadInput::new("Invalid branch name")
	.arg_name("<BRANCH_NAME>")
	.arg_value(user_provided_branch_name)
	.hint(format!("Try '{normalized}' instead"))
	.into_cli_result();
	return Err(BadInput::new("Invalid branch name")
	.arg_name("<BRANCH_NAME>")
	.arg_value(user_provided_branch_name)
	.hint(format!("Try '{normalized}' instead"))
	.into());

I am pretty sure that this wouldn't format nicely, so maybe it's just something to get used to.
What might be nicer is a macro akin to bail!(), but more like this (I don't know if this is possible, but…)

Suggested change

	return BadInput::new("Invalid branch name")
	.arg_name("<BRANCH_NAME>")
	.arg_value(user_provided_branch_name)
	.hint(format!("Try '{normalized}' instead"))
	.into_cli_result();
	bad_input!("Invalid branch name", <BRANCH_NAME> = user_provided_branch_name, "Try '{normalized}' instead");

[Byron]

And if you want to beef up the CLI as much as it seems like, I think it's worth investing into ergonomics.

[Byron]

Avoid passing ctx down to utilities, it's an antipattern. The god object should stay on top, with specifics passed down. Here repo is enough, and it's much clearer that way as well.