[PATCH 0/2] t0000: truly leak free

[PATCH 0/2] t0000: truly leak free

[Carlo Marcelo Arenas Belón]

While looking at the leaks reported by our new CI job, noticed there
was a hidden one when running t0000 in macOS.

The first patch fixex that leak, and the second one fixes the reason
why it was hidden.

  [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA
  [PATCH 2/2] t0000: avoid masking git exit value through pipes

Carlo

[PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA

[Carlo Marcelo Arenas Belón]

b8ba412bf7 (tree-diff: avoid alloca for large allocations, 2016-06-07)
adds a way to route some bigger allocations out of the stack and free
them through the addition of two conveniently named macros, but leaves
the calls to free the xalloca part, which could be also in the heap,
if the system doesn't HAVE_ALLOCA (ex: macOS).

Add the missing free call, and while at it, change the expression to
match in both macros for easy of readability.

This avoids a leak reported by LSAN as while running t0000 but that
wouldn't fail the test (which will be fixed next) :

  SUMMARY: LeakSanitizer: 1034 byte(s) leaked in 15 allocation(s).

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 tree-diff.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tree-diff.c b/tree-diff.c
index 1572615bd9..437c98a70e 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -21,7 +21,9 @@
 		ALLOC_ARRAY((x), nr); \
 } while(0)
 #define FAST_ARRAY_FREE(x, nr) do { \
-	if ((nr) > 2) \
+	if ((nr) <= 2) \
+		xalloca_free((x)); \
+	else \
 		free((x)); \
 } while(0)
 
-- 
2.33.0.481.g26d3bed244

[PATCH 2/2] t0000: avoid masking git exit value through pipes

[Carlo Marcelo Arenas Belón]

9af0b8dbe2 (t0000-basic: more commit-tree tests., 2006-04-26) adds
tets for commit-tree that mask the return exit from git as described
in a378fee5b07.

Fix the tests, to avoid pipes by using instead a temporary file.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 t/t0000-basic.sh | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index cb87768513..545ff5af13 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -1270,26 +1270,31 @@ test_expect_success 'no diff after checkout and git update-index --refresh' '
 P=$(test_oid root)
 
 test_expect_success 'git commit-tree records the correct tree in a commit' '
-	commit0=$(echo NO | git commit-tree $P) &&
-	tree=$(git show --pretty=raw $commit0 |
-		 sed -n -e "s/^tree //p" -e "/^author /q") &&
+	echo NO | git commit-tree $P >out &&
+	commit0=$(cat out) &&
+	git show --pretty=raw $commit0 >out &&
+	tree=$(cat out | sed -n -e "s/^tree //p" -e "/^author /q") &&
 	test "z$tree" = "z$P"
 '
 
 test_expect_success 'git commit-tree records the correct parent in a commit' '
-	commit1=$(echo NO | git commit-tree $P -p $commit0) &&
-	parent=$(git show --pretty=raw $commit1 |
-		sed -n -e "s/^parent //p" -e "/^author /q") &&
+	echo NO | git commit-tree $P -p $commit0 >out &&
+	commit1=$(cat out) &&
+	git show --pretty=raw $commit1 >out &&
+	parent=$(cat out | sed -n -e "s/^parent //p" -e "/^author /q") &&
 	test "z$commit0" = "z$parent"
 '
 
 test_expect_success 'git commit-tree omits duplicated parent in a commit' '
-	commit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&
-	     parent=$(git show --pretty=raw $commit2 |
+	echo NO | git commit-tree $P -p $commit0 -p $commit0 >out &&
+	commit2=$(cat out) &&
+	git show --pretty=raw $commit2 >out &&
+	parent=$(cat out |
 		sed -n -e "s/^parent //p" -e "/^author /q" |
 		sort -u) &&
 	test "z$commit0" = "z$parent" &&
-	numparent=$(git show --pretty=raw $commit2 |
+	git show --pretty=raw $commit2 >out &&
+	numparent=$(cat out |
 		sed -n -e "s/^parent //p" -e "/^author /q" |
 		wc -l) &&
 	test $numparent = 1
-- 
2.33.0.481.g26d3bed244

Re: [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA

[Taylor Blau]

On Wed, Sep 15, 2021 at 07:37:05PM -0700, Carlo Marcelo Arenas Belón wrote:
> b8ba412bf7 (tree-diff: avoid alloca for large allocations, 2016-06-07)
> adds a way to route some bigger allocations out of the stack and free
> them through the addition of two conveniently named macros, but leaves
> the calls to free the xalloca part, which could be also in the heap,
> if the system doesn't HAVE_ALLOCA (ex: macOS).
>
> Add the missing free call, and while at it, change the expression to
> match in both macros for easy of readability.

s/easy/ease/ or s/easy of/easier/.

> This avoids a leak reported by LSAN as while running t0000 but that
> wouldn't fail the test (which will be fixed next) :

Nit; extra space between the closing parenthesis and colon.

> diff --git a/tree-diff.c b/tree-diff.c
> index 1572615bd9..437c98a70e 100644
> --- a/tree-diff.c
> +++ b/tree-diff.c
> @@ -21,7 +21,9 @@
>  		ALLOC_ARRAY((x), nr); \
>  } while(0)
>  #define FAST_ARRAY_FREE(x, nr) do { \
> -	if ((nr) > 2) \
> +	if ((nr) <= 2) \
> +		xalloca_free((x)); \

OK. So the point is that FAST_ARRAY_ALLOC uses xalloca() for small
arrays. But that might turn into a full-blown malloc() if we don't have
alloca.h. So we need to call xalloca_free() which is a noop if we used
alloca(), but calls free() if we actually used malloc() instead.

Now that I wrote it out myself, I think you basically said as much in
the patch message. But it may have been clearer to say:

    Add the missing free call, [xmalloca_free(), which is a noop if we
    allocated memory in the stack frame, but a real free() if we
    allocaegd in the heap instead].

Thanks,
Taylor

Re: [PATCH 2/2] t0000: avoid masking git exit value through pipes

[Taylor Blau]

On Wed, Sep 15, 2021 at 07:37:06PM -0700, Carlo Marcelo Arenas Belón wrote:
> 9af0b8dbe2 (t0000-basic: more commit-tree tests., 2006-04-26) adds
> tets for commit-tree that mask the return exit from git as described

s/tets/tests

> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index cb87768513..545ff5af13 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -1270,26 +1270,31 @@ test_expect_success 'no diff after checkout and git update-index --refresh' '
>  P=$(test_oid root)
>
>  test_expect_success 'git commit-tree records the correct tree in a commit' '
> -	commit0=$(echo NO | git commit-tree $P) &&
> -	tree=$(git show --pretty=raw $commit0 |
> -		 sed -n -e "s/^tree //p" -e "/^author /q") &&
> +	echo NO | git commit-tree $P >out &&
> +	commit0=$(cat out) &&
> +	git show --pretty=raw $commit0 >out &&
> +	tree=$(cat out | sed -n -e "s/^tree //p" -e "/^author /q") &&

In this and the below tests which had a similar transformation, the
first invocation does not mask its error, since it's on the right-hand
side of a pipe.

But piping "git show" to sed will mask the exit code of the former. So
that makes sense. But I would like to see us avoid an unnecessary
cat-into-pipe and instead redirect out into sed, like "sed -n -e ...
<out".

Thanks,
Taylor

Re: [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> b8ba412bf7 (tree-diff: avoid alloca for large allocations, 2016-06-07)
> adds a way to route some bigger allocations out of the stack and free
> them through the addition of two conveniently named macros, but leaves
> the calls to free the xalloca part, which could be also in the heap,
> if the system doesn't HAVE_ALLOCA (ex: macOS).

Nicely spotted and analyzed.  Thanks.

Re: [PATCH 2/2] t0000: avoid masking git exit value through pipes

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> 9af0b8dbe2 (t0000-basic: more commit-tree tests., 2006-04-26) adds
> tets for commit-tree that mask the return exit from git as described

"tests" (no need to resend---already locally tweaked).

Thanks.

> in a378fee5b07.
>
> Fix the tests, to avoid pipes by using instead a temporary file.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  t/t0000-basic.sh | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index cb87768513..545ff5af13 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -1270,26 +1270,31 @@ test_expect_success 'no diff after checkout and git update-index --refresh' '
>  P=$(test_oid root)
>  
>  test_expect_success 'git commit-tree records the correct tree in a commit' '
> -	commit0=$(echo NO | git commit-tree $P) &&
> -	tree=$(git show --pretty=raw $commit0 |
> -		 sed -n -e "s/^tree //p" -e "/^author /q") &&
> +	echo NO | git commit-tree $P >out &&
> +	commit0=$(cat out) &&
> +	git show --pretty=raw $commit0 >out &&
> +	tree=$(cat out | sed -n -e "s/^tree //p" -e "/^author /q") &&
>  	test "z$tree" = "z$P"
>  '
>  
>  test_expect_success 'git commit-tree records the correct parent in a commit' '
> -	commit1=$(echo NO | git commit-tree $P -p $commit0) &&
> -	parent=$(git show --pretty=raw $commit1 |
> -		sed -n -e "s/^parent //p" -e "/^author /q") &&
> +	echo NO | git commit-tree $P -p $commit0 >out &&
> +	commit1=$(cat out) &&
> +	git show --pretty=raw $commit1 >out &&
> +	parent=$(cat out | sed -n -e "s/^parent //p" -e "/^author /q") &&
>  	test "z$commit0" = "z$parent"
>  '
>  
>  test_expect_success 'git commit-tree omits duplicated parent in a commit' '
> -	commit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&
> -	     parent=$(git show --pretty=raw $commit2 |
> +	echo NO | git commit-tree $P -p $commit0 -p $commit0 >out &&
> +	commit2=$(cat out) &&
> +	git show --pretty=raw $commit2 >out &&
> +	parent=$(cat out |
>  		sed -n -e "s/^parent //p" -e "/^author /q" |
>  		sort -u) &&
>  	test "z$commit0" = "z$parent" &&
> -	numparent=$(git show --pretty=raw $commit2 |
> +	git show --pretty=raw $commit2 >out &&
> +	numparent=$(cat out |
>  		sed -n -e "s/^parent //p" -e "/^author /q" |
>  		wc -l) &&
>  	test $numparent = 1

Re: [PATCH 2/2] t0000: avoid masking git exit value through pipes

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>> +	tree=$(cat out | sed -n -e "s/^tree //p" -e "/^author /q") &&
>
> In this and the below tests which had a similar transformation, the
> first invocation does not mask its error, since it's on the right-hand
> side of a pipe.
>
> But piping "git show" to sed will mask the exit code of the former. So
> that makes sense. But I would like to see us avoid an unnecessary
> cat-into-pipe and instead redirect out into sed, like "sed -n -e ...
> <out".

Good eyes.  There is no reason why we want to cat a single file into
pipe (unless we are testing the pipe mechanism of the underlying OS,
that is)---the downstream command can be fed the file from its
standard input, or for commands like "sed" that takes its input from
files listed on the command line, you should be able to write it
without the input "<" redirection.

Thanks.

[PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users

[Carlo Marcelo Arenas Belón]

While looking at the leaks reported by our new CI job, noticed there
was a hidden one when running t0000 in macOS.

The first one fixes the leak and the second one the reason why it was
silent.

v2 includes all suggestions and feedback, and tries probably too hard
to modify the last test in a way that wouldn't require long lines.

Carlo Marcelo Arenas Belón (2):
  tree-diff: fix leak when not HAVE_ALLOCA_H
  t0000: avoid masking git exit value through pipes

 t/t0000-basic.sh | 23 ++++++++++++-----------
 tree-diff.c      |  4 +++-
 2 files changed, 15 insertions(+), 12 deletions(-)

-- 
2.33.0.481.g26d3bed244

[PATCH v2 1/2] tree-diff: fix leak when not HAVE_ALLOCA_H

[Carlo Marcelo Arenas Belón]

b8ba412bf7 (tree-diff: avoid alloca for large allocations, 2016-06-07)
adds a way to route some bigger allocations out of the stack and free
them through the addition of two conveniently named macros, but leaves
the calls to free the xalloca part, which could be also in the heap,
if the system doesn't HAVE_ALLOCA_H (ex: macOS and other BSD).

Add the missing free call, xalloca_free(), which is a noop if we
allocated memory in the stack frame, but a real free() if we
allocated in the heap instead, and while at it, change the expression
to match in both macros for ease of readability.

This avoids a leak reported by LSAN while running t0000 but that
wouldn't fail the test (which is fixed in the next patch):

  SUMMARY: LeakSanitizer: 1034 byte(s) leaked in 15 allocation(s).

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v2: includes an improved commit message, thanks to Taylor

 tree-diff.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tree-diff.c b/tree-diff.c
index 1572615bd9..437c98a70e 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -21,7 +21,9 @@
 		ALLOC_ARRAY((x), nr); \
 } while(0)
 #define FAST_ARRAY_FREE(x, nr) do { \
-	if ((nr) > 2) \
+	if ((nr) <= 2) \
+		xalloca_free((x)); \
+	else \
 		free((x)); \
 } while(0)
 
-- 
2.33.0.481.g26d3bed244

[PATCH v2 2/2] t0000: avoid masking git exit value through pipes

[Carlo Marcelo Arenas Belón]

9af0b8dbe2 (t0000-basic: more commit-tree tests., 2006-04-26) adds
tests for commit-tree that mask the return exit from git as described
in a378fee5b07 (Documentation: add shell guidelines, 2018-10-05).

Fix the tests, to avoid pipes by using a temporary file instead.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v2:
* avoid changing some of the code as suggested by Taylor
* no need for pipes or stdin redirection as suggested by Junio

 t/t0000-basic.sh | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)y

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index cb87768513..5c342de713 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -1271,28 +1271,29 @@ P=$(test_oid root)
 
 test_expect_success 'git commit-tree records the correct tree in a commit' '
 	commit0=$(echo NO | git commit-tree $P) &&
-	tree=$(git show --pretty=raw $commit0 |
-		 sed -n -e "s/^tree //p" -e "/^author /q") &&
+	git show --pretty=raw $commit0 >out &&
+	tree=$(sed -n -e "s/^tree //p" -e "/^author /q" out) &&
 	test "z$tree" = "z$P"
 '
 
 test_expect_success 'git commit-tree records the correct parent in a commit' '
 	commit1=$(echo NO | git commit-tree $P -p $commit0) &&
-	parent=$(git show --pretty=raw $commit1 |
-		sed -n -e "s/^parent //p" -e "/^author /q") &&
+	git show --pretty=raw $commit1 >out &&
+	parent=$(sed -n -e "s/^parent //p" -e "/^author /q" out) &&
 	test "z$commit0" = "z$parent"
 '
 
 test_expect_success 'git commit-tree omits duplicated parent in a commit' '
 	commit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&
-	     parent=$(git show --pretty=raw $commit2 |
-		sed -n -e "s/^parent //p" -e "/^author /q" |
-		sort -u) &&
+	git show --pretty=raw $commit2 >out &&
+	cat >match.sed <<-\EOF &&
+	s/^parent //p
+	/^author /q
+	EOF
+	parent=$(sed -n -f match.sed out | sort -u) &&
 	test "z$commit0" = "z$parent" &&
-	numparent=$(git show --pretty=raw $commit2 |
-		sed -n -e "s/^parent //p" -e "/^author /q" |
-		wc -l) &&
-	test $numparent = 1
+	git show --pretty=raw $commit2 >out &&
+	test_stdout_line_count = 1 sed -n -f match.sed out
 '
 
 test_expect_success 'update-index D/F conflict' '
-- 
2.33.0.481.g26d3bed244

Re: [PATCH 2/2] t0000: avoid masking git exit value through pipes

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 15 2021, Carlo Marcelo Arenas Belón wrote:

> 9af0b8dbe2 (t0000-basic: more commit-tree tests., 2006-04-26) adds
> tets for commit-tree that mask the return exit from git as described
> in a378fee5b07.
>
> Fix the tests, to avoid pipes by using instead a temporary file.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  t/t0000-basic.sh | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index cb87768513..545ff5af13 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -1270,26 +1270,31 @@ test_expect_success 'no diff after checkout and git update-index --refresh' '
>  P=$(test_oid root)
>  
>  test_expect_success 'git commit-tree records the correct tree in a commit' '
> -	commit0=$(echo NO | git commit-tree $P) &&
> -	tree=$(git show --pretty=raw $commit0 |
> -		 sed -n -e "s/^tree //p" -e "/^author /q") &&
> +	echo NO | git commit-tree $P >out &&
> +	commit0=$(cat out) &&
> +	git show --pretty=raw $commit0 >out &&
> +	tree=$(cat out | sed -n -e "s/^tree //p" -e "/^author /q") &&
>  	test "z$tree" = "z$P"
>  '
>  
>  test_expect_success 'git commit-tree records the correct parent in a commit' '
> -	commit1=$(echo NO | git commit-tree $P -p $commit0) &&
> -	parent=$(git show --pretty=raw $commit1 |
> -		sed -n -e "s/^parent //p" -e "/^author /q") &&
> +	echo NO | git commit-tree $P -p $commit0 >out &&
> +	commit1=$(cat out) &&
> +	git show --pretty=raw $commit1 >out &&
> +	parent=$(cat out | sed -n -e "s/^parent //p" -e "/^author /q") &&
>  	test "z$commit0" = "z$parent"
>  '
>  
>  test_expect_success 'git commit-tree omits duplicated parent in a commit' '
> -	commit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&
> -	     parent=$(git show --pretty=raw $commit2 |
> +	echo NO | git commit-tree $P -p $commit0 -p $commit0 >out &&
> +	commit2=$(cat out) &&
> +	git show --pretty=raw $commit2 >out &&
> +	parent=$(cat out |
>  		sed -n -e "s/^parent //p" -e "/^author /q" |
>  		sort -u) &&
>  	test "z$commit0" = "z$parent" &&
> -	numparent=$(git show --pretty=raw $commit2 |
> +	git show --pretty=raw $commit2 >out &&
> +	numparent=$(cat out |
>  		sed -n -e "s/^parent //p" -e "/^author /q" |
>  		wc -l) &&
>  	test $numparent = 1

Well spotted. This looks good to me sans the cat v.s. pipe to sed that
was already pointed out. In addition to that (Taylor may have meant
this, but not said so explicitly) it looks like you can also e.g.:

    v=$(echo foo | ...) &&
    git show ... $v

Instead of:

    echo foo | ... >out &&
    v=$(cat out) &&
    git show ... $v

But that's a small nit either way.

On the change as a whole:

For what it's worth two ways we could have avoided this sort of edge
case is if my SANITIZE=leak series would e.g. save the log of leaks
somewhere and scour it later, i.e. something like what Jeff King
suggested in[1]. I just re-rolled it at [2], but not with that approach
(but response to your comments on another thread).

I don't think that's worth doing for an intial implementation of that
feature for the reasons argued in its 2/2, just say'n.

The other (and more general) way would be to resurrect my
GIT_TEST_PIPEFAIL mode[3]. I just tried it now in combination with the
SANITIZE=leak test mode, and it would have caught this issue[4]!

I'll see if I can re-poke the bash maintainer (Chet Ramey) about some
way forward for that mode. I had an off-list discussion with him about
my proposed "set -o pipefail" change back in January and he rightly
pointed out that it's intended behavior, meant to catch the sort of
thing that was discussed here on-list in the thread around pagers and
pipefail [5].

So since writing that WIP patch I've come around to his view that "set
-o pipefail" can't be changed like that in general, but perhaps he'd
accept a patch for an optional configuration on top of that. I'll
contact him.

1. https://lore.kernel.org/git/cover-v4-0.3-00000000000-20210907T151855Z-avarab@gmail.com/
2. https://lore.kernel.org/git/cover-v6-0.2-00000000000-20210916T085311Z-avarab@gmail.com/
3. https://lore.kernel.org/git/20210116153554.12604-12-avarab@gmail.com/
4. https://lore.kernel.org/git/cover-v4-0.3-00000000000-20210907T151855Z-avarab@gmail.com/
5. https://lore.kernel.org/git/YAG%2FvzctP4JwSp5x@zira.vinc17.org/

Re: [PATCH v2 1/2] tree-diff: fix leak when not HAVE_ALLOCA_H

[Jeff King]

On Thu, Sep 16, 2021 at 01:55:22AM -0700, Carlo Marcelo Arenas Belón wrote:

> b8ba412bf7 (tree-diff: avoid alloca for large allocations, 2016-06-07)
> adds a way to route some bigger allocations out of the stack and free
> them through the addition of two conveniently named macros, but leaves
> the calls to free the xalloca part, which could be also in the heap,
> if the system doesn't HAVE_ALLOCA_H (ex: macOS and other BSD).
> 
> Add the missing free call, xalloca_free(), which is a noop if we
> allocated memory in the stack frame, but a real free() if we
> allocated in the heap instead, and while at it, change the expression
> to match in both macros for ease of readability.

Thanks, this is definitely my bug introduced by b8ba412bf7 and this is
the right fix.

I continue to find the whole xalloca() thing pretty gross, and doubly so
now that there are _two_ layers of "maybe alloca(), and maybe malloc()"
logic (one in xalloca(), and one in this FAST_ARRAY stuff).

We should definitely take this fix to address the immediate problem, but
I wonder if this size logic should be pushed into xalloca to make this
kind of problem harder. Of course this is the only caller, so it might
not matter much either way.

(I'd also be really happy to see it go away entirely, as alloca() is a
foot-gun in the first place. But I think it did make things slightly
faster. It might be worth re-measuring).

-Peff

Re: [PATCH 2/2] t0000: avoid masking git exit value through pipes

[Taylor Blau]

On Wed, Sep 15, 2021 at 11:23:28PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> >> +	tree=$(cat out | sed -n -e "s/^tree //p" -e "/^author /q") &&
> >
> > In this and the below tests which had a similar transformation, the
> > first invocation does not mask its error, since it's on the right-hand
> > side of a pipe.
> >
> > But piping "git show" to sed will mask the exit code of the former. So
> > that makes sense. But I would like to see us avoid an unnecessary
> > cat-into-pipe and instead redirect out into sed, like "sed -n -e ...
> > <out".
>
> [...] or for commands like "sed" that takes its input from files
> listed on the command line, you should be able to write it without the
> input "<" redirection.

Even better, thanks for noticing the flaw in my suggestion.

Thanks,
Taylor

Re: [PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users

[Taylor Blau]

On Thu, Sep 16, 2021 at 01:55:21AM -0700, Carlo Marcelo Arenas Belón wrote:
> v2 includes all suggestions and feedback, and tries probably too hard
> to modify the last test in a way that wouldn't require long lines.

Thanks, this version looks good to me. I suspect the "tries probably too
hard" is referring to putting the sed expressions into a file and then
loading them from that file with `-f`.

I wouldn't mind the long line, or splitting it across multiple lines
with `\`. But what you wrote is fine, too.

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor

BTW: no need to put "reroll for" in the subject line for this series.
The [PATCH v2] indicates that it is a reroll.