Re: [PATCH 1/2] mingw: remove duplicate `USE_NED_ALLOCATOR` directive

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 18, 2022, 01:54 UTC
Message-ID: <xmqq1qoxzdam.fsf@gitster.g>
In-Reply-To: <bc79dfcc4d44e0f006dc64d75a2c7f8a11834229.1670274213.git.gitgitgadget@gmail.com>

"Harshil-Jani via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

> From: Harshil-Jani <harshiljani2002@gmail.com>
>
> nedalloc was added to fix the slowness of memory allocator. Here
> specifically for the MSys2 build there seems to be a duplication of
> USE_NED_ALLOCATOR directive.

Yes, if your platform's "uname -S" says "MINGW", your build is done in a directory with ../THIS_IS_MSYSGIT marker file, and your "uname -R" does not begin with "1.", then there are two USE_NED_ALLOCATOR=YesPlease in effect.

> So this patch intends to remove the
> duplicate USE_NED_ALLOCATOR and keeping it only into the MSys2 config
> section so it still uses the nedalloc.

What about other folks whose "uname -S" says "MINGW"?

Show 17 quoted lines

> Signed-off-by: Harshil-Jani <harshiljani2002@gmail.com>
> ---
>  config.mak.uname | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/config.mak.uname b/config.mak.uname
> index d63629fe807..377667c4bbc 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -652,7 +652,6 @@ ifeq ($(uname_S),MINGW)
>  	USE_WIN32_IPC = YesPlease
>  	USE_WIN32_MMAP = YesPlease
>  	MMAP_PREVENTS_DELETE = UnfortunatelyYes
> -	USE_NED_ALLOCATOR = YesPlease
>  	UNRELIABLE_FSTAT = UnfortunatelyYes
>  	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
>  	NO_REGEX = YesPlease

The original in a wider context looks like this:

        ifeq ($(uname_S),MINGW)
                ...
                MMAP_PREVENTS_DELETE = UnfortunatelyYes
                USE_NED_ALLOCATOR = YesPlease
                UNRELIABLE_FSTAT = UnfortunatelyYes
                ...
                X = .exe
        ifneq (,$(wildcard ../THIS_IS_MSYSGIT))
                htmldir = doc/git/html/
                ...
                COMPAT_CFLAGS += -D__USE_MINGW_ACCESS
        else
                ifneq ($(shell expr "$(uname_R)" : '1\.'),2)
                        # MSys2
                        ...
                        USE_LIBPCRE = YesPlease
                        USE_NED_ALLOCATOR = YesPlease
                        ifeq (/mingw64,$(subst 32,64,$(prefix)))
                                # Move system config into top-level /etc/
                                ...
                        endif
                else
                        ...
                endif
        endif
        endif

With this patch, a build that has ../THIS_IS_MSYSGIT marker file, or whose "uname -R" output begins with "1.", will no longer get USE_NED_ALLOCATOR. Intended?

Without knowing much about the Windows/MSYS/Git for Windows SDK ecosystem, it is the inner one that looks duplicated, but the patch is removing the outer one that helps every platform whose "uname -S" identifies it as MINGW.

Perhaps that is what the patch meant to do, but then the proposed log message explains it very differently. It only talks about the no-op case. It does not explain why other folks whose "uname -S" says MINGW (1) are not broken if this patch suddenly robs NED allocator from them, and (2) are better off without using NED allocator.