[v5,1/9] Start to implement a built-in version of `git add --interactive`

Commit Message

Linus Arver via GitGitGadget Nov. 4, 2019, 12:15 p.m. UTC

From: Johannes Schindelin <[e-mail]>

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. The sole
reason for this 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.

Instead, we follow the opposite route where 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.

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

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 Documentation/config/add.txt |  5 +++++
 Makefile                     |  1 +
 add-interactive.c            |  7 +++++++
 add-interactive.h            |  8 ++++++++
 builtin/add.c                | 10 ++++++++++
 t/README                     |  4 ++++
 6 files changed, 35 insertions(+)
 create mode 100644 add-interactive.c
 create mode 100644 add-interactive.h

Comments

Junio C Hamano Nov. 8, 2019, 4:49 a.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> From: Johannes Schindelin <[e-mail]>
>
> 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.

> diff --git a/Documentation/config/add.txt b/Documentation/config/add.txt
>  	variables.
> +
> +add.interactive.useBuiltin::
> +	[EXPERIMENTAL] Set to `true` to use the experimental built-in
> +	implementation of the interactive version of linkgit:git-add[1]
> +	instead of the Perl script version. Is `false` by default.

Good.

> diff --git a/Makefile b/Makefile
> index 58b92af54b..6c4a1e0ee5 100644
> --- a/Makefile
> +++ b/Makefile
>  LIB_OBJS += abspath.o
> +LIB_OBJS += add-interactive.o
>  LIB_OBJS += advice.o
>  LIB_OBJS += alias.o
>  LIB_OBJS += alloc.o

OK.

> diff --git a/add-interactive.c b/add-interactive.c
> new file mode 100644
> index 0000000000..482e458dc6
> --- /dev/null
> +++ b/add-interactive.c
> @@ -0,0 +1,7 @@
> +#include "cache.h"
> +#include "add-interactive.h"
> +
> +int run_add_i(struct repository *r, const struct pathspec *ps)
> +{
> +	die(_("No commands are available in the built-in `git add -i` yet!"));
> +}

OK, with or without s/commands/sub&/;

> diff --git a/add-interactive.h b/add-interactive.h
> new file mode 100644
> index 0000000000..7043b8741d
> --- /dev/null
> +++ b/add-interactive.h

OK.

> 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).

> +	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.

> +GIT_TEST_ADD_I_USE_BUILTIN=<boolean>, when true, enables the
> +built-in version of git add -i. See 'add.interactive.useBuiltin' in
> +git-config(1).

Makes sense.

Thanks.

Johannes Schindelin Nov. 9, 2019, 11:06 a.m. UTC | #2

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> writes:
>
> > From: Johannes Schindelin <[e-mail]>
> >
> > 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

Junio C Hamano Nov. 10, 2019, 7:18 a.m. UTC | #3

Johannes Schindelin <[e-mail]> writes:

> 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?

Not really.  I find the "we could do the other way but we don't, and
I hate Perl" totally irrelevant and misleading.

Unless it is in GSoC or something that wants to avoid a total
failure of nothing to show at the end of the period, in which case a
piecemeal "we did not finish, but at least we have a handful of
subcommands rewritten to show for the consolation prize" might be a
way to have "something" that resembles "working".  But if we value
the quality of the final product over having to have something to
show in a set term (like you as a paid programmer working as a
professional), building piece by piece in the final framework
(i.e. "in C as a builtin") and flipping the "useBuiltin" to turn on
the whole thing at the end would be the preferrable way to do this
kind of thing, I would think.  Also, it is less wasteful, not having
to worry about the inter-language glue code.  Even if the original
is in shell, not in Perl, we have quite a lot of glue code to throw
values back and forth across the language boundary to imitate what
used to be mere assignments to global shell variables between shell
functions and their callers, and always somebody screws up quoting
there ;-)

>> > +	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);
> 	}

Doesn't look so bad as I feared.

Thanks.

Johannes Schindelin Nov. 11, 2019, 9:15 a.m. UTC | #4

Hi Junio,

On Sun, 10 Nov 2019, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
>
> > 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?
>
> Not really.  I find the "we could do the other way but we don't, and
> I hate Perl" totally irrelevant and misleading.

Okay, I understand that you take exception at my criticism of Git's use
of Perl, and I fully understand that you think I blame you for it
because you added most of it.

And I agree that this sidetrack is totally irrelevant for the patch
under discussion.

I do think, however, that the discussion of "we wanted to do it the
other way, but when we tried, it did not work" is relevant, even if I
shortened it to "we use a different approach than previous conversions,
because that previous approach would not work".

Truth be told: I would have _much rather_ stayed with the previous
`--helper` approach, as that would have made it possible to have a
passing test suite at every step, with and without
`GIT_TEST_ADD_I_USE_BUILTIN=true`.

The "let GIT_TEST_ADD_I_USE_BUILTIN=true use the built-in even for
functions we _know_ are not implemented" way only gives us the full
comfort of a passing test suite at the very end of all six patch series,
if which the patch series we are currently discussing is merely the
first.

If I was a reviewer of this patch series rather than the sender, I would
be a bit uncomfortable with the fact that `GIT_TEST_ADD_I_USE_BUILTIN`
cannot be added to the CI/PR builds' `linux-gcc` over-job, not until
much, much later. In fact, it can only be added as the very last patch
in the very last of the six patch series.

And as I write this, I realize that I never spelled that out in the
commit message, and it is a rather important point for reviewers to see
addressed pre-emptively, in my opinion.

Therefore I revised the commit message again:

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

    Unlike previous conversions to C, where we started with a built-in
    helper, we start this conversion by adding an interception in the
    `run_add_interactive()` function when the new opt-in
    `add.interactive.useBuiltin` config knob is turned on (or the
    corresponding environment variable `GIT_TEST_ADD_I_USE_BUILTIN`), and
    calling the new internal API function `run_add_i()` that is implemented
    directly in libgit.a.

    At this point, the built-in version of `git add -i` only states that it
    cannot do anything yet. In subsequent patches/patch series, the
    `run_add_i()` function will gain more and more functionality, until it
    is feature complete. The whole arc of the conversion can be found in the
    PRs #170-175 at https://github.com/gitgitgadget/git.

    The "--helper approach" can unfortunately not be used here: on Windows
    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`. Which prevents us from
    implementing the main loop in C and still trying to hand off to the Perl
    script.

    The very real downside of the approach we have to take here is that the
    test suite won't pass with `GIT_TEST_ADD_I_USE_BUILTIN=true` until the
    conversion is complete (the `--helper` approach would have let it pass,
    even at each of the incremental conversion steps).

Any suggestions how to improve that commit message?

Ciao,
Dscho

Junio C Hamano Nov. 11, 2019, 12:09 p.m. UTC | #5

Johannes Schindelin <[e-mail]> writes:

> And I agree that this sidetrack is totally irrelevant for the patch
> under discussion.
>
> I do think, however, that the discussion of "we wanted to do it the
> other way, but when we tried, it did not work" is relevant, even if I
> shortened it to "we use a different approach than previous conversions,
> because that previous approach would not work".

Regardless of the language the scripted version was written in, I
think the '--helper' approach is always the poorer choice between
the two [*1*].  It limits the modular decomposition to what suits the
original language, the impedance mismatch between the original and
target language forces us to unnatural style of inter module
communication, and the unnatural interface layer, which we know has
to be discarded at the end, must be written [*2*].

So, I'd prefer to see "because this is a better way in the longer
term" over "because the --helper approach would not work".

[Footnote]

*1* In only one case I would recommend using "--helper" approach,
    though.  When you are not expecting the developer to be able to
    come up with a better split of the program into modules than how
    the scripted version is, and you want to ensure that the
    developer have something to show when they faild to complete the
    project after N weeks.  You are a more experienced developer
    than an average GSoC student, and there is no pencils-down time,
    so the exception would not apply.

*2* In "git submodule" for example it was quite natural for the
    module that gives a list of submodules with its traits the
    program cares about to be written as a shell function that
    writes the data to its standard output.  And consuming modules
    sit at the downstream of a pipe, accepting its output.  When you
    are writing these modules both in C, you wouldn't connect them
    with pipe to carry the list of submodules, but a piecemeal
    conversion using the "--helper" approach meant that there always
    remained _some_ consumer that wants to read from the pipe, so
    long after the module lister was rewritten in C, it still needed
    to support a mode where it sends its output to the pipe, instead
    of just passing an array of structures.

Johannes Schindelin Nov. 12, 2019, 3:03 p.m. UTC | #6

Hi Junio,

On Mon, 11 Nov 2019, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
>
> > And I agree that this sidetrack is totally irrelevant for the patch
> > under discussion.
> >
> > I do think, however, that the discussion of "we wanted to do it the
> > other way, but when we tried, it did not work" is relevant, even if I
> > shortened it to "we use a different approach than previous conversions,
> > because that previous approach would not work".
>
> Regardless of the language the scripted version was written in, I
> think the '--helper' approach is always the poorer choice between
> the two [*1*].  It limits the modular decomposition to what suits the
> original language, the impedance mismatch between the original and
> target language forces us to unnatural style of inter module
> communication, and the unnatural interface layer, which we know has
> to be discarded at the end, must be written [*2*].
>
> So, I'd prefer to see "because this is a better way in the longer
> term" over "because the --helper approach would not work".

Hmm. I feel distinctly unheard.

It may appear compelling, conceptually, to shun the `--helper` approach,
but the more important reality is that it is the only one that makes an
incremental conversion possible at all.

It took an entire month of 60-hour weeks to complete the conversion of
`git add -i`/`git add -p` to C, and only at the very end was I able to
run the test suite with `GIT_TEST_ADD_I_USE_BUILTIN=true` and see it
pass.

That is an awfully long time, and you know fully well that this amount
of work equates to three to four Outreachy/GSoC seasons. That would be
an insane amount of time to go without the confidence of a passing test
suite.

In contrast, we were able to complete the conversions of the interactive
rebase as well as of `git stash` within _a single season_. I attribute a
large part of that success to the ability to keep the tests green during
the incremental conversion _because of_ the `--helper` approach.

So no, I do not think that your suggestion to reword the commit message
is something we want to do.  Instead, I think the commit message needs
to be rephrased until I get the point across clearly.

It is indeed _in spite of_ the success of the `--helper` approach that
we cannot use it here.

Ciao,
Dscho

>
> [Footnote]
>
> *1* In only one case I would recommend using "--helper" approach,
>     though.  When you are not expecting the developer to be able to
>     come up with a better split of the program into modules than how
>     the scripted version is, and you want to ensure that the
>     developer have something to show when they faild to complete the
>     project after N weeks.  You are a more experienced developer
>     than an average GSoC student, and there is no pencils-down time,
>     so the exception would not apply.
>
> *2* In "git submodule" for example it was quite natural for the
>     module that gives a list of submodules with its traits the
>     program cares about to be written as a shell function that
>     writes the data to its standard output.  And consuming modules
>     sit at the downstream of a pipe, accepting its output.  When you
>     are writing these modules both in C, you wouldn't connect them
>     with pipe to carry the list of submodules, but a piecemeal
>     conversion using the "--helper" approach meant that there always
>     remained _some_ consumer that wants to read from the pipe, so
>     long after the module lister was rewritten in C, it still needed
>     to support a mode where it sends its output to the pipe, instead
>     of just passing an array of structures.
>

Junio C Hamano Nov. 13, 2019, 3:54 a.m. UTC | #7

Johannes Schindelin <[e-mail]> writes:

>> Regardless of the language the scripted version was written in, I
>> think the '--helper' approach is always the poorer choice between
>> the two [*1*].  It limits the modular decomposition to what suits the
>> original language, the impedance mismatch between the original and
>> target language forces us to unnatural style of inter module
>> communication, and the unnatural interface layer, which we know has
>> to be discarded at the end, must be written [*2*].
>>
>> So, I'd prefer to see "because this is a better way in the longer
>> term" over "because the --helper approach would not work".
>
> Hmm. I feel distinctly unheard.

The feeling is mutual ;-)

> It may appear compelling, conceptually, to shun the `--helper` approach,
> but the more important reality is that it is the only one that makes an
> incremental conversion possible at all.
>
> It took an entire month of 60-hour weeks to complete the conversion of
> `git add -i`/`git add -p` to C, and only at the very end was I able to
> run the test suite with `GIT_TEST_ADD_I_USE_BUILTIN=true` and see it
> pass.

Yeah, that is developer comfort, and of course it is nice to have
than not to have it.

But compared to the downside impact to the quality of end result
that is inherent to the '--helper' approach, I'd prioritize the
quality of the end result over developer comfort.

> It is indeed _in spite of_ the success of the `--helper` approach that
> we cannot use it here.

As I do not see those past '--helper' ones necessarily successes, we
must agree to disagree here.

In any case, the log message needs to express why _you_ ended up
taking the non-helper approach.  Even though it is far less
relevant, compared to that, what other approach you instead wanted
to take, I do not veto you from having your own opinion.

>> [Footnote]
>>
>> *1* In only one case I would recommend using "--helper" approach,
>>     though.  When you are not expecting the developer to be able to
>>     come up with a better split of the program into modules than how
>>     the scripted version is, and you want to ensure that the
>>     developer have something to show when they faild to complete the
>>     project after N weeks.  You are a more experienced developer
>>     than an average GSoC student, and there is no pencils-down time,
>>     so the exception would not apply.
>>
>> *2* In "git submodule" for example it was quite natural for the
>>     module that gives a list of submodules with its traits the
>>     program cares about to be written as a shell function that
>>     writes the data to its standard output.  And consuming modules
>>     sit at the downstream of a pipe, accepting its output.  When you
>>     are writing these modules both in C, you wouldn't connect them
>>     with pipe to carry the list of submodules, but a piecemeal
>>     conversion using the "--helper" approach meant that there always
>>     remained _some_ consumer that wants to read from the pipe, so
>>     long after the module lister was rewritten in C, it still needed
>>     to support a mode where it sends its output to the pipe, instead
>>     of just passing an array of structures.
>>

Johannes Schindelin Nov. 13, 2019, 12:30 p.m. UTC | #8

Hi Junio,

On Wed, 13 Nov 2019, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
>
> >> Regardless of the language the scripted version was written in, I
> >> think the '--helper' approach is always the poorer choice between
> >> the two [*1*].  It limits the modular decomposition to what suits the
> >> original language, the impedance mismatch between the original and
> >> target language forces us to unnatural style of inter module
> >> communication, and the unnatural interface layer, which we know has
> >> to be discarded at the end, must be written [*2*].
> >>
> >> So, I'd prefer to see "because this is a better way in the longer
> >> term" over "because the --helper approach would not work".
> >
> > Hmm. I feel distinctly unheard.
>
> The feeling is mutual ;-)

Maybe I could take your comments more seriously if you acknowledged the
fact that I am _very_ familiar with the vagaries of converting scripts
to C. Like, very, very, very familiar.

> > It may appear compelling, conceptually, to shun the `--helper` approach,
> > but the more important reality is that it is the only one that makes an
> > incremental conversion possible at all.
> >
> > It took an entire month of 60-hour weeks to complete the conversion of
> > `git add -i`/`git add -p` to C, and only at the very end was I able to
> > run the test suite with `GIT_TEST_ADD_I_USE_BUILTIN=true` and see it
> > pass.
>
> Yeah, that is developer comfort, and of course it is nice to have
> than not to have it.

Comfort has little to do with it. Driving out bugs has a lot more to do
with it. Which is the point I am trying to get across the entire time.

> But compared to the downside impact to the quality of end result
> that is inherent to the '--helper' approach, I'd prioritize the
> quality of the end result over developer comfort.
>
> > It is indeed _in spite of_ the success of the `--helper` approach that
> > we cannot use it here.
>
> As I do not see those past '--helper' ones necessarily successes, we
> must agree to disagree here.

Right. But if I recall, you never even saw the need for the conversions
in the first place. Maybe you still don't?

> In any case, the log message needs to express why _you_ ended up
> taking the non-helper approach.  Even though it is far less
> relevant, compared to that, what other approach you instead wanted
> to take, I do not veto you from having your own opinion.

Okay. I will take that as an indication that I can go forward with the
latest proposal. After all, I described pretty well, I think, why _I_
ended up taking the non-helper approach.

Thanks,
Dscho

> >> [Footnote]
> >>
> >> *1* In only one case I would recommend using "--helper" approach,
> >>     though.  When you are not expecting the developer to be able to
> >>     come up with a better split of the program into modules than how
> >>     the scripted version is, and you want to ensure that the
> >>     developer have something to show when they faild to complete the
> >>     project after N weeks.  You are a more experienced developer
> >>     than an average GSoC student, and there is no pencils-down time,
> >>     so the exception would not apply.
> >>
> >> *2* In "git submodule" for example it was quite natural for the
> >>     module that gives a list of submodules with its traits the
> >>     program cares about to be written as a shell function that
> >>     writes the data to its standard output.  And consuming modules
> >>     sit at the downstream of a pipe, accepting its output.  When you
> >>     are writing these modules both in C, you wouldn't connect them
> >>     with pipe to carry the list of submodules, but a piecemeal
> >>     conversion using the "--helper" approach meant that there always
> >>     remained _some_ consumer that wants to read from the pipe, so
> >>     long after the module lister was rewritten in C, it still needed
> >>     to support a mode where it sends its output to the pipe, instead
> >>     of just passing an array of structures.
> >>
>

Junio C Hamano Nov. 13, 2019, 2:01 p.m. UTC | #9

Johannes Schindelin <[e-mail]> writes:

>> As I do not see those past '--helper' ones necessarily successes, we
>> must agree to disagree here.
>
> Right. But if I recall, you never even saw the need for the conversions
> in the first place. Maybe you still don't?

You probably are forgetting the fact that I was very supportive for
the rewrite of checkout, commit and format-patch (the last one being
my favorite) in C from scripted Porcelain.  

None of these were '--helper' style conversion and I would consider
them much more successful than the recent ones, some of which still
suffer from impedance mismatch bugs (e.g. some parts of the C
implementation work on the in-core index, while other parts working
on the on the on-disk index, letting them become out of sync and
introducing bugs).

Patch

diff --git a/Documentation/config/add.txt b/Documentation/config/add.txt
index 4d753f006e..c9f748f81c 100644
--- a/Documentation/config/add.txt
+++ b/Documentation/config/add.txt
@@ -5,3 +5,8 @@  add.ignore-errors (deprecated)::
 	option of linkgit:git-add[1].  `add.ignore-errors` is deprecated,
 	as it does not follow the usual naming convention for configuration
 	variables.
+
+add.interactive.useBuiltin::
+	[EXPERIMENTAL] Set to `true` to use the experimental built-in
+	implementation of the interactive version of linkgit:git-add[1]
+	instead of the Perl script version. Is `false` by default.
diff --git a/Makefile b/Makefile
index 58b92af54b..6c4a1e0ee5 100644
--- a/Makefile
+++ b/Makefile
@@ -823,6 +823,7 @@  LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentat
 	-name '*.h' -print)))
 
 LIB_OBJS += abspath.o
+LIB_OBJS += add-interactive.o
 LIB_OBJS += advice.o
 LIB_OBJS += alias.o
 LIB_OBJS += alloc.o
diff --git a/add-interactive.c b/add-interactive.c
new file mode 100644
index 0000000000..482e458dc6
--- /dev/null
+++ b/add-interactive.c
@@ -0,0 +1,7 @@ 
+#include "cache.h"
+#include "add-interactive.h"
+
+int run_add_i(struct repository *r, const struct pathspec *ps)
+{
+	die(_("No commands are available in the built-in `git add -i` yet!"));
+}
diff --git a/add-interactive.h b/add-interactive.h
new file mode 100644
index 0000000000..7043b8741d
--- /dev/null
+++ b/add-interactive.h
@@ -0,0 +1,8 @@ 
+#ifndef ADD_INTERACTIVE_H
+#define ADD_INTERACTIVE_H
+
+struct repository;
+struct pathspec;
+int run_add_i(struct repository *r, const struct pathspec *ps);
+
+#endif
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);
+	if (use_builtin_add_i < 0)
+		git_config_get_bool("add.interactive.usebuiltin",
+				    &use_builtin_add_i);
+
+	if (use_builtin_add_i == 1 && !patch_mode)
+		return !!run_add_i(the_repository, pathspec);
 
 	argv_array_push(&argv, "add--interactive");
 	if (patch_mode)
@@ -319,6 +328,7 @@  static int add_config(const char *var, const char *value, void *cb)
 		ignore_add_errors = git_config_bool(var, value);
 		return 0;
 	}
+
 	return git_default_config(var, value, cb);
 }
 
diff --git a/t/README b/t/README
index 60d5b77bcc..5132ec83f8 100644
--- a/t/README
+++ b/t/README
@@ -397,6 +397,10 @@  GIT_TEST_STASH_USE_BUILTIN=<boolean>, when false, disables the
 built-in version of git-stash. See 'stash.useBuiltin' in
 git-config(1).
 
+GIT_TEST_ADD_I_USE_BUILTIN=<boolean>, when true, enables the
+built-in version of git add -i. See 'add.interactive.useBuiltin' in
+git-config(1).
+
 GIT_TEST_INDEX_THREADS=<n> enables exercising the multi-threaded loading
 of the index for the whole test suite by bypassing the default number of
 cache entries and thread minimums. Setting this to 1 will make the