[PATCH 1/2] git-bisect.sh : create a file if the bisection is in old/new mode, named "BISECT_OLDNEWMODE", so it can easily be seen outside the program without having to read BISECT_TERMS. This will have to be changed in further versions if new terms are introduced.

Signed-off-by: Louis Stuber <redacted>
Signed-off-by: Antoine Delaite <redacted>
---
 git-bisect.sh |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 109bd65..d3d19cb 100644
--- a/git-bisect.sh
+++ b/git-bisect.sh

@@ -183,6 +183,10 @@ bisect_start() {
 	then
 		echo "$BISECT_BAD" >"$GIT_DIR/BISECT_TERMS" &&
 		echo "$BISECT_GOOD" >>"$GIT_DIR/BISECT_TERMS"
+		if test "$BISECT_BAD" = "new"
+		then
+			echo "" > "$GIT_DIR/BISECT_OLDNEWMODE"
+		fi
 	fi &&
 	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
 	#

@@ -416,6 +420,7 @@ bisect_clean_state() {
 	rm -f "$GIT_DIR/BISECT_NAMES" &&
 	rm -f "$GIT_DIR/BISECT_RUN" &&
 	rm -f "$GIT_DIR/BISECT_TERMS" &&
+	rm -f "$GIT_DIR/BISECT_OLDNEWMODE" &&
 	# Cleanup head-name if it got left by an old version of git-bisect
 	rm -f "$GIT_DIR/head-name" &&
 	git update-ref -d --no-deref BISECT_HEAD &&

@@ -544,7 +549,8 @@ check_and_set_terms () {
 			if test ! -s "$GIT_DIR/BISECT_TERMS"
 			then
 				echo "new" >"$GIT_DIR/BISECT_TERMS" &&
-				echo "old" >>"$GIT_DIR/BISECT_TERMS"
+				echo "old" >>"$GIT_DIR/BISECT_TERMS" &&
+				echo "" > "$GIT_DIR/BISECT_OLDNEWMODE"
 			fi
 			BISECT_BAD="new"
 			BISECT_GOOD="old" ;;

-- 
1.7.1

Signed-off-by: Louis Stuber <redacted>
Signed-off-by: Antoine Delaite <redacted>
---
 revision.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index 7ddbaa0..b631596 100644
--- a/revision.c
+++ b/revision.c

@@ -2075,12 +2075,23 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 
 static int for_each_bad_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
 {
-	return for_each_ref_in_submodule(submodule, "refs/bisect/bad", fn, cb_data);
+	/*
+	 * if BISECT_OLDNEWMODE exists, this is an old/new bisect and the path is different
+	 */
+	struct stat st;
+	if (stat(git_path("BISECT_OLDNEWMODE"), &st))
+		return for_each_ref_in_submodule(submodule, "refs/bisect/bad", fn, cb_data);
+	else
+		return for_each_ref_in_submodule(submodule, "refs/bisect/new", fn, cb_data);
 }
 
 static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
 {
-	return for_each_ref_in_submodule(submodule, "refs/bisect/good", fn, cb_data);
+	struct stat st;
+	if (stat(git_path("BISECT_OLDNEWMODE"), &st))
+		return for_each_ref_in_submodule(submodule, "refs/bisect/good", fn, cb_data);
+	else
+		return for_each_ref_in_submodule(submodule, "refs/bisect/old", fn, cb_data);
 }
 
 static int handle_revision_pseudo_opt(const char *submodule,

-- 
1.7.1

On Fri, Jun 5, 2015 at 12:34 PM, Louis Stuber
[off-list ref] wrote:

git-bisect.sh : create a file if the bisection is in old/new mode,
named "BISECT_OLDNEWMODE", so it can easily be seen outside the
program without having to read BISECT_TERMS. This will have to be
changed in further versions if new terms are introduced.

Documentation/SubmittingPatches contains instructions for how to write
a good commit message. The first line should be a very brief
high-level overview of the change, followed by a blank line, followed
by one or more paragraphs justifying and explaining the change. Also,
wrap the commit message to 70-72 columns.

This commit message doesn't do a very good job of explaining the
problem this change is trying to solve or justifying why this solution
is preferable. Justification is particularly important considering the
ominous-sounding final sentence of the commit message (which itself
seems to imply that this is not a very good change).

Nit: Unnecessary quotes around "new" make the code a bit more noisy,
thus slightly more difficult to read.

+               then
+                       echo "" > "$GIT_DIR/BISECT_OLDNEWMODE"

Style: Drop space after redirection operator.

If only the file's existence is important, but not its content, then
you could phrase this more concisely without the 'echo'. Just use the
redirection operator without any command in front of it: >"$somefile"

Same comments apply below.

On Fri, Jun 5, 2015 at 12:34 PM, Louis Stuber
[off-list ref] wrote:

Fix git rev-list --bisect and git bisect visualize when the bisection
is done in old/new mode.

See my review of patch 1/2 regarding writing a good commit message. In
particular, explain what is broken about "git rev-list --bisect" and
"git bisect visualize" so that the reader can understand what this
patch is actually fixing.

Comments which merely repeat what the code itself already clearly says
don't add value, and are thus noise which impede comprehension by
distracting the reader from digesting the underlying logic flow.

+       struct stat st;
+       if (stat(git_path("BISECT_OLDNEWMODE"), &st))
+               return for_each_ref_in_submodule(submodule, "refs/bisect/bad", fn, cb_data);
+       else
+               return for_each_ref_in_submodule(submodule, "refs/bisect/new", fn, cb_data);

Since the two for_each_ref_in_submodule() calls are identical except
for the second argument, the more natural and easier to comprehend way
to phrase this would be be to assign "refs/bisect/bad" or
"refs/bisect/new" to a variable, and then have just a single
invocation of for_each_ref_in_submodule() which uses that variable as
its second argument.

Stepping back a moment: My reading of these two patches is that
BISECT_OLDNEWMODE is introduced as a simple way to detect if "old/new"
mode is being used rather than gleaning that knowledge from the
existing BISECT_TERMS file. Is that correct?

If so, then these changes are likely going in the wrong direction. The
ominous final sentence of the commit message of patch 1/2 is already a
good clue that this approach won't scale well. Further, the approach
taken here undesirably emphasizes ease of implementation and its
attendant fragility over well thought out design.

 }

 static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
 {
-       return for_each_ref_in_submodule(submodule, "refs/bisect/good", fn, cb_data);
+       struct stat st;
+       if (stat(git_path("BISECT_OLDNEWMODE"), &st))
+               return for_each_ref_in_submodule(submodule, "refs/bisect/good", fn, cb_data);
+       else
+               return for_each_ref_in_submodule(submodule, "refs/bisect/old", fn, cb_data);
 }

 static int handle_revision_pseudo_opt(const char *submodule,
--
1.7.1

On Fri, Jun 5, 2015 at 6:34 PM, Louis Stuber
[off-list ref] wrote:

Signed-off-by: Louis Stuber <redacted>
Signed-off-by: Antoine Delaite <redacted>
---

It looks like this patch applies on top of the bisect old/new series
posted by Antoine.
This should be stated somewhere.

I am not sure it's worth it to have both BISECT_TERMS and BISECT_OLDNEWMODE.

Also please note that I suggested to Antoine that the BISECT_BAD and
BISECT_GOOD variables be renamed to something else, like I already did
in some C files.