Re: [PATCH v5 02/10] add--helper: create builtin helper for interactive add

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Daniel Ferreira via GitGitGadget" <gitgitgadget@gmail.com>,
	git@vger.kernel.org, "Phillip Wood" <phillip.wood@dunelm.org.uk>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Daniel Ferreira" <bnmvco@gmail.com>
Subject: Re: [PATCH v5 02/10] add--helper: create builtin helper for interactive add
Date: Fri, 8 Mar 2019 21:48:01 +0100 (STD)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1903082138350.41@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <xmqqzhqpt483.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Thu, 21 Feb 2019, Junio C Hamano wrote:

> "Daniel Ferreira via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > diff --git a/git.c b/git.c
> > index 2dd588674f..cb42591f37 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -444,6 +444,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
> >  
> >  static struct cmd_struct commands[] = {
> >  	{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
> > +	{ "add--helper", cmd_add__helper, RUN_SETUP | NEED_WORK_TREE },
> >  	{ "am", cmd_am, RUN_SETUP | NEED_WORK_TREE },
> >  	{ "annotate", cmd_annotate, RUN_SETUP | NO_PARSEOPT },
> >  	{ "apply", cmd_apply, RUN_SETUP_GENTLY },
> 
> When adding a more complex replacement command we often cannot write
> RUN_SETUP nor NEED_WORK_TREE here and instead do the job ourselves
> in cmd_ourcommand() the hard way.  But because this one is only for
> helping "add -i", we can let the git.c::run_builtin() aka "git
> potty" do the hard part, which is very nice.

Sadly, I ran into a quite curious (and seemingly equally insurmountable)
problem with this approach: when I use `system()` to execute a Git
built-in from the Perl script, and when that built-in then consumes even a
single byte of stdin, that file descriptor gets closed in the Perl
process, and no amount of file descriptor duplication and redirecting that
I could come up helped. After calling the built-in helper to run, say, the
`update` command, the main interactive add process always exited on me due
to EOF on stdin, and I could not make it work.

To be sure, this happens only with MSYS2's Perl. If I try the same on
Linux (WSL, really), everything works Just Fine.

So my current approach is actually the other way round: I started
implementing the main loop in C, and implemented all commands except for
`patch` so far, and I think I will just hand off to the Perl script for
that command for now.

Unfortunately that means that my current state looks already quite a bit
different than what you reviewed. My hope is, however, that it will make
for a nice "story" once I have a version to show that I think could be
applied as-is.

Ciao,
Dscho