Hi, On Tue, Aug 16, 2022 at 3:49 AM Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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@xxxxxxxxxxxxxxxxxxx> Junio: This link came up dead for me; I think the intended link was 220627.86ilolhnnn.gmgdl@xxxxxxxxxxxxxxxxxxx ? > > source: <pull.1132.v4.git.1656354677.gitgitgadget@xxxxxxxxx> > > 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@xxxxxxxxxxxxxx/. 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@xxxxxxxxx/ [2] https://lore.kernel.org/git/20220812150420.GA3790@xxxxxxxxxx/ [3] https://lore.kernel.org/git/1p04q351-9938-r0r7-snr6-9s8237s27459@xxxxxx/ [4] https://lore.kernel.org/git/8o63pp64-4s00-1000-42s1-38so68398337@xxxxxx/
