Re: [PATCH v7 03/15] bugreport: add tool to generate debugging info

[Junio C Hamano <gitster@pobox.com>]

Emily Shaffer <emilyshaffer@google.com> writes:

> On Fri, Feb 14, 2020 at 09:25:12AM -0800, Junio C Hamano wrote:
>> Emily Shaffer <emilyshaffer@google.com> writes:
>> 
>> > +	switch (safe_create_leading_directories(report_path.buf)) {
>> 
>> This helper is about creating paths in the working tree and Git
>> repository,
>
> It's also used by cmd_format_patch() with --output-directory specified,
> which is how I found it.

And that is an example of a good use of this helper.  What will be
written out there should be visible by the same people as those who
have access to the repository; get_shared_repo() and adjust_perm()
based on what the repository you are taking patches from is
perfectly sensible.  And as it is format-patch, we know we have
get_shared_repo() working, and we know which repository we are
working on.

Output directory for bugreport is on the same boat when we know the
users are producing a report on their use of Git within a
repository.  It is not clear if the tool is started without any
repository---it happens to do a random safe thing (i.e. I think
get_shared_repo() gives PERM_UMASK, which tells adjust_perm() not to
do anything especial) right now, but there is no guarantee that we
will keep it working like that.  Somebody may come and demaind
get_shared_perm() to die() when outside a repository, for example,
and that would break the nice property that lets bugreport working
outside a repository.

I just wanted to make sure that somebody will be keeping an eye to
remind those who propose such a change in the future.  A comment
near where get_shared_repo() happens may be something that can be
done with a minimum effort when code that depends on that property
is introduced at the same time.

>> I thought I read somewhere that this tool is meant to be usable
>> outside a repository?  If that is not the case, then the use of this
>> helper is OK.  If not, we may want to make sure that it will stay to
>> be safe to use the helper (I think it happens to be OK right now,

I am actually OK if we limit the use of this tool to "use with a
repository that is not corrupt", as coping with these kinds of
breakages that in the main Git executable we deem "needs manual
intervention" inside a single process is too painful (it would have
been easier to cope with these too if we stuck with a script that
invokes many discrete commands and acts on their errors, but that is
optimizing for rare case and not recommended).  But we should tell
users about the limitation and encourage them to ask for help in non
automatable means.

Thanks.