From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 12 Aug 2022 20:31:58 GMT
Subject: Re: [PATCH v4 06/11] diagnose.c: add option to configure archive contents
Message-ID: <xmqq35e1b3up.fsf@gitster.g>
In-Reply-To: <710b67e5776363d199ed5043d019386819d44e7e.1660335019.git.gitgitgadget@gmail.com>

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

> -int create_diagnostics_archive(struct strbuf *zip_path)
> +int create_diagnostics_archive(struct strbuf *zip_path, enum diagnose_mode mode)
>  {
>  	struct strvec archiver_args = STRVEC_INIT;
>  	char **argv_copy = NULL;
>  	int stdout_fd = -1, archiver_fd = -1;
>  	struct strbuf buf = STRBUF_INIT;
> -	int res;
> +	int res, i;
> +	struct archive_dir archive_dirs[] = {
> +		{ ".git", 0 },
> +		{ ".git/hooks", 0 },
> +		{ ".git/info", 0 },
> +		{ ".git/logs", 1 },
> +		{ ".git/objects/info", 0 }
> +	};
> +
> +	if (mode == DIAGNOSE_NONE) {
> +		res = 0;
> +		goto diagnose_cleanup;
> +	}
>  
>  	stdout_fd = dup(STDOUT_FILENO);
>  	if (stdout_fd < 0) {
> @@ -177,12 +194,18 @@ 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)))
> -		goto diagnose_cleanup;
> +	/* Only include this if explicitly requested */
> +	if (mode == DIAGNOSE_ALL) {
> +		for (i = 0; i < ARRAY_SIZE(archive_dirs); i++) {
> +			if (add_directory_to_archiver(&archiver_args,
> +						      archive_dirs[i].path,
> +						      archive_dirs[i].recursive)) {
> +				res = error_errno(_("could not add directory '%s' to archiver"),
> +						  archive_dirs[i].path);
> +				goto diagnose_cleanup;
> +			}
> +		}
> +	}

Even without the "only include under DIAGNOSE_ALL" support added by
this step, the table-driven organization is much nicer.  The earlier
"move to common" step aimed to be as close as pure move, so this
step is our first opportunity to make this clean-up, so I do not
mind too much about this step doing two unrelated things (one is to
clean-up the if (A||B||C) into a loop over A, B and C, the other is
to introduce the diagnose_mode) at once.

Thanks.