diff mbox series

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

Message ID 0a6c55696d88cde666c11cd6b5d723c9e75a3b76.1660174473.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Generalize 'scalar diagnose' into 'git diagnose' and 'git bugreport --diagnose' | expand

Commit Message

Victoria Dye Aug. 10, 2022, 11:34 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.

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

Comments

Junio C Hamano Aug. 11, 2022, 12:16 a.m. UTC | #1 
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

> @@ -177,11 +182,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 (mode == DIAGNOSE_ALL &&
> +	    ((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;

At first glance, it looks as if this part fails silently, but
add_directory_to_archiver() states what failed there, so we show
necessary error messages and do not silently fail, which is good.

There is a "failed to write archive" message after write_archive()
call returns non-zero, but presumably write_archive() itself gives
diagnostics (like "oh, I was told to archive this file but I cannot
read it") when it does so, so in a sense, giving the concluding
"failed to write" only in that case might make the error messages
uneven.  If we fail to enlist ".git/hooks" directory, we may want to
say why we failed to do so, and then want to see the concluding
"failed to write" at the end, just like the case where write_archive()
failed.

It is a truely minor point, and if it turns out to be worth fixing,
it can be easily done by moving the diagnose_clean_up label a bit
higher, i.e.

	...
	res = write_archive(...);

diagnose_cleanup:
	if (res)
		error(_("failed to write archive"));
	else
        	fprintf(stderr, "\n"
			"Diagnostics complete.\n"
			"All of the gathered info is captured in '%s'\n",
			zip_path->buf);

	if (archiver_fd >= 0) {
		... restore FD#1 and close stdout_fd and archiver_fd
	}
	...


Other than that, this new patch looks good to me.

Thanks.
Ævar Arnfjörð Bjarmason Aug. 11, 2022, 10:51 a.m. UTC | #2 
On Wed, Aug 10 2022, Victoria Dye via GitGitGadget wrote:

> index 06dca69bdac..9bb6049bf0c 100644
> --- a/diagnose.h
> +++ b/diagnose.h
> @@ -2,7 +2,14 @@
>  #define DIAGNOSE_H
>  
>  #include "strbuf.h"
> +#include "parse-options.h"

This is a stray include that isn't needed at this point, some mistake,
or needed by a subsequent patch?
Victoria Dye Aug. 11, 2022, 3:43 p.m. UTC | #3 
Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Aug 10 2022, Victoria Dye via GitGitGadget wrote:
> 
>> index 06dca69bdac..9bb6049bf0c 100644
>> --- a/diagnose.h
>> +++ b/diagnose.h
>> @@ -2,7 +2,14 @@
>>  #define DIAGNOSE_H
>>  
>>  #include "strbuf.h"
>> +#include "parse-options.h"
> 
> This is a stray include that isn't needed at this point, some mistake,
> or needed by a subsequent patch?

It's needed by patch 8 [1]. If I re-roll again, I'll move this '#include' to
that patch. Thanks!

[1] https://lore.kernel.org/git/3da0cb725c927d08dd9486286e06bdb76896f5b7.1660174473.git.gitgitgadget@gmail.com/
Victoria Dye Aug. 12, 2022, 5 p.m. UTC | #4 
Junio C Hamano wrote:
> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> @@ -177,11 +182,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 (mode == DIAGNOSE_ALL &&
>> +	    ((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;
> 
> At first glance, it looks as if this part fails silently, but
> add_directory_to_archiver() states what failed there, so we show
> necessary error messages and do not silently fail, which is good.
> 
> There is a "failed to write archive" message after write_archive()
> call returns non-zero, but presumably write_archive() itself gives
> diagnostics (like "oh, I was told to archive this file but I cannot
> read it") when it does so, so in a sense, giving the concluding
> "failed to write" only in that case might make the error messages
> uneven.  If we fail to enlist ".git/hooks" directory, we may want to
> say why we failed to do so, and then want to see the concluding
> "failed to write" at the end, just like the case where write_archive()
> failed.
> 
> It is a truely minor point, and if it turns out to be worth fixing,
> it can be easily done by moving the diagnose_clean_up label a bit
> higher, i.e.
> 
> 	...
> 	res = write_archive(...);
> 
> diagnose_cleanup:
> 	if (res)
> 		error(_("failed to write archive"));
> 	else
>         	fprintf(stderr, "\n"
> 			"Diagnostics complete.\n"
> 			"All of the gathered info is captured in '%s'\n",
> 			zip_path->buf);
> 
> 	if (archiver_fd >= 0) {
> 		... restore FD#1 and close stdout_fd and archiver_fd
> 	}
> 	...
>

I like this idea, since I think there's value in indicating both the cause
("could not open directory") and effect ("failed to write archive") of the
error. I'll include this and [1] in a re-roll. Thanks!

[1] https://lore.kernel.org/git/9d1b0cb9-5c21-c101-8597-2fe166cb6abe@github.com/

> 
> Other than that, this new patch looks good to me.
> 
> 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 509d582f0ea..aadc3d4b21f 100644
--- a/diagnose.c
+++ b/diagnose.c
@@ -132,7 +132,7 @@  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;
@@ -140,6 +140,11 @@  int create_diagnostics_archive(struct strbuf *zip_path)
 	struct strbuf buf = STRBUF_INIT;
 	int res;
 
+	if (mode == DIAGNOSE_NONE) {
+		res = 0;
+		goto diagnose_cleanup;
+	}
+
 	stdout_fd = dup(STDOUT_FILENO);
 	if (stdout_fd < 0) {
 		res = error_errno(_("could not duplicate stdout"));
@@ -177,11 +182,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 (mode == DIAGNOSE_ALL &&
+	    ((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;
 
 	strvec_pushl(&archiver_args, "--prefix=",
diff --git a/diagnose.h b/diagnose.h
index 06dca69bdac..9bb6049bf0c 100644
--- a/diagnose.h
+++ b/diagnose.h
@@ -2,7 +2,14 @@ 
 #define DIAGNOSE_H
 
 #include "strbuf.h"
+#include "parse-options.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 */