{"thread":{"id":"30210","subject":"[PATCH] fetch: Only call a new ref a \"branch\" if it's under refs/heads/.","startedAt":"2012-04-11T14:29:29Z","lastAt":"2012-04-13T16:25:16Z","messageCount":8,"participants":["marcnarc@xiplink.com","Jeff King","Jens Lehmann","Marc Branchaud"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"188966","messageId":"1334154569-26124-1-git-send-email-marcnarc@xiplink.com","threadId":"30210","inReplyTo":null,"subject":"[PATCH] fetch: Only call a new ref a \"branch\" if it's under refs/heads/.","fromName":"","fromEmail":"marcnarc@xiplink.com","sentAt":"2012-04-11T14:29:29Z","receivedAt":"2012-04-11T14:29:29Z","isPatch":true,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"From: Marc Branchaud \n\nSigned-off-by: Marc Branchaud \n---\n\nThis has been a minor irritant in my life for a while now... :)\n\n builtin/fetch.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 65f5f9b..57be58a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -298,8 +298,13 @@ static int update_local_ref(struct ref *ref,\n \t\t\twhat = _(\"[new tag]\");\n \t\t}\n \t\telse {\n-\t\t\tmsg = \"storing head\";\n-\t\t\twhat = _(\"[new branch]\");\n+\t\t\tif (!prefixcmp(ref->name, \"refs/heads/\")) {\n+\t\t\t\tmsg = \"storing head\";\n+\t\t\t\twhat = _(\"[new branch]\");\n+\t\t\t} else {\n+\t\t\t\tmsg = \"storing ref\";\n+\t\t\t\twhat = _(\"[new ref]\");\n+\t\t\t}\n \t\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n \t\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n \t\t\t\tcheck_for_new_submodule_commits(ref->new_sha1);\n-- \n1.7.10.1.g2bbc8\n"},{"id":"189076","messageId":"20120412055216.GC27369@sigill.intra.peff.net","threadId":"30210","inReplyTo":"1334154569-26124-1-git-send-email-marcnarc@xiplink.com","subject":"Re: [PATCH] fetch: Only call a new ref a \"branch\" if it's under refs/heads/.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-12T05:52:17Z","receivedAt":"2012-04-12T05:52:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 11, 2012 at 10:29:29AM -0400, marcnarc@xiplink.com wrote:\n\n> builtin/fetch.c | 9 +++++++--\n> 1 file changed, 7 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 65f5f9b..57be58a 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -298,8 +298,13 @@ static int update_local_ref(struct ref *ref,\n> \t\t\twhat = _(\"[new tag]\");\n> \t\t}\n> \t\telse {\n> -\t\t\tmsg = \"storing head\";\n> -\t\t\twhat = _(\"[new branch]\");\n> +\t\t\tif (!prefixcmp(ref->name, \"refs/heads/\")) {\n> +\t\t\t\tmsg = \"storing head\";\n> +\t\t\t\twhat = _(\"[new branch]\");\n> +\t\t\t} else {\n> +\t\t\t\tmsg = \"storing ref\";\n> +\t\t\t\twhat = _(\"[new ref]\");\n> +\t\t\t}\n> \t\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n> \t\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n> \t\t\t\tcheck_for_new_submodule_commits(ref->new_sha1);\n\nIt looks like you kept the behavior the same with respect to\nrecurse_submodules, which will continue to run for everything except\ntags. Which is probably a good choice, since your patch only wanted to\ndeal with the status message, but I am left to wonder: if submodules\nwere intended to be recursed for branches but not tags, what should\nhappen for other types of refs? Was it intentional that they fell into\nthe \"branch\" category here, or were they following the same failure to\ndistinguish that the message-writing code had?\n\nThis code block handles only new refs. If you look at the code below,\nupdates of existing refs (forced or not) will happen for all refs,\nincluding tags.\n\nJens, can you double-check the intended logic?\n\n-Peff\n"},{"id":"189135","messageId":"4F8737C8.1020501@web.de","threadId":"30210","inReplyTo":"20120412055216.GC27369@sigill.intra.peff.net","subject":"Re: [PATCH] fetch: Only call a new ref a \"branch\" if it's under refs/heads/.","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-04-12T20:15:04Z","receivedAt":"2012-04-12T20:15:04Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 12.04.2012 07:52, schrieb Jeff King:\n> On Wed, Apr 11, 2012 at 10:29:29AM -0400, marcnarc@xiplink.com wrote:\n> \n>> builtin/fetch.c | 9 +++++++--\n>> 1 file changed, 7 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index 65f5f9b..57be58a 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -298,8 +298,13 @@ static int update_local_ref(struct ref *ref,\n>> \t\t\twhat = _(\"[new tag]\");\n>> \t\t}\n>> \t\telse {\n>> -\t\t\tmsg = \"storing head\";\n>> -\t\t\twhat = _(\"[new branch]\");\n>> +\t\t\tif (!prefixcmp(ref->name, \"refs/heads/\")) {\n>> +\t\t\t\tmsg = \"storing head\";\n>> +\t\t\t\twhat = _(\"[new branch]\");\n>> +\t\t\t} else {\n>> +\t\t\t\tmsg = \"storing ref\";\n>> +\t\t\t\twhat = _(\"[new ref]\");\n>> +\t\t\t}\n>> \t\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n>> \t\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n>> \t\t\t\tcheck_for_new_submodule_commits(ref->new_sha1);\n> \n> It looks like you kept the behavior the same with respect to\n> recurse_submodules, which will continue to run for everything except\n> tags. Which is probably a good choice, since your patch only wanted to\n> deal with the status message, but I am left to wonder: if submodules\n> were intended to be recursed for branches but not tags, what should\n> happen for other types of refs? Was it intentional that they fell into\n> the \"branch\" category here, or were they following the same failure to\n> distinguish that the message-writing code had?\n> \n> This code block handles only new refs. If you look at the code below,\n> updates of existing refs (forced or not) will happen for all refs,\n> including tags.\n> \n> Jens, can you double-check the intended logic?\n\nThanks for spotting this inconsistency. I think it makes sense to\ncheck for new submodule commits no matter if we fetched a new tag,\nbranch or other ref. I can't remember a reason why I put that code\ninto the refs & branches part instead of doing that for every new\nref. Patch following ...\n"},{"id":"189138","messageId":"4F873CD0.4050204@xiplink.com","threadId":"30210","inReplyTo":"4F8737C8.1020501@web.de","subject":"Re: [PATCH] fetch: Only call a new ref a \"branch\" if it's under refs/heads/.","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2012-04-12T20:36:32Z","receivedAt":"2012-04-12T20:36:32Z","isPatch":true,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 12-04-12 04:15 PM, Jens Lehmann wrote:\n> Am 12.04.2012 07:52, schrieb Jeff King:\n>> On Wed, Apr 11, 2012 at 10:29:29AM -0400, marcnarc@xiplink.com wrote:\n>>\n>>> builtin/fetch.c | 9 +++++++--\n>>> 1 file changed, 7 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>>> index 65f5f9b..57be58a 100644\n>>> --- a/builtin/fetch.c\n>>> +++ b/builtin/fetch.c\n>>> @@ -298,8 +298,13 @@ static int update_local_ref(struct ref *ref,\n>>> \t\t\twhat = _(\"[new tag]\");\n>>> \t\t}\n>>> \t\telse {\n>>> -\t\t\tmsg = \"storing head\";\n>>> -\t\t\twhat = _(\"[new branch]\");\n>>> +\t\t\tif (!prefixcmp(ref->name, \"refs/heads/\")) {\n>>> +\t\t\t\tmsg = \"storing head\";\n>>> +\t\t\t\twhat = _(\"[new branch]\");\n>>> +\t\t\t} else {\n>>> +\t\t\t\tmsg = \"storing ref\";\n>>> +\t\t\t\twhat = _(\"[new ref]\");\n>>> +\t\t\t}\n>>> \t\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n>>> \t\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n>>> \t\t\t\tcheck_for_new_submodule_commits(ref->new_sha1);\n>>\n>> It looks like you kept the behavior the same with respect to\n>> recurse_submodules, which will continue to run for everything except\n>> tags. Which is probably a good choice, since your patch only wanted to\n>> deal with the status message, but I am left to wonder: if submodules\n>> were intended to be recursed for branches but not tags, what should\n>> happen for other types of refs? Was it intentional that they fell into\n>> the \"branch\" category here, or were they following the same failure to\n>> distinguish that the message-writing code had?\n>>\n>> This code block handles only new refs. If you look at the code below,\n>> updates of existing refs (forced or not) will happen for all refs,\n>> including tags.\n>>\n>> Jens, can you double-check the intended logic?\n> \n> Thanks for spotting this inconsistency. I think it makes sense to\n> check for new submodule commits no matter if we fetched a new tag,\n> branch or other ref. I can't remember a reason why I put that code\n> into the refs & branches part instead of doing that for every new\n> ref. Patch following ...\n\nI assumed it was an optimization of some sort -- that since tags are normally\nonly fetched when they're part of a requested branch's history (right?),\nthere was no point in doing submodule recursion on the fetched tags since\nthose tagged tree-ishes had already been submodule-recursed.\n\nOr something like that. :)\n\n\t\tM.\n"},{"id":"189140","messageId":"4F873E2C.6080808@web.de","threadId":"30210","inReplyTo":"4F873CD0.4050204@xiplink.com","subject":"Re: [PATCH] fetch: Only call a new ref a \"branch\" if it's under refs/heads/.","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-04-12T20:42:20Z","receivedAt":"2012-04-12T20:42:20Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 12.04.2012 22:36, schrieb Marc Branchaud:\n> I assumed it was an optimization of some sort -- that since tags are normally\n> only fetched when they're part of a requested branch's history (right?),\n> there was no point in doing submodule recursion on the fetched tags since\n> those tagged tree-ishes had already been submodule-recursed.\n\nIf that is the case the patch I just sent is pointless, but adding a comment\nthere explaining that would be a good thing ;-)\n"},{"id":"189144","messageId":"20120412210542.GD21018@sigill.intra.peff.net","threadId":"30210","inReplyTo":"4F873E2C.6080808@web.de","subject":"Re: [PATCH] fetch: Only call a new ref a \"branch\" if it's under refs/heads/.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-12T21:05:42Z","receivedAt":"2012-04-12T21:05:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 12, 2012 at 10:42:20PM +0200, Jens Lehmann wrote:\n\n> Am 12.04.2012 22:36, schrieb Marc Branchaud:\n> > I assumed it was an optimization of some sort -- that since tags are normally\n> > only fetched when they're part of a requested branch's history (right?),\n> > there was no point in doing submodule recursion on the fetched tags since\n> > those tagged tree-ishes had already been submodule-recursed.\n> \n> If that is the case the patch I just sent is pointless, but adding a comment\n> there explaining that would be a good thing ;-)\n\nThat is often the case, but not always (I might explicitly fetch a tag,\nor have refs/tags/ in my refspec). So I think you want to handle both to\ncover all cases. I haven't looked at the submodule recursion code, but I\nwould hope that it would eliminate duplicates (so if I get a branch and\na tag that is on the branch, we would look at the commit in question\nonly once).\n\n-Peff\n"},{"id":"189177","messageId":"20120413070423.GA16292@sigill.intra.peff.net","threadId":"30210","inReplyTo":"20120412210542.GD21018@sigill.intra.peff.net","subject":"Re: [PATCH] fetch: Only call a new ref a \"branch\" if it's under refs/heads/.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-13T07:04:23Z","receivedAt":"2012-04-13T07:04:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 12, 2012 at 05:05:42PM -0400, Jeff King wrote:\n\n> > Am 12.04.2012 22:36, schrieb Marc Branchaud:\n> > > I assumed it was an optimization of some sort -- that since tags are normally\n> > > only fetched when they're part of a requested branch's history (right?),\n> > > there was no point in doing submodule recursion on the fetched tags since\n> > > those tagged tree-ishes had already been submodule-recursed.\n> > \n> > If that is the case the patch I just sent is pointless, but adding a comment\n> > there explaining that would be a good thing ;-)\n> \n> That is often the case, but not always (I might explicitly fetch a tag,\n> or have refs/tags/ in my refspec). So I think you want to handle both to\n> cover all cases. I haven't looked at the submodule recursion code, but I\n> would hope that it would eliminate duplicates (so if I get a branch and\n> a tag that is on the branch, we would look at the commit in question\n> only once).\n\nOK, I checked. Yes, we handle this just fine. After my 6859de4, we\ncollect the ref tips before and after the fetch and run only a single\nrev-list. So processing the tags will result in just an extra\ninteresting commit, which was either:\n\n 1. accessible by another fetched branch, in which case it was going to\n be processed as interesting anyway\n\n 2. not accessible, in which case we have fixed a bug. :)\n\nSo I think we should check all incoming refs, including tags.\n\n-Peff\n"},{"id":"189196","messageId":"4F88536C.2090003@web.de","threadId":"30210","inReplyTo":"20120413070423.GA16292@sigill.intra.peff.net","subject":"[PATCH] submodules: recursive fetch also checks new tags for submodule commits","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-04-13T16:25:16Z","receivedAt":"2012-04-13T16:25:16Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Since 88a21979c (fetch/pull: recurse into submodules when necessary) all\nfetched commits are examined if they contain submodule changes (unless\nconfiguration or command line options inhibit that). If a newly recorded\nsubmodule commit is not present in the submodule, a fetch is run inside\nit to download that commit.\n\nChecking new refs was done in an else branch where it wasn't executed for\ntags. This normally isn't a problem because tags are only fetched with\nthe branches they live on, then checking the new commits in the fetched\nbranches for submodule commits will also process all tags. But when a\nspecific tag is fetched (or the refspec contains refs/tags/) commits only\nreachable by tags won't be searched for submodule commits, which is a bug.\n\nFix that by moving the code outside the if/else construct to handle new\ntags just like any other ref. The performance impact of adding tags that\nmost of the time lie on a branch which is checked anyway for new submodule\ncommit should be minimal, as since 6859de4 (fetch: avoid quadratic loop\nchecking for updated submodules) all ref-tips are collected first and then\nfed to a single rev-list.\n\nSpotted-by: Jeff King \nSigned-off-by: Jens Lehmann \n---\n\nAm 13.04.2012 09:04, schrieb Jeff King:\n> OK, I checked. Yes, we handle this just fine. After my 6859de4, we\n> collect the ref tips before and after the fetch and run only a single\n> rev-list. So processing the tags will result in just an extra\n> interesting commit, which was either:\n> \n> 1. accessible by another fetched branch, in which case it was going to\n> be processed as interesting anyway\n> \n> 2. not accessible, in which case we have fixed a bug. :)\n> \n> So I think we should check all incoming refs, including tags.\n\nThanks for you analysis!\n\n\n builtin/fetch.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 65f5f9b..cfb43df 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -300,11 +300,11 @@ static int update_local_ref(struct ref *ref,\n \t\telse {\n \t\t\tmsg = \"storing head\";\n \t\t\twhat = _(\"[new branch]\");\n-\t\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n-\t\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n-\t\t\t\tcheck_for_new_submodule_commits(ref->new_sha1);\n \t\t}\n\n+\t\tif ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&\n+\t\t (recurse_submodules != RECURSE_SUBMODULES_ON))\n+\t\t\tcheck_for_new_submodule_commits(ref->new_sha1);\n \t\tr = s_update_ref(msg, ref, 0);\n \t\tstrbuf_addf(display, \"%c %-*s %-*s -> %s%s\",\n \t\t\t r ? '!' : '*',\n-- \n1.7.10.131.gd0e498\n"}]}