"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
}
...
Other than that, this new patch looks good to me.
Thanks.
