Re: [PATCH 2/7] builtin/bugreport.c: create '--diagnose' option

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 2, 2022, 02:17 UTC
Message-ID: <220802.86edxzgz2z.gmgdl@evledraar.gmail.com>
In-Reply-To: <932dc8cddacf1841996cd3e0d512b95828213015.1659388498.git.gitgitgadget@gmail.com>

On Mon, Aug 01 2022, Victoria Dye via GitGitGadget wrote:

Show 7 quoted lines

> From: Victoria Dye <vdye@github.com>
> [...]
>  Documentation/git-bugreport.txt |  11 +-
>  builtin/bugreport.c             | 282 +++++++++++++++++++++++++++++++-
>  t/t0091-bugreport.sh            |  20 +++
>  3 files changed, 309 insertions(+), 4 deletions(-)
> [...]

Maybe it's not easy in this case, but I wonder if this series can't be re-arranged in a way that more directly benefits from the diff move detection.

E.g. if we moved the unchanged functions to a new repo-disk-usage.c or something we could have an intermediate step of having both use that, and then going forward would work towards a better lib/built-in split-up...

Show 14 quoted lines

> --- a/Documentation/git-bugreport.txt
> +++ b/Documentation/git-bugreport.txt
> @@ -8,7 +8,7 @@ git-bugreport - Collect information for user to file a bug report
>  SYNOPSIS
>  --------
>  [verse]
> -'git bugreport' [(-o | --output-directory) <path>] [(-s | --suffix) <format>]
> +'git bugreport' [<options>]
> [...]
>  static const char * const bugreport_usage[] = {
> -	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
> +	N_("git bugreport [<options>]"),
>  	NULL
>  };

We have some built-ins that punt on re-listing the synopsis in the -h output, but we always list the full usage in the SYNOPSIS.

I think both of these hunks should be dropped, instead we should (presumably) add a "git bugreport --diagnose" to this, and if it combines (or not) with other options, let's update both accordingly.

> [...]
> +	strvec_pushl(&archiver_args, "scalar-diagnose", "--format=zip", NULL);
>

Is the "scalar-diagnose" here a mistake?