p9ufs: improve command line help

[abitrolly]

• print usage to stdout when command is not given
• do not mix usage help with errors
• show command description and args order in usage

$ ./p9ufs
p9ufs - local 9P2000.L server in userspace

usage: ./p9ufs [options] <bind-addr:port>

options:
  -root string
        root dir of file system to expose (default "/")
  -unix
        use unix domain socket instead of TCP
  -v    verbose logging
$ ./p9ufs 31
err binding: listen tcp: address 31: missing port in address
$ ./p9ufs -df
flag provided but not defined: -df
$ ./p9ufs -h
p9ufs - local 9P2000.L server in userspace

usage: ./p9ufs [options] <bind-addr:port>

options:
  -root string
        root dir of file system to expose (default "/")
  -unix
        use unix domain socket instead of TCP
  -v    verbose logging

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.50%. Comparing base (d48681e) to head (38b0a37).

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #94   +/-   ##
=======================================
  Coverage   58.50%   58.50%           
=======================================
  Files          37       37           
  Lines        4779     4779           
=======================================
  Hits         2796     2796           
  Misses       1776     1776           
  Partials      207      207           

Flag	Coverage Δ
macos-latest-unit	42.59% <ø> (ø)
ubuntu-latest-unit	41.82% <ø> (ø)
ubuntu-vm-amd64	58.64% <ø> (ø)
ubuntu-vm-arm	56.82% <ø> (-0.05%)	⬇️
ubuntu-vm-arm64	56.87% <ø> (ø)
windows-latest-unit	42.94% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[abitrolly]

@hugelgupf do these changes make sense? I can leave them in my fork, but it may be useful to have them there.

[djdv]

I attempted to break this up a little, and added some other changes on top.
How do you feel about these changes? 4c1a395
We could tweak it and incorporate parts into this PR if it seems sensible.

2 is conventionally used as a "help-text" & "incorrect usage" exit code for command-utilities, so I added that in the commit.
but this differs from Go's defaults behavior in flag.
I'm not sure which one makes more sense to stick with. But it's easy to swap this around if decided.

Any rationale for changing the output writer from stderr to stdout?
I assume this is for piping but I'm not sure if it makes sense to redirect error text to something besides stderr under normal circumstances.

[abitrolly]

@djdv in 4c1a395 I like separate error codes. The confusing part is that parsing and setting global vars values occurs in a function. Maybe wrapping it all in a struct would be better. I haven't tested the new code, but it seems to me it will print help after errors.

Maybe we can use https://pkg.go.dev/github.com/rogpeppe/go-internal/testscript to craft desired output. But I am not sure how to test binaries that are just compiled.

but this differs from Go's defaults behavior in flag.

flag usability is awful, and it won't be changed, because a promise is made.

Any rationale for changing the output writer from stderr to stdout?

Only help is written to stdout, for grepping. Errors still go to stderr.

[djdv]

There was a bad bug in my previous commit: https://github.com/hugelgupf/p9/compare/4c1a3950faedd7f36122ae91e0d19b2af57b8fa6..bcc32f1
That check is still unsafe, but is supposed to be checked earlier, on the correct reference. flag's global reference tripped me up.

---

abitrolly, I wrote some thoughts on this.

The confusing part is that parsing and setting global vars values occurs in a function. Maybe wrapping it all in a struct would be better.

For locality, the variables are defined and described in the scope they're used, and can be passed, rather than globally referenced.
We could break this out into a struct, but that would require adding a type + constructor like:
func parseArgs(arguments ...string) (settings, error)
and the references in the local scope change from someNameto settings.someName.
I think flag.FlagSet and the Parse methods already serve a similar purpose in this case, without requiring a new container type.
Although in my own code that does /not/ depend on flag, I sometimes use that pattern.
And it might make sense if the parameter set gets too large, however, the flagset's ability to create and reference variables makes it somewhat easy to hook into already.

But I am not sure how to test binaries that are just compiled.

If you checkout or apply that commit, you can run go run ./cmd/p9ufs -help to see if the output looks alright.
Or the same with go build ./cmd/p9ufs then just calling ./p9ufs -help.

I haven't tested the new code, but it seems to me it will print help after errors.

For parsing errors yes, but not for other errors (it could though). This is internal to flag in various places, for example: https://github.com/golang/go/blob/6ebb5f56d9ed35588970ce69cbad63508403bb8d/src/flag/flag.go#L1058

On my machine the output is like this:

> go run .\cmd\p9ufs -help
p9ufs - local 9P2000.L server in userspace

usage: p9ufs [flags] <bind-addr:port>

flags:
  -root string
    	root dir of file system to expose (default "/")
  -unix
    	use unix domain socket instead of TCP
  -v	verbose logging

> go run .\cmd\p9ufs -invalid
flag provided but not defined: -invalid
p9ufs - local 9P2000.L server in userspace

usage: p9ufs [flags] <bind-addr:port>

flags:
  -root string
    	root dir of file system to expose (default "/")
  -unix
    	use unix domain socket instead of TCP
  -v	verbose logging

The second command exits with status 2

>go run .\cmd\p9ufs invalid
err binding: listen tcp: address invalid: missing port in address

The third command exits with status 1

Maybe we can use https://pkg.go.dev/github.com/rogpeppe/go-internal/testscript to craft desired output.

I'd probably stick with go's standard test library for automated testing, but no strong opinion.
go test supports comparing the output already https://go.dev/blog/examples
as well as hooking TestMain.
But would require refactoring+adding code to support it.

In that commit, we also have control of the flagset's output which during testing could be some buffer/whatever that we use to compare if needed. (When flagSet.Usage is called, it will use the flagSet's output variable.)

flag usability is awful

Non-ideal in some cases for sure. I've tried replacing it but am not yet sure if my own attempts have been better yet. lol

Only help is written to stdout, for grepping. Errors still go to stderr.

That seems fair, but I'm also not familiar with the modern conventions.
I think historically lots of usage text was/is considered "error text", but making it output text / using stdout makes sense in the case of -help / misuse errors.

I highly doubt anyone/thing depends on the current behavior of the usage text going to stderr, but that might technically be a breaking change.

[abitrolly]

If you checkout or apply that commit, you can run go run ./cmd/p9ufs -help to see if the output looks alright.

I don't trust manual testing. It is too fragile, and hard to remember how the alright look should be.

Maybe we can use https://pkg.go.dev/github.com/rogpeppe/go-internal/testscript to craft desired output.

I'd probably stick with go's standard test library for automated testing, but no strong opinion.

I believe testscript is still in standard library. The repo hosts an improved version with a command line tool. I still don't know how to use without manually compiling binary first.

go test supports comparing the output already https://go.dev/blog/examples as well as hooking TestMain. But would require refactoring+adding code to support it.

Maybe it doesn't worth it. Just ship an improved version and then gradually enhance it.

flag usability is awful

Non-ideal in some cases for sure. I've tried replacing it but am not yet sure if my own attempts have been better yet. lol

There are quite a few alternatives, but I am haven't tested the overhead of replacing flag. Maybe sticking to this stdlib module is not an improvement at all, and just a huge waste of time.

I highly doubt anyone/thing depends on the current behavior of the usage text going to stderr, but that might technically be a breaking change.

These guys should have sent the patch upstream instead of forcing us to make workarounds for their "breaking change". )