| Message ID | 20200923170915.21748-1-chriscool@tuxfamily.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bisect: don't use invalid oid as rev when starting | expand |
Christian Couder <christian.couder@gmail.com> writes: > In 06f5608c14 (bisect--helper: `bisect_start` shell function > partially in C, 2019-01-02), we changed the following shell > code: Wow, that's an ancient breakage (goes back to 2.21 or something). Thanks; will queue. > > - rev=$(git rev-parse -q --verify "$arg^{commit}") || { > - test $has_double_dash -eq 1 && > - die "$(eval_gettext "'\$arg' does not appear to be a valid revision")" > - break > - } > - revs="$revs $rev" > > into: > > + char *commit_id = xstrfmt("%s^{commit}", arg); > + if (get_oid(commit_id, &oid) && has_double_dash) > + die(_("'%s' does not appear to be a valid " > + "revision"), arg); > + > + string_list_append(&revs, oid_to_hex(&oid)); > + free(commit_id); > > In case of an invalid "arg" when "has_double_dash" is false, the old > code would "break" out of the argument loop. > > In the new C code though, `oid_to_hex(&oid)` is unconditonally > appended to "revs". This is wrong first because "oid" is junk as > `get_oid(commit_id, &oid)` failed and second because it doesn't break > out of the argument loop. > > Not breaking out of the argument loop means that "arg" is then not > treated as a path restriction (which is wrong). > > Signed-off-by: Christian Couder <chriscool@tuxfamily.org> > --- > This is a bug fix for the bug Miriam talks about in: > > https://lore.kernel.org/git/20200923072740.20772-1-mirucam@gmail.com/ > > and: > > https://lore.kernel.org/git/CAN7CjDDVp_i7dhpbAq5zrGW69nE6+SfivJQ-dembmu+WyqKiQQ@mail.gmail.com/ > > builtin/bisect--helper.c | 14 +++++++++----- > t/t6030-bisect-porcelain.sh | 7 +++++++ > 2 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 7dcc1b5188..538fa6f16b 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -486,12 +486,16 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc) > return error(_("unrecognized option: '%s'"), arg); > } else { > char *commit_id = xstrfmt("%s^{commit}", arg); > - if (get_oid(commit_id, &oid) && has_double_dash) > - die(_("'%s' does not appear to be a valid " > - "revision"), arg); > - > - string_list_append(&revs, oid_to_hex(&oid)); > + int res = get_oid(commit_id, &oid); > free(commit_id); > + if (res) { > + if (has_double_dash) > + die(_("'%s' does not appear to be a valid " > + "revision"), arg); > + break; > + } else { > + string_list_append(&revs, oid_to_hex(&oid)); > + } > } > } > pathspec_pos = i; > diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh > index b886529e59..cb645cf8c8 100755 > --- a/t/t6030-bisect-porcelain.sh > +++ b/t/t6030-bisect-porcelain.sh > @@ -82,6 +82,13 @@ test_expect_success 'bisect fails if given any junk instead of revs' ' > git bisect bad $HASH4 > ' > > +test_expect_success 'bisect start without -- uses unknown stuff as path restriction' ' > + git bisect reset && > + git bisect start foo bar && > + grep foo ".git/BISECT_NAMES" && > + grep bar ".git/BISECT_NAMES" > +' > + > test_expect_success 'bisect reset: back in the master branch' ' > git bisect reset && > echo "* master" > branch.expect &&
Hi Christian, On Wed, 23 Sep 2020, Christian Couder wrote: > In 06f5608c14 (bisect--helper: `bisect_start` shell function > partially in C, 2019-01-02), we changed the following shell > code: > > - rev=$(git rev-parse -q --verify "$arg^{commit}") || { > - test $has_double_dash -eq 1 && > - die "$(eval_gettext "'\$arg' does not appear to be a valid revision")" > - break > - } > - revs="$revs $rev" > > into: > > + char *commit_id = xstrfmt("%s^{commit}", arg); > + if (get_oid(commit_id, &oid) && has_double_dash) > + die(_("'%s' does not appear to be a valid " > + "revision"), arg); > + > + string_list_append(&revs, oid_to_hex(&oid)); > + free(commit_id); > > In case of an invalid "arg" when "has_double_dash" is false, the old > code would "break" out of the argument loop. > > In the new C code though, `oid_to_hex(&oid)` is unconditonally > appended to "revs". This is wrong first because "oid" is junk as > `get_oid(commit_id, &oid)` failed and second because it doesn't break > out of the argument loop. > > Not breaking out of the argument loop means that "arg" is then not > treated as a path restriction (which is wrong). Good catch! > This is a bug fix for the bug Miriam talks about in: > > https://lore.kernel.org/git/20200923072740.20772-1-mirucam@gmail.com/ > > and: > > https://lore.kernel.org/git/CAN7CjDDVp_i7dhpbAq5zrGW69nE6+SfivJQ-dembmu+WyqKiQQ@mail.gmail.com/ Thank you for clarifying. > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 7dcc1b5188..538fa6f16b 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -486,12 +486,16 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc) > return error(_("unrecognized option: '%s'"), arg); > } else { > char *commit_id = xstrfmt("%s^{commit}", arg); > - if (get_oid(commit_id, &oid) && has_double_dash) > - die(_("'%s' does not appear to be a valid " > - "revision"), arg); > - > - string_list_append(&revs, oid_to_hex(&oid)); > + int res = get_oid(commit_id, &oid); > free(commit_id); > + if (res) { > + if (has_double_dash) > + die(_("'%s' does not appear to be a valid " > + "revision"), arg); > + break; > + } else { > + string_list_append(&revs, oid_to_hex(&oid)); > + } I would find that a lot easier to read if it was reordered thusly: if (!get_oidf(&oid, "%s^{commit}", arg)) string_list_append(&revs, oid_to_hex(&oid)); else if (!has_double_dash) break; else die(_("'%s' does not appear to be a valid " revision"), arg); And it would actually probably make sense to replace the `get_oid()` by `get_oid_committish()` in the first place. > } > } > pathspec_pos = i; > diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh > index b886529e59..cb645cf8c8 100755 > --- a/t/t6030-bisect-porcelain.sh > +++ b/t/t6030-bisect-porcelain.sh > @@ -82,6 +82,13 @@ test_expect_success 'bisect fails if given any junk instead of revs' ' > git bisect bad $HASH4 > ' > > +test_expect_success 'bisect start without -- uses unknown stuff as path restriction' ' s/stuff/arg/ maybe? The rest of the patch looks good to me. Thank you, Dscho > + git bisect reset && > + git bisect start foo bar && > + grep foo ".git/BISECT_NAMES" && > + grep bar ".git/BISECT_NAMES" > +' > + > test_expect_success 'bisect reset: back in the master branch' ' > git bisect reset && > echo "* master" > branch.expect && > -- > 2.28.0.587.g1c7fdf1d8b > >
Hi Christian, On Wed, 23 Sep 2020, Johannes Schindelin wrote: > On Wed, 23 Sep 2020, Christian Couder wrote: > > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > > index 7dcc1b5188..538fa6f16b 100644 > > --- a/builtin/bisect--helper.c > > +++ b/builtin/bisect--helper.c > > @@ -486,12 +486,16 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc) > > return error(_("unrecognized option: '%s'"), arg); > > } else { > > char *commit_id = xstrfmt("%s^{commit}", arg); > > - if (get_oid(commit_id, &oid) && has_double_dash) > > - die(_("'%s' does not appear to be a valid " > > - "revision"), arg); > > - > > - string_list_append(&revs, oid_to_hex(&oid)); > > + int res = get_oid(commit_id, &oid); > > free(commit_id); > > + if (res) { > > + if (has_double_dash) > > + die(_("'%s' does not appear to be a valid " > > + "revision"), arg); > > + break; > > + } else { > > + string_list_append(&revs, oid_to_hex(&oid)); > > + } > > I would find that a lot easier to read if it was reordered thusly: > > if (!get_oidf(&oid, "%s^{commit}", arg)) > string_list_append(&revs, oid_to_hex(&oid)); > else if (!has_double_dash) > break; > else > die(_("'%s' does not appear to be a valid " > revision"), arg); > > And it would actually probably make sense to replace the `get_oid()` by > `get_oid_committish()` in the first place. I verified that this actually works, and figured out that we can save yet another indentation level (as well as avoid awfully long lines): -- snipsnap -- From f673cea53e046774847be918f4023430e56bf6cb Mon Sep 17 00:00:00 2001 From: Christian Couder <christian.couder@gmail.com> Date: Wed, 23 Sep 2020 19:09:15 +0200 Subject: [PATCH] bisect: don't use invalid oid as rev when starting In 06f5608c14 (bisect--helper: `bisect_start` shell function partially in C, 2019-01-02), we changed the following shell code: rev=$(git rev-parse -q --verify "$arg^{commit}") || { test $has_double_dash -eq 1 && die "$(eval_gettext "'\$arg' does not appear to be a valid revision")" break } revs="$revs $rev" into: char *commit_id = xstrfmt("%s^{commit}", arg); if (get_oid(commit_id, &oid) && has_double_dash) die(_("'%s' does not appear to be a valid " "revision"), arg); string_list_append(&revs, oid_to_hex(&oid)); free(commit_id); In case of an invalid "arg" when "has_double_dash" is false, the old code would "break" out of the argument loop. In the new C code though, `oid_to_hex(&oid)` is unconditonally appended to "revs". This is wrong first because "oid" is junk as `get_oid(commit_id, &oid)` failed and second because it doesn't break out of the argument loop. Not breaking out of the argument loop means that "arg" is then not treated as a path restriction (which is wrong). Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- builtin/bisect--helper.c | 17 ++++++----------- t/t6030-bisect-porcelain.sh | 7 +++++++ 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 93e855271b9..d11d4c9bbb5 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -660,18 +660,13 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a terms->term_bad = xstrdup(arg); } else if (starts_with(arg, "--")) { return error(_("unrecognized option: '%s'"), arg); - } else { - char *commit_id = xstrfmt("%s^{commit}", arg); - if (get_oid(commit_id, &oid) && has_double_dash) { - error(_("'%s' does not appear to be a valid " - "revision"), arg); - free(commit_id); - return BISECT_FAILED; - } - + } else if (!get_oid_committish(arg, &oid)) string_list_append(&revs, oid_to_hex(&oid)); - free(commit_id); - } + else if (has_double_dash) + die(_("'%s' does not appear to be a valid " + "revision"), arg); + else + break; } pathspec_pos = i; diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index b886529e596..d6b4bca482a 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -82,6 +82,13 @@ test_expect_success 'bisect fails if given any junk instead of revs' ' git bisect bad $HASH4 ' +test_expect_success 'bisect start without -- uses unknown arg as pathspec' ' + git bisect reset && + git bisect start foo bar && + grep foo ".git/BISECT_NAMES" && + grep bar ".git/BISECT_NAMES" +' + test_expect_success 'bisect reset: back in the master branch' ' git bisect reset && echo "* master" > branch.expect && -- 2.28.0.windows.1.18.g5300e52e185
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Christian, > ... > -- snipsnap -- > From f673cea53e046774847be918f4023430e56bf6cb Mon Sep 17 00:00:00 2001 > From: Christian Couder <christian.couder@gmail.com> > Date: Wed, 23 Sep 2020 19:09:15 +0200 > Subject: [PATCH] bisect: don't use invalid oid as rev when starting > ... > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 93e855271b9..d11d4c9bbb5 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c Unfortunately this does not apply to the broken commit or 'master' or anywhere else, it seems (no such blob as 93e855271b9 found at the path). It is better to make it applicable at least to 'master'. Making it also apply to 'maint' is optional, I would say, as the bug it fixes is not so critical. Thanks.
On Wed, Sep 23, 2020 at 11:39 PM Junio C Hamano <gitster@pobox.com> wrote: > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > From f673cea53e046774847be918f4023430e56bf6cb Mon Sep 17 00:00:00 2001 > > From: Christian Couder <christian.couder@gmail.com> > > Date: Wed, 23 Sep 2020 19:09:15 +0200 > > Subject: [PATCH] bisect: don't use invalid oid as rev when starting > > ... > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > > index 93e855271b9..d11d4c9bbb5 100644 > > --- a/builtin/bisect--helper.c > > +++ b/builtin/bisect--helper.c > > Unfortunately this does not apply to the broken commit or 'master' > or anywhere else, it seems (no such blob as 93e855271b9 found at the > path). > > It is better to make it applicable at least to 'master'. Making it > also apply to 'maint' is optional, I would say, as the bug it fixes > is not so critical. Sorry, I don't know what happened. It seemed to me that my branch was based on master, but maybe I did something wrong. Hopefully the V2 I just sent will be better anyway.
Christian Couder <christian.couder@gmail.com> writes: > On Wed, Sep 23, 2020 at 11:39 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> >> > From f673cea53e046774847be918f4023430e56bf6cb Mon Sep 17 00:00:00 2001 >> > From: Christian Couder <christian.couder@gmail.com> >> > Date: Wed, 23 Sep 2020 19:09:15 +0200 >> > Subject: [PATCH] bisect: don't use invalid oid as rev when starting >> > ... >> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c >> > index 93e855271b9..d11d4c9bbb5 100644 >> > --- a/builtin/bisect--helper.c >> > +++ b/builtin/bisect--helper.c >> >> Unfortunately this does not apply to the broken commit or 'master' >> or anywhere else, it seems (no such blob as 93e855271b9 found at the >> path). >> >> It is better to make it applicable at least to 'master'. Making it >> also apply to 'maint' is optional, I would say, as the bug it fixes >> is not so critical. > > Sorry, I don't know what happened. It seemed to me that my branch was > based on master, but maybe I did something wrong. Oh, you didn't do anything wrong. What was queued was your patch which applied cleanly to maint, master and the old version that brought the breakage into the codebase. I think I queued it on maint-2.25 or something sufficiently old, but as I said, a patch that applies to 'master' would be good enough. What I had trouble applying to see how it improves upon yours was the one from Dscho. Thanks.
Hi Christian & Junio, On Thu, 24 Sep 2020, Christian Couder wrote: > On Wed, Sep 23, 2020 at 11:39 PM Junio C Hamano <gitster@pobox.com> wrote: > > > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > > > From f673cea53e046774847be918f4023430e56bf6cb Mon Sep 17 00:00:00 2001 > > > From: Christian Couder <christian.couder@gmail.com> > > > Date: Wed, 23 Sep 2020 19:09:15 +0200 > > > Subject: [PATCH] bisect: don't use invalid oid as rev when starting > > > ... > > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > > > index 93e855271b9..d11d4c9bbb5 100644 > > > --- a/builtin/bisect--helper.c > > > +++ b/builtin/bisect--helper.c > > > > Unfortunately this does not apply to the broken commit or 'master' > > or anywhere else, it seems (no such blob as 93e855271b9 found at the > > path). > > > > It is better to make it applicable at least to 'master'. Making it > > also apply to 'maint' is optional, I would say, as the bug it fixes > > is not so critical. > > Sorry, I don't know what happened. It seemed to me that my branch was > based on master, but maybe I did something wrong. > > Hopefully the V2 I just sent will be better anyway. FWIW I was working off of Miriam's `git-bisect-work-part2-v8` branch at https://gitlab.com/mirucam/git.git. I'm happy with Christian's v2 (with or without the indentation fixes I suggested). Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> Hopefully the V2 I just sent will be better anyway. > > FWIW I was working off of Miriam's `git-bisect-work-part2-v8` branch at > https://gitlab.com/mirucam/git.git. > > I'm happy with Christian's v2 (with or without the indentation fixes I > suggested). Thanks, both of you. The updated one does look good.
Junio C Hamano <gitster@pobox.com> writes: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > >>> Hopefully the V2 I just sent will be better anyway. >> >> FWIW I was working off of Miriam's `git-bisect-work-part2-v8` branch at >> https://gitlab.com/mirucam/git.git. >> >> I'm happy with Christian's v2 (with or without the indentation fixes I >> suggested). > > Thanks, both of you. The updated one does look good. Oops, do you mean s/path restriction/pathspec/ fix? v2 looks OK nesting-wise and I think your indentex-fix suggestion was for the previous one. Just making sure... Thanks.
Hi Junio, On Thu, 24 Sep 2020, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > >>> Hopefully the V2 I just sent will be better anyway. > >> > >> FWIW I was working off of Miriam's `git-bisect-work-part2-v8` branch at > >> https://gitlab.com/mirucam/git.git. > >> > >> I'm happy with Christian's v2 (with or without the indentation fixes I > >> suggested). > > > > Thanks, both of you. The updated one does look good. > > Oops, do you mean s/path restriction/pathspec/ fix? v2 looks OK nesting-wise > and I think your indentex-fix suggestion was for the previous one. I was referring to the indentation of the -/+ lines in the commit message. Your current `SQUASH???` does not include the line-shortening, but that's okay. I slightly prefer the version where single conditional statements lose the curly brackets, but that's just nit-picking. A slightly bigger question is whether `git_oid_committish()` would be okay enough, or whether we do need `get_oidf(&oid, "%s^{commit}", arg)` (as your `SQUASH???` does). I'm not quite sure: aren't those two calls idempotent, with the latter going through some unnecessary string processing dances? Ciao, Dscho
Hi, On Fri, 25 Sep 2020, Johannes Schindelin wrote: > On Thu, 24 Sep 2020, Junio C Hamano wrote: > > > Junio C Hamano <gitster@pobox.com> writes: > > > > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > > > >>> Hopefully the V2 I just sent will be better anyway. > > >> > > >> FWIW I was working off of Miriam's `git-bisect-work-part2-v8` branch at > > >> https://gitlab.com/mirucam/git.git. > > >> > > >> I'm happy with Christian's v2 (with or without the indentation fixes I > > >> suggested). > > > > > > Thanks, both of you. The updated one does look good. > > > > Oops, do you mean s/path restriction/pathspec/ fix? v2 looks OK nesting-wise > > and I think your indentex-fix suggestion was for the previous one. > > I was referring to the indentation of the -/+ lines in the commit message. > Your current `SQUASH???` does not include the line-shortening, but that's > okay. I slightly prefer the version where single conditional statements > lose the curly brackets, but that's just nit-picking. > > A slightly bigger question is whether `git_oid_committish()` would be okay > enough, or whether we do need `get_oidf(&oid, "%s^{commit}", arg)` (as > your `SQUASH???` does). > > I'm not quite sure: aren't those two calls idempotent, with the latter > going through some unnecessary string processing dances? Whoops. You explained that elsewhere in the thread. My bad. Ignore me. Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > A slightly bigger question is whether `git_oid_committish()` would be okay > enough, or whether we do need `get_oidf(&oid, "%s^{commit}", arg)` (as > your `SQUASH???` does). > > I'm not quite sure: aren't those two calls idempotent, with the latter > going through some unnecessary string processing dances? I tend to agree that get_oidf() is not quite satisfactory but it is a handy short-hand that is readily available to us to parse a string into the name of the object of type commit the string peels into. While get_oid_X_ish() makes sure the result can be peeled to type X, it gives the outer object back to the caller to allow it to behave differently and it is up to the caller to peel the tag down to commit. I audited all uses of get_oid_X_ish(), in the hope that perhaps we have enough callers that want to get peeled object back, and more importantly, no caller that wants the original. Then we could change these to peel for the callers, and we may be able to lose a few lines from the callers. But unfortunately that was not the case. A new helper int oid_of_type(struct object_id *result_oid, const char *name_text, enum object_type peel_to); is a possibility, but I have a suspicion that the API in question encourages to manipulate and swap objects at their hash level for too long with little upside. If we were considering to clean the callers up, we would want to encourage them to work with in-core objects longer. IOW, repo_peel_to_type() might be a better fit for some callers that use get_oid_X_ish() and then peel the result to obtain an object of desired type. Thanks.
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 7dcc1b5188..538fa6f16b 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -486,12 +486,16 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc) return error(_("unrecognized option: '%s'"), arg); } else { char *commit_id = xstrfmt("%s^{commit}", arg); - if (get_oid(commit_id, &oid) && has_double_dash) - die(_("'%s' does not appear to be a valid " - "revision"), arg); - - string_list_append(&revs, oid_to_hex(&oid)); + int res = get_oid(commit_id, &oid); free(commit_id); + if (res) { + if (has_double_dash) + die(_("'%s' does not appear to be a valid " + "revision"), arg); + break; + } else { + string_list_append(&revs, oid_to_hex(&oid)); + } } } pathspec_pos = i; diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index b886529e59..cb645cf8c8 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -82,6 +82,13 @@ test_expect_success 'bisect fails if given any junk instead of revs' ' git bisect bad $HASH4 ' +test_expect_success 'bisect start without -- uses unknown stuff as path restriction' ' + git bisect reset && + git bisect start foo bar && + grep foo ".git/BISECT_NAMES" && + grep bar ".git/BISECT_NAMES" +' + test_expect_success 'bisect reset: back in the master branch' ' git bisect reset && echo "* master" > branch.expect &&
In 06f5608c14 (bisect--helper: `bisect_start` shell function partially in C, 2019-01-02), we changed the following shell code: - rev=$(git rev-parse -q --verify "$arg^{commit}") || { - test $has_double_dash -eq 1 && - die "$(eval_gettext "'\$arg' does not appear to be a valid revision")" - break - } - revs="$revs $rev" into: + char *commit_id = xstrfmt("%s^{commit}", arg); + if (get_oid(commit_id, &oid) && has_double_dash) + die(_("'%s' does not appear to be a valid " + "revision"), arg); + + string_list_append(&revs, oid_to_hex(&oid)); + free(commit_id); In case of an invalid "arg" when "has_double_dash" is false, the old code would "break" out of the argument loop. In the new C code though, `oid_to_hex(&oid)` is unconditonally appended to "revs". This is wrong first because "oid" is junk as `get_oid(commit_id, &oid)` failed and second because it doesn't break out of the argument loop. Not breaking out of the argument loop means that "arg" is then not treated as a path restriction (which is wrong). Signed-off-by: Christian Couder <chriscool@tuxfamily.org> --- This is a bug fix for the bug Miriam talks about in: https://lore.kernel.org/git/20200923072740.20772-1-mirucam@gmail.com/ and: https://lore.kernel.org/git/CAN7CjDDVp_i7dhpbAq5zrGW69nE6+SfivJQ-dembmu+WyqKiQQ@mail.gmail.com/ builtin/bisect--helper.c | 14 +++++++++----- t/t6030-bisect-porcelain.sh | 7 +++++++ 2 files changed, 16 insertions(+), 5 deletions(-)