git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/6] bisect: follow-up fixes from js/bisect-in-c
@ 2022-12-15  9:47 Ævar Arnfjörð Bjarmason
  2022-12-15  9:47 ` [PATCH 1/6] bisect--helper: simplify exit code computation Ævar Arnfjörð Bjarmason
                   ` (7 more replies)
  0 siblings, 8 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-12-15  9:47 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Johannes Schindelin,
	Đoàn Trần Công Danh,
	Ævar Arnfjörð Bjarmason

The js/bisect-in-c topic got replaced by dd/bisect-helper-subcommand
and later dd/git-bisect-builtin to fix a regression in "bisect".

This small set of fixes is a cherry-pick of various miscellanious
fixes that were part of js/bisect-in-c that are still worth
having. Johannes and I have coordinated my submission of this
follow-up topic off-list.

The only "new" commit here is the 2/6. It's a cherry-pick of
Johannes's commit to do the same, but as the change to the base was so
extensive (the range diff won't even pick it up anymore with the
default --creation-factor) he suggested I change the authorship, which
I've done here.

There's still other stuff worth picking up from js/bisect-in-c. In
particular we should be using parse_options() for the various
sub-commands of "bisect". But as those changes had much more extensive
conflicts let's leave them for later.

The range-diff here is to pr-1132/dscho/bisect-in-c-v6[1]. My own
branch & CI for this is at [2].

1. https://lore.kernel.org/git/pull.1132.v6.git.1661885419.gitgitgadget@gmail.com/
2. https://github.com/avar/git/tree/avar-js/bisect-in-c-rebased

Johannes Schindelin (5):
  bisect--helper: simplify exit code computation
  bisect: verify that a bogus option won't try to start a bisection
  bisect run: fix the error message
  bisect: remove Cogito-related code
  bisect: no longer try to clean up left-over `.git/head-name` files

Ævar Arnfjörð Bjarmason (1):
  bisect--helper: make the order consistently `argc, argv`

 bisect.c                    |  3 ---
 builtin/bisect.c            | 52 ++++++++++++++-----------------------
 t/t6030-bisect-porcelain.sh | 21 ++++++++++++++-
 3 files changed, 40 insertions(+), 36 deletions(-)

Range-diff:
 1:  05262b6a7d1 <  -:  ----------- bisect--helper: retire the --no-log option
 2:  1e43148864a <  -:  ----------- bisect--helper: really retire --bisect-next-check
 3:  1a1649d9d0d <  -:  ----------- bisect--helper: really retire `--bisect-autostart`
 4:  9ab30552c6a !  1:  c8c648e4b8c bisect--helper: simplify exit code computation
    @@ Commit message
         Let's use it instead of duplicating the logic.
     
         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    - ## builtin/bisect--helper.c ##
    -@@ builtin/bisect--helper.c: int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
    + ## builtin/bisect.c ##
    +@@ builtin/bisect.c: int cmd_bisect(int argc, const char **argv, const char *prefix)
    + 		res = fn(argc, argv, prefix);
      	}
    - 	free_terms(&terms);
      
     -	/*
     -	 * Handle early success
 5:  92b3b116ef8 <  -:  ----------- bisect--helper: make `terms` an explicit singleton
 6:  c9dc0281e38 <  -:  ----------- bisect--helper: make the order consistently `argc, argv`
 7:  e97e187bbec <  -:  ----------- bisect--helper: migrate to OPT_SUBCOMMAND()
 -:  ----------- >  2:  a0de7ad6836 bisect--helper: make the order consistently `argc, argv`
 8:  30c87f2e92e !  3:  e1e31278fef bisect: verify that a bogus option won't try to start a bisection
    @@ Commit message
         by "git bisect start"` and fail if it was found.
     
         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## t/t6030-bisect-porcelain.sh ##
     @@ t/t6030-bisect-porcelain.sh: test_expect_success 'bisect start with one term1 and term2' '
 9:  4696652b99c !  4:  59a8a3085b1 bisect run: fix the error message
    @@ Commit message
     
         Helped-by: Elijah Newren <newren@gmail.com>
         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    - ## builtin/bisect--helper.c ##
    -@@ builtin/bisect--helper.c: static int cmd_bisect_run(int argc, const char **argv, const char *prefix)
    - 			printf(_("bisect found first bad commit"));
    + ## builtin/bisect.c ##
    +@@ builtin/bisect.c: static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
    + 			puts(_("bisect found first bad commit"));
      			res = BISECT_OK;
      		} else if (res) {
    --			error(_("bisect run failed: 'git bisect--helper --bisect-state"
    -+			error(_("bisect run failed: 'git bisect"
    - 			" %s' exited with error code %d"), new_state, res);
    +-			error(_("bisect run failed: 'bisect-state %s'"
    ++			error(_("bisect run failed: 'git bisect %s'"
    + 				" exited with error code %d"), new_state, res);
      		} else {
      			continue;
     
10:  b202a0e386c <  -:  ----------- bisect: avoid double-quoting when printing the failed command
11:  3376b450867 <  -:  ----------- bisect--helper: calling `bisect_state()` without an argument is a bug
12:  e7623508f90 <  -:  ----------- bisect--helper: make `state` optional
13:  3f052580c95 <  -:  ----------- bisect: move even the command-line parsing to `bisect--helper`
14:  a83fe3dc3c2 <  -:  ----------- Turn `git bisect` into a full built-in
15:  f2132b61ff7 !  5:  1b70cd79cae bisect: remove Cogito-related code
    @@ Commit message
         remove the last remnant of Cogito-accommodating code.
     
         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/bisect.c ##
     @@ builtin/bisect.c: static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
    @@ builtin/bisect.c: static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXP
      static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
      static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
      static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
    -@@ builtin/bisect.c: static int cmd_bisect_start(int argc, const char **argv, const char *prefix)
    +@@ builtin/bisect.c: static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
      			strbuf_addstr(&start_head, oid_to_hex(&head_oid));
      		} else if (!get_oid(head, &head_oid) &&
      			   skip_prefix(head, "refs/heads/", &head)) {
16:  4f93692e071 !  6:  2ad89aca728 bisect: no longer try to clean up left-over `.git/head-name` files
    @@ Commit message
         So let's remove that code, at long last.
     
         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## bisect.c ##
     @@ bisect.c: static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
-- 
2.39.0.rc2.1048.g0e5493b8d5b


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

* [PATCH 1/6] bisect--helper: simplify exit code computation
  2022-12-15  9:47 [PATCH 0/6] bisect: follow-up fixes from js/bisect-in-c Ævar Arnfjörð Bjarmason
@ 2022-12-15  9:47 ` Ævar Arnfjörð Bjarmason
  2022-12-15  9:47 ` [PATCH 2/6] bisect--helper: make the order consistently `argc, argv` Ævar Arnfjörð Bjarmason
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-12-15  9:47 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Johannes Schindelin,
	Đoàn Trần Công Danh, Johannes Schindelin,
	Ævar Arnfjörð Bjarmason

From: Johannes Schindelin <johannes.schindelin@gmx.de>

We _already_ have a function to determine whether a given `enum
bisect_error` value is non-zero but still _actually_ indicates success.

Let's use it instead of duplicating the logic.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bisect.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/builtin/bisect.c b/builtin/bisect.c
index cc9483e8515..09505fc4dce 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -1440,12 +1440,5 @@ int cmd_bisect(int argc, const char **argv, const char *prefix)
 		res = fn(argc, argv, prefix);
 	}
 
-	/*
-	 * Handle early success
-	 * From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all
-	 */
-	if ((res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) || (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND))
-		res = BISECT_OK;
-
-	return -res;
+	return is_bisect_success(res) ? 0 : -res;
 }
-- 
2.39.0.rc2.1048.g0e5493b8d5b


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

* [PATCH 2/6] bisect--helper: make the order consistently `argc, argv`
  2022-12-15  9:47 [PATCH 0/6] bisect: follow-up fixes from js/bisect-in-c Ævar Arnfjörð Bjarmason
  2022-12-15  9:47 ` [PATCH 1/6] bisect--helper: simplify exit code computation Ævar Arnfjörð Bjarmason
@ 2022-12-15  9:47 ` Ævar Arnfjörð Bjarmason
  2022-12-15  9:47 ` [PATCH 3/6] bisect: verify that a bogus option won't try to start a bisection Ævar Arnfjörð Bjarmason
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-12-15  9:47 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Johannes Schindelin,
	Đoàn Trần Công Danh,
	Ævar Arnfjörð Bjarmason, Johannes Schindelin

In C, the natural order is for `argc` to come before `argv` by virtue of
the `main()` function declaring the parameters in precisely that order.

It is confusing & distracting, then, when readers familiar with the C
language read code where that order is switched around.

Let's just change the order and avoid that type of developer friction.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/bisect.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/builtin/bisect.c b/builtin/bisect.c
index 09505fc4dce..9fc8db06944 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -678,7 +678,8 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char
 	return bisect_next(terms, prefix);
 }
 
-static enum bisect_error bisect_start(struct bisect_terms *terms, const char **argv, int argc)
+static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
+				      const char **argv)
 {
 	int no_checkout = 0;
 	int first_parent_only = 0;
@@ -908,13 +909,13 @@ static int bisect_autostart(struct bisect_terms *terms)
 	yesno = git_prompt(_("Do you want me to do it for you "
 			     "[Y/n]? "), PROMPT_ECHO);
 	res = tolower(*yesno) == 'n' ?
-		-1 : bisect_start(terms, empty_strvec, 0);
+		-1 : bisect_start(terms, 0, empty_strvec);
 
 	return res;
 }
 
-static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
-				      int argc)
+static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
+				      const char **argv)
 {
 	const char *state;
 	int i, verify_expected = 1;
@@ -1033,7 +1034,7 @@ static int process_replay_line(struct bisect_terms *terms, struct strbuf *line)
 		struct strvec argv = STRVEC_INIT;
 		int res;
 		sq_dequote_to_strvec(rev, &argv);
-		res = bisect_start(terms, argv.v, argv.nr);
+		res = bisect_start(terms, argv.nr, argv.v);
 		strvec_clear(&argv);
 		return res;
 	}
@@ -1083,7 +1084,8 @@ static enum bisect_error bisect_replay(struct bisect_terms *terms, const char *f
 	return bisect_auto_next(terms, NULL);
 }
 
-static enum bisect_error bisect_skip(struct bisect_terms *terms, const char **argv, int argc)
+static enum bisect_error bisect_skip(struct bisect_terms *terms, int argc,
+				     const char **argv)
 {
 	int i;
 	enum bisect_error res;
@@ -1113,13 +1115,14 @@ static enum bisect_error bisect_skip(struct bisect_terms *terms, const char **ar
 			strvec_push(&argv_state, argv[i]);
 		}
 	}
-	res = bisect_state(terms, argv_state.v, argv_state.nr);
+	res = bisect_state(terms, argv_state.nr, argv_state.v);
 
 	strvec_clear(&argv_state);
 	return res;
 }
 
-static int bisect_visualize(struct bisect_terms *terms, const char **argv, int argc)
+static int bisect_visualize(struct bisect_terms *terms, int argc,
+			    const char **argv)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct strbuf sb = STRBUF_INIT;
@@ -1202,7 +1205,7 @@ static int verify_good(const struct bisect_terms *terms, const char *command)
 	return rc;
 }
 
-static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
+static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
 {
 	int res = BISECT_OK;
 	struct strbuf command = STRBUF_INIT;
@@ -1271,7 +1274,7 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 		saved_stdout = dup(1);
 		dup2(temporary_stdout_fd, 1);
 
-		res = bisect_state(terms, &new_state, 1);
+		res = bisect_state(terms, 1, &new_state);
 
 		fflush(stdout);
 		dup2(saved_stdout, 1);
@@ -1328,7 +1331,7 @@ static int cmd_bisect__start(int argc, const char **argv, const char *prefix UNU
 	struct bisect_terms terms = { 0 };
 
 	set_terms(&terms, "bad", "good");
-	res = bisect_start(&terms, argv, argc);
+	res = bisect_start(&terms, argc, argv);
 	free_terms(&terms);
 	return res;
 }
@@ -1372,7 +1375,7 @@ static int cmd_bisect__skip(int argc, const char **argv, const char *prefix UNUS
 
 	set_terms(&terms, "bad", "good");
 	get_terms(&terms);
-	res = bisect_skip(&terms, argv, argc);
+	res = bisect_skip(&terms, argc, argv);
 	free_terms(&terms);
 	return res;
 }
@@ -1383,7 +1386,7 @@ static int cmd_bisect__visualize(int argc, const char **argv, const char *prefix
 	struct bisect_terms terms = { 0 };
 
 	get_terms(&terms);
-	res = bisect_visualize(&terms, argv, argc);
+	res = bisect_visualize(&terms, argc, argv);
 	free_terms(&terms);
 	return res;
 }
@@ -1396,7 +1399,7 @@ static int cmd_bisect__run(int argc, const char **argv, const char *prefix UNUSE
 	if (!argc)
 		return error(_("'%s' failed: no command provided."), "git bisect run");
 	get_terms(&terms);
-	res = bisect_run(&terms, argv, argc);
+	res = bisect_run(&terms, argc, argv);
 	free_terms(&terms);
 	return res;
 }
@@ -1432,7 +1435,7 @@ int cmd_bisect(int argc, const char **argv, const char *prefix)
 		if (check_and_set_terms(&terms, argv[0]))
 			usage_msg_optf(_("unknown command: '%s'"), git_bisect_usage,
 				       options, argv[0]);
-		res = bisect_state(&terms, argv, argc);
+		res = bisect_state(&terms, argc, argv);
 		free_terms(&terms);
 	} else {
 		argc--;
-- 
2.39.0.rc2.1048.g0e5493b8d5b


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

* [PATCH 3/6] bisect: verify that a bogus option won't try to start a bisection
  2022-12-15  9:47 [PATCH 0/6] bisect: follow-up fixes from js/bisect-in-c Ævar Arnfjörð Bjarmason
  2022-12-15  9:47 ` [PATCH 1/6] bisect--helper: simplify exit code computation Ævar Arnfjörð Bjarmason
  2022-12-15  9:47 ` [PATCH 2/6] bisect--helper: make the order consistently `argc, argv` Ævar Arnfjörð Bjarmason
@ 2022-12-15  9:47 ` Ævar Arnfjörð Bjarmason
  2022-12-15  9:47 ` [PATCH 4/6] bisect run: fix the error message Ævar Arnfjörð Bjarmason
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-12-15  9:47 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Johannes Schindelin,
	Đoàn Trần Công Danh, Johannes Schindelin,
	Ævar Arnfjörð Bjarmason

From: Johannes Schindelin <johannes.schindelin@gmx.de>

We do not want `git bisect --bogus-option` to start a bisection. To
verify that, we look for the tell-tale error message `You need to start
by "git bisect start"` and fail if it was found.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6030-bisect-porcelain.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 98a72ff78a7..9e56b42b5da 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -1058,6 +1058,16 @@ test_expect_success 'bisect start with one term1 and term2' '
 	git bisect reset
 '
 
+test_expect_success 'bogus command does not start bisect' '
+	git bisect reset &&
+	test_must_fail git bisect --bisect-terms 1 2 2>out &&
+	! grep "You need to start" out &&
+	test_must_fail git bisect --bisect-terms 2>out &&
+	! grep "You need to start" out &&
+	grep "git bisect.*visualize" out &&
+	git bisect reset
+'
+
 test_expect_success 'bisect replay with term1 and term2' '
 	git bisect replay log_to_replay.txt >bisect_result &&
 	grep "$HASH2 is the first term1 commit" bisect_result &&
-- 
2.39.0.rc2.1048.g0e5493b8d5b


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

* [PATCH 4/6] bisect run: fix the error message
  2022-12-15  9:47 [PATCH 0/6] bisect: follow-up fixes from js/bisect-in-c Ævar Arnfjörð Bjarmason
                   ` (2 preceding siblings ...)
  2022-12-15  9:47 ` [PATCH 3/6] bisect: verify that a bogus option won't try to start a bisection Ævar Arnfjörð Bjarmason
@ 2022-12-15  9:47 ` Ævar Arnfjörð Bjarmason
  2022-12-15 15:08   ` Đoàn Trần Công Danh
  2022-12-15  9:47 ` [PATCH 5/6] bisect: remove Cogito-related code Ævar Arnfjörð Bjarmason
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-12-15  9:47 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Johannes Schindelin,
	Đoàn Trần Công Danh, Johannes Schindelin,
	Elijah Newren, Ævar Arnfjörð Bjarmason

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In d1bbbe45df8 (bisect--helper: reimplement `bisect_run` shell function
in C, 2021-09-13), we ported the `bisect run` subcommand to C, including
the part that prints out an error message when the implicit `git bisect
bad` or `git bisect good` failed.

However, the error message was supposed to print out whether the state
was "good" or "bad", but used a bogus (because non-populated) `args`
variable for it. This was fixed in 80c2e9657f2 (bisect--helper: report
actual bisect_state() argument on error, 2022-01-18), but the error
message still talks about `bisect--helper`, which is an implementation
detail that should not concern end users.

Fix that, and add a regression test to ensure that the intended form of
the error message.

Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bisect.c            |  2 +-
 t/t6030-bisect-porcelain.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/bisect.c b/builtin/bisect.c
index 9fc8db06944..0786ebf4012 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -1292,7 +1292,7 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
 			puts(_("bisect found first bad commit"));
 			res = BISECT_OK;
 		} else if (res) {
-			error(_("bisect run failed: 'bisect-state %s'"
+			error(_("bisect run failed: 'git bisect %s'"
 				" exited with error code %d"), new_state, res);
 		} else {
 			continue;
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 9e56b42b5da..0a62ea2b3ce 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -1221,4 +1221,14 @@ test_expect_success 'bisect state output with bad commit' '
 	grep -F "waiting for good commit(s), bad commit known" output
 '
 
+test_expect_success 'verify correct error message' '
+	git bisect reset &&
+	git bisect start $HASH4 $HASH1 &&
+	write_script test_script.sh <<-\EOF &&
+	rm .git/BISECT*
+	EOF
+	test_must_fail git bisect run ./test_script.sh 2>error &&
+	grep "git bisect good.*exited with error code" error
+'
+
 test_done
-- 
2.39.0.rc2.1048.g0e5493b8d5b


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

* [PATCH 5/6] bisect: remove Cogito-related code
  2022-12-15  9:47 [PATCH 0/6] bisect: follow-up fixes from js/bisect-in-c Ævar Arnfjörð Bjarmason
                   ` (3 preceding siblings ...)
  2022-12-15  9:47 ` [PATCH 4/6] bisect run: fix the error message Ævar Arnfjörð Bjarmason
@ 2022-12-15  9:47 ` Ævar Arnfjörð Bjarmason
  2022-12-15  9:47 ` [PATCH 6/6] bisect: no longer try to clean up left-over `.git/head-name` files Ævar Arnfjörð Bjarmason
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-12-15  9:47 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Johannes Schindelin,
	Đoàn Trần Công Danh, Johannes Schindelin,
	Ævar Arnfjörð Bjarmason

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Once upon a time, there was this idea that Git would not actually be a
single coherent program, but rather a set of low-level programs that
users cobble together via shell scripts, or develop high-level user
interfaces for Git, or both.

Cogito was such a high-level user interface, incidentally implemented
via shell scripts that cobble together Git calls.

It did turn out relatively quickly that Git would much rather provide a
useful high-level user interface itself.

As of April 19th, 2007, Cogito was therefore discontinued (see
https://lore.kernel.org/git/20070419124648.GL4489@pasky.or.cz/).

Nevertheless, for almost 15 years after that announcement, Git carried
special code in `git bisect` to accommodate Cogito.

Since it is beyond doubt that there are no more Cogito users, let's
remove the last remnant of Cogito-accommodating code.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bisect.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/builtin/bisect.c b/builtin/bisect.c
index 0786ebf4012..73017402671 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -15,7 +15,6 @@ static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
 static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
-static GIT_PATH_FUNC(git_path_head_name, "head-name")
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
 static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
@@ -808,13 +807,6 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
 			strbuf_addstr(&start_head, oid_to_hex(&head_oid));
 		} else if (!get_oid(head, &head_oid) &&
 			   skip_prefix(head, "refs/heads/", &head)) {
-			/*
-			 * This error message should only be triggered by
-			 * cogito usage, and cogito users should understand
-			 * it relates to cg-seek.
-			 */
-			if (!is_empty_or_missing_file(git_path_head_name()))
-				return error(_("won't bisect on cg-seek'ed tree"));
 			strbuf_addstr(&start_head, head);
 		} else {
 			return error(_("bad HEAD - strange symbolic ref"));
-- 
2.39.0.rc2.1048.g0e5493b8d5b


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

* [PATCH 6/6] bisect: no longer try to clean up left-over `.git/head-name` files
  2022-12-15  9:47 [PATCH 0/6] bisect: follow-up fixes from js/bisect-in-c Ævar Arnfjörð Bjarmason
                   ` (4 preceding siblings ...)
  2022-12-15  9:47 ` [PATCH 5/6] bisect: remove Cogito-related code Ævar Arnfjörð Bjarmason
@ 2022-12-15  9:47 ` Ævar Arnfjörð Bjarmason
  2022-12-15 10:45 ` [PATCH 0/6] bisect: follow-up fixes from js/bisect-in-c Christian Couder
  2023-01-12 15:19 ` [PATCH v2 " Ævar Arnfjörð Bjarmason
  7 siblings, 0 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-12-15  9:47 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Johannes Schindelin,
	Đoàn Trần Công Danh, Johannes Schindelin,
	Ævar Arnfjörð Bjarmason

From: Johannes Schindelin <johannes.schindelin@gmx.de>

As per the code comment, the `.git/head-name` files were cleaned up for
backwards-compatibility: an old version of `git bisect` could have left
them behind.

Now, just how old would such a version be? As of 0f497e75f05 (Eliminate
confusing "won't bisect on seeked tree" failure, 2008-02-23), `git
bisect` does not write that file anymore. Which corresponds to Git
v1.5.4.4.

Even if the likelihood is non-nil that there might still be users out
there who use such an old version to start a bisection, but then decide
to continue bisecting with a current Git version, it is highly
improbable.

So let's remove that code, at long last.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 bisect.c                    | 3 ---
 t/t6030-bisect-porcelain.sh | 1 -
 2 files changed, 4 deletions(-)

diff --git a/bisect.c b/bisect.c
index ec7487e6836..ef5ee5a6436 100644
--- a/bisect.c
+++ b/bisect.c
@@ -472,7 +472,6 @@ static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
-static GIT_PATH_FUNC(git_path_head_name, "head-name")
 
 static void read_bisect_paths(struct strvec *array)
 {
@@ -1188,8 +1187,6 @@ int bisect_clean_state(void)
 	unlink_or_warn(git_path_bisect_run());
 	unlink_or_warn(git_path_bisect_terms());
 	unlink_or_warn(git_path_bisect_first_parent());
-	/* Cleanup head-name if it got left by an old version of git-bisect */
-	unlink_or_warn(git_path_head_name());
 	/*
 	 * Cleanup BISECT_START last to support the --no-checkout option
 	 * introduced in the commit 4796e823a.
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 0a62ea2b3ce..3ba4fdf6153 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -1158,7 +1158,6 @@ test_expect_success 'git bisect reset cleans bisection state properly' '
 	test_path_is_missing ".git/BISECT_LOG" &&
 	test_path_is_missing ".git/BISECT_RUN" &&
 	test_path_is_missing ".git/BISECT_TERMS" &&
-	test_path_is_missing ".git/head-name" &&
 	test_path_is_missing ".git/BISECT_HEAD" &&
 	test_path_is_missing ".git/BISECT_START"
 '
-- 
2.39.0.rc2.1048.g0e5493b8d5b


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

* Re: [PATCH 0/6] bisect: follow-up fixes from js/bisect-in-c
  2022-12-15  9:47 [PATCH 0/6] bisect: follow-up fixes from js/bisect-in-c Ævar Arnfjörð Bjarmason
                   ` (5 preceding siblings ...)
  2022-12-15  9:47 ` [PATCH 6/6] bisect: no longer try to clean up left-over `.git/head-name` files Ævar Arnfjörð Bjarmason
@ 2022-12-15 10:45 ` Christian Couder
  2023-01-12 15:19 ` [PATCH v2 " Ævar Arnfjörð Bjarmason
  7 siblings, 0 replies; 16+ messages in thread
From: Christian Couder @ 2022-12-15 10:45 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Johannes Schindelin,
	Đoàn Trần Công Danh

On Thu, Dec 15, 2022 at 11:27 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:

> This small set of fixes is a cherry-pick of various miscellanious

s/miscellanious/miscellaneous/

> fixes that were part of js/bisect-in-c that are still worth
> having.

Thanks! All the patches make sense and look good to me.

> Johannes Schindelin (5):
>   bisect--helper: simplify exit code computation
>   bisect: verify that a bogus option won't try to start a bisection
>   bisect run: fix the error message

I just noticed a nit in the commit message of the above patch. It contains:

"Fix that, and add a regression test to ensure that the intended form of
the error message."

And it seems to me that something like "is printed" is missing from
the end of that sentence.

>   bisect: remove Cogito-related code
>   bisect: no longer try to clean up left-over `.git/head-name` files
>
> Ævar Arnfjörð Bjarmason (1):
>   bisect--helper: make the order consistently `argc, argv`

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

* Re: [PATCH 4/6] bisect run: fix the error message
  2022-12-15  9:47 ` [PATCH 4/6] bisect run: fix the error message Ævar Arnfjörð Bjarmason
@ 2022-12-15 15:08   ` Đoàn Trần Công Danh
  0 siblings, 0 replies; 16+ messages in thread
From: Đoàn Trần Công Danh @ 2022-12-15 15:08 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Johannes Schindelin, Elijah Newren

On 2022-12-15 10:47:47+0100, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> In d1bbbe45df8 (bisect--helper: reimplement `bisect_run` shell function
> in C, 2021-09-13), we ported the `bisect run` subcommand to C, including
> the part that prints out an error message when the implicit `git bisect
> bad` or `git bisect good` failed.
> 
> However, the error message was supposed to print out whether the state
> was "good" or "bad", but used a bogus (because non-populated) `args`
> variable for it. This was fixed in 80c2e9657f2 (bisect--helper: report
> actual bisect_state() argument on error, 2022-01-18), but the error
> message still talks about `bisect--helper`, which is an implementation
> detail that should not concern end users.

I don't think the error still talks about `bisect--helper`.
The error is talking about some "bisect-state" which is not it Git's
glossary, though.

> Fix that, and add a regression test to ensure that the intended form of
> the error message.
> 
> Helped-by: Elijah Newren <newren@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/bisect.c            |  2 +-
>  t/t6030-bisect-porcelain.sh | 10 ++++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/bisect.c b/builtin/bisect.c
> index 9fc8db06944..0786ebf4012 100644
> --- a/builtin/bisect.c
> +++ b/builtin/bisect.c
> @@ -1292,7 +1292,7 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
>  			puts(_("bisect found first bad commit"));
>  			res = BISECT_OK;
>  		} else if (res) {
> -			error(_("bisect run failed: 'bisect-state %s'"
> +			error(_("bisect run failed: 'git bisect %s'"
>  				" exited with error code %d"), new_state, res);
>  		} else {
>  			continue;
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index 9e56b42b5da..0a62ea2b3ce 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -1221,4 +1221,14 @@ test_expect_success 'bisect state output with bad commit' '
>  	grep -F "waiting for good commit(s), bad commit known" output
>  '
>  
> +test_expect_success 'verify correct error message' '
> +	git bisect reset &&
> +	git bisect start $HASH4 $HASH1 &&
> +	write_script test_script.sh <<-\EOF &&
> +	rm .git/BISECT*
> +	EOF
> +	test_must_fail git bisect run ./test_script.sh 2>error &&
> +	grep "git bisect good.*exited with error code" error
> +'
> +
>  test_done
> -- 
> 2.39.0.rc2.1048.g0e5493b8d5b
> 

-- 
Danh

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

* [PATCH v2 0/6] bisect: follow-up fixes from js/bisect-in-c
  2022-12-15  9:47 [PATCH 0/6] bisect: follow-up fixes from js/bisect-in-c Ævar Arnfjörð Bjarmason
                   ` (6 preceding siblings ...)
  2022-12-15 10:45 ` [PATCH 0/6] bisect: follow-up fixes from js/bisect-in-c Christian Couder
@ 2023-01-12 15:19 ` Ævar Arnfjörð Bjarmason
  2023-01-12 15:19   ` [PATCH v2 1/6] bisect--helper: simplify exit code computation Ævar Arnfjörð Bjarmason
                     ` (5 more replies)
  7 siblings, 6 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 15:19 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Johannes Schindelin,
	Đoàn Trần Công Danh,
	Ævar Arnfjörð Bjarmason

A trivial update to this topic of follow-up fixes to "bisect", which
is now a built-in. See
https://lore.kernel.org/git/cover-0.6-00000000000-20221215T094038Z-avarab@gmail.com/
for the v1 & general summary.

Change since v1:

 * Rephrased & updated a commit message which was outdated as of
   f37d0bdd42d, see the range-diff. Thanks to Đoàn Trần Công Danh for
   spotting it.

Johannes Schindelin (5):
  bisect--helper: simplify exit code computation
  bisect: verify that a bogus option won't try to start a bisection
  bisect run: fix the error message
  bisect: remove Cogito-related code
  bisect: no longer try to clean up left-over `.git/head-name` files

Ævar Arnfjörð Bjarmason (1):
  bisect--helper: make the order consistently `argc, argv`

 bisect.c                    |  3 ---
 builtin/bisect.c            | 52 ++++++++++++++-----------------------
 t/t6030-bisect-porcelain.sh | 21 ++++++++++++++-
 3 files changed, 40 insertions(+), 36 deletions(-)

Range-diff against v1:
1:  c8c648e4b8c = 1:  32c45bbf851 bisect--helper: simplify exit code computation
2:  a0de7ad6836 = 2:  1f4449dd081 bisect--helper: make the order consistently `argc, argv`
3:  e1e31278fef = 3:  0cfb7dc572c bisect: verify that a bogus option won't try to start a bisection
4:  59a8a3085b1 ! 4:  4dda1019767 bisect run: fix the error message
    @@ Commit message
     
         However, the error message was supposed to print out whether the state
         was "good" or "bad", but used a bogus (because non-populated) `args`
    -    variable for it. This was fixed in 80c2e9657f2 (bisect--helper: report
    -    actual bisect_state() argument on error, 2022-01-18), but the error
    -    message still talks about `bisect--helper`, which is an implementation
    -    detail that should not concern end users.
    +    variable for it. This was fixed in [1], but as of [2] (when
    +    `bisect--helper` was changed to the present `bisect-state') the error
    +    message still talks about implementation details that should not
    +    concern end users.
     
         Fix that, and add a regression test to ensure that the intended form of
         the error message.
     
    +    1. 80c2e9657f2 (bisect--helper: report actual bisect_state() argument
    +       on error, 2022-01-18
    +    2. f37d0bdd42d (bisect: fix output regressions in v2.30.0, 2022-11-10)
    +
         Helped-by: Elijah Newren <newren@gmail.com>
         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
5:  1b70cd79cae = 5:  1600ef41608 bisect: remove Cogito-related code
6:  2ad89aca728 = 6:  817fe726b4b bisect: no longer try to clean up left-over `.git/head-name` files
-- 
2.39.0.1215.g1ba3f685d4f


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

* [PATCH v2 1/6] bisect--helper: simplify exit code computation
  2023-01-12 15:19 ` [PATCH v2 " Ævar Arnfjörð Bjarmason
@ 2023-01-12 15:19   ` Ævar Arnfjörð Bjarmason
  2023-01-12 15:19   ` [PATCH v2 2/6] bisect--helper: make the order consistently `argc, argv` Ævar Arnfjörð Bjarmason
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 15:19 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Johannes Schindelin,
	Đoàn Trần Công Danh, Johannes Schindelin,
	Ævar Arnfjörð Bjarmason

From: Johannes Schindelin <johannes.schindelin@gmx.de>

We _already_ have a function to determine whether a given `enum
bisect_error` value is non-zero but still _actually_ indicates success.

Let's use it instead of duplicating the logic.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bisect.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/builtin/bisect.c b/builtin/bisect.c
index cc9483e8515..09505fc4dce 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -1440,12 +1440,5 @@ int cmd_bisect(int argc, const char **argv, const char *prefix)
 		res = fn(argc, argv, prefix);
 	}
 
-	/*
-	 * Handle early success
-	 * From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all
-	 */
-	if ((res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) || (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND))
-		res = BISECT_OK;
-
-	return -res;
+	return is_bisect_success(res) ? 0 : -res;
 }
-- 
2.39.0.1215.g1ba3f685d4f


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

* [PATCH v2 2/6] bisect--helper: make the order consistently `argc, argv`
  2023-01-12 15:19 ` [PATCH v2 " Ævar Arnfjörð Bjarmason
  2023-01-12 15:19   ` [PATCH v2 1/6] bisect--helper: simplify exit code computation Ævar Arnfjörð Bjarmason
@ 2023-01-12 15:19   ` Ævar Arnfjörð Bjarmason
  2023-01-12 15:19   ` [PATCH v2 3/6] bisect: verify that a bogus option won't try to start a bisection Ævar Arnfjörð Bjarmason
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 15:19 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Johannes Schindelin,
	Đoàn Trần Công Danh,
	Ævar Arnfjörð Bjarmason, Johannes Schindelin

In C, the natural order is for `argc` to come before `argv` by virtue of
the `main()` function declaring the parameters in precisely that order.

It is confusing & distracting, then, when readers familiar with the C
language read code where that order is switched around.

Let's just change the order and avoid that type of developer friction.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/bisect.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/builtin/bisect.c b/builtin/bisect.c
index 09505fc4dce..9fc8db06944 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -678,7 +678,8 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char
 	return bisect_next(terms, prefix);
 }
 
-static enum bisect_error bisect_start(struct bisect_terms *terms, const char **argv, int argc)
+static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
+				      const char **argv)
 {
 	int no_checkout = 0;
 	int first_parent_only = 0;
@@ -908,13 +909,13 @@ static int bisect_autostart(struct bisect_terms *terms)
 	yesno = git_prompt(_("Do you want me to do it for you "
 			     "[Y/n]? "), PROMPT_ECHO);
 	res = tolower(*yesno) == 'n' ?
-		-1 : bisect_start(terms, empty_strvec, 0);
+		-1 : bisect_start(terms, 0, empty_strvec);
 
 	return res;
 }
 
-static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
-				      int argc)
+static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
+				      const char **argv)
 {
 	const char *state;
 	int i, verify_expected = 1;
@@ -1033,7 +1034,7 @@ static int process_replay_line(struct bisect_terms *terms, struct strbuf *line)
 		struct strvec argv = STRVEC_INIT;
 		int res;
 		sq_dequote_to_strvec(rev, &argv);
-		res = bisect_start(terms, argv.v, argv.nr);
+		res = bisect_start(terms, argv.nr, argv.v);
 		strvec_clear(&argv);
 		return res;
 	}
@@ -1083,7 +1084,8 @@ static enum bisect_error bisect_replay(struct bisect_terms *terms, const char *f
 	return bisect_auto_next(terms, NULL);
 }
 
-static enum bisect_error bisect_skip(struct bisect_terms *terms, const char **argv, int argc)
+static enum bisect_error bisect_skip(struct bisect_terms *terms, int argc,
+				     const char **argv)
 {
 	int i;
 	enum bisect_error res;
@@ -1113,13 +1115,14 @@ static enum bisect_error bisect_skip(struct bisect_terms *terms, const char **ar
 			strvec_push(&argv_state, argv[i]);
 		}
 	}
-	res = bisect_state(terms, argv_state.v, argv_state.nr);
+	res = bisect_state(terms, argv_state.nr, argv_state.v);
 
 	strvec_clear(&argv_state);
 	return res;
 }
 
-static int bisect_visualize(struct bisect_terms *terms, const char **argv, int argc)
+static int bisect_visualize(struct bisect_terms *terms, int argc,
+			    const char **argv)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct strbuf sb = STRBUF_INIT;
@@ -1202,7 +1205,7 @@ static int verify_good(const struct bisect_terms *terms, const char *command)
 	return rc;
 }
 
-static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
+static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
 {
 	int res = BISECT_OK;
 	struct strbuf command = STRBUF_INIT;
@@ -1271,7 +1274,7 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 		saved_stdout = dup(1);
 		dup2(temporary_stdout_fd, 1);
 
-		res = bisect_state(terms, &new_state, 1);
+		res = bisect_state(terms, 1, &new_state);
 
 		fflush(stdout);
 		dup2(saved_stdout, 1);
@@ -1328,7 +1331,7 @@ static int cmd_bisect__start(int argc, const char **argv, const char *prefix UNU
 	struct bisect_terms terms = { 0 };
 
 	set_terms(&terms, "bad", "good");
-	res = bisect_start(&terms, argv, argc);
+	res = bisect_start(&terms, argc, argv);
 	free_terms(&terms);
 	return res;
 }
@@ -1372,7 +1375,7 @@ static int cmd_bisect__skip(int argc, const char **argv, const char *prefix UNUS
 
 	set_terms(&terms, "bad", "good");
 	get_terms(&terms);
-	res = bisect_skip(&terms, argv, argc);
+	res = bisect_skip(&terms, argc, argv);
 	free_terms(&terms);
 	return res;
 }
@@ -1383,7 +1386,7 @@ static int cmd_bisect__visualize(int argc, const char **argv, const char *prefix
 	struct bisect_terms terms = { 0 };
 
 	get_terms(&terms);
-	res = bisect_visualize(&terms, argv, argc);
+	res = bisect_visualize(&terms, argc, argv);
 	free_terms(&terms);
 	return res;
 }
@@ -1396,7 +1399,7 @@ static int cmd_bisect__run(int argc, const char **argv, const char *prefix UNUSE
 	if (!argc)
 		return error(_("'%s' failed: no command provided."), "git bisect run");
 	get_terms(&terms);
-	res = bisect_run(&terms, argv, argc);
+	res = bisect_run(&terms, argc, argv);
 	free_terms(&terms);
 	return res;
 }
@@ -1432,7 +1435,7 @@ int cmd_bisect(int argc, const char **argv, const char *prefix)
 		if (check_and_set_terms(&terms, argv[0]))
 			usage_msg_optf(_("unknown command: '%s'"), git_bisect_usage,
 				       options, argv[0]);
-		res = bisect_state(&terms, argv, argc);
+		res = bisect_state(&terms, argc, argv);
 		free_terms(&terms);
 	} else {
 		argc--;
-- 
2.39.0.1215.g1ba3f685d4f


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

* [PATCH v2 3/6] bisect: verify that a bogus option won't try to start a bisection
  2023-01-12 15:19 ` [PATCH v2 " Ævar Arnfjörð Bjarmason
  2023-01-12 15:19   ` [PATCH v2 1/6] bisect--helper: simplify exit code computation Ævar Arnfjörð Bjarmason
  2023-01-12 15:19   ` [PATCH v2 2/6] bisect--helper: make the order consistently `argc, argv` Ævar Arnfjörð Bjarmason
@ 2023-01-12 15:19   ` Ævar Arnfjörð Bjarmason
  2023-01-12 15:19   ` [PATCH v2 4/6] bisect run: fix the error message Ævar Arnfjörð Bjarmason
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 15:19 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Johannes Schindelin,
	Đoàn Trần Công Danh, Johannes Schindelin,
	Ævar Arnfjörð Bjarmason

From: Johannes Schindelin <johannes.schindelin@gmx.de>

We do not want `git bisect --bogus-option` to start a bisection. To
verify that, we look for the tell-tale error message `You need to start
by "git bisect start"` and fail if it was found.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6030-bisect-porcelain.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 98a72ff78a7..9e56b42b5da 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -1058,6 +1058,16 @@ test_expect_success 'bisect start with one term1 and term2' '
 	git bisect reset
 '
 
+test_expect_success 'bogus command does not start bisect' '
+	git bisect reset &&
+	test_must_fail git bisect --bisect-terms 1 2 2>out &&
+	! grep "You need to start" out &&
+	test_must_fail git bisect --bisect-terms 2>out &&
+	! grep "You need to start" out &&
+	grep "git bisect.*visualize" out &&
+	git bisect reset
+'
+
 test_expect_success 'bisect replay with term1 and term2' '
 	git bisect replay log_to_replay.txt >bisect_result &&
 	grep "$HASH2 is the first term1 commit" bisect_result &&
-- 
2.39.0.1215.g1ba3f685d4f


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

* [PATCH v2 4/6] bisect run: fix the error message
  2023-01-12 15:19 ` [PATCH v2 " Ævar Arnfjörð Bjarmason
                     ` (2 preceding siblings ...)
  2023-01-12 15:19   ` [PATCH v2 3/6] bisect: verify that a bogus option won't try to start a bisection Ævar Arnfjörð Bjarmason
@ 2023-01-12 15:19   ` Ævar Arnfjörð Bjarmason
  2023-01-12 15:19   ` [PATCH v2 5/6] bisect: remove Cogito-related code Ævar Arnfjörð Bjarmason
  2023-01-12 15:19   ` [PATCH v2 6/6] bisect: no longer try to clean up left-over `.git/head-name` files Ævar Arnfjörð Bjarmason
  5 siblings, 0 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 15:19 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Johannes Schindelin,
	Đoàn Trần Công Danh, Johannes Schindelin,
	Elijah Newren, Ævar Arnfjörð Bjarmason

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In d1bbbe45df8 (bisect--helper: reimplement `bisect_run` shell function
in C, 2021-09-13), we ported the `bisect run` subcommand to C, including
the part that prints out an error message when the implicit `git bisect
bad` or `git bisect good` failed.

However, the error message was supposed to print out whether the state
was "good" or "bad", but used a bogus (because non-populated) `args`
variable for it. This was fixed in [1], but as of [2] (when
`bisect--helper` was changed to the present `bisect-state') the error
message still talks about implementation details that should not
concern end users.

Fix that, and add a regression test to ensure that the intended form of
the error message.

1. 80c2e9657f2 (bisect--helper: report actual bisect_state() argument
   on error, 2022-01-18
2. f37d0bdd42d (bisect: fix output regressions in v2.30.0, 2022-11-10)

Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bisect.c            |  2 +-
 t/t6030-bisect-porcelain.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/bisect.c b/builtin/bisect.c
index 9fc8db06944..0786ebf4012 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -1292,7 +1292,7 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
 			puts(_("bisect found first bad commit"));
 			res = BISECT_OK;
 		} else if (res) {
-			error(_("bisect run failed: 'bisect-state %s'"
+			error(_("bisect run failed: 'git bisect %s'"
 				" exited with error code %d"), new_state, res);
 		} else {
 			continue;
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 9e56b42b5da..0a62ea2b3ce 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -1221,4 +1221,14 @@ test_expect_success 'bisect state output with bad commit' '
 	grep -F "waiting for good commit(s), bad commit known" output
 '
 
+test_expect_success 'verify correct error message' '
+	git bisect reset &&
+	git bisect start $HASH4 $HASH1 &&
+	write_script test_script.sh <<-\EOF &&
+	rm .git/BISECT*
+	EOF
+	test_must_fail git bisect run ./test_script.sh 2>error &&
+	grep "git bisect good.*exited with error code" error
+'
+
 test_done
-- 
2.39.0.1215.g1ba3f685d4f


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

* [PATCH v2 5/6] bisect: remove Cogito-related code
  2023-01-12 15:19 ` [PATCH v2 " Ævar Arnfjörð Bjarmason
                     ` (3 preceding siblings ...)
  2023-01-12 15:19   ` [PATCH v2 4/6] bisect run: fix the error message Ævar Arnfjörð Bjarmason
@ 2023-01-12 15:19   ` Ævar Arnfjörð Bjarmason
  2023-01-12 15:19   ` [PATCH v2 6/6] bisect: no longer try to clean up left-over `.git/head-name` files Ævar Arnfjörð Bjarmason
  5 siblings, 0 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 15:19 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Johannes Schindelin,
	Đoàn Trần Công Danh, Johannes Schindelin,
	Ævar Arnfjörð Bjarmason

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Once upon a time, there was this idea that Git would not actually be a
single coherent program, but rather a set of low-level programs that
users cobble together via shell scripts, or develop high-level user
interfaces for Git, or both.

Cogito was such a high-level user interface, incidentally implemented
via shell scripts that cobble together Git calls.

It did turn out relatively quickly that Git would much rather provide a
useful high-level user interface itself.

As of April 19th, 2007, Cogito was therefore discontinued (see
https://lore.kernel.org/git/20070419124648.GL4489@pasky.or.cz/).

Nevertheless, for almost 15 years after that announcement, Git carried
special code in `git bisect` to accommodate Cogito.

Since it is beyond doubt that there are no more Cogito users, let's
remove the last remnant of Cogito-accommodating code.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bisect.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/builtin/bisect.c b/builtin/bisect.c
index 0786ebf4012..73017402671 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -15,7 +15,6 @@ static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
 static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
-static GIT_PATH_FUNC(git_path_head_name, "head-name")
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
 static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
@@ -808,13 +807,6 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
 			strbuf_addstr(&start_head, oid_to_hex(&head_oid));
 		} else if (!get_oid(head, &head_oid) &&
 			   skip_prefix(head, "refs/heads/", &head)) {
-			/*
-			 * This error message should only be triggered by
-			 * cogito usage, and cogito users should understand
-			 * it relates to cg-seek.
-			 */
-			if (!is_empty_or_missing_file(git_path_head_name()))
-				return error(_("won't bisect on cg-seek'ed tree"));
 			strbuf_addstr(&start_head, head);
 		} else {
 			return error(_("bad HEAD - strange symbolic ref"));
-- 
2.39.0.1215.g1ba3f685d4f


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

* [PATCH v2 6/6] bisect: no longer try to clean up left-over `.git/head-name` files
  2023-01-12 15:19 ` [PATCH v2 " Ævar Arnfjörð Bjarmason
                     ` (4 preceding siblings ...)
  2023-01-12 15:19   ` [PATCH v2 5/6] bisect: remove Cogito-related code Ævar Arnfjörð Bjarmason
@ 2023-01-12 15:19   ` Ævar Arnfjörð Bjarmason
  5 siblings, 0 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 15:19 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Johannes Schindelin,
	Đoàn Trần Công Danh, Johannes Schindelin,
	Ævar Arnfjörð Bjarmason

From: Johannes Schindelin <johannes.schindelin@gmx.de>

As per the code comment, the `.git/head-name` files were cleaned up for
backwards-compatibility: an old version of `git bisect` could have left
them behind.

Now, just how old would such a version be? As of 0f497e75f05 (Eliminate
confusing "won't bisect on seeked tree" failure, 2008-02-23), `git
bisect` does not write that file anymore. Which corresponds to Git
v1.5.4.4.

Even if the likelihood is non-nil that there might still be users out
there who use such an old version to start a bisection, but then decide
to continue bisecting with a current Git version, it is highly
improbable.

So let's remove that code, at long last.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 bisect.c                    | 3 ---
 t/t6030-bisect-porcelain.sh | 1 -
 2 files changed, 4 deletions(-)

diff --git a/bisect.c b/bisect.c
index ec7487e6836..ef5ee5a6436 100644
--- a/bisect.c
+++ b/bisect.c
@@ -472,7 +472,6 @@ static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
-static GIT_PATH_FUNC(git_path_head_name, "head-name")
 
 static void read_bisect_paths(struct strvec *array)
 {
@@ -1188,8 +1187,6 @@ int bisect_clean_state(void)
 	unlink_or_warn(git_path_bisect_run());
 	unlink_or_warn(git_path_bisect_terms());
 	unlink_or_warn(git_path_bisect_first_parent());
-	/* Cleanup head-name if it got left by an old version of git-bisect */
-	unlink_or_warn(git_path_head_name());
 	/*
 	 * Cleanup BISECT_START last to support the --no-checkout option
 	 * introduced in the commit 4796e823a.
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 0a62ea2b3ce..3ba4fdf6153 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -1158,7 +1158,6 @@ test_expect_success 'git bisect reset cleans bisection state properly' '
 	test_path_is_missing ".git/BISECT_LOG" &&
 	test_path_is_missing ".git/BISECT_RUN" &&
 	test_path_is_missing ".git/BISECT_TERMS" &&
-	test_path_is_missing ".git/head-name" &&
 	test_path_is_missing ".git/BISECT_HEAD" &&
 	test_path_is_missing ".git/BISECT_START"
 '
-- 
2.39.0.1215.g1ba3f685d4f


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

end of thread, other threads:[~2023-01-12 15:27 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-15  9:47 [PATCH 0/6] bisect: follow-up fixes from js/bisect-in-c Ævar Arnfjörð Bjarmason
2022-12-15  9:47 ` [PATCH 1/6] bisect--helper: simplify exit code computation Ævar Arnfjörð Bjarmason
2022-12-15  9:47 ` [PATCH 2/6] bisect--helper: make the order consistently `argc, argv` Ævar Arnfjörð Bjarmason
2022-12-15  9:47 ` [PATCH 3/6] bisect: verify that a bogus option won't try to start a bisection Ævar Arnfjörð Bjarmason
2022-12-15  9:47 ` [PATCH 4/6] bisect run: fix the error message Ævar Arnfjörð Bjarmason
2022-12-15 15:08   ` Đoàn Trần Công Danh
2022-12-15  9:47 ` [PATCH 5/6] bisect: remove Cogito-related code Ævar Arnfjörð Bjarmason
2022-12-15  9:47 ` [PATCH 6/6] bisect: no longer try to clean up left-over `.git/head-name` files Ævar Arnfjörð Bjarmason
2022-12-15 10:45 ` [PATCH 0/6] bisect: follow-up fixes from js/bisect-in-c Christian Couder
2023-01-12 15:19 ` [PATCH v2 " Ævar Arnfjörð Bjarmason
2023-01-12 15:19   ` [PATCH v2 1/6] bisect--helper: simplify exit code computation Ævar Arnfjörð Bjarmason
2023-01-12 15:19   ` [PATCH v2 2/6] bisect--helper: make the order consistently `argc, argv` Ævar Arnfjörð Bjarmason
2023-01-12 15:19   ` [PATCH v2 3/6] bisect: verify that a bogus option won't try to start a bisection Ævar Arnfjörð Bjarmason
2023-01-12 15:19   ` [PATCH v2 4/6] bisect run: fix the error message Ævar Arnfjörð Bjarmason
2023-01-12 15:19   ` [PATCH v2 5/6] bisect: remove Cogito-related code Ævar Arnfjörð Bjarmason
2023-01-12 15:19   ` [PATCH v2 6/6] bisect: no longer try to clean up left-over `.git/head-name` files Ævar Arnfjörð Bjarmason

Code repositories for project(s) associated with this public 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).