From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mon, 29 Aug 2022 10:11:21 GMT
Subject: Re: [PATCH v5 11/16] bisect--helper: calling `bisect_state()` without an argument is a bug
Message-ID: <220829.86sflf2w57.gmgdl@evledraar.gmail.com>
In-Reply-To: <8a0adfe3867157102e75d53ed928603ad634b904.1661604264.git.gitgitgadget@gmail.com>


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.

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?