{"thread":{"id":"64936","subject":"[PATCH] [RFC][GSoC][PATCH] attr: use local repository state in read_attr","startedAt":"2026-02-07T11:40:23Z","lastAt":"2026-02-14T06:47:50Z","messageCount":10,"participants":["Ayush Jha","Tian Yuchen","Lucas Seiki Oshiro","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"535412","messageId":"20260207114007.40-1-kumarayushjha123@gmail.com","threadId":"64936","inReplyTo":null,"subject":"[PATCH] [RFC][GSoC][PATCH] attr: use local repository state in read_attr","fromName":"Ayush Jha","fromEmail":"kumarayushjha123@gmail.com","sentAt":"2026-02-07T11:40:07Z","receivedAt":"2026-02-07T11:40:23Z","isPatch":true,"sender":{"key":"kumarayushjha123@gmail.com","avatar":null},"body":"read_attr() currently relies on is_bare_repository(), which\nimplicitly depends on the global the_repository.\n\nAs attr.c is a reusable library component used by multiple\ncommands, this prevents correct behavior when operating on\nsecondary repositories (e.g. submodules or in-process repos)\nwhose bareness may differ from the_repository.\n\nUpdate read_attr() to determine bareness using the repository\nassociated with istate->repo, based on repository configuration\nand worktree presence, instead of relying on global state.\n\nNo functional change is intended for the primary repository case.\n\nSigned-off-by: Ayush Jha \n---\n attr.c | 36 ++++++++++++++++++++++--------------\n 1 file changed, 22 insertions(+), 14 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 4999b7e09d..f2d25b1863 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -848,21 +848,29 @@ static struct attr_stack *read_attr(struct index_state *istate,\n \t\tres = read_attr_from_index(istate, path, flags);\n \t} else if (tree_oid) {\n \t\tres = read_attr_from_blob(istate, tree_oid, path, flags);\n-\t} else if (!is_bare_repository()) {\n-\t\tif (direction == GIT_ATTR_CHECKOUT) {\n-\t\t\tres = read_attr_from_index(istate, path, flags);\n-\t\t\tif (!res)\n-\t\t\t\tres = read_attr_from_file(path, flags);\n-\t\t} else if (direction == GIT_ATTR_CHECKIN) {\n-\t\t\tres = read_attr_from_file(path, flags);\n-\t\t\tif (!res)\n-\t\t\t\t/*\n-\t\t\t\t * There is no checked out .gitattributes file\n-\t\t\t\t * there, but we might have it in the index.\n-\t\t\t\t * We allow operation in a sparsely checked out\n-\t\t\t\t * work tree, so read from it.\n-\t\t\t\t */\n+\t} else {\n+\t\tint is_bare;\n+\t\tint is_bare_cfg = -1;\n+\n+\t\trepo_config_get_bool(istate->repo, \"core.bare\", &is_bare_cfg);\n+\t\tis_bare = is_bare_cfg && !repo_get_work_tree(istate->repo);\n+\n+\t\tif (!is_bare) {\n+\t\t\tif (direction == GIT_ATTR_CHECKOUT) {\n \t\t\t\tres = read_attr_from_index(istate, path, flags);\n+\t\t\t\tif (!res)\n+\t\t\t\t\tres = read_attr_from_file(path, flags);\n+\t\t\t} else if (direction == GIT_ATTR_CHECKIN) {\n+\t\t\t\tres = read_attr_from_file(path, flags);\n+\t\t\t\tif (!res)\n+\t\t\t\t\t/*\n+\t\t\t\t\t * There is no checked out .gitattributes file\n+\t\t\t\t\t * there, but we might have it in the index.\n+\t\t\t\t\t * We allow operation in a sparsely checked out\n+\t\t\t\t\t * work tree, so read from it.\n+\t\t\t\t\t */\n+\t\t\t\t\tres = read_attr_from_index(istate, path, flags);\n+\t\t\t}\n \t\t}\n \t}\n \n-- \n2.53.0.windows.1\n\n"},{"id":"535424","messageId":"cc2f400e-49c2-4de0-9c51-9a5c0294735e@gmail.com","threadId":"64936","inReplyTo":"20260207114007.40-1-kumarayushjha123@gmail.com","subject":"Re: [PATCH] [RFC][GSoC][PATCH] attr: use local repository state in read_attr","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-07T19:12:16Z","receivedAt":"2026-02-07T19:12:22Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"On 2/7/26 19:40, Ayush Jha wrote:\n> read_attr() currently relies on is_bare_repository(), which\n> implicitly depends on the global the_repository.\n> \n> As attr.c is a reusable library component used by multiple\n> commands, this prevents correct behavior when operating on\n> secondary repositories (e.g. submodules or in-process repos)\n> whose bareness may differ from the_repository.\n> \n> Update read_attr() to determine bareness using the repository\n> associated with istate->repo, based on repository configuration\n> and worktree presence, instead of relying on global state.\n> \n> No functional change is intended for the primary repository case.\n> \n> Signed-off-by: Ayush Jha \n> ---\n> attr.c | 36 ++++++++++++++++++++++--------------\n> 1 file changed, 22 insertions(+), 14 deletions(-)\n> \n> diff --git a/attr.c b/attr.c\n> index 4999b7e09d..f2d25b1863 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -848,21 +848,29 @@ static struct attr_stack *read_attr(struct index_state *istate,\n> \t\tres = read_attr_from_index(istate, path, flags);\n> \t} else if (tree_oid) {\n> \t\tres = read_attr_from_blob(istate, tree_oid, path, flags);\n> -\t} else if (!is_bare_repository()) {\n> -\t\tif (direction == GIT_ATTR_CHECKOUT) {\n> -\t\t\tres = read_attr_from_index(istate, path, flags);\n> -\t\t\tif (!res)\n> -\t\t\t\tres = read_attr_from_file(path, flags);\n> -\t\t} else if (direction == GIT_ATTR_CHECKIN) {\n> -\t\t\tres = read_attr_from_file(path, flags);\n> -\t\t\tif (!res)\n> -\t\t\t\t/*\n> -\t\t\t\t * There is no checked out .gitattributes file\n> -\t\t\t\t * there, but we might have it in the index.\n> -\t\t\t\t * We allow operation in a sparsely checked out\n> -\t\t\t\t * work tree, so read from it.\n> -\t\t\t\t */\n> +\t} else {\n> +\t\tint is_bare;\n> +\t\tint is_bare_cfg = -1;\n> +\n> +\t\trepo_config_get_bool(istate->repo, \"core.bare\", &is_bare_cfg);\n> +\t\tis_bare = is_bare_cfg && !repo_get_work_tree(istate->repo);\n> +\n> +\t\tif (!is_bare) {\n> +\t\t\tif (direction == GIT_ATTR_CHECKOUT) {\n> \t\t\t\tres = read_attr_from_index(istate, path, flags);\n> +\t\t\t\tif (!res)\n> +\t\t\t\t\tres = read_attr_from_file(path, flags);\n> +\t\t\t} else if (direction == GIT_ATTR_CHECKIN) {\n> +\t\t\t\tres = read_attr_from_file(path, flags);\n> +\t\t\t\tif (!res)\n> +\t\t\t\t\t/*\n> +\t\t\t\t\t * There is no checked out .gitattributes file\n> +\t\t\t\t\t * there, but we might have it in the index.\n> +\t\t\t\t\t * We allow operation in a sparsely checked out\n> +\t\t\t\t\t * work tree, so read from it.\n> +\t\t\t\t\t */\n> +\t\t\t\t\tres = read_attr_from_index(istate, path, flags);\n> +\t\t\t}\n> \t\t}\n> \t}\n> \n\nHi Ayush,\n\nI'm new to the community ;)\n\nInitially, the concern that replacing 'is_bare_repository' with \n'repo_config_get_bool()' inside 'read_attr()' might introduce a \nperformance regression came to my mind immediately. To verify this, I \nran a benchmark using hyperfine, and the script was like:\n\n >#!/bin/bash\n >mkdir -p perf-test && cd perf-test\n >git init\n >\n >git config user.email \"malon7782@yahoo.com\"\n >git config user.name \"ILOVEGIT\"\n >\n >for i in {1..10000}; do\n >\techo \"content\" > \"file_$i.txt\"\n >done\n >\n >git add .\n >git commit -m \"initial commit\"\n >\n >echo \"* test=auto\" > .gitattributes\n >git add .gitattributes\n >git commit -m \"add attr\"\n\nThen\n\n >'git ls-files > files_list.txt'\n\n( I wrote it this way primarily because I didn't anticipate that too \nmany files would return a 126 error code. So I improvised my switching \nto reading from standard input:/\n\nIt didn't result in a noticeable performance difference (Even if the \nnumber of loop was increased to 300000). Then I started to create a \nstricter benchmark environment to defeat the attribute stack caching:\n\n\n >mkdir -p dir_A dir_B\n >\n >echo \"* text=auto\" > dir_A/.gitattributes\n >echo \"* text=auto\" > dir_B/.gitattributes\n >\n >for i in {1..5000}; do\n > echo \"dir_A/file\"\n > echo \"dir_B/file\"\n >done > thrashing_list.txt\n\nStill, no difference in performance. Has anyone else conducted any \nsimilar performance test? Were the results the same as mine? Is it \nnormal, or there is something wrong with my test?\n\nRegards,\n\nYuchen\n"},{"id":"535442","messageId":"E605A7F6-AF4D-463F-8316-6BE69AFE0369@gmail.com","threadId":"64936","inReplyTo":"20260207114007.40-1-kumarayushjha123@gmail.com","subject":"Re: [PATCH] [RFC][GSoC][PATCH] attr: use local repository state in read_attr","fromName":"Lucas Seiki Oshiro","fromEmail":"lucasseikioshiro@gmail.com","sentAt":"2026-02-07T21:02:46Z","receivedAt":"2026-02-07T21:03:02Z","isPatch":true,"sender":{"key":"lucasseikioshiro@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701580?v=4"},"body":"Hi Ayush!\n\nThanks for your interest in helping git-repo-info, even though\nthis second patch changes the focus. It was my GSoC project last\nyear. \n\nNitpick: there are two [PATCH] in the subject, and usually we use\nonly one subject prefix. I also think that this could be a v2 of\nyour first patch [1]. This way, this subject should be:\n\"[RFC GSoC PATCH v2] attr: use local repository state in read_attr\".\n\nTip: use `--subject-prefix='RFC GSoC PATCH'` in this case, and\nset `format.subjectPrefix='GSoC PATCH'` for your future GSoC\npatches.\n\n> read_attr() currently relies on is_bare_repository(), which\n> implicitly depends on the global the_repository.\n\nSo, wouldn't it be better to make is_bare_repository depend\non a `struct repository *repo` instead of `the_repository`?\n\nI don't know how feasible it is to do that, but it seems to\nme that your change could benefit other places that depend\non that function.\n\n\n> + int is_bare;\n> + int is_bare_cfg = -1;\n> +\n> + repo_config_get_bool(istate->repo, \"core.bare\", &is_bare_cfg);\n\nThis function returns 0 when the config key is found. If the key\ncan't be found, it returns 1 and doesn't touch `is_bare_cfg`.\nThis means that if \"core.bare\" doesn't exist (which is unlikely,\nbut, who knows...) we'll proceed with is_bare_cfg = -1, and...\n\n> + is_bare = is_bare_cfg && !repo_get_work_tree(istate->repo);\n\nsince -1 is a truthy value in C, then in this case we're\ndeciding it based on having or not a work tree. I don't know if\nsomeone with more experience than me see something wrong with\nthis but it seems ok to me. However, I found this way a little\nconfusing to read. Perhaps it would be clearer if it was written\nlike this:\n\nint is_bare;\n\nif (repo_config_get_bool(istate->repo, \"core.bare\", &is_bare_cfg))\n\tis_bare = !repo_get_work_tree(istate->repo);\n\n\n[1] 20260206152002.1244-1-kumarayushjha123@gmail.com\n"},{"id":"535447","messageId":"xmqqbji0b5ak.fsf@gitster.g","threadId":"64936","inReplyTo":"E605A7F6-AF4D-463F-8316-6BE69AFE0369@gmail.com","subject":"Re: [PATCH] [RFC][GSoC][PATCH] attr: use local repository state in read_attr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-07T21:41:07Z","receivedAt":"2026-02-07T21:41:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lucas Seiki Oshiro writes:\n\n>> read_attr() currently relies on is_bare_repository(), which\n>> implicitly depends on the global the_repository.\n>\n> So, wouldn't it be better to make is_bare_repository depend\n> on a `struct repository *repo` instead of `the_repository`?\n\nThe codepath read_attr() is in is usually not that hot but it is not\ncheap.\n\nThe repository object should have a boolean that says \"I am bare\",\nperhaps initialized lazily, and your version of is_bare_repository\nthat takes a repository object would be a good entry point to it.\n\nAlso, IIRC, there is another releated effort to allow attribute data\nsource to become per repository. This change may want to coordinate\nwith it.\n\nThanks.\n"},{"id":"535463","messageId":"96329bc6-0490-454b-a21b-babb85c98bc9@gmail.com","threadId":"64936","inReplyTo":"20260207114007.40-1-kumarayushjha123@gmail.com","subject":"Re: [PATCH] [RFC][GSoC][PATCH] attr: use local repository state in read_attr","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-08T04:42:07Z","receivedAt":"2026-02-08T04:42:12Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Junio C Hamano writes:\n\n >The codepath read_attr() is in is usually not that hot but it is not\n >cheap.\n\nI'm a bit curious—under what circumstances would calling this method \nresult in significant performance regression?\n\nRegards,\n\nYuchen\n\n\n\n"},{"id":"535676","messageId":"CAFNBzOckR2yfGvLMHm0VZW+iKJTgFxzfxQAskdBV2HQ_3yXggA@mail.gmail.com","threadId":"64936","inReplyTo":"96329bc6-0490-454b-a21b-babb85c98bc9@gmail.com","subject":"Re: [PATCH] [RFC][GSoC][PATCH] attr: use local repository state in read_attr","fromName":"Ayush Jha","fromEmail":"kumarayushjha123@gmail.com","sentAt":"2026-02-10T14:05:40Z","receivedAt":"2026-02-10T14:05:54Z","isPatch":true,"sender":{"key":"kumarayushjha123@gmail.com","avatar":null},"body":"Hello everyone,\n\nI’ve incorporated the feedback provided on this patch and sent an\nupdated version as a follow-up patch series:\n\n[RFC GSoC PATCH v3 0/2] Make read_attr() repository-aware by\nintroducing a lazy bare state\n[RFC GSoC PATCH v3 1/2] repo-settings: add repo_settings_get_is_bare\n[RFC GSoC PATCH v3 2/2] attr: use local repository state in read_attr\n\nSince I’m still new to the Git community and the mailing-list\nworkflow, I wanted to check whether I might have missed anything in\nthe process (such as CCs, subject tags, or proper threading), as I\nhaven’t received feedback on the updated patches yet.\n\nPlease let me know if there’s anything I should fix or do differently.\nI’d really appreciate any guidance.\n\nThank you for your time and for the earlier feedback.\n\nBest regards,\nAyush Jha\n\n\nOn Sun, Feb 8, 2026 at 10:12 AM Tian Yuchen wrote:\n>\n> Junio C Hamano writes:\n>\n> >The codepath read_attr() is in is usually not that hot but it is not\n> >cheap.\n>\n> I'm a bit curious—under what circumstances would calling this method\n> result in significant performance regression?\n>\n> Regards,\n>\n> Yuchen\n>\n>\n>\n"},{"id":"535726","messageId":"xmqqqzqsw4og.fsf@gitster.g","threadId":"64936","inReplyTo":"96329bc6-0490-454b-a21b-babb85c98bc9@gmail.com","subject":"Re: [PATCH] [RFC][GSoC][PATCH] attr: use local repository state in read_attr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-10T23:37:51Z","receivedAt":"2026-02-10T23:37:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tian Yuchen writes:\n\n> Junio C Hamano writes:\n>\n> >The codepath read_attr() is in is usually not that hot but it is not\n> >cheap.\n>\n> I'm a bit curious—under what circumstances would calling this method \n> result in significant performance regression?\n\nSignificant? I dunno.\n\nAnd quite honestly, I do not care about significance very much in a\ncase like this. Doing things that do not make sense, like checking\nthe same configuration variable again and again when you _know_ that\nyou never switched to a different repository since you last checked,\nis simply wrong. It burdens the readers with unnecessary cognitive\nload by making them wonder why you do such a nonsensical thing.\n\nThe read_attr() is called during an attr stack construction, which\ntraverses the directory hierarchy of a single repositry's working\ntree (we do not traverse across submodule boundaries), and the same\nistate (i.e., index contents) structure is passed around throughout\nthe callchain. The repository instance at istate->repo may be a\ngood place to store \"am I bare?\" bit that is computed just once and\nreused whenever we need to know, like in the funcion under\ndiscussion.\n"},{"id":"535781","messageId":"83365a16-68c2-4429-926e-3071df3b9bfb@gmail.com","threadId":"64936","inReplyTo":"xmqqqzqsw4og.fsf@gitster.g","subject":"Re: [PATCH] [RFC][GSoC][PATCH] attr: use local repository state in read_attr","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-02-11T16:43:03Z","receivedAt":"2026-02-11T16:43:10Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"On 2/11/26 07:37, Junio C Hamano wrote:\n> Tian Yuchen writes:\n> \n>> Junio C Hamano writes:\n>>\n>> >The codepath read_attr() is in is usually not that hot but it is not\n>> >cheap.\n>>\n>> I'm a bit curious—under what circumstances would calling this method\n>> result in significant performance regression?\n> Significant? I dunno.\n> \n> And quite honestly, I do not care about significance very much in a\n> case like this. Doing things that do not make sense, like checking\n> the same configuration variable again and again when you_know_ that\n> you never switched to a different repository since you last checked,\n> is simply wrong. It burdens the readers with unnecessary cognitive\n> load by making them wonder why you do such a nonsensical thing.\n> \n> The read_attr() is called during an attr stack construction, which\n> traverses the directory hierarchy of a single repositry's working\n> tree (we do not traverse across submodule boundaries), and the same\n> istate (i.e., index contents) structure is passed around throughout\n> the callchain. The repository instance at istate->repo may be a\n> good place to store \"am I bare?\" bit that is computed just once and\n> reused whenever we need to know, like in the funcion under\n> discussion.\n\nHi Junio,\n\nI completely agree that computing it once and storing it in \n'istate->repo' is the right fix. Optimizing for logical clarity and \nreducing load is indeed more important than micro-benchmarking here.\n\nThanks for the insight!\n\nRegards,\n\nYuchen\n"},{"id":"535991","messageId":"03F6CE03-751E-43D5-80E2-E799D97B09B2@gmail.com","threadId":"64936","inReplyTo":"CAFNBzOckR2yfGvLMHm0VZW+iKJTgFxzfxQAskdBV2HQ_3yXggA@mail.gmail.com","subject":"Re: [PATCH] [RFC][GSoC][PATCH] attr: use local repository state in read_attr","fromName":"Lucas Seiki Oshiro","fromEmail":"lucasseikioshiro@gmail.com","sentAt":"2026-02-14T00:04:35Z","receivedAt":"2026-02-14T00:04:51Z","isPatch":true,"sender":{"key":"lucasseikioshiro@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701580?v=4"},"body":"\n> Hello everyone,\n\nHello again, Ayush!\n\n> I’ve incorporated the feedback provided on this patch and sent an\n> updated version as a follow-up patch series:\n> \n> [RFC GSoC PATCH v3 0/2] Make read_attr() repository-aware by\n> introducing a lazy bare state\n> [RFC GSoC PATCH v3 1/2] repo-settings: add repo_settings_get_is_bare\n> [RFC GSoC PATCH v3 2/2] attr: use local repository state in read_attr\n\nTwo tips about working with our mailing lists:\n\n1. Normally here we sent next versions replying to the cover letter\n of the first version. You can use the flag --in-reply-to in\n git-send-email(1) to do that. (This is a Git thing, some other\n patch-based FLOSS projects don't do that).\n\n2. When we reference other messages, we use the message id instead of\n the subject. For example, I'm referencing your message [1] in this\n sentence.\n\n> Best regards,\n> Ayush Jha\n\nThanks and welcome!\n\n[1] CAFNBzOckR2yfGvLMHm0VZW+iKJTgFxzfxQAskdBV2HQ_3yXggA@mail.gmail.com/"},{"id":"536002","messageId":"CAFNBzOecmfobNTX_j+3E60avv=0XWG79KPc5JY_+kPqLHn=wdA@mail.gmail.com","threadId":"64936","inReplyTo":"03F6CE03-751E-43D5-80E2-E799D97B09B2@gmail.com","subject":"Re: [PATCH] [RFC][GSoC][PATCH] attr: use local repository state in read_attr","fromName":"Ayush Jha","fromEmail":"kumarayushjha123@gmail.com","sentAt":"2026-02-14T06:47:37Z","receivedAt":"2026-02-14T06:47:50Z","isPatch":true,"sender":{"key":"kumarayushjha123@gmail.com","avatar":null},"body":"Hi Lucas,\n\nThank you for the clarification and for explaining the mailing list conventions.\n\nI understand now that future versions should be sent in reply to the\noriginal cover letter using --in-reply-to, and that referencing\nmessages by their message-id is preferred. I’ll follow this approach\nin the next revision.\n\nThanks again for the guidance.\n\nBest regards,\nAyush Jha\n"}]}