Junio C Hamano wrote:
> "Victoria Dye via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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
> }
> ...
>
I like this idea, since I think there's value in indicating both the cause
("could not open directory") and effect ("failed to write archive") of the
error. I'll include this and [1] in a re-roll. Thanks!
[1] https://lore.kernel.org/git/9d1b0cb9-5c21-c101-8597-2fe166cb6abe@xxxxxxxxxx/
>
> Other than that, this new patch looks good to me.
>
> Thanks.
