threads / patch / 22426

patchrerere: fix too-short initialization

Subject: [PATCH] rerere: fix too-short initialization

## tl;dr

13 messages between Jan 28, 2010 and Jan 31, 2010. Diffs are folded; open one to read it.

replies: 12people: 3as markdown or json

Jeff King· Jan 28, 2010, 14:52 UTC · lore

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(-)
Show changes to rerere.c +1 −1
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
Jeff King· Jan 29, 2010, 10:25 UTC · re: Jeff King · lore

[PATCH 0/3] valgrind bug roundup

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
Jeff King· Jan 29, 2010, 10:28 UTC · re: Jeff King · lore

[PATCH 1/3] fix memcpy of overlapping area

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(-)
Show changes to commit.c +1 −1
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
Jeff King· Jan 29, 2010, 10:31 UTC · re: Jeff King · lore

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

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(-)
Show changes to builtin-push.c +1 −1
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
Jeff King· Jan 29, 2010, 10:37 UTC · re: Jeff King · lore

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

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(-)
Show changes to git-mergetool--lib.sh +1 −0
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
Jonathan Nieder· Jan 29, 2010, 14:50 UTC · re: Jeff King · lore

[PATCH] Do not install shell libraries executable

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:
Show 5 quoted lines
> 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
Show changes to 2 files +18 −6

Makefile, 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
Junio C Hamano· Jan 31, 2010, 07:08 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Do not install shell libraries executable

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 13 quoted lines
> @@ -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.

Jonathan Nieder· Jan 31, 2010, 08:34 UTC · re: Junio C Hamano · lore

[PATCH v2] 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>
---
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
Show changes to 3 files +25 −14

Makefile, git-parse-remote.sh, 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
Junio C Hamano· Jan 31, 2010, 19:46 UTC · re: Jonathan Nieder · lore

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

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.

Show 9 quoted lines
> +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?
Show 6 quoted lines
> @@ -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".

Show 6 quoted lines
> @@ -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.
Show 9 quoted lines
>  	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
Show changes to 3 files +28 −14

Makefile, git-parse-remote.sh, 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
Jonathan Nieder· Jan 31, 2010, 20:00 UTC · re: Junio C Hamano · lore

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

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.

Show 19 quoted lines
> > +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

Junio C Hamano· Jan 31, 2010, 20:05 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 9 quoted lines
> 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?
Jonathan Nieder· Jan 31, 2010, 21:00 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 20 quoted lines
> 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

Junio C Hamano· Jan 31, 2010, 21:08 UTC · re: Jonathan Nieder · lore

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

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.

← back to recent threads