{"thread":{"id":"55422","subject":"should git maintenance prefetch be taught to honor remote.fetch refspec?","startedAt":"2021-04-01T18:53:00Z","lastAt":"2021-04-05T20:54:54Z","messageCount":27,"participants":["Tom Saeger","Derrick Stolee","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"420841","messageId":"20210401184914.qmr7jhjbhp2mt3h6@dhcp-10-154-148-175.vpn.oracle.com","threadId":"55422","inReplyTo":null,"subject":"should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Tom Saeger","fromEmail":"tom.saeger@oracle.com","sentAt":"2021-04-01T18:49:14Z","receivedAt":"2021-04-01T18:53:00Z","isPatch":false,"sender":{"key":"tom.saeger@oracle.com","avatar":null},"body":"I've recently setup git maintenance and noticed prefetch task\nfetches ALL remote refs (well not tags) and does not honor\nremote fetch config settings.\n\nDetails:\n\n# narrowed set of remote refs produced by `git fetch --all --prune`\n$ git show-ref | grep \"refs/remotes/\" | wc -l\n81\n\n# git maintenance prefetch refs\n$ git show-ref | grep \"refs/prefetch/\" | wc -l\n262\n\n# git worktree with 7 remotes\n$ git remote | wc -l\n7\n\n# some remotes configured to fetch narrowed set of remote refs\n$ git config --local --list | grep \".fetch=\" | wc -l\n11\n\n# concrete example of remote fetch config\n$ git config --local --list | grep \"stable.fetch=\"\nremote.stable.fetch=+refs/heads/linux-4.14.y:refs/remotes/stable/linux-4.14.y\nremote.stable.fetch=+refs/heads/linux-5.4.y:refs/remotes/stable/linux-5.4.y\n\n\nShould git maintenance prefetch task be taught to honor remote fetch config settings?\n\n\nSomething equivalent to:\n\ngit config --local --list \\\n| sed -ne '/remote\\..*fetch=/{s#refs/remotes/#refs/prefetch/#;s#^remote\\.\\([^\\.]\\+\\)\\.fetch=#\\1 #;p;};' \\\n| while read -r remote refspec ; do\n git fetch ${remote} --prune --no-tags --no-write-fetch-head --recurse-submodules=no --refmap= --quiet ${refspec};\ndone\n\n\nRegards,\n\n--Tom\n"},{"id":"420845","messageId":"d246df21-fdaa-a391-847a-e03e8e664af1@gmail.com","threadId":"55422","inReplyTo":"20210401184914.qmr7jhjbhp2mt3h6@dhcp-10-154-148-175.vpn.oracle.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-01T19:07:37Z","receivedAt":"2021-04-01T19:27:58Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/1/2021 2:49 PM, Tom Saeger wrote:\n> I've recently setup git maintenance and noticed prefetch task\n> fetches ALL remote refs (well not tags) and does not honor\n> remote fetch config settings.\n\nThis is intentional. The point is to get the latest objects\nwithout modifying any local copies of refs. You still need\nto run \"git fetch\" manually to update the refs, but that\nshould be faster because you have most of the objects already\nin your copy of the repository.\n\nHere is the essential part of the documentation [1]:\n\n\tThe refmap is custom to avoid updating local or\n\tremote branches (those in refs/heads or refs/remotes).\n\tInstead, the remote refs are stored in\n\trefs/prefetch//. Also, tags are not updated.\n\n\tThis is done to avoid disrupting the remote-tracking\n\tbranches. The end users expect these refs to stay\n\tunmoved unless they initiate a fetch. With prefetch\n\ttask, however, the objects necessary to complete a\n\tlater real fetch would already be obtained, so the real\n\tfetch would go faster. In the ideal case, it will just\n\tbecome an update to a bunch of remote-tracking branches\n\twithout any object transfer.\n\n[1] https://git-scm.com/docs/git-maintenance#Documentation/git-maintenance.txt-prefetch\n\nThanks,\n-Stolee\n"},{"id":"420846","messageId":"20210401194231.aeazn5izmxsk5hcn@dhcp-10-154-148-175.vpn.oracle.com","threadId":"55422","inReplyTo":"d246df21-fdaa-a391-847a-e03e8e664af1@gmail.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Tom Saeger","fromEmail":"tom.saeger@oracle.com","sentAt":"2021-04-01T19:42:31Z","receivedAt":"2021-04-01T19:42:39Z","isPatch":false,"sender":{"key":"tom.saeger@oracle.com","avatar":null},"body":"On Thu, Apr 01, 2021 at 03:07:37PM -0400, Derrick Stolee wrote:\n> On 4/1/2021 2:49 PM, Tom Saeger wrote:\n> > I've recently setup git maintenance and noticed prefetch task\n> > fetches ALL remote refs (well not tags) and does not honor\n> > remote fetch config settings.\n> \n> This is intentional. The point is to get the latest objects\n> without modifying any local copies of refs. You still need\n> to run \"git fetch\" manually to update the refs, but that\n> should be faster because you have most of the objects already\n> in your copy of the repository.\n\nI get all that.\n\nIn my particular use-case - I constrained the fetch refspec to purposely\nlimit the number of objects and remote refs only to those of interest.\n\n\"git fetch\" won't ever use the other 120 refs that got pulled in by\nprefetch.\n\nIf the configured remote fetch refspecs were honored,\nprefetch would only update those objects/refs which `git fetch` would\nhave.\n\n> \n> Here is the essential part of the documentation [1]:\n> \n> \tThe refmap is custom to avoid updating local or\n> \tremote branches (those in refs/heads or refs/remotes).\n> \tInstead, the remote refs are stored in\n> \trefs/prefetch//. Also, tags are not updated.\n> \n> \tThis is done to avoid disrupting the remote-tracking\n> \tbranches. The end users expect these refs to stay\n> \tunmoved unless they initiate a fetch. With prefetch\n> \ttask, however, the objects necessary to complete a\n> \tlater real fetch would already be obtained, so the real\n> \tfetch would go faster. In the ideal case, it will just\n> \tbecome an update to a bunch of remote-tracking branches\n> \twithout any object transfer.\n> \n> [1] https://git-scm.com/docs/git-maintenance#Documentation/git-maintenance.txt-prefetch\n> \n> Thanks,\n> -Stolee\n"},{"id":"420851","messageId":"xmqq8s613gqa.fsf@gitster.g","threadId":"55422","inReplyTo":"d246df21-fdaa-a391-847a-e03e8e664af1@gmail.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-01T20:14:05Z","receivedAt":"2021-04-01T20:14:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 4/1/2021 2:49 PM, Tom Saeger wrote:\n>> I've recently setup git maintenance and noticed prefetch task\n>> fetches ALL remote refs (well not tags) and does not honor\n>> remote fetch config settings.\n>\n> This is intentional. The point is to get the latest objects\n> without modifying any local copies of refs. You still need\n> to run \"git fetch\" manually to update the refs, but that\n> should be faster because you have most of the objects already\n> in your copy of the repository.\n\nYou answered only half of the question, I think.\n\nThe plain vanilla refspec after you clone would be\n\n [remote \"origin\"]\n\tfetch = +refs/heads/*:refs/remotes/origin/*\n\nand \"maintenance prefetch\" intentionally redirect the destination\npart away from refs/remotes/origin/ to avoid disrupting the\nreference to @{upstream} etc. that are used locally. And\nintentionally doing so is good.\n\nBut imagine you are tracking my 'todo' branch which has nothing\ncommon with the history of Git. You'd have\n\n [remote \"origin\"]\n\turl = git://git.kernel.org/pub/scm/git/git.git\n\tfetch = +refs/heads/todo:refs/remotes/origin/todo\n\nIf \"maintenance prefetch\" does \"+refs/heads/*:refs/prefetch/origin/*\",\nyou'd end up the history of Git, which is not interesting for you who\nare only interested in the 'todo' branch (to be checked out in the\nMeta subdirectory to use Meta/cook script or read Meta/whats-cooking.txt\nfile).\n\nSo, redirecting the right-hand of configured refspec is a good idea;\nnot copying the left-hand of configured refspec, and unconditionally\nusing \"refs/heads/*\" is not.\n\n"},{"id":"420856","messageId":"20210401221108.oy66vviohvin5se3@brm-x62-17.us.oracle.com","threadId":"55422","inReplyTo":"xmqq8s613gqa.fsf@gitster.g","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Tom Saeger","fromEmail":"tom.saeger@oracle.com","sentAt":"2021-04-01T22:11:08Z","receivedAt":"2021-04-01T22:11:19Z","isPatch":false,"sender":{"key":"tom.saeger@oracle.com","avatar":null},"body":"On Thu, Apr 01, 2021 at 01:14:05PM -0700, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n> > On 4/1/2021 2:49 PM, Tom Saeger wrote:\n> >> I've recently setup git maintenance and noticed prefetch task\n> >> fetches ALL remote refs (well not tags) and does not honor\n> >> remote fetch config settings.\n> >\n> > This is intentional. The point is to get the latest objects\n> > without modifying any local copies of refs. You still need\n> > to run \"git fetch\" manually to update the refs, but that\n> > should be faster because you have most of the objects already\n> > in your copy of the repository.\n> \n> You answered only half of the question, I think.\n> \n> The plain vanilla refspec after you clone would be\n> \n> [remote \"origin\"]\n> \tfetch = +refs/heads/*:refs/remotes/origin/*\n> \n> and \"maintenance prefetch\" intentionally redirect the destination\n> part away from refs/remotes/origin/ to avoid disrupting the\n> reference to @{upstream} etc. that are used locally. And\n> intentionally doing so is good.\n> \n> But imagine you are tracking my 'todo' branch which has nothing\n> common with the history of Git. You'd have\n> \n> [remote \"origin\"]\n> \turl = git://git.kernel.org/pub/scm/git/git.git\n> \tfetch = +refs/heads/todo:refs/remotes/origin/todo\n> \n> If \"maintenance prefetch\" does \"+refs/heads/*:refs/prefetch/origin/*\",\n> you'd end up the history of Git, which is not interesting for you who\n> are only interested in the 'todo' branch (to be checked out in the\n> Meta subdirectory to use Meta/cook script or read Meta/whats-cooking.txt\n> file).\n\nYes - precisely.\n\n> \n> So, redirecting the right-hand of configured refspec is a good idea;\n> not copying the left-hand of configured refspec, and unconditionally\n> using \"refs/heads/*\" is not.\n> \n\nAnd multiple values can be set for remote.remotename.fetch\n\n\nBehavior equivalent to:\n\ngit remote | while read -r remote; do\n refspecs=$(\n git config --get-all \"remote.${remote}.fetch\" \\\n | sed 's#refs/remotes/#refs/prefetch/#' \\\n | xargs)\n git fetch \"${remote}\" --prune --no-tags --no-write-fetch-head --recurse-submodules=no --refmap= --quiet ${refspecs};\ndone\n"},{"id":"420857","messageId":"3bfd9a88-10f9-df71-bf96-f9c5654e48eb@gmail.com","threadId":"55422","inReplyTo":"xmqq8s613gqa.fsf@gitster.g","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-01T22:25:36Z","receivedAt":"2021-04-01T22:25:41Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/1/2021 4:14 PM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n>> On 4/1/2021 2:49 PM, Tom Saeger wrote:\n>>> I've recently setup git maintenance and noticed prefetch task\n>>> fetches ALL remote refs (well not tags) and does not honor\n>>> remote fetch config settings.\n>>\n>> This is intentional. The point is to get the latest objects\n>> without modifying any local copies of refs. You still need\n>> to run \"git fetch\" manually to update the refs, but that\n>> should be faster because you have most of the objects already\n>> in your copy of the repository.\n> \n> You answered only half of the question, I think.\n\nYou are right. Thanks, both, for pointing that out.\n \n> The plain vanilla refspec after you clone would be\n> \n> [remote \"origin\"]\n> \tfetch = +refs/heads/*:refs/remotes/origin/*\n> \n> and \"maintenance prefetch\" intentionally redirect the destination\n> part away from refs/remotes/origin/ to avoid disrupting the\n> reference to @{upstream} etc. that are used locally. And\n> intentionally doing so is good.\n> \n> But imagine you are tracking my 'todo' branch which has nothing\n> common with the history of Git. You'd have\n> \n> [remote \"origin\"]\n> \turl = git://git.kernel.org/pub/scm/git/git.git\n> \tfetch = +refs/heads/todo:refs/remotes/origin/todo\n> \n> If \"maintenance prefetch\" does \"+refs/heads/*:refs/prefetch/origin/*\",\n> you'd end up the history of Git, which is not interesting for you who\n> are only interested in the 'todo' branch (to be checked out in the\n> Meta subdirectory to use Meta/cook script or read Meta/whats-cooking.txt\n> file).\n> \n> So, redirecting the right-hand of configured refspec is a good idea;\n> not copying the left-hand of configured refspec, and unconditionally\n> using \"refs/heads/*\" is not.\n \nThis makes sense as a way to augment the feature. It doesn't seem\nlike a common scenario, but it would be good for users to have\nthat flexibility.\n\nUpon initial inspection, it shouldn't be too much work. However,\nthere is some generality to the refspec that might not be wholly\nappropriate for prefetch (such as the exact_sha1 option). I'm\nunfamiliar with the advanced forms of the refspec, so it'll take\nsome time to have confidence in this approach.\n\nThanks,\n-Stolee\n"},{"id":"420888","messageId":"20210402182716.trbaflsjcvouff2y@brm-x62-17.us.oracle.com","threadId":"55422","inReplyTo":"3bfd9a88-10f9-df71-bf96-f9c5654e48eb@gmail.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Tom Saeger","fromEmail":"tom.saeger@oracle.com","sentAt":"2021-04-02T18:27:16Z","receivedAt":"2021-04-02T18:27:26Z","isPatch":false,"sender":{"key":"tom.saeger@oracle.com","avatar":null},"body":"On Thu, Apr 01, 2021 at 06:25:36PM -0400, Derrick Stolee wrote:\n> On 4/1/2021 4:14 PM, Junio C Hamano wrote:\n> > Derrick Stolee writes:\n> > \n> >> On 4/1/2021 2:49 PM, Tom Saeger wrote:\n> >>> I've recently setup git maintenance and noticed prefetch task\n> >>> fetches ALL remote refs (well not tags) and does not honor\n> >>> remote fetch config settings.\n> >>\n> >> This is intentional. The point is to get the latest objects\n> >> without modifying any local copies of refs. You still need\n> >> to run \"git fetch\" manually to update the refs, but that\n> >> should be faster because you have most of the objects already\n> >> in your copy of the repository.\n> > \n> > You answered only half of the question, I think.\n> \n> You are right. Thanks, both, for pointing that out.\n> \n> > The plain vanilla refspec after you clone would be\n> > \n> > [remote \"origin\"]\n> > \tfetch = +refs/heads/*:refs/remotes/origin/*\n> > \n> > and \"maintenance prefetch\" intentionally redirect the destination\n> > part away from refs/remotes/origin/ to avoid disrupting the\n> > reference to @{upstream} etc. that are used locally. And\n> > intentionally doing so is good.\n> > \n> > But imagine you are tracking my 'todo' branch which has nothing\n> > common with the history of Git. You'd have\n> > \n> > [remote \"origin\"]\n> > \turl = git://git.kernel.org/pub/scm/git/git.git\n> > \tfetch = +refs/heads/todo:refs/remotes/origin/todo\n> > \n> > If \"maintenance prefetch\" does \"+refs/heads/*:refs/prefetch/origin/*\",\n> > you'd end up the history of Git, which is not interesting for you who\n> > are only interested in the 'todo' branch (to be checked out in the\n> > Meta subdirectory to use Meta/cook script or read Meta/whats-cooking.txt\n> > file).\n> > \n> > So, redirecting the right-hand of configured refspec is a good idea;\n> > not copying the left-hand of configured refspec, and unconditionally\n> > using \"refs/heads/*\" is not.\n> \n> This makes sense as a way to augment the feature. It doesn't seem\n> like a common scenario, but it would be good for users to have\n> that flexibility.\n\nIt's common for me, especially on repos requiring 'maintenance'.\n\n> \n> Upon initial inspection, it shouldn't be too much work. However,\n> there is some generality to the refspec that might not be wholly\n> appropriate for prefetch (such as the exact_sha1 option). I'm\n> unfamiliar with the advanced forms of the refspec, so it'll take\n> some time to have confidence in this approach.\n\nDidn't know about exact_sha1. prefetch probably wouldn't do\nanything in that case?\n\n'negative' refspecs - hmm haven't tried those. I see how that might\ncomplicate things or maybe not.\n\ngenerally isn't it still changing the right-hand side of refspec?\n\nreplacing \":refs/\" with \":refs/prefetch/\"\n\nThis would still work for refspecs with negative patterns right?\n\nI'm willing to help with/test/review this, thanks for investigating.\n\n\n> \n> Thanks,\n> -Stolee\n"},{"id":"420890","messageId":"41dc2961-7ba5-a882-3416-45631e2cbb33@gmail.com","threadId":"55422","inReplyTo":"20210402182716.trbaflsjcvouff2y@brm-x62-17.us.oracle.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-02T20:43:16Z","receivedAt":"2021-04-02T20:43:20Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/2/2021 2:27 PM, Tom Saeger wrote:\n> On Thu, Apr 01, 2021 at 06:25:36PM -0400, Derrick Stolee wrote:\n>> On 4/1/2021 4:14 PM, Junio C Hamano wrote:\n>>> Derrick Stolee writes:\n>>>\n>>>> On 4/1/2021 2:49 PM, Tom Saeger wrote:\n>>>\n>>> So, redirecting the right-hand of configured refspec is a good idea;\n>>> not copying the left-hand of configured refspec, and unconditionally\n>>> using \"refs/heads/*\" is not.\n>> \n>> This makes sense as a way to augment the feature. It doesn't seem\n>> like a common scenario, but it would be good for users to have\n>> that flexibility.\n> \n> It's common for me, especially on repos requiring 'maintenance'.\n\nI'm sure that once this is a tool in your belt, then it becomes\ncommon. The number of users who think about refspecs is likely a\nsmall proportion. But features should work as well as they can\nfor as many users as possible. There's a way forward here, it\njust is a little tricky due to the generality of refspecs.\n\n>> Upon initial inspection, it shouldn't be too much work. However,\n>> there is some generality to the refspec that might not be wholly\n>> appropriate for prefetch (such as the exact_sha1 option). I'm\n>> unfamiliar with the advanced forms of the refspec, so it'll take\n>> some time to have confidence in this approach.\n> \n> Didn't know about exact_sha1. prefetch probably wouldn't do\n> anything in that case?\n\nMy guess is that those should be dropped and ignored. But\nmaybe the approach below will still work?\n\n> 'negative' refspecs - hmm haven't tried those. I see how that might\n> complicate things or maybe not.\n> \n> generally isn't it still changing the right-hand side of refspec?\n> \n> replacing \":refs/\" with \":refs/prefetch/\"\n\nRight, this substring replacement might be easiest to achieve. The\n'struct refspec' doesn't make it incredibly easy. Perhaps skipping\nthe refspec parsing and just doing that substring swap directly from\nthe config value might be the best approach.\n\n> This would still work for refspecs with negative patterns right?\n\nOne of the issues is that negative patterns have no \":refs/\"\nsubstring.\n\nThe other issue is that exact matches (no \"*\") have an exact\nstring in the destination, too, so replacing the _entire_\ndestination with \"refs/prefetch//*\" breaks the refspec.\nI think the substring approach will still work here.\n\n> I'm willing to help with/test/review this, thanks for investigating.\nI have a branch available [1], but I'm seeing some failures only\non FreeBSD [2] and I can't understand why that platform is failing\nthis test. The current version (as of this writing) does not do\nthe substring replacement technique, and hence it just gives up\non exact matches. I will try the substring approach as an\nalternative and see where that gets me.\n\n[1] https://github.com/gitgitgadget/git/pull/924\n[2] https://github.com/gitgitgadget/git/pull/924/checks?check_run_id=2256079534\n\nThanks,\n-Stolee\n"},{"id":"420894","messageId":"a4cf3e48-aed5-ba1f-298d-1f2cc2359be2@gmail.com","threadId":"55422","inReplyTo":"41dc2961-7ba5-a882-3416-45631e2cbb33@gmail.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-02T21:07:09Z","receivedAt":"2021-04-02T21:07:22Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/2/2021 4:43 PM, Derrick Stolee wrote:\n> On 4/2/2021 2:27 PM, Tom Saeger wrote:\n>> generally isn't it still changing the right-hand side of refspec?\n>>\n>> replacing \":refs/\" with \":refs/prefetch/\"\n> \n> Right, this substring replacement might be easiest to achieve. The\n> 'struct refspec' doesn't make it incredibly easy. Perhaps skipping\n> the refspec parsing and just doing that substring swap directly from\n> the config value might be the best approach.\n> \n>> This would still work for refspecs with negative patterns right?\n> \n> One of the issues is that negative patterns have no \":refs/\"\n> substring.\n> \n> The other issue is that exact matches (no \"*\") have an exact\n> string in the destination, too, so replacing the _entire_\n> destination with \"refs/prefetch//*\" breaks the refspec.\n> I think the substring approach will still work here.\n\nI updated my branch with the substring approach, which is\nprobably the better solution. Please give it a try. I don't\nexpect that change to help the FreeBSD build, but we will see.\n \n> [1] https://github.com/gitgitgadget/git/pull/924\n> [2] https://github.com/gitgitgadget/git/pull/924/checks?check_run_id=2256079534\n\nThanks,\n-Stolee\n"},{"id":"420896","messageId":"20210402211519.l2bnzx5higqfcoyi@brm-x62-17.us.oracle.com","threadId":"55422","inReplyTo":"41dc2961-7ba5-a882-3416-45631e2cbb33@gmail.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Tom Saeger","fromEmail":"tom.saeger@oracle.com","sentAt":"2021-04-02T21:15:19Z","receivedAt":"2021-04-02T21:15:33Z","isPatch":false,"sender":{"key":"tom.saeger@oracle.com","avatar":null},"body":"On Fri, Apr 02, 2021 at 04:43:16PM -0400, Derrick Stolee wrote:\n> On 4/2/2021 2:27 PM, Tom Saeger wrote:\n> > On Thu, Apr 01, 2021 at 06:25:36PM -0400, Derrick Stolee wrote:\n> >> On 4/1/2021 4:14 PM, Junio C Hamano wrote:\n> >>> Derrick Stolee writes:\n> >>>\n> >>>> On 4/1/2021 2:49 PM, Tom Saeger wrote:\n> >>>\n> >>> So, redirecting the right-hand of configured refspec is a good idea;\n> >>> not copying the left-hand of configured refspec, and unconditionally\n> >>> using \"refs/heads/*\" is not.\n> >> \n> >> This makes sense as a way to augment the feature. It doesn't seem\n> >> like a common scenario, but it would be good for users to have\n> >> that flexibility.\n> > \n> > It's common for me, especially on repos requiring 'maintenance'.\n> \n> I'm sure that once this is a tool in your belt, then it becomes\n> common. The number of users who think about refspecs is likely a\n> small proportion. But features should work as well as they can\n> for as many users as possible. There's a way forward here, it\n> just is a little tricky due to the generality of refspecs.\n> \n> >> Upon initial inspection, it shouldn't be too much work. However,\n> >> there is some generality to the refspec that might not be wholly\n> >> appropriate for prefetch (such as the exact_sha1 option). I'm\n> >> unfamiliar with the advanced forms of the refspec, so it'll take\n> >> some time to have confidence in this approach.\n> > \n> > Didn't know about exact_sha1. prefetch probably wouldn't do\n> > anything in that case?\n> \n> My guess is that those should be dropped and ignored. But\n> maybe the approach below will still work?\n> \n> > 'negative' refspecs - hmm haven't tried those. I see how that might\n> > complicate things or maybe not.\n> > \n> > generally isn't it still changing the right-hand side of refspec?\n> > \n> > replacing \":refs/\" with \":refs/prefetch/\"\n> \n> Right, this substring replacement might be easiest to achieve. The\n> 'struct refspec' doesn't make it incredibly easy. Perhaps skipping\n> the refspec parsing and just doing that substring swap directly from\n> the config value might be the best approach.\n> \n> > This would still work for refspecs with negative patterns right?\n> \n> One of the issues is that negative patterns have no \":refs/\"\n> substring.\n> \n> The other issue is that exact matches (no \"*\") have an exact\n> string in the destination, too, so replacing the _entire_\n> destination with \"refs/prefetch//*\" breaks the refspec.\n> I think the substring approach will still work here.\n> \n> > I'm willing to help with/test/review this, thanks for investigating.\n> I have a branch available [1], but I'm seeing some failures only\n> on FreeBSD [2] and I can't understand why that platform is failing\n> this test. The current version (as of this writing) does not do\n> the substring replacement technique, and hence it just gives up\n> on exact matches. I will try the substring approach as an\n> alternative and see where that gets me.\n\nI also worked up a patch, not nearly as elegant as yours, but it did\nwork.\nI didn't think about changing fetch_remote to take struct remote like what\nyou've done.\n\nThanks - I'll give this a try.\n\n--Tom\n\n> \n> [1] https://github.com/gitgitgadget/git/pull/924\n> [2] https://github.com/gitgitgadget/git/pull/924/checks?check_run_id=2256079534\n> \n> Thanks,\n> -Stolee\n"},{"id":"420897","messageId":"xmqq1rbs4c6t.fsf@gitster.g","threadId":"55422","inReplyTo":"41dc2961-7ba5-a882-3416-45631e2cbb33@gmail.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-02T21:19:06Z","receivedAt":"2021-04-02T21:19:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> The other issue is that exact matches (no \"*\") have an exact\n> string in the destination, too, so replacing the _entire_\n> destination with \"refs/prefetch//*\" breaks the refspec.\n> I think the substring approach will still work here.\n\nI wonder if that matters. If the exact format says\n\n\t[remote \"origin\"]\n\t url = git://git.kernel.org/pub/scm/git/git.git/\n\t\tfetch = +refs/heads/todo:refs/x/y/z\n\nyou can just add refs/prefetch// to the entire RHS to ensure\nthat (1) the prefetch would not affect anything outside the 'prefetch'\nto break @{upstream} and friends, and (2) the prefetch from this remote\nwould not affect anything used for other remotes.\n\nIOW, the RHS, as long as it is hidden from normal operations and\ndoes not conflict with interaction with other repositories, where\nexactly in the refs hierarchy these objects are \"parked\" does not\nmatter, I would think.\n\nI do not recommend unparsed refspec and textually munging, by the\nway. Doesn't\n\n\tgit fetch master:remotes/origin/master\n\nfirst parse to normalize the src/dst sides to turn it into\n\n\tgit fetch refs/heads/master:refs/remotes/origin/master\n\nwhich is what you want to further redirect to the prefetch hierarchy\n\n\tgit fetch +refs/heads/master:refs/prefetch/origin/refs/remotes/origin/master\n\nor whatever munging scheme is used?\n\nAlso, I wonder if there should be a mechanism to prune the\nprefetched refs, but that is totally outside the scope of the\nproblem we have been discussing in this thread.\n"},{"id":"420901","messageId":"20210402213312.2rpsjqcuhl4n46il@brm-x62-17.us.oracle.com","threadId":"55422","inReplyTo":"xmqq1rbs4c6t.fsf@gitster.g","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Tom Saeger","fromEmail":"tom.saeger@oracle.com","sentAt":"2021-04-02T21:33:12Z","receivedAt":"2021-04-02T21:33:20Z","isPatch":false,"sender":{"key":"tom.saeger@oracle.com","avatar":null},"body":"On Fri, Apr 02, 2021 at 02:19:06PM -0700, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n> > The other issue is that exact matches (no \"*\") have an exact\n> > string in the destination, too, so replacing the _entire_\n> > destination with \"refs/prefetch//*\" breaks the refspec.\n> > I think the substring approach will still work here.\n> \n> I wonder if that matters. If the exact format says\n> \n> \t[remote \"origin\"]\n> \t url = git://git.kernel.org/pub/scm/git/git.git/\n> \t\tfetch = +refs/heads/todo:refs/x/y/z\n> \n> you can just add refs/prefetch// to the entire RHS to ensure\n> that (1) the prefetch would not affect anything outside the 'prefetch'\n> to break @{upstream} and friends, and (2) the prefetch from this remote\n> would not affect anything used for other remotes.\n> \n> IOW, the RHS, as long as it is hidden from normal operations and\n> does not conflict with interaction with other repositories, where\n> exactly in the refs hierarchy these objects are \"parked\" does not\n> matter, I would think.\n> \n> I do not recommend unparsed refspec and textually munging, by the\n> way. Doesn't\n> \n> \tgit fetch master:remotes/origin/master\n> \n> first parse to normalize the src/dst sides to turn it into\n> \n> \tgit fetch refs/heads/master:refs/remotes/origin/master\n> \n> which is what you want to further redirect to the prefetch hierarchy\n> \n> \tgit fetch +refs/heads/master:refs/prefetch/origin/refs/remotes/origin/master\n> \n> or whatever munging scheme is used?\n> \n> Also, I wonder if there should be a mechanism to prune the\n> prefetched refs, but that is totally outside the scope of the\n> problem we have been discussing in this thread.\n\nFWIW - did this locally to test:\n\ngit show-ref | grep -F \"prefetch\" | cut -d' ' -f2 | while read -r ref ; do git update-ref -d $ref; done\ngit maintenance run --task prefetch\ngit show-ref | egrep \"refs/remotes\" | wc -l\ngit show-ref | egrep \"refs/prefetch\" | wc -l\n"},{"id":"420904","messageId":"20210402213909.zuyyqqisvq7gjcxo@brm-x62-17.us.oracle.com","threadId":"55422","inReplyTo":"a4cf3e48-aed5-ba1f-298d-1f2cc2359be2@gmail.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Tom Saeger","fromEmail":"tom.saeger@oracle.com","sentAt":"2021-04-02T21:39:09Z","receivedAt":"2021-04-02T21:39:18Z","isPatch":false,"sender":{"key":"tom.saeger@oracle.com","avatar":null},"body":"On Fri, Apr 02, 2021 at 05:07:09PM -0400, Derrick Stolee wrote:\n> On 4/2/2021 4:43 PM, Derrick Stolee wrote:\n> > On 4/2/2021 2:27 PM, Tom Saeger wrote:\n> >> generally isn't it still changing the right-hand side of refspec?\n> >>\n> >> replacing \":refs/\" with \":refs/prefetch/\"\n> > \n> > Right, this substring replacement might be easiest to achieve. The\n> > 'struct refspec' doesn't make it incredibly easy. Perhaps skipping\n> > the refspec parsing and just doing that substring swap directly from\n> > the config value might be the best approach.\n> > \n> >> This would still work for refspecs with negative patterns right?\n> > \n> > One of the issues is that negative patterns have no \":refs/\"\n> > substring.\n> > \n> > The other issue is that exact matches (no \"*\") have an exact\n> > string in the destination, too, so replacing the _entire_\n> > destination with \"refs/prefetch//*\" breaks the refspec.\n> > I think the substring approach will still work here.\n> \n> I updated my branch with the substring approach, which is\n> probably the better solution. Please give it a try. I don't\n> expect that change to help the FreeBSD build, but we will see.\n\n\nThis worked for all the scenarios I tried, which had both negatives and\nmulti remote fetch values.\n\nLooks good!\n\nReviewed-by: Tom Saeger \n\n> \n> > [1] https://github.com/gitgitgadget/git/pull/924\n> > [2] https://github.com/gitgitgadget/git/pull/924/checks?check_run_id=2256079534\n> \n> Thanks,\n> -Stolee\n"},{"id":"420905","messageId":"xmqqft082vb6.fsf@gitster.g","threadId":"55422","inReplyTo":"20210402213909.zuyyqqisvq7gjcxo@brm-x62-17.us.oracle.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-02T22:09:01Z","receivedAt":"2021-04-02T22:09:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tom Saeger writes:\n\n> On Fri, Apr 02, 2021 at 05:07:09PM -0400, Derrick Stolee wrote:\n>> On 4/2/2021 4:43 PM, Derrick Stolee wrote:\n>> > On 4/2/2021 2:27 PM, Tom Saeger wrote:\n>> >> generally isn't it still changing the right-hand side of refspec?\n>> >>\n>> >> replacing \":refs/\" with \":refs/prefetch/\"\n>> > \n>> > Right, this substring replacement might be easiest to achieve. The\n>> > 'struct refspec' doesn't make it incredibly easy. Perhaps skipping\n>> > the refspec parsing and just doing that substring swap directly from\n>> > the config value might be the best approach.\n>> > \n>> >> This would still work for refspecs with negative patterns right?\n>> > \n>> > One of the issues is that negative patterns have no \":refs/\"\n>> > substring.\n>> > \n>> > The other issue is that exact matches (no \"*\") have an exact\n>> > string in the destination, too, so replacing the _entire_\n>> > destination with \"refs/prefetch//*\" breaks the refspec.\n>> > I think the substring approach will still work here.\n>> \n>> I updated my branch with the substring approach, which is\n>> probably the better solution. Please give it a try. I don't\n>> expect that change to help the FreeBSD build, but we will see.\n>\n>\n> This worked for all the scenarios I tried, which had both negatives and\n> multi remote fetch values.\n>\n> Looks good!\n>\n> Reviewed-by: Tom Saeger \n\nThat sounds more like tested-by, but anyway, thanks for working well\ntogether.\n\n\n\n>\n>> \n>> > [1] https://github.com/gitgitgadget/git/pull/924\n>> > [2] https://github.com/gitgitgadget/git/pull/924/checks?check_run_id=2256079534\n>> \n>> Thanks,\n>> -Stolee\n"},{"id":"420909","messageId":"20210402222746.722vf7tssojrzm26@brm-x62-17.us.oracle.com","threadId":"55422","inReplyTo":"xmqqft082vb6.fsf@gitster.g","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Tom Saeger","fromEmail":"tom.saeger@oracle.com","sentAt":"2021-04-02T22:27:46Z","receivedAt":"2021-04-02T22:27:56Z","isPatch":false,"sender":{"key":"tom.saeger@oracle.com","avatar":null},"body":"On Fri, Apr 02, 2021 at 03:09:01PM -0700, Junio C Hamano wrote:\n> Tom Saeger writes:\n> \n> > On Fri, Apr 02, 2021 at 05:07:09PM -0400, Derrick Stolee wrote:\n> >> On 4/2/2021 4:43 PM, Derrick Stolee wrote:\n> >> > On 4/2/2021 2:27 PM, Tom Saeger wrote:\n> >> >> generally isn't it still changing the right-hand side of refspec?\n> >> >>\n> >> >> replacing \":refs/\" with \":refs/prefetch/\"\n> >> > \n> >> > Right, this substring replacement might be easiest to achieve. The\n> >> > 'struct refspec' doesn't make it incredibly easy. Perhaps skipping\n> >> > the refspec parsing and just doing that substring swap directly from\n> >> > the config value might be the best approach.\n> >> > \n> >> >> This would still work for refspecs with negative patterns right?\n> >> > \n> >> > One of the issues is that negative patterns have no \":refs/\"\n> >> > substring.\n> >> > \n> >> > The other issue is that exact matches (no \"*\") have an exact\n> >> > string in the destination, too, so replacing the _entire_\n> >> > destination with \"refs/prefetch//*\" breaks the refspec.\n> >> > I think the substring approach will still work here.\n> >> \n> >> I updated my branch with the substring approach, which is\n> >> probably the better solution. Please give it a try. I don't\n> >> expect that change to help the FreeBSD build, but we will see.\n> >\n> >\n> > This worked for all the scenarios I tried, which had both negatives and\n> > multi remote fetch values.\n> >\n> > Looks good!\n> >\n> > Reviewed-by: Tom Saeger \n> \n> That sounds more like tested-by, but anyway, thanks for working well\n> together.\n\nTested-by: Tom Saeger \n\nworks for me, I did review the code but perhaps it's best to leave\nreviews to others.\n\n> \n> \n> \n> >\n> >> \n> >> > [1] https://github.com/gitgitgadget/git/pull/924\n> >> > [2] https://github.com/gitgitgadget/git/pull/924/checks?check_run_id=2256079534\n> >> \n> >> Thanks,\n> >> -Stolee\n"},{"id":"420910","messageId":"CAPig+cRxjLObLvF9kUAuftCxQ+iiFEisagDBWpAAPwdVMUATKQ@mail.gmail.com","threadId":"55422","inReplyTo":"41dc2961-7ba5-a882-3416-45631e2cbb33@gmail.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-04-02T22:32:56Z","receivedAt":"2021-04-02T22:33:10Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 2, 2021 at 4:43 PM Derrick Stolee wrote:\n> I have a branch available [1], but I'm seeing some failures only\n> on FreeBSD [2] and I can't understand why that platform is failing\n> this test. The current version (as of this writing) does not do\n> the substring replacement technique, and hence it just gives up\n> on exact matches. I will try the substring approach as an\n> alternative and see where that gets me.\n>\n> [1] https://github.com/gitgitgadget/git/pull/924\n> [2] https://github.com/gitgitgadget/git/pull/924/checks?check_run_id=2256079534\n\nThe \"+\" in patterns such as `+refs/heads/\\\\*:refs/prefetch...` is what\nis throwing it off. FreeBSD `grep` doesn't seem to like it, though\nit's not clear why. Escaping it the same way as you escaped \"*\"\ndoesn't make it work. Replacing \"+\" with catchall \".\" does work, so\nthat's one way to fix it.\n\nHowever, all the escaping you need to do in these refspec patterns to\npass them to `grep` is ugly. A much better solution may be to change\nthe `grep` in test-lib-functions.sh:test_subcommand() to `grep -F` to\nforce it to match literally. That way, you can drop all the backslash\nescaping, including those in front of \"[\" and \"]\". A cursory audit of\ncallers test_subcommand() seems to indicate that none of them pass\nregex patterns, so using `-F` is probably safe and a good idea.\n\nBy the way, the `coccinelle` check is also \"failing\", correctly\nsuggesting that you change:\n\n strbuf_addf(&replace, \":refs/prefetch/\");\n\nto:\n\n strbuf_addstr(&replace, \":refs/prefetch/\");\n\nin `builtin/gc.c`.\n"},{"id":"420942","messageId":"20210403202142.w4b25fhptcaguxyx@brm-x62-17.us.oracle.com","threadId":"55422","inReplyTo":"CAPig+cRxjLObLvF9kUAuftCxQ+iiFEisagDBWpAAPwdVMUATKQ@mail.gmail.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Tom Saeger","fromEmail":"tom.saeger@oracle.com","sentAt":"2021-04-03T20:21:42Z","receivedAt":"2021-04-03T20:22:00Z","isPatch":false,"sender":{"key":"tom.saeger@oracle.com","avatar":null},"body":"On Fri, Apr 02, 2021 at 06:32:56PM -0400, Eric Sunshine wrote:\n> On Fri, Apr 2, 2021 at 4:43 PM Derrick Stolee wrote:\n> > I have a branch available [1], but I'm seeing some failures only\n> > on FreeBSD [2] and I can't understand why that platform is failing\n> > this test. The current version (as of this writing) does not do\n> > the substring replacement technique, and hence it just gives up\n> > on exact matches. I will try the substring approach as an\n> > alternative and see where that gets me.\n> >\n> > [1] https://github.com/gitgitgadget/git/pull/924\n> > [2] https://github.com/gitgitgadget/git/pull/924/checks?check_run_id=2256079534\n> \n> The \"+\" in patterns such as `+refs/heads/\\\\*:refs/prefetch...` is what\n> is throwing it off. FreeBSD `grep` doesn't seem to like it, though\n> it's not clear why. Escaping it the same way as you escaped \"*\"\n> doesn't make it work. Replacing \"+\" with catchall \".\" does work, so\n> that's one way to fix it.\n> \n> However, all the escaping you need to do in these refspec patterns to\n> pass them to `grep` is ugly. A much better solution may be to change\n> the `grep` in test-lib-functions.sh:test_subcommand() to `grep -F` to\n> force it to match literally. That way, you can drop all the backslash\n> escaping, including those in front of \"[\" and \"]\". A cursory audit of\n> callers test_subcommand() seems to indicate that none of them pass\n> regex patterns, so using `-F` is probably safe and a good idea.\n> \n> By the way, the `coccinelle` check is also \"failing\", correctly\n> suggesting that you change:\n> \n> strbuf_addf(&replace, \":refs/prefetch/\");\n> \n> to:\n> \n> strbuf_addstr(&replace, \":refs/prefetch/\");\n> \n> in `builtin/gc.c`.\n\nWas curious - so rolled all Eric's feedback into a patch.\n\nThis passes 'make test' and 'make coccicheck' locally, not sure about FreeBSD.\n\nStolee - can you squash this onto your PR and try it if you agree?\n\n--Tom\n\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 98229bda25fd..eb4a03a2b47b 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -894,7 +894,7 @@ static int fetch_remote(struct remote *remote, void *cbdata)\n \t\tif (index_of_refs) {\n \t\t\tstruct strbuf replace = STRBUF_INIT;\n \t\t\tstrbuf_add(&replace, raw, index_of_refs - raw);\n-\t\t\tstrbuf_addf(&replace, \":refs/prefetch/\");\n+\t\t\tstrbuf_addstr(&replace, \":refs/prefetch/\");\n \t\t\tstrbuf_addstr(&replace, index_of_refs + 6);\n \n \t\t\tstrvec_push(&child.args, replace.buf);\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 93ae93b73611..3366ea188782 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -142,8 +142,8 @@ test_expect_success 'prefetch multiple remotes' '\n \ttest_commit -C clone2 two &&\n \tGIT_TRACE2_EVENT=\"$(pwd)/run-prefetch.txt\" git maintenance run --task=prefetch 2>/dev/null &&\n \tfetchargs=\"--prune --no-tags --no-write-fetch-head --recurse-submodules=no --refmap= --quiet\" &&\n-\ttest_subcommand git fetch remote1 $fetchargs +refs/heads/\\\\*:refs/prefetch/remotes/remote1/\\\\* On Fri, Apr 02, 2021 at 06:32:56PM -0400, Eric Sunshine wrote:\n>> On Fri, Apr 2, 2021 at 4:43 PM Derrick Stolee wrote:\n>>> I have a branch available [1], but I'm seeing some failures only\n>>> on FreeBSD [2] and I can't understand why that platform is failing\n>>> this test. The current version (as of this writing) does not do\n>>> the substring replacement technique, and hence it just gives up\n>>> on exact matches. I will try the substring approach as an\n>>> alternative and see where that gets me.\n>>>\n>>> [1] https://github.com/gitgitgadget/git/pull/924\n>>> [2] https://github.com/gitgitgadget/git/pull/924/checks?check_run_id=2256079534\n>>\n>> The \"+\" in patterns such as `+refs/heads/\\\\*:refs/prefetch...` is what\n>> is throwing it off. FreeBSD `grep` doesn't seem to like it, though\n>> it's not clear why. Escaping it the same way as you escaped \"*\"\n>> doesn't make it work. Replacing \"+\" with catchall \".\" does work, so\n>> that's one way to fix it.\n>>\n>> However, all the escaping you need to do in these refspec patterns to\n>> pass them to `grep` is ugly. A much better solution may be to change\n>> the `grep` in test-lib-functions.sh:test_subcommand() to `grep -F` to\n>> force it to match literally. That way, you can drop all the backslash\n>> escaping, including those in front of \"[\" and \"]\". A cursory audit of\n>> callers test_subcommand() seems to indicate that none of them pass\n>> regex patterns, so using `-F` is probably safe and a good idea.\n\nYes, this is an excellent idea. Thanks.\n\n>> By the way, the `coccinelle` check is also \"failing\", correctly\n>> suggesting that you change:\n>>\n>> strbuf_addf(&replace, \":refs/prefetch/\");\n>>\n>> to:\n>>\n>> strbuf_addstr(&replace, \":refs/prefetch/\");\n>>\n>> in `builtin/gc.c`.\n\nGood point! I'll get that, too.\n\n> Was curious - so rolled all Eric's feedback into a patch.\n> \n> This passes 'make test' and 'make coccicheck' locally, not sure about FreeBSD.\n> \n> Stolee - can you squash this onto your PR and try it if you agree?\n\nI made my own version of a patch before seeing this one, but\nthanks for working on it.\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 6348e8d7339c..e9a1cf3e227a 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -1652,9 +1652,9 @@ test_subcommand () {\n> \n> \tif test -n \"$negate\"\n> \tthen\n> -\t\t! grep \"\\[$expr\\]\"\n> +\t\t! grep -F \"$expr\"\n> \telse\n> -\t\tgrep \"\\[$expr\\]\"\n> +\t\tgrep -F \"$expr\"\n> \tfi\n\nSpecifically I would use \"[$expr]\" here so it includes the\nterminating characters of the JSON array that we are testing\nagainst. It can be important that we have the complete set\nof arguments, so these brackets are important.\n\nThanks, all. I'm taking a look at a mechanism for doing this\nwithout munging the raw string directly, as Junio mentioned.\nIt might require adding an output format method in the refspec\nAPI, but that's probably a good thing, anyway.\n\nThanks,\n-Stolee\n"},{"id":"420973","messageId":"f113284b-a7fe-ba7f-ce1c-d214efd5d0c6@gmail.com","threadId":"55422","inReplyTo":"xmqq1rbs4c6t.fsf@gitster.g","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-04T20:25:44Z","receivedAt":"2021-04-04T20:25:55Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"\n\nOn 4/2/2021 5:19 PM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n>> The other issue is that exact matches (no \"*\") have an exact\n>> string in the destination, too, so replacing the _entire_\n>> destination with \"refs/prefetch//*\" breaks the refspec.\n>> I think the substring approach will still work here.\n> \n> I wonder if that matters. If the exact format says\n> \n> \t[remote \"origin\"]\n> \t url = git://git.kernel.org/pub/scm/git/git.git/\n> \t\tfetch = +refs/heads/todo:refs/x/y/z\n> \n> you can just add refs/prefetch// to the entire RHS to ensure\n> that (1) the prefetch would not affect anything outside the 'prefetch'\n> to break @{upstream} and friends, and (2) the prefetch from this remote\n> would not affect anything used for other remotes.\n> \n> IOW, the RHS, as long as it is hidden from normal operations and\n> does not conflict with interaction with other repositories, where\n> exactly in the refs hierarchy these objects are \"parked\" does not\n> matter, I would think.\n> \n> I do not recommend unparsed refspec and textually munging, by the\n> way. Doesn't\n> \n> \tgit fetch master:remotes/origin/master\n> \n> first parse to normalize the src/dst sides to turn it into\n> \n> \tgit fetch refs/heads/master:refs/remotes/origin/master\n\nI do not see evidence of this being reflected in my testing. The\ngood news is that I'm creating a test scenario that will give us\na clear way to test what the refspec parsing (and new output\nformat logic) is doing.\n\nNow, it might be that we are having an equivalent behavior to\nwhat you are suggesting because of the way refs are interpreted\nwhen those refs are created based on the refspec. This complicates\nthings slightly, and I can work to see if there is a good way to\nmodify the refspec parsing to make this an explicit assumption.\n\n> which is what you want to further redirect to the prefetch hierarchy\n> \n> \tgit fetch +refs/heads/master:refs/prefetch/origin/refs/remotes/origin/master\n> \n> or whatever munging scheme is used?\n\nThe replacement will use the existing refspec's patterns for\npartitioning the refspace, except we add 'prefetch' and no longer\ninsert the remote name ourselves. This allows us to have shorter\nrefnames while respecting the given layout as much as possible.\n\n> Also, I wonder if there should be a mechanism to prune the\n> prefetched refs, but that is totally outside the scope of the\n> problem we have been discussing in this thread.\n \nYes, this is a good idea. Prefetch refs are going to add the\n\"force\" option (starts with '+') because these refs should be\nupdated automatically when the remote refs are force-pushed.\nHaving a mechanism for removing dropped refs is a good idea.\n\nThanks,\n-Stolee\n"},{"id":"420974","messageId":"xmqqft057ijc.fsf@gitster.g","threadId":"55422","inReplyTo":"f113284b-a7fe-ba7f-ce1c-d214efd5d0c6@gmail.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-04T23:10:31Z","receivedAt":"2021-04-04T23:10:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n>> I do not recommend unparsed refspec and textually munging, by the\n>> way. Doesn't\n>> \n>> \tgit fetch master:remotes/origin/master\n>> \n>> first parse to normalize the src/dst sides to turn it into\n>> \n>> \tgit fetch refs/heads/master:refs/remotes/origin/master\n\nI tried to jug my memory in this area a bit by reading the relevant\ncode. For non-wildcard refspec, e.g. with\n\n [remote \"origin\"]\n\turl = ../git.git/\n\tfetch = master:remotes/origin/master\n\ttagopt = --no-tags\n\nyou'd get\n\n $ git fetch -v\n From ../git\n * [new branch] master -> origin/master\n $ git for-each-ref\n 2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48 commit\trefs/remotes/origin/master\n\nIt all happens inside remote.c::get_fetch_map(), I think.\n\nThanks.\n"},{"id":"420987","messageId":"e75b1e72-6c9f-d466-ac52-24b324b44b3c@gmail.com","threadId":"55422","inReplyTo":"xmqqft057ijc.fsf@gitster.g","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-05T13:20:41Z","receivedAt":"2021-04-05T13:20:47Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/4/2021 7:10 PM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n>> On 4/2/2021 5:19 PM, Junio C Hamano wrote:\n>>> I do not recommend unparsed refspec and textually munging, by the\n>>> way. Doesn't\n>>>\n>>> \tgit fetch master:remotes/origin/master\n>>>\n>>> first parse to normalize the src/dst sides to turn it into\n>>>\n>>> \tgit fetch refs/heads/master:refs/remotes/origin/master\n> \n> I tried to jug my memory in this area a bit by reading the relevant\n> code. For non-wildcard refspec, e.g. with\n> \n> [remote \"origin\"]\n> \turl = ../git.git/\n> \tfetch = master:remotes/origin/master\n> \ttagopt = --no-tags\n> \n> you'd get\n> \n> $ git fetch -v\n> From ../git\n> * [new branch] master -> origin/master\n> $ git for-each-ref\n> 2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48 commit\trefs/remotes/origin/master\n> \n> It all happens inside remote.c::get_fetch_map(), I think.\n\nI see there that some refs are being matched with some\nexpectation of remote refs inside get_remote_ref() (which\ncalls find_ref_by_name_abbrev() as a helper). There does not\nappear to be any place that modifies the refs directly in\ngeneral, and instead refs are matched from the refspec using\nstandard short-ref rules.\n\nThis is particularly shown in examples like \"git push topic\"\nnever modifying the push-refspec from having 'src' equal to\n\"topic\" and instead the ref machinery discovers that \"topic\"\nreally means \"refs/heads/topic\".\n\nI took your advice to not munge raw refpsecs and instead\nworked directly on the 'dst' of the struct refspec_item. This\nrequired adding a method that formats a refspec, which I test\ncarefully. I spent about an hour trying to have parse_refspec()\nadd the appropriate \"refs/\" prefix, but it becomes difficult\nwhen we intend to make a distinction between \"refs/heads/\" and\nother sub-areas within \"refs/\". Finally, I punted on that\nconversion and made the logic in 'prefetch' extremely obvious:\n\n1. If the refspec's 'dst' starts with \"refs/\", then replace\n that prefix with \"refs/prefetch/\".\n\n2. If the refspec's 'dst' does not start with \"refs/\", then\n concatenate \"refs/prefetch/\" and 'dst'.\n\nThis will keep a roughly-equivalent partition of refs (some\nmight have previously collided that will not any more).\n\nI have posted my patch series [1], so please take a look. It\nbuilds up the infrastructure to properly test such a refspec\nexpansion, if we wish to do so.\n\n[1] https://lore.kernel.org/git/pull.924.git.1617627856.gitgitgadget@gmail.com/\n\nThanks,\n-Stolee\n"},{"id":"421002","messageId":"xmqqr1jo5zzb.fsf@gitster.g","threadId":"55422","inReplyTo":"e75b1e72-6c9f-d466-ac52-24b324b44b3c@gmail.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-05T18:48:56Z","receivedAt":"2021-04-05T18:49:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> ... but it becomes difficult\n> when we intend to make a distinction between \"refs/heads/\" and\n> other sub-areas within \"refs/\". Finally, I punted on that\n> conversion and made the logic in 'prefetch' extremely obvious:\n>\n> 1. If the refspec's 'dst' starts with \"refs/\", then replace\n> that prefix with \"refs/prefetch/\".\n>\n> 2. If the refspec's 'dst' does not start with \"refs/\", then\n> concatenate \"refs/prefetch/\" and 'dst'.\n>\n> This will keep a roughly-equivalent partition of refs (some\n> might have previously collided that will not any more).\n\nMakes sense. Do we need to add another rule?\n\n3. If the refspec does not have 'dst', ignore it.\n\n> I have posted my patch series [1], so please take a look. It\n> builds up the infrastructure to properly test such a refspec\n> expansion, if we wish to do so.\n>\n> [1] https://lore.kernel.org/git/pull.924.git.1617627856.gitgitgadget@gmail.com/\n>\n> Thanks,\n> -Stolee\n\nThanks.\n"},{"id":"421009","messageId":"20210405203832.x275fqoinfsw5gik@dhcp-10-154-153-195.vpn.oracle.com","threadId":"55422","inReplyTo":"xmqqr1jo5zzb.fsf@gitster.g","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Tom Saeger","fromEmail":"tom.saeger@oracle.com","sentAt":"2021-04-05T20:38:32Z","receivedAt":"2021-04-05T20:38:42Z","isPatch":false,"sender":{"key":"tom.saeger@oracle.com","avatar":null},"body":"On Mon, Apr 05, 2021 at 11:48:56AM -0700, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n> > ... but it becomes difficult\n> > when we intend to make a distinction between \"refs/heads/\" and\n> > other sub-areas within \"refs/\". Finally, I punted on that\n> > conversion and made the logic in 'prefetch' extremely obvious:\n> >\n> > 1. If the refspec's 'dst' starts with \"refs/\", then replace\n> > that prefix with \"refs/prefetch/\".\n> >\n> > 2. If the refspec's 'dst' does not start with \"refs/\", then\n> > concatenate \"refs/prefetch/\" and 'dst'.\n> >\n> > This will keep a roughly-equivalent partition of refs (some\n> > might have previously collided that will not any more).\n> \n> Makes sense. Do we need to add another rule?\n> \n> 3. If the refspec does not have 'dst', ignore it.\n\nI just tried what I think you're saying here.\n\nConsider this fetch config:\n\n$ git config --local --get-regexp \"pr-924\"\nremote.pr-924.url https://github.com/gitgitgadget/git\nremote.pr-924.fetch +refs/tags/pr-924/derrickstolee/maintenance/refspec-v1\n\nSeems legal, fetch even works\n\n$ git fetch pr-924\nFetching pr-924\nFrom https://github.com/gitgitgadget/git\n * tag pr-924/derrickstolee/maintenance/refspec-v1 -> FETCH_HEAD\n\n\nPrefetch segfaults\n\n$ git maintenance run --task prefetch\n[1] 3811315 segmentation fault (core dumped) git maintenance run --task prefetch\n\n\nrefspec which isn't a 'negative' with a src, but no dst.\n\n\nQuick hack:\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 92cb8b4e0bfa..2f4b8f2375c3 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -899,6 +899,10 @@ static int fetch_remote(struct remote *remote, void *cbdata)\n continue;\n }\n\n+ if (!rsi->dst) {\n+ continue;\n+ }\n+\n refspec_item_init(&replace, remote->fetch.raw[i], 1);\n\n /*\n\n\nBetter, but now a different issue:\n\n$ git maintenance run --task prefetch\nfatal: --refmap option is only meaningful with command-line refspec(s).\nerror: failed to prefetch remotes\nerror: task 'prefetch' failed\n\nSo a \"remote\" where the only fetch url is skipped.\n\n\n> \n> > I have posted my patch series [1], so please take a look. It\n> > builds up the infrastructure to properly test such a refspec\n> > expansion, if we wish to do so.\n> >\n> > [1] https://lore.kernel.org/git/pull.924.git.1617627856.gitgitgadget@gmail.com/\n> >\n> > Thanks,\n> > -Stolee\n> \n> Thanks.\n"},{"id":"421010","messageId":"xmqq5z105uia.fsf@gitster.g","threadId":"55422","inReplyTo":"20210405203832.x275fqoinfsw5gik@dhcp-10-154-153-195.vpn.oracle.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-05T20:47:09Z","receivedAt":"2021-04-05T20:47:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tom Saeger writes:\n\n> $ git config --local --get-regexp \"pr-924\"\n> remote.pr-924.url https://github.com/gitgitgadget/git\n> remote.pr-924.fetch +refs/tags/pr-924/derrickstolee/maintenance/refspec-v1\n>\n> Seems legal, fetch even works\n\nYes. For a ref that is one-shot use (like PR tags), this does not\nmake much sense, but\n\n [remote \"submaintainer1\"]\n\turl = ... repository of submaintainer #1 ...\n\tfetch = master\n\ttagopts = --no-tags\n\nis a reasonable thing to have for those who regularly work with\nsubmaintainer(s) of their project. They'd do\n\n\t$ git pull submaintainer1\n\nto accept the work their submaintainers have done.\n"},{"id":"421011","messageId":"5c432329-1e42-c29c-242d-a2955174296b@gmail.com","threadId":"55422","inReplyTo":"xmqq5z105uia.fsf@gitster.g","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-05T20:49:29Z","receivedAt":"2021-04-05T20:49:35Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/5/2021 4:47 PM, Junio C Hamano wrote:\n> Tom Saeger writes:\n> \n>> $ git config --local --get-regexp \"pr-924\"\n>> remote.pr-924.url https://github.com/gitgitgadget/git\n>> remote.pr-924.fetch +refs/tags/pr-924/derrickstolee/maintenance/refspec-v1\n>>\n>> Seems legal, fetch even works\n> \n> Yes. For a ref that is one-shot use (like PR tags), this does not\n> make much sense, but\n> \n> [remote \"submaintainer1\"]\n> \turl = ... repository of submaintainer #1 ...\n> \tfetch = master\n> \ttagopts = --no-tags\n> \n> is a reasonable thing to have for those who regularly work with\n> submaintainer(s) of their project. They'd do\n> \n> \t$ git pull submaintainer1\n> \n> to accept the work their submaintainers have done.\n\nThanks for the extra testing! I'll be sure to fix this bug in v2.\n\n-Stolee\n \n"},{"id":"421012","messageId":"20210405205056.czzkjdxr3guca3mf@dhcp-10-154-153-195.vpn.oracle.com","threadId":"55422","inReplyTo":"5c432329-1e42-c29c-242d-a2955174296b@gmail.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Tom Saeger","fromEmail":"tom.saeger@oracle.com","sentAt":"2021-04-05T20:50:56Z","receivedAt":"2021-04-05T20:51:08Z","isPatch":false,"sender":{"key":"tom.saeger@oracle.com","avatar":null},"body":"On Mon, Apr 05, 2021 at 04:49:29PM -0400, Derrick Stolee wrote:\n> On 4/5/2021 4:47 PM, Junio C Hamano wrote:\n> > Tom Saeger writes:\n> > \n> >> $ git config --local --get-regexp \"pr-924\"\n> >> remote.pr-924.url https://github.com/gitgitgadget/git\n> >> remote.pr-924.fetch +refs/tags/pr-924/derrickstolee/maintenance/refspec-v1\n> >>\n> >> Seems legal, fetch even works\n> > \n> > Yes. For a ref that is one-shot use (like PR tags), this does not\n> > make much sense, but\n> > \n> > [remote \"submaintainer1\"]\n> > \turl = ... repository of submaintainer #1 ...\n> > \tfetch = master\n> > \ttagopts = --no-tags\n> > \n> > is a reasonable thing to have for those who regularly work with\n> > submaintainer(s) of their project. They'd do\n> > \n> > \t$ git pull submaintainer1\n> > \n> > to accept the work their submaintainers have done.\n> \n> Thanks for the extra testing! I'll be sure to fix this bug in v2.\n> \n> -Stolee\n> \n\nHacked this:\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 92cb8b4e0bfa..8c0fcbd3bb7e 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -879,6 +879,7 @@ static int fetch_remote(struct remote *remote, void *cbdata)\n struct maintenance_run_opts *opts = cbdata;\n struct child_process child = CHILD_PROCESS_INIT;\n int i;\n+ int nargs;\n\n child.git_cmd = 1;\n strvec_pushl(&child.args, \"fetch\", remote->name, \"--prune\", \"--no-tags\",\n@@ -888,6 +889,8 @@ static int fetch_remote(struct remote *remote, void *cbdata)\n if (opts->quiet)\n strvec_push(&child.args, \"--quiet\");\n\n+ nargs = child.args.nr;\n+\n for (i = 0; i < remote->fetch.nr; i++) {\n struct refspec_item replace;\n struct refspec_item *rsi = &remote->fetch.items[i];\n@@ -899,6 +902,10 @@ static int fetch_remote(struct remote *remote, void *cbdata)\n continue;\n }\n\n+ if (!rsi->dst) {\n+ continue;\n+ }\n+\n refspec_item_init(&replace, remote->fetch.raw[i], 1);\n\n /*\n@@ -920,6 +927,10 @@ static int fetch_remote(struct remote *remote, void *cbdata)\n refspec_item_clear(&replace);\n }\n\n+ /* skip remote if no refspecs to fetch */\n+ if (child.args.nr <= nargs)\n+ return 0;\n+\n return !!run_command(&child);\n }\n\n"},{"id":"421013","messageId":"c06a198a-2043-27a2-cab3-3471190754cc@gmail.com","threadId":"55422","inReplyTo":"20210405205056.czzkjdxr3guca3mf@dhcp-10-154-153-195.vpn.oracle.com","subject":"Re: should git maintenance prefetch be taught to honor remote.fetch refspec?","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-05T20:54:44Z","receivedAt":"2021-04-05T20:54:54Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/5/2021 4:50 PM, Tom Saeger wrote:\n> On Mon, Apr 05, 2021 at 04:49:29PM -0400, Derrick Stolee wrote:\n>> On 4/5/2021 4:47 PM, Junio C Hamano wrote:\n>>> Tom Saeger writes:\n>>>\n>>>> $ git config --local --get-regexp \"pr-924\"\n>>>> remote.pr-924.url https://github.com/gitgitgadget/git\n>>>> remote.pr-924.fetch +refs/tags/pr-924/derrickstolee/maintenance/refspec-v1\n>>>>\n>>>> Seems legal, fetch even works\n>>>\n>>> Yes. For a ref that is one-shot use (like PR tags), this does not\n>>> make much sense, but\n>>>\n>>> [remote \"submaintainer1\"]\n>>> \turl = ... repository of submaintainer #1 ...\n>>> \tfetch = master\n>>> \ttagopts = --no-tags\n>>>\n>>> is a reasonable thing to have for those who regularly work with\n>>> submaintainer(s) of their project. They'd do\n>>>\n>>> \t$ git pull submaintainer1\n>>>\n>>> to accept the work their submaintainers have done.\n>>\n>> Thanks for the extra testing! I'll be sure to fix this bug in v2.\n>>\n>> -Stolee\n>> \n> \n> Hacked this:\n>\n> @@ -920,6 +927,10 @@ static int fetch_remote(struct remote *remote, void *cbdata)\n> refspec_item_clear(&replace);\n> }\n> \n> + /* skip remote if no refspecs to fetch */\n> + if (child.args.nr <= nargs)\n> + return 0;\n> +\n\nThis is a good approach, but we'll need to clean up the data in\n'child' since run_command() no longer does it for us.\n\n> return !!run_command(&child);\n\nThanks,\n-Stolee\n"}]}