diff mbox series

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

Message ID 4bc290fbf43e0193aae288b79249014d899ea34a.1659388498.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Generalize 'scalar diagnose' into 'git bugreport --diagnose' | expand

Commit Message

Victoria Dye Aug. 1, 2022, 9:14 p.m. UTC 
From: Victoria Dye <vdye@github.com>

If a directory added to the '--diagnose' archiver does not exist, warn and
return 0 from 'add_directory_to_archiver()' rather than failing with a fatal
error. This handles a failure edge case where the '.git/logs' has not yet
been created when running 'git bugreport --diagnose', but extends to any
situation where a directory may be missing in the '.git' dir.

Now, when a directory is missing a warning is captured in the diagnostic
logs. This provides a user with more complete information than if 'git
bugreport' simply failed with an error.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/bugreport.c  |  8 +++++++-
 t/t0091-bugreport.sh | 11 ++++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Aug. 1, 2022, 10:22 p.m. UTC | #1 
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  	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.

> 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
Victoria Dye Aug. 2, 2022, 3:43 p.m. UTC | #2 
Junio C Hamano wrote:
> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>>  	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?
> 

The "gentleness" was meant to be a reference only to the error -> warning
change, the TOCTTOU change was just a miss by me. I'll fix it in the next
version, thanks!

> Thanks.
> 
>> 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
diff mbox series

Patch

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 720889a37ad..dea11f91386 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -176,12 +176,18 @@  static int add_directory_to_archiver(struct strvec *archiver_args,
 				     const char *path, int recurse)
 {
 	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);
 
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