All of lore.kernel.org
 help / color / mirror / 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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.