Re: [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 26, 2020, 19:34 UTC
- Message-ID
- <xmqqzhd5i1na.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20200226101429.81327-3-mirucam@gmail.com>
Miriam Rubio <mirucam@gmail.com> writes:
Show 24 quoted lines
> +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?
Show 11 quoted lines
> +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.
Show 17 quoted lines
> +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?
Show 19 quoted lines
> +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?