Hi Junio,
thank you very much for reviewing, I have just sent the new version of
this patch series with your suggestions.
El vie., 3 abr. 2020 a las 23:19, Junio C Hamano (<gitster@xxxxxxxxx>) escribió:
>
> Miriam Rubio <mirucam@xxxxxxxxx> writes:
>
> > 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).
>
I cannot change bisect_auto_next() to return void because
in shell the bisect_next() function used "exit $res" , so the following code:
git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :
can result that whole `git bisect` exits with an error code when
bisect_next() does an "exit $res" with $res != 0.
So errors from bisect_next() are not ignored and I cannot make
bisect_auto_next() return void.
Best,
Miriam.
> > +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?
>
> > +
> > + 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....
>
> > + } 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).
>
>
> > +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.
>
> > +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.
>
> > +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.
>
> > +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.
>
> > + 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.
>
> > + /*
> > + * 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.
>
> > +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;
> > +}
> > +
