Skip to content

git add -i: add a rudimentary version in C (supporting only status and help so far) #170

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 9 commits into from
Closed

git add -i: add a rudimentary version in C (supporting only status and help so far) #170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5d9962d
Start to implement a built-in version of `git add --interactive`
dscho Mar 7, 2019
f42d7b1
diff: export diffstat interface
theiostream May 16, 2017
4836191
built-in add -i: implement the `status` command
theiostream May 16, 2017
d61cf9d
built-in add -i: color the header in the `status` command
slavicaDj May 5, 2019
601c371
built-in add -i: implement the main loop
dscho Mar 7, 2019
978bb25
built-in add -i: show unique prefixes of the commands
dscho Oct 18, 2019
d7d4470
built-in add -i: support `?` (prompt help)
dscho Mar 5, 2019
bcf4aa7
built-in add -i: use color in the main loop
slavicaDj Mar 3, 2019
ccc6fb4
built-in add -i: implement the `help` command
slavicaDj Feb 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Documentation/config/add.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,8 @@ add.ignore-errors (deprecated)::
option of linkgit:git-add[1]. `add.ignore-errors` is deprecated,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:

> From: Johannes Schindelin <[email protected]>
>
> 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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <[email protected]>
> writes:
>
> > From: Johannes Schindelin <[email protected]>
> >
> > This is hardly the first conversion of a Git command that is implement=
ed
> > as a script to a built-in. So far, the most successful strategy for su=
ch
> > conversions has been to add a built-in helper and call that for more a=
nd
> > 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 mo=
re
    functionality to it, until it is feature complete.

    This is in contrast to previous conversions to C, where we started wit=
h
    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 a=
dd
    -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 i=
t
    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[] =3D {
> >  	N_("git add [<options>] [--] <pathspec>..."),
> > @@ -185,6 +186,14 @@ int run_add_interactive(const char *revision, con=
st char *patch_mode,
> >  {
> >  	int status, i;
> >  	struct argv_array argv =3D ARGV_ARRAY_INIT;
> > +	int use_builtin_add_i =3D
> > +		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 =3D=3D 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 =3D=3D 1)
			return !!run_add_i(the_repository, pathspec);
	}

Thanks,
Dscho

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <[email protected]> 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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio,

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

> Johannes Schindelin <[email protected]> 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 an=
d 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 optionall=
y
> >     executes the C code instead. The sole reason for this deviation fr=
om
> >     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 `st=
din`
> >     in the parent process when the spawned process consumes even one
> >     character from `stdin`. And that just does not work for us here, a=
s 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 `g=
it add
> >     -i`.
> >
> >     It is almost as if Perl told us once again that it does not want u=
s to
> >     use it on Windows.
> >
> >     At this point, the built-in version of `git add -i` only states th=
at 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=3Dtrue`.

The "let GIT_TEST_ADD_I_USE_BUILTIN=3Dtrue 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 implemente=
d
    directly in libgit.a.

    At this point, the built-in version of `git add -i` only states that i=
t
    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 t=
he
    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 fr=
om
    implementing the main loop in C and still trying to hand off to the Pe=
rl
    script.

    The very real downside of the approach we have to take here is that th=
e
    test suite won't pass with `GIT_TEST_ADD_I_USE_BUILTIN=3Dtrue` until t=
he
    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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <[email protected]> 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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio,

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

> Johannes Schindelin <[email protected]> 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=3Dtrue` 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.
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <[email protected]> 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.
>>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio,

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

> Johannes Schindelin <[email protected]> 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` approac=
h,
> > but the more important reality is that it is the only one that makes a=
n
> > 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=3Dtrue` 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.
> >>
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <[email protected]> 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).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:

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

That actually is an issue of the quality of the tests you add for
this series.  If they are organized along the arc of run_add_i()
gaining features, the invocation of "git add -i" in the tests can
selectively use USE_BUILTIN=true to verify what has been rewritten
so far.

In any case, I think I've seen the patches in this part of the
metaseries enough and I think they are quickly stabilizing.  Let's
see if others can find and raise issues and otherwise start cooking
in 'next' sometime next week.

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio,

On Thu, 14 Nov 2019, Junio C Hamano wrote:

> any case, I think I've seen the patches in this part of the metaseries
> enough and I think they are quickly stabilizing.  Let's see if others
> can find and raise issues and otherwise start cooking in 'next'
> sometime next week.

While reviewing the next patch series in this arc, I *just* noticed a
buffer overrun: in the main loop, `path + sep` might point to the
trailing `NUL`, but we assign `p += sep + 1;` at the end (which is only
correct when `path + sep` points at whitespace).

The fix is already pushed up into gitgitgadget/git#170, and the relevant
part of the range-diff reads like this:

    @@ add-interactive.c: static void list(struct add_i_state *s, struct string_list *l
     +					index = -1;
     +			}
     +
    -+			p[sep] = '\0';
    ++			if (p[sep])
    ++				p[sep++] = '\0';
     +			if (index < 0 || index >= items->nr)
     +				printf(_("Huh (%s)?\n"), p);
     +			else {
    @@ add-interactive.c: static void list(struct add_i_state *s, struct string_list *l
     +				break;
     +			}
     +
    -+			p += sep + 1;
    ++			p += sep;
     +		}
     +
     +		if (res != LIST_AND_CHOOSE_ERROR)

I plan on waiting for the PR build to finish, and maybe wait until
tomorrow just in case any further suggestion rolls in, then submit the
hopefully final iteration.

And yes, I think it is a good time to start cooking this in `next`, I,
for one, am prone to overlook anything crucial in those patches because
I have stared at them so often.

Ciao,
Dscho

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <[email protected]> writes:

> I plan on waiting for the PR build to finish, and maybe wait until
> tomorrow just in case any further suggestion rolls in, then submit the
> hopefully final iteration.

Thanks.

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.
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading