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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 5349 bytes --]

Hi Ævar,

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

> 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