installation issue when building with NO_CURL=YesPlease

installation issue when building with NO_CURL=YesPlease

[Paul Walker]

As I could not find any bug reporting information on the wiki I  
thought I would mention this here, please let me know if there is a  
better forum for bug reports.  I believe the latest git release  
(1.7.1.) has an installation bug when building with  
"NO_CURL=YesPlease".  Looking at the Makefile line 1999 it reads

for p in $(REMOTE_CURL_ALIASES); do

which results in "/bin/sh: syntax error at line 1 : `;' unexpected" as  
REMOTE_CURL_ALIASES is empty.

Re: installation issue when building with NO_CURL=YesPlease

[Michael J Gruber]

Paul Walker venit, vidit, dixit 26.05.2010 15:35:
> As I could not find any bug reporting information on the wiki I  
> thought I would mention this here, please let me know if there is a  
> better forum for bug reports.  I believe the latest git release  
> (1.7.1.) has an installation bug when building with  
> "NO_CURL=YesPlease".  Looking at the Makefile line 1999 it reads
> 
> for p in $(REMOTE_CURL_ALIASES); do
> 
> which results in "/bin/sh: syntax error at line 1 : `;' unexpected" as  
> REMOTE_CURL_ALIASES is empty.

That seems to depend on the shell. My bash 4.1.2 doesn't care. What does
/bin/sh --version say for you?

Michael

Re: installation issue when building with NO_CURL=YesPlease

[Ramkumar Ramachandra]

Hi,

> As I could not find any bug reporting information on the wiki I thought I
> would mention this here, please let me know if there is a better forum for
> bug reports.

Bugs are usually reported on this mailing list.

> I believe the latest git release (1.7.1.) has an installation
> bug when building with "NO_CURL=YesPlease".  Looking at the Makefile line
> 1999 it reads
>
> for p in $(REMOTE_CURL_ALIASES); do
>
> which results in "/bin/sh: syntax error at line 1 : `;' unexpected" as
> REMOTE_CURL_ALIASES is empty.

I can't reproduce this. How exactly are you building? I'm using
$ NO_CURL=YesPlease make
... and it builds fine for me.

-- Ram

Re: installation issue when building with NO_CURL=YesPlease

[Paul Walker]

On 26 May 2010, at 14:58, Ramkumar Ramachandra wrote:

> Hi,
>
>> As I could not find any bug reporting information on the wiki I  
>> thought I
>> would mention this here, please let me know if there is a better  
>> forum for
>> bug reports.
>
> Bugs are usually reported on this mailing list.
>
>>  I believe the latest git release (1.7.1.) has an installation
>> bug when building with "NO_CURL=YesPlease".  Looking at the  
>> Makefile line
>> 1999 it reads
>>
>> for p in $(REMOTE_CURL_ALIASES); do
>>
>> which results in "/bin/sh: syntax error at line 1 : `;' unexpected"  
>> as
>> REMOTE_CURL_ALIASES is empty.
>
> I can't reproduce this. How exactly are you building? I'm using
> $ NO_CURL=YesPlease make
> ... and it builds fine for me.
>
> -- Ram

I used:

gmake prefix=<path> NO_CURL=YesPlease NO_PYTHON=YesPlease all

which works fine, followed by

gmake prefix=<path> NO_CURL=YesPlease NO_PYTHON=YesPlease install

which reported the above error

[PATCH] Makefile: reenable install with NO_CURL

[Michael J Gruber]

Setting NO_CURL leaves some variables like REMOTE_CURL_ALIASES
empty, which creates no fun when for-looping over
$(REMOTE_CURL_ALIASES) unconditionally. Make it conditional.

Reported-by: Paul Walker <PWalker752@aol.com>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Against master but suggested for maint.
Note that the problem occurs during "make install", not "make".

 Makefile |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 07cab8f..d5d6565 100644
--- a/Makefile
+++ b/Makefile
@@ -2008,12 +2008,13 @@ endif
 		ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
 		cp "$$execdir/git$X" "$$execdir/$$p" || exit; \
 	  done; } && \
-	{ for p in $(REMOTE_CURL_ALIASES); do \
+	{ test x"$(REMOTE_CURL_ALIASES)" = x || \
+		{ for p in $(REMOTE_CURL_ALIASES); do \
 		$(RM) "$$execdir/$$p" && \
 		ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
 		ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
 		cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; \
-	  done; } && \
+	  done; } ; } && \
 	./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
 
 install-gitweb:
-- 
1.7.1.232.g2311e.dirty

Re: installation issue when building with NO_CURL=YesPlease

[Paul Walker]

On 26 May 2010, at 14:58, Michael J Gruber wrote:

> Paul Walker venit, vidit, dixit 26.05.2010 15:35:
>> As I could not find any bug reporting information on the wiki I
>> thought I would mention this here, please let me know if there is a
>> better forum for bug reports.  I believe the latest git release
>> (1.7.1.) has an installation bug when building with
>> "NO_CURL=YesPlease".  Looking at the Makefile line 1999 it reads
>>
>> for p in $(REMOTE_CURL_ALIASES); do
>>
>> which results in "/bin/sh: syntax error at line 1 : `;' unexpected"  
>> as
>> REMOTE_CURL_ALIASES is empty.
>
> That seems to depend on the shell. My bash 4.1.2 doesn't care. What  
> does
> /bin/sh --version say for you?
>
> Michael

I am struggling to work this out as my version of sh does not support  
the --version flag.
I tried a bunch of other options provided by google with not success.
If it helps I believe that /bin/sh is a version of ksh that comes with  
AIX 6.1

Re: installation issue when building with NO_CURL=YesPlease

[Dirk Süsserott]

Am 26.05.2010 16:15 schrieb Paul Walker:
> 
> On 26 May 2010, at 14:58, Ramkumar Ramachandra wrote:
> 
>> Hi,
>>
>>> As I could not find any bug reporting information on the wiki I 
>>> thought I
>>> would mention this here, please let me know if there is a better 
>>> forum for
>>> bug reports.
>>
>> Bugs are usually reported on this mailing list.
>>
>>>  I believe the latest git release (1.7.1.) has an installation
>>> bug when building with "NO_CURL=YesPlease".  Looking at the Makefile 
>>> line
>>> 1999 it reads
>>>
>>> for p in $(REMOTE_CURL_ALIASES); do
>>>
>>> which results in "/bin/sh: syntax error at line 1 : `;' unexpected" as
>>> REMOTE_CURL_ALIASES is empty.
>>
>> I can't reproduce this. How exactly are you building? I'm using
>> $ NO_CURL=YesPlease make
>> ... and it builds fine for me.
>>
>> -- Ram
> 
> I used:
> 
> gmake prefix=<path> NO_CURL=YesPlease NO_PYTHON=YesPlease all
> 
> which works fine, followed by
> 
> gmake prefix=<path> NO_CURL=YesPlease NO_PYTHON=YesPlease install
> 
> which reported the above error
> 

I had a similar problem when "make install"ing under AIX. Not with 
NO_CURL but with some other NO_* option. I forgot which.
This yealded to an empty $(REMOTE_***_ALIASES) macro 
(REMOTE_CURL_ALIASES in your case) which my AIX shell cannot handle.

It reads "for p in; do" which makes it unhappy. I solved my problem with 
  the SHELL_PATH environment variable (look at the first few lines in 
the Makefile).

$ SHELL_PATH=/bin/bash NO_SOMETHING=YesPlease make install

then worked fine for me.

HTH,
     Dirk

Re: installation issue when building with NO_CURL=YesPlease

[Bruce Stephens]

Dirk Süsserott <newsletter@dirk.my1.cc> writes:

[...]

> I had a similar problem when "make install"ing under AIX. Not with
> NO_CURL but with some other NO_* option. I forgot which.
> This yealded to an empty $(REMOTE_***_ALIASES) macro
> (REMOTE_CURL_ALIASES in your case) which my AIX shell cannot handle.
>
> It reads "for p in; do" which makes it unhappy. I solved my problem
> with the SHELL_PATH environment variable (look at the first few lines
> in the Makefile).
>
> $ SHELL_PATH=/bin/bash NO_SOMETHING=YesPlease make install
>
> then worked fine for me.

It wouldn't be too horrible to fix the Makefiles, though.  Doing stuff
like this works portably (judging by what some OpenSSL Makefiles do):

	foo="$(REMOTE_CURL_ALIASES)"; for i in $$foo; do \

[...]

Re: installation issue when building with NO_CURL=YesPlease

[Dirk Süsserott]

Am 26.05.2010 20:45 schrieb Bruce Stephens:
> Dirk Süsserott <newsletter@dirk.my1.cc> writes:
> 
> [...]
> 
>> I had a similar problem when "make install"ing under AIX. Not with
>> NO_CURL but with some other NO_* option. I forgot which.
>> This yealded to an empty $(REMOTE_***_ALIASES) macro
>> (REMOTE_CURL_ALIASES in your case) which my AIX shell cannot handle.
>>
>> It reads "for p in; do" which makes it unhappy. I solved my problem
>> with the SHELL_PATH environment variable (look at the first few lines
>> in the Makefile).
>>
>> $ SHELL_PATH=/bin/bash NO_SOMETHING=YesPlease make install
>>
>> then worked fine for me.
> 
> It wouldn't be too horrible to fix the Makefiles, though.  Doing stuff
> like this works portably (judging by what some OpenSSL Makefiles do):
> 
> 	foo="$(REMOTE_CURL_ALIASES)"; for i in $$foo; do \
> 
> [...]
> 

Bruce,

I just saw that Michael posted a patch which tests for the emptyness of 
REMOTE_CURL_ALIASES. Probably that's a more convenient solution.

Dirk

[PATCH 1/2] Makefile: remove some unnecessary curly braces

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>


Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 Makefile |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 9aca8a1..527d872 100644
--- a/Makefile
+++ b/Makefile
@@ -2079,19 +2079,19 @@ endif
 		test -z "$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
 		ln "$$bindir/git$X" "$$execdir/git$X" 2>/dev/null || \
 		cp "$$bindir/git$X" "$$execdir/git$X"; } ; } && \
-	{ for p in $(BUILT_INS); do \
+	for p in $(BUILT_INS); do \
 		$(RM) "$$execdir/$$p" && \
 		ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
 		ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
 		cp "$$execdir/git$X" "$$execdir/$$p" || exit; \
-	  done; } && \
+	done && \
 	{ test x"$(REMOTE_CURL_ALIASES)" = x || \
-		{ for p in $(REMOTE_CURL_ALIASES); do \
+		for p in $(REMOTE_CURL_ALIASES); do \
 		$(RM) "$$execdir/$$p" && \
 		ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
 		ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
 		cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; \
-	  done; } ; } && \
+	done; } && \
 	./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
 
 install-gitweb:
-- 
1.7.2.rc1

[PATCH 2/2] Makefile: work around ksh's failure to handle missing list argument to for loop

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

ksh does not like it when the list argument is missing in a for loop.  This
can happen when NO_CURL is set which causes REMOTE_CURL_ALIASES to be unset.
In this case, the for loop in the Makefile expands to look like this:

   for p in ; do

and ksh complains like this:

   /bin/ksh: syntax error at line 15 : `;' unexpected

The existing attempt to work around this issue, introduced by 70b89f87,
tried to protect the for loop by first testing whether REMOTE_CURL_ALIASES
was empty, but it does not seem to work.  So adopt Bruce Stephens's
suggestion (which comes from OpenSSL) for working around this issue.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 Makefile |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 527d872..bc3c570 100644
--- a/Makefile
+++ b/Makefile
@@ -2085,13 +2085,13 @@ endif
 		ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
 		cp "$$execdir/git$X" "$$execdir/$$p" || exit; \
 	done && \
-	{ test x"$(REMOTE_CURL_ALIASES)" = x || \
-		for p in $(REMOTE_CURL_ALIASES); do \
+	remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \
+	for p in $$remote_curl_aliases; do \
 		$(RM) "$$execdir/$$p" && \
 		ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
 		ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
 		cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; \
-	done; } && \
+	done && \
 	./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
 
 install-gitweb:
-- 
1.7.2.rc1

Re: [PATCH 2/2] Makefile: work around ksh's failure to handle missing list argument to for loop

[Raja R Harinath]

Hi,

Brandon Casey <casey@nrlssc.navy.mil> writes:

> ksh does not like it when the list argument is missing in a for loop.  This
> can happen when NO_CURL is set which causes REMOTE_CURL_ALIASES to be unset.
> In this case, the for loop in the Makefile expands to look like this:
>
>    for p in ; do
>
> and ksh complains like this:
>
>    /bin/ksh: syntax error at line 15 : `;' unexpected
[snip]
> -	{ test x"$(REMOTE_CURL_ALIASES)" = x || \
> -		for p in $(REMOTE_CURL_ALIASES); do \
> +	remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \
> +	for p in $$remote_curl_aliases; do \

I believe the idiom

  test x'$(foo)' = x || for p in ''$(foo); do

works equally well in this case, and is less invasive.

- Hari

Re: [PATCH 2/2] Makefile: work around ksh's failure to handle missing list argument to for loop

[Michael J Gruber]

Brandon Casey venit, vidit, dixit 02.07.2010 20:50:
> From: Brandon Casey <drafnel@gmail.com>
> 
> ksh does not like it when the list argument is missing in a for loop.  This
> can happen when NO_CURL is set which causes REMOTE_CURL_ALIASES to be unset.
> In this case, the for loop in the Makefile expands to look like this:
> 
>    for p in ; do
> 
> and ksh complains like this:
> 
>    /bin/ksh: syntax error at line 15 : `;' unexpected
> 
> The existing attempt to work around this issue, introduced by 70b89f87,
> tried to protect the for loop by first testing whether REMOTE_CURL_ALIASES
> was empty, but it does not seem to work.  So adopt Bruce Stephens's

What does that mean? Either it works or it doesn't. I did work back
then. Does it (i.e.: the test for emtyness) fail to work for certain shells?

Michael

Re: [PATCH 2/2] Makefile: work around ksh's failure to handle missing list argument to for loop

[Johannes Sixt]

Am 7/4/2010 20:37, schrieb Michael J Gruber:
> Brandon Casey venit, vidit, dixit 02.07.2010 20:50:
>> In this case, the for loop in the Makefile expands to look like this:
>>
>>    for p in ; do
>>
>> and ksh complains like this:
>>
>>    /bin/ksh: syntax error at line 15 : `;' unexpected
>>
>> The existing attempt to work around this issue, introduced by 70b89f87,
>> tried to protect the for loop by first testing whether REMOTE_CURL_ALIASES
>> was empty, but it does not seem to work.  So adopt Bruce Stephens's
> 
> What does that mean? Either it works or it doesn't. I did work back
> then. Does it (i.e.: the test for emtyness) fail to work for certain shells?

Before the test for emptyness can happen, the complete statement must be
parsed, but ksh finds a syntax error in the statement and, therefore,
cannot even begin to execute the statement. (ksh doesn't follow POSIX in
this regard, where this would not be a syntax error.)

-- Hannes

Re: [PATCH 2/2] Makefile: work around ksh's failure to handle missing list argument to for loop

[Michael J Gruber]

Johannes Sixt venit, vidit, dixit 05.07.2010 08:19:
> Am 7/4/2010 20:37, schrieb Michael J Gruber:
>> Brandon Casey venit, vidit, dixit 02.07.2010 20:50:
>>> In this case, the for loop in the Makefile expands to look like this:
>>>
>>>    for p in ; do
>>>
>>> and ksh complains like this:
>>>
>>>    /bin/ksh: syntax error at line 15 : `;' unexpected
>>>
>>> The existing attempt to work around this issue, introduced by 70b89f87,
>>> tried to protect the for loop by first testing whether REMOTE_CURL_ALIASES
>>> was empty, but it does not seem to work.  So adopt Bruce Stephens's
>>
>> What does that mean? Either it works or it doesn't. I did work back
>> then. Does it (i.e.: the test for emtyness) fail to work for certain shells?
> 
> Before the test for emptyness can happen, the complete statement must be
> parsed, but ksh finds a syntax error in the statement and, therefore,
> cannot even begin to execute the statement. (ksh doesn't follow POSIX in
> this regard, where this would not be a syntax error.)

OK, thanks for clarifying. I suggest this to go into the commit message
so that the "does not seem to work" is qualified.

The OP back then (before 70b89f87) used ksh on AIX 6.1, but maybe he
left the thread without testing. I assume Hari's suggestion works on
ksh, as well?

If we go for Brandon's version: Is there a reason for small-casing the
var name? It looks as if we had two different variables with different
case (which we don't).

BTW: Is the $$var gmake specific? Has anyone tested the new version on,
say, AIX, not just on Linux with ksh?

Michael

Re: [PATCH 2/2] Makefile: work around ksh's failure to handle missing list argument to for loop

[Johannes Sixt]

Am 7/5/2010 10:14, schrieb Michael J Gruber:
> BTW: Is the $$var gmake specific?

No. $ is a (special?) make variable that contains only a dollar sign. To
expand the variable in the Makefile, you have to write $$ (like for any
other Makefile variable whose name has only a single character, like $@,
$<, etc). As a result, you get a single dollar sign in the shell command
text. :-)

-- Hannes

Re: [PATCH 2/2] Makefile: work around ksh's failure to handle missing list argument to for loop

[Michael J Gruber]

Johannes Sixt venit, vidit, dixit 05.07.2010 11:15:
> Am 7/5/2010 10:14, schrieb Michael J Gruber:
>> BTW: Is the $$var gmake specific?
> 
> No. $ is a (special?) make variable that contains only a dollar sign. To
> expand the variable in the Makefile, you have to write $$ (like for any
> other Makefile variable whose name has only a single character, like $@,
> $<, etc). As a result, you get a single dollar sign in the shell command
> text. :-)

I guess I need this in my personal tree:

diff --git a/Michael b/Michael
index 5318944..2ff6a75 100644
--- a/Michael
+++ b/Michael
@@ -0815,2 +0815,2 @@ endif

-stupid remark about lower case
-stupid question about escaping $
+compensate for lack of morning coffee
+note that an empty shell var is more than nothing at all

Michael

Re: [PATCH 2/2] Makefile: work around ksh's failure to handle missing list argument to for loop

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Before the test for emptyness can happen, the complete statement must be
> parsed, but ksh finds a syntax error in the statement and, therefore,
> cannot even begin to execute the statement. (ksh doesn't follow POSIX in
> this regard, where this would not be a syntax error.)

I had to stare at Brandon's patch that was essentially:

-    for p in $(FOO); do echo $$p; done
+    foo=$(FOO); for p in $$foo; do echo $$p; done

and the above two doesn't look like there should be any difference; your
explanation makes quite a lot of sense but that's arcane.  I doubt I will
be able to justify and explain the latter construction without consulting
your message I am responsing to, if somebody asks "why do we assign $(FOO)
to a shell variable and then iterate over it?" 6 months from now.

It might make sense to use $(foreach) instead of rolling our own loop in
the shell to avoid glitches like this.

 Makefile |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/Makefile b/Makefile
index 9aca8a1..8bbb574 100644
--- a/Makefile
+++ b/Makefile
@@ -2085,13 +2085,13 @@ endif
 		ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
 		cp "$$execdir/git$X" "$$execdir/$$p" || exit; \
 	  done; } && \
-	{ test x"$(REMOTE_CURL_ALIASES)" = x || \
-		{ for p in $(REMOTE_CURL_ALIASES); do \
-		$(RM) "$$execdir/$$p" && \
-		ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
-		ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
-		cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; \
-	  done; } ; } && \
+	$(foreach p,$(REMOTE_CURL_ALIASES), \
+		{ \
+		$(RM) "$$execdir/$p" && \
+		ln "$$execdir/git-remote-http$X" "$$execdir/$p" 2>/dev/null || \
+		ln -s "git-remote-http$X" "$$execdir/$p" 2>/dev/null || \
+		cp "$$execdir/git-remote-http$X" "$$execdir/$p" || exit; \
+		} && ) : \
 	./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
 
 install-gitweb:

Re: [PATCH 2/2] Makefile: work around ksh's failure to handle missing list argument to for loop

[Andreas Schwab]

Junio C Hamano <gitster@pobox.com> writes:

> +	$(foreach p,$(REMOTE_CURL_ALIASES), \
> +		{ \
> +		$(RM) "$$execdir/$p" && \
> +		ln "$$execdir/git-remote-http$X" "$$execdir/$p" 2>/dev/null || \
> +		ln -s "git-remote-http$X" "$$execdir/$p" 2>/dev/null || \
> +		cp "$$execdir/git-remote-http$X" "$$execdir/$p" || exit; \
> +		} && ) : \
                       ^^
>  	./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"

Missing && at the end of the preceding line (otherwise the last line is
effectively commented out).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 2/2] Makefile: work around ksh's failure to handle missing list argument to for loop

[Brandon Casey]

It looks like Johannes has already supplied the explanation that was
missing from my commit message.  Thanks.


On Mon, Jul 5, 2010 at 1:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j.sixt@viscovery.net> writes:
>
>> Before the test for emptyness can happen, the complete statement must be
>> parsed, but ksh finds a syntax error in the statement and, therefore,
>> cannot even begin to execute the statement. (ksh doesn't follow POSIX in
>> this regard, where this would not be a syntax error.)
>
> I had to stare at Brandon's patch that was essentially:
>
> -    for p in $(FOO); do echo $$p; done
> +    foo=$(FOO); for p in $$foo; do echo $$p; done
>
> and the above two doesn't look like there should be any difference; your
> explanation makes quite a lot of sense but that's arcane.  I doubt I will
> be able to justify and explain the latter construction without consulting
> your message I am responsing to, if somebody asks "why do we assign $(FOO)
> to a shell variable and then iterate over it?" 6 months from now.
>
> It might make sense to use $(foreach) instead of rolling our own loop in
> the shell to avoid glitches like this.

$(foreach) works too.  I only avoided it because it has already caused a
problem once before by creating a command line that exceeded the
maximum argument list length on IRIX.

REMOTE_CURL_ALIASES only has 3 items in it right now, and probably
won't grow much larger, if it grows at all, so there is little chance of
exceeding the maximum argument list length on IRIX.  So $(foreach) is
fine with me if you think that reads better.

-Brandon

Re: [PATCH 2/2] Makefile: work around ksh's failure to handle missing list argument to for loop

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> $(foreach) works too.  I only avoided it because it has already caused a
> problem once before by creating a command line that exceeded the
> maximum argument list length on IRIX.

Ok, fair enough.

> REMOTE_CURL_ALIASES only has 3 items in it right now, and probably
> won't grow much larger, if it grows at all, so there is little chance of
> exceeding the maximum argument list length on IRIX.  So $(foreach) is
> fine with me if you think that reads better.

Well your patch fixes the issue, and I would actually prefer it as long as
it is explained well ;-).

Thanks.

[PATCH 2/2 v2] Makefile: work around ksh's failure to handle missing list argument to for loop

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

ksh does not like it when the list argument is missing in a 'for' loop.
This can happen when NO_CURL is set which causes REMOTE_CURL_ALIASES to be
unset.  In this case, the 'for' loop in the Makefile is expanded to look
like this:

   for p in ; do

and ksh complains like this:

   /bin/ksh: syntax error at line 15 : `;' unexpected

The existing attempt to work around this issue, introduced by 70b89f87,
tried to protect the 'for' loop by first testing whether REMOTE_CURL_ALIASES
was empty, but this does not work since, as Johannes Sixt explains, "Before
the test for emptyness can happen, the complete statement must be parsed,
but ksh finds a syntax error in the statement and, therefore, cannot even
begin to execute the statement. (ksh doesn't follow POSIX in this regard,
where this would not be a syntax error.)".

Make's $(foreach) function could be used to avoid this shell glitch, but
since it has already caused a problem once before by generating a command
line that exceeded the maximum argument list length on IRIX, let's adopt
Bruce Stephens's suggestion for working around this issue in the same way
the OpenSSL folks have done it.  This solution first assigns the contents
of the REMOTE_CURL_ALIASES make variable to a shell variable and then
supplies the shell variable as the list argument in the 'for' loop.  This
satisfies ksh and has the expected behavior even if $(REMOTE_CURL_ALIASES)
is empty.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---


On 07/05/2010 09:36 PM, Junio C Hamano wrote:
> Well your patch fixes the issue, and I would actually prefer it as long as
> it is explained well ;-).

Heh, ok, hopefully this commit message does a better job.

-Brandon


 Makefile |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 527d872..bc3c570 100644
--- a/Makefile
+++ b/Makefile
@@ -2085,13 +2085,13 @@ endif
 		ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
 		cp "$$execdir/git$X" "$$execdir/$$p" || exit; \
 	done && \
-	{ test x"$(REMOTE_CURL_ALIASES)" = x || \
-		for p in $(REMOTE_CURL_ALIASES); do \
+	remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \
+	for p in $$remote_curl_aliases; do \
 		$(RM) "$$execdir/$$p" && \
 		ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
 		ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
 		cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; \
-	done; } && \
+	done && \
 	./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
 
 install-gitweb:
-- 
1.7.2.rc1