Junio C Hamano wrote:

"Victoria Dye via GitGitGadget" [off-list ref] writes:

quoted

@@ -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@github.com/ (local)

Other than that, this new patch looks good to me.

Thanks.