Re: [PATCH v2] bisect: don't use invalid oid as rev when starting
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Sep 24, 2020, 07:49 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2009240947080.5061@tvgsbejvaqbjf.bet>
- In-Reply-To
- <20200924060344.15541-1-chriscool@tuxfamily.org>
Hi Christian,
On Thu, 24 Sep 2020, Christian Couder wrote:
Show 10 quoted lines
> 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.
Show 56 quoted lines
> 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!
Show 12 quoted lines
> } > 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
Show 13 quoted lines
> + 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 > >