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

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

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.