Re: [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C

[Junio C Hamano <gitster@pobox.com>]

Miriam Rubio <mirucam@gmail.com> writes:

> +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);

Why do you need good_revs string_list in the first place?  Wouldn't
it be simpler and easier to understand to pass in the argv as part
of the callback data and push the good refs in the callback function?

> +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.

"setup" is not a verb X-<.  "... in turn sets up a ..." would be OK.

> +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);
> +		clear_commit_marks(commit, ALL_REV_FLAGS);

clear_commit_marks() is to clear the given flag bits from the named
commit *AND* all of its ancestors (recursively) as long as they have
these bits on, and it typically is used in order to clean up the
state bits left on objects _after_ a revision walk is _done_.

Calling it, especially to clear ALL_REV_FLAGS that contains crucial
flag bits necessary to drive get_revision(), inside a loop that is
still walking commits one by one by calling get_revision(), is
extremely unusual.

It would be surprising if this code were correct.  It may be that it
happens to (appear to) work because parents of the commit hasn't
been painted and calling the helper only clears the mark from the
commit (so we won't see repeated "painting down to the root commit")
in which case this might be an extremely expensive looking variant
of

	commit->object.flags &= ~ALL_REV_FLAGS;

that only confuses the readers.

Even then, I think by clearing bits like SEEN from commit, it breaks
the revision traversal machinery---for example, doesn't this mean
that the commit we just processed can be re-visited by
get_revision() without deduping in a history with forks and merges?

Has this been shown to any of your mentors before sending it to the
list?

> +static int bisect_successful(struct bisect_terms *terms)
> +{
> +	struct object_id oid;
> +	struct commit *commit;
> +	struct pretty_print_context pp = {0};
> +	struct strbuf commit_name = STRBUF_INIT;
> +	char *bad_ref = xstrfmt("refs/bisect/%s",
> +				terms->term_bad);
> +	char *content;
> +	int res;
> +
> +	read_ref(bad_ref, &oid);
> +	printf("%s\n", bad_ref);
> +	commit = lookup_commit_reference(the_repository, &oid);
> +	format_commit_message(commit, "%s", &commit_name, &pp);
> +
> +	content = xstrfmt("# first %s commit: [%s] %s",
> +	terms->term_bad, oid_to_hex(&oid),
> +	commit_name.buf);

Strange indentation.

> +	res = write_in_file(git_path_bisect_log(), content, 1);

So this is a new use of the helper introduced in [01/10].  It is
true that use of it lets this caller not to worry about opening,
writing and closing, but it needs an extra allocation to prepare
"content".

If the calling convention were more like this:

	res = write_to_file(git_path_bisect_log(), "a",
			    "# first %s commit: [%s] %s",
			    terms->term_bad, oid_to_hex(&oid), commit_name.buf,
			    NULL);

this callsite and the callsite in [01/10] wouldn't have had to make
an unnecessary allocation, perhaps?