Re: [PATCH v4 05/11] scalar-diagnose: move functionality to common location
- From
Victoria Dye <vdye@github.com>
- Date
- Aug 12, 2022, 21:00 UTC
- Message-ID
- <a728c894-4e4a-44c3-a1f5-f87c8bfae13a@github.com>
- In-Reply-To
- <xmqq7d3db43z.fsf@gitster.g>
Junio C Hamano wrote:
Show 38 quoted lines
> "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]:
Show 12 quoted lines
> 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/