Re: [PATCH v3 09/11] builtin/bugreport.c: create '--diagnose' option

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Victoria Dye <vdye@github.com>
Cc: Victoria Dye via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, derrickstolee@github.com,
	johannes.schindelin@gmx.de
Subject: Re: [PATCH v3 09/11] builtin/bugreport.c: create '--diagnose' option
Date: Thu, 11 Aug 2022 22:30:22 +0200	[thread overview]
Message-ID: <220811.86fsi2plk7.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <f9a5f658-7c6f-bb3a-d6df-0ab7d2908395@github.com>

On Thu, Aug 11 2022, Victoria Dye wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Wed, Aug 10 2022, Victoria Dye via GitGitGadget wrote:
>> 
>>> From: Victoria Dye <vdye@github.com>
>>>
>>> Create a '--diagnose' option for 'git bugreport' to collect additional
>>> information about the repository and write it to a zipped archive.
>>> [...]
>>>  'git bugreport' [(-o | --output-directory) <path>] [(-s | --suffix) <format>]
>>> +		[--diagnose[=<mode>]]
>>> [...]
>>>  static const char * const bugreport_usage[] = {
>>> -	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
>>> +	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>] [--diagnose[=<mode>]"),
>>>  	NULL
>>>  };
>> 
>> This still has the SYNOPSIS v.s. -h discrepancy noted in
>> https://lore.kernel.org/git/220804.86v8r8ec4s.gmgdl@evledraar.gmail.com/
>
> The discrepancy you pointed out was on 'git diagnose' (which has since been
> fixed),

Ah, sorry. I missed that & conflated the two.

> this is a pre-existing one in 'git bugreport'. I decided against
> fixing *this* one because it didn't really fit into any of the patches in
> this series, so it would need its own patch. When balancing "leave things
> better than you found them" vs. "stay focused on the purpose of the series",
> I leaned towards the latter to avoid setting a precedent for other 'git
> bugreport'-related scope creep.

In any case, I'm pointing out the difference in one of them having
\n-wrapping inconsistent with the other, which is an addition in this
series, sorry about not being clear.

I see that there's also the difference in how they format "--suffix",
but that's pre-existing & we can leave it for now. I think that's what
you're pointing out here as pre-existing.

> If you have the patches to audit this sort of thing, I think a nice place to
> fix this might be in a dedicated series fixing discrepancies tree-wide. Even
> better, you could include the patches in your tree that detect them as part
> of e.g. the 'static-analysis' CI job.

Yeah I do have those, and will probably submit those sooner than later,
and I'll end up spotting differences once they land on "master"
(e.g. [1] is one such case).

But this one is just one I eyeballed during review.

1. https://lore.kernel.org/git/220811.86o7wrov26.gmgdl@evledraar.gmail.com/