Re: [PATCH v2 07/10] builtin/diagnose.c: gate certain data behind '--all'

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 4, 2022, 06:39 UTC
Message-ID: <220804.86v8r8ec4s.gmgdl@evledraar.gmail.com>
In-Reply-To: <a3e62a4a0418abbff5015302f0102ffacccf0ca4.1659577543.git.gitgitgadget@gmail.com>

On Thu, Aug 04 2022, Victoria Dye via GitGitGadget wrote:

Show 6 quoted lines

> From: Victoria Dye <vdye@github.com>
> [...]
>  --------
>  [verse]
>  'git diagnose' [(-o | --output-directory) <path>] [(-s | --suffix) <format>]
> +	       [-a | --all]

I have some local patches that...

Show 5 quoted lines

>  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.

Show 15 quoted lines

> @@ -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...

Show 20 quoted lines

>  {
>  	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.