{"thread":{"id":"29078","subject":"Evaluation of ref-api branch status","startedAt":"2011-12-03T23:52:14Z","lastAt":"2011-12-06T06:57:46Z","messageCount":4,"participants":["Michael Haggerty","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"180288","messageId":"4EDAB62E.5070204@alum.mit.edu","threadId":"29078","inReplyTo":null,"subject":"Evaluation of ref-api branch status","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-12-03T23:52:14Z","receivedAt":"2011-12-03T23:52:14Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Junio,\n\nNow that 1.7.8 is out, I wanted to figure out the status of the\nremaining ref-api changes that are in flight, including the differences\nbetween between my tree and yours.\n\nI understand that \"next\" will soon be re-rolled.  Will the re-roll be\nbased on the current \"pu\", or will you start from scratch?\n\n\nAFAIK there are three groups of ref-api patch series in flight, some of\nwhich have gone through multiple iterations:\n\nref-api-B == mh/ref-api-2\n    [PATCH v3 00/14] Tidying up references code\n\nref-api-C == mh/ref-api-3\n    [PATCH v2 00/12] Use refs API more consistently\n\nref-api-D == mh/ref-api-take-2\n    [PATCH 00/28] Store references hierarchically in cache\n\nOn 2011-10-28 I sent re-rolled versions of ref-api-B and ref-api-C to\nthe list followed by the first version of ref-api-D.  You committed\nthese three patch series to your repository as a single branch,\n\"mh/ref-api-take-2\".  The mh/ref-api-take-2 branch thus includes the\nlogical equivalent of all three of the patch series listed above.\n\nAdditionally, on 2011-11-15 I sent a fix to patch 26/28 of ref-api-D:\n\n    [PATCH] Fix \"is_refname_available(): query only possibly-conflicting\n references\"\n\nI don't see this patch in your tree at all.\n\n\nMeanwhile, you added the following fix on top of master, mh/ref-api-2,\nand ref-api-3:\n\n    refs: loosen over-strict \"format\" check\n\n[Aside: I don't understand why you created three independent commits\nrather than create an ur-commit on $(git merge-base master mh/ref-api-2\nmh/ref-api-3) then merging it to each of the branches.]\n\n\nBoth mh/ref-api-2 (including your fix) and mh/ref-api-3 (including your\nfix) have been merged into \"next\".  You have never merged\nmh/ref-api-take-2 anywhere.  On the mh/ref-api-take-2 branch:\n\n* ce766d41fa corresponds to mh/ref-api-2 (without your fix)\n* 633ebc45c0 corresponds to mh/ref-api-3 (without your fix)\n\nTherefore, your 633ebc45c0..mh/ref-api-take-2 corresponds to my newest\n\"ref-api-D\" patch series.  It does not include my fix from 2011-11-15.\nNor does it include any form of your fix \"refs: loosen over-strict\n\"format\" check\".\n\n\nWhat is the difference between the beginning of mh/ref-api-take-2 as\ncompared to mh/ref-api-2 + mh/ref-api-3?  Equivalently, what is the\ndifference between the last versions of ref-api-B and ref-api-C vs.\nearlier versions of those patch series?  The differences are relatively\nminor:\n\n* A whitespace problem in the earlier version of the series was fixed.\nThis whitespace problem was introduced in caa80697 and has gotten into\n\"pu\" (in refs.c starting at line 1093).\n\n* resolve_gitlink_packed_ref() was simplified a bit and got a docstring.\n\n* A function do_for_each_ref_in_arrays() was extracted.\n\n* The series were rebased.  The changes made to rebase them are roughly\nequivalent to the work that you did in merges bea03b2455 and 773b817986.\n (BTW, your merges look correct to me.)\n\n* It does *not* include your fix \"refs: loosen over-strict \"format\" check\".\n\nSUMMARY:\n\nThe patches from mh/ref-api-take-2 through 633ebc45c0 plus your fix are\nslightly preferable to the patches in mh/ref-api-2 and mh/ref-api-3, in\nthe ways listed in the bullet-points above.  If you are rewinding \"next\"\nanyway, I suggest that you take the former instead of the latter.  If\nyou prefer to stick with the latter, let me know and I will submit the\nremaining differences as patches to be applied on top of mh/ref-api-3.\n\n\nI hope that this summary is helpful to you.  (It certainly helped me\nfigure out where things stand.)\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"180308","messageId":"7vfwgynacr.fsf@alter.siamese.dyndns.org","threadId":"29078","inReplyTo":"4EDAB62E.5070204@alum.mit.edu","subject":"Re: Evaluation of ref-api branch status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-05T18:26:28Z","receivedAt":"2011-12-05T18:26:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Now that 1.7.8 is out, I wanted to figure out the status of the\n> remaining ref-api changes that are in flight, including the differences\n> between between my tree and yours.\n\nI understand that the ultimate and largest objective of these series is to\nmake sure that the performance does not suffer from very many negative\nlookups on the refs/replace hierarchy (which almost always is empty in a\nsane repository), and I do want to see that happen. I also think it is\ngood that the series tries to make sure that the various codepaths we\ncreate new refs do not let the user accidentally create refs with bad\nnames and/or contents.\n\nWhen we see a questionable ref that is _already_ in the respository,\nhowever, we may warn but it is wrong to declare the repository to be\nbroken and refuse to work. That would make it cumbersome or impossible to\neven _fix_ such breakage. Regressions of that kind wer in the earlier part\nof the series already in 1.7.8 and it was rather unpleasant having to\nhotfix them. As our test suite does not deliberately create these\nquestionable refs and/or try to use them (and I personally do not in my\nregular workflow either), I am afraid that it is rather hard to realize\nwhat kind of refnames are what we intended to forbid even from earlier\ndays but did not _bother_ to check and enforce the rule against so far,\nbut are forbidden by the updated code, until we unleash the new logic to\nthe end users with various third party tools that created them [*1*]. I\nwould want to see us get this part right and solid, and include it in\n1.7.9.\n\nIt would be nice if we can include also the bits to read hierarchies\nlazily, but as they come on top of your B & C, it may end up in 1.7.10 and\nI do not mind it at all. Reference resolution is one of the central things\nin the user experience, and we cannot afford to ship anything that is\nhalf-done-and-mostly-works over slow-but-correct.\n\nAlternatively you _could_ swap the order of your B & C and D and try to\nhave your D early while giving B & C more time to cook, but I suspect the\norder you chose would be better in the longer term.\n\n> I understand that \"next\" will soon be re-rolled.  Will the re-roll be\n> based on the current \"pu\", or will you start from scratch?\n\nAs to the two quickfix patches that are on two remaining topics from you\nin my tree, I did them merely as a short-term band-aid. I was expecting,\nafter 1.7.8 when we eject the topics out of 'next', that they will be\nrebased on top of 'master' that already contains a proper fix before these\ntopics started touching the same codepath.\n\nMy reading of your summary suggests that it would be easiest to drop the\nthree mh/ref-api* topics from my tree, especially the 'refs: loosen\nover-strict \"format\" check' band-aid patches, and re-queue a re-roll from\nyou.\n\nThanks.\n\n[Footnote]\n\n*1* Trying to be lenient when reading and being strict when writing as the\ngeneral principle would be the safe and sane way forward, and making sure\nthat we warn *loudly* when we think we are merely being lenient (i.e. we\nthink we found a bad ref with questionable name and/or contents) would be\na good way to catch our mistakes early.  E.g. \".git/config does not record\na correct object name\" glitch was caught only because you added the\nwarning so while it was painful to hotfix that late in the cycle, the\nwarning was worth it.\n"},{"id":"180341","messageId":"4EDDAC4A.4030805@alum.mit.edu","threadId":"29078","inReplyTo":"7vfwgynacr.fsf@alter.siamese.dyndns.org","subject":"Re: Evaluation of ref-api branch status","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-12-06T05:46:50Z","receivedAt":"2011-12-06T05:46:50Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/05/2011 07:26 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n>> Now that 1.7.8 is out, I wanted to figure out the status of the\n>> remaining ref-api changes that are in flight, including the differences\n>> between between my tree and yours.\n> \n> I understand that the ultimate and largest objective of these series is to\n> make sure that the performance does not suffer from very many negative\n> lookups on the refs/replace hierarchy (which almost always is empty in a\n> sane repository), and I do want to see that happen. I also think it is\n> good that the series tries to make sure that the various codepaths we\n> create new refs do not let the user accidentally create refs with bad\n> names and/or contents.\n> \n> When we see a questionable ref that is _already_ in the respository,\n> however, we may warn but it is wrong to declare the repository to be\n> broken and refuse to work. [...]  I\n> would want to see us get this part right and solid, and include it in\n> 1.7.9.\n> [...]\n> \n> Alternatively you _could_ swap the order of your B & C and D and try to\n> have your D early while giving B & C more time to cook, but I suspect the\n> order you chose would be better in the longer term.\n\nI will evaluate whether this is feasible.\n\n>> I understand that \"next\" will soon be re-rolled.  Will the re-roll be\n>> based on the current \"pu\", or will you start from scratch?\n> \n> As to the two quickfix patches that are on two remaining topics from you\n> in my tree, I did them merely as a short-term band-aid. I was expecting,\n> after 1.7.8 when we eject the topics out of 'next', that they will be\n> rebased on top of 'master' that already contains a proper fix before these\n> topics started touching the same codepath.\n> \n> My reading of your summary suggests that it would be easiest to drop the\n> three mh/ref-api* topics from my tree, especially the 'refs: loosen\n> over-strict \"format\" check' band-aid patches, and re-queue a re-roll from\n> you.\n\nOK, then, I will try to re-roll the series on top of master, and build\nthe equivalent of your quick-fix into the logical point in the series.\nI will also check whether it is feasible to push more of the refname\nsanity checks to later in the series and make them more informational\nwhere old data is concerned.  How much time to I have to work on this\nwhile still leaving enough time to comfortably integrate it into 1.7.9?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"180359","messageId":"7vzkf6gpat.fsf@alter.siamese.dyndns.org","threadId":"29078","inReplyTo":"4EDDAC4A.4030805@alum.mit.edu","subject":"Re: Evaluation of ref-api branch status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-06T06:57:46Z","receivedAt":"2011-12-06T06:57:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> On 12/05/2011 07:26 PM, Junio C Hamano wrote:\n> ...\n>> My reading of your summary suggests that it would be easiest to drop the\n>> three mh/ref-api* topics from my tree, especially the 'refs: loosen\n>> over-strict \"format\" check' band-aid patches, and re-queue a re-roll from\n>> you.\n>\n> OK, then, I will try to re-roll the series on top of master, and build\n> the equivalent of your quick-fix into the logical point in the series.\n\nThese quick-fixes were necessary _only_ because the queued topics predate\nthe real fix already in 1.7.8 (and I generally refuse to criss cross merge\nfrom master to topics), so I expect you won't need their equivalents if\nthe topic is rebased on top of 1.7.8\n\n> ... How much time to I have to work on this\n> while still leaving enough time to comfortably integrate it into 1.7.9?\n\nI am hoping we can have rc0 very early next year [*1*].\n\n\n[Reference]\n\n*1* https://www.google.com/calendar/embed?src=jfgbl2mrlipp4pb6ieih0qr3so%40group.calendar.google.com&ctz=America/Los_Angeles\n"}]}