Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Aaron Lipman <alipman88@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v3 0/4] Introduce --first-parent flag for git bisect
Date: Sat, 01 Aug 2020 12:06:21 -0700
Message-ID: <xmqq36562nki.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200801175840.1877-1-alipman88@gmail.com> (Aaron Lipman's message of "Sat, 1 Aug 2020 13:58:36 -0400")

Aaron Lipman <alipman88@gmail.com> writes:

>> I suspect that you would ask the same question to whoever added
>> no_checkout to this thing, and I wouldn't be surprised if we end up
>> removing both of these parameters (and parsing of the options inside
>> cmd_bisect__helper()) after thinking about them, but anyway, let's
>> read on.
>
> Hmm. Is there a preferred way to to add a --first-parent line to the
> output of "git bisect start --help" via the parse-options API without
> removing the --first-parent option from argv in the process?

I'd rather not to see the code made ugly if the only reason why you
have duplicated command line parsing is to support "git bisect start
-h" while cmd_bisect__helper() still exists.  

That function is supposed to be a thin shim layer whose only reason
of its existence is to serve as an interface with the scripted
version of "git bisect".  When everything is migrated from the shell
script, cmd_bisect__helper() should disappear.

In its place, cmd_bisect() written in C would become a dispatcher
that only looks at the first token on the command line that comes
after ["git", "bisect"] and calls "bisect_start()", "bisect_next()",
etc. with the remainder of the command line with options such as
"--first-parent" and "--no-checkout", which will be parsed by
bisect_start() etc. parsing its argc/argv[] passed by cmd_bisect().
It is likely that cmd_bisect() would not have any call to
parse_options(); instead cmd_start() etc. would call parse_options()
with their own option[] table.

So I am not sure if the change between v2 and v3 is going in the
right direction.

> As long as we're capturing the --first-parent option in
> cmd_bisect__helper(), checking argv for a --first-parent flag in
> bisect_start() is pointless.

This is going backwards, as far as I can tell.  If anything, I'd
rather see cmd_bisect__helper() get fixed so that it does not parse
"--first-parent" (and similarly, "--no-checkout" that you imitated)
into first_parent_only (and no_checkout) variables and passed as
parameters to bisect_start().  cmd_bisect__helper() should instead
just let these flags (and there may be others) sit in argc/argv[]
and have bisect_start() parse them, together with all other options
bisect_start() already has its parser for.

Thanks.

  parent reply index

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-28 15:44 [PATCH 0/3] " Aaron Lipman via GitGitGadget
2020-07-28 15:44 ` [PATCH 1/3] rev-list: allow bisect and first-parent flags Aaron Lipman via GitGitGadget
2020-07-28 20:23   ` Junio C Hamano
2020-07-28 21:53     ` Junio C Hamano
2020-07-28 15:44 ` [PATCH 2/3] bisect: introduce first-parent flag Aaron Lipman via GitGitGadget
2020-07-28 15:44 ` [PATCH 3/3] bisect: combine args passed to find_bisection() Aaron Lipman via GitGitGadget
2020-07-30  0:27 ` [PATCH v2 0/3] Introduce --first-parent flag for git bisect Aaron Lipman
2020-07-30  0:27   ` [PATCH v2 1/3] rev-list: allow bisect and first-parent flags Aaron Lipman
2020-07-30  0:27   ` [PATCH v2 2/3] bisect: introduce first-parent flag Aaron Lipman
2020-07-30  4:17     ` Junio C Hamano
2020-07-30  0:27   ` [PATCH v2 3/3] bisect: combine args passed to find_bisection() Aaron Lipman
2020-07-30  4:32     ` Junio C Hamano
2020-07-30  0:48   ` [PATCH v2 0/3] Introduce --first-parent flag for git bisect Junio C Hamano
2020-08-01 17:58   ` [PATCH v3 0/4] " Aaron Lipman
2020-08-01 17:58     ` [PATCH v3 1/4] rev-list: allow bisect and first-parent flags Aaron Lipman
2020-08-01 17:58     ` [PATCH v3 2/4] bisect: introduce first-parent flag Aaron Lipman
2020-08-01 17:58     ` [PATCH v3 3/4] bisect: combine args passed to find_bisection() Aaron Lipman
2020-08-01 19:30       ` Martin Ågren
2020-08-01 17:58     ` [PATCH v3 4/4] bisect: consistent style for git bisect run tests Aaron Lipman
2020-08-01 19:27       ` Martin Ågren
2020-08-01 19:06     ` Junio C Hamano [this message]
2020-08-04 22:01     ` [PATCH v4 0/5] Introduce --first-parent flag for git bisect Aaron Lipman
2020-08-04 22:01       ` [PATCH v4 1/5] t6030: modernize "git bisect run" tests Aaron Lipman
2020-08-05  6:11         ` Christian Couder
2020-08-04 22:01       ` [PATCH v4 2/5] rev-list: allow bisect and first-parent flags Aaron Lipman
2020-08-05  0:38         ` Eric Sunshine
2020-08-04 22:01       ` [PATCH v4 3/5] cmd_bisect__helper: defer parsing no-checkout flag Aaron Lipman
2020-08-04 22:01       ` [PATCH v4 4/5] bisect: introduce first-parent flag Aaron Lipman
2020-08-04 22:01       ` [PATCH v4 5/5] bisect: combine args passed to find_bisection() Aaron Lipman
2020-08-04 22:19       ` [PATCH v4 0/5] Introduce --first-parent flag for git bisect Junio C Hamano
2020-08-05  5:55       ` Christian Couder
2020-08-07 21:05         ` Junio C Hamano
2020-08-07 21:17           ` Eric Sunshine
2020-08-07 22:07             ` Junio C Hamano
2020-08-07 22:20               ` Eric Sunshine
2020-08-05  6:18       ` Martin Ågren
2020-08-07 21:58       ` [PATCH v5 " Aaron Lipman
2020-08-07 21:58         ` [PATCH v5 1/5] t6030: modernize "git bisect run" tests Aaron Lipman
2020-08-07 21:58         ` [PATCH v5 2/5] rev-list: allow bisect and first-parent flags Aaron Lipman
2020-08-07 21:58         ` [PATCH v5 3/5] cmd_bisect__helper: defer parsing no-checkout flag Aaron Lipman
2020-08-07 21:58         ` [PATCH v5 4/5] bisect: introduce first-parent flag Aaron Lipman
2020-08-07 21:58         ` [PATCH v5 5/5] bisect: combine args passed to find_bisection() Aaron Lipman

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=xmqq36562nki.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=alipman88@gmail.com \
    --cc=git@vger.kernel.org \
    /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

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git