* [PATCH] rerere: fix too-short initialization @ 2010-01-28 14:52 Jeff King 2010-01-29 10:25 ` [PATCH 0/3] valgrind bug roundup Jeff King 0 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2010-01-28 14:52 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 0/3] valgrind bug roundup 2010-01-28 14:52 [PATCH] rerere: fix too-short initialization Jeff King @ 2010-01-29 10:25 ` Jeff King 2010-01-29 10:28 ` [PATCH 1/3] fix memcpy of overlapping area Jeff King ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Jeff King @ 2010-01-29 10:25 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] fix memcpy of overlapping area 2010-01-29 10:25 ` [PATCH 0/3] valgrind bug roundup Jeff King @ 2010-01-29 10:28 ` Jeff King 2010-01-29 10:31 ` [PATCH 2/3] fix off-by-one allocation error Jeff King 2010-01-29 10:37 ` [PATCH 3/3] add shebang line to git-mergetool--lib.sh Jeff King 2 siblings, 0 replies; 13+ messages in thread From: Jeff King @ 2010-01-29 10:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin, git 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] fix off-by-one allocation error 2010-01-29 10:25 ` [PATCH 0/3] valgrind bug roundup Jeff King 2010-01-29 10:28 ` [PATCH 1/3] fix memcpy of overlapping area Jeff King @ 2010-01-29 10:31 ` Jeff King 2010-01-29 10:37 ` [PATCH 3/3] add shebang line to git-mergetool--lib.sh Jeff King 2 siblings, 0 replies; 13+ messages in thread From: Jeff King @ 2010-01-29 10:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jan Krüger, git 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] add shebang line to git-mergetool--lib.sh 2010-01-29 10:25 ` [PATCH 0/3] valgrind bug roundup Jeff King 2010-01-29 10:28 ` [PATCH 1/3] fix memcpy of overlapping area Jeff King 2010-01-29 10:31 ` [PATCH 2/3] fix off-by-one allocation error Jeff King @ 2010-01-29 10:37 ` Jeff King 2010-01-29 14:50 ` [PATCH] Do not install shell libraries executable Jonathan Nieder 2 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2010-01-29 10:37 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin, David Aguilar, git 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] Do not install shell libraries executable 2010-01-29 10:37 ` [PATCH 3/3] add shebang line to git-mergetool--lib.sh Jeff King @ 2010-01-29 14:50 ` Jonathan Nieder 2010-01-31 7:08 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Nieder @ 2010-01-29 14:50 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, Johannes Schindelin, David Aguilar, git 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] Do not install shell libraries executable 2010-01-29 14:50 ` [PATCH] Do not install shell libraries executable Jonathan Nieder @ 2010-01-31 7:08 ` Junio C Hamano 2010-01-31 8:34 ` [PATCH v2] " Jonathan Nieder 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2010-01-31 7:08 UTC (permalink / raw) To: Jonathan Nieder Cc: Jeff King, Junio C Hamano, Johannes Schindelin, David Aguilar, git 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] Do not install shell libraries executable 2010-01-31 7:08 ` Junio C Hamano @ 2010-01-31 8:34 ` Jonathan Nieder 2010-01-31 19:46 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Nieder @ 2010-01-31 8:34 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, David Aguilar, git 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] Do not install shell libraries executable 2010-01-31 8:34 ` [PATCH v2] " Jonathan Nieder @ 2010-01-31 19:46 ` Junio C Hamano 2010-01-31 20:00 ` Jonathan Nieder 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2010-01-31 19:46 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Jeff King, Johannes Schindelin, David Aguilar, git 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] Do not install shell libraries executable 2010-01-31 19:46 ` Junio C Hamano @ 2010-01-31 20:00 ` Jonathan Nieder 2010-01-31 20:05 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Nieder @ 2010-01-31 20:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, David Aguilar, git 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] Do not install shell libraries executable 2010-01-31 20:00 ` Jonathan Nieder @ 2010-01-31 20:05 ` Junio C Hamano 2010-01-31 21:00 ` Jonathan Nieder 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2010-01-31 20:05 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Jeff King, Johannes Schindelin, David Aguilar, git 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] Do not install shell libraries executable 2010-01-31 20:05 ` Junio C Hamano @ 2010-01-31 21:00 ` Jonathan Nieder 2010-01-31 21:08 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Nieder @ 2010-01-31 21:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, David Aguilar, git 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] Do not install shell libraries executable 2010-01-31 21:00 ` Jonathan Nieder @ 2010-01-31 21:08 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2010-01-31 21:08 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Jeff King, Johannes Schindelin, David Aguilar, git 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2010-01-31 21:09 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-01-28 14:52 [PATCH] rerere: fix too-short initialization Jeff King 2010-01-29 10:25 ` [PATCH 0/3] valgrind bug roundup Jeff King 2010-01-29 10:28 ` [PATCH 1/3] fix memcpy of overlapping area Jeff King 2010-01-29 10:31 ` [PATCH 2/3] fix off-by-one allocation error Jeff King 2010-01-29 10:37 ` [PATCH 3/3] add shebang line to git-mergetool--lib.sh Jeff King 2010-01-29 14:50 ` [PATCH] Do not install shell libraries executable Jonathan Nieder 2010-01-31 7:08 ` Junio C Hamano 2010-01-31 8:34 ` [PATCH v2] " Jonathan Nieder 2010-01-31 19:46 ` Junio C Hamano 2010-01-31 20:00 ` Jonathan Nieder 2010-01-31 20:05 ` Junio C Hamano 2010-01-31 21:00 ` Jonathan Nieder 2010-01-31 21:08 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).