Re: [PATCH v6+ 4/7] scalar: implement `scalar diagnose`

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Jun 10 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Sat, May 28 2022, Junio C Hamano wrote:
>>
>>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>> [...]
>>> The `diagnose` command is the culmination of this hard-won knowledge: it
>>> gathers the installed hooks, the config, a couple statistics describing
>>> the data shape, among other pieces of information, and then wraps
>>> everything up in a tidy, neat `.zip` archive.
>>> [...]
>>> +	if ((res = add_directory_to_archiver(&archiver_args, ".git", 0)) ||
>>> +	    (res = add_directory_to_archiver(&archiver_args, ".git/hooks", 0)) ||
>>> +	    (res = add_directory_to_archiver(&archiver_args, ".git/info", 0)) ||
>>> +	    (res = add_directory_to_archiver(&archiver_args, ".git/logs", 1)) ||
>>> +	    (res = add_directory_to_archiver(&archiver_args, ".git/objects/info", 0)))
>>> +		goto diagnose_cleanup;
>>
>> Noticed on top of some local changes I have to not add a
>> .git/hooks (the --no-template topic), but this fails to diagnose
>> any repo that doesn't have these paths, which are optional, either
>> because a user could have manually removed them, or used
>> --template=.
>
> Quite honestly, if it lacks any directory that we traditionally
> created upon "git init", with our standard templates, we can and
> should call such a repository "broken" and move on.

In our own test suite we do e.g. (and did more of that until some recent
changes of mine):

    git mv .git/hooks .git/hooks.disabled

We've never documented in "git init" or the like that these very
optional directories in .git/ were some sort of hard requirenment, and
e.g. core.hooksPath and gitrepository-layout(5) explicitly seem to
suggest otherwise.

In any case, there's the golden rule about being strict in what you emit
and loose in what you accept, which we've taken with repository
compatibility. Having a tool that's designed to aid bugreporting be
picky about what sort of repository it supports seems to go against the
point of such a tool.

Particularly in this case, where it seems easy to just guard it with a
stat() check, or not error out if we fail to add this to the *.zip file,
no?