Re: [PATCH v2] Do not install shell libraries executable

[Junio C Hamano <gitster@pobox.com>]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Last time I also missed git-parse-remote, so while at it, I am taking
> the opportunity to add that to SCRIPT_LIB_SH, too.

Your patch says it was generated by 1.7.0-rc1, but the change itself
seems to be based on an older version.  Curious.

How much would it hurt the distro packagers, if we don't take this patch
before 1.7.0?  If this would help a lot, let's give it a bit higher
priority and make sure 1.7.0 ships with (a corrected version of) it;
otherwise I'd say we should not merge this before 1.7.0.

> +SCRIPT_LIB_SH += git-mergetool--lib.sh
> +SCRIPT_LIB_SH += git-parse-remote.sh
> +SCRIPT_LIB_SH += git-sh-setup.sh
> + ...
> @@ -1792,6 +1802,7 @@ install: all
>  	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(bindir_SQ)'
>  	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
>  	$(INSTALL) $(ALL_PROGRAMS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
> +	$(INSTALL) -m 644 $(SCRIPT_LIB_SH) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'

I can understand that you didn't want to include the "included scriptlets"
as part of ALL_PROGRAMS because $(INSTALL) may flip 'x' bit on.

But you should then be installing %(patsubst %.sh,%,$(SCRIPT_LIB_SH));
otherwise, you are installing git-sh-setup.sh and

    . git-sh-setup

would not work.  Have you ever tested this?

> @@ -1901,7 +1912,7 @@ distclean: clean
>  clean:
>  	$(RM) *.o block-sha1/*.o ppc/*.o compat/*.o compat/*/*.o xdiff/*.o \
>  		$(LIB_FILE) $(XDIFF_LIB)
> -	$(RM) $(ALL_PROGRAMS) $(BUILT_INS) git$X
> +	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB_SH) $(BUILT_INS) git$X

And this is even worse.  You are removing the _source_ here.  I can see
you didn't even test the very basic: "make clean && make".

> @@ -1930,7 +1941,7 @@ endif
>  ### Check documentation
>  #
>  check-docs::
> -	@(for v in $(ALL_PROGRAMS) $(BUILT_INS) git gitk; \
> +	@(for v in $(ALL_PROGRAMS) $(SCRIPT_LIB_SH) $(BUILT_INS) git gitk; \

Likewise.

>  	do \
>  		case "$$v" in \
>  		git-merge-octopus | git-merge-ours | git-merge-recursive | \
> @@ -1975,7 +1986,7 @@ check-docs::
>  		documented,gittutorial-2 | \
>  		sentinel,not,matching,is,ok ) continue ;; \
>  		esac; \
> -		case " $(ALL_PROGRAMS) $(BUILT_INS) git gitk " in \
> +		case " $(ALL_PROGRAMS) $(SCRIPT_LIB_SH) $(BUILT_INS) git gitk " in \

Likewise.

Wouldn't it make a bit more sense to do it like this instead?  I at least
did "make clean && make" ;-)

-- >8 --
From: Jonathan Nieder <jrnieder@gmail.com>
Subject: [PATCH] Do not install shell libraries executable

Some scripts are expected to be sourced instead of executed on their own.
Avoid some confusion by not marking them executable.

The executable bit was confusing the valgrind support of our test scripts,
which assumed that any executable without a #!-line should be intercepted
and run through valgrind.  So during valgrind-enabled tests, any script
sourcing these files actually sourced the valgrind interception script
instead.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile            |   42 ++++++++++++++++++++++++++++--------------
 1 files changed, 28 insertions(+), 14 deletions(-)
 mode change 100755 => 100644 git-parse-remote.sh
 mode change 100755 => 100644 git-sh-setup.sh

diff --git a/Makefile b/Makefile
index af08c8f..6bbeb24 100644
--- a/Makefile
+++ b/Makefile
@@ -341,6 +341,7 @@ PROGRAMS =
 SCRIPT_PERL =
 SCRIPT_PYTHON =
 SCRIPT_SH =
+SCRIPT_LIB =
 TEST_PROGRAMS =
 
 SCRIPT_SH += git-am.sh
@@ -352,20 +353,21 @@ SCRIPT_SH += git-merge-octopus.sh
 SCRIPT_SH += git-merge-one-file.sh
 SCRIPT_SH += git-merge-resolve.sh
 SCRIPT_SH += git-mergetool.sh
-SCRIPT_SH += git-mergetool--lib.sh
 SCRIPT_SH += git-notes.sh
-SCRIPT_SH += git-parse-remote.sh
 SCRIPT_SH += git-pull.sh
 SCRIPT_SH += git-quiltimport.sh
 SCRIPT_SH += git-rebase--interactive.sh
 SCRIPT_SH += git-rebase.sh
 SCRIPT_SH += git-repack.sh
 SCRIPT_SH += git-request-pull.sh
-SCRIPT_SH += git-sh-setup.sh
 SCRIPT_SH += git-stash.sh
 SCRIPT_SH += git-submodule.sh
 SCRIPT_SH += git-web--browse.sh
 
+SCRIPT_LIB += git-mergetool--lib
+SCRIPT_LIB += git-parse-remote
+SCRIPT_LIB += git-sh-setup
+
 SCRIPT_PERL += git-add--interactive.perl
 SCRIPT_PERL += git-difftool.perl
 SCRIPT_PERL += git-archimport.perl
@@ -1454,7 +1456,7 @@ export TAR INSTALL DESTDIR SHELL_PATH
 
 SHELL = $(SHELL_PATH)
 
-all:: shell_compatibility_test $(ALL_PROGRAMS) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
+all:: shell_compatibility_test $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
 ifneq (,$X)
 	$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
 endif
@@ -1505,17 +1507,25 @@ common-cmds.h: ./generate-cmdlist.sh command-list.txt
 common-cmds.h: $(wildcard Documentation/git-*.txt)
 	$(QUIET_GEN)./generate-cmdlist.sh > $@+ && mv $@+ $@
 
+define cmd_munge_script
+$(RM) $@ $@+ && \
+sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+    -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
+    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
+    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
+    -e $(BROKEN_PATH_FIX) \
+    $@.sh >$@+
+endef
+
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
-	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
-	    -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
-	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
-	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
-	    -e $(BROKEN_PATH_FIX) \
-	    $@.sh >$@+ && \
+	$(QUIET_GEN)$(cmd_munge_script) && \
 	chmod +x $@+ && \
 	mv $@+ $@
 
+$(SCRIPT_LIB) : % : %.sh
+	$(QUIET_GEN)$(cmd_munge_script) && \
+	mv $@+ $@
+
 ifndef NO_PERL
 $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 
@@ -1866,6 +1876,7 @@ install: all
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) $(ALL_PROGRAMS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
+	$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
 ifndef NO_PERL
@@ -1985,7 +1996,7 @@ distclean: clean
 clean:
 	$(RM) *.o block-sha1/*.o ppc/*.o compat/*.o compat/*/*.o xdiff/*.o \
 		$(LIB_FILE) $(XDIFF_LIB)
-	$(RM) $(ALL_PROGRAMS) $(BUILT_INS) git$X
+	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
 	$(RM) $(TEST_PROGRAMS)
 	$(RM) -r bin-wrappers
 	$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h TAGS tags cscope*
@@ -2017,7 +2028,7 @@ endif
 ### Check documentation
 #
 check-docs::
-	@(for v in $(ALL_PROGRAMS) $(BUILT_INS) git gitk; \
+	@(for v in $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git gitk; \
 	do \
 		case "$$v" in \
 		git-merge-octopus | git-merge-ours | git-merge-recursive | \
@@ -2060,9 +2071,12 @@ check-docs::
 		documented,gitrepository-layout | \
 		documented,gittutorial | \
 		documented,gittutorial-2 | \
+		documented,git-bisect-lk2009 | \
+		documented.git-remote-helpers | \
+		documented,gitworkflows | \
 		sentinel,not,matching,is,ok ) continue ;; \
 		esac; \
-		case " $(ALL_PROGRAMS) $(BUILT_INS) git gitk " in \
+		case " $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git gitk " in \
 		*" $$cmd "*)	;; \
 		*) echo "removed but $$how: $$cmd" ;; \
 		esac; \
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
old mode 100755
new mode 100644
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
old mode 100755
new mode 100644
-- 
1.7.0.rc1.141.gd3fd2