In D46321#1056465, @emaste wrote:

In D46321#1056451, @pstef wrote:

https://reviews.freebsd.org/P645 is a diff that teaches libedit how to parse it, by the way.

That change LGTM if you want to commit it.

I believe the problem was introduced in 5fe973734866 or ef0d94a3d34c where ^[[A is introduced. I think libedit doesn't have the code to parse that properly. Fortunately it can be replaced with \e.

I'd say that we shouldn't make any recommendations on how to use parts of a language in our manual. We should instead focus on facts, list them all and describe as they are. If there is some kind of interaction between parts of the language that is possibly counter-intuitive then we can and probably should indicate directly or indirectly that something requires consideration, but at the same time we should keep the manual free of judgement.

@jhb do you think the following is related?

Already committed.

FWIW, Chromium also complains about the lack of kcmp.

I'm not sure about how useful this is to me. I use ifconfig to take a glance at all interfaces and if I want to learn more specifically about the state of the wg interface, I just run wg. I know it's obvious, I just mean that I never missed this information when looking at ifconfig output. On the other hand, with this patch, the output is longer and somewhat less readable.

Thanks for the information and apologies for the mess.
For me the case where the filesystem name was wrong and mount complained about file not existing was more common and more annoying since it was easy to see that the designated mount point clearly existed.

What are the user-facing errors in various cases after this?

I'm not sure but it seems that on the changed lines you replaced space indentation with tabs.

As of stable/14 this no longer compiles for me due to

/usr/src/lib/libcmb/cmb.c:854:6: error: invalid argument type 'void' to unary expression
        if (!BN_zero(count))
            ^~~~~~~~~~~~~~~
/usr/src/lib/libcmb/cmb.c:1032:7: error: invalid argument type 'void' to unary expression
                if (!BN_zero(seq))
                    ^~~~~~~~~~~~~
2 errors generated.

1. -D doesn't imply -d anymore. I found that to be unnecessarily limiting, now users can decide on the visual format.
2. The usage output and the manual page are updated.

Add -D both.

Follow advice from Jamie and use -D up and -D down.

Address comments from Jilles.

If it's just an effort to synchronize with OpenBSD then I vote against this change. I intentionally removed this option years ago, so I have to assume that the difference here comes from the fact that OpenBSD didn't.
If, however, you want to restore this functionality for whatever reason, I will only make a face and accept it.

What are the functions? Generally, passing a null pointer to a standard string function invokes UB regardless of the passed size, and some compilers do make optimizations based on this assumption.

Reduce code duplication.
I was worried about some repetitive code here when adding the second source of commands (built-ins, after files from $PATH), but it became visibly worse when I started working on adding a third source (aliases). So the sooner I factor this out, the cleaner the future commits will be.

Address reviews.

builtincmd doesn't hold C strings, use memcmp().

This doesn't follow the Don't Repeat Yourself principle, but currently I don't have a better idea how to express this code.

My question about fprintf() remains unanswered, but it is not a blocker.

TODO: write the manpage

Minor style improvements, nitpicking and questions.

Please provide more diff context (diff -U999999).

Back out the previous update.
The -1 was there for the i++ that the loop always performs, so it's needed regardless of whether strftime() returned 0 or more.
Split the calculation that was there before into two parts, one of which just decrements i and makes a comment on why it's done.

Minutes after posting the previous version, I thought about handling the rare corner case when strftime() returns 0.

Simplify the code part a bit.
Also improve comments and some variable names.

When I was looking at this code today, I thought that the unconditional memccpy() is a bit wasteful in the \D{} case, because the one copied byte is a special case and when it's noticed, the copied byte is overwriten by strcpy() with "%X".
There is little to be gained in either performance or legibility of the code, but when I rewrote this locally to be less wasteful, it didn't look much worse, nor did it look much better.
Opinions?

In this iteration I tried to address all new feedback.
I also was annoyed by the current formatting of the file, so this is a diff against D37926 where I reduce indentation.

Drop PS4 support and update the manual page changes according to feedback.

In D37753#860082, @stevek wrote:

Hi, thanks for working on this.

LGTM FWIW

Should this version become the default? On the one hand, it has POLA violation potential, on the other hand, it's what the command does under another system.

Initial review.