Re: [PATCH v2 03/11] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C
Miriam Rubio <mirucam@gmail.com> writes:
Show 14 quoted lines
> diff --git a/bisect.c b/bisect.c
> index 9154f810f7..a50278ea7e 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -635,6 +635,11 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,
> struct argv_array rev_argv = ARGV_ARRAY_INIT;
> int i;
>
> + /*
> + * `revs` could has been used before.
> + * Thus we first need to reset it.
> + */
> + reset_revision_walk();
> repo_init_revisions(r, revs, prefix);
I don't have enough time and concentration to follow all the codepaths in "bisect" that walk the commit graph starting here, but seeing one codepath, e.g. check_ancestors(), after calling this and walking with bisect_common(), uses clear_commit_marks_many() to clear ALL_REV_FLAGS, not just the ones that reset_revision_walk() clears, I am not sure what value this addition has.
To put it differently, what codepath are you protecting the revision walk that is about to happen against with this "reset"? Who are the callers that could have used `revs` before calling this function and touched only SEEN, ADDED, SHOWN, etc. without touching other bits like COUNTED, TREESAME, UNINTERESTING that matter to the correct operation of "bisect"?
The rest of the patch looks quite reasonably done. Let me comment on the patch out of order (in other words, I'll rearrange the functions in the order I read them). I am realizing that I feel it easier to understand to read the code in a bottom-up fashion.
> +static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)
> +{
> + if (bisect_next_check(terms, NULL))
> + return BISECT_OK;The bisect_next_check() function returns what decide_next() returns, which is either 0 or error() which is -1. So this says "if nect-check says there was an error, we return OK". For the purpose of not proceeding to bisect_next(), returning is perfectly correct, but is the value returned correct? The scripted original does
git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :
meaning "try next-check and go on to bisect_next if check succeeds; either way, ignore all errors from them". So it probably is more faithful conversion to make the returned value from auto_next() void.
> +
> + return bisect_next(terms, prefix);
> +}
In any case, this conversion of auto_next looks like a good one, with or without fixing its type. The caller in cmd_bisect__helper() seems to store the returned value in 'res' and uses it for the exit status, but for this to be a faithful conversion, it should ignore the returned value from here and always exit with success (and if we do so, it is one more reason why we would want to update the type of this function to return void).
Show 7 quoted lines
> +static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
> +{
> + int no_checkout;
> + enum bisect_error res;
>
> + if (bisect_next_check(terms, terms->term_good))
> + return BISECT_FAILED;The original makes sure it does not get any argument, but that is done in cmd_bisect__helper(), so it is OK.
The next thing the original does is to call bisect_autostart, before doing the next-check. Was it a dead code that wasn't necessary, or is its loss a regression during the conversion?
Show 5 quoted lines
> +
> + no_checkout = !is_empty_or_missing_file(git_path_bisect_head());
> +
> + /* Perform all bisection computation, display and checkout */
> + res = bisect_next_all(the_repository, prefix, no_checkout);
Looking good. We've already converted next_all() in the previous series, and we call it the same way as the original by checking if $GIT_DIR/BISECT_HEAD is there. The original said "if BISECT_HEAD exists as a file, use '--no-checkout'" and did not care if its empty or not, but the updated code seems to care. Does the difference matter (i.e. is it more correct to ignore an empty BISECT_HEAD and pretend as if it did not exist)?
> + if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
> + res = bisect_successful(terms);
> + return res ? res : BISECT_INTERNAL_SUCCESS_MERGE_BASE;It is unclear why "1st bad commit found" is turned into "success merge base" here. bisect_successfull() returns an error when it cannot append to the log, and otherwise we would want to relay "we are done successfully" back to the caller, no? Puzzled....
Show 7 quoted lines
> + } else if (res == BISECT_ONLY_SKIPPED_LEFT) {
> + res = bisect_skipped_commits(terms);
> + return res ? res : BISECT_ONLY_SKIPPED_LEFT;
> + }
> + return res;
> +}
> +This side, I can understand what it wants to do to the "we only have skipped ones so we cannot continue" status.
What is done in bisect_skipped_commits() is dubious, though (we'll see it later).
Show 20 quoted lines
> +static int bisect_skipped_commits(struct bisect_terms *terms)
> +{
> + int res = 0;
> + FILE *fp = NULL;
> + struct rev_info revs;
> +
> + fp = fopen(git_path_bisect_log(), "a");
> + if (!fp)
> + return error_errno(_("could not open '%s' for appending"),
> + git_path_bisect_log());
> +
> + res = prepare_revs(terms, &revs);
> +
> + if (!res)
> + res = process_skipped_commits(fp, terms, &revs);
> +
> + fclose(fp);
> + return res;
> +}
> +Opens the log to append to, or returns if we cannot. Calls prepare_revs() and process() if successfully prepared, and then close. No resource leak, and the returned value is the result code from the last function that matters. Looks good.
Show 22 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
> + * sets up a revision walk.
> + */
> + reset_revision_walk();
> + init_revisions(revs, NULL);
> + rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL);
> + if (prepare_revision_walk(revs))
> + res = error(_("revision walk setup failed\n"));
> +
> + argv_array_clear(&rev_argv);
> + return res;
> +}
> +Unlike the one in rev_setup() above, the only codepath this thing is used is quite limited (i.e. after doing all the bisection computation including walking the graph and counting the weights with various bits in bisect_next) and we know all that is left to do is to run a single rev-list call, so it is clear to see that reset_revision_walk() is sufficient here.
Show 10 quoted lines
> +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)
> +{
> + char *term_good = xstrfmt("%s-*", terms->term_good);
> +
> + argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);
> + for_each_glob_ref_in(register_good_ref, term_good, "refs/bisect/", rev_argv);
> +
> + free(term_good);
> +}
> +This sets up to find commits that can be reached by BAD that cannot be reached by any GOOD revs, which is a quite faithful translation from the original one.
Show 7 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;What's that comparison with "< 1" doing? That's a 36-byte message, and you are saying that it is OK if we showed only 10-byte from it, but it is not OK, even if the function did not report an output error by returning a negative value, if it returned that it wrote 0 bytes?
I can understand it if it were
if (fprintf(fp, "...") < 0)
return error_errno(_("failed to write to ..."));though.
Show 9 quoted lines
> + 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);
> + }Again, this is a faithful translation of the rev-list that appears in the original, provided that &revs was set up correctly, and relevant object flags cleared correctly before we start the traversal, both of which seem to be the case.
Show 9 quoted lines
> + /*
> + * Reset the flags used by revision walks in case
> + * there is another revision walk after this one.
> + */
> + reset_revision_walk();
> +
> + return 0;
> +}
> +
So, overall, this step was a quite pleasant read, except for the very first bit above.
Thanks.
Show 33 quoted lines
> +static int register_good_ref(const char *refname,
> + const struct object_id *oid, int flags,
> + void *cb_data)
> +{
> + struct argv_array *rev_argv = cb_data;
> +
> + argv_array_push(rev_argv, oid_to_hex(oid));
> + return 0;
> +}
> +
> +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);
> + 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);
> +
> + res = write_in_file(git_path_bisect_log(), "a", "# first %s commit: [%s] %s\n",
> + terms->term_bad, oid_to_hex(&oid),
> + commit_name.buf);
> +
> + strbuf_release(&commit_name);
> + free(bad_ref);
> + return res;
> +}
> +
Miriam Rubio
