From: Johannes Schindelin <Johannes.Schindelin@gmx.de> To: Junio C Hamano <firstname.lastname@example.org> Cc: Jonathan Tan <email@example.com>, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org Subject: Re: [PATCH v7 1/1] Implement rev-list --bisect* --first-parent Date: Wed, 6 Nov 2019 12:30:19 +0100 (CET) [thread overview] Message-ID: <nycvar.QRO.email@example.com> (raw) In-Reply-To: <firstname.lastname@example.org> Hi, On Wed, 6 Nov 2019, Junio C Hamano wrote: > Jonathan Tan <email@example.com> writes: > > > Your commit message title should be of the form "<component>: <change>", > > e.g.: > > > > rev-list: support --first-parent with --bisect* > > Good suggestion. > > > I would be much more laconic (in particular, omitting subjective terms > > like "minutiae" and "mountains of irrelevant data"), but perhaps that is > > just a matter of subjective style. > > FWIW, I had the same reaction. That part of the message was too > noisy without adding much actual value. > > >> Note, bisecting on --first-parent becomes part of findall's previously > >> existing pass-through as an "option state" flag. > > > > I don't understand this part. > > Me neither. > > > Also, clarify in the commit message somewhere that this commit does not > > change the behavior of "git bisect". > > s/\.$/ when used without the "--first-parent" option&/; you mean? > > > As for the diff, besides my comments below, a change in the user-facing > > documentation of "rev-list" is needed, since --bisect and --first-parent > > now work together. > > True. I too am, like you are, happy to see that these two options > made to work well together. > > Thanks, both, for the patch and useful comments. My own review on > it may take a bit more time. While I sadly won't have time to review this patch, let me first state that I am very excited that you revived this stalled patch. Thank you! In addition to Jonathan's comments, I would like to add another one: I would have loved for this patch to be split in two, the first one introducing the `bisect_flags` and using it with `BISECT_FIND_ALL` instead of doing the `find_all` thing, the second one building the first-parent feature on top. Thanks, Dscho
next prev parent reply other threads:[~2019-11-06 11:30 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-05 5:21 [Outreachy] [PATCH v7 0/1] Revenge of --bisect --first-parent workingjubilee 2019-11-05 5:21 ` [PATCH v7 1/1] Implement rev-list --bisect* --first-parent workingjubilee 2019-11-05 23:04 ` Jonathan Tan 2019-11-06 2:42 ` Junio C Hamano 2019-11-06 11:30 ` Johannes Schindelin [this message] 2019-11-06 21:36 ` Jonathan Tan 2019-11-07 3:35 ` Junio C Hamano 2019-11-07 5:07 ` Jubilee Young 2019-11-07 16:00 ` Jubilee Young 2019-11-07 18:56 ` Jonathan Tan
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 * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=nycvar.QRO.firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH v7 1/1] Implement rev-list --bisect* --first-parent' \ /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
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 NNTP newsgroup(s).