Re: [PATCH 4/7] builtin/bugreport.c: add directory to archiver more gently

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 1, 2022, 22:22 UTC
Message-ID: <xmqqles7mwnc.fsf@gitster.g>
In-Reply-To: <4bc290fbf43e0193aae288b79249014d899ea34a.1659388498.git.gitgitgadget@gmail.com>

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 16 quoted lines

>  	int at_root = !*path;
> -	DIR *dir = opendir(at_root ? "." : path);
> +	DIR *dir;
>  	struct dirent *e;
>  	struct strbuf buf = STRBUF_INIT;
>  	size_t len;
>  	int res = 0;
>  
> +	if (!file_exists(at_root ? "." : path)) {
> +		warning(_("directory '%s' does not exist, will not be archived"), path);
> +		return 0;
> +	}
> +
> +	dir = opendir(at_root ? "." : path);
>  	if (!dir)
>  		return error_errno(_("could not open directory '%s'"), path);

I am not sure if TOCTTOU is how we want to be more gentle. Do we rather want to do something like this

	dir = opendir(...);
	if (!dir) {
		if (errno == ENOENT) {
			warning(_("not archiving missing directory '%s'", path);
		        return 0;
		}
                return error_errno(_("cannot open directory '%s'"), path);
	}

or am I missing something subtle?

Thanks.

Show 27 quoted lines

> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> index 3cf983aa67f..e9db89ef2c8 100755
> --- a/t/t0091-bugreport.sh
> +++ b/t/t0091-bugreport.sh
> @@ -78,7 +78,7 @@ test_expect_success 'indicates populated hooks' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure UNZIP '--diagnose creates diagnostics zip archive' '
> +test_expect_success UNZIP '--diagnose creates diagnostics zip archive' '
>  	test_when_finished rm -rf report &&
>  
>  	git bugreport --diagnose -o report -s test >out &&
> @@ -98,4 +98,13 @@ test_expect_failure UNZIP '--diagnose creates diagnostics zip archive' '
>  	grep "^Total: [0-9][0-9]*" out
>  '
>  
> +test_expect_success '--diagnose warns when archived dir does not exist' '
> +	test_when_finished rm -rf report &&
> +
> +	# Remove logs - not guaranteed to exist
> +	rm -rf .git/logs &&
> +	git bugreport --diagnose -o report -s test 2>err &&
> +	grep "directory .\.git/logs. does not exist, will not be archived" err
> +'
> +
>  test_done