Re: [PATCH v13 04/20] index-helper: new daemon for caching index and related stuff

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Duy Nguyen <pclouds@gmail.com>
Cc: David Turner <novalis@novalis.org>,
	Git Mailing List <git@vger.kernel.org>,
	Keith McGuigan <kamggg@gmail.com>,
	David Turner <dturner@twopensource.com>,
	Ramsay Jones <ramsay@ramsayjones.plus.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v13 04/20] index-helper: new daemon for caching index and related stuff
Date: Sat, 2 Jul 2016 13:20:44 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.20.1607021312210.12947@virtualbox> (raw)
In-Reply-To: <CACsJy8DuTsDAYzRVk=mW7WX5CZb0Z5bAPnzV_2KXK-BcX=tcVg@mail.gmail.com>

Hi Duy,

On Thu, 30 Jun 2016, Duy Nguyen wrote:

> On Thu, Jun 30, 2016 at 3:06 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Even when NO_MMAP is empty, there might be no Unix sockets available (such
> > as is the case on Windows). In any case, you really only want to skip
> > these tests when index-helper is not available, so would you mind
> > squashing this patch in?
> >
> > -- snipsnap --
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > Subject: [PATCH] fixup! index-helper: new daemon for caching index and related
> >  stuff
> >
> > ---
> >  t/t7900-index-helper.sh | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
> > index 6c9b4dd..12b5bf7 100755
> > --- a/t/t7900-index-helper.sh
> > +++ b/t/t7900-index-helper.sh
> > @@ -10,8 +10,10 @@ Testing git index-helper
> >
> >  . ./test-lib.sh
> >
> > -test -n "$NO_MMAP" && {
> > -       skip_all='skipping index-helper tests: no mmap'
> > +git index-helper -h 2>/dev/null
> > +test $? = 129 ||
> 
> So when NO_MMAP is set, "git index-helper -h" will set $? to 1.

Not quite.

When NO_MMAP is set, index-helper will not be compiled. Or at least it
should not be:

> +ifndef NO_MMAP
> +ifndef NO_UNIX_SOCKETS
> +       PROGRAM_OBJS += index-helper.o
> +endif
> +endif

If it is *unset*, *and* if NO_UNIX_SOCKETS is *also* unset, index-helper
gets compiled, and -h triggers code in parse-options.c or usage.c that
exits with status 129.

So I do not think that this is subtle.

But it just occurred to me that the #ifndef NO_MMAP in index-helper.c is
unnecessary and that its #else clause (containing a loop() that fails)
contains dead code: we never compile this code with NO_MMAP, and neither
should we.

Dave, would you mind taking that #ifndef NO_MMAP out of "index-helper: new
daemon for caching index and related stuff" when you re-roll?

Thanks,
Dscho