Re: js/bisect-in-c, was Re: What's cooking in git.git (Jul 2022, #03; Mon, 11)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 14, 2022, 00:22 UTC
Message-ID: <220714.86fsj4356l.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqtu7ldmrz.fsf@gitster.g>

On Wed, Jul 13 2022, Junio C Hamano wrote:

Show 28 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> I'm not claiming that we always use 129 when we're fed bad options etc.,
>> but rather that that's what parse_options() does, so at this point most
>> commands do that consistently.
>> 	
>> 	./git --blah >/dev/null 2>&1; echo $?
>> 	129
>> 	./git status --blah >/dev/null 2>&1; echo $?
>> 	129
>>
>> But yes, you can find exceptions still, e.g. try that with "git log" and
>> it'll return 128.
>
> Yup, that was my understanding as well.  We may have existing
> breakage that we shouldn't be actively imitating when we do not have
> to.
>
>> Which, I'm not saying should hold this series up, but just that having
>> reviewed it for a few iterations I'm not comfortable saying we haven't
>> missed something else, and given the nature of the past whack-a-mole
>> fixes it looks like you haven't really looked it either.
>
> "We haven't missed" is not something we can comfortably say, ever,
> aobut a series with any meaningful size.  I am not so worried about
> it, if it is your only worries.  Would it make it less likely to
> have introduced unintended bugs if we find a way to keep using
> parse-options?

I started writing something here, but found myself rewriting the last 3 paragraphs of [1] seen in the context below, so I'll just refer to that.

FWIW ejecting 11-14/16, i.e. these patches:

	- Turn `git bisect` into a full built-in
	- bisect: move even the command-line parsing to `bisect--helper`
	- bisect: teach the `bisect--helper` command to show the correct usage strings
	- bisect--helper: return only correct exit codes in `cmd_*()`

Yields a result that I've got no concerns about whatsoever, and reduces the diffstat from:

    7 files changed, 110 insertions(+), 207 deletions(-)

To just:

    4 files changed, 71 insertions(+), 67 deletions(-)

Which I think might be worth considering, similar to how the submodule migration is happening in multi-step fashion. I.e. advancing the parts that don't migrate it away from parse_options(), since I showed a way to keep using it in [1] (while changing less code).

Or just merge it down, up to you :)

Show 11 quoted lines

>> I'm referring to e.g. the thread ending at [3], where I pointed out that
>> "git <subcommand> -h" was broken, you apparently tested one of the
>> subcommands and concluded it wasn't broken, but clearly not all of them.
>>
>> Which, I'd be less concerned about if as pointed out in [1] we don't
>> have entirte bisect sub-commands that don't have any test coverage at
>> all, so whatever coverage we have probably has major gaps.
>>
>> 1. https://lore.kernel.org/git/220627.86mtdxhnwk.gmgdl@evledraar.gmail.com/
>> 2. https://lore.kernel.org/git/220523.865ylwzgji.gmgdl@evledraar.gmail.com/
>> 3. https://lore.kernel.org/git/220225.86ilt27uln.gmgdl@evledraar.gmail.com/