git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH v4] t6030: add test for git bisect skip started with --term* arguments
@ 2021-04-29  7:24 Bagas Sanjaya
  2021-04-29  8:05 ` Christian Couder
  0 siblings, 1 reply; 3+ messages in thread
From: Bagas Sanjaya @ 2021-04-29  7:24 UTC (permalink / raw)
  To: git
  Cc: Christian Couder, Christian Couder, Pranit Bauva, Eric Sunshine,
	Junio C Hamano, Ramsay Jones, Bagas Sanjaya, Trygve Aaberge

Trygve Aaberge reported git bisect breakage when the bisection
is started with --term* arguments (--term-new and --term-old).

For example, suppose that we have repository with 10 commits, and we
start the bisection from HEAD to first commit (HEAD~9) with:

  $ git bisect start --term-new=fixed --term-old=unfixed HEAD HEAD~9

The bisection then stopped at HEAD~5 (fifth commit), and we choose
to skip (git bisect skip). The HEAD should now at HEAD~4 (sixth commit).
In the breakage, however, the HEAD after skipping stayed at HEAD~5
(not changed). The breakage is caused by forgetting to read '.git/BISECT_TERMS' during implementation of `'bisect skip' subcommand in C.

The fix is in commit 002241336f (bisect--helper: use BISECT_TERMS in
'bisect skip' command, 2021-04-25). To verify it fixes the breakage, add
the test.

Reported-by: Trygve Aaberge <trygveaa@gmail.com>
Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
---
  Changes from v3 [1]:
    * Lowercase local name variable hash_skipped_from and hash_skipped_to.
      Christian Couder and Eric Sunshine argued that uppercase local
      variable names make the reader confused them with global
      variables.
    * Mention breakage fix commit 002241336f in the commit message.

  [1]:
https://lore.kernel.org/git/20210428113805.109528-1-bagasdotme@gmail.com/

 t/t6030-bisect-porcelain.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 32bb66e1ed..a1baf4e451 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -922,6 +922,17 @@ test_expect_success 'bisect start takes options and revs in any order' '
 	test_cmp expected actual
 '
 
+# Bisect is started with --term-new and --term-old arguments,
+# then skip. The HEAD should be changed.
+test_expect_success 'bisect skip works with --term*' '
+	git bisect reset &&
+	git bisect start --term-new=fixed --term-old=unfixed HEAD $HASH1 &&
+	hash_skipped_from=$(git rev-parse --verify HEAD) &&
+	git bisect skip &&
+	hash_skipped_to=$(git rev-parse --verify HEAD) &&
+	test "$hash_skipped_from" != "$hash_skipped_to"
+'
+
 test_expect_success 'git bisect reset cleans bisection state properly' '
 	git bisect reset &&
 	git bisect start &&

base-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a
-- 
2.25.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v4] t6030: add test for git bisect skip started with --term* arguments
  2021-04-29  7:24 [PATCH v4] t6030: add test for git bisect skip started with --term* arguments Bagas Sanjaya
@ 2021-04-29  8:05 ` Christian Couder
  2021-04-29  8:20   ` Junio C Hamano
  0 siblings, 1 reply; 3+ messages in thread
From: Christian Couder @ 2021-04-29  8:05 UTC (permalink / raw)
  To: Bagas Sanjaya
  Cc: git, Christian Couder, Pranit Bauva, Eric Sunshine,
	Junio C Hamano, Ramsay Jones, Trygve Aaberge

On Thu, Apr 29, 2021 at 9:25 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> Trygve Aaberge reported git bisect breakage when the bisection
> is started with --term* arguments (--term-new and --term-old).

Sorry if my previous comment was not clear about it, but I think it's
enough to mention Trygve in a "Reported-by:" trailer below.

One thing that might be interesting to tell is that the breakage was
introduced by the following commit:

e4c7b33747 (bisect--helper: reimplement `bisect_skip` shell function
in C, 2021-02-03)

So maybe:

"Since e4c7b33747 (bisect--helper: reimplement `bisect_skip` shell
function in C, 2021-02-03), `git bisect skip` has been broken when the
bisection is started with --term* arguments (--term-new and
--term-old)."

> For example, suppose that we have repository with 10 commits, and we
> start the bisection from HEAD to first commit (HEAD~9) with:
>
>   $ git bisect start --term-new=fixed --term-old=unfixed HEAD HEAD~9
>
> The bisection then stopped at HEAD~5 (fifth commit), and we choose
> to skip (git bisect skip). The HEAD should now at HEAD~4 (sixth commit).
> In the breakage, however, the HEAD after skipping stayed at HEAD~5
> (not changed). The breakage is caused by forgetting to read '.git/BISECT_TERMS' during implementation of `'bisect skip' subcommand in C.
>
> The fix is in commit 002241336f (bisect--helper: use BISECT_TERMS in
> 'bisect skip' command, 2021-04-25). To verify it fixes the breakage, add
> the test.

I am not sure how safe it is to use the hash of a commit that is in
seen but not yet in next. I suggested using "a previous commit"
instead as I thought that both the fix and this commit should be part
of the same branch, and then it would be obvious which commit it is.
Maybe we should wait for Junio to come back from vacation and decide
about this.

> Reported-by: Trygve Aaberge <trygveaa@gmail.com>
> Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>

Thanks!

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v4] t6030: add test for git bisect skip started with --term* arguments
  2021-04-29  8:05 ` Christian Couder
@ 2021-04-29  8:20   ` Junio C Hamano
  0 siblings, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2021-04-29  8:20 UTC (permalink / raw)
  To: Christian Couder
  Cc: Bagas Sanjaya, git, Christian Couder, Pranit Bauva,
	Eric Sunshine, Ramsay Jones, Trygve Aaberge

Christian Couder <christian.couder@gmail.com> writes:

> I am not sure how safe it is to use the hash of a commit that is in
> seen but not yet in next. I suggested using "a previous commit"
> instead as I thought that both the fix and this commit should be part
> of the same branch, and then it would be obvious which commit it is.
> Maybe we should wait for Junio to come back from vacation and decide
> about this.

It is totally unsafe.  Besides, I do not think it is worth to make
the fix and the test as two separate commits---can I ask you to help
coordinate co-authorship between Ramsay and Bagas to come up with a
combined single patch?

Thanks.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-04-29  8:20 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-29  7:24 [PATCH v4] t6030: add test for git bisect skip started with --term* arguments Bagas Sanjaya
2021-04-29  8:05 ` Christian Couder
2021-04-29  8:20   ` Junio C Hamano

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).