Subject: Re: [PATCH v6 05/15] reftable: ignore remove() return value in stack_test.c

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Fri, 21 Jan 2022 12:46:05 +0100

On Thu, Jan 20 2022, Han-Wen Nienhuys via GitGitGadget wrote:

> From: Han-Wen Nienhuys <[email-masked]>
>
> If the cleanup fails, there is nothing we can do.
>
> Signed-off-by: Han-Wen Nienhuys <[email-masked]>
> ---
>  reftable/stack_test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/reftable/stack_test.c b/reftable/stack_test.c
> index e84f50d27ff..19fe4e20085 100644
> --- a/reftable/stack_test.c
> +++ b/reftable/stack_test.c
> @@ -90,7 +90,7 @@ static void test_read_file(void)
>  		EXPECT(0 == strcmp(want[i], names[i]));
>  	}
>  	free_names(names);
> -	remove(fn);
> +	(void) remove(fn);
>  }
>  
>  static void test_parse_names(void)

Well, if we fail here due to a permission error or other I/O weirdness
surely it's better to:

	if (remove(fn) < 0)
		die_errno("unable to remove '%s'", fn);

Otherwise we're just silently sweeping that under the rug, and likely
having the "rm -rf" we'll shortly do in test-lib.sh catch it at a
distance.

Also why are we using remove() here at all? Shouldn't this just be
unlink()? Or per the feedback above unlink_or_warn() or
remove_or_warn()? I.e. looking at the context we just open()'d this
"fn", so we're not unsure if it's a directory or a file, are we?