* [PATCH] git bisect old/new
@ 2012-06-12 2:03 Valentin Duperray
2012-06-12 5:25 ` Christian Couder
2012-06-12 22:56 ` [PATCHv2] " Valentin Duperray
0 siblings, 2 replies; 14+ messages in thread
From: Valentin Duperray @ 2012-06-12 2:03 UTC (permalink / raw)
To: git
Cc: Valentin Duperray, Lucien Kong, Franck Jonas, Thomas Nguy,
Huynh Khoi Nguyen Nguyen, Matthieu Moy
When not looking for a regression during a bisect but for a fix or a
change in another given property, it can be confusing to use 'good'
and 'bad'.
This patch introduce `git bisect new` and `git bisect old` as an
alternative to 'bad' and good' : the commits which have the most
recent version of the property must be marked as `new` and the ones
with the older version as `old`.
The output will be the first commit after the change in the property.
During a new/old bisect session you cannot use bad/good commands and
vice-versa.
Some commands are still not available for old/new:
* git bisect start [<new> [<old>...]] is not possible : the
commits will be treated as bad and good.
* git rev-list --bisect does not treat the revs/bisect/new and
revs/bisect/old-SHA1 files.
* thus, git bisect run <cmd> is not available for new/old.
* git bisect visualize seem to work partially : the tags are
displayed correctly but the tree is not limited to the bisect
section.
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
Documentation/git-bisect.txt | 40 +++++++++++++++
bisect.c | 88 ++++++++++++++++++++++++---------
git-bisect.sh | 113 ++++++++++++++++++++++++++++++++---------
t/t6030-bisect-porcelain.sh | 33 ++++++++++++
4 files changed, 226 insertions(+), 48 deletions(-)
diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index e4f46bc..25673c9 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -20,6 +20,8 @@ on the subcommand:
git bisect start [--no-checkout] [<bad> [<good>...]] [--] [<paths>...]
git bisect bad [<rev>]
git bisect good [<rev>...]
+ git bisect new [<rev>]
+ git bisect old [<rev>...]
git bisect skip [(<rev>|<range>)...]
git bisect reset [<commit>]
git bisect visualize
@@ -104,6 +106,44 @@ For example, `git bisect reset HEAD` will leave you on the current
bisection commit and avoid switching commits at all, while `git bisect
reset bisect/bad` will check out the first bad revision.
+Alternative research: bisect new and bisect old
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+If you are not looking for a regression but for a change of a given
+property, you can use:
+
+------------------------------------------------
+git bisect new [<rev>]
+------------------------------------------------
+
+Mark the commits that have the new version of the property.
+
+------------------------------------------------
+git bisect old [<rev>...]
+------------------------------------------------
+
+Mark the commits that have the old version of the property.
+
+For example, when looking for a fix in the code, the "new" commits are
+the fixed ones and the "old" commits are the unfixed ones.
+
+------------------------------------------------
+$ git bisect start
+$ git bisect new # Current version is fixed
+$ git bisect old bugged_version # bugged_version was the last version
+ # known to be unfixed
+------------------------------------------------
+
+At the end of the commit session, you will have the first commit that
+have the new version of the property ("fixed" here).
+
+You must run `git bisect start` without commits as argument and run
+`git bisect new <rev>`/`git bisect old <rev>...` after to add the
+commits.
+The bisect old/new sessions and the good/bad ones cannot be mixed.
+You must use `git bisect reset` and start again in order to change
+the mode.
+
Bisect visualize
~~~~~~~~~~~~~~~~
diff --git a/bisect.c b/bisect.c
index 48acf73..474b615 100644
--- a/bisect.c
+++ b/bisect.c
@@ -21,6 +21,9 @@ static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
static const char *argv_update_ref[] = {"update-ref", "--no-deref", "BISECT_HEAD", NULL, NULL};
+static const char *bisect_term_bad;
+static const char *bisect_term_good;
+
/* bits #0-15 in revision.h */
#define COUNTED (1u<<16)
@@ -403,9 +406,10 @@ struct commit_list *find_bisection(struct commit_list *list,
static int register_ref(const char *refname, const unsigned char *sha1,
int flags, void *cb_data)
{
- if (!strcmp(refname, "bad")) {
+ if (!strcmp(refname, bisect_term_bad)) {
current_bad_sha1 = sha1;
- } else if (!prefixcmp(refname, "good-")) {
+ } else if (!prefixcmp(refname, "good-") ||
+ !prefixcmp(refname, "old-")) {
sha1_array_append(&good_revs, sha1);
} else if (!prefixcmp(refname, "skip-")) {
sha1_array_append(&skipped_revs, sha1);
@@ -633,7 +637,7 @@ static void exit_if_skipped_commits(struct commit_list *tried,
return;
printf("There are only 'skip'ped commits left to test.\n"
- "The first bad commit could be any of:\n");
+ "The first %s commit could be any of:\n", bisect_term_bad);
print_commit_list(tried, "%s\n", "%s\n");
if (bad)
printf("%s\n", sha1_to_hex(bad));
@@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)
if (is_expected_rev(current_bad_sha1)) {
char *bad_hex = sha1_to_hex(current_bad_sha1);
char *good_hex = join_sha1_array_hex(&good_revs, ' ');
-
- fprintf(stderr, "The merge base %s is bad.\n"
- "This means the bug has been fixed "
- "between %s and [%s].\n",
- bad_hex, bad_hex, good_hex);
-
+ if (!strcmp(bisect_term_bad,"bad")) {
+ fprintf(stderr, "The merge base %s is bad.\n"
+ "This means the bug has been fixed "
+ "between %s and [%s].\n",
+ bad_hex, bad_hex, good_hex);
+ } else {
+ fprintf(stderr, "The merge base %s is new.\n"
+ "The property has changed "
+ "between %s and [%s].\n",
+ bad_hex, bad_hex, good_hex);
+ }
exit(3);
}
- fprintf(stderr, "Some good revs are not ancestor of the bad rev.\n"
+ fprintf(stderr, "Some %s revs are not ancestor of the %s rev.\n"
"git bisect cannot work properly in this case.\n"
- "Maybe you mistake good and bad revs?\n");
+ "Maybe you mistake %s and %s revs?\n",
+ bisect_term_good, bisect_term_bad, bisect_term_good,
+ bisect_term_bad);
exit(1);
}
@@ -754,19 +765,19 @@ static void handle_skipped_merge_base(const unsigned char *mb)
warning("the merge base between %s and [%s] "
"must be skipped.\n"
- "So we cannot be sure the first bad commit is "
+ "So we cannot be sure the first %s commit is "
"between %s and %s.\n"
"We continue anyway.",
- bad_hex, good_hex, mb_hex, bad_hex);
+ bad_hex, good_hex, bisect_term_bad, mb_hex, bad_hex);
free(good_hex);
}
/*
- * "check_merge_bases" checks that merge bases are not "bad".
+ * "check_merge_bases" checks that merge bases are not "bad" (resp. "new").
*
- * - If one is "bad", it means the user assumed something wrong
+ * - If one is "bad" (resp. "new"), it means the user assumed something wrong
* and we must exit with a non 0 error code.
- * - If one is "good", that's good, we have nothing to do.
+ * - If one is "good" (resp. "old"), that's good, we have nothing to do.
* - If one is "skipped", we can't know but we should warn.
* - If we don't know, we should check it out and ask the user to test.
*/
@@ -825,7 +836,8 @@ static int check_ancestors(const char *prefix)
/*
* "check_good_are_ancestors_of_bad" checks that all "good" revs are
- * ancestor of the "bad" rev.
+ * ancestor of the "bad" rev. (resp. all "old" revs are ancestor of
+ * the "new" rev).
*
* If that's not the case, we need to check the merge bases.
* If a merge base must be tested by the user, its source code will be
@@ -838,7 +850,7 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
int fd;
if (!current_bad_sha1)
- die("a bad revision is needed");
+ die("a %s revision is needed", bisect_term_bad);
/* Check if file BISECT_ANCESTORS_OK exists. */
if (!stat(filename, &st) && S_ISREG(st.st_mode))
@@ -889,6 +901,30 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
}
/*
+ * The terms used for this bisect session are stocked in
+ * BISECT_TERMS: it can be bad/good or new/old.
+ * We read them and stock them to adapt the messages
+ * accordingly.
+ */
+void read_bisect_terms(void)
+{
+ struct strbuf str = STRBUF_INIT;
+ const char *filename = git_path("BISECT_TERMS");
+ FILE *fp = fopen(filename, "r");
+
+ if (!fp)
+ die_errno("Could not open file '%s'", filename);
+
+ strbuf_getline(&str, fp, '\n');
+ bisect_term_bad = strbuf_detach(&str, NULL);
+ strbuf_getline(&str, fp, '\n');
+ bisect_term_good = strbuf_detach(&str, NULL);
+
+ strbuf_release(&str);
+ fclose(fp);
+}
+
+/*
* We use the convention that exiting with an exit code 10 means that
* the bisection process finished successfully.
* In this case the calling shell script should exit 0.
@@ -898,6 +934,8 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
*/
int bisect_next_all(const char *prefix, int no_checkout)
{
+ read_bisect_terms();
+
struct rev_info revs;
struct commit_list *tried;
int reaches = 0, all = 0, nr, steps;
@@ -920,13 +958,14 @@ int bisect_next_all(const char *prefix, int no_checkout)
if (!revs.commits) {
/*
- * We should exit here only if the "bad"
+ * We should exit here only if the "bad" (or "new")
* commit is also a "skip" commit.
*/
exit_if_skipped_commits(tried, NULL);
- printf("%s was both good and bad\n",
- sha1_to_hex(current_bad_sha1));
+ printf("%s was both %s and %s\n",
+ sha1_to_hex(current_bad_sha1), bisect_term_good,
+ bisect_term_bad);
exit(1);
}
@@ -941,7 +980,8 @@ int bisect_next_all(const char *prefix, int no_checkout)
if (!hashcmp(bisect_rev, current_bad_sha1)) {
exit_if_skipped_commits(tried, current_bad_sha1);
- printf("%s is the first bad commit\n", bisect_rev_hex);
+ printf("%s is the first %s commit\n", bisect_rev_hex,
+ bisect_term_bad);
show_diff_tree(prefix, revs.commits->item);
/* This means the bisection process succeeded. */
exit(10);
@@ -953,6 +993,8 @@ int bisect_next_all(const char *prefix, int no_checkout)
"(roughly %d step%s)\n", nr, (nr == 1 ? "" : "s"),
steps, (steps == 1 ? "" : "s"));
+ free((char*)bisect_term_bad);
+ free((char*)bisect_term_good);
+
return bisect_checkout(bisect_rev_hex, no_checkout);
}
-
diff --git a/git-bisect.sh b/git-bisect.sh
index 99efbe8..152b4f3 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -1,6 +1,6 @@
#!/bin/sh
-USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run]'
+USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run|new|old]'
LONG_USAGE='git bisect help
print this long help message.
git bisect start [--no-checkout] [<bad> [<good>...]] [--] [<pathspec>...]
@@ -22,7 +22,15 @@ git bisect replay <logfile>
git bisect log
show bisect log.
git bisect run <cmd>...
- use <cmd>... to automatically bisect.
+ use <cmd>... to automatically bisect
+
+When looking for a change in a given property instead of a regression
+you can use
+
+git bisect new [<rev>]
+ mark <rev> as not having the property anymore
+git bisect old [<rev>]
+ mark <rev>... as having the property
Please use "git help bisect" to get the full man page.'
@@ -32,6 +40,8 @@ OPTIONS_SPEC=
_x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
_x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
+NEW="bad"
+OLD="good"
bisect_head()
{
@@ -66,7 +76,7 @@ bisect_autostart() {
bisect_start() {
#
- # Check for one bad and then some good revisions.
+ # Check for one bad (or new) and then some good (or old) revisions.
#
has_double_dash=0
for arg; do
@@ -75,6 +85,7 @@ bisect_start() {
orig_args=$(git rev-parse --sq-quote "$@")
bad_seen=0
eval=''
+ start_bad_good=0
if test "z$(git rev-parse --is-bare-repository)" != zfalse
then
mode=--no-checkout
@@ -99,6 +110,16 @@ bisect_start() {
die "$(eval_gettext "'\$arg' does not appear to be a valid revision")"
break
}
+
+ if test -s "$GIT_DIR/BISECT_TERMS"
+ then
+ if $(sed -n 1p "$GIT_DIR/BISECT_TERMS") != 'bad'
+ then
+ die "$(gettext "you are already bisecting in old/new mode")"
+ fi
+ fi
+ start_bad_good=1
+
case $bad_seen in
0) state='bad' ; bad_seen=1 ;;
*) state='good' ;;
@@ -170,6 +191,11 @@ bisect_start() {
} &&
git rev-parse --sq-quote "$@" >"$GIT_DIR/BISECT_NAMES" &&
eval "$eval true" &&
+ if test $start_bad_good -eq 1 -a ! -s "$GIT_DIR/BISECT_TERMS"
+ then
+ echo "bad" >"$GIT_DIR/BISECT_TERMS" &&
+ echo "good" >>"$GIT_DIR/BISECT_TERMS"
+ fi &&
echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
#
# Check if we can proceed to the next bisect state.
@@ -184,8 +210,8 @@ bisect_write() {
rev="$2"
nolog="$3"
case "$state" in
- bad) tag="$state" ;;
- good|skip) tag="$state"-"$rev" ;;
+ bad|new) tag="$state" ;;
+ good|skip|old) tag="$state"-"$rev" ;;
*) die "$(eval_gettext "Bad bisect_write argument: \$state")" ;;
esac
git update-ref "refs/bisect/$tag" "$rev" || exit
@@ -230,12 +256,12 @@ bisect_state() {
case "$#,$state" in
0,*)
die "$(gettext "Please call 'bisect_state' with at least one argument.")" ;;
- 1,bad|1,good|1,skip)
+ 1,bad|1,good|1,skip|1,new|1,old)
rev=$(git rev-parse --verify $(bisect_head)) ||
die "$(gettext "Bad rev input: $(bisect_head)")"
bisect_write "$state" "$rev"
check_expected_revs "$rev" ;;
- 2,bad|*,good|*,skip)
+ 2,bad|*,good|*,skip|2,new|*,old)
shift
eval=''
for rev in "$@"
@@ -246,8 +272,8 @@ bisect_state() {
done
eval "$eval"
check_expected_revs "$@" ;;
- *,bad)
- die "$(gettext "'git bisect bad' can take only one argument.")" ;;
+ *,bad|*,new)
+ die "$(gettext "'git bisect $NEW' can take only one argument.")" ;;
*)
usage ;;
esac
@@ -256,21 +282,21 @@ bisect_state() {
bisect_next_check() {
missing_good= missing_bad=
- git show-ref -q --verify refs/bisect/bad || missing_bad=t
- test -n "$(git for-each-ref "refs/bisect/good-*")" || missing_good=t
+ git show-ref -q --verify refs/bisect/$NEW || missing_bad=t
+ test -n "$(git for-each-ref "refs/bisect/$OLD-*")" || missing_good=t
case "$missing_good,$missing_bad,$1" in
,,*)
- : have both good and bad - ok
+ : have both good and bad or old and new - ok
;;
*,)
# do not have both but not asked to fail - just report.
false
;;
- t,,good)
- # have bad but not good. we could bisect although
+ t,,good|t,,old)
+ # have bad (or new) but not good (or old). we could bisect although
# this is less optimum.
- gettextln "Warning: bisecting only with a bad commit." >&2
+ gettextln "Warning: bisecting only with a $NEW commit." >&2
if test -t 0
then
# TRANSLATORS: Make sure to include [Y] and [n] in your
@@ -280,17 +306,17 @@ bisect_next_check() {
read yesno
case "$yesno" in [Nn]*) exit 1 ;; esac
fi
- : bisect without good...
+ : bisect without $OLD...
;;
*)
if test -s "$GIT_DIR/BISECT_START"
then
- gettextln "You need to give me at least one good and one bad revisions.
+ gettextln "You need to give me at least one good and one bad (or one old and one new) revisions.
(You can use \"git bisect bad\" and \"git bisect good\" for that.)" >&2
else
gettextln "You need to start by \"git bisect start\".
-You then need to give me at least one good and one bad revisions.
+You then need to give me at least one good and one bad (or one old and one new) revisions.
(You can use \"git bisect bad\" and \"git bisect good\" for that.)" >&2
fi
exit 1 ;;
@@ -304,7 +330,7 @@ bisect_auto_next() {
bisect_next() {
case "$#" in 0) ;; *) usage ;; esac
bisect_autostart
- bisect_next_check good
+ bisect_next_check $OLD
# Perform all bisection computation, display and checkout
git bisect--helper --next-all $(test -f "$GIT_DIR/BISECT_HEAD" && echo --no-checkout)
@@ -378,6 +404,7 @@ bisect_clean_state() {
rm -f "$GIT_DIR/BISECT_LOG" &&
rm -f "$GIT_DIR/BISECT_NAMES" &&
rm -f "$GIT_DIR/BISECT_RUN" &&
+ rm -f "$GIT_DIR/BISECT_TERMS" &&
# 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 &&
@@ -402,7 +429,7 @@ bisect_replay () {
start)
cmd="bisect_start $rev"
eval "$cmd" ;;
- good|bad|skip)
+ good|bad|skip|old|new)
bisect_write "$command" "$rev" ;;
*)
die "$(gettext "?? what are you talking about?")" ;;
@@ -436,9 +463,9 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
state='skip'
elif [ $res -gt 0 ]
then
- state='bad'
+ state="$NEW"
else
- state='good'
+ state="$OLD"
fi
# We have to use a subshell because "bisect_state" can exit.
@@ -447,7 +474,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
cat "$GIT_DIR/BISECT_RUN"
- if sane_grep "first bad commit could be any of" "$GIT_DIR/BISECT_RUN" \
+ if sane_grep "first $NEW commit could be any of" "$GIT_DIR/BISECT_RUN" \
> /dev/null
then
gettextln "bisect run cannot continue any more" >&2
@@ -461,7 +488,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
exit $res
fi
- if sane_grep "is the first bad commit" "$GIT_DIR/BISECT_RUN" > /dev/null
+ if sane_grep "is the first $NEW commit" "$GIT_DIR/BISECT_RUN" > /dev/null
then
gettextln "bisect run success"
exit 0;
@@ -475,18 +502,54 @@ bisect_log () {
cat "$GIT_DIR/BISECT_LOG"
}
+get_mode () {
+ if test -s "$GIT_DIR/BISECT_TERMS"
+ then
+ NEW="$(sed -n 1p "$GIT_DIR/BISECT_TERMS")"
+ OLD="$(sed -n 2p "$GIT_DIR/BISECT_TERMS")"
+ fi
+}
+
case "$#" in
0)
usage ;;
*)
cmd="$1"
+ get_mode
+ case "$cmd" in
+ bad|good|new|old)
+ if test -s "$GIT_DIR/BISECT_TERMS" -a "$cmd" != "$NEW" -a "$cmd" != "$OLD"
+ then
+ die "$(eval_gettext "Invalid command : you're currently in a \$NEW/\$OLD bisect.")"
+ fi
+ case "$cmd" in
+ bad|good)
+ if test ! -s "$GIT_DIR/BISECT_TERMS"
+ then
+ echo "bad" >"$GIT_DIR/BISECT_TERMS" &&
+ echo "good" >>"$GIT_DIR/BISECT_TERMS"
+ fi
+ NEW="bad"
+ OLD="good";;
+ new|old)
+ if test ! -s "$GIT_DIR/BISECT_TERMS"
+ then
+ echo "new" >"$GIT_DIR/BISECT_TERMS" &&
+ echo "old" >>"$GIT_DIR/BISECT_TERMS"
+ fi
+ NEW="new"
+ OLD="old";;
+
+
+ esac;;
+ esac
shift
case "$cmd" in
help)
git bisect -h ;;
start)
bisect_start "$@" ;;
- bad|good)
+ bad|good|new|old)
bisect_state "$cmd" "$@" ;;
skip)
bisect_skip "$@" ;;
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 72e28ee..28a9c95 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -741,6 +741,39 @@ test_expect_success 'bisect: demonstrate identification of damage boundary' "
test \$rc = 0' &&
check_same BROKEN_HASH6 bisect/bad &&
git bisect reset
+ git checkout master
+
"
+test_expect_success 'bisect starts with only one new' '
+ git bisect reset &&
+ git bisect start &&
+ git bisect new $HASH4 &&
+ git bisect next
+'
+test_expect_success 'bisect does not start with only one old' '
+ git bisect reset &&
+ git bisect start &&
+ git bisect old $HASH1 || return 1
+ test_must_fail git bisect next
+
+'
+
+test_expect_success 'bisect start with one new and old' '
+ git bisect reset &&
+ git bisect start &&
+ git bisect old $HASH1 &&
+ git bisect new $HASH4 &&
+ git bisect new &&
+ git bisect new >bisect_result &&
+ grep "$HASH2 is the first new commit" bisect_result &&
+ git bisect reset
+'
+
+test_expect_success 'bisect cannot mix old/new and good/bad' '
+ git bisect start &&
+ git bisect bad $HASH4 &&
+ test_must_fail git bisect old $HASH1
+'
+
test_done
--
1.7.8
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] git bisect old/new
2012-06-12 2:03 [PATCH] git bisect old/new Valentin Duperray
@ 2012-06-12 5:25 ` Christian Couder
2012-06-12 5:43 ` Junio C Hamano
2012-06-12 22:56 ` [PATCHv2] " Valentin Duperray
1 sibling, 1 reply; 14+ messages in thread
From: Christian Couder @ 2012-06-12 5:25 UTC (permalink / raw)
To: Valentin Duperray
Cc: git, Lucien Kong, Franck Jonas, Thomas Nguy,
Huynh Khoi Nguyen Nguyen, Matthieu Moy
Hi,
On Tue, Jun 12, 2012 at 4:03 AM, Valentin Duperray
<Valentin.Duperray@ensimag.imag.fr> wrote:
> When not looking for a regression during a bisect but for a fix or a
> change in another given property, it can be confusing to use 'good'
> and 'bad'.
> This patch introduce `git bisect new` and `git bisect old` as an
> alternative to 'bad' and good' : the commits which have the most
> recent version of the property must be marked as `new` and the ones
> with the older version as `old`.
> The output will be the first commit after the change in the property.
> During a new/old bisect session you cannot use bad/good commands and
> vice-versa.
> Some commands are still not available for old/new:
> * git bisect start [<new> [<old>...]] is not possible : the
> commits will be treated as bad and good.
> * git rev-list --bisect does not treat the revs/bisect/new and
> revs/bisect/old-SHA1 files.
> * thus, git bisect run <cmd> is not available for new/old.
> * git bisect visualize seem to work partially : the tags are
> displayed correctly but the tree is not limited to the bisect
> section.
>
> Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
> Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
> Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
> Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
> Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
If you used some design that was discussed on the mailing list or if
there have been relevant discussions on the mailing list, it would be
nice to have links to the email thread in the commit message.
> ---
> Documentation/git-bisect.txt | 40 +++++++++++++++
> bisect.c | 88 ++++++++++++++++++++++++---------
> git-bisect.sh | 113 ++++++++++++++++++++++++++++++++---------
> t/t6030-bisect-porcelain.sh | 33 ++++++++++++
> 4 files changed, 226 insertions(+), 48 deletions(-)
>
> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
> index e4f46bc..25673c9 100644
> --- a/Documentation/git-bisect.txt
> +++ b/Documentation/git-bisect.txt
> @@ -20,6 +20,8 @@ on the subcommand:
> git bisect start [--no-checkout] [<bad> [<good>...]] [--] [<paths>...]
> git bisect bad [<rev>]
> git bisect good [<rev>...]
> + git bisect new [<rev>]
> + git bisect old [<rev>...]
maybe:
git bisect (bad|new) [<rev>]
git bisect (good|old) [<rev>...]
> git bisect skip [(<rev>|<range>)...]
> git bisect reset [<commit>]
> git bisect visualize
> @@ -104,6 +106,44 @@ For example, `git bisect reset HEAD` will leave you on the current
> bisection commit and avoid switching commits at all, while `git bisect
> reset bisect/bad` will check out the first bad revision.
>
> +Alternative research: bisect new and bisect old
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +If you are not looking for a regression but for a change of a given
> +property, you can use:
I would rather say:
Alternative terms: bisect new and bisect old
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
If you are not at ease with the terms "bad" and "good", perhaps
because you are looking for the commit that introduced a fix, you can
alternatively use "new" and "old" instead.
But note that you cannot mix "bad" and good" with "new" and "old".
------------------------------------------------
git bisect new [<rev>]
------------------------------------------------
Same as "git bisect bad [<rev>]".
------------------------------------------------
git bisect old [<rev>...]
------------------------------------------------
Same as "git bisect good [<rev>...]".
> +
> +------------------------------------------------
> +git bisect new [<rev>]
> +------------------------------------------------
> +
> +Mark the commits that have the new version of the property.
> +
> +------------------------------------------------
> +git bisect old [<rev>...]
> +------------------------------------------------
> +
> +Mark the commits that have the old version of the property.
> +
> +For example, when looking for a fix in the code, the "new" commits are
> +the fixed ones and the "old" commits are the unfixed ones.
Please put this in the example section of the doc.
> +------------------------------------------------
> +$ git bisect start
> +$ git bisect new # Current version is fixed
> +$ git bisect old bugged_version # bugged_version was the last version
> + # known to be unfixed
> +------------------------------------------------
> +
> +At the end of the commit session, you will have the first commit that
> +have the new version of the property ("fixed" here).
> +
> +You must run `git bisect start` without commits as argument and run
> +`git bisect new <rev>`/`git bisect old <rev>...` after to add the
> +commits.
> +The bisect old/new sessions and the good/bad ones cannot be mixed.
> +You must use `git bisect reset` and start again in order to change
> +the mode.
> +
> Bisect visualize
> ~~~~~~~~~~~~~~~~
>
> diff --git a/bisect.c b/bisect.c
> index 48acf73..474b615 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -21,6 +21,9 @@ static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
> static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
> static const char *argv_update_ref[] = {"update-ref", "--no-deref", "BISECT_HEAD", NULL, NULL};
>
> +static const char *bisect_term_bad;
> +static const char *bisect_term_good;
> +
> /* bits #0-15 in revision.h */
>
> #define COUNTED (1u<<16)
> @@ -403,9 +406,10 @@ struct commit_list *find_bisection(struct commit_list *list,
> static int register_ref(const char *refname, const unsigned char *sha1,
> int flags, void *cb_data)
> {
> - if (!strcmp(refname, "bad")) {
> + if (!strcmp(refname, bisect_term_bad)) {
> current_bad_sha1 = sha1;
> - } else if (!prefixcmp(refname, "good-")) {
> + } else if (!prefixcmp(refname, "good-") ||
> + !prefixcmp(refname, "old-")) {
I don't like very much "good" and "old" to be hardcoded here.
> @@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)
> if (is_expected_rev(current_bad_sha1)) {
> char *bad_hex = sha1_to_hex(current_bad_sha1);
> char *good_hex = join_sha1_array_hex(&good_revs, ' ');
> -
> - fprintf(stderr, "The merge base %s is bad.\n"
> - "This means the bug has been fixed "
> - "between %s and [%s].\n",
> - bad_hex, bad_hex, good_hex);
> -
> + if (!strcmp(bisect_term_bad,"bad")) {
> + fprintf(stderr, "The merge base %s is bad.\n"
> + "This means the bug has been fixed "
> + "between %s and [%s].\n",
> + bad_hex, bad_hex, good_hex);
> + } else {
> + fprintf(stderr, "The merge base %s is new.\n"
> + "The property has changed "
> + "between %s and [%s].\n",
> + bad_hex, bad_hex, good_hex);
> + }
I don't like very much "new" to be harcoded here too.
>
> /*
> - * "check_merge_bases" checks that merge bases are not "bad".
> + * "check_merge_bases" checks that merge bases are not "bad" (resp. "new").
> *
> - * - If one is "bad", it means the user assumed something wrong
> + * - If one is "bad" (resp. "new"), it means the user assumed something wrong
> * and we must exit with a non 0 error code.
> - * - If one is "good", that's good, we have nothing to do.
> + * - If one is "good" (resp. "old"), that's good, we have nothing to do.
> * - If one is "skipped", we can't know but we should warn.
> * - If we don't know, we should check it out and ask the user to test.
> */
I am not sure changing the comments is worth it...
> @@ -825,7 +836,8 @@ static int check_ancestors(const char *prefix)
>
> /*
> * "check_good_are_ancestors_of_bad" checks that all "good" revs are
> - * ancestor of the "bad" rev.
> + * ancestor of the "bad" rev. (resp. all "old" revs are ancestor of
> + * the "new" rev).
...because we don't change the name of the function anyway.
> *
> * If that's not the case, we need to check the merge bases.
> * If a merge base must be tested by the user, its source code will be
> @@ -838,7 +850,7 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
> int fd;
>
> if (!current_bad_sha1)
> - die("a bad revision is needed");
> + die("a %s revision is needed", bisect_term_bad);
>
> /* Check if file BISECT_ANCESTORS_OK exists. */
> if (!stat(filename, &st) && S_ISREG(st.st_mode))
> @@ -889,6 +901,30 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
> }
>
> /*
> + * The terms used for this bisect session are stocked in
> + * BISECT_TERMS: it can be bad/good or new/old.
I am not sure saying "it can be bad/good or new/old" adds anything.
You could just say: the defaults are "bad"/"good"
> + * We read them and stock them to adapt the messages
> + * accordingly.
> + */
> +void read_bisect_terms(void)
> +{
> + struct strbuf str = STRBUF_INIT;
> + const char *filename = git_path("BISECT_TERMS");
> + FILE *fp = fopen(filename, "r");
> +
> + if (!fp)
> + die_errno("Could not open file '%s'", filename);
This is not very compatible with older git versions.
I know that it's kind of strange to upgrade git in the middle of a
bisection but why not just use "bad"/"good" if there is no file?
> + strbuf_getline(&str, fp, '\n');
> + bisect_term_bad = strbuf_detach(&str, NULL);
> + strbuf_getline(&str, fp, '\n');
> + bisect_term_good = strbuf_detach(&str, NULL);
> +
> + strbuf_release(&str);
> + fclose(fp);
> +}
> +
> +/*
> * We use the convention that exiting with an exit code 10 means that
> * the bisection process finished successfully.
> * In this case the calling shell script should exit 0.
> @@ -898,6 +934,8 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
> */
> int bisect_next_all(const char *prefix, int no_checkout)
> {
> + read_bisect_terms();
> +
> struct rev_info revs;
> struct commit_list *tried;
> int reaches = 0, all = 0, nr, steps;
We put all declarations at the beginning of functions.
> @@ -920,13 +958,14 @@ int bisect_next_all(const char *prefix, int no_checkout)
>
> if (!revs.commits) {
> /*
> - * We should exit here only if the "bad"
> + * We should exit here only if the "bad" (or "new")
Again I don't think it's worth changing comments.
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 99efbe8..152b4f3 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -1,6 +1,6 @@
> #!/bin/sh
>
> -USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run]'
> +USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run|new|old]'
> LONG_USAGE='git bisect help
> print this long help message.
> git bisect start [--no-checkout] [<bad> [<good>...]] [--] [<pathspec>...]
> @@ -22,7 +22,15 @@ git bisect replay <logfile>
> git bisect log
> show bisect log.
> git bisect run <cmd>...
> - use <cmd>... to automatically bisect.
> + use <cmd>... to automatically bisect
Why this change?
> +
> +When looking for a change in a given property instead of a regression
> +you can use
> +
> +git bisect new [<rev>]
> + mark <rev> as not having the property anymore
> +git bisect old [<rev>]
It is: [<rev>...]
> + mark <rev>... as having the property
But anyway if possible I'd rather have:
git bisect (bad|new) [<rev>]
git bisect (good|old) [<rev>...]
> Please use "git help bisect" to get the full man page.'
>
> @@ -32,6 +40,8 @@ OPTIONS_SPEC=
>
> _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
> _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
> +NEW="bad"
> +OLD="good"
Why not BISECT_BAD_TERM/BISECT_GOOD_TERM instead of NEW/OLD?
It should be consistent with bisect.c
>
> bisect_head()
> {
> @@ -66,7 +76,7 @@ bisect_autostart() {
>
> bisect_start() {
> #
> - # Check for one bad and then some good revisions.
> + # Check for one bad (or new) and then some good (or old) revisions.
I don't think it's worth changing the comments.
> @@ -184,8 +210,8 @@ bisect_write() {
> rev="$2"
> nolog="$3"
> case "$state" in
> - bad) tag="$state" ;;
> - good|skip) tag="$state"-"$rev" ;;
> + bad|new) tag="$state" ;;
> + good|skip|old) tag="$state"-"$rev" ;;
Why not "$BISECT_TERM_BAD" instead of "bad|new" and
"$BISECT_TERM_GOOD|skip" instead of "good|skip|old"?
> *) die "$(eval_gettext "Bad bisect_write argument: \$state")" ;;
> esac
> git update-ref "refs/bisect/$tag" "$rev" || exit
> @@ -230,12 +256,12 @@ bisect_state() {
> case "$#,$state" in
> 0,*)
> die "$(gettext "Please call 'bisect_state' with at least one argument.")" ;;
> - 1,bad|1,good|1,skip)
> + 1,bad|1,good|1,skip|1,new|1,old)
Idem.
> rev=$(git rev-parse --verify $(bisect_head)) ||
> die "$(gettext "Bad rev input: $(bisect_head)")"
> bisect_write "$state" "$rev"
> check_expected_revs "$rev" ;;
> - 2,bad|*,good|*,skip)
> + 2,bad|*,good|*,skip|2,new|*,old)
Idem.
> shift
> eval=''
> for rev in "$@"
> @@ -246,8 +272,8 @@ bisect_state() {
> done
> eval "$eval"
> check_expected_revs "$@" ;;
> - *,bad)
> - die "$(gettext "'git bisect bad' can take only one argument.")" ;;
> + *,bad|*,new)
Idem.
> + die "$(gettext "'git bisect $NEW' can take only one argument.")" ;;
> *)
> usage ;;
> esac
> @@ -256,21 +282,21 @@ bisect_state() {
>
> bisect_next_check() {
> missing_good= missing_bad=
> - git show-ref -q --verify refs/bisect/bad || missing_bad=t
> - test -n "$(git for-each-ref "refs/bisect/good-*")" || missing_good=t
> + git show-ref -q --verify refs/bisect/$NEW || missing_bad=t
> + test -n "$(git for-each-ref "refs/bisect/$OLD-*")" || missing_good=t
>
> case "$missing_good,$missing_bad,$1" in
> ,,*)
> - : have both good and bad - ok
> + : have both good and bad or old and new - ok
> ;;
> *,)
> # do not have both but not asked to fail - just report.
> false
> ;;
> - t,,good)
> - # have bad but not good. we could bisect although
> + t,,good|t,,old)
Idem.
Thanks,
Christian.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] git bisect old/new
2012-06-12 5:25 ` Christian Couder
@ 2012-06-12 5:43 ` Junio C Hamano
2012-06-12 19:41 ` Phil Hord
0 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2012-06-12 5:43 UTC (permalink / raw)
To: Christian Couder
Cc: Valentin Duperray, git, Lucien Kong, Franck Jonas, Thomas Nguy,
Huynh Khoi Nguyen Nguyen, Matthieu Moy
Christian Couder <christian.couder@gmail.com> writes:
Thanks for comments.
> If you used some design that was discussed on the mailing list or if
> there have been relevant discussions on the mailing list, it would be
> nice to have links to the email thread in the commit message.
Perhaps.
>> + git bisect new [<rev>]
>> + git bisect old [<rev>...]
>
> maybe:
>
> git bisect (bad|new) [<rev>]
> git bisect (good|old) [<rev>...]
Definitely.
>> @@ -104,6 +106,44 @@ For example, `git bisect reset HEAD` will leave you on the current
>> bisection commit and avoid switching commits at all, while `git bisect
>> reset bisect/bad` will check out the first bad revision.
>>
>> +Alternative research: bisect new and bisect old
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +If you are not looking for a regression but for a change of a given
>> +property, you can use:
>
> I would rather say:
> ...
Good.
>> @@ -403,9 +406,10 @@ struct commit_list *find_bisection(struct commit_list *list,
>> static int register_ref(const char *refname, const unsigned char *sha1,
>> int flags, void *cb_data)
>> {
>> - if (!strcmp(refname, "bad")) {
>> + if (!strcmp(refname, bisect_term_bad)) {
>> current_bad_sha1 = sha1;
>> - } else if (!prefixcmp(refname, "good-")) {
>> + } else if (!prefixcmp(refname, "good-") ||
>> + !prefixcmp(refname, "old-")) {
>
> I don't like very much "good" and "old" to be hardcoded here.
Really?
>> @@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)
>> if (is_expected_rev(current_bad_sha1)) {
>> char *bad_hex = sha1_to_hex(current_bad_sha1);
>> char *good_hex = join_sha1_array_hex(&good_revs, ' ');
>> + if (!strcmp(bisect_term_bad,"bad")) {
>> + fprintf(stderr, "The merge base %s is bad.\n"
>> + "This means the bug has been fixed "
>> + "between %s and [%s].\n",
>> + bad_hex, bad_hex, good_hex);
>> + } else {
>> + fprintf(stderr, "The merge base %s is new.\n"
>> + "The property has changed "
>> + "between %s and [%s].\n",
>> + bad_hex, bad_hex, good_hex);
>> + }
>
> I don't like very much "new" to be harcoded here too.
Why not? It is not like we will be adding any more synonym pair
beyond good/bad, so...
>>
>> /*
>> - * "check_merge_bases" checks that merge bases are not "bad".
>> + * "check_merge_bases" checks that merge bases are not "bad" (resp. "new").
>> *
>> - * - If one is "bad", it means the user assumed something wrong
>> + * - If one is "bad" (resp. "new"), it means the user assumed something wrong
>> * and we must exit with a non 0 error code.
>> - * - If one is "good", that's good, we have nothing to do.
>> + * - If one is "good" (resp. "old"), that's good, we have nothing to do.
>> * - If one is "skipped", we can't know but we should warn.
>> * - If we don't know, we should check it out and ask the user to test.
>> */
>
> I am not sure changing the comments is worth it...
I think it is probably a good idea to cast in stone that we support
two pairs, i.e. good/bad or old/new. I would have said "or" instead
of "resp." above, though.
>> @@ -889,6 +901,30 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
>> }
>>
>> /*
>> + * The terms used for this bisect session are stocked in
>> + * BISECT_TERMS: it can be bad/good or new/old.
>
> I am not sure saying "it can be bad/good or new/old" adds anything.
It makes it clear that we are not allowing arbitrary pair of words
to substitute the good/bad pair, which is a plus.
>> +void read_bisect_terms(void)
>> +{
>> + struct strbuf str = STRBUF_INIT;
>> + const char *filename = git_path("BISECT_TERMS");
>> + FILE *fp = fopen(filename, "r");
>> +
>> + if (!fp)
>> + die_errno("Could not open file '%s'", filename);
>
> This is not very compatible with older git versions.
> I know that it's kind of strange to upgrade git in the middle of a
> bisection but why not just use "bad"/"good" if there is no file?
Good thinking.
>> @@ -898,6 +934,8 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
>> */
>> int bisect_next_all(const char *prefix, int no_checkout)
>> {
>> + read_bisect_terms();
>> +
>> struct rev_info revs;
>> struct commit_list *tried;
>> int reaches = 0, all = 0, nr, steps;
>
> We put all declarations at the beginning of functions.
Good eyes.
>> @@ -22,7 +22,15 @@ git bisect replay <logfile>
>> git bisect log
>> show bisect log.
>> git bisect run <cmd>...
>> - use <cmd>... to automatically bisect.
>> + use <cmd>... to automatically bisect
>
> Why this change?
To end a sentence with a full-stop?
> But anyway if possible I'd rather have:
>
> git bisect (bad|new) [<rev>]
> git bisect (good|old) [<rev>...]
Yes.
>> @@ -32,6 +40,8 @@ OPTIONS_SPEC=
>>
>> _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
>> _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
>> +NEW="bad"
>> +OLD="good"
>
> Why not BISECT_BAD_TERM/BISECT_GOOD_TERM instead of NEW/OLD?
> It should be consistent with bisect.c
It's kind of too long. Isn't BISECT_GOOD vs BISECT_BAD good enough
(and if so make bisect.c consistent with it).
>> @@ -184,8 +210,8 @@ bisect_write() {
>> rev="$2"
>> nolog="$3"
>> case "$state" in
>> - bad) tag="$state" ;;
>> - good|skip) tag="$state"-"$rev" ;;
>> + bad|new) tag="$state" ;;
>> + good|skip|old) tag="$state"-"$rev" ;;
>
> Why not "$BISECT_TERM_BAD" instead of "bad|new" and
> "$BISECT_TERM_GOOD|skip" instead of "good|skip|old"?
If the point is to make sure "git bisect good" will error out when
we are in new/old mode, I agree (and also the other case/esac in the
remainder of the patch that allows you feed bad and new mixed).
These case arms look indented in a funny way, but is it only because
of e-mail quoting, by the way?
>> *) die "$(eval_gettext "Bad bisect_write argument: \$state")" ;;
>> esac
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] git bisect old/new
2012-06-12 5:43 ` Junio C Hamano
@ 2012-06-12 19:41 ` Phil Hord
2012-06-13 10:12 ` Christian Couder
2012-06-13 17:30 ` Junio C Hamano
0 siblings, 2 replies; 14+ messages in thread
From: Phil Hord @ 2012-06-12 19:41 UTC (permalink / raw)
To: Junio C Hamano
Cc: Christian Couder, Valentin Duperray, git, Lucien Kong,
Franck Jonas, Thomas Nguy, Huynh Khoi Nguyen Nguyen,
Matthieu Moy
On Tue, Jun 12, 2012 at 1:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> Thanks for comments.
>
> > If you used some design that was discussed on the mailing list or if
> > there have been relevant discussions on the mailing list, it would be
> > nice to have links to the email thread in the commit message.
>
> Perhaps.
>
> >> + git bisect new [<rev>]
> >> + git bisect old [<rev>...]
> >
> > maybe:
> >
> > git bisect (bad|new) [<rev>]
> > git bisect (good|old) [<rev>...]
>
> Definitely.
>
> >> @@ -104,6 +106,44 @@ For example, `git bisect reset HEAD` will leave
> >> you on the current
> >> bisection commit and avoid switching commits at all, while `git bisect
> >> reset bisect/bad` will check out the first bad revision.
> >>
> >> +Alternative research: bisect new and bisect old
> >> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> +
> >> +If you are not looking for a regression but for a change of a given
> >> +property, you can use:
> >
> > I would rather say:
> > ...
>
> Good.
>
> >> @@ -403,9 +406,10 @@ struct commit_list *find_bisection(struct
> >> commit_list *list,
> >> static int register_ref(const char *refname, const unsigned char
> >> *sha1,
> >> int flags, void *cb_data)
> >> {
> >> - if (!strcmp(refname, "bad")) {
> >> + if (!strcmp(refname, bisect_term_bad)) {
> >> current_bad_sha1 = sha1;
> >> - } else if (!prefixcmp(refname, "good-")) {
> >> + } else if (!prefixcmp(refname, "good-") ||
> >> + !prefixcmp(refname, "old-")) {
> >
> > I don't like very much "good" and "old" to be hardcoded here.
>
> Really?
I tend to agree. But I like more generic code and less hard-coded in
almost all cases.
> >> @@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)
> >> if (is_expected_rev(current_bad_sha1)) {
> >> char *bad_hex = sha1_to_hex(current_bad_sha1);
> >> char *good_hex = join_sha1_array_hex(&good_revs, ' ');
> >> + if (!strcmp(bisect_term_bad,"bad")) {
> >> + fprintf(stderr, "The merge base %s is bad.\n"
> >> + "This means the bug has been fixed "
> >> + "between %s and [%s].\n",
> >> + bad_hex, bad_hex, good_hex);
> >> + } else {
> >> + fprintf(stderr, "The merge base %s is new.\n"
> >> + "The property has changed "
> >> + "between %s and [%s].\n",
> >> + bad_hex, bad_hex, good_hex);
> >> + }
> >
> > I don't like very much "new" to be harcoded here too.
>
> Why not? It is not like we will be adding any more synonym pair
> beyond good/bad, so...
Previously we discussed using yes/no, among others.
http://permalink.gmane.org/gmane.comp.version-control.git/182496
>
> >>
> >> /*
> >> - * "check_merge_bases" checks that merge bases are not "bad".
> >> + * "check_merge_bases" checks that merge bases are not "bad" (resp.
> >> "new").
> >> *
> >> - * - If one is "bad", it means the user assumed something wrong
> >> + * - If one is "bad" (resp. "new"), it means the user assumed
> >> something wrong
> >> * and we must exit with a non 0 error code.
> >> - * - If one is "good", that's good, we have nothing to do.
> >> + * - If one is "good" (resp. "old"), that's good, we have nothing to
> >> do.
> >> * - If one is "skipped", we can't know but we should warn.
> >> * - If we don't know, we should check it out and ask the user to test.
> >> */
> >
> > I am not sure changing the comments is worth it...
>
> I think it is probably a good idea to cast in stone that we support
> two pairs, i.e. good/bad or old/new. I would have said "or" instead
> of "resp." above, though.
>
> >> @@ -889,6 +901,30 @@ static void show_diff_tree(const char *prefix,
> >> struct commit *commit)
> >> }
> >>
> >> /*
> >> + * The terms used for this bisect session are stocked in
> >> + * BISECT_TERMS: it can be bad/good or new/old.
> >
> > I am not sure saying "it can be bad/good or new/old" adds anything.
>
> It makes it clear that we are not allowing arbitrary pair of words
> to substitute the good/bad pair, which is a plus.
>
> >> +void read_bisect_terms(void)
> >> +{
> >> + struct strbuf str = STRBUF_INIT;
> >> + const char *filename = git_path("BISECT_TERMS");
> >> + FILE *fp = fopen(filename, "r");
> >> +
> >> + if (!fp)
> >> + die_errno("Could not open file '%s'", filename);
> >
> > This is not very compatible with older git versions.
> > I know that it's kind of strange to upgrade git in the middle of a
> > bisection but why not just use "bad"/"good" if there is no file?
>
> Good thinking.
>
> >> @@ -898,6 +934,8 @@ static void show_diff_tree(const char *prefix,
> >> struct commit *commit)
> >> */
> >> int bisect_next_all(const char *prefix, int no_checkout)
> >> {
> >> + read_bisect_terms();
> >> +
> >> struct rev_info revs;
> >> struct commit_list *tried;
> >> int reaches = 0, all = 0, nr, steps;
> >
> > We put all declarations at the beginning of functions.
>
> Good eyes.
>
> >> @@ -22,7 +22,15 @@ git bisect replay <logfile>
> >> git bisect log
> >> show bisect log.
> >> git bisect run <cmd>...
> >> - use <cmd>... to automatically bisect.
> >> + use <cmd>... to automatically bisect
> >
> > Why this change?
>
> To end a sentence with a full-stop?
I think you read this backwards. This change removes the full-stop.
Phil
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCHv2] git bisect old/new
2012-06-12 2:03 [PATCH] git bisect old/new Valentin Duperray
2012-06-12 5:25 ` Christian Couder
@ 2012-06-12 22:56 ` Valentin Duperray
2012-06-12 23:54 ` Junio C Hamano
2012-06-13 10:05 ` Christian Couder
1 sibling, 2 replies; 14+ messages in thread
From: Valentin Duperray @ 2012-06-12 22:56 UTC (permalink / raw)
To: git
Cc: Valentin Duperray, Franck Jonas, Lucien Kong, Thomas Nguy,
Huynh Khoi Nguyen Nguyen, Matthieu Moy
When not looking for a regression during a bisect but for a fix or a
change in another given property, it can be confusing to use 'good'
and 'bad'.
This patch introduce `git bisect new` and `git bisect old` as an
alternative to 'bad' and good': the commits which have the most
recent version of the property must be marked as `new` and the ones
with the older version as `old`.
The output will be the first commit after the change in the property.
During a new/old bisect session you cannot use bad/good commands and
vice-versa.
`git bisect replay` works fine for old/new bisect sessions.
Some commands are still not available for old/new:
* git bisect start [<new> [<old>...]] is not possible: the
commits will be treated as bad and good.
* git rev-list --bisect does not treat the revs/bisect/new and
revs/bisect/old-SHA1 files.
* thus, git bisect run <cmd> is not available for new/old.
* git bisect visualize seem to work partially: the tags are
displayed correctly but the tree is not limited to the bisect
section.
Related discussions:
- http://thread.gmane.org/gmane.comp.version-control.git/86063
introduced bisect fix unfixed to find fix.
- http://thread.gmane.org/gmane.comp.version-control.git/182398
discussion around bisect yes/no or old/new.
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
Documentation/git-bisect.txt | 43 ++++++++++++++-
bisect.c | 84 +++++++++++++++++++++--------
git-bisect.sh | 121 ++++++++++++++++++++++++++++++++----------
t/t6030-bisect-porcelain.sh | 40 ++++++++++++++
4 files changed, 236 insertions(+), 52 deletions(-)
diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index e4f46bc..fc63894 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -18,8 +18,10 @@ on the subcommand:
git bisect help
git bisect start [--no-checkout] [<bad> [<good>...]] [--] [<paths>...]
- git bisect bad [<rev>]
- git bisect good [<rev>...]
+ git bisect (bad|new) [<rev>]
+ git bisect (good|old) [<rev>...]
+ git bisect new [<rev>]
+ git bisect old [<rev>...]
git bisect skip [(<rev>|<range>)...]
git bisect reset [<commit>]
git bisect visualize
@@ -104,6 +106,31 @@ For example, `git bisect reset HEAD` will leave you on the current
bisection commit and avoid switching commits at all, while `git bisect
reset bisect/bad` will check out the first bad revision.
+
+Alternative terms: bisect new and bisect old
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+If you are not at ease with the terms "bad" and "good", perhaps
+because you are looking for the commit that introduced a fix, you can
+alternatively use "new" and "old" instead.
+But note that you cannot mix "bad" and good" with "new" and "old".
+
+------------------------------------------------
+git bisect new [<rev>]
+------------------------------------------------
+
+Same as "git bisect bad [<rev>]".
+
+------------------------------------------------
+git bisect old [<rev>...]
+------------------------------------------------
+
+Same as "git bisect good [<rev>...]".
+
+You must run `git bisect start` without commits as argument and run
+`git bisect new <rev>`/`git bisect old <rev>...` after to add the
+commits.
+
Bisect visualize
~~~~~~~~~~~~~~~~
@@ -374,6 +401,18 @@ In this case, when 'git bisect run' finishes, bisect/bad will refer to a commit
has at least one parent whose reachable graph is fully traversable in the sense
required by 'git pack objects'.
+* Look for a fix instead of a regression in the code
++
+------------
+$ git bisect start
+$ git bisect new # Current version is fixed
+$ git bisect old bugged_version # bugged_version was the last version
+ # known to be unfixed
+------------
++
+The "new" commits are the fixed ones and the "old" commits are the unfixed ones.
+At the end of the commit session, you will have the first fixed commit.
+
SEE ALSO
--------
diff --git a/bisect.c b/bisect.c
index 48acf73..38de2d5 100644
--- a/bisect.c
+++ b/bisect.c
@@ -21,6 +21,9 @@ static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
static const char *argv_update_ref[] = {"update-ref", "--no-deref", "BISECT_HEAD", NULL, NULL};
+static const char *bisect_bad;
+static const char *bisect_good;
+
/* bits #0-15 in revision.h */
#define COUNTED (1u<<16)
@@ -403,9 +406,10 @@ struct commit_list *find_bisection(struct commit_list *list,
static int register_ref(const char *refname, const unsigned char *sha1,
int flags, void *cb_data)
{
- if (!strcmp(refname, "bad")) {
+ if (!strcmp(refname, bisect_bad)) {
current_bad_sha1 = sha1;
- } else if (!prefixcmp(refname, "good-")) {
+ } else if (!prefixcmp(refname, "good-") ||
+ !prefixcmp(refname, "old-")) {
sha1_array_append(&good_revs, sha1);
} else if (!prefixcmp(refname, "skip-")) {
sha1_array_append(&skipped_revs, sha1);
@@ -633,7 +637,7 @@ static void exit_if_skipped_commits(struct commit_list *tried,
return;
printf("There are only 'skip'ped commits left to test.\n"
- "The first bad commit could be any of:\n");
+ "The first %s commit could be any of:\n", bisect_bad);
print_commit_list(tried, "%s\n", "%s\n");
if (bad)
printf("%s\n", sha1_to_hex(bad));
@@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)
if (is_expected_rev(current_bad_sha1)) {
char *bad_hex = sha1_to_hex(current_bad_sha1);
char *good_hex = join_sha1_array_hex(&good_revs, ' ');
-
- fprintf(stderr, "The merge base %s is bad.\n"
- "This means the bug has been fixed "
- "between %s and [%s].\n",
- bad_hex, bad_hex, good_hex);
-
+ if (!strcmp(bisect_bad,"bad")) {
+ fprintf(stderr, "The merge base %s is bad.\n"
+ "This means the bug has been fixed "
+ "between %s and [%s].\n",
+ bad_hex, bad_hex, good_hex);
+ } else {
+ fprintf(stderr, "The merge base %s is new.\n"
+ "The property has changed "
+ "between %s and [%s].\n",
+ bad_hex, bad_hex, good_hex);
+ }
exit(3);
}
- fprintf(stderr, "Some good revs are not ancestor of the bad rev.\n"
+ fprintf(stderr, "Some %s revs are not ancestor of the %s rev.\n"
"git bisect cannot work properly in this case.\n"
- "Maybe you mistake good and bad revs?\n");
+ "Maybe you mistake %s and %s revs?\n",
+ bisect_good, bisect_bad, bisect_good,
+ bisect_bad);
exit(1);
}
@@ -754,19 +765,19 @@ static void handle_skipped_merge_base(const unsigned char *mb)
warning("the merge base between %s and [%s] "
"must be skipped.\n"
- "So we cannot be sure the first bad commit is "
+ "So we cannot be sure the first %s commit is "
"between %s and %s.\n"
"We continue anyway.",
- bad_hex, good_hex, mb_hex, bad_hex);
+ bad_hex, good_hex, bisect_bad, mb_hex, bad_hex);
free(good_hex);
}
/*
- * "check_merge_bases" checks that merge bases are not "bad".
+ * "check_merge_bases" checks that merge bases are not "bad" (or "new").
*
- * - If one is "bad", it means the user assumed something wrong
+ * - If one is "bad" (or "new"), it means the user assumed something wrong
* and we must exit with a non 0 error code.
- * - If one is "good", that's good, we have nothing to do.
+ * - If one is "good" (or "old"), that's good, we have nothing to do.
* - If one is "skipped", we can't know but we should warn.
* - If we don't know, we should check it out and ask the user to test.
*/
@@ -838,7 +849,7 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
int fd;
if (!current_bad_sha1)
- die("a bad revision is needed");
+ die("a %s revision is needed", bisect_bad);
/* Check if file BISECT_ANCESTORS_OK exists. */
if (!stat(filename, &st) && S_ISREG(st.st_mode))
@@ -889,6 +900,31 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
}
/*
+ * The terms used for this bisect session are stocked in
+ * BISECT_TERMS: it can be bad/good or new/old.
+ * We read them and stock them to adapt the messages
+ * accordingly. Default is bad/good.
+ */
+void read_bisect_terms(void)
+{
+ struct strbuf str = STRBUF_INIT;
+ const char *filename = git_path("BISECT_TERMS");
+ FILE *fp = fopen(filename, "r");
+
+ if (!fp) {
+ bisect_bad = "bad";
+ bisect_good = "good";
+ } else {
+ strbuf_getline(&str, fp, '\n');
+ bisect_bad = strbuf_detach(&str, NULL);
+ strbuf_getline(&str, fp, '\n');
+ bisect_good = strbuf_detach(&str, NULL);
+ }
+ strbuf_release(&str);
+ fclose(fp);
+}
+
+/*
* We use the convention that exiting with an exit code 10 means that
* the bisection process finished successfully.
* In this case the calling shell script should exit 0.
@@ -903,7 +939,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
int reaches = 0, all = 0, nr, steps;
const unsigned char *bisect_rev;
char bisect_rev_hex[41];
-
+ read_bisect_terms();
if (read_bisect_refs())
die("reading bisect refs failed");
@@ -925,8 +961,9 @@ int bisect_next_all(const char *prefix, int no_checkout)
*/
exit_if_skipped_commits(tried, NULL);
- printf("%s was both good and bad\n",
- sha1_to_hex(current_bad_sha1));
+ printf("%s was both %s and %s\n",
+ sha1_to_hex(current_bad_sha1), bisect_good,
+ bisect_bad);
exit(1);
}
@@ -941,7 +978,8 @@ int bisect_next_all(const char *prefix, int no_checkout)
if (!hashcmp(bisect_rev, current_bad_sha1)) {
exit_if_skipped_commits(tried, current_bad_sha1);
- printf("%s is the first bad commit\n", bisect_rev_hex);
+ printf("%s is the first %s commit\n", bisect_rev_hex,
+ bisect_bad);
show_diff_tree(prefix, revs.commits->item);
/* This means the bisection process succeeded. */
exit(10);
@@ -953,6 +991,8 @@ int bisect_next_all(const char *prefix, int no_checkout)
"(roughly %d step%s)\n", nr, (nr == 1 ? "" : "s"),
steps, (steps == 1 ? "" : "s"));
+ free((char*)bisect_bad);
+ free((char*)bisect_good);
+
return bisect_checkout(bisect_rev_hex, no_checkout);
}
-
diff --git a/git-bisect.sh b/git-bisect.sh
index 99efbe8..6ba87e5 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -1,14 +1,16 @@
#!/bin/sh
-USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run]'
+USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run|new|old]'
LONG_USAGE='git bisect help
print this long help message.
git bisect start [--no-checkout] [<bad> [<good>...]] [--] [<pathspec>...]
reset bisect state and start bisection.
-git bisect bad [<rev>]
- mark <rev> a known-bad revision.
-git bisect good [<rev>...]
- mark <rev>... known-good revisions.
+git bisect (bad|new) [<rev>]
+ mark <rev> a known-bad revision/
+ a revision after change in a given property.
+git bisect (good|old) [<rev>...]
+ mark <rev>... known-good revisions/
+ revisions before change in a given property.
git bisect skip [(<rev>|<range>)...]
mark <rev>... untestable revisions.
git bisect next
@@ -32,6 +34,8 @@ OPTIONS_SPEC=
_x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
_x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
+BISECT_BAD="bad"
+BISECT_GOOD="good"
bisect_head()
{
@@ -75,6 +79,7 @@ bisect_start() {
orig_args=$(git rev-parse --sq-quote "$@")
bad_seen=0
eval=''
+ start_bad_good=0
if test "z$(git rev-parse --is-bare-repository)" != zfalse
then
mode=--no-checkout
@@ -99,6 +104,16 @@ bisect_start() {
die "$(eval_gettext "'\$arg' does not appear to be a valid revision")"
break
}
+
+ if test -s "$GIT_DIR/BISECT_TERMS"
+ then
+ if $(sed -n 1p "$GIT_DIR/BISECT_TERMS") != 'bad'
+ then
+ die "$(gettext "you are already bisecting in old/new mode")"
+ fi
+ fi
+ start_bad_good=1
+
case $bad_seen in
0) state='bad' ; bad_seen=1 ;;
*) state='good' ;;
@@ -170,6 +185,11 @@ bisect_start() {
} &&
git rev-parse --sq-quote "$@" >"$GIT_DIR/BISECT_NAMES" &&
eval "$eval true" &&
+ if test $start_bad_good -eq 1 -a ! -s "$GIT_DIR/BISECT_TERMS"
+ then
+ echo "bad" >"$GIT_DIR/BISECT_TERMS" &&
+ echo "good" >>"$GIT_DIR/BISECT_TERMS"
+ fi &&
echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
#
# Check if we can proceed to the next bisect state.
@@ -184,9 +204,12 @@ bisect_write() {
rev="$2"
nolog="$3"
case "$state" in
- bad) tag="$state" ;;
- good|skip) tag="$state"-"$rev" ;;
- *) die "$(eval_gettext "Bad bisect_write argument: \$state")" ;;
+ "$BISECT_BAD")
+ tag="$state" ;;
+ "$BISECT_GOOD"|skip)
+ tag="$state"-"$rev" ;;
+ *)
+ die "$(eval_gettext "Bad bisect_write argument: \$state")" ;;
esac
git update-ref "refs/bisect/$tag" "$rev" || exit
echo "# $state: $(git show-branch $rev)" >>"$GIT_DIR/BISECT_LOG"
@@ -230,12 +253,12 @@ bisect_state() {
case "$#,$state" in
0,*)
die "$(gettext "Please call 'bisect_state' with at least one argument.")" ;;
- 1,bad|1,good|1,skip)
+ 1,"$BISECT_BAD"|1,"$BISECT_GOOD"|1,skip)
rev=$(git rev-parse --verify $(bisect_head)) ||
die "$(gettext "Bad rev input: $(bisect_head)")"
bisect_write "$state" "$rev"
check_expected_revs "$rev" ;;
- 2,bad|*,good|*,skip)
+ 2,"$BISECT_BAD"|*,"$BISECT_GOOD"|*,skip)
shift
eval=''
for rev in "$@"
@@ -246,8 +269,8 @@ bisect_state() {
done
eval "$eval"
check_expected_revs "$@" ;;
- *,bad)
- die "$(gettext "'git bisect bad' can take only one argument.")" ;;
+ *,"$BISECT_BAD")
+ die "$(gettext "'git bisect $BISECT_BAD' can take only one argument.")" ;;
*)
usage ;;
esac
@@ -256,21 +279,21 @@ bisect_state() {
bisect_next_check() {
missing_good= missing_bad=
- git show-ref -q --verify refs/bisect/bad || missing_bad=t
- test -n "$(git for-each-ref "refs/bisect/good-*")" || missing_good=t
+ git show-ref -q --verify refs/bisect/$BISECT_BAD || missing_bad=t
+ test -n "$(git for-each-ref "refs/bisect/$BISECT_GOOD-*")" || missing_good=t
case "$missing_good,$missing_bad,$1" in
,,*)
- : have both good and bad - ok
+ : have both good and bad or old and new - ok
;;
*,)
# do not have both but not asked to fail - just report.
false
;;
- t,,good)
- # have bad but not good. we could bisect although
+ t,,"$BISECT_GOOD")
+ # have bad (or new) but not good (or old). we could bisect although
# this is less optimum.
- gettextln "Warning: bisecting only with a bad commit." >&2
+ gettextln "Warning: bisecting only with a $BISECT_BAD commit." >&2
if test -t 0
then
# TRANSLATORS: Make sure to include [Y] and [n] in your
@@ -280,17 +303,17 @@ bisect_next_check() {
read yesno
case "$yesno" in [Nn]*) exit 1 ;; esac
fi
- : bisect without good...
+ : bisect without $BISECT_GOOD...
;;
*)
if test -s "$GIT_DIR/BISECT_START"
then
- gettextln "You need to give me at least one good and one bad revisions.
+ gettextln "You need to give me at least one good and one bad (or one old and one new) revisions.
(You can use \"git bisect bad\" and \"git bisect good\" for that.)" >&2
else
gettextln "You need to start by \"git bisect start\".
-You then need to give me at least one good and one bad revisions.
+You then need to give me at least one good and one bad (or one old and one new) revisions.
(You can use \"git bisect bad\" and \"git bisect good\" for that.)" >&2
fi
exit 1 ;;
@@ -304,7 +327,7 @@ bisect_auto_next() {
bisect_next() {
case "$#" in 0) ;; *) usage ;; esac
bisect_autostart
- bisect_next_check good
+ bisect_next_check $BISECT_GOOD
# Perform all bisection computation, display and checkout
git bisect--helper --next-all $(test -f "$GIT_DIR/BISECT_HEAD" && echo --no-checkout)
@@ -378,6 +401,7 @@ bisect_clean_state() {
rm -f "$GIT_DIR/BISECT_LOG" &&
rm -f "$GIT_DIR/BISECT_NAMES" &&
rm -f "$GIT_DIR/BISECT_RUN" &&
+ rm -f "$GIT_DIR/BISECT_TERMS" &&
# 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 &&
@@ -398,11 +422,13 @@ bisect_replay () {
rev="$command"
command="$bisect"
fi
+ get_terms
+ check_and_set_terms "$command"
case "$command" in
start)
cmd="bisect_start $rev"
eval "$cmd" ;;
- good|bad|skip)
+ good|bad|skip|old|new)
bisect_write "$command" "$rev" ;;
*)
die "$(gettext "?? what are you talking about?")" ;;
@@ -436,9 +462,9 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
state='skip'
elif [ $res -gt 0 ]
then
- state='bad'
+ state="$BISECT_BAD"
else
- state='good'
+ state="$BISECT_GOOD"
fi
# We have to use a subshell because "bisect_state" can exit.
@@ -447,7 +473,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
cat "$GIT_DIR/BISECT_RUN"
- if sane_grep "first bad commit could be any of" "$GIT_DIR/BISECT_RUN" \
+ if sane_grep "first $BISECT_BAD commit could be any of" "$GIT_DIR/BISECT_RUN" \
> /dev/null
then
gettextln "bisect run cannot continue any more" >&2
@@ -461,7 +487,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
exit $res
fi
- if sane_grep "is the first bad commit" "$GIT_DIR/BISECT_RUN" > /dev/null
+ if sane_grep "is the first $BISECT_BAD commit" "$GIT_DIR/BISECT_RUN" > /dev/null
then
gettextln "bisect run success"
exit 0;
@@ -475,18 +501,57 @@ bisect_log () {
cat "$GIT_DIR/BISECT_LOG"
}
+get_terms () {
+ if test -s "$GIT_DIR/BISECT_TERMS"
+ then
+ BISECT_BAD="$(sed -n 1p "$GIT_DIR/BISECT_TERMS")"
+ BISECT_GOOD="$(sed -n 2p "$GIT_DIR/BISECT_TERMS")"
+ fi
+}
+
+check_and_set_terms () {
+ cmd="$1"
+ case "$cmd" in
+ bad|good|new|old)
+ if test -s "$GIT_DIR/BISECT_TERMS" -a "$cmd" != "$BISECT_BAD" -a "$cmd" != "$BISECT_GOOD"
+ then
+ die "$(eval_gettext "Invalid command : you're currently in a \$BISECT_BAD/\$BISECT_GOOD bisect.")"
+ fi
+ case "$cmd" in
+ bad|good)
+ if test ! -s "$GIT_DIR/BISECT_TERMS"
+ then
+ echo "bad" >"$GIT_DIR/BISECT_TERMS" &&
+ echo "good" >>"$GIT_DIR/BISECT_TERMS"
+ fi
+ BISECT_BAD="bad"
+ BISECT_GOOD="good" ;;
+ new|old)
+ if test ! -s "$GIT_DIR/BISECT_TERMS"
+ then
+ echo "new" >"$GIT_DIR/BISECT_TERMS" &&
+ echo "old" >>"$GIT_DIR/BISECT_TERMS"
+ fi
+ BISECT_BAD="new"
+ BISECT_GOOD="old" ;;
+ esac ;;
+ esac
+}
+
case "$#" in
0)
usage ;;
*)
cmd="$1"
+ get_terms
+ check_and_set_terms "$cmd"
shift
case "$cmd" in
help)
git bisect -h ;;
start)
bisect_start "$@" ;;
- bad|good)
+ bad|good|new|old)
bisect_state "$cmd" "$@" ;;
skip)
bisect_skip "$@" ;;
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 72e28ee..c4a5d6b 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -741,6 +741,46 @@ test_expect_success 'bisect: demonstrate identification of damage boundary' "
test \$rc = 0' &&
check_same BROKEN_HASH6 bisect/bad &&
git bisect reset
+ git checkout master
+
"
+test_expect_success 'bisect starts with only one new' '
+ git bisect reset &&
+ git bisect start &&
+ git bisect new $HASH4 &&
+ git bisect next
+'
+test_expect_success 'bisect does not start with only one old' '
+ git bisect reset &&
+ git bisect start &&
+ git bisect old $HASH1 || return 1
+ test_must_fail git bisect next
+
+'
+
+test_expect_success 'bisect start with one new and old' '
+ git bisect reset &&
+ git bisect start &&
+ git bisect old $HASH1 &&
+ git bisect new $HASH4 &&
+ git bisect new &&
+ git bisect new >bisect_result &&
+ grep "$HASH2 is the first new commit" bisect_result &&
+ git bisect log > log_to_replay.txt &&
+ git bisect reset
+'
+
+test_expect_success 'bisect replay with old and new' '
+ git bisect replay log_to_replay.txt > bisect_result &&
+ grep "$HASH2 is the first new commit" bisect_result &&
+ git bisect reset
+'
+
+test_expect_success 'bisect cannot mix old/new and good/bad' '
+ git bisect start &&
+ git bisect bad $HASH4 &&
+ test_must_fail git bisect old $HASH1
+'
+
test_done
--
1.7.8
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCHv2] git bisect old/new
2012-06-12 22:56 ` [PATCHv2] " Valentin Duperray
@ 2012-06-12 23:54 ` Junio C Hamano
2012-06-13 18:06 ` duperrav
2012-06-13 10:05 ` Christian Couder
1 sibling, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2012-06-12 23:54 UTC (permalink / raw)
To: Valentin Duperray
Cc: git, Franck Jonas, Lucien Kong, Thomas Nguy,
Huynh Khoi Nguyen Nguyen, Matthieu Moy
Valentin Duperray <Valentin.Duperray@ensimag.imag.fr> writes:
Please make sure that you have Christian who took the time to review
the previous round on Cc:, and have addressed the issues raised in
the review.
> Some commands are still not available for old/new:
>
> * git bisect start [<new> [<old>...]] is not possible: the
> commits will be treated as bad and good.
> * git rev-list --bisect does not treat the revs/bisect/new and
> revs/bisect/old-SHA1 files.
> * thus, git bisect run <cmd> is not available for new/old.
> * git bisect visualize seem to work partially: the tags are
> displayed correctly but the tree is not limited to the bisect
> section.
Would be easier to review if the subject is marked as RFC while
these todo items are still there.
Also before going too far into the implementation, I think it is a
good idea to think how you are going to address the above issues. I
suspect the changes to bisect.c will have to be vastly different
depending on that plan.
> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
> index e4f46bc..fc63894 100644
> --- a/Documentation/git-bisect.txt
> +++ b/Documentation/git-bisect.txt
> @@ -18,8 +18,10 @@ on the subcommand:
>
> git bisect help
> git bisect start [--no-checkout] [<bad> [<good>...]] [--] [<paths>...]
> - git bisect bad [<rev>]
> - git bisect good [<rev>...]
> + git bisect (bad|new) [<rev>]
> + git bisect (good|old) [<rev>...]
> + git bisect new [<rev>]
> + git bisect old [<rev>...]
Huh?
> +If you are not at ease with the terms "bad" and "good", perhaps
> +because you are looking for the commit that introduced a fix, you can
> +alternatively use "new" and "old" instead.
> +But note that you cannot mix "bad" and good" with "new" and "old".
> +
> +------------------------------------------------
> +git bisect new [<rev>]
> +------------------------------------------------
> +
> +Same as "git bisect bad [<rev>]".
It somewhat makes me feel uneasy to see a sentence without any verb.
> +------------------------------------------------
> +git bisect old [<rev>...]
> +------------------------------------------------
> +
> +Same as "git bisect good [<rev>...]".
Likewise.
> +You must run `git bisect start` without commits as argument and run
> +`git bisect new <rev>`/`git bisect old <rev>...` after to add the
> +commits.
What happens when you do:
git bisect start
git bisect new HEAD
git bisect old v1.0.0
and then
git bisect bad v1.2.0
Does it error out? For that matter, what happens if you do this?
git bisect start
git bisect new HEAD
git bisect good v1.0.0
Note that I am not suggesting to document the behaviour here.
> @@ -374,6 +401,18 @@ In this case, when 'git bisect run' finishes, bisect/bad will refer to a commit
> has at least one parent whose reachable graph is fully traversable in the sense
> required by 'git pack objects'.
>
> +* Look for a fix instead of a regression in the code
> ++
> +------------
> +$ git bisect start
> +$ git bisect new # Current version is fixed
> +$ git bisect old bugged_version # bugged_version was the last version
> + # known to be unfixed
We do not usually use "bug" as a transitive verb that means "to
break". Perhaps "buggy_version" is easier to read and gramatically
more correct.
> ++
> +The "new" commits are the fixed ones and the "old" commits are the unfixed ones.
> +At the end of the commit session, you will have the first fixed commit.
> +
commit session? Is it a bisect session?
> diff --git a/bisect.c b/bisect.c
> index 48acf73..38de2d5 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -21,6 +21,9 @@ static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
> static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
> static const char *argv_update_ref[] = {"update-ref", "--no-deref", "BISECT_HEAD", NULL, NULL};
>
> +static const char *bisect_bad;
> +static const char *bisect_good;
> +
> /* bits #0-15 in revision.h */
>
> #define COUNTED (1u<<16)
> @@ -403,9 +406,10 @@ struct commit_list *find_bisection(struct commit_list *list,
> static int register_ref(const char *refname, const unsigned char *sha1,
> int flags, void *cb_data)
> {
> - if (!strcmp(refname, "bad")) {
> + if (!strcmp(refname, bisect_bad)) {
> current_bad_sha1 = sha1;
> - } else if (!prefixcmp(refname, "good-")) {
> + } else if (!prefixcmp(refname, "good-") ||
> + !prefixcmp(refname, "old-")) {
It feels kind of strange that only one of "bad" or "new" is allowed,
while "good-X" and "old-Y" can coexist and be used interchangeably.
> @@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)
> if (is_expected_rev(current_bad_sha1)) {
> char *bad_hex = sha1_to_hex(current_bad_sha1);
> char *good_hex = join_sha1_array_hex(&good_revs, ' ');
> + if (!strcmp(bisect_bad,"bad")) {
s/,/, /;
But see below. It feels wrong to always running string comparison
when we know there are either good/bad mode or old/new mode.
> - fprintf(stderr, "Some good revs are not ancestor of the bad rev.\n"
> + fprintf(stderr, "Some %s revs are not ancestor of the %s rev.\n"
> "git bisect cannot work properly in this case.\n"
> - "Maybe you mistake good and bad revs?\n");
> + "Maybe you mistake %s and %s revs?\n",
> + bisect_good, bisect_bad, bisect_good,
> + bisect_bad);
You merely inherited this from the original, but shouldn't it be
"mistook" in the past tense?
> @@ -889,6 +900,31 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
> }
>
> /*
> + * The terms used for this bisect session are stocked in
> + * BISECT_TERMS: it can be bad/good or new/old.
> + * We read them and stock them to adapt the messages
> + * accordingly. Default is bad/good.
> + */
> +void read_bisect_terms(void)
> +{
> + struct strbuf str = STRBUF_INIT;
> + const char *filename = git_path("BISECT_TERMS");
> + FILE *fp = fopen(filename, "r");
> +
> + if (!fp) {
> + bisect_bad = "bad";
> + bisect_good = "good";
> + } else {
> + strbuf_getline(&str, fp, '\n');
> + bisect_bad = strbuf_detach(&str, NULL);
> + strbuf_getline(&str, fp, '\n');
> + bisect_good = strbuf_detach(&str, NULL);
> + }
> + strbuf_release(&str);
> + fclose(fp);
> +}
While this is not wrong per-se, I am not sure if storing and reading
two lines from this file is really worth the trouble.
Wouldn't it be easier to change the convention so that the presense
of BISECT_OLDNEW file signals that the program is working in the
old/new mode as opposed to the traditional good/bad mode, or perhaps
a single line "true" or "false" in the file tells us if we are in
OLDNEW mode, or something?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2] git bisect old/new
2012-06-12 22:56 ` [PATCHv2] " Valentin Duperray
2012-06-12 23:54 ` Junio C Hamano
@ 2012-06-13 10:05 ` Christian Couder
1 sibling, 0 replies; 14+ messages in thread
From: Christian Couder @ 2012-06-13 10:05 UTC (permalink / raw)
To: Valentin Duperray
Cc: git, Franck Jonas, Lucien Kong, Thomas Nguy,
Huynh Khoi Nguyen Nguyen, Matthieu Moy
On Wed, Jun 13, 2012 at 12:56 AM, Valentin Duperray
<Valentin.Duperray@ensimag.imag.fr> wrote:
>
> Related discussions:
>
> - http://thread.gmane.org/gmane.comp.version-control.git/86063
> introduced bisect fix unfixed to find fix.
> - http://thread.gmane.org/gmane.comp.version-control.git/182398
> discussion around bisect yes/no or old/new.
Thanks!
> /*
> + * The terms used for this bisect session are stocked in
> + * BISECT_TERMS: it can be bad/good or new/old.
> + * We read them and stock them to adapt the messages
> + * accordingly. Default is bad/good.
> + */
> +void read_bisect_terms(void)
> +{
> + struct strbuf str = STRBUF_INIT;
> + const char *filename = git_path("BISECT_TERMS");
> + FILE *fp = fopen(filename, "r");
> +
> + if (!fp) {
> + bisect_bad = "bad";
> + bisect_good = "good";
Ok, but...
> + } else {
> + strbuf_getline(&str, fp, '\n');
> + bisect_bad = strbuf_detach(&str, NULL);
> + strbuf_getline(&str, fp, '\n');
> + bisect_good = strbuf_detach(&str, NULL);
> + }
> + strbuf_release(&str);
> + fclose(fp);
> +}
> @@ -953,6 +991,8 @@ int bisect_next_all(const char *prefix, int no_checkout)
> "(roughly %d step%s)\n", nr, (nr == 1 ? "" : "s"),
> steps, (steps == 1 ? "" : "s"));
>
> + free((char*)bisect_bad);
> + free((char*)bisect_good);
...it is not a good thing to free these variables if they were not allocated.
> +
> return bisect_checkout(bisect_rev_hex, no_checkout);
> }
> case "$missing_good,$missing_bad,$1" in
> ,,*)
> - : have both good and bad - ok
> + : have both good and bad or old and new - ok
Maybe ": have both $BISECT_GOOD and $BISECT_BAD - ok".
Thanks,
Christian.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] git bisect old/new
2012-06-12 19:41 ` Phil Hord
@ 2012-06-13 10:12 ` Christian Couder
2012-06-13 17:30 ` Junio C Hamano
1 sibling, 0 replies; 14+ messages in thread
From: Christian Couder @ 2012-06-13 10:12 UTC (permalink / raw)
To: Phil Hord
Cc: Junio C Hamano, Valentin Duperray, git, Lucien Kong,
Franck Jonas, Thomas Nguy, Huynh Khoi Nguyen Nguyen,
Matthieu Moy
On Tue, Jun 12, 2012 at 9:41 PM, Phil Hord <phil.hord@gmail.com> wrote:
> On Tue, Jun 12, 2012 at 1:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> >> @@ -403,9 +406,10 @@ struct commit_list *find_bisection(struct
>> >> commit_list *list,
>> >> static int register_ref(const char *refname, const unsigned char
>> >> *sha1,
>> >> int flags, void *cb_data)
>> >> {
>> >> - if (!strcmp(refname, "bad")) {
>> >> + if (!strcmp(refname, bisect_term_bad)) {
>> >> current_bad_sha1 = sha1;
>> >> - } else if (!prefixcmp(refname, "good-")) {
>> >> + } else if (!prefixcmp(refname, "good-") ||
>> >> + !prefixcmp(refname, "old-")) {
>> >
>> > I don't like very much "good" and "old" to be hardcoded here.
>>
>> Really?
>
> I tend to agree. But I like more generic code and less hard-coded in
> almost all cases.
Yeah, I prefer generic code too.
>> >> @@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)
>> >> if (is_expected_rev(current_bad_sha1)) {
>> >> char *bad_hex = sha1_to_hex(current_bad_sha1);
>> >> char *good_hex = join_sha1_array_hex(&good_revs, ' ');
>> >> + if (!strcmp(bisect_term_bad,"bad")) {
>> >> + fprintf(stderr, "The merge base %s is bad.\n"
>> >> + "This means the bug has been fixed "
>> >> + "between %s and [%s].\n",
>> >> + bad_hex, bad_hex, good_hex);
>> >> + } else {
>> >> + fprintf(stderr, "The merge base %s is new.\n"
>> >> + "The property has changed "
>> >> + "between %s and [%s].\n",
>> >> + bad_hex, bad_hex, good_hex);
>> >> + }
>> >
>> > I don't like very much "new" to be harcoded here too.
>>
>> Why not? It is not like we will be adding any more synonym pair
>> beyond good/bad, so...
>
> Previously we discussed using yes/no, among others.
> http://permalink.gmane.org/gmane.comp.version-control.git/182496
And even if we add "yes/no" maybe people will still want to use
"good/bad" instead of "bad/good", "new/old" or "yes/no".
Thanks,
Christian.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] git bisect old/new
2012-06-12 19:41 ` Phil Hord
2012-06-13 10:12 ` Christian Couder
@ 2012-06-13 17:30 ` Junio C Hamano
1 sibling, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2012-06-13 17:30 UTC (permalink / raw)
To: Phil Hord
Cc: Christian Couder, Valentin Duperray, git, Lucien Kong,
Franck Jonas, Thomas Nguy, Huynh Khoi Nguyen Nguyen,
Matthieu Moy
Phil Hord <phil.hord@gmail.com> writes:
> On Tue, Jun 12, 2012 at 1:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Why not? It is not like we will be adding any more synonym pair
>> beyond good/bad, so...
>
> Previously we discussed using yes/no, among others.
> http://permalink.gmane.org/gmane.comp.version-control.git/182496
Yes, but didn't we dismiss yes/no and others as no better than
bad/good after discussing?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2] git bisect old/new
2012-06-12 23:54 ` Junio C Hamano
@ 2012-06-13 18:06 ` duperrav
2012-06-14 9:56 ` Christian Couder
0 siblings, 1 reply; 14+ messages in thread
From: duperrav @ 2012-06-13 18:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Valentin Duperray, git, Franck Jonas, Lucien Kong, Thomas Nguy,
Huynh Khoi Nguyen Nguyen, Matthieu Moy, christian.couder
Junio C Hamano <gitster@pobox.com> a écrit :
>> Some commands are still not available for old/new:
>>
>> * git bisect start [<new> [<old>...]] is not possible: the
>> commits will be treated as bad and good.
>> * git rev-list --bisect does not treat the revs/bisect/new and
>> revs/bisect/old-SHA1 files.
>> * thus, git bisect run <cmd> is not available for new/old.
>> * git bisect visualize seem to work partially: the tags are
>> displayed correctly but the tree is not limited to the bisect
>> section.
>
> Would be easier to review if the subject is marked as RFC while
> these todo items are still there.
>
> Also before going too far into the implementation, I think it is a
> good idea to think how you are going to address the above issues. I
> suspect the changes to bisect.c will have to be vastly different
> depending on that plan.
* git bisect start [<new> [<old>...]]:
The idea would be to add a "--new" option to start in new/old mode.
* git rev-list --bisect:
I see two solutions for this:
- read revisions from both refs/bisect/bad and refs/bisect/new
(resp. refs/bisect/good and refs/bisect/old).
- read revisions only from refs/bisect/bad and refs/bisect/good
when the BISECT_TERMS doesn't exist or contains bad/good
and
read revisions only from refs/bisect/new and refs/bisect/old
when the BISECT_TERMS exists and contains new/old.
I prefer the latter because I don't really know how reading all files
will affect the calls of "git rev-list" outside of a bisect session and
the two types of files should not be present simultaneously anyway.
> What happens when you do:
>
> git bisect start
> git bisect new HEAD
> git bisect old v1.0.0
>
> and then
>
> git bisect bad v1.2.0
>
> Does it error out? For that matter, what happens if you do this?
>
> git bisect start
> git bisect new HEAD
> git bisect good v1.0.0
>
In both cases, the `git bisect good`/`git bisect bad`command is
considered invalid. The message
"Invalid command : you're currently in a new/old bisect."
is displayed and the bisect section is not reseted.
Same thing happens if you try to use new/old in a bad/good
bisect session.
>> @@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)
>> if (is_expected_rev(current_bad_sha1)) {
>> char *bad_hex = sha1_to_hex(current_bad_sha1);
>> char *good_hex = join_sha1_array_hex(&good_revs, ' ');
>> + if (!strcmp(bisect_bad,"bad")) {
>
> s/,/, /;
>
> But see below. It feels wrong to always running string comparison
> when we know there are either good/bad mode or old/new mode.
>
>> @@ -889,6 +900,31 @@ static void show_diff_tree(const char *prefix,
>> struct commit *commit)
>> }
>>
>> /*
>> + * The terms used for this bisect session are stocked in
>> + * BISECT_TERMS: it can be bad/good or new/old.
>> + * We read them and stock them to adapt the messages
>> + * accordingly. Default is bad/good.
>> + */
>> +void read_bisect_terms(void)
>> +{
>> + struct strbuf str = STRBUF_INIT;
>> + const char *filename = git_path("BISECT_TERMS");
>> + FILE *fp = fopen(filename, "r");
>> +
>> + if (!fp) {
>> + bisect_bad = "bad";
>> + bisect_good = "good";
>> + } else {
>> + strbuf_getline(&str, fp, '\n');
>> + bisect_bad = strbuf_detach(&str, NULL);
>> + strbuf_getline(&str, fp, '\n');
>> + bisect_good = strbuf_detach(&str, NULL);
>> + }
>> + strbuf_release(&str);
>> + fclose(fp);
>> +}
>
> While this is not wrong per-se, I am not sure if storing and reading
> two lines from this file is really worth the trouble.
>
> Wouldn't it be easier to change the convention so that the presense
> of BISECT_OLDNEW file signals that the program is working in the
> old/new mode as opposed to the traditional good/bad mode, or perhaps
> a single line "true" or "false" in the file tells us if we are in
> OLDNEW mode, or something?
If there is consensus around the fact that no other terms will be added
after old/new, only checking if the file is present would be easier
indeed.
Thanks,
Valentin
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2] git bisect old/new
2012-06-13 18:06 ` duperrav
@ 2012-06-14 9:56 ` Christian Couder
2012-06-14 17:34 ` Junio C Hamano
0 siblings, 1 reply; 14+ messages in thread
From: Christian Couder @ 2012-06-14 9:56 UTC (permalink / raw)
To: duperrav
Cc: Junio C Hamano, Valentin Duperray, git, Franck Jonas,
Lucien Kong, Thomas Nguy, Huynh Khoi Nguyen Nguyen, Matthieu Moy,
Peff
On Wed, Jun 13, 2012 at 8:06 PM, <duperrav@minatec.inpg.fr> wrote:
>
> Junio C Hamano <gitster@pobox.com> a écrit :
>
>
>>> Some commands are still not available for old/new:
>>>
>>> * git bisect start [<new> [<old>...]] is not possible: the
>>> commits will be treated as bad and good.
>>> * git rev-list --bisect does not treat the revs/bisect/new and
>>> revs/bisect/old-SHA1 files.
>>> * thus, git bisect run <cmd> is not available for new/old.
>>> * git bisect visualize seem to work partially: the tags are
>>> displayed correctly but the tree is not limited to the bisect
>>> section.
>>
>>
>> Would be easier to review if the subject is marked as RFC while
>> these todo items are still there.
>>
>> Also before going too far into the implementation, I think it is a
>> good idea to think how you are going to address the above issues. I
>> suspect the changes to bisect.c will have to be vastly different
>> depending on that plan.
>
>
> * git bisect start [<new> [<old>...]]:
>
> The idea would be to add a "--new" option to start in new/old mode.
I am ok with that.
> * git rev-list --bisect:
>
> I see two solutions for this:
>
> - read revisions from both refs/bisect/bad and refs/bisect/new
> (resp. refs/bisect/good and refs/bisect/old).
>
> - read revisions only from refs/bisect/bad and refs/bisect/good
> when the BISECT_TERMS doesn't exist or contains bad/good
> and
> read revisions only from refs/bisect/new and refs/bisect/old
> when the BISECT_TERMS exists and contains new/old.
>
> I prefer the latter because I don't really know how reading all files
> will affect the calls of "git rev-list" outside of a bisect session and
> the two types of files should not be present simultaneously anyway.
Why didn't you consider adding another option: "--bisect-terms-new" or
"--bisect-terms=new,old" or "--bisect-refs=new,old"?
By the way, I just looked at the doc for "--bisect-vars". This outputs
text ready to be eval'ed by the shell, and, among the variables it
outputs, there are "bisect_bad" and "bisect_good".
So maybe we should avoid using BISECT_BAD and BISECT_GOOD shell
variables in git-bisect.sh to avoid confusion.
>> While this is not wrong per-se, I am not sure if storing and reading
>> two lines from this file is really worth the trouble.
>>
>> Wouldn't it be easier to change the convention so that the presense
>> of BISECT_OLDNEW file signals that the program is working in the
>> old/new mode as opposed to the traditional good/bad mode, or perhaps
>> a single line "true" or "false" in the file tells us if we are in
>> OLDNEW mode, or something?
>
>
> If there is consensus around the fact that no other terms will be added
> after old/new, only checking if the file is present would be easier
> indeed.
Here is the end of the previous thread where old/new was originaly discussed:
http://thread.gmane.org/gmane.comp.version-control.git/182398/focus=183410
Peff said:
-------
Hmm. I think this is not quite as nice, but it is way simpler. It may be
worth trying for a bit to see how people like it. If they don't, the
cost of failure is that we have to maintain "old/new" forever, even
after we implement a yes/no reversible scheme. But maintaining the
old/new mapping from yes/no would not be any harder than the good/bad
mapping, which we would need to do anyway.
So it sounds like a reasonable first step.
-------
So in my opinion, there was a consensus that if old/new is not enough
it should not be a big deal to maintain anyway.
And I agree with this provided that we indeed implement old/new so
that it's not a big deal to maintain if we change our mind later.
(For example we might later want "yes/no", or "good/bad" with a
meaning reversed, or perhaps something else.)
So I'd rather have a file with a generic name like "BISECT_TERMS", but
it may contain just one line like for example "new/old".
We could just check that the content of the line is "new/old" and
die("Only 'new/old' is supported in $GIT_DIR/BISECT_TERMS") if it is
something else.
Thanks,
Christian.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2] git bisect old/new
2012-06-14 9:56 ` Christian Couder
@ 2012-06-14 17:34 ` Junio C Hamano
2012-06-15 20:20 ` Phil Hord
0 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2012-06-14 17:34 UTC (permalink / raw)
To: Christian Couder
Cc: duperrav, Valentin Duperray, git, Franck Jonas, Lucien Kong,
Thomas Nguy, Huynh Khoi Nguyen Nguyen, Matthieu Moy, Peff
Christian Couder <christian.couder@gmail.com> writes:
> Peff said:
>
> -------
> Hmm. I think this is not quite as nice, but it is way simpler. It may be
> worth trying for a bit to see how people like it. If they don't, the
> cost of failure is that we have to maintain "old/new" forever, even
> after we implement a yes/no reversible scheme. But maintaining the
> old/new mapping from yes/no would not be any harder than the good/bad
> mapping, which we would need to do anyway.
>
> So it sounds like a reasonable first step.
> -------
The above is a very reasonable stand. But I do not think it leads
to the following at all.
> So I'd rather have a file with a generic name like "BISECT_TERMS", but
> it may contain just one line like for example "new/old".
That is forcing an unnecessary complexity, when we do not even know
if we need anything more than old/new. We can start simple and the
result may be sufficient and in which case we can stop there.
In fact, Peff's advice you quoted is against doing what you just
said, which is to do more than we know that we need right now.
If we end up needing arbitrary pair of words, then at that point we
may add a mechanism that records the pair of words in use, be they
<yes/no>, or <frotz/xyzzy>. And when that happens, <new/old> will
continue to be supported for free--there is no extra work to support
<new/old> in addition to the work needed to support <good/bad> and
<arbitrary1/arbitrary2> that we need to support anyway.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2] git bisect old/new
2012-06-14 17:34 ` Junio C Hamano
@ 2012-06-15 20:20 ` Phil Hord
2012-06-15 21:06 ` Junio C Hamano
0 siblings, 1 reply; 14+ messages in thread
From: Phil Hord @ 2012-06-15 20:20 UTC (permalink / raw)
To: Junio C Hamano
Cc: Christian Couder, duperrav, Valentin Duperray, git, Franck Jonas,
Lucien Kong, Thomas Nguy, Huynh Khoi Nguyen Nguyen, Matthieu Moy,
Peff
On Thu, Jun 14, 2012 at 1:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > Peff said:
> >
> > -------
> > Hmm. I think this is not quite as nice, but it is way simpler. It may be
> > worth trying for a bit to see how people like it. If they don't, the
> > cost of failure is that we have to maintain "old/new" forever, even
> > after we implement a yes/no reversible scheme. But maintaining the
> > old/new mapping from yes/no would not be any harder than the good/bad
> > mapping, which we would need to do anyway.
> >
> > So it sounds like a reasonable first step.
> > -------
>
> The above is a very reasonable stand. But I do not think it leads
> to the following at all.
>
> > So I'd rather have a file with a generic name like "BISECT_TERMS", but
> > it may contain just one line like for example "new/old".
>
> That is forcing an unnecessary complexity, when we do not even know
> if we need anything more than old/new. We can start simple and the
> result may be sufficient and in which case we can stop there.
>
> In fact, Peff's advice you quoted is against doing what you just
> said, which is to do more than we know that we need right now.
>
> If we end up needing arbitrary pair of words, then at that point we
> may add a mechanism that records the pair of words in use, be they
> <yes/no>, or <frotz/xyzzy>. And when that happens, <new/old> will
> continue to be supported for free--there is no extra work to support
> <new/old> in addition to the work needed to support <good/bad> and
> <arbitrary1/arbitrary2> that we need to support anyway.
Except that most of the work to support <arbitrary1/arbitrary2> is
already being done in this patch to add <new/old>, but it will need
re-doing or un-doing to move to arbitrary terms. Supporting arbitrary
terms today would save work (the re-doing and the un-doring), if we
went that way in the future.
But perhaps we won't.
Phil
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv2] git bisect old/new
2012-06-15 20:20 ` Phil Hord
@ 2012-06-15 21:06 ` Junio C Hamano
0 siblings, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2012-06-15 21:06 UTC (permalink / raw)
To: Phil Hord
Cc: Christian Couder, duperrav, Valentin Duperray, git, Franck Jonas,
Lucien Kong, Thomas Nguy, Huynh Khoi Nguyen Nguyen, Matthieu Moy,
Peff
Phil Hord <phil.hord@gmail.com> writes:
> Except that most of the work to support <arbitrary1/arbitrary2> is
> already being done in this patch to add <new/old>, but it will need
> re-doing or un-doing to move to arbitrary terms. Supporting arbitrary
> terms today would save work (the re-doing and the un-doring), if we
> went that way in the future.
You are forgetting that "somebody wrote the code" does not matter
much. What counds more is that the code is more complex than
necessary to support the case we are immediately interested in,
which means it is much more likely to have undiscovered bugs and
will take more effort to update if we need to change the aspects
other than "arbitrary two tokens" later. "Somebody wrote the code
already" matters only when we are sure that we want to build on top
of it. In this case, we don't.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2012-06-15 21:11 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-12 2:03 [PATCH] git bisect old/new Valentin Duperray
2012-06-12 5:25 ` Christian Couder
2012-06-12 5:43 ` Junio C Hamano
2012-06-12 19:41 ` Phil Hord
2012-06-13 10:12 ` Christian Couder
2012-06-13 17:30 ` Junio C Hamano
2012-06-12 22:56 ` [PATCHv2] " Valentin Duperray
2012-06-12 23:54 ` Junio C Hamano
2012-06-13 18:06 ` duperrav
2012-06-14 9:56 ` Christian Couder
2012-06-14 17:34 ` Junio C Hamano
2012-06-15 20:20 ` Phil Hord
2012-06-15 21:06 ` Junio C Hamano
2012-06-13 10:05 ` Christian Couder
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.