Organize mingw includes #1985
Open
Organize mingw includes #1985
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We want to make them relative to the top-level directory. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
It allows for more consistent patches that way. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
/submit |
|
Submitted as pull.1985.git.1759995961.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): 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 |
|
User |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Patrick,
On Fri, 10 Oct 2025, Patrick Steinhardt wrote:
> 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 |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 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. |
|
This patch series was integrated into seen via git@4bf6293. |
| @@ -1,22 +1,22 @@ | |||
| #define USE_THE_REPOSITORY_VARIABLE | |||
There was a problem hiding this comment.
On the Git mailing list, Matthias Aßhauer wrote (reply to this):
On Thu, 9 Oct 2025, Johannes Schindelin via GitGitGadget wrote:
> 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"?
> #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|
User |
|
This branch is now known as |
| @@ -1,22 +1,22 @@ | |||
| #define USE_THE_REPOSITORY_VARIABLE | |||
There was a problem hiding this comment.
On the Git mailing list, Johannes Sixt wrote (reply to this):
Am 09.10.25 um 09:46 schrieb Johannes Schindelin via GitGitGadget:
> 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
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
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.
> 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.|
User |
|
This patch series was integrated into seen via git@c0cccf7. |
|
This patch series was integrated into next via git@b7144c1. |
|
This patch series was integrated into seen via git@db47a71. |
|
This patch series was integrated into seen via git@cdf376c. |
|
This patch series was integrated into seen via git@37368fa. |
|
There was a status update in the "Cooking" section about the branch Code clean-up. Will merge to 'master'. source: <pull.1985.git.1759995961.gitgitgadget@gmail.com> |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following in the footsteps of the many, many recent
#includerefactorings, this patch series orders the#includestatements incompat/mingw.c.cc: Patrick Steinhardt ps@pks.im
cc: Matthias Aßhauer mha1993@live.de
cc: Johannes Sixt j6t@kdbg.org