[PATCH 0/2] auto-detect getdelim()

[PATCH 0/2] auto-detect getdelim()

[Eric Sunshine]

As an optimization, strbuf takes advantage of getdelim() when available
(HAVE_GETDELIM). Currently, HAVE_GETDELIM is defined automatically only
for Linux. This patch series updates config.mak.uname to define
HAVE_GETDELIM on Mac OS X (Darwin) based upon version ("uname -r"), and
more generally via a configure script check.

Eric Sunshine (2):
  config.mak.uname: Darwin: define HAVE_GETDELIM for modern OS X
    releases
  configure: add getdelim() check

 config.mak.uname | 3 +++
 configure.ac     | 6 ++++++
 2 files changed, 9 insertions(+)

-- 
2.4.2.598.gb4379f4

[PATCH 1/2] config.mak.uname: Darwin: define HAVE_GETDELIM for modern OS X releases

[Eric Sunshine]

On Mac OS X, getdelim() first became available with Xcode 4.1[1], which
was released the same day as OS X 10.7 "Lion", so assume getdelim()
availability from 10.7 onward. (As of this writing, OS X is at 10.10
"Yosemite".)

According to Wikipedia[2], 4.1 was also available for download by paying
developers on OS X 10.6 "Snow Leopard", so it's possible that some 10.6
machines may have getdelim(). However, as strbuf's use of getdelim() is
purely an optimization, let's be conservative and assume 10.6 and
earlier lack getdelim().

[1]: Or, possibly with Xcode 4.0, but that version is no longer
     available for download, or not available to non-paying developers,
     so testing is not possible.

[2]: http://en.wikipedia.org/wiki/Xcode

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

Tested on OS X 10.10.3 "Yosemite" with Xcode 6.3.2 and OS X 10.5.8
"Leopard" with Xcode 3.1.

The use of 'expr' in this new test is decidedly different from existing
instances which merely check if `uname -R` matches a particular single
digit and a period. If the new test took the same approach, it would
have to match either one digit (in a particular range) plus a period, or
two digits with the first being "1", plus a period. The resulting 'expr'
expression quickly becomes ugly and quite difficult to decipher. Hence,
the new test instead takes advantage of expr's relational operator '>='
to keep things simple and make the test easy to understand at a glance
("if version >= 11" where 11 is the Darwin major version number of OS X
10.7).

 config.mak.uname | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/config.mak.uname b/config.mak.uname
index d26665f..46a415c 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -102,6 +102,9 @@ ifeq ($(uname_S),Darwin)
 	ifeq ($(shell expr "$(uname_R)" : '[15]\.'),2)
 		NO_STRLCPY = YesPlease
 	endif
+	ifeq ($(shell expr $(shell expr "$(uname_R)" : '\([0-9][0-9]*\)\.') '>=' 11),1)
+		HAVE_GETDELIM = YesPlease
+	endif
 	NO_MEMMEM = YesPlease
 	USE_ST_TIMESPEC = YesPlease
 	HAVE_DEV_TTY = YesPlease
-- 
2.4.2.598.gb4379f4

Re: [PATCH 1/2] config.mak.uname: Darwin: define HAVE_GETDELIM for modern OS X releases

[Jeff King]

On Tue, Jun 02, 2015 at 02:18:57PM -0400, Eric Sunshine wrote:

> The use of 'expr' in this new test is decidedly different from existing
> instances which merely check if `uname -R` matches a particular single
> digit and a period. If the new test took the same approach, it would
> have to match either one digit (in a particular range) plus a period, or
> two digits with the first being "1", plus a period. The resulting 'expr'
> expression quickly becomes ugly and quite difficult to decipher. Hence,
> the new test instead takes advantage of expr's relational operator '>='
> to keep things simple and make the test easy to understand at a glance
> ("if version >= 11" where 11 is the Darwin major version number of OS X
> 10.7).

I think that is OK with respect to portability; we are already inside a
$(uname_S) check, so we know we are on some form of Mac OS. But...

> +	ifeq ($(shell expr $(shell expr "$(uname_R)" : '\([0-9][0-9]*\)\.') '>=' 11),1)

Do you need to spawn two shells? It seems like:

  $(shell expr `expr "$(uname_R)" : '\([0-9][0-9]*\)'` '>=' 11),1)

should do the same thing.

-Peff

Re: [PATCH 1/2] config.mak.uname: Darwin: define HAVE_GETDELIM for modern OS X releases

[Jeff King]

On Tue, Jun 02, 2015 at 02:44:13PM -0400, Jeff King wrote:

> > +	ifeq ($(shell expr $(shell expr "$(uname_R)" : '\([0-9][0-9]*\)\.') '>=' 11),1)
> 
> Do you need to spawn two shells? It seems like:
> 
>   $(shell expr `expr "$(uname_R)" : '\([0-9][0-9]*\)'` '>=' 11),1)

Oops, I missed the trailing '.' in the regex there, and it probably
needs double-quotes in case the inner expr fails to match anything.

We could also use "test -gt" instead of the outer expr, which is more
idiomatic shell. But it reports via exit code, so you'd need "&& echo 1"
at the end.

-Peff

Re: [PATCH 1/2] config.mak.uname: Darwin: define HAVE_GETDELIM for modern OS X releases

[Eric Sunshine]

On Tue, Jun 2, 2015 at 3:04 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jun 02, 2015 at 02:44:13PM -0400, Jeff King wrote:
>> > +   ifeq ($(shell expr $(shell expr "$(uname_R)" : '\([0-9][0-9]*\)\.') '>=' 11),1)
>>
>> Do you need to spawn two shells? It seems like:
>>
>>   $(shell expr `expr "$(uname_R)" : '\([0-9][0-9]*\)'` '>=' 11),1)

I considered that and waffled on it. Either approach uses an extra
process, but I suppose `...` would likely be less expensive since it's
just forking the shell rather than exec()ing a new one.

> Oops, I missed the trailing '.' in the regex there, and it probably
> needs double-quotes in case the inner expr fails to match anything.

Which is messy considering the double quotes already surrounding
$(uname_R). Suggestions?

> We could also use "test -gt" instead of the outer expr, which is more
> idiomatic shell. But it reports via exit code, so you'd need "&& echo 1"
> at the end.

Yes, I messed around with that as well but didn't want to stray too
far from existing practice.

I suppose the combination of `...` with built-in 'test' and built-in
'echo' would be the most efficient choice. Do you want it re-rolled?

Re: [PATCH 1/2] config.mak.uname: Darwin: define HAVE_GETDELIM for modern OS X releases

[Jeff King]

On Tue, Jun 02, 2015 at 03:57:44PM -0400, Eric Sunshine wrote:

> > Oops, I missed the trailing '.' in the regex there, and it probably
> > needs double-quotes in case the inner expr fails to match anything.
> 
> Which is messy considering the double quotes already surrounding
> $(uname_R). Suggestions?

The shell should do the right thing with nested quotes inside backticks.
So just (untested):

  $(shell expr "`expr "$(uname_R)" : '\([0-9][0-9]*\.\)'`" '>=' 11),1)

> I suppose the combination of `...` with built-in 'test' and built-in
> 'echo' would be the most efficient choice. Do you want it re-rolled?

I can live with it either way. It's all pretty horrible and ugly; the
saving grace is that we hopefully never have to touch that line again.
;)

-Peff

Re: [PATCH 1/2] config.mak.uname: Darwin: define HAVE_GETDELIM for modern OS X releases

[Eric Sunshine]

On Tue, Jun 2, 2015 at 4:01 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jun 02, 2015 at 03:57:44PM -0400, Eric Sunshine wrote:
>
>> > Oops, I missed the trailing '.' in the regex there, and it probably
>> > needs double-quotes in case the inner expr fails to match anything.
>>
>> Which is messy considering the double quotes already surrounding
>> $(uname_R). Suggestions?
>
> The shell should do the right thing with nested quotes inside backticks.
> So just (untested):
>
>   $(shell expr "`expr "$(uname_R)" : '\([0-9][0-9]*\.\)'`" '>=' 11),1)

Right. Temporary brain derailment on my part.

>> I suppose the combination of `...` with built-in 'test' and built-in
>> 'echo' would be the most efficient choice. Do you want it re-rolled?
>
> I can live with it either way. It's all pretty horrible and ugly; the
> saving grace is that we hopefully never have to touch that line again.

I'll re-roll, taking advantage of `...` and (typically, builtin)
'test' and 'echo'. This:

$(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -ge 11 && echo 1)

Already tested on OS X 10.10 (Yosemite) and 10.5 (Leopard).

[PATCH 2/2] configure: add getdelim() check

[Eric Sunshine]

As an optimization, strbuf will take advantage of getdelim() if
available, so add a configure check which defines HAVE_GETDELIM if
found.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

Tested on:
* OS X 10.10.3 "Yosemite" with Xcode 6.3.2
* OS X 10.5.8 "Leopard" with Xcode 3.1
* Linux
* FreeBSD

 configure.ac | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/configure.ac b/configure.ac
index bbdde85..14012fa 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1041,6 +1041,12 @@ GIT_CHECK_FUNC(initgroups,
 [NO_INITGROUPS=YesPlease])
 GIT_CONF_SUBST([NO_INITGROUPS])
 #
+# Define HAVE_GETDELIM if you have getdelim in the C library.
+GIT_CHECK_FUNC(getdelim,
+[HAVE_GETDELIM=YesPlease],
+[HAVE_GETDELIM=])
+GIT_CONF_SUBST([HAVE_GETDELIM])
+#
 #
 # Define NO_MMAP if you want to avoid mmap.
 #
-- 
2.4.2.598.gb4379f4

Re: [PATCH 2/2] configure: add getdelim() check

[Jeff King]

On Tue, Jun 02, 2015 at 02:18:58PM -0400, Eric Sunshine wrote:

> As an optimization, strbuf will take advantage of getdelim() if
> available, so add a configure check which defines HAVE_GETDELIM if
> found.

Thanks, looks good.

-Peff