From: Victoria Dye <vdye@github.com>
To: Junio C Hamano <gitster@pobox.com>,
Victoria Dye via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, derrickstolee@github.com,
johannes.schindelin@gmx.de,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v3 06/11] diagnose.c: add option to configure archive contents
Date: Fri, 12 Aug 2022 10:00:30 -0700 [thread overview]
Message-ID: <32f2cadc-556e-1cd5-a238-c8f1cdaaf470@github.com> (raw)
In-Reply-To: <xmqq8rnviqhz.fsf@gitster.g>
Junio C Hamano wrote:
> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> @@ -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/
>
> Other than that, this new patch looks good to me.
>
> Thanks.
next prev parent reply other threads:[~2022-08-12 17:00 UTC|newest]
Thread overview: 94+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-01 21:14 [PATCH 0/7] Generalize 'scalar diagnose' into 'git bugreport --diagnose' Victoria Dye via GitGitGadget
2022-08-01 21:14 ` [PATCH 1/7] scalar: use "$GIT_UNZIP" in 'scalar diagnose' test Victoria Dye via GitGitGadget
2022-08-01 21:46 ` Junio C Hamano
2022-08-01 21:14 ` [PATCH 2/7] builtin/bugreport.c: create '--diagnose' option Victoria Dye via GitGitGadget
2022-08-01 22:16 ` Junio C Hamano
2022-08-02 15:40 ` Victoria Dye
2022-08-02 2:17 ` Ævar Arnfjörð Bjarmason
2022-08-01 21:14 ` [PATCH 3/7] builtin/bugreport.c: avoid size_t overflow Victoria Dye via GitGitGadget
2022-08-01 22:18 ` Junio C Hamano
2022-08-02 16:26 ` Victoria Dye
2022-08-02 20:51 ` Junio C Hamano
2022-08-02 2:03 ` Ævar Arnfjörð Bjarmason
2022-08-02 16:26 ` Victoria Dye
2022-08-03 12:25 ` Ævar Arnfjörð Bjarmason
2022-08-01 21:14 ` [PATCH 4/7] builtin/bugreport.c: add directory to archiver more gently Victoria Dye via GitGitGadget
2022-08-01 22:22 ` Junio C Hamano
2022-08-02 15:43 ` Victoria Dye
2022-08-01 21:14 ` [PATCH 5/7] builtin/bugreport.c: add '--no-report' option Victoria Dye via GitGitGadget
2022-08-01 22:31 ` Junio C Hamano
2022-08-02 19:46 ` Victoria Dye
2022-08-01 21:14 ` [PATCH 6/7] scalar: use 'git bugreport --diagnose' in 'scalar diagnose' Victoria Dye via GitGitGadget
2022-08-01 21:14 ` [PATCH 7/7] scalar: update technical doc roadmap Victoria Dye via GitGitGadget
2022-08-01 21:34 ` [PATCH 0/7] Generalize 'scalar diagnose' into 'git bugreport --diagnose' Junio C Hamano
2022-08-02 2:49 ` Ævar Arnfjörð Bjarmason
2022-08-02 19:48 ` Victoria Dye
2022-08-03 12:34 ` Ævar Arnfjörð Bjarmason
2022-08-04 1:45 ` [PATCH v2 00/10] Generalize 'scalar diagnose' into 'git diagnose' and " Victoria Dye via GitGitGadget
2022-08-04 1:45 ` [PATCH v2 01/10] scalar-diagnose: use "$GIT_UNZIP" in test Victoria Dye via GitGitGadget
2022-08-04 1:45 ` [PATCH v2 02/10] scalar-diagnose: avoid 32-bit overflow of size_t Victoria Dye via GitGitGadget
2022-08-04 1:45 ` [PATCH v2 03/10] scalar-diagnose: add directory to archiver more gently Victoria Dye via GitGitGadget
2022-08-04 6:19 ` Ævar Arnfjörð Bjarmason
2022-08-04 17:12 ` Junio C Hamano
2022-08-04 20:12 ` Ævar Arnfjörð Bjarmason
2022-08-04 21:09 ` Junio C Hamano
2022-08-04 1:45 ` [PATCH v2 04/10] scalar-diagnose: move 'get_disk_info()' to 'compat/' Victoria Dye via GitGitGadget
2022-08-04 1:45 ` [PATCH v2 05/10] scalar-diagnose: move functionality to common location Victoria Dye via GitGitGadget
2022-08-04 6:24 ` Ævar Arnfjörð Bjarmason
2022-08-04 1:45 ` [PATCH v2 06/10] builtin/diagnose.c: create 'git diagnose' builtin Victoria Dye via GitGitGadget
2022-08-04 6:27 ` Ævar Arnfjörð Bjarmason
2022-08-05 19:38 ` Derrick Stolee
2022-08-11 11:06 ` Ævar Arnfjörð Bjarmason
2022-08-05 19:11 ` Derrick Stolee
2022-08-04 1:45 ` [PATCH v2 07/10] builtin/diagnose.c: gate certain data behind '--all' Victoria Dye via GitGitGadget
2022-08-04 6:39 ` Ævar Arnfjörð Bjarmason
2022-08-04 1:45 ` [PATCH v2 08/10] builtin/bugreport.c: create '--diagnose' option Victoria Dye via GitGitGadget
2022-08-05 19:35 ` Derrick Stolee
2022-08-09 23:53 ` Victoria Dye
2022-08-10 12:52 ` Derrick Stolee
2022-08-10 16:13 ` Victoria Dye
2022-08-10 16:47 ` Derrick Stolee
2022-08-04 1:45 ` [PATCH v2 09/10] scalar-diagnose: use 'git diagnose --all' Victoria Dye via GitGitGadget
2022-08-04 6:54 ` Ævar Arnfjörð Bjarmason
2022-08-09 16:54 ` Victoria Dye
2022-08-04 1:45 ` [PATCH v2 10/10] scalar: update technical doc roadmap Victoria Dye via GitGitGadget
2022-08-04 17:22 ` [PATCH v2 00/10] Generalize 'scalar diagnose' into 'git diagnose' and 'git bugreport --diagnose' Junio C Hamano
2022-08-09 16:17 ` Victoria Dye
2022-08-09 16:50 ` Junio C Hamano
2022-08-10 23:34 ` [PATCH v3 00/11] " Victoria Dye via GitGitGadget
2022-08-10 23:34 ` [PATCH v3 01/11] scalar-diagnose: use "$GIT_UNZIP" in test Victoria Dye via GitGitGadget
2022-08-10 23:34 ` [PATCH v3 02/11] scalar-diagnose: avoid 32-bit overflow of size_t Victoria Dye via GitGitGadget
2022-08-10 23:34 ` [PATCH v3 03/11] scalar-diagnose: add directory to archiver more gently Victoria Dye via GitGitGadget
2022-08-10 23:34 ` [PATCH v3 04/11] scalar-diagnose: move 'get_disk_info()' to 'compat/' Victoria Dye via GitGitGadget
2022-08-10 23:34 ` [PATCH v3 05/11] scalar-diagnose: move functionality to common location Victoria Dye via GitGitGadget
2022-08-10 23:34 ` [PATCH v3 06/11] diagnose.c: add option to configure archive contents Victoria Dye via GitGitGadget
2022-08-11 0:16 ` Junio C Hamano
2022-08-12 17:00 ` Victoria Dye [this message]
2022-08-11 10:51 ` Ævar Arnfjörð Bjarmason
2022-08-11 15:43 ` Victoria Dye
2022-08-10 23:34 ` [PATCH v3 07/11] builtin/diagnose.c: create 'git diagnose' builtin Victoria Dye via GitGitGadget
2022-08-10 23:34 ` [PATCH v3 08/11] builtin/diagnose.c: add '--mode' option Victoria Dye via GitGitGadget
2022-08-10 23:34 ` [PATCH v3 09/11] builtin/bugreport.c: create '--diagnose' option Victoria Dye via GitGitGadget
2022-08-11 10:53 ` Ævar Arnfjörð Bjarmason
2022-08-11 15:40 ` Victoria Dye
2022-08-11 20:30 ` Ævar Arnfjörð Bjarmason
2022-08-10 23:34 ` [PATCH v3 10/11] scalar-diagnose: use 'git diagnose --mode=all' Victoria Dye via GitGitGadget
2022-08-10 23:34 ` [PATCH v3 11/11] scalar: update technical doc roadmap Victoria Dye via GitGitGadget
2022-08-12 20:10 ` [PATCH v4 00/11] Generalize 'scalar diagnose' into 'git diagnose' and 'git bugreport --diagnose' Victoria Dye via GitGitGadget
2022-08-12 20:10 ` [PATCH v4 01/11] scalar-diagnose: use "$GIT_UNZIP" in test Victoria Dye via GitGitGadget
2022-08-12 20:10 ` [PATCH v4 02/11] scalar-diagnose: avoid 32-bit overflow of size_t Victoria Dye via GitGitGadget
2022-08-12 20:10 ` [PATCH v4 03/11] scalar-diagnose: add directory to archiver more gently Victoria Dye via GitGitGadget
2022-08-12 20:10 ` [PATCH v4 04/11] scalar-diagnose: move 'get_disk_info()' to 'compat/' Victoria Dye via GitGitGadget
2022-08-12 20:10 ` [PATCH v4 05/11] scalar-diagnose: move functionality to common location Victoria Dye via GitGitGadget
2022-08-12 20:26 ` Junio C Hamano
2022-08-12 21:00 ` Victoria Dye
2022-08-12 21:20 ` Junio C Hamano
2022-08-12 20:10 ` [PATCH v4 06/11] diagnose.c: add option to configure archive contents Victoria Dye via GitGitGadget
2022-08-12 20:31 ` Junio C Hamano
2022-08-12 20:10 ` [PATCH v4 07/11] builtin/diagnose.c: create 'git diagnose' builtin Victoria Dye via GitGitGadget
2022-08-18 18:43 ` Ævar Arnfjörð Bjarmason
2022-08-18 19:12 ` Junio C Hamano
2022-08-12 20:10 ` [PATCH v4 08/11] builtin/diagnose.c: add '--mode' option Victoria Dye via GitGitGadget
2022-08-12 20:10 ` [PATCH v4 09/11] builtin/bugreport.c: create '--diagnose' option Victoria Dye via GitGitGadget
2022-08-12 20:10 ` [PATCH v4 10/11] scalar-diagnose: use 'git diagnose --mode=all' Victoria Dye via GitGitGadget
2022-08-12 20:10 ` [PATCH v4 11/11] scalar: update technical doc roadmap Victoria Dye via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=32f2cadc-556e-1cd5-a238-c8f1cdaaf470@github.com \
--to=vdye@github.com \
--cc=avarab@gmail.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=johannes.schindelin@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.