{"thread":{"id":"41393","subject":"Gated Merge?","startedAt":"2016-02-11T22:06:55Z","lastAt":"2016-02-12T19:26:13Z","messageCount":7,"participants":["Junio C Hamano","Andrew Ardill","Stefan Beller","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"277967","messageId":"xmqqvb5uvr40.fsf@gitster.mtv.corp.google.com","threadId":"41393","inReplyTo":null,"subject":"Gated Merge?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-11T22:06:55Z","receivedAt":"2016-02-11T22:06:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I wish there were a way to let me annotate a commit with a statement\nlike this:\n\n * When merging a branch that does not have me in it into a trunk\n that already has me, do this extra check on the result of the\n merge before allowing it to be recorded.\n\nFor example, suppose there is a topic that changes all instances of\nthis\n\n\t__attribute__((format(printf, X, Y)))\n\ninto a new macro FORMAT_PRINTF(X, Y). With such a facility, I can\nannotate the commit at the tip of that series to define an extra\ncheck:\n\n ! git diff HEAD | grep '^+.*__attribute__((format(printf'\n\nwhich would prevent me from merging a topic that introduces a new\ninstance of the printf-format attribute that is spelled in an\nold-fashioned way.\n\nIn this particular example, it might be even better if the facility\nallowed me to declare something like this instead:\n\n * When merging a branch that does not have me in it into a trunk\n that already has me, do this extra processing on the result of\n the merge before recording it.\n\nThe difference between them is the \"extra processing\" part. For\nthis example, it would be something that is built around:\n\n perl -p -e '\n s{__attribute__\\(\\(format\\(\\s*printf,\\s*(\\d+),\\s*(\\d+)\\)\\)\\)}\n {FORMAT_PRINTF($1, $2)}gx\n '\n\nto update the old-fashioned one to use the new macro, and the result\nwould become an evil merge.\n\nTo realize this, there are low-hanging-fruit building blocks that\nare trivial:\n\n - Such an annotation can be made as a note attached to the commit\n in question;\n\n - Such a check can be done in pre-commit hook;\n\n - Such a pre-commit hook can iterate over this set of commits\n\n DIFF=$(git rev-list --left-right HEAD...MERGE_HEAD)\n \n collect these Merge Gate annotations from the commit appears on\n the left hand side (e.g. only exists on the trunk that a side\n branch is about to be merged in), and run them.\n\nBut the last one feels very costly, and I suspect that there must be\na better way to do this. I do not think we want the \"what to do\"\npart (i.e. checking new lines for __attribute__ in the first\nexample, or rewriting new lines that has __attribute_ in the second\nexample) to be in git-core; that clearly is outside the scope of the\nplumbing. But the part that finds these \"annotations\", especially\nif we can come up with a fast implementation that may be hard to\nscript, may be a good addition to the plumbing command set.\n\nThoughts?\n"},{"id":"277976","messageId":"CAH5451=rVmdTra0rVNEkfdfOeSyeETSMo7YGWdYozZr_=YyAtw@mail.gmail.com","threadId":"41393","inReplyTo":"xmqqvb5uvr40.fsf@gitster.mtv.corp.google.com","subject":"Re: Gated Merge?","fromName":"Andrew Ardill","fromEmail":"andrew.ardill@gmail.com","sentAt":"2016-02-11T22:42:51Z","receivedAt":"2016-02-11T22:42:51Z","isPatch":false,"sender":{"key":"andrew.ardill@gmail.com","avatar":"https://gravatar.com/avatar/da14cb7c091dd44dc6c63a4d3361b149acaf25226dc78eb4131a17b93d9b0993?d=mp&s=160"},"body":"On 12 February 2016 at 09:06, Junio C Hamano wrote:\n>\n> To realize this, there are low-hanging-fruit building blocks that\n> are trivial:\n>\n> - Such an annotation can be made as a note attached to the commit\n> in question;\n>\n> - Such a check can be done in pre-commit hook;\n>\n> - Such a pre-commit hook can iterate over this set of commits\n>\n> DIFF=$(git rev-list --left-right HEAD...MERGE_HEAD)\n>\n> collect these Merge Gate annotations from the commit appears on\n> the left hand side (e.g. only exists on the trunk that a side\n> branch is about to be merged in), and run them.\n>\n> But the last one feels very costly, and I suspect that there must be\n> a better way to do this. I do not think we want the \"what to do\"\n> part (i.e. checking new lines for __attribute__ in the first\n> example, or rewriting new lines that has __attribute_ in the second\n> example) to be in git-core; that clearly is outside the scope of the\n> plumbing. But the part that finds these \"annotations\", especially\n> if we can come up with a fast implementation that may be hard to\n> script, may be a good addition to the plumbing command set.\n>\n> Thoughts?\n\nWhat is the benefit in doing this in notes vs having the tests in the\nworking tree?\n\nPros:\n\n - merge-gates can be added after the commit, but will stick with the\ncommit if it moves around (as opposed to creating a second commit to\nadd the merge-gate to the working tree)\n\n - cross repository standards can be established that allow\nmerge-gates to be detected and run automatically (arguably could be\ndone with a standardised folder structure too, but that is more\ndisruptive)\n\n - can view the relevant merge-gates in git log against each commit\nthat they are protecting\n\nCons:\n\n - difficult to see the current complete set of merge-gates\n\n - difficult to make changes to a number of merge-gates at the same time\n\n - poorly defined behaviour when multiple merge-gates overlap in\nfunctionality. Which gates execute first? What if I reorder the\ncommits?\n\n\nMy practical knowledge of notes is severely lacking so excuse me if I\nmissed anything obvious.\n\nRegards,\n\nAndrew Ardill\n\n(sorry for the double post Junio, gmail ate my plain text encoding at\nsome point...)\n"},{"id":"277979","messageId":"CAGZ79kY5MT_HZQHwh2EaoN01hEDLcA-=dCTxuejF7yiYwpPRoQ@mail.gmail.com","threadId":"41393","inReplyTo":"CAH5451=rVmdTra0rVNEkfdfOeSyeETSMo7YGWdYozZr_=YyAtw@mail.gmail.com","subject":"Re: Gated Merge?","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-11T23:53:49Z","receivedAt":"2016-02-11T23:53:49Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 11, 2016 at 2:42 PM, Andrew Ardill wrote:\n> On 12 February 2016 at 09:06, Junio C Hamano wrote:\n>>\n>> To realize this, there are low-hanging-fruit building blocks that\n>> are trivial:\n>>\n>> - Such an annotation can be made as a note attached to the commit\n>> in question;\n>>\n>> - Such a check can be done in pre-commit hook;\n>>\n>> - Such a pre-commit hook can iterate over this set of commits\n>>\n>> DIFF=$(git rev-list --left-right HEAD...MERGE_HEAD)\n>>\n>> collect these Merge Gate annotations from the commit appears on\n>> the left hand side (e.g. only exists on the trunk that a side\n>> branch is about to be merged in), and run them.\n>>\n>> But the last one feels very costly, and I suspect that there must be\n>> a better way to do this. I do not think we want the \"what to do\"\n>> part (i.e. checking new lines for __attribute__ in the first\n>> example, or rewriting new lines that has __attribute_ in the second\n>> example) to be in git-core; that clearly is outside the scope of the\n>> plumbing. But the part that finds these \"annotations\", especially\n>> if we can come up with a fast implementation that may be hard to\n>> script, may be a good addition to the plumbing command set.\n>>\n>> Thoughts?\n>\n> What is the benefit in doing this in notes vs having the tests in the\n> working tree?\n>\n> Pros:\n>\n> - merge-gates can be added after the commit, but will stick with the\n> commit if it moves around (as opposed to creating a second commit to\n> add the merge-gate to the working tree)\n>\n> - cross repository standards can be established that allow\n> merge-gates to be detected and run automatically (arguably could be\n> done with a standardised folder structure too, but that is more\n> disruptive)\n>\n> - can view the relevant merge-gates in git log against each commit\n> that they are protecting\n>\n> Cons:\n>\n> - difficult to see the current complete set of merge-gates\n>\n> - difficult to make changes to a number of merge-gates at the same time\n>\n> - poorly defined behaviour when multiple merge-gates overlap in\n> functionality. Which gates execute first? What if I reorder the\n> commits?\n\nThe way I could imagine \"merge gates\" currently:\n\n * they are some kind of invariant to be checked, or an action\nperformed, an event\n * they can be introduced by a commit X.\n * they can be turned off by a commit Y\n * an offspring commit inherits all \"merge gates\" from its parents (in\norder of the parents\n\nSo the event for one merge gate is in effect iff you merge one commit from X..Y\n\nNow we want to know for a given commit C, which merge gates are there?\nTo find all events we would need to walk the whole history of C and record\nall found events, which are not turned off.\n\nSo I'd think that could be accelerated with event bitmaps (which work\nsimilar to the\nreachability bitmaps, but having a different goal and record which\nevents are alive\nfor certain commits, such that you only need to walk a bounded set of commits to\nfind out.\n\n\nWhen first reading this email I thought this is a typical maintainers problem,\nso it should happen all the time in larger projects, such as linux, how do they\nhandle system wide refactorings?\n\n>\n>\n> My practical knowledge of notes is severely lacking so excuse me if I\n> missed anything obvious.\n>\n> Regards,\n>\n> Andrew Ardill\n>\n> (sorry for the double post Junio, gmail ate my plain text encoding at\n> some point...)\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"278018","messageId":"xmqqr3ghvn6a.fsf@gitster.mtv.corp.google.com","threadId":"41393","inReplyTo":"CAH5451=rVmdTra0rVNEkfdfOeSyeETSMo7YGWdYozZr_=YyAtw@mail.gmail.com","subject":"Re: Gated Merge?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-12T17:44:13Z","receivedAt":"2016-02-12T17:44:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrew Ardill writes:\n\n> What is the benefit in doing this in notes vs having the tests in the\n> working tree?\n\nInteresting. I have never thought of adding this information to the\nproject history proper---I've viewed this as primarily an aid for\nkeeping track of topics in-flight by an individual, i.e. something\nthat the rest of the project do not want to even see.\n\n> Pros:\n>\n> - merge-gates can be added after the commit, but will stick with the\n> commit if it moves around (as opposed to creating a second commit to\n> add the merge-gate to the working tree)\n\nI think that this will not be a problem in practice if we took your\nalternative approach to cap the topic with an extra commit that adds\nthe Merge Gate test; because the workflow this targets treat a topic\nas a single unit, the extra commit will be rebased together with the\nreal commits in the topic.\n\n> - cross repository standards can be established that allow\n> merge-gates to be detected and run automatically (arguably could be\n> done with a standardised folder structure too, but that is more\n> disruptive)\n\nYeah, I think that this is very similar to \"something that the rest\nof the project do not want to even see\" problem, if you use an in-tree\napproach.\n\n> Cons:\n>\n> - difficult to see the current complete set of merge-gates\n\nYes, we would need to have a quick way to enumerate commits with\nthese notes in the rev-list output.\n\n> - difficult to make changes to a number of merge-gates at the same time\n\nHmm, I am not sure if/how that will be an issue.\n\n> - poorly defined behaviour when multiple merge-gates overlap in\n> functionality. Which gates execute first? What if I reorder the\n> commits?\n\nTrue. With an in-tree approach, you can define the order with the\nfilenames, for example, and the above will be clearer. Instead, you\nwould need to worry about name clashes, though.\n"},{"id":"278029","messageId":"20160212190017.GA20801@sigill.intra.peff.net","threadId":"41393","inReplyTo":"xmqqr3ghvn6a.fsf@gitster.mtv.corp.google.com","subject":"Re: Gated Merge?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-12T19:00:17Z","receivedAt":"2016-02-12T19:00:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 12, 2016 at 09:44:13AM -0800, Junio C Hamano wrote:\n\n> Andrew Ardill writes:\n> \n> > What is the benefit in doing this in notes vs having the tests in the\n> > working tree?\n> \n> Interesting. I have never thought of adding this information to the\n> project history proper---I've viewed this as primarily an aid for\n> keeping track of topics in-flight by an individual, i.e. something\n> that the rest of the project do not want to even see.\n\nAfter reading Andrew's message, I wondered if these are really any\ndifferent than regular tests at all.\n\nLet's say I implement feature X on a topic branch, and I add a\nregression test for it. Once that is merged, now we have that regression\ntest forever[1], and any future topic branches that get merged from\nmaster must pass that test or be rejected.\n\nIf I update the interface for foo(), it is the same thing. We do not\nwrite a specific test for it, but we expect that the compiler will catch\nany callers of the old foo, because the new tree carries the updated\ndefinition that will not work with them (and we often structure our\ninterface refactoring exactly to catch such things).\n\nSo let's go back to your FORMAT_PRINT example. The topic changes all of\nthe format-attributes into FORMAT_PRINTF, but we never add a \"test\" that\nsays \"now there are no more bare format-attributes, and if there are, it\nis a regression\".\n\nBut I don't think this needs to have anything to do with merges in\nparticular, or rules like \"when merging a branch that does not have me\nin it\". It is about saying \"from here on out, the tree state should\nmatch this property, and we can test it by running this script\". And\nthen running \"make code-lint-tests\" becomes part of the acceptance\ntesting for a proposed topic merge, just like \"make test\" is already\n(and likewise, people forking _new_ branches from master after the topic\nis merged would make sure they do not fail the code-lint tests before\neven submitting the topic).\n\n-Peff\n\n[1] Of course it doesn't _have_ to be forever. We sometimes modify or\n back out tests as new situations come up, and we could do the same for\n these sorts of code-lint tests.\n"},{"id":"278030","messageId":"xmqq1t8hvjdv.fsf@gitster.mtv.corp.google.com","threadId":"41393","inReplyTo":"20160212190017.GA20801@sigill.intra.peff.net","subject":"Re: Gated Merge?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-12T19:06:04Z","receivedAt":"2016-02-12T19:06:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But I don't think this needs to have anything to do with merges in\n> particular, or rules like \"when merging a branch that does not have me\n> in it\". It is about saying \"from here on out, the tree state should\n> match this property, and we can test it by running this script\". And\n> then running \"make code-lint-tests\" becomes part of the acceptance\n> testing for a proposed topic merge, just like \"make test\" is already\n> (and likewise, people forking _new_ branches from master after the topic\n> is merged would make sure they do not fail the code-lint tests before\n> even submitting the topic).\n\nThat certainly is true, but this strays more and more from my\noriginal motive of implementing an automated evil-merge scheme that\nis better than what I currently have.\n\nWe try to do our tree-wide refactoring in such a way that it would\nbreak the compilation (by changing function signature) when we can.\nCatching with \"make test\" would certainly generalize it, but the\nendgame result I was shooting for was to come up with a solution for\neach topic in-flight just once and keep replaying it until it is\nmerged.\n"},{"id":"278032","messageId":"20160212192612.GA20992@sigill.intra.peff.net","threadId":"41393","inReplyTo":"xmqq1t8hvjdv.fsf@gitster.mtv.corp.google.com","subject":"Re: Gated Merge?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-12T19:26:13Z","receivedAt":"2016-02-12T19:26:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 12, 2016 at 11:06:04AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > But I don't think this needs to have anything to do with merges in\n> > particular, or rules like \"when merging a branch that does not have me\n> > in it\". It is about saying \"from here on out, the tree state should\n> > match this property, and we can test it by running this script\". And\n> > then running \"make code-lint-tests\" becomes part of the acceptance\n> > testing for a proposed topic merge, just like \"make test\" is already\n> > (and likewise, people forking _new_ branches from master after the topic\n> > is merged would make sure they do not fail the code-lint tests before\n> > even submitting the topic).\n> \n> That certainly is true, but this strays more and more from my\n> original motive of implementing an automated evil-merge scheme that\n> is better than what I currently have.\n> \n> We try to do our tree-wide refactoring in such a way that it would\n> break the compilation (by changing function signature) when we can.\n> Catching with \"make test\" would certainly generalize it, but the\n> endgame result I was shooting for was to come up with a solution for\n> each topic in-flight just once and keep replaying it until it is\n> merged.\n\nYeah, that makes sense. I do repeated merges myself, and it is a pain.\nI do not usually use your Reintegrate scripts, though when I have done\nso, they work pretty well. And I don't think you're going to come up\nwith anything much better for the general case.\n\nLet's split the problem into \"detect a problem\" from \"apply the fix for\na topic\".\n\nYou do not want to stop detecting once the original topic is merged. The\nnew rule is \"we spell this as FORMAT_PRINTF\" and you want to detect it\nin simultaneous topics, _and_ in future topics. So if it is worth\nchecking in an auomated way, that should go into the tree.\n\nSo once we've detected a problem, how do we fix it?\n\nFor the specific case of FORMAT_PRINTF, you showed a possible \"extra\nprocessing\" snippet to fix the problem in an automated way. But if we\nrealize that these gated attributes are really just another form of\ntest, it should be obvious that we cannot do so automatically in the\ngeneral case. You do not expect to fix a failed \"make test\" on a merge\nin an automated way; there are too many possible resolutions. The best\nwe can do is detect the problem, create a patch, and then apply that\npatch as appropriate.\n\nSo I'm somewhat doubtful that it would be worth the effort to create\ninfrastructure for \"automate this fix\" that is anything except \"apply\nthis patch and tell me if we now pass the test\".\n\n-Peff\n"}]}