On Tue, Mar 04, 2025 at 02:30:10AM -0500, Jeff King wrote:
> On Mon, Mar 03, 2025 at 06:07:29AM -0800, Junio C Hamano wrote:
>
> > Patrick Steinhardt <ps@xxxxxx> writes:
> >
> > > Maybe. I guess for unit tests it's a lot less clear cut as most of the
> > > tests won't depend on such a controlled environment. So sanitizing the
> > > environment would be a good enough first step for me, and if we see
> > > demand for making specific information available to lots of tests we
> > > could still start to expose those at a later point.
> >
> > Fair enough.
> >
> > To put it another way, if you write a test and if it gets affected
> > by externalities, perhaps you are testing a function that is at too
> > high a level that is not a suitable target for unit tested?
>
> I think one problem with this approach is that breakage is likely going
> to depend on the user's environment. So something that works just fine
> for you, the test author, may introduce a hidden dependency that breaks
> for somebody else much later.
>
> Some examples, assuming we just suppress reading Git config:
>
> - Without an explicit ident, we fall back to constructing one from
> system info. So if a unit test ever creates a commit, it will work
> fine for most people, but not for somebody with a blank GECOS field
> in /etc/passwd. (We do look at that field for reflogs, which current
> unit tests already do, but we are more forgiving there since we
> don't pass IDENT_STRICT).
>
> - Other programs we call (e.g., imagine gpg or ssh for commit signing
> or verification) may read their own config based on $HOME,
> $XDG_CONFIG_HOME, etc. I don't know if Patrick was including that in
> "sanitizing the environment" or not.
Oh, yes. I didn't mean to say we shouldn't sanitize at all, I rather
meant to say we should sanitize to values that simply cause us to do a
no-op in the relevant parts. That means we'd:
- Unset a bunch of environment variables where we know that they
impact Git.
- Set config-related environment variables to read configuration
from "/dev/null".
This is in contrast to the more involved fix here, which would be to
populate a temporary home directory with gitconfig files and whatnot.
Patrick
