[v2,1/3] ident: stop assuming that `gw_gecos` is writable

Commit Message

Johannes Schindelin March 6, 2025, 10:26 a.m. UTC

From: Johannes Schindelin <[e-mail]>

In 590e081dea7c (ident: add NO_GECOS_IN_PWENT for systems without
pw_gecos in struct passwd, 2011-05-19), code was introduced to iterate
over the `gw_gecos` field; The loop variable is of type `char *`, which
assumes that `gw_gecos` is writable.

However, it is not necessarily writable (and it is a bad idea to have it
writable in the first place), so let's switch the loop variable type to
`const char *`.

This is not a new problem, but what is new is the Meson build. While it
does not trigger in CI builds, imitating the commands of
`ci/run-build-and-tests.sh` in a regular Git for Windows SDK (`meson
setup build . --fatal-meson-warnings --warnlevel 2 --werror --wrap-mode
nofallback -Dfuzzers=true` followed by `meson compile -C build --`
results in this beautiful error:

  "cc" [...] -o libgit.a.p/ident.c.obj "-c" ../ident.c
  ../ident.c: In function 'copy_gecos':
  ../ident.c:68:18: error: assignment discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
     68 |         for (src = get_gecos(w); *src && *src != ','; src++) {
        |                  ^
  cc1.exe: all warnings being treated as errors

Now, why does this not trigger in CI? The answer is as simple as it is
puzzling: The `win+Meson` job completely side-steps Git for Windows'
development environment, opting instead to use the GCC that is on the
`PATH` in GitHub-hosted `windows-latest` runners. That GCC is pinned to
v12.2.0 and targets the UCRT (unlikely to change any time soon, see
https://github.com/actions/runner-images/blob/win25/20250303.1/images/windows/toolsets/toolset-2022.json#L132-L141).
That is in stark contrast to Git for Windows, which uses GCC v14.2.0 and
targets MSVCRT. Git for Windows' `Makefile`-based build also obviously
uses different compiler flags, otherwise this compile error would have
had plenty of opportunity in almost 14 years to surface.

In other words, contrary to my expectations, the `win+Meson` job is
ill-equipped to replace the `win build` job because it exercises a
completely different tool version/compiler flags vector than what Git
for Windows needs.

Nevertheless, there is currently this huge push, including breaking
changes after -rc1 and all, for switching to Meson. Therefore, we need
to make it work, somehow, even in Git for Windows' SDK, hence this
patch, at this point in time.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 ident.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Patrick Steinhardt March 6, 2025, 10:50 a.m. UTC | #1

On Thu, Mar 06, 2025 at 10:26:18AM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[e-mail]>
> Now, why does this not trigger in CI? The answer is as simple as it is
> puzzling: The `win+Meson` job completely side-steps Git for Windows'
> development environment, opting instead to use the GCC that is on the
> `PATH` in GitHub-hosted `windows-latest` runners. That GCC is pinned to
> v12.2.0 and targets the UCRT (unlikely to change any time soon, see
> https://github.com/actions/runner-images/blob/win25/20250303.1/images/windows/toolsets/toolset-2022.json#L132-L141).
> That is in stark contrast to Git for Windows, which uses GCC v14.2.0 and
> targets MSVCRT. Git for Windows' `Makefile`-based build also obviously
> uses different compiler flags, otherwise this compile error would have
> had plenty of opportunity in almost 14 years to surface.

Oh, interesting. I didn't even know that the Windows runners had GCC in
their PATH, and thus I didn't expect it to use that compiler at all. On
GitLab for example we can see that it uses the MSVC compiler as I did
expect [1]:

    Activating VS 17.10.2
    C compiler for the host machine: cl (msvc 19.40.33811 "Microsoft (R) C/C++ Optimizing Compiler Version 19.40.33811 for x64")
    C linker for the host machine: link link 14.40.33811.0

But you're right, on GitHub that's not the case:

    C compiler for the host machine: gcc (gcc 12.2.0 "gcc (x86_64-posix-seh-rev2, Built by MinGW-W64 project) 12.2.0")
    C linker for the host machine: gcc ld.bfd 2.39

We can easily fix that by passing the `--vsenv` flag to Meson. I'll send
a patch soonish.

Patrick

[1]: https://gitlab.com/gitlab-org/git/-/jobs/9324989037#L95
[2]: https://github.com/git/git/actions/runs/13686408338/job/38270746786#step:5:15

Johannes Schindelin March 6, 2025, 12:26 p.m. UTC | #2

Hi Patrick,

On Thu, 6 Mar 2025, Patrick Steinhardt wrote:

> On Thu, Mar 06, 2025 at 10:26:18AM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <[e-mail]>
> > Now, why does this not trigger in CI? The answer is as simple as it is
> > puzzling: The `win+Meson` job completely side-steps Git for Windows'
> > development environment, opting instead to use the GCC that is on the
> > `PATH` in GitHub-hosted `windows-latest` runners. That GCC is pinned to
> > v12.2.0 and targets the UCRT (unlikely to change any time soon, see
> > https://github.com/actions/runner-images/blob/win25/20250303.1/images/windows/toolsets/toolset-2022.json#L132-L141).
> > That is in stark contrast to Git for Windows, which uses GCC v14.2.0 and
> > targets MSVCRT. Git for Windows' `Makefile`-based build also obviously
> > uses different compiler flags, otherwise this compile error would have
> > had plenty of opportunity in almost 14 years to surface.
>
> Oh, interesting. I didn't even know that the Windows runners had GCC in
> their PATH, and thus I didn't expect it to use that compiler at all. On
> GitLab for example we can see that it uses the MSVC compiler as I did
> expect [1]:
>
>     Activating VS 17.10.2
>     C compiler for the host machine: cl (msvc 19.40.33811 "Microsoft (R) C/C++ Optimizing Compiler Version 19.40.33811 for x64")
>     C linker for the host machine: link link 14.40.33811.0
>
> But you're right, on GitHub that's not the case:
>
>     C compiler for the host machine: gcc (gcc 12.2.0 "gcc (x86_64-posix-seh-rev2, Built by MinGW-W64 project) 12.2.0")
>     C linker for the host machine: gcc ld.bfd 2.39
>
> We can easily fix that by passing the `--vsenv` flag to Meson. I'll send
> a patch soonish.
>
> Patrick
>
> [1]: https://gitlab.com/gitlab-org/git/-/jobs/9324989037#L95
> [2]: https://github.com/git/git/actions/runs/13686408338/job/38270746786#step:5:15

Please do not invest more time on the Visual Studio support via Meson. No
contributor will use this, and I want to stop spending my time on this.
The user experience of configuring a Visual Studio build via Meson is just
too weak compared to the ease of CMake-based builds, and while not many
Visual Studio users are familiar with CMake, even dramatically less will
even so much as know about Meson.

I plan on dropping all pretense that Git supports Visual Studio-based
contributions soon after v2.49.0 comes out, e.g. by deleting the CMake
definition and also deleting whatever Meson-specific stuff I can get away
deleting in Git for Windows. It was not worth the time I invested.

Ciao,
Johannes

Junio C Hamano March 6, 2025, 4:33 p.m. UTC | #3

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> In other words, contrary to my expectations, the `win+Meson` job is
> ill-equipped to replace the `win build` job because it exercises a
> completely different tool version/compiler flags vector than what Git
> for Windows needs.

It is apparent that meson support is a new procedure to build our
codebase that is untested and unproven on Windows at all, given that
among all people who may have stake in Windows you are discovering
problems in it this late in the cycle.  Nobody knows what other
breakages, other than something obvious and easy to catch like "ah,
compiler refuses to go further", are lurking under the radar.

I would be reluctant to trust the build artifact out of meson-based
build on Windows after seeing your report, especially the above
part.

A reasonable alternative may be to declare that meson-based build is
not ready yet at this point, and possibly disable win+Meson jobs to
punt and divert our engineering resources elsewhere in the meantime.
For a new thing, having an uneven support depending on the platform
early in the evolution is not unusual or to be ashamed of.

> Nevertheless, there is currently this huge push, including breaking
> changes after -rc1 and all, for switching to Meson. Therefore, we need
> to make it work, somehow, even in Git for Windows' SDK, hence this
> patch, at this point in time.

As I said earlier already, I do not mind turning the type of this
pointer, which is only used to read from a struct member, like this
patch does.  It is the right thing to do, so I'll apply.

But I personally would not be comfortable with the product built
with "completely different tool version/compiler flags vector than
what G4W needs", even the compilation passes with just this small
change.  If I were using Windows, that is.

> Signed-off-by: Johannes Schindelin <[e-mail]>
> ---
>  ident.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, will apply.

> diff --git a/ident.c b/ident.c
> index caf41fb2a98..967895d8850 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -59,7 +59,7 @@ static struct passwd *xgetpwuid_self(int *is_bogus)
>  
>  static void copy_gecos(const struct passwd *w, struct strbuf *name)
>  {
> -	char *src;
> +	const char *src;
>  
>  	/* Traditionally GECOS field had office phone numbers etc, separated
>  	 * with commas.  Also & stands for capitalized form of the login name.

Patch

diff --git a/ident.c b/ident.c
index caf41fb2a98..967895d8850 100644
--- a/ident.c
+++ b/ident.c
@@ -59,7 +59,7 @@  static struct passwd *xgetpwuid_self(int *is_bogus)
 
 static void copy_gecos(const struct passwd *w, struct strbuf *name)
 {
-	char *src;
+	const char *src;
 
 	/* Traditionally GECOS field had office phone numbers etc, separated
 	 * with commas.  Also & stands for capitalized form of the login name.