On Mon, Nov 30, 2020 at 4:20 AM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
> LGTM. FWIW I think your v1 is fine too, just meant to comment on the
> basis of "you could also do it like that". Having a C program call
> getuid() is fine, so is faking it. If you prefer the latter, cool.
I do like the simplicity of the latter, and I wasn't super happy about
introducing a new test-tool subcommand just to make this one test
pass, especially since `test-tool getuid` is so different from most
other subcommands which are typically added to give us access to some
internal element of Git otherwise not accessible to the test scripts.
> I did wonder "why not just call perl -e 'print $<' ?" first. But then
> found (by reading the Perl source[1], didn't actually test this) that it
> fakes up UID=0 for everything on Windows.
I totally forgot about Perl's `$<`.
Under the Git for Windows SDK, Perl's `$<` returns a large positive
number. I suspect this differs from what you saw in the Perl source
code because the Windows-specific code you looked at does not come
into play in this case. For Git for Windows SDK, Perl is almost
certainly instead built in Unix-like mode, linking against the MSYS2
library for its POSIX-like emulation, thus the Windows-specific Perl
goop is not needed.
> I couldn't find any "is root?" in our tree that relies on Perl's $< in a
> bad way (the couuple of occurances seem innocuous), we have some "id -u"
> checks, but those also seem OK if it returned 0 on Windows (what does it
> return?). Seems the worst we'd do there is unintentionally skip some
> "skip this as root" tests.
Under Git for Windows SDK, `id -u` returns the same large positive
number as returned by Perl's `$<`, which makes sense since `id` is
also likely linked against the MSYS2 library.
As for getuid() in Git itself, that always returns 1. I see now that's
because 1 is hard-coded in Git's compat/mingw.h override of getuid().
So, an alternative would have been for the test to use `uid=1` on
MINGW, but I like the current approach better of having the test be
UID-agnostic.
> It's also my impression that just using $("$PERL_PATH" -e ...) is fine,
> and at least to my reading the Perl RX is more readable with look-behind
> assertions, but I'm biased by familiarity with it.
The `sed` version seems simpler and more straightforward to me,
whereas look-behind feels harder to reason about, but of course it's
all subjective and not terribly important. Either would be fine.
> Our PERL prereq & NO_PERL=YesPlease is just for "this may require a
> non-ancient Perl" & "don't install Perl for runtime stuff"
> respectively. Is that not the case and we'd like to avoid new perl
> invocations where possible?
>
> I don't really care either way (or, if your switch in this case was just
> a personal preference, also fine), but if we are trying to somewhat
> discourage perl use (and maybe eventually get rid of it entirely) that
> would be a useful t/README doc update.
Using `sed` was just a personal preference, partly because the `sed`
expression seems simpler to me, but mostly because Perl still feels
heavyweight to me compared to `sed`. It also may be that I'm just
old-school, preferring the small, sharp utilities (`sed`, `grep`,
`sort`, etc.), and only turning to Perl (or one of its ilk) when the
task demands a more general-purpose tool.
I haven't measured, but it's possible that Perl may indeed be
heavyweight on Windows in terms of startup time.
> I know Johannes (CC'd) has (this is from wetware memory) wanted to
> (understandably) not need to bother with Perl as part of GFW, but I
> can't remember if that's for a reason covered by NO_PERL=YesPlease,
> i.e. packaging it up, or whether it's also to not wanting to provide a
> perl for the test suite.
I _think_ that was for the NO_PERL=YesPlease case, but I expect Dscho
can answer more concretely.
