Re: [PATCH v5 11/16] bisect--helper: calling `bisect_state()` without an argument is a bug
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Aug 30, 2022, 14:47 UTC
- Message-ID
- <5594qo11-716s-61qn-2rn9-q114199sp3p0@tzk.qr>
- In-Reply-To
- <220829.86sflf2w57.gmgdl@evledraar.gmail.com>
Hi Ævar,
On Mon, 29 Aug 2022, Ævar Arnfjörð Bjarmason wrote:
Show 10 quoted lines
> > 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()`.
Show 53 quoted lines
> 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