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
next prev parent 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).