Re: [PATCH] diagnose: require repository

From: Martin Ågren <martin.agren@gmail.com>
Date: Oct 19, 2023, 13:18 UTC
Message-ID: <CAN0heSqmZ7QXJbet2Tp=YYCjBLToOHtNy+n=zcf29XYaukYN0w@mail.gmail.com>
In-Reply-To: <xmqq5y39unvc.fsf@gitster.g>

On Sat, 14 Oct 2023 at 19:15, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> > Switch from the gentle setup to requiring a git directory. Without a git
> > repo, there isn't really much to diagnose.
> >
> > We could possibly do a best-effort collection of information about the
> > machine and then give up. That would roughly be today's behavior but
> > with a controlled exit rather than a segfault. However, the purpose of
> > this tool is largely to create a zip archive. Rather than creating an
> > empty zip file or no zip file at all, and having to explain that

Correcting myself: The zip archive would actually contain `diagnostics.log` with some general info about the machine and Git build.

Show 9 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`.

Show 5 quoted lines

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

Martin