threads / patch / 59829

patchstatinfo.h: move DTYPE defines from dir.h

Subject: [PATCH] statinfo.h: move DTYPE defines from dir.h

## tl;dr

12 messages between Jun 2, 2023 and Jun 12, 2023. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Aleajndro R Sedeño· Jun 2, 2023, 18:45 UTC · lore

From: Alejandro R. Sedeño <asedeno@mit.edu>

These definitions are used in cache.h, which can't include dir.h without causing name-info.cc to have two definitions of `struct dir_entry`.

Both dir.h and cache.h include statinfo.h, and this seems a reasonable place for these definitions.

This change fixes a broken build issue on old SunOS.

Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
Signed-off-by: Alejandro R Sedeño <asedeno@google.com>
---
 dir.h      | 14 --------------
 statinfo.h | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 14 deletions(-)

Show changes to 2 files +14 −14

dir.h, statinfo.h

diff --git a/dir.h b/dir.h
index 79b85a01ee..d65a40126c 100644
--- a/dir.h
+++ b/dir.h
@@ -641,18 +641,4 @@ static inline int starts_with_dot_dot_slash_native(const char *const path)
 	return path_match_flags(path, what | PATH_MATCH_NATIVE);
 }
 
-#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
-#define DTYPE(de)	((de)->d_type)
-#else
-#undef DT_UNKNOWN
-#undef DT_DIR
-#undef DT_REG
-#undef DT_LNK
-#define DT_UNKNOWN	0
-#define DT_DIR		1
-#define DT_REG		2
-#define DT_LNK		3
-#define DTYPE(de)	DT_UNKNOWN
-#endif
-
 #endif
diff --git a/statinfo.h b/statinfo.h
index e49e3054ea..fe8df633a4 100644
--- a/statinfo.h
+++ b/statinfo.h
@@ -21,4 +21,18 @@ struct stat_data {
 	unsigned int sd_size;
 };
 
+#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
+#define DTYPE(de)	((de)->d_type)
+#else
+#undef DT_UNKNOWN
+#undef DT_DIR
+#undef DT_REG
+#undef DT_LNK
+#define DT_UNKNOWN	0
+#define DT_DIR		1
+#define DT_REG		2
+#define DT_LNK		3
+#define DTYPE(de)	DT_UNKNOWN
+#endif
+
 #endif

-- 
2.41.0.rc2.161.g9c6817b8e7-goog

Alejandro Sedeño· Jun 2, 2023, 18:50 UTC · re: Aleajndro R Sedeño · lore

Re: [PATCH] statinfo.h: move DTYPE defines from dir.h

And today is the day I notice I misspelled my name in my git sendmail config at work. Cool. (Fixed.)

-Alejandro

On Fri, Jun 2, 2023 at 2:46 PM Aleajndro R Sedeño <asedeno@google.com> wrote:

Show 68 quoted lines

>
> From: Alejandro R. Sedeño <asedeno@mit.edu>
>
> These definitions are used in cache.h, which can't include dir.h
> without causing name-info.cc to have two definitions of
> `struct dir_entry`.
>
> Both dir.h and cache.h include statinfo.h, and this seems a reasonable
> place for these definitions.
>
> This change fixes a broken build issue on old SunOS.
>
> Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
> Signed-off-by: Alejandro R Sedeño <asedeno@google.com>
> ---
>  dir.h      | 14 --------------
>  statinfo.h | 14 ++++++++++++++
>  2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/dir.h b/dir.h
> index 79b85a01ee..d65a40126c 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -641,18 +641,4 @@ static inline int starts_with_dot_dot_slash_native(const char *const path)
>         return path_match_flags(path, what | PATH_MATCH_NATIVE);
>  }
>
> -#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
> -#define DTYPE(de)      ((de)->d_type)
> -#else
> -#undef DT_UNKNOWN
> -#undef DT_DIR
> -#undef DT_REG
> -#undef DT_LNK
> -#define DT_UNKNOWN     0
> -#define DT_DIR         1
> -#define DT_REG         2
> -#define DT_LNK         3
> -#define DTYPE(de)      DT_UNKNOWN
> -#endif
> -
>  #endif
> diff --git a/statinfo.h b/statinfo.h
> index e49e3054ea..fe8df633a4 100644
> --- a/statinfo.h
> +++ b/statinfo.h
> @@ -21,4 +21,18 @@ struct stat_data {
>         unsigned int sd_size;
>  };
>
> +#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
> +#define DTYPE(de)      ((de)->d_type)
> +#else
> +#undef DT_UNKNOWN
> +#undef DT_DIR
> +#undef DT_REG
> +#undef DT_LNK
> +#define DT_UNKNOWN     0
> +#define DT_DIR         1
> +#define DT_REG         2
> +#define DT_LNK         3
> +#define DTYPE(de)      DT_UNKNOWN
> +#endif
> +
>  #endif
> --
> 2.41.0.rc2.161.g9c6817b8e7-goog
>

Eric Sunshine· Jun 2, 2023, 19:06 UTC · re: Aleajndro R Sedeño · lore

Re: [PATCH] statinfo.h: move DTYPE defines from dir.h

On Fri, Jun 2, 2023 at 3:03 PM Aleajndro R Sedeño <asedeno@google.com> wrote:

> These definitions are used in cache.h, which can't include dir.h
> without causing name-info.cc to have two definitions of
> `struct dir_entry`.

What is `name-info.cc`?

Show 7 quoted lines

> Both dir.h and cache.h include statinfo.h, and this seems a reasonable
> place for these definitions.
>
> This change fixes a broken build issue on old SunOS.
>
> Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
> Signed-off-by: Alejandro R Sedeño <asedeno@google.com>

Alejandro Sedeño· Jun 2, 2023, 19:21 UTC · re: Eric Sunshine · lore

Re: [PATCH] statinfo.h: move DTYPE defines from dir.h

That is a valid question, and it's another typo. I meant name-hash.c.

-Alejandro

On Fri, Jun 2, 2023 at 3:06 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 15 quoted lines

>
> On Fri, Jun 2, 2023 at 3:03 PM Aleajndro R Sedeño <asedeno@google.com> wrote:
> > These definitions are used in cache.h, which can't include dir.h
> > without causing name-info.cc to have two definitions of
> > `struct dir_entry`.
>
> What is `name-info.cc`?
>
> > Both dir.h and cache.h include statinfo.h, and this seems a reasonable
> > place for these definitions.
> >
> > This change fixes a broken build issue on old SunOS.
> >
> > Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
> > Signed-off-by: Alejandro R Sedeño <asedeno@google.com>

Alejandro R Sedeño· Jun 2, 2023, 19:27 UTC · re: Alejandro Sedeño · lore

From: Alejandro R. Sedeño <asedeno@mit.edu>

These definitions are used in cache.h, which can't include dir.h without causing name-hash.c to have two definitions of `struct dir_entry`.

Both dir.h and cache.h include statinfo.h, and this seems a reasonable place for these definitions.

This change fixes a broken build issue on old SunOS.

Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
Signed-off-by: Alejandro R Sedeño <asedeno@google.com>
---
 dir.h      | 14 --------------
 statinfo.h | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 14 deletions(-)

Show changes to 2 files +14 −14

dir.h, statinfo.h

diff --git a/dir.h b/dir.h
index 79b85a01ee..d65a40126c 100644
--- a/dir.h
+++ b/dir.h
@@ -641,18 +641,4 @@ static inline int starts_with_dot_dot_slash_native(const char *const path)
 	return path_match_flags(path, what | PATH_MATCH_NATIVE);
 }
 
-#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
-#define DTYPE(de)	((de)->d_type)
-#else
-#undef DT_UNKNOWN
-#undef DT_DIR
-#undef DT_REG
-#undef DT_LNK
-#define DT_UNKNOWN	0
-#define DT_DIR		1
-#define DT_REG		2
-#define DT_LNK		3
-#define DTYPE(de)	DT_UNKNOWN
-#endif
-
 #endif
diff --git a/statinfo.h b/statinfo.h
index e49e3054ea..fe8df633a4 100644
--- a/statinfo.h
+++ b/statinfo.h
@@ -21,4 +21,18 @@ struct stat_data {
 	unsigned int sd_size;
 };
 
+#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
+#define DTYPE(de)	((de)->d_type)
+#else
+#undef DT_UNKNOWN
+#undef DT_DIR
+#undef DT_REG
+#undef DT_LNK
+#define DT_UNKNOWN	0
+#define DT_DIR		1
+#define DT_REG		2
+#define DT_LNK		3
+#define DTYPE(de)	DT_UNKNOWN
+#endif
+
 #endif

-- 
2.41.0.rc2.161.g9c6817b8e7-goog

Elijah Newren· Jun 3, 2023, 01:47 UTC · re: Alejandro R Sedeño · lore

Re: [PATCH] statinfo.h: move DTYPE defines from dir.h

On Fri, Jun 2, 2023 at 12:27 PM Alejandro R Sedeño <asedeno@google.com> wrote:

Show 6 quoted lines

>
> From: Alejandro R. Sedeño <asedeno@mit.edu>
>
> These definitions are used in cache.h, which can't include dir.h
> without causing name-hash.c to have two definitions of
> `struct dir_entry`.

...are _currently_ used in cache.h (your commit message is fine, just pointing it out for below...)

> Both dir.h and cache.h include statinfo.h, and this seems a reasonable
> place for these definitions.
>
> This change fixes a broken build issue on old SunOS.

Maintainer note for Junio: en/header-split-cache-h-part-3 moves the inline functions in cache.h that use the DT_* defines, but that series should both textually and semantically merge cleanly with this change. (Just noting this for your peace of mind.)

After en/header-split-cache-h-part-3 merges down, I might opt for a different fix (I'm still mulling it over), but that other fix isn't possible until cache.h is split up more. Alejandro's fix is the cleanest interim solution I can think of.

Reviewed-by: Elijah Newren <newren@gmail.com>

Junio C Hamano· Jun 3, 2023, 01:56 UTC · re: Alejandro R Sedeño · lore

Re: [PATCH] statinfo.h: move DTYPE defines from dir.h

"Alejandro R Sedeño" <asedeno@google.com> writes:

Show 13 quoted lines

> From: Alejandro R. Sedeño <asedeno@mit.edu>
>
> These definitions are used in cache.h, which can't include dir.h
> without causing name-hash.c to have two definitions of
> `struct dir_entry`.
>
> Both dir.h and cache.h include statinfo.h, and this seems a reasonable
> place for these definitions.
>
> This change fixes a broken build issue on old SunOS.
>
> Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
> Signed-off-by: Alejandro R Sedeño <asedeno@google.com>

This is a bit unusual; do you want to publish both names (I am assuming that they are the same single person)?

I thought somebody in the earlier discussion identified the topic that was problematic by bisecting. It is a shame to lose that. Perhaps it is a good idea to rephrase the beginning of the proposed commit log message to mention that, like

    592fc5b3 (dir.h: move DTYPE defines from cache.h, 2023-04-22)
    moved DTYPE macros from cache.h to dir.h, but are still used
    by cache.h to implement ce_to_dtype(); but cache.h cannot
    include dir.h because ...

or something?

Why does name-hash.c end up with two definitions? Aren't we properly guarding against multiple inclusions with

    #ifndef __DIR_H__
    #define __DIR_H__
	...
    struct dir_entry {
	...
    };
    #endif

or is there something funny going on?

Thanks.

Elijah Newren· Jun 3, 2023, 02:04 UTC · re: Junio C Hamano · lore

Re: [PATCH] statinfo.h: move DTYPE defines from dir.h

On Fri, Jun 2, 2023 at 6:56 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

>
> Why does name-hash.c end up with two definitions?  Aren't we
> properly guarding against multiple inclusions with
>
>     #ifndef __DIR_H__
>     #define __DIR_H__
>         ...
>     struct dir_entry {
>         ...
>     };
>     #endif
>
> or is there something funny going on?

There are two _different_ things named "struct dir_entry" in the codebase:

dir.h:struct dir_entry { dir.h- unsigned int len; dir.h- char name[FLEX_ARRAY]; /* more */ dir.h-}; -- name-hash.c:struct dir_entry { name-hash.c- struct hashmap_entry ent; name-hash.c- struct dir_entry *parent; name-hash.c- int nr; name-hash.c- unsigned int namelen; name-hash.c- char name[FLEX_ARRAY]; name-hash.c-};

So, name-hash.c cannot include anything that includes dir.h.

Junio C Hamano· Jun 3, 2023, 02:30 UTC · re: Elijah Newren · lore

Re: [PATCH] statinfo.h: move DTYPE defines from dir.h

Elijah Newren <newren@gmail.com> writes:

> There are two _different_ things named "struct dir_entry" in the codebase:

In the longer term, we should rename such a local type to avoid name clashes with the global one. But I of course am OK to leave it outside the topic to clean it up.

In any case, it is worth saying in the proposed log message why name-hash cannot use cache.h if we make it include dir.h; it is easy to do so (i.e. "it has its own 'dir_entry' that is used for other purpose").

Thanks.

Alejandro Sedeño· Jun 3, 2023, 03:02 UTC · re: Junio C Hamano · lore

Re: [PATCH] statinfo.h: move DTYPE defines from dir.h

Sorry for the dupes; resending as plain-text as per list requirements.

On Fri, Jun 2, 2023 at 9:56 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

>
> "Alejandro R Sedeño" <asedeno@google.com> writes:
>
> > …
> > Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
> > Signed-off-by: Alejandro R Sedeño <asedeno@google.com>

They are both me; it's my way of sticking with my historical personal address and still attaching my work address for work reasons.

Show 6 quoted lines

>
> This is a bit unusual; do you want to publish both names (I am
> assuming that they are the same single person)?
>
> I thought somebody in the earlier discussion identified the topic
> that was problematic by bisecting.  It is a shame to lose that.

I identified it earlier, by inspection because the machine I build on is slow and this was easy to track down.

Show 9 quoted lines

> Perhaps it is a good idea to rephrase the beginning of the proposed
> commit log message to mention that, like
>
>     592fc5b3 (dir.h: move DTYPE defines from cache.h, 2023-04-22)
>     moved DTYPE macros from cache.h to dir.h, but are still used
>     by cache.h to implement ce_to_dtype(); but cache.h cannot
>     include dir.h because ...
>
> or something?

Happy to rephrase.

-Alejandro

Alejandro R Sedeño· Jun 6, 2023, 20:59 UTC · re: Junio C Hamano · lore

From: Alejandro R. Sedeño <asedeno@mit.edu>

592fc5b3 (dir.h: move DTYPE defines from cache.h, 2023-04-22) moved DTYPE macros from cache.h to dir.h, but they are still used by cache.h to implement ce_to_dtype(); cache.h cannot include dir.h because that would cause name-hash.c to have two different and conflicting definitions of `struct dir_entry`. (That should be separately fixed.)

Both dir.h and cache.h include statinfo.h, and this seems a reasonable place for these definitions.

This change fixes a broken build issue on old SunOS.

Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
Signed-off-by: Alejandro R Sedeño <asedeno@google.com>
---
 dir.h      | 14 --------------
 statinfo.h | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 14 deletions(-)

Show changes to 2 files +14 −14

dir.h, statinfo.h

diff --git a/dir.h b/dir.h
index 79b85a01ee..d65a40126c 100644
--- a/dir.h
+++ b/dir.h
@@ -641,18 +641,4 @@ static inline int starts_with_dot_dot_slash_native(const char *const path)
 	return path_match_flags(path, what | PATH_MATCH_NATIVE);
 }
 
-#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
-#define DTYPE(de)	((de)->d_type)
-#else
-#undef DT_UNKNOWN
-#undef DT_DIR
-#undef DT_REG
-#undef DT_LNK
-#define DT_UNKNOWN	0
-#define DT_DIR		1
-#define DT_REG		2
-#define DT_LNK		3
-#define DTYPE(de)	DT_UNKNOWN
-#endif
-
 #endif
diff --git a/statinfo.h b/statinfo.h
index e49e3054ea..fe8df633a4 100644
--- a/statinfo.h
+++ b/statinfo.h
@@ -21,4 +21,18 @@ struct stat_data {
 	unsigned int sd_size;
 };
 
+#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
+#define DTYPE(de)	((de)->d_type)
+#else
+#undef DT_UNKNOWN
+#undef DT_DIR
+#undef DT_REG
+#undef DT_LNK
+#define DT_UNKNOWN	0
+#define DT_DIR		1
+#define DT_REG		2
+#define DT_LNK		3
+#define DTYPE(de)	DT_UNKNOWN
+#endif
+
 #endif

-- 
2.41.0.rc2.161.g9c6817b8e7-goog

Junio C Hamano· Jun 12, 2023, 18:00 UTC · re: Alejandro R Sedeño · lore

Re: [PATCH] statinfo.h: move DTYPE defines from dir.h

"Alejandro R Sedeño" <asedeno@google.com> writes:

Show 19 quoted lines

> From: Alejandro R. Sedeño <asedeno@mit.edu>
>
> 592fc5b3 (dir.h: move DTYPE defines from cache.h, 2023-04-22) moved
> DTYPE macros from cache.h to dir.h, but they are still used by cache.h
> to implement ce_to_dtype(); cache.h cannot include dir.h because that
> would cause name-hash.c to have two different and conflicting
> definitions of `struct dir_entry`. (That should be separately fixed.)
>
> Both dir.h and cache.h include statinfo.h, and this seems a reasonable
> place for these definitions.
>
> This change fixes a broken build issue on old SunOS.
>
> Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
> Signed-off-by: Alejandro R Sedeño <asedeno@google.com>
> ---
>  dir.h      | 14 --------------
>  statinfo.h | 14 ++++++++++++++
>  2 files changed, 14 insertions(+), 14 deletions(-)

Thanks.  Looking great.

Show 46 quoted lines

> diff --git a/dir.h b/dir.h
> index 79b85a01ee..d65a40126c 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -641,18 +641,4 @@ static inline int starts_with_dot_dot_slash_native(const char *const path)
>  	return path_match_flags(path, what | PATH_MATCH_NATIVE);
>  }
>  
> -#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
> -#define DTYPE(de)	((de)->d_type)
> -#else
> -#undef DT_UNKNOWN
> -#undef DT_DIR
> -#undef DT_REG
> -#undef DT_LNK
> -#define DT_UNKNOWN	0
> -#define DT_DIR		1
> -#define DT_REG		2
> -#define DT_LNK		3
> -#define DTYPE(de)	DT_UNKNOWN
> -#endif
> -
>  #endif
> diff --git a/statinfo.h b/statinfo.h
> index e49e3054ea..fe8df633a4 100644
> --- a/statinfo.h
> +++ b/statinfo.h
> @@ -21,4 +21,18 @@ struct stat_data {
>  	unsigned int sd_size;
>  };
>  
> +#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
> +#define DTYPE(de)	((de)->d_type)
> +#else
> +#undef DT_UNKNOWN
> +#undef DT_DIR
> +#undef DT_REG
> +#undef DT_LNK
> +#define DT_UNKNOWN	0
> +#define DT_DIR		1
> +#define DT_REG		2
> +#define DT_LNK		3
> +#define DTYPE(de)	DT_UNKNOWN
> +#endif
> +
>  #endif