git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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 v2] bisect: don't use invalid oid as rev when starting
Date: Thu, 24 Sep 2020 11:55:53 -0700	[thread overview]
Message-ID: <xmqqsgb7m2bq.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200924060344.15541-1-chriscool@tuxfamily.org> (Christian Couder's message of "Thu, 24 Sep 2020 08:03:44 +0200")

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 &&

  parent reply	other threads:[~2020-09-24 18:56 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-23 17:09 [PATCH] " 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 [this message]
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=xmqqsgb7m2bq.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 v2] 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).