From: Victoria Dye <vdye@github.com>
To: Derrick Stolee <derrickstolee@github.com>,
Victoria Dye via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Cc: johannes.schindelin@gmx.de, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v2 08/10] builtin/bugreport.c: create '--diagnose' option
Date: Tue, 9 Aug 2022 16:53:16 -0700 [thread overview]
Message-ID: <a45f5693-7186-2953-6620-3f1359a12238@github.com> (raw)
In-Reply-To: <3dc402e1-1f27-8a24-544d-d90d403a7da0@github.com>
Derrick Stolee wrote:
> On 8/3/2022 9:45 PM, Victoria Dye via GitGitGadget wrote:
>> From: Victoria Dye <vdye@github.com>
>
>> +--diagnose[=(basic|all)]::
>> + Create a zip archive of information about the repository including logs
>
> logs? I think the reflogs are not included unless "all" is specified. Perhaps
> we can unify this description with the beginning of git-diagnose.txt:
>
> Collects detailed information about the user's machine, Git client, and
> repository state and packages that information into a zip archive.
>
> resulting in
>
> Create a zip archive containing information about the user's machine,
> Git client, and repository state.
>
[...]
>> + and certain statistics describing the data shape of the repository. The
>> + archive is written to the same output directory as the bug report and is
>> + named 'git-diagnostics-<formatted suffix>'.
>> ++
>> +By default, `--diagnose` (equivalent to `--diagnose=basic`) will collect only
>> +statistics and summarized data about the repository and filesystem. Specifying
>> +`--diagnose=all` will create an archive with the same contents generated by `git
>> +diagnose --all`; this archive will be much larger, and will contain potentially
>> +sensitive information about the repository. See linkgit:git-diagnose[1] for more
>> +details on the contents of the diagnostic archive.
>
> Perhaps here (and git-diagnose.txt) should be really explicit about sharing the
> "all" mode output only with trusted parties. Let the user decide what level of
> trust is necessary depending on their situation (we don't need to say "open source
> repos are fine to share" or something).
Both of these documentation suggestions make sense to me; I'll update
accordingly in V3.
>
>> +enum diagnose_mode {
>> + DIAGNOSE_NONE,
>> + DIAGNOSE_BASIC,
>> + DIAGNOSE_ALL
>> +};
>
> This enum makes me think that it might be nice to use this in diagnose.h
> along with an array that pairs strings with the enum. We could unify the
> options by having 'git diagnose --mode=(basic|all)' which could be
> extended in the future with another mode that might be in between the two.
>
> It may also be a waste of time to set up that infrastructure without it
> actually mattering in the future, but I thought I'd mention it as an
> alternative, in case that inspires you.
Your suggestion is more extensible than the boolean "include_everything" I'm
using right now in 'create_diagnostics_archive()'. I'll incorporate it into
my next re-roll, thanks!
>
>> static void get_system_info(struct strbuf *sys_info)
>> {
>> @@ -91,6 +97,23 @@ static void get_header(struct strbuf *buf, const char *title)
>> strbuf_addf(buf, "\n\n[%s]\n", title);
>> }
>>
>> +static int option_parse_diagnose(const struct option *opt,
>> + const char *arg, int unset)
>> +{
>> + enum diagnose_mode *diagnose = opt->value;
>> +
>> + BUG_ON_OPT_NEG(unset);
>> +
>> + if (!arg || !strcmp(arg, "basic"))
>> + *diagnose = DIAGNOSE_BASIC;
>> + else if (!strcmp(arg, "all"))
>> + *diagnose = DIAGNOSE_ALL;
>
> Should we allow "none" to reset the value to DIAGNOSE_NONE?
As far as I can tell, while some builtins have options that match the
default behavior of the command (e.g., '--no-autosquash' in 'git rebase'),
those options typically exist to override a config setting (e.g.,
'rebase.autosquash'). No config exists for 'bugreport --diagnose' (and I
don't think it would make sense to add one), so '--diagnose=none' would only
be used to override another '--diagnose' specification in the same
command/alias (e.g., 'git bugreport --diagnose=basic --diagnose=none').
That use case seems unlikely, but if there's precedent or use cases I'm not
accounting for, I'm happy to add the option.
>
>> + else
>> + die(_("diagnose mode must be either 'basic' or 'all'"));
>
> I wondered initially if this should be a usage() call instead. But we have
> plenty of examples of using die() to report an issue with a single option
> or a combination of options.
After looking at other 'OPT_CALLBACK_F' examples (e.g., the 'diff_opt_stat'
options in 'diff.c'), I think this should at least change to 'return
error(<something>)' to get a more appropriate return code (129):
else
return error(_("invalid --%s mode '%s'"), opt->long_name, arg);
I also looked into using 'usage_msg_opt()' to print both an error message
and the usage string, but doing so would require making the 'bugreport'
options structure & variables static so they're accessible by
'option_parse_diagnose()'. If you think it would be valuable to include that
information, I'll add an extra refactoring patch to include it.
>
>> const struct option bugreport_options[] = {
>> + OPT_CALLBACK_F(0, "diagnose", &diagnose, N_("(basic|all)"),
>> + N_("create an additional zip archive of detailed diagnostics"),
>> + PARSE_OPT_NONEG | PARSE_OPT_OPTARG, option_parse_diagnose),
>
> The biggest reason for this to be an OPT_CALLBACK_F is because of the
> '--diagnose' option (without '='), so an OPT_STRING would not be
> appropriate here.
>
>> @@ -119,6 +147,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>> option_output ? option_output : "");
>> strbuf_addstr(&report_path, prefixed_filename);
>> strbuf_complete(&report_path, '/');
>> + output_path_len = report_path.len;
>
> Perhaps this should be renamed to output_dir_len, since we know this is
> a directory that will contain all of the output files.
Agreed, 'path' is a bit ambiguous and 'dir' more clearly refers to the
directory containing the output files.
>
>> + /* Prepare diagnostics, if requested */
>> + if (diagnose != DIAGNOSE_NONE) {
>> + struct strbuf zip_path = STRBUF_INIT;
>> + strbuf_add(&zip_path, report_path.buf, output_path_len);
>> + strbuf_addstr(&zip_path, "git-diagnostics-");
>> + strbuf_addftime(&zip_path, option_suffix, localtime_r(&now, &tm), 0, 0);
>> + strbuf_addstr(&zip_path, ".zip");
>> +
>> + if (create_diagnostics_archive(&zip_path, diagnose == DIAGNOSE_ALL))
>
> (Just pausing to say this could be create_diagnostics_archive(&zip_path, diagnose)
> if we use the enum inside diagnose.c.
>
> Thanks,
> -Stolee
next prev parent reply other threads:[~2022-08-09 23:53 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 [this message]
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
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=a45f5693-7186-2953-6620-3f1359a12238@github.com \
--to=vdye@github.com \
--cc=avarab@gmail.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.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.