Fix git-instaweb breakage on MacOS X due to the limited sed functionality

PATCH In review Charles Bailey 2 participants 9 messages Jun 15, 2016 Kbuild

Share & tools

Charles Bailey

author

2016-06-15 22:44 UTC #0 lore raw

PATCH 1/1 Fix git-instaweb breakage on MacOS X due to the limited sed functionality

git-instaweb relied on a pipe in a sed script, but this is not supported by MacOS X sed when using BREs. git-instaweb relies on a working perl in any case, and perl re are more consistent between platforms, so replace sed invocation with an equivalent perl invocation.

Also, fix the documented -b "" to work without giving a spurious 'command not found' error.

Signed-off-by Charles Bailey <[email protected]>

---
 git-instaweb.sh |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

git-instaweb.sh +8 -6

index 42d8d7f..3565734 100755
@@ -232,16 +232,18 @@ EOF
 }
 
 script='
-s#^\(my\|our\) $projectroot =.*#\1 $projectroot = "'$(dirname "$fqgitdir")'";#
-s#\(my\|our\) $gitbin =.*#\1 $gitbin = "'$GIT_EXEC_PATH'";#
-s#\(my\|our\) $projects_list =.*#\1 $projects_list = $projectroot;#
-s#\(my\|our\) $git_temp =.*#\1 $git_temp = "'$fqgitdir/gitweb/tmp'";#'
+s#^(my|our) \$projectroot =.*#$1 \$projectroot = "'$(dirname "$fqgitdir")'";#;
+s#(my|our) \$gitbin =.*#$1 \$gitbin = "'$GIT_EXEC_PATH'";#;
+s#(my|our) \$projects_list =.*#$1 \$projects_list = \$projectroot;#;
+s#(my|our) \$git_temp =.*#$1 \$git_temp = "'$fqgitdir/gitweb/tmp'";#;'
 
 gitweb_cgi () {
 	cat > "$1.tmp" <<\EOFGITWEB
 @@GITWEB_CGI@@
 EOFGITWEB
-	sed "$script" "$1.tmp"  > "$1"
+	# The generated scripts assume that perl is /usr/bin/perl
+	# so the assumption here should be no more harmful
+	/usr/bin/perl -p -e "$script" "$1.tmp"  > "$1"
 	chmod +x "$1"
 	rm -f "$1.tmp"
 }

@@ -273,4 +275,4 @@ esac
 
 start_httpd
 url=http://127.0.0.1:$port
-"$browser" $url || echo $url
+test -n "$browser" && "$browser" $url || echo $url

Signature · 1 line

1.5.4.rc0

Wincent Colaiuta

2016-06-15 22:44 UTC #1 lore raw

git-instaweb.sh EOF · line 246

Quoted 4 lines · Charles Bailey

git-instaweb.sh +3 -1

-	sed "$script" "$1.tmp"  > "$1"
+	# The generated scripts assume that perl is /usr/bin/perl
+	# so the assumption here should be no more harmful
+	/usr/bin/perl -p -e "$script" "$1.tmp"  > "$1"

I think it's a bad idea to hard-code the perl path there; the generated scripts only assume /usr/bin/perl if the user hasn't overridden it at build time:

PERL_PATH=/foo/perl make ...

Using just this should work fine anyway:

	perl -p -e ...

Cheers,
Wincent

Charles Bailey

author

2016-06-15 22:44 UTC #2 lore raw

Quoted 11 lines · Wincent Colaiuta

I think it's a bad idea to hard-code the perl path there; the generated scripts only assume /usr/bin/perl if the user hasn't overridden it at build time:
PERL_PATH=/foo/perl make ...
Using just this should work fine anyway:
	perl -p -e ...

I agree completely, but all the generated scripts output hard coded paths so it would seem inconsistent not to qualify the path in this case too. Would @@PERL_PATH@@perl -p -e work, do you know?

Charles.

Wincent Colaiuta

2016-06-15 22:44 UTC #3 lore raw

Quoted 8 lines · Charles Bailey

Quoted 7 lines · Wincent Colaiuta
I think it's a bad idea to hard-code the perl path there; the generated scripts only assume /usr/bin/perl if the user hasn't overridden it at build time:
PERL_PATH=/foo/perl make ...

Quoted 8 lines

Quoted 4 lines
Using just this should work fine anyway:
	perl -p -e ...
I agree completely, but all the generated scripts output hard coded paths so it would seem inconsistent not to qualify the path in this case too.

It's not hard-coded, it's dynamic. Witness:

$ make PERL_PATH=/Volumes/Clon/usr/bin/perl
$ head -1 git-add--interactive
#!/Volumes/Clon/usr/bin/perl -w

Quoted 1 line

Would @@PERL_PATH@@perl -p -e work, do you know?

I don't think so, but judging from the following section of the Makefile, I think @@PERL@@ would. Why don't you give it a try?

$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh

         $(QUIET_GEN)$(RM) $@ $@+ && \
         sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
             -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
             -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
             -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
             -e 's|@@HTMLDIR@@|$(htmldir_SQ)|g' \
             [email protected] >$@+ && \
         chmod +x $@+ && \
         mv $@+ $@

Cheers,
Wincent

Charles Bailey

author

2016-06-15 22:44 UTC #4 lore raw

Quoted 9 lines · Wincent Colaiuta

Quoted 4 lines · Charles Bailey
I agree completely, but all the generated scripts output hard coded paths so it would seem inconsistent not to qualify the path in this case too.
It's not hard-coded, it's dynamic. Witness:

It's *output* hardcoded, it's dynamic during script generation.

A raw 'perl' in a shell script is dynamic during script run.

Quoted 10 lines

$ make PERL_PATH=/Volumes/Clon/usr/bin/perl
$ head -1 git-add--interactive
#!/Volumes/Clon/usr/bin/perl -w
Quoted 1 line
Would @@PERL_PATH@@perl -p -e work, do you know?
I don't think so, but judging from the following section of the Makefile, I think @@PERL@@ would. Why don't you give it a try?
$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh

Quoted 10 lines

        $(QUIET_GEN)$(RM) $@ $@+ && \
        sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
            -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
            -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
            -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
            -e 's|@@HTMLDIR@@|$(htmldir_SQ)|g' \
            [email protected] >$@+ && \
        chmod +x $@+ && \
        mv $@+ $@

git-instaweb is treated specially, so the answer is 'no, not yet', but I have a patch on the way.

Charles.

Charles Bailey

author

2016-06-15 22:44 UTC #5 lore raw

Also, fix the documented -b "" to work without giving a spurious 'command not found' error.

Signed-off-by Charles Bailey <[email protected]>

---
 Makefile        |    1 +
 git-instaweb.sh |   15 +++++++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

Makefile +1 -0

index 617e5f5..d6d3d65 100644
@@ -880,6 +880,7 @@ git-instaweb: git-instaweb.sh gitweb/gitweb.cgi gitweb/gitweb.css
 	    -e '/@@GITWEB_CGI@@/d' \
 	    -e '/@@GITWEB_CSS@@/r gitweb/gitweb.css' \
 	    -e '/@@GITWEB_CSS@@/d' \
+	    -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
 	    [email protected] > $@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@

git-instaweb.sh +9 -6

index 42d8d7f..9f86709 100755
@@ -3,6 +3,7 @@
 # Copyright (c) 2006 Eric Wong
 #
 
+PERL='@@PERL@@'
 OPTIONS_KEEPDASHDASH=
 OPTIONS_SPEC="\
 git-instaweb [options] (--start | --stop | --restart)

@@ -232,16 +233,18 @@ EOF
 }
 
 script='
-s#^\(my\|our\) $projectroot =.*#\1 $projectroot = "'$(dirname "$fqgitdir")'";#
-s#\(my\|our\) $gitbin =.*#\1 $gitbin = "'$GIT_EXEC_PATH'";#
-s#\(my\|our\) $projects_list =.*#\1 $projects_list = $projectroot;#
-s#\(my\|our\) $git_temp =.*#\1 $git_temp = "'$fqgitdir/gitweb/tmp'";#'
+s#^(my|our) \$projectroot =.*#$1 \$projectroot = "'$(dirname "$fqgitdir")'";#;
+s#(my|our) \$gitbin =.*#$1 \$gitbin = "'$GIT_EXEC_PATH'";#;
+s#(my|our) \$projects_list =.*#$1 \$projects_list = \$projectroot;#;
+s#(my|our) \$git_temp =.*#$1 \$git_temp = "'$fqgitdir/gitweb/tmp'";#;'
 
 gitweb_cgi () {
 	cat > "$1.tmp" <<\EOFGITWEB
 @@GITWEB_CGI@@
 EOFGITWEB
-	sed "$script" "$1.tmp"  > "$1"
+	# The generated scripts assume that perl is /usr/bin/perl
+	# so the assumption here should be no more harmful
+	"$PERL" -p -e "$script" "$1.tmp"  > "$1"
 	chmod +x "$1"
 	rm -f "$1.tmp"
 }

@@ -273,4 +276,4 @@ esac
 
 start_httpd
 url=http://127.0.0.1:$port
-"$browser" $url || echo $url
+test -n "$browser" && "$browser" $url || echo $url

Signature · 1 line

1.5.4.rc0

Charles Bailey

author

2016-06-15 22:44 UTC #6 lore raw

Now that I think about it, this replacement patch has invalidated the original comment to some extent. A replacement patch follows, with a more consistent comment.

Charles.

Charles Bailey

author

2016-06-15 22:44 UTC #7 lore raw

PATCH 1/1 Fix git-instaweb breakage on MacOS X due to the limited sed functionality

Also, fix the documented -b "" to work without giving a spurious 'command not found' error.

Signed-off-by Charles Bailey <[email protected]>

---
 Makefile        |    1 +
 git-instaweb.sh |   15 +++++++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

Makefile +1 -0

index 617e5f5..d6d3d65 100644
@@ -880,6 +880,7 @@ git-instaweb: git-instaweb.sh gitweb/gitweb.cgi gitweb/gitweb.css
 	    -e '/@@GITWEB_CGI@@/d' \
 	    -e '/@@GITWEB_CSS@@/r gitweb/gitweb.css' \
 	    -e '/@@GITWEB_CSS@@/d' \
+	    -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
 	    [email protected] > $@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@

git-instaweb.sh +9 -6

index 42d8d7f..ad0723c 100755
@@ -3,6 +3,7 @@
 # Copyright (c) 2006 Eric Wong
 #
 
+PERL='@@PERL@@'
 OPTIONS_KEEPDASHDASH=
 OPTIONS_SPEC="\
 git-instaweb [options] (--start | --stop | --restart)

@@ -232,16 +233,18 @@ EOF
 }
 
 script='
-s#^\(my\|our\) $projectroot =.*#\1 $projectroot = "'$(dirname "$fqgitdir")'";#
-s#\(my\|our\) $gitbin =.*#\1 $gitbin = "'$GIT_EXEC_PATH'";#
-s#\(my\|our\) $projects_list =.*#\1 $projects_list = $projectroot;#
-s#\(my\|our\) $git_temp =.*#\1 $git_temp = "'$fqgitdir/gitweb/tmp'";#'
+s#^(my|our) \$projectroot =.*#$1 \$projectroot = "'$(dirname "$fqgitdir")'";#;
+s#(my|our) \$gitbin =.*#$1 \$gitbin = "'$GIT_EXEC_PATH'";#;
+s#(my|our) \$projects_list =.*#$1 \$projects_list = \$projectroot;#;
+s#(my|our) \$git_temp =.*#$1 \$git_temp = "'$fqgitdir/gitweb/tmp'";#;'
 
 gitweb_cgi () {
 	cat > "$1.tmp" <<\EOFGITWEB
 @@GITWEB_CGI@@
 EOFGITWEB
-	sed "$script" "$1.tmp"  > "$1"
+	# Use the configured full path to perl to match the generated
+	# scripts' 'hashpling' line
+	"$PERL" -p -e "$script" "$1.tmp"  > "$1"
 	chmod +x "$1"
 	rm -f "$1.tmp"
 }

@@ -273,4 +276,4 @@ esac
 
 start_httpd
 url=http://127.0.0.1:$port
-"$browser" $url || echo $url
+test -n "$browser" && "$browser" $url || echo $url

Signature · 6 lines

1.5.4.rc0


-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/

Wincent Colaiuta

2016-06-15 22:44 UTC #8 lore raw

In reply to Charles Bailey

Quoted 10 lines · Charles Bailey

Quoted 7 lines · Wincent Colaiuta
Quoted 3 lines · Charles Bailey
I agree completely, but all the generated scripts output hard coded paths so it would seem inconsistent not to qualify the path in this case too.
It's not hard-coded, it's dynamic. Witness:
It's *output* hardcoded, it's dynamic during script generation.

Ah, ok. Seems like we were working with different definitions of "hard- coded". When I said "hard-coded" I meant "determined by you and not overrideable by the user doing the build without hand-editing the source file".

Quoted 1 line

A raw 'perl' in a shell script is dynamic during script run.

Sure, and seeing as you're only doing a simple find/replace any version of perl installed on the path is probably fine.

But if you are going to provide an absolute path then you should at least make it user-configurable like all the other perl paths.

Cheers,
Wincent