git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Miriam R." <mirucam@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git <git@vger.kernel.org>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Christian Couder <chriscool@tuxfamily.org>
Subject: Re: [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C
Date: Thu, 27 Feb 2020 16:34:46 +0100	[thread overview]
Message-ID: <CAN7CjDDwgR=y8gyYmDzmuTW3AKvb1N=EdCtH-8Tr7T=b6cG5gQ@mail.gmail.com> (raw)
In-Reply-To: <xmqqzhd5i1na.fsf@gitster-ct.c.googlers.com>

Hi Junio,

El mié., 26 feb. 2020 a las 20:34, Junio C Hamano
(<gitster@pobox.com>) escribió:
>
> 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?
>
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@tvgsbejvaqbjf.bet/

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@tvgsbejvaqbjf.bet/

Thank you very much for reviewing!.

Best,
Miriam

  reply	other threads:[~2020-02-27 15:34 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-26 10:14 [Outreachy][PATCH 00/10] Finish converting git bisect to C part 2 Miriam Rubio
2020-02-26 10:14 ` [PATCH 01/10] bisect--helper: introduce new `write_in_file()` function Miriam Rubio
2020-02-26 19:06   ` Junio C Hamano
2020-02-26 10:14 ` [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C Miriam Rubio
2020-02-26 19:34   ` Junio C Hamano
2020-02-27 15:34     ` Miriam R. [this message]
2020-02-27 16:41       ` Junio C Hamano
2020-03-06 18:19         ` Miriam R.
2020-03-06 19:05           ` Junio C Hamano
2020-03-11 18:58             ` Christian Couder
2020-02-26 10:14 ` [PATCH 03/10] bisect--helper: finish porting `bisect_start()` to C Miriam Rubio
2020-02-26 10:14 ` [PATCH 04/10] bisect--helper: retire `--bisect-clean-state` subcommand Miriam Rubio
2020-02-26 10:14 ` [PATCH 05/10] bisect--helper: retire `--next-all` subcommand Miriam Rubio
2020-02-26 10:14 ` [PATCH 06/10] bisect--helper: reimplement `bisect_autostart` shell function in C Miriam Rubio
2020-02-27 21:40   ` Johannes Schindelin
2020-02-26 10:14 ` [PATCH 07/10] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions " Miriam Rubio
2020-02-27 23:12   ` Johannes Schindelin
2020-02-26 10:14 ` [PATCH 08/10] bisect--helper: retire `--check-expected-revs` subcommand Miriam Rubio
2020-02-26 10:14 ` [PATCH 09/10] bisect--helper: retire `--write-terms` subcommand Miriam Rubio
2020-02-26 10:14 ` [PATCH 10/10] bisect--helper: retire `--bisect-autostart` subcommand Miriam Rubio

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAN7CjDDwgR=y8gyYmDzmuTW3AKvb1N=EdCtH-8Tr7T=b6cG5gQ@mail.gmail.com' \
    --to=mirucam@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=chriscool@tuxfamily.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).