{"thread":{"id":"56510","subject":"[PATCH 0/2] t0000: truly leak free","startedAt":"2021-09-16T02:37:49Z","lastAt":"2021-09-16T18:35:55Z","messageCount":15,"participants":["Carlo Marcelo Arenas Belón","Taylor Blau","Junio C Hamano","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"436059","messageId":"20210916023706.55760-1-carenas@gmail.com","threadId":"56510","inReplyTo":null,"subject":"[PATCH 0/2] t0000: truly leak free","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-16T02:37:04Z","receivedAt":"2021-09-16T02:37:49Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"While looking at the leaks reported by our new CI job, noticed there\nwas a hidden one when running t0000 in macOS.\n\nThe first patch fixex that leak, and the second one fixes the reason\nwhy it was hidden.\n\n  [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA\n  [PATCH 2/2] t0000: avoid masking git exit value through pipes\n\nCarlo\n\n"},{"id":"436060","messageId":"20210916023706.55760-2-carenas@gmail.com","threadId":"56510","inReplyTo":"20210916023706.55760-1-carenas@gmail.com","subject":"[PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-16T02:37:05Z","receivedAt":"2021-09-16T02:37:50Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"b8ba412bf7 (tree-diff: avoid alloca for large allocations, 2016-06-07)\nadds a way to route some bigger allocations out of the stack and free\nthem through the addition of two conveniently named macros, but leaves\nthe calls to free the xalloca part, which could be also in the heap,\nif the system doesn't HAVE_ALLOCA (ex: macOS).\n\nAdd the missing free call, and while at it, change the expression to\nmatch in both macros for easy of readability.\n\nThis avoids a leak reported by LSAN as while running t0000 but that\nwouldn't fail the test (which will be fixed next) :\n\n  SUMMARY: LeakSanitizer: 1034 byte(s) leaked in 15 allocation(s).\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n tree-diff.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 1572615bd9..437c98a70e 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -21,7 +21,9 @@\n \t\tALLOC_ARRAY((x), nr); \\\n } while(0)\n #define FAST_ARRAY_FREE(x, nr) do { \\\n-\tif ((nr) > 2) \\\n+\tif ((nr) <= 2) \\\n+\t\txalloca_free((x)); \\\n+\telse \\\n \t\tfree((x)); \\\n } while(0)\n \n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"436061","messageId":"20210916023706.55760-3-carenas@gmail.com","threadId":"56510","inReplyTo":"20210916023706.55760-1-carenas@gmail.com","subject":"[PATCH 2/2] t0000: avoid masking git exit value through pipes","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-16T02:37:06Z","receivedAt":"2021-09-16T02:37:53Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"9af0b8dbe2 (t0000-basic: more commit-tree tests., 2006-04-26) adds\ntets for commit-tree that mask the return exit from git as described\nin a378fee5b07.\n\nFix the tests, to avoid pipes by using instead a temporary file.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n t/t0000-basic.sh | 23 ++++++++++++++---------\n 1 file changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex cb87768513..545ff5af13 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -1270,26 +1270,31 @@ test_expect_success 'no diff after checkout and git update-index --refresh' '\n P=$(test_oid root)\n \n test_expect_success 'git commit-tree records the correct tree in a commit' '\n-\tcommit0=$(echo NO | git commit-tree $P) &&\n-\ttree=$(git show --pretty=raw $commit0 |\n-\t\t sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n+\techo NO | git commit-tree $P >out &&\n+\tcommit0=$(cat out) &&\n+\tgit show --pretty=raw $commit0 >out &&\n+\ttree=$(cat out | sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n \ttest \"z$tree\" = \"z$P\"\n '\n \n test_expect_success 'git commit-tree records the correct parent in a commit' '\n-\tcommit1=$(echo NO | git commit-tree $P -p $commit0) &&\n-\tparent=$(git show --pretty=raw $commit1 |\n-\t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\") &&\n+\techo NO | git commit-tree $P -p $commit0 >out &&\n+\tcommit1=$(cat out) &&\n+\tgit show --pretty=raw $commit1 >out &&\n+\tparent=$(cat out | sed -n -e \"s/^parent //p\" -e \"/^author /q\") &&\n \ttest \"z$commit0\" = \"z$parent\"\n '\n \n test_expect_success 'git commit-tree omits duplicated parent in a commit' '\n-\tcommit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&\n-\t     parent=$(git show --pretty=raw $commit2 |\n+\techo NO | git commit-tree $P -p $commit0 -p $commit0 >out &&\n+\tcommit2=$(cat out) &&\n+\tgit show --pretty=raw $commit2 >out &&\n+\tparent=$(cat out |\n \t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" |\n \t\tsort -u) &&\n \ttest \"z$commit0\" = \"z$parent\" &&\n-\tnumparent=$(git show --pretty=raw $commit2 |\n+\tgit show --pretty=raw $commit2 >out &&\n+\tnumparent=$(cat out |\n \t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" |\n \t\twc -l) &&\n \ttest $numparent = 1\n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"436072","messageId":"YULTcXX6i2pXW0BE@nand.local","threadId":"56510","inReplyTo":"20210916023706.55760-2-carenas@gmail.com","subject":"Re: [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-16T05:17:37Z","receivedAt":"2021-09-16T05:17:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 15, 2021 at 07:37:05PM -0700, Carlo Marcelo Arenas Belón wrote:\n> b8ba412bf7 (tree-diff: avoid alloca for large allocations, 2016-06-07)\n> adds a way to route some bigger allocations out of the stack and free\n> them through the addition of two conveniently named macros, but leaves\n> the calls to free the xalloca part, which could be also in the heap,\n> if the system doesn't HAVE_ALLOCA (ex: macOS).\n>\n> Add the missing free call, and while at it, change the expression to\n> match in both macros for easy of readability.\n\ns/easy/ease/ or s/easy of/easier/.\n\n> This avoids a leak reported by LSAN as while running t0000 but that\n> wouldn't fail the test (which will be fixed next) :\n\nNit; extra space between the closing parenthesis and colon.\n\n> diff --git a/tree-diff.c b/tree-diff.c\n> index 1572615bd9..437c98a70e 100644\n> --- a/tree-diff.c\n> +++ b/tree-diff.c\n> @@ -21,7 +21,9 @@\n>  \t\tALLOC_ARRAY((x), nr); \\\n>  } while(0)\n>  #define FAST_ARRAY_FREE(x, nr) do { \\\n> -\tif ((nr) > 2) \\\n> +\tif ((nr) <= 2) \\\n> +\t\txalloca_free((x)); \\\n\nOK. So the point is that FAST_ARRAY_ALLOC uses xalloca() for small\narrays. But that might turn into a full-blown malloc() if we don't have\nalloca.h. So we need to call xalloca_free() which is a noop if we used\nalloca(), but calls free() if we actually used malloc() instead.\n\nNow that I wrote it out myself, I think you basically said as much in\nthe patch message. But it may have been clearer to say:\n\n    Add the missing free call, [xmalloca_free(), which is a noop if we\n    allocated memory in the stack frame, but a real free() if we\n    allocaegd in the heap instead].\n\nThanks,\nTaylor\n"},{"id":"436073","messageId":"YULUS3kYkB4RXv2K@nand.local","threadId":"56510","inReplyTo":"20210916023706.55760-3-carenas@gmail.com","subject":"Re: [PATCH 2/2] t0000: avoid masking git exit value through pipes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-16T05:21:15Z","receivedAt":"2021-09-16T05:21:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 15, 2021 at 07:37:06PM -0700, Carlo Marcelo Arenas Belón wrote:\n> 9af0b8dbe2 (t0000-basic: more commit-tree tests., 2006-04-26) adds\n> tets for commit-tree that mask the return exit from git as described\n\ns/tets/tests\n\n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index cb87768513..545ff5af13 100755\n> --- a/t/t0000-basic.sh\n> +++ b/t/t0000-basic.sh\n> @@ -1270,26 +1270,31 @@ test_expect_success 'no diff after checkout and git update-index --refresh' '\n>  P=$(test_oid root)\n>\n>  test_expect_success 'git commit-tree records the correct tree in a commit' '\n> -\tcommit0=$(echo NO | git commit-tree $P) &&\n> -\ttree=$(git show --pretty=raw $commit0 |\n> -\t\t sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n> +\techo NO | git commit-tree $P >out &&\n> +\tcommit0=$(cat out) &&\n> +\tgit show --pretty=raw $commit0 >out &&\n> +\ttree=$(cat out | sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n\nIn this and the below tests which had a similar transformation, the\nfirst invocation does not mask its error, since it's on the right-hand\nside of a pipe.\n\nBut piping \"git show\" to sed will mask the exit code of the former. So\nthat makes sense. But I would like to see us avoid an unnecessary\ncat-into-pipe and instead redirect out into sed, like \"sed -n -e ...\n<out\".\n\nThanks,\nTaylor\n"},{"id":"436074","messageId":"xmqqpmt9um1t.fsf@gitster.g","threadId":"56510","inReplyTo":"20210916023706.55760-2-carenas@gmail.com","subject":"Re: [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-16T05:27:58Z","receivedAt":"2021-09-16T05:28:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n\n> b8ba412bf7 (tree-diff: avoid alloca for large allocations, 2016-06-07)\n> adds a way to route some bigger allocations out of the stack and free\n> them through the addition of two conveniently named macros, but leaves\n> the calls to free the xalloca part, which could be also in the heap,\n> if the system doesn't HAVE_ALLOCA (ex: macOS).\n\nNicely spotted and analyzed.  Thanks.\n"},{"id":"436075","messageId":"xmqqlf3xulzj.fsf@gitster.g","threadId":"56510","inReplyTo":"20210916023706.55760-3-carenas@gmail.com","subject":"Re: [PATCH 2/2] t0000: avoid masking git exit value through pipes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-16T05:29:20Z","receivedAt":"2021-09-16T05:29:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n\n> 9af0b8dbe2 (t0000-basic: more commit-tree tests., 2006-04-26) adds\n> tets for commit-tree that mask the return exit from git as described\n\n\"tests\" (no need to resend---already locally tweaked).\n\nThanks.\n\n> in a378fee5b07.\n>\n> Fix the tests, to avoid pipes by using instead a temporary file.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  t/t0000-basic.sh | 23 ++++++++++++++---------\n>  1 file changed, 14 insertions(+), 9 deletions(-)\n>\n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index cb87768513..545ff5af13 100755\n> --- a/t/t0000-basic.sh\n> +++ b/t/t0000-basic.sh\n> @@ -1270,26 +1270,31 @@ test_expect_success 'no diff after checkout and git update-index --refresh' '\n>  P=$(test_oid root)\n>  \n>  test_expect_success 'git commit-tree records the correct tree in a commit' '\n> -\tcommit0=$(echo NO | git commit-tree $P) &&\n> -\ttree=$(git show --pretty=raw $commit0 |\n> -\t\t sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n> +\techo NO | git commit-tree $P >out &&\n> +\tcommit0=$(cat out) &&\n> +\tgit show --pretty=raw $commit0 >out &&\n> +\ttree=$(cat out | sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n>  \ttest \"z$tree\" = \"z$P\"\n>  '\n>  \n>  test_expect_success 'git commit-tree records the correct parent in a commit' '\n> -\tcommit1=$(echo NO | git commit-tree $P -p $commit0) &&\n> -\tparent=$(git show --pretty=raw $commit1 |\n> -\t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\") &&\n> +\techo NO | git commit-tree $P -p $commit0 >out &&\n> +\tcommit1=$(cat out) &&\n> +\tgit show --pretty=raw $commit1 >out &&\n> +\tparent=$(cat out | sed -n -e \"s/^parent //p\" -e \"/^author /q\") &&\n>  \ttest \"z$commit0\" = \"z$parent\"\n>  '\n>  \n>  test_expect_success 'git commit-tree omits duplicated parent in a commit' '\n> -\tcommit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&\n> -\t     parent=$(git show --pretty=raw $commit2 |\n> +\techo NO | git commit-tree $P -p $commit0 -p $commit0 >out &&\n> +\tcommit2=$(cat out) &&\n> +\tgit show --pretty=raw $commit2 >out &&\n> +\tparent=$(cat out |\n>  \t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" |\n>  \t\tsort -u) &&\n>  \ttest \"z$commit0\" = \"z$parent\" &&\n> -\tnumparent=$(git show --pretty=raw $commit2 |\n> +\tgit show --pretty=raw $commit2 >out &&\n> +\tnumparent=$(cat out |\n>  \t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" |\n>  \t\twc -l) &&\n>  \ttest $numparent = 1\n"},{"id":"436086","messageId":"xmqq8rzxujhb.fsf@gitster.g","threadId":"56510","inReplyTo":"YULUS3kYkB4RXv2K@nand.local","subject":"Re: [PATCH 2/2] t0000: avoid masking git exit value through pipes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-16T06:23:28Z","receivedAt":"2021-09-16T06:23:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n>> +\ttree=$(cat out | sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n>\n> In this and the below tests which had a similar transformation, the\n> first invocation does not mask its error, since it's on the right-hand\n> side of a pipe.\n>\n> But piping \"git show\" to sed will mask the exit code of the former. So\n> that makes sense. But I would like to see us avoid an unnecessary\n> cat-into-pipe and instead redirect out into sed, like \"sed -n -e ...\n> <out\".\n\nGood eyes.  There is no reason why we want to cat a single file into\npipe (unless we are testing the pipe mechanism of the underlying OS,\nthat is)---the downstream command can be fed the file from its\nstandard input, or for commands like \"sed\" that takes its input from\nfiles listed on the command line, you should be able to write it\nwithout the input \"<\" redirection.\n\nThanks.\n"},{"id":"436090","messageId":"20210916085523.68581-1-carenas@gmail.com","threadId":"56510","inReplyTo":"20210916023706.55760-1-carenas@gmail.com","subject":"[PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-16T08:55:21Z","receivedAt":"2021-09-16T08:55:40Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"While looking at the leaks reported by our new CI job, noticed there\nwas a hidden one when running t0000 in macOS.\n\nThe first one fixes the leak and the second one the reason why it was\nsilent.\n\nv2 includes all suggestions and feedback, and tries probably too hard\nto modify the last test in a way that wouldn't require long lines.\n\nCarlo Marcelo Arenas Belón (2):\n  tree-diff: fix leak when not HAVE_ALLOCA_H\n  t0000: avoid masking git exit value through pipes\n\n t/t0000-basic.sh | 23 ++++++++++++-----------\n tree-diff.c      |  4 +++-\n 2 files changed, 15 insertions(+), 12 deletions(-)\n\n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"436091","messageId":"20210916085523.68581-2-carenas@gmail.com","threadId":"56510","inReplyTo":"20210916085523.68581-1-carenas@gmail.com","subject":"[PATCH v2 1/2] tree-diff: fix leak when not HAVE_ALLOCA_H","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-16T08:55:22Z","receivedAt":"2021-09-16T08:55:43Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"b8ba412bf7 (tree-diff: avoid alloca for large allocations, 2016-06-07)\nadds a way to route some bigger allocations out of the stack and free\nthem through the addition of two conveniently named macros, but leaves\nthe calls to free the xalloca part, which could be also in the heap,\nif the system doesn't HAVE_ALLOCA_H (ex: macOS and other BSD).\n\nAdd the missing free call, xalloca_free(), which is a noop if we\nallocated memory in the stack frame, but a real free() if we\nallocated in the heap instead, and while at it, change the expression\nto match in both macros for ease of readability.\n\nThis avoids a leak reported by LSAN while running t0000 but that\nwouldn't fail the test (which is fixed in the next patch):\n\n  SUMMARY: LeakSanitizer: 1034 byte(s) leaked in 15 allocation(s).\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\nv2: includes an improved commit message, thanks to Taylor\n\n tree-diff.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 1572615bd9..437c98a70e 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -21,7 +21,9 @@\n \t\tALLOC_ARRAY((x), nr); \\\n } while(0)\n #define FAST_ARRAY_FREE(x, nr) do { \\\n-\tif ((nr) > 2) \\\n+\tif ((nr) <= 2) \\\n+\t\txalloca_free((x)); \\\n+\telse \\\n \t\tfree((x)); \\\n } while(0)\n \n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"436092","messageId":"20210916085523.68581-3-carenas@gmail.com","threadId":"56510","inReplyTo":"20210916085523.68581-1-carenas@gmail.com","subject":"[PATCH v2 2/2] t0000: avoid masking git exit value through pipes","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-16T08:55:23Z","receivedAt":"2021-09-16T08:55:43Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"9af0b8dbe2 (t0000-basic: more commit-tree tests., 2006-04-26) adds\ntests for commit-tree that mask the return exit from git as described\nin a378fee5b07 (Documentation: add shell guidelines, 2018-10-05).\n\nFix the tests, to avoid pipes by using a temporary file instead.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\nv2:\n* avoid changing some of the code as suggested by Taylor\n* no need for pipes or stdin redirection as suggested by Junio\n\n t/t0000-basic.sh | 23 ++++++++++++-----------\n 1 file changed, 12 insertions(+), 11 deletions(-)y\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex cb87768513..5c342de713 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -1271,28 +1271,29 @@ P=$(test_oid root)\n \n test_expect_success 'git commit-tree records the correct tree in a commit' '\n \tcommit0=$(echo NO | git commit-tree $P) &&\n-\ttree=$(git show --pretty=raw $commit0 |\n-\t\t sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n+\tgit show --pretty=raw $commit0 >out &&\n+\ttree=$(sed -n -e \"s/^tree //p\" -e \"/^author /q\" out) &&\n \ttest \"z$tree\" = \"z$P\"\n '\n \n test_expect_success 'git commit-tree records the correct parent in a commit' '\n \tcommit1=$(echo NO | git commit-tree $P -p $commit0) &&\n-\tparent=$(git show --pretty=raw $commit1 |\n-\t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\") &&\n+\tgit show --pretty=raw $commit1 >out &&\n+\tparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" out) &&\n \ttest \"z$commit0\" = \"z$parent\"\n '\n \n test_expect_success 'git commit-tree omits duplicated parent in a commit' '\n \tcommit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&\n-\t     parent=$(git show --pretty=raw $commit2 |\n-\t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" |\n-\t\tsort -u) &&\n+\tgit show --pretty=raw $commit2 >out &&\n+\tcat >match.sed <<-\\EOF &&\n+\ts/^parent //p\n+\t/^author /q\n+\tEOF\n+\tparent=$(sed -n -f match.sed out | sort -u) &&\n \ttest \"z$commit0\" = \"z$parent\" &&\n-\tnumparent=$(git show --pretty=raw $commit2 |\n-\t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" |\n-\t\twc -l) &&\n-\ttest $numparent = 1\n+\tgit show --pretty=raw $commit2 >out &&\n+\ttest_stdout_line_count = 1 sed -n -f match.sed out\n '\n \n test_expect_success 'update-index D/F conflict' '\n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"436110","messageId":"871r5ohiy7.fsf@evledraar.gmail.com","threadId":"56510","inReplyTo":"20210916023706.55760-3-carenas@gmail.com","subject":"Re: [PATCH 2/2] t0000: avoid masking git exit value through pipes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-16T10:45:52Z","receivedAt":"2021-09-16T11:13:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 15 2021, Carlo Marcelo Arenas Belón wrote:\n\n> 9af0b8dbe2 (t0000-basic: more commit-tree tests., 2006-04-26) adds\n> tets for commit-tree that mask the return exit from git as described\n> in a378fee5b07.\n>\n> Fix the tests, to avoid pipes by using instead a temporary file.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  t/t0000-basic.sh | 23 ++++++++++++++---------\n>  1 file changed, 14 insertions(+), 9 deletions(-)\n>\n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index cb87768513..545ff5af13 100755\n> --- a/t/t0000-basic.sh\n> +++ b/t/t0000-basic.sh\n> @@ -1270,26 +1270,31 @@ test_expect_success 'no diff after checkout and git update-index --refresh' '\n>  P=$(test_oid root)\n>  \n>  test_expect_success 'git commit-tree records the correct tree in a commit' '\n> -\tcommit0=$(echo NO | git commit-tree $P) &&\n> -\ttree=$(git show --pretty=raw $commit0 |\n> -\t\t sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n> +\techo NO | git commit-tree $P >out &&\n> +\tcommit0=$(cat out) &&\n> +\tgit show --pretty=raw $commit0 >out &&\n> +\ttree=$(cat out | sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n>  \ttest \"z$tree\" = \"z$P\"\n>  '\n>  \n>  test_expect_success 'git commit-tree records the correct parent in a commit' '\n> -\tcommit1=$(echo NO | git commit-tree $P -p $commit0) &&\n> -\tparent=$(git show --pretty=raw $commit1 |\n> -\t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\") &&\n> +\techo NO | git commit-tree $P -p $commit0 >out &&\n> +\tcommit1=$(cat out) &&\n> +\tgit show --pretty=raw $commit1 >out &&\n> +\tparent=$(cat out | sed -n -e \"s/^parent //p\" -e \"/^author /q\") &&\n>  \ttest \"z$commit0\" = \"z$parent\"\n>  '\n>  \n>  test_expect_success 'git commit-tree omits duplicated parent in a commit' '\n> -\tcommit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&\n> -\t     parent=$(git show --pretty=raw $commit2 |\n> +\techo NO | git commit-tree $P -p $commit0 -p $commit0 >out &&\n> +\tcommit2=$(cat out) &&\n> +\tgit show --pretty=raw $commit2 >out &&\n> +\tparent=$(cat out |\n>  \t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" |\n>  \t\tsort -u) &&\n>  \ttest \"z$commit0\" = \"z$parent\" &&\n> -\tnumparent=$(git show --pretty=raw $commit2 |\n> +\tgit show --pretty=raw $commit2 >out &&\n> +\tnumparent=$(cat out |\n>  \t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" |\n>  \t\twc -l) &&\n>  \ttest $numparent = 1\n\nWell spotted. This looks good to me sans the cat v.s. pipe to sed that\nwas already pointed out. In addition to that (Taylor may have meant\nthis, but not said so explicitly) it looks like you can also e.g.:\n\n    v=$(echo foo | ...) &&\n    git show ... $v\n\nInstead of:\n\n    echo foo | ... >out &&\n    v=$(cat out) &&\n    git show ... $v\n\nBut that's a small nit either way.\n\nOn the change as a whole:\n\nFor what it's worth two ways we could have avoided this sort of edge\ncase is if my SANITIZE=leak series would e.g. save the log of leaks\nsomewhere and scour it later, i.e. something like what Jeff King\nsuggested in[1]. I just re-rolled it at [2], but not with that approach\n(but response to your comments on another thread).\n\nI don't think that's worth doing for an intial implementation of that\nfeature for the reasons argued in its 2/2, just say'n.\n\nThe other (and more general) way would be to resurrect my\nGIT_TEST_PIPEFAIL mode[3]. I just tried it now in combination with the\nSANITIZE=leak test mode, and it would have caught this issue[4]!\n\nI'll see if I can re-poke the bash maintainer (Chet Ramey) about some\nway forward for that mode. I had an off-list discussion with him about\nmy proposed \"set -o pipefail\" change back in January and he rightly\npointed out that it's intended behavior, meant to catch the sort of\nthing that was discussed here on-list in the thread around pagers and\npipefail [5].\n\nSo since writing that WIP patch I've come around to his view that \"set\n-o pipefail\" can't be changed like that in general, but perhaps he'd\naccept a patch for an optional configuration on top of that. I'll\ncontact him.\n\n1. https://lore.kernel.org/git/cover-v4-0.3-00000000000-20210907T151855Z-avarab@gmail.com/\n2. https://lore.kernel.org/git/cover-v6-0.2-00000000000-20210916T085311Z-avarab@gmail.com/\n3. https://lore.kernel.org/git/20210116153554.12604-12-avarab@gmail.com/\n4. https://lore.kernel.org/git/cover-v4-0.3-00000000000-20210907T151855Z-avarab@gmail.com/\n5. https://lore.kernel.org/git/YAG%2FvzctP4JwSp5x@zira.vinc17.org/\n"},{"id":"436124","messageId":"YUNcDsMkSPhIsFoI@coredump.intra.peff.net","threadId":"56510","inReplyTo":"20210916085523.68581-2-carenas@gmail.com","subject":"Re: [PATCH v2 1/2] tree-diff: fix leak when not HAVE_ALLOCA_H","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-16T15:00:30Z","receivedAt":"2021-09-16T15:00:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 16, 2021 at 01:55:22AM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> b8ba412bf7 (tree-diff: avoid alloca for large allocations, 2016-06-07)\n> adds a way to route some bigger allocations out of the stack and free\n> them through the addition of two conveniently named macros, but leaves\n> the calls to free the xalloca part, which could be also in the heap,\n> if the system doesn't HAVE_ALLOCA_H (ex: macOS and other BSD).\n> \n> Add the missing free call, xalloca_free(), which is a noop if we\n> allocated memory in the stack frame, but a real free() if we\n> allocated in the heap instead, and while at it, change the expression\n> to match in both macros for ease of readability.\n\nThanks, this is definitely my bug introduced by b8ba412bf7 and this is\nthe right fix.\n\nI continue to find the whole xalloca() thing pretty gross, and doubly so\nnow that there are _two_ layers of \"maybe alloca(), and maybe malloc()\"\nlogic (one in xalloca(), and one in this FAST_ARRAY stuff).\n\nWe should definitely take this fix to address the immediate problem, but\nI wonder if this size logic should be pushed into xalloca to make this\nkind of problem harder. Of course this is the only caller, so it might\nnot matter much either way.\n\n(I'd also be really happy to see it go away entirely, as alloca() is a\nfoot-gun in the first place. But I think it did make things slightly\nfaster. It might be worth re-measuring).\n\n-Peff\n"},{"id":"436131","messageId":"YUN1igX4f4hLUMvA@nand.local","threadId":"56510","inReplyTo":"xmqq8rzxujhb.fsf@gitster.g","subject":"Re: [PATCH 2/2] t0000: avoid masking git exit value through pipes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-16T16:49:14Z","receivedAt":"2021-09-16T18:31:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 15, 2021 at 11:23:28PM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> >> +\ttree=$(cat out | sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n> >\n> > In this and the below tests which had a similar transformation, the\n> > first invocation does not mask its error, since it's on the right-hand\n> > side of a pipe.\n> >\n> > But piping \"git show\" to sed will mask the exit code of the former. So\n> > that makes sense. But I would like to see us avoid an unnecessary\n> > cat-into-pipe and instead redirect out into sed, like \"sed -n -e ...\n> > <out\".\n>\n> [...] or for commands like \"sed\" that takes its input from files\n> listed on the command line, you should be able to write it without the\n> input \"<\" redirection.\n\nEven better, thanks for noticing the flaw in my suggestion.\n\nThanks,\nTaylor\n"},{"id":"436132","messageId":"YUN2lwA3w8zAoYhS@nand.local","threadId":"56510","inReplyTo":"20210916085523.68581-1-carenas@gmail.com","subject":"Re: [PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-16T16:53:43Z","receivedAt":"2021-09-16T18:35:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 16, 2021 at 01:55:21AM -0700, Carlo Marcelo Arenas Belón wrote:\n> v2 includes all suggestions and feedback, and tries probably too hard\n> to modify the last test in a way that wouldn't require long lines.\n\nThanks, this version looks good to me. I suspect the \"tries probably too\nhard\" is referring to putting the sed expressions into a file and then\nloading them from that file with `-f`.\n\nI wouldn't mind the long line, or splitting it across multiple lines\nwith `\\`. But what you wrote is fine, too.\n\n  Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\nThanks,\nTaylor\n\nBTW: no need to put \"reroll for\" in the subject line for this series.\nThe [PATCH v2] indicates that it is a reroll.\n"}]}