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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Jul 13 2022, Johannes Schindelin wrote:

> Hi Junio,
>
> On Tue, 12 Jul 2022, Junio C Hamano wrote:
>
>> * js/bisect-in-c (2022-06-27) 16 commits
>>  - bisect: no longer try to clean up left-over `.git/head-name` files
>>  - bisect: remove Cogito-related code
>>  - 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_*()`
>>  - bisect--helper: move the `BISECT_STATE` case to the end
>>  - bisect--helper: make `--bisect-state` optional
>>  - bisect--helper: align the sub-command order with git-bisect.sh
>>  - bisect--helper: using `--bisect-state` without an argument is a bug
>>  - bisect--helper: really retire `--bisect-autostart`
>>  - bisect--helper: really retire --bisect-next-check
>>  - bisect--helper: retire the --no-log option
>>  - bisect: avoid double-quoting when printing the failed command
>>  - bisect run: fix the error message
>>  - bisect: verify that a bogus option won't try to start a bisection
>>
>>  Final bits of "git bisect.sh" have been rewritten in C.
>>
>>  Expecting a (hopefully final) reroll.
>>  cf. <20627.86ilolhnnn.gmgdl@evledraar.gmail.com>
>
> I did not find that one, but I found
> https://lore.kernel.org/git/220627.86ilolhnnn.gmgdl@evledraar.gmail.com/
>
> And that claims that Git has a convention to universally exit with code
> 129 when options are passed with incorrect values.
>
> That claim does not survive even minimal contact with Git's source
> code, though.

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.

Your series migrates the bisect--helper.c away from parse_options() in a
a way that I don't think is necessary, but leaving that aside mimicking
the exit codes we'd get from parse_options() for those cases you're
handling in your custom parsing seems like a good thing.

> If I find some time, I will respond to that mail, but a reroll is actually
> unnecessary.

There's some more in [1], but there's at least one outstanding bug in
this series, i.e. that you're moving away from parse_options(), but
forgot to change the corresponding flag in git.c for the
built-in. That's then used by the completion mechanism.

But as noted in [2] and more recently in [1] I'm most concerned about us
having outstanding bugs due to past iterations of this having played
whack-a-mole in fixing specific edge cases I found, but not gone back
and added missing test coverage for the series beforehand.

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.

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/