Re: js/bisect-in-c, was Re: What's cooking in git.git (Aug 2022, #05; Mon, 15)

[Elijah Newren <newren@gmail.com>]

Hi,

On Tue, Aug 16, 2022 at 3:49 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Junio,
>
> On Mon, 15 Aug 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>

Junio: This link came up dead for me; I think the intended link was
220627.86ilolhnnn.gmgdl@evledraar.gmail.com ?

> >  source: <pull.1132.v4.git.1656354677.gitgitgadget@gmail.com>
>
> I had another look at the thread and did not see any feedback that focuses
> on the actual scope of the patch series. Conversions from scripted parts
> of Git to built-ins are always a bit finicky (and hard to review, I
> admit).
>
> Therefore I would like to move the status to "needs review".
>
> I do not think that there are any major issues with it (Ævar's feedback
> notwithstanding, it focuses on tangents that should be addressed after the
> conversion, to avoid losing focus), but I would love to see a thorough
> review of the conversion to avoid obvious regressions like the one in the
> built-in interactive `add` I had to fix recently.

I reviewed it --
https://lore.kernel.org/git/CABPp-BEOX+zxR9-yyx-EaiOV-Z9yD0YP_Kwvu4iGB8enz40XXQ@mail.gmail.com/.
I looked over the subsequent iterations too, and they still look good
to me.

Could I vote for just merging it down, as-is?  As far as I can tell,
this series was stalled for 6-7 months over "doesn't use
parse_options() API", even though Dscho addressed that directly in v1
in one of his commit messages stating he had already investigated that
route and found it wanting, at least in its current state[1].  It
appears, from my reading, that using parse_options() API was insisted
upon...though it turns out that making parse_options() capable of
handling such things is at least another 20-patch series[2] (which may
not be enough either[3]).  Further, such changes, while likely
desirable for consistency among Git commands, would likely move us
away from "faithful conversion from shell to C", and thus is likely
better to be done as a separate step on top of the existing series
anyway[4].

If we merge down, as-is, then Ævar can go to town on it, possibly
using SZEDER's changes[2], and convert the new bisect code over to
using parse_options(), and show Dscho what he meant by how it could be
done.  Maybe Ævar will even demonstrate that it is quite simple to do.
Or, perhaps, Ævar will discover what Dscho did and agree that
parse_options() really can't handle that case.  I wouldn't be
surprised by either outcome.  Either way, I think it'd resolve the
issue much faster than going round and round in discussions.

And I think the result would be more to everyone's liking -- Dscho
gets to concentrate on the stuff he cares about (user experience,
converting shell to C faithfully), and Ævar gets to concentrate on the
stuff he cares about (internal code consistency, tree-wide
refactorings).  And in the meantime, the rest of us get to enjoy the
fruits of three separate GSoC projects plus Dscho's attempt to tie it
all together.  I think Dscho's Dscho's submission improves upon
current Git and is good enough to merge, even if there may be ways to
make it even better.

Anyway, just my $0.02.

[1] https://lore.kernel.org/git/515e86e20758ed7f5b4a8ce8f38bfbbac27ec42a.1643328752.git.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/20220812150420.GA3790@szeder.dev/
[3] https://lore.kernel.org/git/1p04q351-9938-r0r7-snr6-9s8237s27459@tzk.qr/
[4] https://lore.kernel.org/git/8o63pp64-4s00-1000-42s1-38so68398337@tzk.qr/