On Thu, Aug 04 2022, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@xxxxxxxxxx>
> [...]
> --------
> [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.
> @@ -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...
> {
> 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.
