threads / patch / 64281

patch, 2 partsOrganize mingw includes

Subject: [PATCH 0/2] Organize mingw includes

## tl;dr

9 messages between Oct 9, 2025 and Oct 13, 2025. Diffs are folded; open one to read it.

replies: 8people: 5as markdown or json

Johannes Schindelin via GitGitGadget· Oct 9, 2025, 07:45 UTC · lore

Following in the footsteps of the many, many recent #include refactorings, this patch series orders the #include statements in compat/mingw.c.

Johannes Schindelin (2):
  mingw: avoid relative `#include`s
  mingw: order `#include`s alphabetically
 compat/mingw.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)
base-commit: 79cf913ea9321f774da29b2330b5781d5ff420ef
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1985%2Fdscho%2Forganize-mingw-includes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1985/dscho/organize-mingw-includes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1985
-- 
gitgitgadget
Johannes Schindelin via GitGitGadget· Oct 9, 2025, 07:46 UTC · re: Johannes Schindelin via GitGitGadget · lore

[PATCH 1/2] mingw: avoid relative `#include`s

From: Johannes Schindelin <johannes.schindelin@gmx.de>
We want to make them relative to the top-level directory.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)
Show changes to compat/mingw.c +10 −10
diff --git a/compat/mingw.c b/compat/mingw.c
index 8538e3d172..da99473f56 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1,22 +1,22 @@
 #define USE_THE_REPOSITORY_VARIABLE
 #define DISABLE_SIGN_COMPARE_WARNINGS
 
-#include "../git-compat-util.h"
+#include "git-compat-util.h"
 #include "win32.h"
 #include <aclapi.h>
 #include <sddl.h>
 #include <conio.h>
 #include <wchar.h>
-#include "../strbuf.h"
-#include "../run-command.h"
-#include "../abspath.h"
-#include "../alloc.h"
+#include "strbuf.h"
+#include "run-command.h"
+#include "abspath.h"
+#include "alloc.h"
 #include "win32/lazyload.h"
-#include "../config.h"
-#include "../environment.h"
-#include "../trace2.h"
-#include "../symlinks.h"
-#include "../wrapper.h"
+#include "config.h"
+#include "environment.h"
+#include "trace2.h"
+#include "symlinks.h"
+#include "wrapper.h"
 #include "dir.h"
 #include "gettext.h"
 #define SECURITY_WIN32
-- 
gitgitgadget
Matthias Aßhauer· Oct 11, 2025, 09:03 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH 1/2] mingw: avoid relative `#include`s

On Thu, 9 Oct 2025, Johannes Schindelin via GitGitGadget wrote:
Show 20 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> We want to make them relative to the top-level directory.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> compat/mingw.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 8538e3d172..da99473f56 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1,22 +1,22 @@
> #define USE_THE_REPOSITORY_VARIABLE
> #define DISABLE_SIGN_COMPARE_WARNINGS
>
> -#include "../git-compat-util.h"
> +#include "git-compat-util.h"
> #include "win32.h"

Isn't this include also relative to compat/ and should become #include "compat/win32.h"?

Show 13 quoted lines
> #include <aclapi.h>
> #include <sddl.h>
> #include <conio.h>
> #include <wchar.h>
> -#include "../strbuf.h"
> -#include "../run-command.h"
> -#include "../abspath.h"
> -#include "../alloc.h"
> +#include "strbuf.h"
> +#include "run-command.h"
> +#include "abspath.h"
> +#include "alloc.h"
> #include "win32/lazyload.h"
Same situation here.
best regards
Matthias
Johannes Sixt· Oct 12, 2025, 11:45 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH 1/2] mingw: avoid relative `#include`s

Am 09.10.25 um 09:46 schrieb Johannes Schindelin via GitGitGadget:
Show 46 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> We want to make them relative to the top-level directory.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  compat/mingw.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 8538e3d172..da99473f56 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1,22 +1,22 @@
>  #define USE_THE_REPOSITORY_VARIABLE
>  #define DISABLE_SIGN_COMPARE_WARNINGS
>  
> -#include "../git-compat-util.h"
> +#include "git-compat-util.h"
>  #include "win32.h"
>  #include <aclapi.h>
>  #include <sddl.h>
>  #include <conio.h>
>  #include <wchar.h>
> -#include "../strbuf.h"
> -#include "../run-command.h"
> -#include "../abspath.h"
> -#include "../alloc.h"
> +#include "strbuf.h"
> +#include "run-command.h"
> +#include "abspath.h"
> +#include "alloc.h"
>  #include "win32/lazyload.h"
> -#include "../config.h"
> -#include "../environment.h"
> -#include "../trace2.h"
> -#include "../symlinks.h"
> -#include "../wrapper.h"
> +#include "config.h"
> +#include "environment.h"
> +#include "trace2.h"
> +#include "symlinks.h"
> +#include "wrapper.h"
>  #include "dir.h"
>  #include "gettext.h"
>  #define SECURITY_WIN32
Why is this needed?

With #include "foo" it is quite clear that the file is first looked up from the directory of the file being processed. The changed code requires that the top-level directory is among the -I directives of the command lines. Then it would be much more logical to use #include <foo> instead. But that I wouldn't regard as desirable, either, because the included file isn't from a subordinate module or library.

So, IMO, the status quo is perfect and does not need this change.
-- Hannes
Junio C Hamano· Oct 13, 2025, 16:29 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/2] mingw: avoid relative `#include`s

Johannes Sixt <j6t@kdbg.org> writes:
> Why is this needed?
;-)

As pointed out by Matthias, the changes in the posted patch are not complete/comprehensive.

Show 5 quoted lines
> With #include "foo" it is quite clear that the file is first looked up
> from the directory of the file being processed. The changed code
> requires that the top-level directory is among the -I directives of the
> command lines. Then it would be much more logical to use #include <foo>
> instead.

I actually prefer that, but that is a taste thing I do not want to impose on this project.

> So, IMO, the status quo is perfect and does not need this change.

I tend to agree, but it would be a waste of time to further discuss on this. As long as it does not break compilation, I'll just let the patch graduate.

My preference is to 
 * always name custom headers using the path from the top-level (we
   use -I in BASIC_CFLAGS exactly for this purpose), 
   e.g.
	#include "compat/win32.h" (good)
	#include "win32.h" (not good)
 * compat header that aims to replace system supplied headers like
   <regex.h> should use -I appropriately and appear as if they are
   from the system, e.g.
	#include <regex.h> (good)
	#include "compat/regex/regex.h" (not good)

If somebody truly wants to improve things once the dust settles from these patches, I would appreciate they keep the above in mind.

THanks.
Johannes Schindelin via GitGitGadget· Oct 9, 2025, 07:46 UTC · re: Johannes Schindelin via GitGitGadget · lore

[PATCH 2/2] mingw: order `#include`s alphabetically

From: Johannes Schindelin <johannes.schindelin@gmx.de>
It allows for more consistent patches that way.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)
Show changes to compat/mingw.c +11 −11
diff --git a/compat/mingw.c b/compat/mingw.c
index da99473f56..736a07a028 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2,25 +2,25 @@
 #define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "git-compat-util.h"
-#include "win32.h"
-#include <aclapi.h>
-#include <sddl.h>
-#include <conio.h>
-#include <wchar.h>
-#include "strbuf.h"
-#include "run-command.h"
 #include "abspath.h"
 #include "alloc.h"
-#include "win32/lazyload.h"
 #include "config.h"
+#include "dir.h"
 #include "environment.h"
-#include "trace2.h"
+#include "gettext.h"
+#include "run-command.h"
+#include "strbuf.h"
 #include "symlinks.h"
+#include "trace2.h"
+#include "win32.h"
+#include "win32/lazyload.h"
 #include "wrapper.h"
-#include "dir.h"
-#include "gettext.h"
+#include <aclapi.h>
+#include <conio.h>
+#include <sddl.h>
 #define SECURITY_WIN32
 #include <sspi.h>
+#include <wchar.h>
 #include <winternl.h>
 
 #define STATUS_DELETE_PENDING ((NTSTATUS) 0xC0000056)
-- 
gitgitgadget
Patrick Steinhardt· Oct 10, 2025, 09:53 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH 0/2] Organize mingw includes

On Thu, Oct 09, 2025 at 07:45:59AM +0000, Johannes Schindelin via GitGitGadget wrote:
> Following in the footsteps of the many, many recent #include refactorings,
> this patch series orders the #include statements in compat/mingw.c.

Both of these patches look good to me and I like the improved consistency that they bring. I may also do the same for our code in "refs/", where some of the files use relative includes, as well. Might be worth to document this somewhere if it isn't already, but that doesn't have to be part of your patch series here.

Sorting them also makes sense. It's another thing where I wish that we had a tool to enforce this. clang-format supports this in theory, but it's disabled right now. And I'm not even sure whether it can be told to include e.g. "git-compat-util.h" first.

Thanks!
Patrick
Johannes Schindelin· Oct 10, 2025, 13:55 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 0/2] Organize mingw includes

Hi Patrick,
On Fri, 10 Oct 2025, Patrick Steinhardt wrote:
Show 9 quoted lines
> On Thu, Oct 09, 2025 at 07:45:59AM +0000, Johannes Schindelin via GitGitGadget wrote:
> > Following in the footsteps of the many, many recent #include refactorings,
> > this patch series orders the #include statements in compat/mingw.c.
> 
> Both of these patches look good to me and I like the improved
> consistency that they bring. I may also do the same for our code in
> "refs/", where some of the files use relative includes, as well. Might
> be worth to document this somewhere if it isn't already, but that
> doesn't have to be part of your patch series here.

There's already such a lot of documentation that I fear it is intimidating to any new contributor (and if I was one, I'd be tempted to read it exclusively via AI summaries). I don't want to add to that amount.

> Sorting them also makes sense. It's another thing where I wish that we
> had a tool to enforce this. clang-format supports this in theory, but
> it's disabled right now. And I'm not even sure whether it can be told to
> include e.g. "git-compat-util.h" first.

In theory, I am totally with you: Sorting `#include`s is a job best left to tools. But then, I say the same about formatting, and I see a lot of appetite on this list for preventing that to become a tool-driven job, and instead requiring developer time to be spent on it. Therefore, I don't want to spend any effort trying to get this automated when I see small chances of success in the endeavor of freeing up cognitive cycles for tasks I find more intellectually rewarding than figuring out where a space or a line break goes.

Ciao, Johannes

Junio C Hamano· Oct 10, 2025, 16:18 UTC · re: Johannes Schindelin · lore

Re: [PATCH 0/2] Organize mingw includes

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 7 quoted lines
>> Sorting them also makes sense. It's another thing where I wish that we
>> had a tool to enforce this. clang-format supports this in theory, but
>> it's disabled right now. And I'm not even sure whether it can be told to
>> include e.g. "git-compat-util.h" first.
>
> In theory, I am totally with you: Sorting `#include`s is a job best left
> to tools. But then, I say the same about formatting,

I am afraid that it is apples-to-oranges comparison. Nobody has to read the #include directives; they may have to see if a header they care about (because they are planning to add a call to a function that hasn't been used in the particular C source file, perhaps) is already included, and sorted list of includes is a good tool to help them. IOW, they do not read them, they scan in them. But the code, the result of formatting, must be readable by people.

← back to recent threads