Subject: Re: [PATCH] tr2: log parent process name

From: Emily Shaffer <[email-masked]>
Date: Fri, 14 May 2021 15:06:59 -0700

On Mon, May 10, 2021 at 02:29:15PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> 
> On Thu, May 06 2021, Emily Shaffer wrote:
> 
> > It can be useful to tell who invoked Git - was it invoked manually by a
> > user via CLI or script? By an IDE? Knowing where the Git invocation came
> > from can help with debugging to isolate where the problem came from.
> 
> Aside from the portability concerns others have raised, I don't really
> see why you'd need this.
> 
> We already have the nest-level as part of the SID, so isn't it
> sufficient (and portable) at the top-level to log what isatty says + set
> the initial SID "root" in the IDE (which presumably knows about git).

If you already know all the IDEs and scripts which invoke Git, sure, you
could set the SID root - we do this with 'repo' tool today. But actually
we want this because we aren't sure exactly who at Google is invoking
Git in their tooling, how, why, etc. - this logline was supposed to help
with that. Chicken, egg, etc.

Or else I'm misunderstanding your suggestion; to me it sounded like "go
fix your VSCode plugin so that it sets an additional envvar" and I
responded accordingly. If you mean something else I didn't see,
implemented in Git land, then I'm curious to hear more.

> 
> Wouldn't this log passwords in cases of e.g.:
> 
>     some-script --git-password secret # invokes "git"

I have nothing but nasty things to say about scripts which would do
that, but your point is valid enough to make me think this logline
should be gated behind a config and posted much later (when config is
available - I think it's not yet, with the patch as-is).

> 
> In older versions of linux reading e.g. smaps from /proc/self would
> stall the kernel while the read was happening, I haven't checked whether
> cmdline is such a thing (probably not), but it's a subtle thing to have
> in mind for this / follow-ups if it's made portable (is that an issue on
> other OS's?).

That's an interesting point and I wonder how I can validate if it
does/doesn't stall in this way - I guess I can go manpage diving?

 - Emily