[PATCH] bisect: don't use invalid oid as rev when starting

[PATCH] bisect: don't use invalid oid as rev when starting

[Christian Couder]

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

Re: [PATCH] bisect: don't use invalid oid as rev when starting

[Junio C Hamano]

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

Re: [PATCH] bisect: don't use invalid oid as rev when starting

[Johannes Schindelin]

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

Re: [PATCH] bisect: don't use invalid oid as rev when starting

[Johannes Schindelin]

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

Re: [PATCH] bisect: don't use invalid oid as rev when starting

[Junio C Hamano]

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.

Re: [PATCH] bisect: don't use invalid oid as rev when starting

[Christian Couder]

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.

Re: [PATCH] bisect: don't use invalid oid as rev when starting

[Junio C Hamano]

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.

Re: [PATCH] bisect: don't use invalid oid as rev when starting

[Johannes Schindelin]

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

Re: [PATCH] bisect: don't use invalid oid as rev when starting

[Junio C Hamano]

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.

Re: [PATCH] bisect: don't use invalid oid as rev when starting

[Junio C Hamano]

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.

Re: [PATCH] bisect: don't use invalid oid as rev when starting

[Johannes Schindelin]

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

Re: [PATCH] bisect: don't use invalid oid as rev when starting

[Johannes Schindelin]

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

Re: [PATCH] bisect: don't use invalid oid as rev when starting

[Junio C Hamano]

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.

[PATCH v2] bisect: don't use invalid oid as rev when starting

[Christian Couder]

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

Re: [PATCH v2] bisect: don't use invalid oid as rev when starting

[Johannes Schindelin]

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

Re: [PATCH v2] bisect: don't use invalid oid as rev when starting

[Christian Couder]

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.

Re: [PATCH v2] bisect: don't use invalid oid as rev when starting

[Junio C Hamano]

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...?

Re: [PATCH v2] bisect: don't use invalid oid as rev when starting

[Junio C Hamano]

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

Re: [PATCH v2] bisect: don't use invalid oid as rev when starting

[Junio C Hamano]

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

Re: [PATCH v2] bisect: don't use invalid oid as rev when starting

[Junio C Hamano]

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

Re: [PATCH v2] bisect: don't use invalid oid as rev when starting

[Junio C Hamano]

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.

Re: [PATCH v2] bisect: don't use invalid oid as rev when starting

[Christian Couder]

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.

[PATCH v3] bisect: don't use invalid oid as rev when starting

[Christian Couder]

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