Re: [PATCH v4 05/11] scalar-diagnose: move functionality to common location

[Victoria Dye <vdye@github.com>]

Junio C Hamano wrote:
> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> +	res = write_archive(archiver_args.nr, (const char **)argv_copy, NULL,
>> +			    the_repository, NULL, 0);
>> +	if (res) {
>> +		error(_("failed to write archive"));
>> +		goto diagnose_cleanup;
>> +	}
>> +
>> +	fprintf(stderr, "\n"
>> +		"Diagnostics complete.\n"
>> +		"All of the gathered info is captured in '%s'\n",
>> +		zip_path->buf);
>> +
>> +diagnose_cleanup:
>> +	if (archiver_fd >= 0) {
>> +		dup2(stdout_fd, STDOUT_FILENO);
>> +		close(stdout_fd);
>> +		close(archiver_fd);
>> +	}
> 
> Hmph, after reading 
> 
> https://lore.kernel.org/git/32f2cadc-556e-1cd5-a238-c8f1cdaaf470@github.com/
> 
> I would have expected to see the above part more like:
> 
>                 res = write_archive(...);
> 
>         diagnose_cleanup:
>                 if (res)
>                         error(...);
>                 else
>                         fprintf(stderr, "Diag complete");
> 
>                 if (archiver_fd >= 0) {
>                         ...
> 

I originally planned to implement it this way, but instead went with adding
an error printout explicitly for failed 'add_directory_to_archiver()' calls
(in patch 6 [1]). I elaborated on the thought process/reasoning for
modifying the approach in the cover letter [2]:

> Improved error reporting in 'create_diagnostics_archive()'. I was
> originally going to modify the "failed to write archive" error to trigger
> whenever 'create_diagnostics_archive()' returned a nonzero value.
> However, while working on it I realized the message would no longer be
> tied to a failure of 'write_archive()', making it less helpful in
> pinpointing an issue. To address the original issue
> ('add_directory_to_archiver()' silently failing in
> 'create_diagnostics_archive()'), I instead refactored those calls into a
> loop and added the error message. Now, there's exactly one error message
> printed for each possible early exit scenario from
> 'create_diagnostics_archive()', hopefully avoiding both redundancy &
> under-reporting.

To add a bit more context: when I used the "move 'diagnose_cleanup'"
approach, I felt that the added message was either redundant or too general
to help a user identify an issue. Redundancy appeared when, for example,
'dup2()' returned a nonzero code; if that happened, we'd get the printouts:

$ git diagnose --suffix test
error: could not redirect output: <ERRNO error message>
error: could not write archive
error: unable to create diagnostics archive 'git-diagnostics-test.zip': <ERRNO error message>

The first two are from 'create_diagnostics_archive()', and the second
doesn't really give us information that we don't get out of the third (in
'builtin/diagnose.c').

Conversely, a failure in 'add_directory_to_archiver()' and 'write_archive()'
would give us the same printouts (at least within the scope of
'diagnose.c'/'builtin/diagnose.c'):

$ git diagnose --suffix test
error: could not write archive
error: unable to create diagnostics archive 'git-diagnostics-test.zip: <some error message>

Previously, "could not write archive" would point someone debugging to
'write_archive()'; now, it's unclear.

I ended up settling on adding the error message directly to the
'add_directory_to_archiver()' loop in patch 6 because it meant that:

1. 'create_diagnostics_archive()' would only ever print one error message;
   the others printed would indicate (IMO more clearly) where in the call
   stack the error happened
2. there would be a unique error message for each condition that caused
   'create_diagnostics_archive()' to exit early

Apologies for not sending another reply with these details before
re-rolling. I'll be more direct when changing plans in the future.

Thanks!

[1] https://lore.kernel.org/git/710b67e5776363d199ed5043d019386819d44e7e.1660335019.git.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/pull.1310.v4.git.1660335019.gitgitgadget@gmail.com/