{"thread":{"id":"64839","subject":"[PATCH v1][RFC] symlinks: use unsigned int for flags","startedAt":"2026-01-20T15:22:36Z","lastAt":"2026-01-21T09:39:29Z","messageCount":4,"participants":["Tian Yuchen","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"534247","messageId":"20260120152219.398999-1-a3205153416@gmail.com","threadId":"64839","inReplyTo":null,"subject":"[PATCH v1][RFC] symlinks: use unsigned int for flags","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-01-20T15:22:19Z","receivedAt":"2026-01-20T15:22:36Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"The 'flags' and 'track_flags' fields in symlinks.c are used\nstrictly as a collection of bits (using bitwise operators including\n&, |, ~). Using a signed integer for bitmasks may lead to undefined\nbehavior with shift operations and logic errors if the MSB is touched.\n\nChange these fields from 'int' to 'unsigned int' to align with C\nstandards and typical usage patterns.\n\nSigned-off-by: Tian Yuchen <a3205153416@gmail.com>\n---\n symlinks.c | 12 ++++++------\n symlinks.h |  4 ++--\n 2 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/symlinks.c b/symlinks.c\nindex 9cc090d42c..ed63891149 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -74,11 +74,11 @@ static inline void reset_lstat_cache(struct cache_def *cache)\n  */\n static int lstat_cache_matchlen(struct cache_def *cache,\n \t\t\t\tconst char *name, int len,\n-\t\t\t\tint *ret_flags, int track_flags,\n+\t\t\t\tunsigned int *ret_flags, unsigned int track_flags,\n \t\t\t\tint prefix_len_stat_func)\n {\n \tint match_len, last_slash, last_slash_dir, previous_slash;\n-\tint save_flags, ret, saved_errno = 0;\n+\tunsigned int save_flags, ret, saved_errno = 0;\n \tstruct stat st;\n \n \tif (cache->track_flags != track_flags ||\n@@ -192,10 +192,10 @@ static int lstat_cache_matchlen(struct cache_def *cache,\n \treturn match_len;\n }\n \n-static int lstat_cache(struct cache_def *cache, const char *name, int len,\n-\t\t       int track_flags, int prefix_len_stat_func)\n+static unsigned int lstat_cache(struct cache_def *cache, const char *name, int len,\n+\t\t       unsigned int track_flags, int prefix_len_stat_func)\n {\n-\tint flags;\n+\tunsigned int flags;\n \t(void)lstat_cache_matchlen(cache, name, len, &flags, track_flags,\n \t\t\tprefix_len_stat_func);\n \treturn flags;\n@@ -234,7 +234,7 @@ int check_leading_path(const char *name, int len, int warn_on_lstat_err)\n static int threaded_check_leading_path(struct cache_def *cache, const char *name,\n \t\t\t\t       int len, int warn_on_lstat_err)\n {\n-\tint flags;\n+\tunsigned int flags;\n \tint match_len = lstat_cache_matchlen(cache, name, len, &flags,\n \t\t\t   FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT);\n \tint saved_errno = errno;\ndiff --git a/symlinks.h b/symlinks.h\nindex 7ae3d5b856..25bf04f54f 100644\n--- a/symlinks.h\n+++ b/symlinks.h\n@@ -5,8 +5,8 @@\n \n struct cache_def {\n \tstruct strbuf path;\n-\tint flags;\n-\tint track_flags;\n+\tunsigned int flags;\n+\tunsigned int track_flags;\n \tint prefix_len_stat_func;\n };\n #define CACHE_DEF_INIT { \\\n-- \n2.43.0\n\n"},{"id":"534263","messageId":"CA+rU_o6Mrw9ga0TST6p+8MANYaNGiKP9qud8izHL+hwxou9upA@mail.gmail.com","threadId":"64839","inReplyTo":"20260120152219.398999-1-a3205153416@gmail.com","subject":"Re: [PATCH v1][RFC] symlinks: use unsigned int for flags","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-01-20T15:36:25Z","receivedAt":"2026-01-20T15:36:38Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Me as total newbie to the git community (also preparing for GSoC\n2026), welcome comments or any possible suggestions!\n\nWhile preparing v2 to fix the return type of lstat_cache(), a broader\nquestion regarding coding style came to mind:\n\nI realized that even without changing the return type, the code\ncompiles and runs because of C's implicit integer conversion\n (since the flag values don't exceed INT_MAX).\n\nMy question is: In the Git codebase, are such \"safe\" implicit conversions\ngenerally tolerated to minimize code churn, or is it considered a\nbest practice to strictly avoid them and match types explicitly whenever\npossible?\n\nI want to ensure I have the right standard for type strictness in\n future contributions.\n\nTian Yuchen <a3205153416@gmail.com> 于2026年1月20日周二 23:22写道：\n>\n> The 'flags' and 'track_flags' fields in symlinks.c are used\n> strictly as a collection of bits (using bitwise operators including\n> &, |, ~). Using a signed integer for bitmasks may lead to undefined\n> behavior with shift operations and logic errors if the MSB is touched.\n>\n> Change these fields from 'int' to 'unsigned int' to align with C\n> standards and typical usage patterns.\n>\n> Signed-off-by: Tian Yuchen <a3205153416@gmail.com>\n> ---\n>  symlinks.c | 12 ++++++------\n>  symlinks.h |  4 ++--\n>  2 files changed, 8 insertions(+), 8 deletions(-)\n>\n> diff --git a/symlinks.c b/symlinks.c\n> index 9cc090d42c..ed63891149 100644\n> --- a/symlinks.c\n> +++ b/symlinks.c\n> @@ -74,11 +74,11 @@ static inline void reset_lstat_cache(struct cache_def *cache)\n>   */\n>  static int lstat_cache_matchlen(struct cache_def *cache,\n>                                 const char *name, int len,\n> -                               int *ret_flags, int track_flags,\n> +                               unsigned int *ret_flags, unsigned int track_flags,\n>                                 int prefix_len_stat_func)\n>  {\n>         int match_len, last_slash, last_slash_dir, previous_slash;\n> -       int save_flags, ret, saved_errno = 0;\n> +       unsigned int save_flags, ret, saved_errno = 0;\n>         struct stat st;\n>\n>         if (cache->track_flags != track_flags ||\n> @@ -192,10 +192,10 @@ static int lstat_cache_matchlen(struct cache_def *cache,\n>         return match_len;\n>  }\n>\n> -static int lstat_cache(struct cache_def *cache, const char *name, int len,\n> -                      int track_flags, int prefix_len_stat_func)\n> +static unsigned int lstat_cache(struct cache_def *cache, const char *name, int len,\n> +                      unsigned int track_flags, int prefix_len_stat_func)\n>  {\n> -       int flags;\n> +       unsigned int flags;\n>         (void)lstat_cache_matchlen(cache, name, len, &flags, track_flags,\n>                         prefix_len_stat_func);\n>         return flags;\n> @@ -234,7 +234,7 @@ int check_leading_path(const char *name, int len, int warn_on_lstat_err)\n>  static int threaded_check_leading_path(struct cache_def *cache, const char *name,\n>                                        int len, int warn_on_lstat_err)\n>  {\n> -       int flags;\n> +       unsigned int flags;\n>         int match_len = lstat_cache_matchlen(cache, name, len, &flags,\n>                            FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT);\n>         int saved_errno = errno;\n> diff --git a/symlinks.h b/symlinks.h\n> index 7ae3d5b856..25bf04f54f 100644\n> --- a/symlinks.h\n> +++ b/symlinks.h\n> @@ -5,8 +5,8 @@\n>\n>  struct cache_def {\n>         struct strbuf path;\n> -       int flags;\n> -       int track_flags;\n> +       unsigned int flags;\n> +       unsigned int track_flags;\n>         int prefix_len_stat_func;\n>  };\n>  #define CACHE_DEF_INIT { \\\n> --\n> 2.43.0\n>\n"},{"id":"534321","messageId":"aXCdVyySI_biFPzS@pks.im","threadId":"64839","inReplyTo":"20260120152219.398999-1-a3205153416@gmail.com","subject":"Re: [PATCH v1][RFC] symlinks: use unsigned int for flags","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-21T09:33:11Z","receivedAt":"2026-01-21T09:33:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 20, 2026 at 11:22:19PM +0800, Tian Yuchen wrote:\n> The 'flags' and 'track_flags' fields in symlinks.c are used\n> strictly as a collection of bits (using bitwise operators including\n> &, |, ~).\n\nThat's one important data point. The other important data point is\nwhether or not we ever use any negative values here. But these flags are\ndefined like this:\n\n    #define FL_DIR      (1 << 0)\n    #define FL_NOENT    (1 << 1)\n    #define FL_SYMLINK  (1 << 2)\n    #define FL_LSTATERR (1 << 3)\n    #define FL_ERR      (1 << 4)\n    #define FL_FULLPATH (1 << 5)\n\nSo they indeed are only ever positive.\n\n> Using a signed integer for bitmasks may lead to undefined\n> behavior with shift operations and logic errors if the MSB is touched.\n\nMostly a theoretical issue, but we indeed prefer to use unsigned ints\nfor bitsets like this.\n\n> diff --git a/symlinks.c b/symlinks.c\n> index 9cc090d42c..ed63891149 100644\n> --- a/symlinks.c\n> +++ b/symlinks.c\n> @@ -74,11 +74,11 @@ static inline void reset_lstat_cache(struct cache_def *cache)\n>   */\n>  static int lstat_cache_matchlen(struct cache_def *cache,\n>  \t\t\t\tconst char *name, int len,\n> -\t\t\t\tint *ret_flags, int track_flags,\n> +\t\t\t\tunsigned int *ret_flags, unsigned int track_flags,\n\n`ret_flags` is also a combination of `FL_*` flags, so it's never\nexpected to be negative, either.\n\n>  \t\t\t\tint prefix_len_stat_func)\n>  {\n>  \tint match_len, last_slash, last_slash_dir, previous_slash;\n> -\tint save_flags, ret, saved_errno = 0;\n> +\tunsigned int save_flags, ret, saved_errno = 0;\n\nChanging the type of `save_flags` makes sense, but you also change the\ntype of `ret` and `saved_errno` to become unsigned, which does not make\nsense.\n\n> @@ -192,10 +192,10 @@ static int lstat_cache_matchlen(struct cache_def *cache,\n>  \treturn match_len;\n>  }\n>  \n> -static int lstat_cache(struct cache_def *cache, const char *name, int len,\n> -\t\t       int track_flags, int prefix_len_stat_func)\n> +static unsigned int lstat_cache(struct cache_def *cache, const char *name, int len,\n> +\t\t       unsigned int track_flags, int prefix_len_stat_func)\n>  {\n> -\tint flags;\n> +\tunsigned int flags;\n>  \t(void)lstat_cache_matchlen(cache, name, len, &flags, track_flags,\n>  \t\t\tprefix_len_stat_func);\n>  \treturn flags;\n\nAll callers already pass unsigned flags, so that's good. But there's two\ncallers that use the return value of this function:\n\n  - `threaded_has_symlink_leading_path()` uses `lstat_cache() & FL_SYMLINK`.\n\n  - `threaded_has_dirs_only_path()` uses `lstat_cache() & FL_DIR`.\n\nBoth of these functions really only care about whether or not the\nstatement evaluates to a truish value. Maybe it would make sense to have\na preparatory commit where convert the return values of those functions\nto be booleans?\n\n> @@ -234,7 +234,7 @@ int check_leading_path(const char *name, int len, int warn_on_lstat_err)\n>  static int threaded_check_leading_path(struct cache_def *cache, const char *name,\n>  \t\t\t\t       int len, int warn_on_lstat_err)\n>  {\n> -\tint flags;\n> +\tunsigned int flags;\n>  \tint match_len = lstat_cache_matchlen(cache, name, len, &flags,\n>  \t\t\t   FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT);\n>  \tint saved_errno = errno;\n\nLooks good.\n\n> diff --git a/symlinks.h b/symlinks.h\n> index 7ae3d5b856..25bf04f54f 100644\n> --- a/symlinks.h\n> +++ b/symlinks.h\n> @@ -5,8 +5,8 @@\n>  \n>  struct cache_def {\n>  \tstruct strbuf path;\n> -\tint flags;\n> -\tint track_flags;\n> +\tunsigned int flags;\n> +\tunsigned int track_flags;\n>  \tint prefix_len_stat_func;\n>  };\n>  #define CACHE_DEF_INIT { \\\n\nThere is only one user of `struct cache_def` outside of \"symlink.c\",\nwhich is \"preload-index.c\". That user doesn't care about the flag\ndefinitions though, so this conversion looks good to me.\n\nThanks!\n\nPatrick\n"},{"id":"534322","messageId":"aXCey7ysfqORONXr@pks.im","threadId":"64839","inReplyTo":"CA+rU_o6Mrw9ga0TST6p+8MANYaNGiKP9qud8izHL+hwxou9upA@mail.gmail.com","subject":"Re: [PATCH v1][RFC] symlinks: use unsigned int for flags","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-21T09:39:23Z","receivedAt":"2026-01-21T09:39:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 20, 2026 at 11:36:25PM +0800, Tian Yuchen wrote:\n> Me as total newbie to the git community (also preparing for GSoC\n> 2026), welcome comments or any possible suggestions!\n> \n> While preparing v2 to fix the return type of lstat_cache(), a broader\n> question regarding coding style came to mind:\n> \n> I realized that even without changing the return type, the code\n> compiles and runs because of C's implicit integer conversion\n>  (since the flag values don't exceed INT_MAX).\n> \n> My question is: In the Git codebase, are such \"safe\" implicit conversions\n> generally tolerated to minimize code churn, or is it considered a\n> best practice to strictly avoid them and match types explicitly whenever\n> possible?\n\nI wouldn't say \"strictly\". We have lots of cases where we do in fact\nrely on implicit conversions. In some cases it's a code smell, in lots\nof other cases it's fine. We have tried to become a bit more mindful\naround such implicit conversions as those have bitten us in the past,\nbut we're not on a crusade against them.\n\nSo I guess the answer is \"it depends\". A slow trickle of improvements in\nthis area does make sense, but we don't want to convert all of our code\nbase in large patch series just for the sake of it.\n\nPatrick\n"}]}