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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 16, 2022, 08:51 UTC
Message-ID: <8o63pp64-4s00-1000-42s1-38so68398337@tzk.qr>
In-Reply-To: <220714.86mtdb1jmp.gmgdl@evledraar.gmail.com>

Hi Ævar,

On Thu, 14 Jul 2022, Ævar Arnfjörð Bjarmason wrote:

Show 33 quoted lines

> On Thu, Jul 14 2022, Johannes Schindelin wrote:
>
> > On Wed, 13 Jul 2022, Junio C Hamano wrote:
> >
> >> Æ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.
> >
> > This patch series already implements `git bisect` in the desired way:
> >
> > 	$ ./git bisect --invalid; echo $?
> > 	usage: git bisect [help|start|bad|good|new|old|terms|skip|next|reset|visualize|view|replay|log|run]
> > 	129
>
> It doesn't, the claim isn't that there's no way to have it return exit
> code 129 on *some* invalid usage. In this case we do the "right" thing.
>
> Rather that as noted in [1] there's other cases where we call die() and
> should call usage_msg_opt().

It would have been better to take the time to spell out clearly that you are taking offense in `git bisect start -h` not behaving in the way you think is the rule in Git: to print a _subcommand_ usage and exit with code 129.

However, this feedback fails to recognize the scope of this patch series.

The patch series' intention is not to fix anything that is currently broken. And it is already broken, my patch series does not introduce that breakage (and it would make more sense to address this breakage _after_ the conversion, to avoid doubling the effort): The current output of `git bisect start -h` shows the usage of `bisect--helper`!

Instead, the scope of the patch series is to finalize converting the `bisect` command to a full built-in, implemented in C, and avoiding the portability cost of running a POSIX shell script.

Note: I agree with you that it would be nice for `git bisect start -h` to
output a proper usage. There will be a time to discuss that, that time is
just simply not right now.

Since the scope is so different from what your feedback suggests, I have to admit that it taxes my patience to see that laser focus on aspects that are almost irrelevant compared to the aspects that should concern any good review of this series: the correctness of the conversion, with a heavy focus on the non-failure modes.

No user would care about the exit code of a failure mode (as long as it is non-zero), if there are regressions e.g. in how `git bisect start --good=Ævar --bad=Dscho` behaves.

So this hyper focus on what look like less relevant aspects is not only irritating, it actively distracts me, others and even yourself from the thorough review I would like to get: There have not been any thorough reviews of this patch series so far, and I think it is because of this here distraction.

The cost of this distraction is quite real: not only is there a performance penalty of running POSIX shell scripts on Windows, there are also problems with anti-malware disliking the way the POSIX emulation layer works that we currently have to use on Windows to run `git bisect`, which would be fixed by `bisect` being a full built-in. This distracting feedback that prevents a thorough code review delays that fix for those users.

To understand what I am aiming for, look at the deep analysis of the rot13 filter conversion from Perl to C in https://lore.kernel.org/git/4n20476q-6ssr-osp8-q5o3-p8ns726q4pn3@tzk.qr/, where I carefully compared the behavior of the scripted code with the C code that was designed to replace it.

At this point, it is good to recall Parkinson's law of triviality:

	Parkinson observed and illustrated that a committee whose job was
	to approve plans for a nuclear power plant spent the majority of
	its time with pointless discussions on relatively trivial and
	unimportant but easy-to-grasp issues, such as what materials to
	use for the staff bike-shed, while neglecting the less-trivial
	proposed design of the nuclear power plant itself, which is far
	more important but also a far more difficult and complex task to
	criticise constructively.

We've seen quite a few regressions as of recent that would have likely been prevented by thorough reviews that do not distract themselves with tangents, pet peeves and personal taste.

It would do good if we the reviewers on the Git mailing list took to heart that Git is a software that millions of users depend on, not just our toy to play with, and therefore the purpose of our reviews should aim to keep Git working and safe. We will achieve that only if we avoid what Parkinson called pointless discussions and instead put in the effort to provide high-quality feedback that helps improve the design and the correctness of the code.

In this instance, the discussion about exit codes and usage messages should be postponed, in favor of focusing on the actual scope of this patch series.

Ciao, Johannes