From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Tue, 02 Aug 2022 02:17:52 GMT
Subject: Re: [PATCH 2/7] builtin/bugreport.c: create '--diagnose' option
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:

> 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...

> --- 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?