{"thread":{"id":"47136","subject":"[RFC PATCH] rebisect: add script for easier bisect log editing","startedAt":"2017-11-08T14:00:40Z","lastAt":"2017-11-22T05:35:51Z","messageCount":9,"participants":["Adam Dinwoodie","Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"332074","messageId":"20171108135931.166880-1-adam@dinwoodie.org","threadId":"47136","inReplyTo":null,"subject":"[RFC PATCH] rebisect: add script for easier bisect log editing","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2017-11-08T13:59:31Z","receivedAt":"2017-11-08T14:00:40Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"Add a short script, vaguely inspired by `git rebase --interactive`, to\nease the process described in the `git bisect` documentation of saving\noff a bisect log, editing it, then replaying it.\n\nSigned-off-by: Adam Dinwoodie <adam@dinwoodie.org>\n---\n\nWhen I'm bisecting, I find I need to semi-regularly go back and change\nmy good/bad/skip response for some commits.  The bisect documentation\ndescribes doing this by saving `git bisect log` output, editing it, then\nusing `git bisect replay`.  Which is a perfectly fine technique, but\nautomation is A Good Thing(TM).  The below script is a short proof of\nconcept for changing this process to be a single command.\n\nIdeally (at least from my perspective), this function would be rolled\ninto the main `git bisect` tool, as `git bisect edit` or similar.\nBefore I start working on that, however, I wanted to see what the list\nthought of the idea.\n\n contrib/git-rebisect.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n create mode 100755 contrib/git-rebisect.sh\n\ndiff --git a/contrib/git-rebisect.sh b/contrib/git-rebisect.sh\nnew file mode 100755\nindex 000000000..60f20b278\n--- /dev/null\n+++ b/contrib/git-rebisect.sh\n@@ -0,0 +1,12 @@\n+#!/bin/sh\n+\n+GIT_EDITOR=\"$(git var GIT_EDITOR)\"\n+GIT_DIR=\"$(git rev-parse --git-dir)\"\n+GIT_BISECT_LOG_TMP=\"${GIT_DIR}/BISECT_LOG_EDIT\"\n+\n+git bisect log >\"$GIT_BISECT_LOG_TMP\"\n+\"$GIT_EDITOR\" \"$GIT_BISECT_LOG_TMP\"\n+git bisect reset HEAD\n+git bisect start\n+git bisect replay \"$GIT_BISECT_LOG_TMP\"\n+rm -f \"$GIT_BISECT_LOG_TMP\"\n-- \n2.14.3\n\n"},{"id":"332080","messageId":"CAP8UFD015i76L4BgSZdr2k2TZk+C0vRAqOsj4DaqtNYuJjtNxQ@mail.gmail.com","threadId":"47136","inReplyTo":"20171108135931.166880-1-adam@dinwoodie.org","subject":"Re: [RFC PATCH] rebisect: add script for easier bisect log editing","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-11-08T16:12:18Z","receivedAt":"2017-11-08T16:12:24Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Nov 8, 2017 at 2:59 PM, Adam Dinwoodie <adam@dinwoodie.org> wrote:\n> Add a short script, vaguely inspired by `git rebase --interactive`, to\n> ease the process described in the `git bisect` documentation of saving\n> off a bisect log, editing it, then replaying it.\n\nNice idea.\n\n> Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>\n> ---\n>\n> When I'm bisecting, I find I need to semi-regularly go back and change\n> my good/bad/skip response for some commits.  The bisect documentation\n> describes doing this by saving `git bisect log` output, editing it, then\n> using `git bisect replay`.  Which is a perfectly fine technique, but\n> automation is A Good Thing(TM).  The below script is a short proof of\n> concept for changing this process to be a single command.\n>\n> Ideally (at least from my perspective), this function would be rolled\n> into the main `git bisect` tool, as `git bisect edit` or similar.\n\nI agree and I don't think it would be very difficult to convert to\nsuch a sub command.\n\n> Before I start working on that, however, I wanted to see what the list\n> thought of the idea.\n>\n>  contrib/git-rebisect.sh | 12 ++++++++++++\n>  1 file changed, 12 insertions(+)\n>  create mode 100755 contrib/git-rebisect.sh\n>\n> diff --git a/contrib/git-rebisect.sh b/contrib/git-rebisect.sh\n> new file mode 100755\n> index 000000000..60f20b278\n> --- /dev/null\n> +++ b/contrib/git-rebisect.sh\n> @@ -0,0 +1,12 @@\n> +#!/bin/sh\n> +\n> +GIT_EDITOR=\"$(git var GIT_EDITOR)\"\n> +GIT_DIR=\"$(git rev-parse --git-dir)\"\n> +GIT_BISECT_LOG_TMP=\"${GIT_DIR}/BISECT_LOG_EDIT\"\n> +\n> +git bisect log >\"$GIT_BISECT_LOG_TMP\"\n> +\"$GIT_EDITOR\" \"$GIT_BISECT_LOG_TMP\"\n> +git bisect reset HEAD\n\nI guess that using \"reset HEAD\" could be cheaper than just \"reset\" and\nthat's the reason you are using it.\n\n> +git bisect start\n\nAre you sure that this \"start\" is necessary? The doc says that \"reset\"\nfollowed by \"replay that-file\" should be enough.\n\n> +git bisect replay \"$GIT_BISECT_LOG_TMP\"\n> +rm -f \"$GIT_BISECT_LOG_TMP\"\n\nThanks,\nChristian.\n"},{"id":"332081","messageId":"CAP8UFD35yFTB5_D6=WyXN47Lgo3PvLJi3yWfzTAK5aYjE9YjNg@mail.gmail.com","threadId":"47136","inReplyTo":"CAP8UFD015i76L4BgSZdr2k2TZk+C0vRAqOsj4DaqtNYuJjtNxQ@mail.gmail.com","subject":"Re: [RFC PATCH] rebisect: add script for easier bisect log editing","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-11-08T16:15:05Z","receivedAt":"2017-11-08T16:15:10Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":">> +git bisect replay \"$GIT_BISECT_LOG_TMP\"\n>> +rm -f \"$GIT_BISECT_LOG_TMP\"\n\nWhile at it, is there a reason for the -f option above?\n"},{"id":"332082","messageId":"20171108163250.GE20681@dinwoodie.org","threadId":"47136","inReplyTo":"CAP8UFD015i76L4BgSZdr2k2TZk+C0vRAqOsj4DaqtNYuJjtNxQ@mail.gmail.com","subject":"Re: [RFC PATCH] rebisect: add script for easier bisect log editing","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2017-11-08T16:32:50Z","receivedAt":"2017-11-08T16:32:59Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Wednesday 08 November 2017 at 05:12 pm +0100, Christian Couder wrote:\n> On Wed, Nov 8, 2017 at 2:59 PM, Adam Dinwoodie <adam@dinwoodie.org> wrote:\n> > +git bisect reset HEAD\n> \n> I guess that using \"reset HEAD\" could be cheaper than just \"reset\" and\n> that's the reason you are using it.\n\nExactly that, yes.  I often use `reset HEAD` in my own workflows in the\nname of speed, and I can't see any disadvantages of doing it here, too.\n\n> > +git bisect start\n> \n> Are you sure that this \"start\" is necessary? The doc says that \"reset\"\n> followed by \"replay that-file\" should be enough.\n\nIt isn't necessary, in that the process works if you skip that command.\nHowever, without it, the `git bisect replay` command prints \"We are not\nbisecting\" before it does anything else, so having the `bisect start`\nthere explicitly removes that extraneous output.\n\nIf the script were integrated into git-bisect itself, it would probably\nmake sense to change that behaviour so the warning isn't printed.  (It\nquite possibly makes sense to remove the warning when running `bisect\nreplay` regardless.)  But when writing the stand-alone script I wanted\nthings to work without any changes to the core Git code.\n"},{"id":"332083","messageId":"20171108165033.GF20681@dinwoodie.org","threadId":"47136","inReplyTo":"CAP8UFD35yFTB5_D6=WyXN47Lgo3PvLJi3yWfzTAK5aYjE9YjNg@mail.gmail.com","subject":"Re: [RFC PATCH] rebisect: add script for easier bisect log editing","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2017-11-08T16:50:33Z","receivedAt":"2017-11-08T16:50:41Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Wednesday 08 November 2017 at 05:15 pm +0100, Christian Couder wrote:\n> >> +git bisect replay \"$GIT_BISECT_LOG_TMP\"\n> >> +rm -f \"$GIT_BISECT_LOG_TMP\"\n> \n> While at it, is there a reason for the -f option above?\n\nI was following the lead of git-bisect.sh, which has used `rm -f` for\nsuch things ever since it was first introduced[^1], although it appears\nthat, since v2.15.0, all the `rm`s in that script have been moved to the\nC code[^2].\n\nActually applying thought, rather than just following existing\nprecedent, I suspect having `-f` is useful because it means the command\nwill work even if the shell has picked up that `rm` should otherwise\nhave a `-i` argument from somewhere.\n\n[^1]: 8cc6a0831 (\"[PATCH] Making it easier to find which change introduced a bug\", 2005-07-30)\n[^2]: fb71a3299 (\"bisect--helper: `bisect_clean_state` shell function in C\", 2017-09-29)\n"},{"id":"332925","messageId":"cover.1511200589.git.adam@dinwoodie.org","threadId":"47136","inReplyTo":"20171108135931.166880-1-adam@dinwoodie.org","subject":"[RFC PATCH v2 0/2] bisect: add a single command for editing logs","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2017-11-20T18:24:37Z","receivedAt":"2017-11-20T18:24:52Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"When I'm bisecting, I sometimes want to edit the bisection log, e.g. to\nremove the \"skip\" marker by a commit I've now found a way to avoid\nskipping.  Rather than requiring users to save off the log, edit it,\nthen replay the edited log as separate commands, this patch series adds\nsupport for a \"git bisect edit\" command which does all three steps in\none.\n\nChristian Couder has already said he's happy with the broad idea in the\nprevious spin of this RFC, so here's a first attempt at actually\nimplementing the function within \"git bisect\".\n\nThere are a few issues of varying significance before I think this is\nready to be actually used.  I'm not sure how to approach them, and would\nbe very grateful for advice from the list:\n\n- It's possible to start a bisect session with a command like `git\n  bisect @ @~10`.  This will lead to the bisect log including the `@`\n  and `@~10` literally, and the interpretation of those values changes\n  depending on the current HEAD.  As a result, if you do a `git bisect\n  edit` after starting a bisect like that, but don't actually edit the\n  file, you'll nonetheless be in a different state.\n\n  I can see a few ways of coping with that:\n\n  1. Change the existing `git bisect start` behaviour to run arguments\n     through `git rev-parse` before recording them.  It appears `git\n     bisect good` et al. already do that, but it is a change in\n     behaviour that I guess could impact badly on other people using\n     `git bisect log`-based workflows.\n\n  2. Do a full `git bisect reset` before replaying the log, so the\n     revisions will be parsed in the same way as they were originally.\n     I'd be slightly sad about that, as it seems an unnecessary\n     inefficiency, but it may well be the simplest approach.\n\n  3. Somehow get Git to parse the relative references as relative to the\n     original commit rather than the current HEAD.  I'm not sure if\n     there's code for doing this already, but if not I suspect it's\n     beyond my ability to implement in the immediate term.\n\n  4. Just detect when users are in this scenario, and warn them that\n     Git's behaviour might be unexpected.\n\n- I can see `git rebase --interactive` detects when the edited file\n  hasn't changed, and in that case prints a success message but\n  otherwise takes no action.  I've not implemented that behaviour here\n  because I couldn't immediately work out how rebase does it, and I\n  didn't want to reinvent that particular wheel.  (Plus I think the\n  impact of performing such unnecessary steps will be considerably lower\n  than the equivalent with rebase.)\n\n- I'm not entirely happy with the error handling, primarily as I\n  couldn't seem to find a consensus on what best practice is for\n  handling errors between the existing shell code in this script and\n  git-rebase--interactive.sh.\n\n- There aren't yet any tests or documentation changes; I wanted to get\n  commentary on the initial code changes before I spent time on those\n  parts.\n\nAdam Dinwoodie (2):\n  bisect: split out replay file parsing\n  bisect: add \"edit\" command\n\n builtin/bisect--helper.c |  3 ++-\n git-bisect.sh            | 25 +++++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 1 deletion(-)\n\n-- \n2.15.0.281.g87c0a7615\n\n"},{"id":"332926","messageId":"e3bfd0ab1f7b17eb43cf50c91e17f498db56cdee.1511200589.git.adam@dinwoodie.org","threadId":"47136","inReplyTo":"cover.1511200589.git.adam@dinwoodie.org","subject":"[RFC PATCH v2 1/2] bisect: split out replay file parsing","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2017-11-20T18:24:38Z","receivedAt":"2017-11-20T18:24:56Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"In order to allow a git bisect log file to be replayed without using all\nthe surrounding code to do things like clean the repository state, split\nout the file-parsing part of bisect_replay into a separate function.\n\nSigned-off-by: Adam Dinwoodie <adam@dinwoodie.org>\n---\n git-bisect.sh | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 54cbfecc5..895d7976a 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -422,6 +422,14 @@ bisect_replay () {\n \ttest \"$#\" -eq 1 || die \"$(gettext \"No logfile given\")\"\n \ttest -r \"$file\" || die \"$(eval_gettext \"cannot read \\$file for replaying\")\"\n \tbisect_reset\n+\tbisect_replay_file \"$file\"\n+\tbisect_auto_next\n+}\n+\n+bisect_replay_file() {\n+\tfile=\"$1\"\n+\ttest \"$#\" -eq 1 || die \"$(gettext \"No logfile given\")\"\n+\ttest -r \"$file\" || die \"$(eval_gettext \"cannot read \\$file for replaying\")\"\n \twhile read git bisect command rev\n \tdo\n \t\ttest \"$git $bisect\" = \"git bisect\" || test \"$git\" = \"git-bisect\" || continue\n@@ -444,7 +452,6 @@ bisect_replay () {\n \t\t\tdie \"$(gettext \"?? what are you talking about?\")\" ;;\n \t\tesac\n \tdone <\"$file\"\n-\tbisect_auto_next\n }\n \n bisect_run () {\n-- \n2.15.0.281.g87c0a7615\n\n"},{"id":"332927","messageId":"6cef29705cb22fa23847e255acd7e7623dc9d805.1511200589.git.adam@dinwoodie.org","threadId":"47136","inReplyTo":"cover.1511200589.git.adam@dinwoodie.org","subject":"[RFC PATCH v2 2/2] bisect: add \"edit\" command","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2017-11-20T18:24:39Z","receivedAt":"2017-11-20T18:24:58Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"Add an \"edit\" command to git bisect, which will save the current\nbisection log to a file, open an editor to allow the user to replay the\nbisection log, then replay the edited log file.\n\nThis can already be done as separate steps, and doing so is described in\nthe bisect documentation; this commit merely reduces those separate\nsteps to a single step.\n\nSigned-off-by: Adam Dinwoodie <adam@dinwoodie.org>\n---\n builtin/bisect--helper.c |  3 ++-\n git-bisect.sh            | 18 ++++++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 4b5fadcbe..980e3e09b 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -46,7 +46,8 @@ static int check_term_format(const char *term, const char *orig_term)\n \t\treturn error(_(\"'%s' is not a valid term\"), term);\n \n \tif (one_of(term, \"help\", \"start\", \"skip\", \"next\", \"reset\",\n-\t\t\t\"visualize\", \"view\", \"replay\", \"log\", \"run\", \"terms\", NULL))\n+\t\t\t\"visualize\", \"view\", \"replay\", \"log\", \"edit\", \"run\",\n+\t\t\t\"terms\", NULL))\n \t\treturn error(_(\"can't use the builtin command '%s' as a term\"), term);\n \n \t/*\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 895d7976a..bcc02a3f2 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -26,6 +26,8 @@ git bisect replay <logfile>\n \treplay bisection log.\n git bisect log\n \tshow bisect log.\n+git bisect edit\n+\tedit and replay bisect log.\n git bisect run <cmd>...\n \tuse <cmd>... to automatically bisect.\n \n@@ -454,6 +456,20 @@ bisect_replay_file() {\n \tdone <\"$file\"\n }\n \n+bisect_edit () {\n+\ttest -s \"$GIT_DIR/BISECT_LOG\" || die \"$(gettext \"We are not bisecting.\")\"\n+\tcp \"$GIT_DIR/BISECT_LOG\" \"$GIT_DIR/BISECT_LOG_EDIT\"\n+\tgit_editor \"$GIT_DIR/BISECT_LOG_EDIT\" ||\n+\t\tdie \"$(gettext \"Could not execute editor\")\"\n+\ttest -n \"$(git stripspace --strip-comments <\"$GIT_DIR/BISECT_LOG_EDIT\")\" ||\n+\t\tdie \"$(gettext \"Nothing to do\")\"\n+\tgit bisect--helper --bisect-clean-state ||\n+\t\tdie \"$(gettext \"Unable to clean repository\")\"\n+\tbisect_replay_file \"$GIT_DIR/BISECT_LOG_EDIT\"\n+\trm -f \"$GIT_DIR/BISECT_LOG_EDIT\"\n+\tbisect_auto_next\n+}\n+\n bisect_run () {\n \tbisect_next_check fail\n \n@@ -625,6 +641,8 @@ case \"$#\" in\n \t\tbisect_replay \"$@\" ;;\n \tlog)\n \t\tbisect_log ;;\n+\tedit)\n+\t\tbisect_edit ;;\n \trun)\n \t\tbisect_run \"$@\" ;;\n \tterms)\n-- \n2.15.0.281.g87c0a7615\n\n"},{"id":"333247","messageId":"xmqqshd6ub8f.fsf@gitster.mtv.corp.google.com","threadId":"47136","inReplyTo":"cover.1511200589.git.adam@dinwoodie.org","subject":"Re: [RFC PATCH v2 0/2] bisect: add a single command for editing logs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T05:35:28Z","receivedAt":"2017-11-22T05:35:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Dinwoodie <adam@dinwoodie.org> writes:\n\n> - It's possible to start a bisect session with a command like `git\n>   bisect @ @~10`.  This will lead to the bisect log including the `@`\n>   and `@~10` literally, and the interpretation of those values changes\n>   depending on the current HEAD.  As a result, if you do a `git bisect\n>   edit` after starting a bisect like that, but don't actually edit the\n>   file, you'll nonetheless be in a different state.\n\nThis is a tangent, but for writing to the general public, please do\nspell out HEAD, not the line noise synonym \"@\" that confuses readers.\n\n>   I can see a few ways of coping with that:\n>\n>   1. Change the existing `git bisect start` behaviour to run arguments\n>      through `git rev-parse` before recording them.  It appears `git\n>      bisect good` et al. already do that, but it is a change in\n>      behaviour that I guess could impact badly on other people using\n>      `git bisect log`-based workflows.\n\nThe issue is not just HEAD but also for anything fruid, i.e. the\nname of a branch, a search result \":/pattern\", etc., and if we want\nto allow restarting a previously failed bisect session from a\nmidpoint, we should be recording things in absolute terms as early\nas possible.  I'd think it was an oversight the \"log\" thing did not\ndo so.\n\n>   2. Do a full `git bisect reset` before replaying the log, so the\n>      revisions will be parsed in the same way as they were originally.\n>      I'd be slightly sad about that, as it seems an unnecessary\n>      inefficiency, but it may well be the simplest approach.\n\nIt is not just inefficient, but would require there is no a local\nchange; I thought that the current system allows you to have a local\nmodification to a path that is not involved in the bisect session\nand losing that property would be sad.\n\n> - There aren't yet any tests or documentation changes; I wanted to get\n>   commentary on the initial code changes before I spent time on those\n>   parts.\n\nThere are some chicken-and-egg around this area.  For some changes,\nwithout a doc update and test addition, it is harder to judge if a\nreviewer can agree with the proposed change, as there is only a high\nlevel description \"we allow editing\" and the lowest level changes to\nthe actual code, without anything in between that describes the\nguiding principle and design decision that lead to the patch.\n\nI'll need to see if the changes in the patch is clear/trivial enough\nto see where you are trying to go to see if it is the case for this\npatch, though, so read the above paragraph as a general guideline.\n\n\n"}]}