Hi Pranit,
in this mail I review the "second part" of your patch: the transition of
bisect_next and bisect_auto_next to C.
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 1d3e17f..fcd7574 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -408,6 +411,136 @@ static int bisect_terms(struct bisect_terms *terms,
> const char **argv, int argc)
> return 0;
> }
>
> +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 int bisect_next(struct bisect_terms *terms, const char *prefix)
> +{
> + int res, no_checkout;
> +
> + /*
> + * 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".
> + */
The comment above makes no sense here, or does it?
> + 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(prefix , no_checkout);
Style: there is a space left of the comma.
> +
> + if (res == 10) {
> + FILE *fp = NULL;
> + unsigned char sha1[20];
> + 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 retval = 0;
> +
> + read_ref(bad_ref, sha1);
> + commit = lookup_commit_reference(sha1);
> + format_commit_message(commit, "%s", &commit_name, &pp);
> + fp = fopen(git_path_bisect_log(), "a");
> + if (!fp) {
> + retval = -1;
> + goto finish_10;
> + }
> + if (fprintf(fp, "# first %s commit: [%s] %s\n",
> + terms->term_bad, sha1_to_hex(sha1),
> + commit_name.buf) < 1){
> + retval = -1;
> + goto finish_10;
> + }
> + goto finish_10;
> + finish_10:
> + if (fp)
> + fclose(fp);
> + strbuf_release(&commit_name);
> + free(bad_ref);
> + return retval;
> + }
> + else if (res == 2) {
> + FILE *fp = NULL;
> + struct rev_info revs;
> + struct argv_array rev_argv = ARGV_ARRAY_INIT;
> + struct string_list good_revs = STRING_LIST_INIT_DUP;
> + struct pretty_print_context pp = {0};
> + struct commit *commit;
> + char *term_good = xstrfmt("%s-*", terms->term_good);
> + int i, retval = 0;
> +
> + fp = fopen(git_path_bisect_log(), "a");
> + if (!fp) {
> + retval = -1;
> + goto finish_2;
> + }
> + if (fprintf(fp, "# only skipped commits left to test\n") < 1) {
> + retval = -1;
> + goto finish_2;
> + }
> + for_each_glob_ref_in(register_good_ref, term_good,
> + "refs/bisect/", (void *) &good_revs);
> +
> + argv_array_pushl(&rev_argv, "skipped_commits",
> "refs/bisect/bad", "--not", NULL);
> + for (i = 0; i < good_revs.nr; i++)
> + argv_array_push(&rev_argv, good_revs.items[i].string);
> +
> + /* 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);
> + argv_array_clear(&rev_argv);
> + string_list_clear(&good_revs, 0);
> + if (prepare_revision_walk(&revs))
> + die(_("revision walk setup failed\n"));
> +
> + 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);
> + }
> + goto finish_2;
> + finish_2:
> + if (fp)
> + fclose(fp);
> + string_list_clear(&good_revs, 0);
> + argv_array_clear(&rev_argv);
> + free(term_good);
> + if (retval)
> + return retval;
> + else
> + return res;
> + }
> + return res;
> +}
It would be much nicer if you put the (res == 10) branch and the
(res == 2) branch into separate functions and just call them.
Then you also won't need ugly label naming like finish_10 or finish_2.
I'd also (again) recommend to use goto fail instead of setting retval to
-1 separately each time.
I'd also recommend to use a separate function to append to the bisect
log file. There is a lot of duplicated opening, checking, closing code;
IIRC such a function would also already be handy for some of the
previous patches.
> +
> +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;
> +}
Hmm, the handling of the return values is a little confusing. However,
if I understand the sh source correctly, it always returns success, no
matter if bisect_next failed or not. I do not know if you had something
special in mind here.
> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> @@ -643,6 +794,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 find the next
> bisection state"), BISECT_AUTO_NEXT),
The next bisection *state* is found?
> diff --git a/git-bisect.sh b/git-bisect.sh
> index f0896b3..d574c44 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -139,45 +119,7 @@ bisect_state() {
> *)
> usage ;;
> esac
> - bisect_auto_next
[...deleted lines...]
> + git bisect--helper --bisect-auto-next || exit
Why is the "|| exit" necessary?
> @@ -319,14 +260,15 @@ 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)
> bisect_skip "$@" ;;
> next)
> # Not sure we want "next" at the UI level anymore.
> - bisect_next "$@" ;;
> + get_terms
> + git bisect--helper --bisect-next "$@" || exit ;;
Why is the "|| exit" necessary? ;)
Furthermore:
Where is the bisect_autostart call from bisect_next() sh source gone?
Was it not necessary?
~Stephan
