{"thread":{"id":"60514","subject":"Re: [PATCH 1/1] attr: add native file mode values support","startedAt":"2023-11-14T02:28:14Z","lastAt":"2024-01-12T06:12:35Z","messageCount":15,"participants":["Joanna Wang","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"484839","messageId":"CAMmZTi-7=L6XOB9-MwZCpD5QaziTi0x6J7m-UTtwQ0S00wnVgQ@mail.gmail.com","threadId":"60514","inReplyTo":null,"subject":"Re: [PATCH 1/1] attr: add native file mode values support","fromName":"Joanna Wang","fromEmail":"jojwang@google.com","sentAt":"2023-11-14T02:28:00Z","receivedAt":"2023-11-14T02:28:14Z","isPatch":true,"sender":{"key":"jojwang@google.com","avatar":"https://avatars.githubusercontent.com/u/120585822?v=4"},"body":">> Even if we assume that this code is currently meant to work only\n>> with GIT_ATTR_CHECKIN, I do not think this is what you want. When\n>> asked to perform \"git add . ':(exclude,mode=160000)'\", you not only\n>> want to exclude the submodules that are already known to this\n>> superproject, but also a repository that _can_ become a submodule of\n>> this superproject when added, no?\nSorry, I was totally ignorant of two key concepts that you mention here.\nI somehow missed the concept of git_attr_direction altogether and I also\ndid not know submodules could be added with git-add (I was only\nfamiliar with how\nthey are currently added in chromium via git update-index).\nThis makes sense now. I'll fix in the next patch.\n\n>> On the other hand, the GIT_ATTR_CHECKOUT direction is hopefully much\n>> simpler. You'd see what the path in the index is, among a gitlink,\n>> a regular non-executable file, an executable file, or a symlink.\nI noticed for both the GIT_ATTR_CHECKOUT and GIT_ATTR_CHECKIN directions,\nin read_attr(), the indexed .gitattributes file is checked with the\nactual file as fallback or vice versa.\nI would think that we'd only want to use attributes from one state\n(e.g. what's actually in the file)\nor the other (e.g. what's indexed).\nSo I guess I'm still not sure what the \"direction\" concept is.\nFor GIT_ATTR_CHECKOUT, would we want to fallback to lstat?\n\n>> \"ls-tree\" documentation seems to call it %(objectmode).\nI can change it to 'objectmode' in a followup patch, if there are no\nobjections to this.\n\n>> I think the idea is that \"mode\" being a too generic word can be used\n>> for totally different purposes\nMy thinking was, no matter how generic or rare a name we choose, there\nis always a chance\nno matter how tiny, that the name will be in use in someone's .gitattributes.\nBut if people are ok with choosing something less generic\nand have that become a 'reserved' attribute name and not have any\nexisting values in .gitattributes\ntake precedence I can do that.\nI just don't know how git has historically balanced breaking existing\nworkflows vs easier/more reasonable\nimplementation/behavior.\n\n>> But stepping back a bit,\n>> such an application is likely marking selected few paths with the\n>> attribute, and paths for which \"mode\" was \"unset\" are now given\n>> these natural \"mode\"; it is inevitable to crash with such uses.\nI'm confused. This does not match what I think is the current behavior\nof my patch.\nIf \"mode\" was unset or removed for a path (meaning ' !mode' was\nadded to .gitattributes),\nthe code in my patch would respect that and not return the native mode.\nIt would return 'unspecified' or 'unset'. I have tests for these in\nthe patch, but if I've missed some\ntest cases please let me know.\n\n\n>> Again, this has one hole, I think. Paths that are not mentioned\n>> (not even with \"!mode\") would come to the function as ATTR__UNKNOWN\n>> and trigger the fallback behaviour, even when other paths are given\n>> end-user specified \"mode\" attribute values.\nWhat you are describing sounds correct/what I intended.\nSo are you saying that the expected behavior is actually:\nIf the user sets 'mode' for 1+ paths in the repo, then the native mode\nfallback should\nNOT be used for all paths in the repo?\n"},{"id":"484841","messageId":"xmqqbkbx11x2.fsf@gitster.g","threadId":"60514","inReplyTo":"CAMmZTi-7=L6XOB9-MwZCpD5QaziTi0x6J7m-UTtwQ0S00wnVgQ@mail.gmail.com","subject":"Re: [PATCH 1/1] attr: add native file mode values support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-14T02:52:57Z","receivedAt":"2023-11-14T02:53:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joanna Wang writes:\n\n>>> Even if we assume that this code is currently meant to work only\n>>> with GIT_ATTR_CHECKIN, I do not think this is what you want. When\n>>> asked to perform \"git add . ':(exclude,mode=160000)'\", you not only\n>>> want to exclude the submodules that are already known to this\n>>> superproject, but also a repository that _can_ become a submodule of\n>>> this superproject when added, no?\n> Sorry, I was totally ignorant of ...\n\nNothing to be sorry about, and if I sounded like I was upset or\nsomething, that wasn't my intention. Reviewers and more experienced\ndevelopers read posted patches to give pieces of advice exactly\nbecause no single human is perfect and knows everything. We cover\nholes in each others' knowledge and attention.\n\n>>> On the other hand, the GIT_ATTR_CHECKOUT direction is hopefully much\n>>> simpler. You'd see what the path in the index is, among a gitlink,\n>>> a regular non-executable file, an executable file, or a symlink.\n> I noticed for both the GIT_ATTR_CHECKOUT and GIT_ATTR_CHECKIN directions,\n> in read_attr(), the indexed .gitattributes file is checked with the\n> actual file as fallback or vice versa.\n> I would think that we'd only want to use attributes from one state\n> (e.g. what's actually in the file)\n> or the other (e.g. what's indexed).\n> So I guess I'm still not sure what the \"direction\" concept is.\n> For GIT_ATTR_CHECKOUT, would we want to fallback to lstat?\n\nWhen checking things out of the index, the index should be the\nsource of the truth. If something is not in the index, is there a\npoint in falling back to the workint tree state to decide if the\nthing should be checked out of the index?\n\n>>> But stepping back a bit,\n>>> such an application is likely marking selected few paths with the\n>>> attribute, and paths for which \"mode\" was \"unset\" are now given\n>>> these natural \"mode\"; it is inevitable to crash with such uses.\n> I'm confused. This does not match what I think is the current behavior\n> of my patch.\n> If \"mode\" was unset or removed for a path (meaning ' !mode' was\n> added to .gitattributes),\n> the code in my patch would respect that and not return the native mode.\n> It would return 'unspecified' or 'unset'.\n\nBut the usual practice is *not* to cover all paths with explicit\nattribute definition, isn't it? If somebody used the \"foo\"\nattribute in their project to decide certain paths are worth giving\nspecial treatments, there are paths with that attribute, perhaps\n(totally made up example):\n\n\t*.c\tfoo=yes\n\nNow, if you add a new \"built-in\" attribute next to 'mode' that\nassigns \"foo\" in attr.c:git_check_attr() the same way to those paths\nwhose value is still ATTR__UNKNOWN after collect_some_attrs returns,\nwouldn't a \"hello.c\" file get attribute 'foo' with value 'yes',\nwhile a \"hello.h\" file (not mentioned by .gitattributes) will get\nwhatever value the built-in logic computed for it? If that existing\nproject were using \"mode\" (instead of \"foo\"), then doesn't this patch\nchange the behaviour for them?\n\n>>> Again, this has one hole, I think. Paths that are not mentioned\n>>> (not even with \"!mode\") would come to the function as ATTR__UNKNOWN\n>>> and trigger the fallback behaviour, even when other paths are given\n>>> end-user specified \"mode\" attribute values.\n> What you are describing sounds correct/what I intended.\n> So are you saying that the expected behavior is actually:\n> If the user sets 'mode' for 1+ paths in the repo, then the native mode\n> fallback should\n> NOT be used for all paths in the repo?\n\nThat might be workable, but it breaks a well working system suddenly\nwhen somebody uses \"mode\" in their .gitattibutes and we do not even\nwarn about the breakage, which is not ideal.\n\nI think, when we see such an attribute is defined while reading from\n.gitattributes and friends, we would probably want to warn and\nignore their definition altogether and use *only* the computed\nvalue. This cannot be done in a backward compatible way, so it\nwould be better to carve out a namespace (and avoid overly generic\nword like \"mode\") for ourselves to define built-in attributes that\ndo not overlap with end-user defined attribute names.\n\nPerhaps call this 'builtin-object-mode' or something and document\nthat any attribute with a name that begins with 'builtin-' will\nalways get a computed value (possibly \"unset\"), it is an error\nto define such an attribute in .gitattributes system, or something?\n\n\n"},{"id":"484894","messageId":"20231114214934.3484892-1-jojwang@google.com","threadId":"60514","inReplyTo":"xmqqbkbx11x2.fsf@gitster.g","subject":"[PATCH 1/1] attr: add builtin objectmode values support","fromName":"Joanna Wang","fromEmail":"jojwang@google.com","sentAt":"2023-11-14T21:49:34Z","receivedAt":"2023-11-14T21:49:39Z","isPatch":true,"sender":{"key":"jojwang@google.com","avatar":"https://avatars.githubusercontent.com/u/120585822?v=4"},"body":"Gives all paths builtin objectmode values based on the paths' modes\n(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:builtin_objectmode=160000)' could filter for submodules without\nneeding to have `builtin_objectmode=160000` to be set in .gitattributes\nfor every submodule path.\n\nThese values are also reflected in `git check-attr` results.\nIf the git_attr_direction is set to GIT_ATTR_INDEX or GIT_ATTR_CHECKIN\nand a path is not found in the index, the value will be unspecified.\n\nThis patch also reserves the builtin_* attribute namespace for objectmode\nand any future builtin attributes. Any user defined attributes using this\nreserved namespace will result in a warning. This is a breaking change for\nany existing builtin_* attributes.\nPathspecs with some builtin_* attribute name (excluding builtin_objectmode)\nwill behave like any attribute where there are no user specified values.\n\nSigned-off-by: Joanna Wang \n---\n\n>But the usual practice is *not* to cover all paths with explicit\n> attribute definition, isn't it?\nGot it, the breaking change is that we're giving mode a value when\nuser may expect no value.\n\n>When checking things out of the index, the index should be the\n>source of the truth. If something is not in the index, is there a\n>point in falling back to the workint tree state to decide if the\n>thing should be checked out of the index?\nI don't think working tree objectmode fallback makes sense for\nGIT_ATTR_CHECKOUT and GIT_ATTR_INDEX. Updated patch implements\nwithout fallback.\n\n> static struct git_attr mode_attr;\n>\n> if (!mode_attr)\n> mode_attr = git_attr(\"mode\");\nIs there an idiomatic way to check if this git_attr (struct or pointer)\nhas been initialized? I could not find an anything for C but maybe I've\nmissed a common way to do this in the codebase.\n\nI also addressed the style nits. Thanks for the review/fedback so far. \n\n Documentation/gitattributes.txt | 14 +++++++\n attr.c | 67 +++++++++++++++++++++++++++++++--\n t/t0003-attributes.sh | 44 ++++++++++++++++++++++\n t/t6135-pathspec-with-attrs.sh | 25 ++++++++++++\n 4 files changed, 147 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 8c1793c148..84bad3e741 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -100,6 +100,20 @@ for a path to `Unspecified` state. This can be done by listing\n the name of the attribute prefixed with an exclamation point `!`.\n \n \n+RESERVED BUILTIN_* ATTRIBUTES\n+-----------------------------\n+\n+builtin_* is a reserved namespace for builtin attribute values. Any\n+user defined attributes under this namespace will result in a warning.\n+\n+`builtin_objectmode`\n+^^^^^^^^^^^^^^^^^^^^\n+This attribute is for filtering files by their file bit modes (40000,\n+120000, 160000, 100755, 100644). e.g. ':(attr:builtin_objectmode=160000)'.\n+You may also check these values with `git check-attr builtin_objectmode -- `.\n+If the object is not in the index `git check-attr --cached` will return unspecified.\n+\n+\n EFFECTS\n -------\n \ndiff --git a/attr.c b/attr.c\nindex e62876dfd3..8493f2c5c0 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -17,6 +17,7 @@\n #include \"utf8.h\"\n #include \"quote.h\"\n #include \"read-cache-ll.h\"\n+#include \"refs.h\"\n #include \"revision.h\"\n #include \"object-store-ll.h\"\n #include \"setup.h\"\n@@ -183,6 +184,15 @@ static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check)\n \t}\n }\n \n+/*\n+ * Atribute name cannot begin with \"builtin_\" which\n+ * is a reserved namespace for built in attributes values.\n+ */\n+static int attr_name_reserved(const char *name)\n+{\n+\treturn !strncmp(name, \"builtin_\", strlen(\"builtin_\"));\n+}\n+\n static int attr_name_valid(const char *name, size_t namelen)\n {\n \t/*\n@@ -315,7 +325,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,\n \t\t\tcp++;\n \t\t\tlen--;\n \t\t}\n-\t\tif (!attr_name_valid(cp, len)) {\n+\t\tif (!attr_name_valid(cp, len) || attr_name_reserved(cp)) {\n \t\t\treport_invalid_attr(cp, len, src, lineno);\n \t\t\treturn NULL;\n \t\t}\n@@ -379,7 +389,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t\tname += strlen(ATTRIBUTE_MACRO_PREFIX);\n \t\tname += strspn(name, blank);\n \t\tnamelen = strcspn(name, blank);\n-\t\tif (!attr_name_valid(name, namelen)) {\n+\t\tif (!attr_name_valid(name, namelen) || attr_name_reserved(name)) {\n \t\t\treport_invalid_attr(name, namelen, src, lineno);\n \t\t\tgoto fail_return;\n \t\t}\n@@ -1240,6 +1250,57 @@ static struct object_id *default_attr_source(void)\n \treturn &attr_source;\n }\n \n+static const char *builtin_object_mode_attr(struct index_state *istate, const char *path)\n+{\n+\tunsigned int mode;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\t\n+\tif (direction == GIT_ATTR_CHECKIN) {\n+\t\tstruct object_id oid;\n+\t\tstruct stat st;\n+\t\tif (lstat(path, &st))\n+\t\t\tdie_errno(_(\"unable to stat '%s'\"), path);\n+\t\tmode = canon_mode(st.st_mode);\n+\t\tif (S_ISDIR(mode)) {\n+\t\t\t/* `path` is either a directory or it is a submodule,\n+\t\t\t * in which case it is already indexed as submodule\n+\t\t\t * or it does not exist in the index yet and we need to\n+\t\t\t * check if we can resolve to a ref.\n+\t\t\t*/\n+\t\t\tint pos = index_name_pos(istate, path, strlen(path));\n+\t\t\tif (pos >= 0) {\n+\t\t\t\t if (S_ISGITLINK(istate->cache[pos]->ce_mode))\n+\t\t\t\t\t mode = istate->cache[pos]->ce_mode;\n+\t\t\t} else if (resolve_gitlink_ref(path, \"HEAD\", &oid) == 0)\n+\t\t\t\tmode = S_IFGITLINK;\n+\t\t}\n+\t} else {\n+\t\t/*\n+\t\t * For GIT_ATTR_CHECKOUT and GIT_ATTR_INDEX we only check\n+\t\t * for mode in the index.\n+\t\t */\n+\t\tint pos = index_name_pos(istate, path, strlen(path));\n+\t\tif (pos >= 0)\n+\t\t\tmode = istate->cache[pos]->ce_mode;\n+\t\telse \n+\t\t\treturn ATTR__UNSET;\n+\t}\n+\tstrbuf_addf(&sb, \"%06o\", mode);\n+\treturn sb.buf;\n+\t\n+}\n+\n+\n+static const char *compute_builtin_attr(struct index_state *istate,\n+\t\t\t\t\t const char *path,\n+\t\t\t\t\t const struct git_attr *attr) {\n+\tconst struct git_attr *object_mode_attr = git_attr(\"builtin_objectmode\");\n+\t\n+\tif (attr == object_mode_attr)\n+\t\treturn builtin_object_mode_attr(istate, path);\n+\treturn ATTR__UNSET;\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@@ -1253,7 +1314,7 @@ void git_check_attr(struct index_state *istate,\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\t\tvalue = compute_builtin_attr(istate, path, check->all_attrs[n].attr);\n \t\tcheck->items[i].value = value;\n \t}\n }\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex aee2298f01..25aa3fbd05 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -19,6 +19,16 @@ attr_check () {\n \ttest_must_be_empty err\n }\n \n+attr_check_object_mode () {\n+\tpath=\"$1\" &&\n+\texpect=\"$2\" &&\n+\tcheck_opts=\"$3\" &&\n+\tgit check-attr $check_opts builtin_objectmode -- \"$path\" >actual 2>err &&\n+\techo \"$path: builtin_objectmode: $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 +568,38 @@ test_expect_success EXPENSIVE 'large attributes file ignored in index' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'native object mode attributes work' '\n+\t>exec && chmod +x exec && attr_check_object_mode exec 100755 &&\n+\t>normal && attr_check_object_mode normal 100644 &&\n+\tmkdir dir && attr_check_object_mode dir 040000 &&\n+\t>to_sym ln -s to_sym sym && attr_check_object_mode sym 120000\n+'\n+\n+test_expect_success 'native object mode attributes work with --cached' '\n+\t>normal && attr_check_object_mode normal unspecified --cached &&\n+\tgit add normal && attr_check_object_mode normal 100644 --cached\n+'\n+\n+test_expect_success 'check object mode attributes work for submodules' '\n+\tmkdir sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tgit init &&\n+\t\tmv .git .real &&\n+\t\techo \"gitdir: .real\" >.git &&\n+\t\ttest_commit first\n+\t) &&\n+\tattr_check_object_mode sub 160000 &&\n+\tattr_check_object_mode sub unspecified --cached &&\n+\tgit add sub &&\n+\tattr_check_object_mode sub 160000 --cached\n+'\n+\n+test_expect_success 'we do not allow user defined builtin_* attributes' '\n+\techo \"foo* builtin_foo\" >.gitattributes &&\n+\tgit add .gitattributes 2>actual &&\n+\techo \"builtin_foo is not a valid attribute name: .gitattributes:1\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex a9c1e4e0ec..b08a32ea68 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -295,4 +295,29 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pathspec with builtin_objectmode attr can be used' '\n+\t>mode_exec_file_1 &&\n+\n+\tgit status -s \":(attr:builtin_objectmode=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:builtin_objectmode=100755)mode_exec_*\" >actual &&\n+\techo AM mode_exec_file_1 >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'builtin_objectmode 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:builtin_objectmode=100644)\" \"mode_1_*\" >actual &&\n+\techo ?? mode_1_exec >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit status -s \":(exclude,attr:builtin_objectmode=100755)\" \"mode_1_*\" >actual &&\n+\techo ?? mode_1_regular >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.43.0.rc0.421.g78406f8d94-goog\n\n"},{"id":"484956","messageId":"xmqqy1eytrns.fsf@gitster.g","threadId":"60514","inReplyTo":"20231114214934.3484892-1-jojwang@google.com","subject":"Re: [PATCH 1/1] attr: add builtin objectmode values support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-16T01:26:15Z","receivedAt":"2023-11-16T01:26:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joanna Wang writes:\n\n>> static struct git_attr mode_attr;\n>>\n>> if (!mode_attr)\n>> mode_attr = git_attr(\"mode\");\n> Is there an idiomatic way to check if this git_attr (struct or pointer)\n> has been initialized?\n\nSorry (I think the above is a typo from the \"to illustrate the idea\"\ncode snippet in my message), the static one should be a pointer to a\nstruct, as git_attr() is meant to \"intern\" the given name and return\na singleton instance.\n\nThe idiom is to see if it is still NULL and then call git_attr().\nOf course, you could repeatedly call git_attr() with the same name\nand get the same singleton instance, so without the \"have we\ninitialized?\" check, your code will still be correct, but it would\nincur a locked hashmap lookup (to ensure that we do not create the\nsecond instance of the same name), so...\n\n> +RESERVED BUILTIN_* ATTRIBUTES\n> +-----------------------------\n> +\n> +builtin_* is a reserved namespace for builtin attribute values. Any\n> +user defined attributes under this namespace will result in a warning.\n\nwarning and then? I think we would want to do \"warn and ignore\",\nnot \"warn and disable all the builtin_ computation\", i.e.,\n\n\t... will be ignored and a warning message is given.\n\n> +/*\n> + * Atribute name cannot begin with \"builtin_\" which\n> + * is a reserved namespace for built in attributes values.\n> + */\n> +static int attr_name_reserved(const char *name)\n> +{\n> +\treturn !strncmp(name, \"builtin_\", strlen(\"builtin_\"));\n> +}\n> +\n\nYou'd want to use starts_with() probably.\n\n> @@ -1240,6 +1250,57 @@ static struct object_id *default_attr_source(void)\n> \treturn &attr_source;\n> }\n> \n> +static const char *builtin_object_mode_attr(struct index_state *istate, const char *path)\n> +{\n> +\tunsigned int mode;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\t\n> +\tif (direction == GIT_ATTR_CHECKIN) {\n> +\t\tstruct object_id oid;\n> +\t\tstruct stat st;\n> +\t\tif (lstat(path, &st))\n> +\t\t\tdie_errno(_(\"unable to stat '%s'\"), path);\n> +\t\tmode = canon_mode(st.st_mode);\n> +\t\tif (S_ISDIR(mode)) {\n> +\t\t\t/* `path` is either a directory or it is a submodule,\n> +\t\t\t * in which case it is already indexed as submodule\n> +\t\t\t * or it does not exist in the index yet and we need to\n> +\t\t\t * check if we can resolve to a ref.\n> +\t\t\t*/\n\n\t/*\n\t * Our multi-line comment begins and ends with\n\t * slash-asterisk and asterisk-slash on their\n\t * own lines without anything else.\n\t */\n\n> +\t\t\tint pos = index_name_pos(istate, path, strlen(path));\n> +\t\t\tif (pos >= 0) {\n> +\t\t\t\t if (S_ISGITLINK(istate->cache[pos]->ce_mode))\n> +\t\t\t\t\t mode = istate->cache[pos]->ce_mode;\n> +\t\t\t} else if (resolve_gitlink_ref(path, \"HEAD\", &oid) == 0)\n> +\t\t\t\tmode = S_IFGITLINK;\n> +\t\t}\n\nGive {braces} around the else if block as the other side has it\n(Documentation/CodingGuidelines):\n\n\t- When there are multiple arms to a conditional and some of them\n\t require braces, enclose even a single line block in braces for\n\t consistency.\n\n> +\t} else {\n> +\t\t/*\n> +\t\t * For GIT_ATTR_CHECKOUT and GIT_ATTR_INDEX we only check\n> +\t\t * for mode in the index.\n> +\t\t */\n> +\t\tint pos = index_name_pos(istate, path, strlen(path));\n> +\t\tif (pos >= 0)\n> +\t\t\tmode = istate->cache[pos]->ce_mode;\n> +\t\telse \n> +\t\t\treturn ATTR__UNSET;\n> +\t}\n> +\tstrbuf_addf(&sb, \"%06o\", mode);\n> +\treturn sb.buf;\n> +\t\n\nThe struct itself will be reclaimed as it is on stack, and sb.buf is\ngiven back to the caller, so there technically is no leak, but I think\na more kosher way to do this involves the use of strbuf_detach().\n\nLet's lose the blank line at the end of the function that is not\nserving much useful purpose.\n\n> void git_check_attr(struct index_state *istate,\n> \t\t const char *path,\n> \t\t struct attr_check *check)\n> @@ -1253,7 +1314,7 @@ void git_check_attr(struct index_state *istate,\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\t\tvalue = compute_builtin_attr(istate, path, check->all_attrs[n].attr);\n> \t\tcheck->items[i].value = value;\n> \t}\n> }\n\nNicely done.\n\n> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\n> index aee2298f01..25aa3fbd05 100755\n> --- a/t/t0003-attributes.sh\n> +++ b/t/t0003-attributes.sh\n> @@ -19,6 +19,16 @@ attr_check () {\n> \ttest_must_be_empty err\n> }\n> \n> +attr_check_object_mode () {\n> +\tpath=\"$1\" &&\n> +\texpect=\"$2\" &&\n> +\tcheck_opts=\"$3\" &&\n> +\tgit check-attr $check_opts builtin_objectmode -- \"$path\" >actual 2>err &&\n> +\techo \"$path: builtin_objectmode: $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 +568,38 @@ test_expect_success EXPENSIVE 'large attributes file ignored in index' '\n> \ttest_cmp expect err\n> '\n> \n> +test_expect_success 'native object mode attributes work' '\n\n> +\t>exec && chmod +x exec && attr_check_object_mode exec 100755 &&\n> +\t>normal && attr_check_object_mode normal 100644 &&\n> +\tmkdir dir && attr_check_object_mode dir 040000 &&\n\nJust a style thing, but we tend to write one statement on each line.\n\n> +\t>to_sym ln -s to_sym sym && attr_check_object_mode sym 120000\n> +'\n\nYou do not need to_sym file, do you? Symbolic links can be left\ndangling.\n\nIn any case, the above test would not work on a filesystem without\nnative executable bit support. Also, you cannot do \"ln -s\" on\ncertain filesystems. It is a good idea to split the above into\nthree tests, one that is run unconditionally (for \"normal\" and\n\"dir\"), another that is run under the POSIXPERM prerequisite, and\nthe third that is run under the SYNLINKS prerequisite.\n\n> +test_expect_success 'native object mode attributes work with --cached' '\n> +\t>normal && attr_check_object_mode normal unspecified --cached &&\n> +\tgit add normal && attr_check_object_mode normal 100644 --cached\n> +'\n\nFor \"--cached test\", on the other hand, we should be able to set the\nexecutable bit or record a symbolic link regardless of the\nfilesystem using \"update-index\", e.g.,\n\n\t>normal && git add normal &&\n\tempty_blob=$(git rev-parse :normal)\n\tcat <<-EOF | git update-index --index-info\n\t100755 $empty_blob 0\texec\n\t120000 $empty_blob 0\tsymlink\n\tEOF\n\tattr_check_object_mode normal 100644 --cached &&\n\tattr_check_object_mode exec 100755 --cached &&\n\tattr_check_object_mode symlink 120000 --cached\n\nor something.\n\n> +test_expect_success 'check object mode attributes work for submodules' '\n> +\tmkdir sub &&\n> +\t(\n> +\t\tcd sub &&\n> +\t\tgit init &&\n> +\t\tmv .git .real &&\n> +\t\techo \"gitdir: .real\" >.git &&\n> +\t\ttest_commit first\n> +\t) &&\n> +\tattr_check_object_mode sub 160000 &&\n> +\tattr_check_object_mode sub unspecified --cached &&\n> +\tgit add sub &&\n> +\tattr_check_object_mode sub 160000 --cached\n> +'\n\nSounds good.\n\n> +test_expect_success 'we do not allow user defined builtin_* attributes' '\n> +\techo \"foo* builtin_foo\" >.gitattributes &&\n> +\tgit add .gitattributes 2>actual &&\n> +\techo \"builtin_foo is not a valid attribute name: .gitattributes:1\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n\nAnother thing that is worth checking is what happens when there is an\nconflicting entry:\n\n\techo \"foo builtin_objectmode=200\" >.gitattributes &&\n\t>foo &&\n\tattr_check_object_mode foo 100644\n\n> test_done\n> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\n> index a9c1e4e0ec..b08a32ea68 100755\n> --- a/t/t6135-pathspec-with-attrs.sh\n> +++ b/t/t6135-pathspec-with-attrs.sh\n> @@ -295,4 +295,29 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'pathspec with builtin_objectmode attr can be used' '\n> +\t>mode_exec_file_1 &&\n> +\n> +\tgit status -s \":(attr:builtin_objectmode=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:builtin_objectmode=100755)mode_exec_*\" >actual &&\n> +\techo AM mode_exec_file_1 >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'builtin_objectmode 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:builtin_objectmode=100644)\" \"mode_1_*\" >actual &&\n> +\techo ?? mode_1_exec >expect &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tgit status -s \":(exclude,attr:builtin_objectmode=100755)\" \"mode_1_*\" >actual &&\n> +\techo ?? mode_1_regular >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"484957","messageId":"CAPig+cSyrD71NMnfVCTHEf+K8vop9QHLet7uyOerrq=v9SVbFw@mail.gmail.com","threadId":"60514","inReplyTo":"xmqqy1eytrns.fsf@gitster.g","subject":"Re: [PATCH 1/1] attr: add builtin objectmode values support","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-11-16T01:37:11Z","receivedAt":"2023-11-16T01:37:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 15, 2023 at 8:26 PM Junio C Hamano wrote:\n> Joanna Wang writes:\n> > +test_expect_success 'native object mode attributes work with --cached' '\n> > + >normal && attr_check_object_mode normal unspecified --cached &&\n> > + git add normal && attr_check_object_mode normal 100644 --cached\n> > +'\n>\n> For \"--cached test\", on the other hand, we should be able to set the\n> executable bit or record a symbolic link regardless of the\n> filesystem using \"update-index\", e.g.,\n>\n> empty_blob=$(git rev-parse :normal)\n> cat <<-EOF | git update-index --index-info\n> 100755 $empty_blob 0 exec\n> 120000 $empty_blob 0 symlink\n> EOF\n>\n> or something.\n\nA bit more idiomatic in this codebase would be:\n\n git update-index --index-info <<-EOF\n 100755 $empty_blob 0 exec\n 120000 $empty_blob 0 symlink\n EOF\n\nNo need for `cat`.\n"},{"id":"484958","messageId":"xmqqttpmtnn5.fsf@gitster.g","threadId":"60514","inReplyTo":"CAPig+cSyrD71NMnfVCTHEf+K8vop9QHLet7uyOerrq=v9SVbFw@mail.gmail.com","subject":"Re: [PATCH 1/1] attr: add builtin objectmode values support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-16T02:53:02Z","receivedAt":"2023-11-16T02:53:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> A bit more idiomatic in this codebase would be:\n>\n> git update-index --index-info <<-EOF\n> 100755 $empty_blob 0 exec\n> 120000 $empty_blob 0 symlink\n> EOF\n>\n> No need for `cat`.\n\nAye, of course. We should not cat a single-source datastream into a\npipe. Thanks for correcting me.\n"},{"id":"484965","messageId":"20231116054437.2343549-1-jojwang@google.com","threadId":"60514","inReplyTo":"xmqqttpmtnn5.fsf@gitster.g","subject":"[PATCH 1/1] attr: add builtin objectmode values support","fromName":"Joanna Wang","fromEmail":"jojwang@google.com","sentAt":"2023-11-16T05:44:37Z","receivedAt":"2023-11-16T05:44:44Z","isPatch":true,"sender":{"key":"jojwang@google.com","avatar":"https://avatars.githubusercontent.com/u/120585822?v=4"},"body":"Gives all paths builtin objectmode values based on the paths' modes\n(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:builtin_objectmode=160000)' could filter for submodules without\nneeding to have `builtin_objectmode=160000` to be set in .gitattributes\nfor every submodule path.\n\nThese values are also reflected in `git check-attr` results.\nIf the git_attr_direction is set to GIT_ATTR_INDEX or GIT_ATTR_CHECKIN\nand a path is not found in the index, the value will be unspecified.\n\nThis patch also reserves the builtin_* attribute namespace for objectmode\nand any future builtin attributes. Any user defined attributes using this\nreserved namespace will result in a warning. This is a breaking change for\nany existing builtin_* attributes.\nPathspecs with some builtin_* attribute name (excluding builtin_objectmode)\nwill behave like any attribute where there are no user specified values.\n\nSigned-off-by: Joanna Wang \n---\n\n>The idiom is to see if it is still NULL and then call git_attr().\nGot it, thank you for explaining again.\n\n>We need a precondition here:\n>test_expect_success SYMLINKS\nTorsten pointed this out in a previous version,\nsorry i missed this until Junio pointed it out again.\n\nI've addressed the all the latest feedback in this update.\n\n Documentation/gitattributes.txt | 15 +\n attr.c | 71 ++++-\n t/#t1700-split-index.sh# | 547 ++++++++++++++++++++++++++++++++\n t/t0003-attributes.sh | 75 +++++\n t/t6135-pathspec-with-attrs.sh | 25 ++\n 5 files changed, 730 insertions(+), 3 deletions(-)\n create mode 100755 t/#t1700-split-index.sh#\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 8c1793c148..784aa9d4de 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -100,6 +100,21 @@ for a path to `Unspecified` state. This can be done by listing\n the name of the attribute prefixed with an exclamation point `!`.\n \n \n+RESERVED BUILTIN_* ATTRIBUTES\n+-----------------------------\n+\n+builtin_* is a reserved namespace for builtin attribute values. Any\n+user defined attributes under this namespace will be ignored and\n+trigger a warning.\n+\n+`builtin_objectmode`\n+^^^^^^^^^^^^^^^^^^^^\n+This attribute is for filtering files by their file bit modes (40000,\n+120000, 160000, 100755, 100644). e.g. ':(attr:builtin_objectmode=160000)'.\n+You may also check these values with `git check-attr builtin_objectmode -- `.\n+If the object is not in the index `git check-attr --cached` will return unspecified.\n+\n+\n EFFECTS\n -------\n \ndiff --git a/attr.c b/attr.c\nindex e62876dfd3..f8a94aeb23 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -17,6 +17,7 @@\n #include \"utf8.h\"\n #include \"quote.h\"\n #include \"read-cache-ll.h\"\n+#include \"refs.h\"\n #include \"revision.h\"\n #include \"object-store-ll.h\"\n #include \"setup.h\"\n@@ -183,6 +184,15 @@ static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check)\n \t}\n }\n \n+/*\n+ * Atribute name cannot begin with \"builtin_\" which\n+ * is a reserved namespace for built in attributes values.\n+ */\n+static int attr_name_reserved(const char *name)\n+{\n+\treturn starts_with(name, \"builtin_\");\n+}\n+\n static int attr_name_valid(const char *name, size_t namelen)\n {\n \t/*\n@@ -315,7 +325,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,\n \t\t\tcp++;\n \t\t\tlen--;\n \t\t}\n-\t\tif (!attr_name_valid(cp, len)) {\n+\t\tif (!attr_name_valid(cp, len) || attr_name_reserved(cp)) {\n \t\t\treport_invalid_attr(cp, len, src, lineno);\n \t\t\treturn NULL;\n \t\t}\n@@ -379,7 +389,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t\tname += strlen(ATTRIBUTE_MACRO_PREFIX);\n \t\tname += strspn(name, blank);\n \t\tnamelen = strcspn(name, blank);\n-\t\tif (!attr_name_valid(name, namelen)) {\n+\t\tif (!attr_name_valid(name, namelen) || attr_name_reserved(name)) {\n \t\t\treport_invalid_attr(name, namelen, src, lineno);\n \t\t\tgoto fail_return;\n \t\t}\n@@ -1240,6 +1250,61 @@ static struct object_id *default_attr_source(void)\n \treturn &attr_source;\n }\n \n+static const char *builtin_object_mode_attr(struct index_state *istate, const char *path)\n+{\n+\tunsigned int mode;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\t\n+\tif (direction == GIT_ATTR_CHECKIN) {\n+\t\tstruct object_id oid;\n+\t\tstruct stat st;\n+\t\tif (lstat(path, &st))\n+\t\t\tdie_errno(_(\"unable to stat '%s'\"), path);\n+\t\tmode = canon_mode(st.st_mode);\n+\t\tif (S_ISDIR(mode)) {\n+\t\t\t/*\n+\t\t\t *`path` is either a directory or it is a submodule,\n+\t\t\t * in which case it is already indexed as submodule\n+\t\t\t * or it does not exist in the index yet and we need to\n+\t\t\t * check if we can resolve to a ref.\n+\t\t\t*/\n+\t\t\tint pos = index_name_pos(istate, path, strlen(path));\n+\t\t\tif (pos >= 0) {\n+\t\t\t\t if (S_ISGITLINK(istate->cache[pos]->ce_mode))\n+\t\t\t\t\t mode = istate->cache[pos]->ce_mode;\n+\t\t\t} else if (resolve_gitlink_ref(path, \"HEAD\", &oid) == 0) {\n+\t\t\t\tmode = S_IFGITLINK;\n+\t\t\t}\n+\t\t}\n+\t} else {\n+\t\t/*\n+\t\t * For GIT_ATTR_CHECKOUT and GIT_ATTR_INDEX we only check\n+\t\t * for mode in the index.\n+\t\t */\n+\t\tint pos = index_name_pos(istate, path, strlen(path));\n+\t\tif (pos >= 0)\n+\t\t\tmode = istate->cache[pos]->ce_mode;\n+\t\telse \n+\t\t\treturn ATTR__UNSET;\n+\t}\n+\tstrbuf_addf(&sb, \"%06o\", mode);\n+\treturn strbuf_detach(&sb, NULL);\n+}\n+\n+\n+static const char *compute_builtin_attr(struct index_state *istate,\n+\t\t\t\t\t const char *path,\n+\t\t\t\t\t const struct git_attr *attr) {\n+\tstatic const struct git_attr *object_mode_attr;\n+\n+\tif (!object_mode_attr)\n+\t\tobject_mode_attr = git_attr(\"builtin_objectmode\");\n+\t\n+\tif (attr == object_mode_attr)\n+\t\treturn builtin_object_mode_attr(istate, path);\n+\treturn ATTR__UNSET;\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@@ -1253,7 +1318,7 @@ void git_check_attr(struct index_state *istate,\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\t\tvalue = compute_builtin_attr(istate, path, check->all_attrs[n].attr);\n \t\tcheck->items[i].value = value;\n \t}\n }\ndiff --git a/t/#t1700-split-index.sh# b/t/#t1700-split-index.sh#\nnew file mode 100755\nindex 0000000000..a7b7263b35\n--- /dev/null\n+++ b/t/#t1700-split-index.sh#\n@@ -0,0 +1,547 @@\n+#!/bin/sh\n+\n+test_description='split index mode tests'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./test-lib.sh\n+\n+# We need total control of index splitting here\n+sane_unset GIT_TEST_SPLIT_INDEX\n+\n+# Testing a hard coded SHA against an index with an extension\n+# that can vary from run to run is problematic so we disable\n+# those extensions.\n+sane_unset GIT_TEST_FSMONITOR\n+sane_unset GIT_TEST_INDEX_THREADS\n+\n+# Create a file named as $1 with content read from stdin.\n+# Set the file's mtime to a few seconds in the past to avoid racy situations.\n+create_non_racy_file () {\n+\tcat >\"$1\" &&\n+\ttest-tool chmtime =-5 \"$1\"\n+}\n+\n+test_expect_success 'setup' '\n+\ttest_oid_cache <<-EOF\n+\town_v3 sha1:8299b0bcd1ac364e5f1d7768efb62fa2da79a339\n+\town_v3 sha256:38a6d2925e3eceec33ad7b34cbff4e0086caa0daf28f31e51f5bd94b4a7af86b\n+\n+\tbase_v3 sha1:39d890139ee5356c7ef572216cebcd27aa41f9df\n+\tbase_v3 sha256:c9baeadf905112bf6c17aefbd7d02267afd70ded613c30cafed2d40cb506e1ed\n+\n+\town_v4 sha1:432ef4b63f32193984f339431fd50ca796493569\n+\town_v4 sha256:6738ac6319c25b694afa7bcc313deb182d1a59b68bf7a47b4296de83478c0420\n+\n+\tbase_v4 sha1:508851a7f0dfa8691e9f69c7f055865389012491\n+\tbase_v4 sha256:3177d4adfdd4b6904f7e921d91d715a471c0dde7cf6a4bba574927f02b699508\n+\tEOF\n+'\n+\n+test_expect_success 'enable split index' '\n+\tgit config splitIndex.maxPercentChange 100 &&\n+\tgit update-index --split-index &&\n+\ttest-tool dump-split-index .git/index >actual &&\n+\tindexversion=$(git update-index --show-index-version) &&\n+\n+\t# NEEDSWORK: Stop hard-coding checksums.\n+\tif test \"$indexversion\" = \"4\"\n+\tthen\n+\t\town=$(test_oid own_v4) &&\n+\t\tbase=$(test_oid base_v4)\n+\telse\n+\t\town=$(test_oid own_v3) &&\n+\t\tbase=$(test_oid base_v3)\n+\tfi &&\n+\n+\tcat >expect <<-EOF &&\n+\town $own\n+\tbase $base\n+\treplacements:\n+\tdeletions:\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add one file' '\n+\tcreate_non_racy_file one &&\n+\tgit update-index --add one &&\n+\tgit ls-files --stage >ls-files.actual &&\n+\tcat >ls-files.expect <<-EOF &&\n+\t100644 $EMPTY_BLOB 0\tone\n+\tEOF\n+\ttest_cmp ls-files.expect ls-files.actual &&\n+\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <<-EOF &&\n+\tbase $base\n+\t100644 $EMPTY_BLOB 0\tone\n+\treplacements:\n+\tdeletions:\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'disable split index' '\n+\tgit update-index --no-split-index &&\n+\tgit ls-files --stage >ls-files.actual &&\n+\tcat >ls-files.expect <<-EOF &&\n+\t100644 $EMPTY_BLOB 0\tone\n+\tEOF\n+\ttest_cmp ls-files.expect ls-files.actual &&\n+\n+\tBASE=$(test-tool dump-split-index .git/index | sed -n \"s/^own/base/p\") &&\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <<-EOF &&\n+\tnot a split index\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'enable split index again, \"one\" now belongs to base index\"' '\n+\tgit update-index --split-index &&\n+\tgit ls-files --stage >ls-files.actual &&\n+\tcat >ls-files.expect <<-EOF &&\n+\t100644 $EMPTY_BLOB 0\tone\n+\tEOF\n+\ttest_cmp ls-files.expect ls-files.actual &&\n+\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <<-EOF &&\n+\t$BASE\n+\treplacements:\n+\tdeletions:\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'modify original file, base index untouched' '\n+\techo modified | create_non_racy_file one &&\n+\tfile1_blob=$(git hash-object one) &&\n+\tgit update-index one &&\n+\tgit ls-files --stage >ls-files.actual &&\n+\tcat >ls-files.expect <<-EOF &&\n+\t100644 $file1_blob 0\tone\n+\tEOF\n+\ttest_cmp ls-files.expect ls-files.actual &&\n+\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tq_to_tab >expect <<-EOF &&\n+\t$BASE\n+\t100644 $file1_blob 0Q\n+\treplacements: 0\n+\tdeletions:\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add another file, which stays index' '\n+\tcreate_non_racy_file two &&\n+\tgit update-index --add two &&\n+\tgit ls-files --stage >ls-files.actual &&\n+\tcat >ls-files.expect <<-EOF &&\n+\t100644 $file1_blob 0\tone\n+\t100644 $EMPTY_BLOB 0\ttwo\n+\tEOF\n+\ttest_cmp ls-files.expect ls-files.actual &&\n+\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tq_to_tab >expect <<-EOF &&\n+\t$BASE\n+\t100644 $file1_blob 0Q\n+\t100644 $EMPTY_BLOB 0\ttwo\n+\treplacements: 0\n+\tdeletions:\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'remove file not in base index' '\n+\tgit update-index --force-remove two &&\n+\tgit ls-files --stage >ls-files.actual &&\n+\tcat >ls-files.expect <<-EOF &&\n+\t100644 $file1_blob 0\tone\n+\tEOF\n+\ttest_cmp ls-files.expect ls-files.actual &&\n+\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tq_to_tab >expect <<-EOF &&\n+\t$BASE\n+\t100644 $file1_blob 0Q\n+\treplacements: 0\n+\tdeletions:\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'remove file in base index' '\n+\tgit update-index --force-remove one &&\n+\tgit ls-files --stage >ls-files.actual &&\n+\ttest_must_be_empty ls-files.actual &&\n+\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <<-EOF &&\n+\t$BASE\n+\treplacements:\n+\tdeletions: 0\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add original file back' '\n+\tcreate_non_racy_file one &&\n+\tgit update-index --add one &&\n+\tgit ls-files --stage >ls-files.actual &&\n+\tcat >ls-files.expect <<-EOF &&\n+\t100644 $EMPTY_BLOB 0\tone\n+\tEOF\n+\ttest_cmp ls-files.expect ls-files.actual &&\n+\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <<-EOF &&\n+\t$BASE\n+\t100644 $EMPTY_BLOB 0\tone\n+\treplacements:\n+\tdeletions: 0\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add new file' '\n+\tcreate_non_racy_file two &&\n+\tgit update-index --add two &&\n+\tgit ls-files --stage >actual &&\n+\tcat >expect <<-EOF &&\n+\t100644 $EMPTY_BLOB 0\tone\n+\t100644 $EMPTY_BLOB 0\ttwo\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'unify index, two files remain' '\n+\tgit update-index --no-split-index &&\n+\tgit ls-files --stage >ls-files.actual &&\n+\tcat >ls-files.expect <<-EOF &&\n+\t100644 $EMPTY_BLOB 0\tone\n+\t100644 $EMPTY_BLOB 0\ttwo\n+\tEOF\n+\ttest_cmp ls-files.expect ls-files.actual &&\n+\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <<-EOF &&\n+\tnot a split index\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rev-parse --shared-index-path' '\n+\ttest_create_repo split-index &&\n+\t(\n+\t\tcd split-index &&\n+\t\tgit update-index --split-index &&\n+\t\techo .git/sharedindex* >expect &&\n+\t\tgit rev-parse --shared-index-path >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tmkdir subdirectory &&\n+\t\tcd subdirectory &&\n+\t\techo ../.git/sharedindex* >expect &&\n+\t\tgit rev-parse --shared-index-path >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'set core.splitIndex config variable to true' '\n+\tgit config core.splitIndex true &&\n+\tcreate_non_racy_file three &&\n+\tgit update-index --add three &&\n+\tgit ls-files --stage >ls-files.actual &&\n+\tcat >ls-files.expect <<-EOF &&\n+\t100644 $EMPTY_BLOB 0\tone\n+\t100644 $EMPTY_BLOB 0\tthree\n+\t100644 $EMPTY_BLOB 0\ttwo\n+\tEOF\n+\ttest_cmp ls-files.expect ls-files.actual &&\n+\tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <<-EOF &&\n+\t$BASE\n+\treplacements:\n+\tdeletions:\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'set core.splitIndex config variable to false' '\n+\tgit config core.splitIndex false &&\n+\tgit update-index --force-remove three &&\n+\tgit ls-files --stage >ls-files.actual &&\n+\tcat >ls-files.expect <<-EOF &&\n+\t100644 $EMPTY_BLOB 0\tone\n+\t100644 $EMPTY_BLOB 0\ttwo\n+\tEOF\n+\ttest_cmp ls-files.expect ls-files.actual &&\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <<-EOF &&\n+\tnot a split index\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'set core.splitIndex config variable back to true' '\n+\tgit config core.splitIndex true &&\n+\tcreate_non_racy_file three &&\n+\tgit update-index --add three &&\n+\tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <<-EOF &&\n+\t$BASE\n+\treplacements:\n+\tdeletions:\n+\tEOF\n+\ttest_cmp expect actual &&\n+\tcreate_non_racy_file four &&\n+\tgit update-index --add four &&\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <<-EOF &&\n+\t$BASE\n+\t100644 $EMPTY_BLOB 0\tfour\n+\treplacements:\n+\tdeletions:\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check behavior with splitIndex.maxPercentChange unset' '\n+\tgit config --unset splitIndex.maxPercentChange &&\n+\tcreate_non_racy_file five &&\n+\tgit update-index --add five &&\n+\tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <<-EOF &&\n+\t$BASE\n+\treplacements:\n+\tdeletions:\n+\tEOF\n+\ttest_cmp expect actual &&\n+\tcreate_non_racy_file six &&\n+\tgit update-index --add six &&\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <<-EOF &&\n+\t$BASE\n+\t100644 $EMPTY_BLOB 0\tsix\n+\treplacements:\n+\tdeletions:\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check splitIndex.maxPercentChange set to 0' '\n+\tgit config splitIndex.maxPercentChange 0 &&\n+\tcreate_non_racy_file seven &&\n+\tgit update-index --add seven &&\n+\tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <<-EOF &&\n+\t$BASE\n+\treplacements:\n+\tdeletions:\n+\tEOF\n+\ttest_cmp expect actual &&\n+\tcreate_non_racy_file eight &&\n+\tgit update-index --add eight &&\n+\tBASE=$(test-tool dump-split-index .git/index | grep \"^base\") &&\n+\ttest-tool dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <<-EOF &&\n+\t$BASE\n+\treplacements:\n+\tdeletions:\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'shared index files expire after 2 weeks by default' '\n+\tcreate_non_racy_file ten &&\n+\tgit update-index --add ten &&\n+\ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n+\tjust_under_2_weeks_ago=$((5-14*86400)) &&\n+\ttest-tool chmtime =$just_under_2_weeks_ago .git/sharedindex.* &&\n+\tcreate_non_racy_file eleven &&\n+\tgit update-index --add eleven &&\n+\ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n+\tjust_over_2_weeks_ago=$((-1-14*86400)) &&\n+\ttest-tool chmtime =$just_over_2_weeks_ago .git/sharedindex.* &&\n+\tcreate_non_racy_file twelve &&\n+\tgit update-index --add twelve &&\n+\ttest $(ls .git/sharedindex.* | wc -l) -le 2\n+'\n+\n+test_expect_success 'check splitIndex.sharedIndexExpire set to 16 days' '\n+\tgit config splitIndex.sharedIndexExpire \"16.days.ago\" &&\n+\ttest-tool chmtime =$just_over_2_weeks_ago .git/sharedindex.* &&\n+\tcreate_non_racy_file thirteen &&\n+\tgit update-index --add thirteen &&\n+\ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n+\tjust_over_16_days_ago=$((-1-16*86400)) &&\n+\ttest-tool chmtime =$just_over_16_days_ago .git/sharedindex.* &&\n+\tcreate_non_racy_file fourteen &&\n+\tgit update-index --add fourteen &&\n+\ttest $(ls .git/sharedindex.* | wc -l) -le 2\n+'\n+\n+test_expect_success 'check splitIndex.sharedIndexExpire set to \"never\" and \"now\"' '\n+\tgit config splitIndex.sharedIndexExpire never &&\n+\tjust_10_years_ago=$((-365*10*86400)) &&\n+\ttest-tool chmtime =$just_10_years_ago .git/sharedindex.* &&\n+\tcreate_non_racy_file fifteen &&\n+\tgit update-index --add fifteen &&\n+\ttest $(ls .git/sharedindex.* | wc -l) -gt 2 &&\n+\tgit config splitIndex.sharedIndexExpire now &&\n+\tjust_1_second_ago=-1 &&\n+\ttest-tool chmtime =$just_1_second_ago .git/sharedindex.* &&\n+\tcreate_non_racy_file sixteen &&\n+\tgit update-index --add sixteen &&\n+\ttest $(ls .git/sharedindex.* | wc -l) -le 2\n+'\n+\n+test_expect_success POSIXPERM 'same mode for index & split index' '\n+\tgit init same-mode &&\n+\t(\n+\t\tcd same-mode &&\n+\t\ttest_commit A &&\n+\t\ttest_modebits .git/index >index_mode &&\n+\t\ttest_must_fail git config core.sharedRepository &&\n+\t\tgit -c core.splitIndex=true status &&\n+\t\tshared=$(ls .git/sharedindex.*) &&\n+\t\tcase \"$shared\" in\n+\t\t*\" \"*)\n+\t\t\t# we have more than one???\n+\t\t\tfalse ;;\n+\t\t*)\n+\t\t\ttest_modebits \"$shared\" >split_index_mode &&\n+\t\t\ttest_cmp index_mode split_index_mode ;;\n+\t\tesac\n+\t)\n+'\n+\n+while read -r mode modebits\n+do\n+\ttest_expect_success POSIXPERM \"split index respects core.sharedrepository $mode\" '\n+\t\t# Remove existing shared index files\n+\t\tgit config core.splitIndex false &&\n+\t\tgit update-index --force-remove one &&\n+\t\trm -f .git/sharedindex.* &&\n+\t\t# Create one new shared index file\n+\t\tgit config core.sharedrepository \"$mode\" &&\n+\t\tgit config core.splitIndex true &&\n+\t\tcreate_non_racy_file one &&\n+\t\tgit update-index --add one &&\n+\t\techo \"$modebits\" >expect &&\n+\t\ttest_modebits .git/index >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tshared=$(ls .git/sharedindex.*) &&\n+\t\tcase \"$shared\" in\n+\t\t*\" \"*)\n+\t\t\t# we have more than one???\n+\t\t\tfalse ;;\n+\t\t*)\n+\t\t\ttest_modebits \"$shared\" >actual &&\n+\t\t\ttest_cmp expect actual ;;\n+\t\tesac\n+\t'\n+done <<\\EOF\n+0666 -rw-rw-rw-\n+0642 -rw-r---w-\n+EOF\n+\n+test_expect_success POSIXPERM,SANITY 'graceful handling when splitting index is not allowed' '\n+\ttest_create_repo ro &&\n+\t(\n+\t\tcd ro &&\n+\t\ttest_commit initial &&\n+\t\tgit update-index --split-index &&\n+\t\ttest -f .git/sharedindex.*\n+\t) &&\n+\tcp ro/.git/index new-index &&\n+\ttest_when_finished \"chmod u+w ro/.git\" &&\n+\tchmod u-w ro/.git &&\n+\tGIT_INDEX_FILE=\"$(pwd)/new-index\" git -C ro update-index --split-index &&\n+\tchmod u+w ro/.git &&\n+\trm ro/.git/sharedindex.* &&\n+\tGIT_INDEX_FILE=new-index git ls-files >actual &&\n+\techo initial.t >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'writing split index with null sha1 does not write cache tree' '\n+\tgit config core.splitIndex true &&\n+\tgit config splitIndex.maxPercentChange 0 &&\n+\tgit commit -m \"commit\" &&\n+\t{\n+\t\tgit ls-tree HEAD &&\n+\t\tprintf \"160000 commit $ZERO_OID\\\\tbroken\\\\n\"\n+\t} >broken-tree &&\n+\techo \"add broken entry\" >msg &&\n+\n+\ttree=$(git mktree cache-tree.out &&\n+\ttest_line_count = 0 cache-tree.out\n+'\n+\n+test_expect_success 'do not refresh null base index' '\n+\ttest_create_repo merge &&\n+\t(\n+\t\tcd merge &&\n+\t\ttest_commit initial &&\n+\t\tgit checkout -b side-branch &&\n+\t\ttest_commit extra &&\n+\t\tgit checkout main &&\n+\t\tgit update-index --split-index &&\n+\t\ttest_commit more &&\n+\t\t# must not write a new shareindex, or we wont catch the problem\n+\t\tgit -c splitIndex.maxPercentChange=100 merge --no-edit side-branch 2>err &&\n+\t\t# i.e. do not expect warnings like\n+\t\t# could not freshen shared index .../shareindex.00000...\n+\t\ttest_must_be_empty err\n+\t)\n+'\n+\n+test_expect_success 'reading split index at alternate location' '\n+\tgit init reading-alternate-location &&\n+\t(\n+\t\tcd reading-alternate-location &&\n+\t\t>file-in-alternate &&\n+\t\tgit update-index --split-index --add file-in-alternate\n+\t) &&\n+\techo file-in-alternate >expect &&\n+\n+\t# Should be able to find the shared index both right next to\n+\t# the specified split index file ...\n+\tGIT_INDEX_FILE=./reading-alternate-location/.git/index \\\n+\tgit ls-files --cached >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# ... and, for backwards compatibility, in the current GIT_DIR\n+\t# as well.\n+\tmv -v ./reading-alternate-location/.git/sharedindex.* .git &&\n+\tGIT_INDEX_FILE=./reading-alternate-location/.git/index \\\n+\tgit ls-files --cached >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'GIT_TEST_SPLIT_INDEX works' '\n+\tgit init git-test-split-index &&\n+\t(\n+\t\tcd git-test-split-index &&\n+\t\t>file &&\n+\t\tGIT_TEST_SPLIT_INDEX=1 git update-index --add file &&\n+\t\tls -l .git/sharedindex.* >actual &&\n+\t\ttest_line_count = 1 actual\n+\t)\n+'\n+\n+test_done\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex aee2298f01..86f8681570 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -19,6 +19,20 @@ attr_check () {\n \ttest_must_be_empty err\n }\n \n+attr_check_object_mode_basic () {\n+\tpath=\"$1\" &&\n+\texpect=\"$2\" &&\n+\tcheck_opts=\"$3\" &&\n+\tgit check-attr $check_opts builtin_objectmode -- \"$path\" >actual 2>err &&\n+\techo \"$path: builtin_objectmode: $expect\" >expect &&\n+\ttest_cmp expect actual\n+}\n+\n+attr_check_object_mode () {\n+\tattr_check_object_mode_basic \"$@\" &&\n+\ttest_must_be_empty err\n+}\n+\n attr_check_quote () {\n \tpath=\"$1\" quoted_path=\"$2\" expect=\"$3\" &&\n \n@@ -558,4 +572,65 @@ test_expect_success EXPENSIVE 'large attributes file ignored in index' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'builtin object mode attributes work (dir and regular paths)' '\n+\t>normal &&\n+\tattr_check_object_mode normal 100644 &&\n+\tmkdir dir &&\n+\tattr_check_object_mode dir 040000\n+'\n+\n+test_expect_success POSIXPERM 'builtin object mode attributes work (executable)' '\n+\t>exec && chmod +x exec &&\n+\tattr_check_object_mode exec 100755\n+'\n+\n+test_expect_success SYMLINKS 'builtin object mode attributes work (symlinks)' '\n+\t>to_sym ln -s to_sym sym &&\n+\tattr_check_object_mode sym 120000\n+'\n+\n+test_expect_success 'native object mode attributes work with --cached' '\n+\t>normal &&\n+\tgit add normal &&\n+\tempty_blob=$(git rev-parse :normal) &&\n+\tgit update-index --index-info <<-EOF &&\n+\t100755 $empty_blob 0\texec\n+\t120000 $empty_blob 0\tsymlink\n+\tEOF\n+\tattr_check_object_mode normal 100644 --cached &&\n+\tattr_check_object_mode exec 100755 --cached &&\n+\tattr_check_object_mode symlink 120000 --cached\n+'\n+\n+test_expect_success 'check object mode attributes work for submodules' '\n+\tmkdir sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tgit init &&\n+\t\tmv .git .real &&\n+\t\techo \"gitdir: .real\" >.git &&\n+\t\ttest_commit first\n+\t) &&\n+\tattr_check_object_mode sub 160000 &&\n+\tattr_check_object_mode sub unspecified --cached &&\n+\tgit add sub &&\n+\tattr_check_object_mode sub 160000 --cached\n+'\n+\n+test_expect_success 'we do not allow user defined builtin_* attributes' '\n+\techo \"foo* builtin_foo\" >.gitattributes &&\n+\tgit add .gitattributes 2>actual &&\n+\techo \"builtin_foo is not a valid attribute name: .gitattributes:1\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'user defined builtin_objectmode values are ignored' '\n+\techo \"foo* builtin_objectmode=12345\" >.gitattributes &&\n+\tgit add .gitattributes &&\n+\t>foo_1 &&\n+\tattr_check_object_mode_basic foo_1 100644 &&\n+\techo \"builtin_objectmode is not a valid attribute name: .gitattributes:1\" >expect &&\n+\ttest_cmp expect err\n+'\n+\n test_done\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex a9c1e4e0ec..b08a32ea68 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -295,4 +295,29 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pathspec with builtin_objectmode attr can be used' '\n+\t>mode_exec_file_1 &&\n+\n+\tgit status -s \":(attr:builtin_objectmode=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:builtin_objectmode=100755)mode_exec_*\" >actual &&\n+\techo AM mode_exec_file_1 >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'builtin_objectmode 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:builtin_objectmode=100644)\" \"mode_1_*\" >actual &&\n+\techo ?? mode_1_exec >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit status -s \":(exclude,attr:builtin_objectmode=100755)\" \"mode_1_*\" >actual &&\n+\techo ?? mode_1_regular >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.43.0.rc0.421.g78406f8d94-goog\n\n"},{"id":"484967","messageId":"xmqqsf56ql14.fsf@gitster.g","threadId":"60514","inReplyTo":"20231116054437.2343549-1-jojwang@google.com","subject":"Re: [PATCH 1/1] attr: add builtin objectmode values support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-16T06:17:43Z","receivedAt":"2023-11-16T06:17:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joanna Wang writes:\n\n> t/#t1700-split-index.sh# | 547 ++++++++++++++++++++++++++++++++\n\nThis was added by mistake and we can safely ignore it? IOW, is\neverything else in the patch, other than the addition of this path,\nwhat you intended to send out?\n\nOther than the removal of that file, I locally applied the following\nfix-up while checking the difference relative to the previous\niteration. Other than these, the updated version looked very clear\nand nicely done.\n\nThanks.\n\n t/t0003-attributes.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 86f8681570..774b52c298 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -580,12 +580,13 @@ test_expect_success 'builtin object mode attributes work (dir and regular paths)\n '\n \n test_expect_success POSIXPERM 'builtin object mode attributes work (executable)' '\n-\t>exec && chmod +x exec &&\n+\t>exec &&\n+\tchmod +x exec &&\n \tattr_check_object_mode exec 100755\n '\n \n test_expect_success SYMLINKS 'builtin object mode attributes work (symlinks)' '\n-\t>to_sym ln -s to_sym sym &&\n+\tln -s to_sym sym &&\n \tattr_check_object_mode sym 120000\n '\n \n"},{"id":"484969","messageId":"xmqqmsveqgex.fsf@gitster.g","threadId":"60514","inReplyTo":"20231116054437.2343549-1-jojwang@google.com","subject":"Re: [PATCH 1/1] attr: add builtin objectmode values support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-16T07:57:26Z","receivedAt":"2023-11-16T07:57:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joanna Wang writes:\n\n> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\n> index a9c1e4e0ec..b08a32ea68 100755\n> --- a/t/t6135-pathspec-with-attrs.sh\n> +++ b/t/t6135-pathspec-with-attrs.sh\n> @@ -295,4 +295,29 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'pathspec with builtin_objectmode attr can be used' '\n> +\t>mode_exec_file_1 &&\n> +\n> +\tgit status -s \":(attr:builtin_objectmode=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\nThis again would break with filesystems that are incapable of\nsupporting executable bit natively. Writing one command per line,\ndoing something like this may help\n\n\tgit add mode_exec_file_1 &&\n\tgit update-index --chmod=+x mode_exec_file_1 &&\n\n> +\tgit status -s \":(attr:builtin_objectmode=100755)mode_exec_*\" >actual &&\n> +\techo AM mode_exec_file_1 >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'builtin_objectmode attr can be excluded' '\n> +\t>mode_1_regular &&\n> +\t>mode_1_exec && chmod +x mode_1_exec &&\n\nDitto.\n\n> +\tgit status -s \":(exclude,attr:builtin_objectmode=100644)\" \"mode_1_*\" >actual &&\n> +\techo ?? mode_1_exec >expect &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tgit status -s \":(exclude,attr:builtin_objectmode=100755)\" \"mode_1_*\" >actual &&\n> +\techo ?? mode_1_regular >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"484970","messageId":"xmqqil62qfvr.fsf@gitster.g","threadId":"60514","inReplyTo":"xmqqsf56ql14.fsf@gitster.g","subject":"Re: [PATCH 1/1] attr: add builtin objectmode values support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-16T08:08:56Z","receivedAt":"2023-11-16T08:08:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Other than the removal of that file, I locally applied the following\n> fix-up while checking the difference relative to the previous\n> iteration.\n\nCumulatively, aside from the removal of the t/#t* file, here is what\nI ended up with so far.\n\nSubject: [PATCH] SQUASH???\n\n---\n Documentation/gitattributes.txt | 2 +-\n neue | 0\n t/t0003-attributes.sh | 5 +++--\n t/t6135-pathspec-with-attrs.sh | 10 ++++++----\n 4 files changed, 10 insertions(+), 7 deletions(-)\n create mode 100644 neue\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 784aa9d4de..201bdf5edb 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -108,7 +108,7 @@ user defined attributes under this namespace will be ignored and\n trigger a warning.\n \n `builtin_objectmode`\n-^^^^^^^^^^^^^^^^^^^^\n+~~~~~~~~~~~~~~~~~~~~\n This attribute is for filtering files by their file bit modes (40000,\n 120000, 160000, 100755, 100644). e.g. ':(attr:builtin_objectmode=160000)'.\n You may also check these values with `git check-attr builtin_objectmode -- `.\ndiff --git a/neue b/neue\nnew file mode 100644\nindex 0000000000..e69de29bb2\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 86f8681570..774b52c298 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -580,12 +580,13 @@ test_expect_success 'builtin object mode attributes work (dir and regular paths)\n '\n \n test_expect_success POSIXPERM 'builtin object mode attributes work (executable)' '\n-\t>exec && chmod +x exec &&\n+\t>exec &&\n+\tchmod +x exec &&\n \tattr_check_object_mode exec 100755\n '\n \n test_expect_success SYMLINKS 'builtin object mode attributes work (symlinks)' '\n-\t>to_sym ln -s to_sym sym &&\n+\tln -s to_sym sym &&\n \tattr_check_object_mode sym 120000\n '\n \ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex b08a32ea68..f6403ebbda 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -295,22 +295,24 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'pathspec with builtin_objectmode attr can be used' '\n+test_expect_success POSIXPERM 'pathspec with builtin_objectmode attr can be used' '\n \t>mode_exec_file_1 &&\n \n \tgit status -s \":(attr:builtin_objectmode=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 add mode_exec_file_1 &&\n+\tchmod +x mode_exec_file_1 &&\n \tgit status -s \":(attr:builtin_objectmode=100755)mode_exec_*\" >actual &&\n \techo AM mode_exec_file_1 >expect &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'builtin_objectmode attr can be excluded' '\n+test_expect_success POSIXPERM 'builtin_objectmode attr can be excluded' '\n \t>mode_1_regular &&\n-\t>mode_1_exec && chmod +x mode_1_exec &&\n+\t>mode_1_exec &&\n+\tchmod +x mode_1_exec &&\n \tgit status -s \":(exclude,attr:builtin_objectmode=100644)\" \"mode_1_*\" >actual &&\n \techo ?? mode_1_exec >expect &&\n \ttest_cmp expect actual &&\n-- \n2.43.0-rc2\n\n"},{"id":"484979","messageId":"CAMmZTi_BKfqw3984CyNdDUNLdMK2hFLtxeJfu_iEw23m8HW-dA@mail.gmail.com","threadId":"60514","inReplyTo":"xmqqil62qfvr.fsf@gitster.g","subject":"Re: [PATCH 1/1] attr: add builtin objectmode values support","fromName":"Joanna Wang","fromEmail":"jojwang@google.com","sentAt":"2023-11-16T17:54:35Z","receivedAt":"2023-11-16T17:54:47Z","isPatch":true,"sender":{"key":"jojwang@google.com","avatar":"https://avatars.githubusercontent.com/u/120585822?v=4"},"body":"yes sorry, t/#t1700 was a mistake.\nThank you for the review and further fixes\n\nOn Thu, Nov 16, 2023 at 12:08 AM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > Other than the removal of that file, I locally applied the following\n> > fix-up while checking the difference relative to the previous\n> > iteration.\n>\n> Cumulatively, aside from the removal of the t/#t* file, here is what\n> I ended up with so far.\n>\n> Subject: [PATCH] SQUASH???\n>\n> ---\n> Documentation/gitattributes.txt | 2 +-\n> neue | 0\n> t/t0003-attributes.sh | 5 +++--\n> t/t6135-pathspec-with-attrs.sh | 10 ++++++----\n> 4 files changed, 10 insertions(+), 7 deletions(-)\n> create mode 100644 neue\n>\n> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\n> index 784aa9d4de..201bdf5edb 100644\n> --- a/Documentation/gitattributes.txt\n> +++ b/Documentation/gitattributes.txt\n> @@ -108,7 +108,7 @@ user defined attributes under this namespace will be ignored and\n> trigger a warning.\n>\n> `builtin_objectmode`\n> -^^^^^^^^^^^^^^^^^^^^\n> +~~~~~~~~~~~~~~~~~~~~\n> This attribute is for filtering files by their file bit modes (40000,\n> 120000, 160000, 100755, 100644). e.g. ':(attr:builtin_objectmode=160000)'.\n> You may also check these values with `git check-attr builtin_objectmode -- `.\n> diff --git a/neue b/neue\n> new file mode 100644\n> index 0000000000..e69de29bb2\n> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\n> index 86f8681570..774b52c298 100755\n> --- a/t/t0003-attributes.sh\n> +++ b/t/t0003-attributes.sh\n> @@ -580,12 +580,13 @@ test_expect_success 'builtin object mode attributes work (dir and regular paths)\n> '\n>\n> test_expect_success POSIXPERM 'builtin object mode attributes work (executable)' '\n> - >exec && chmod +x exec &&\n> + >exec &&\n> + chmod +x exec &&\n> attr_check_object_mode exec 100755\n> '\n>\n> test_expect_success SYMLINKS 'builtin object mode attributes work (symlinks)' '\n> - >to_sym ln -s to_sym sym &&\n> + ln -s to_sym sym &&\n> attr_check_object_mode sym 120000\n> '\n>\n> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\n> index b08a32ea68..f6403ebbda 100755\n> --- a/t/t6135-pathspec-with-attrs.sh\n> +++ b/t/t6135-pathspec-with-attrs.sh\n> @@ -295,22 +295,24 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '\n> test_cmp expect actual\n> '\n>\n> -test_expect_success 'pathspec with builtin_objectmode attr can be used' '\n> +test_expect_success POSIXPERM 'pathspec with builtin_objectmode attr can be used' '\n> >mode_exec_file_1 &&\n>\n> git status -s \":(attr:builtin_objectmode=100644)mode_exec_*\" >actual &&\n> echo ?? mode_exec_file_1 >expect &&\n> test_cmp expect actual &&\n>\n> - git add mode_exec_file_1 && chmod +x mode_exec_file_1 &&\n> + git add mode_exec_file_1 &&\n> + chmod +x mode_exec_file_1 &&\n> git status -s \":(attr:builtin_objectmode=100755)mode_exec_*\" >actual &&\n> echo AM mode_exec_file_1 >expect &&\n> test_cmp expect actual\n> '\n>\n> -test_expect_success 'builtin_objectmode attr can be excluded' '\n> +test_expect_success POSIXPERM 'builtin_objectmode attr can be excluded' '\n> >mode_1_regular &&\n> - >mode_1_exec && chmod +x mode_1_exec &&\n> + >mode_1_exec &&\n> + chmod +x mode_1_exec &&\n> git status -s \":(exclude,attr:builtin_objectmode=100644)\" \"mode_1_*\" >actual &&\n> echo ?? mode_1_exec >expect &&\n> test_cmp expect actual &&\n> --\n> 2.43.0-rc2\n>\n"},{"id":"485297","messageId":"CAMmZTi-U_ufzoBLCDWKbrf=3GZzGszxnM1_Pu6ufBeoYjj7Gdw@mail.gmail.com","threadId":"60514","inReplyTo":"xmqqil62qfvr.fsf@gitster.g","subject":"Re: [PATCH 1/1] attr: add builtin objectmode values support","fromName":"Joanna Wang","fromEmail":"jojwang@google.com","sentAt":"2023-12-01T04:01:54Z","receivedAt":"2023-12-01T04:02:07Z","isPatch":true,"sender":{"key":"jojwang@google.com","avatar":"https://avatars.githubusercontent.com/u/120585822?v=4"},"body":"> Cumulatively, aside from the removal of the t/#t* file, here is what\n> I ended up with so far.\n\nI want to double check if I should followup here.\nI assumed that you had already applied these final fixes on my behalf,\nsimilar to my patch for enabling attr for `git-add`. But if I was wrong,\nI'm happy to send another update with all the fixes.\n\n\nOn Thu, Nov 16, 2023 at 4:08 PM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > Other than the removal of that file, I locally applied the following\n> > fix-up while checking the difference relative to the previous\n> > iteration.\n>\n> Cumulatively, aside from the removal of the t/#t* file, here is what\n> I ended up with so far.\n>\n> Subject: [PATCH] SQUASH???\n>\n> ---\n> Documentation/gitattributes.txt | 2 +-\n> neue | 0\n> t/t0003-attributes.sh | 5 +++--\n> t/t6135-pathspec-with-attrs.sh | 10 ++++++----\n> 4 files changed, 10 insertions(+), 7 deletions(-)\n> create mode 100644 neue\n>\n> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\n> index 784aa9d4de..201bdf5edb 100644\n> --- a/Documentation/gitattributes.txt\n> +++ b/Documentation/gitattributes.txt\n> @@ -108,7 +108,7 @@ user defined attributes under this namespace will be ignored and\n> trigger a warning.\n>\n> `builtin_objectmode`\n> -^^^^^^^^^^^^^^^^^^^^\n> +~~~~~~~~~~~~~~~~~~~~\n> This attribute is for filtering files by their file bit modes (40000,\n> 120000, 160000, 100755, 100644). e.g. ':(attr:builtin_objectmode=160000)'.\n> You may also check these values with `git check-attr builtin_objectmode -- `.\n> diff --git a/neue b/neue\n> new file mode 100644\n> index 0000000000..e69de29bb2\n> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\n> index 86f8681570..774b52c298 100755\n> --- a/t/t0003-attributes.sh\n> +++ b/t/t0003-attributes.sh\n> @@ -580,12 +580,13 @@ test_expect_success 'builtin object mode attributes work (dir and regular paths)\n> '\n>\n> test_expect_success POSIXPERM 'builtin object mode attributes work (executable)' '\n> - >exec && chmod +x exec &&\n> + >exec &&\n> + chmod +x exec &&\n> attr_check_object_mode exec 100755\n> '\n>\n> test_expect_success SYMLINKS 'builtin object mode attributes work (symlinks)' '\n> - >to_sym ln -s to_sym sym &&\n> + ln -s to_sym sym &&\n> attr_check_object_mode sym 120000\n> '\n>\n> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\n> index b08a32ea68..f6403ebbda 100755\n> --- a/t/t6135-pathspec-with-attrs.sh\n> +++ b/t/t6135-pathspec-with-attrs.sh\n> @@ -295,22 +295,24 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '\n> test_cmp expect actual\n> '\n>\n> -test_expect_success 'pathspec with builtin_objectmode attr can be used' '\n> +test_expect_success POSIXPERM 'pathspec with builtin_objectmode attr can be used' '\n> >mode_exec_file_1 &&\n>\n> git status -s \":(attr:builtin_objectmode=100644)mode_exec_*\" >actual &&\n> echo ?? mode_exec_file_1 >expect &&\n> test_cmp expect actual &&\n>\n> - git add mode_exec_file_1 && chmod +x mode_exec_file_1 &&\n> + git add mode_exec_file_1 &&\n> + chmod +x mode_exec_file_1 &&\n> git status -s \":(attr:builtin_objectmode=100755)mode_exec_*\" >actual &&\n> echo AM mode_exec_file_1 >expect &&\n> test_cmp expect actual\n> '\n>\n> -test_expect_success 'builtin_objectmode attr can be excluded' '\n> +test_expect_success POSIXPERM 'builtin_objectmode attr can be excluded' '\n> >mode_1_regular &&\n> - >mode_1_exec && chmod +x mode_1_exec &&\n> + >mode_1_exec &&\n> + chmod +x mode_1_exec &&\n> git status -s \":(exclude,attr:builtin_objectmode=100644)\" \"mode_1_*\" >actual &&\n> echo ?? mode_1_exec >expect &&\n> test_cmp expect actual &&\n> --\n> 2.43.0-rc2\n>\n"},{"id":"485600","messageId":"xmqqedfrovsb.fsf@gitster.g","threadId":"60514","inReplyTo":"CAMmZTi-U_ufzoBLCDWKbrf=3GZzGszxnM1_Pu6ufBeoYjj7Gdw@mail.gmail.com","subject":"Re: [PATCH 1/1] attr: add builtin objectmode values support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-12T23:17:40Z","receivedAt":"2023-12-12T23:17:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joanna Wang writes:\n\n>> Cumulatively, aside from the removal of the t/#t* file, here is what\n>> I ended up with so far.\n>\n> I want to double check if I should followup here.\n> I assumed that you had already applied these final fixes on my behalf,\n> similar to my patch for enabling attr for `git-add`. But if I was wrong,\n> I'm happy to send another update with all the fixes.\n\nI've squashed the fix in, so no need to resend only to patch up what\nI pointed out earlier.\n\nThe end result fails t0003 under GIT_TEST_PASSING_SANITIZE_LEAK\nthough. As the synthetic attribute values are allocated without\nbeing in the hashmap based on the value read from .gitattributes\nfiles, somebody needs to hold pointers to them *and* we need to\navoid allocating unbounded number of them.\n\nThe attached is one possible way to plug the leak; I am not sure if\nit is the best one, though. One thing I like about the solution is\nthat the approach makes sure that the mode attributes we would ever\nreturn are very tightly controlled and does not allow a buggy code\nto come up with \"mode\" to be passed to this new helper function to\npass random and unsupported mode bits without triggering the BUG().\n\n attr.c | 30 +++++++++++++++++++++++++++---\n 1 file changed, 27 insertions(+), 3 deletions(-)\n\ndiff --git c/attr.c w/attr.c\nindex b03c20f768..679e42258c 100644\n--- c/attr.c\n+++ w/attr.c\n@@ -1250,10 +1250,34 @@ static struct object_id *default_attr_source(void)\n \treturn &attr_source;\n }\n \n+static const char *interned_mode_string(unsigned int mode)\n+{\n+\tstatic struct {\n+\t\tunsigned int val;\n+\t\tchar str[7];\n+\t} mode_string[] = {\n+\t\t{ .val = 0040000 },\n+\t\t{ .val = 0100644 },\n+\t\t{ .val = 0100755 },\n+\t\t{ .val = 0120000 },\n+\t\t{ .val = 0160000 },\n+\t};\n+\tint i;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(mode_string); i++) {\n+\t\tif (mode_string[i].val != mode)\n+\t\t\tcontinue;\n+\t\tif (!*mode_string[i].str)\n+\t\t\tsnprintf(mode_string[i].str, sizeof(mode_string[i].str),\n+\t\t\t\t \"%06o\", mode);\n+\t\treturn mode_string[i].str;\n+\t}\n+\tBUG(\"Unsupported mode 0%o\", mode);\n+}\n+\n static const char *builtin_object_mode_attr(struct index_state *istate, const char *path)\n {\n \tunsigned int mode;\n-\tstruct strbuf sb = STRBUF_INIT;\n \n \tif (direction == GIT_ATTR_CHECKIN) {\n \t\tstruct object_id oid;\n@@ -1287,8 +1311,8 @@ static const char *builtin_object_mode_attr(struct index_state *istate, const ch\n \t\telse\n \t\t\treturn ATTR__UNSET;\n \t}\n-\tstrbuf_addf(&sb, \"%06o\", mode);\n-\treturn strbuf_detach(&sb, NULL);\n+\n+\treturn interned_mode_string(mode);\n }\n \n \n\n"},{"id":"485890","messageId":"xmqq5y0ssknj.fsf@gitster.g","threadId":"60514","inReplyTo":"xmqqedfrovsb.fsf@gitster.g","subject":"Re: [PATCH 1/1] attr: add builtin objectmode values support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-20T20:07:12Z","receivedAt":"2023-12-20T20:07:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The attached is one possible way to plug the leak; I am not sure if\n> it is the best one, though. One thing I like about the solution is\n> that the approach makes sure that the mode attributes we would ever\n> return are very tightly controlled and does not allow a buggy code\n> to come up with \"mode\" to be passed to this new helper function to\n> pass random and unsupported mode bits without triggering the BUG().\n>\n> attr.c | 30 +++++++++++++++++++++++++++---\n> 1 file changed, 27 insertions(+), 3 deletions(-)\n\nAnybody who want to propose a better leakfix (we cannot afford to do\nwith UNLEAK() as the number of leaked mode string will be unbounded)?\n\nOtherwise, I'll squash it in to Jonanna's patch and merge it down to\n'next'.\n\nThanks.\n\n> diff --git c/attr.c w/attr.c\n> index b03c20f768..679e42258c 100644\n> --- c/attr.c\n> +++ w/attr.c\n> @@ -1250,10 +1250,34 @@ static struct object_id *default_attr_source(void)\n> \treturn &attr_source;\n> }\n> \n> +static const char *interned_mode_string(unsigned int mode)\n> +{\n> +\tstatic struct {\n> +\t\tunsigned int val;\n> +\t\tchar str[7];\n> +\t} mode_string[] = {\n> +\t\t{ .val = 0040000 },\n> +\t\t{ .val = 0100644 },\n> +\t\t{ .val = 0100755 },\n> +\t\t{ .val = 0120000 },\n> +\t\t{ .val = 0160000 },\n> +\t};\n> +\tint i;\n> +\n> +\tfor (i = 0; i < ARRAY_SIZE(mode_string); i++) {\n> +\t\tif (mode_string[i].val != mode)\n> +\t\t\tcontinue;\n> +\t\tif (!*mode_string[i].str)\n> +\t\t\tsnprintf(mode_string[i].str, sizeof(mode_string[i].str),\n> +\t\t\t\t \"%06o\", mode);\n> +\t\treturn mode_string[i].str;\n> +\t}\n> +\tBUG(\"Unsupported mode 0%o\", mode);\n> +}\n> +\n> static const char *builtin_object_mode_attr(struct index_state *istate, const char *path)\n> {\n> \tunsigned int mode;\n> -\tstruct strbuf sb = STRBUF_INIT;\n> \n> \tif (direction == GIT_ATTR_CHECKIN) {\n> \t\tstruct object_id oid;\n> @@ -1287,8 +1311,8 @@ static const char *builtin_object_mode_attr(struct index_state *istate, const ch\n> \t\telse\n> \t\t\treturn ATTR__UNSET;\n> \t}\n> -\tstrbuf_addf(&sb, \"%06o\", mode);\n> -\treturn strbuf_detach(&sb, NULL);\n> +\n> +\treturn interned_mode_string(mode);\n> }\n> \n> \n"},{"id":"486684","messageId":"CAMmZTi_W6Di3xQnEuiVYc+eL8pOPbqnoJm_OuhwLQ6vLVy=PLQ@mail.gmail.com","threadId":"60514","inReplyTo":"xmqq5y0ssknj.fsf@gitster.g","subject":"Re: [PATCH 1/1] attr: add builtin objectmode values support","fromName":"Joanna Wang","fromEmail":"jojwang@google.com","sentAt":"2024-01-12T06:12:21Z","receivedAt":"2024-01-12T06:12:35Z","isPatch":true,"sender":{"key":"jojwang@google.com","avatar":"https://avatars.githubusercontent.com/u/120585822?v=4"},"body":"(I was out all month.) Thank you for the additional fixes.\n\n\nOn Thu, Dec 21, 2023 at 4:07 AM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > The attached is one possible way to plug the leak; I am not sure if\n> > it is the best one, though. One thing I like about the solution is\n> > that the approach makes sure that the mode attributes we would ever\n> > return are very tightly controlled and does not allow a buggy code\n> > to come up with \"mode\" to be passed to this new helper function to\n> > pass random and unsupported mode bits without triggering the BUG().\n> >\n> > attr.c | 30 +++++++++++++++++++++++++++---\n> > 1 file changed, 27 insertions(+), 3 deletions(-)\n>\n> Anybody who want to propose a better leakfix (we cannot afford to do\n> with UNLEAK() as the number of leaked mode string will be unbounded)?\n>\n> Otherwise, I'll squash it in to Jonanna's patch and merge it down to\n> 'next'.\n>\n> Thanks.\n>\n> > diff --git c/attr.c w/attr.c\n> > index b03c20f768..679e42258c 100644\n> > --- c/attr.c\n> > +++ w/attr.c\n> > @@ -1250,10 +1250,34 @@ static struct object_id *default_attr_source(void)\n> > return &attr_source;\n> > }\n> >\n> > +static const char *interned_mode_string(unsigned int mode)\n> > +{\n> > + static struct {\n> > + unsigned int val;\n> > + char str[7];\n> > + } mode_string[] = {\n> > + { .val = 0040000 },\n> > + { .val = 0100644 },\n> > + { .val = 0100755 },\n> > + { .val = 0120000 },\n> > + { .val = 0160000 },\n> > + };\n> > + int i;\n> > +\n> > + for (i = 0; i < ARRAY_SIZE(mode_string); i++) {\n> > + if (mode_string[i].val != mode)\n> > + continue;\n> > + if (!*mode_string[i].str)\n> > + snprintf(mode_string[i].str, sizeof(mode_string[i].str),\n> > + \"%06o\", mode);\n> > + return mode_string[i].str;\n> > + }\n> > + BUG(\"Unsupported mode 0%o\", mode);\n> > +}\n> > +\n> > static const char *builtin_object_mode_attr(struct index_state *istate, const char *path)\n> > {\n> > unsigned int mode;\n> > - struct strbuf sb = STRBUF_INIT;\n> >\n> > if (direction == GIT_ATTR_CHECKIN) {\n> > struct object_id oid;\n> > @@ -1287,8 +1311,8 @@ static const char *builtin_object_mode_attr(struct index_state *istate, const ch\n> > else\n> > return ATTR__UNSET;\n> > }\n> > - strbuf_addf(&sb, \"%06o\", mode);\n> > - return strbuf_detach(&sb, NULL);\n> > +\n> > + return interned_mode_string(mode);\n> > }\n> >\n> >\n"}]}