{"thread":{"id":"44621","subject":"Should reset_revision_walk clear more flags?","startedAt":"2016-12-04T23:10:09Z","lastAt":"2016-12-05T07:37:22Z","messageCount":5,"participants":["Mike Hommey","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"306905","messageId":"20161204230958.h3ilhueqqptv253u@glandium.org","threadId":"44621","inReplyTo":null,"subject":"Should reset_revision_walk clear more flags?","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-12-04T23:09:58Z","receivedAt":"2016-12-04T23:10:09Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Hi,\n\nWhile trying to use the revision walking API twice in a row, I noticed\nthat the second time for the same setup would not yield the same result.\nIn my case, it turns out I was requesting boundaries, and\nreset_revision_walk() is not resetting CHILD_SHOWN and BOUNDARY, both\nrequired to be reset for the second revision walk to work.\n\nSo the question is, are consumers supposed to reset those flags on their\nown, or should reset_revision_walk clear them?\n\nMike\n"},{"id":"306906","messageId":"20161204232446.tbu6dhwfrw3n5ueo@glandium.org","threadId":"44621","inReplyTo":"20161204230958.h3ilhueqqptv253u@glandium.org","subject":"Re: Should reset_revision_walk clear more flags?","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-12-04T23:24:46Z","receivedAt":"2016-12-04T23:24:56Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Mon, Dec 05, 2016 at 08:09:58AM +0900, Mike Hommey wrote:\n> Hi,\n> \n> While trying to use the revision walking API twice in a row, I noticed\n> that the second time for the same setup would not yield the same result.\n> In my case, it turns out I was requesting boundaries, and\n> reset_revision_walk() is not resetting CHILD_SHOWN and BOUNDARY, both\n> required to be reset for the second revision walk to work.\n\nand TREESAME when using different pathspecs.\n\nMike\n"},{"id":"306910","messageId":"20161205054013.taosbwjamxiwzocn@sigill.intra.peff.net","threadId":"44621","inReplyTo":"20161204230958.h3ilhueqqptv253u@glandium.org","subject":"Re: Should reset_revision_walk clear more flags?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-05T05:40:13Z","receivedAt":"2016-12-05T05:40:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 05, 2016 at 08:09:58AM +0900, Mike Hommey wrote:\n\n> While trying to use the revision walking API twice in a row, I noticed\n> that the second time for the same setup would not yield the same result.\n> In my case, it turns out I was requesting boundaries, and\n> reset_revision_walk() is not resetting CHILD_SHOWN and BOUNDARY, both\n> required to be reset for the second revision walk to work.\n> \n> So the question is, are consumers supposed to reset those flags on their\n> own, or should reset_revision_walk clear them?\n\nI would think that reset_revision_walk() should reset any flags set by\nthe revision-walking code (so anything set by calling init_revisions(),\nand then either a call into list_objects() or repeated calls of\nget_revision()).\n\nWhich I think would include both of the flags you mentioned, along with\nothers like SYMMETRIC_LEFT, PATCHSAME, etc. Probably really everything\nmentioned in revision.h, which should be summed up as ALL_REV_FLAGS.\nSome callsites already seem to feed that to clear_commit_marks().\n\nI doubt you can go too wrong by clearing more flags. It's possible that\nsome caller is relying on a flag _not_ being cleared between two\ntraversals, but in that case it should probably be using\nclear_commit_marks() or clear_object_flags() explicitly to make it clear\nwhat it expects to be saved.\n\n-Peff\n"},{"id":"306918","messageId":"xmqqmvga6dmb.fsf@gitster.mtv.corp.google.com","threadId":"44621","inReplyTo":"20161205054013.taosbwjamxiwzocn@sigill.intra.peff.net","subject":"Re: Should reset_revision_walk clear more flags?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-05T07:26:04Z","receivedAt":"2016-12-05T07:26:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Which I think would include both of the flags you mentioned, along with\n> others like SYMMETRIC_LEFT, PATCHSAME, etc. Probably really everything\n> mentioned in revision.h, which should be summed up as ALL_REV_FLAGS.\n> Some callsites already seem to feed that to clear_commit_marks().\n>\n> I doubt you can go too wrong by clearing more flags. \n\nThis and ...\n\n> It's possible that\n> some caller is relying on a flag _not_ being cleared between two\n> traversals, but in that case it should probably be using\n> clear_commit_marks() or clear_object_flags() explicitly to make it clear\n> what it expects to be saved.\n\n... this are contradictory, no?  \n\nA caller may use two sets of its own flags in addition to letting\nthe traversal machinery use the basic ones, and it may want to keep\none of its own two sets while clearing the other set.  It would\nclear_commit_marks() to clear the latter.  And then it would let\nthat the next traversal to reset_revision_walk() clear the basic\nones.\n\nSo if you make reset_revision_walk() clear \"more flags\", that would\nbreak such a caller that is using clear_commit_marks() to make it\nclear what its expectations are, no?\n\nI dunno.\n\n"},{"id":"306920","messageId":"20161205073619.6h6rm2z4av3mie2y@sigill.intra.peff.net","threadId":"44621","inReplyTo":"xmqqmvga6dmb.fsf@gitster.mtv.corp.google.com","subject":"Re: Should reset_revision_walk clear more flags?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-05T07:36:19Z","receivedAt":"2016-12-05T07:37:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 04, 2016 at 11:26:04PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Which I think would include both of the flags you mentioned, along with\n> > others like SYMMETRIC_LEFT, PATCHSAME, etc. Probably really everything\n> > mentioned in revision.h, which should be summed up as ALL_REV_FLAGS.\n> > Some callsites already seem to feed that to clear_commit_marks().\n> >\n> > I doubt you can go too wrong by clearing more flags. \n> \n> This and ...\n> \n> > It's possible that\n> > some caller is relying on a flag _not_ being cleared between two\n> > traversals, but in that case it should probably be using\n> > clear_commit_marks() or clear_object_flags() explicitly to make it clear\n> > what it expects to be saved.\n> \n> ... this are contradictory, no?\n\nI don't think so. If you are calling reset_revision_walk(), then I'm not\nsure you have any right to expect that particular flags are left intact.\nYou _should_ be using an option that lets you specify the full set of\nflags, rather than making an implicit assumption about which flags are\nleft.\n\nIn other words, I'd much rather see:\n\n  clear_object_flags(ALL_REV_FLAGS & (~TMP_MARK));\n\nthan:\n\n  /* This leaves TMP_MARK intact! */\n  reset_revision_walk();\n\nif you need to leave TMP_MARK intact.\n\nWhich isn't to say every caller does it correctly already. My claim\nabove is only \"should\", not \"does\". :)\n\nBut given that many of them _do_ use the more specific flag-clearing\nfunctions, I think most of the calls to reset_revision_walk() are\nblanket \"I'm done now, clean up after me\" calls.\n\n> A caller may use two sets of its own flags in addition to letting\n> the traversal machinery use the basic ones, and it may want to keep\n> one of its own two sets while clearing the other set.  It would\n> clear_commit_marks() to clear the latter.  And then it would let\n> that the next traversal to reset_revision_walk() clear the basic\n> ones.\n> \n> So if you make reset_revision_walk() clear \"more flags\", that would\n> break such a caller that is using clear_commit_marks() to make it\n> clear what its expectations are, no?\n\nI'd argue that it should not be calling reset_revision_walk() at all if\nit wants anything saved. It should mask out the flags it wants, as\nabove. But I do realize that reasoning is circular: it's OK for\nreset_revision_walk() to reset everything because that's what it does,\nor at least should do from its name. :)\n\n-Peff\n"}]}