* [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1
@ 2020-01-20 14:37 Miriam Rubio
2020-01-20 14:37 ` [PATCH 01/29] bisect--helper: convert `vocab_*` char pointers to char arrays Miriam Rubio
` (29 more replies)
0 siblings, 30 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 UTC (permalink / raw)
To: git; +Cc: Miriam Rubio
--- Changes since Tanushree’s pr117 sent patch series:
https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) ---
General changes
---------------
* Rebase on master branch.
* Improve commit messages.
* Amend patch series titles.
* Reorder commits: first clean-up/preparatory commits, squash or split
commits.
Specific changes
----------------
[1/29] bisect--helper: convert `vocab_*` char pointers to char arrays
* New patch to convert `vocab_bad` and `vocab_good` char pointers
to char arrays
--
[2/29] bisect--helper: change `retval` to `res`
* Replace one last variable `retval` to `res`.
--
[3/29] bisect: use the standard 'if (!var)' way to check for 0
* New patch to use '!var' and make 'bisect.c' more consistent with the
rest of the code
--
[4/29] run-command: make `exists_in_PATH()` non-static
* Add comment before function declaration.
* Move function declaration in `run-command.h`.
--
[6/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*`
and its dependents
* Fix `mark_edges_uninteresting()` and `show_diff_tree()` calls after
rebase on master.
--
[7/29] bisect: libify `bisect_checkout`
* Fix `memcpy()` call after rebase on master.
* Introduce `res` variable to return `bisect_checkout()` output.
* Fix `get_commit_reference()` declaration after rebase on master.
--
[8/29] bisect: libify `check_merge_bases` and its dependents
State: Previously sent
* Fix `check_ancestors()` declaration after rebase on master.
* Fix `get_bad_and_good_commits()` call after rebase on master.
--
[9/29] bisect: libify `check_good_are_ancestors_of_bad` and its
dependents
State: Previously sent
* Fix `check_good_are_ancestors_of_bad()` declaration after rebase on
master.
* Fix `check_good_are_ancestors_of_bad()`, `bisect_next_all()`
and `bisect_rev_setup()` calls after rebase on master.
--
[11/29] bisect: libify `bisect_next_all`
State: Previously sent
* Fix `show_diff_tree()` call after rebase on master.
--
[12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next`
shell functions in C
* Fix `repo_init_revisions()` and `bisect_next_all()` calls after rebase
on master.
* Remove `goto` statement in `bisect_skipped_commits()`
--
[13/29] bisect--helper: finish porting `bisect_start()` to C
* Change `return` statement instead of `die` in error handling.
* Remove `goto` statements in `bisect_skipped_commits()`.
--
[21/29] bisect--helper: reimplement `bisect_replay` shell function in C
* Add blank line in `get_next_word()`.
* Remove `goto` statements in `bisect_replay()`.
--
[23/29] bisect--helper: use `res` instead of return in BISECT_RESET case
option
* New patch to split previous commit in two.
--
[26/29] bisect--helper: reimplement `bisect_skip` shell function in C
State: Previously sent
* Add blank line.
--
[28/29] bisect--helper: reimplement `bisect_visualize()`shell function
in C
New patch:
* Reimplement the `bisect_visualize()` shell function in C.
* Add `--bisect-visualize` subcommand.
* Fix long code line.
--
[29/29] bisect--helper: reimplement `bisect_run` shell function in C
New patch:
* Reimplement the `bisect_run()` shell function in C.
* Add `--bisect-run` subcommand.
* Remove blank line.
--
Miriam Rubio (2):
bisect--helper: convert `vocab_*` char pointers to char arrays
bisect: use the standard 'if (!var)' way to check for 0
Pranit Bauva (24):
run-command: make `exists_in_PATH()` non-static
bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and
its dependents
bisect: libify `bisect_checkout`
bisect: libify `check_merge_bases` and its dependents
bisect: libify `check_good_are_ancestors_of_bad` and its dependents
bisect: libify `handle_bad_merge_base` and its dependents
bisect: libify `bisect_next_all`
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_autostart` shell function in C
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: reimplement `bisect_log` shell function in C
bisect--helper: reimplement `bisect_replay` shell function in C
bisect--helper: retire `--bisect-write` subcommand
bisect--helper: use `res` instead of return in BISECT_RESET case
option
bisect--helper: retire `--bisect-autostart` subcommand
bisect--helper: retire `--bisect-auto-next` subcommand
bisect--helper: reimplement `bisect_skip` shell function in C
bisect--helper: retire `--check-and-set-terms` subcommand
bisect--helper: reimplement `bisect_visualize()`shell function in C
Tanushree Tumane (3):
bisect--helper: change `retval` to `res`
bisect--helper: introduce new `decide_next()` function
bisect--helper: reimplement `bisect_run` shell function in C
bisect.c | 146 +++++---
builtin/bisect--helper.c | 776 +++++++++++++++++++++++++++++++++------
git-bisect.sh | 279 +-------------
run-command.c | 2 +-
run-command.h | 11 +
5 files changed, 793 insertions(+), 421 deletions(-)
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 01/29] bisect--helper: convert `vocab_*` char pointers to char arrays
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 02/29] bisect--helper: change `retval` to `res` Miriam Rubio
` (28 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 UTC (permalink / raw)
To: git; +Cc: Miriam Rubio, Christian Couder
Instead of using a pointer that points at a constant string,
just give name directly to the constant string; this way, we
do not have to allocate a pointer variable in addition to
the string we want to use.
Let's convert `vocab_bad` and `vocab_good` char pointers to char arrays.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 1718df7f09..36c09b7238 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -52,8 +52,8 @@ static void set_terms(struct bisect_terms *terms, const char *bad,
terms->term_bad = xstrdup(bad);
}
-static const char *vocab_bad = "bad|new";
-static const char *vocab_good = "good|old";
+static const char vocab_bad[] = "bad|new";
+static const char vocab_good[] = "good|old";
/*
* Check whether the string `term` belongs to the set of strings
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 02/29] bisect--helper: change `retval` to `res`
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
2020-01-20 14:37 ` [PATCH 01/29] bisect--helper: convert `vocab_*` char pointers to char arrays Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 03/29] bisect: use the standard 'if (!var)' way to check for 0 Miriam Rubio
` (27 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 UTC (permalink / raw)
To: git; +Cc: Tanushree Tumane, Christian Couder, Miriam Rubio
From: Tanushree Tumane <tanushreetumane@gmail.com>
Let's rename variable retval to res, so that variable names
in bisect--helper.c are more consistent.
After this change, there are 110 occurrences of res in the file
and zero of retval, while there were 26 instances of retval before.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 52 ++++++++++++++++++++--------------------
1 file changed, 26 insertions(+), 26 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 36c09b7238..21de5c096c 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -206,31 +206,31 @@ static int bisect_write(const char *state, const char *rev,
struct object_id oid;
struct commit *commit;
FILE *fp = NULL;
- int retval = 0;
+ int res = 0;
if (!strcmp(state, terms->term_bad)) {
strbuf_addf(&tag, "refs/bisect/%s", state);
} else if (one_of(state, terms->term_good, "skip", NULL)) {
strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
} else {
- retval = error(_("Bad bisect_write argument: %s"), state);
+ res = error(_("Bad bisect_write argument: %s"), state);
goto finish;
}
if (get_oid(rev, &oid)) {
- retval = error(_("couldn't get the oid of the rev '%s'"), rev);
+ res = error(_("couldn't get the oid of the rev '%s'"), rev);
goto finish;
}
if (update_ref(NULL, tag.buf, &oid, NULL, 0,
UPDATE_REFS_MSG_ON_ERR)) {
- retval = -1;
+ res = -1;
goto finish;
}
fp = fopen(git_path_bisect_log(), "a");
if (!fp) {
- retval = error_errno(_("couldn't open the file '%s'"), git_path_bisect_log());
+ res = error_errno(_("couldn't open the file '%s'"), git_path_bisect_log());
goto finish;
}
@@ -244,7 +244,7 @@ static int bisect_write(const char *state, const char *rev,
if (fp)
fclose(fp);
strbuf_release(&tag);
- return retval;
+ return res;
}
static int check_and_set_terms(struct bisect_terms *terms, const char *cmd)
@@ -294,7 +294,7 @@ static const char need_bisect_start_warning[] =
static int bisect_next_check(const struct bisect_terms *terms,
const char *current_term)
{
- int missing_good = 1, missing_bad = 1, retval = 0;
+ int missing_good = 1, missing_bad = 1, res = 0;
const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
const char *good_glob = xstrfmt("%s-*", terms->term_good);
@@ -308,7 +308,7 @@ static int bisect_next_check(const struct bisect_terms *terms,
goto finish;
if (!current_term) {
- retval = -1;
+ res = -1;
goto finish;
}
@@ -329,21 +329,21 @@ static int bisect_next_check(const struct bisect_terms *terms,
*/
yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
if (starts_with(yesno, "N") || starts_with(yesno, "n"))
- retval = -1;
+ res = -1;
goto finish;
}
if (!is_empty_or_missing_file(git_path_bisect_start())) {
- retval = error(_(need_bad_and_good_revision_warning),
+ res = error(_(need_bad_and_good_revision_warning),
vocab_bad, vocab_good, vocab_bad, vocab_good);
} else {
- retval = error(_(need_bisect_start_warning),
+ res = error(_(need_bisect_start_warning),
vocab_good, vocab_bad, vocab_good, vocab_bad);
}
finish:
free((void *) good_glob);
free((void *) bad_ref);
- return retval;
+ return res;
}
static int get_terms(struct bisect_terms *terms)
@@ -397,7 +397,7 @@ static int bisect_terms(struct bisect_terms *terms, const char *option)
static int bisect_append_log_quoted(const char **argv)
{
- int retval = 0;
+ int res = 0;
FILE *fp = fopen(git_path_bisect_log(), "a");
struct strbuf orig_args = STRBUF_INIT;
@@ -405,25 +405,25 @@ static int bisect_append_log_quoted(const char **argv)
return -1;
if (fprintf(fp, "git bisect start") < 1) {
- retval = -1;
+ res = -1;
goto finish;
}
sq_quote_argv(&orig_args, argv);
if (fprintf(fp, "%s\n", orig_args.buf) < 1)
- retval = -1;
+ res = -1;
finish:
fclose(fp);
strbuf_release(&orig_args);
- return retval;
+ return res;
}
static int 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, retval = 0;
+ int flags, pathspec_pos, res = 0;
struct string_list revs = STRING_LIST_INIT_DUP;
struct string_list states = STRING_LIST_INIT_DUP;
struct strbuf start_head = STRBUF_INIT;
@@ -524,7 +524,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
argv_array_pushl(&argv, "checkout", start_head.buf,
"--", NULL);
if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
- retval = error(_("checking out '%s' failed."
+ res = error(_("checking out '%s' failed."
" Try 'git bisect start "
"<valid-branch>'."),
start_head.buf);
@@ -572,12 +572,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
if (no_checkout) {
if (get_oid(start_head.buf, &oid) < 0) {
- retval = error(_("invalid ref: '%s'"), start_head.buf);
+ res = error(_("invalid ref: '%s'"), start_head.buf);
goto finish;
}
if (update_ref(NULL, "BISECT_HEAD", &oid, NULL, 0,
UPDATE_REFS_MSG_ON_ERR)) {
- retval = -1;
+ res = -1;
goto finish;
}
}
@@ -589,26 +589,26 @@ 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)) {
- retval = -1;
+ res = -1;
goto finish;
}
if (must_write_terms && write_terms(terms->term_bad,
terms->term_good)) {
- retval = -1;
+ res = -1;
goto finish;
}
- retval = bisect_append_log_quoted(argv);
- if (retval)
- retval = -1;
+ res = bisect_append_log_quoted(argv);
+ if (res)
+ res = -1;
finish:
string_list_clear(&revs, 0);
string_list_clear(&states, 0);
strbuf_release(&start_head);
strbuf_release(&bisect_names);
- return retval;
+ return res;
}
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 03/29] bisect: use the standard 'if (!var)' way to check for 0
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
2020-01-20 14:37 ` [PATCH 01/29] bisect--helper: convert `vocab_*` char pointers to char arrays Miriam Rubio
2020-01-20 14:37 ` [PATCH 02/29] bisect--helper: change `retval` to `res` Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 04/29] run-command: make `exists_in_PATH()` non-static Miriam Rubio
` (26 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 UTC (permalink / raw)
To: git; +Cc: Miriam Rubio, Christian Couder
Instead of using 'var == 0' in an if condition, let's use '!var' and
make 'bisect.c' more consistent with the rest of the code.
Mentored-by: Christian Couder <chriscool@tuxfamil.org>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
bisect.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/bisect.c b/bisect.c
index e81c91d02c..83cb5b3a98 100644
--- a/bisect.c
+++ b/bisect.c
@@ -572,7 +572,7 @@ static int sqrti(int val)
{
float d, x = val;
- if (val == 0)
+ if (!val)
return 0;
do {
@@ -869,7 +869,7 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
goto done;
/* Bisecting with no good rev is ok. */
- if (good_revs.nr == 0)
+ if (!good_revs.nr)
goto done;
/* Check if all good revs are ancestor of the bad rev. */
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 04/29] run-command: make `exists_in_PATH()` non-static
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (2 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 03/29] bisect: use the standard 'if (!var)' way to check for 0 Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 05/29] bisect--helper: introduce new `decide_next()` function Miriam Rubio
` (25 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 UTC (permalink / raw)
To: git; +Cc: Pranit Bauva, Tanushree Tumane, Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
Removes the `static` keyword from `exists_in_PATH()` function
and declares the function in `run-command.h` file.
The function will be used in bisect_visualize() in a later
commit.
Mentored by: Christian Couder <chriscool@tuxfamily.org>
Mentored by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
run-command.c | 2 +-
run-command.h | 11 +++++++++++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/run-command.c b/run-command.c
index 9942f120a9..096faea5b9 100644
--- a/run-command.c
+++ b/run-command.c
@@ -210,7 +210,7 @@ static char *locate_in_PATH(const char *file)
return NULL;
}
-static int exists_in_PATH(const char *file)
+int exists_in_PATH(const char *file)
{
char *r = locate_in_PATH(file);
free(r);
diff --git a/run-command.h b/run-command.h
index 592d9dc035..7c8e206d97 100644
--- a/run-command.h
+++ b/run-command.h
@@ -172,6 +172,17 @@ void child_process_clear(struct child_process *);
int is_executable(const char *name);
+/**
+ * Returns if a $PATH given by parameter is found or not (it is NULL). This
+ * function uses locate_in_PATH() function that emulates the path search that
+ * execvp would perform. Memory used to store the resultant path is freed by
+ * the function.
+ *
+ * The caller should ensure that $PATH contains no directory
+ * separators.
+ */
+int exists_in_PATH(const char *);
+
/**
* Start a sub-process. Takes a pointer to a `struct child_process`
* that specifies the details and returns pipe FDs (if requested).
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 05/29] bisect--helper: introduce new `decide_next()` function
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (3 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 04/29] run-command: make `exists_in_PATH()` non-static Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 06/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents Miriam Rubio
` (24 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 UTC (permalink / raw)
To: git; +Cc: Tanushree Tumane, Christian Couder, Miriam Rubio
From: Tanushree Tumane <tanushreetumane@gmail.com>
Let's refactor code from bisect_next_check() into a new
decide_next() helper function.
This removes some goto statements and makes the code simpler,
clearer and easier to understand.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 62 +++++++++++++++++++++-------------------
1 file changed, 32 insertions(+), 30 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 21de5c096c..826fcba2ed 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -291,26 +291,14 @@ static const char need_bisect_start_warning[] =
"You then need to give me at least one %s and %s revision.\n"
"You can use \"git bisect %s\" and \"git bisect %s\" for that.");
-static int bisect_next_check(const struct bisect_terms *terms,
- const char *current_term)
+static int decide_next(const struct bisect_terms *terms,
+ const char *current_term, int missing_good,
+ int missing_bad)
{
- int missing_good = 1, missing_bad = 1, res = 0;
- const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
- const char *good_glob = xstrfmt("%s-*", terms->term_good);
-
- if (ref_exists(bad_ref))
- missing_bad = 0;
-
- for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
- (void *) &missing_good);
-
if (!missing_good && !missing_bad)
- goto finish;
-
- if (!current_term) {
- res = -1;
- goto finish;
- }
+ return 0;
+ if (!current_term)
+ return -1;
if (missing_good && !missing_bad &&
!strcmp(current_term, terms->term_good)) {
@@ -321,7 +309,7 @@ static int bisect_next_check(const struct bisect_terms *terms,
*/
warning(_("bisecting only with a %s commit"), terms->term_bad);
if (!isatty(0))
- goto finish;
+ return 0;
/*
* TRANSLATORS: Make sure to include [Y] and [n] in your
* translation. The program will only accept English input
@@ -329,21 +317,35 @@ static int bisect_next_check(const struct bisect_terms *terms,
*/
yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
if (starts_with(yesno, "N") || starts_with(yesno, "n"))
- res = -1;
- goto finish;
- }
- if (!is_empty_or_missing_file(git_path_bisect_start())) {
- res = error(_(need_bad_and_good_revision_warning),
- vocab_bad, vocab_good, vocab_bad, vocab_good);
- } else {
- res = error(_(need_bisect_start_warning),
- vocab_good, vocab_bad, vocab_good, vocab_bad);
+ return -1;
+ return 0;
}
-finish:
+ if (!is_empty_or_missing_file(git_path_bisect_start()))
+ return error(_(need_bad_and_good_revision_warning),
+ vocab_bad, vocab_good, vocab_bad, vocab_good);
+ else
+ return error(_(need_bisect_start_warning),
+ vocab_good, vocab_bad, vocab_good, vocab_bad);
+}
+
+static int bisect_next_check(const struct bisect_terms *terms,
+ const char *current_term)
+{
+ int missing_good = 1, missing_bad = 1;
+ const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
+ const char *good_glob = xstrfmt("%s-*", terms->term_good);
+
+ if (ref_exists(bad_ref))
+ missing_bad = 0;
+
+ for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
+ (void *) &missing_good);
+
free((void *) good_glob);
free((void *) bad_ref);
- return res;
+
+ return decide_next(terms, current_term, missing_good, missing_bad);
}
static int get_terms(struct bisect_terms *terms)
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 06/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (4 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 05/29] bisect--helper: introduce new `decide_next()` function Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 21:57 ` Johannes Schindelin
2020-01-20 14:37 ` [PATCH 07/29] bisect: libify `bisect_checkout` Miriam Rubio
` (23 subsequent siblings)
29 siblings, 1 reply; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Christian Couder, Johannes Schindelin,
Tanushree Tumane, Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
Since we want to get rid of git-bisect.sh it would be necessary to
convert those exit() calls to return statements so that errors can be
reported.
Emulate try catch in C by converting `exit(<positive-value>)` to
`return <negative-value>`. Follow POSIX conventions to return
<negative-value> to indicate error.
Modify `cmd_bisect_helper()` to handle these negative returns.
Turn `exit()` to `return` calls in `exit_if_skipped_commits()` and rename
the method to `error_if_skipped_commits()`.
Handle this return in dependant function `bisect_next_all()`.
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 | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/bisect.c b/bisect.c
index 83cb5b3a98..2ac0463327 100644
--- a/bisect.c
+++ b/bisect.c
@@ -661,11 +661,11 @@ static void bisect_common(struct rev_info *revs)
mark_edges_uninteresting(revs, NULL, 0);
}
-static void exit_if_skipped_commits(struct commit_list *tried,
+static int error_if_skipped_commits(struct commit_list *tried,
const struct object_id *bad)
{
if (!tried)
- return;
+ return 0;
printf("There are only 'skip'ped commits left to test.\n"
"The first %s commit could be any of:\n", term_bad);
@@ -676,7 +676,13 @@ static void exit_if_skipped_commits(struct commit_list *tried,
if (bad)
printf("%s\n", oid_to_hex(bad));
printf(_("We cannot bisect more!\n"));
- exit(2);
+
+ /*
+ * We don't want to clean the bisection state
+ * as we need to get back to where we started
+ * by using `git bisect reset`.
+ */
+ return -2;
}
static int is_expected_rev(const struct object_id *oid)
@@ -949,7 +955,7 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
{
struct rev_info revs;
struct commit_list *tried;
- int reaches = 0, all = 0, nr, steps;
+ int reaches = 0, all = 0, nr, steps, res;
struct object_id *bisect_rev;
char *steps_msg;
@@ -972,8 +978,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
* We should exit here only if the "bad"
* commit is also a "skip" commit.
*/
- exit_if_skipped_commits(tried, NULL);
-
+ res = error_if_skipped_commits(tried, NULL);
+ if (res)
+ exit(-res);
printf(_("%s was both %s and %s\n"),
oid_to_hex(current_bad_oid),
term_good,
@@ -990,7 +997,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
bisect_rev = &revs.commits->item->object.oid;
if (oideq(bisect_rev, current_bad_oid)) {
- exit_if_skipped_commits(tried, current_bad_oid);
+ res = error_if_skipped_commits(tried, current_bad_oid);
+ if (res)
+ exit(-res);
printf("%s is the first %s commit\n", oid_to_hex(bisect_rev),
term_bad);
show_diff_tree(r, prefix, revs.commits->item);
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 07/29] bisect: libify `bisect_checkout`
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (5 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 06/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 08/29] bisect: libify `check_merge_bases` and its dependents Miriam Rubio
` (22 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Christian Couder, Johannes Schindelin,
Tanushree Tumane, Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
Since we want to get rid of git-bisect.sh it would be necessary to
convert those exit() calls to return statements so that errors can be
reported.
Emulate try catch in C by converting `exit(<positive-value>)` to
`return <negative-value>`. Follow POSIX conventions to return
<negative-value> to indicate error.
Turn `exit()` to `return` calls in `bisect_checkout()`.
Changes related to return values have no bad side effects on the
code that calls `bisect_checkout()`.
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 | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/bisect.c b/bisect.c
index 2ac0463327..385afaf875 100644
--- a/bisect.c
+++ b/bisect.c
@@ -713,6 +713,7 @@ static int bisect_checkout(const struct object_id *bisect_rev, int no_checkout)
{
char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
+ int res = 0;
memcpy(bisect_rev_hex, oid_to_hex(bisect_rev), the_hash_algo->hexsz + 1);
update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
@@ -721,14 +722,14 @@ static int bisect_checkout(const struct object_id *bisect_rev, int no_checkout)
update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
UPDATE_REFS_DIE_ON_ERR);
} else {
- int res;
res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
if (res)
- exit(res);
+ return res > 0 ? -res : res;
}
argv_show_branch[1] = bisect_rev_hex;
- return run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
+ res = run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
+ return res > 0 ? -res : res;
}
static struct commit *get_commit_reference(struct repository *r,
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 08/29] bisect: libify `check_merge_bases` and its dependents
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (6 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 07/29] bisect: libify `bisect_checkout` Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 22:09 ` Johannes Schindelin
2020-01-20 14:37 ` [PATCH 09/29] bisect: libify `check_good_are_ancestors_of_bad` " Miriam Rubio
` (21 subsequent siblings)
29 siblings, 1 reply; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 UTC (permalink / raw)
To: git; +Cc: Pranit Bauva, Christian Couder, Tanushree Tumane, Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
Since we want to get rid of git-bisect.sh it would be necessary to
convert those exit() calls to return statements so that errors can be
reported.
Emulate try catch in C by converting `exit(<positive-value>)` to
`return <negative-value>`. Follow POSIX conventions to return
<negative-value> to indicate error.
Turn `exit()` to `return` calls in `check_merge_bases()`.
In `check_merge_bases()` there is an early success special case,
so we have introduced special error code `-11` which indicates early
success. This `-11` is converted back to `0` in `check_good_are_ancestors_of_bad()`.
Handle the return value in dependent function `check_good_are_ancestors_of_bad()`.
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 | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/bisect.c b/bisect.c
index 385afaf875..367258b0dd 100644
--- a/bisect.c
+++ b/bisect.c
@@ -806,13 +806,16 @@ static void handle_skipped_merge_base(const struct object_id *mb)
* "check_merge_bases" checks that merge bases are not "bad" (or "new").
*
* - If one is "bad" (or "new"), it means the user assumed something wrong
- * and we must exit with a non 0 error code.
+ * and we must return error with a non 0 error code.
* - If one is "good" (or "old"), that's good, we have nothing to do.
* - If one is "skipped", we can't know but we should warn.
* - If we don't know, we should check it out and ask the user to test.
+ * - If a merge base must be tested, on success return -11 a special condition
+ * for early success, this will be converted back to 0 in check_good_are_ancestors_of_bad().
*/
-static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
+static int check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
{
+ int res = 0;
struct commit_list *result;
result = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1);
@@ -827,11 +830,16 @@ static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
handle_skipped_merge_base(mb);
} else {
printf(_("Bisecting: a merge base must be tested\n"));
- exit(bisect_checkout(mb, no_checkout));
+ res = bisect_checkout(mb, no_checkout);
+ if (!res)
+ /* indicate early success */
+ res = -11;
+ break;
}
}
free_commit_list(result);
+ return res;
}
static int check_ancestors(struct repository *r, int rev_nr,
@@ -865,7 +873,7 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
{
char *filename = git_pathdup("BISECT_ANCESTORS_OK");
struct stat st;
- int fd, rev_nr;
+ int fd, rev_nr, res = 0;
struct commit **rev;
if (!current_bad_oid)
@@ -880,10 +888,13 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
goto done;
/* Check if all good revs are ancestor of the bad rev. */
+
rev = get_bad_and_good_commits(r, &rev_nr);
if (check_ancestors(r, rev_nr, rev, prefix))
- check_merge_bases(rev_nr, rev, no_checkout);
+ res = check_merge_bases(rev_nr, rev, no_checkout);
free(rev);
+ if(res)
+ exit(res == -11 ? 0 : -res);
/* Create file BISECT_ANCESTORS_OK. */
fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 09/29] bisect: libify `check_good_are_ancestors_of_bad` and its dependents
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (7 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 08/29] bisect: libify `check_merge_bases` and its dependents Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 22:20 ` Johannes Schindelin
2020-01-20 14:37 ` [PATCH 10/29] bisect: libify `handle_bad_merge_base` " Miriam Rubio
` (20 subsequent siblings)
29 siblings, 1 reply; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 UTC (permalink / raw)
To: git; +Cc: Pranit Bauva, Christian Couder, Tanushree Tumane, Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
Since we want to get rid of git-bisect.sh it would be necessary to
convert those exit() calls to return statements so that errors can be
reported.
Emulate try catch in C by converting `exit(<positive-value>)` to
`return <negative-value>`. Follow POSIX conventions to return
<negative-value> to indicate error.
Turn `exit()` to `return` calls in `check_good_are_ancestors_of_bad()`.
Code that turns -11(early success code) to 0 from
`check_good_are_ancestors_of_bad()` has been moved to
`cmd_bisect__helper()`.
Handle the return value in dependent function `bisect_next_all()`.
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 | 42 ++++++++++++++++++++++++++--------------
builtin/bisect--helper.c | 12 ++++++++++--
2 files changed, 37 insertions(+), 17 deletions(-)
diff --git a/bisect.c b/bisect.c
index 367258b0dd..2b80597a1d 100644
--- a/bisect.c
+++ b/bisect.c
@@ -865,9 +865,10 @@ static int check_ancestors(struct repository *r, int rev_nr,
*
* If that's not the case, we need to check the merge bases.
* If a merge base must be tested by the user, its source code will be
- * checked out to be tested by the user and we will exit.
+ * checked out to be tested by the user and we will return.
*/
-static void check_good_are_ancestors_of_bad(struct repository *r,
+
+static int check_good_are_ancestors_of_bad(struct repository *r,
const char *prefix,
int no_checkout)
{
@@ -876,8 +877,15 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
int fd, rev_nr, res = 0;
struct commit **rev;
- if (!current_bad_oid)
- die(_("a %s revision is needed"), term_bad);
+ /*
+ * We don't want to clean the bisection state
+ * as we need to get back to where we started
+ * by using `git bisect reset`.
+ */
+ if (!current_bad_oid) {
+ res = error(_("a %s revision is needed"), term_bad);
+ goto done;
+ }
/* Check if file BISECT_ANCESTORS_OK exists. */
if (!stat(filename, &st) && S_ISREG(st.st_mode))
@@ -893,18 +901,20 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
if (check_ancestors(r, rev_nr, rev, prefix))
res = check_merge_bases(rev_nr, rev, no_checkout);
free(rev);
- if(res)
- exit(res == -11 ? 0 : -res);
-
- /* Create file BISECT_ANCESTORS_OK. */
- fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
- if (fd < 0)
- warning_errno(_("could not create file '%s'"),
- filename);
- else
- close(fd);
+
+ if (!res)
+ {
+ /* Create file BISECT_ANCESTORS_OK. */
+ fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
+ if (fd < 0)
+ warning_errno(_("could not create file '%s'"),
+ filename);
+ else
+ close(fd);
+ }
done:
free(filename);
+ return res;
}
/*
@@ -975,7 +985,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
if (read_bisect_refs())
die(_("reading bisect refs failed"));
- check_good_are_ancestors_of_bad(r, prefix, no_checkout);
+ res = check_good_are_ancestors_of_bad(r, prefix, no_checkout);
+ if (res)
+ return res;
bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1);
revs.limited = 1;
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 826fcba2ed..5e0f759d50 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -666,7 +666,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
switch (cmdmode) {
case NEXT_ALL:
- return bisect_next_all(the_repository, prefix, no_checkout);
+ res = bisect_next_all(the_repository, prefix, no_checkout);
+ break;
case WRITE_TERMS:
if (argc != 2)
return error(_("--write-terms requires two arguments"));
@@ -713,5 +714,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
return error("BUG: unknown subcommand '%d'", cmdmode);
}
free_terms(&terms);
- return !!res;
+ /*
+ * Handle early success
+ * From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all
+ */
+ if (res == -11)
+ res = 0;
+
+ return res < 0 ? -res : res;
}
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 10/29] bisect: libify `handle_bad_merge_base` and its dependents
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (8 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 09/29] bisect: libify `check_good_are_ancestors_of_bad` " Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 22:23 ` Johannes Schindelin
2020-01-20 14:37 ` [PATCH 11/29] bisect: libify `bisect_next_all` Miriam Rubio
` (19 subsequent siblings)
29 siblings, 1 reply; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Christian Couder, Johannes Schindelin,
Tanushree Tumane, Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
Since we want to get rid of git-bisect.sh it would be necessary to
convert those exit() calls to return statements so that errors can be
reported.
Emulate try catch in C by converting `exit(<positive-value>)` to
`return <negative-value>`. Follow POSIX conventions to return
<negative-value> to indicate error.
Turn `exit()` to `return` calls in `handle_bad_merge_base()`.
Handle the return value in dependent function check_merge_bases().
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 | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/bisect.c b/bisect.c
index 2b80597a1d..acb5a13911 100644
--- a/bisect.c
+++ b/bisect.c
@@ -756,7 +756,7 @@ static struct commit **get_bad_and_good_commits(struct repository *r,
return rev;
}
-static void handle_bad_merge_base(void)
+static int handle_bad_merge_base(void)
{
if (is_expected_rev(current_bad_oid)) {
char *bad_hex = oid_to_hex(current_bad_oid);
@@ -777,14 +777,14 @@ static void handle_bad_merge_base(void)
"between %s and [%s].\n"),
bad_hex, term_bad, term_good, bad_hex, good_hex);
}
- exit(3);
+ return -3;
}
fprintf(stderr, _("Some %s revs are not ancestors of the %s rev.\n"
"git bisect cannot work properly in this case.\n"
"Maybe you mistook %s and %s revs?\n"),
term_good, term_bad, term_good, term_bad);
- exit(1);
+ return -1;
}
static void handle_skipped_merge_base(const struct object_id *mb)
@@ -823,7 +823,8 @@ static int check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
for (; result; result = result->next) {
const struct object_id *mb = &result->item->object.oid;
if (oideq(mb, current_bad_oid)) {
- handle_bad_merge_base();
+ res = handle_bad_merge_base();
+ break;
} else if (0 <= oid_array_lookup(&good_revs, mb)) {
continue;
} else if (0 <= oid_array_lookup(&skipped_revs, mb)) {
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 11/29] bisect: libify `bisect_next_all`
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (9 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 10/29] bisect: libify `handle_bad_merge_base` " Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 22:29 ` Johannes Schindelin
2020-01-20 14:37 ` [PATCH 12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C Miriam Rubio
` (18 subsequent siblings)
29 siblings, 1 reply; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Christian Couder, Johannes Schindelin,
Tanushree Tumane, Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
Since we want to get rid of git-bisect.sh it would be necessary to
convert those exit() calls to return statements so that errors can be
reported.
Emulate try catch in C by converting `exit(<positive-value>)` to
`return <negative-value>`. Follow POSIX conventions to return
<negative-value> to indicate error.
Turn `exit()` to `return` calls in `bisect_next_all()`.
All the functions calling `bisect_next_all()` are already able to
handle return values from it.
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 | 40 +++++++++++++++++++++++++++++-----------
1 file changed, 29 insertions(+), 11 deletions(-)
diff --git a/bisect.c b/bisect.c
index acb5a13911..33f2829c19 100644
--- a/bisect.c
+++ b/bisect.c
@@ -967,10 +967,10 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
}
/*
- * We use the convention that exiting with an exit code 10 means that
- * the bisection process finished successfully.
- * In this case the calling shell script should exit 0.
- *
+ * We use the convention that return -10 means the bisection process
+ * finished successfully.
+ * In this case the calling function or command should not turn a -10
+ * return code into an error or a non zero exit code.
* If no_checkout is non-zero, the bisection process does not
* checkout the trial commit but instead simply updates BISECT_HEAD.
*/
@@ -1000,23 +1000,35 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
if (!revs.commits) {
/*
- * We should exit here only if the "bad"
+ * We should return error here only if the "bad"
* commit is also a "skip" commit.
*/
res = error_if_skipped_commits(tried, NULL);
if (res)
- exit(-res);
+ return res;
printf(_("%s was both %s and %s\n"),
oid_to_hex(current_bad_oid),
term_good,
term_bad);
- exit(1);
+
+ /*
+ * We don't want to clean the bisection state
+ * as we need to get back to where we started
+ * by using `git bisect reset`.
+ */
+ return -1;
}
if (!all) {
fprintf(stderr, _("No testable commit found.\n"
"Maybe you started with bad path parameters?\n"));
- exit(4);
+
+ /*
+ * We don't want to clean the bisection state
+ * as we need to get back to where we started
+ * by using `git bisect reset`.
+ */
+ return -4;
}
bisect_rev = &revs.commits->item->object.oid;
@@ -1024,12 +1036,18 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
if (oideq(bisect_rev, current_bad_oid)) {
res = error_if_skipped_commits(tried, current_bad_oid);
if (res)
- exit(-res);
+ return res;
printf("%s is the first %s commit\n", oid_to_hex(bisect_rev),
term_bad);
+
show_diff_tree(r, prefix, revs.commits->item);
- /* This means the bisection process succeeded. */
- exit(10);
+ /*
+ * This means the bisection process succeeded.
+ * Using -10 so that the call chain can simply check
+ * for negative return values for early returns up
+ * until the cmd_bisect__helper() caller.
+ */
+ return -10;
}
nr = all - reaches - 1;
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (10 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 11/29] bisect: libify `bisect_next_all` Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-30 22:47 ` Johannes Schindelin
2020-01-20 14:37 ` [PATCH 13/29] bisect--helper: finish porting `bisect_start()` to C Miriam Rubio
` (17 subsequent siblings)
29 siblings, 1 reply; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 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 .
Using `--bisect-next` and `--bisect-auto-start` 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-start`
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 | 10 +++
builtin/bisect--helper.c | 174 ++++++++++++++++++++++++++++++++++++++-
git-bisect.sh | 47 ++---------
3 files changed, 188 insertions(+), 43 deletions(-)
diff --git a/bisect.c b/bisect.c
index 33f2829c19..1c13da8a28 100644
--- a/bisect.c
+++ b/bisect.c
@@ -635,6 +635,12 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,
struct argv_array rev_argv = ARGV_ARRAY_INIT;
int i;
+ /*
+ * Since the code is slowly being converted to C, there might be
+ * instances where the revisions were initialized before. Thus
+ * we first need to reset it.
+ */
+ reset_revision_walk();
repo_init_revisions(r, revs, prefix);
revs->abbrev = 0;
revs->commit_format = CMIT_FMT_UNSPECIFIED;
@@ -971,6 +977,10 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
* finished successfully.
* In this case the calling function or command should not turn a -10
* return code into an error or a non zero exit code.
+ * This returned -10 is checked in bisect_helper::bisect_next() and
+ * eventually transformed to 0 at the end of
+ * bisect_helper::cmd_bisect__helper().
+ *
* If no_checkout is non-zero, the bisection process does not
* checkout the trial commit but instead simply updates BISECT_HEAD.
*/
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 5e0f759d50..29bbc1573b 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"),
NULL
};
@@ -421,6 +424,157 @@ 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 string_list *good_refs = cb_data;
+ string_list_append(good_refs, oid_to_hex(oid));
+ return 0;
+}
+
+static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)
+{
+ struct string_list good_revs = STRING_LIST_INIT_DUP;
+ char *term_good = xstrfmt("%s-*", terms->term_good);
+
+ for_each_glob_ref_in(register_good_ref, term_good,
+ "refs/bisect/", &good_revs);
+
+ argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);
+ for (int i = 0; i < good_revs.nr; i++)
+ argv_array_push(rev_argv, good_revs.items[i].string);
+
+ string_list_clear(&good_revs, 0);
+ 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
+ * setups 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") < 1)
+ return -1;
+
+ 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);
+ }
+
+ 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)
+{
+ FILE *fp = NULL;
+ 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 = 0;
+
+ 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);
+
+ fp = fopen(git_path_bisect_log(), "a");
+ if (fp) {
+ if (fprintf(fp, "# first %s commit: [%s] %s\n",
+ terms->term_bad, oid_to_hex(&oid),
+ commit_name.buf) < 1)
+ res = -1;
+ fclose(fp);
+ } else {
+ res = error_errno(_("could not open '%s' for "
+ "appending"),
+ git_path_bisect_log());
+ }
+ strbuf_release(&commit_name);
+ free(bad_ref);
+ return res;
+}
+
+static int bisect_next(struct bisect_terms *terms, const char *prefix)
+{
+ int res, no_checkout;
+
+ if (bisect_next_check(terms, terms->term_good))
+ return -1;
+
+ no_checkout = !is_empty_or_missing_file(git_path_bisect_head());
+
+ /* Perform all bisection computation, display and checkout */
+ res = bisect_next_all(the_repository, prefix, no_checkout);
+
+ if (res == -10) {
+ res = bisect_successful(terms);
+ return res ? res : -11;
+ } else if (res == -2) {
+ res = bisect_skipped_commits(terms);
+ return res ? res : -2;
+ }
+ return res;
+}
+
+static int bisect_auto_next(struct bisect_terms *terms, const char *prefix)
+{
+ if (!bisect_next_check(terms, NULL))
+ return bisect_next(terms, prefix);
+
+ return 0;
+}
+
static int bisect_start(struct bisect_terms *terms, int no_checkout,
const char **argv, int argc)
{
@@ -625,7 +779,9 @@ 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_NEXT,
+ BISECT_AUTO_NEXT,
} cmdmode = 0;
int no_checkout = 0, res = 0, nolog = 0;
struct option options[] = {
@@ -649,6 +805,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_BOOL(0, "no-checkout", &no_checkout,
N_("update BISECT_HEAD instead of checking out the current commit")),
OPT_BOOL(0, "no-log", &nolog,
@@ -710,6 +870,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;
default:
return error("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index efee12b8b1..7531b74708 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -87,7 +87,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
}
@@ -140,45 +140,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
- 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() {
@@ -232,7 +194,7 @@ bisect_replay () {
die "$(gettext "?? what are you talking about?")" ;;
esac
done <"$file"
- bisect_auto_next
+ git bisect--helper --bisect-auto-next
}
bisect_run () {
@@ -329,7 +291,8 @@ case "$#" in
bisect_skip "$@" ;;
next)
# Not sure we want "next" at the UI level anymore.
- bisect_next "$@" ;;
+ get_terms
+ git bisect--helper --bisect-next "$@" || exit ;;
visualize|view)
bisect_visualize "$@" ;;
reset)
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 13/29] bisect--helper: finish porting `bisect_start()` to C
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (11 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 14/29] bisect--helper: retire `--bisect-clean-state` subcommand Miriam Rubio
` (16 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 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 | 38 +++++++++++++++++++++++++++++++++-----
git-bisect.sh | 29 +++--------------------------
2 files changed, 36 insertions(+), 31 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 29bbc1573b..e604334c91 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -632,9 +632,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 -1;
+ }
string_list_append(&revs, oid_to_hex(&oid));
free(commit_id);
@@ -715,9 +718,9 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
return -1;
/*
- * 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".
*/
@@ -764,6 +767,31 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
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 -11 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 = -11
+ */
+ if (res && res != -11)
+ bisect_clean_state();
return res;
}
diff --git a/git-bisect.sh b/git-bisect.sh
index 7531b74708..fec527e1ef 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -63,35 +63,13 @@ bisect_autostart() {
[Nn]*)
exit ;;
esac
- bisect_start
+ git bisect--helper --bisect-start
else
exit 1
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.
- #
- get_terms
- git bisect--helper --bisect-auto-next || exit
-
- trap '-' 0
-}
-
bisect_skip() {
all=''
for arg in "$@"
@@ -184,8 +162,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)
@@ -284,7 +261,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.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 14/29] bisect--helper: retire `--bisect-clean-state` subcommand
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (12 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 13/29] bisect--helper: finish porting `bisect_start()` to C Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 15/29] bisect--helper: retire `--next-all` subcommand Miriam Rubio
` (15 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 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 e604334c91..7190304a98 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>"),
@@ -800,7 +799,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,
@@ -817,8 +815,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,
@@ -860,10 +856,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.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 15/29] bisect--helper: retire `--next-all` subcommand
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (13 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 14/29] bisect--helper: retire `--bisect-clean-state` subcommand Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 16/29] bisect--helper: reimplement `bisect_autostart` shell function in C Miriam Rubio
` (14 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 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 7190304a98..6953c68f93 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>"),
@@ -797,8 +796,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
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,
@@ -811,8 +809,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,
@@ -849,9 +845,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.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 16/29] bisect--helper: reimplement `bisect_autostart` shell function in C
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (14 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 15/29] bisect--helper: retire `--next-all` subcommand Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 17/29] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions " Miriam Rubio
` (13 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 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 | 39 +++++++++++++++++++++++++++++++++++++++
git-bisect.sh | 23 +----------------------
2 files changed, 40 insertions(+), 22 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 6953c68f93..f03ed23431 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -30,6 +30,7 @@ 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"),
NULL
};
@@ -56,6 +57,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.
@@ -547,6 +550,7 @@ static int bisect_next(struct bisect_terms *terms, const char *prefix)
{
int res, no_checkout;
+ bisect_autostart(terms);
if (bisect_next_check(terms, terms->term_good))
return -1;
@@ -793,6 +797,32 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
return res;
}
+static int bisect_autostart(struct bisect_terms *terms)
+{
+ if (is_empty_or_missing_file(git_path_bisect_start())) {
+ const char *yesno;
+ const char *argv[] = {NULL};
+ 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, argv, 0);
+ }
+ return 0;
+}
+
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
@@ -806,6 +836,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
BISECT_START,
BISECT_NEXT,
BISECT_AUTO_NEXT,
+ BISECT_AUTOSTART,
} cmdmode = 0;
int no_checkout = 0, res = 0, nolog = 0;
struct option options[] = {
@@ -829,6 +860,8 @@ 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_BOOL(0, "no-checkout", &no_checkout,
N_("update BISECT_HEAD instead of checking out the current commit")),
OPT_BOOL(0, "no-log", &nolog,
@@ -895,6 +928,12 @@ 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;
default:
return error("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index fec527e1ef..97bb15b38f 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
- git bisect--helper --bisect-start
- else
- exit 1
- fi
- }
-}
-
bisect_skip() {
all=''
for arg in "$@"
@@ -86,7 +65,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
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 17/29] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (15 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 16/29] bisect--helper: reimplement `bisect_autostart` shell function in C Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 18/29] bisect--helper: retire `--check-expected-revs` subcommand Miriam Rubio
` (12 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 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 | 80 ++++++++++++++++++++++++++++++++++++++++
git-bisect.sh | 55 ++-------------------------
2 files changed, 84 insertions(+), 51 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index f03ed23431..fdcc5f47ec 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
};
@@ -823,6 +825,74 @@ static int bisect_autostart(struct bisect_terms *terms)
return 0;
}
+static char *bisect_head(void)
+{
+ if (is_empty_or_missing_file(git_path_bisect_head()))
+ return "HEAD";
+ else
+ return "BISECT_HEAD";
+}
+
+static int bisect_state(struct bisect_terms *terms, const char **argv,
+ int argc)
+{
+ const char *state = argv[0];
+
+ if (check_and_set_terms(terms, state))
+ return -1;
+
+ if (!argc)
+ return error(_("Please call `--bisect-state` with at least one argument"));
+
+ if (argc == 1 && one_of(state, terms->term_good,
+ terms->term_bad, "skip", NULL)) {
+ const char *bisected_head = xstrdup(bisect_head());
+ const char *hex[1];
+ struct object_id oid;
+
+ if (get_oid(bisected_head, &oid))
+ return error(_("Bad rev input: %s"), bisected_head);
+ if (bisect_write(state, oid_to_hex(&oid), terms, 0))
+ return -1;
+
+ *hex = xstrdup(oid_to_hex(&oid));
+ check_expected_revs(hex, 1);
+ return bisect_auto_next(terms, NULL);
+ }
+
+ if ((argc == 2 && !strcmp(state, terms->term_bad)) ||
+ one_of(state, terms->term_good, "skip", NULL)) {
+ int i;
+ struct string_list hex = STRING_LIST_INIT_DUP;
+
+ for (i = 1; i < argc; i++) {
+ struct object_id oid;
+
+ if (get_oid(argv[i], &oid)) {
+ string_list_clear(&hex, 0);
+ return error(_("Bad rev input: %s"), argv[i]);
+ }
+ string_list_append(&hex, oid_to_hex(&oid));
+ }
+ for (i = 0; i < hex.nr; i++) {
+ const char **hex_string = (const char **) &hex.items[i].string;
+ if (bisect_write(state, *hex_string, terms, 0)) {
+ string_list_clear(&hex, 0);
+ return -1;
+ }
+ check_expected_revs(hex_string, 1);
+ }
+ string_list_clear(&hex, 0);
+ return bisect_auto_next(terms, NULL);
+ }
+
+ if (!strcmp(state, terms->term_bad))
+ return error(_("'git bisect %s' can take only one argument."),
+ terms->term_bad);
+
+ return -1;
+}
+
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
@@ -837,6 +907,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
BISECT_NEXT,
BISECT_AUTO_NEXT,
BISECT_AUTOSTART,
+ BISECT_STATE
} cmdmode = 0;
int no_checkout = 0, res = 0, nolog = 0;
struct option options[] = {
@@ -862,6 +933,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,
@@ -934,6 +1007,13 @@ 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:
+ if (!argc)
+ return error(_("--bisect-state requires at least one revision"));
+ set_terms(&terms, "bad", "good");
+ get_terms(&terms);
+ res = bisect_state(&terms, argv, argc);
+ break;
default:
return error("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index 97bb15b38f..4a5afc7a93 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.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 18/29] bisect--helper: retire `--check-expected-revs` subcommand
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (16 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 17/29] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions " Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 19/29] bisect--helper: retire `--write-terms` subcommand Miriam Rubio
` (11 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 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 fdcc5f47ec..3868d34a29 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -897,7 +897,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,
@@ -913,8 +912,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,
@@ -955,9 +952,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.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 19/29] bisect--helper: retire `--write-terms` subcommand
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (17 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 18/29] bisect--helper: retire `--check-expected-revs` subcommand Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 20/29] bisect--helper: reimplement `bisect_log` shell function in C Miriam Rubio
` (10 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 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 3868d34a29..4ff8035cd8 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>"),
@@ -896,8 +895,7 @@ static int bisect_state(struct bisect_terms *terms, const char **argv,
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,
@@ -910,8 +908,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,
@@ -948,10 +944,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.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 20/29] bisect--helper: reimplement `bisect_log` shell function in C
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (18 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 19/29] bisect--helper: retire `--write-terms` subcommand Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 21/29] bisect--helper: reimplement `bisect_replay` " Miriam Rubio
` (9 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 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_log()` shell function in C and also add
`--bisect-log` subcommand to `git bisect--helper` to call it from
git-bisect.sh .
Using `--bisect-log` subcommand is a temporary measure to port shell
function to C so as to use the existing test suite.
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 | 22 +++++++++++++++++++++-
git-bisect.sh | 7 +------
2 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 4ff8035cd8..4795b7880c 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -892,6 +892,18 @@ static int bisect_state(struct bisect_terms *terms, const char **argv,
return -1;
}
+static int bisect_log(void)
+{
+ int fd, status;
+ fd = open(git_path_bisect_log(), O_RDONLY);
+ if (fd < 0)
+ return -1;
+
+ status = copy_fd(fd, STDOUT_FILENO);
+ close(fd);
+ return status ? -1 : 0;
+}
+
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
@@ -904,7 +916,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
BISECT_NEXT,
BISECT_AUTO_NEXT,
BISECT_AUTOSTART,
- BISECT_STATE
+ BISECT_STATE,
+ BISECT_LOG
} cmdmode = 0;
int no_checkout = 0, res = 0, nolog = 0;
struct option options[] = {
@@ -928,6 +941,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
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_CMDMODE(0, "bisect-log", &cmdmode,
+ N_("output the contents of BISECT_LOG"), BISECT_LOG),
OPT_BOOL(0, "no-checkout", &no_checkout,
N_("update BISECT_HEAD instead of checking out the current commit")),
OPT_BOOL(0, "no-log", &nolog,
@@ -1000,6 +1015,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
get_terms(&terms);
res = bisect_state(&terms, argv, argc);
break;
+ case BISECT_LOG:
+ if (argc > 1)
+ return error(_("--bisect-log requires 0 arguments"));
+ res = bisect_log();
+ break;
default:
return error("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index 4a5afc7a93..151358aeda 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -167,11 +167,6 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
done
}
-bisect_log () {
- test -s "$GIT_DIR/BISECT_LOG" || die "$(gettext "We are not bisecting.")"
- cat "$GIT_DIR/BISECT_LOG"
-}
-
get_terms () {
if test -s "$GIT_DIR/BISECT_TERMS"
then
@@ -209,7 +204,7 @@ case "$#" in
replay)
bisect_replay "$@" ;;
log)
- bisect_log ;;
+ git bisect--helper --bisect-log ;;
run)
bisect_run "$@" ;;
terms)
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 21/29] bisect--helper: reimplement `bisect_replay` shell function in C
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (19 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 20/29] bisect--helper: reimplement `bisect_log` shell function in C Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 22/29] bisect--helper: retire `--bisect-write` subcommand Miriam Rubio
` (8 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 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_replay` shell function in C and also add
`--bisect-replay` subcommand to `git bisect--helper` to call it from
git-bisect.sh
Using `--bisect-replay` subcommand is a temporary measure to port shell
function to C so as to use the existing test suite.
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 | 127 ++++++++++++++++++++++++++++++++++++++-
git-bisect.sh | 32 +---------
2 files changed, 127 insertions(+), 32 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 4795b7880c..4b4ce13b50 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -32,6 +32,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-autostart"),
N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
+ N_("git bisect--helper --bisect-replay <filename>"),
NULL
};
@@ -904,6 +905,121 @@ static int bisect_log(void)
return status ? -1 : 0;
}
+static int get_next_word(const char *line, int pos, struct strbuf *word)
+{
+ int i, len = strlen(line), begin = 0;
+
+ strbuf_reset(word);
+ for (i = pos; i < len; i++) {
+ if (line[i] == ' ' && begin)
+ return i + 1;
+
+ if (!begin)
+ begin = 1;
+ strbuf_addch(word, line[i]);
+ }
+
+ return i;
+}
+
+static int process_line(struct bisect_terms *terms, struct strbuf *line, struct strbuf *word)
+{
+ int res = 0;
+ int pos = 0;
+
+ while (pos < line->len) {
+ pos = get_next_word(line->buf, pos, word);
+
+ if (!strcmp(word->buf, "git"))
+ continue;
+ else if (!strcmp(word->buf, "git-bisect"))
+ continue;
+ else if (!strcmp(word->buf, "bisect"))
+ continue;
+ else if (starts_with(word->buf, "#"))
+ break;
+
+ get_terms(terms);
+ if (check_and_set_terms(terms, word->buf))
+ return -1;
+
+ if (!strcmp(word->buf, "start")) {
+ struct argv_array argv = ARGV_ARRAY_INIT;
+ int res;
+ sq_dequote_to_argv_array(line->buf+pos, &argv);
+ res = bisect_start(terms, 0, argv.argv, argv.argc);
+ argv_array_clear(&argv);
+ if (res)
+ return -1;
+ break;
+ }
+
+ if (one_of(word->buf, terms->term_good,
+ terms->term_bad, "skip", NULL)) {
+ if (bisect_write(word->buf, line->buf+pos, terms, 0))
+ return -1;
+ break;
+ }
+
+ if (!strcmp(word->buf, "terms")) {
+ struct argv_array argv = ARGV_ARRAY_INIT;
+ int res;
+ sq_dequote_to_argv_array(line->buf+pos, &argv);
+ res = bisect_terms(terms, argv.argc == 1 ? argv.argv[0] : NULL);
+ argv_array_clear(&argv);
+ if (res)
+ return -1;
+ break;
+ }
+
+ error(_("Replay file contains rubbish (\"%s\")"),
+ word->buf);
+ res = -1;
+ }
+ return res;
+}
+
+static int process_replay_file(FILE *fp, struct bisect_terms *terms)
+{
+ struct strbuf line = STRBUF_INIT;
+ struct strbuf word = STRBUF_INIT;
+ int res = 0;
+
+ while (strbuf_getline(&line, fp) != EOF) {
+ res = process_line(terms, &line, &word);
+ if (res)
+ break;
+ }
+
+ strbuf_release(&line);
+ strbuf_release(&word);
+ return res;
+}
+
+static int bisect_replay(struct bisect_terms *terms, const char *filename)
+{
+ FILE *fp = NULL;
+ int res = 0;
+
+ if (is_empty_or_missing_file(filename))
+ return error(_("cannot read file '%s' for replaying"), filename);
+
+ if (bisect_reset(NULL))
+ return -1;
+
+ fp = fopen(filename, "r");
+ if (!fp)
+ return -1;
+
+ res = process_replay_file(fp, terms);
+ fclose(fp);
+
+ if (res)
+ return -1;
+
+ return bisect_auto_next(terms, NULL);
+}
+
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
@@ -917,7 +1033,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
BISECT_AUTO_NEXT,
BISECT_AUTOSTART,
BISECT_STATE,
- BISECT_LOG
+ BISECT_LOG,
+ BISECT_REPLAY
} cmdmode = 0;
int no_checkout = 0, res = 0, nolog = 0;
struct option options[] = {
@@ -943,6 +1060,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
N_("mark the state of ref (or refs)"), BISECT_STATE),
OPT_CMDMODE(0, "bisect-log", &cmdmode,
N_("output the contents of BISECT_LOG"), BISECT_LOG),
+ OPT_CMDMODE(0, "bisect-replay", &cmdmode,
+ N_("replay the bisection process from the given file"), BISECT_REPLAY),
OPT_BOOL(0, "no-checkout", &no_checkout,
N_("update BISECT_HEAD instead of checking out the current commit")),
OPT_BOOL(0, "no-log", &nolog,
@@ -1020,6 +1139,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
return error(_("--bisect-log requires 0 arguments"));
res = bisect_log();
break;
+ case BISECT_REPLAY:
+ if (argc != 1)
+ return error(_("no logfile given"));
+ set_terms(&terms, "bad", "good");
+ res = bisect_replay(&terms, argv[0]);
+ break;
default:
return error("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index 151358aeda..0555191c41 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -77,36 +77,6 @@ bisect_visualize() {
eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")
}
-bisect_replay () {
- file="$1"
- test "$#" -eq 1 || die "$(gettext "No logfile given")"
- test -r "$file" || die "$(eval_gettext "cannot read \$file for replaying")"
- git bisect--helper --bisect-reset || exit
- while read git bisect command rev
- do
- test "$git $bisect" = "git bisect" || test "$git" = "git-bisect" || continue
- if test "$git" = "git-bisect"
- then
- rev="$command"
- command="$bisect"
- fi
- get_terms
- git bisect--helper --check-and-set-terms "$command" "$TERM_GOOD" "$TERM_BAD" || exit
- get_terms
- case "$command" in
- start)
- 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)
- git bisect--helper --bisect-terms $rev || exit;;
- *)
- die "$(gettext "?? what are you talking about?")" ;;
- esac
- done <"$file"
- git bisect--helper --bisect-auto-next
-}
-
bisect_run () {
git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
@@ -202,7 +172,7 @@ case "$#" in
reset)
git bisect--helper --bisect-reset "$@" ;;
replay)
- bisect_replay "$@" ;;
+ git bisect--helper --bisect-replay "$@" ;;
log)
git bisect--helper --bisect-log ;;
run)
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 22/29] bisect--helper: retire `--bisect-write` subcommand
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (20 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 21/29] bisect--helper: reimplement `bisect_replay` " Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 23/29] bisect--helper: use `res` instead of return in BISECT_RESET case option Miriam Rubio
` (7 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 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-write` subcommand is no longer used from the
git-bisect.sh shell script. Instead the function `bisect_write()`
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 | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 4b4ce13b50..95ac1a4558 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -21,7 +21,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
static const char * const git_bisect_helper_usage[] = {
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>"),
N_("git bisect--helper --bisect-next-check <good_term> <bad_term> [<term>]"),
N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
@@ -1024,7 +1023,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
BISECT_RESET = 1,
- BISECT_WRITE,
CHECK_AND_SET_TERMS,
BISECT_NEXT_CHECK,
BISECT_TERMS,
@@ -1040,8 +1038,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
struct option options[] = {
OPT_CMDMODE(0, "bisect-reset", &cmdmode,
N_("reset the bisection state"), BISECT_RESET),
- OPT_CMDMODE(0, "bisect-write", &cmdmode,
- N_("write out the bisection state in BISECT_LOG"), BISECT_WRITE),
OPT_CMDMODE(0, "check-and-set-terms", &cmdmode,
N_("check and set terms in a bisection state"), CHECK_AND_SET_TERMS),
OPT_CMDMODE(0, "bisect-next-check", &cmdmode,
@@ -1082,11 +1078,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
if (argc > 1)
return error(_("--bisect-reset requires either no argument or a commit"));
return !!bisect_reset(argc ? argv[0] : NULL);
- case BISECT_WRITE:
- if (argc != 4 && argc != 5)
- return error(_("--bisect-write requires either 4 or 5 arguments"));
- set_terms(&terms, argv[3], argv[2]);
- res = bisect_write(argv[0], argv[1], &terms, nolog);
break;
case CHECK_AND_SET_TERMS:
if (argc != 3)
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 23/29] bisect--helper: use `res` instead of return in BISECT_RESET case option
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (21 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 22/29] bisect--helper: retire `--bisect-write` subcommand Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 24/29] bisect--helper: retire `--bisect-autostart` subcommand Miriam Rubio
` (6 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 UTC (permalink / raw)
To: git; +Cc: Pranit Bauva, Christian Couder, Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
Use `res` variable to store `bisect_reset()` output in BISECT_RESET
case option to make bisect--helper.c more consistent.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
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 95ac1a4558..378b41cf70 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -1077,7 +1077,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
case BISECT_RESET:
if (argc > 1)
return error(_("--bisect-reset requires either no argument or a commit"));
- return !!bisect_reset(argc ? argv[0] : NULL);
+ res = bisect_reset(argc ? argv[0] : NULL);
break;
case CHECK_AND_SET_TERMS:
if (argc != 3)
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 24/29] bisect--helper: retire `--bisect-autostart` subcommand
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (22 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 23/29] bisect--helper: use `res` instead of return in BISECT_RESET case option Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 25/29] bisect--helper: retire `--bisect-auto-next` subcommand Miriam Rubio
` (5 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 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 378b41cf70..9becc1d514 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -28,7 +28,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>...]"),
N_("git bisect--helper --bisect-replay <filename>"),
@@ -1029,7 +1028,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
BISECT_START,
BISECT_NEXT,
BISECT_AUTO_NEXT,
- BISECT_AUTOSTART,
BISECT_STATE,
BISECT_LOG,
BISECT_REPLAY
@@ -1050,8 +1048,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_CMDMODE(0, "bisect-log", &cmdmode,
@@ -1112,12 +1108,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:
if (!argc)
return error(_("--bisect-state requires at least one revision"));
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 25/29] bisect--helper: retire `--bisect-auto-next` subcommand
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (23 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 24/29] bisect--helper: retire `--bisect-autostart` subcommand Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 26/29] bisect--helper: reimplement `bisect_skip` shell function in C Miriam Rubio
` (4 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 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-auto-next subcommand is no longer used from the
git-bisect.sh shell script. Instead the function bisect_auto_next()
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 9becc1d514..a40477811e 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -27,7 +27,6 @@ static const char * const git_bisect_helper_usage[] = {
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-state (bad|new) [<rev>]"),
N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
N_("git bisect--helper --bisect-replay <filename>"),
@@ -1027,7 +1026,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
BISECT_TERMS,
BISECT_START,
BISECT_NEXT,
- BISECT_AUTO_NEXT,
BISECT_STATE,
BISECT_LOG,
BISECT_REPLAY
@@ -1046,8 +1044,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
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-state", &cmdmode,
N_("mark the state of ref (or refs)"), BISECT_STATE),
OPT_CMDMODE(0, "bisect-log", &cmdmode,
@@ -1102,12 +1098,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
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_STATE:
if (!argc)
return error(_("--bisect-state requires at least one revision"));
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 26/29] bisect--helper: reimplement `bisect_skip` shell function in C
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (24 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 25/29] bisect--helper: retire `--bisect-auto-next` subcommand Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 27/29] bisect--helper: retire `--check-and-set-terms` subcommand Miriam Rubio
` (3 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 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_skip()` shell function in C and also add
`bisect-skip` subcommand to `git bisect--helper` to call it from
git-bisect.sh
Using `--bisect-skip` subcommand is a temporary measure to port shell
function to C so as to use the existing test suite.
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 | 46 +++++++++++++++++++++++++++++++++++++++-
git-bisect.sh | 17 +--------------
2 files changed, 46 insertions(+), 17 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index a40477811e..841a76fc7c 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -30,6 +30,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
N_("git bisect--helper --bisect-replay <filename>"),
+ N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"),
NULL
};
@@ -1017,6 +1018,42 @@ static int bisect_replay(struct bisect_terms *terms, const char *filename)
return bisect_auto_next(terms, NULL);
}
+static int bisect_skip(struct bisect_terms *terms, const char **argv, int argc)
+{
+ int i, res;
+ const char *pattern = "*..*";
+ struct argv_array argv_state = ARGV_ARRAY_INIT;
+
+ argv_array_push(&argv_state, "skip");
+
+ for (i = 0; i < argc; i++) {
+ if (!wildmatch(pattern, argv[i], 0)) {
+ struct rev_info revs;
+ struct commit *commit;
+ struct argv_array rev_argv = ARGV_ARRAY_INIT;
+
+ argv_array_pushl(&rev_argv, "skipped_commits", argv[i], NULL);
+ init_revisions(&revs, NULL);
+ setup_revisions(rev_argv.argc, rev_argv.argv, &revs, NULL);
+ argv_array_clear(&rev_argv);
+
+ if (prepare_revision_walk(&revs))
+ die(_("revision walk setup failed\n"));
+ while ((commit = get_revision(&revs)) != NULL)
+ argv_array_push(&argv_state,
+ oid_to_hex(&commit->object.oid));
+
+ reset_revision_walk();
+ } else {
+ argv_array_push(&argv_state, argv[i]);
+ }
+ }
+ res = bisect_state(terms, argv_state.argv, argv_state.argc);
+
+ argv_array_clear(&argv_state);
+ return res;
+}
+
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
@@ -1028,7 +1065,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
BISECT_NEXT,
BISECT_STATE,
BISECT_LOG,
- BISECT_REPLAY
+ BISECT_REPLAY,
+ BISECT_SKIP
} cmdmode = 0;
int no_checkout = 0, res = 0, nolog = 0;
struct option options[] = {
@@ -1050,6 +1088,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
N_("output the contents of BISECT_LOG"), BISECT_LOG),
OPT_CMDMODE(0, "bisect-replay", &cmdmode,
N_("replay the bisection process from the given file"), BISECT_REPLAY),
+ OPT_CMDMODE(0, "bisect-skip", &cmdmode,
+ N_("skip some commits for checkout"), BISECT_SKIP),
OPT_BOOL(0, "no-checkout", &no_checkout,
N_("update BISECT_HEAD instead of checking out the current commit")),
OPT_BOOL(0, "no-log", &nolog,
@@ -1116,6 +1156,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
set_terms(&terms, "bad", "good");
res = bisect_replay(&terms, argv[0]);
break;
+ case BISECT_SKIP:
+ set_terms(&terms, "bad", "good");
+ res = bisect_skip(&terms, argv, argc);
+ break;
default:
return error("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index 0555191c41..edfd3f8b3d 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -39,21 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
TERM_BAD=bad
TERM_GOOD=good
-bisect_skip() {
- all=''
- for arg in "$@"
- do
- case "$arg" in
- *..*)
- revs=$(git rev-list "$arg") || die "$(eval_gettext "Bad rev input: \$arg")" ;;
- *)
- revs=$(git rev-parse --sq-quote "$arg") ;;
- esac
- all="$all $revs"
- done
- eval git bisect--helper --bisect-state 'skip' $all
-}
-
bisect_visualize() {
git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
@@ -162,7 +147,7 @@ case "$#" in
bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
git bisect--helper --bisect-state "$cmd" "$@" ;;
skip)
- bisect_skip "$@" ;;
+ git bisect--helper --bisect-skip "$@" ;;
next)
# Not sure we want "next" at the UI level anymore.
get_terms
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 27/29] bisect--helper: retire `--check-and-set-terms` subcommand
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (25 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 26/29] bisect--helper: reimplement `bisect_skip` shell function in C Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:37 ` [PATCH 28/29] bisect--helper: reimplement `bisect_visualize()`shell function in C Miriam Rubio
` (2 subsequent siblings)
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 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-and-set-terms` subcommand is no longer from the
git-bisect.sh shell script. Instead the function
`check_and_set_terms()` is called from the C implementation.
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamil.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 841a76fc7c..7bc28d728e 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -21,7 +21,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-reset [<commit>]"),
- N_("git bisect--helper --bisect-check-and-set-terms <command> <good_term> <bad_term>"),
N_("git bisect--helper --bisect-next-check <good_term> <bad_term> [<term>]"),
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>]"
@@ -1058,7 +1057,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
BISECT_RESET = 1,
- CHECK_AND_SET_TERMS,
BISECT_NEXT_CHECK,
BISECT_TERMS,
BISECT_START,
@@ -1072,8 +1070,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
struct option options[] = {
OPT_CMDMODE(0, "bisect-reset", &cmdmode,
N_("reset the bisection state"), BISECT_RESET),
- OPT_CMDMODE(0, "check-and-set-terms", &cmdmode,
- N_("check and set terms in a bisection state"), CHECK_AND_SET_TERMS),
OPT_CMDMODE(0, "bisect-next-check", &cmdmode,
N_("check whether bad or good terms exist"), BISECT_NEXT_CHECK),
OPT_CMDMODE(0, "bisect-terms", &cmdmode,
@@ -1111,12 +1107,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
return error(_("--bisect-reset requires either no argument or a commit"));
res = bisect_reset(argc ? argv[0] : NULL);
break;
- case CHECK_AND_SET_TERMS:
- if (argc != 3)
- return error(_("--check-and-set-terms requires 3 arguments"));
- set_terms(&terms, argv[2], argv[1]);
- res = check_and_set_terms(&terms, argv[0]);
- break;
case BISECT_NEXT_CHECK:
if (argc != 2 && argc != 3)
return error(_("--bisect-next-check requires 2 or 3 arguments"));
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 28/29] bisect--helper: reimplement `bisect_visualize()`shell function in C
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (26 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 27/29] bisect--helper: retire `--check-and-set-terms` subcommand Miriam Rubio
@ 2020-01-20 14:37 ` Miriam Rubio
2020-01-20 14:38 ` [PATCH 29/29] bisect--helper: reimplement `bisect_run` shell " Miriam Rubio
2020-01-20 21:41 ` [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Johannes Schindelin
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:37 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Christian Couder, Johannes Schindelin,
Tanushree Tumane, Miriam Rubio
From: Pranit Bauva <pranit.bauva@gmail.com>
Reimplement the `bisect_visualize()` shell function
in C and also add `--bisect-visualize` subcommand to
`git bisect--helper` to call it from git-bisect.sh.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 48 +++++++++++++++++++++++++++++++++++++++-
git-bisect.sh | 25 +--------------------
2 files changed, 48 insertions(+), 25 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 7bc28d728e..4b41cc7749 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -30,6 +30,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
N_("git bisect--helper --bisect-replay <filename>"),
N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"),
+ N_("git bisect--helper --bisect-visualize"),
NULL
};
@@ -1053,6 +1054,44 @@ static int bisect_skip(struct bisect_terms *terms, const char **argv, int argc)
return res;
}
+static int bisect_visualize(struct bisect_terms *terms, const char **argv, int argc)
+{
+ struct argv_array args = ARGV_ARRAY_INIT;
+ int flags = RUN_COMMAND_NO_STDIN, res = 0;
+ struct strbuf sb;
+
+ if (!bisect_next_check(terms, NULL))
+ return -1;
+
+ if (!argc) {
+ if ((getenv("DISPLAY") || getenv("SESSIONNAME") || getenv("MSYSTEM") ||
+ getenv("SECURITYSESSIONID")) && exists_in_PATH("gitk"))
+ argv_array_push(&args, "gitk");
+ else {
+ argv_array_pushl(&args, "log", NULL);
+ flags |= RUN_GIT_CMD;
+ }
+ } else {
+ if (argv[0][0] == '-') {
+ argv_array_pushl(&args, "log", NULL);
+ flags |= RUN_GIT_CMD;
+ } else if (strcmp(argv[0], "tig") && !starts_with(argv[0], "git"))
+ flags |= RUN_GIT_CMD;
+
+ argv_array_pushv(&args, argv);
+ }
+
+ argv_array_pushl(&args, "--bisect", NULL);
+
+ strbuf_read_file(&sb, git_path_bisect_names(), 0);
+ argv_array_split(&args, sb.buf);
+ strbuf_release(&sb);
+
+ res = run_command_v_opt(args.argv, flags);
+ argv_array_clear(&args);
+ return res;
+}
+
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
@@ -1064,7 +1103,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
BISECT_STATE,
BISECT_LOG,
BISECT_REPLAY,
- BISECT_SKIP
+ BISECT_SKIP,
+ BISECT_VISUALIZE
} cmdmode = 0;
int no_checkout = 0, res = 0, nolog = 0;
struct option options[] = {
@@ -1086,6 +1126,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
N_("replay the bisection process from the given file"), BISECT_REPLAY),
OPT_CMDMODE(0, "bisect-skip", &cmdmode,
N_("skip some commits for checkout"), BISECT_SKIP),
+ OPT_CMDMODE(0, "bisect-visualize", &cmdmode,
+ N_("visualize the bisection"), BISECT_VISUALIZE),
OPT_BOOL(0, "no-checkout", &no_checkout,
N_("update BISECT_HEAD instead of checking out the current commit")),
OPT_BOOL(0, "no-log", &nolog,
@@ -1150,6 +1192,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
set_terms(&terms, "bad", "good");
res = bisect_skip(&terms, argv, argc);
break;
+ case BISECT_VISUALIZE:
+ get_terms(&terms);
+ res = bisect_visualize(&terms, argv, argc);
+ break;
default:
return error("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index edfd3f8b3d..084766636d 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -39,29 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
TERM_BAD=bad
TERM_GOOD=good
-bisect_visualize() {
- git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
-
- if test $# = 0
- then
- if test -n "${DISPLAY+set}${SESSIONNAME+set}${MSYSTEM+set}${SECURITYSESSIONID+set}" &&
- type gitk >/dev/null 2>&1
- then
- set gitk
- else
- set git log
- fi
- else
- case "$1" in
- git*|tig) ;;
- -*) set git log "$@" ;;
- *) set git "$@" ;;
- esac
- fi
-
- eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")
-}
-
bisect_run () {
git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
@@ -153,7 +130,7 @@ case "$#" in
get_terms
git bisect--helper --bisect-next "$@" || exit ;;
visualize|view)
- bisect_visualize "$@" ;;
+ git bisect--helper --bisect-visualize "$@" ;;
reset)
git bisect--helper --bisect-reset "$@" ;;
replay)
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 29/29] bisect--helper: reimplement `bisect_run` shell function in C
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (27 preceding siblings ...)
2020-01-20 14:37 ` [PATCH 28/29] bisect--helper: reimplement `bisect_visualize()`shell function in C Miriam Rubio
@ 2020-01-20 14:38 ` Miriam Rubio
2020-01-20 21:41 ` [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Johannes Schindelin
29 siblings, 0 replies; 54+ messages in thread
From: Miriam Rubio @ 2020-01-20 14:38 UTC (permalink / raw)
To: git; +Cc: Tanushree Tumane, Christian Couder, Miriam Rubio
From: Tanushree Tumane <tanushreetumane@gmail.com>
Reimplement the `bisect_run()` shell function
in C and also add `--bisect-run` subcommand to
`git bisect--helper` to call it from git-bisect.sh.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
builtin/bisect--helper.c | 66 +++++++++++++++++++++++++++++++++++++++-
git-bisect.sh | 62 +------------------------------------
2 files changed, 66 insertions(+), 62 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 4b41cc7749..1899cc8114 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -31,6 +31,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-replay <filename>"),
N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"),
N_("git bisect--helper --bisect-visualize"),
+ N_("git bisect--helper --bisect-run <cmd>..."),
NULL
};
@@ -1092,6 +1093,60 @@ static int bisect_visualize(struct bisect_terms *terms, const char **argv, int a
return res;
}
+static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
+{
+ int i, res = 0;
+ struct strbuf command = STRBUF_INIT;
+ struct argv_array args = ARGV_ARRAY_INIT;
+
+ if (bisect_next_check(terms, NULL))
+ return -1;
+
+ for (i = 0; i < argc; i++) {
+ strbuf_addstr(&command, argv[i]);
+ strbuf_addstr(&command, " ");
+ }
+
+ while (1) {
+ argv_array_clear(&args);
+
+ printf(_("running %s"), command.buf);
+ res = run_command_v_opt(argv, RUN_USING_SHELL);
+
+ if (res < 0 && res >= 128) {
+ error(_("bisect run failed: exit code %d from"
+ " '%s' is < 0 or >= 128"), res, command.buf);
+ strbuf_release(&command);
+ return res;
+ }
+
+ if (res == 125)
+ argv_array_push(&args, "skip");
+ else if (res > 0)
+ argv_array_push(&args, terms->term_bad);
+ else
+ argv_array_push(&args, terms->term_good);
+
+ res = bisect_state(terms, args.argv, args.argc);
+
+ if (res == -11) {
+ printf(_("bisect run success"));
+ res = 0;
+ }
+ else if (res == -2)
+ error(_("bisect run cannot continue any more"));
+ else if (res)
+ error(_("bisect run failed:'git bisect--helper --bisect-state"
+ " %s' exited with error code %d"), args.argv[0], res);
+ else
+ continue;
+
+ strbuf_release(&command);
+ argv_array_clear(&args);
+ return res;
+ }
+}
+
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
@@ -1104,7 +1159,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
BISECT_LOG,
BISECT_REPLAY,
BISECT_SKIP,
- BISECT_VISUALIZE
+ BISECT_VISUALIZE,
+ BISECT_RUN,
} cmdmode = 0;
int no_checkout = 0, res = 0, nolog = 0;
struct option options[] = {
@@ -1128,6 +1184,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
N_("skip some commits for checkout"), BISECT_SKIP),
OPT_CMDMODE(0, "bisect-visualize", &cmdmode,
N_("visualize the bisection"), BISECT_VISUALIZE),
+ OPT_CMDMODE(0, "bisect-run", &cmdmode,
+ N_("use <cmd>... to automatically bisect."), BISECT_RUN),
OPT_BOOL(0, "no-checkout", &no_checkout,
N_("update BISECT_HEAD instead of checking out the current commit")),
OPT_BOOL(0, "no-log", &nolog,
@@ -1196,6 +1254,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
get_terms(&terms);
res = bisect_visualize(&terms, argv, argc);
break;
+ case BISECT_RUN:
+ if (!argc)
+ return error(_("bisect run failed: no command provided."));
+ get_terms(&terms);
+ res = bisect_run(&terms, argv, argc);
+ break;
default:
return error("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index 084766636d..d4f64674fe 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -39,66 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
TERM_BAD=bad
TERM_GOOD=good
-bisect_run () {
- git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
-
- test -n "$*" || die "$(gettext "bisect run failed: no command provided.")"
-
- while true
- do
- command="$@"
- eval_gettextln "running \$command"
- "$@"
- res=$?
-
- # Check for really bad run error.
- if [ $res -lt 0 -o $res -ge 128 ]
- then
- eval_gettextln "bisect run failed:
-exit code \$res from '\$command' is < 0 or >= 128" >&2
- exit $res
- fi
-
- # Find current state depending on run success or failure.
- # A special exit code of 125 means cannot test.
- if [ $res -eq 125 ]
- then
- state='skip'
- elif [ $res -gt 0 ]
- then
- state="$TERM_BAD"
- else
- state="$TERM_GOOD"
- fi
-
- ( git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN" )
- res=$?
-
- cat "$GIT_DIR/BISECT_RUN"
-
- if sane_grep "first $TERM_BAD commit could be any of" "$GIT_DIR/BISECT_RUN" \
- >/dev/null
- then
- gettextln "bisect run cannot continue any more" >&2
- exit $res
- fi
-
- if [ $res -ne 0 ]
- then
- eval_gettextln "bisect run failed:
-'git bisect--helper --bisect-state \$state' exited with error code \$res" >&2
- exit $res
- fi
-
- if sane_grep "is the first $TERM_BAD commit" "$GIT_DIR/BISECT_RUN" >/dev/null
- then
- gettextln "bisect run success"
- exit 0;
- fi
-
- done
-}
-
get_terms () {
if test -s "$GIT_DIR/BISECT_TERMS"
then
@@ -138,7 +78,7 @@ case "$#" in
log)
git bisect--helper --bisect-log ;;
run)
- bisect_run "$@" ;;
+ git bisect--helper --bisect-run "$(git rev-parse --sq-quote "$@")" ;;
terms)
git bisect--helper --bisect-terms "$@" || exit;;
*)
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
` (28 preceding siblings ...)
2020-01-20 14:38 ` [PATCH 29/29] bisect--helper: reimplement `bisect_run` shell " Miriam Rubio
@ 2020-01-20 21:41 ` Johannes Schindelin
2020-01-20 23:24 ` Christian Couder
2020-01-21 8:44 ` Miriam R.
29 siblings, 2 replies; 54+ messages in thread
From: Johannes Schindelin @ 2020-01-20 21:41 UTC (permalink / raw)
To: Miriam Rubio; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 6589 bytes --]
Hi Miriam,
On Mon, 20 Jan 2020, Miriam Rubio wrote:
> --- Changes since Tanushree’s pr117 sent patch series:
> https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) ---
>
> General changes
> ---------------
>
> * Rebase on master branch.
> * Improve commit messages.
> * Amend patch series titles.
> * Reorder commits: first clean-up/preparatory commits, squash or split
> commits.
Great!
> Specific changes
> ----------------
>
> [1/29] bisect--helper: convert `vocab_*` char pointers to char arrays
>
> * New patch to convert `vocab_bad` and `vocab_good` char pointers
> to char arrays
29 patches is _a lot_ to review. I would have preferred a series of
smaller patch series.
For example, the first three patches would have made for a fine "some
cleanups" patch series, from my point of view.
Also, as the mail's subject says "part 1", it would be good to have an
overview how this part fits into the overall story of converting `git
bisect` into a built-in.
Finally, it would be nice to have a link to a public repository with the
branch from which these mails were generated.
I will try to review this patch series in its entirety, but it will take
me a while.
Ciao,
Johannes
>
> --
>
> [2/29] bisect--helper: change `retval` to `res`
>
> * Replace one last variable `retval` to `res`.
>
> --
>
> [3/29] bisect: use the standard 'if (!var)' way to check for 0
>
> * New patch to use '!var' and make 'bisect.c' more consistent with the
> rest of the code
>
> --
>
> [4/29] run-command: make `exists_in_PATH()` non-static
>
> * Add comment before function declaration.
> * Move function declaration in `run-command.h`.
>
> --
>
> [6/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*`
> and its dependents
>
> * Fix `mark_edges_uninteresting()` and `show_diff_tree()` calls after
> rebase on master.
>
> --
>
> [7/29] bisect: libify `bisect_checkout`
>
> * Fix `memcpy()` call after rebase on master.
> * Introduce `res` variable to return `bisect_checkout()` output.
> * Fix `get_commit_reference()` declaration after rebase on master.
>
> --
>
> [8/29] bisect: libify `check_merge_bases` and its dependents
>
> State: Previously sent
>
> * Fix `check_ancestors()` declaration after rebase on master.
> * Fix `get_bad_and_good_commits()` call after rebase on master.
>
> --
>
> [9/29] bisect: libify `check_good_are_ancestors_of_bad` and its
> dependents
>
> State: Previously sent
>
> * Fix `check_good_are_ancestors_of_bad()` declaration after rebase on
> master.
> * Fix `check_good_are_ancestors_of_bad()`, `bisect_next_all()`
> and `bisect_rev_setup()` calls after rebase on master.
>
> --
>
> [11/29] bisect: libify `bisect_next_all`
>
> State: Previously sent
>
> * Fix `show_diff_tree()` call after rebase on master.
>
> --
>
> [12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next`
> shell functions in C
>
> * Fix `repo_init_revisions()` and `bisect_next_all()` calls after rebase
> on master.
> * Remove `goto` statement in `bisect_skipped_commits()`
>
> --
>
> [13/29] bisect--helper: finish porting `bisect_start()` to C
>
> * Change `return` statement instead of `die` in error handling.
> * Remove `goto` statements in `bisect_skipped_commits()`.
>
> --
>
> [21/29] bisect--helper: reimplement `bisect_replay` shell function in C
>
> * Add blank line in `get_next_word()`.
> * Remove `goto` statements in `bisect_replay()`.
>
> --
>
> [23/29] bisect--helper: use `res` instead of return in BISECT_RESET case
> option
>
> * New patch to split previous commit in two.
>
> --
>
> [26/29] bisect--helper: reimplement `bisect_skip` shell function in C
>
> State: Previously sent
>
> * Add blank line.
>
> --
>
> [28/29] bisect--helper: reimplement `bisect_visualize()`shell function
> in C
>
> New patch:
>
> * Reimplement the `bisect_visualize()` shell function in C.
> * Add `--bisect-visualize` subcommand.
> * Fix long code line.
>
> --
>
> [29/29] bisect--helper: reimplement `bisect_run` shell function in C
>
> New patch:
>
> * Reimplement the `bisect_run()` shell function in C.
> * Add `--bisect-run` subcommand.
> * Remove blank line.
>
> --
>
> Miriam Rubio (2):
> bisect--helper: convert `vocab_*` char pointers to char arrays
> bisect: use the standard 'if (!var)' way to check for 0
>
> Pranit Bauva (24):
> run-command: make `exists_in_PATH()` non-static
> bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and
> its dependents
> bisect: libify `bisect_checkout`
> bisect: libify `check_merge_bases` and its dependents
> bisect: libify `check_good_are_ancestors_of_bad` and its dependents
> bisect: libify `handle_bad_merge_base` and its dependents
> bisect: libify `bisect_next_all`
> 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_autostart` shell function in C
> 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: reimplement `bisect_log` shell function in C
> bisect--helper: reimplement `bisect_replay` shell function in C
> bisect--helper: retire `--bisect-write` subcommand
> bisect--helper: use `res` instead of return in BISECT_RESET case
> option
> bisect--helper: retire `--bisect-autostart` subcommand
> bisect--helper: retire `--bisect-auto-next` subcommand
> bisect--helper: reimplement `bisect_skip` shell function in C
> bisect--helper: retire `--check-and-set-terms` subcommand
> bisect--helper: reimplement `bisect_visualize()`shell function in C
>
> Tanushree Tumane (3):
> bisect--helper: change `retval` to `res`
> bisect--helper: introduce new `decide_next()` function
> bisect--helper: reimplement `bisect_run` shell function in C
>
> bisect.c | 146 +++++---
> builtin/bisect--helper.c | 776 +++++++++++++++++++++++++++++++++------
> git-bisect.sh | 279 +-------------
> run-command.c | 2 +-
> run-command.h | 11 +
> 5 files changed, 793 insertions(+), 421 deletions(-)
>
> --
> 2.21.1 (Apple Git-122.3)
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 06/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents
2020-01-20 14:37 ` [PATCH 06/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents Miriam Rubio
@ 2020-01-20 21:57 ` Johannes Schindelin
2020-01-21 6:40 ` Christian Couder
0 siblings, 1 reply; 54+ messages in thread
From: Johannes Schindelin @ 2020-01-20 21:57 UTC (permalink / raw)
To: Miriam Rubio; +Cc: git, Pranit Bauva, Christian Couder, Tanushree Tumane
Hi Miriam,
On Mon, 20 Jan 2020, Miriam Rubio wrote:
> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Since we want to get rid of git-bisect.sh it would be necessary to
> convert those exit() calls to return statements so that errors can be
> reported.
>
> Emulate try catch in C by converting `exit(<positive-value>)` to
> `return <negative-value>`. Follow POSIX conventions to return
> <negative-value> to indicate error.
Good.
> Modify `cmd_bisect_helper()` to handle these negative returns.
>
> Turn `exit()` to `return` calls in `exit_if_skipped_commits()` and rename
> the method to `error_if_skipped_commits()`.
I would remove these two sentences, as I deem it obvious from the
rationale above that this needs to be done, and the patch repeats it.
>
> Handle this return in dependant function `bisect_next_all()`.
>
> 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 | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index 83cb5b3a98..2ac0463327 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -661,11 +661,11 @@ static void bisect_common(struct rev_info *revs)
> mark_edges_uninteresting(revs, NULL, 0);
> }
>
> -static void exit_if_skipped_commits(struct commit_list *tried,
> +static int error_if_skipped_commits(struct commit_list *tried,
> const struct object_id *bad)
> {
> if (!tried)
> - return;
> + return 0;
>
> printf("There are only 'skip'ped commits left to test.\n"
> "The first %s commit could be any of:\n", term_bad);
> @@ -676,7 +676,13 @@ static void exit_if_skipped_commits(struct commit_list *tried,
> if (bad)
> printf("%s\n", oid_to_hex(bad));
> printf(_("We cannot bisect more!\n"));
> - exit(2);
> +
> + /*
> + * We don't want to clean the bisection state
> + * as we need to get back to where we started
> + * by using `git bisect reset`.
> + */
> + return -2;
This comment is a good indicator that the constant `-2` here is a "magic"
number and it would most likely make sense to turn the return type from an
`int` into an `enum` instead.
> }
>
> static int is_expected_rev(const struct object_id *oid)
> @@ -949,7 +955,7 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
> {
> struct rev_info revs;
> struct commit_list *tried;
> - int reaches = 0, all = 0, nr, steps;
> + int reaches = 0, all = 0, nr, steps, res;
> struct object_id *bisect_rev;
> char *steps_msg;
>
> @@ -972,8 +978,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
> * We should exit here only if the "bad"
> * commit is also a "skip" commit.
> */
> - exit_if_skipped_commits(tried, NULL);
> -
> + res = error_if_skipped_commits(tried, NULL);
> + if (res)
> + exit(-res);
So we still `exit()` in `libgit.a`? I hoped for a more thorough
libification.
Besides, the `if (res)` probably wants to be an `if (res < 0)`, right?
Ciao,
Johannes
> printf(_("%s was both %s and %s\n"),
> oid_to_hex(current_bad_oid),
> term_good,
> @@ -990,7 +997,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
> bisect_rev = &revs.commits->item->object.oid;
>
> if (oideq(bisect_rev, current_bad_oid)) {
> - exit_if_skipped_commits(tried, current_bad_oid);
> + res = error_if_skipped_commits(tried, current_bad_oid);
> + if (res)
> + exit(-res);
> printf("%s is the first %s commit\n", oid_to_hex(bisect_rev),
> term_bad);
> show_diff_tree(r, prefix, revs.commits->item);
> --
> 2.21.1 (Apple Git-122.3)
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 08/29] bisect: libify `check_merge_bases` and its dependents
2020-01-20 14:37 ` [PATCH 08/29] bisect: libify `check_merge_bases` and its dependents Miriam Rubio
@ 2020-01-20 22:09 ` Johannes Schindelin
2020-01-21 9:59 ` Miriam R.
0 siblings, 1 reply; 54+ messages in thread
From: Johannes Schindelin @ 2020-01-20 22:09 UTC (permalink / raw)
To: Miriam Rubio; +Cc: git, Pranit Bauva, Christian Couder, Tanushree Tumane
Hi,
On Mon, 20 Jan 2020, Miriam Rubio wrote:
> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Since we want to get rid of git-bisect.sh it would be necessary to
> convert those exit() calls to return statements so that errors can be
> reported.
>
> Emulate try catch in C by converting `exit(<positive-value>)` to
> `return <negative-value>`. Follow POSIX conventions to return
> <negative-value> to indicate error.
>
> Turn `exit()` to `return` calls in `check_merge_bases()`.
>
> In `check_merge_bases()` there is an early success special case,
> so we have introduced special error code `-11` which indicates early
> success. This `-11` is converted back to `0` in `check_good_are_ancestors_of_bad()`.
>
> Handle the return value in dependent function `check_good_are_ancestors_of_bad()`.
This is a lot of repeated text from earlier commit messages. It might make
sense to condense it a bit, and while at it, to remove sentences that
essentially repeat what the diff says.
>
> 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 | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index 385afaf875..367258b0dd 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -806,13 +806,16 @@ static void handle_skipped_merge_base(const struct object_id *mb)
> * "check_merge_bases" checks that merge bases are not "bad" (or "new").
> *
> * - If one is "bad" (or "new"), it means the user assumed something wrong
> - * and we must exit with a non 0 error code.
> + * and we must return error with a non 0 error code.
> * - If one is "good" (or "old"), that's good, we have nothing to do.
> * - If one is "skipped", we can't know but we should warn.
> * - If we don't know, we should check it out and ask the user to test.
> + * - If a merge base must be tested, on success return -11 a special condition
> + * for early success, this will be converted back to 0 in check_good_are_ancestors_of_bad().
> */
> -static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
> +static int check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
> {
> + int res = 0;
> struct commit_list *result;
>
> result = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1);
> @@ -827,11 +830,16 @@ static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
> handle_skipped_merge_base(mb);
> } else {
> printf(_("Bisecting: a merge base must be tested\n"));
> - exit(bisect_checkout(mb, no_checkout));
> + res = bisect_checkout(mb, no_checkout);
> + if (!res)
> + /* indicate early success */
> + res = -11;
This is yet another good candidate for an `enum`.
> + break;
> }
> }
>
> free_commit_list(result);
> + return res;
> }
>
> static int check_ancestors(struct repository *r, int rev_nr,
> @@ -865,7 +873,7 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
> {
> char *filename = git_pathdup("BISECT_ANCESTORS_OK");
> struct stat st;
> - int fd, rev_nr;
> + int fd, rev_nr, res = 0;
> struct commit **rev;
>
> if (!current_bad_oid)
> @@ -880,10 +888,13 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
> goto done;
>
> /* Check if all good revs are ancestor of the bad rev. */
> +
> rev = get_bad_and_good_commits(r, &rev_nr);
> if (check_ancestors(r, rev_nr, rev, prefix))
> - check_merge_bases(rev_nr, rev, no_checkout);
> + res = check_merge_bases(rev_nr, rev, no_checkout);
> free(rev);
> + if(res)
Please put a space between the `if` keyword and the `(` following it.
Thanks,
Johannes
> + exit(res == -11 ? 0 : -res);
>
> /* Create file BISECT_ANCESTORS_OK. */
> fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> --
> 2.21.1 (Apple Git-122.3)
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 09/29] bisect: libify `check_good_are_ancestors_of_bad` and its dependents
2020-01-20 14:37 ` [PATCH 09/29] bisect: libify `check_good_are_ancestors_of_bad` " Miriam Rubio
@ 2020-01-20 22:20 ` Johannes Schindelin
2020-01-21 6:59 ` Christian Couder
0 siblings, 1 reply; 54+ messages in thread
From: Johannes Schindelin @ 2020-01-20 22:20 UTC (permalink / raw)
To: Miriam Rubio; +Cc: git, Pranit Bauva, Christian Couder, Tanushree Tumane
Hi,
On Mon, 20 Jan 2020, Miriam Rubio wrote:
> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Since we want to get rid of git-bisect.sh it would be necessary to
> convert those exit() calls to return statements so that errors can be
> reported.
>
> Emulate try catch in C by converting `exit(<positive-value>)` to
> `return <negative-value>`. Follow POSIX conventions to return
> <negative-value> to indicate error.
>
> Turn `exit()` to `return` calls in `check_good_are_ancestors_of_bad()`.
>
> Code that turns -11(early success code) to 0 from
> `check_good_are_ancestors_of_bad()` has been moved to
> `cmd_bisect__helper()`.
>
> Handle the return value in dependent function `bisect_next_all()`.
>
> 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 | 42 ++++++++++++++++++++++++++--------------
> builtin/bisect--helper.c | 12 ++++++++++--
> 2 files changed, 37 insertions(+), 17 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index 367258b0dd..2b80597a1d 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -865,9 +865,10 @@ static int check_ancestors(struct repository *r, int rev_nr,
> *
> * If that's not the case, we need to check the merge bases.
> * If a merge base must be tested by the user, its source code will be
> - * checked out to be tested by the user and we will exit.
> + * checked out to be tested by the user and we will return.
> */
> -static void check_good_are_ancestors_of_bad(struct repository *r,
> +
> +static int check_good_are_ancestors_of_bad(struct repository *r,
> const char *prefix,
> int no_checkout)
> {
> @@ -876,8 +877,15 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
> int fd, rev_nr, res = 0;
> struct commit **rev;
>
> - if (!current_bad_oid)
> - die(_("a %s revision is needed"), term_bad);
> + /*
> + * We don't want to clean the bisection state
> + * as we need to get back to where we started
> + * by using `git bisect reset`.
> + */
> + if (!current_bad_oid) {
> + res = error(_("a %s revision is needed"), term_bad);
> + goto done;
> + }
Why not just return here? Ah, there is a `filename` that was allocated...
it is too bad that we have a mailing-list based review, as the hunk
context simply cannot be extended in a mail.
Personally, I think it would be nicer to split the definition of
`filename` from its declaration and move it _after_ this conditional code,
so that we can `return` right away.
However, there is a more pressing issue than that: `die()` exits with exit
code 128, so in keeping with the idea to hand down negative exit codes as
return values, should we not assign `res = -128` here?
>
> /* Check if file BISECT_ANCESTORS_OK exists. */
> if (!stat(filename, &st) && S_ISREG(st.st_mode))
> @@ -893,18 +901,20 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
> if (check_ancestors(r, rev_nr, rev, prefix))
> res = check_merge_bases(rev_nr, rev, no_checkout);
> free(rev);
> - if(res)
> - exit(res == -11 ? 0 : -res);
> -
> - /* Create file BISECT_ANCESTORS_OK. */
> - fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> - if (fd < 0)
> - warning_errno(_("could not create file '%s'"),
> - filename);
> - else
> - close(fd);
> +
> + if (!res)
> + {
We usually put the `{` on the same line as the `if` condition (like you
did in the `if (!current_bad_oid)` line above.
The rest looks reasonable. Thank you,
Johannes
> + /* Create file BISECT_ANCESTORS_OK. */
> + fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> + if (fd < 0)
> + warning_errno(_("could not create file '%s'"),
> + filename);
> + else
> + close(fd);
> + }
> done:
> free(filename);
> + return res;
> }
>
> /*
> @@ -975,7 +985,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
> if (read_bisect_refs())
> die(_("reading bisect refs failed"));
>
> - check_good_are_ancestors_of_bad(r, prefix, no_checkout);
> + res = check_good_are_ancestors_of_bad(r, prefix, no_checkout);
> + if (res)
> + return res;
>
> bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1);
> revs.limited = 1;
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 826fcba2ed..5e0f759d50 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -666,7 +666,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>
> switch (cmdmode) {
> case NEXT_ALL:
> - return bisect_next_all(the_repository, prefix, no_checkout);
> + res = bisect_next_all(the_repository, prefix, no_checkout);
> + break;
> case WRITE_TERMS:
> if (argc != 2)
> return error(_("--write-terms requires two arguments"));
> @@ -713,5 +714,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> return error("BUG: unknown subcommand '%d'", cmdmode);
> }
> free_terms(&terms);
> - return !!res;
> + /*
> + * Handle early success
> + * From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all
> + */
> + if (res == -11)
> + res = 0;
> +
> + return res < 0 ? -res : res;
> }
> --
> 2.21.1 (Apple Git-122.3)
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 10/29] bisect: libify `handle_bad_merge_base` and its dependents
2020-01-20 14:37 ` [PATCH 10/29] bisect: libify `handle_bad_merge_base` " Miriam Rubio
@ 2020-01-20 22:23 ` Johannes Schindelin
2020-01-21 7:05 ` Christian Couder
0 siblings, 1 reply; 54+ messages in thread
From: Johannes Schindelin @ 2020-01-20 22:23 UTC (permalink / raw)
To: Miriam Rubio; +Cc: git, Pranit Bauva, Christian Couder, Tanushree Tumane
Hi Miriam,
On Mon, 20 Jan 2020, Miriam Rubio wrote:
> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Since we want to get rid of git-bisect.sh it would be necessary to
> convert those exit() calls to return statements so that errors can be
> reported.
>
> Emulate try catch in C by converting `exit(<positive-value>)` to
> `return <negative-value>`. Follow POSIX conventions to return
> <negative-value> to indicate error.
>
> Turn `exit()` to `return` calls in `handle_bad_merge_base()`.
>
> Handle the return value in dependent function check_merge_bases().
This is again a lot of essentially repeated text from earlier commit
messages, but the most pressing question is not addressed...
>
> 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 | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index 2b80597a1d..acb5a13911 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -756,7 +756,7 @@ static struct commit **get_bad_and_good_commits(struct repository *r,
> return rev;
> }
>
> -static void handle_bad_merge_base(void)
> +static int handle_bad_merge_base(void)
> {
> if (is_expected_rev(current_bad_oid)) {
> char *bad_hex = oid_to_hex(current_bad_oid);
> @@ -777,14 +777,14 @@ static void handle_bad_merge_base(void)
> "between %s and [%s].\n"),
> bad_hex, term_bad, term_good, bad_hex, good_hex);
> }
> - exit(3);
> + return -3;
... which is: what does `3` stand for?
Thanks,
Johannes
> }
>
> fprintf(stderr, _("Some %s revs are not ancestors of the %s rev.\n"
> "git bisect cannot work properly in this case.\n"
> "Maybe you mistook %s and %s revs?\n"),
> term_good, term_bad, term_good, term_bad);
> - exit(1);
> + return -1;
> }
>
> static void handle_skipped_merge_base(const struct object_id *mb)
> @@ -823,7 +823,8 @@ static int check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
> for (; result; result = result->next) {
> const struct object_id *mb = &result->item->object.oid;
> if (oideq(mb, current_bad_oid)) {
> - handle_bad_merge_base();
> + res = handle_bad_merge_base();
> + break;
> } else if (0 <= oid_array_lookup(&good_revs, mb)) {
> continue;
> } else if (0 <= oid_array_lookup(&skipped_revs, mb)) {
> --
> 2.21.1 (Apple Git-122.3)
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 11/29] bisect: libify `bisect_next_all`
2020-01-20 14:37 ` [PATCH 11/29] bisect: libify `bisect_next_all` Miriam Rubio
@ 2020-01-20 22:29 ` Johannes Schindelin
2020-01-21 7:15 ` Christian Couder
0 siblings, 1 reply; 54+ messages in thread
From: Johannes Schindelin @ 2020-01-20 22:29 UTC (permalink / raw)
To: Miriam Rubio; +Cc: git, Pranit Bauva, Christian Couder, Tanushree Tumane
Hi Miriam,
On Mon, 20 Jan 2020, Miriam Rubio wrote:
> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Since we want to get rid of git-bisect.sh it would be necessary to
> convert those exit() calls to return statements so that errors can be
> reported.
>
> Emulate try catch in C by converting `exit(<positive-value>)` to
> `return <negative-value>`. Follow POSIX conventions to return
> <negative-value> to indicate error.
>
> Turn `exit()` to `return` calls in `bisect_next_all()`.
>
> All the functions calling `bisect_next_all()` are already able to
> handle return values from it.
So this patch brings more magic values that really would like to become
`enum` values. At this point, we're talking about `bisect_next_all()`
which is _not_ a file-local (`static`) function. Therefore, we now really
wade into API territory where an `enum` is no longer just a nice thing,
but a necessary thing: `bisect_next_all()` is a function that can be
called from other source files.
As far as I can see, this patch concludes the "libify" part of the patch
series (read: it would have been the second patch series I would have
suggested to split out from an otherwise too-long-to-be-reviewed set of
patches, if I had been your mentor), and I'll leave it at that for
tonight; Hopefully I will find some time to review more of these patches
tomorrow.
Ciao,
Johannes
>
> 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 | 40 +++++++++++++++++++++++++++++-----------
> 1 file changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index acb5a13911..33f2829c19 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -967,10 +967,10 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
> }
>
> /*
> - * We use the convention that exiting with an exit code 10 means that
> - * the bisection process finished successfully.
> - * In this case the calling shell script should exit 0.
> - *
> + * We use the convention that return -10 means the bisection process
> + * finished successfully.
> + * In this case the calling function or command should not turn a -10
> + * return code into an error or a non zero exit code.
> * If no_checkout is non-zero, the bisection process does not
> * checkout the trial commit but instead simply updates BISECT_HEAD.
> */
> @@ -1000,23 +1000,35 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
>
> if (!revs.commits) {
> /*
> - * We should exit here only if the "bad"
> + * We should return error here only if the "bad"
> * commit is also a "skip" commit.
> */
> res = error_if_skipped_commits(tried, NULL);
> if (res)
> - exit(-res);
> + return res;
> printf(_("%s was both %s and %s\n"),
> oid_to_hex(current_bad_oid),
> term_good,
> term_bad);
> - exit(1);
> +
> + /*
> + * We don't want to clean the bisection state
> + * as we need to get back to where we started
> + * by using `git bisect reset`.
> + */
> + return -1;
> }
>
> if (!all) {
> fprintf(stderr, _("No testable commit found.\n"
> "Maybe you started with bad path parameters?\n"));
> - exit(4);
> +
> + /*
> + * We don't want to clean the bisection state
> + * as we need to get back to where we started
> + * by using `git bisect reset`.
> + */
> + return -4;
> }
>
> bisect_rev = &revs.commits->item->object.oid;
> @@ -1024,12 +1036,18 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
> if (oideq(bisect_rev, current_bad_oid)) {
> res = error_if_skipped_commits(tried, current_bad_oid);
> if (res)
> - exit(-res);
> + return res;
> printf("%s is the first %s commit\n", oid_to_hex(bisect_rev),
> term_bad);
> +
> show_diff_tree(r, prefix, revs.commits->item);
> - /* This means the bisection process succeeded. */
> - exit(10);
> + /*
> + * This means the bisection process succeeded.
> + * Using -10 so that the call chain can simply check
> + * for negative return values for early returns up
> + * until the cmd_bisect__helper() caller.
> + */
> + return -10;
> }
>
> nr = all - reaches - 1;
> --
> 2.21.1 (Apple Git-122.3)
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1
2020-01-20 21:41 ` [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Johannes Schindelin
@ 2020-01-20 23:24 ` Christian Couder
2020-01-30 15:12 ` Johannes Schindelin
2020-01-21 8:44 ` Miriam R.
1 sibling, 1 reply; 54+ messages in thread
From: Christian Couder @ 2020-01-20 23:24 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Miriam Rubio, git
Hi Dscho,
On Mon, Jan 20, 2020 at 10:43 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> On Mon, 20 Jan 2020, Miriam Rubio wrote:
> > [1/29] bisect--helper: convert `vocab_*` char pointers to char arrays
> >
> > * New patch to convert `vocab_bad` and `vocab_good` char pointers
> > to char arrays
>
> 29 patches is _a lot_ to review. I would have preferred a series of
> smaller patch series.
Yeah, it's possible to split it into smaller patch series. There are a
many similar patches in the series so it was easier to work on
everything together to make similar and consistent changes to all the
patches at once.
> For example, the first three patches would have made for a fine "some
> cleanups" patch series, from my point of view.
Yeah, but this might then be rejected by Junio as it would be only "code churn".
> Also, as the mail's subject says "part 1", it would be good to have an
> overview how this part fits into the overall story of converting `git
> bisect` into a built-in.
We don't know how the rest will be split yet. Hopefully there will be
only one other smaller patch series after this one.
> Finally, it would be nice to have a link to a public repository with the
> branch from which these mails were generated.
Yeah, I agree that would be nice.
> I will try to review this patch series in its entirety, but it will take
> me a while.
Great, thanks!
Christian.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 06/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents
2020-01-20 21:57 ` Johannes Schindelin
@ 2020-01-21 6:40 ` Christian Couder
2020-01-21 10:00 ` Miriam R.
0 siblings, 1 reply; 54+ messages in thread
From: Christian Couder @ 2020-01-21 6:40 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Miriam Rubio, git, Pranit Bauva, Christian Couder,
Tanushree Tumane
Hi Dscho,
On Mon, Jan 20, 2020 at 10:57 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> On Mon, 20 Jan 2020, Miriam Rubio wrote:
> > printf("There are only 'skip'ped commits left to test.\n"
> > "The first %s commit could be any of:\n", term_bad);
> > @@ -676,7 +676,13 @@ static void exit_if_skipped_commits(struct commit_list *tried,
> > if (bad)
> > printf("%s\n", oid_to_hex(bad));
> > printf(_("We cannot bisect more!\n"));
> > - exit(2);
> > +
> > + /*
> > + * We don't want to clean the bisection state
> > + * as we need to get back to where we started
> > + * by using `git bisect reset`.
> > + */
> > + return -2;
>
> This comment is a good indicator that the constant `-2` here is a "magic"
> number and it would most likely make sense to turn the return type from an
> `int` into an `enum` instead.
Many functions use `return error(...)` and error codes from these
functions and from exit_if_skipped_commits() are going to get mixed.
So I am not sure that using an enum for only some of the error codes
will make things clearer.
> > static int is_expected_rev(const struct object_id *oid)
> > @@ -949,7 +955,7 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
> > {
> > struct rev_info revs;
> > struct commit_list *tried;
> > - int reaches = 0, all = 0, nr, steps;
> > + int reaches = 0, all = 0, nr, steps, res;
> > struct object_id *bisect_rev;
> > char *steps_msg;
> >
> > @@ -972,8 +978,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
> > * We should exit here only if the "bad"
> > * commit is also a "skip" commit.
> > */
> > - exit_if_skipped_commits(tried, NULL);
> > -
> > + res = error_if_skipped_commits(tried, NULL);
> > + if (res)
> > + exit(-res);
>
> So we still `exit()` in `libgit.a`? I hoped for a more thorough
> libification.
The exit() calls are removed in later patches.
> Besides, the `if (res)` probably wants to be an `if (res < 0)`, right?
Yeah, I agree.
Thanks for your review,
Christian.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 09/29] bisect: libify `check_good_are_ancestors_of_bad` and its dependents
2020-01-20 22:20 ` Johannes Schindelin
@ 2020-01-21 6:59 ` Christian Couder
2020-01-21 10:00 ` Miriam R.
0 siblings, 1 reply; 54+ messages in thread
From: Christian Couder @ 2020-01-21 6:59 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Miriam Rubio, git, Pranit Bauva, Christian Couder,
Tanushree Tumane
On Mon, Jan 20, 2020 at 11:20 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> On Mon, 20 Jan 2020, Miriam Rubio wrote:
> > @@ -876,8 +877,15 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
> > int fd, rev_nr, res = 0;
> > struct commit **rev;
> >
> > - if (!current_bad_oid)
> > - die(_("a %s revision is needed"), term_bad);
> > + /*
> > + * We don't want to clean the bisection state
> > + * as we need to get back to where we started
> > + * by using `git bisect reset`.
> > + */
> > + if (!current_bad_oid) {
> > + res = error(_("a %s revision is needed"), term_bad);
> > + goto done;
> > + }
>
> Why not just return here? Ah, there is a `filename` that was allocated...
> it is too bad that we have a mailing-list based review, as the hunk
> context simply cannot be extended in a mail.
>
> Personally, I think it would be nicer to split the definition of
> `filename` from its declaration and move it _after_ this conditional code,
> so that we can `return` right away.
Yeah, I agree.
> However, there is a more pressing issue than that: `die()` exits with exit
> code 128, so in keeping with the idea to hand down negative exit codes as
> return values, should we not assign `res = -128` here?
I think it has been ok when converting git-bisect.sh to C to just
convert `die(...)` into `return error(...)`.
> > /* Check if file BISECT_ANCESTORS_OK exists. */
> > if (!stat(filename, &st) && S_ISREG(st.st_mode))
> > @@ -893,18 +901,20 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
> > if (check_ancestors(r, rev_nr, rev, prefix))
> > res = check_merge_bases(rev_nr, rev, no_checkout);
> > free(rev);
> > - if(res)
> > - exit(res == -11 ? 0 : -res);
> > -
> > - /* Create file BISECT_ANCESTORS_OK. */
> > - fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> > - if (fd < 0)
> > - warning_errno(_("could not create file '%s'"),
> > - filename);
> > - else
> > - close(fd);
> > +
> > + if (!res)
> > + {
>
> We usually put the `{` on the same line as the `if` condition (like you
> did in the `if (!current_bad_oid)` line above.
>
> The rest looks reasonable. Thank you,
Thank you for your review,
Christian.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 10/29] bisect: libify `handle_bad_merge_base` and its dependents
2020-01-20 22:23 ` Johannes Schindelin
@ 2020-01-21 7:05 ` Christian Couder
2020-01-21 10:00 ` Miriam R.
0 siblings, 1 reply; 54+ messages in thread
From: Christian Couder @ 2020-01-21 7:05 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Miriam Rubio, git, Pranit Bauva, Christian Couder,
Tanushree Tumane
On Mon, Jan 20, 2020 at 11:23 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Mon, 20 Jan 2020, Miriam Rubio wrote:
> > @@ -777,14 +777,14 @@ static void handle_bad_merge_base(void)
> > "between %s and [%s].\n"),
> > bad_hex, term_bad, term_good, bad_hex, good_hex);
> > }
> > - exit(3);
> > + return -3;
>
> ... which is: what does `3` stand for?
Maybe the question should have been answered by adding a comment to
the previous patch that added the `exit(3)` statement.
So yeah we could here add a separate patch that just adds such a comment.
Or maybe we can add such a comment in this patch and say something
like "while at it let's explain a bit the '3' error code" in the
commit message.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 11/29] bisect: libify `bisect_next_all`
2020-01-20 22:29 ` Johannes Schindelin
@ 2020-01-21 7:15 ` Christian Couder
2020-01-30 15:18 ` Johannes Schindelin
0 siblings, 1 reply; 54+ messages in thread
From: Christian Couder @ 2020-01-21 7:15 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Miriam Rubio, git, Pranit Bauva, Christian Couder,
Tanushree Tumane
On Mon, Jan 20, 2020 at 11:29 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Mon, 20 Jan 2020, Miriam Rubio wrote:
> > Since we want to get rid of git-bisect.sh it would be necessary to
> > convert those exit() calls to return statements so that errors can be
> > reported.
> >
> > Emulate try catch in C by converting `exit(<positive-value>)` to
> > `return <negative-value>`. Follow POSIX conventions to return
> > <negative-value> to indicate error.
> >
> > Turn `exit()` to `return` calls in `bisect_next_all()`.
> >
> > All the functions calling `bisect_next_all()` are already able to
> > handle return values from it.
>
> So this patch brings more magic values that really would like to become
> `enum` values. At this point, we're talking about `bisect_next_all()`
> which is _not_ a file-local (`static`) function. Therefore, we now really
> wade into API territory where an `enum` is no longer just a nice thing,
> but a necessary thing: `bisect_next_all()` is a function that can be
> called from other source files.
I agree that return values could be better documented. About enum
though, I am perhaps wrong but I don't think that we use them often
for error codes. Do you have examples in the code base where they are
used for such a purpose along with regular `error(...)` calls?
> As far as I can see, this patch concludes the "libify" part of the patch
> series (read: it would have been the second patch series I would have
> suggested to split out from an otherwise too-long-to-be-reviewed set of
> patches, if I had been your mentor),
I am ok with saying that "part 1" stops after this patch and that the
rest will be "part 2" and will not be resent in the version 2 of the
"part 1" patch series, but will rather be resent later, when "part 1"
will be in next for example.
> and I'll leave it at that for
> tonight; Hopefully I will find some time to review more of these patches
> tomorrow.
That would be great!
Thanks,
Christian.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1
2020-01-20 21:41 ` [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Johannes Schindelin
2020-01-20 23:24 ` Christian Couder
@ 2020-01-21 8:44 ` Miriam R.
2020-01-30 15:13 ` Johannes Schindelin
1 sibling, 1 reply; 54+ messages in thread
From: Miriam R. @ 2020-01-21 8:44 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
Hi Johannes,
El lun., 20 ene. 2020 a las 22:41, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi Miriam,
>
> On Mon, 20 Jan 2020, Miriam Rubio wrote:
>
> > --- Changes since Tanushree’s pr117 sent patch series:
> > https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) ---
> >
> > General changes
> > ---------------
> >
> > * Rebase on master branch.
> > * Improve commit messages.
> > * Amend patch series titles.
> > * Reorder commits: first clean-up/preparatory commits, squash or split
> > commits.
>
> Great!
>
> > Specific changes
> > ----------------
> >
> > [1/29] bisect--helper: convert `vocab_*` char pointers to char arrays
> >
> > * New patch to convert `vocab_bad` and `vocab_good` char pointers
> > to char arrays
>
> 29 patches is _a lot_ to review. I would have preferred a series of
> smaller patch series.
Ok, I will discuss with my mentor how is the best way to split all this work.
>
> For example, the first three patches would have made for a fine "some
> cleanups" patch series, from my point of view.
>
> Also, as the mail's subject says "part 1", it would be good to have an
> overview how this part fits into the overall story of converting `git
> bisect` into a built-in.
>
> Finally, it would be nice to have a link to a public repository with the
> branch from which these mails were generated.
>
This is the link of the current branch:
https://gitlab.com/mirucam/git/commits/git-bisect-work2.8.2
I will add the public repository link on next cover-letters.
Thank you for the suggestion.
> I will try to review this patch series in its entirety, but it will take
> me a while.
Thank you very much for reviewing.
Best,
Miriam
>
> Ciao,
> Johannes
>
> >
> > --
> >
> > [2/29] bisect--helper: change `retval` to `res`
> >
> > * Replace one last variable `retval` to `res`.
> >
> > --
> >
> > [3/29] bisect: use the standard 'if (!var)' way to check for 0
> >
> > * New patch to use '!var' and make 'bisect.c' more consistent with the
> > rest of the code
> >
> > --
> >
> > [4/29] run-command: make `exists_in_PATH()` non-static
> >
> > * Add comment before function declaration.
> > * Move function declaration in `run-command.h`.
> >
> > --
> >
> > [6/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*`
> > and its dependents
> >
> > * Fix `mark_edges_uninteresting()` and `show_diff_tree()` calls after
> > rebase on master.
> >
> > --
> >
> > [7/29] bisect: libify `bisect_checkout`
> >
> > * Fix `memcpy()` call after rebase on master.
> > * Introduce `res` variable to return `bisect_checkout()` output.
> > * Fix `get_commit_reference()` declaration after rebase on master.
> >
> > --
> >
> > [8/29] bisect: libify `check_merge_bases` and its dependents
> >
> > State: Previously sent
> >
> > * Fix `check_ancestors()` declaration after rebase on master.
> > * Fix `get_bad_and_good_commits()` call after rebase on master.
> >
> > --
> >
> > [9/29] bisect: libify `check_good_are_ancestors_of_bad` and its
> > dependents
> >
> > State: Previously sent
> >
> > * Fix `check_good_are_ancestors_of_bad()` declaration after rebase on
> > master.
> > * Fix `check_good_are_ancestors_of_bad()`, `bisect_next_all()`
> > and `bisect_rev_setup()` calls after rebase on master.
> >
> > --
> >
> > [11/29] bisect: libify `bisect_next_all`
> >
> > State: Previously sent
> >
> > * Fix `show_diff_tree()` call after rebase on master.
> >
> > --
> >
> > [12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next`
> > shell functions in C
> >
> > * Fix `repo_init_revisions()` and `bisect_next_all()` calls after rebase
> > on master.
> > * Remove `goto` statement in `bisect_skipped_commits()`
> >
> > --
> >
> > [13/29] bisect--helper: finish porting `bisect_start()` to C
> >
> > * Change `return` statement instead of `die` in error handling.
> > * Remove `goto` statements in `bisect_skipped_commits()`.
> >
> > --
> >
> > [21/29] bisect--helper: reimplement `bisect_replay` shell function in C
> >
> > * Add blank line in `get_next_word()`.
> > * Remove `goto` statements in `bisect_replay()`.
> >
> > --
> >
> > [23/29] bisect--helper: use `res` instead of return in BISECT_RESET case
> > option
> >
> > * New patch to split previous commit in two.
> >
> > --
> >
> > [26/29] bisect--helper: reimplement `bisect_skip` shell function in C
> >
> > State: Previously sent
> >
> > * Add blank line.
> >
> > --
> >
> > [28/29] bisect--helper: reimplement `bisect_visualize()`shell function
> > in C
> >
> > New patch:
> >
> > * Reimplement the `bisect_visualize()` shell function in C.
> > * Add `--bisect-visualize` subcommand.
> > * Fix long code line.
> >
> > --
> >
> > [29/29] bisect--helper: reimplement `bisect_run` shell function in C
> >
> > New patch:
> >
> > * Reimplement the `bisect_run()` shell function in C.
> > * Add `--bisect-run` subcommand.
> > * Remove blank line.
> >
> > --
> >
> > Miriam Rubio (2):
> > bisect--helper: convert `vocab_*` char pointers to char arrays
> > bisect: use the standard 'if (!var)' way to check for 0
> >
> > Pranit Bauva (24):
> > run-command: make `exists_in_PATH()` non-static
> > bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and
> > its dependents
> > bisect: libify `bisect_checkout`
> > bisect: libify `check_merge_bases` and its dependents
> > bisect: libify `check_good_are_ancestors_of_bad` and its dependents
> > bisect: libify `handle_bad_merge_base` and its dependents
> > bisect: libify `bisect_next_all`
> > 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_autostart` shell function in C
> > 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: reimplement `bisect_log` shell function in C
> > bisect--helper: reimplement `bisect_replay` shell function in C
> > bisect--helper: retire `--bisect-write` subcommand
> > bisect--helper: use `res` instead of return in BISECT_RESET case
> > option
> > bisect--helper: retire `--bisect-autostart` subcommand
> > bisect--helper: retire `--bisect-auto-next` subcommand
> > bisect--helper: reimplement `bisect_skip` shell function in C
> > bisect--helper: retire `--check-and-set-terms` subcommand
> > bisect--helper: reimplement `bisect_visualize()`shell function in C
> >
> > Tanushree Tumane (3):
> > bisect--helper: change `retval` to `res`
> > bisect--helper: introduce new `decide_next()` function
> > bisect--helper: reimplement `bisect_run` shell function in C
> >
> > bisect.c | 146 +++++---
> > builtin/bisect--helper.c | 776 +++++++++++++++++++++++++++++++++------
> > git-bisect.sh | 279 +-------------
> > run-command.c | 2 +-
> > run-command.h | 11 +
> > 5 files changed, 793 insertions(+), 421 deletions(-)
> >
> > --
> > 2.21.1 (Apple Git-122.3)
> >
> >
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 08/29] bisect: libify `check_merge_bases` and its dependents
2020-01-20 22:09 ` Johannes Schindelin
@ 2020-01-21 9:59 ` Miriam R.
0 siblings, 0 replies; 54+ messages in thread
From: Miriam R. @ 2020-01-21 9:59 UTC (permalink / raw)
To: Johannes Schindelin, git, Christian Couder
Hi,
El lun., 20 ene. 2020 a las 23:09, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi,
>
> On Mon, 20 Jan 2020, Miriam Rubio wrote:
>
> > From: Pranit Bauva <pranit.bauva@gmail.com>
> >
> > Since we want to get rid of git-bisect.sh it would be necessary to
> > convert those exit() calls to return statements so that errors can be
> > reported.
> >
> > Emulate try catch in C by converting `exit(<positive-value>)` to
> > `return <negative-value>`. Follow POSIX conventions to return
> > <negative-value> to indicate error.
> >
> > Turn `exit()` to `return` calls in `check_merge_bases()`.
> >
> > In `check_merge_bases()` there is an early success special case,
> > so we have introduced special error code `-11` which indicates early
> > success. This `-11` is converted back to `0` in `check_good_are_ancestors_of_bad()`.
> >
> > Handle the return value in dependent function `check_good_are_ancestors_of_bad()`.
>
> This is a lot of repeated text from earlier commit messages. It might make
> sense to condense it a bit, and while at it, to remove sentences that
> essentially repeat what the diff says.
>
Ok. Noted!
> >
> > 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 | 21 ++++++++++++++++-----
> > 1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/bisect.c b/bisect.c
> > index 385afaf875..367258b0dd 100644
> > --- a/bisect.c
> > +++ b/bisect.c
> > @@ -806,13 +806,16 @@ static void handle_skipped_merge_base(const struct object_id *mb)
> > * "check_merge_bases" checks that merge bases are not "bad" (or "new").
> > *
> > * - If one is "bad" (or "new"), it means the user assumed something wrong
> > - * and we must exit with a non 0 error code.
> > + * and we must return error with a non 0 error code.
> > * - If one is "good" (or "old"), that's good, we have nothing to do.
> > * - If one is "skipped", we can't know but we should warn.
> > * - If we don't know, we should check it out and ask the user to test.
> > + * - If a merge base must be tested, on success return -11 a special condition
> > + * for early success, this will be converted back to 0 in check_good_are_ancestors_of_bad().
> > */
> > -static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
> > +static int check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
> > {
> > + int res = 0;
> > struct commit_list *result;
> >
> > result = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1);
> > @@ -827,11 +830,16 @@ static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
> > handle_skipped_merge_base(mb);
> > } else {
> > printf(_("Bisecting: a merge base must be tested\n"));
> > - exit(bisect_checkout(mb, no_checkout));
> > + res = bisect_checkout(mb, no_checkout);
> > + if (!res)
> > + /* indicate early success */
> > + res = -11;
>
> This is yet another good candidate for an `enum`.
>
> > + break;
> > }
> > }
> >
> > free_commit_list(result);
> > + return res;
> > }
> >
> > static int check_ancestors(struct repository *r, int rev_nr,
> > @@ -865,7 +873,7 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
> > {
> > char *filename = git_pathdup("BISECT_ANCESTORS_OK");
> > struct stat st;
> > - int fd, rev_nr;
> > + int fd, rev_nr, res = 0;
> > struct commit **rev;
> >
> > if (!current_bad_oid)
> > @@ -880,10 +888,13 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
> > goto done;
> >
> > /* Check if all good revs are ancestor of the bad rev. */
> > +
> > rev = get_bad_and_good_commits(r, &rev_nr);
> > if (check_ancestors(r, rev_nr, rev, prefix))
> > - check_merge_bases(rev_nr, rev, no_checkout);
> > + res = check_merge_bases(rev_nr, rev, no_checkout);
> > free(rev);
> > + if(res)
>
> Please put a space between the `if` keyword and the `(` following it.
Ok.
>
> Thanks,
> Johannes
>
> > + exit(res == -11 ? 0 : -res);
> >
> > /* Create file BISECT_ANCESTORS_OK. */
> > fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> > --
> > 2.21.1 (Apple Git-122.3)
> >
> >
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 06/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents
2020-01-21 6:40 ` Christian Couder
@ 2020-01-21 10:00 ` Miriam R.
0 siblings, 0 replies; 54+ messages in thread
From: Miriam R. @ 2020-01-21 10:00 UTC (permalink / raw)
To: Christian Couder; +Cc: Johannes Schindelin, git
Hi,
El mar., 21 ene. 2020 a las 7:40, Christian Couder
(<christian.couder@gmail.com>) escribió:
>
> Hi Dscho,
>
> On Mon, Jan 20, 2020 at 10:57 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Mon, 20 Jan 2020, Miriam Rubio wrote:
>
> > > printf("There are only 'skip'ped commits left to test.\n"
> > > "The first %s commit could be any of:\n", term_bad);
> > > @@ -676,7 +676,13 @@ static void exit_if_skipped_commits(struct commit_list *tried,
> > > if (bad)
> > > printf("%s\n", oid_to_hex(bad));
> > > printf(_("We cannot bisect more!\n"));
> > > - exit(2);
> > > +
> > > + /*
> > > + * We don't want to clean the bisection state
> > > + * as we need to get back to where we started
> > > + * by using `git bisect reset`.
> > > + */
> > > + return -2;
> >
> > This comment is a good indicator that the constant `-2` here is a "magic"
> > number and it would most likely make sense to turn the return type from an
> > `int` into an `enum` instead.
>
> Many functions use `return error(...)` and error codes from these
> functions and from exit_if_skipped_commits() are going to get mixed.
> So I am not sure that using an enum for only some of the error codes
> will make things clearer.
>
> > > static int is_expected_rev(const struct object_id *oid)
> > > @@ -949,7 +955,7 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
> > > {
> > > struct rev_info revs;
> > > struct commit_list *tried;
> > > - int reaches = 0, all = 0, nr, steps;
> > > + int reaches = 0, all = 0, nr, steps, res;
> > > struct object_id *bisect_rev;
> > > char *steps_msg;
> > >
> > > @@ -972,8 +978,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
> > > * We should exit here only if the "bad"
> > > * commit is also a "skip" commit.
> > > */
> > > - exit_if_skipped_commits(tried, NULL);
> > > -
> > > + res = error_if_skipped_commits(tried, NULL);
> > > + if (res)
> > > + exit(-res);
> >
> > So we still `exit()` in `libgit.a`? I hoped for a more thorough
> > libification.
>
> The exit() calls are removed in later patches.
>
> > Besides, the `if (res)` probably wants to be an `if (res < 0)`, right?
>
> Yeah, I agree.
>
Noted!
Thank you Johannes and Christian.
> Thanks for your review,
> Christian.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 09/29] bisect: libify `check_good_are_ancestors_of_bad` and its dependents
2020-01-21 6:59 ` Christian Couder
@ 2020-01-21 10:00 ` Miriam R.
0 siblings, 0 replies; 54+ messages in thread
From: Miriam R. @ 2020-01-21 10:00 UTC (permalink / raw)
To: Christian Couder, git, Johannes Schindelin
Hi,
El mar., 21 ene. 2020 a las 7:59, Christian Couder
(<christian.couder@gmail.com>) escribió:
>
> On Mon, Jan 20, 2020 at 11:20 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Mon, 20 Jan 2020, Miriam Rubio wrote:
>
> > > @@ -876,8 +877,15 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
> > > int fd, rev_nr, res = 0;
> > > struct commit **rev;
> > >
> > > - if (!current_bad_oid)
> > > - die(_("a %s revision is needed"), term_bad);
> > > + /*
> > > + * We don't want to clean the bisection state
> > > + * as we need to get back to where we started
> > > + * by using `git bisect reset`.
> > > + */
> > > + if (!current_bad_oid) {
> > > + res = error(_("a %s revision is needed"), term_bad);
> > > + goto done;
> > > + }
> >
> > Why not just return here? Ah, there is a `filename` that was allocated...
> > it is too bad that we have a mailing-list based review, as the hunk
> > context simply cannot be extended in a mail.
> >
> > Personally, I think it would be nicer to split the definition of
> > `filename` from its declaration and move it _after_ this conditional code,
> > so that we can `return` right away.
>
> Yeah, I agree.
Ok. Noted.
>
> > However, there is a more pressing issue than that: `die()` exits with exit
> > code 128, so in keeping with the idea to hand down negative exit codes as
> > return values, should we not assign `res = -128` here?
>
> I think it has been ok when converting git-bisect.sh to C to just
> convert `die(...)` into `return error(...)`.
>
> > > /* Check if file BISECT_ANCESTORS_OK exists. */
> > > if (!stat(filename, &st) && S_ISREG(st.st_mode))
> > > @@ -893,18 +901,20 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
> > > if (check_ancestors(r, rev_nr, rev, prefix))
> > > res = check_merge_bases(rev_nr, rev, no_checkout);
> > > free(rev);
> > > - if(res)
> > > - exit(res == -11 ? 0 : -res);
> > > -
> > > - /* Create file BISECT_ANCESTORS_OK. */
> > > - fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> > > - if (fd < 0)
> > > - warning_errno(_("could not create file '%s'"),
> > > - filename);
> > > - else
> > > - close(fd);
> > > +
> > > + if (!res)
> > > + {
> >
> > We usually put the `{` on the same line as the `if` condition (like you
> > did in the `if (!current_bad_oid)` line above.
Ok. I will change that.
> >
> > The rest looks reasonable. Thank you,
Great! Thank you for your review!
>
> Thank you for your review,
> Christian.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 10/29] bisect: libify `handle_bad_merge_base` and its dependents
2020-01-21 7:05 ` Christian Couder
@ 2020-01-21 10:00 ` Miriam R.
0 siblings, 0 replies; 54+ messages in thread
From: Miriam R. @ 2020-01-21 10:00 UTC (permalink / raw)
To: Christian Couder, git, Johannes Schindelin
Hi,
El mar., 21 ene. 2020 a las 8:05, Christian Couder
(<christian.couder@gmail.com>) escribió:
>
> On Mon, Jan 20, 2020 at 11:23 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > On Mon, 20 Jan 2020, Miriam Rubio wrote:
>
> > > @@ -777,14 +777,14 @@ static void handle_bad_merge_base(void)
> > > "between %s and [%s].\n"),
> > > bad_hex, term_bad, term_good, bad_hex, good_hex);
> > > }
> > > - exit(3);
> > > + return -3;
> >
> > ... which is: what does `3` stand for?
>
> Maybe the question should have been answered by adding a comment to
> the previous patch that added the `exit(3)` statement.
>
> So yeah we could here add a separate patch that just adds such a comment.
>
> Or maybe we can add such a comment in this patch and say something
> like "while at it let's explain a bit the '3' error code" in the
> commit message.
I like most your first option because we explain the code in the patch
where it is added, but the second one is also ok for me :).
Thanks.
Best,
Miriam
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1
2020-01-20 23:24 ` Christian Couder
@ 2020-01-30 15:12 ` Johannes Schindelin
2020-01-30 21:12 ` Junio C Hamano
0 siblings, 1 reply; 54+ messages in thread
From: Johannes Schindelin @ 2020-01-30 15:12 UTC (permalink / raw)
To: Christian Couder; +Cc: Miriam Rubio, git
Hi Chris,
On Tue, 21 Jan 2020, Christian Couder wrote:
> On Mon, Jan 20, 2020 at 10:43 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Mon, 20 Jan 2020, Miriam Rubio wrote:
>
> > > [1/29] bisect--helper: convert `vocab_*` char pointers to char arrays
> > >
> > > * New patch to convert `vocab_bad` and `vocab_good` char pointers
> > > to char arrays
> >
> > 29 patches is _a lot_ to review. I would have preferred a series of
> > smaller patch series.
>
> Yeah, it's possible to split it into smaller patch series. There are a
> many similar patches in the series so it was easier to work on
> everything together to make similar and consistent changes to all the
> patches at once.
>
> > For example, the first three patches would have made for a fine "some
> > cleanups" patch series, from my point of view.
>
> Yeah, but this might then be rejected by Junio as it would be only "code
> churn".
I don't think so. We have ample "prior art" where a patch series came with
a wonderful cover letter that explained why such a cleanup prepares for a
later patch series rather than being code churn.
In general, it appears to me that cleanups are much appreciated over here
rather than discouraged.
I also have to admit that I can justify _a lot better_ to set aside time
for reviewing a tiny patch series consisting of three or so pure cleanups
on one day, and then a now-smaller patch series to libify `bisect.c` on
another day. Much easier to justify than trying to find the time to read
through one honking 29-strong patch series, where realistically I have to
re-read all of the patches upon the next iteration because I filled my
brain with so many other things in the meantime.
I think that it would be good mentor advice to pass along: not only the
commits are ideally so small as to be _trivial_ to review, also the patch
series should be structured in such a way. The scarce resource is the
reviewer time, after all.
> > Also, as the mail's subject says "part 1", it would be good to have an
> > overview how this part fits into the overall story of converting `git
> > bisect` into a built-in.
>
> We don't know how the rest will be split yet. Hopefully there will be
> only one other smaller patch series after this one.
That is a wonderful contradiction: first you say that you cannot know, and
then you state that there will be only one patch series after this ;-)
Seriously again, nobody will hold you to a statement like "this is the
first of three patch series" when you later explain "originally, I
intended this patch series to be the final one, but decided to split it
even further, to make it easier on reviewers" or some such.
But done is done, v2 was a nice read.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1
2020-01-21 8:44 ` Miriam R.
@ 2020-01-30 15:13 ` Johannes Schindelin
0 siblings, 0 replies; 54+ messages in thread
From: Johannes Schindelin @ 2020-01-30 15:13 UTC (permalink / raw)
To: Miriam R.; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 545 bytes --]
Hi Miriam,
On Tue, 21 Jan 2020, Miriam R. wrote:
> El lun., 20 ene. 2020 a las 22:41, Johannes Schindelin
> (<Johannes.Schindelin@gmx.de>) escribió:
> >
> > Finally, it would be nice to have a link to a public repository with the
> > branch from which these mails were generated.
> >
>
> This is the link of the current branch:
> https://gitlab.com/mirucam/git/commits/git-bisect-work2.8.2
> I will add the public repository link on next cover-letters.
> Thank you for the suggestion.
Excellent, thank you very much!
Dscho
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 11/29] bisect: libify `bisect_next_all`
2020-01-21 7:15 ` Christian Couder
@ 2020-01-30 15:18 ` Johannes Schindelin
0 siblings, 0 replies; 54+ messages in thread
From: Johannes Schindelin @ 2020-01-30 15:18 UTC (permalink / raw)
To: Christian Couder
Cc: Miriam Rubio, git, Pranit Bauva, Christian Couder,
Tanushree Tumane
Hi Chris,
On Tue, 21 Jan 2020, Christian Couder wrote:
> On Mon, Jan 20, 2020 at 11:29 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > On Mon, 20 Jan 2020, Miriam Rubio wrote:
>
> > > Since we want to get rid of git-bisect.sh it would be necessary to
> > > convert those exit() calls to return statements so that errors can be
> > > reported.
> > >
> > > Emulate try catch in C by converting `exit(<positive-value>)` to
> > > `return <negative-value>`. Follow POSIX conventions to return
> > > <negative-value> to indicate error.
> > >
> > > Turn `exit()` to `return` calls in `bisect_next_all()`.
> > >
> > > All the functions calling `bisect_next_all()` are already able to
> > > handle return values from it.
> >
> > So this patch brings more magic values that really would like to become
> > `enum` values. At this point, we're talking about `bisect_next_all()`
> > which is _not_ a file-local (`static`) function. Therefore, we now really
> > wade into API territory where an `enum` is no longer just a nice thing,
> > but a necessary thing: `bisect_next_all()` is a function that can be
> > called from other source files.
>
> I agree that return values could be better documented. About enum
> though, I am perhaps wrong but I don't think that we use them often
> for error codes.
It does not matter how often we actually use them, it matters more whether
we _want_ to use them. And in the recent years, we have definitely made
more use of enums than before, to allow the compiler to catch mistakes
earlier. We even started to convert existing constants to enums, for
example.
So I don't think that it is sound advice here to point to the code base.
If you _must_ value the existing practice over a clearly communicated
review, then please look no further than at the declaration of
`safe_create_leading_directories()`.
> Do you have examples in the code base where they are used for such a
> purpose along with regular `error(...)` calls?
It's all in the code. You don't need me to read it aloud for you.
:-)
Ciao,
Dscho
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1
2020-01-30 15:12 ` Johannes Schindelin
@ 2020-01-30 21:12 ` Junio C Hamano
0 siblings, 0 replies; 54+ messages in thread
From: Junio C Hamano @ 2020-01-30 21:12 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Christian Couder, Miriam Rubio, git
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> In general, it appears to me that cleanups are much appreciated over here
> rather than discouraged.
Yup.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C
2020-01-20 14:37 ` [PATCH 12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C Miriam Rubio
@ 2020-01-30 22:47 ` Johannes Schindelin
2020-01-31 10:53 ` Miriam R.
2020-02-17 7:20 ` Christian Couder
0 siblings, 2 replies; 54+ messages in thread
From: Johannes Schindelin @ 2020-01-30 22:47 UTC (permalink / raw)
To: Miriam Rubio
Cc: git, Pranit Bauva, Lars Schneider, Christian Couder,
Tanushree Tumane
Hi Miriam,
I started looking at this patch, and will just send the comments, but
please note that I would not mind at all leaving the review for later,
when the libifying patches that you kept in v2 (and probably will send out
a v3 for) made it into `next` and you send the remainder as a new patch
series.
On Mon, 20 Jan 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 .
>
> Using `--bisect-next` and `--bisect-auto-start` 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-start`
> subcommand will be retired and will be called by some other methods.
This still sounds clear enough.
> 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 | 10 +++
> builtin/bisect--helper.c | 174 ++++++++++++++++++++++++++++++++++++++-
> git-bisect.sh | 47 ++---------
> 3 files changed, 188 insertions(+), 43 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index 33f2829c19..1c13da8a28 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -635,6 +635,12 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,
> struct argv_array rev_argv = ARGV_ARRAY_INIT;
> int i;
>
> + /*
> + * Since the code is slowly being converted to C, there might be
> + * instances where the revisions were initialized before. Thus
> + * we first need to reset it.
> + */
This comment sounds good right now, but is prone to get stale rather
quickly.
But that's actually remedied rather easily: if the comment is reworded
only slightly, to avoid talking about the conversion process, and to
mention instead that `revs` could have been used before, then we're
golden.
> + reset_revision_walk();
> repo_init_revisions(r, revs, prefix);
> revs->abbrev = 0;
> revs->commit_format = CMIT_FMT_UNSPECIFIED;
> @@ -971,6 +977,10 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
> * finished successfully.
> * In this case the calling function or command should not turn a -10
> * return code into an error or a non zero exit code.
I'd like to have an empty line here (well, a line that only contains an
indented `*`).
> + * This returned -10 is checked in bisect_helper::bisect_next() and
> + * eventually transformed to 0 at the end of
> + * bisect_helper::cmd_bisect__helper().
This says _what_ it does. But not why. I would contend that it is much
more important to know what role the `-10` serves than explaining where
the role is acted out.
> + *
> * If no_checkout is non-zero, the bisection process does not
> * checkout the trial commit but instead simply updates BISECT_HEAD.
> */
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 5e0f759d50..29bbc1573b 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"),
> NULL
> };
>
> @@ -421,6 +424,157 @@ 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 string_list *good_refs = cb_data;
> + string_list_append(good_refs, oid_to_hex(oid));
> + return 0;
> +}
> +
> +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)
> +{
> + struct string_list good_revs = STRING_LIST_INIT_DUP;
> + char *term_good = xstrfmt("%s-*", terms->term_good);
> +
> + for_each_glob_ref_in(register_good_ref, term_good,
> + "refs/bisect/", &good_revs);
> +
> + argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);
> + for (int i = 0; i < good_revs.nr; i++)
> + argv_array_push(rev_argv, good_revs.items[i].string);
> +
> + string_list_clear(&good_revs, 0);
> + free(term_good);
> +}
Maybe we should fold that into `prepare_revs()`? We could then render the
arguments directly into `revs` (via `add_pending_object()`, after setting
obj->flags |= UNINTERESTING`) rather than formatting them into a string
list, then deep-copy them into an `argv_array` only to parse them back
into OIDs that we already had in the first place.
> +
> +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
> + * setups 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") < 1)
> + return -1;
> +
> + 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);
> + }
In the interest of allowing further revision walks, we will probably need
to re-set the flags via `clear_commit_marks()`, just like
`check_ancestors()` does.
> +
> + 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;
> +}
This is again a very short wrapper around another function, so it will
probably make sense to merge the two, otherwise the boilerplate might very
well outweigh the actual code doing actual work.
> +
> +static int bisect_successful(struct bisect_terms *terms)
> +{
> + FILE *fp = NULL;
> + 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 = 0;
> +
> + 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);
> +
There is a trailing tab here. Maybe it would make sense to check the
patches via `git log --check`?
> + fp = fopen(git_path_bisect_log(), "a");
> + if (fp) {
> + if (fprintf(fp, "# first %s commit: [%s] %s\n",
> + terms->term_bad, oid_to_hex(&oid),
> + commit_name.buf) < 1)
> + res = -1;
This would probably do with an error message, i.e. `res =
error_errno(...);`
> + fclose(fp);
> + } else {
> + res = error_errno(_("could not open '%s' for "
> + "appending"),
> + git_path_bisect_log());
> + }
This pattern of opening a file, writing something into it, and then return
success, otherwise failure, seems like a repeated pattern. In other words,
it would be a good candidate for factoring out into its own function.
> + strbuf_release(&commit_name);
> + free(bad_ref);
> + return res;
> +}
> +
> +static int bisect_next(struct bisect_terms *terms, const char *prefix)
> +{
> + int res, no_checkout;
> +
> + if (bisect_next_check(terms, terms->term_good))
> + return -1;
> +
> + no_checkout = !is_empty_or_missing_file(git_path_bisect_head());
> +
> + /* Perform all bisection computation, display and checkout */
> + res = bisect_next_all(the_repository, prefix, no_checkout);
> +
> + if (res == -10) {
> + res = bisect_successful(terms);
> + return res ? res : -11;
> + } else if (res == -2) {
> + res = bisect_skipped_commits(terms);
> + return res ? res : -2;
> + }
I know exactly what I'll think if I see those constants six months from
now, when I forgot most of the details of our conversation over here. A
-10 means.. wait, what?
Seriously, it is quite bad to keep those constants unexplained.
In contrast, look at this here code:
enum scld_error {
SCLD_OK = 0,
SCLD_FAILED = -1,
SCLD_PERMS = -2,
SCLD_EXISTS = -3,
SCLD_VANISHED = -4
};
enum scld_error safe_create_leading_directories(char *path);
What do you think? Will any reader stumble over this and say "what the
heck is going on? What are these return values even _supposed_ to mean?"?
Even better, it seems as if modern debuggers can figure out that a value
-4 returned from `safe_create_leading_directories()` actually mean
`SCLD_VANISHED` and display that to the user.
So armed with this example, you could of course go back to your mentor and
ask for permission to change the bisect code accordingly.
You could also just decide on your own that this is what you want to do
because it is so much more elegant, anyway.
> + return res;
> +}
> +
> +static int bisect_auto_next(struct bisect_terms *terms, const char *prefix)
> +{
> + if (!bisect_next_check(terms, NULL))
> + return bisect_next(terms, prefix);
> +
> + return 0;
A common pattern in Git's source code is to present the early return
first, i.e.
if (bisect_next_check(terms, NULL))
return 0;
return bisect_next(terms, prefix);
I do find it easier to read that way, too.
> +
> static int bisect_start(struct bisect_terms *terms, int no_checkout,
> const char **argv, int argc)
> {
> @@ -625,7 +779,9 @@ 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_NEXT,
> + BISECT_AUTO_NEXT,
> } cmdmode = 0;
> int no_checkout = 0, res = 0, nolog = 0;
> struct option options[] = {
> @@ -649,6 +805,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_BOOL(0, "no-checkout", &no_checkout,
> N_("update BISECT_HEAD instead of checking out the current commit")),
> OPT_BOOL(0, "no-log", &nolog,
> @@ -710,6 +870,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;
> default:
> return error("BUG: unknown subcommand '%d'", cmdmode);
> }
> diff --git a/git-bisect.sh b/git-bisect.sh
> index efee12b8b1..7531b74708 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -87,7 +87,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
> }
> @@ -140,45 +140,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
> - 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
Beautiful.
> }
>
> bisect_visualize() {
> @@ -232,7 +194,7 @@ bisect_replay () {
> die "$(gettext "?? what are you talking about?")" ;;
> esac
> done <"$file"
> - bisect_auto_next
> + git bisect--helper --bisect-auto-next
> }
>
> bisect_run () {
> @@ -329,7 +291,8 @@ case "$#" in
> bisect_skip "$@" ;;
> next)
> # Not sure we want "next" at the UI level anymore.
> - bisect_next "$@" ;;
> + get_terms
I vaguely remember that we talked about this, or at least about a similar
scenario. It needs to be explained in the commit message why we need to
call `get_terms` here when previously, we did not.
Of course, after thinking about this and looking around for a couple of
minutes, I know why. My point is that I, or for that matter, any reader of
this commit, should not need to repeat that analysis.
Other than that, the patch looks good.
As I said, I will stop reviewing the remainder of this patch series, as it
has been removed from v2 and will probably be presented as a follow-up
patch series soon.
Thanks,
Dscho
> + git bisect--helper --bisect-next "$@" || exit ;;
> visualize|view)
> bisect_visualize "$@" ;;
> reset)
> --
> 2.21.1 (Apple Git-122.3)
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C
2020-01-30 22:47 ` Johannes Schindelin
@ 2020-01-31 10:53 ` Miriam R.
2020-02-17 7:20 ` Christian Couder
1 sibling, 0 replies; 54+ messages in thread
From: Miriam R. @ 2020-01-31 10:53 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
Hi,
El jue., 30 ene. 2020 a las 23:47, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi Miriam,
>
> I started looking at this patch, and will just send the comments, but
> please note that I would not mind at all leaving the review for later,
> when the libifying patches that you kept in v2 (and probably will send out
> a v3 for) made it into `next` and you send the remainder as a new patch
> series.
Yes, sure. Thank you, Johannes.
>
> On Mon, 20 Jan 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 .
> >
> > Using `--bisect-next` and `--bisect-auto-start` 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-start`
> > subcommand will be retired and will be called by some other methods.
>
> This still sounds clear enough.
>
> > 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 | 10 +++
> > builtin/bisect--helper.c | 174 ++++++++++++++++++++++++++++++++++++++-
> > git-bisect.sh | 47 ++---------
> > 3 files changed, 188 insertions(+), 43 deletions(-)
> >
> > diff --git a/bisect.c b/bisect.c
> > index 33f2829c19..1c13da8a28 100644
> > --- a/bisect.c
> > +++ b/bisect.c
> > @@ -635,6 +635,12 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,
> > struct argv_array rev_argv = ARGV_ARRAY_INIT;
> > int i;
> >
> > + /*
> > + * Since the code is slowly being converted to C, there might be
> > + * instances where the revisions were initialized before. Thus
> > + * we first need to reset it.
> > + */
>
> This comment sounds good right now, but is prone to get stale rather
> quickly.
>
> But that's actually remedied rather easily: if the comment is reworded
> only slightly, to avoid talking about the conversion process, and to
> mention instead that `revs` could have been used before, then we're
> golden.
Ok
>
> > + reset_revision_walk();
> > repo_init_revisions(r, revs, prefix);
> > revs->abbrev = 0;
> > revs->commit_format = CMIT_FMT_UNSPECIFIED;
> > @@ -971,6 +977,10 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
> > * finished successfully.
> > * In this case the calling function or command should not turn a -10
> > * return code into an error or a non zero exit code.
>
> I'd like to have an empty line here (well, a line that only contains an
> indented `*`).
Noted.
>
> > + * This returned -10 is checked in bisect_helper::bisect_next() and
> > + * eventually transformed to 0 at the end of
> > + * bisect_helper::cmd_bisect__helper().
>
> This says _what_ it does. But not why. I would contend that it is much
> more important to know what role the `-10` serves than explaining where
> the role is acted out.
Aha.
>
> > + *
> > * If no_checkout is non-zero, the bisection process does not
> > * checkout the trial commit but instead simply updates BISECT_HEAD.
> > */
> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> > index 5e0f759d50..29bbc1573b 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"),
> > NULL
> > };
> >
> > @@ -421,6 +424,157 @@ 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 string_list *good_refs = cb_data;
> > + string_list_append(good_refs, oid_to_hex(oid));
> > + return 0;
> > +}
> > +
> > +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)
> > +{
> > + struct string_list good_revs = STRING_LIST_INIT_DUP;
> > + char *term_good = xstrfmt("%s-*", terms->term_good);
> > +
> > + for_each_glob_ref_in(register_good_ref, term_good,
> > + "refs/bisect/", &good_revs);
> > +
> > + argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);
> > + for (int i = 0; i < good_revs.nr; i++)
> > + argv_array_push(rev_argv, good_revs.items[i].string);
> > +
> > + string_list_clear(&good_revs, 0);
> > + free(term_good);
> > +}
>
> Maybe we should fold that into `prepare_revs()`? We could then render the
> arguments directly into `revs` (via `add_pending_object()`, after setting
> obj->flags |= UNINTERESTING`) rather than formatting them into a string
> list, then deep-copy them into an `argv_array` only to parse them back
> into OIDs that we already had in the first place.
>
> > +
> > +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
> > + * setups 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") < 1)
> > + return -1;
> > +
> > + 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);
> > + }
>
> In the interest of allowing further revision walks, we will probably need
> to re-set the flags via `clear_commit_marks()`, just like
> `check_ancestors()` does.
>
> > +
> > + 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;
> > +}
>
> This is again a very short wrapper around another function, so it will
> probably make sense to merge the two, otherwise the boilerplate might very
> well outweigh the actual code doing actual work.
>
> > +
> > +static int bisect_successful(struct bisect_terms *terms)
> > +{
> > + FILE *fp = NULL;
> > + 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 = 0;
> > +
> > + 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);
> > +
>
> There is a trailing tab here. Maybe it would make sense to check the
> patches via `git log --check`?
These extra trailing tabs are removed in my working branch :)
This is my last branch:
https://gitlab.com/mirucam/git/commits/git-bisect-work3.3.1.
>
> > + fp = fopen(git_path_bisect_log(), "a");
> > + if (fp) {
> > + if (fprintf(fp, "# first %s commit: [%s] %s\n",
> > + terms->term_bad, oid_to_hex(&oid),
> > + commit_name.buf) < 1)
> > + res = -1;
>
> This would probably do with an error message, i.e. `res =
> error_errno(...);`
>
> > + fclose(fp);
> > + } else {
> > + res = error_errno(_("could not open '%s' for "
> > + "appending"),
> > + git_path_bisect_log());
> > + }
>
> This pattern of opening a file, writing something into it, and then return
> success, otherwise failure, seems like a repeated pattern. In other words,
> it would be a good candidate for factoring out into its own function.
>
> > + strbuf_release(&commit_name);
> > + free(bad_ref);
> > + return res;
> > +}
> > +
> > +static int bisect_next(struct bisect_terms *terms, const char *prefix)
> > +{
> > + int res, no_checkout;
> > +
> > + if (bisect_next_check(terms, terms->term_good))
> > + return -1;
> > +
> > + no_checkout = !is_empty_or_missing_file(git_path_bisect_head());
> > +
> > + /* Perform all bisection computation, display and checkout */
> > + res = bisect_next_all(the_repository, prefix, no_checkout);
> > +
> > + if (res == -10) {
> > + res = bisect_successful(terms);
> > + return res ? res : -11;
> > + } else if (res == -2) {
> > + res = bisect_skipped_commits(terms);
> > + return res ? res : -2;
> > + }
>
> I know exactly what I'll think if I see those constants six months from
> now, when I forgot most of the details of our conversation over here. A
> -10 means.. wait, what?
>
> Seriously, it is quite bad to keep those constants unexplained.
>
> In contrast, look at this here code:
>
> enum scld_error {
> SCLD_OK = 0,
> SCLD_FAILED = -1,
> SCLD_PERMS = -2,
> SCLD_EXISTS = -3,
> SCLD_VANISHED = -4
> };
> enum scld_error safe_create_leading_directories(char *path);
>
> What do you think? Will any reader stumble over this and say "what the
> heck is going on? What are these return values even _supposed_ to mean?"?
>
> Even better, it seems as if modern debuggers can figure out that a value
> -4 returned from `safe_create_leading_directories()` actually mean
> `SCLD_VANISHED` and display that to the user.
>
> So armed with this example, you could of course go back to your mentor and
> ask for permission to change the bisect code accordingly.
>
> You could also just decide on your own that this is what you want to do
> because it is so much more elegant, anyway.
>
> > + return res;
> > +}
> > +
> > +static int bisect_auto_next(struct bisect_terms *terms, const char *prefix)
> > +{
> > + if (!bisect_next_check(terms, NULL))
> > + return bisect_next(terms, prefix);
> > +
> > + return 0;
>
> A common pattern in Git's source code is to present the early return
> first, i.e.
>
> if (bisect_next_check(terms, NULL))
> return 0;
>
> return bisect_next(terms, prefix);
>
> I do find it easier to read that way, too.
>
> > +
> > static int bisect_start(struct bisect_terms *terms, int no_checkout,
> > const char **argv, int argc)
> > {
> > @@ -625,7 +779,9 @@ 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_NEXT,
> > + BISECT_AUTO_NEXT,
> > } cmdmode = 0;
> > int no_checkout = 0, res = 0, nolog = 0;
> > struct option options[] = {
> > @@ -649,6 +805,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_BOOL(0, "no-checkout", &no_checkout,
> > N_("update BISECT_HEAD instead of checking out the current commit")),
> > OPT_BOOL(0, "no-log", &nolog,
> > @@ -710,6 +870,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;
> > default:
> > return error("BUG: unknown subcommand '%d'", cmdmode);
> > }
> > diff --git a/git-bisect.sh b/git-bisect.sh
> > index efee12b8b1..7531b74708 100755
> > --- a/git-bisect.sh
> > +++ b/git-bisect.sh
> > @@ -87,7 +87,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
> > }
> > @@ -140,45 +140,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
> > - 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
>
> Beautiful.
>
> > }
> >
> > bisect_visualize() {
> > @@ -232,7 +194,7 @@ bisect_replay () {
> > die "$(gettext "?? what are you talking about?")" ;;
> > esac
> > done <"$file"
> > - bisect_auto_next
> > + git bisect--helper --bisect-auto-next
> > }
> >
> > bisect_run () {
> > @@ -329,7 +291,8 @@ case "$#" in
> > bisect_skip "$@" ;;
> > next)
> > # Not sure we want "next" at the UI level anymore.
> > - bisect_next "$@" ;;
> > + get_terms
>
> I vaguely remember that we talked about this, or at least about a similar
> scenario. It needs to be explained in the commit message why we need to
> call `get_terms` here when previously, we did not.
>
> Of course, after thinking about this and looking around for a couple of
> minutes, I know why. My point is that I, or for that matter, any reader of
> this commit, should not need to repeat that analysis.
>
> Other than that, the patch looks good.
>
For the rest of your suggestions I haven't answered, I will wait to my
mentor opinion first. :)
Thank you.
> As I said, I will stop reviewing the remainder of this patch series, as it
> has been removed from v2 and will probably be presented as a follow-up
> patch series soon.
>
Yes, they will be sent as soon as the part1 is in `next`:).
Best,
Miriam
> Thanks,
> Dscho
>
> > + git bisect--helper --bisect-next "$@" || exit ;;
> > visualize|view)
> > bisect_visualize "$@" ;;
> > reset)
> > --
> > 2.21.1 (Apple Git-122.3)
> >
> >
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C
2020-01-30 22:47 ` Johannes Schindelin
2020-01-31 10:53 ` Miriam R.
@ 2020-02-17 7:20 ` Christian Couder
2020-02-17 22:00 ` Johannes Schindelin
1 sibling, 1 reply; 54+ messages in thread
From: Christian Couder @ 2020-02-17 7:20 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Miriam Rubio, git, Pranit Bauva, Lars Schneider, Christian Couder,
Tanushree Tumane
Hi Dscho,
Thanks for your review! I agree with everything you said except a few
things below.
On Thu, Jan 30, 2020 at 11:47 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> On Mon, 20 Jan 2020, Miriam Rubio wrote:
> > +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)> > +{
> > + struct string_list good_revs = STRING_LIST_INIT_DUP;
> > + char *term_good = xstrfmt("%s-*", terms->term_good);
> > +
> > + for_each_glob_ref_in(register_good_ref, term_good,
> > + "refs/bisect/", &good_revs);
> > +
> > + argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);
> > + for (int i = 0; i < good_revs.nr; i++)
> > + argv_array_push(rev_argv, good_revs.items[i].string);
> > +
> > + string_list_clear(&good_revs, 0);
> > + free(term_good);
> > +}
>
> Maybe we should fold that into `prepare_revs()`? We could then render the
> arguments directly into `revs` (via `add_pending_object()`, after setting
> obj->flags |= UNINTERESTING`) rather than formatting them into a string
> list, then deep-copy them into an `argv_array` only to parse them back
> into OIDs that we already had in the first place.
The current code is a straightforward port from shell. If we do what
you suggest, yeah, it will be less wasteful, but on the other hand it
will be less easy to see that we are doing a good job of properly
porting code from shell. I suggest we try to focus on the later rather
than the former right now, especially as performance is not very
important here. Further improvements on top could be left for another
patch series or perhaps as microprojects for GSoC or Outreachy
applicants.
Using small functions also makes it easy to see that we are properly
releasing memory. A previous version of this code had everything into
a big function that used goto statements and it was less clear that we
released everything.
> > +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;
> > +}
>
> This is again a very short wrapper around another function, so it will
> probably make sense to merge the two, otherwise the boilerplate might very
> well outweigh the actual code doing actual work.
Yeah, there is perhaps a significant amount of boiler plate, but the
code is much easier to check for leaks than when everything was in the
same big function and there were goto statements, so I think it's a
reasonable trade-off
> > + fclose(fp);
> > + } else {
> > + res = error_errno(_("could not open '%s' for "
> > + "appending"),
> > + git_path_bisect_log());
> > + }
>
> This pattern of opening a file, writing something into it, and then return
> success, otherwise failure, seems like a repeated pattern. In other words,
> it would be a good candidate for factoring out into its own function.
Yeah, but it seems that in this patch series we use the pattern only
once. So I think it's fair to leave that for another patch series with
cleanups and performance improvements or perhaps for microprojects.
Thanks,
Christian.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C
2020-02-17 7:20 ` Christian Couder
@ 2020-02-17 22:00 ` Johannes Schindelin
0 siblings, 0 replies; 54+ messages in thread
From: Johannes Schindelin @ 2020-02-17 22:00 UTC (permalink / raw)
To: Christian Couder
Cc: Miriam Rubio, git, Pranit Bauva, Lars Schneider, Christian Couder,
Tanushree Tumane
Hi Chris,
On Mon, 17 Feb 2020, Christian Couder wrote:
> On Thu, Jan 30, 2020 at 11:47 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Mon, 20 Jan 2020, Miriam Rubio wrote:
>
> > > +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)> > +{
> > > + struct string_list good_revs = STRING_LIST_INIT_DUP;
> > > + char *term_good = xstrfmt("%s-*", terms->term_good);
> > > +
> > > + for_each_glob_ref_in(register_good_ref, term_good,
> > > + "refs/bisect/", &good_revs);
> > > +
> > > + argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);
> > > + for (int i = 0; i < good_revs.nr; i++)
> > > + argv_array_push(rev_argv, good_revs.items[i].string);
> > > +
> > > + string_list_clear(&good_revs, 0);
> > > + free(term_good);
> > > +}
> >
> > Maybe we should fold that into `prepare_revs()`? We could then render the
> > arguments directly into `revs` (via `add_pending_object()`, after setting
> > obj->flags |= UNINTERESTING`) rather than formatting them into a string
> > list, then deep-copy them into an `argv_array` only to parse them back
> > into OIDs that we already had in the first place.
>
> The current code is a straightforward port from shell. If we do what
> you suggest, yeah, it will be less wasteful, but on the other hand it
> will be less easy to see that we are doing a good job of properly
> porting code from shell.
If you reason that way, you will have to use tons of `run_command()` calls
to translate the shell code as verbatim as possible.
However, as you can see from our commit history, we do not do that.
Instead, we use the more powerful expressiveness of C to come up with more
elegant code than to slavishly convert shell code to inelegant C code.
> I suggest we try to focus on the later rather than the former right now,
> especially as performance is not very important here.
Oh, but my comment was totally not about performance, and pretty much
exclusively about readability.
If Miriam goes with my suggestion, it will result in more readable code
that is easier to review and therefore much more likely to be free of
unintentional bugs.
> Using small functions also makes it easy to see that we are properly
> releasing memory. A previous version of this code had everything into
> a big function that used goto statements and it was less clear that we
> released everything.
If you want to make it easier to avoid double-free()s and memory leaks, I
am a bit puzzled how you want to claim that the current "we're copying the
strings so often that pretty much everybody loses track of them" approach
should be superior to adding the strings once, and once only, to a string
array.
> > > +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;
> > > +}
> >
> > This is again a very short wrapper around another function, so it will
> > probably make sense to merge the two, otherwise the boilerplate might very
> > well outweigh the actual code doing actual work.
>
> Yeah, there is perhaps a significant amount of boiler plate, but the
> code is much easier to check for leaks than when everything was in the
> same big function and there were goto statements, so I think it's a
> reasonable trade-off
Given this snippet, I would strongly disagree with this assessment:
fp = fopen(git_path_bisect_log(), "a");
if (!fp)
res = error_errno(_("could not open '%s' for appending"),
git_path_bisect_log());
else
res = prepare_revs(terms, &revs);
if (!res)
res = process_skipped_commits(fp, terms, &revs);
if (fp)
fclose(fp);
There is positively no need for a `goto` whatsoever.
> > > + fclose(fp);
> > > + } else {
> > > + res = error_errno(_("could not open '%s' for "
> > > + "appending"),
> > > + git_path_bisect_log());
> > > + }
> >
> > This pattern of opening a file, writing something into it, and then return
> > success, otherwise failure, seems like a repeated pattern. In other words,
> > it would be a good candidate for factoring out into its own function.
>
> Yeah, but it seems that in this patch series we use the pattern only
> once. So I think it's fair to leave that for another patch series with
> cleanups and performance improvements or perhaps for microprojects.
Sure, we could repeat past mistakes in this patch series, too.
If, on the other hand, we use this patch series as "an excuse" to
introduce such a helper, no future patch series will have to use the same
kind of argument as you just offered. Instead, we will have an improved
API that will help not only this patch series, but many more to come.
There is tons of precedent for this kind of thing, where we add an
introductory patch at the beginning of a patch series, factoring out
already-existing code into a more reusable shape, and then use it.
So why not repeat that pattern and do the same thing here?
Ciao,
Johannes
^ permalink raw reply [flat|nested] 54+ messages in thread
end of thread, other threads:[~2020-02-17 22:01 UTC | newest]
Thread overview: 54+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-20 14:37 [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Miriam Rubio
2020-01-20 14:37 ` [PATCH 01/29] bisect--helper: convert `vocab_*` char pointers to char arrays Miriam Rubio
2020-01-20 14:37 ` [PATCH 02/29] bisect--helper: change `retval` to `res` Miriam Rubio
2020-01-20 14:37 ` [PATCH 03/29] bisect: use the standard 'if (!var)' way to check for 0 Miriam Rubio
2020-01-20 14:37 ` [PATCH 04/29] run-command: make `exists_in_PATH()` non-static Miriam Rubio
2020-01-20 14:37 ` [PATCH 05/29] bisect--helper: introduce new `decide_next()` function Miriam Rubio
2020-01-20 14:37 ` [PATCH 06/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents Miriam Rubio
2020-01-20 21:57 ` Johannes Schindelin
2020-01-21 6:40 ` Christian Couder
2020-01-21 10:00 ` Miriam R.
2020-01-20 14:37 ` [PATCH 07/29] bisect: libify `bisect_checkout` Miriam Rubio
2020-01-20 14:37 ` [PATCH 08/29] bisect: libify `check_merge_bases` and its dependents Miriam Rubio
2020-01-20 22:09 ` Johannes Schindelin
2020-01-21 9:59 ` Miriam R.
2020-01-20 14:37 ` [PATCH 09/29] bisect: libify `check_good_are_ancestors_of_bad` " Miriam Rubio
2020-01-20 22:20 ` Johannes Schindelin
2020-01-21 6:59 ` Christian Couder
2020-01-21 10:00 ` Miriam R.
2020-01-20 14:37 ` [PATCH 10/29] bisect: libify `handle_bad_merge_base` " Miriam Rubio
2020-01-20 22:23 ` Johannes Schindelin
2020-01-21 7:05 ` Christian Couder
2020-01-21 10:00 ` Miriam R.
2020-01-20 14:37 ` [PATCH 11/29] bisect: libify `bisect_next_all` Miriam Rubio
2020-01-20 22:29 ` Johannes Schindelin
2020-01-21 7:15 ` Christian Couder
2020-01-30 15:18 ` Johannes Schindelin
2020-01-20 14:37 ` [PATCH 12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C Miriam Rubio
2020-01-30 22:47 ` Johannes Schindelin
2020-01-31 10:53 ` Miriam R.
2020-02-17 7:20 ` Christian Couder
2020-02-17 22:00 ` Johannes Schindelin
2020-01-20 14:37 ` [PATCH 13/29] bisect--helper: finish porting `bisect_start()` to C Miriam Rubio
2020-01-20 14:37 ` [PATCH 14/29] bisect--helper: retire `--bisect-clean-state` subcommand Miriam Rubio
2020-01-20 14:37 ` [PATCH 15/29] bisect--helper: retire `--next-all` subcommand Miriam Rubio
2020-01-20 14:37 ` [PATCH 16/29] bisect--helper: reimplement `bisect_autostart` shell function in C Miriam Rubio
2020-01-20 14:37 ` [PATCH 17/29] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions " Miriam Rubio
2020-01-20 14:37 ` [PATCH 18/29] bisect--helper: retire `--check-expected-revs` subcommand Miriam Rubio
2020-01-20 14:37 ` [PATCH 19/29] bisect--helper: retire `--write-terms` subcommand Miriam Rubio
2020-01-20 14:37 ` [PATCH 20/29] bisect--helper: reimplement `bisect_log` shell function in C Miriam Rubio
2020-01-20 14:37 ` [PATCH 21/29] bisect--helper: reimplement `bisect_replay` " Miriam Rubio
2020-01-20 14:37 ` [PATCH 22/29] bisect--helper: retire `--bisect-write` subcommand Miriam Rubio
2020-01-20 14:37 ` [PATCH 23/29] bisect--helper: use `res` instead of return in BISECT_RESET case option Miriam Rubio
2020-01-20 14:37 ` [PATCH 24/29] bisect--helper: retire `--bisect-autostart` subcommand Miriam Rubio
2020-01-20 14:37 ` [PATCH 25/29] bisect--helper: retire `--bisect-auto-next` subcommand Miriam Rubio
2020-01-20 14:37 ` [PATCH 26/29] bisect--helper: reimplement `bisect_skip` shell function in C Miriam Rubio
2020-01-20 14:37 ` [PATCH 27/29] bisect--helper: retire `--check-and-set-terms` subcommand Miriam Rubio
2020-01-20 14:37 ` [PATCH 28/29] bisect--helper: reimplement `bisect_visualize()`shell function in C Miriam Rubio
2020-01-20 14:38 ` [PATCH 29/29] bisect--helper: reimplement `bisect_run` shell " Miriam Rubio
2020-01-20 21:41 ` [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1 Johannes Schindelin
2020-01-20 23:24 ` Christian Couder
2020-01-30 15:12 ` Johannes Schindelin
2020-01-30 21:12 ` Junio C Hamano
2020-01-21 8:44 ` Miriam R.
2020-01-30 15:13 ` Johannes Schindelin
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).