{"thread":{"id":"55535","subject":"reflog existence & reftable","startedAt":"2021-04-21T10:02:50Z","lastAt":"2021-04-27T06:52:11Z","messageCount":7,"participants":["Han-Wen Nienhuys","Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"422555","messageId":"CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com","threadId":"55535","inReplyTo":null,"subject":"reflog existence & reftable","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-04-21T10:02:32Z","receivedAt":"2021-04-21T10:02:50Z","isPatch":false,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"Hi there,\n\n(splitting off from a code review of my test cleanups.)\n\nCurrently, reflogs are stored in .git/log/*. Git adds entries to the\nreflog only if the reflog already exists (See the log_ref_setup()\nfunction).\n\nThe current iteration of the reftable design has a unified key space\nof {refname,index-number} for reflog entries. This causes there to be\nno distinction between\n\n 1) reflog is empty (.git/logs/blah is a 0-byte file)\n 2) reflog does not exist (.git/logs/blah does not exist)\n\nThis trips up some current tests that make assumptions on reflog existence.\n\nI don't know why one can tweak reflog to be written or not, but the\ncurrent functionality will cause a change in operation with reftable.\nI see two ways forward:\n\n1) Have different functionality in case of reftable: you cannot query\nfor the existence of reflogs, and writing reflogs doesn't depend on\nthe existence of a reflog.\n\n2) Add a reflog existence feature to reftable. We could introduce a\nmagical reflog entry, which indicates that the reflog exists (but\nmight be empty). This adds some complexity to the C code, but lets us\nmaintain backward compatibility.\n\nWhat do you think?\n\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"422575","messageId":"87im4f3l62.fsf@evledraar.gmail.com","threadId":"55535","inReplyTo":"CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com","subject":"Re: reflog existence & reftable","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-21T11:57:09Z","receivedAt":"2021-04-21T11:57:15Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 21 2021, Han-Wen Nienhuys wrote:\n\n> Hi there,\n>\n> (splitting off from a code review of my test cleanups.)\n\nJust changing the \"Subject\" won't break In-Reply-To and prune CC, FWIW\nthe missing In-Reply-To is (probably):\nhttps://lore.kernel.org/git/87pmyo3zvw.fsf@evledraar.gmail.com/\n\n> Currently, reflogs are stored in .git/log/*. Git adds entries to the\n> reflog only if the reflog already exists (See the log_ref_setup()\n> function).\n>\n> The current iteration of the reftable design has a unified key space\n> of {refname,index-number} for reflog entries. This causes there to be\n> no distinction between\n>\n> 1) reflog is empty (.git/logs/blah is a 0-byte file)\n> 2) reflog does not exist (.git/logs/blah does not exist)\n>\n> This trips up some current tests that make assumptions on reflog existence.\n>\n> I don't know why one can tweak reflog to be written or not, but the\n> current functionality will cause a change in operation with reftable.\n> I see two ways forward:\n>\n> 1) Have different functionality in case of reftable: you cannot query\n> for the existence of reflogs, and writing reflogs doesn't depend on\n> the existence of a reflog.\n>\n> 2) Add a reflog existence feature to reftable. We could introduce a\n> magical reflog entry, which indicates that the reflog exists (but\n> might be empty). This adds some complexity to the C code, but lets us\n> maintain backward compatibility.\n>\n> What do you think?\n\nI think we should fix the tests first, per [1] :)\n\nBecause there's a third case revealed by the test case, which would be\nteased out by not entirely skipping the test with REFFILES, but\nincrementally splitting it up:\n\nWhich is that the current reftable implementation is failing a test\n(well, probably a lot more, but the one under discussion) that:\n\n A. Sets core.logAllRefUpdates=false\n B. Checks out an orphan branch\n C. Checks that it has no existing reflog\n D. Makes a commit there\n E. Checks that it has no reflog\n\nOnly the \"E\" case is covered by your summary above.\n\nBut no matter how reftable's behavior is under\ncore.logAllRefUpdates=false it should surely not be returning true in\nthe \"C\" case, because there's no log entry to serve up, and indeed the\nbranch being asked for has no commits.\n\nSo for that case somewhere in the guts of the reftable integration we're\nlosing the distinction between asking for a log that can't exist\nv.s. one that's empty, maybe the reftable code is returning \"yes I have\nlogging on\" or \"yes I have some entries somewhere\" in that case?\n\nAnd in \"E\", related to \"C\" isn't in unambiguous to not write it if\nthere's no existing entry for the branch in question and\ncore.logAllRefUpdates=false is in effect?\n\nFor the rest of this is the behavior under reftable indistinguishable\nfrom having core.logAllRefUpdates=always set?\n\nIn any case, I don't think the emergent behavior of the files backend is\nworth emulating, but maybe if some feel that way it might be better to\ntransition the setting in general to core.logAllRefUpdates being a\nglobal on/off boolean, and having a branch..logRefUpdates, but I\nsuspect that there's not going to be any/many users of this selective\nlogging feature.\n\n1. https://lore.kernel.org/git/87lf9b3mth.fsf@evledraar.gmail.com/\n"},{"id":"422603","messageId":"xmqqim4fzier.fsf@gitster.g","threadId":"55535","inReplyTo":"87im4f3l62.fsf@evledraar.gmail.com","subject":"Re: reflog existence & reftable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-21T16:55:40Z","receivedAt":"2021-04-21T16:55:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> So for that case somewhere in the guts of the reftable integration we're\n> losing the distinction between asking for a log that can't exist\n> v.s. one that's empty, maybe the reftable code is returning \"yes I have\n> logging on\" or \"yes I have some entries somewhere\" in that case?\n>\n> And in \"E\", related to \"C\" isn't in unambiguous to not write it if\n> there's no existing entry for the branch in question and\n> core.logAllRefUpdates=false is in effect?\n\nSo, in short, we can rely on the fact that if reflog exists it will\nhave at least one element? \n\nBut the original \"turn logallrefupdates to false and touch the\nreflog files for refs you care about\" (which I recall I did for my\nown use case) allowed an empty reflog in preparation to have new\nentries to be appended, so in its initial state an existing reflog\ncan have zero element.\n\nIs there a documented way to just \"enable\" a single reflog via any\nGit command? That \"if a file exists, append\" code dates back for\nmore than 15 years and I do not remember if in the target use case\nI was happy enough to tell people to just \"touch\" the reflog file\nof interest, or if I bothered to add a command support (e.g. \"git\nreflog create 'refs/heads/next'\").\n\nIf there isn't, then we could do either one of these two things.\n\n (1) we could add \"git reflog create \" and the reftable can\n record the fact that \"reflog exists for the ref, but no ref\n movement recorded yet\". Then the condition C can be checked.\n\n (2) we could declare that there is no way to create an empty reflog\n supported across ref backends, and make the tests that rely on\n the \"feature\" conditional on REF_FILES prerequisite.\n\nI have no strong preference. In the early days I found the ability\nto limit which branches get logged convenient, so if reftable\nbackend can learn the similar trick, we would want to go route (1)\n(the convenience largely came from the fact that there was no need\nto add one configuration item per branch, so I do not think we would\nwant to bother with branch..reflog=bool configuration---that\nwon't be an easy-to-use substitute). On the other hand, logs are\nuseful, and dormant logs are not costing anything (other than holding\nonto stale objects we may no longer want), so it could be that it\nmay not be as convenient as it used to be to be able to turn logs on\nonly on selected refs, in which case approach (2) is fine.\n\n\n\n\n"},{"id":"422737","messageId":"CAFQ2z_OGv3qNp9jaeuMij5gqv1MoOeb73zH9mOvikLs8dWvmmg@mail.gmail.com","threadId":"55535","inReplyTo":"xmqqim4fzier.fsf@gitster.g","subject":"Re: reflog existence & reftable","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-04-23T09:20:52Z","receivedAt":"2021-04-23T09:21:10Z","isPatch":false,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Apr 21, 2021 at 6:55 PM Junio C Hamano wrote:\n> If there isn't, then we could do either one of these two things.\n>\n> (1) we could add \"git reflog create \" and the reftable can\n> record the fact that \"reflog exists for the ref, but no ref\n> movement recorded yet\". Then the condition C can be checked.\n>\n> (2) we could declare that there is no way to create an empty reflog\n> supported across ref backends, and make the tests that rely on\n> the \"feature\" conditional on REF_FILES prerequisite.\n>\n> I have no strong preference. In the early days I found the ability\n> to limit which branches get logged convenient, so if reftable\n> backend can learn the similar trick, we would want to go route (1)\n> (the convenience largely came from the fact that there was no need\n> to add one configuration item per branch, so I do not think we would\n> want to bother with branch..reflog=bool configuration---that\n> won't be an easy-to-use substitute). On the other hand, logs are\n> useful, and dormant logs are not costing anything (other than holding\n> onto stale objects we may no longer want), so it could be that it\n> may not be as convenient as it used to be to be able to turn logs on\n> only on selected refs, in which case approach (2) is fine.\n\nExactly, these are the two options I outlined in my original message.\nBoth can be made to work. I slightly prefer 2 (empty reflogs don't\nexist, and make logging a global switch), because it is simpler to\nunderstand and document. The divergence with the files backend itself\nis extra complexity, though. Maybe we could deprecate the behavior and\nalways write reflogs in the files backend too.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"422753","messageId":"YILUs3T4oM78ZcW2@coredump.intra.peff.net","threadId":"55535","inReplyTo":"CAFQ2z_OGv3qNp9jaeuMij5gqv1MoOeb73zH9mOvikLs8dWvmmg@mail.gmail.com","subject":"Re: reflog existence & reftable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-23T14:07:47Z","receivedAt":"2021-04-23T14:07:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 23, 2021 at 11:20:52AM +0200, Han-Wen Nienhuys wrote:\n\n> On Wed, Apr 21, 2021 at 6:55 PM Junio C Hamano wrote:\n> > If there isn't, then we could do either one of these two things.\n> >\n> > (1) we could add \"git reflog create \" and the reftable can\n> > record the fact that \"reflog exists for the ref, but no ref\n> > movement recorded yet\". Then the condition C can be checked.\n> >\n> > (2) we could declare that there is no way to create an empty reflog\n> > supported across ref backends, and make the tests that rely on\n> > the \"feature\" conditional on REF_FILES prerequisite.\n> >\n> > I have no strong preference. In the early days I found the ability\n> > to limit which branches get logged convenient, so if reftable\n> > backend can learn the similar trick, we would want to go route (1)\n> > (the convenience largely came from the fact that there was no need\n> > to add one configuration item per branch, so I do not think we would\n> > want to bother with branch..reflog=bool configuration---that\n> > won't be an easy-to-use substitute). On the other hand, logs are\n> > useful, and dormant logs are not costing anything (other than holding\n> > onto stale objects we may no longer want), so it could be that it\n> > may not be as convenient as it used to be to be able to turn logs on\n> > only on selected refs, in which case approach (2) is fine.\n> \n> Exactly, these are the two options I outlined in my original message.\n> Both can be made to work. I slightly prefer 2 (empty reflogs don't\n> exist, and make logging a global switch), because it is simpler to\n> understand and document. The divergence with the files backend itself\n> is extra complexity, though. Maybe we could deprecate the behavior and\n> always write reflogs in the files backend too.\n\nYeah, I like (2) as well. This \"write a reflog if it always exists\"\nbehavior has always seemed hacky, and like a leftover from early days\nwhen we didn't just turn reflogs on by default. Given that it was\ndocumented as \"touch the file\", I don't see any need to pretend that it\nmakes any sense at all in a reftables world.\n\nI'd also be perfectly happy with removing the feature on the files\nbackend (and perhaps replacing it with a simple globbing config value,\nin case anybody really wants to log only some refs). I find it hard to\nimagine that anybody would really care, but it _is_ a\nbackwards-incompatible change. So possibly we should do the usual\ndeprecation thing, or wait for a major version bump. I dunno.\n\n-Peff\n"},{"id":"422988","messageId":"CAFQ2z_OEB_jnL-G=GfypMiLLjE3gwp07ZMo3uF5aGkp3o6Yb9A@mail.gmail.com","threadId":"55535","inReplyTo":"YILUs3T4oM78ZcW2@coredump.intra.peff.net","subject":"Re: reflog existence & reftable","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-04-26T17:33:14Z","receivedAt":"2021-04-26T17:33:35Z","isPatch":false,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Fri, Apr 23, 2021 at 4:07 PM Jeff King wrote:\n>\n> On Fri, Apr 23, 2021 at 11:20:52AM +0200, Han-Wen Nienhuys wrote:\n>\n> > On Wed, Apr 21, 2021 at 6:55 PM Junio C Hamano wrote:\n\n> > > (2) we could declare that there is no way to create an empty reflog\n> > > supported across ref backends, and make the tests that rely on\n> > > the \"feature\" conditional on REF_FILES prerequisite.\n> > >\n> > > I have no strong preference. In the early days I found the ability\n> > > to limit which branches get logged convenient, so if reftable\n> > > backend can learn the similar trick, we would want to go route (1)\n> > > (the convenience largely came from the fact that there was no need\n> > > to add one configuration item per branch, so I do not think we would\n> > > want to bother with branch..reflog=bool configuration---that\n> > > won't be an easy-to-use substitute). On the other hand, logs are\n> > > useful, and dormant logs are not costing anything (other than holding\n> > > onto stale objects we may no longer want), so it could be that it\n> > > may not be as convenient as it used to be to be able to turn logs on\n> > > only on selected refs, in which case approach (2) is fine.\n> >\n> > Exactly, these are the two options I outlined in my original message.\n> > Both can be made to work. I slightly prefer 2 (empty reflogs don't\n> > exist, and make logging a global switch), because it is simpler to\n> > understand and document. The divergence with the files backend itself\n> > is extra complexity, though. Maybe we could deprecate the behavior and\n> > always write reflogs in the files backend too.\n>\n> Yeah, I like (2) as well. This \"write a reflog if it always exists\"\n> behavior has always seemed hacky, and like a leftover from early days\n> when we didn't just turn reflogs on by default. Given that it was\n> documented as \"touch the file\", I don't see any need to pretend that it\n> makes any sense at all in a reftables world.\n\nThanks. Does that count as consensus? Junio?\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"423059","messageId":"xmqqy2d4dxt6.fsf@gitster.g","threadId":"55535","inReplyTo":"CAFQ2z_OEB_jnL-G=GfypMiLLjE3gwp07ZMo3uF5aGkp3o6Yb9A@mail.gmail.com","subject":"Re: reflog existence & reftable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-27T06:52:05Z","receivedAt":"2021-04-27T06:52:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys writes:\n\n> Thanks. Does that count as consensus? Junio?\n\nSounds like it, even though I have been absent for most of the\nperiod ;-)\n"}]}