{"thread":{"id":"46203","subject":"[PATCH v2 0/2] Fix a refname trimming problem in `log --bisect`","startedAt":"2017-06-18T13:40:04Z","lastAt":"2017-06-19T16:06:41Z","messageCount":6,"participants":["Michael Haggerty","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"322527","messageId":"cover.1497792827.git.mhagger@alum.mit.edu","threadId":"46203","inReplyTo":null,"subject":"[PATCH v2 0/2] Fix a refname trimming problem in `log --bisect`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-06-18T13:39:40Z","receivedAt":"2017-06-18T13:40:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is v2 of these patches. Thanks to Peff and Junio for their\nfeedback about v1 [1].\n\nChanges since v1:\n\n* Added a test and tweaked the commit message of the first patch.\n\nAs before, the second patch is optional. If it is omitted, it might\nflush out any other bugs like this one in client code. If it is\nincluded, regressions are less likely, but we won't learn about other\nmisuses of the API. I have no strong opinion either way.\n\nMichael\n\n[1] http://public-inbox.org/git/cover.1497430232.git.mhagger@alum.mit.edu/\n\nMichael Haggerty (2):\n  for_each_bisect_ref(): don't trim refnames\n  prefix_ref_iterator_advance(): relax the check of trim length\n\n refs.c                     | 12 ++++++++++++\n refs.h                     |  5 ++++-\n refs/iterator.c            |  8 ++++----\n revision.c                 |  2 +-\n t/t6002-rev-list-bisect.sh | 14 ++++++++++++++\n 5 files changed, 35 insertions(+), 6 deletions(-)\n\n-- \n2.11.0\n\n"},{"id":"322528","messageId":"dc91209f6e11a00a8d9fa70a74cf4b25528a67d1.1497792827.git.mhagger@alum.mit.edu","threadId":"46203","inReplyTo":"cover.1497792827.git.mhagger@alum.mit.edu","subject":"[PATCH v2 1/2] for_each_bisect_ref(): don't trim refnames","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-06-18T13:39:41Z","receivedAt":"2017-06-18T13:40:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"`for_each_bisect_ref()` is called by `for_each_bad_bisect_ref()` with\na term \"bad\". This used to make it call `for_each_ref_in_submodule()`\nwith a prefix \"refs/bisect/bad\". But the latter is the name of the\nreference that is being sought, so the empty string was being passed\nto the callback as the trimmed refname. Moreover, this questionable\npractice was turned into an error by\n\n    b9c8e7f2fb prefix_ref_iterator: don't trim too much, 2017-05-22\n\nIt makes more sense (and agrees better with the documentation of\n`--bisect`) for the callers to receive the full reference names. So\n\n* Add a new function, `for_each_fullref_in_submodule()`, to the refs\n  API. This plugs a gap in the existing functionality, analogous to\n  `for_each_fullref_in()` but accepting a `submodule` argument.\n\n* Change `for_each_bad_bisect_ref()` to call the new function rather\n  than `for_each_ref_in_submodule()`.\n\n* Add a test.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c                     | 12 ++++++++++++\n refs.h                     |  5 ++++-\n revision.c                 |  2 +-\n t/t6002-rev-list-bisect.sh | 14 ++++++++++++++\n 4 files changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex f0685c9251..32177969f0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1341,6 +1341,18 @@ int for_each_ref_in_submodule(const char *submodule, const char *prefix,\n \t\t\t\t    prefix, fn, cb_data);\n }\n \n+int for_each_fullref_in_submodule(const char *submodule, const char *prefix,\n+\t\t\t\t  each_ref_fn fn, void *cb_data,\n+\t\t\t\t  unsigned int broken)\n+{\n+\tunsigned int flag = 0;\n+\n+\tif (broken)\n+\t\tflag = DO_FOR_EACH_INCLUDE_BROKEN;\n+\treturn do_for_each_ref(get_submodule_ref_store(submodule),\n+\t\t\t       prefix, fn, 0, flag, cb_data);\n+}\n+\n int for_each_replace_ref(each_ref_fn fn, void *cb_data)\n {\n \treturn do_for_each_ref(get_main_ref_store(),\ndiff --git a/refs.h b/refs.h\nindex 4be14c4b3c..aa4ecc83d0 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -303,7 +303,10 @@ int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n int for_each_ref_submodule(const char *submodule,\n \t\t\t   each_ref_fn fn, void *cb_data);\n int for_each_ref_in_submodule(const char *submodule, const char *prefix,\n-\t\teach_ref_fn fn, void *cb_data);\n+\t\t\t      each_ref_fn fn, void *cb_data);\n+int for_each_fullref_in_submodule(const char *submodule, const char *prefix,\n+\t\t\t\t  each_ref_fn fn, void *cb_data,\n+\t\t\t\t  unsigned int broken);\n int for_each_tag_ref_submodule(const char *submodule,\n \t\t\t       each_ref_fn fn, void *cb_data);\n int for_each_branch_ref_submodule(const char *submodule,\ndiff --git a/revision.c b/revision.c\nindex 9c67cb6026..50039c92d6 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2044,7 +2044,7 @@ static int for_each_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_d\n \tstruct strbuf bisect_refs = STRBUF_INIT;\n \tint status;\n \tstrbuf_addf(&bisect_refs, \"refs/bisect/%s\", term);\n-\tstatus = for_each_ref_in_submodule(submodule, bisect_refs.buf, fn, cb_data);\n+\tstatus = for_each_fullref_in_submodule(submodule, bisect_refs.buf, fn, cb_data, 0);\n \tstrbuf_release(&bisect_refs);\n \treturn status;\n }\ndiff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh\nindex 3bf2759eae..534903bbd2 100755\n--- a/t/t6002-rev-list-bisect.sh\n+++ b/t/t6002-rev-list-bisect.sh\n@@ -235,4 +235,18 @@ test_sequence \"--bisect\"\n \n #\n #\n+\n+test_expect_success '--bisect can default to good/bad refs' '\n+\tgit update-ref refs/bisect/bad c3 &&\n+\tgood=$(git rev-parse b1) &&\n+\tgit update-ref refs/bisect/good-$good $good &&\n+\tgood=$(git rev-parse c1) &&\n+\tgit update-ref refs/bisect/good-$good $good &&\n+\n+\t# the only thing between c3 and c1 is c2\n+\tgit rev-parse c2 >expect &&\n+\tgit rev-list --bisect >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.11.0\n\n"},{"id":"322529","messageId":"7ea7841cf2ea9eb17535d5e655c7834347d5acfe.1497792827.git.mhagger@alum.mit.edu","threadId":"46203","inReplyTo":"cover.1497792827.git.mhagger@alum.mit.edu","subject":"[PATCH v2 2/2] prefix_ref_iterator_advance(): relax the check of trim length","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-06-18T13:39:42Z","receivedAt":"2017-06-18T13:40:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Before the previous commit, `for_each_bad_bisect_ref()` called\n`for_each_fullref_in_submodule()` in such a way as to trim the whole\nrefname away. This is a questionable use of the API, but is not ipso\nfacto dangerous, so tolerate it in case there are other callers\nrelying on this behavior. But continue to refuse to trim *more*\ncharacters than the refname contains, as that really makes no sense.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/iterator.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/refs/iterator.c b/refs/iterator.c\nindex 4cf449ef66..de52d5fe93 100644\n--- a/refs/iterator.c\n+++ b/refs/iterator.c\n@@ -298,11 +298,11 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \t\t\t * you haven't already checked for via a\n \t\t\t * prefix check, whether via this\n \t\t\t * `prefix_ref_iterator` or upstream in\n-\t\t\t * `iter0`). So if there wouldn't be at least\n-\t\t\t * one character left in the refname after\n-\t\t\t * trimming, report it as a bug:\n+\t\t\t * `iter0`. So consider it a bug if we are\n+\t\t\t * asked to trim off more characters than the\n+\t\t\t * refname contains:\n \t\t\t */\n-\t\t\tif (strlen(iter->iter0->refname) <= iter->trim)\n+\t\t\tif (strlen(iter->iter0->refname) < iter->trim)\n \t\t\t\tdie(\"BUG: attempt to trim too many characters\");\n \t\t\titer->base.refname = iter->iter0->refname + iter->trim;\n \t\t} else {\n-- \n2.11.0\n\n"},{"id":"322552","messageId":"20170619091654.vb7krwcmw2y22v7m@sigill.intra.peff.net","threadId":"46203","inReplyTo":"cover.1497792827.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 0/2] Fix a refname trimming problem in `log --bisect`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-19T09:16:54Z","receivedAt":"2017-06-19T09:17:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 18, 2017 at 03:39:40PM +0200, Michael Haggerty wrote:\n\n> This is v2 of these patches. Thanks to Peff and Junio for their\n> feedback about v1 [1].\n> \n> Changes since v1:\n> \n> * Added a test and tweaked the commit message of the first patch.\n\nThanks, both changes look good to me.\n\n> As before, the second patch is optional. If it is omitted, it might\n> flush out any other bugs like this one in client code. If it is\n> included, regressions are less likely, but we won't learn about other\n> misuses of the API. I have no strong opinion either way.\n\nMy feeling is still slightly towards \"don't include\", but I also don't\nhave a strong opinion.\n\n-Peff\n"},{"id":"322568","messageId":"xmqqo9tkt1qz.fsf@gitster.mtv.corp.google.com","threadId":"46203","inReplyTo":"20170619091654.vb7krwcmw2y22v7m@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/2] Fix a refname trimming problem in `log --bisect`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-19T15:51:00Z","receivedAt":"2017-06-19T15:51:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sun, Jun 18, 2017 at 03:39:40PM +0200, Michael Haggerty wrote:\n>\n>> As before, the second patch is optional. If it is omitted, it might\n>> flush out any other bugs like this one in client code. If it is\n>> included, regressions are less likely, but we won't learn about other\n>> misuses of the API. I have no strong opinion either way.\n>\n> My feeling is still slightly towards \"don't include\", but I also don't\n> have a strong opinion.\n\nI am inclined to the \"don't include 2/2 and cook 1/2 alone but a bit\nlonger\" approach.\n\nThanks, both.\n"},{"id":"322570","messageId":"20170619160551.cdw3eslu7ehiivh2@sigill.intra.peff.net","threadId":"46203","inReplyTo":"xmqqo9tkt1qz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/2] Fix a refname trimming problem in `log --bisect`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-19T16:05:51Z","receivedAt":"2017-06-19T16:06:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 19, 2017 at 08:51:00AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Sun, Jun 18, 2017 at 03:39:40PM +0200, Michael Haggerty wrote:\n> >\n> >> As before, the second patch is optional. If it is omitted, it might\n> >> flush out any other bugs like this one in client code. If it is\n> >> included, regressions are less likely, but we won't learn about other\n> >> misuses of the API. I have no strong opinion either way.\n> >\n> > My feeling is still slightly towards \"don't include\", but I also don't\n> > have a strong opinion.\n> \n> I am inclined to the \"don't include 2/2 and cook 1/2 alone but a bit\n> longer\" approach.\n\nI don't think it helps to cook 1/2 longer. The assertion that triggered\nis already in master. Patch 1 fixes one case in an obviously-correct\nway. Patch 2 is just about guessing whether _other_ cases may trigger.\n\nSo if we wanted to cook longer we'd have to revert b9c8e7f2f\n(prefix_ref_iterator: don't trim too much, 2017-05-22), but I don't\nthink that's worth doing.\n\n-Peff\n"}]}