Re: [PATCH] diagnose: require repository

From: Victoria Dye <vdye@github.com>
Date: Oct 19, 2023, 18:16 UTC
Message-ID: <01569dd1-f807-d56d-a123-5e8f3c930503@github.com>
In-Reply-To: <CAN0heSqmZ7QXJbet2Tp=YYCjBLToOHtNy+n=zcf29XYaukYN0w@mail.gmail.com>

Martin Ågren wrote:

Show 28 quoted lines

>>> behavior, it seems more helpful to bail out clearly and early with a
>>> succinct error message.
>>
>> Without having thought things through, offhand I agree with your "no
>> repository?  there is nothing worth tarring up then" assessment.
>>
>> Because "git bugreport --diag" unconditionally spawns "git
>> diagnose", the former may also want to be extra careful, perhaps
>> like the attached patch.
> 
> Good point. TBH, I had no idea about `git bugreport --diagnose`.
> 
>> +       if (!startup_info->have_repository && diagnose != DIAGNOSE_NONE) {
>> +               warning(_("no repository--diagnostic output disabled"));
>> +               diagnose = DIAGNOSE_NONE;
>> +       }
>> +
> 
> When the user explicitly provides that option, it seems unfortunate to
> me to drop it. Yes, we'd warn, but `git bugreport` then pops a text
> editor, so you would only see the warning after finishing up the report.
> (Maybe. By the time you quit your editor, you might not consider
> checking the terminal for warnings and such.)
> 
> So I'm inclined to instead just die if we see the option outside a repo.
> If `diagnose` the command fundamentally requires a repo (as with my
> patch) it seems surprising to me to not have `--diagnose` the option
> behave the same.

I agree - it was an oversight on my part to not firmly require the existence of a repository with 'git diagnose', and the same applies to 'bugreport --diagnose'.

For reference, there is one other usage of 'git diagnose' (in 'scalar diagnose'). However, it's already guarded by 'setup_git_directory()' so it shouldn't need to be updated.

> 
> Martin