{"thread":{"id":"26875","subject":"feature request - telling git bisect to skip, from inside a commit","startedAt":"2011-03-26T18:48:25Z","lastAt":"2011-03-29T18:23:00Z","messageCount":6,"participants":["Jim Cromie","Christian Couder","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"164378","messageId":"AANLkTinCiM9uqK8Yr=pKaeKytWXqpWF898AeTwvHKg4-@mail.gmail.com","threadId":"26875","inReplyTo":null,"subject":"feature request - telling git bisect to skip, from inside a commit","fromName":"Jim Cromie","fromEmail":"jim.cromie@gmail.com","sentAt":"2011-03-26T18:48:25Z","receivedAt":"2011-03-26T18:48:25Z","isPatch":false,"sender":{"key":"jim.cromie@gmail.com","avatar":null},"body":"sometimes its feels clearer to devote a commit to changing (for example)\nthe definition of a struct;  and changing all users of that struct in\nthe next commit.\nThis isolates and highlights the definitional change, rather than burying it in\nthe middle of a huge diff.\n\nThe downside of doing this is that git bisect will trip over this 1/2 change.\nIt would be nice if a committer could mark the commit as not bisectable,\nperhaps by just adding this, on a separate line, to the commit-message:\n\n    \"git bisect skip [optional range]\"\n\nthe range presumably would be something like CURRENT^1\nexcept that it would make more sense to flag successors than ancestors,\nand of course, CURRENT would have to mean something.\n\nAnyway, range isnt really needed, as any subsequent interim commits\ncould also flag themselves as such.\n\ngit bisect already has ability to skip a commit, this just helps an automated\nbisection script.\n"},{"id":"164405","messageId":"201103280603.16256.chriscool@tuxfamily.org","threadId":"26875","inReplyTo":"AANLkTinCiM9uqK8Yr=pKaeKytWXqpWF898AeTwvHKg4-@mail.gmail.com","subject":"Re: feature request - telling git bisect to skip, from inside a commit","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2011-03-28T04:03:16Z","receivedAt":"2011-03-28T04:03:16Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Saturday 26 March 2011 19:48:25 Jim Cromie wrote:\n> sometimes its feels clearer to devote a commit to changing (for example)\n> the definition of a struct;  and changing all users of that struct in\n> the next commit.\n> This isolates and highlights the definitional change, rather than burying\n> it in the middle of a huge diff.\n> \n> The downside of doing this is that git bisect will trip over this 1/2\n> change. It would be nice if a committer could mark the commit as not\n> bisectable, perhaps by just adding this, on a separate line, to the\n> commit-message:\n> \n>     \"git bisect skip [optional range]\"\n> \n> the range presumably would be something like CURRENT^1\n> except that it would make more sense to flag successors than ancestors,\n> and of course, CURRENT would have to mean something.\n> \n> Anyway, range isnt really needed, as any subsequent interim commits\n> could also flag themselves as such.\n> \n> git bisect already has ability to skip a commit, this just helps an\n> automated bisection script.\n\nPlease look at this recent thread:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/169026/\n\nwhere there are other suggestions and discussions about how to deal with this \nkind of problems.\n\nThanks,\nChristian.\n"},{"id":"164457","messageId":"20110328163153.GA18774@sigill.intra.peff.net","threadId":"26875","inReplyTo":"AANLkTinCiM9uqK8Yr=pKaeKytWXqpWF898AeTwvHKg4-@mail.gmail.com","subject":"Re: feature request - telling git bisect to skip, from inside a commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-28T16:31:53Z","receivedAt":"2011-03-28T16:31:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 26, 2011 at 12:48:25PM -0600, Jim Cromie wrote:\n\n> sometimes its feels clearer to devote a commit to changing (for example)\n> the definition of a struct;  and changing all users of that struct in\n> the next commit.\n> This isolates and highlights the definitional change, rather than burying it in\n> the middle of a huge diff.\n> \n> The downside of doing this is that git bisect will trip over this 1/2 change.\n> It would be nice if a committer could mark the commit as not bisectable,\n> perhaps by just adding this, on a separate line, to the commit-message:\n> \n>     \"git bisect skip [optional range]\"\n> \n> the range presumably would be something like CURRENT^1\n> except that it would make more sense to flag successors than ancestors,\n> and of course, CURRENT would have to mean something.\n\nThat could work, though I would spell it as a pseudo-header:\n\n  Bisect: Skip\n\nat the end of each commit. But I'm not sure a range makes sense. Commits\ncan get rebased, or patches applied in a different order. So putting\nsomething like that into the message at commit time can be fragile.\n\nI wonder if doing this at commit time at all really makes sense, though.\nIt means you have to know up-front that you are breaking things. Which\nyes, does happen. But it also happens frequently that you only realize\nlater while bisecting that your commit is bogus. Or maybe you realize\ntwo commits down the line that your HEAD~2 is broken, but you can't use\n\"rebase -i\" to fix it because you already pushed it.\n\nSo what if instead of marking at commit time, we kept a \"skip cache\".\nBisection would consult any entries marked in the skip cache and skip\nthem automatically. When you issued a \"git bisect skip\", it would not\nonly skip the commits now, but would also add them to the skip cache.\n\nIf we kept the skip cache in a git-notes tree, then you could share the\ncache with others (though to be fair, this is slightly less convenient\nthan simply having it in the commit header, which survives over things\nlike format-patch).\n\nAnd of course nothing would stop you from marking an item in the skip\ncache at commit time, or any other time.\n\nOne downside to this scheme (or any skip-marking scheme) is that some\nskips may depend on the bisection you are doing. Obviously if the\nprogram doesn't build, that breaks everyone. But let's say you skip a\nseries of commits because they have a bug in the \"foo\" program, and your\nbisection script must run \"foo\" then \"bar\" to get its result. But later,\nyou do another bisection that doesn't care about \"foo\" at all, but the\nresult of the bisection would be in the middle of the skipped series.\nYou won't find it exactly, because you are skipping too many commits.\n\n> git bisect already has ability to skip a commit, this just helps an\n> automated bisection script.\n\nIf you have an automated script, you could do something like this\noutside of git-bisect entirely.\n\nWhen you want to mark a commit as bad, do:\n\n  git notes --ref=skip add -m \"some reason it is bogus\" <commit>\n\nThen at the beginning of your test script, do something like:\n\n  skip=`git notes --ref=skip show`\n  if test -n \"$skip\"; then\n    echo >&2 \"Skipping commit: $skip\"\n    exit 125\n  fi\n\nOf course, for the example you gave, it may be even easier. The code in\nyour patch 1/2 would fail to compile. So just doing \"make || exit 125\"\nin your bisect script would be enough, without any skip cache. But there\nare certainly cases where it is nice to be able to mark something as\nskippable (e.g., a kernel that builds, but is flaky on your test\nhardware. You would much rather save the build and reboot just to find\nout it is broken).\n\n-Peff\n"},{"id":"164461","messageId":"7vipv31943.fsf@alter.siamese.dyndns.org","threadId":"26875","inReplyTo":"20110328163153.GA18774@sigill.intra.peff.net","subject":"Re: feature request - telling git bisect to skip, from inside a commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-28T16:51:40Z","receivedAt":"2011-03-28T16:51:40Z","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> That could work, though I would spell it as a pseudo-header:\n>\n>   Bisect: Skip\n>\n> at the end of each commit.\n\nI think that is a saner approach, and further say it would be much saner\nto make that token something like\n\n    Broken: does not build\n\nA commit may or may not build, a build product may work in some areas just\nfine and may have known bugs in some other areas, depending on what kind\nof breakage you are interested in.  You might even be looking for a change\nthat fixed a bug for cherry picking.  In short, \"Bisect: Skip\" is too\nbroad a brush, and does not convey enough information.\n\nAnd then teach the script you give to \"bisect run\" to grep for that\n\"^Broken: \" pattern to answer with exit 125 (cannot test), and you are\ndone.\n"},{"id":"164582","messageId":"AANLkTin+0yScj2ejVgPSMmY6Q+Qk0MM+W+EqqLGphCrz@mail.gmail.com","threadId":"26875","inReplyTo":"7vipv31943.fsf@alter.siamese.dyndns.org","subject":"Re: feature request - telling git bisect to skip, from inside a commit","fromName":"Jim Cromie","fromEmail":"jim.cromie@gmail.com","sentAt":"2011-03-29T17:00:00Z","receivedAt":"2011-03-29T17:00:00Z","isPatch":false,"sender":{"key":"jim.cromie@gmail.com","avatar":null},"body":"On Mon, Mar 28, 2011 at 10:51 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> That could work, though I would spell it as a pseudo-header:\n>>\n>>   Bisect: Skip\n>>\n>> at the end of each commit.\n\nI like this generically.  It is more constrained than a\n/^bisect skip(.*)$/ matching, and perhaps easier to code.\n\nThank you for reading into my request and formulating a better version,\nthe discussion/explication of issues also helps.\n\nIf the pseudo-header can be added into the commit message,\nit is trivial to use.\n\nI see the merit of a skip-cache as working after-the-fact\non already published/shared patch-sets, but Im unsure how the\nskip-cache can be shared currently.\n\n>\n> I think that is a saner approach, and further say it would be much saner\n> to make that token something like\n>\n>    Broken: does not build\n\nMy only concern here is the negative connotation of Broken.\nAt least in my 1/2 change scenario, thats known to be incomplete,\nits kinda pejorative.  But it certainly gets the job done.\n\nSkip: make rc 125\nSkip: 125\nSkip: gcc error: no such field: foo\nSkip: api change only, users must follow\n\nSkip: has less pejorative, and closer name-association linkage\nto the bisect skip command that it\n\n>\n> A commit may or may not build, a build product may work in some areas just\n> fine and may have known bugs in some other areas, depending on what kind\n> of breakage you are interested in.  You might even be looking for a change\n> that fixed a bug for cherry picking.  In short, \"Bisect: Skip\" is too\n> broad a brush, and does not convey enough information.\n>\n\nagreed -  It would be interesting, 6 months after the feature is added (I hope),\n to search commit messages and see how the header has been used.\n\n> And then teach the script you give to \"bisect run\" to grep for that\n> \"^Broken: \" pattern to answer with exit 125 (cannot test), and you are\n> done.\n>\n\nWith this, nothing needs to be added.\nThe only advantage to bisect looking for the pseudo-header is that\na convention is supported, such that it might get used.\nI suppose a 1 line addition to git bisect run documentation would\ndo just as well.\n\nthanks\n"},{"id":"164602","messageId":"20110329182300.GB20784@sigill.intra.peff.net","threadId":"26875","inReplyTo":"AANLkTin+0yScj2ejVgPSMmY6Q+Qk0MM+W+EqqLGphCrz@mail.gmail.com","subject":"Re: feature request - telling git bisect to skip, from inside a commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-29T18:23:00Z","receivedAt":"2011-03-29T18:23:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 29, 2011 at 11:00:00AM -0600, Jim Cromie wrote:\n\n> If the pseudo-header can be added into the commit message,\n> it is trivial to use.\n> \n> I see the merit of a skip-cache as working after-the-fact\n> on already published/shared patch-sets, but Im unsure how the\n> skip-cache can be shared currently.\n\nIf you use git-notes as I did in the example of my first response, then\nyou will end up with a ref \"refs/notes/skip\". You can push it to\nwherever:\n\n  git push origin refs/notes/skip\n\nand then everybody else can configure their repos to fetch it:\n\n  git config --add remote.origin.fetch \\\n    +refs/notes/skip:refs/notes/origin/skip\n\nI think they would still have to manually merge it into their own skip\ncache with \"git notes merge\". I'm not sure. The notes-merging code is\nvery new (v1.7.4, I believe), and I think we are still figuring out what\nworkflows make sense around it (which is why it is still manual).\n\n> > I think that is a saner approach, and further say it would be much saner\n> > to make that token something like\n> >\n> >    Broken: does not build\n> \n> My only concern here is the negative connotation of Broken.\n> At least in my 1/2 change scenario, thats known to be incomplete,\n> its kinda pejorative.  But it certainly gets the job done.\n> \n> Skip: make rc 125\n> Skip: 125\n> Skip: gcc error: no such field: foo\n> Skip: api change only, users must follow\n> \n> Skip: has less pejorative, and closer name-association linkage\n> to the bisect skip command that it\n\nYeah, I think a more neutral name makes sense, since there are many\nreasons to skip (and in fact, broken builds are among the least\ninteresting to mark, since those are easy to detect at bisection time).\n\n> > And then teach the script you give to \"bisect run\" to grep for that\n> > \"^Broken: \" pattern to answer with exit 125 (cannot test), and you are\n> > done.\n> >\n> \n> With this, nothing needs to be added.\n> The only advantage to bisect looking for the pseudo-header is that\n> a convention is supported, such that it might get used.\n\nIf you do use this, I would love to hear a report on how it works 6\nmonths in. If it's useful, then it might make more sense to clean up\nrough edges with better tool support. Or maybe it works perfectly\nwithout tool support, or maybe it turns out to be a flawed idea. :)\n\n-Peff\n"}]}