{"thread":{"id":"16013","subject":"[RFC PATCH 0/5] valgrind in test scripts","startedAt":"2008-10-22T20:28:10Z","lastAt":"2008-10-23T16:47:03Z","messageCount":13,"participants":["Jeff King","Johannes Schindelin","Junio C Hamano","Nicolas Pitre"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"93715","messageId":"20081022202810.GA4439@coredump.intra.peff.net","threadId":"16013","inReplyTo":null,"subject":"[RFC PATCH 0/5] valgrind in test scripts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-22T20:28:10Z","receivedAt":"2008-10-22T20:28:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I spent some time last week running git through the paces with\nvalgrind's memcheck tool. The good news is that I didn't find any\nserious issues, so we are doing a pretty good job overall. The bad news\nis that running the whole test suite with valgrind takes a few hours on\na quad-core (but thank goodness for \"make -j4 test\").\n\nI did uncover a few potential problems, and patches are in the latter\npart of the series. I suppose an argument could be made that these fixes\nare code churn, since these are not problems in practice, but I think\nthey are worth fixing. The fixes are few in number and small, and we are\nvery close to a valgrind-error-free code-base, which would make it easy\nto spot any new problems when they arise. We could always suppress these\nerrors, but there is the possibility of an overzealous suppression\nmasking a real problem (especially if one of these issues changes from\ntheoretical to practical).\n\nThere are a few things I don't like:\n\n 1. The \"fake git PATH\" is set up by the Makefile, since it needs to\n    know which dashed commands to override (based on $(PROGRAMS)).\n\n    I think it would be nicer if the test script itself set this up when\n    --memcheck was requested, so that we always know it is fresh. But:\n\n      - if the fake PATH isn't inside the trash directory, then we have\n        a problem with multiple tests trying to set it up at the same\n        time\n\n      - if the fake PATH is inside the trash directory, I'm not sure of\n        the best place to put it, as I don't want to influence the\n        outcome of the tests. It could go into .git/valgrind, without\n        hurting anything.\n\n 2. I wanted to have a completely clean valgrind run before posting.\n    With these patches, the only errors I get are for an uninitialized\n    \"struct stat\" in t4121 and t4127. After much looking (including\n    carefully reading the code, and using a a debugger, which shows the\n    data in question looking very much initialized) I can't figure out\n    what the problem might be. So it might simply be a false positive in\n    valgrind, but I would like another set of eyes.\n\n-Peff\n"},{"id":"93716","messageId":"20081022202915.GA4547@coredump.intra.peff.net","threadId":"16013","inReplyTo":"20081022202810.GA4439@coredump.intra.peff.net","subject":"[RFC PATCH 1/5] add valgrind support in test scripts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-22T20:29:15Z","receivedAt":"2008-10-22T20:29:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This patch adds the ability to use valgrind's memcheck tool\nto diagnose memory problems in git while running the test\nscripts. It works by placing a \"fake\" git in the front of\nthe test script's PATH; this fake git runs the real git\nunder valgrind. It also points the exec-path such that any\nstand-alone dashed git programs are run using the same\nscript. In this way we avoid having to modify the actual git\ncode in any way.\n\nThe memcheck tool can be used by specifying\n\"GIT_TEST_OPTS=--memcheck\" in the make invocation. Any\ninvocation of git that finds any errors under valgrind will\nexit with failure code 126. Any valgrind output will go to\nthe usual stderr channel for tests (i.e., /dev/null, unless\n-v has been specified).\n\nA few default suppressions are included, since libz seems to\ntrigger quite a few false positives. We'll assume that libz\nworks and that we can ignore any errors which are reported\nthere.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n .gitignore              |    1 +\n Makefile                |   12 ++++++++++--\n t/test-lib.sh           |   11 +++++++++++\n t/valgrind/.gitignore   |    1 +\n t/valgrind/default.supp |   21 +++++++++++++++++++++\n test-valgrind.sh        |   24 ++++++++++++++++++++++++\n 6 files changed, 68 insertions(+), 2 deletions(-)\n create mode 100644 t/valgrind/.gitignore\n create mode 100644 t/valgrind/default.supp\n create mode 100755 test-valgrind.sh\n\ndiff --git a/.gitignore b/.gitignore\nindex bbaf9de..45045cb 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -151,6 +151,7 @@ test-match-trees\n test-parse-options\n test-path-utils\n test-sha1\n+test-valgrind\n common-cmds.h\n *.tar.gz\n *.dsc\ndiff --git a/Makefile b/Makefile\nindex d6f3695..68f0172 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -279,6 +279,8 @@ SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH)) \\\n \t  $(patsubst %.perl,%,$(SCRIPT_PERL)) \\\n \t  git-instaweb\n \n+VALGRIND_SH += test-valgrind.sh\n+\n # Empty...\n EXTRA_PROGRAMS =\n \n@@ -1139,7 +1141,7 @@ common-cmds.h: ./generate-cmdlist.sh command-list.txt\n common-cmds.h: $(wildcard Documentation/git-*.txt)\n \t$(QUIET_GEN)./generate-cmdlist.sh > $@+ && mv $@+ $@\n \n-$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh\n+$(patsubst %.sh,%,$(SCRIPT_SH) $(VALGRIND_SH)) : % : %.sh\n \t$(QUIET_GEN)$(RM) $@ $@+ && \\\n \tsed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n \t    -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \\\n@@ -1343,7 +1345,12 @@ all:: $(TEST_PROGRAMS)\n \n export NO_SVN_TESTS\n \n-test: all\n+valgrind-setup: $(patsubst %.sh,%,$(VALGRIND_SH))\n+\trm -rf t/valgrind/bin\n+\tmkdir t/valgrind/bin\n+\tfor i in git $(PROGRAMS); do cp test-valgrind t/valgrind/bin/$$i; done\n+\n+test: all valgrind-setup\n \t$(MAKE) -C t/ all\n \n test-date$X: date.o ctype.o\n@@ -1501,6 +1508,7 @@ endif\n .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell\n .PHONY: .FORCE-GIT-VERSION-FILE TAGS tags cscope .FORCE-GIT-CFLAGS\n .PHONY: .FORCE-GIT-BUILD-OPTIONS\n+.PHONY: valgrind-setup\n \n ### Check documentation\n #\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 8936173..e753654 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -94,6 +94,8 @@ do\n \t--no-python)\n \t\t# noop now...\n \t\tshift ;;\n+\t-m|--m|--me|--mem|--memc|--memch|--memche|--memchec|--memcheck)\n+\t\tmemcheck=t shift ;;\n \t*)\n \t\tbreak ;;\n \tesac\n@@ -468,6 +470,15 @@ test_done () {\n # t/ subdirectory and are run in 'trash directory' subdirectory.\n TEST_DIRECTORY=$(pwd)\n PATH=$TEST_DIRECTORY/..:$PATH\n+if test -n \"$memcheck\"; then\n+\tPATH=$TEST_DIRECTORY/valgrind/bin:$PATH\n+\tGIT_VALGRIND=$TEST_DIRECTORY/valgrind; export GIT_VALGRIND\n+\tif ! test -f \"$GIT_VALGRIND/bin/git\"; then\n+\t\techo >&2 'You need to setup the valgrind bin:'\n+\t\techo >&2 'Run \"make valgrind-setup\" in the (toplevel) directory'\n+\t\texit 1\n+\tfi\n+fi\n GIT_EXEC_PATH=$(pwd)/..\n GIT_TEMPLATE_DIR=$(pwd)/../templates/blt\n unset GIT_CONFIG\ndiff --git a/t/valgrind/.gitignore b/t/valgrind/.gitignore\nnew file mode 100644\nindex 0000000..ba077a4\n--- /dev/null\n+++ b/t/valgrind/.gitignore\n@@ -0,0 +1 @@\n+bin\ndiff --git a/t/valgrind/default.supp b/t/valgrind/default.supp\nnew file mode 100644\nindex 0000000..2482b3b\n--- /dev/null\n+++ b/t/valgrind/default.supp\n@@ -0,0 +1,21 @@\n+{\n+\tignore-zlib-errors-cond\n+\tMemcheck:Cond\n+\tobj:*libz.so*\n+}\n+\n+{\n+\tignore-zlib-errors-value4\n+\tMemcheck:Value4\n+\tobj:*libz.so*\n+}\n+\n+{\n+\twriting-data-from-zlib-triggers-errors\n+\tMemcheck:Param\n+\twrite(buf)\n+\tobj:/lib/ld-*.so\n+\tfun:write_in_full\n+\tfun:write_buffer\n+\tfun:write_loose_object\n+}\ndiff --git a/test-valgrind.sh b/test-valgrind.sh\nnew file mode 100755\nindex 0000000..61c1428\n--- /dev/null\n+++ b/test-valgrind.sh\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+\n+base=`echo $0 | sed 's,.*/,,'`\n+case \"$base\" in\n+git-*)\n+\tset -- \"$GIT_EXEC_PATH/$base\" \"$@\"\n+\t;;\n+git)\n+\tset -- \"$GIT_EXEC_PATH/git\" --exec-path=\"$GIT_VALGRIND/bin\" \"$@\"\n+\t;;\n+*)\n+\techo >&2 \"test-valgrind invoked on non-git command: $0\"\n+\texit 1\n+esac\n+\n+exec valgrind -q --error-exitcode=126 \\\n+\t--leak-check=no \\\n+\t--suppressions=\"$GIT_VALGRIND/default.supp\" \\\n+\t--gen-suppressions=all \\\n+\t--log-fd=4 \\\n+\t--input-fd=4 \\\n+\t${GIT_VALGRIND_DEBUG:+--db-attach=yes} \\\n+\t${GIT_VALGRIND_DEBUGCMD:+--db-command=\"$GIT_VALGRIND_DEBUGCMD\"} \\\n+\t\"$@\"\n-- \n1.6.0.2.825.g6d19d\n"},{"id":"93717","messageId":"20081022203045.GB4547@coredump.intra.peff.net","threadId":"16013","inReplyTo":"20081022202810.GA4439@coredump.intra.peff.net","subject":"[RFC PATCH 2/5] valgrind: ignore ldso errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-22T20:30:45Z","receivedAt":"2008-10-22T20:30:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On some Linux systems, we get a host of Cond and Addr errors\nfrom calls to dlopen that are caused by nss modules. We\nshould be able to safely ignore anything happening in\nld-*.so as \"not our problem.\"\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis was from a Debian etch system, while my lenny systems seemed to\nhave no trouble. I'm sure we will pick up a few commits like these as\npeople run it on various platforms, but hopefully they should shake out\npretty quickly.\n\n t/valgrind/default.supp |   12 ++++++++++++\n 1 files changed, 12 insertions(+), 0 deletions(-)\n\ndiff --git a/t/valgrind/default.supp b/t/valgrind/default.supp\nindex 2482b3b..1013847 100644\n--- a/t/valgrind/default.supp\n+++ b/t/valgrind/default.supp\n@@ -11,6 +11,18 @@\n }\n \n {\n+\tignore-ldso-cond\n+\tMemcheck:Cond\n+\tobj:*ld-*.so\n+}\n+\n+{\n+\tignore-ldso-addr8\n+\tMemcheck:Addr8\n+\tobj:*ld-*.so\n+}\n+\n+{\n \twriting-data-from-zlib-triggers-errors\n \tMemcheck:Param\n \twrite(buf)\n-- \n1.6.0.2.825.g6d19d\n"},{"id":"93718","messageId":"20081022203058.GC4547@coredump.intra.peff.net","threadId":"16013","inReplyTo":"20081022202810.GA4439@coredump.intra.peff.net","subject":"[RFC PATCH 3/5] correct cache_entry allocation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-22T20:30:58Z","receivedAt":"2008-10-22T20:30:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Most cache_entry structs are allocated by using the\ncache_entry_size macro, which rounds the size of the struct\nup to the nearest multiple of 8 bytes (presumably to avoid\nmemory fragmentation).\n\nThere is one exception: the special \"conflict entry\" is\nallocated with an empty name, and so is explicitly given\njust one extra byte to hold the NUL.\n\nHowever, later code doesn't realize that this particular\nstruct has been allocated differently, and happily tries\nreading and copying it based on the ce_size macro, which\nassumes the 8-byte alignment.\n\nThis can lead to reading uninitalized data, though since\nthat data is simply padding, there shouldn't be any problem\nas a result. Still, it makes sense to hold the padding\nassumption so as not to surprise later maintainers.\n\nThis fixes valgrind errors in t1005, t3030, t4002, and\nt4114.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n unpack-trees.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex e59d144..e5749ef 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -382,7 +382,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \to->merge_size = len;\n \n \tif (!dfc)\n-\t\tdfc = xcalloc(1, sizeof(struct cache_entry) + 1);\n+\t\tdfc = xcalloc(1, cache_entry_size(0));\n \to->df_conflict_entry = dfc;\n \n \tif (len) {\n-- \n1.6.0.2.825.g6d19d\n"},{"id":"93719","messageId":"20081022203103.GD4547@coredump.intra.peff.net","threadId":"16013","inReplyTo":"20081022202810.GA4439@coredump.intra.peff.net","subject":"[RFC PATCH 4/5] pack-objects: avoid reading uninitalized data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-22T20:31:03Z","receivedAt":"2008-10-22T20:31:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the main loop of find_deltas, we do:\n\n  struct object_entry *entry = *list++;\n  ...\n  if (!*list_size)\n\t  ...\n\t  break\n\nBecause we look at and increment *list _before_ the check of\nlist_size, in the very last iteration of the loop we will\nlook at uninitialized data, and increment the pointer beyond\none past the end of the allocated space. Since we don't\nactually do anything with the data until after the check,\nthis is not a problem in practice.\n\nBut since it technically violates the C standard, and\nbecause it provokes a spurious valgrind warning, let's just\nmove the initialization of entry to a safe place.\n\nThis fixes valgrind errors in t5300, t5301, t5302, t303, and\nt9400.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin-pack-objects.c |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 59c30d1..15b80db 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1375,7 +1375,7 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,\n \tarray = xcalloc(window, sizeof(struct unpacked));\n \n \tfor (;;) {\n-\t\tstruct object_entry *entry = *list++;\n+\t\tstruct object_entry *entry;\n \t\tstruct unpacked *n = array + idx;\n \t\tint j, max_depth, best_base = -1;\n \n@@ -1384,6 +1384,7 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,\n \t\t\tprogress_unlock();\n \t\t\tbreak;\n \t\t}\n+\t\tentry = *list++;\n \t\t(*list_size)--;\n \t\tif (!entry->preferred_base) {\n \t\t\t(*processed)++;\n-- \n1.6.0.2.825.g6d19d\n"},{"id":"93721","messageId":"20081022203222.GE4547@coredump.intra.peff.net","threadId":"16013","inReplyTo":"20081022202810.GA4439@coredump.intra.peff.net","subject":"[RFC PATCH 5/5] fix overlapping memcpy in normalize_absolute_path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-22T20:32:23Z","receivedAt":"2008-10-22T20:32:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The comments for normalize_absolute_path explicitly claim\nthat the source and destination buffers may be the same\n(though they may not otherwise overlap). Thus the call to\nmemcpy may involve copying overlapping data, and memmove\nshould be used instead.\n\nThis fixes a valgrind error in t1504.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAn alternative fix, since we declare that only true equality is OK,\nwould be to keep the memcpy and explicitly check for \"comp_start ==\ndst\". That might have a performance benefit if memcpy is faster than\nmemmove, but I don't know that normalize_absolute_path is in any\ncritical paths.\n\n path.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 76e8872..c1cb54b 100644\n--- a/path.c\n+++ b/path.c\n@@ -348,7 +348,7 @@ int normalize_absolute_path(char *buf, const char *path)\n \t\t\tgoto next;\n \t\t}\n \n-\t\tmemcpy(dst, comp_start, comp_len);\n+\t\tmemmove(dst, comp_start, comp_len);\n \t\tdst += comp_len;\n \tnext:\n \t\tcomp_start = comp_end;\n-- \n1.6.0.2.825.g6d19d\n"},{"id":"93736","messageId":"alpine.DEB.1.00.0810230008430.22125@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"16013","inReplyTo":"20081022202915.GA4547@coredump.intra.peff.net","subject":"Re: [RFC PATCH 1/5] add valgrind support in test scripts","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-10-22T22:13:47Z","receivedAt":"2008-10-22T22:13:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 22 Oct 2008, Jeff King wrote:\n\n> diff --git a/Makefile b/Makefile\n> index d6f3695..68f0172 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1343,7 +1345,12 @@ all:: $(TEST_PROGRAMS)\n>  \n>  export NO_SVN_TESTS\n>  \n> -test: all\n> +valgrind-setup: $(patsubst %.sh,%,$(VALGRIND_SH))\n> +\trm -rf t/valgrind/bin\n> +\tmkdir t/valgrind/bin\n> +\tfor i in git $(PROGRAMS); do cp test-valgrind t/valgrind/bin/$$i; done\n\nI wonder if it would not be better to scrap the t/valgrind/ directory and \nregenerate it everytime you run a test manually; I'd use \"ln\" instead of \n\"cp\", and also parse command-list.txt to catch really all of them (even if \na dashed form is used for a builtin by mistake).\n\nOtherwise: good work, I like it!\n\nCiao,\nDscho\n"},{"id":"93748","messageId":"7v7i80tber.fsf@gitster.siamese.dyndns.org","threadId":"16013","inReplyTo":"alpine.DEB.1.00.0810230008430.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [RFC PATCH 1/5] add valgrind support in test scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-23T00:14:52Z","receivedAt":"2008-10-23T00:14:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi,\n>\n> On Wed, 22 Oct 2008, Jeff King wrote:\n>\n>> diff --git a/Makefile b/Makefile\n>> index d6f3695..68f0172 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -1343,7 +1345,12 @@ all:: $(TEST_PROGRAMS)\n>>  \n>>  export NO_SVN_TESTS\n>>  \n>> -test: all\n>> +valgrind-setup: $(patsubst %.sh,%,$(VALGRIND_SH))\n>> +\trm -rf t/valgrind/bin\n>> +\tmkdir t/valgrind/bin\n>> +\tfor i in git $(PROGRAMS); do cp test-valgrind t/valgrind/bin/$$i; done\n>\n> I wonder if it would not be better to scrap the t/valgrind/ directory and \n> regenerate it everytime you run a test manually; I'd use \"ln\" instead of \n> \"cp\", and also parse command-list.txt to catch really all of them (even if \n> a dashed form is used for a builtin by mistake).\n\nGoing one step further, I wonder if this approach can also be used to\ncatch such a mistake.  Install a dashed form that records the fact that it\nwas called when it shouldn't, and by whom.\n"},{"id":"93751","messageId":"alpine.LFD.2.00.0810222107540.26244@xanadu.home","threadId":"16013","inReplyTo":"20081022203103.GD4547@coredump.intra.peff.net","subject":"Re: [RFC PATCH 4/5] pack-objects: avoid reading uninitalized data","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-23T01:11:16Z","receivedAt":"2008-10-23T01:11:16Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 22 Oct 2008, Jeff King wrote:\n\n> In the main loop of find_deltas, we do:\n> \n>   struct object_entry *entry = *list++;\n>   ...\n>   if (!*list_size)\n> \t  ...\n> \t  break\n> \n> Because we look at and increment *list _before_ the check of\n> list_size, in the very last iteration of the loop we will\n> look at uninitialized data, and increment the pointer beyond\n> one past the end of the allocated space. Since we don't\n> actually do anything with the data until after the check,\n> this is not a problem in practice.\n> \n> But since it technically violates the C standard, and\n> because it provokes a spurious valgrind warning, let's just\n> move the initialization of entry to a safe place.\n> \n> This fixes valgrind errors in t5300, t5301, t5302, t303, and\n> t9400.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n\nOK.  Minor nit...\n\n> ---\n>  builtin-pack-objects.c |    3 ++-\n>  1 files changed, 2 insertions(+), 1 deletions(-)\n> \n> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\n> index 59c30d1..15b80db 100644\n> --- a/builtin-pack-objects.c\n> +++ b/builtin-pack-objects.c\n> @@ -1375,7 +1375,7 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,\n>  \tarray = xcalloc(window, sizeof(struct unpacked));\n>  \n>  \tfor (;;) {\n> -\t\tstruct object_entry *entry = *list++;\n> +\t\tstruct object_entry *entry;\n>  \t\tstruct unpacked *n = array + idx;\n>  \t\tint j, max_depth, best_base = -1;\n>  \n> @@ -1384,6 +1384,7 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,\n>  \t\t\tprogress_unlock();\n>  \t\t\tbreak;\n>  \t\t}\n\n---> Please preserve the empty line here so the previous code\n     chunk still appears logically separate.\n\n> +\t\tentry = *list++;\n>  \t\t(*list_size)--;\n>  \t\tif (!entry->preferred_base) {\n>  \t\t\t(*processed)++;\n\n\nNicolas\n"},{"id":"93788","messageId":"20081023151959.GA10804@coredump.intra.peff.net","threadId":"16013","inReplyTo":"alpine.DEB.1.00.0810230008430.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [RFC PATCH 1/5] add valgrind support in test scripts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-23T15:19:59Z","receivedAt":"2008-10-23T15:19:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 23, 2008 at 12:13:47AM +0200, Johannes Schindelin wrote:\n\n> I wonder if it would not be better to scrap the t/valgrind/ directory and \n> regenerate it everytime you run a test manually;\n\nYeah, I mentioned that in my 0/5 cover letter. The problem is where to\nput it that won't impact test results, but also allow running multiple\ntests simultaneously. I'm going to try sticking it in .git/valgrind in\nthe trash directory, which presumably won't affect any tests.\n\n> I'd use \"ln\" instead of \"cp\"\n\nI specifically stayed away from 'ln' for Windows portability. It looks\nlike for builtins, we do \"ln || ln -s || cp\". We can probably do the\nsame here.\n\nI also failed to use git$X in the fake path, which would probably be\nnecessary for Windows.\n\n> and also parse command-list.txt to catch really all of them (even if \n> a dashed form is used for a builtin by mistake).\n\nThat is a little bit trickier. I don't actually want to intercept\ngit-am, for example, since I have no interest in running valgrind on the\nshell. But it is do-able; I will give details in my response to Junio's\nsuggestion.\n\n> Otherwise: good work, I like it!\n\nThanks.\n\n-Peff\n"},{"id":"93789","messageId":"20081023152922.GB10804@coredump.intra.peff.net","threadId":"16013","inReplyTo":"7v7i80tber.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC PATCH 1/5] add valgrind support in test scripts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-23T15:29:23Z","receivedAt":"2008-10-23T15:29:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 22, 2008 at 05:14:52PM -0700, Junio C Hamano wrote:\n\n> > I wonder if it would not be better to scrap the t/valgrind/ directory and \n> > regenerate it everytime you run a test manually; I'd use \"ln\" instead of \n> > \"cp\", and also parse command-list.txt to catch really all of them (even if \n> > a dashed form is used for a builtin by mistake).\n> \n> Going one step further, I wonder if this approach can also be used to\n> catch such a mistake.  Install a dashed form that records the fact that it\n> was called when it shouldn't, and by whom.\n\nI think that makes sense, though it is somewhat orthogonal to valgrind.\nDo we want to always set up such a fake path? It could actually be even\nsimpler than a wrapper; just stop adding the build directory to the\nPATH, and instead have a pseudo-installation directory with the bin and\nexec-path directories set up appropriately. This would more closely\nmodel the actual installation.\n\nI think there are actually several classes of dashed commands we need to\ndifferentiate:\n\n 1. dashed commands which get installed in bin; these should be allowed\n\n 2. dashed commands which don't get installed in bin; these should be\n    flagged as an error\n\n 3. dashed commands in exec-path which are stand-alone C programs. These\n    should be run under valgrind.\n\n 4. dashed commands in exec-path which are scripts. These do get run,\n    but should not be run under valgrind.\n\n 5. dashed commands in exec-path which are builtins. These should never\n    get run by our test scripts, since they are \"legacy\" links for people\n    who want to put the exec-path in their PATH\n\nRight now we always point PATH at the build directory, which has\neverything. So we can't easily differentiate between '1' and '2'. I used\nthe $(PROGRAMS) variable in the Makefile to find '3' (as opposed to '4'\nand '5').\n\n-Peff\n"},{"id":"93790","messageId":"20081023153329.GC10804@coredump.intra.peff.net","threadId":"16013","inReplyTo":"alpine.LFD.2.00.0810222107540.26244@xanadu.home","subject":"Re: [RFC PATCH 4/5] pack-objects: avoid reading uninitalized data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-23T15:33:30Z","receivedAt":"2008-10-23T15:33:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 22, 2008 at 09:11:16PM -0400, Nicolas Pitre wrote:\n\n> >  \tfor (;;) {\n> > -\t\tstruct object_entry *entry = *list++;\n> > +\t\tstruct object_entry *entry;\n> >  \t\tstruct unpacked *n = array + idx;\n> >  \t\tint j, max_depth, best_base = -1;\n> >  \n> > @@ -1384,6 +1384,7 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,\n> >  \t\t\tprogress_unlock();\n> >  \t\t\tbreak;\n> >  \t\t}\n> \n> ---> Please preserve the empty line here so the previous code\n>      chunk still appears logically separate.\n> \n> > +\t\tentry = *list++;\n> >  \t\t(*list_size)--;\n> >  \t\tif (!entry->preferred_base) {\n> >  \t\t\t(*processed)++;\n\nEr, there was no empty line there (or else there would have been a '-'\nline in the diff). I am happy to add it, like:\n\n  if (!*list_size) {\n    progress_unlock();\n    break;\n  }\n\n  entry = *list++;\n  (*list_size)--;\n  if (!entry->preferred_base)\n    ...\n\nif you like, but the current version seems to format it as one stanza\ninside of the progress_lock/progress_unlock. Look at the version in\n'master' to see what I mean.\n\n-Peff\n"},{"id":"93793","messageId":"alpine.LFD.2.00.0810231246040.26244@xanadu.home","threadId":"16013","inReplyTo":"20081023153329.GC10804@coredump.intra.peff.net","subject":"Re: [RFC PATCH 4/5] pack-objects: avoid reading uninitalized data","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-23T16:47:03Z","receivedAt":"2008-10-23T16:47:03Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 23 Oct 2008, Jeff King wrote:\n\n> On Wed, Oct 22, 2008 at 09:11:16PM -0400, Nicolas Pitre wrote:\n> \n> > >  \tfor (;;) {\n> > > -\t\tstruct object_entry *entry = *list++;\n> > > +\t\tstruct object_entry *entry;\n> > >  \t\tstruct unpacked *n = array + idx;\n> > >  \t\tint j, max_depth, best_base = -1;\n> > >  \n> > > @@ -1384,6 +1384,7 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,\n> > >  \t\t\tprogress_unlock();\n> > >  \t\t\tbreak;\n> > >  \t\t}\n> > \n> > ---> Please preserve the empty line here so the previous code\n> >      chunk still appears logically separate.\n> > \n> > > +\t\tentry = *list++;\n> > >  \t\t(*list_size)--;\n> > >  \t\tif (!entry->preferred_base) {\n> > >  \t\t\t(*processed)++;\n> \n> Er, there was no empty line there (or else there would have been a '-'\n> line in the diff).\n\nOh, right.  I'm confused.\nNevermind...\n\n\nNicolas\n"}]}