threads / rfc / 47136

RFC patchrebisect: add script for easier bisect log editing

Subject: [RFC PATCH] rebisect: add script for easier bisect log editing

## tl;dr

9 messages between Nov 8, 2017 and Nov 22, 2017. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Adam Dinwoodie· Nov 8, 2017, 13:59 UTC · lore

Add a short script, vaguely inspired by `git rebase --interactive`, to ease the process described in the `git bisect` documentation of saving off a bisect log, editing it, then replaying it.

Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
---

When I'm bisecting, I find I need to semi-regularly go back and change my good/bad/skip response for some commits. The bisect documentation describes doing this by saving `git bisect log` output, editing it, then using `git bisect replay`. Which is a perfectly fine technique, but automation is A Good Thing(TM). The below script is a short proof of concept for changing this process to be a single command.

Ideally (at least from my perspective), this function would be rolled into the main `git bisect` tool, as `git bisect edit` or similar. Before I start working on that, however, I wanted to see what the list thought of the idea.

 contrib/git-rebisect.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)
 create mode 100755 contrib/git-rebisect.sh
Show changes to contrib/git-rebisect.sh +12 −0
diff --git a/contrib/git-rebisect.sh b/contrib/git-rebisect.sh
new file mode 100755
index 000000000..60f20b278
--- /dev/null
+++ b/contrib/git-rebisect.sh
@@ -0,0 +1,12 @@
+#!/bin/sh
+
+GIT_EDITOR="$(git var GIT_EDITOR)"
+GIT_DIR="$(git rev-parse --git-dir)"
+GIT_BISECT_LOG_TMP="${GIT_DIR}/BISECT_LOG_EDIT"
+
+git bisect log >"$GIT_BISECT_LOG_TMP"
+"$GIT_EDITOR" "$GIT_BISECT_LOG_TMP"
+git bisect reset HEAD
+git bisect start
+git bisect replay "$GIT_BISECT_LOG_TMP"
+rm -f "$GIT_BISECT_LOG_TMP"

-- 
2.14.3
Christian Couder· Nov 8, 2017, 16:12 UTC · re: Adam Dinwoodie · lore

Re: [RFC PATCH] rebisect: add script for easier bisect log editing

On Wed, Nov 8, 2017 at 2:59 PM, Adam Dinwoodie <adam@dinwoodie.org> wrote:
> Add a short script, vaguely inspired by `git rebase --interactive`, to
> ease the process described in the `git bisect` documentation of saving
> off a bisect log, editing it, then replaying it.
Nice idea.
Show 12 quoted lines
> Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
> ---
>
> When I'm bisecting, I find I need to semi-regularly go back and change
> my good/bad/skip response for some commits.  The bisect documentation
> describes doing this by saving `git bisect log` output, editing it, then
> using `git bisect replay`.  Which is a perfectly fine technique, but
> automation is A Good Thing(TM).  The below script is a short proof of
> concept for changing this process to be a single command.
>
> Ideally (at least from my perspective), this function would be rolled
> into the main `git bisect` tool, as `git bisect edit` or similar.

I agree and I don't think it would be very difficult to convert to such a sub command.

Show 22 quoted lines
> Before I start working on that, however, I wanted to see what the list
> thought of the idea.
>
>  contrib/git-rebisect.sh | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>  create mode 100755 contrib/git-rebisect.sh
>
> diff --git a/contrib/git-rebisect.sh b/contrib/git-rebisect.sh
> new file mode 100755
> index 000000000..60f20b278
> --- /dev/null
> +++ b/contrib/git-rebisect.sh
> @@ -0,0 +1,12 @@
> +#!/bin/sh
> +
> +GIT_EDITOR="$(git var GIT_EDITOR)"
> +GIT_DIR="$(git rev-parse --git-dir)"
> +GIT_BISECT_LOG_TMP="${GIT_DIR}/BISECT_LOG_EDIT"
> +
> +git bisect log >"$GIT_BISECT_LOG_TMP"
> +"$GIT_EDITOR" "$GIT_BISECT_LOG_TMP"
> +git bisect reset HEAD

I guess that using "reset HEAD" could be cheaper than just "reset" and that's the reason you are using it.

> +git bisect start

Are you sure that this "start" is necessary? The doc says that "reset" followed by "replay that-file" should be enough.

> +git bisect replay "$GIT_BISECT_LOG_TMP"
> +rm -f "$GIT_BISECT_LOG_TMP"

Thanks, Christian.

Adam Dinwoodie· Nov 8, 2017, 16:50 UTC · re: Christian Couder · lore

Re: [RFC PATCH] rebisect: add script for easier bisect log editing

On Wednesday 08 November 2017 at 05:15 pm +0100, Christian Couder wrote:
> >> +git bisect replay "$GIT_BISECT_LOG_TMP"
> >> +rm -f "$GIT_BISECT_LOG_TMP"
> 
> While at it, is there a reason for the -f option above?

I was following the lead of git-bisect.sh, which has used `rm -f` for such things ever since it was first introduced[^1], although it appears that, since v2.15.0, all the `rm`s in that script have been moved to the C code[^2].

Actually applying thought, rather than just following existing precedent, I suspect having `-f` is useful because it means the command will work even if the shell has picked up that `rm` should otherwise have a `-i` argument from somewhere.

[^1]: 8cc6a0831 ("[PATCH] Making it easier to find which change introduced a bug", 2005-07-30) [^2]: fb71a3299 ("bisect--helper: `bisect_clean_state` shell function in C", 2017-09-29)

Adam Dinwoodie· Nov 8, 2017, 16:32 UTC · re: Christian Couder · lore

Re: [RFC PATCH] rebisect: add script for easier bisect log editing

On Wednesday 08 November 2017 at 05:12 pm +0100, Christian Couder wrote:
Show 5 quoted lines
> On Wed, Nov 8, 2017 at 2:59 PM, Adam Dinwoodie <adam@dinwoodie.org> wrote:
> > +git bisect reset HEAD
> 
> I guess that using "reset HEAD" could be cheaper than just "reset" and
> that's the reason you are using it.

Exactly that, yes. I often use `reset HEAD` in my own workflows in the name of speed, and I can't see any disadvantages of doing it here, too.

> > +git bisect start
> 
> Are you sure that this "start" is necessary? The doc says that "reset"
> followed by "replay that-file" should be enough.

It isn't necessary, in that the process works if you skip that command. However, without it, the `git bisect replay` command prints "We are not bisecting" before it does anything else, so having the `bisect start` there explicitly removes that extraneous output.

If the script were integrated into git-bisect itself, it would probably make sense to change that behaviour so the warning isn't printed. (It quite possibly makes sense to remove the warning when running `bisect replay` regardless.) But when writing the stand-alone script I wanted things to work without any changes to the core Git code.

Adam Dinwoodie· Nov 20, 2017, 18:24 UTC · re: Adam Dinwoodie · lore

[RFC PATCH v2 0/2] bisect: add a single command for editing logs

When I'm bisecting, I sometimes want to edit the bisection log, e.g. to remove the "skip" marker by a commit I've now found a way to avoid skipping. Rather than requiring users to save off the log, edit it, then replay the edited log as separate commands, this patch series adds support for a "git bisect edit" command which does all three steps in one.

Christian Couder has already said he's happy with the broad idea in the previous spin of this RFC, so here's a first attempt at actually implementing the function within "git bisect".

There are a few issues of varying significance before I think this is ready to be actually used. I'm not sure how to approach them, and would be very grateful for advice from the list:

- It's possible to start a bisect session with a command like `git
  bisect @ @~10`.  This will lead to the bisect log including the `@`
  and `@~10` literally, and the interpretation of those values changes
  depending on the current HEAD.  As a result, if you do a `git bisect
  edit` after starting a bisect like that, but don't actually edit the
  file, you'll nonetheless be in a different state.
  I can see a few ways of coping with that:
  1. Change the existing `git bisect start` behaviour to run arguments
     through `git rev-parse` before recording them.  It appears `git
     bisect good` et al. already do that, but it is a change in
     behaviour that I guess could impact badly on other people using
     `git bisect log`-based workflows.
  2. Do a full `git bisect reset` before replaying the log, so the
     revisions will be parsed in the same way as they were originally.
     I'd be slightly sad about that, as it seems an unnecessary
     inefficiency, but it may well be the simplest approach.
  3. Somehow get Git to parse the relative references as relative to the
     original commit rather than the current HEAD.  I'm not sure if
     there's code for doing this already, but if not I suspect it's
     beyond my ability to implement in the immediate term.
  4. Just detect when users are in this scenario, and warn them that
     Git's behaviour might be unexpected.
- I can see `git rebase --interactive` detects when the edited file
  hasn't changed, and in that case prints a success message but
  otherwise takes no action.  I've not implemented that behaviour here
  because I couldn't immediately work out how rebase does it, and I
  didn't want to reinvent that particular wheel.  (Plus I think the
  impact of performing such unnecessary steps will be considerably lower
  than the equivalent with rebase.)
- I'm not entirely happy with the error handling, primarily as I
  couldn't seem to find a consensus on what best practice is for
  handling errors between the existing shell code in this script and
  git-rebase--interactive.sh.
- There aren't yet any tests or documentation changes; I wanted to get
  commentary on the initial code changes before I spent time on those
  parts.
Adam Dinwoodie (2):
  bisect: split out replay file parsing
  bisect: add "edit" command
 builtin/bisect--helper.c |  3 ++-
 git-bisect.sh            | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)
-- 
2.15.0.281.g87c0a7615
Adam Dinwoodie· Nov 20, 2017, 18:24 UTC · re: Adam Dinwoodie · lore

[RFC PATCH v2 1/2] bisect: split out replay file parsing

In order to allow a git bisect log file to be replayed without using all the surrounding code to do things like clean the repository state, split out the file-parsing part of bisect_replay into a separate function.

Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
---
 git-bisect.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
Show changes to git-bisect.sh +8 −1
diff --git a/git-bisect.sh b/git-bisect.sh
index 54cbfecc5..895d7976a 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -422,6 +422,14 @@ bisect_replay () {
 	test "$#" -eq 1 || die "$(gettext "No logfile given")"
 	test -r "$file" || die "$(eval_gettext "cannot read \$file for replaying")"
 	bisect_reset
+	bisect_replay_file "$file"
+	bisect_auto_next
+}
+
+bisect_replay_file() {
+	file="$1"
+	test "$#" -eq 1 || die "$(gettext "No logfile given")"
+	test -r "$file" || die "$(eval_gettext "cannot read \$file for replaying")"
 	while read git bisect command rev
 	do
 		test "$git $bisect" = "git bisect" || test "$git" = "git-bisect" || continue
@@ -444,7 +452,6 @@ bisect_replay () {
 			die "$(gettext "?? what are you talking about?")" ;;
 		esac
 	done <"$file"
-	bisect_auto_next
 }
 
 bisect_run () {

-- 
2.15.0.281.g87c0a7615
Adam Dinwoodie· Nov 20, 2017, 18:24 UTC · re: Adam Dinwoodie · lore

[RFC PATCH v2 2/2] bisect: add "edit" command

Add an "edit" command to git bisect, which will save the current bisection log to a file, open an editor to allow the user to replay the bisection log, then replay the edited log file.

This can already be done as separate steps, and doing so is described in the bisect documentation; this commit merely reduces those separate steps to a single step.

Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
---
 builtin/bisect--helper.c |  3 ++-
 git-bisect.sh            | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)
Show changes to 2 files +20 −1

builtin/bisect--helper.c, git-bisect.sh

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 4b5fadcbe..980e3e09b 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -46,7 +46,8 @@ static int check_term_format(const char *term, const char *orig_term)
 		return error(_("'%s' is not a valid term"), term);
 
 	if (one_of(term, "help", "start", "skip", "next", "reset",
-			"visualize", "view", "replay", "log", "run", "terms", NULL))
+			"visualize", "view", "replay", "log", "edit", "run",
+			"terms", NULL))
 		return error(_("can't use the builtin command '%s' as a term"), term);
 
 	/*
diff --git a/git-bisect.sh b/git-bisect.sh
index 895d7976a..bcc02a3f2 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -26,6 +26,8 @@ git bisect replay <logfile>
 	replay bisection log.
 git bisect log
 	show bisect log.
+git bisect edit
+	edit and replay bisect log.
 git bisect run <cmd>...
 	use <cmd>... to automatically bisect.
 
@@ -454,6 +456,20 @@ bisect_replay_file() {
 	done <"$file"
 }
 
+bisect_edit () {
+	test -s "$GIT_DIR/BISECT_LOG" || die "$(gettext "We are not bisecting.")"
+	cp "$GIT_DIR/BISECT_LOG" "$GIT_DIR/BISECT_LOG_EDIT"
+	git_editor "$GIT_DIR/BISECT_LOG_EDIT" ||
+		die "$(gettext "Could not execute editor")"
+	test -n "$(git stripspace --strip-comments <"$GIT_DIR/BISECT_LOG_EDIT")" ||
+		die "$(gettext "Nothing to do")"
+	git bisect--helper --bisect-clean-state ||
+		die "$(gettext "Unable to clean repository")"
+	bisect_replay_file "$GIT_DIR/BISECT_LOG_EDIT"
+	rm -f "$GIT_DIR/BISECT_LOG_EDIT"
+	bisect_auto_next
+}
+
 bisect_run () {
 	bisect_next_check fail
 
@@ -625,6 +641,8 @@ case "$#" in
 		bisect_replay "$@" ;;
 	log)
 		bisect_log ;;
+	edit)
+		bisect_edit ;;
 	run)
 		bisect_run "$@" ;;
 	terms)

-- 
2.15.0.281.g87c0a7615
Junio C Hamano· Nov 22, 2017, 05:35 UTC · re: Adam Dinwoodie · lore

Re: [RFC PATCH v2 0/2] bisect: add a single command for editing logs

Adam Dinwoodie <adam@dinwoodie.org> writes:
Show 6 quoted lines
> - It's possible to start a bisect session with a command like `git
>   bisect @ @~10`.  This will lead to the bisect log including the `@`
>   and `@~10` literally, and the interpretation of those values changes
>   depending on the current HEAD.  As a result, if you do a `git bisect
>   edit` after starting a bisect like that, but don't actually edit the
>   file, you'll nonetheless be in a different state.

This is a tangent, but for writing to the general public, please do spell out HEAD, not the line noise synonym "@" that confuses readers.

Show 7 quoted lines
>   I can see a few ways of coping with that:
>
>   1. Change the existing `git bisect start` behaviour to run arguments
>      through `git rev-parse` before recording them.  It appears `git
>      bisect good` et al. already do that, but it is a change in
>      behaviour that I guess could impact badly on other people using
>      `git bisect log`-based workflows.

The issue is not just HEAD but also for anything fruid, i.e. the name of a branch, a search result ":/pattern", etc., and if we want to allow restarting a previously failed bisect session from a midpoint, we should be recording things in absolute terms as early as possible. I'd think it was an oversight the "log" thing did not do so.

>   2. Do a full `git bisect reset` before replaying the log, so the
>      revisions will be parsed in the same way as they were originally.
>      I'd be slightly sad about that, as it seems an unnecessary
>      inefficiency, but it may well be the simplest approach.

It is not just inefficient, but would require there is no a local change; I thought that the current system allows you to have a local modification to a path that is not involved in the bisect session and losing that property would be sad.

> - There aren't yet any tests or documentation changes; I wanted to get
>   commentary on the initial code changes before I spent time on those
>   parts.

There are some chicken-and-egg around this area. For some changes, without a doc update and test addition, it is harder to judge if a reviewer can agree with the proposed change, as there is only a high level description "we allow editing" and the lowest level changes to the actual code, without anything in between that describes the guiding principle and design decision that lead to the patch.

I'll need to see if the changes in the patch is clear/trivial enough to see where you are trying to go to see if it is the case for this patch, though, so read the above paragraph as a general guideline.

← back to recent threads