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

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

[Adam Dinwoodie]

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

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

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

[Christian Couder]

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.

> 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.

> 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.

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

[Christian Couder]

>> +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?

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

[Adam Dinwoodie]

On Wednesday 08 November 2017 at 05:12 pm +0100, Christian Couder wrote:
> 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.

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

[Adam Dinwoodie]

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)

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

[Adam Dinwoodie]

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

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

[Adam Dinwoodie]

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(-)

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

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

[Adam Dinwoodie]

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(-)

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

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

[Junio C Hamano]

Adam Dinwoodie <adam@dinwoodie.org> writes:

> - 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.

>   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.