Hi Junio,
El mié., 26 feb. 2020 a las 20:34, Junio C Hamano
(<gitster@xxxxxxxxx>) escribió:
>
> Miriam Rubio <mirucam@xxxxxxxxx> 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?
>
Ok, I will do it that way.
> > +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.
>
Noted.
> > +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?
Adding clear_commit_marks() was a suggestion of a previous review of this patch:
https://public-inbox.org/git/nycvar.QRO.7.76.6.2001301619340.46@xxxxxxxxxxxxxxxxx/
And of course, my mentor always reviews my patch series before sending.
>
> > +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.
Noted.
>
> > + 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?
>
Aha. I will change it.
This helper function was also a suggestion of the previous reviewer:
https://public-inbox.org/git/nycvar.QRO.7.76.6.2001301619340.46@xxxxxxxxxxxxxxxxx/
Thank you very much for reviewing!.
Best,
Miriam
