Junio C Hamano wrote:
> "Victoria Dye via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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@xxxxxxxxxx/
>
> 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@xxxxxxxxx/
[2] https://lore.kernel.org/git/pull.1310.v4.git.1660335019.gitgitgadget@xxxxxxxxx/
