{"thread":{"id":"62661","subject":"[PATCH] refs: exit early from the loop if it is not a main worktree","startedAt":"2024-12-18T02:20:48Z","lastAt":"2024-12-27T14:34:38Z","messageCount":8,"participants":["AreaZR via GitGitGadget","shejialuo","Eric Sunshine","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"509264","messageId":"pull.1848.git.git.1734488445457.gitgitgadget@gmail.com","threadId":"62661","inReplyTo":null,"subject":"[PATCH] refs: exit early from the loop if it is not a main worktree","fromName":"AreaZR via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-12-18T02:20:45Z","receivedAt":"2024-12-18T02:20:48Z","isPatch":true,"sender":{"key":"name:AreaZR","avatar":null},"body":"From: Seija Kijin \n\nThe is_main_worktree function just checks for !wt->id,\nbut the compiler doesn't know this as it is in a different\nfile, so just exit out early.\n\nSigned-off-by: Seija Kijin \n---\n refs: exit early from the loop if it is not a main worktree\n \n The is_main_worktree function just checks for !wt->id, but the compiler\n doesn't know this as it is in a different file, so just exit out early.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1848%2FAreaZR%2Fexit-early-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1848/AreaZR/exit-early-v1\nPull-Request: https://github.com/git/git/pull/1848\n\n refs.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/refs.c b/refs.c\nindex 8b713692359..cce63a618d7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2791,6 +2791,7 @@ static int has_worktrees(void)\n \t\tif (is_main_worktree(worktrees[i]))\n \t\t\tcontinue;\n \t\tret = 1;\n+\t\tbreak;\n \t}\n \n \tfree_worktrees(worktrees);\n\nbase-commit: 063bcebf0c917140ca0e705cbe0fdea127e90086\n-- \ngitgitgadget\n"},{"id":"509291","messageId":"Z2LOpOxu0oAY0DW3@ArchLinux","threadId":"62661","inReplyTo":"pull.1848.git.git.1734488445457.gitgitgadget@gmail.com","subject":"Re: [PATCH] refs: exit early from the loop if it is not a main worktree","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-12-18T13:31:16Z","receivedAt":"2024-12-18T13:30:36Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Wed, Dec 18, 2024 at 02:20:45AM +0000, AreaZR via GitGitGadget wrote:\n> From: Seija Kijin \n> \n> The is_main_worktree function just checks for !wt->id,\n> but the compiler doesn't know this as it is in a different\n> file, so just exit out early.\n> \n\nI think maybe we should exit out the loop early. However, the above\nstatement is confusing. As you have said, `is_main_worktree` checks\nwhether the `wt->id` is NULL. Why compiler doesn't know this? And why we\nneed to exit out the loop due to above reason?\n\n> Signed-off-by: Seija Kijin \n> ---\n> refs: exit early from the loop if it is not a main worktree\n> \n> The is_main_worktree function just checks for !wt->id, but the compiler\n> doesn't know this as it is in a different file, so just exit out early.\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1848%2FAreaZR%2Fexit-early-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1848/AreaZR/exit-early-v1\n> Pull-Request: https://github.com/git/git/pull/1848\n> \n> refs.c | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/refs.c b/refs.c\n> index 8b713692359..cce63a618d7 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2791,6 +2791,7 @@ static int has_worktrees(void)\n> \t\tif (is_main_worktree(worktrees[i]))\n> \t\t\tcontinue;\n> \t\tret = 1;\n> +\t\tbreak;\n\nSo, when we find a linked worktree, we just return the value. From my\nperspective, if we decide to optimize like this way, we could drop the\nloop because the first element of the result of `get_worktrees` is the\nmain worktree. And we could just check whether the \"worktrees[1]\" is\nNULL to do above.\n\nHowever, I don't know whether it's a good idea to exit the loop early\nin the first place. CC Patrick to help.\n\nThanks,\nJialuo\n"},{"id":"509314","messageId":"CAPig+cQd=vc5rte47biFbR+w_DV2OhdCRpC2WH_dKsSi4wvZ2A@mail.gmail.com","threadId":"62661","inReplyTo":"Z2LOpOxu0oAY0DW3@ArchLinux","subject":"Re: [PATCH] refs: exit early from the loop if it is not a main worktree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-12-18T23:52:02Z","receivedAt":"2024-12-18T23:52:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 18, 2024 at 8:30 AM shejialuo wrote:\n> On Wed, Dec 18, 2024 at 02:20:45AM +0000, AreaZR via GitGitGadget wrote:\n> > if (is_main_worktree(worktrees[i]))\n> > continue;\n> > ret = 1;\n> > + break;\n>\n> So, when we find a linked worktree, we just return the value. From my\n> perspective, if we decide to optimize like this way, we could drop the\n> loop because the first element of the result of `get_worktrees` is the\n> main worktree. And we could just check whether the \"worktrees[1]\" is\n> NULL to do above.\n\nYou're correct. get_worktrees() guarantees that the main worktree (or\nbare repository) is the first item in the list, so merely checking\nwhether `worktrees[1]` is non-NULL would be sufficient to answer\nwhether linked worktrees are present; no looping is required.\n\n> However, I don't know whether it's a good idea to exit the loop early\n> in the first place. CC Patrick to help.\n\nIf the loop is retained for some reason (though it really isn't\nneeded), then exiting early is indeed desirable. I suspect that the\nmissing `break` was just a silly oversight on Patrick's part.\n"},{"id":"509315","messageId":"xmqqv7vgo4u7.fsf@gitster.g","threadId":"62661","inReplyTo":"CAPig+cQd=vc5rte47biFbR+w_DV2OhdCRpC2WH_dKsSi4wvZ2A@mail.gmail.com","subject":"Re: [PATCH] refs: exit early from the loop if it is not a main worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-19T01:10:24Z","receivedAt":"2024-12-19T01:10:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Wed, Dec 18, 2024 at 8:30 AM shejialuo wrote:\n>> On Wed, Dec 18, 2024 at 02:20:45AM +0000, AreaZR via GitGitGadget wrote:\n>> > if (is_main_worktree(worktrees[i]))\n>> > continue;\n>> > ret = 1;\n>> > + break;\n>>\n>> So, when we find a linked worktree, we just return the value. From my\n>> perspective, if we decide to optimize like this way, we could drop the\n>> loop because the first element of the result of `get_worktrees` is the\n>> main worktree. And we could just check whether the \"worktrees[1]\" is\n>> NULL to do above.\n>\n> You're correct. get_worktrees() guarantees that the main worktree (or\n> bare repository) is the first item in the list, so merely checking\n> whether `worktrees[1]` is non-NULL would be sufficient to answer\n> whether linked worktrees are present; no looping is required.\n\nThanks for a well-reasoned write-up.\n\nWould many other callers potentially want to know if the repository\nhas more than one worktree? It looks to me that the has_worktrees()\nhelper function in refs.c is a sign that the worktree API is missing\na function. Calling get_worktrees() to prepare a list of worktrees\nand then counting the result, only to see if there are more than\none, sounds a bit wasteful if we need to do so too often.\n\n\n"},{"id":"509325","messageId":"CAPig+cTf=Lm_yNWQPq2SKfuROhSMMwNVXNmXhags43SRKonwxA@mail.gmail.com","threadId":"62661","inReplyTo":"xmqqv7vgo4u7.fsf@gitster.g","subject":"Re: [PATCH] refs: exit early from the loop if it is not a main worktree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-12-19T05:54:55Z","receivedAt":"2024-12-19T05:55:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 18, 2024 at 8:10 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > On Wed, Dec 18, 2024 at 8:30 AM shejialuo wrote:\n> >> On Wed, Dec 18, 2024 at 02:20:45AM +0000, AreaZR via GitGitGadget wrote:\n> >> > if (is_main_worktree(worktrees[i]))\n> >> > continue;\n> >> > ret = 1;\n> >> > + break;\n> >>\n> >> So, when we find a linked worktree, we just return the value. From my\n> >> perspective, if we decide to optimize like this way, we could drop the\n> >> loop because the first element of the result of `get_worktrees` is the\n> >> main worktree. And we could just check whether the \"worktrees[1]\" is\n> >> NULL to do above.\n> >\n> > You're correct. get_worktrees() guarantees that the main worktree (or\n> > bare repository) is the first item in the list, so merely checking\n> > whether `worktrees[1]` is non-NULL would be sufficient to answer\n> > whether linked worktrees are present; no looping is required.\n>\n> Would many other callers potentially want to know if the repository\n> has more than one worktree? It looks to me that the has_worktrees()\n> helper function in refs.c is a sign that the worktree API is missing\n> a function. Calling get_worktrees() to prepare a list of worktrees\n> and then counting the result, only to see if there are more than\n> one, sounds a bit wasteful if we need to do so too often.\n\nIf the need to answer this question does become common, then I can\nimagine a function being added to the worktree API which tries to be\nsmart about it by only calling readdir() -- and validating a\n.git/worktrees// metainformation -- enough times to be able to\nanswer the question.\n\nHowever, although I haven't audited the code, I suspect the question\n\"are there any linked worktrees\" is rare, possibly only asked by\n`refs.c`. And in that case, it is asked only at the start of a\nrefs-migration operation. Moreover, it appears that even that case of\nasking the question is probably temporary, existing only until someone\nextends the migration logic to work correctly in the presence of\nworktrees. (I'm sure Patrick can shed more light on this, though.)\n"},{"id":"509328","messageId":"xmqqcyhonquf.fsf@gitster.g","threadId":"62661","inReplyTo":"CAPig+cTf=Lm_yNWQPq2SKfuROhSMMwNVXNmXhags43SRKonwxA@mail.gmail.com","subject":"Re: [PATCH] refs: exit early from the loop if it is not a main worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-19T06:12:40Z","receivedAt":"2024-12-19T06:12:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> However, although I haven't audited the code, I suspect the question\n> \"are there any linked worktrees\" is rare, possibly only asked by\n> `refs.c`. And in that case, it is asked only at the start of a\n> refs-migration operation.\n\nThat matches my gut feeling. Thanks.\n\n"},{"id":"509634","messageId":"Z2665YeBYXUYB12D@pks.im","threadId":"62661","inReplyTo":"CAPig+cTf=Lm_yNWQPq2SKfuROhSMMwNVXNmXhags43SRKonwxA@mail.gmail.com","subject":"Re: [PATCH] refs: exit early from the loop if it is not a main worktree","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T14:34:13Z","receivedAt":"2024-12-27T14:34:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 19, 2024 at 12:54:55AM -0500, Eric Sunshine wrote:\n> On Wed, Dec 18, 2024 at 8:10 PM Junio C Hamano wrote:\n> > Eric Sunshine writes:\n> > > On Wed, Dec 18, 2024 at 8:30 AM shejialuo wrote:\n> > >> On Wed, Dec 18, 2024 at 02:20:45AM +0000, AreaZR via GitGitGadget wrote:\n> > >> > if (is_main_worktree(worktrees[i]))\n> > >> > continue;\n> > >> > ret = 1;\n> > >> > + break;\n> > >>\n> > >> So, when we find a linked worktree, we just return the value. From my\n> > >> perspective, if we decide to optimize like this way, we could drop the\n> > >> loop because the first element of the result of `get_worktrees` is the\n> > >> main worktree. And we could just check whether the \"worktrees[1]\" is\n> > >> NULL to do above.\n> > >\n> > > You're correct. get_worktrees() guarantees that the main worktree (or\n> > > bare repository) is the first item in the list, so merely checking\n> > > whether `worktrees[1]` is non-NULL would be sufficient to answer\n> > > whether linked worktrees are present; no looping is required.\n> >\n> > Would many other callers potentially want to know if the repository\n> > has more than one worktree? It looks to me that the has_worktrees()\n> > helper function in refs.c is a sign that the worktree API is missing\n> > a function. Calling get_worktrees() to prepare a list of worktrees\n> > and then counting the result, only to see if there are more than\n> > one, sounds a bit wasteful if we need to do so too often.\n> \n> If the need to answer this question does become common, then I can\n> imagine a function being added to the worktree API which tries to be\n> smart about it by only calling readdir() -- and validating a\n> .git/worktrees// metainformation -- enough times to be able to\n> answer the question.\n> \n> However, although I haven't audited the code, I suspect the question\n> \"are there any linked worktrees\" is rare, possibly only asked by\n> `refs.c`. And in that case, it is asked only at the start of a\n> refs-migration operation. Moreover, it appears that even that case of\n> asking the question is probably temporary, existing only until someone\n> extends the migration logic to work correctly in the presence of\n> worktrees. (I'm sure Patrick can shed more light on this, though.)\n\nOverall I have to say that I found the worktree APIs to be quite\nlacking. It's surprising that you don't even have a way to load a single\nworktree, for example.\n\nIn any case, for this specific case I'm not aware of any other users of\nthe functionality, so there is no immediate need to lift it up into the\nworktree subsystem.\n\nPatrick\n"},{"id":"509635","messageId":"Z2666eLeDbWE2yKc@pks.im","threadId":"62661","inReplyTo":"CAPig+cQd=vc5rte47biFbR+w_DV2OhdCRpC2WH_dKsSi4wvZ2A@mail.gmail.com","subject":"Re: [PATCH] refs: exit early from the loop if it is not a main worktree","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T14:34:17Z","receivedAt":"2024-12-27T14:34:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 18, 2024 at 06:52:02PM -0500, Eric Sunshine wrote:\n> On Wed, Dec 18, 2024 at 8:30 AM shejialuo wrote:\n> > On Wed, Dec 18, 2024 at 02:20:45AM +0000, AreaZR via GitGitGadget wrote:\n> > > if (is_main_worktree(worktrees[i]))\n> > > continue;\n> > > ret = 1;\n> > > + break;\n> >\n> > So, when we find a linked worktree, we just return the value. From my\n> > perspective, if we decide to optimize like this way, we could drop the\n> > loop because the first element of the result of `get_worktrees` is the\n> > main worktree. And we could just check whether the \"worktrees[1]\" is\n> > NULL to do above.\n> \n> You're correct. get_worktrees() guarantees that the main worktree (or\n> bare repository) is the first item in the list, so merely checking\n> whether `worktrees[1]` is non-NULL would be sufficient to answer\n> whether linked worktrees are present; no looping is required.\n> \n> > However, I don't know whether it's a good idea to exit the loop early\n> > in the first place. CC Patrick to help.\n> \n> If the loop is retained for some reason (though it really isn't\n> needed), then exiting early is indeed desirable. I suspect that the\n> missing `break` was just a silly oversight on Patrick's part.\n\nYes, indeed, breaking out of the loop feels sensible to me. Checking\nwhether `worktrees[1]` is non-NULL would be fine, as well.\n\nPatrick\n"}]}