Re: [PATCH v2 06/10] builtin/diagnose.c: create 'git diagnose' builtin

[Derrick Stolee <derrickstolee@github.com>]

On 8/3/2022 9:45 PM, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
> 
> Create a 'git diagnose' builtin to generate a standalone zip archive of
> repository diagnostics.

> +  * The contents of the `.git`, `.git/hooks`, `.git/info`, `.git/logs`, and
> +    `.git/objects/info` directories

You remove these lines in the next patch, which is called "gate certain data
behind '--all'" but maybe we shouldn't have this functionality now and
instead add it in the future.

The biggest reason for the --all option is that these contents will likely
include private IP (path names and branch names, but not file contents) that
the user would probably not want to share with the public mailing list, but
might want to share with a trusted Git expert in order to resolve a problem.
You mention earlier that

  The generated archive can then, for example, be shared with the
  Git mailing list to help debug an issue or serve as a reference for
  independent debugging.

So, if you're sending a v3, then moving this out of this patch and into the
next one would be a good way to be sure that this possibly-private data is
not mentioned as something to share super publicly.

(Of course, this requires making the change to create_diagnostics_archive()
in advance of creating the builtin, so maybe this reorganization isn't
worth it.)

> @@ -0,0 +1,58 @@
> +#include "builtin.h"
> +#include "parse-options.h"
> +#include "diagnose.h"
> +
> +

nit: double empty line

> +++ b/t/t0092-diagnose.sh
> @@ -0,0 +1,28 @@
> +#!/bin/sh
> +
> +test_description='git diagnose'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
> +. ./test-lib.sh
> +
> +test_expect_success UNZIP 'creates diagnostics zip archive' '
> +	test_when_finished rm -rf report &&
> +
> +	git diagnose -o report -s test >out &&
> +
> +	zip_path=report/git-diagnostics-test.zip &&
> +	grep "Available space" out &&
> +	test_path_is_file "$zip_path" &&

nit: 'grep' the output immediately after the 'git diagnose' command and
keep the zip_path use immediately after its definition.

Thanks,
-Stolee