From: Junio C Hamano <gitster@pobox.com> To: Christian Couder <christian.couder@gmail.com> Cc: git@vger.kernel.org, Christian Couder <chriscool@tuxfamily.org>, Miriam Rubio <mirucam@gmail.com>, Johannes Schindelin <Johannes.Schindelin@gmx.de> Subject: Re: [PATCH] bisect: don't use invalid oid as rev when starting Date: Wed, 23 Sep 2020 10:27:14 -0700 [thread overview] Message-ID: <xmqqwo0kietp.fsf@gitster.c.googlers.com> (raw) In-Reply-To: <20200923170915.21748-1-chriscool@tuxfamily.org> (Christian Couder's message of "Wed, 23 Sep 2020 19:09:15 +0200") 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 &&
next prev parent reply other threads:[~2020-09-23 17:27 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-23 17:09 Christian Couder 2020-09-23 17:27 ` Junio C Hamano [this message] 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
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=xmqqwo0kietp.fsf@gitster.c.googlers.com \ --to=gitster@pobox.com \ --cc=Johannes.Schindelin@gmx.de \ --cc=chriscool@tuxfamily.org \ --cc=christian.couder@gmail.com \ --cc=git@vger.kernel.org \ --cc=mirucam@gmail.com \ --subject='Re: [PATCH] bisect: don'\''t use invalid oid as rev when starting' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
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).