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

[Victoria Dye <vdye@github.com>]

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