Re: [PATCH v2 03/11] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;
> > +}
> > +




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux