diff mbox series

[v4,06/11] diagnose.c: add option to configure archive contents

Message ID 710b67e5776363d199ed5043d019386819d44e7e.1660335019.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 33cba726f03e1e06b10b85cf4a1cbd1017810486
Headers show
Series Generalize 'scalar diagnose' into 'git diagnose' and 'git bugreport --diagnose' | expand

Commit Message

Victoria Dye Aug. 12, 2022, 8:10 p.m. UTC 
From: Victoria Dye <vdye@github.com>

Update 'create_diagnostics_archive()' to take an argument 'mode'. When
archiving diagnostics for a repository, 'mode' is used to selectively
include/exclude information based on its value. The initial options for
'mode' are:

* DIAGNOSE_NONE: do not collect any diagnostics or create an archive
  (no-op).
* DIAGNOSE_STATS: collect basic repository metadata (Git version, repo path,
  filesystem available space) as well as sizing and count statistics for the
  repository's objects and packfiles.
* DIAGNOSE_ALL: collect basic repository metadata, sizing/count statistics,
  and copies of the '.git', '.git/hooks', '.git/info', '.git/logs', and
  '.git/objects/info' directories.

These modes are introduced to provide users the option to collect
diagnostics without the sensitive information included in copies of '.git'
dir contents. At the moment, only 'scalar diagnose' uses
'create_diagnostics_archive()' (with a hardcoded 'DIAGNOSE_ALL' mode to
match existing functionality), but more callers will be introduced in
subsequent patches.

Finally, refactor from a hardcoded set of 'add_directory_to_archiver()'
calls to iterative invocations gated by 'DIAGNOSE_ALL'. This allows for
easier future modification of the set of directories to archive and improves
error reporting when 'add_directory_to_archiver()' fails.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 contrib/scalar/scalar.c |  2 +-
 diagnose.c              | 39 +++++++++++++++++++++++++++++++--------
 diagnose.h              |  8 +++++++-
 3 files changed, 39 insertions(+), 10 deletions(-)

Comments

Junio C Hamano Aug. 12, 2022, 8:31 p.m. UTC | #1 
"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.
diff mbox series

Patch

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 3983def760a..d538b8b8f14 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -534,7 +534,7 @@  static int cmd_diagnose(int argc, const char **argv)
 		goto diagnose_cleanup;
 	}
 
-	res = create_diagnostics_archive(&zip_path);
+	res = create_diagnostics_archive(&zip_path, DIAGNOSE_ALL);
 
 diagnose_cleanup:
 	strbuf_release(&zip_path);
diff --git a/diagnose.c b/diagnose.c
index f0dcbfe1a2a..9270056db2f 100644
--- a/diagnose.c
+++ b/diagnose.c
@@ -8,6 +8,11 @@ 
 #include "object-store.h"
 #include "packfile.h"
 
+struct archive_dir {
+	const char *path;
+	int recursive;
+};
+
 static void dir_file_stats_objects(const char *full_path, size_t full_path_len,
 				   const char *file_name, void *data)
 {
@@ -132,13 +137,25 @@  static int add_directory_to_archiver(struct strvec *archiver_args,
 	return res;
 }
 
-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;
+			}
+		}
+	}
 
 	strvec_pushl(&archiver_args, "--prefix=",
 		     oid_to_hex(the_hash_algo->empty_tree), "--", NULL);
diff --git a/diagnose.h b/diagnose.h
index 06dca69bdac..998775857a0 100644
--- a/diagnose.h
+++ b/diagnose.h
@@ -3,6 +3,12 @@ 
 
 #include "strbuf.h"
 
-int create_diagnostics_archive(struct strbuf *zip_path);
+enum diagnose_mode {
+	DIAGNOSE_NONE,
+	DIAGNOSE_STATS,
+	DIAGNOSE_ALL
+};
+
+int create_diagnostics_archive(struct strbuf *zip_path, enum diagnose_mode mode);
 
 #endif /* DIAGNOSE_H */