git-arc: Handle -h on subcommands and align usage with the man page

[dteske]

Summary

git-sh-setup treats -h as help against an empty USAGE, so
"git arc create -h" prints "usage: git arc". Handle -h before
sourcing it so every subcommand prints the real synopsis.

The create, stage, and update synopses showed optional commit-refs
while git-arc(1) and the code require them. Advertise -p parent on
create; the option was already implemented and documented.

Sort create sub-command option-arguments alphabetically in three
places: (1) synopsis from tool, (2) man-page synopsis, and (3)
man-page description.

Check for jq(1) / arc after checking for usage so -h always works.

While here, fix missing "local o" in gitarc__stage().

[adrian]

thanks! @markj what do you think?

[markj]

tools/tools/git/git-arc.sh

1050

I still don't see why the individual subcommand option parsers all handle -h. In my testing, it's not needed after you added this check. But ok, it's harmless.

[dteske]

tools/tools/git/git-arc.sh

1050

I still don't see why the individual subcommand option parsers all handle -h. In my testing, it's not needed after you added this check. But ok, it's harmless.

I still don’t understand what you are trying to say here.

Are you trying to say that the sub-commands don’t need to handle -h at all?

Or are you trying to say that it is acceptable to return the single line of “Usage: git arc” and nothing else when a sub-command is passed -h

I invite you to remove these lines or comment them out and see quite clearly that, for example, “git arc create -h” will result in “Usage: git arc”

I tried to explain quite clearly that the check of $# returns 1 when -h is passed because we do not shift the -h away, and that is precisely why we need this check.

Line 921 above does not trigger usage becuase $# is not zero. It is two.

[markj]

tools/tools/git/git-arc.sh

1050

I am saying that if I add just the block

for arg in "$@"; do
    case "$arg" in
    -h)
        err_usage
        ;;
    --)
        break
        ;;
    esac
done

that you added here, then git arc create -h prints the usage message. So I don't understand why gitarc__create() and all the other subcommand handlers now try to handle -h as well. It looks like dead code.

[dteske]

tools/tools/git/git-arc.sh

1050

I am saying that if I add just the block

for arg in "$@"; do
    case "$arg" in
    -h)
        err_usage
        ;;
    --)
        break
        ;;
    esac
done

that you added here, then git arc create -h prints the usage message. So I don't understand why gitarc__create() and all the other subcommand handlers now try to handle -h as well. It looks like dead code.

And where in this diff do you see the thing for which you are complaining?

Can you point to where that is in this diff?

[dteske]

tools/tools/git/git-arc.sh

1050

I thought I had removed them all. I don't know how those came back. That is my mistake. I clearly removed those checks previously. I must have rolled this review from a stale diff.

[dteske]

tools/tools/git/git-arc.sh

1050

I checked, my mistake. It was here https://reviews.freebsd.org/D59019?vs=184413&id=184578#toc where I had not removed all the instances.

[dteske]

tools/tools/git/git-arc.sh

1050

I've given this a lot of thought, and the quintessential problem here is that:

1. many sub-commands already use getopts and it is semantically correct to have them check for -h (it is actually wholly incorrect to be checking for -h prior to invoking the sub-command because without adding logic of which option flags take an argument, one cannot know if the -h that is witnessed is a request for -h or an argument to an option-flag specific to one or more sub-commands; for example, "git arc create -t -h" is a desire to pass -h to the "-t tag" usage).
2. The reason to want to check for -h after checking the sub-command is valid and before sourcing git-sh-setup is to "Do The Right Thing™" by avoiding the unnecessary source of git-sh-setup when usage is requested (in the same vein as avoiding checking for jq(1) or arc when -h is requested)

So after giving it considerable thought, I have decided that the overall best solution/approach is going to be to take all the commands that exist between sub-command validation and sub-command invocation and stuff them into a setup function that is only sourced after we have considered the sub-command specific usage requirements (read: getopts).

That way, we can ensure that we properly (a) avoid checking for error conditions before we know what is being requested (b) avoid unnecessarily sourcing git-sh-setup (b) know for certain in the face of having other options that take arguments when and when-not usage is requested

[dteske]

Refactor. While considering the value of checking for -h at the sub-
command level versus globally, it became clear that we could do one
better. Checking for -h globally is woefully incongruent with the
given sub-command's option-flags. To be concise: one cannot know
whether -h is a true request for usage or something that by POSIX
standards should be considered an argument. To avoid having to
teach the global check about all the possible getopts variations of
each sub-command, it was deemed most-appropriate to allow handing-
off the actual usage check to each sub-command. However, that left-
open the quagmire that was prematurely sourcing git-sh-setup where
I believe it to be much more correct to only source that (and check
for external dependencies) after one has determined that the user
has not requested the usage statement. Generally speaking, the user
should be able to get the usage statement without having to first
satisfy every dependency (wherein git-sh-setup is also considered
another dependency).

In short, this new approach makes -h a first-class UX experience.
We align wholly and completely the synopsis from the tool with that
of the man-page (in all manner of usage request variations) as well
as make it available without jumping through a lot of hoops.