* [PATCH] bisect: don't use invalid oid as rev when starting @ 2020-09-23 17:09 Christian Couder 2020-09-23 17:27 ` Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 23+ messages in thread From: Christian Couder @ 2020-09-23 17:09 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Christian Couder, Miriam Rubio, Johannes Schindelin 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(-) 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 && -- 2.28.0.587.g1c7fdf1d8b ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] bisect: don't use invalid oid as rev when starting 2020-09-23 17:09 [PATCH] bisect: don't use invalid oid as rev when starting Christian Couder @ 2020-09-23 17:27 ` Junio C Hamano 2020-09-23 20:37 ` Johannes Schindelin 2020-09-24 6:03 ` [PATCH v2] " Christian Couder 2 siblings, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2020-09-23 17:27 UTC (permalink / raw) To: Christian Couder; +Cc: git, Christian Couder, Miriam Rubio, Johannes Schindelin 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 && ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] bisect: don't use invalid oid as rev when starting 2020-09-23 17:09 [PATCH] bisect: don't use invalid oid as rev when starting Christian Couder 2020-09-23 17:27 ` Junio C Hamano @ 2020-09-23 20:37 ` Johannes Schindelin 2020-09-23 21:05 ` Johannes Schindelin 2020-09-24 6:03 ` [PATCH v2] " Christian Couder 2 siblings, 1 reply; 23+ messages in thread From: Johannes Schindelin @ 2020-09-23 20:37 UTC (permalink / raw) To: Christian Couder; +Cc: git, Junio C Hamano, Christian Couder, Miriam Rubio 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 > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] bisect: don't use invalid oid as rev when starting 2020-09-23 20:37 ` Johannes Schindelin @ 2020-09-23 21:05 ` Johannes Schindelin 2020-09-23 21:39 ` Junio C Hamano 0 siblings, 1 reply; 23+ messages in thread From: Johannes Schindelin @ 2020-09-23 21:05 UTC (permalink / raw) To: Christian Couder; +Cc: git, Junio C Hamano, Christian Couder, Miriam Rubio 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] bisect: don't use invalid oid as rev when starting 2020-09-23 21:05 ` Johannes Schindelin @ 2020-09-23 21:39 ` Junio C Hamano 2020-09-24 6:10 ` Christian Couder 0 siblings, 1 reply; 23+ messages in thread From: Junio C Hamano @ 2020-09-23 21:39 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Christian Couder, git, Christian Couder, Miriam Rubio 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] bisect: don't use invalid oid as rev when starting 2020-09-23 21:39 ` Junio C Hamano @ 2020-09-24 6:10 ` Christian Couder 2020-09-24 6:48 ` Junio C Hamano 2020-09-24 7:51 ` Johannes Schindelin 0 siblings, 2 replies; 23+ messages in thread From: Christian Couder @ 2020-09-24 6:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin, git, Christian Couder, Miriam Rubio 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] bisect: don't use invalid oid as rev when starting 2020-09-24 6:10 ` Christian Couder @ 2020-09-24 6:48 ` Junio C Hamano 2020-09-24 7:51 ` Johannes Schindelin 1 sibling, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2020-09-24 6:48 UTC (permalink / raw) To: Christian Couder; +Cc: Johannes Schindelin, git, Christian Couder, Miriam Rubio 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] bisect: don't use invalid oid as rev when starting 2020-09-24 6:10 ` Christian Couder 2020-09-24 6:48 ` Junio C Hamano @ 2020-09-24 7:51 ` Johannes Schindelin 2020-09-24 16:39 ` Junio C Hamano 1 sibling, 1 reply; 23+ messages in thread From: Johannes Schindelin @ 2020-09-24 7:51 UTC (permalink / raw) To: Christian Couder; +Cc: Junio C Hamano, git, Christian Couder, Miriam Rubio 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] bisect: don't use invalid oid as rev when starting 2020-09-24 7:51 ` Johannes Schindelin @ 2020-09-24 16:39 ` Junio C Hamano 2020-09-24 18:38 ` Junio C Hamano 0 siblings, 1 reply; 23+ messages in thread From: Junio C Hamano @ 2020-09-24 16:39 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Christian Couder, git, Christian Couder, Miriam Rubio 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] bisect: don't use invalid oid as rev when starting 2020-09-24 16:39 ` Junio C Hamano @ 2020-09-24 18:38 ` Junio C Hamano 2020-09-25 7:13 ` Johannes Schindelin 0 siblings, 1 reply; 23+ messages in thread From: Junio C Hamano @ 2020-09-24 18:38 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Christian Couder, git, Christian Couder, Miriam Rubio 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] bisect: don't use invalid oid as rev when starting 2020-09-24 18:38 ` Junio C Hamano @ 2020-09-25 7:13 ` Johannes Schindelin 2020-09-25 7:14 ` Johannes Schindelin 2020-09-25 16:54 ` Junio C Hamano 0 siblings, 2 replies; 23+ messages in thread From: Johannes Schindelin @ 2020-09-25 7:13 UTC (permalink / raw) To: Junio C Hamano; +Cc: Christian Couder, git, Christian Couder, Miriam Rubio 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] bisect: don't use invalid oid as rev when starting 2020-09-25 7:13 ` Johannes Schindelin @ 2020-09-25 7:14 ` Johannes Schindelin 2020-09-25 16:54 ` Junio C Hamano 1 sibling, 0 replies; 23+ messages in thread From: Johannes Schindelin @ 2020-09-25 7:14 UTC (permalink / raw) To: Junio C Hamano; +Cc: Christian Couder, git, Christian Couder, Miriam Rubio 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] bisect: don't use invalid oid as rev when starting 2020-09-25 7:13 ` Johannes Schindelin 2020-09-25 7:14 ` Johannes Schindelin @ 2020-09-25 16:54 ` Junio C Hamano 1 sibling, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2020-09-25 16:54 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Christian Couder, git, Christian Couder, Miriam Rubio 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2] bisect: don't use invalid oid as rev when starting 2020-09-23 17:09 [PATCH] bisect: don't use invalid oid as rev when starting Christian Couder 2020-09-23 17:27 ` Junio C Hamano 2020-09-23 20:37 ` Johannes Schindelin @ 2020-09-24 6:03 ` Christian Couder 2020-09-24 7:49 ` Johannes Schindelin ` (2 more replies) 2 siblings, 3 replies; 23+ messages in thread From: Christian Couder @ 2020-09-24 6:03 UTC (permalink / raw) To: git Cc: Junio C Hamano, Christian Couder, Miriam Rubio, Johannes Schindelin, Johannes Schindelin 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. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- This patch is made on top of e1cfff6765 (Sixteenth batch, 2020-09-22) and incorporates Dscho's suggestions. Thanks to Junio and Dscho for reviewing the first version. builtin/bisect--helper.c | 14 ++++++-------- t/t6030-bisect-porcelain.sh | 7 +++++++ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 7dcc1b5188..f4762e1774 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -484,15 +484,13 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc) 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) - die(_("'%s' does not appear to be a valid " - "revision"), arg); - + } 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 b886529e59..70c39a9459 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 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 && -- 2.28.0.585.ge1cfff6765 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2] bisect: don't use invalid oid as rev when starting 2020-09-24 6:03 ` [PATCH v2] " Christian Couder @ 2020-09-24 7:49 ` Johannes Schindelin 2020-09-24 11:08 ` Christian Couder 2020-09-24 18:55 ` Junio C Hamano 2020-09-25 13:01 ` [PATCH v3] " Christian Couder 2 siblings, 1 reply; 23+ messages in thread From: Johannes Schindelin @ 2020-09-24 7:49 UTC (permalink / raw) To: Christian Couder; +Cc: git, Junio C Hamano, Christian Couder, Miriam Rubio Hi Christian, On Thu, 24 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" These are awfully long lines. The reason is that you kept the indentation of the diff. But that's actually not necessary, because we do not need to apply a diff here; This code snippet is intended purely for human consumption. What I suggested in my adaptation of your patch was to lose the diff markers and to decrease the insane amount of indentation to just one (and two) horizontal tabs. > 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. > > Signed-off-by: Christian Couder <chriscool@tuxfamily.org> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > This patch is made on top of e1cfff6765 (Sixteenth batch, 2020-09-22) > and incorporates Dscho's suggestions. > > Thanks to Junio and Dscho for reviewing the first version. > > builtin/bisect--helper.c | 14 ++++++-------- > t/t6030-bisect-porcelain.sh | 7 +++++++ > 2 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 7dcc1b5188..f4762e1774 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -484,15 +484,13 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc) > 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) > - die(_("'%s' does not appear to be a valid " > - "revision"), arg); > - > + } 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; Thank you! > } > pathspec_pos = i; > > diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh > index b886529e59..70c39a9459 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 path restriction' ' To avoid the overly long line (and also to re-use existing naming conventions), I replaced "path restrictions" by "pathspecs" here. What do you think? Ciao, 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.585.ge1cfff6765 > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2] bisect: don't use invalid oid as rev when starting 2020-09-24 7:49 ` Johannes Schindelin @ 2020-09-24 11:08 ` Christian Couder 2020-09-24 16:44 ` Junio C Hamano 0 siblings, 1 reply; 23+ messages in thread From: Christian Couder @ 2020-09-24 11:08 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git, Junio C Hamano, Christian Couder, Miriam Rubio Hi Dscho, On Thu, Sep 24, 2020 at 11:59 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > On Thu, 24 Sep 2020, Christian Couder wrote: > > - 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" > > These are awfully long lines. The reason is that you kept the indentation > of the diff. But that's actually not necessary, because we do not need to > apply a diff here; This code snippet is intended purely for human > consumption. > > What I suggested in my adaptation of your patch was to lose the diff > markers and to decrease the insane amount of indentation to just one (and > two) horizontal tabs. Yeah, I didn't realize that. When I am sent some code or patch like this, I often hesitate between: - using it verbatim, which can create issues as it makes me more likely to overlook something in the case the sender didn't fully check everything - looking at the differences with the existing code/patch and applying them one by one, which has the risk of missing or forgetting a difference I guess the best would be to do both and then check the differences between the 2 results, but it feels like twice the amount of work for this step. > > diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh > > index b886529e59..70c39a9459 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 path restriction' ' > > To avoid the overly long line (and also to re-use existing naming > conventions), I replaced "path restrictions" by "pathspecs" here. What do > you think? It's not a huge issue, but I tend to prefer using "restrictions" because the tests that check that these arguments are used properly are called "restricting bisection on one dir" and "restricting bisection on one dir and a file". So I feel that it keeps test names more coherent. Best, Christian. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2] bisect: don't use invalid oid as rev when starting 2020-09-24 11:08 ` Christian Couder @ 2020-09-24 16:44 ` Junio C Hamano 0 siblings, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2020-09-24 16:44 UTC (permalink / raw) To: Christian Couder; +Cc: Johannes Schindelin, git, Christian Couder, Miriam Rubio Christian Couder <christian.couder@gmail.com> writes: >> > +test_expect_success 'bisect start without -- uses unknown arg as path restriction' ' >> >> To avoid the overly long line (and also to re-use existing naming >> conventions), I replaced "path restrictions" by "pathspecs" here. What do >> you think? > > It's not a huge issue, but I tend to prefer using "restrictions" > because the tests that check that these arguments are used properly > are called "restricting bisection on one dir" and "restricting > bisection on one dir and a file". So I feel that it keeps test names > more coherent. Hmph, but in the context of a sentence "use an arg as X", we should try to pick a well-known word to describe various Git arguments for X, no? The one you are using, in order to filter the set of commits involved in the operation to those that touch specific set of paths, already has its own established name and the word is "pathspec". So...? ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2] bisect: don't use invalid oid as rev when starting 2020-09-24 6:03 ` [PATCH v2] " Christian Couder 2020-09-24 7:49 ` Johannes Schindelin @ 2020-09-24 18:55 ` Junio C Hamano 2020-09-24 19:25 ` Junio C Hamano ` (2 more replies) 2020-09-25 13:01 ` [PATCH v3] " Christian Couder 2 siblings, 3 replies; 23+ messages in thread From: Junio C Hamano @ 2020-09-24 18:55 UTC (permalink / raw) To: Christian Couder; +Cc: git, Christian Couder, Miriam Rubio, Johannes Schindelin Christian Couder <christian.couder@gmail.com> writes: > - } 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); > - > + } else if (!get_oid_committish(arg, &oid)) This is wrong, I think. The "_committish" only applies to the fact that the disambiguation includes only the objects that are committishes, and the result are not peeled---you'll get an annotated tag back in oid, if you give it an annotated tag that points at a commit. This is not what ^{commit} does. It may happen to work if the downstream consumers peel objects (which are appended to the 'revs' here) down to commit when they are used, and if somebody verified that is indeed the case, it would be OK, but if this patch is presented as "unlike the previous broken one, this is the faithful conversion of the original in shell, so there is no need to verify anything outside of the patch context", that is a problem. We may need to audit recent additions of get_oid_committish() calls in our codebase. I suspect there may be other instances of the same mistake. Other than that, the code structure does look more straight-forward compared to the previous round. A fix on this version would involve peeling what is in oid down to commit, and you need to handle errors during that process, so it may not stay pretty with a fix, though. I dunno. Thanks. > 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 b886529e59..70c39a9459 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 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 && ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2] bisect: don't use invalid oid as rev when starting 2020-09-24 18:55 ` Junio C Hamano @ 2020-09-24 19:25 ` Junio C Hamano 2020-09-24 19:56 ` Junio C Hamano 2020-09-25 13:09 ` Christian Couder 2 siblings, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2020-09-24 19:25 UTC (permalink / raw) To: Christian Couder; +Cc: git, Christian Couder, Miriam Rubio, Johannes Schindelin Junio C Hamano <gitster@pobox.com> writes: > We may need to audit recent additions of get_oid_committish() calls > in our codebase. I suspect there may be other instances of the same > mistake. > > Other than that, the code structure does look more straight-forward > compared to the previous round. A fix on this version would involve > peeling what is in oid down to commit, and you need to handle errors > during that process, so it may not stay pretty with a fix, though. > I dunno. I'll queue it with this band-aid on top for now. Thanks. builtin/bisect--helper.c | 7 ++++--- t/t6030-bisect-porcelain.sh | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index a1f97e3f6c..2fcc023a3b 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -474,13 +474,14 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout, } else if (starts_with(arg, "--") && !one_of(arg, "--term-good", "--term-bad", NULL)) { return error(_("unrecognized option: '%s'"), arg); - } else if (!get_oid_committish(arg, &oid)) + } else if (!get_oidf(&oid, "%s^{commit}", arg)) { string_list_append(&revs, oid_to_hex(&oid)); - else if (has_double_dash) + } else if (has_double_dash) { die(_("'%s' does not appear to be a valid " "revision"), arg); - else + } else { break; + } } pathspec_pos = i; diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index 94179b6acf..4dbfa63ca1 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -82,7 +82,7 @@ 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 path restriction' ' +test_expect_success 'bisect start without -- takes unknown arg as pathspec' ' git bisect reset && git bisect start foo bar && grep foo ".git/BISECT_NAMES" && ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2] bisect: don't use invalid oid as rev when starting 2020-09-24 18:55 ` Junio C Hamano 2020-09-24 19:25 ` Junio C Hamano @ 2020-09-24 19:56 ` Junio C Hamano 2020-09-24 20:53 ` Junio C Hamano 2020-09-25 13:09 ` Christian Couder 2 siblings, 1 reply; 23+ messages in thread From: Junio C Hamano @ 2020-09-24 19:56 UTC (permalink / raw) To: Christian Couder; +Cc: git, Christian Couder, Miriam Rubio, Johannes Schindelin Junio C Hamano <gitster@pobox.com> writes: > We may need to audit recent additions of get_oid_committish() calls > in our codebase. I suspect there may be other instances of the same > mistake. Interim progress report. I've only looked at files that use get_oid_treeish() but audited all uses of get_oid_*ish() in them. The results are as follows. * builtin/reset.c::parse_args() makes get_oid_committish() and get_oid_treeish() only to discard the object name, because it wants to ensure the args can be peeled down to such. OK. * builtin/reset.c::cmd_reset() applies get_oid_committish() and get_oid_treeish() to the result taken from the above, but then uses lookup_commit_reference() and parse_tree_indirect() to peel the result to the desired type. OK. * notes.c::init_notes() uses get_oid_treeish() to validate that the notes ref can be read as a tree, and then uses get_tree_entry() on it, which in turn uses read_object_with_reference() for tree_type so it tolerates a commit object. OK. I didn't audit the following hits of get_oid_committish(). There might be a similar mistake as you made in v2, or there may not be. I am undecided if I should just move on, marking them as left-over-bits ;-) builtin/blame.c: if (get_oid_committish(i->string, &oid)) builtin/checkout.c: repo_get_oid_committish(the_repository, branch->name, &branch->oid); builtin/rev-parse.c: if (!get_oid_committish(start, &start_oid) && !get_oid_committish(end, &end_oid)) { builtin/rev-parse.c: if (get_oid_committish(arg, &oid) || commit.c: if (get_oid_committish(name, &oid)) revision.c: if (get_oid_committish(arg, &oid)) sequencer.c: !get_oid_committish(buf.buf, &oid)) sha1-name.c: st = repo_get_oid_committish(r, sb.buf, &oid_tmp); sha1-name.c: if (repo_get_oid_committish(r, dots[3] ? (dots + 3) : "HEAD", &oid_tmp)) sha1-name.c:int repo_get_oid_committish(struct repository *r, t/helper/test-reach.c: if (get_oid_committish(buf.buf + 2, &oid)) ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2] bisect: don't use invalid oid as rev when starting 2020-09-24 19:56 ` Junio C Hamano @ 2020-09-24 20:53 ` Junio C Hamano 0 siblings, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2020-09-24 20:53 UTC (permalink / raw) To: Christian Couder; +Cc: git, Christian Couder, Miriam Rubio, Johannes Schindelin Junio C Hamano <gitster@pobox.com> writes: > I didn't audit the following hits of get_oid_committish(). There > might be a similar mistake as you made in v2, or there may not be. > > I am undecided if I should just move on, marking them as > left-over-bits ;-) > > > > builtin/blame.c: if (get_oid_committish(i->string, &oid)) This one throws the object name of revs to be skipped to a list, and because revision traversal works on commit objects, if the user gives an annotated tag and expects the underlying commit is ignored, it may appear as a bug. But in the same function a list of revs to be ignored is read from file using oidset_parse_file() that in turn uses parse_oid_hex() without even validating if the named object exists, I would say it is OK---after all, if it hurts, the user can refrain from doing so ;-) But it would be nice to fix all issues around this caller. After collecting the object names to an oidset, somebody should go through the list, peel them down to commit and make sure they exist, or something like that. A possible #leftoverbits. > builtin/checkout.c: repo_get_oid_committish(the_repository, branch->name, &branch->oid); This one is probably OK as branch refs are supposed to point at commits and not annotated tags that point at commits. > builtin/rev-parse.c: if (!get_oid_committish(start, &start_oid) && !get_oid_committish(end, &end_oid)) { This one handles "rev-parse v1.0..v2.0" which gives "^v1.0 v2.0" but using the (unpeeled) object name. It is fine and should not be changed to auto-peel. > builtin/rev-parse.c: if (get_oid_committish(arg, &oid) || This is immediately followed by lookup_commit_reference() to peel as needed. OK. > commit.c: if (get_oid_committish(name, &oid)) This is part of lookup_commit_reference_by_name(), which peels and parses it down to an in-core commit object instance. OK. > revision.c: if (get_oid_committish(arg, &oid)) This is followed by a loop to peel it as needed. OK. > sequencer.c: !get_oid_committish(buf.buf, &oid)) This feeds the contents of rebase-merge/stopped-sha file. I presume that the contents of this file (which is not directly shown to the end users) is always a commit object name, so this is OK. Use of _committish() may probably be overkill for this internal bookkeeping file. If we stop make_patch() from shortening then probably we can change it to parse_oid_hex() to expect and read the full object name. > sha1-name.c: st = repo_get_oid_committish(r, sb.buf, &oid_tmp); > sha1-name.c: if (repo_get_oid_committish(r, dots[3] ? (dots + 3) : "HEAD", &oid_tmp)) Since I know those who wrote this old part of the codebase knew what they were doing, I do not have to comment, but these are fine. They are all peeled to commit as appropriate by calling lookup_commit_reference_gently() before feeding the result to get_merge_bases(). > sha1-name.c:int repo_get_oid_committish(struct repository *r, This is the implementation ;-) > t/helper/test-reach.c: if (get_oid_committish(buf.buf + 2, &oid)) This peels afterwards, so it is OK. The true reason I went through all the callers was to see if _all_ the callers want to either ignore the resulting object name (i.e. they want to make sure that the arg given can be peeled down to an appropriate type) or wants the object name to be peeled to the type. If that were the case (and from the above, it clearly isn't), we could change the semantics of get_oid_*ish() so that the resulting oid is already peeled down to the wanted type and that could simplify the current callers that are peeling the result themselves. But because some callers do not want to see the result peeled, we shouldn't touch what get_oid_*ish() functions do. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2] bisect: don't use invalid oid as rev when starting 2020-09-24 18:55 ` Junio C Hamano 2020-09-24 19:25 ` Junio C Hamano 2020-09-24 19:56 ` Junio C Hamano @ 2020-09-25 13:09 ` Christian Couder 2 siblings, 0 replies; 23+ messages in thread From: Christian Couder @ 2020-09-25 13:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Christian Couder, Miriam Rubio, Johannes Schindelin On Thu, Sep 24, 2020 at 8:55 PM Junio C Hamano <gitster@pobox.com> wrote: > > Christian Couder <christian.couder@gmail.com> writes: > > > - } 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); > > - > > + } else if (!get_oid_committish(arg, &oid)) > > This is wrong, I think. The "_committish" only applies to the fact > that the disambiguation includes only the objects that are > committishes, and the result are not peeled---you'll get an > annotated tag back in oid, if you give it an annotated tag that > points at a commit. > > This is not what ^{commit} does. Thanks for finding this. > It may happen to work if the downstream consumers peel objects > (which are appended to the 'revs' here) down to commit when they are > used, and if somebody verified that is indeed the case, it would be > OK, but if this patch is presented as "unlike the previous broken > one, this is the faithful conversion of the original in shell, so > there is no need to verify anything outside of the patch context", > that is a problem. I agree that it's better if there is no need to verify anything outside of the patch context. So I agree with your fix. I am also ok with using "pathspec" in the test description of the new test. Thanks, Christian. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3] bisect: don't use invalid oid as rev when starting 2020-09-24 6:03 ` [PATCH v2] " Christian Couder 2020-09-24 7:49 ` Johannes Schindelin 2020-09-24 18:55 ` Junio C Hamano @ 2020-09-25 13:01 ` Christian Couder 2 siblings, 0 replies; 23+ messages in thread From: Christian Couder @ 2020-09-25 13:01 UTC (permalink / raw) To: git Cc: Junio C Hamano, Christian Couder, Miriam Rubio, Johannes Schindelin, Johannes Schindelin 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> --- Compared to V2, this applies Junio's `SQUASH???` commit and improves indentation in the commit message. builtin/bisect--helper.c | 13 ++++++------- t/t6030-bisect-porcelain.sh | 7 +++++++ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 7dcc1b5188..2f8ef0ea30 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -484,14 +484,13 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc) 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) - die(_("'%s' does not appear to be a valid " - "revision"), arg); - + } else if (!get_oidf(&oid, "%s^{commit}", arg)) { 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 b886529e59..aa226381be 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 -- takes 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.585.ge1cfff6765 ^ permalink raw reply related [flat|nested] 23+ messages in thread
end of thread, other threads:[~2020-09-25 16:54 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-23 17:09 [PATCH] bisect: don't use invalid oid as rev when starting Christian Couder 2020-09-23 17:27 ` Junio C Hamano 2020-09-23 20:37 ` Johannes Schindelin 2020-09-23 21:05 ` Johannes Schindelin 2020-09-23 21:39 ` Junio C Hamano 2020-09-24 6:10 ` Christian Couder 2020-09-24 6:48 ` Junio C Hamano 2020-09-24 7:51 ` Johannes Schindelin 2020-09-24 16:39 ` Junio C Hamano 2020-09-24 18:38 ` Junio C Hamano 2020-09-25 7:13 ` Johannes Schindelin 2020-09-25 7:14 ` Johannes Schindelin 2020-09-25 16:54 ` Junio C Hamano 2020-09-24 6:03 ` [PATCH v2] " Christian Couder 2020-09-24 7:49 ` Johannes Schindelin 2020-09-24 11:08 ` Christian Couder 2020-09-24 16:44 ` Junio C Hamano 2020-09-24 18:55 ` Junio C Hamano 2020-09-24 19:25 ` Junio C Hamano 2020-09-24 19:56 ` Junio C Hamano 2020-09-24 20:53 ` Junio C Hamano 2020-09-25 13:09 ` Christian Couder 2020-09-25 13:01 ` [PATCH v3] " Christian Couder
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).