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

>
> 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?
>