git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Jeff Hostetler <git@jeffhostetler.com>,
	Jeff King <peff@peff.net>
Subject: Re: [PATCH v5 1/9] Start to implement a built-in version of `git add --interactive`
Date: Sat, 9 Nov 2019 12:06:07 +0100 (CET)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1911091154550.46@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <xmqqh83fnfah.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Fri, 8 Nov 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > This is hardly the first conversion of a Git command that is implemented
> > as a script to a built-in. So far, the most successful strategy for such
> > conversions has been to add a built-in helper and call that for more and
> > more functionality from the script, as more and more parts are
> > converted.
> >
> > With the interactive add, we choose a different strategy....
>
> This is hardly the first conversion that we took the "build the
> whole program piece by piece and flip the whole thing on with
> usebuiltin" conversion successfully.  Pratik's rebase-in-c series
> comes to mind.
>
> Personally, I do not think the first two paragraphs of the proposed
> log message do not belong here.  Cover letter is a different story
> and it may make sense to explain why the approach was taken there,
> but here, I'd prefer to see it more succinctly tell what approach is
> taken and go directly to describe what this step in that approach
> does to the readers, which is more important.

I reworded the commit message:

    Start to implement a built-in version of `git add --interactive`

    To convert the interactive `add` to C, we start with a bare-bones
    version of the built-in interactive add, guarded by the new
    `add.interactive.useBuiltin` config variable, and then add more and more
    functionality to it, until it is feature complete.

    This is in contrast to previous conversions to C, where we started with
    a built-in helper that spawns the script by default, but optionally
    executes the C code instead. The sole reason for this deviation from
    previous practice is that on Windows (where such a conversion has the
    most benefits in terms of speed and robustness) we face the very
    specific problem that a `system()` call in Perl seems to close `stdin`
    in the parent process when the spawned process consumes even one
    character from `stdin`. And that just does not work for us here, as it
    would stop the main loop as soon as any interactive command was
    performed by the helper. Which is almost all of the commands in `git add
    -i`.

    It is almost as if Perl told us once again that it does not want us to
    use it on Windows.

    At this point, the built-in version of `git add -i` only states that it
    cannot do anything yet ;-)

Hopefully you like this one better?

> > diff --git a/builtin/add.c b/builtin/add.c
> > index dd18e5c9b6..4f625691b5 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -20,6 +20,7 @@
> >  #include "bulk-checkin.h"
> >  #include "argv-array.h"
> >  #include "submodule.h"
> > +#include "add-interactive.h"
> >
> >  static const char * const builtin_add_usage[] = {
> >  	N_("git add [<options>] [--] <pathspec>..."),
> > @@ -185,6 +186,14 @@ int run_add_interactive(const char *revision, const char *patch_mode,
> >  {
> >  	int status, i;
> >  	struct argv_array argv = ARGV_ARRAY_INIT;
> > +	int use_builtin_add_i =
> > +		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
>
> Have blank here at the boundary between decl and stmt ...
>
> > +	if (use_builtin_add_i < 0)
> > +		git_config_get_bool("add.interactive.usebuiltin",
> > +				    &use_builtin_add_i);
> > +
>
> ... and lose it here (optional).

Done.
>
> > +	if (use_builtin_add_i == 1 && !patch_mode)
> > +		return !!run_add_i(the_repository, pathspec);
> >
>
> Strictly speaking, we can bypass the probing of environment and
> config when upon the entry of the function, where patch_mode is
> already known.  I do not know offhand if rearranging the code to
> take advantage of that fact would result in a flow that is also
> easier to follow, but I suspect it would.

Okay. I changed it to:

	if (!patch_mode) {
		if (use_builtin_add_i < 0)
			git_config_get_bool("add.interactive.usebuiltin",
					    &use_builtin_add_i);
		if (use_builtin_add_i == 1)
			return !!run_add_i(the_repository, pathspec);
	}

Thanks,
Dscho

  reply	other threads:[~2019-11-09 11:06 UTC|newest]

Thread overview: 124+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-10 17:37 [PATCH 00/11] git add -i: add a rudimentary version in C (supporting only status and help so far) Johannes Schindelin via GitGitGadget
2019-04-10 17:37 ` [PATCH 01/11] Start to implement a built-in version of `git add --interactive` Johannes Schindelin via GitGitGadget
2019-04-18 14:31   ` Jeff Hostetler
2019-04-18 16:06     ` Jeff King
2019-04-30 23:40       ` Johannes Schindelin
2019-05-01  2:21         ` Jeff King
2019-05-13 11:14           ` Johannes Schindelin
2019-04-10 17:37 ` [PATCH 02/11] diff: export diffstat interface Daniel Ferreira via GitGitGadget
2019-04-10 17:37 ` [PATCH 03/11] built-in add -i: implement the `status` command Daniel Ferreira via GitGitGadget
2019-04-10 17:37 ` [PATCH 04/11] built-in add -i: refresh the index before running `status` Johannes Schindelin via GitGitGadget
2019-04-10 17:37 ` [PATCH 05/11] built-in add -i: color the header in the `status` command Johannes Schindelin via GitGitGadget
2019-04-10 17:37 ` [PATCH 06/11] built-in add -i: implement the main loop Johannes Schindelin via GitGitGadget
2019-04-18 16:49   ` Jeff Hostetler
2019-05-13 12:04     ` Johannes Schindelin
2019-04-10 17:37 ` [PATCH 07/11] Add a function to determine unique prefixes for a list of strings Slavica Djukic via GitGitGadget
2019-04-18 17:57   ` Jeff Hostetler
2019-05-13 12:48     ` Johannes Schindelin
2019-04-10 17:37 ` [PATCH 08/11] built-in add -i: show unique prefixes of the commands Slavica Djukic via GitGitGadget
2019-04-10 17:37 ` [PATCH 09/11] built-in add -i: support `?` (prompt help) Johannes Schindelin via GitGitGadget
2019-04-10 17:37 ` [PATCH 10/11] built-in add -i: use color in the main loop Slavica Djukic via GitGitGadget
2019-04-10 17:37 ` [PATCH 11/11] built-in add -i: implement the `help` command Johannes Schindelin via GitGitGadget
2019-05-13 17:27 ` [PATCH v2 00/11] git add -i: add a rudimentary version in C (supporting only status and help so far) Johannes Schindelin via GitGitGadget
2019-05-13 17:27   ` [PATCH v2 01/11] Start to implement a built-in version of `git add --interactive` Johannes Schindelin via GitGitGadget
2019-05-13 17:27   ` [PATCH v2 02/11] diff: export diffstat interface Daniel Ferreira via GitGitGadget
2019-05-13 17:27   ` [PATCH v2 03/11] built-in add -i: implement the `status` command Daniel Ferreira via GitGitGadget
2019-05-13 17:27   ` [PATCH v2 04/11] built-in add -i: refresh the index before running `status` Johannes Schindelin via GitGitGadget
2019-05-13 17:27   ` [PATCH v2 06/11] built-in add -i: implement the main loop Johannes Schindelin via GitGitGadget
2019-05-13 17:27   ` [PATCH v2 05/11] built-in add -i: color the header in the `status` command Johannes Schindelin via GitGitGadget
2019-05-13 17:27   ` [PATCH v2 07/11] Add a function to determine unique prefixes for a list of strings Slavica Djukic via GitGitGadget
2019-05-13 17:27   ` [PATCH v2 08/11] built-in add -i: show unique prefixes of the commands Slavica Djukic via GitGitGadget
2019-05-13 17:28   ` [PATCH v2 09/11] built-in add -i: support `?` (prompt help) Johannes Schindelin via GitGitGadget
2019-05-13 17:28   ` [PATCH v2 10/11] built-in add -i: use color in the main loop Slavica Djukic via GitGitGadget
2019-05-13 17:28   ` [PATCH v2 11/11] built-in add -i: implement the `help` command Johannes Schindelin via GitGitGadget
2019-07-16 14:58   ` [PATCH v3 00/11] git add -i: add a rudimentary version in C (supporting only status and help so far) Johannes Schindelin via GitGitGadget
2019-07-16 14:58     ` [PATCH v3 01/11] Start to implement a built-in version of `git add --interactive` Johannes Schindelin via GitGitGadget
2019-07-31 17:52       ` Junio C Hamano
2019-08-26 21:26         ` Johannes Schindelin
2019-08-27 22:25           ` Junio C Hamano
2019-08-28 15:06             ` Johannes Schindelin
2019-08-28 15:37               ` Junio C Hamano
2019-07-16 14:58     ` [PATCH v3 03/11] built-in add -i: implement the `status` command Daniel Ferreira via GitGitGadget
2019-07-31 18:12       ` Junio C Hamano
2019-08-27 10:04         ` Johannes Schindelin
2019-07-16 14:58     ` [PATCH v3 02/11] diff: export diffstat interface Daniel Ferreira via GitGitGadget
2019-07-31 17:59       ` Junio C Hamano
2019-08-27  9:22         ` Johannes Schindelin
2019-07-16 14:58     ` [PATCH v3 04/11] built-in add -i: refresh the index before running `status` Johannes Schindelin via GitGitGadget
2019-07-16 14:58     ` [PATCH v3 05/11] built-in add -i: color the header in the `status` command Johannes Schindelin via GitGitGadget
2019-07-16 14:58     ` [PATCH v3 06/11] built-in add -i: implement the main loop Johannes Schindelin via GitGitGadget
2019-07-31 18:14       ` Junio C Hamano
2019-07-16 14:58     ` [PATCH v3 07/11] Add a function to determine unique prefixes for a list of strings Slavica Djukic via GitGitGadget
2019-07-31 18:39       ` Junio C Hamano
2019-08-24 12:38       ` SZEDER Gábor
2019-08-27 12:14         ` Johannes Schindelin
2019-08-28 16:30           ` SZEDER Gábor
2019-08-28 16:34             ` [PATCH] [PoC] A simpler find_unique_prefixes() implementation SZEDER Gábor
2019-08-30 20:12               ` Johannes Schindelin
2019-07-16 14:58     ` [PATCH v3 08/11] built-in add -i: show unique prefixes of the commands Slavica Djukic via GitGitGadget
2019-07-16 14:58     ` [PATCH v3 09/11] built-in add -i: support `?` (prompt help) Johannes Schindelin via GitGitGadget
2019-07-16 14:58     ` [PATCH v3 10/11] built-in add -i: use color in the main loop Slavica Djukic via GitGitGadget
2019-07-16 14:58     ` [PATCH v3 11/11] built-in add -i: implement the `help` command Johannes Schindelin via GitGitGadget
2019-08-02 21:04       ` Junio C Hamano
2019-08-02 22:26         ` Jeff King
2019-07-16 18:38     ` [PATCH v3 00/11] git add -i: add a rudimentary version in C (supporting only status and help so far) Johannes Schindelin
2019-08-02 21:06       ` Junio C Hamano
2019-08-27 12:57     ` [PATCH v4 " Johannes Schindelin via GitGitGadget
2019-08-27 12:57       ` [PATCH v4 01/11] Start to implement a built-in version of `git add --interactive` Johannes Schindelin via GitGitGadget
2019-08-27 12:57       ` [PATCH v4 02/11] diff: export diffstat interface Daniel Ferreira via GitGitGadget
2019-08-27 12:57       ` [PATCH v4 04/11] built-in add -i: refresh the index before running `status` Johannes Schindelin via GitGitGadget
2019-08-27 12:57       ` [PATCH v4 03/11] built-in add -i: implement the `status` command Daniel Ferreira via GitGitGadget
2019-08-27 12:57       ` [PATCH v4 05/11] built-in add -i: color the header in " Johannes Schindelin via GitGitGadget
2019-08-27 12:57       ` [PATCH v4 07/11] Add a function to determine unique prefixes for a list of strings Slavica Djukic via GitGitGadget
2019-08-27 12:57       ` [PATCH v4 06/11] built-in add -i: implement the main loop Johannes Schindelin via GitGitGadget
2019-08-27 12:57       ` [PATCH v4 08/11] built-in add -i: show unique prefixes of the commands Slavica Djukic via GitGitGadget
2019-08-27 12:58       ` [PATCH v4 09/11] built-in add -i: support `?` (prompt help) Johannes Schindelin via GitGitGadget
2019-08-27 12:58       ` [PATCH v4 10/11] built-in add -i: use color in the main loop Slavica Djukic via GitGitGadget
2019-08-27 12:58       ` [PATCH v4 11/11] built-in add -i: implement the `help` command Johannes Schindelin via GitGitGadget
2019-11-04 12:15       ` [PATCH v5 0/9] git add -i: add a rudimentary version in C (supporting only status and help so far) Johannes Schindelin via GitGitGadget
2019-11-04 12:15         ` [PATCH v5 1/9] Start to implement a built-in version of `git add --interactive` Johannes Schindelin via GitGitGadget
2019-11-08  4:49           ` Junio C Hamano
2019-11-09 11:06             ` Johannes Schindelin [this message]
2019-11-10  7:18               ` Junio C Hamano
2019-11-11  9:15                 ` Johannes Schindelin
2019-11-11 12:09                   ` Junio C Hamano
2019-11-12 15:03                     ` Johannes Schindelin
2019-11-13  3:54                       ` Junio C Hamano
2019-11-13 12:30                         ` Johannes Schindelin
2019-11-13 14:01                           ` Junio C Hamano
2019-11-04 12:15         ` [PATCH v5 2/9] diff: export diffstat interface Daniel Ferreira via GitGitGadget
2019-11-08  4:56           ` Junio C Hamano
2019-11-04 12:15         ` [PATCH v5 3/9] built-in add -i: implement the `status` command Daniel Ferreira via GitGitGadget
2019-11-08  5:01           ` Junio C Hamano
2019-11-04 12:15         ` [PATCH v5 4/9] built-in add -i: color the header in " Slavica Đukić via GitGitGadget
2019-11-04 12:15         ` [PATCH v5 5/9] built-in add -i: implement the main loop Johannes Schindelin via GitGitGadget
2019-11-08  5:17           ` Junio C Hamano
2019-11-09 11:21             ` Johannes Schindelin
2019-11-04 12:15         ` [PATCH v5 6/9] built-in add -i: show unique prefixes of the commands Johannes Schindelin via GitGitGadget
2019-11-04 12:15         ` [PATCH v5 7/9] built-in add -i: support `?` (prompt help) Johannes Schindelin via GitGitGadget
2019-11-04 12:15         ` [PATCH v5 8/9] built-in add -i: use color in the main loop Slavica Đukić via GitGitGadget
2019-11-04 12:15         ` [PATCH v5 9/9] built-in add -i: implement the `help` command Slavica Đukić via GitGitGadget
2019-11-13 12:40         ` [PATCH v6 0/9] git add -i: add a rudimentary version in C (supporting only status and help so far) Johannes Schindelin via GitGitGadget
2019-11-13 12:40           ` [PATCH v6 1/9] Start to implement a built-in version of `git add --interactive` Johannes Schindelin via GitGitGadget
2019-11-14  2:15             ` Junio C Hamano
2019-11-14 15:07               ` Johannes Schindelin
2019-11-15  4:35                 ` Junio C Hamano
2019-11-13 12:40           ` [PATCH v6 2/9] diff: export diffstat interface Daniel Ferreira via GitGitGadget
2019-11-13 12:40           ` [PATCH v6 3/9] built-in add -i: implement the `status` command Daniel Ferreira via GitGitGadget
2019-11-13 12:41           ` [PATCH v6 4/9] built-in add -i: color the header in " Slavica Đukić via GitGitGadget
2019-11-13 12:41           ` [PATCH v6 5/9] built-in add -i: implement the main loop Johannes Schindelin via GitGitGadget
2019-11-13 12:41           ` [PATCH v6 6/9] built-in add -i: show unique prefixes of the commands Johannes Schindelin via GitGitGadget
2019-11-13 12:41           ` [PATCH v6 7/9] built-in add -i: support `?` (prompt help) Johannes Schindelin via GitGitGadget
2019-11-13 12:41           ` [PATCH v6 8/9] built-in add -i: use color in the main loop Slavica Đukić via GitGitGadget
2019-11-13 12:41           ` [PATCH v6 9/9] built-in add -i: implement the `help` command Slavica Đukić via GitGitGadget
2019-11-13 12:46           ` [PATCH v6 0/9] git add -i: add a rudimentary version in C (supporting only status and help so far) Johannes Schindelin
2019-11-15 11:11           ` [PATCH v7 " Johannes Schindelin via GitGitGadget
2019-11-15 11:11             ` [PATCH v7 1/9] Start to implement a built-in version of `git add --interactive` Johannes Schindelin via GitGitGadget
2019-11-15 11:11             ` [PATCH v7 2/9] diff: export diffstat interface Daniel Ferreira via GitGitGadget
2019-11-15 11:11             ` [PATCH v7 3/9] built-in add -i: implement the `status` command Daniel Ferreira via GitGitGadget
2019-11-15 11:11             ` [PATCH v7 4/9] built-in add -i: color the header in " Slavica Đukić via GitGitGadget
2019-11-15 11:11             ` [PATCH v7 5/9] built-in add -i: implement the main loop Johannes Schindelin via GitGitGadget
2019-11-15 11:11             ` [PATCH v7 6/9] built-in add -i: show unique prefixes of the commands Johannes Schindelin via GitGitGadget
2019-11-15 11:11             ` [PATCH v7 7/9] built-in add -i: support `?` (prompt help) Johannes Schindelin via GitGitGadget
2019-11-15 11:11             ` [PATCH v7 8/9] built-in add -i: use color in the main loop Slavica Đukić via GitGitGadget
2019-11-15 11:11             ` [PATCH v7 9/9] built-in add -i: implement the `help` command Slavica Đukić via GitGitGadget

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=nycvar.QRO.7.76.6.1911091154550.46@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --subject='Re: [PATCH v5 1/9] Start to implement a built-in version of `git add --interactive`' \
    /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@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

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

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

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

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