{"thread":{"id":"60503","subject":"[PATCH 1/1] attr: add native file mode values support","startedAt":"2023-11-11T04:03:16Z","lastAt":"2023-11-13T22:22:53Z","messageCount":4,"participants":["Joanna Wang","Junio C Hamano","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"484746","messageId":"20231111040309.2848560-1-jojwang@google.com","threadId":"60503","inReplyTo":null,"subject":"[PATCH 1/1] attr: add native file mode values support","fromName":"Joanna Wang","fromEmail":"jojwang@google.com","sentAt":"2023-11-11T04:03:08Z","receivedAt":"2023-11-11T04:03:16Z","isPatch":true,"sender":{"key":"jojwang@google.com","avatar":"https://avatars.githubusercontent.com/u/120585822?v=4"},"body":"Gives all paths inherent 'mode' attribute values based on the paths'\nmodes (one of 100644, 100755, 120000, 040000, 160000). Users may use\nthis feature to filter by file types. For example a pathspec such as\n':(attr:mode=160000)' could filter for submodules without needing\n`mode=160000` to actually be specified for each subdmoule path in\n.gitattributes. The native mode values are also reflected in\n`git check-attr` results.\n\nIf there is any existing mode attribute for a path (e.g. there is\n!mode, -mode, mode, mode=<value> in .gitattributes) that setting will\ntake precedence over the native mode value.\n\n---\n\nI went with 'mode' because that is the word used in documentation\n(e.g. https://git-scm.com/book/sv/v2/Git-Internals-Git-Objects)\nand in the code (e.g. `ce_mode`). Please let me know what you think\nof this UX.\n\nThe implementation happens within git_check_attr() method which is\nthe common mathod called for getting a pathspec attribute value.\n\nThe previous discussed idea did not work with `git check-attr`.\n(https://lore.kernel.org/all/CAMmZTi8swsSMcLUcW+YwUDg8GcrY_ks2+i35-nsHE3o9MNpsUQ@mail.gmail.com/).\n\nThere are no tests for excluding based on pathspec attrs in subdirectories\ndue to an existing bug. Since it is not specific to native mode, I thought\nit would be better to fix separately.\nhttps://lore.kernel.org/all/CAMmZTi89Un+bsLXdEdYs44oT8eLNp8y=Pm8ywaurcQ7ccRKGdQ@mail.gmail.com/\n\n Documentation/gitattributes.txt | 10 +++++++\n attr.c                          | 42 ++++++++++++++++++++++++--\n t/t0003-attributes.sh           | 40 +++++++++++++++++++++++++\n t/t6135-pathspec-with-attrs.sh  | 53 +++++++++++++++++++++++++++++++++\n 4 files changed, 143 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 8c1793c148..bb3c11f151 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -156,6 +156,16 @@ Unspecified::\n Any other value causes Git to act as if `text` has been left\n unspecified.\n \n+`mode`\n+^^^^^\n+\n+This attribute is for filtering files by their file bit modes\n+(40000, 120000, 160000, 100755, 100644) and has native support in git,\n+meaning values for this attribute are natively set (e.g. mode=100644) by\n+git without having to specify them in .gitattributes. However, if\n+'mode' is set in .gitattributest for some path, that value takes precendence,\n+whether it is 'set', 'unset', 'unspecified', or some other value.\n+\n `eol`\n ^^^^^\n \ndiff --git a/attr.c b/attr.c\nindex e62876dfd3..95dc1cf695 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1240,20 +1240,58 @@ static struct object_id *default_attr_source(void)\n \treturn &attr_source;\n }\n \n+\n+/*\n+ * This implements native file mode attr support.\n+ * We always return the current mode of the path, not the mode stored\n+ * in the index, unless the path is a directory where we check the index\n+ * to see if it is a GITLINK. It is ok to rely on the index for GITLINK\n+ * modes because a path cannot become a GITLINK (which is a git concept only)\n+ * without having it indexed with a GITLINK mode in git.\n+ */\n+static unsigned int native_mode_attr(struct index_state *istate, const char *path)\n+{\n+\tstruct stat st;\n+\tunsigned int mode;\n+\tif (lstat(path, &st))\n+\t\tdie_errno(_(\"unable to stat '%s'\"), path);\n+\tmode = canon_mode(st.st_mode);\n+\tif (S_ISDIR(mode)) {\n+\t\tint pos = index_name_pos(istate, path, strlen(path));\n+\t\tif (pos >= 0)\n+\t\t\tif (S_ISGITLINK(istate->cache[pos]->ce_mode))\n+\t\t\t\treturn istate->cache[pos]->ce_mode;\n+\t}\n+\treturn mode;\n+}\n+\n+\n void git_check_attr(struct index_state *istate,\n \t\t    const char *path,\n \t\t    struct attr_check *check)\n {\n \tint i;\n \tconst struct object_id *tree_oid = default_attr_source();\n+\tstruct strbuf sb = STRBUF_INIT;\n \n \tcollect_some_attrs(istate, tree_oid, path, check);\n \n \tfor (i = 0; i < check->nr; i++) {\n \t\tunsigned int n = check->items[i].attr->attr_nr;\n \t\tconst char *value = check->all_attrs[n].value;\n-\t\tif (value == ATTR__UNKNOWN)\n-\t\t\tvalue = ATTR__UNSET;\n+\t\tif (value == ATTR__UNKNOWN){\n+\t\t\tif (strcmp(check->all_attrs[n].attr->name, \"mode\") == 0) {\n+\t\t\t\t/*\n+\t\t\t\t * If value is ATTR_UNKNOWN then it has not been set\n+\t\t\t\t * anywhere with -mode (ATTR_FALSE), !mode (ATTR_UNSET),\n+\t\t\t\t * mode (ATTR_TRUE), or an explicit value. We fill\n+\t\t\t\t * value with the native mode value.\n+\t\t\t\t */\n+\t\t\t\tstrbuf_addf(&sb, \"%06o\", native_mode_attr(istate, path));\n+\t\t\t\tvalue = sb.buf;\n+\t\t\t} else\n+\t\t\t\tvalue = ATTR__UNSET;\n+\t\t}\n \t\tcheck->items[i].value = value;\n \t}\n }\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex aee2298f01..9c2603d8e2 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -19,6 +19,15 @@ attr_check () {\n \ttest_must_be_empty err\n }\n \n+attr_check_mode () {\n+\tpath=\"$1\" expect=\"$2\" git_opts=\"$3\" &&\n+\n+\tgit $git_opts check-attr mode -- \"$path\" >actual 2>err &&\n+\techo \"$path: mode: $expect\" >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_must_be_empty err\n+}\n+\n attr_check_quote () {\n \tpath=\"$1\" quoted_path=\"$2\" expect=\"$3\" &&\n \n@@ -558,4 +567,35 @@ test_expect_success EXPENSIVE 'large attributes file ignored in index' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'submodule with .git file' '\n+\tmkdir sub &&\n+\t(cd sub &&\n+\tgit init &&\n+\tmv .git .real &&\n+\techo \"gitdir: .real\" >.git &&\n+\ttest_commit first) &&\n+\tgit update-index --add -- sub\n+'\n+\n+test_expect_success 'native mode attributes work' '\n+\t>exec && chmod +x exec && attr_check_mode exec 100755 &&\n+\t>normal && attr_check_mode normal 100644 &&\n+\tmkdir dir && attr_check_mode dir 040000 &&\n+\tln -s normal normal_sym && attr_check_mode normal_sym 120000 &&\n+\tattr_check_mode sub 160000\n+'\n+\n+test_expect_success '.gitattributes mode values take precedence' '\n+\t(\n+\t\techo \"mode_value* mode=myownmode\" &&\n+\t\techo \"mode_set* mode\" &&\n+\t\techo \"mode_unset* -mode\" &&\n+\t\techo \"mode_unspecified* !mode\"\n+\t) >.gitattributes &&\n+\t>mode_value && attr_check_mode mode_value myownmode &&\n+\t>mode_unset && attr_check_mode mode_unset unset &&\n+\t>mode_unspecified && attr_check_mode mode_unspecified unspecified &&\n+\t>mode_set && attr_check_mode mode_set set\n+'\n+\n test_done\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex a9c1e4e0ec..fd9569d39b 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -64,6 +64,9 @@ test_expect_success 'setup .gitattributes' '\n \tfileSetLabel label\n \tfileValue label=foo\n \tfileWrongLabel label☺\n+\tmode_set* mode=1234\n+\tmode_unset* -mode\n+\tmode_unspecified* !mode\n \tEOF\n \techo fileSetLabel label1 >sub/.gitattributes &&\n \tgit add .gitattributes sub/.gitattributes &&\n@@ -295,4 +298,54 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'mode attr is handled correctly for overriden values' '\n+\t>mode_set_1 &&\n+\t>mode_unset_1 &&\n+\t>mode_unspecified_1 &&\n+\t>mode_regular_file_1 &&\n+\n+\tgit status -s \":(attr:mode=1234)mode*\" >actual &&\n+\tcat <<-\\EOF >expect &&\n+\t?? mode_set_1\n+\tEOF\n+\ttest_cmp expect actual &&\n+\n+\tgit status -s \":(attr:-mode)mode*\" >actual &&\n+\techo ?? mode_unset_1 >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit status -s \":(attr:!mode)mode*\" >actual &&\n+\techo ?? mode_unspecified_1 >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit status -s \":(attr:mode=100644)mode*\" >actual &&\n+\techo ?? mode_regular_file_1 >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'mode attr values are current file modes, not indexed modes' '\n+\t>mode_exec_file_1 &&\n+\n+\tgit status -s \":(attr:mode=100644)mode_exec_*\" >actual &&\n+\techo ?? mode_exec_file_1 >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit add mode_exec_file_1 && chmod +x mode_exec_file_1 &&\n+\tgit status -s \":(attr:mode=100755)mode_exec_*\" >actual &&\n+\techo AM mode_exec_file_1 >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'mode attr can be excluded' '\n+\t>mode_1_regular &&\n+\t>mode_1_exec  && chmod +x mode_1_exec &&\n+\tgit status -s \":(exclude,attr:mode=100644)\" \"mode_1_*\" >actual &&\n+\techo ?? mode_1_exec >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit status -s \":(exclude,attr:mode=100755)\" \"mode_1_*\" >actual &&\n+\techo ?? mode_1_regular >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.42.0.869.gea05f2083d-goog\n\n"},{"id":"484748","messageId":"xmqqpm0grj2s.fsf@gitster.g","threadId":"60503","inReplyTo":"20231111040309.2848560-1-jojwang@google.com","subject":"Re: [PATCH 1/1] attr: add native file mode values support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-11T04:48:43Z","receivedAt":"2023-11-11T04:48:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joanna Wang <jojwang@google.com> writes:\n\n> +/*\n> + * This implements native file mode attr support.\n> + * We always return the current mode of the path, not the mode stored\n> + * in the index, unless the path is a directory where we check the index\n> + * to see if it is a GITLINK. It is ok to rely on the index for GITLINK\n> + * modes because a path cannot become a GITLINK (which is a git concept only)\n> + * without having it indexed with a GITLINK mode in git.\n> + */\n> +static unsigned int native_mode_attr(struct index_state *istate, const char *path)\n> +{\n> +\tstruct stat st;\n> +\tunsigned int mode;\n\nPlease have a blank line here between decls and the first statement.\n\n> +\tif (lstat(path, &st))\n> +\t\tdie_errno(_(\"unable to stat '%s'\"), path);\n\nFor checking in, this is probably OK, but don't we need to switch\nbetween lstat() on the filesystem entity and inspecting ce_mode()\nfor the in-index cache_entry, depending on the git_attr_direction?\n\n> +\tmode = canon_mode(st.st_mode);\n> +\tif (S_ISDIR(mode)) {\n> +\t\tint pos = index_name_pos(istate, path, strlen(path));\n> +\t\tif (pos >= 0)\n> +\t\t\tif (S_ISGITLINK(istate->cache[pos]->ce_mode))\n> +\t\t\t\treturn istate->cache[pos]->ce_mode;\n\nEven if we assume that this code is currently meant to work only\nwith GIT_ATTR_CHECKIN, I do not think this is what you want.  When\nasked to perform \"git add . ':(exclude,mode=160000)'\", you not only\nwant to exclude the submodules that are already known to this\nsuperproject, but also a repository that _can_ become a submodule of\nthis superproject when added, no?  You are missing \"if (pos < 0)\"\ncase where you'd probably want to see if the directory at the path\nlooks like the top of a working tree with \".git\" subdirectory that\nis a valid repository, or something like that to detect such a case.\n\nOn the other hand, the GIT_ATTR_CHECKOUT direction is hopefully much\nsimpler.  You'd see what the path in the index is, among a gitlink,\na regular non-executable file, an executable file, or a symlink.\n\n> +\t}\n> +\treturn mode;\n> +}\n> +\n> +\n>  void git_check_attr(struct index_state *istate,\n>  \t\t    const char *path,\n>  \t\t    struct attr_check *check)\n>  {\n>  \tint i;\n>  \tconst struct object_id *tree_oid = default_attr_source();\n> +\tstruct strbuf sb = STRBUF_INIT;\n>  \n>  \tcollect_some_attrs(istate, tree_oid, path, check);\n>  \n>  \tfor (i = 0; i < check->nr; i++) {\n>  \t\tunsigned int n = check->items[i].attr->attr_nr;\n>  \t\tconst char *value = check->all_attrs[n].value;\n> -\t\tif (value == ATTR__UNKNOWN)\n> -\t\t\tvalue = ATTR__UNSET;\n> +\t\tif (value == ATTR__UNKNOWN){\n\nStyle.  Missing SP between \"){\".\n\n> +\t\t\tif (strcmp(check->all_attrs[n].attr->name, \"mode\") == 0) {\n\nStyle.  We avoid comparison with 0; so say\n\n\t\t\tif (!strcmp(check->all_attrs[n].attr->name, \"mode\") == 0) {\n\ninstead.\n\nIt is disturbing that we always need to perform a string comparison\nin this loop (and the function is called repeatedly while traversing\nthe tree looking for paths that match pathspec).  The attribute objects\nare designed to be interned, so at least you should be able to do\n\n\tmode_attr = git_attr(\"mode\");\n\nbefore the loop and then compare check->all_attrs[n].attr and\nmode_attr as two addresses.\n\n> +\t\t\t\t/*\n> +\t\t\t\t * If value is ATTR_UNKNOWN then it has not been set\n> +\t\t\t\t * anywhere with -mode (ATTR_FALSE), !mode (ATTR_UNSET),\n> +\t\t\t\t * mode (ATTR_TRUE), or an explicit value. We fill\n> +\t\t\t\t * value with the native mode value.\n> +\t\t\t\t */\n> +\t\t\t\tstrbuf_addf(&sb, \"%06o\", native_mode_attr(istate, path));\n> +\t\t\t\tvalue = sb.buf;\n\nOr even better yet, as this is not a place to write too many lines\nfor a single oddball attribute, it might make even more sense to\nintroduce a helper function and make a call to it like so:\n\n\t\tif (value == ATTR__UNKNOWN)\n-\t\t\tvalue = ATTR__UNSET;\n+\t\t\tvalue = compute_attr_from_path(istate, path, check_items[i].attr);\n\nwith the new helper function do all the heavy lifting, e.g.,\n\n\tstatic const char *compute_attr_from_path(struct index_state *istate,\n\t\t\t\t\t\t  const char *path,\n\t\t\t\t\t\t  const struct git_attr *attr) {\n\t\tstatic struct git_attr mode_attr;\n\n\t\tif (!mode_attr)\n\t\t\tmode_attr = git_attr(\"mode\");\n\n\t\tif (attr == mode_attr) {\n\t\t\tcall native_mode_attr(), stringify, and\n\t\t\treturn it;\n\t\t}\n\t\treturn ATTR__UNSET;\n\t}\n\nwhich will allow us to easily add in the future special attribute\nsimilar to \"mode\" whose fallback value is computed.\n\nOtherwise, looking pretty good.  Thanks for working on this.\n"},{"id":"484803","messageId":"20231113165031.GA28778@tb-raspi4","threadId":"60503","inReplyTo":"20231111040309.2848560-1-jojwang@google.com","subject":"Re: [PATCH 1/1] attr: add native file mode values support","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2023-11-13T16:50:31Z","receivedAt":"2023-11-13T16:50:39Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Sat, Nov 11, 2023 at 04:03:08AM +0000, Joanna Wang wrote:\n\nSome thoughts and comments inline...\n\n> Gives all paths inherent 'mode' attribute values based on the paths'\n> modes (one of 100644, 100755, 120000, 040000, 160000). Users may use\n> this feature to filter by file types. For example a pathspec such as\n> ':(attr:mode=160000)' could filter for submodules without needing\n\nMy spontanous feeling is that filetype may be another choice:\n> ':(attr:filetype=160000)' could filter for submodules without needing\nAnd having written this, we can think using something borrowed from\n`find . -type f`\n\n:(attr:filetype=f)' or :(attr:filetype=x)' (for executable)\n\n> `mode=160000` to actually be specified for each subdmoule path in\n> .gitattributes. The native mode values are also reflected in\n> `git check-attr` results.\n\nBut then I missed the point why we need an attribute here?\nThe mode is already defined by the the file system (and Git),\nis there a special reason that the user can define or re-define the\nvalue here ?\nMay be there is, when working with pathspec.\nBut then \"pathspec=\" could be a better construction.\nSince \"mode\" could make a reader think that Git does somewhat with the file\nwhen checking out.\nMy personal hope reading \"mode=100755\" in .gitattributes would\nbe that Git makes it executable when checking out, if if it is\nrecorded in Git as 100644, probably coming from a file-system that\ndoesn't support the executable bit in a Unix way.\n\n\n> If there is any existing mode attribute for a path (e.g. there is\n> !mode, -mode, mode, mode=<value> in .gitattributes) that setting will\n> take precedence over the native mode value.\n>\n> ---\n>\n> I went with 'mode' because that is the word used in documentation\n> (e.g. https://git-scm.com/book/sv/v2/Git-Internals-Git-Objects)\n> and in the code (e.g. `ce_mode`). Please let me know what you think\n> of this UX.\n>\n> The implementation happens within git_check_attr() method which is\n> the common mathod called for getting a pathspec attribute value.\n>\n> The previous discussed idea did not work with `git check-attr`.\n> (https://lore.kernel.org/all/CAMmZTi8swsSMcLUcW+YwUDg8GcrY_ks2+i35-nsHE3o9MNpsUQ@mail.gmail.com/).\n>\n> There are no tests for excluding based on pathspec attrs in subdirectories\n> due to an existing bug. Since it is not specific to native mode, I thought\n> it would be better to fix separately.\n> https://lore.kernel.org/all/CAMmZTi89Un+bsLXdEdYs44oT8eLNp8y=Pm8ywaurcQ7ccRKGdQ@mail.gmail.com/\n\nReading \"pathspec attrs\" above it feels better to call the new attribute pathspec.\nBut why should a user be allowed to overwrite what we have in the index ?\nThat is somewhat missing in the motivation for this change,\nit would be good to have this explained in the commit message.\n\n>\n>  Documentation/gitattributes.txt | 10 +++++++\n>  attr.c                          | 42 ++++++++++++++++++++++++--\n>  t/t0003-attributes.sh           | 40 +++++++++++++++++++++++++\n>  t/t6135-pathspec-with-attrs.sh  | 53 +++++++++++++++++++++++++++++++++\n>  4 files changed, 143 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\n> index 8c1793c148..bb3c11f151 100644\n> --- a/Documentation/gitattributes.txt\n> +++ b/Documentation/gitattributes.txt\n> @@ -156,6 +156,16 @@ Unspecified::\n>  Any other value causes Git to act as if `text` has been left\n>  unspecified.\n>\n> +`mode`\n> +^^^^^\n> +\n> +This attribute is for filtering files by their file bit modes\n> +(40000, 120000, 160000, 100755, 100644) and has native support in git,\n> +meaning values for this attribute are natively set (e.g. mode=100644) by\n> +git without having to specify them in .gitattributes. However, if\n> +'mode' is set in .gitattributest for some path, that value takes precendence,\n> +whether it is 'set', 'unset', 'unspecified', or some other value.\n> +\n>  `eol`\n>  ^^^^^\n>\n> diff --git a/attr.c b/attr.c\n> index e62876dfd3..95dc1cf695 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -1240,20 +1240,58 @@ static struct object_id *default_attr_source(void)\n>  \treturn &attr_source;\n>  }\n>\n> +\n> +/*\n> + * This implements native file mode attr support.\n> + * We always return the current mode of the path, not the mode stored\n> + * in the index, unless the path is a directory where we check the index\n> + * to see if it is a GITLINK. It is ok to rely on the index for GITLINK\n> + * modes because a path cannot become a GITLINK (which is a git concept only)\n> + * without having it indexed with a GITLINK mode in git.\n> + */\n> +static unsigned int native_mode_attr(struct index_state *istate, const char *path)\n> +{\n> +\tstruct stat st;\n> +\tunsigned int mode;\n> +\tif (lstat(path, &st))\n> +\t\tdie_errno(_(\"unable to stat '%s'\"), path);\n> +\tmode = canon_mode(st.st_mode);\n> +\tif (S_ISDIR(mode)) {\n> +\t\tint pos = index_name_pos(istate, path, strlen(path));\n> +\t\tif (pos >= 0)\n> +\t\t\tif (S_ISGITLINK(istate->cache[pos]->ce_mode))\n> +\t\t\t\treturn istate->cache[pos]->ce_mode;\n> +\t}\n> +\treturn mode;\n> +}\n> +\n> +\n>  void git_check_attr(struct index_state *istate,\n>  \t\t    const char *path,\n>  \t\t    struct attr_check *check)\n>  {\n>  \tint i;\n>  \tconst struct object_id *tree_oid = default_attr_source();\n> +\tstruct strbuf sb = STRBUF_INIT;\n>\n>  \tcollect_some_attrs(istate, tree_oid, path, check);\n>\n>  \tfor (i = 0; i < check->nr; i++) {\n>  \t\tunsigned int n = check->items[i].attr->attr_nr;\n>  \t\tconst char *value = check->all_attrs[n].value;\n> -\t\tif (value == ATTR__UNKNOWN)\n> -\t\t\tvalue = ATTR__UNSET;\n> +\t\tif (value == ATTR__UNKNOWN){\n> +\t\t\tif (strcmp(check->all_attrs[n].attr->name, \"mode\") == 0) {\n> +\t\t\t\t/*\n> +\t\t\t\t * If value is ATTR_UNKNOWN then it has not been set\n> +\t\t\t\t * anywhere with -mode (ATTR_FALSE), !mode (ATTR_UNSET),\n> +\t\t\t\t * mode (ATTR_TRUE), or an explicit value. We fill\n> +\t\t\t\t * value with the native mode value.\n> +\t\t\t\t */\n> +\t\t\t\tstrbuf_addf(&sb, \"%06o\", native_mode_attr(istate, path));\n> +\t\t\t\tvalue = sb.buf;\n> +\t\t\t} else\n> +\t\t\t\tvalue = ATTR__UNSET;\n> +\t\t}\n>  \t\tcheck->items[i].value = value;\n>  \t}\n>  }\n> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\n> index aee2298f01..9c2603d8e2 100755\n> --- a/t/t0003-attributes.sh\n> +++ b/t/t0003-attributes.sh\n> @@ -19,6 +19,15 @@ attr_check () {\n>  \ttest_must_be_empty err\n>  }\n>\n> +attr_check_mode () {\n> +\tpath=\"$1\" expect=\"$2\" git_opts=\"$3\" &&\nIt would be easier to read, if each assignment is on it's on line\n> +\n> +\tgit $git_opts check-attr mode -- \"$path\" >actual 2>err &&\n> +\techo \"$path: mode: $expect\" >expect &&\n> +\ttest_cmp expect actual &&\n> +\ttest_must_be_empty err\n> +}\n> +\n>  attr_check_quote () {\n>  \tpath=\"$1\" quoted_path=\"$2\" expect=\"$3\" &&\n>\n> @@ -558,4 +567,35 @@ test_expect_success EXPENSIVE 'large attributes file ignored in index' '\n>  \ttest_cmp expect err\n>  '\n>\n> +test_expect_success 'submodule with .git file' '\n> +\tmkdir sub &&\n> +\t(cd sub &&\n> +\tgit init &&\n> +\tmv .git .real &&\n> +\techo \"gitdir: .real\" >.git &&\n> +\ttest_commit first) &&\n> +\tgit update-index --add -- sub\n\nStyle and indentation (Please use TAB for indenting)\nUsing sub-shells is good. Somewhat easier to read would be this:\n\n\tmkdir sub &&\n\t(\n\t  cd sub &&\n\t  git init &&\n\t  mv .git .real &&\n\t  echo \"gitdir: .real\" >.git &&\n\t  test_commit first\n\t)  &&\n\n\n\n\n> +'\n> +\n> +test_expect_success 'native mode attributes work' '\n> +\t>exec && chmod +x exec && attr_check_mode exec 100755 &&\n> +\t>normal && attr_check_mode normal 100644 &&\n> +\tmkdir dir && attr_check_mode dir 040000 &&\n> +\tln -s normal normal_sym && attr_check_mode normal_sym 120000 &&\n> +\tattr_check_mode sub 160000\n> +'\n\nWe need a precondition here:\ntest_expect_success SYMLINKS\n\n> +\n> +test_expect_success '.gitattributes mode values take precedence' '\n> +\t(\n> +\t\techo \"mode_value* mode=myownmode\" &&\n> +\t\techo \"mode_set* mode\" &&\n> +\t\techo \"mode_unset* -mode\" &&\n> +\t\techo \"mode_unspecified* !mode\"\n> +\t) >.gitattributes &&\n\nCan this be written using a\n\tcat <<-\\EOF >expect &&\n\n\tEOF\nexpression ?\n\n> +\t>mode_value && attr_check_mode mode_value myownmode &&\n> +\t>mode_unset && attr_check_mode mode_unset unset &&\n> +\t>mode_unspecified && attr_check_mode mode_unspecified unspecified &&\n> +\t>mode_set && attr_check_mode mode_set set\n> +'\n> +\n>  test_done\n> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\n> index a9c1e4e0ec..fd9569d39b 100755\n> --- a/t/t6135-pathspec-with-attrs.sh\n> +++ b/t/t6135-pathspec-with-attrs.sh\n> @@ -64,6 +64,9 @@ test_expect_success 'setup .gitattributes' '\n>  \tfileSetLabel label\n>  \tfileValue label=foo\n>  \tfileWrongLabel label☺\n> +\tmode_set* mode=1234\n> +\tmode_unset* -mode\n> +\tmode_unspecified* !mode\n>  \tEOF\n>  \techo fileSetLabel label1 >sub/.gitattributes &&\n>  \tgit add .gitattributes sub/.gitattributes &&\n> @@ -295,4 +298,54 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '\n>  \ttest_cmp expect actual\n>  '\n>\n> +test_expect_success 'mode attr is handled correctly for overriden values' '\n> +\t>mode_set_1 &&\n> +\t>mode_unset_1 &&\n> +\t>mode_unspecified_1 &&\n> +\t>mode_regular_file_1 &&\n> +\n> +\tgit status -s \":(attr:mode=1234)mode*\" >actual &&\n> +\tcat <<-\\EOF >expect &&\n> +\t?? mode_set_1\n> +\tEOF\n> +\ttest_cmp expect actual &&\n> +\n> +\tgit status -s \":(attr:-mode)mode*\" >actual &&\n> +\techo ?? mode_unset_1 >expect &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tgit status -s \":(attr:!mode)mode*\" >actual &&\n> +\techo ?? mode_unspecified_1 >expect &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tgit status -s \":(attr:mode=100644)mode*\" >actual &&\n> +\techo ?? mode_regular_file_1 >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'mode attr values are current file modes, not indexed modes' '\n> +\t>mode_exec_file_1 &&\n> +\n> +\tgit status -s \":(attr:mode=100644)mode_exec_*\" >actual &&\n> +\techo ?? mode_exec_file_1 >expect &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tgit add mode_exec_file_1 && chmod +x mode_exec_file_1 &&\n> +\tgit status -s \":(attr:mode=100755)mode_exec_*\" >actual &&\n> +\techo AM mode_exec_file_1 >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'mode attr can be excluded' '\n> +\t>mode_1_regular &&\n> +\t>mode_1_exec  && chmod +x mode_1_exec &&\n> +\tgit status -s \":(exclude,attr:mode=100644)\" \"mode_1_*\" >actual &&\n> +\techo ?? mode_1_exec >expect &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tgit status -s \":(exclude,attr:mode=100755)\" \"mode_1_*\" >actual &&\n> +\techo ?? mode_1_regular >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n> --\n> 2.42.0.869.gea05f2083d-goog\n>\n>\n"},{"id":"484820","messageId":"xmqqo7fx5m4r.fsf@gitster.g","threadId":"60503","inReplyTo":"20231113165031.GA28778@tb-raspi4","subject":"Re: [PATCH 1/1] attr: add native file mode values support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-13T22:22:44Z","receivedAt":"2023-11-13T22:22:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> On Sat, Nov 11, 2023 at 04:03:08AM +0000, Joanna Wang wrote:\n>\n> Some thoughts and comments inline...\n>\n>> Gives all paths inherent 'mode' attribute values based on the paths'\n>> modes (one of 100644, 100755, 120000, 040000, 160000). Users may use\n>> this feature to filter by file types. For example a pathspec such as\n>> ':(attr:mode=160000)' could filter for submodules without needing\n>\n> My spontanous feeling is that filetype may be another choice:\n>> ':(attr:filetype=160000)' could filter for submodules without needing\n\nI do agree that \"mode\" invites \"mode of what???\" reaction, and that\na term that narrows the scope would be preferrable.  \"Filemode\" is a\nbit questionable, though, as we give this permbits to non-files like\nsubmodules.  \"ls-tree\" documentation seems to call it %(objectmode).\n\n> And having written this, we can think using something borrowed from\n> `find . -type f`\n>\n> :(attr:filetype=f)' or :(attr:filetype=x)' (for executable)\n\nThis would not work for submodules, though.  Naively one might want\nto abuse 'd' but I suspect we would eventually want to be able to\ngive the mode bits to an out-of-cone directory storeed in the index\nas a tree in a cone-mode sparse checkout, which would be 040000,\nwhich deserves 'd' more than submodules.\n\n> But then I missed the point why we need an attribute here?\n> The mode is already defined by the the file system (and Git),\n> is there a special reason that the user can define or re-define the\n> value here ?\n\nI think the idea is that \"mode\" being a too generic word can be used\nfor totally different purposes in existing projects and the addition\ndid not want to disturb their own use.  But stepping back a bit,\nsuch an application is likely marking selected few paths with the\nattribute, and paths for which \"mode\" was \"unset\" are now given\nthese natural \"mode\"; it is inevitable to crash with such uses.  If\nwe want to introduce \"native\" attributes of this kind, we would\nprobably need to carve out namespaces a bit more clearaly.\n\n> May be there is, when working with pathspec.\n> But then \"pathspec=\" could be a better construction.\n> Since \"mode\" could make a reader think that Git does somewhat with the file\n> when checking out.\n> My personal hope reading \"mode=100755\" in .gitattributes would\n> be that Git makes it executable when checking out, if if it is\n> recorded in Git as 100644, probably coming from a file-system that\n> doesn't support the executable bit in a Unix way.\n\nThat is not the intended way this attribute is to be used.  Perhaps\nwe should make it an error (or ignored) when certain built-in/native\nattributes are seen in the attribute file, but again that takes some\nnamespace carved out to avoid crashing with end-user names.\n\n>> If there is any existing mode attribute for a path (e.g. there is\n>> !mode, -mode, mode, mode=<value> in .gitattributes) that setting will\n>> take precedence over the native mode value.\n\nAgain, this has one hole, I think.  Paths that are not mentioned\n(not even with \"!mode\") would come to the function as ATTR__UNKNOWN\nand trigger the fallback behaviour, even when other paths are given\nend-user specified \"mode\" attribute values.\n"}]}