Re: [PATCH v3 06/11] diagnose.c: add option to configure archive contents

> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> @@ -177,11 +182,13 @@ int create_diagnostics_archive(struct strbuf *zip_path)
>>  	loose_objs_stats(&buf, ".git/objects");
>>  	strvec_push(&archiver_args, buf.buf);
>>  
>> -	if ((res = add_directory_to_archiver(&archiver_args, ".git", 0)) ||
>> -	    (res = add_directory_to_archiver(&archiver_args, ".git/hooks", 0)) ||
>> -	    (res = add_directory_to_archiver(&archiver_args, ".git/info", 0)) ||
>> -	    (res = add_directory_to_archiver(&archiver_args, ".git/logs", 1)) ||
>> -	    (res = add_directory_to_archiver(&archiver_args, ".git/objects/info", 0)))
>> +	/* Only include this if explicitly requested */
>> +	if (mode == DIAGNOSE_ALL &&
>> +	    ((res = add_directory_to_archiver(&archiver_args, ".git", 0)) ||
>> +	     (res = add_directory_to_archiver(&archiver_args, ".git/hooks", 0)) ||
>> +	     (res = add_directory_to_archiver(&archiver_args, ".git/info", 0)) ||
>> +	     (res = add_directory_to_archiver(&archiver_args, ".git/logs", 1)) ||
>> +	     (res = add_directory_to_archiver(&archiver_args, ".git/objects/info", 0))))
>>  		goto diagnose_cleanup;
> 
> At first glance, it looks as if this part fails silently, but
> add_directory_to_archiver() states what failed there, so we show
> necessary error messages and do not silently fail, which is good.
> 
> There is a "failed to write archive" message after write_archive()
> call returns non-zero, but presumably write_archive() itself gives
> diagnostics (like "oh, I was told to archive this file but I cannot
> read it") when it does so, so in a sense, giving the concluding
> "failed to write" only in that case might make the error messages
> uneven.  If we fail to enlist ".git/hooks" directory, we may want to
> say why we failed to do so, and then want to see the concluding
> "failed to write" at the end, just like the case where write_archive()
> failed.
> 
> It is a truely minor point, and if it turns out to be worth fixing,
> it can be easily done by moving the diagnose_clean_up label a bit
> higher, i.e.
> 
> 	...
> 	res = write_archive(...);
> 
> diagnose_cleanup:
> 	if (res)
> 		error(_("failed to write archive"));
> 	else
>         	fprintf(stderr, "\n"
> 			"Diagnostics complete.\n"
> 			"All of the gathered info is captured in '%s'\n",
> 			zip_path->buf);
> 
> 	if (archiver_fd >= 0) {
> 		... restore FD#1 and close stdout_fd and archiver_fd
> 	}
> 	...
>