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:

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.

The next bisection *state* is found?

[...deleted lines...]

+	git bisect--helper --bisect-auto-next || exit

Why is the "|| exit" necessary?

Why is the "|| exit" necessary? ;)


Furthermore:
Where is the bisect_autostart call from bisect_next() sh source gone?
Was it not necessary?

~Stephan