* [PATCH v3 00/12] Finish converting git bisect to C part 2
@ 2020-04-23 7:06 Miriam Rubio
2020-04-23 7:06 ` [PATCH v3 01/12] bisect--helper: fix `cmd_*()` function switch default return Miriam Rubio
` (13 more replies)
0 siblings, 14 replies; 28+ messages in thread
From: Miriam Rubio @ 2020-04-23 7:06 UTC (permalink / raw)
To: git; +Cc: Miriam Rubio
These patches correspond to a second part of patch series
of Outreachy project "Finish converting `git bisect` from shell to C"
started by Pranit Bauva and Tanushree Tumane
(https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and
continued by me.
These patch series emails were generated from:
https://gitlab.com/mirucam/git/commits/git-bisect-work-part2-v3.
I would like to thank Junio Hamano for reviewing this patch series and
Christian Couder for his help.
--- Changes since v2 Finish converting git bisect to C part 2 patch series ---
General changes
---------------
* Rebase on master branch: efe3874640 (Sync with v2.26.1, 2020-04-13)
Specific changes
----------------
[1/12] bisect--helper: fix `cmd_*()` function switch default return
* Use `BUG()` instead of `return error()` in default switch.
--
[2/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return
* New patch: use '-res' instead of 'abs(res)'.
--
[3/12] bisect--helper: introduce new `write_in_file()` function
* Rename input parameter `filepath` to `path`.
* Change `error_errno()` to `error()` in mode checking.
* Change error message when file cannot be opened.
* Add `fclose()` before error return.
--
[4/12] bisect--helper: reimplement `bisect_autostart` shell function in C
* Reorder patch before `reimplement `bisect_next` and `bisect_auto_next`
shell functions in C` to use `bisect_autostart()` function in
`bisect_append_log_quoted()`.
--
`
[5/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
functions in C
* Amend commit message.
* Add `clear_commit_marks()` at the end of bisect_next_all() and remove it from
`bisect_rev_setup()`.
* Fix if condition.
* Add `bisect_autostart()` in `bisect_append_log_quoted()`.
* Check `git_path_bisect_head()` with `file_exist()` instead of
`is_empty_or_missing_file()`.
* Fix return with BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.
* Add conversion in `cmd_*()` to BISECT_OK when return is
BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.
* Note to previous reviewer: suggestion to change `bisect_auto_next()` function
to return void cannot be done because errors from `bisect_next()` are not
ignored.
--
[5/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell
functions in C
* Check `git_path_bisect_head()` with `file_exist()` instead of
`is_empty_or_missing_file()`.
--
Miriam Rubio (3):
bisect--helper: fix `cmd_*()` function switch default return
bisect--helper: use '-res' in 'cmd_bisect__helper' return
bisect--helper: introduce new `write_in_file()` function
Pranit Bauva (9):
bisect--helper: reimplement `bisect_autostart` shell function in C
bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
functions in C
bisect--helper: finish porting `bisect_start()` to C
bisect--helper: retire `--bisect-clean-state` subcommand
bisect--helper: retire `--next-all` subcommand
bisect--helper: reimplement `bisect_state` & `bisect_head` shell
functions in C
bisect--helper: retire `--check-expected-revs` subcommand
bisect--helper: retire `--write-terms` subcommand
bisect--helper: retire `--bisect-autostart` subcommand
bisect.c | 8 +
builtin/bisect--helper.c | 383 +++++++++++++++++++++++++++++++++------
git-bisect.sh | 145 +--------------
3 files changed, 347 insertions(+), 189 deletions(-)
--
2.25.0
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 01/12] bisect--helper: fix `cmd_*()` function switch default return
2020-04-23 7:06 [PATCH v3 00/12] Finish converting git bisect to C part 2 Miriam Rubio
@ 2020-04-23 7:06 ` Miriam Rubio
2020-05-22 13:14 ` Johannes Schindelin
2020-04-23 7:06 ` [PATCH v3 02/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return Miriam Rubio
` (12 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Miriam Rubio @ 2020-04-23 7:06 UTC (permalink / raw)
To: git; +Cc: Miriam Rubio, Christian Couder, Johannes Schindelin
In a `cmd_*()` function, return `error()` cannot be used
because that translates to `-1` and `cmd_*()` functions need
to return exit codes.
Let's fix switch default return.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c1c40b516d..a81a2c76ff 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -711,7 +711,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
res = bisect_start(&terms, no_checkout, argv, argc);
break;
default:
- return error("BUG: unknown subcommand '%d'", cmdmode);
+ BUG("unknown subcommand %d", (int)cmdmode);
}
free_terms(&terms);
--
2.25.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 02/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return
2020-04-23 7:06 [PATCH v3 00/12] Finish converting git bisect to C part 2 Miriam Rubio
2020-04-23 7:06 ` [PATCH v3 01/12] bisect--helper: fix `cmd_*()` function switch default return Miriam Rubio
@ 2020-04-23 7:06 ` Miriam Rubio
2020-05-22 13:16 ` Johannes Schindelin
2020-04-23 7:06 ` [PATCH v3 03/12] bisect--helper: introduce new `write_in_file()` function Miriam Rubio
` (11 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Miriam Rubio @ 2020-04-23 7:06 UTC (permalink / raw)
To: git; +Cc: Miriam Rubio, Christian Couder, Junio C Hamano
Following 'enum bisect_error' vocabulary, return variable 'res' is
always non-positive.
Let's use '-res' instead of 'abs(res)' to make the code clearer.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
---
builtin/bisect--helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index a81a2c76ff..0fbd924aac 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -722,5 +722,5 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)
res = BISECT_OK;
- return abs(res);
+ return -res;
}
--
2.25.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 03/12] bisect--helper: introduce new `write_in_file()` function
2020-04-23 7:06 [PATCH v3 00/12] Finish converting git bisect to C part 2 Miriam Rubio
2020-04-23 7:06 ` [PATCH v3 01/12] bisect--helper: fix `cmd_*()` function switch default return Miriam Rubio
2020-04-23 7:06 ` [PATCH v3 02/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return Miriam Rubio
@ 2020-04-23 7:06 ` Miriam Rubio
2020-05-22 13:25 ` Johannes Schindelin
2020-05-23 1:53 ` Đoàn Trần Công Danh
2020-04-23 7:06 ` [PATCH v3 04/12] bisect--helper: reimplement `bisect_autostart` shell function in C Miriam Rubio
` (10 subsequent siblings)
13 siblings, 2 replies; 28+ messages in thread
From: Miriam Rubio @ 2020-04-23 7:06 UTC (permalink / raw)
To: git; +Cc: Miriam Rubio, Christian Couder, Johannes Schindelin
Let's refactor code adding a new `write_in_file()` function
that opens a file for writing a message and closes it.
This helper will be used in later steps and makes the code
simpler and easier to understand.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 29 +++++++++++++++++++++++------
1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 0fbd924aac..d3b2b33df0 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -74,6 +74,28 @@ static int one_of(const char *term, ...)
return res;
}
+static int write_in_file(const char *path, const char *mode, const char *format,...)
+{
+ FILE *fp = NULL;
+ va_list args;
+ int res = 0;
+
+ if (!strcmp(mode, "a") && !strcmp(mode, "w"))
+ return error(_("wrong writing mode '%s'"), mode);
+ fp = fopen(path, mode);
+ if (!fp)
+ return error_errno(_("cannot open file '%s' in mode '%s'"), path, mode);
+ va_start(args, format);
+ res = vfprintf(fp, format, args);
+ va_end(args);
+ if (!res) {
+ fclose(fp);
+ return error_errno(_("could not write to file '%s'"), path);
+ }
+
+ return fclose(fp);
+}
+
static int check_term_format(const char *term, const char *orig_term)
{
int res;
@@ -104,7 +126,6 @@ static int check_term_format(const char *term, const char *orig_term)
static int write_terms(const char *bad, const char *good)
{
- FILE *fp = NULL;
int res;
if (!strcmp(bad, good))
@@ -113,12 +134,8 @@ static int write_terms(const char *bad, const char *good)
if (check_term_format(bad, "bad") || check_term_format(good, "good"))
return -1;
- fp = fopen(git_path_bisect_terms(), "w");
- if (!fp)
- return error_errno(_("could not open the file BISECT_TERMS"));
+ res = write_in_file(git_path_bisect_terms(), "w", "%s\n%s\n", bad, good);
- res = fprintf(fp, "%s\n%s\n", bad, good);
- res |= fclose(fp);
return (res < 0) ? -1 : 0;
}
--
2.25.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 04/12] bisect--helper: reimplement `bisect_autostart` shell function in C
2020-04-23 7:06 [PATCH v3 00/12] Finish converting git bisect to C part 2 Miriam Rubio
` (2 preceding siblings ...)
2020-04-23 7:06 ` [PATCH v3 03/12] bisect--helper: introduce new `write_in_file()` function Miriam Rubio
@ 2020-04-23 7:06 ` Miriam Rubio
2020-05-22 19:27 ` Johannes Schindelin
2020-04-23 7:06 ` [PATCH v3 05/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions " Miriam Rubio
` (9 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Miriam Rubio @ 2020-04-23 7:06 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Lars Schneider, Christian Couder,
Johannes Schindelin, Tanushree Tumane, Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
Reimplement the `bisect_autostart()` shell function in C and add the
C implementation from `bisect_next()` which was previously left
uncovered. Also add a subcommand `--bisect-autostart` to
`git bisect--helper` be called from `bisect_state()` from
git-bisect.sh .
Using `--bisect-autostart` subcommand is a temporary measure to port
shell function to C so as to use the existing test suite. As more
functions are ported, this subcommand will be retired and
bisect_autostart() will be called directly by `bisect_state()`.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 40 +++++++++++++++++++++++++++++++++++++++-
git-bisect.sh | 25 ++-----------------------
2 files changed, 41 insertions(+), 24 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index d3b2b33df0..9df69800e3 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -29,6 +29,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
N_("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
"[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
+ N_("git bisect--helper --bisect-autostart"),
NULL
};
@@ -55,6 +56,8 @@ static void set_terms(struct bisect_terms *terms, const char *bad,
static const char vocab_bad[] = "bad|new";
static const char vocab_good[] = "good|old";
+static int bisect_autostart(struct bisect_terms *terms);
+
/*
* Check whether the string `term` belongs to the set of strings
* included in the variable arguments.
@@ -630,6 +633,32 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
return res;
}
+static int bisect_autostart(struct bisect_terms *terms)
+{
+ const char *yesno;
+
+ if (!is_empty_or_missing_file(git_path_bisect_start()))
+ return 0;
+
+ fprintf(stderr, _("You need to start by \"git bisect "
+ "start\"\n"));
+
+ if (!isatty(STDIN_FILENO))
+ return 1;
+
+ /*
+ * TRANSLATORS: Make sure to include [Y] and [n] in your
+ * translation. The program will only accept English input
+ * at this point.
+ */
+ yesno = git_prompt(_("Do you want me to do it for you "
+ "[Y/n]? "), PROMPT_ECHO);
+ if (starts_with(yesno, _("n")) || starts_with(yesno, _("N")))
+ return 1;
+
+ return bisect_start(terms, 0, NULL, 0);
+}
+
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
@@ -642,7 +671,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
CHECK_AND_SET_TERMS,
BISECT_NEXT_CHECK,
BISECT_TERMS,
- BISECT_START
+ BISECT_START,
+ BISECT_AUTOSTART,
} cmdmode = 0;
int no_checkout = 0, res = 0, nolog = 0;
struct option options[] = {
@@ -666,6 +696,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
N_("print out the bisect terms"), BISECT_TERMS),
OPT_CMDMODE(0, "bisect-start", &cmdmode,
N_("start the bisect session"), BISECT_START),
+ OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
+ N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
OPT_BOOL(0, "no-checkout", &no_checkout,
N_("update BISECT_HEAD instead of checking out the current commit")),
OPT_BOOL(0, "no-log", &nolog,
@@ -727,6 +759,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
set_terms(&terms, "bad", "good");
res = bisect_start(&terms, no_checkout, argv, argc);
break;
+ case BISECT_AUTOSTART:
+ if (argc)
+ return error(_("--bisect-autostart requires 0 arguments"));
+ set_terms(&terms, "bad", "good");
+ res = bisect_autostart(&terms);
+ break;
default:
BUG("unknown subcommand %d", (int)cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index efee12b8b1..426d443e7e 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -49,27 +49,6 @@ bisect_head()
fi
}
-bisect_autostart() {
- test -s "$GIT_DIR/BISECT_START" || {
- gettextln "You need to start by \"git bisect start\"" >&2
- if test -t 0
- then
- # TRANSLATORS: Make sure to include [Y] and [n] in your
- # translation. The program will only accept English input
- # at this point.
- gettext "Do you want me to do it for you [Y/n]? " >&2
- read yesno
- case "$yesno" in
- [Nn]*)
- exit ;;
- esac
- bisect_start
- else
- exit 1
- fi
- }
-}
-
bisect_start() {
git bisect--helper --bisect-start $@ || exit
@@ -108,7 +87,7 @@ bisect_skip() {
}
bisect_state() {
- bisect_autostart
+ git bisect--helper --bisect-autostart
state=$1
git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
get_terms
@@ -149,7 +128,7 @@ bisect_auto_next() {
bisect_next() {
case "$#" in 0) ;; *) usage ;; esac
- bisect_autostart
+ git bisect--helper --bisect-autostart
git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit
# Perform all bisection computation, display and checkout
--
2.25.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 05/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C
2020-04-23 7:06 [PATCH v3 00/12] Finish converting git bisect to C part 2 Miriam Rubio
` (3 preceding siblings ...)
2020-04-23 7:06 ` [PATCH v3 04/12] bisect--helper: reimplement `bisect_autostart` shell function in C Miriam Rubio
@ 2020-04-23 7:06 ` Miriam Rubio
2020-05-22 20:47 ` Johannes Schindelin
2020-04-23 7:06 ` [PATCH v3 06/12] bisect--helper: finish porting `bisect_start()` to C Miriam Rubio
` (8 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Miriam Rubio @ 2020-04-23 7:06 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Lars Schneider, Christian Couder,
Johannes Schindelin, Tanushree Tumane, Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
Reimplement the `bisect_next()` and the `bisect_auto_next()` shell functions
in C and add the subcommands to `git bisect--helper` to call them from
git-bisect.sh .
bisect_auto_next() function returns an enum bisect_error type as whole
`git bisect` can exit with an error code when bisect_next() does.
Using `--bisect-next` and `--bisect-auto-next` subcommands is a
temporary measure to port shell function to C so as to use the existing
test suite. As more functions are ported, `--bisect-auto-next`
subcommand will be retired and will be called by some other methods.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
bisect.c | 8 ++
builtin/bisect--helper.c | 169 ++++++++++++++++++++++++++++++++++++++-
git-bisect.sh | 47 +----------
3 files changed, 179 insertions(+), 45 deletions(-)
diff --git a/bisect.c b/bisect.c
index 9154f810f7..31cab3f0b0 100644
--- a/bisect.c
+++ b/bisect.c
@@ -980,6 +980,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
* the bisection process finished successfully.
* In this case the calling function or command should not turn a
* BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND return code into an error or a non zero exit code.
+ *
+ * Checking BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND
+ * in bisect_helper::bisect_next() and only transforming it to 0 at
+ * the end of bisect_helper::cmd_bisect__helper() helps bypassing
+ * all the code related to finding a commit to test.
+ *
* If no_checkout is non-zero, the bisection process does not
* checkout the trial commit but instead simply updates BISECT_HEAD.
*/
@@ -1064,6 +1070,8 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
"Bisecting: %d revisions left to test after this %s\n",
nr), nr, steps_msg);
free(steps_msg);
+ /* Clean up objects used, as they will be reused. */
+ clear_commit_marks_all(ALL_REV_FLAGS);
return bisect_checkout(bisect_rev, no_checkout);
}
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 9df69800e3..c6aaa8eb15 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -8,6 +8,7 @@
#include "run-command.h"
#include "prompt.h"
#include "quote.h"
+#include "revision.h"
static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -29,6 +30,8 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
N_("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
"[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
+ N_("git bisect--helper --bisect-next"),
+ N_("git bisect--helper --bisect-auto-next"),
N_("git bisect--helper --bisect-autostart"),
NULL
};
@@ -441,6 +444,150 @@ static int bisect_append_log_quoted(const char **argv)
return res;
}
+static int register_good_ref(const char *refname,
+ const struct object_id *oid, int flags,
+ void *cb_data)
+{
+ struct argv_array *rev_argv = cb_data;
+
+ argv_array_push(rev_argv, oid_to_hex(oid));
+ return 0;
+}
+
+static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)
+{
+ char *term_good = xstrfmt("%s-*", terms->term_good);
+
+ argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);
+ for_each_glob_ref_in(register_good_ref, term_good, "refs/bisect/", rev_argv);
+
+ free(term_good);
+}
+
+static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)
+{
+ int res = 0;
+ struct argv_array rev_argv = ARGV_ARRAY_INIT;
+
+ prepare_rev_argv(terms, &rev_argv);
+
+ /*
+ * It is important to reset the flags used by revision walks
+ * as the previous call to bisect_next_all() in turn
+ * sets up a revision walk.
+ */
+ reset_revision_walk();
+ init_revisions(revs, NULL);
+ rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL);
+ if (prepare_revision_walk(revs))
+ res = error(_("revision walk setup failed\n"));
+
+ argv_array_clear(&rev_argv);
+ return res;
+}
+
+static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)
+{
+ struct commit *commit;
+ struct pretty_print_context pp = {0};
+
+ if (fprintf(fp, "# only skipped commits left to test\n") < 0)
+ return error_errno(_("failed to write to '%s'"), git_path_bisect_log());
+
+ while ((commit = get_revision(revs)) != NULL) {
+ struct strbuf commit_name = STRBUF_INIT;
+ format_commit_message(commit, "%s",
+ &commit_name, &pp);
+ fprintf(fp, "# possible first %s commit: [%s] %s\n",
+ terms->term_bad, oid_to_hex(&commit->object.oid),
+ commit_name.buf);
+ strbuf_release(&commit_name);
+ }
+
+ /*
+ * Reset the flags used by revision walks in case
+ * there is another revision walk after this one.
+ */
+ reset_revision_walk();
+
+ return 0;
+}
+
+static int bisect_skipped_commits(struct bisect_terms *terms)
+{
+ int res = 0;
+ FILE *fp = NULL;
+ struct rev_info revs;
+
+ fp = fopen(git_path_bisect_log(), "a");
+ if (!fp)
+ return error_errno(_("could not open '%s' for appending"),
+ git_path_bisect_log());
+
+ res = prepare_revs(terms, &revs);
+
+ if (!res)
+ res = process_skipped_commits(fp, terms, &revs);
+
+ fclose(fp);
+ return res;
+}
+
+static int bisect_successful(struct bisect_terms *terms)
+{
+ struct object_id oid;
+ struct commit *commit;
+ struct pretty_print_context pp = {0};
+ struct strbuf commit_name = STRBUF_INIT;
+ char *bad_ref = xstrfmt("refs/bisect/%s",terms->term_bad);
+ int res;
+
+ read_ref(bad_ref, &oid);
+ printf("%s\n", bad_ref);
+ commit = lookup_commit_reference(the_repository, &oid);
+ format_commit_message(commit, "%s", &commit_name, &pp);
+
+ res = write_in_file(git_path_bisect_log(), "a", "# first %s commit: [%s] %s\n",
+ terms->term_bad, oid_to_hex(&oid),
+ commit_name.buf);
+
+ strbuf_release(&commit_name);
+ free(bad_ref);
+ return res;
+}
+
+static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
+{
+ int no_checkout;
+ enum bisect_error res;
+
+ bisect_autostart(terms);
+ if (bisect_next_check(terms, terms->term_good))
+ return BISECT_FAILED;
+
+ no_checkout = file_exists(git_path_bisect_head());
+
+ /* Perform all bisection computation, display and checkout */
+ res = bisect_next_all(the_repository, prefix, no_checkout);
+
+ if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
+ res = bisect_successful(terms);
+ return res ? res : BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND;
+ } else if (res == BISECT_ONLY_SKIPPED_LEFT) {
+ res = bisect_skipped_commits(terms);
+ return res ? res : BISECT_ONLY_SKIPPED_LEFT;
+ }
+ return res;
+}
+
+static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)
+{
+ if (bisect_next_check(terms, NULL))
+ return BISECT_OK;
+
+ return bisect_next(terms, prefix);
+}
+
static int bisect_start(struct bisect_terms *terms, int no_checkout,
const char **argv, int argc)
{
@@ -672,7 +819,9 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
BISECT_NEXT_CHECK,
BISECT_TERMS,
BISECT_START,
- BISECT_AUTOSTART,
+ BISECT_NEXT,
+ BISECT_AUTO_NEXT,
+ BISECT_AUTOSTART
} cmdmode = 0;
int no_checkout = 0, res = 0, nolog = 0;
struct option options[] = {
@@ -696,6 +845,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
N_("print out the bisect terms"), BISECT_TERMS),
OPT_CMDMODE(0, "bisect-start", &cmdmode,
N_("start the bisect session"), BISECT_START),
+ OPT_CMDMODE(0, "bisect-next", &cmdmode,
+ N_("find the next bisection commit"), BISECT_NEXT),
+ OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
+ N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
OPT_BOOL(0, "no-checkout", &no_checkout,
@@ -759,6 +912,18 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
set_terms(&terms, "bad", "good");
res = bisect_start(&terms, no_checkout, argv, argc);
break;
+ case BISECT_NEXT:
+ if (argc)
+ return error(_("--bisect-next requires 0 arguments"));
+ get_terms(&terms);
+ res = bisect_next(&terms, prefix);
+ break;
+ case BISECT_AUTO_NEXT:
+ if (argc)
+ return error(_("--bisect-auto-next requires 0 arguments"));
+ get_terms(&terms);
+ res = bisect_auto_next(&terms, prefix);
+ break;
case BISECT_AUTOSTART:
if (argc)
return error(_("--bisect-autostart requires 0 arguments"));
@@ -774,7 +939,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
* Handle early success
* From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all
*/
- if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)
+ if ((res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) || (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND))
res = BISECT_OK;
return -res;
diff --git a/git-bisect.sh b/git-bisect.sh
index 426d443e7e..897825b675 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -65,8 +65,7 @@ bisect_start() {
#
# Check if we can proceed to the next bisect state.
#
- get_terms
- bisect_auto_next
+ git bisect--helper --bisect-auto-next || exit
trap '-' 0
}
@@ -119,45 +118,7 @@ bisect_state() {
*)
usage ;;
esac
- bisect_auto_next
-}
-
-bisect_auto_next() {
- git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :
-}
-
-bisect_next() {
- case "$#" in 0) ;; *) usage ;; esac
- git bisect--helper --bisect-autostart
- git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit
-
- # Perform all bisection computation, display and checkout
- git bisect--helper --next-all $(test -f "$GIT_DIR/BISECT_HEAD" && echo --no-checkout)
- res=$?
-
- # Check if we should exit because bisection is finished
- if test $res -eq 10
- then
- bad_rev=$(git show-ref --hash --verify refs/bisect/$TERM_BAD)
- bad_commit=$(git show-branch $bad_rev)
- echo "# first $TERM_BAD commit: $bad_commit" >>"$GIT_DIR/BISECT_LOG"
- exit 0
- elif test $res -eq 2
- then
- echo "# only skipped commits left to test" >>"$GIT_DIR/BISECT_LOG"
- good_revs=$(git for-each-ref --format="%(objectname)" "refs/bisect/$TERM_GOOD-*")
- for skipped in $(git rev-list refs/bisect/$TERM_BAD --not $good_revs)
- do
- skipped_commit=$(git show-branch $skipped)
- echo "# possible first $TERM_BAD commit: $skipped_commit" >>"$GIT_DIR/BISECT_LOG"
- done
- exit $res
- fi
-
- # Check for an error in the bisection process
- test $res -ne 0 && exit $res
-
- return 0
+ git bisect--helper --bisect-auto-next
}
bisect_visualize() {
@@ -211,7 +172,7 @@ bisect_replay () {
die "$(gettext "?? what are you talking about?")" ;;
esac
done <"$file"
- bisect_auto_next
+ git bisect--helper --bisect-auto-next
}
bisect_run () {
@@ -308,7 +269,7 @@ case "$#" in
bisect_skip "$@" ;;
next)
# Not sure we want "next" at the UI level anymore.
- bisect_next "$@" ;;
+ git bisect--helper --bisect-next "$@" || exit ;;
visualize|view)
bisect_visualize "$@" ;;
reset)
--
2.25.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 06/12] bisect--helper: finish porting `bisect_start()` to C
2020-04-23 7:06 [PATCH v3 00/12] Finish converting git bisect to C part 2 Miriam Rubio
` (4 preceding siblings ...)
2020-04-23 7:06 ` [PATCH v3 05/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions " Miriam Rubio
@ 2020-04-23 7:06 ` Miriam Rubio
2020-05-22 21:08 ` Johannes Schindelin
2020-04-23 7:06 ` [PATCH v3 07/12] bisect--helper: retire `--bisect-clean-state` subcommand Miriam Rubio
` (7 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Miriam Rubio @ 2020-04-23 7:06 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Christian Couder, Johannes Schindelin,
Tanushree Tumane, Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
Add the subcommand to `git bisect--helper` and call it from
git-bisect.sh.
With the conversion of `bisect_auto_next()` from shell to C in a
previous commit, `bisect_start()` can now be fully ported to C.
So let's complete the `--bisect-start` subcommand of
`git bisect--helper` so that it fully implements `bisect_start()`,
and let's use this subcommand in `git-bisect.sh` instead of
`bisect_start()`.
This removes the signal handling we had in `bisect_start()` as we
don't really need it. While at it, "trap" is changed to "handle".
As "trap" is a reference to the shell "trap" builtin, which isn't
used anymore.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 54 +++++++++++++++++++++++++++++++---------
git-bisect.sh | 26 ++-----------------
2 files changed, 44 insertions(+), 36 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c6aaa8eb15..f2ec7f89e4 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -588,11 +588,12 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char
return bisect_next(terms, prefix);
}
-static int bisect_start(struct bisect_terms *terms, int no_checkout,
+static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkout,
const char **argv, int argc)
{
int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
- int flags, pathspec_pos, res = 0;
+ int flags, pathspec_pos;
+ enum bisect_error res = BISECT_OK;
struct string_list revs = STRING_LIST_INIT_DUP;
struct string_list states = STRING_LIST_INIT_DUP;
struct strbuf start_head = STRBUF_INIT;
@@ -645,9 +646,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
return error(_("unrecognized option: '%s'"), arg);
} else {
char *commit_id = xstrfmt("%s^{commit}", arg);
- if (get_oid(commit_id, &oid) && has_double_dash)
- die(_("'%s' does not appear to be a valid "
- "revision"), arg);
+ if (get_oid(commit_id, &oid) && has_double_dash) {
+ error(_("'%s' does not appear to be a valid "
+ "revision"), arg);
+ free(commit_id);
+ return BISECT_FAILED;
+ }
string_list_append(&revs, oid_to_hex(&oid));
free(commit_id);
@@ -725,12 +729,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
* Get rid of any old bisect state.
*/
if (bisect_clean_state())
- return -1;
+ return BISECT_FAILED;
/*
- * In case of mistaken revs or checkout error, or signals received,
+ * In case of mistaken revs or checkout error,
* "bisect_auto_next" below may exit or misbehave.
- * We have to trap this to be able to clean up using
+ * We have to handle this to be able to clean up using
* "bisect_clean_state".
*/
@@ -746,7 +750,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
}
if (update_ref(NULL, "BISECT_HEAD", &oid, NULL, 0,
UPDATE_REFS_MSG_ON_ERR)) {
- res = -1;
+ res = BISECT_FAILED;
goto finish;
}
}
@@ -758,25 +762,51 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
for (i = 0; i < states.nr; i++)
if (bisect_write(states.items[i].string,
revs.items[i].string, terms, 1)) {
- res = -1;
+ res = BISECT_FAILED;
goto finish;
}
if (must_write_terms && write_terms(terms->term_bad,
terms->term_good)) {
- res = -1;
+ res = BISECT_FAILED;
goto finish;
}
res = bisect_append_log_quoted(argv);
if (res)
- res = -1;
+ res = BISECT_FAILED;
finish:
string_list_clear(&revs, 0);
string_list_clear(&states, 0);
strbuf_release(&start_head);
strbuf_release(&bisect_names);
+ if (res)
+ return res;
+
+ res = bisect_auto_next(terms, NULL);
+ /*
+ * In case of mistaken revs or checkout error, or signals received,
+ * "bisect_auto_next" below may exit or misbehave.
+ * We have to handle this to be able to clean up using
+ * "bisect_clean_state".
+ * return code BISECT_INTERNAL_SUCCESS_MERGE_BASE is special code
+ * that indicates special success.
+ * -> bisect_start()
+ * . res = bisect_auto_next()
+ * -> bisect_auto_next()
+ * . return bisect_next()
+ * -> bisect_next()
+ * . res = bisect_next_all()
+ * -> bisect_next_all()
+ * . res = check_good_are_ancestors_of_bad()
+ * -> check_good_are_ancestors_of_bad()
+ * . res = check_merge_bases()
+ * -> check_merge_bases()
+ * . res = BISECT_INTERNAL_SUCCESS_MERGE_BASE
+ */
+ if (res && res != BISECT_INTERNAL_SUCCESS_MERGE_BASE)
+ bisect_clean_state();
return res;
}
diff --git a/git-bisect.sh b/git-bisect.sh
index 897825b675..049ffacdff 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -49,27 +49,6 @@ bisect_head()
fi
}
-bisect_start() {
- git bisect--helper --bisect-start $@ || exit
-
- #
- # Change state.
- # In case of mistaken revs or checkout error, or signals received,
- # "bisect_auto_next" below may exit or misbehave.
- # We have to trap this to be able to clean up using
- # "bisect_clean_state".
- #
- trap 'git bisect--helper --bisect-clean-state' 0
- trap 'exit 255' 1 2 3 15
-
- #
- # Check if we can proceed to the next bisect state.
- #
- git bisect--helper --bisect-auto-next || exit
-
- trap '-' 0
-}
-
bisect_skip() {
all=''
for arg in "$@"
@@ -162,8 +141,7 @@ bisect_replay () {
get_terms
case "$command" in
start)
- cmd="bisect_start $rev"
- eval "$cmd" ;;
+ eval "git bisect--helper --bisect-start $rev" ;;
"$TERM_GOOD"|"$TERM_BAD"|skip)
git bisect--helper --bisect-write "$command" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit;;
terms)
@@ -262,7 +240,7 @@ case "$#" in
help)
git bisect -h ;;
start)
- bisect_start "$@" ;;
+ git bisect--helper --bisect-start "$@" ;;
bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
bisect_state "$cmd" "$@" ;;
skip)
--
2.25.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 07/12] bisect--helper: retire `--bisect-clean-state` subcommand
2020-04-23 7:06 [PATCH v3 00/12] Finish converting git bisect to C part 2 Miriam Rubio
` (5 preceding siblings ...)
2020-04-23 7:06 ` [PATCH v3 06/12] bisect--helper: finish porting `bisect_start()` to C Miriam Rubio
@ 2020-04-23 7:06 ` Miriam Rubio
2020-04-23 7:07 ` [PATCH v3 08/12] bisect--helper: retire `--next-all` subcommand Miriam Rubio
` (6 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Miriam Rubio @ 2020-04-23 7:06 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Lars Schneider, Christian Couder, Tanushree Tumane,
Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
The `--bisect-clean-state` subcommand is no longer used from the
git-bisect.sh shell script. Instead the function
`bisect_clean_state()` is directly called from the C
implementation.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index f2ec7f89e4..1b4d0a0f87 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -22,7 +22,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
N_("git bisect--helper --write-terms <bad_term> <good_term>"),
- N_("git bisect--helper --bisect-clean-state"),
N_("git bisect--helper --bisect-reset [<commit>]"),
N_("git bisect--helper --bisect-write [--no-log] <state> <revision> <good_term> <bad_term>"),
N_("git bisect--helper --bisect-check-and-set-terms <command> <good_term> <bad_term>"),
@@ -841,7 +840,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
enum {
NEXT_ALL = 1,
WRITE_TERMS,
- BISECT_CLEAN_STATE,
CHECK_EXPECTED_REVS,
BISECT_RESET,
BISECT_WRITE,
@@ -859,8 +857,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
N_("perform 'git bisect next'"), NEXT_ALL),
OPT_CMDMODE(0, "write-terms", &cmdmode,
N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
- OPT_CMDMODE(0, "bisect-clean-state", &cmdmode,
- N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
N_("check for expected revs"), CHECK_EXPECTED_REVS),
OPT_CMDMODE(0, "bisect-reset", &cmdmode,
@@ -904,10 +900,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
if (argc != 2)
return error(_("--write-terms requires two arguments"));
return write_terms(argv[0], argv[1]);
- case BISECT_CLEAN_STATE:
- if (argc != 0)
- return error(_("--bisect-clean-state requires no arguments"));
- return bisect_clean_state();
case CHECK_EXPECTED_REVS:
check_expected_revs(argv, argc);
return 0;
--
2.25.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 08/12] bisect--helper: retire `--next-all` subcommand
2020-04-23 7:06 [PATCH v3 00/12] Finish converting git bisect to C part 2 Miriam Rubio
` (6 preceding siblings ...)
2020-04-23 7:06 ` [PATCH v3 07/12] bisect--helper: retire `--bisect-clean-state` subcommand Miriam Rubio
@ 2020-04-23 7:07 ` Miriam Rubio
2020-04-23 7:07 ` [PATCH v3 09/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C Miriam Rubio
` (5 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Miriam Rubio @ 2020-04-23 7:07 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Lars Schneider, Christian Couder, Tanushree Tumane,
Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
The `--next-all` subcommand is no longer used from the git-bisect.sh
shell script. Instead the function `bisect_next_all()` is called from
the C implementation of `bisect_next()`.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 1b4d0a0f87..2d8660c79f 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_head_name, "head-name")
static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
static const char * const git_bisect_helper_usage[] = {
- N_("git bisect--helper --next-all [--no-checkout]"),
N_("git bisect--helper --write-terms <bad_term> <good_term>"),
N_("git bisect--helper --bisect-reset [<commit>]"),
N_("git bisect--helper --bisect-write [--no-log] <state> <revision> <good_term> <bad_term>"),
@@ -838,8 +837,7 @@ static int bisect_autostart(struct bisect_terms *terms)
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
- NEXT_ALL = 1,
- WRITE_TERMS,
+ WRITE_TERMS = 1,
CHECK_EXPECTED_REVS,
BISECT_RESET,
BISECT_WRITE,
@@ -853,8 +851,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
} cmdmode = 0;
int no_checkout = 0, res = 0, nolog = 0;
struct option options[] = {
- OPT_CMDMODE(0, "next-all", &cmdmode,
- N_("perform 'git bisect next'"), NEXT_ALL),
OPT_CMDMODE(0, "write-terms", &cmdmode,
N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
@@ -893,9 +889,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
usage_with_options(git_bisect_helper_usage, options);
switch (cmdmode) {
- case NEXT_ALL:
- res = bisect_next_all(the_repository, prefix, no_checkout);
- break;
case WRITE_TERMS:
if (argc != 2)
return error(_("--write-terms requires two arguments"));
--
2.25.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 09/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C
2020-04-23 7:06 [PATCH v3 00/12] Finish converting git bisect to C part 2 Miriam Rubio
` (7 preceding siblings ...)
2020-04-23 7:07 ` [PATCH v3 08/12] bisect--helper: retire `--next-all` subcommand Miriam Rubio
@ 2020-04-23 7:07 ` Miriam Rubio
2020-05-22 22:06 ` Johannes Schindelin
2020-04-23 7:07 ` [PATCH v3 10/12] bisect--helper: retire `--check-expected-revs` subcommand Miriam Rubio
` (4 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Miriam Rubio @ 2020-04-23 7:07 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Lars Schneider, Christian Couder,
Johannes Schindelin, Tanushree Tumane, Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
Reimplement the `bisect_state()` shell functions in C and also add a
subcommand `--bisect-state` to `git-bisect--helper` to call them from
git-bisect.sh .
Using `--bisect-state` subcommand is a temporary measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired and will be called by some
other methods.
`bisect_head()` is only called from `bisect_state()`, thus it is not
required to introduce another subcommand.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 70 +++++++++++++++++++++++++++++++++++++++-
git-bisect.sh | 55 +++----------------------------
2 files changed, 73 insertions(+), 52 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 2d8660c79f..9db72f5891 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-next"),
N_("git bisect--helper --bisect-auto-next"),
N_("git bisect--helper --bisect-autostart"),
+ N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
+ N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
NULL
};
@@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms)
return bisect_start(terms, 0, NULL, 0);
}
+static int bisect_head(struct object_id *oid)
+{
+ if (!file_exists(git_path_bisect_head()))
+ return get_oid("HEAD", oid);
+
+ return get_oid("BISECT_HEAD", oid);
+}
+
+static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
+ int argc)
+{
+ const char *state;
+ const char *hex;
+ int i;
+ struct oid_array revs = OID_ARRAY_INIT;
+ struct object_id oid;
+
+ if (!argc)
+ return error(_("Please call `--bisect-state` with at least one argument"));
+ state = argv[0];
+ if (check_and_set_terms(terms, state) ||
+ !one_of(state, terms->term_good,terms->term_bad, "skip", NULL))
+ return BISECT_FAILED;
+ argv++;
+ argc--;
+ if (!strcmp(state, terms->term_bad) && (argc > 1))
+ return error(_("'git bisect %s' can take only one argument."),terms->term_bad);
+ if (argc == 0) {
+ if (bisect_head(&oid))
+ return error(_("Bad bisect_head rev input"));
+ hex = oid_to_hex(&oid);
+ if (bisect_write(state, hex, terms, 0))
+ return BISECT_FAILED;
+ check_expected_revs(&hex, 1);
+ return bisect_auto_next(terms, NULL);
+ }
+
+ /* Here argc > 0 */
+ for (; argc; argc--, argv++) {
+ struct object_id oid;
+ if (get_oid(*argv, &oid))
+ return error(_("Bad rev input: %s"), *argv);
+ oid_array_append(&revs, &oid);
+ }
+
+ for (i = 0; i < revs.nr; i++) {
+ hex = oid_to_hex(&revs.oid[i]);
+ if (bisect_write(state, hex, terms, 0)) {
+ oid_array_clear(&revs);
+ return BISECT_FAILED;
+ }
+ check_expected_revs(&hex, 1);
+ }
+
+ oid_array_clear(&revs);
+ return bisect_auto_next(terms, NULL);
+}
+
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
@@ -847,7 +907,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
BISECT_START,
BISECT_NEXT,
BISECT_AUTO_NEXT,
- BISECT_AUTOSTART
+ BISECT_AUTOSTART,
+ BISECT_STATE
} cmdmode = 0;
int no_checkout = 0, res = 0, nolog = 0;
struct option options[] = {
@@ -873,6 +934,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
+ OPT_CMDMODE(0, "bisect-state", &cmdmode,
+ N_("mark the state of ref (or refs)"), BISECT_STATE),
OPT_BOOL(0, "no-checkout", &no_checkout,
N_("update BISECT_HEAD instead of checking out the current commit")),
OPT_BOOL(0, "no-log", &nolog,
@@ -945,6 +1008,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
set_terms(&terms, "bad", "good");
res = bisect_autostart(&terms);
break;
+ case BISECT_STATE:
+ set_terms(&terms, "bad", "good");
+ get_terms(&terms);
+ res = bisect_state(&terms, argv, argc);
+ break;
default:
BUG("unknown subcommand %d", (int)cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index 049ffacdff..2da0810b1a 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -39,16 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
TERM_BAD=bad
TERM_GOOD=good
-bisect_head()
-{
- if test -f "$GIT_DIR/BISECT_HEAD"
- then
- echo BISECT_HEAD
- else
- echo HEAD
- fi
-}
-
bisect_skip() {
all=''
for arg in "$@"
@@ -61,43 +51,7 @@ bisect_skip() {
esac
all="$all $revs"
done
- eval bisect_state 'skip' $all
-}
-
-bisect_state() {
- git bisect--helper --bisect-autostart
- state=$1
- git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
- get_terms
- case "$#,$state" in
- 0,*)
- die "Please call 'bisect_state' with at least one argument." ;;
- 1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip)
- bisected_head=$(bisect_head)
- rev=$(git rev-parse --verify "$bisected_head") ||
- die "$(eval_gettext "Bad rev input: \$bisected_head")"
- git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
- git bisect--helper --check-expected-revs "$rev" ;;
- 2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
- shift
- hash_list=''
- for rev in "$@"
- do
- sha=$(git rev-parse --verify "$rev^{commit}") ||
- die "$(eval_gettext "Bad rev input: \$rev")"
- hash_list="$hash_list $sha"
- done
- for rev in $hash_list
- do
- git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
- done
- git bisect--helper --check-expected-revs $hash_list ;;
- *,"$TERM_BAD")
- die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;;
- *)
- usage ;;
- esac
- git bisect--helper --bisect-auto-next
+ eval git bisect--helper --bisect-state 'skip' $all
}
bisect_visualize() {
@@ -185,8 +139,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
state="$TERM_GOOD"
fi
- # We have to use a subshell because "bisect_state" can exit.
- ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
+ git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN"
res=$?
cat "$GIT_DIR/BISECT_RUN"
@@ -201,7 +154,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
if [ $res -ne 0 ]
then
eval_gettextln "bisect run failed:
-'bisect_state \$state' exited with error code \$res" >&2
+'git bisect--helper --bisect-state \$state' exited with error code \$res" >&2
exit $res
fi
@@ -242,7 +195,7 @@ case "$#" in
start)
git bisect--helper --bisect-start "$@" ;;
bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
- bisect_state "$cmd" "$@" ;;
+ git bisect--helper --bisect-state "$cmd" "$@" ;;
skip)
bisect_skip "$@" ;;
next)
--
2.25.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 10/12] bisect--helper: retire `--check-expected-revs` subcommand
2020-04-23 7:06 [PATCH v3 00/12] Finish converting git bisect to C part 2 Miriam Rubio
` (8 preceding siblings ...)
2020-04-23 7:07 ` [PATCH v3 09/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C Miriam Rubio
@ 2020-04-23 7:07 ` Miriam Rubio
2020-04-23 7:07 ` [PATCH v3 11/12] bisect--helper: retire `--write-terms` subcommand Miriam Rubio
` (3 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Miriam Rubio @ 2020-04-23 7:07 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Lars Schneider, Christian Couder,
Johannes Schindelin, Tanushree Tumane, Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
The `--check-expected-revs` subcommand is no longer used from the
git-bisect.sh shell script. Instead the function
`check_expected_revs()` is called from the C implementation of
`bisect-next()`.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 9db72f5891..7db58ada6f 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -898,7 +898,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
WRITE_TERMS = 1,
- CHECK_EXPECTED_REVS,
BISECT_RESET,
BISECT_WRITE,
CHECK_AND_SET_TERMS,
@@ -914,8 +913,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
struct option options[] = {
OPT_CMDMODE(0, "write-terms", &cmdmode,
N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
- OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
- N_("check for expected revs"), CHECK_EXPECTED_REVS),
OPT_CMDMODE(0, "bisect-reset", &cmdmode,
N_("reset the bisection state"), BISECT_RESET),
OPT_CMDMODE(0, "bisect-write", &cmdmode,
@@ -956,9 +953,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
if (argc != 2)
return error(_("--write-terms requires two arguments"));
return write_terms(argv[0], argv[1]);
- case CHECK_EXPECTED_REVS:
- check_expected_revs(argv, argc);
- return 0;
case BISECT_RESET:
if (argc > 1)
return error(_("--bisect-reset requires either no argument or a commit"));
--
2.25.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 11/12] bisect--helper: retire `--write-terms` subcommand
2020-04-23 7:06 [PATCH v3 00/12] Finish converting git bisect to C part 2 Miriam Rubio
` (9 preceding siblings ...)
2020-04-23 7:07 ` [PATCH v3 10/12] bisect--helper: retire `--check-expected-revs` subcommand Miriam Rubio
@ 2020-04-23 7:07 ` Miriam Rubio
2020-04-23 7:07 ` [PATCH v3 12/12] bisect--helper: retire `--bisect-autostart` subcommand Miriam Rubio
` (2 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Miriam Rubio @ 2020-04-23 7:07 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Lars Schneider, Christian Couder,
Johannes Schindelin, Tanushree Tumane, Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
The `--write-terms` subcommand is no longer used from the
git-bisect.sh shell script. Instead the function `write_terms()`
is called from the C implementation of `set_terms()` and
`bisect_start()`.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 7db58ada6f..e7467dc1c7 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_head_name, "head-name")
static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
static const char * const git_bisect_helper_usage[] = {
- N_("git bisect--helper --write-terms <bad_term> <good_term>"),
N_("git bisect--helper --bisect-reset [<commit>]"),
N_("git bisect--helper --bisect-write [--no-log] <state> <revision> <good_term> <bad_term>"),
N_("git bisect--helper --bisect-check-and-set-terms <command> <good_term> <bad_term>"),
@@ -897,8 +896,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
- WRITE_TERMS = 1,
- BISECT_RESET,
+ BISECT_RESET = 1,
BISECT_WRITE,
CHECK_AND_SET_TERMS,
BISECT_NEXT_CHECK,
@@ -911,8 +909,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
} cmdmode = 0;
int no_checkout = 0, res = 0, nolog = 0;
struct option options[] = {
- OPT_CMDMODE(0, "write-terms", &cmdmode,
- N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
OPT_CMDMODE(0, "bisect-reset", &cmdmode,
N_("reset the bisection state"), BISECT_RESET),
OPT_CMDMODE(0, "bisect-write", &cmdmode,
@@ -949,10 +945,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
usage_with_options(git_bisect_helper_usage, options);
switch (cmdmode) {
- case WRITE_TERMS:
- if (argc != 2)
- return error(_("--write-terms requires two arguments"));
- return write_terms(argv[0], argv[1]);
case BISECT_RESET:
if (argc > 1)
return error(_("--bisect-reset requires either no argument or a commit"));
--
2.25.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 12/12] bisect--helper: retire `--bisect-autostart` subcommand
2020-04-23 7:06 [PATCH v3 00/12] Finish converting git bisect to C part 2 Miriam Rubio
` (10 preceding siblings ...)
2020-04-23 7:07 ` [PATCH v3 11/12] bisect--helper: retire `--write-terms` subcommand Miriam Rubio
@ 2020-04-23 7:07 ` Miriam Rubio
2020-04-23 20:01 ` [PATCH v3 00/12] Finish converting git bisect to C part 2 Junio C Hamano
2020-05-22 22:09 ` Johannes Schindelin
13 siblings, 0 replies; 28+ messages in thread
From: Miriam Rubio @ 2020-04-23 7:07 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Lars Schneider, Christian Couder,
Johannes Schindelin, Tanushree Tumane, Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
The `--bisect-autostart` subcommand is no longer used from the
git-bisect.sh shell script. Instead the function
`bisect_autostart()` is directly called from the C implementation.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index e7467dc1c7..66f9b8eab6 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -29,7 +29,6 @@ static const char * const git_bisect_helper_usage[] = {
"[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
N_("git bisect--helper --bisect-next"),
N_("git bisect--helper --bisect-auto-next"),
- N_("git bisect--helper --bisect-autostart"),
N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
NULL
@@ -904,7 +903,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
BISECT_START,
BISECT_NEXT,
BISECT_AUTO_NEXT,
- BISECT_AUTOSTART,
BISECT_STATE
} cmdmode = 0;
int no_checkout = 0, res = 0, nolog = 0;
@@ -925,8 +923,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
N_("find the next bisection commit"), BISECT_NEXT),
OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
- OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
- N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
OPT_CMDMODE(0, "bisect-state", &cmdmode,
N_("mark the state of ref (or refs)"), BISECT_STATE),
OPT_BOOL(0, "no-checkout", &no_checkout,
@@ -988,12 +984,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
get_terms(&terms);
res = bisect_auto_next(&terms, prefix);
break;
- case BISECT_AUTOSTART:
- if (argc)
- return error(_("--bisect-autostart requires 0 arguments"));
- set_terms(&terms, "bad", "good");
- res = bisect_autostart(&terms);
- break;
case BISECT_STATE:
set_terms(&terms, "bad", "good");
get_terms(&terms);
--
2.25.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v3 00/12] Finish converting git bisect to C part 2
2020-04-23 7:06 [PATCH v3 00/12] Finish converting git bisect to C part 2 Miriam Rubio
` (11 preceding siblings ...)
2020-04-23 7:07 ` [PATCH v3 12/12] bisect--helper: retire `--bisect-autostart` subcommand Miriam Rubio
@ 2020-04-23 20:01 ` Junio C Hamano
2020-04-25 10:57 ` Miriam R.
2020-05-22 22:09 ` Johannes Schindelin
13 siblings, 1 reply; 28+ messages in thread
From: Junio C Hamano @ 2020-04-23 20:01 UTC (permalink / raw)
To: Miriam Rubio; +Cc: git
Miriam Rubio <mirucam@gmail.com> writes:
> --- Changes since v2 Finish converting git bisect to C part 2 patch series ---
>
> General changes
> ---------------
>
> * Rebase on master branch: efe3874640 (Sync with v2.26.1, 2020-04-13)
Was there a particular reason why you needed to do this rebase? It
seems that the patches apply cleanly on the same base as v2 has been
queued? Does the result cause hard-to-resolve conflicts when merged
to the 'master', 'next' or 'pu' branches unless you do that rebase?
> Specific changes
> ----------------
>
> [1/12] bisect--helper: fix `cmd_*()` function switch default return
>
> * Use `BUG()` instead of `return error()` in default switch.
This rings a bell. range-diff looks good.
> [2/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return
>
> * New patch: use '-res' instead of 'abs(res)'.
Alright.
> [3/12] bisect--helper: introduce new `write_in_file()` function
>
> * Rename input parameter `filepath` to `path`.
> * Change `error_errno()` to `error()` in mode checking.
> * Change error message when file cannot be opened.
> * Add `fclose()` before error return.
OK. I am not sure if the error behaviour when fclose() fails is
ideal, but I do not think it is worth further polishing.
> [4/12] bisect--helper: reimplement `bisect_autostart` shell function in C
>
> * Reorder patch before `reimplement `bisect_next` and `bisect_auto_next`
> shell functions in C` to use `bisect_autostart()` function in
> `bisect_append_log_quoted()`.
OK.
Will take a look at individual patches later.
Thanks.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 00/12] Finish converting git bisect to C part 2
2020-04-23 20:01 ` [PATCH v3 00/12] Finish converting git bisect to C part 2 Junio C Hamano
@ 2020-04-25 10:57 ` Miriam R.
0 siblings, 0 replies; 28+ messages in thread
From: Miriam R. @ 2020-04-25 10:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Hi Junio,
El jue., 23 abr. 2020 a las 22:01, Junio C Hamano
(<gitster@pobox.com>) escribió:
>
> Miriam Rubio <mirucam@gmail.com> writes:
>
> > --- Changes since v2 Finish converting git bisect to C part 2 patch series ---
> >
> > General changes
> > ---------------
> >
> > * Rebase on master branch: efe3874640 (Sync with v2.26.1, 2020-04-13)
>
> Was there a particular reason why you needed to do this rebase? It
> seems that the patches apply cleanly on the same base as v2 has been
> queued? Does the result cause hard-to-resolve conflicts when merged
> to the 'master', 'next' or 'pu' branches unless you do that rebase?
>
No particular reason. I use to do a rebase on master before applying
reviewers suggestions if I see there are new commits on master.
> > Specific changes
> > ----------------
> >
> > [1/12] bisect--helper: fix `cmd_*()` function switch default return
> >
> > * Use `BUG()` instead of `return error()` in default switch.
>
> This rings a bell. range-diff looks good.
>
> > [2/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return
> >
> > * New patch: use '-res' instead of 'abs(res)'.
>
> Alright.
>
> > [3/12] bisect--helper: introduce new `write_in_file()` function
> >
> > * Rename input parameter `filepath` to `path`.
> > * Change `error_errno()` to `error()` in mode checking.
> > * Change error message when file cannot be opened.
> > * Add `fclose()` before error return.
>
> OK. I am not sure if the error behaviour when fclose() fails is
> ideal, but I do not think it is worth further polishing.
>
> > [4/12] bisect--helper: reimplement `bisect_autostart` shell function in C
> >
> > * Reorder patch before `reimplement `bisect_next` and `bisect_auto_next`
> > shell functions in C` to use `bisect_autostart()` function in
> > `bisect_append_log_quoted()`.
>
> OK.
>
> Will take a look at individual patches later.
Great, thank you!
>
> Thanks.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 01/12] bisect--helper: fix `cmd_*()` function switch default return
2020-04-23 7:06 ` [PATCH v3 01/12] bisect--helper: fix `cmd_*()` function switch default return Miriam Rubio
@ 2020-05-22 13:14 ` Johannes Schindelin
0 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin @ 2020-05-22 13:14 UTC (permalink / raw)
To: Miriam Rubio; +Cc: git, Christian Couder
Hi Miriam,
On Thu, 23 Apr 2020, Miriam Rubio wrote:
> In a `cmd_*()` function, return `error()` cannot be used
> because that translates to `-1` and `cmd_*()` functions need
> to return exit codes.
>
> Let's fix switch default return.
I think there is an even stronger argument for not returning here: it
actually _would_ be a bug, not a user error, if the `default` branch was
reached.
The patch looks good.
Ciao,
Dscho
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
> builtin/bisect--helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index c1c40b516d..a81a2c76ff 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -711,7 +711,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> res = bisect_start(&terms, no_checkout, argv, argc);
> break;
> default:
> - return error("BUG: unknown subcommand '%d'", cmdmode);
> + BUG("unknown subcommand %d", (int)cmdmode);
> }
> free_terms(&terms);
>
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 02/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return
2020-04-23 7:06 ` [PATCH v3 02/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return Miriam Rubio
@ 2020-05-22 13:16 ` Johannes Schindelin
0 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin @ 2020-05-22 13:16 UTC (permalink / raw)
To: Miriam Rubio; +Cc: git, Christian Couder, Junio C Hamano
Hi Miriam,
On Thu, 23 Apr 2020, Miriam Rubio wrote:
> Following 'enum bisect_error' vocabulary, return variable 'res' is
> always non-positive.
> Let's use '-res' instead of 'abs(res)' to make the code clearer.
I agree with this reasoning, and with the patch.
Thank you,
Dscho
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> ---
> builtin/bisect--helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index a81a2c76ff..0fbd924aac 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -722,5 +722,5 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)
> res = BISECT_OK;
>
> - return abs(res);
> + return -res;
> }
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 03/12] bisect--helper: introduce new `write_in_file()` function
2020-04-23 7:06 ` [PATCH v3 03/12] bisect--helper: introduce new `write_in_file()` function Miriam Rubio
@ 2020-05-22 13:25 ` Johannes Schindelin
2020-05-23 1:53 ` Đoàn Trần Công Danh
1 sibling, 0 replies; 28+ messages in thread
From: Johannes Schindelin @ 2020-05-22 13:25 UTC (permalink / raw)
To: Miriam Rubio; +Cc: git, Christian Couder
Hi Miriam,
On Thu, 23 Apr 2020, Miriam Rubio wrote:
> Let's refactor code adding a new `write_in_file()` function
> that opens a file for writing a message and closes it.
Don't we have that already?
https://github.com/git/git/blob/v2.27.0-rc1/wrapper.c#L625-L638
If that function does not meet the bill, it would make more sense to
extend it (or refactor it, providing a new global function that does meet
the bill).
> This helper will be used in later steps and makes the code
> simpler and easier to understand.
Makes a lot of sense to me!
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
> builtin/bisect--helper.c | 29 +++++++++++++++++++++++------
> 1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 0fbd924aac..d3b2b33df0 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -74,6 +74,28 @@ static int one_of(const char *term, ...)
> return res;
> }
>
> +static int write_in_file(const char *path, const char *mode, const char *format,...)
I wonder whether there is _ever_ a case where we want to set `mode` to
anything else than `"w"`? Ah...
> +{
> + FILE *fp = NULL;
> + va_list args;
> + int res = 0;
> +
> + if (!strcmp(mode, "a") && !strcmp(mode, "w"))
> + return error(_("wrong writing mode '%s'"), mode);
... rather than doing this, I would prefer a pair of functions (that can
call the same helper function doing the actual work): `write_to_file()`
and `append_to_file()`.
Apart from that, the patch looks good to me,
Dscho
> + fp = fopen(path, mode);
> + if (!fp)
> + return error_errno(_("cannot open file '%s' in mode '%s'"), path, mode);
> + va_start(args, format);
> + res = vfprintf(fp, format, args);
> + va_end(args);
> + if (!res) {
> + fclose(fp);
> + return error_errno(_("could not write to file '%s'"), path);
> + }
> +
> + return fclose(fp);
> +}
> +
> static int check_term_format(const char *term, const char *orig_term)
> {
> int res;
> @@ -104,7 +126,6 @@ static int check_term_format(const char *term, const char *orig_term)
>
> static int write_terms(const char *bad, const char *good)
> {
> - FILE *fp = NULL;
> int res;
>
> if (!strcmp(bad, good))
> @@ -113,12 +134,8 @@ static int write_terms(const char *bad, const char *good)
> if (check_term_format(bad, "bad") || check_term_format(good, "good"))
> return -1;
>
> - fp = fopen(git_path_bisect_terms(), "w");
> - if (!fp)
> - return error_errno(_("could not open the file BISECT_TERMS"));
> + res = write_in_file(git_path_bisect_terms(), "w", "%s\n%s\n", bad, good);
>
> - res = fprintf(fp, "%s\n%s\n", bad, good);
> - res |= fclose(fp);
> return (res < 0) ? -1 : 0;
> }
>
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 04/12] bisect--helper: reimplement `bisect_autostart` shell function in C
2020-04-23 7:06 ` [PATCH v3 04/12] bisect--helper: reimplement `bisect_autostart` shell function in C Miriam Rubio
@ 2020-05-22 19:27 ` Johannes Schindelin
2020-05-22 20:50 ` Johannes Schindelin
0 siblings, 1 reply; 28+ messages in thread
From: Johannes Schindelin @ 2020-05-22 19:27 UTC (permalink / raw)
To: Miriam Rubio
Cc: git, Pranit Bauva, Lars Schneider, Christian Couder, Tanushree Tumane
Hi Miriam,
On Thu, 23 Apr 2020, Miriam Rubio wrote:
> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Reimplement the `bisect_autostart()` shell function in C and add the
> C implementation from `bisect_next()` which was previously left
> uncovered. Also add a subcommand `--bisect-autostart` to
> `git bisect--helper` be called from `bisect_state()` from
> git-bisect.sh .
I know you did not author this, but maybe I can still ask you to address a
couple of concerns I have? I kind of stumble when I read "Also add a
subcommand ... be called from ... from ...".
Also, why the funny space before the full stop?
> Using `--bisect-autostart` subcommand is a temporary measure to port
> shell function to C so as to use the existing test suite. As more
Likewise, "... to port shell function..." might benefit from inserting
"the" in the middle?
> functions are ported, this subcommand will be retired and
> bisect_autostart() will be called directly by `bisect_state()`.
>
> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
> builtin/bisect--helper.c | 40 +++++++++++++++++++++++++++++++++++++++-
> git-bisect.sh | 25 ++-----------------------
> 2 files changed, 41 insertions(+), 24 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index d3b2b33df0..9df69800e3 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -29,6 +29,7 @@ static const char * const git_bisect_helper_usage[] = {
> N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
> N_("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
> "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
> + N_("git bisect--helper --bisect-autostart"),
> NULL
> };
>
> @@ -55,6 +56,8 @@ static void set_terms(struct bisect_terms *terms, const char *bad,
> static const char vocab_bad[] = "bad|new";
> static const char vocab_good[] = "good|old";
>
> +static int bisect_autostart(struct bisect_terms *terms);
> +
Why forward-declare it? There is no reference before it is defined below,
so I think this forward-declaration can be removed without any problem.
> /*
> * Check whether the string `term` belongs to the set of strings
> * included in the variable arguments.
> @@ -630,6 +633,32 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
> return res;
> }
>
> +static int bisect_autostart(struct bisect_terms *terms)
> +{
> + const char *yesno;
> +
> + if (!is_empty_or_missing_file(git_path_bisect_start()))
> + return 0;
This is a bit more convoluted to read than the shell script:
bisect_autostart() {
test -s "$GIT_DIR/BISECT_START" || {
gettextln "You need to start by \"git bisect start\"" >&2
[...]
}
}
The `test -s <file>` command succeeds if the file exists and is non-empty.
Which makes sense: either the file exists and is non-empty, or we need to
do something.
When I read the C version, it takes me a bit more time to figure out what
is going on here. Maybe we could introduce a helper function like this and
use it here?
static inline int file_is_not_empty(const char *path)
{
return !is_empty_or_missing_file(path);
}
> +
> + fprintf(stderr, _("You need to start by \"git bisect "
> + "start\"\n"));
If you use `fprintf_ln()` here, you can keep the string identical to the
shell script version, i.e. without the trailing newline. That will allow
the translations to be re-used, lessening the burden of the i18n team.
> +
> + if (!isatty(STDIN_FILENO))
> + return 1;
I would prefer it if only `cmd_*()` functions return positive values for
the error case.
> +
> + /*
> + * TRANSLATORS: Make sure to include [Y] and [n] in your
> + * translation. The program will only accept English input
> + * at this point.
> + */
> + yesno = git_prompt(_("Do you want me to do it for you "
> + "[Y/n]? "), PROMPT_ECHO);
Technically, `PROMPT_ECHO` is not required here as `git_prompt()` adds
that flag implicitly unless you pass in `PASS_ASKPASS`, but I guess it
does not hurt here.
However, I cannot fail to note that this constitutes a change of behavior,
as `git_prompt()` does _not_ read from `stdin`.
Maybe we should call `git_read_line_interactively()` instead, after
writing the prompt to `stderr` explicitly?
> + if (starts_with(yesno, _("n")) || starts_with(yesno, _("N")))
This is wrong: "n" and "N" should not be translated, as they are not
translated in the shell script version, either:
case "$yesno" in
[Nn]*)
exit ;;
esac
In light of this, a much, much shorter version would be:
if (tolower(*yesno) == 'n')
return -1;
(I use `-1` here because this is not within a `cmd_*()` function.)
However, I still think that we need to call `free(yesno);` or we leak
memory. My preferred way would be:
int ret;
[...]
ret = tolower(*yesno) == 'n' ?
-1 : bisect_start(terms, 0, NULL, 0);
free(yesno);
return ret;
> + return 1;
> +
> + return bisect_start(terms, 0, NULL, 0);
> +}
> +
> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> {
> enum {
> @@ -642,7 +671,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> CHECK_AND_SET_TERMS,
> BISECT_NEXT_CHECK,
> BISECT_TERMS,
> - BISECT_START
> + BISECT_START,
> + BISECT_AUTOSTART,
> } cmdmode = 0;
> int no_checkout = 0, res = 0, nolog = 0;
> struct option options[] = {
> @@ -666,6 +696,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> N_("print out the bisect terms"), BISECT_TERMS),
> OPT_CMDMODE(0, "bisect-start", &cmdmode,
> N_("start the bisect session"), BISECT_START),
> + OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
> + N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
Not that it matters a lot because this command mode will go away soon
thereafter, but this describes an implementation detail rather than the
purpose of the function. I would much rather read "start the bisection if
it has not yet been started" instead.
> OPT_BOOL(0, "no-checkout", &no_checkout,
> N_("update BISECT_HEAD instead of checking out the current commit")),
> OPT_BOOL(0, "no-log", &nolog,
> @@ -727,6 +759,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> set_terms(&terms, "bad", "good");
> res = bisect_start(&terms, no_checkout, argv, argc);
> break;
> + case BISECT_AUTOSTART:
> + if (argc)
> + return error(_("--bisect-autostart requires 0 arguments"));
Maybe "--bisect-autostart does not accept arguments" instead?
Ciao,
Dscho
> + set_terms(&terms, "bad", "good");
> + res = bisect_autostart(&terms);
> + break;
> default:
> BUG("unknown subcommand %d", (int)cmdmode);
> }
> diff --git a/git-bisect.sh b/git-bisect.sh
> index efee12b8b1..426d443e7e 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -49,27 +49,6 @@ bisect_head()
> fi
> }
>
> -bisect_autostart() {
> - test -s "$GIT_DIR/BISECT_START" || {
> - gettextln "You need to start by \"git bisect start\"" >&2
> - if test -t 0
> - then
> - # TRANSLATORS: Make sure to include [Y] and [n] in your
> - # translation. The program will only accept English input
> - # at this point.
> - gettext "Do you want me to do it for you [Y/n]? " >&2
> - read yesno
> - case "$yesno" in
> - [Nn]*)
> - exit ;;
> - esac
> - bisect_start
> - else
> - exit 1
> - fi
> - }
> -}
> -
> bisect_start() {
> git bisect--helper --bisect-start $@ || exit
>
> @@ -108,7 +87,7 @@ bisect_skip() {
> }
>
> bisect_state() {
> - bisect_autostart
> + git bisect--helper --bisect-autostart
> state=$1
> git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
> get_terms
> @@ -149,7 +128,7 @@ bisect_auto_next() {
>
> bisect_next() {
> case "$#" in 0) ;; *) usage ;; esac
> - bisect_autostart
> + git bisect--helper --bisect-autostart
> git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit
>
> # Perform all bisection computation, display and checkout
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 05/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C
2020-04-23 7:06 ` [PATCH v3 05/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions " Miriam Rubio
@ 2020-05-22 20:47 ` Johannes Schindelin
0 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin @ 2020-05-22 20:47 UTC (permalink / raw)
To: Miriam Rubio
Cc: git, Pranit Bauva, Lars Schneider, Christian Couder, Tanushree Tumane
Hi Miriam,
On Thu, 23 Apr 2020, Miriam Rubio wrote:
> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Reimplement the `bisect_next()` and the `bisect_auto_next()` shell functions
> in C and add the subcommands to `git bisect--helper` to call them from
> git-bisect.sh .
>
> bisect_auto_next() function returns an enum bisect_error type as whole
> `git bisect` can exit with an error code when bisect_next() does.
>
> Using `--bisect-next` and `--bisect-auto-next` subcommands is a
> temporary measure to port shell function to C so as to use the existing
> test suite. As more functions are ported, `--bisect-auto-next`
> subcommand will be retired and will be called by some other methods.
>
> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
> bisect.c | 8 ++
> builtin/bisect--helper.c | 169 ++++++++++++++++++++++++++++++++++++++-
> git-bisect.sh | 47 +----------
> 3 files changed, 179 insertions(+), 45 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index 9154f810f7..31cab3f0b0 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -980,6 +980,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
> * the bisection process finished successfully.
> * In this case the calling function or command should not turn a
> * BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND return code into an error or a non zero exit code.
> + *
> + * Checking BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND
> + * in bisect_helper::bisect_next() and only transforming it to 0 at
> + * the end of bisect_helper::cmd_bisect__helper() helps bypassing
> + * all the code related to finding a commit to test.
Nice!
> + *
> * If no_checkout is non-zero, the bisection process does not
> * checkout the trial commit but instead simply updates BISECT_HEAD.
> */
> @@ -1064,6 +1070,8 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
> "Bisecting: %d revisions left to test after this %s\n",
> nr), nr, steps_msg);
> free(steps_msg);
> + /* Clean up objects used, as they will be reused. */
> + clear_commit_marks_all(ALL_REV_FLAGS);
That strikes me as a change that is orthogonal to the purpose claimed by
the commit message. As such, I think it wants to live in its own commit.
>
> return bisect_checkout(bisect_rev, no_checkout);
> }
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 9df69800e3..c6aaa8eb15 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -8,6 +8,7 @@
> #include "run-command.h"
> #include "prompt.h"
> #include "quote.h"
> +#include "revision.h"
>
> static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
> static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
> @@ -29,6 +30,8 @@ static const char * const git_bisect_helper_usage[] = {
> N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
> N_("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
> "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
> + N_("git bisect--helper --bisect-next"),
> + N_("git bisect--helper --bisect-auto-next"),
> N_("git bisect--helper --bisect-autostart"),
> NULL
> };
> @@ -441,6 +444,150 @@ static int bisect_append_log_quoted(const char **argv)
> return res;
> }
>
> +static int register_good_ref(const char *refname,
> + const struct object_id *oid, int flags,
> + void *cb_data)
> +{
> + struct argv_array *rev_argv = cb_data;
> +
> + argv_array_push(rev_argv, oid_to_hex(oid));
> + return 0;
> +}
> +
> +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)
> +{
> + char *term_good = xstrfmt("%s-*", terms->term_good);
This could be `xstrfmt("refs/bisect/%s-*", terms->term_good)` instead, and
that would have avoided some head-scratching on my side. The
"refs/bisect/" prefix passed to `for_each_glob_ref_in()` makes this
correct, of course.
> +
> + argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);
This hard-codes `bad` where the shell script version uses
`refs/bisect/$TERM_BAD`.
> + for_each_glob_ref_in(register_good_ref, term_good, "refs/bisect/", rev_argv);
> +
> + free(term_good);
> +}
> +
> +static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)
> +{
> + int res = 0;
> + struct argv_array rev_argv = ARGV_ARRAY_INIT;
> +
> + prepare_rev_argv(terms, &rev_argv);
> +
> + /*
> + * It is important to reset the flags used by revision walks
> + * as the previous call to bisect_next_all() in turn
> + * sets up a revision walk.
> + */
> + reset_revision_walk();
> + init_revisions(revs, NULL);
> + rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL);
I have not heard any valid argument against my suggestion in
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet/
to render into the pending objects directly. Yes, there was a hand-waving
"but the shell script version does not do that" but that's just not a
valid argument because if the shell script version could, it would, but it
cannot: it's shell script, for crying out loud. It has no way to use the
appropriate data structures. C code does. And should. We're not in the
business of converting OIDs into strings that are appended to string lists
so that they can be re-parsed into OIDs.
Therefore, I would suggest (and now that almost four months have passed
since my initial suggestion, rather strongly) to do something like this:
struct add_bisect_ref_data {
struct rev_info *revs;
int object_flags;
};
static int add_bisect_ref(const char *refname, const struct object_id *oid,
int flags, void *cb)
{
struct add_bisect_ref_data *data = cb;
add_pending_oid(data->revs, refname, oid, data->object_flags);
return 0;
}
[...]
struct add_bisect_ref_data cb = { &revs };
char *good = xstrfmt("%s-*", terms->term_good);
[...]
setup_revisions(0, NULL, revs, NULL);
for_each_glob_ref_in(add_bisect_ref, terms->term_bad, "refs/bisect/", &cb);
cb.object_flags = UNINTERESTING;
for_each_glob_ref_in(add_bisect_ref, good, "refs/bisect/", &cb);
This would remove one (unnecessary) level of indirection, and converting
OIDs from/to/from strings.
> + if (prepare_revision_walk(revs))
> + res = error(_("revision walk setup failed\n"));
> +
> + argv_array_clear(&rev_argv);
> + return res;
> +}
> +
> +static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)
> +{
> + struct commit *commit;
> + struct pretty_print_context pp = {0};
> +
> + if (fprintf(fp, "# only skipped commits left to test\n") < 0)
> + return error_errno(_("failed to write to '%s'"), git_path_bisect_log());
> +
> + while ((commit = get_revision(revs)) != NULL) {
> + struct strbuf commit_name = STRBUF_INIT;
I missed this in my review end of January: it is an anti-pattern to create
a new `strbuf` and releasing it right away in the same loop iteration.
Let's instead move the declaration of `commit_name` to the top of the
function, then call `strbuf_reset()` at the beginning of the loop, and
finally call `strbuf_release()` only _once_, at the end of the function.
> + format_commit_message(commit, "%s",
> + &commit_name, &pp);
> + fprintf(fp, "# possible first %s commit: [%s] %s\n",
> + terms->term_bad, oid_to_hex(&commit->object.oid),
> + commit_name.buf);
> + strbuf_release(&commit_name);
> + }
> +
> + /*
> + * Reset the flags used by revision walks in case
> + * there is another revision walk after this one.
> + */
> + reset_revision_walk();
> +
> + return 0;
> +}
> +
> +static int bisect_skipped_commits(struct bisect_terms *terms)
> +{
> + int res = 0;
> + FILE *fp = NULL;
> + struct rev_info revs;
> +
> + fp = fopen(git_path_bisect_log(), "a");
> + if (!fp)
> + return error_errno(_("could not open '%s' for appending"),
> + git_path_bisect_log());
> +
> + res = prepare_revs(terms, &revs);
> +
> + if (!res)
> + res = process_skipped_commits(fp, terms, &revs);
> +
> + fclose(fp);
> + return res;
It would make the code easier to follow if `prepare_revs()` was called
before opening the file and returning early in case of an error.
Further, I do not see the point of having the code in
`process_skipped_commits()` there is only this caller, in a tiny function.
It would be better to fold the code into a single function.
I did point that out already in my review at the end of this past January.
> +}
> +
> +static int bisect_successful(struct bisect_terms *terms)
> +{
> + struct object_id oid;
> + struct commit *commit;
> + struct pretty_print_context pp = {0};
> + struct strbuf commit_name = STRBUF_INIT;
> + char *bad_ref = xstrfmt("refs/bisect/%s",terms->term_bad);
> + int res;
> +
> + read_ref(bad_ref, &oid);
> + printf("%s\n", bad_ref);
I missed this in January: is this a left-over debug printf? I cannot find
the equivalent in the shell script version.
> + commit = lookup_commit_reference(the_repository, &oid);
There is a relatively recent convention to initialize a local variable
`struct repository *r = the_repository;` at the beginning of the function
and then using `r` throughout the function. I think I'd like to use that
here, too, even if it is only used once.
Any reason not to use `lookup_commit_reference_by_name(r, bad_ref)`
directly instead? You can still reference `commit->object.oid` instead of
`oid` later on.
> + format_commit_message(commit, "%s", &commit_name, &pp);
> +
> + res = write_in_file(git_path_bisect_log(), "a", "# first %s commit: [%s] %s\n",
> + terms->term_bad, oid_to_hex(&oid),
> + commit_name.buf);
> +
> + strbuf_release(&commit_name);
> + free(bad_ref);
> + return res;
> +}
> +
> +static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
> +{
> + int no_checkout;
> + enum bisect_error res;
> +
> + bisect_autostart(terms);
> + if (bisect_next_check(terms, terms->term_good))
> + return BISECT_FAILED;
> +
> + no_checkout = file_exists(git_path_bisect_head());
> +
> + /* Perform all bisection computation, display and checkout */
> + res = bisect_next_all(the_repository, prefix, no_checkout);
> +
> + if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
> + res = bisect_successful(terms);
> + return res ? res : BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND;
> + } else if (res == BISECT_ONLY_SKIPPED_LEFT) {
> + res = bisect_skipped_commits(terms);
> + return res ? res : BISECT_ONLY_SKIPPED_LEFT;
> + }
This looks much, much nicer than the equivalent code I reviewed almost
four months ago.
Personally, I would have used the idiom
if (!res)
return BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND;
but I can live with the current version.
> + return res;
> +}
> +
> +static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)
> +{
> + if (bisect_next_check(terms, NULL))
> + return BISECT_OK;
This is a straight-forward translation from the shell code.
I would like to take a step back, though: the only two differences between the
`bisect_next_check()` call here and the one in `bisect_next()` are:
- the return value if `bisect_next_check()` failed: the "auto" version
seems to report success, the non-"auto" version failure
- the "auto" version seems not to accept missing "good" commits
Seeing this, it does strike me odd that `bisect_next()` is called from
`bisect_auto_next()`, and I would have rather expected an `int auto`
parameter to the `bisect_next()` function, where the `bisect_autostart()`
call is skipped in case `auto != 0`.
The rest of the patch looks fine to me.
Ciao,
Dscho
> +
> + return bisect_next(terms, prefix);
> +}
> +
> static int bisect_start(struct bisect_terms *terms, int no_checkout,
> const char **argv, int argc)
> {
> @@ -672,7 +819,9 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> BISECT_NEXT_CHECK,
> BISECT_TERMS,
> BISECT_START,
> - BISECT_AUTOSTART,
> + BISECT_NEXT,
> + BISECT_AUTO_NEXT,
> + BISECT_AUTOSTART
> } cmdmode = 0;
> int no_checkout = 0, res = 0, nolog = 0;
> struct option options[] = {
> @@ -696,6 +845,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> N_("print out the bisect terms"), BISECT_TERMS),
> OPT_CMDMODE(0, "bisect-start", &cmdmode,
> N_("start the bisect session"), BISECT_START),
> + OPT_CMDMODE(0, "bisect-next", &cmdmode,
> + N_("find the next bisection commit"), BISECT_NEXT),
> + OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
> + N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
> OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
> N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
> OPT_BOOL(0, "no-checkout", &no_checkout,
> @@ -759,6 +912,18 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> set_terms(&terms, "bad", "good");
> res = bisect_start(&terms, no_checkout, argv, argc);
> break;
> + case BISECT_NEXT:
> + if (argc)
> + return error(_("--bisect-next requires 0 arguments"));
> + get_terms(&terms);
> + res = bisect_next(&terms, prefix);
> + break;
> + case BISECT_AUTO_NEXT:
> + if (argc)
> + return error(_("--bisect-auto-next requires 0 arguments"));
> + get_terms(&terms);
> + res = bisect_auto_next(&terms, prefix);
> + break;
> case BISECT_AUTOSTART:
> if (argc)
> return error(_("--bisect-autostart requires 0 arguments"));
> @@ -774,7 +939,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> * Handle early success
> * From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all
> */
> - if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)
> + if ((res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) || (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND))
> res = BISECT_OK;
>
> return -res;
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 426d443e7e..897825b675 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -65,8 +65,7 @@ bisect_start() {
> #
> # Check if we can proceed to the next bisect state.
> #
> - get_terms
> - bisect_auto_next
> + git bisect--helper --bisect-auto-next || exit
>
> trap '-' 0
> }
> @@ -119,45 +118,7 @@ bisect_state() {
> *)
> usage ;;
> esac
> - bisect_auto_next
> -}
> -
> -bisect_auto_next() {
> - git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :
> -}
> -
> -bisect_next() {
> - case "$#" in 0) ;; *) usage ;; esac
> - git bisect--helper --bisect-autostart
> - git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit
> -
> - # Perform all bisection computation, display and checkout
> - git bisect--helper --next-all $(test -f "$GIT_DIR/BISECT_HEAD" && echo --no-checkout)
> - res=$?
> -
> - # Check if we should exit because bisection is finished
> - if test $res -eq 10
> - then
> - bad_rev=$(git show-ref --hash --verify refs/bisect/$TERM_BAD)
> - bad_commit=$(git show-branch $bad_rev)
> - echo "# first $TERM_BAD commit: $bad_commit" >>"$GIT_DIR/BISECT_LOG"
> - exit 0
> - elif test $res -eq 2
> - then
> - echo "# only skipped commits left to test" >>"$GIT_DIR/BISECT_LOG"
> - good_revs=$(git for-each-ref --format="%(objectname)" "refs/bisect/$TERM_GOOD-*")
> - for skipped in $(git rev-list refs/bisect/$TERM_BAD --not $good_revs)
> - do
> - skipped_commit=$(git show-branch $skipped)
> - echo "# possible first $TERM_BAD commit: $skipped_commit" >>"$GIT_DIR/BISECT_LOG"
> - done
> - exit $res
> - fi
> -
> - # Check for an error in the bisection process
> - test $res -ne 0 && exit $res
> -
> - return 0
> + git bisect--helper --bisect-auto-next
> }
>
> bisect_visualize() {
> @@ -211,7 +172,7 @@ bisect_replay () {
> die "$(gettext "?? what are you talking about?")" ;;
> esac
> done <"$file"
> - bisect_auto_next
> + git bisect--helper --bisect-auto-next
> }
>
> bisect_run () {
> @@ -308,7 +269,7 @@ case "$#" in
> bisect_skip "$@" ;;
> next)
> # Not sure we want "next" at the UI level anymore.
> - bisect_next "$@" ;;
> + git bisect--helper --bisect-next "$@" || exit ;;
> visualize|view)
> bisect_visualize "$@" ;;
> reset)
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 04/12] bisect--helper: reimplement `bisect_autostart` shell function in C
2020-05-22 19:27 ` Johannes Schindelin
@ 2020-05-22 20:50 ` Johannes Schindelin
0 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin @ 2020-05-22 20:50 UTC (permalink / raw)
To: Miriam Rubio
Cc: git, Pranit Bauva, Lars Schneider, Christian Couder, Tanushree Tumane
Hi Miriam,
On Fri, 22 May 2020, Johannes Schindelin wrote:
> On Thu, 23 Apr 2020, Miriam Rubio wrote:
>
> > + /*
> > + * TRANSLATORS: Make sure to include [Y] and [n] in your
> > + * translation. The program will only accept English input
> > + * at this point.
> > + */
> > + yesno = git_prompt(_("Do you want me to do it for you "
> > + "[Y/n]? "), PROMPT_ECHO);
>
> [...]
>
> I still think that we need to call `free(yesno);` or we leak memory.
Never mind. I got fooled by the fact that `git_prompt()` returns `char *`.
It actually returns the pointer `buf.buf` of a `static `strbuf buf`, so
there is nothing to `free()` here.
Sorry for the noise,
Dscho
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 06/12] bisect--helper: finish porting `bisect_start()` to C
2020-04-23 7:06 ` [PATCH v3 06/12] bisect--helper: finish porting `bisect_start()` to C Miriam Rubio
@ 2020-05-22 21:08 ` Johannes Schindelin
0 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin @ 2020-05-22 21:08 UTC (permalink / raw)
To: Miriam Rubio; +Cc: git, Pranit Bauva, Christian Couder, Tanushree Tumane
Hi Miriam,
On Thu, 23 Apr 2020, Miriam Rubio wrote:
> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Add the subcommand to `git bisect--helper` and call it from
> git-bisect.sh.
>
> With the conversion of `bisect_auto_next()` from shell to C in a
> previous commit, `bisect_start()` can now be fully ported to C.
>
> So let's complete the `--bisect-start` subcommand of
> `git bisect--helper` so that it fully implements `bisect_start()`,
> and let's use this subcommand in `git-bisect.sh` instead of
> `bisect_start()`.
>
> This removes the signal handling we had in `bisect_start()` as we
> don't really need it. While at it, "trap" is changed to "handle".
I do not see any "handle" in the diff...
> As "trap" is a reference to the shell "trap" builtin, which isn't
> used anymore.
Otherwise, the commit message looks good to me!
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
> builtin/bisect--helper.c | 54 +++++++++++++++++++++++++++++++---------
> git-bisect.sh | 26 ++-----------------
> 2 files changed, 44 insertions(+), 36 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index c6aaa8eb15..f2ec7f89e4 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -588,11 +588,12 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char
> return bisect_next(terms, prefix);
> }
>
> -static int bisect_start(struct bisect_terms *terms, int no_checkout,
> +static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkout,
> const char **argv, int argc)
> {
> int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
> - int flags, pathspec_pos, res = 0;
> + int flags, pathspec_pos;
> + enum bisect_error res = BISECT_OK;
> struct string_list revs = STRING_LIST_INIT_DUP;
> struct string_list states = STRING_LIST_INIT_DUP;
> struct strbuf start_head = STRBUF_INIT;
> @@ -645,9 +646,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
> return error(_("unrecognized option: '%s'"), arg);
> } else {
> char *commit_id = xstrfmt("%s^{commit}", arg);
> - if (get_oid(commit_id, &oid) && has_double_dash)
> - die(_("'%s' does not appear to be a valid "
> - "revision"), arg);
> + if (get_oid(commit_id, &oid) && has_double_dash) {
> + error(_("'%s' does not appear to be a valid "
> + "revision"), arg);
> + free(commit_id);
> + return BISECT_FAILED;
> + }
Makes sense.
>
> string_list_append(&revs, oid_to_hex(&oid));
> free(commit_id);
> @@ -725,12 +729,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
> * Get rid of any old bisect state.
> */
> if (bisect_clean_state())
> - return -1;
> + return BISECT_FAILED;
>
> /*
> - * In case of mistaken revs or checkout error, or signals received,
> + * In case of mistaken revs or checkout error,
> * "bisect_auto_next" below may exit or misbehave.
> - * We have to trap this to be able to clean up using
> + * We have to handle this to be able to clean up using
> * "bisect_clean_state".
> */
>
> @@ -746,7 +750,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
> }
> if (update_ref(NULL, "BISECT_HEAD", &oid, NULL, 0,
> UPDATE_REFS_MSG_ON_ERR)) {
> - res = -1;
> + res = BISECT_FAILED;
> goto finish;
> }
> }
> @@ -758,25 +762,51 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
> for (i = 0; i < states.nr; i++)
> if (bisect_write(states.items[i].string,
> revs.items[i].string, terms, 1)) {
> - res = -1;
> + res = BISECT_FAILED;
> goto finish;
> }
>
> if (must_write_terms && write_terms(terms->term_bad,
> terms->term_good)) {
> - res = -1;
> + res = BISECT_FAILED;
> goto finish;
> }
>
> res = bisect_append_log_quoted(argv);
> if (res)
> - res = -1;
> + res = BISECT_FAILED;
>
> finish:
> string_list_clear(&revs, 0);
> string_list_clear(&states, 0);
> strbuf_release(&start_head);
> strbuf_release(&bisect_names);
> + if (res)
> + return res;
> +
> + res = bisect_auto_next(terms, NULL);
> + /*
> + * In case of mistaken revs or checkout error, or signals received,
> + * "bisect_auto_next" below may exit or misbehave.
This "below" was true in the shell script version, but now it should be
"above".
> + * We have to handle this to be able to clean up using
> + * "bisect_clean_state".
> + * return code BISECT_INTERNAL_SUCCESS_MERGE_BASE is special code
> + * that indicates special success.
Maybe we should have a helper function and use it here:
static int is_bisect_success(enum bisect_error res)
{
return !res ||
res == BISECT_INTERNAL_1ST_BAD_FOUND ||
res == BISECT_INTERNAL_SUCCESS_MERGE_BASE;
}
(Note that I added the `_1ST_BAD_FOUND` case here.)
> + * -> bisect_start()
> + * . res = bisect_auto_next()
> + * -> bisect_auto_next()
> + * . return bisect_next()
> + * -> bisect_next()
> + * . res = bisect_next_all()
> + * -> bisect_next_all()
> + * . res = check_good_are_ancestors_of_bad()
> + * -> check_good_are_ancestors_of_bad()
> + * . res = check_merge_bases()
> + * -> check_merge_bases()
> + * . res = BISECT_INTERNAL_SUCCESS_MERGE_BASE
While this might be an interesting tidbit for the commit message, I do not
think that it is a good idea to keep this as a code comment; It is prone
to become stale/incorrect over time.
> + */
> + if (res && res != BISECT_INTERNAL_SUCCESS_MERGE_BASE)
> + bisect_clean_state();
> return res;
> }
>
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 897825b675..049ffacdff 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -49,27 +49,6 @@ bisect_head()
> fi
> }
>
> -bisect_start() {
> - git bisect--helper --bisect-start $@ || exit
> -
> - #
> - # Change state.
> - # In case of mistaken revs or checkout error, or signals received,
> - # "bisect_auto_next" below may exit or misbehave.
> - # We have to trap this to be able to clean up using
> - # "bisect_clean_state".
> - #
> - trap 'git bisect--helper --bisect-clean-state' 0
> - trap 'exit 255' 1 2 3 15
> -
> - #
> - # Check if we can proceed to the next bisect state.
> - #
> - git bisect--helper --bisect-auto-next || exit
> -
> - trap '-' 0
> -}
> -
> bisect_skip() {
> all=''
> for arg in "$@"
> @@ -162,8 +141,7 @@ bisect_replay () {
> get_terms
> case "$command" in
> start)
> - cmd="bisect_start $rev"
> - eval "$cmd" ;;
> + eval "git bisect--helper --bisect-start $rev" ;;
The original code _had_ to use `eval` because `bisect_start` could cause
the code to `exit` and we did _not_ want to let that exit the `replay`
process.
However, we are calling `git bisect--helper` here, which runs in its own
process anyway, so the `eval` should be dropped now.
Otherwise, the patch looks fine to me,
Dscho
> "$TERM_GOOD"|"$TERM_BAD"|skip)
> git bisect--helper --bisect-write "$command" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit;;
> terms)
> @@ -262,7 +240,7 @@ case "$#" in
> help)
> git bisect -h ;;
> start)
> - bisect_start "$@" ;;
> + git bisect--helper --bisect-start "$@" ;;
> bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
> bisect_state "$cmd" "$@" ;;
> skip)
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 09/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C
2020-04-23 7:07 ` [PATCH v3 09/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C Miriam Rubio
@ 2020-05-22 22:06 ` Johannes Schindelin
2020-06-20 8:04 ` Miriam R.
0 siblings, 1 reply; 28+ messages in thread
From: Johannes Schindelin @ 2020-05-22 22:06 UTC (permalink / raw)
To: Miriam Rubio
Cc: git, Pranit Bauva, Lars Schneider, Christian Couder, Tanushree Tumane
Hi Miriam,
On Thu, 23 Apr 2020, Miriam Rubio wrote:
> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Reimplement the `bisect_state()` shell functions in C and also add a
> subcommand `--bisect-state` to `git-bisect--helper` to call them from
> git-bisect.sh .
>
> Using `--bisect-state` subcommand is a temporary measure to port shell
> function to C so as to use the existing test suite. As more functions
> are ported, this subcommand will be retired and will be called by some
> other methods.
>
> `bisect_head()` is only called from `bisect_state()`, thus it is not
> required to introduce another subcommand.
>
> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
> builtin/bisect--helper.c | 70 +++++++++++++++++++++++++++++++++++++++-
> git-bisect.sh | 55 +++----------------------------
> 2 files changed, 73 insertions(+), 52 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 2d8660c79f..9db72f5891 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {
> N_("git bisect--helper --bisect-next"),
> N_("git bisect--helper --bisect-auto-next"),
> N_("git bisect--helper --bisect-autostart"),
> + N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
> + N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
> NULL
> };
>
> @@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms)
> return bisect_start(terms, 0, NULL, 0);
> }
>
> +static int bisect_head(struct object_id *oid)
> +{
> + if (!file_exists(git_path_bisect_head()))
> + return get_oid("HEAD", oid);
> +
> + return get_oid("BISECT_HEAD", oid);
This can be easily reduced to
return get_oid(file_exists(git_path_bisect_head()) ?
"BISECT_HEAD" : "HEAD", oid);
At the same time, it is wrong, just like the shell script version was
wrong: in particular in light of the `hn/reftable` effort, we do _not_
want to assume that all refs are backed by files!
So really, what this should do instead is this:
enum get_oid_result res = get_oid("BISECT_HEAD", oid);
if (res == MISSING_OBJECT)
res = get_oid("HEAD", oid);
Given that this is still only three lines long, the overhead of having it
in its own function for just a _single_ call seems excessive. I'd prefer
it to be inlined in `bisect_state()`.
> +}
> +
> +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
> + int argc)
> +{
I offered a lengthy discussion about this function in
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002272244150.9783@tvgsbejvaqbjf.bet/
It does not look, however, as if v3 benefitted from the entirety of my
analysis: All the `check_expected_revs()` function does is to verify that
the passed list of revs matches exactly the contents of the
`BISECT_EXPECTED_REV` file.
That can be done in a much simpler way, though, by first reading the file
and parsing the contents into an OID, and then comparing to that parsed
OID instead.
Besides, `check_expected_revs()` is only used to check one rev at a time.
In other words, it could be simplified to something like this:
static void check_expected_rev(struct object_id *oid)
{
struct object_id expected;
struct strbuf buf = STRBUF_INIT;
if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0)
< the_hash_algo->hexsz ||
get_oid_hex(buf.buf, &expected) < 0)
return; /* Ignore invalid file contents */
if (!oideq(oid, &expected)) {
... unlink ...
return;
}
}
But even that would be wasteful, as we would read the file over and over
and over again.
The good news is that we do not even _need_ `check_expected_rev()`.
Because we do not need to have two call sites, we can simplify the code
much further. See below:
> + const char *state;
> + const char *hex;
> + int i;
> + struct oid_array revs = OID_ARRAY_INIT;
> + struct object_id oid;
> +
> + if (!argc)
> + return error(_("Please call `--bisect-state` with at least one argument"));
> + state = argv[0];
> + if (check_and_set_terms(terms, state) ||
> + !one_of(state, terms->term_good,terms->term_bad, "skip", NULL))
> + return BISECT_FAILED;
> + argv++;
> + argc--;
> + if (!strcmp(state, terms->term_bad) && (argc > 1))
> + return error(_("'git bisect %s' can take only one argument."),terms->term_bad);
> + if (argc == 0) {
> + if (bisect_head(&oid))
> + return error(_("Bad bisect_head rev input"));
> + hex = oid_to_hex(&oid);
> + if (bisect_write(state, hex, terms, 0))
> + return BISECT_FAILED;
> + check_expected_revs(&hex, 1);
> + return bisect_auto_next(terms, NULL);
> + }
> +
> + /* Here argc > 0 */
> + for (; argc; argc--, argv++) {
> + struct object_id oid;
> + if (get_oid(*argv, &oid))
> + return error(_("Bad rev input: %s"), *argv);
> + oid_array_append(&revs, &oid);
> + }
It really does not make sense to parse the arguments into an OID array,
_then_ iterate over the array once, and then immediately releasing it.
That OID array is not needed at all.
So we'll end up with this loop in case `argc > 0` (where we now call
`get_oid()`, too), and note how the loop body looks _eerily_ similar to
the conditional `argc == 0` code block above?
> +
> + for (i = 0; i < revs.nr; i++) {
> + hex = oid_to_hex(&revs.oid[i]);
> + if (bisect_write(state, hex, terms, 0)) {
> + oid_array_clear(&revs);
> + return BISECT_FAILED;
> + }
> + check_expected_revs(&hex, 1);
> + }
> +
> + oid_array_clear(&revs);
> + return bisect_auto_next(terms, NULL);
> +}
So really, this function pretty much _wants_ to look this way (modulo
bugs, as I did not even test-compile the code):
static enum bisect_error bisect_state(struct bisect_terms *terms,
const char **argv, int argc)
{
const char *state;
int i, verify_expected = 1;
struct object_id oid, expected;
struct strbuf buf = STRBUF_INIT;
if (!argc)
return error(_("Please call `--bisect-state` with at least one argument"));
state = argv[0];
if (check_and_set_terms(terms, state) ||
!one_of(state, terms->term_good, terms->term_bad, "skip", NULL))
return BISECT_FAILED;
argv++;
argc--;
if (argc > 1 && !strcmp(state, terms->term_bad))
return error(_("'git bisect %s' can take only one argument."), terms->term_bad);
if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||
get_oid_hex(buf.buf, &expected) < 0)
verify_expected = 0; /* Ignore invalid file contents */
for (i = 0; i < argc + !argc; i++) {
if (argc) {
if (get_oid(argv[i], &oid)) {
error(_("Bad rev input: %s"), *argv);
return BISECT_FAILED;
}
} else {
enum get_oid_result res = get_oid("BISECT_HEAD", &oid);
if (res == MISSING_OBJECT)
res = get_oid("HEAD", &oid);
if (res) {
error(_("Bad bisect_head rev input"));
return BISECT_FAILED;
}
}
if (bisect_write(state, oid_to_hex(&oid), terms, 0))
return BISECT_FAILED;
if (verify_expected && !oideq(&oid, &expected)) {
unlink_or_warn(git_path_bisect_ancestors_ok());
unlink_or_warn(git_path_bisect_expected_rev());
verify_expected = 0;
}
}
return bisect_auto_next(terms, NULL);
}
There, not bad, is it?
> +
> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> {
> enum {
> @@ -847,7 +907,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> BISECT_START,
> BISECT_NEXT,
> BISECT_AUTO_NEXT,
> - BISECT_AUTOSTART
> + BISECT_AUTOSTART,
> + BISECT_STATE
> } cmdmode = 0;
> int no_checkout = 0, res = 0, nolog = 0;
> struct option options[] = {
> @@ -873,6 +934,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
> OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
> N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
> + OPT_CMDMODE(0, "bisect-state", &cmdmode,
> + N_("mark the state of ref (or refs)"), BISECT_STATE),
> OPT_BOOL(0, "no-checkout", &no_checkout,
> N_("update BISECT_HEAD instead of checking out the current commit")),
> OPT_BOOL(0, "no-log", &nolog,
> @@ -945,6 +1008,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> set_terms(&terms, "bad", "good");
> res = bisect_autostart(&terms);
> break;
> + case BISECT_STATE:
> + set_terms(&terms, "bad", "good");
> + get_terms(&terms);
> + res = bisect_state(&terms, argv, argc);
> + break;
> default:
> BUG("unknown subcommand %d", (int)cmdmode);
> }
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 049ffacdff..2da0810b1a 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -39,16 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
> TERM_BAD=bad
> TERM_GOOD=good
>
> -bisect_head()
> -{
> - if test -f "$GIT_DIR/BISECT_HEAD"
> - then
> - echo BISECT_HEAD
> - else
> - echo HEAD
> - fi
> -}
> -
> bisect_skip() {
> all=''
> for arg in "$@"
> @@ -61,43 +51,7 @@ bisect_skip() {
> esac
> all="$all $revs"
> done
> - eval bisect_state 'skip' $all
> -}
> -
> -bisect_state() {
> - git bisect--helper --bisect-autostart
> - state=$1
> - git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
> - get_terms
> - case "$#,$state" in
> - 0,*)
> - die "Please call 'bisect_state' with at least one argument." ;;
> - 1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip)
> - bisected_head=$(bisect_head)
> - rev=$(git rev-parse --verify "$bisected_head") ||
> - die "$(eval_gettext "Bad rev input: \$bisected_head")"
> - git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
> - git bisect--helper --check-expected-revs "$rev" ;;
> - 2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
> - shift
> - hash_list=''
> - for rev in "$@"
> - do
> - sha=$(git rev-parse --verify "$rev^{commit}") ||
> - die "$(eval_gettext "Bad rev input: \$rev")"
> - hash_list="$hash_list $sha"
> - done
> - for rev in $hash_list
> - do
> - git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
> - done
> - git bisect--helper --check-expected-revs $hash_list ;;
> - *,"$TERM_BAD")
> - die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;;
> - *)
> - usage ;;
> - esac
> - git bisect--helper --bisect-auto-next
> + eval git bisect--helper --bisect-state 'skip' $all
> }
>
> bisect_visualize() {
> @@ -185,8 +139,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> state="$TERM_GOOD"
> fi
>
> - # We have to use a subshell because "bisect_state" can exit.
> - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
> + git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN"
> res=$?
>
> cat "$GIT_DIR/BISECT_RUN"
> @@ -201,7 +154,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> if [ $res -ne 0 ]
> then
> eval_gettextln "bisect run failed:
> -'bisect_state \$state' exited with error code \$res" >&2
> +'git bisect--helper --bisect-state \$state' exited with error code \$res" >&2
This is not your fault, of course, but it does make me shudder to see such
an obvious implementation detail in a user-facing error message.
Maybe something to fix up in a follow-up?
Ciao,
Dscho
> exit $res
> fi
>
> @@ -242,7 +195,7 @@ case "$#" in
> start)
> git bisect--helper --bisect-start "$@" ;;
> bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
> - bisect_state "$cmd" "$@" ;;
> + git bisect--helper --bisect-state "$cmd" "$@" ;;
> skip)
> bisect_skip "$@" ;;
> next)
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 00/12] Finish converting git bisect to C part 2
2020-04-23 7:06 [PATCH v3 00/12] Finish converting git bisect to C part 2 Miriam Rubio
` (12 preceding siblings ...)
2020-04-23 20:01 ` [PATCH v3 00/12] Finish converting git bisect to C part 2 Junio C Hamano
@ 2020-05-22 22:09 ` Johannes Schindelin
2020-05-24 21:19 ` Miriam R.
13 siblings, 1 reply; 28+ messages in thread
From: Johannes Schindelin @ 2020-05-22 22:09 UTC (permalink / raw)
To: Miriam Rubio; +Cc: git
Hi Miriam,
On Thu, 23 Apr 2020, Miriam Rubio wrote:
> These patches correspond to a second part of patch series
> of Outreachy project "Finish converting `git bisect` from shell to C"
> started by Pranit Bauva and Tanushree Tumane
> (https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and
> continued by me.
>
> These patch series emails were generated from:
> https://gitlab.com/mirucam/git/commits/git-bisect-work-part2-v3.
>
> I would like to thank Junio Hamano for reviewing this patch series and
> Christian Couder for his help.
>
> --- Changes since v2 Finish converting git bisect to C part 2 patch series ---
Thank you for this detailed summary!
I reviewed the patches in their entirety, and left a couple of
suggestions, some of them minor.
Hopefully you find them helpful!
Ciao,
Dscho
>
> General changes
> ---------------
>
> * Rebase on master branch: efe3874640 (Sync with v2.26.1, 2020-04-13)
>
> Specific changes
> ----------------
>
> [1/12] bisect--helper: fix `cmd_*()` function switch default return
>
> * Use `BUG()` instead of `return error()` in default switch.
>
> --
>
> [2/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return
>
> * New patch: use '-res' instead of 'abs(res)'.
>
> --
>
> [3/12] bisect--helper: introduce new `write_in_file()` function
>
> * Rename input parameter `filepath` to `path`.
> * Change `error_errno()` to `error()` in mode checking.
> * Change error message when file cannot be opened.
> * Add `fclose()` before error return.
>
> --
>
> [4/12] bisect--helper: reimplement `bisect_autostart` shell function in C
>
> * Reorder patch before `reimplement `bisect_next` and `bisect_auto_next`
> shell functions in C` to use `bisect_autostart()` function in
> `bisect_append_log_quoted()`.
>
> --
> `
> [5/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
> functions in C
>
> * Amend commit message.
> * Add `clear_commit_marks()` at the end of bisect_next_all() and remove it from
> `bisect_rev_setup()`.
> * Fix if condition.
> * Add `bisect_autostart()` in `bisect_append_log_quoted()`.
> * Check `git_path_bisect_head()` with `file_exist()` instead of
> `is_empty_or_missing_file()`.
> * Fix return with BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.
> * Add conversion in `cmd_*()` to BISECT_OK when return is
> BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.
>
> * Note to previous reviewer: suggestion to change `bisect_auto_next()` function
> to return void cannot be done because errors from `bisect_next()` are not
> ignored.
>
> --
>
> [5/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell
> functions in C
>
> * Check `git_path_bisect_head()` with `file_exist()` instead of
> `is_empty_or_missing_file()`.
>
> --
>
> Miriam Rubio (3):
> bisect--helper: fix `cmd_*()` function switch default return
> bisect--helper: use '-res' in 'cmd_bisect__helper' return
> bisect--helper: introduce new `write_in_file()` function
>
> Pranit Bauva (9):
> bisect--helper: reimplement `bisect_autostart` shell function in C
> bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
> functions in C
> bisect--helper: finish porting `bisect_start()` to C
> bisect--helper: retire `--bisect-clean-state` subcommand
> bisect--helper: retire `--next-all` subcommand
> bisect--helper: reimplement `bisect_state` & `bisect_head` shell
> functions in C
> bisect--helper: retire `--check-expected-revs` subcommand
> bisect--helper: retire `--write-terms` subcommand
> bisect--helper: retire `--bisect-autostart` subcommand
>
> bisect.c | 8 +
> builtin/bisect--helper.c | 383 +++++++++++++++++++++++++++++++++------
> git-bisect.sh | 145 +--------------
> 3 files changed, 347 insertions(+), 189 deletions(-)
>
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 03/12] bisect--helper: introduce new `write_in_file()` function
2020-04-23 7:06 ` [PATCH v3 03/12] bisect--helper: introduce new `write_in_file()` function Miriam Rubio
2020-05-22 13:25 ` Johannes Schindelin
@ 2020-05-23 1:53 ` Đoàn Trần Công Danh
1 sibling, 0 replies; 28+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-23 1:53 UTC (permalink / raw)
To: Miriam Rubio; +Cc: git, Christian Couder, Johannes Schindelin
On 2020-04-23 09:06:55+0200, Miriam Rubio <mirucam@gmail.com> wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 0fbd924aac..d3b2b33df0 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -74,6 +74,28 @@ static int one_of(const char *term, ...)
> return res;
> }
>
> +static int write_in_file(const char *path, const char *mode, const char *format,...)
Nit: It looks like we want to add a space in the end ", ...)"
Since "one_of" above also has that space.
Otherwise, look good.
--
Danh
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 00/12] Finish converting git bisect to C part 2
2020-05-22 22:09 ` Johannes Schindelin
@ 2020-05-24 21:19 ` Miriam R.
0 siblings, 0 replies; 28+ messages in thread
From: Miriam R. @ 2020-05-24 21:19 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
Hi Johannes,
El sáb., 23 may. 2020 a las 0:09, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi Miriam,
>
> On Thu, 23 Apr 2020, Miriam Rubio wrote:
>
> > These patches correspond to a second part of patch series
> > of Outreachy project "Finish converting `git bisect` from shell to C"
> > started by Pranit Bauva and Tanushree Tumane
> > (https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and
> > continued by me.
> >
> > These patch series emails were generated from:
> > https://gitlab.com/mirucam/git/commits/git-bisect-work-part2-v3.
> >
> > I would like to thank Junio Hamano for reviewing this patch series and
> > Christian Couder for his help.
> >
> > --- Changes since v2 Finish converting git bisect to C part 2 patch series ---
>
> Thank you for this detailed summary!
>
> I reviewed the patches in their entirety, and left a couple of
> suggestions, some of them minor.
>
Thank you very much for reviewing.
> Hopefully you find them helpful!
Of course! :)
I hope I can send the next version soon.
Best,
Miriam
>
> Ciao,
> Dscho
>
> >
> > General changes
> > ---------------
> >
> > * Rebase on master branch: efe3874640 (Sync with v2.26.1, 2020-04-13)
> >
> > Specific changes
> > ----------------
> >
> > [1/12] bisect--helper: fix `cmd_*()` function switch default return
> >
> > * Use `BUG()` instead of `return error()` in default switch.
> >
> > --
> >
> > [2/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return
> >
> > * New patch: use '-res' instead of 'abs(res)'.
> >
> > --
> >
> > [3/12] bisect--helper: introduce new `write_in_file()` function
> >
> > * Rename input parameter `filepath` to `path`.
> > * Change `error_errno()` to `error()` in mode checking.
> > * Change error message when file cannot be opened.
> > * Add `fclose()` before error return.
> >
> > --
> >
> > [4/12] bisect--helper: reimplement `bisect_autostart` shell function in C
> >
> > * Reorder patch before `reimplement `bisect_next` and `bisect_auto_next`
> > shell functions in C` to use `bisect_autostart()` function in
> > `bisect_append_log_quoted()`.
> >
> > --
> > `
> > [5/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
> > functions in C
> >
> > * Amend commit message.
> > * Add `clear_commit_marks()` at the end of bisect_next_all() and remove it from
> > `bisect_rev_setup()`.
> > * Fix if condition.
> > * Add `bisect_autostart()` in `bisect_append_log_quoted()`.
> > * Check `git_path_bisect_head()` with `file_exist()` instead of
> > `is_empty_or_missing_file()`.
> > * Fix return with BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.
> > * Add conversion in `cmd_*()` to BISECT_OK when return is
> > BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.
> >
> > * Note to previous reviewer: suggestion to change `bisect_auto_next()` function
> > to return void cannot be done because errors from `bisect_next()` are not
> > ignored.
> >
> > --
> >
> > [5/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell
> > functions in C
> >
> > * Check `git_path_bisect_head()` with `file_exist()` instead of
> > `is_empty_or_missing_file()`.
> >
> > --
> >
> > Miriam Rubio (3):
> > bisect--helper: fix `cmd_*()` function switch default return
> > bisect--helper: use '-res' in 'cmd_bisect__helper' return
> > bisect--helper: introduce new `write_in_file()` function
> >
> > Pranit Bauva (9):
> > bisect--helper: reimplement `bisect_autostart` shell function in C
> > bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
> > functions in C
> > bisect--helper: finish porting `bisect_start()` to C
> > bisect--helper: retire `--bisect-clean-state` subcommand
> > bisect--helper: retire `--next-all` subcommand
> > bisect--helper: reimplement `bisect_state` & `bisect_head` shell
> > functions in C
> > bisect--helper: retire `--check-expected-revs` subcommand
> > bisect--helper: retire `--write-terms` subcommand
> > bisect--helper: retire `--bisect-autostart` subcommand
> >
> > bisect.c | 8 +
> > builtin/bisect--helper.c | 383 +++++++++++++++++++++++++++++++++------
> > git-bisect.sh | 145 +--------------
> > 3 files changed, 347 insertions(+), 189 deletions(-)
> >
> > --
> > 2.25.0
> >
> >
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 09/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C
2020-06-20 8:04 ` Miriam R.
@ 2020-06-19 13:57 ` Johannes Schindelin
0 siblings, 0 replies; 28+ messages in thread
From: Johannes Schindelin @ 2020-06-19 13:57 UTC (permalink / raw)
To: Miriam R.; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 17614 bytes --]
Hi Miriam,
On Sat, 20 Jun 2020, Miriam R. wrote:
> Hi Johannes,
> I'm finishing the next patch series version and I have an issue about
> one of your suggestions:
>
> El sáb., 23 may. 2020 a las 0:06, Johannes Schindelin
> (<Johannes.Schindelin@gmx.de>) escribió:
> >
> > Hi Miriam,
> >
> > On Thu, 23 Apr 2020, Miriam Rubio wrote:
> >
> > > From: Pranit Bauva <pranit.bauva@gmail.com>
> > >
> > > Reimplement the `bisect_state()` shell functions in C and also add a
> > > subcommand `--bisect-state` to `git-bisect--helper` to call them from
> > > git-bisect.sh .
> > >
> > > Using `--bisect-state` subcommand is a temporary measure to port shell
> > > function to C so as to use the existing test suite. As more functions
> > > are ported, this subcommand will be retired and will be called by some
> > > other methods.
> > >
> > > `bisect_head()` is only called from `bisect_state()`, thus it is not
> > > required to introduce another subcommand.
> > >
> > > Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> > > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > > Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > > Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> > > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> > > Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> > > ---
> > > builtin/bisect--helper.c | 70 +++++++++++++++++++++++++++++++++++++++-
> > > git-bisect.sh | 55 +++----------------------------
> > > 2 files changed, 73 insertions(+), 52 deletions(-)
> > >
> > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> > > index 2d8660c79f..9db72f5891 100644
> > > --- a/builtin/bisect--helper.c
> > > +++ b/builtin/bisect--helper.c
> > > @@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {
> > > N_("git bisect--helper --bisect-next"),
> > > N_("git bisect--helper --bisect-auto-next"),
> > > N_("git bisect--helper --bisect-autostart"),
> > > + N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
> > > + N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
> > > NULL
> > > };
> > >
> > > @@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms)
> > > return bisect_start(terms, 0, NULL, 0);
> > > }
> > >
> > > +static int bisect_head(struct object_id *oid)
> > > +{
> > > + if (!file_exists(git_path_bisect_head()))
> > > + return get_oid("HEAD", oid);
> > > +
> > > + return get_oid("BISECT_HEAD", oid);
> >
> > This can be easily reduced to
> >
> > return get_oid(file_exists(git_path_bisect_head()) ?
> > "BISECT_HEAD" : "HEAD", oid);
> >
> > At the same time, it is wrong, just like the shell script version was
> > wrong: in particular in light of the `hn/reftable` effort, we do _not_
> > want to assume that all refs are backed by files!
> >
> > So really, what this should do instead is this:
> >
> > enum get_oid_result res = get_oid("BISECT_HEAD", oid);
> >
> > if (res == MISSING_OBJECT)
> > res = get_oid("HEAD", oid);
> >
> > Given that this is still only three lines long, the overhead of having it
> > in its own function for just a _single_ call seems excessive. I'd prefer
> > it to be inlined in `bisect_state()`.
> >
> > > +}
> > > +
> > > +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
> > > + int argc)
> > > +{
> >
> > I offered a lengthy discussion about this function in
> > https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002272244150.9783@tvgsbejvaqbjf.bet/
> >
> > It does not look, however, as if v3 benefitted from the entirety of my
> > analysis: All the `check_expected_revs()` function does is to verify that
> > the passed list of revs matches exactly the contents of the
> > `BISECT_EXPECTED_REV` file.
> >
> > That can be done in a much simpler way, though, by first reading the file
> > and parsing the contents into an OID, and then comparing to that parsed
> > OID instead.
> >
> > Besides, `check_expected_revs()` is only used to check one rev at a time.
> >
> > In other words, it could be simplified to something like this:
> >
> > static void check_expected_rev(struct object_id *oid)
> > {
> > struct object_id expected;
> > struct strbuf buf = STRBUF_INIT;
> >
> > if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0)
> > < the_hash_algo->hexsz ||
> > get_oid_hex(buf.buf, &expected) < 0)
> > return; /* Ignore invalid file contents */
> >
> > if (!oideq(oid, &expected)) {
> > ... unlink ...
> > return;
> > }
> > }
> >
> > But even that would be wasteful, as we would read the file over and over
> > and over again.
> >
> > The good news is that we do not even _need_ `check_expected_rev()`.
> > Because we do not need to have two call sites, we can simplify the code
> > much further. See below:
> >
> > > + const char *state;
> > > + const char *hex;
> > > + int i;
> > > + struct oid_array revs = OID_ARRAY_INIT;
> > > + struct object_id oid;
> > > +
> > > + if (!argc)
> > > + return error(_("Please call `--bisect-state` with at least one argument"));
> > > + state = argv[0];
> > > + if (check_and_set_terms(terms, state) ||
> > > + !one_of(state, terms->term_good,terms->term_bad, "skip", NULL))
> > > + return BISECT_FAILED;
> > > + argv++;
> > > + argc--;
> > > + if (!strcmp(state, terms->term_bad) && (argc > 1))
> > > + return error(_("'git bisect %s' can take only one argument."),terms->term_bad);
> > > + if (argc == 0) {
> > > + if (bisect_head(&oid))
> > > + return error(_("Bad bisect_head rev input"));
> > > + hex = oid_to_hex(&oid);
> > > + if (bisect_write(state, hex, terms, 0))
> > > + return BISECT_FAILED;
> > > + check_expected_revs(&hex, 1);
> > > + return bisect_auto_next(terms, NULL);
> > > + }
> > > +
> > > + /* Here argc > 0 */
> > > + for (; argc; argc--, argv++) {
> > > + struct object_id oid;
> > > + if (get_oid(*argv, &oid))
> > > + return error(_("Bad rev input: %s"), *argv);
> > > + oid_array_append(&revs, &oid);
> > > + }
> >
> > It really does not make sense to parse the arguments into an OID array,
> > _then_ iterate over the array once, and then immediately releasing it.
> > That OID array is not needed at all.
> >
> > So we'll end up with this loop in case `argc > 0` (where we now call
> > `get_oid()`, too), and note how the loop body looks _eerily_ similar to
> > the conditional `argc == 0` code block above?
> >
> > > +
> > > + for (i = 0; i < revs.nr; i++) {
> > > + hex = oid_to_hex(&revs.oid[i]);
> > > + if (bisect_write(state, hex, terms, 0)) {
> > > + oid_array_clear(&revs);
> > > + return BISECT_FAILED;
> > > + }
> > > + check_expected_revs(&hex, 1);
> > > + }
> > > +
> > > + oid_array_clear(&revs);
> > > + return bisect_auto_next(terms, NULL);
> > > +}
> >
> > So really, this function pretty much _wants_ to look this way (modulo
> > bugs, as I did not even test-compile the code):
> >
> > static enum bisect_error bisect_state(struct bisect_terms *terms,
> > const char **argv, int argc)
> > {
> > const char *state;
> > int i, verify_expected = 1;
> > struct object_id oid, expected;
> > struct strbuf buf = STRBUF_INIT;
> >
> > if (!argc)
> > return error(_("Please call `--bisect-state` with at least one argument"));
> >
> > state = argv[0];
> > if (check_and_set_terms(terms, state) ||
> > !one_of(state, terms->term_good, terms->term_bad, "skip", NULL))
> > return BISECT_FAILED;
> >
> > argv++;
> > argc--;
> > if (argc > 1 && !strcmp(state, terms->term_bad))
> > return error(_("'git bisect %s' can take only one argument."), terms->term_bad);
> >
> > if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||
> > get_oid_hex(buf.buf, &expected) < 0)
> > verify_expected = 0; /* Ignore invalid file contents */
> >
> >
> > for (i = 0; i < argc + !argc; i++) {
> > if (argc) {
> > if (get_oid(argv[i], &oid)) {
> > error(_("Bad rev input: %s"), *argv);
> > return BISECT_FAILED;
> > }
> > } else {
> > enum get_oid_result res = get_oid("BISECT_HEAD", &oid);
> >
> > if (res == MISSING_OBJECT)
> > res = get_oid("HEAD", &oid);
> > if (res) {
> > error(_("Bad bisect_head rev input"));
> > return BISECT_FAILED;
> > }
> > }
> >
> > if (bisect_write(state, oid_to_hex(&oid), terms, 0))
> > return BISECT_FAILED;
> >
> > if (verify_expected && !oideq(&oid, &expected)) {
> > unlink_or_warn(git_path_bisect_ancestors_ok());
> > unlink_or_warn(git_path_bisect_expected_rev());
> > verify_expected = 0;
> > }
> > }
> >
> > return bisect_auto_next(terms, NULL);
> > }
> >
> > There, not bad, is it?
> >
> After implementing this solution some tests failed. After debugging
> them, I found that with Pranit's solution, that arguments were parsed
> into an OID array, if bisect received some junk rev the function
> returned and bisect_write() was not executed.
That's a good point, and should be turned into a code comment lest
overzealous developers would attempt the simplification I proposed in the
future.
> With the new solution, if junk rev is received after a valid rev,
> bisect_write() was executed for the valid and the function returned with
> the junk rev.
> So, there is garbage in the file and when for example bisect-porcelain
> test number 5 - 'bisect fails if given any junk instead of revs'
> executes 'test -z' fails.
>
> Should I keep the original patch and add a comment in the code that
> explains why we use an oid array?.
I still think that the `check_expected_revs()` function is not needed, and
unnecessarily complex.
If you _want_ to keep Pranit's original implementation, then I would _at
least_ want you to forward-port the `verify_expected` changes I suggested.
>
> (I also have implemented an alternative solution that when some junk
> rev is found, I delete all refs written, but maybe is too complicated
> or not totally correct:
> https://gitlab.com/mirucam/git/-/commit/93f669877b87d09a30a5d07f0967667b22026511
> )
You seem to share my preference for not writing out bogus revs in the
first place.
Thanks,
Johannes
>
>
> > > +
> > > int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> > > {
> > > enum {
> > > @@ -847,7 +907,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> > > BISECT_START,
> > > BISECT_NEXT,
> > > BISECT_AUTO_NEXT,
> > > - BISECT_AUTOSTART
> > > + BISECT_AUTOSTART,
> > > + BISECT_STATE
> > > } cmdmode = 0;
> > > int no_checkout = 0, res = 0, nolog = 0;
> > > struct option options[] = {
> > > @@ -873,6 +934,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> > > N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
> > > OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
> > > N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
> > > + OPT_CMDMODE(0, "bisect-state", &cmdmode,
> > > + N_("mark the state of ref (or refs)"), BISECT_STATE),
> > > OPT_BOOL(0, "no-checkout", &no_checkout,
> > > N_("update BISECT_HEAD instead of checking out the current commit")),
> > > OPT_BOOL(0, "no-log", &nolog,
> > > @@ -945,6 +1008,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> > > set_terms(&terms, "bad", "good");
> > > res = bisect_autostart(&terms);
> > > break;
> > > + case BISECT_STATE:
> > > + set_terms(&terms, "bad", "good");
> > > + get_terms(&terms);
> > > + res = bisect_state(&terms, argv, argc);
> > > + break;
> > > default:
> > > BUG("unknown subcommand %d", (int)cmdmode);
> > > }
> > > diff --git a/git-bisect.sh b/git-bisect.sh
> > > index 049ffacdff..2da0810b1a 100755
> > > --- a/git-bisect.sh
> > > +++ b/git-bisect.sh
> > > @@ -39,16 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
> > > TERM_BAD=bad
> > > TERM_GOOD=good
> > >
> > > -bisect_head()
> > > -{
> > > - if test -f "$GIT_DIR/BISECT_HEAD"
> > > - then
> > > - echo BISECT_HEAD
> > > - else
> > > - echo HEAD
> > > - fi
> > > -}
> > > -
> > > bisect_skip() {
> > > all=''
> > > for arg in "$@"
> > > @@ -61,43 +51,7 @@ bisect_skip() {
> > > esac
> > > all="$all $revs"
> > > done
> > > - eval bisect_state 'skip' $all
> > > -}
> > > -
> > > -bisect_state() {
> > > - git bisect--helper --bisect-autostart
> > > - state=$1
> > > - git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
> > > - get_terms
> > > - case "$#,$state" in
> > > - 0,*)
> > > - die "Please call 'bisect_state' with at least one argument." ;;
> > > - 1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip)
> > > - bisected_head=$(bisect_head)
> > > - rev=$(git rev-parse --verify "$bisected_head") ||
> > > - die "$(eval_gettext "Bad rev input: \$bisected_head")"
> > > - git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
> > > - git bisect--helper --check-expected-revs "$rev" ;;
> > > - 2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
> > > - shift
> > > - hash_list=''
> > > - for rev in "$@"
> > > - do
> > > - sha=$(git rev-parse --verify "$rev^{commit}") ||
> > > - die "$(eval_gettext "Bad rev input: \$rev")"
> > > - hash_list="$hash_list $sha"
> > > - done
> > > - for rev in $hash_list
> > > - do
> > > - git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
> > > - done
> > > - git bisect--helper --check-expected-revs $hash_list ;;
> > > - *,"$TERM_BAD")
> > > - die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;;
> > > - *)
> > > - usage ;;
> > > - esac
> > > - git bisect--helper --bisect-auto-next
> > > + eval git bisect--helper --bisect-state 'skip' $all
> > > }
> > >
> > > bisect_visualize() {
> > > @@ -185,8 +139,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> > > state="$TERM_GOOD"
> > > fi
> > >
> > > - # We have to use a subshell because "bisect_state" can exit.
> > > - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
> > > + git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN"
> > > res=$?
> > >
> > > cat "$GIT_DIR/BISECT_RUN"
> > > @@ -201,7 +154,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> > > if [ $res -ne 0 ]
> > > then
> > > eval_gettextln "bisect run failed:
> > > -'bisect_state \$state' exited with error code \$res" >&2
> > > +'git bisect--helper --bisect-state \$state' exited with error code \$res" >&2
> >
> > This is not your fault, of course, but it does make me shudder to see such
> > an obvious implementation detail in a user-facing error message.
> >
> > Maybe something to fix up in a follow-up?
> >
> > Ciao,
> > Dscho
> >
> > > exit $res
> > > fi
> > >
> > > @@ -242,7 +195,7 @@ case "$#" in
> > > start)
> > > git bisect--helper --bisect-start "$@" ;;
> > > bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
> > > - bisect_state "$cmd" "$@" ;;
> > > + git bisect--helper --bisect-state "$cmd" "$@" ;;
> > > skip)
> > > bisect_skip "$@" ;;
> > > next)
> > > --
> > > 2.25.0
> > >
> > >
> Best,
> Miriam.
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 09/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C
2020-05-22 22:06 ` Johannes Schindelin
@ 2020-06-20 8:04 ` Miriam R.
2020-06-19 13:57 ` Johannes Schindelin
0 siblings, 1 reply; 28+ messages in thread
From: Miriam R. @ 2020-06-20 8:04 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
Hi Johannes,
I'm finishing the next patch series version and I have an issue about
one of your suggestions:
El sáb., 23 may. 2020 a las 0:06, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi Miriam,
>
> On Thu, 23 Apr 2020, Miriam Rubio wrote:
>
> > From: Pranit Bauva <pranit.bauva@gmail.com>
> >
> > Reimplement the `bisect_state()` shell functions in C and also add a
> > subcommand `--bisect-state` to `git-bisect--helper` to call them from
> > git-bisect.sh .
> >
> > Using `--bisect-state` subcommand is a temporary measure to port shell
> > function to C so as to use the existing test suite. As more functions
> > are ported, this subcommand will be retired and will be called by some
> > other methods.
> >
> > `bisect_head()` is only called from `bisect_state()`, thus it is not
> > required to introduce another subcommand.
> >
> > Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> > Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> > ---
> > builtin/bisect--helper.c | 70 +++++++++++++++++++++++++++++++++++++++-
> > git-bisect.sh | 55 +++----------------------------
> > 2 files changed, 73 insertions(+), 52 deletions(-)
> >
> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> > index 2d8660c79f..9db72f5891 100644
> > --- a/builtin/bisect--helper.c
> > +++ b/builtin/bisect--helper.c
> > @@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {
> > N_("git bisect--helper --bisect-next"),
> > N_("git bisect--helper --bisect-auto-next"),
> > N_("git bisect--helper --bisect-autostart"),
> > + N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
> > + N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
> > NULL
> > };
> >
> > @@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms)
> > return bisect_start(terms, 0, NULL, 0);
> > }
> >
> > +static int bisect_head(struct object_id *oid)
> > +{
> > + if (!file_exists(git_path_bisect_head()))
> > + return get_oid("HEAD", oid);
> > +
> > + return get_oid("BISECT_HEAD", oid);
>
> This can be easily reduced to
>
> return get_oid(file_exists(git_path_bisect_head()) ?
> "BISECT_HEAD" : "HEAD", oid);
>
> At the same time, it is wrong, just like the shell script version was
> wrong: in particular in light of the `hn/reftable` effort, we do _not_
> want to assume that all refs are backed by files!
>
> So really, what this should do instead is this:
>
> enum get_oid_result res = get_oid("BISECT_HEAD", oid);
>
> if (res == MISSING_OBJECT)
> res = get_oid("HEAD", oid);
>
> Given that this is still only three lines long, the overhead of having it
> in its own function for just a _single_ call seems excessive. I'd prefer
> it to be inlined in `bisect_state()`.
>
> > +}
> > +
> > +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
> > + int argc)
> > +{
>
> I offered a lengthy discussion about this function in
> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002272244150.9783@tvgsbejvaqbjf.bet/
>
> It does not look, however, as if v3 benefitted from the entirety of my
> analysis: All the `check_expected_revs()` function does is to verify that
> the passed list of revs matches exactly the contents of the
> `BISECT_EXPECTED_REV` file.
>
> That can be done in a much simpler way, though, by first reading the file
> and parsing the contents into an OID, and then comparing to that parsed
> OID instead.
>
> Besides, `check_expected_revs()` is only used to check one rev at a time.
>
> In other words, it could be simplified to something like this:
>
> static void check_expected_rev(struct object_id *oid)
> {
> struct object_id expected;
> struct strbuf buf = STRBUF_INIT;
>
> if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0)
> < the_hash_algo->hexsz ||
> get_oid_hex(buf.buf, &expected) < 0)
> return; /* Ignore invalid file contents */
>
> if (!oideq(oid, &expected)) {
> ... unlink ...
> return;
> }
> }
>
> But even that would be wasteful, as we would read the file over and over
> and over again.
>
> The good news is that we do not even _need_ `check_expected_rev()`.
> Because we do not need to have two call sites, we can simplify the code
> much further. See below:
>
> > + const char *state;
> > + const char *hex;
> > + int i;
> > + struct oid_array revs = OID_ARRAY_INIT;
> > + struct object_id oid;
> > +
> > + if (!argc)
> > + return error(_("Please call `--bisect-state` with at least one argument"));
> > + state = argv[0];
> > + if (check_and_set_terms(terms, state) ||
> > + !one_of(state, terms->term_good,terms->term_bad, "skip", NULL))
> > + return BISECT_FAILED;
> > + argv++;
> > + argc--;
> > + if (!strcmp(state, terms->term_bad) && (argc > 1))
> > + return error(_("'git bisect %s' can take only one argument."),terms->term_bad);
> > + if (argc == 0) {
> > + if (bisect_head(&oid))
> > + return error(_("Bad bisect_head rev input"));
> > + hex = oid_to_hex(&oid);
> > + if (bisect_write(state, hex, terms, 0))
> > + return BISECT_FAILED;
> > + check_expected_revs(&hex, 1);
> > + return bisect_auto_next(terms, NULL);
> > + }
> > +
> > + /* Here argc > 0 */
> > + for (; argc; argc--, argv++) {
> > + struct object_id oid;
> > + if (get_oid(*argv, &oid))
> > + return error(_("Bad rev input: %s"), *argv);
> > + oid_array_append(&revs, &oid);
> > + }
>
> It really does not make sense to parse the arguments into an OID array,
> _then_ iterate over the array once, and then immediately releasing it.
> That OID array is not needed at all.
>
> So we'll end up with this loop in case `argc > 0` (where we now call
> `get_oid()`, too), and note how the loop body looks _eerily_ similar to
> the conditional `argc == 0` code block above?
>
> > +
> > + for (i = 0; i < revs.nr; i++) {
> > + hex = oid_to_hex(&revs.oid[i]);
> > + if (bisect_write(state, hex, terms, 0)) {
> > + oid_array_clear(&revs);
> > + return BISECT_FAILED;
> > + }
> > + check_expected_revs(&hex, 1);
> > + }
> > +
> > + oid_array_clear(&revs);
> > + return bisect_auto_next(terms, NULL);
> > +}
>
> So really, this function pretty much _wants_ to look this way (modulo
> bugs, as I did not even test-compile the code):
>
> static enum bisect_error bisect_state(struct bisect_terms *terms,
> const char **argv, int argc)
> {
> const char *state;
> int i, verify_expected = 1;
> struct object_id oid, expected;
> struct strbuf buf = STRBUF_INIT;
>
> if (!argc)
> return error(_("Please call `--bisect-state` with at least one argument"));
>
> state = argv[0];
> if (check_and_set_terms(terms, state) ||
> !one_of(state, terms->term_good, terms->term_bad, "skip", NULL))
> return BISECT_FAILED;
>
> argv++;
> argc--;
> if (argc > 1 && !strcmp(state, terms->term_bad))
> return error(_("'git bisect %s' can take only one argument."), terms->term_bad);
>
> if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||
> get_oid_hex(buf.buf, &expected) < 0)
> verify_expected = 0; /* Ignore invalid file contents */
>
>
> for (i = 0; i < argc + !argc; i++) {
> if (argc) {
> if (get_oid(argv[i], &oid)) {
> error(_("Bad rev input: %s"), *argv);
> return BISECT_FAILED;
> }
> } else {
> enum get_oid_result res = get_oid("BISECT_HEAD", &oid);
>
> if (res == MISSING_OBJECT)
> res = get_oid("HEAD", &oid);
> if (res) {
> error(_("Bad bisect_head rev input"));
> return BISECT_FAILED;
> }
> }
>
> if (bisect_write(state, oid_to_hex(&oid), terms, 0))
> return BISECT_FAILED;
>
> if (verify_expected && !oideq(&oid, &expected)) {
> unlink_or_warn(git_path_bisect_ancestors_ok());
> unlink_or_warn(git_path_bisect_expected_rev());
> verify_expected = 0;
> }
> }
>
> return bisect_auto_next(terms, NULL);
> }
>
> There, not bad, is it?
>
After implementing this solution some tests failed. After debugging
them, I found that with Pranit's solution, that arguments were parsed
into an OID array, if bisect received some junk rev the function
returned and bisect_write() was not executed.
With the new solution, if junk rev is received after a valid rev,
bisect_write() was executed for the valid and the function returned with
the junk rev.
So, there is garbage in the file and when for example bisect-porcelain
test number 5 - 'bisect fails if given any junk instead of revs'
executes 'test -z' fails.
Should I keep the original patch and add a comment in the code that
explains why we use an oid array?.
(I also have implemented an alternative solution that when some junk
rev is found, I delete all refs written, but maybe is too complicated
or not totally correct:
https://gitlab.com/mirucam/git/-/commit/93f669877b87d09a30a5d07f0967667b22026511
)
> > +
> > int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> > {
> > enum {
> > @@ -847,7 +907,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> > BISECT_START,
> > BISECT_NEXT,
> > BISECT_AUTO_NEXT,
> > - BISECT_AUTOSTART
> > + BISECT_AUTOSTART,
> > + BISECT_STATE
> > } cmdmode = 0;
> > int no_checkout = 0, res = 0, nolog = 0;
> > struct option options[] = {
> > @@ -873,6 +934,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> > N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
> > OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
> > N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
> > + OPT_CMDMODE(0, "bisect-state", &cmdmode,
> > + N_("mark the state of ref (or refs)"), BISECT_STATE),
> > OPT_BOOL(0, "no-checkout", &no_checkout,
> > N_("update BISECT_HEAD instead of checking out the current commit")),
> > OPT_BOOL(0, "no-log", &nolog,
> > @@ -945,6 +1008,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> > set_terms(&terms, "bad", "good");
> > res = bisect_autostart(&terms);
> > break;
> > + case BISECT_STATE:
> > + set_terms(&terms, "bad", "good");
> > + get_terms(&terms);
> > + res = bisect_state(&terms, argv, argc);
> > + break;
> > default:
> > BUG("unknown subcommand %d", (int)cmdmode);
> > }
> > diff --git a/git-bisect.sh b/git-bisect.sh
> > index 049ffacdff..2da0810b1a 100755
> > --- a/git-bisect.sh
> > +++ b/git-bisect.sh
> > @@ -39,16 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
> > TERM_BAD=bad
> > TERM_GOOD=good
> >
> > -bisect_head()
> > -{
> > - if test -f "$GIT_DIR/BISECT_HEAD"
> > - then
> > - echo BISECT_HEAD
> > - else
> > - echo HEAD
> > - fi
> > -}
> > -
> > bisect_skip() {
> > all=''
> > for arg in "$@"
> > @@ -61,43 +51,7 @@ bisect_skip() {
> > esac
> > all="$all $revs"
> > done
> > - eval bisect_state 'skip' $all
> > -}
> > -
> > -bisect_state() {
> > - git bisect--helper --bisect-autostart
> > - state=$1
> > - git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
> > - get_terms
> > - case "$#,$state" in
> > - 0,*)
> > - die "Please call 'bisect_state' with at least one argument." ;;
> > - 1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip)
> > - bisected_head=$(bisect_head)
> > - rev=$(git rev-parse --verify "$bisected_head") ||
> > - die "$(eval_gettext "Bad rev input: \$bisected_head")"
> > - git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
> > - git bisect--helper --check-expected-revs "$rev" ;;
> > - 2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
> > - shift
> > - hash_list=''
> > - for rev in "$@"
> > - do
> > - sha=$(git rev-parse --verify "$rev^{commit}") ||
> > - die "$(eval_gettext "Bad rev input: \$rev")"
> > - hash_list="$hash_list $sha"
> > - done
> > - for rev in $hash_list
> > - do
> > - git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
> > - done
> > - git bisect--helper --check-expected-revs $hash_list ;;
> > - *,"$TERM_BAD")
> > - die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;;
> > - *)
> > - usage ;;
> > - esac
> > - git bisect--helper --bisect-auto-next
> > + eval git bisect--helper --bisect-state 'skip' $all
> > }
> >
> > bisect_visualize() {
> > @@ -185,8 +139,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> > state="$TERM_GOOD"
> > fi
> >
> > - # We have to use a subshell because "bisect_state" can exit.
> > - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
> > + git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN"
> > res=$?
> >
> > cat "$GIT_DIR/BISECT_RUN"
> > @@ -201,7 +154,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> > if [ $res -ne 0 ]
> > then
> > eval_gettextln "bisect run failed:
> > -'bisect_state \$state' exited with error code \$res" >&2
> > +'git bisect--helper --bisect-state \$state' exited with error code \$res" >&2
>
> This is not your fault, of course, but it does make me shudder to see such
> an obvious implementation detail in a user-facing error message.
>
> Maybe something to fix up in a follow-up?
>
> Ciao,
> Dscho
>
> > exit $res
> > fi
> >
> > @@ -242,7 +195,7 @@ case "$#" in
> > start)
> > git bisect--helper --bisect-start "$@" ;;
> > bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
> > - bisect_state "$cmd" "$@" ;;
> > + git bisect--helper --bisect-state "$cmd" "$@" ;;
> > skip)
> > bisect_skip "$@" ;;
> > next)
> > --
> > 2.25.0
> >
> >
Best,
Miriam.
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2020-06-22 12:18 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-23 7:06 [PATCH v3 00/12] Finish converting git bisect to C part 2 Miriam Rubio
2020-04-23 7:06 ` [PATCH v3 01/12] bisect--helper: fix `cmd_*()` function switch default return Miriam Rubio
2020-05-22 13:14 ` Johannes Schindelin
2020-04-23 7:06 ` [PATCH v3 02/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return Miriam Rubio
2020-05-22 13:16 ` Johannes Schindelin
2020-04-23 7:06 ` [PATCH v3 03/12] bisect--helper: introduce new `write_in_file()` function Miriam Rubio
2020-05-22 13:25 ` Johannes Schindelin
2020-05-23 1:53 ` Đoàn Trần Công Danh
2020-04-23 7:06 ` [PATCH v3 04/12] bisect--helper: reimplement `bisect_autostart` shell function in C Miriam Rubio
2020-05-22 19:27 ` Johannes Schindelin
2020-05-22 20:50 ` Johannes Schindelin
2020-04-23 7:06 ` [PATCH v3 05/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions " Miriam Rubio
2020-05-22 20:47 ` Johannes Schindelin
2020-04-23 7:06 ` [PATCH v3 06/12] bisect--helper: finish porting `bisect_start()` to C Miriam Rubio
2020-05-22 21:08 ` Johannes Schindelin
2020-04-23 7:06 ` [PATCH v3 07/12] bisect--helper: retire `--bisect-clean-state` subcommand Miriam Rubio
2020-04-23 7:07 ` [PATCH v3 08/12] bisect--helper: retire `--next-all` subcommand Miriam Rubio
2020-04-23 7:07 ` [PATCH v3 09/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C Miriam Rubio
2020-05-22 22:06 ` Johannes Schindelin
2020-06-20 8:04 ` Miriam R.
2020-06-19 13:57 ` Johannes Schindelin
2020-04-23 7:07 ` [PATCH v3 10/12] bisect--helper: retire `--check-expected-revs` subcommand Miriam Rubio
2020-04-23 7:07 ` [PATCH v3 11/12] bisect--helper: retire `--write-terms` subcommand Miriam Rubio
2020-04-23 7:07 ` [PATCH v3 12/12] bisect--helper: retire `--bisect-autostart` subcommand Miriam Rubio
2020-04-23 20:01 ` [PATCH v3 00/12] Finish converting git bisect to C part 2 Junio C Hamano
2020-04-25 10:57 ` Miriam R.
2020-05-22 22:09 ` Johannes Schindelin
2020-05-24 21:19 ` Miriam R.
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.