Re: [PATCH v2 0/2] make macOS `git maintenance` test work on Windows
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Dec 1, 2020, 03:58 UTC
- Message-ID
- <CAPig+cT-axeG3UrxaQR3c+r7dMZkjmS19C3cHRVzu5KSc72BOQ@mail.gmail.com>
- In-Reply-To
- <87blffjkne.fsf@evledraar.gmail.com>
On Mon, Nov 30, 2020 at 4:20 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.
Show 5 quoted lines
> 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.
Show 9 quoted lines
> 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.
Show 5 quoted lines
> 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.