Re: [PATCH v2 07/10] builtin/diagnose.c: gate certain data behind '--all'
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-08-04 06:50:02
On Thu, Aug 04 2022, Victoria Dye via GitGitGadget wrote:
From: Victoria Dye <redacted> [...] -------- [verse] 'git diagnose' [(-o | --output-directory) <path>] [(-s | --suffix) <format>] + [-a | --all]
I have some local patches that...
static const char * const diagnose_usage[] = {
- N_("git diagnose [-o|--output-directory <file>] [-s|--suffix <format>]"),
+ N_("git diagnose [-o|--output-directory <file>] [-s|--suffix <format>] [-a|--all]"),
NULL
};...spot when we have SYNOPSIS & -h discrepancies. In this case we break with a \n after <format> in the SYNOPSIS, but you don't add a "\n" and indentation here in the -h output. The two should be the same.
quoted hunk ↗ jump to hunk
@@ -13,6 +13,7 @@ int cmd_diagnose(int argc, const char **argv, const char *prefix) struct strbuf zip_path = STRBUF_INIT; time_t now = time(NULL); struct tm tm; + int include_everything = 0; char *option_output = NULL; char *option_suffix = "%Y-%m-%d-%H%M"; char *prefixed_filename;@@ -22,6 +23,9 @@ int cmd_diagnose(int argc, const char **argv, const char *prefix) N_("specify a destination for the diagnostics archive")), OPT_STRING('s', "suffix", &option_suffix, N_("format"), N_("specify a strftime format suffix for the filename")), + OPT_BOOL_F('a', "all", &include_everything, + N_("collect complete diagnostic information"), + PARSE_OPT_NONEG),
Nice to have a "stats only" by default and some way to add the whole shebang optionally...
+int create_diagnostics_archive(struct strbuf *zip_path, int include_everything)
...but maybe...
quoted hunk ↗ jump to hunk
{ struct strvec archiver_args = STRVEC_INIT; char **argv_copy = NULL;@@ -176,11 +176,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 (include_everything && + ((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;
...this should be --include-gitdir-extract or something, instead of
"--all" and "--include-everything".
I'd think that "all" would be a thing that would actually tar up my
entire .git directory as-is (in a way that would pass git fsck on the
other end (unless that's the bug being reported...)).
Aside: Since we are getting the churn of adding this, then re-indenting
it maybe a prep step of adding a add_directories_to_archiver() would be
useful, which would just have a data-driven:
{
{ ".git" },
[...],
{ ".git/logs, 1 },
NULL
},
Then loop over that, making it easy to add/declare new subdirs to add.