[PATCH] rerere: fix too-short initialization

[PATCH] rerere: fix too-short initialization

[Jeff King]

This was caused by a typo in the sizeof parameter, and meant
we looked at uninitialized memory.  Caught by valgrind in
t2030.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm running the whole test suite under valgrind for the current
'master'. This was the first hit, but it's very s-l-o-w, so there might
be more as the day progresses.

 rerere.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/rerere.c b/rerere.c
index a86d73d..d1d3e75 100644
--- a/rerere.c
+++ b/rerere.c
@@ -325,7 +325,7 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu
 	for (i = 0; i < 3; i++)
 		free(mmfile[i].ptr);
 
-	memset(&io, 0, sizeof(&io));
+	memset(&io, 0, sizeof(io));
 	io.io.getline = rerere_mem_getline;
 	if (output)
 		io.io.output = fopen(output, "w");
-- 
1.7.0.rc0.41.g538720

[PATCH 0/3] valgrind bug roundup

[Jeff King]

On Thu, Jan 28, 2010 at 09:52:16AM -0500, Jeff King wrote:

> I'm running the whole test suite under valgrind for the current
> 'master'. This was the first hit, but it's very s-l-o-w, so there might
> be more as the day progresses.

And here they are. The first two are actual bugs, the third silences a
false positive:

  [1/3]: fix memcpy of overlapping area
  [2/3]: fix off-by-one allocation error
  [3/3]: add shebang line to git-mergetool--lib.sh

It really has been a while since I've run the whole test suite under
valgrind. The breakage in 3/3 dates back to last April. The bug in 1/3
is also quite old (v1.5.0). I'm not sure why I didn't find it in
previous valgrind runs; perhaps it is only recent valgrinds that have
grown support for finding overlapping memcpy. The bug in 2/3 is new, and
has never been in a released version.

-Peff

[PATCH 1/3] fix memcpy of overlapping area

[Jeff King]

Caught by valgrind in t5500, but it is pretty obvious from
reading the code that this is shifting elements of an array
to the left, which needs memmove.

Signed-off-by: Jeff King <peff@peff.net>
---
This was introduced in f53514b (allow deepening of a shallow repository,
2006-10-30), released as part of v1.5.0. So probably 'maint'-worthy.

 commit.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/commit.c b/commit.c
index 632061c..731191e 100644
--- a/commit.c
+++ b/commit.c
@@ -224,7 +224,7 @@ int unregister_shallow(const unsigned char *sha1)
 	if (pos < 0)
 		return -1;
 	if (pos + 1 < commit_graft_nr)
-		memcpy(commit_graft + pos, commit_graft + pos + 1,
+		memmove(commit_graft + pos, commit_graft + pos + 1,
 				sizeof(struct commit_graft *)
 				* (commit_graft_nr - pos - 1));
 	commit_graft_nr--;
-- 
1.7.0.rc0.41.g538720

[PATCH 2/3] fix off-by-one allocation error

[Jeff King]

Caught by valgrind in t5516. Reading the code shows we
malloc enough for our string, but not trailing NUL.

Signed-off-by: Jeff King <peff@peff.net>
---
This bug was introduced in f517f1f (builtin-push: add --delete as
syntactic sugar for :foo, 2009-12-30), not released yet but part of
1.7.0-rc0. So no need for a 'maint' fix.

An obvious alternative would be to convert it to strbuf (which could
also be used to clean up other non-buggy string generation earlier in
the function).

 builtin-push.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 5df6608..5633f0a 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -52,7 +52,7 @@ static void set_refspecs(const char **refs, int nr)
 		} else if (deleterefs && !strchr(ref, ':')) {
 			char *delref;
 			int len = strlen(ref)+1;
-			delref = xmalloc(len);
+			delref = xmalloc(len+1);
 			strcpy(delref, ":");
 			strcat(delref, ref);
 			ref = delref;
-- 
1.7.0.rc0.41.g538720

[PATCH 3/3] add shebang line to git-mergetool--lib.sh

[Jeff King]

Even though this script is expected to be sourced instead of
executed on its own, the #!/bin/sh line provides simple
documentation about what format the file is in.

In particular, the lack of such a line 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 this file actually sourced the valgrind
interception script instead.

Signed-off-by: Jeff King <peff@peff.net>
---
The valgrind script could perhaps be a bit smarter instead, but checking
#!-lines is nice and simple, and this change makes other programs like
"file" happier, too.

This problem has been around since 21d0ba7 (difftool/mergetool: refactor
commands to use git-mergetool--lib, 2009-04-08), released in v1.6.3. But
since it is only about our internal tests, and even then only about
running them with valgrind enabled, I don't know if it is worth a fix on
'maint'.

 git-mergetool--lib.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 5b62785..51dd0d6 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -1,3 +1,4 @@
+#!/bin/sh
 # git-mergetool--lib is a library for common merge tool functions
 diff_mode() {
 	test "$TOOL_MODE" = diff
-- 
1.7.0.rc0.41.g538720

[PATCH] Do not install shell libraries executable

[Jonathan Nieder]

These 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 this file actually
sourced the valgrind interception script instead.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jeff King wrote:

> This problem has been around since 21d0ba7 (difftool/mergetool: refactor
> commands to use git-mergetool--lib, 2009-04-08), released in v1.6.3. But
> since it is only about our internal tests, and even then only about
> running them with valgrind enabled, I don't know if it is worth a fix on
> 'maint'.

It was also confusing dpkg-shlibdeps, so I recently came up with
this fix.  Both fixes seem like good changes to me, and both
could be applied.  Your fix has the virtue of being shorter,
hence safer.

 Makefile        |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)
 mode change 100755 => 100644 git-sh-setup.sh

diff --git a/Makefile b/Makefile
index fd7f51e..70dcc40 100644
--- a/Makefile
+++ b/Makefile
@@ -342,6 +342,7 @@ LIB_OBJS =
 PROGRAMS =
 SCRIPT_PERL =
 SCRIPT_SH =
+SCRIPT_LIB_SH =
 TEST_PROGRAMS =
 
 SCRIPT_SH += git-am.sh
@@ -353,7 +354,6 @@ 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
@@ -362,11 +362,13 @@ 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_SH += git-mergetool--lib.sh
+SCRIPT_LIB_SH += git-sh-setup.sh
+
 SCRIPT_PERL += git-add--interactive.perl
 SCRIPT_PERL += git-difftool.perl
 SCRIPT_PERL += git-archimport.perl
@@ -1428,7 +1430,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_SH) $(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
@@ -1488,6 +1490,15 @@ $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
 	chmod +x $@+ && \
 	mv $@+ $@
 
+$(patsubst %.sh,%,$(SCRIPT_LIB_SH)) : % : %.sh
+	$(QUIET_GEN)$(RM) $@ $@+ && \
+	sed -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 >$@+ && \
+	mv $@+ $@
+
 ifndef NO_PERL
 $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 
@@ -1792,6 +1803,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)'
 	$(INSTALL) git$X git-upload-pack$X git-receive-pack$X git-upload-archive$X git-shell$X git-cvsserver '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
 ifndef NO_PERL
@@ -1901,7 +1913,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
 	$(RM) $(TEST_PROGRAMS)
 	$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h TAGS tags cscope*
 	$(RM) -r autom4te.cache
@@ -1930,7 +1942,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; \
 	do \
 		case "$$v" in \
 		git-merge-octopus | git-merge-ours | git-merge-recursive | \
@@ -1975,7 +1987,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 \
 		*" $$cmd "*)	;; \
 		*) echo "removed but $$how: $$cmd" ;; \
 		esac; \
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
old mode 100755
new mode 100644
-- 
1.7.0.rc0

Re: [PATCH] Do not install shell libraries executable

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> @@ -1488,6 +1490,15 @@ $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
>  	chmod +x $@+ && \
>  	mv $@+ $@
>  
> +$(patsubst %.sh,%,$(SCRIPT_LIB_SH)) : % : %.sh
> +	$(QUIET_GEN)$(RM) $@ $@+ && \
> +	sed -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 >$@+ && \
> +	mv $@+ $@
> +

Can't you do something about this repetition from existing $(SCRIPT_SH)
munging?  Other than that the patch looks like Ok.

[PATCH v2] Do not install shell libraries executable

[Jonathan Nieder]

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>
---
Junio C Hamano wrote:

> Can't you do something about this repetition from existing $(SCRIPT_SH)
> munging?

Sorry about that.

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.

Good night,
Jonathan

 Makefile            |   39 +++++++++++++++++++++++++--------------
 1 files changed, 25 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 fd7f51e..ce42d93 100644
--- a/Makefile
+++ b/Makefile
@@ -342,6 +342,7 @@ LIB_OBJS =
 PROGRAMS =
 SCRIPT_PERL =
 SCRIPT_SH =
+SCRIPT_LIB_SH =
 TEST_PROGRAMS =
 
 SCRIPT_SH += git-am.sh
@@ -353,20 +354,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_SH += git-mergetool--lib.sh
+SCRIPT_LIB_SH += git-parse-remote.sh
+SCRIPT_LIB_SH += git-sh-setup.sh
+
 SCRIPT_PERL += git-add--interactive.perl
 SCRIPT_PERL += git-difftool.perl
 SCRIPT_PERL += git-archimport.perl
@@ -1428,7 +1430,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_SH) $(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
@@ -1477,17 +1479,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 $@+ $@
 
+$(patsubst %.sh,%,$(SCRIPT_LIB_SH)) : % : %.sh
+	$(QUIET_GEN)$(cmd_munge_script) && \
+	mv $@+ $@
+
 ifndef NO_PERL
 $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 
@@ -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)'
 	$(INSTALL) git$X git-upload-pack$X git-receive-pack$X git-upload-archive$X git-shell$X git-cvsserver '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
 ifndef NO_PERL
@@ -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
 	$(RM) $(TEST_PROGRAMS)
 	$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h TAGS tags cscope*
 	$(RM) -r autom4te.cache
@@ -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; \
 	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 \
 		*" $$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

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

[Junio C Hamano]

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

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

[Jonathan Nieder]

Junio C Hamano wrote:

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

Given that Peff’s fix is in, I don’t think it is needed at all.  So I
would say, better to let it wait.

> > +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?

Good catches, sorry. :(  I did test but clearly I was not thinking very
well when I looked at the result...

> Wouldn't it make a bit more sense to do it like this instead?

Yes, that looks right.  Thanks for cleaning up my mess!

Sorry for the trouble,
Jonathan

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>> 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.
>
> Given that Peff’s fix is in, I don’t think it is needed at all.  So I
> would say, better to let it wait.

I was referring to this from your original:

    It was also confusing dpkg-shlibdeps, so I recently came up with
    this fix.  Both fixes seem like good changes to me, and both
    could be applied.  Your fix has the virtue of being shorter,
    hence safer.

Is Jeff's mergetool-lib change enough to address this issue as well?

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

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Junio C Hamano wrote:
>>
>>> 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.
>>
>> Given that Peff’s fix is in, I don’t think it is needed at all.  So I
>> would say, better to let it wait.
>
> I was referring to this from your original:
> 
>     It was also confusing dpkg-shlibdeps, so I recently came up with
>     this fix.  Both fixes seem like good changes to me, and both
>     could be applied.  Your fix has the virtue of being shorter,
>     hence safer.
> 
> Is Jeff's mergetool-lib change enough to address this issue as well?

I just checked; looks like I was confusing a few issues.

 - dpkg-shlibdeps does not like to be fed scripts, period.  That
   has nothing to do with this.

 - debian/rules in the git-core package feeds every file in
   /usr/bin and gitexecdir that doesn’t start with #! to 'strip'.
   Jeff's change helps that; my fix has nothing to do with it.

 - some other tool must have been happier with these files not
   being executable, but I cannot reproduce this or find it now.

I wrote that patch late at night, and unfortunately, I cannot justify it
to myself now.  With Jeff’s change applied, there is no obvious breakage
that it fixes.  If a problem comes up again, I will let you know.

Embarrassed,
Jonathan

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I wrote that patch late at night, and unfortunately, I cannot justify it
> to myself now.  With Jeff’s change applied, there is no obvious breakage
> that it fixes.  If a problem comes up again, I will let you know.

Thanks for a thorough write-up.  Really appreciated.