Re: [PATCH v5 11/16] bisect--helper: calling `bisect_state()` without an argument is a bug

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 5309 bytes --]

Hi Ævar,

On Mon, 29 Aug 2022, Ævar Arnfjörð Bjarmason wrote:

>
> On Sat, Aug 27 2022, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The `bisect_state()` function is now a purely internal function and must
> > be called with a valid state, everything else is a bug.
>
> I'm confused by the "is now purely an internal", when did that happen
> exactly? That wording is new in this v5.

Yes, it is new. It is part of that huge amount of work to not only convert
the script to a built-in but also "while at it" migrate the entire
`bisect--helper` on top of the subcommand API, as you specifically asked
for, and it was that ask that blocked the patch series which would
probably otherwise have been accepted as-is, with the subcommand migration
left as a follow-up patch series with a much narrower scope than the
current iteration.

As to when it happened exactly? In 07/16 of this patch series iteration,
as explained as part of the commit message:

	Note that a couple of `bisect_*()` functions are not converted into
	`cmd_bisect_*()` functions directly, as they have callers other than the
	`OPT_SUBCOMMAND()` one (and the original functions did not expect
	a subcommand name to be passed as `argv[0]`, unlike the convention for
	the `cmd_*()` functions. In those cases, we introduce wrapper functions
	`cmd_*()` that also call the original function.

I did not repeat in the commit message all details that the diff explains
much more eloquently, such as `cmd_bisect_state()` now being a wrapper
around `bisect_state()`.

> Before this series wasn't the only caller "internal" (git-bisect.sh) as
> well? From the CL:
>
>      -    bisect--helper: using `--bisect-state` without an argument is a bug
>      +    bisect--helper: calling `bisect_state()` without an argument is a bug
>
>      -    The `bisect--helper` command is not expected to be used directly by the
>      -    user. Therefore, it is a bug if it receives no argument to the
>      -    `--bisect-state` command mode, not a user error. Which means that we
>      -    need to call `BUG()` instead of `die()`.
>      +    The `bisect_state()` function is now a purely internal function and must
>      +    be called with a valid state, everything else is a bug.
>
> Before the migration to OPT_SUBCOMMAND earlier in this series:
>
> 	$ ./git bisect--helper state
> 	usage: git bisect--helper --bisect-reset [<commit>]
> 	   or: git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]
> 	   or: git bisect--helper --bisect-start [--term-{new,bad}=<term> --term-{old,good}=<term>] [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]
> 	   or: git bisect--helper --bisect-next
> 	   or: git bisect--helper --bisect-state (bad|new) [<rev>]
> 	   or: git bisect--helper --bisect-state (good|old) [<rev>...]
> 	   or: git bisect--helper --bisect-replay <filename>
> 	   or: git bisect--helper --bisect-skip [(<rev>|<range>)...]
> 	   or: git bisect--helper --bisect-visualize
> 	   or: git bisect--helper --bisect-run <cmd>...
>
> 	    --bisect-reset        reset the bisection state
> 	    --bisect-terms        print out the bisect terms
> 	    --bisect-start        start the bisect session
> 	    --bisect-next         find the next bisection commit
> 	    --bisect-state        mark the state of ref (or refs)
> 	    --bisect-log          list the bisection steps so far
> 	    --bisect-replay       replay the bisection process from the given file
> 	    --bisect-skip         skip some commits for checkout
> 	    --bisect-visualize    visualize the bisection
> 	    --bisect-run          use <cmd>... to automatically bisect
>
> After that:
>
> 	$ ./git bisect--helper state
> 	fatal: need at least one argument
>
> 	usage: git bisect (good|bad) [<rev>...]
>
> So intra-series we were showing the wrong SYNOPSIS for this
> internal-only command. I don't think that matters per-se (and the
> end-state fixes it up), but doesn't it point to some ordering oddity
> here?
>
> AFAICT we couldn't call "state" without an argument from git-bisect.sh
> before, and that's the only (and internal) caller, so shouldn't this
> BUG() come earlier?

Yes, it could come earlier. Or later. It is part of some follow-up patches
that need to come after 07/16, in whatever order.

I appreciate that you want to help.

My concern is that by having to focus on answering such questions that I
consider a thorough review of the iteration to answer handily, I cannot
spend the same time and focus on preventing bugs I consider a lot more
critical. We saw some bug reports about the built-in `add -i` recently,
for example, that could have been prevented if the focus of the code
review was not so much on details that the end user won't ever see (such
as the order of patches or whether to broaden the scope and size of a
patch series instead of leaving follow-up work to subsequent patch
series), and more on unintentional changes that the users very much
experience, and not in a good way. I would appreciate it a lot if we could
focus first and foremost on preventing bugs cause problems to Git's users.

Thank you,
Johannes