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

Subsystems: kernel build + files below scripts/ (unless maintained elsewhere), the rest

4 messages, 3 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:49:05

Johannes Sixt [off-list ref] 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

From: Brandon Casey <hidden>
Date: 2016-06-15 22:49:05

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 [off-list ref] wrote:
Johannes Sixt [off-list ref] writes:
quoted
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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:49:05

Brandon Casey [off-list ref] 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

From: Brandon Casey <hidden>
Date: 2016-06-15 22:49:05

From: Brandon Casey <redacted>

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 <redacted>
---


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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help