{"thread":{"id":"46171","subject":"[BUG] b9c8e7f2fb6e breaks git bisect visualize","startedAt":"2017-06-14T00:06:37Z","lastAt":"2017-06-15T22:26:51Z","messageCount":12,"participants":["Øyvind A. Holm","Michael Haggerty","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"322173","messageId":"20170614000630.44uctc5y7dyyleqy@sunbase.org","threadId":"46171","inReplyTo":null,"subject":"[BUG] b9c8e7f2fb6e breaks git bisect visualize","fromName":"Øyvind A. Holm","fromEmail":"sunny@sunbase.org","sentAt":"2017-06-14T00:06:32Z","receivedAt":"2017-06-14T00:06:37Z","isPatch":false,"sender":{"key":"sunny@sunbase.org","avatar":"https://avatars.githubusercontent.com/u/113445?v=4"},"body":"Commit b9c8e7f2fb6e (\"prefix_ref_iterator: don't trim too much\") breaks \ngit bisect visualize, this reproduces the bug:\n\n  $ git bisect start\n  $ git bisect bad\n  $ git bisect good HEAD^^\n  $ git bisect visualize\n  fatal: BUG: attempt to trim too many characters\n  $\n\nReverting b9c8e7f2fb6e makes git bisect visualize work again.\n\nTested on Debian GNU/Linux 8.8 (jessie).\n\nØyvind\n\nN 60.37604° E 5.33339°\nOpenPGP fingerprint: A006 05D6 E676 B319 55E2  E77E FB0C BEE8 94A5 06E5\ndcacbb24-5094-11e7-b7e4-db5caa6d21d3\n"},{"id":"322203","messageId":"5a3f6af6-f936-50e7-5fca-c41b3aeefdce@alum.mit.edu","threadId":"46171","inReplyTo":"20170614000630.44uctc5y7dyyleqy@sunbase.org","subject":"Re: [BUG] b9c8e7f2fb6e breaks git bisect visualize","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-06-14T08:36:43Z","receivedAt":"2017-06-14T08:37:22Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/14/2017 02:06 AM, Øyvind A. Holm wrote:\n> Commit b9c8e7f2fb6e (\"prefix_ref_iterator: don't trim too much\") breaks \n> git bisect visualize, this reproduces the bug:\n> \n>   $ git bisect start\n>   $ git bisect bad\n>   $ git bisect good HEAD^^\n>   $ git bisect visualize\n>   fatal: BUG: attempt to trim too many characters\n>   $\n> \n> Reverting b9c8e7f2fb6e makes git bisect visualize work again.\n\nThanks for the bug report.\n\nThe same error occurs if the last step is simplified to\n\n    git log --bisect\n\nThe corresponding stack trace is\n\n#0  prefix_ref_iterator_advance (ref_iterator=0x91c5a0) at\nrefs/iterator.c:305\n#1  0x000000000054edd7 in ref_iterator_advance (ref_iterator=0x91c5a0)\n    at refs/iterator.c:13\n#2  0x000000000054f62f in do_for_each_ref_iterator (iter=0x91c5a0,\n    fn=0x56337a <handle_one_ref>, cb_data=0x7fffffffcdb0)\n    at refs/iterator.c:382\n#3  0x0000000000546a40 in do_for_each_ref (refs=0x8ce3c0,\n    prefix=0x8c1af0 \"refs/bisect/bad\", fn=0x56337a <handle_one_ref>,\ntrim=15,\n    flags=0, cb_data=0x7fffffffcdb0) at refs.c:1298\n#4  0x0000000000546b2d in refs_for_each_ref_in (refs=0x8ce3c0,\n    prefix=0x8c1af0 \"refs/bisect/bad\", fn=0x56337a <handle_one_ref>,\n    cb_data=0x7fffffffcdb0) at refs.c:1319\n#5  0x0000000000546bf9 in for_each_ref_in_submodule (submodule=0x0,\n    prefix=0x8c1af0 \"refs/bisect/bad\", fn=0x56337a <handle_one_ref>,\n    cb_data=0x7fffffffcdb0) at refs.c:1340\n#6  0x0000000000566842 in for_each_bisect_ref (submodule=0x0,\n    fn=0x56337a <handle_one_ref>, cb_data=0x7fffffffcdb0, term=0x8ce600\n\"bad\")\n    at revision.c:2083\n#7  0x0000000000566885 in for_each_bad_bisect_ref (submodule=0x0,\n    fn=0x56337a <handle_one_ref>, cb_data=0x7fffffffcdb0) at revision.c:2090\n#8  0x0000000000563541 in handle_refs (submodule=0x0, revs=0x7fffffffd210,\n    flags=0, for_each=0x566856 <for_each_bad_bisect_ref>) at revision.c:1196\n#9  0x0000000000566a09 in handle_revision_pseudo_opt (submodule=0x0,\n    revs=0x7fffffffd210, argc=1, argv=0x7fffffffdd28, flags=0x7fffffffcf44)\n    at revision.c:2125\n#10 0x000000000056711e in setup_revisions (argc=2, argv=0x7fffffffdd20,\n    revs=0x7fffffffd210, opt=0x7fffffffd1f0) at revision.c:2247\n#11 0x0000000000448ce4 in cmd_log_init_finish (argc=2, argv=0x7fffffffdd20,\n    prefix=0x0, rev=0x7fffffffd210, opt=0x7fffffffd1f0) at builtin/log.c:168\n#12 0x0000000000448f53 in cmd_log_init (argc=2, argv=0x7fffffffdd20,\n    prefix=0x0, rev=0x7fffffffd210, opt=0x7fffffffd1f0) at builtin/log.c:220\n#13 0x000000000044a37f in cmd_log (argc=2, argv=0x7fffffffdd20, prefix=0x0)\n    at builtin/log.c:692\n#14 0x0000000000405983 in run_builtin (p=0x870158 <commands+1176>, argc=2,\n    argv=0x7fffffffdd20) at git.c:371\n#15 0x0000000000405bed in handle_builtin (argc=2, argv=0x7fffffffdd20)\n    at git.c:572\n#16 0x0000000000405d62 in run_argv (argcp=0x7fffffffdbdc,\nargv=0x7fffffffdbd0)\n    at git.c:624\n#17 0x0000000000405f04 in cmd_main (argc=2, argv=0x7fffffffdd20) at\ngit.c:701\n#18 0x0000000000498ba6 in main (argc=3, argv=0x7fffffffdd18)\n    at common-main.c:43\n\nThe code for `git log --bisect` is questionable. It calls\n`for_each_ref_in_submodule()` with prefix \"refs/bisect/bad\", which is\nthe actual name (not a prefix) of the reference that it is interested\nin. So the callback is called with the empty string as path, and that in\nturn is passed to a variety of functions, like `ref_excluded()`,\n`get_reference()`, `add_rev_cmdline()`, and `add_pending_oid()`. I'm not\nsure whom to ping; the code in question was introduced eons ago:\n\n    ad3f9a71a8 Add '--bisect' revision machinery argument, 2009-10-27\n\nIt seems to me that we should add a `for_each_fullref_in_submodule()`\nand call that instead. I'll submit a patch doing that, though I'm not\ncertain that no new problems will arise from the callbacks getting full\nrather than trimmed reference names (also for \"refs/bisect/good\").\n\nAnother possible orthogonal \"fix\" is to make the refs side tolerate\nbeing asked to trim a refname down to the empty string, while still\nrefusing to trim even more than that. I'll also submit a patch to that\neffect.\n\nEither of the patches fix the issue that was reported and pass the whole\ntest suite (except for t1308, which seems to be broken in master for\nunrelated reasons).\n\nMichael\n\n"},{"id":"322204","messageId":"cover.1497430232.git.mhagger@alum.mit.edu","threadId":"46171","inReplyTo":"5a3f6af6-f936-50e7-5fca-c41b3aeefdce@alum.mit.edu","subject":"[PATCH 0/2] Fix a refname trimming problem in `log --bisect`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-06-14T09:07:25Z","receivedAt":"2017-06-14T09:09:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The code for `git log --bisect` was calling\n`for_each_ref_in_submodule()` with prefix set to \"refs/bisect/bad\",\nwhich is the actual name of the reference that it wants. This resulted\nin the refname being trimmed completely away and the empty string\nbeing passed to the callback. That became impermissible after\n\n    b9c8e7f2fb prefix_ref_iterator: don't trim too much, 2017-05-22\n\n, so the command was failing.\n\nFix the problem in two orthogonal ways:\n\n1. Add a new function, `for_each_fullref_in_submodule()`, that doesn't\n   trim the refnames that it passes to callbacks, and us that instead.\n   I *think* that this is a strict improvement, though I don't know\n   the `git log` code well enough to be sure that it won't have bad\n   side-effects.\n\n2. Relax the \"trimming too many characters\" check to allow the full\n   length of the refname to be trimmed away (though not more than\n   that).\n\nIn an ideal world the second patch shouldn't be necessary, because\nthis calling pattern is questionable and it might be better that we\nlearn about any other offenders. But if we'd rather be conservative\nand not break any other code that might rely on the old behavior,\npatch 2 is my suggestion for how to do it.\n\nThis patch series can be applied on top of branch\n`mh/packed-ref-store-prep`, but it also applies cleanly to master. It\nis also available as branch `fix-bisect-trim-check` from my GitHub\nfork [1].\n\nMichael\n\n[1] https://github.com/mhagger/git\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 4 files changed, 21 insertions(+), 6 deletions(-)\n\n-- \n2.11.0\n\n"},{"id":"322205","messageId":"0c63f38bf922f285d6d62fc9cbbc3f5b756e75bf.1497430232.git.mhagger@alum.mit.edu","threadId":"46171","inReplyTo":"cover.1497430232.git.mhagger@alum.mit.edu","subject":"[PATCH 2/2] prefix_ref_iterator_advance(): relax the check of trim length","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-06-14T09:07:27Z","receivedAt":"2017-06-14T09:09:27Z","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":"322206","messageId":"3615deefe90bebe746618b04c055a466a442f85b.1497430232.git.mhagger@alum.mit.edu","threadId":"46171","inReplyTo":"cover.1497430232.git.mhagger@alum.mit.edu","subject":"[PATCH 1/2] for_each_bisect_ref(): don't trim refnames","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-06-14T09:07:26Z","receivedAt":"2017-06-14T09:09:40Z","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.\n\n* Change `for_each_bad_bisect_ref()` to call the new function rather\n  than `for_each_ref_in_submodule()`.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c     | 12 ++++++++++++\n refs.h     |  5 ++++-\n revision.c |  2 +-\n 3 files changed, 17 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 }\n-- \n2.11.0\n\n"},{"id":"322207","messageId":"20170614091830.et7bmoxmcmgosun3@sigill.intra.peff.net","threadId":"46171","inReplyTo":"5a3f6af6-f936-50e7-5fca-c41b3aeefdce@alum.mit.edu","subject":"Re: [BUG] b9c8e7f2fb6e breaks git bisect visualize","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-14T09:18:30Z","receivedAt":"2017-06-14T09:18:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 14, 2017 at 10:36:43AM +0200, Michael Haggerty wrote:\n\n> The code for `git log --bisect` is questionable. It calls\n> `for_each_ref_in_submodule()` with prefix \"refs/bisect/bad\", which is\n> the actual name (not a prefix) of the reference that it is interested\n> in. So the callback is called with the empty string as path, and that in\n> turn is passed to a variety of functions, like `ref_excluded()`,\n> `get_reference()`, `add_rev_cmdline()`, and `add_pending_oid()`. I'm not\n> sure whom to ping; the code in question was introduced eons ago:\n> \n>     ad3f9a71a8 Add '--bisect' revision machinery argument, 2009-10-27\n> \n> It seems to me that we should add a `for_each_fullref_in_submodule()`\n> and call that instead. I'll submit a patch doing that, though I'm not\n> certain that no new problems will arise from the callbacks getting full\n> rather than trimmed reference names (also for \"refs/bisect/good\").\n\nI doubt that would be a problem. The current values are nonsensical (an\nempty string for bad, and \"-$sha1\" for good. They're mostly used in the\ncmdline and pending lists. It would affect things like --exclude or\n--source. I doubt anybody cares, but if they do IMHO the full names\nwould be a vast improvement.\n\nAnother option would be for this code to ask for:\n\n  for_each_ref_in(\"refs/bisect\", ...);\n\nand then match the various names in the callback. That gives sane short\nnames (\"bad\" and \"good-$sha1\"). But I think the full names are a much\nbetter outcome. Some code (though note code I'd expect to use with\n--bisect) assumes that the contents of the \"name\" field for pending\nobjects can be used to look up the object again. That's definitely not\ntrue of the nonsense we produce now, but it would also not be true of\n\"bad\" (because we don't DWIM refs/bisect).\n\n> Another possible orthogonal \"fix\" is to make the refs side tolerate\n> being asked to trim a refname down to the empty string, while still\n> refusing to trim even more than that. I'll also submit a patch to that\n> effect.\n\nEven though the resulting \"name\" is silly, it does seem possible that\nsome caller would want to ask for all of refs/foo, even if it didn't\nknow if that was a single ref or a hierarchy. I do agree that any such\ncaller should probably be using for_each_fullref_in, though.\n\n> Either of the patches fix the issue that was reported and pass the whole\n> test suite (except for t1308, which seems to be broken in master for\n> unrelated reasons).\n\nIt's broken if you use autoconf. See the patch I posted a few hours ago:\n\n  http://public-inbox.org/git/20170614053018.pbeftfyz2md4o73h@sigill.intra.peff.net/\n\n-Peff\n"},{"id":"322208","messageId":"20170614092454.7mtaqnvhiho5yslx@sigill.intra.peff.net","threadId":"46171","inReplyTo":"cover.1497430232.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 0/2] Fix a refname trimming problem in `log --bisect`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-14T09:24:54Z","receivedAt":"2017-06-14T09:25:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 14, 2017 at 11:07:25AM +0200, Michael Haggerty wrote:\n\n> Fix the problem in two orthogonal ways:\n> \n> 1. Add a new function, `for_each_fullref_in_submodule()`, that doesn't\n>    trim the refnames that it passes to callbacks, and us that instead.\n>    I *think* that this is a strict improvement, though I don't know\n>    the `git log` code well enough to be sure that it won't have bad\n>    side-effects.\n\nI think this is fine, for the reasons I gave elsewhere in the thread.\n\n> 2. Relax the \"trimming too many characters\" check to allow the full\n>    length of the refname to be trimmed away (though not more than\n>    that).\n> \n> In an ideal world the second patch shouldn't be necessary, because\n> this calling pattern is questionable and it might be better that we\n> learn about any other offenders. But if we'd rather be conservative\n> and not break any other code that might rely on the old behavior,\n> patch 2 is my suggestion for how to do it.\n\nMy preference would be to hold off on (2) if we can avoid it. It's\ncleaner, and I think flushing out these kinds of bugs is useful.\n\n-Peff\n"},{"id":"322209","messageId":"20170614092256.c3fmfcokuwbbcvbz@sigill.intra.peff.net","threadId":"46171","inReplyTo":"3615deefe90bebe746618b04c055a466a442f85b.1497430232.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 1/2] for_each_bisect_ref(): don't trim refnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-14T09:22:57Z","receivedAt":"2017-06-14T09:27:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 14, 2017 at 11:07:26AM +0200, Michael Haggerty wrote:\n\n> `for_each_bisect_ref()` is called by `for_each_bad_bisect_ref()` with\n> a term \"bad\". This used to make it call `for_each_ref_in_submodule()`\n> with a prefix \"refs/bisect/bad\". But the latter is the name of the\n> reference that is being sought, so the empty string was being passed\n> to the callback as the trimmed refname. Moreover, this questionable\n> practice was turned into an error by\n> \n>     b9c8e7f2fb prefix_ref_iterator: don't trim too much, 2017-05-22\n> \n> It 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.\n\nYou might want to mention that this is really just a hole in the\nexisting API. We have for_each_ref_in_submodule() and\nfor_each_fullref_in(), but not the missing link.\n\nI don't think that makes it any more or less correct, but I thought at\nfirst you had to invent a new function totally.\n\n> * Change `for_each_bad_bisect_ref()` to call the new function rather\n>   than `for_each_ref_in_submodule()`.\n> \n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  refs.c     | 12 ++++++++++++\n>  refs.h     |  5 ++++-\n>  revision.c |  2 +-\n>  3 files changed, 17 insertions(+), 2 deletions(-)\n\nThe change itself looks fine to me.\n\nSince we obviously don't have even a single test for \"--bisect\", that\nmight be worth adding.\n\n-Peff\n"},{"id":"322211","messageId":"20170614093242.twipnjncaka2lhyg@sigill.intra.peff.net","threadId":"46171","inReplyTo":"20170614092256.c3fmfcokuwbbcvbz@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] for_each_bisect_ref(): don't trim refnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-14T09:32:42Z","receivedAt":"2017-06-14T09:33:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 14, 2017 at 05:22:56AM -0400, Jeff King wrote:\n\n> >  refs.c     | 12 ++++++++++++\n> >  refs.h     |  5 ++++-\n> >  revision.c |  2 +-\n> >  3 files changed, 17 insertions(+), 2 deletions(-)\n> \n> The change itself looks fine to me.\n> \n> Since we obviously don't have even a single test for \"--bisect\", that\n> might be worth adding.\n\nIt turns out we do, but none that actually check that we use the default\nrefnames. So maybe squash this in?\n\ndiff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh\nindex 3bf2759ea..534903bbd 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"},{"id":"322219","messageId":"xmqqfuf251gi.fsf@gitster.mtv.corp.google.com","threadId":"46171","inReplyTo":"20170614093242.twipnjncaka2lhyg@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] for_each_bisect_ref(): don't trim refnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-14T10:05:17Z","receivedAt":"2017-06-14T10:05:24Z","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>> Since we obviously don't have even a single test for \"--bisect\", that\n>> might be worth adding.\n>\n> It turns out we do, but none that actually check that we use the default\n> refnames. So maybe squash this in?\n\nSounds sensible.  Thanks.\n\n>\n> diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh\n> index 3bf2759ea..534903bbd 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"},{"id":"322220","messageId":"xmqqbmpq519f.fsf@gitster.mtv.corp.google.com","threadId":"46171","inReplyTo":"cover.1497430232.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 0/2] Fix a refname trimming problem in `log --bisect`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-14T10:09:32Z","receivedAt":"2017-06-14T10:09:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> The code for `git log --bisect` was calling\n> `for_each_ref_in_submodule()` with prefix set to \"refs/bisect/bad\",\n> which is the actual name of the reference that it wants. This resulted\n> in the refname being trimmed completely away and the empty string\n> being passed to the callback. That became impermissible after\n>\n>     b9c8e7f2fb prefix_ref_iterator: don't trim too much, 2017-05-22\n>\n> , so the command was failing.\n>\n> Fix the problem in two orthogonal ways:\n>\n> 1. Add a new function, `for_each_fullref_in_submodule()`, that doesn't\n>    trim the refnames that it passes to callbacks, and us that instead.\n>    I *think* that this is a strict improvement, though I don't know\n>    the `git log` code well enough to be sure that it won't have bad\n>    side-effects.\n>\n> 2. Relax the \"trimming too many characters\" check to allow the full\n>    length of the refname to be trimmed away (though not more than\n>    that).\n>\n> In an ideal world the second patch shouldn't be necessary, because\n> this calling pattern is questionable and it might be better that we\n> learn about any other offenders. But if we'd rather be conservative\n> and not break any other code that might rely on the old behavior,\n> patch 2 is my suggestion for how to do it.\n\nThanks for a nice summary.  \n\nI agree that 2. is a nice safety to have, especially if the code\nbefore b9c8e7f2 (\"prefix_ref_iterator: don't trim too much\",\n2017-05-22) has been seeing the completely trimmed result (i.e. an\nempty string) in the callback function.\n\nAnd I agree that 1. is also a good interface to have.\n"},{"id":"322409","messageId":"xmqqwp8cyjiz.fsf@gitster.mtv.corp.google.com","threadId":"46171","inReplyTo":"3615deefe90bebe746618b04c055a466a442f85b.1497430232.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 1/2] for_each_bisect_ref(): don't trim refnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-15T22:26:44Z","receivedAt":"2017-06-15T22:26:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> `for_each_bisect_ref()` is called by `for_each_bad_bisect_ref()` with\n> a term \"bad\". This used to make it call `for_each_ref_in_submodule()`\n> with a prefix \"refs/bisect/bad\". But the latter is the name of the\n> reference that is being sought, so the empty string was being passed\n> to the callback as the trimmed refname. Moreover, this questionable\n> practice was turned into an error by\n>\n>     b9c8e7f2fb prefix_ref_iterator: don't trim too much, 2017-05-22\n>\n> It 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.\n>\n> * Change `for_each_bad_bisect_ref()` to call the new function rather\n>   than `for_each_ref_in_submodule()`.\n\nThis unfortunately makes nd/prune-in-worktree topic rather\nobsolete.  Can somebody volunteer to update it to newer codebase\nincluding this fix?\n\nThanks.\n"}]}