OK, here's take 4! Responding to Junio's feedback, first: > That function [cmd_bisect__helper()] is supposed to be a thin shim > layer whose only reason of its existence is to serve as an interface > with the scripted version of "git bisect". When everything is > migrated from the shell script, cmd_bisect__helper() should disappear. > ... > This is going backwards, as far as I can tell. If anything, I'd > rather see cmd_bisect__helper() get fixed so that it does not parse > "--first-parent" (and similarly, "--no-checkout" that you imitated) > into first_parent_only (and no_checkout) variables and passed as > parameters to bisect_start(). Thanks for the explanation, Junio. (And for bearing with me while I gain familiarity with git's codebase.) Now that I've taken some time to examine how git-bisect.sh and cmd_bisect__helper() fit together, the correct approach is much more clear. I've added a commit (3/5 in this iteration) that updates cmd_bisect__helper() to defer parsing the --no-checkout flag. As no_checkout was also passed to bisect_next_all() [via implicitly checking for the existence of .git/BISECT_HEAD when calling bisect_next() in git-bisect.sh], I've removed that parameter and instead check for .git/BISECT_HEAD in bisect_next_all() to determine whether no-checkout mode is on. This means no-checkout mode can no longer be enabled by "git bisect--helper --next-all --no-checkout" - it can now only be turned on when running "git bisect start". But this doesn't change git bisect's public API, so this should be OK. Other than the changes suggested by Martin (summarized below), the only new change is removing the read_first_parent_option() wrapper function in favor of file_exists() from dir.h. --- Martin, thanks for your suggestions - I've moved the commit updating "git bisect run" tests to 1/5, updated the commit message, and included the changes you provided. (I held off on additional indentation corrections, as it kinda felt like unnecessary code churn/outside the scope of abstracting platform-specific details.) > Also, does "separate between <one thing>" make grammatical sense? > Should it be "separate between <two things>" or "separate [it] from > <one thing>"? Dunno.. I think "separate [it] from <one thing>" is correct, good catch! > (Signed-off-by: Martin Ågren <martin.agren@xxxxxxxxx>, FWIW.) I'm still getting used to the conventions - should I add your name as a signed-off-by tag, a thanks-to tag, or both? Aaron Lipman (5): t6030: modernize "git bisect run" tests rev-list: allow bisect and first-parent flags cmd_bisect__helper: defer parsing no-checkout flag bisect: introduce first-parent flag bisect: combine args passed to find_bisection() Documentation/git-bisect.txt | 13 +++- Documentation/rev-list-options.txt | 7 +- bisect.c | 79 +++++++++++++--------- bisect.h | 9 +-- builtin/bisect--helper.c | 23 ++++--- builtin/rev-list.c | 9 ++- git-bisect.sh | 2 +- revision.c | 3 - t/t6000-rev-list-misc.sh | 4 +- t/t6002-rev-list-bisect.sh | 45 +++++++++++++ t/t6030-bisect-porcelain.sh | 104 ++++++++++++++++------------- 11 files changed, 195 insertions(+), 103 deletions(-) -- 2.24.3 (Apple Git-128)
