Looks like a good patch. Could you add unit tests that make sure the constraint handles actually trigger on error?
Please mention in the commit message that this also documents bsearch_b.

The bsearch_b documentation is handled in a separate commit. It displays correctly for me here in Phabricator, but I’ll shortly update the description with the proposed commit messages, as we’ve agreed to use that convention to make reviews easier and clearer. And as for the tests, sure, I’m working on it; I do my best to submit the commit by tonight.

This looks reasonable. Please see comments for some small items to check.

I admit that I never knew about bsearch(3) before, but given a quick glance, this patch looks good.

I do have some questions about the summary on this review though. Seeing 3 separate commit sections, how do you intend to commit this?

I am asking, not because I think there is something inherently wrong, but because I want to see your life made easier.

These days, I have embraced the workflow of:

1. Working in a branch
2. So that you can pre-squash the commit as you iterate over review comments
3. And when all comments are satisfied and review is approved, ...
4. Then going to main, doing a pull, and then git arc stage

Because "git arc stage" will automate the process of pulling in the "Reviewed By" and "Differential Revision" so that you can:

1. Not have to worry about putting those into the final commit message nor have to worry about using interim placeholders
2. Avoid the worry of typos or mistakes
3. Focus on simply adding "(mentor)" on the appropriate reviewer names or adding "(previous revision)" to any reviewer that approved a previous revision but did not sign-off on the last/final revision.

Ultimately, I'd like to see your life simplified and get you into a continuous routine that works well for you.

I'd like to see the summary updated to be simplified to represent the commits.

I'm fully ready and willing to be told that this review represents multiple commits, but I am embracing a 1:1 commit-to-review flow myself, and wonder if that could not be transferred to your workflow.

I apologize in advance for any annoying extra work this may represent, but, again, I'm trying to make sure we optimize flow with available tooling.

I do have some questions about the summary on this review though. Seeing 3 separate commit sections, how do you intend to commit this?

For each piece of work I undertake, I usually write down the steps needed to reach the goal and break the process into one or several meaningful units (to keep the history clean, narratable, and easy to follow, blame, bisect, revert, and so on and so forth). Each unit becomes a separate commit that I have in mind during the process (though this might change if I notice something related part way through the work). I then create a new branch for it (e.g. libc/bsearch for this specific DR). I might add, parenthetically, I categorise my branches mostly by subsystem, which helps me better manage and predict conflicts across my different work streams.

I then submit the DR, and once my changes are approved and ready for commit, I clean up the commit messages and finalise anything left outstanding. When I'm confident nothing more needs editing, I run git pull --rebase freebsd main, resolve any conflicts, and then, for this DR as an example, git push freebsd libc/bsearch:main.

I've tried git arc, and although I like it a lot, even with some modifications it doesn't quite satisfy me yet, and I still need to work on it further before it becomes my daily driver for Phabricator. Or perhaps its philosophy is just different from what I've learnt to do, and all I need is to change my perspective. The thing is git arc works brilliantly for single-commit work, where only HEAD or a specific commit needs pushing, but for this exact reason I've been using plain arc instead, since it lets me create and/or update a DR more easily by just providing a commit range. That 1:1 mapping between commit and DR is great for keeping things simple and as small as possible, but when there's a flow to be narrated, each baby step tells part of the story and I think that makes both reviewing and following the journey easier, don't you think? But I know there's probably a lot I should change or can do better, so please flag anything that seems risky, like overkill, or that you think might help put my mind more at ease.

I am asking, not because I think there is something inherently wrong, but because I want to see your life made easier.

I know that, Devin, and I am really grateful for this.

I'm fully ready and willing to be told that this review represents multiple commits, but I am embracing a 1:1 commit-to-review flow myself, and wonder if that could not be transferred to your workflow.

Would you explain how you'd handle this DR as an example? I think it's possibly because I've just got so used to doing things this way that I'm struggling to understand how to follow a 1:1 style. I'd appreciate it if you could help me see it more clearly, if you think that's the case too.

I apologize in advance for any annoying extra work this may represent, but, again, I'm trying to make sure we optimize flow with available tooling.

Not at all! I'd much rather take the time to get this right than rush it. If it means we end up with a smoother flow, I'm all in. Ask away, dig in, criticise, fix, whatever it takes! I'm sure I’ll be all the better for it, so please know that I really appreciate all your help.

In D58876#1354646, @kfv wrote:

I do have some questions about the summary on this review though. Seeing 3 separate commit sections, how do you intend to commit this?

For each piece of work I undertake, I usually write down the steps needed to reach the goal and break the process into one or several meaningful units (to keep the history clean, narratable, and easy to follow, blame, bisect, revert, and so on and so forth). Each unit becomes a separate commit that I have in mind during the process (though this might change if I notice something related part way through the work). I then create a new branch for it (e.g. libc/bsearch for this specific DR). I might add, parenthetically, I categorise my branches mostly by subsystem, which helps me better manage and predict conflicts across my different work streams.

Thinking in steps on a branch named by subsystem is fine -- that part can stay. The trouble is only at land, when several git commits all claim one Differential. A review is the unit that was read and tested together; a commit on main is the unit someone will check out, bisect onto, or cherry-pick. If those are not the same object, Differential Revision: on any one commit does not point at "what this is." It points at a larger diff. That map does not go backward: from this review you cannot recover the three commits, and from any one of them you cannot recover the reviewed unit.

Blame/bisect/revert only stay honest if each cut can stand alone. If they can, they each deserved their own review -- which means their own branch, their own squashed commit, and their own Differential. If they cannot, they should not be separate objects on main.

I then submit the DR, and once my changes are approved and ready for commit, I clean up the commit messages and finalise anything left outstanding. When I'm confident nothing more needs editing, I run git pull --rebase freebsd main, resolve any conflicts, and then, for this DR as an example, git push freebsd libc/bsearch:main.

The rebase is the right instinct. Please keep that. What I want to change is the last mile.

git arc stage exists to take the approved commit from the work branch and cherry-pick it onto main. It does not pull, so the safe order is: go to main, update it, *then* stage. At that point HEAD is main, so git arc stage HEAD would stage the wrong object. Name the source branch instead -- and that only means what we want if the branch tip *is* the review, i.e. one squashed commit:

git checkout main
git pull
git arc stage libc/bsearch
git push freebsd main

Trailer edits happen in the editor stage opens -- that is when (mentor) goes on. Same linear history, without hand-building the trailers.

Please do not push commit-by-commit: that opens a window where main has only a prefix of a series and CI can fire on it. One push of three is atomic for the ref, but the middle commits still exist -- bisect and HEAD~1 can land on them, and a later MFC can take a subset we never reviewed or tested. That is the hole: every subset becomes a thing someone might pick, and the count only grows with each extra commit. Squashing to one commit on libc/bsearch closes it for this review.

git pull without --rebase is the other land-mine (a merge commit on the way to freebsd main). You can make rebase the default so you do not have to remember the flag:

git config --global pull.rebase true

(or the same without --global for src only). Then git pull freebsd main rebases. git pull --no-rebase if you ever truly want a merge (which should never go up to main, keep main linear).

I've tried git arc, and although I like it a lot, even with some modifications it doesn't quite satisfy me yet, and I still need to work on it further before it becomes my daily driver for Phabricator. Or perhaps its philosophy is just different from what I've learnt to do, and all I need is to change my perspective. The thing is git arc works brilliantly for single-commit work, where only HEAD or a specific commit needs pushing, but for this exact reason I've been using plain arc instead, since it lets me create and/or update a DR more easily by just providing a commit range. That 1:1 mapping between commit and DR is great for keeping things simple and as small as possible, but when there's a flow to be narrated, each baby step tells part of the story and I think that makes both reviewing and following the journey easier, don't you think? But I know there's probably a lot I should change or can do better, so please flag anything that seems risky, like overkill, or that you think might help put my mind more at ease.

The philosophy *is* a little different, and that is worth a change of perspective -- not because git arc refuses a range. git-arc(1) takes anything git-rev-parse(1) does. The trap is that a branch name is only the tip. That is not a limitation of stage; it is the 1:1 we want: one branch, one squashed commit, one review.

Where you stand decides which name to use. create, update, and diff happen *inside* the work branch, so HEAD is the review:

git arc create HEAD
git arc diff HEAD
git arc update HEAD

I added git arc diff for this. From the work branch it shows the difference between your commit and the Differential -- i.e. what git arc update would upload. Empty means they already match; non-empty is the delta. If you have rebased since the last update, the comparison can fail or look unrelated because it applies the review to HEAD~; that is expected, and a cue to update.

stage happens on main after a pull, so HEAD is main. The source has to be the branch name: git arc stage libc/bsearch.

Plain arc with a range is the other shape: one Differential covering the combined diff. That is how the hybrid happens -- several commits on main that all point at a single review of the bundle. Both tools take ranges; they do not mean the same thing. git arc's 1:1 is commit-to-review, not "squash the story into something tiny."

A real series should stay a series by staying 1:1: a new branch per unit, each one squashed commit, each its own Differential, each staged from main by name. What I want us to avoid is the hybrid. That is the risky bit, and it is the one I would like off the table.

I am asking, not because I think there is something inherently wrong, but because I want to see your life made easier.

I know that, Devin, and I am really grateful for this.

Likewise -- that is the job, and you are making it easy to do.

I'm fully ready and willing to be told that this review represents multiple commits, but I am embracing a 1:1 commit-to-review flow myself, and wonder if that could not be transferred to your workflow.

Would you explain how you'd handle this DR as an example? I think it's possibly because I've just got so used to doing things this way that I'm struggling to understand how to follow a 1:1 style. I'd appreciate it if you could help me see it more clearly, if you think that's the case too.

Yes. For this DR, 1:1 means: on libc/bsearch, squash to one commit, one message covering the whole change (document bsearch_b(), add bsearch_s(), add the tests). The story can still live in the commit message as paragraphs. git arc diff HEAD / git arc update HEAD as needed from that branch. Then on main:

git pull
git arc stage libc/bsearch
git push freebsd HEAD:main

That is the object that was accepted; landing it as three git commits would not be.

If a later piece of work really is three independent units, that is three branches, three reviews, git arc create HEAD on each, and git arc stage <branch> from main for each -- not one review split after the fact, and not several commits on one branch. Two consistent shapes (one branch per review, or one combined commit per review); the hybrid is the third thing, and I do not want that one.

What I do in my working branch when people have requested changes is:

1. make necessary changes
2. git commit --amend use this to either change files, change file contents, or alter commit message
3. If changing the commit message, update the review summary to reflect what the commit message is
4. git arc diff HEAD shows diff between new squashed commit and last diff sent to Phabricator
5. git arc update HEAD sends the new squashed commit to the review (does not touch the review summary)

NB: Empty output from diff means you can skip update

I apologize in advance for any annoying extra work this may represent, but, again, I'm trying to make sure we optimize flow with available tooling.

Not at all! I'd much rather take the time to get this right than rush it. If it means we end up with a smoother flow, I'm all in. Ask away, dig in, criticise, fix, whatever it takes! I'm sure I’ll be all the better for it, so please know that I really appreciate all your help.

Then let's do this one together: squash libc/bsearch to match the review, git arc stage libc/bsearch from an updated main so you see the trailer edit once, and push. Nothing about how you *think* the work in steps has to change -- only that the reviewed unit and the landed unit stay the same object.