threads / patch / 54283

patchbisect: don't use invalid oid as rev when starting

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

## tl;dr

23 messages between Sep 23, 2020 and Sep 25, 2020. Diffs are folded; open one to read it.

replies: 22people: 3as markdown or json

Christian Couder· Sep 23, 2020, 17:09 UTC · lore

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(-)
Show changes to 2 files +16 −5

builtin/bisect--helper.c, t/t6030-bisect-porcelain.sh

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
Junio C Hamano· Sep 23, 2020, 17:27 UTC · re: Christian Couder · lore

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

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.
Show 87 quoted lines
>
> -                       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 &&
Johannes Schindelin· Sep 23, 2020, 20:37 UTC · re: Christian Couder · lore

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

Hi Christian,
On Wed, 23 Sep 2020, Christian Couder wrote:
Show 31 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"
>
> 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!
Show 7 quoted lines
> 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.
Show 23 quoted lines
> 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.

Show 12 quoted lines
>  		}
>  	}
>  	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

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.587.g1c7fdf1d8b
>
>
Johannes Schindelin· Sep 23, 2020, 21:05 UTC · re: Johannes Schindelin · lore

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

Hi Christian,
On Wed, 23 Sep 2020, Johannes Schindelin wrote:
Show 38 quoted lines
> 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(-)
Show changes to 2 files +13 −12

builtin/bisect--helper.c, t/t6030-bisect-porcelain.sh

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
Junio C Hamano· Sep 23, 2020, 21:39 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 12 quoted lines
> 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.
Christian Couder· Sep 24, 2020, 06:10 UTC · re: Junio C Hamano · lore

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

On Wed, Sep 23, 2020 at 11:39 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
>
> 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.
Junio C Hamano· Sep 24, 2020, 06:48 UTC · re: Christian Couder · lore

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

Christian Couder <christian.couder@gmail.com> writes:
Show 24 quoted lines
> 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.
Johannes Schindelin· Sep 24, 2020, 07:51 UTC · re: Christian Couder · lore

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

Hi Christian & Junio,
On Thu, 24 Sep 2020, Christian Couder wrote:
Show 26 quoted lines
> 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

Junio C Hamano· Sep 24, 2020, 16:39 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 7 quoted lines
>> 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.
Junio C Hamano· Sep 24, 2020, 18:38 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> 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.
Johannes Schindelin· Sep 25, 2020, 07:13 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Thu, 24 Sep 2020, Junio C Hamano wrote:
Show 16 quoted lines
> 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

Johannes Schindelin· Sep 25, 2020, 07:14 UTC · re: Johannes Schindelin · lore

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

Hi,
On Fri, 25 Sep 2020, Johannes Schindelin wrote:
Show 30 quoted lines
> 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

Junio C Hamano· Sep 25, 2020, 16:54 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 6 quoted lines
> 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.
Christian Couder· Sep 24, 2020, 06:03 UTC · re: Christian Couder · lore

[PATCH v2] 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.

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(-)
Show changes to 2 files +13 −8

builtin/bisect--helper.c, t/t6030-bisect-porcelain.sh

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
Johannes Schindelin· Sep 24, 2020, 07:49 UTC · re: Christian Couder · lore

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

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
>
>
Christian Couder· Sep 24, 2020, 11:08 UTC · re: Johannes Schindelin · lore

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

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:
Show 15 quoted lines
> > -                       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.

Show 13 quoted lines
> > 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.

Junio C Hamano· Sep 24, 2020, 16:44 UTC · re: Christian Couder · lore

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

Christian Couder <christian.couder@gmail.com> writes:
Show 11 quoted lines
>> > +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...?
Junio C Hamano· Sep 24, 2020, 18:55 UTC · re: Christian Couder · lore

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

Christian Couder <christian.couder@gmail.com> writes:
Show 7 quoted lines
> -		} 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.
Show 29 quoted lines
>  			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 &&
Junio C Hamano· Sep 24, 2020, 19:25 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
> 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(-)
Show changes to 2 files +5 −4

builtin/bisect--helper.c, t/t6030-bisect-porcelain.sh

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" &&
Junio C Hamano· Sep 24, 2020, 19:56 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Sep 24, 2020, 20:53 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
> 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.

Christian Couder· Sep 25, 2020, 13:09 UTC · re: Junio C Hamano · lore

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

On Thu, Sep 24, 2020 at 8:55 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
>
> 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.
Show 7 quoted lines
> 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.

Christian Couder· Sep 25, 2020, 13:01 UTC · re: Christian Couder · lore

[PATCH v3] 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>
---
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(-)
Show changes to 2 files +13 −7

builtin/bisect--helper.c, t/t6030-bisect-porcelain.sh

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

← back to recent threads