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

From: Emily Shaffer <emilyshaffer@google.com>
Date: Feb 18, 2020, 23:46 UTC
Message-ID: <20200218234628.GA1461@google.com>
In-Reply-To: <xmqq1rqvogif.fsf@gitster-ct.c.googlers.com>

On Sat, Feb 15, 2020 at 10:24:40AM -0800, Junio C Hamano wrote:

Show 31 quoted lines

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

Hm, this would break the convention of the name of safe_create_leading_directories() too though right? As I understood it, "safe_foo" in this codebase means "this function will not die()"?

Show 6 quoted lines

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

Ok. I want to make sure I understand you right. I think you're saying, "This is OK, except if someone changes get_shared_repo() it could break, so we need a way to warn someone that it could break." It sounds like you suggested leaving a comment in the safe_create_leading_directories() helper. My own preference is to write a test so that it's explicit that we depend on that behavior, instead - it's easy to gloss over a comment or read a different part of the codebase, but it's hard to ignore a breaking test. It'd be trivial to add one, so I will in v8 - unless I misunderstood you.

Show 15 quoted lines

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

I think you're saying, "Mention this drawback in the manpage for git-bugreport." Sounds like a good idea to me, so I'll add it for v8.

 - Emily