{"thread":{"id":"30775","subject":"[PATCH] git bisect old/new","startedAt":"2012-06-12T02:03:45Z","lastAt":"2012-06-15T21:06:30Z","messageCount":14,"participants":["Valentin Duperray","Christian Couder","Junio C Hamano","Phil Hord","duperrav@minatec.inpg.fr"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"193409","messageId":"1339466625-17461-1-git-send-email-Valentin.Duperray@ensimag.imag.fr","threadId":"30775","inReplyTo":null,"subject":"[PATCH] git bisect old/new","fromName":"Valentin Duperray","fromEmail":"valentin.duperray@ensimag.imag.fr","sentAt":"2012-06-12T02:03:45Z","receivedAt":"2012-06-12T02:03:45Z","isPatch":true,"sender":{"key":"valentin.duperray@ensimag.imag.fr","avatar":null},"body":"When not looking for a regression during a bisect but for a fix or a\nchange in another given property, it can be confusing to use 'good'\nand 'bad'.\nThis patch introduce `git bisect new` and `git bisect old` as an\nalternative to 'bad' and good' : the commits which have the most\nrecent version of the property must be marked as `new` and the ones\nwith the older version as `old`.\nThe output will be the first commit after the change in the property.\nDuring a new/old bisect session you cannot use bad/good commands and\nvice-versa.\nSome commands are still not available for old/new:\n * git bisect start [ [...]] is not possible : the\n commits will be treated as bad and good.\n * git rev-list --bisect does not treat the revs/bisect/new and\n revs/bisect/old-SHA1 files.\n * thus, git bisect run is not available for new/old.\n * git bisect visualize seem to work partially : the tags are\n displayed correctly but the tree is not limited to the bisect\n section.\n\nSigned-off-by: Valentin Duperray \nSigned-off-by: Lucien Kong \nSigned-off-by: Franck Jonas \nSigned-off-by: Thomas Nguy \nSigned-off-by: Huynh Khoi Nguyen Nguyen \nSigned-off-by: Matthieu Moy \n---\n Documentation/git-bisect.txt | 40 +++++++++++++++\n bisect.c | 88 ++++++++++++++++++++++++---------\n git-bisect.sh | 113 ++++++++++++++++++++++++++++++++---------\n t/t6030-bisect-porcelain.sh | 33 ++++++++++++\n 4 files changed, 226 insertions(+), 48 deletions(-)\n\ndiff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt\nindex e4f46bc..25673c9 100644\n--- a/Documentation/git-bisect.txt\n+++ b/Documentation/git-bisect.txt\n@@ -20,6 +20,8 @@ on the subcommand:\n git bisect start [--no-checkout] [ [...]] [--] [...]\n git bisect bad []\n git bisect good [...]\n+ git bisect new []\n+ git bisect old [...]\n git bisect skip [(|)...]\n git bisect reset []\n git bisect visualize\n@@ -104,6 +106,44 @@ For example, `git bisect reset HEAD` will leave you on the current\n bisection commit and avoid switching commits at all, while `git bisect\n reset bisect/bad` will check out the first bad revision.\n \n+Alternative research: bisect new and bisect old\n+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n+\n+If you are not looking for a regression but for a change of a given\n+property, you can use:\n+\n+------------------------------------------------\n+git bisect new []\n+------------------------------------------------\n+\n+Mark the commits that have the new version of the property.\n+\n+------------------------------------------------\n+git bisect old [...]\n+------------------------------------------------\n+\n+Mark the commits that have the old version of the property.\n+\n+For example, when looking for a fix in the code, the \"new\" commits are\n+the fixed ones and the \"old\" commits are the unfixed ones.\n+\n+------------------------------------------------\n+$ git bisect start\n+$ git bisect new\t\t# Current version is fixed\n+$ git bisect old bugged_version\t# bugged_version was the last version\n+\t\t\t\t# known to be unfixed\n+------------------------------------------------\n+\n+At the end of the commit session, you will have the first commit that\n+have the new version of the property (\"fixed\" here).\n+\n+You must run `git bisect start` without commits as argument and run\n+`git bisect new `/`git bisect old ...` after to add the\n+commits.\n+The bisect old/new sessions and the good/bad ones cannot be mixed.\n+You must use `git bisect reset` and start again in order to change\n+the mode.\n+\n Bisect visualize\n ~~~~~~~~~~~~~~~~\n \ndiff --git a/bisect.c b/bisect.c\nindex 48acf73..474b615 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -21,6 +21,9 @@ static const char *argv_checkout[] = {\"checkout\", \"-q\", NULL, \"--\", NULL};\n static const char *argv_show_branch[] = {\"show-branch\", NULL, NULL};\n static const char *argv_update_ref[] = {\"update-ref\", \"--no-deref\", \"BISECT_HEAD\", NULL, NULL};\n \n+static const char *bisect_term_bad;\n+static const char *bisect_term_good;\n+\n /* bits #0-15 in revision.h */\n \n #define COUNTED\t\t(1u<<16)\n@@ -403,9 +406,10 @@ struct commit_list *find_bisection(struct commit_list *list,\n static int register_ref(const char *refname, const unsigned char *sha1,\n \t\t\tint flags, void *cb_data)\n {\n-\tif (!strcmp(refname, \"bad\")) {\n+\tif (!strcmp(refname, bisect_term_bad)) {\n \t\tcurrent_bad_sha1 = sha1;\n-\t} else if (!prefixcmp(refname, \"good-\")) {\n+\t} else if (!prefixcmp(refname, \"good-\") ||\n+\t\t\t!prefixcmp(refname, \"old-\")) {\n \t\tsha1_array_append(&good_revs, sha1);\n \t} else if (!prefixcmp(refname, \"skip-\")) {\n \t\tsha1_array_append(&skipped_revs, sha1);\n@@ -633,7 +637,7 @@ static void exit_if_skipped_commits(struct commit_list *tried,\n \t\treturn;\n \n \tprintf(\"There are only 'skip'ped commits left to test.\\n\"\n-\t \"The first bad commit could be any of:\\n\");\n+\t \"The first %s commit could be any of:\\n\", bisect_term_bad);\n \tprint_commit_list(tried, \"%s\\n\", \"%s\\n\");\n \tif (bad)\n \t\tprintf(\"%s\\n\", sha1_to_hex(bad));\n@@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)\n \tif (is_expected_rev(current_bad_sha1)) {\n \t\tchar *bad_hex = sha1_to_hex(current_bad_sha1);\n \t\tchar *good_hex = join_sha1_array_hex(&good_revs, ' ');\n-\n-\t\tfprintf(stderr, \"The merge base %s is bad.\\n\"\n-\t\t\t\"This means the bug has been fixed \"\n-\t\t\t\"between %s and [%s].\\n\",\n-\t\t\tbad_hex, bad_hex, good_hex);\n-\n+\t\tif (!strcmp(bisect_term_bad,\"bad\")) {\n+\t\t\tfprintf(stderr, \"The merge base %s is bad.\\n\"\n+\t\t\t\t\"This means the bug has been fixed \"\n+\t\t\t\t\"between %s and [%s].\\n\",\n+\t\t\t\tbad_hex, bad_hex, good_hex);\n+\t\t} else {\n+\t\t\tfprintf(stderr, \"The merge base %s is new.\\n\"\n+\t\t\t\t\"The property has changed \"\n+\t\t\t\t\"between %s and [%s].\\n\",\n+\t\t\t\tbad_hex, bad_hex, good_hex);\n+\t\t}\n \t\texit(3);\n \t}\n \n-\tfprintf(stderr, \"Some good revs are not ancestor of the bad rev.\\n\"\n+\tfprintf(stderr, \"Some %s revs are not ancestor of the %s rev.\\n\"\n \t\t\"git bisect cannot work properly in this case.\\n\"\n-\t\t\"Maybe you mistake good and bad revs?\\n\");\n+\t\t\"Maybe you mistake %s and %s revs?\\n\",\n+\t\tbisect_term_good, bisect_term_bad, bisect_term_good,\n+\t\tbisect_term_bad);\n \texit(1);\n }\n \n@@ -754,19 +765,19 @@ static void handle_skipped_merge_base(const unsigned char *mb)\n \n \twarning(\"the merge base between %s and [%s] \"\n \t\t\"must be skipped.\\n\"\n-\t\t\"So we cannot be sure the first bad commit is \"\n+\t\t\"So we cannot be sure the first %s commit is \"\n \t\t\"between %s and %s.\\n\"\n \t\t\"We continue anyway.\",\n-\t\tbad_hex, good_hex, mb_hex, bad_hex);\n+\t\tbad_hex, good_hex, bisect_term_bad, mb_hex, bad_hex);\n \tfree(good_hex);\n }\n \n /*\n- * \"check_merge_bases\" checks that merge bases are not \"bad\".\n+ * \"check_merge_bases\" checks that merge bases are not \"bad\" (resp. \"new\").\n *\n- * - If one is \"bad\", it means the user assumed something wrong\n+ * - If one is \"bad\" (resp. \"new\"), it means the user assumed something wrong\n * and we must exit with a non 0 error code.\n- * - If one is \"good\", that's good, we have nothing to do.\n+ * - If one is \"good\" (resp. \"old\"), that's good, we have nothing to do.\n * - If one is \"skipped\", we can't know but we should warn.\n * - If we don't know, we should check it out and ask the user to test.\n */\n@@ -825,7 +836,8 @@ static int check_ancestors(const char *prefix)\n \n /*\n * \"check_good_are_ancestors_of_bad\" checks that all \"good\" revs are\n- * ancestor of the \"bad\" rev.\n+ * ancestor of the \"bad\" rev. (resp. all \"old\" revs are ancestor of\n+ * the \"new\" rev).\n *\n * If that's not the case, we need to check the merge bases.\n * If a merge base must be tested by the user, its source code will be\n@@ -838,7 +850,7 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)\n \tint fd;\n \n \tif (!current_bad_sha1)\n-\t\tdie(\"a bad revision is needed\");\n+\t\tdie(\"a %s revision is needed\", bisect_term_bad);\n \n \t/* Check if file BISECT_ANCESTORS_OK exists. */\n \tif (!stat(filename, &st) && S_ISREG(st.st_mode))\n@@ -889,6 +901,30 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n }\n \n /*\n+ * The terms used for this bisect session are stocked in\n+ * BISECT_TERMS: it can be bad/good or new/old.\n+ * We read them and stock them to adapt the messages\n+ * accordingly.\n+ */\n+void read_bisect_terms(void)\n+{\n+\tstruct strbuf str = STRBUF_INIT;\n+\tconst char *filename = git_path(\"BISECT_TERMS\");\n+\tFILE *fp = fopen(filename, \"r\");\n+\n+\tif (!fp)\n+\t\tdie_errno(\"Could not open file '%s'\", filename);\n+\n+\tstrbuf_getline(&str, fp, '\\n');\n+\tbisect_term_bad = strbuf_detach(&str, NULL);\n+\tstrbuf_getline(&str, fp, '\\n');\n+\tbisect_term_good = strbuf_detach(&str, NULL);\n+\n+\tstrbuf_release(&str);\n+\tfclose(fp);\n+}\n+\n+/*\n * We use the convention that exiting with an exit code 10 means that\n * the bisection process finished successfully.\n * In this case the calling shell script should exit 0.\n@@ -898,6 +934,8 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n */\n int bisect_next_all(const char *prefix, int no_checkout)\n {\n+\tread_bisect_terms();\n+\n \tstruct rev_info revs;\n \tstruct commit_list *tried;\n \tint reaches = 0, all = 0, nr, steps;\n@@ -920,13 +958,14 @@ int bisect_next_all(const char *prefix, int no_checkout)\n \n \tif (!revs.commits) {\n \t\t/*\n-\t\t * We should exit here only if the \"bad\"\n+\t\t * We should exit here only if the \"bad\" (or \"new\")\n \t\t * commit is also a \"skip\" commit.\n \t\t */\n \t\texit_if_skipped_commits(tried, NULL);\n \n-\t\tprintf(\"%s was both good and bad\\n\",\n-\t\t sha1_to_hex(current_bad_sha1));\n+\t\tprintf(\"%s was both %s and %s\\n\",\n+\t\t sha1_to_hex(current_bad_sha1), bisect_term_good,\n+\t\t bisect_term_bad);\n \t\texit(1);\n \t}\n \n@@ -941,7 +980,8 @@ int bisect_next_all(const char *prefix, int no_checkout)\n \n \tif (!hashcmp(bisect_rev, current_bad_sha1)) {\n \t\texit_if_skipped_commits(tried, current_bad_sha1);\n-\t\tprintf(\"%s is the first bad commit\\n\", bisect_rev_hex);\n+\t\tprintf(\"%s is the first %s commit\\n\", bisect_rev_hex,\n+\t\t\tbisect_term_bad);\n \t\tshow_diff_tree(prefix, revs.commits->item);\n \t\t/* This means the bisection process succeeded. */\n \t\texit(10);\n@@ -953,6 +993,8 @@ int bisect_next_all(const char *prefix, int no_checkout)\n \t \"(roughly %d step%s)\\n\", nr, (nr == 1 ? \"\" : \"s\"),\n \t steps, (steps == 1 ? \"\" : \"s\"));\n \n+\tfree((char*)bisect_term_bad);\n+\tfree((char*)bisect_term_good);\n+\n \treturn bisect_checkout(bisect_rev_hex, no_checkout);\n }\n-\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 99efbe8..152b4f3 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n \n-USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run]'\n+USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run|new|old]'\n LONG_USAGE='git bisect help\n \tprint this long help message.\n git bisect start [--no-checkout] [ [...]] [--] [...]\n@@ -22,7 +22,15 @@ git bisect replay \n git bisect log\n \tshow bisect log.\n git bisect run ...\n-\tuse ... to automatically bisect.\n+\tuse ... to automatically bisect\n+\n+When looking for a change in a given property instead of a regression\n+you can use\n+\n+git bisect new []\n+\tmark as not having the property anymore\n+git bisect old []\n+\tmark ... as having the property\n \n Please use \"git help bisect\" to get the full man page.'\n \n@@ -32,6 +40,8 @@ OPTIONS_SPEC=\n \n _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'\n _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n+NEW=\"bad\"\n+OLD=\"good\"\n \n bisect_head()\n {\n@@ -66,7 +76,7 @@ bisect_autostart() {\n \n bisect_start() {\n \t#\n-\t# Check for one bad and then some good revisions.\n+\t# Check for one bad (or new) and then some good (or old) revisions.\n \t#\n \thas_double_dash=0\n \tfor arg; do\n@@ -75,6 +85,7 @@ bisect_start() {\n \torig_args=$(git rev-parse --sq-quote \"$@\")\n \tbad_seen=0\n \teval=''\n+\tstart_bad_good=0\n \tif test \"z$(git rev-parse --is-bare-repository)\" != zfalse\n \tthen\n \t\tmode=--no-checkout\n@@ -99,6 +110,16 @@ bisect_start() {\n \t\t\t\tdie \"$(eval_gettext \"'\\$arg' does not appear to be a valid revision\")\"\n \t\t\t\tbreak\n \t\t\t}\n+\n+\t\t\tif test -s \"$GIT_DIR/BISECT_TERMS\"\n+\t\t\tthen\n+\t\t\t\tif $(sed -n 1p \"$GIT_DIR/BISECT_TERMS\") != 'bad'\n+\t\t\t\tthen\n+\t\t\t\t\tdie \"$(gettext \"you are already bisecting in old/new mode\")\"\n+\t\t\t\tfi\n+\t\t\tfi\n+\t\t\tstart_bad_good=1\n+\n \t\t\tcase $bad_seen in\n \t\t\t0) state='bad' ; bad_seen=1 ;;\n \t\t\t*) state='good' ;;\n@@ -170,6 +191,11 @@ bisect_start() {\n \t} &&\n \tgit rev-parse --sq-quote \"$@\" >\"$GIT_DIR/BISECT_NAMES\" &&\n \teval \"$eval true\" &&\n+\tif test $start_bad_good -eq 1 -a ! -s \"$GIT_DIR/BISECT_TERMS\"\n+\tthen\n+\t\techo \"bad\" >\"$GIT_DIR/BISECT_TERMS\" &&\n+\t\techo \"good\" >>\"$GIT_DIR/BISECT_TERMS\"\n+\tfi &&\n \techo \"git bisect start$orig_args\" >>\"$GIT_DIR/BISECT_LOG\" || exit\n \t#\n \t# Check if we can proceed to the next bisect state.\n@@ -184,8 +210,8 @@ bisect_write() {\n \trev=\"$2\"\n \tnolog=\"$3\"\n \tcase \"$state\" in\n-\t\tbad)\t\ttag=\"$state\" ;;\n-\t\tgood|skip)\ttag=\"$state\"-\"$rev\" ;;\n+\t\tbad|new)\t\ttag=\"$state\" ;;\n+\t\tgood|skip|old)\ttag=\"$state\"-\"$rev\" ;;\n \t\t*)\t\tdie \"$(eval_gettext \"Bad bisect_write argument: \\$state\")\" ;;\n \tesac\n \tgit update-ref \"refs/bisect/$tag\" \"$rev\" || exit\n@@ -230,12 +256,12 @@ bisect_state() {\n \tcase \"$#,$state\" in\n \t0,*)\n \t\tdie \"$(gettext \"Please call 'bisect_state' with at least one argument.\")\" ;;\n-\t1,bad|1,good|1,skip)\n+\t1,bad|1,good|1,skip|1,new|1,old)\n \t\trev=$(git rev-parse --verify $(bisect_head)) ||\n \t\t\tdie \"$(gettext \"Bad rev input: $(bisect_head)\")\"\n \t\tbisect_write \"$state\" \"$rev\"\n \t\tcheck_expected_revs \"$rev\" ;;\n-\t2,bad|*,good|*,skip)\n+\t2,bad|*,good|*,skip|2,new|*,old)\n \t\tshift\n \t\teval=''\n \t\tfor rev in \"$@\"\n@@ -246,8 +272,8 @@ bisect_state() {\n \t\tdone\n \t\teval \"$eval\"\n \t\tcheck_expected_revs \"$@\" ;;\n-\t*,bad)\n-\t\tdie \"$(gettext \"'git bisect bad' can take only one argument.\")\" ;;\n+\t*,bad|*,new)\n+\t\tdie \"$(gettext \"'git bisect $NEW' can take only one argument.\")\" ;;\n \t*)\n \t\tusage ;;\n \tesac\n@@ -256,21 +282,21 @@ bisect_state() {\n \n bisect_next_check() {\n \tmissing_good= missing_bad=\n-\tgit show-ref -q --verify refs/bisect/bad || missing_bad=t\n-\ttest -n \"$(git for-each-ref \"refs/bisect/good-*\")\" || missing_good=t\n+\tgit show-ref -q --verify refs/bisect/$NEW || missing_bad=t\n+\ttest -n \"$(git for-each-ref \"refs/bisect/$OLD-*\")\" || missing_good=t\n \n \tcase \"$missing_good,$missing_bad,$1\" in\n \t,,*)\n-\t\t: have both good and bad - ok\n+\t\t: have both good and bad or old and new - ok\n \t\t;;\n \t*,)\n \t\t# do not have both but not asked to fail - just report.\n \t\tfalse\n \t\t;;\n-\tt,,good)\n-\t\t# have bad but not good. we could bisect although\n+\tt,,good|t,,old)\n+\t\t# have bad (or new) but not good (or old). we could bisect although\n \t\t# this is less optimum.\n-\t\tgettextln \"Warning: bisecting only with a bad commit.\" >&2\n+\t\tgettextln \"Warning: bisecting only with a $NEW commit.\" >&2\n \t\tif test -t 0\n \t\tthen\n \t\t\t# TRANSLATORS: Make sure to include [Y] and [n] in your\n@@ -280,17 +306,17 @@ bisect_next_check() {\n \t\t\tread yesno\n \t\t\tcase \"$yesno\" in [Nn]*) exit 1 ;; esac\n \t\tfi\n-\t\t: bisect without good...\n+\t\t: bisect without $OLD...\n \t\t;;\n \t*)\n \n \t\tif test -s \"$GIT_DIR/BISECT_START\"\n \t\tthen\n-\t\t\tgettextln \"You need to give me at least one good and one bad revisions.\n+\t\t\tgettextln \"You need to give me at least one good and one bad (or one old and one new) revisions.\n (You can use \\\"git bisect bad\\\" and \\\"git bisect good\\\" for that.)\" >&2\n \t\telse\n \t\t\tgettextln \"You need to start by \\\"git bisect start\\\".\n-You then need to give me at least one good and one bad revisions.\n+You then need to give me at least one good and one bad (or one old and one new) revisions.\n (You can use \\\"git bisect bad\\\" and \\\"git bisect good\\\" for that.)\" >&2\n \t\tfi\n \t\texit 1 ;;\n@@ -304,7 +330,7 @@ bisect_auto_next() {\n bisect_next() {\n \tcase \"$#\" in 0) ;; *) usage ;; esac\n \tbisect_autostart\n-\tbisect_next_check good\n+\tbisect_next_check $OLD\n \n \t# Perform all bisection computation, display and checkout\n \tgit bisect--helper --next-all $(test -f \"$GIT_DIR/BISECT_HEAD\" && echo --no-checkout)\n@@ -378,6 +404,7 @@ bisect_clean_state() {\n \trm -f \"$GIT_DIR/BISECT_LOG\" &&\n \trm -f \"$GIT_DIR/BISECT_NAMES\" &&\n \trm -f \"$GIT_DIR/BISECT_RUN\" &&\n+\trm -f \"$GIT_DIR/BISECT_TERMS\" &&\n \t# Cleanup head-name if it got left by an old version of git-bisect\n \trm -f \"$GIT_DIR/head-name\" &&\n \tgit update-ref -d --no-deref BISECT_HEAD &&\n@@ -402,7 +429,7 @@ bisect_replay () {\n \t\tstart)\n \t\t\tcmd=\"bisect_start $rev\"\n \t\t\teval \"$cmd\" ;;\n-\t\tgood|bad|skip)\n+\t\tgood|bad|skip|old|new)\n \t\t\tbisect_write \"$command\" \"$rev\" ;;\n \t\t*)\n \t\t\tdie \"$(gettext \"?? what are you talking about?\")\" ;;\n@@ -436,9 +463,9 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n \t\t\tstate='skip'\n \t\telif [ $res -gt 0 ]\n \t\tthen\n-\t\t\tstate='bad'\n+\t\t\tstate=\"$NEW\"\n \t\telse\n-\t\t\tstate='good'\n+\t\t\tstate=\"$OLD\"\n \t\tfi\n \n \t\t# We have to use a subshell because \"bisect_state\" can exit.\n@@ -447,7 +474,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n \n \t\tcat \"$GIT_DIR/BISECT_RUN\"\n \n-\t\tif sane_grep \"first bad commit could be any of\" \"$GIT_DIR/BISECT_RUN\" \\\n+\t\tif sane_grep \"first $NEW commit could be any of\" \"$GIT_DIR/BISECT_RUN\" \\\n \t\t\t> /dev/null\n \t\tthen\n \t\t\tgettextln \"bisect run cannot continue any more\" >&2\n@@ -461,7 +488,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n \t\t\texit $res\n \t\tfi\n \n-\t\tif sane_grep \"is the first bad commit\" \"$GIT_DIR/BISECT_RUN\" > /dev/null\n+\t\tif sane_grep \"is the first $NEW commit\" \"$GIT_DIR/BISECT_RUN\" > /dev/null\n \t\tthen\n \t\t\tgettextln \"bisect run success\"\n \t\t\texit 0;\n@@ -475,18 +502,54 @@ bisect_log () {\n \tcat \"$GIT_DIR/BISECT_LOG\"\n }\n \n+get_mode () {\n+\tif test -s \"$GIT_DIR/BISECT_TERMS\"\n+\tthen\n+\t\tNEW=\"$(sed -n 1p \"$GIT_DIR/BISECT_TERMS\")\"\n+\t\tOLD=\"$(sed -n 2p \"$GIT_DIR/BISECT_TERMS\")\"\n+\tfi\n+}\n+\n case \"$#\" in\n 0)\n \tusage ;;\n *)\n \tcmd=\"$1\"\n+\tget_mode\n+\tcase \"$cmd\" in\n+\tbad|good|new|old)\n+\t\tif test -s \"$GIT_DIR/BISECT_TERMS\" -a \"$cmd\" != \"$NEW\" -a \"$cmd\" != \"$OLD\"\n+\t\tthen\n+\t\t\tdie \"$(eval_gettext \"Invalid command : you're currently in a \\$NEW/\\$OLD bisect.\")\"\n+\t\tfi\n+\t\tcase \"$cmd\" in\n+\t\tbad|good)\n+\t\t\tif test ! -s \"$GIT_DIR/BISECT_TERMS\"\n+\t\t\tthen\n+\t\t\t\techo \"bad\" >\"$GIT_DIR/BISECT_TERMS\" &&\n+\t\t\t\techo \"good\" >>\"$GIT_DIR/BISECT_TERMS\"\n+\t\t\tfi\n+\t\t\tNEW=\"bad\"\n+\t\t\tOLD=\"good\";;\n+\t\tnew|old)\n+\t\t\tif test ! -s \"$GIT_DIR/BISECT_TERMS\"\n+\t\t\tthen\n+\t\t\t\techo \"new\" >\"$GIT_DIR/BISECT_TERMS\" &&\n+\t\t\t\techo \"old\" >>\"$GIT_DIR/BISECT_TERMS\"\n+\t\t\tfi\n+\t\t\tNEW=\"new\"\n+\t\t\tOLD=\"old\";;\n+\n+\n+\t\tesac;;\n+\tesac\n \tshift\n \tcase \"$cmd\" in\n \thelp)\n \t\tgit bisect -h ;;\n \tstart)\n \t\tbisect_start \"$@\" ;;\n-\tbad|good)\n+\tbad|good|new|old)\n \t\tbisect_state \"$cmd\" \"$@\" ;;\n \tskip)\n \t\tbisect_skip \"$@\" ;;\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 72e28ee..28a9c95 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -741,6 +741,39 @@ test_expect_success 'bisect: demonstrate identification of damage boundary' \"\n \t\ttest \\$rc = 0' &&\n \tcheck_same BROKEN_HASH6 bisect/bad &&\n \tgit bisect reset\n+\tgit checkout master\n+\n \"\n \n+test_expect_success 'bisect starts with only one new' '\n+\tgit bisect reset &&\n+\tgit bisect start &&\n+\tgit bisect new $HASH4 &&\n+\tgit bisect next\n+'\n+test_expect_success 'bisect does not start with only one old' '\n+\tgit bisect reset &&\n+\tgit bisect start &&\n+\tgit bisect old $HASH1 || return 1\n+\ttest_must_fail git bisect next\n+\n+'\n+\n+test_expect_success 'bisect start with one new and old' '\n+\tgit bisect reset &&\n+\tgit bisect start &&\n+\tgit bisect old $HASH1 &&\n+\tgit bisect new $HASH4 &&\n+\tgit bisect new &&\n+\tgit bisect new >bisect_result &&\n+\tgrep \"$HASH2 is the first new commit\" bisect_result &&\n+\tgit bisect reset\n+'\n+\n+test_expect_success 'bisect cannot mix old/new and good/bad' '\n+\tgit bisect start &&\n+\tgit bisect bad $HASH4 &&\n+\ttest_must_fail git bisect old $HASH1\n+'\n+\n test_done\n-- \n1.7.8\n"},{"id":"193414","messageId":"CAP8UFD1mgKNQRZpuqYgyL__DGpoZ3cOGJC0amoqWT+1Bn_0iTg@mail.gmail.com","threadId":"30775","inReplyTo":"1339466625-17461-1-git-send-email-Valentin.Duperray@ensimag.imag.fr","subject":"Re: [PATCH] git bisect old/new","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2012-06-12T05:25:07Z","receivedAt":"2012-06-12T05:25:07Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Tue, Jun 12, 2012 at 4:03 AM, Valentin Duperray\n wrote:\n> When not looking for a regression during a bisect but for a fix or a\n> change in another given property, it can be confusing to use 'good'\n> and 'bad'.\n> This patch introduce `git bisect new` and `git bisect old` as an\n> alternative to 'bad' and good' : the commits which have the most\n> recent version of the property must be marked as `new` and the ones\n> with the older version as `old`.\n> The output will be the first commit after the change in the property.\n> During a new/old bisect session you cannot use bad/good commands and\n> vice-versa.\n> Some commands are still not available for old/new:\n>     * git bisect start [ [...]] is not possible : the\n>       commits will be treated as bad and good.\n>     * git rev-list --bisect does not treat the revs/bisect/new and\n>       revs/bisect/old-SHA1 files.\n>     * thus, git bisect run is not available for new/old.\n>     * git bisect visualize seem to work partially : the tags are\n>       displayed correctly but the tree is not limited to the bisect\n>       section.\n>\n> Signed-off-by: Valentin Duperray \n> Signed-off-by: Lucien Kong \n> Signed-off-by: Franck Jonas \n> Signed-off-by: Thomas Nguy \n> Signed-off-by: Huynh Khoi Nguyen Nguyen \n> Signed-off-by: Matthieu Moy \n\nIf you used some design that was discussed on the mailing list or if\nthere have been relevant discussions on the mailing list, it would be\nnice to have links to the email thread in the commit message.\n\n> ---\n>  Documentation/git-bisect.txt |   40 +++++++++++++++\n>  bisect.c                     |   88 ++++++++++++++++++++++++---------\n>  git-bisect.sh                |  113 ++++++++++++++++++++++++++++++++---------\n>  t/t6030-bisect-porcelain.sh  |   33 ++++++++++++\n>  4 files changed, 226 insertions(+), 48 deletions(-)\n>\n> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt\n> index e4f46bc..25673c9 100644\n> --- a/Documentation/git-bisect.txt\n> +++ b/Documentation/git-bisect.txt\n> @@ -20,6 +20,8 @@ on the subcommand:\n>  git bisect start [--no-checkout] [ [...]] [--] [...]\n>  git bisect bad []\n>  git bisect good [...]\n> + git bisect new []\n> + git bisect old [...]\n\nmaybe:\n\ngit bisect (bad|new) []\ngit bisect (good|old) [...]\n\n>  git bisect skip [(|)...]\n>  git bisect reset []\n>  git bisect visualize\n> @@ -104,6 +106,44 @@ For example, `git bisect reset HEAD` will leave you on the current\n>  bisection commit and avoid switching commits at all, while `git bisect\n>  reset bisect/bad` will check out the first bad revision.\n>\n> +Alternative research: bisect new and bisect old\n> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> +\n> +If you are not looking for a regression but for a change of a given\n> +property, you can use:\n\nI would rather say:\n\nAlternative terms: bisect new and bisect old\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\nIf you are not at ease with the terms \"bad\" and \"good\", perhaps\nbecause you are looking for the commit that introduced a fix, you can\nalternatively use \"new\" and \"old\" instead.\nBut note that you cannot mix \"bad\" and good\" with \"new\" and \"old\".\n\n------------------------------------------------\ngit bisect new []\n------------------------------------------------\n\nSame as \"git bisect bad []\".\n\n------------------------------------------------\ngit bisect old [...]\n------------------------------------------------\n\nSame as \"git bisect good [...]\".\n\n> +\n> +------------------------------------------------\n> +git bisect new []\n> +------------------------------------------------\n> +\n> +Mark the commits that have the new version of the property.\n> +\n> +------------------------------------------------\n> +git bisect old [...]\n> +------------------------------------------------\n> +\n> +Mark the commits that have the old version of the property.\n> +\n> +For example, when looking for a fix in the code, the \"new\" commits are\n> +the fixed ones and the \"old\" commits are the unfixed ones.\n\nPlease put this in the example section of the doc.\n\n> +------------------------------------------------\n> +$ git bisect start\n> +$ git bisect new               # Current version is fixed\n> +$ git bisect old bugged_version        # bugged_version was the last version\n> +                               # known to be unfixed\n> +------------------------------------------------\n> +\n> +At the end of the commit session, you will have the first commit that\n> +have the new version of the property (\"fixed\" here).\n> +\n> +You must run `git bisect start` without commits as argument and run\n> +`git bisect new `/`git bisect old ...` after to add the\n> +commits.\n> +The bisect old/new sessions and the good/bad ones cannot be mixed.\n> +You must use `git bisect reset` and start again in order to change\n> +the mode.\n> +\n>  Bisect visualize\n>  ~~~~~~~~~~~~~~~~\n>\n> diff --git a/bisect.c b/bisect.c\n> index 48acf73..474b615 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -21,6 +21,9 @@ static const char *argv_checkout[] = {\"checkout\", \"-q\", NULL, \"--\", NULL};\n>  static const char *argv_show_branch[] = {\"show-branch\", NULL, NULL};\n>  static const char *argv_update_ref[] = {\"update-ref\", \"--no-deref\", \"BISECT_HEAD\", NULL, NULL};\n>\n> +static const char *bisect_term_bad;\n> +static const char *bisect_term_good;\n> +\n>  /* bits #0-15 in revision.h */\n>\n>  #define COUNTED                (1u<<16)\n> @@ -403,9 +406,10 @@ struct commit_list *find_bisection(struct commit_list *list,\n>  static int register_ref(const char *refname, const unsigned char *sha1,\n>                        int flags, void *cb_data)\n>  {\n> -       if (!strcmp(refname, \"bad\")) {\n> +       if (!strcmp(refname, bisect_term_bad)) {\n>                current_bad_sha1 = sha1;\n> -       } else if (!prefixcmp(refname, \"good-\")) {\n> +       } else if (!prefixcmp(refname, \"good-\") ||\n> +                       !prefixcmp(refname, \"old-\")) {\n\nI don't like very much \"good\" and \"old\" to be hardcoded here.\n\n> @@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)\n>        if (is_expected_rev(current_bad_sha1)) {\n>                char *bad_hex = sha1_to_hex(current_bad_sha1);\n>                char *good_hex = join_sha1_array_hex(&good_revs, ' ');\n> -\n> -               fprintf(stderr, \"The merge base %s is bad.\\n\"\n> -                       \"This means the bug has been fixed \"\n> -                       \"between %s and [%s].\\n\",\n> -                       bad_hex, bad_hex, good_hex);\n> -\n> +               if (!strcmp(bisect_term_bad,\"bad\")) {\n> +                       fprintf(stderr, \"The merge base %s is bad.\\n\"\n> +                               \"This means the bug has been fixed \"\n> +                               \"between %s and [%s].\\n\",\n> +                               bad_hex, bad_hex, good_hex);\n> +               } else {\n> +                       fprintf(stderr, \"The merge base %s is new.\\n\"\n> +                               \"The property has changed \"\n> +                               \"between %s and [%s].\\n\",\n> +                               bad_hex, bad_hex, good_hex);\n> +               }\n\nI don't like very much \"new\" to be harcoded here too.\n\n>\n>  /*\n> - * \"check_merge_bases\" checks that merge bases are not \"bad\".\n> + * \"check_merge_bases\" checks that merge bases are not \"bad\" (resp. \"new\").\n>  *\n> - * - If one is \"bad\", it means the user assumed something wrong\n> + * - If one is \"bad\" (resp. \"new\"), it means the user assumed something wrong\n>  * and we must exit with a non 0 error code.\n> - * - If one is \"good\", that's good, we have nothing to do.\n> + * - If one is \"good\" (resp. \"old\"), that's good, we have nothing to do.\n>  * - If one is \"skipped\", we can't know but we should warn.\n>  * - If we don't know, we should check it out and ask the user to test.\n>  */\n\nI am not sure changing the comments is worth it...\n\n> @@ -825,7 +836,8 @@ static int check_ancestors(const char *prefix)\n>\n>  /*\n>  * \"check_good_are_ancestors_of_bad\" checks that all \"good\" revs are\n> - * ancestor of the \"bad\" rev.\n> + * ancestor of the \"bad\" rev. (resp. all \"old\" revs are ancestor of\n> + * the \"new\" rev).\n\n...because we don't change the name of the function anyway.\n\n>  *\n>  * If that's not the case, we need to check the merge bases.\n>  * If a merge base must be tested by the user, its source code will be\n> @@ -838,7 +850,7 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)\n>        int fd;\n>\n>        if (!current_bad_sha1)\n> -               die(\"a bad revision is needed\");\n> +               die(\"a %s revision is needed\", bisect_term_bad);\n>\n>        /* Check if file BISECT_ANCESTORS_OK exists. */\n>        if (!stat(filename, &st) && S_ISREG(st.st_mode))\n> @@ -889,6 +901,30 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n>  }\n>\n>  /*\n> + * The terms used for this bisect session are stocked in\n> + * BISECT_TERMS: it can be bad/good or new/old.\n\nI am not sure saying \"it can be bad/good or new/old\" adds anything.\nYou could just say: the defaults are \"bad\"/\"good\"\n\n> + * We read them and stock them to adapt the messages\n> + * accordingly.\n> + */\n> +void read_bisect_terms(void)\n> +{\n> +       struct strbuf str = STRBUF_INIT;\n> +       const char *filename = git_path(\"BISECT_TERMS\");\n> +       FILE *fp = fopen(filename, \"r\");\n> +\n> +       if (!fp)\n> +               die_errno(\"Could not open file '%s'\", filename);\n\nThis is not very compatible with older git versions.\nI know that it's kind of strange to upgrade git in the middle of a\nbisection but why not just use \"bad\"/\"good\" if there is no file?\n\n> +       strbuf_getline(&str, fp, '\\n');\n> +       bisect_term_bad = strbuf_detach(&str, NULL);\n> +       strbuf_getline(&str, fp, '\\n');\n> +       bisect_term_good = strbuf_detach(&str, NULL);\n> +\n> +       strbuf_release(&str);\n> +       fclose(fp);\n> +}\n> +\n> +/*\n>  * We use the convention that exiting with an exit code 10 means that\n>  * the bisection process finished successfully.\n>  * In this case the calling shell script should exit 0.\n> @@ -898,6 +934,8 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n>  */\n>  int bisect_next_all(const char *prefix, int no_checkout)\n>  {\n> +       read_bisect_terms();\n> +\n>        struct rev_info revs;\n>        struct commit_list *tried;\n>        int reaches = 0, all = 0, nr, steps;\n\nWe put all declarations at the beginning of functions.\n\n> @@ -920,13 +958,14 @@ int bisect_next_all(const char *prefix, int no_checkout)\n>\n>        if (!revs.commits) {\n>                /*\n> -                * We should exit here only if the \"bad\"\n> +                * We should exit here only if the \"bad\" (or \"new\")\n\nAgain I don't think it's worth changing comments.\n\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index 99efbe8..152b4f3 100755\n> --- a/git-bisect.sh\n> +++ b/git-bisect.sh\n> @@ -1,6 +1,6 @@\n>  #!/bin/sh\n>\n> -USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run]'\n> +USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run|new|old]'\n>  LONG_USAGE='git bisect help\n>        print this long help message.\n>  git bisect start [--no-checkout] [ [...]] [--] [...]\n> @@ -22,7 +22,15 @@ git bisect replay \n>  git bisect log\n>        show bisect log.\n>  git bisect run ...\n> -       use ... to automatically bisect.\n> +       use ... to automatically bisect\n\nWhy this change?\n\n> +\n> +When looking for a change in a given property instead of a regression\n> +you can use\n> +\n> +git bisect new []\n> +       mark as not having the property anymore\n> +git bisect old []\n\nIt is: [...]\n\n> +       mark ... as having the property\n\nBut anyway if possible I'd rather have:\n\ngit bisect (bad|new) []\ngit bisect (good|old) [...]\n\n>  Please use \"git help bisect\" to get the full man page.'\n>\n> @@ -32,6 +40,8 @@ OPTIONS_SPEC=\n>\n>  _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'\n>  _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n> +NEW=\"bad\"\n> +OLD=\"good\"\n\nWhy not BISECT_BAD_TERM/BISECT_GOOD_TERM instead of NEW/OLD?\nIt should be consistent with bisect.c\n\n>\n>  bisect_head()\n>  {\n> @@ -66,7 +76,7 @@ bisect_autostart() {\n>\n>  bisect_start() {\n>        #\n> -       # Check for one bad and then some good revisions.\n> +       # Check for one bad (or new) and then some good (or old) revisions.\n\nI don't think it's worth changing the comments.\n\n> @@ -184,8 +210,8 @@ bisect_write() {\n>        rev=\"$2\"\n>        nolog=\"$3\"\n>        case \"$state\" in\n> -               bad)            tag=\"$state\" ;;\n> -               good|skip)      tag=\"$state\"-\"$rev\" ;;\n> +               bad|new)                tag=\"$state\" ;;\n> +               good|skip|old)  tag=\"$state\"-\"$rev\" ;;\n\nWhy not \"$BISECT_TERM_BAD\" instead of \"bad|new\" and\n\"$BISECT_TERM_GOOD|skip\" instead of \"good|skip|old\"?\n\n>                *)              die \"$(eval_gettext \"Bad bisect_write argument: \\$state\")\" ;;\n>        esac\n>        git update-ref \"refs/bisect/$tag\" \"$rev\" || exit\n> @@ -230,12 +256,12 @@ bisect_state() {\n>        case \"$#,$state\" in\n>        0,*)\n>                die \"$(gettext \"Please call 'bisect_state' with at least one argument.\")\" ;;\n> -       1,bad|1,good|1,skip)\n> +       1,bad|1,good|1,skip|1,new|1,old)\n\nIdem.\n\n>                rev=$(git rev-parse --verify $(bisect_head)) ||\n>                        die \"$(gettext \"Bad rev input: $(bisect_head)\")\"\n>                bisect_write \"$state\" \"$rev\"\n>                check_expected_revs \"$rev\" ;;\n> -       2,bad|*,good|*,skip)\n> +       2,bad|*,good|*,skip|2,new|*,old)\n\nIdem.\n\n>                shift\n>                eval=''\n>                for rev in \"$@\"\n> @@ -246,8 +272,8 @@ bisect_state() {\n>                done\n>                eval \"$eval\"\n>                check_expected_revs \"$@\" ;;\n> -       *,bad)\n> -               die \"$(gettext \"'git bisect bad' can take only one argument.\")\" ;;\n> +       *,bad|*,new)\n\nIdem.\n\n> +               die \"$(gettext \"'git bisect $NEW' can take only one argument.\")\" ;;\n>        *)\n>                usage ;;\n>        esac\n> @@ -256,21 +282,21 @@ bisect_state() {\n>\n>  bisect_next_check() {\n>        missing_good= missing_bad=\n> -       git show-ref -q --verify refs/bisect/bad || missing_bad=t\n> -       test -n \"$(git for-each-ref \"refs/bisect/good-*\")\" || missing_good=t\n> +       git show-ref -q --verify refs/bisect/$NEW || missing_bad=t\n> +       test -n \"$(git for-each-ref \"refs/bisect/$OLD-*\")\" || missing_good=t\n>\n>        case \"$missing_good,$missing_bad,$1\" in\n>        ,,*)\n> -               : have both good and bad - ok\n> +               : have both good and bad or old and new - ok\n>                ;;\n>        *,)\n>                # do not have both but not asked to fail - just report.\n>                false\n>                ;;\n> -       t,,good)\n> -               # have bad but not good.  we could bisect although\n> +       t,,good|t,,old)\n\nIdem.\n\n\nThanks,\nChristian.\n"},{"id":"193415","messageId":"7v1ull5bld.fsf@alter.siamese.dyndns.org","threadId":"30775","inReplyTo":"CAP8UFD1mgKNQRZpuqYgyL__DGpoZ3cOGJC0amoqWT+1Bn_0iTg@mail.gmail.com","subject":"Re: [PATCH] git bisect old/new","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-12T05:43:42Z","receivedAt":"2012-06-12T05:43:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\nThanks for comments.\n\n> If you used some design that was discussed on the mailing list or if\n> there have been relevant discussions on the mailing list, it would be\n> nice to have links to the email thread in the commit message.\n\nPerhaps.\n\n>> + git bisect new []\n>> + git bisect old [...]\n>\n> maybe:\n>\n> git bisect (bad|new) []\n> git bisect (good|old) [...]\n\nDefinitely.\n\n>> @@ -104,6 +106,44 @@ For example, `git bisect reset HEAD` will leave you on the current\n>> bisection commit and avoid switching commits at all, while `git bisect\n>> reset bisect/bad` will check out the first bad revision.\n>>\n>> +Alternative research: bisect new and bisect old\n>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n>> +\n>> +If you are not looking for a regression but for a change of a given\n>> +property, you can use:\n>\n> I would rather say:\n> ...\n\nGood.\n\n>> @@ -403,9 +406,10 @@ struct commit_list *find_bisection(struct commit_list *list,\n>> static int register_ref(const char *refname, const unsigned char *sha1,\n>> int flags, void *cb_data)\n>> {\n>> - if (!strcmp(refname, \"bad\")) {\n>> + if (!strcmp(refname, bisect_term_bad)) {\n>> current_bad_sha1 = sha1;\n>> - } else if (!prefixcmp(refname, \"good-\")) {\n>> + } else if (!prefixcmp(refname, \"good-\") ||\n>> + !prefixcmp(refname, \"old-\")) {\n>\n> I don't like very much \"good\" and \"old\" to be hardcoded here.\n\nReally?\n\n>> @@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)\n>> if (is_expected_rev(current_bad_sha1)) {\n>> char *bad_hex = sha1_to_hex(current_bad_sha1);\n>> char *good_hex = join_sha1_array_hex(&good_revs, ' ');\n>> + if (!strcmp(bisect_term_bad,\"bad\")) {\n>> + fprintf(stderr, \"The merge base %s is bad.\\n\"\n>> + \"This means the bug has been fixed \"\n>> + \"between %s and [%s].\\n\",\n>> + bad_hex, bad_hex, good_hex);\n>> + } else {\n>> + fprintf(stderr, \"The merge base %s is new.\\n\"\n>> + \"The property has changed \"\n>> + \"between %s and [%s].\\n\",\n>> + bad_hex, bad_hex, good_hex);\n>> + }\n>\n> I don't like very much \"new\" to be harcoded here too.\n\nWhy not? It is not like we will be adding any more synonym pair\nbeyond good/bad, so... \n\n>>\n>> /*\n>> - * \"check_merge_bases\" checks that merge bases are not \"bad\".\n>> + * \"check_merge_bases\" checks that merge bases are not \"bad\" (resp. \"new\").\n>> *\n>> - * - If one is \"bad\", it means the user assumed something wrong\n>> + * - If one is \"bad\" (resp. \"new\"), it means the user assumed something wrong\n>> * and we must exit with a non 0 error code.\n>> - * - If one is \"good\", that's good, we have nothing to do.\n>> + * - If one is \"good\" (resp. \"old\"), that's good, we have nothing to do.\n>> * - If one is \"skipped\", we can't know but we should warn.\n>> * - If we don't know, we should check it out and ask the user to test.\n>> */\n>\n> I am not sure changing the comments is worth it...\n\nI think it is probably a good idea to cast in stone that we support\ntwo pairs, i.e. good/bad or old/new. I would have said \"or\" instead\nof \"resp.\" above, though.\n\n>> @@ -889,6 +901,30 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n>> }\n>>\n>> /*\n>> + * The terms used for this bisect session are stocked in\n>> + * BISECT_TERMS: it can be bad/good or new/old.\n>\n> I am not sure saying \"it can be bad/good or new/old\" adds anything.\n\nIt makes it clear that we are not allowing arbitrary pair of words\nto substitute the good/bad pair, which is a plus.\n\n>> +void read_bisect_terms(void)\n>> +{\n>> + struct strbuf str = STRBUF_INIT;\n>> + const char *filename = git_path(\"BISECT_TERMS\");\n>> + FILE *fp = fopen(filename, \"r\");\n>> +\n>> + if (!fp)\n>> + die_errno(\"Could not open file '%s'\", filename);\n>\n> This is not very compatible with older git versions.\n> I know that it's kind of strange to upgrade git in the middle of a\n> bisection but why not just use \"bad\"/\"good\" if there is no file?\n\nGood thinking.\n\n>> @@ -898,6 +934,8 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n>> */\n>> int bisect_next_all(const char *prefix, int no_checkout)\n>> {\n>> + read_bisect_terms();\n>> +\n>> struct rev_info revs;\n>> struct commit_list *tried;\n>> int reaches = 0, all = 0, nr, steps;\n>\n> We put all declarations at the beginning of functions.\n\nGood eyes.\n\n>> @@ -22,7 +22,15 @@ git bisect replay \n>> git bisect log\n>> show bisect log.\n>> git bisect run ...\n>> - use ... to automatically bisect.\n>> + use ... to automatically bisect\n>\n> Why this change?\n\nTo end a sentence with a full-stop?\n\n> But anyway if possible I'd rather have:\n>\n> git bisect (bad|new) []\n> git bisect (good|old) [...]\n\nYes.\n\n>> @@ -32,6 +40,8 @@ OPTIONS_SPEC=\n>>\n>> _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'\n>> _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n>> +NEW=\"bad\"\n>> +OLD=\"good\"\n>\n> Why not BISECT_BAD_TERM/BISECT_GOOD_TERM instead of NEW/OLD?\n> It should be consistent with bisect.c\n\nIt's kind of too long. Isn't BISECT_GOOD vs BISECT_BAD good enough\n(and if so make bisect.c consistent with it).\n\n>> @@ -184,8 +210,8 @@ bisect_write() {\n>> rev=\"$2\"\n>> nolog=\"$3\"\n>> case \"$state\" in\n>> - bad) tag=\"$state\" ;;\n>> - good|skip) tag=\"$state\"-\"$rev\" ;;\n>> + bad|new) tag=\"$state\" ;;\n>> + good|skip|old) tag=\"$state\"-\"$rev\" ;;\n>\n> Why not \"$BISECT_TERM_BAD\" instead of \"bad|new\" and\n> \"$BISECT_TERM_GOOD|skip\" instead of \"good|skip|old\"?\n\nIf the point is to make sure \"git bisect good\" will error out when\nwe are in new/old mode, I agree (and also the other case/esac in the\nremainder of the patch that allows you feed bad and new mixed).\n\nThese case arms look indented in a funny way, but is it only because\nof e-mail quoting, by the way?\n\n>> *) die \"$(eval_gettext \"Bad bisect_write argument: \\$state\")\" ;;\n>> esac\n"},{"id":"193493","messageId":"CABURp0rWu0GtmSrLV+qEQJVYOt8p=B4bQtG3fzKh2f0fTGHwOQ@mail.gmail.com","threadId":"30775","inReplyTo":"7v1ull5bld.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git bisect old/new","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2012-06-12T19:41:02Z","receivedAt":"2012-06-12T19:41:02Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Tue, Jun 12, 2012 at 1:43 AM, Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> Thanks for comments.\n>\n> > If you used some design that was discussed on the mailing list or if\n> > there have been relevant discussions on the mailing list, it would be\n> > nice to have links to the email thread in the commit message.\n>\n> Perhaps.\n>\n> >> + git bisect new []\n> >> + git bisect old [...]\n> >\n> > maybe:\n> >\n> > git bisect (bad|new) []\n> > git bisect (good|old) [...]\n>\n> Definitely.\n>\n> >> @@ -104,6 +106,44 @@ For example, `git bisect reset HEAD` will leave\n> >> you on the current\n> >>  bisection commit and avoid switching commits at all, while `git bisect\n> >>  reset bisect/bad` will check out the first bad revision.\n> >>\n> >> +Alternative research: bisect new and bisect old\n> >> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> >> +\n> >> +If you are not looking for a regression but for a change of a given\n> >> +property, you can use:\n> >\n> > I would rather say:\n> > ...\n>\n> Good.\n>\n> >> @@ -403,9 +406,10 @@ struct commit_list *find_bisection(struct\n> >> commit_list *list,\n> >>  static int register_ref(const char *refname, const unsigned char\n> >> *sha1,\n> >>                        int flags, void *cb_data)\n> >>  {\n> >> -       if (!strcmp(refname, \"bad\")) {\n> >> +       if (!strcmp(refname, bisect_term_bad)) {\n> >>                current_bad_sha1 = sha1;\n> >> -       } else if (!prefixcmp(refname, \"good-\")) {\n> >> +       } else if (!prefixcmp(refname, \"good-\") ||\n> >> +                       !prefixcmp(refname, \"old-\")) {\n> >\n> > I don't like very much \"good\" and \"old\" to be hardcoded here.\n>\n> Really?\n\nI tend to agree. But I like more generic code and less hard-coded in\nalmost all cases.\n\n\n> >> @@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)\n> >>        if (is_expected_rev(current_bad_sha1)) {\n> >>                char *bad_hex = sha1_to_hex(current_bad_sha1);\n> >>                char *good_hex = join_sha1_array_hex(&good_revs, ' ');\n> >> +               if (!strcmp(bisect_term_bad,\"bad\")) {\n> >> +                       fprintf(stderr, \"The merge base %s is bad.\\n\"\n> >> +                               \"This means the bug has been fixed \"\n> >> +                               \"between %s and [%s].\\n\",\n> >> +                               bad_hex, bad_hex, good_hex);\n> >> +               } else {\n> >> +                       fprintf(stderr, \"The merge base %s is new.\\n\"\n> >> +                               \"The property has changed \"\n> >> +                               \"between %s and [%s].\\n\",\n> >> +                               bad_hex, bad_hex, good_hex);\n> >> +               }\n> >\n> > I don't like very much \"new\" to be harcoded here too.\n>\n> Why not?  It is not like we will be adding any more synonym pair\n> beyond good/bad, so...\n\nPreviously we discussed using yes/no, among others.\nhttp://permalink.gmane.org/gmane.comp.version-control.git/182496\n\n>\n> >>\n> >>  /*\n> >> - * \"check_merge_bases\" checks that merge bases are not \"bad\".\n> >> + * \"check_merge_bases\" checks that merge bases are not \"bad\" (resp.\n> >> \"new\").\n> >>  *\n> >> - * - If one is \"bad\", it means the user assumed something wrong\n> >> + * - If one is \"bad\" (resp. \"new\"), it means the user assumed\n> >> something wrong\n> >>  * and we must exit with a non 0 error code.\n> >> - * - If one is \"good\", that's good, we have nothing to do.\n> >> + * - If one is \"good\" (resp. \"old\"), that's good, we have nothing to\n> >> do.\n> >>  * - If one is \"skipped\", we can't know but we should warn.\n> >>  * - If we don't know, we should check it out and ask the user to test.\n> >>  */\n> >\n> > I am not sure changing the comments is worth it...\n>\n> I think it is probably a good idea to cast in stone that we support\n> two pairs, i.e. good/bad or old/new.  I would have said \"or\" instead\n> of \"resp.\" above, though.\n>\n> >> @@ -889,6 +901,30 @@ static void show_diff_tree(const char *prefix,\n> >> struct commit *commit)\n> >>  }\n> >>\n> >>  /*\n> >> + * The terms used for this bisect session are stocked in\n> >> + * BISECT_TERMS: it can be bad/good or new/old.\n> >\n> > I am not sure saying \"it can be bad/good or new/old\" adds anything.\n>\n> It makes it clear that we are not allowing arbitrary pair of words\n> to substitute the good/bad pair, which is a plus.\n>\n> >> +void read_bisect_terms(void)\n> >> +{\n> >> +       struct strbuf str = STRBUF_INIT;\n> >> +       const char *filename = git_path(\"BISECT_TERMS\");\n> >> +       FILE *fp = fopen(filename, \"r\");\n> >> +\n> >> +       if (!fp)\n> >> +               die_errno(\"Could not open file '%s'\", filename);\n> >\n> > This is not very compatible with older git versions.\n> > I know that it's kind of strange to upgrade git in the middle of a\n> > bisection but why not just use \"bad\"/\"good\" if there is no file?\n>\n> Good thinking.\n>\n> >> @@ -898,6 +934,8 @@ static void show_diff_tree(const char *prefix,\n> >> struct commit *commit)\n> >>  */\n> >>  int bisect_next_all(const char *prefix, int no_checkout)\n> >>  {\n> >> +       read_bisect_terms();\n> >> +\n> >>        struct rev_info revs;\n> >>        struct commit_list *tried;\n> >>        int reaches = 0, all = 0, nr, steps;\n> >\n> > We put all declarations at the beginning of functions.\n>\n> Good eyes.\n>\n> >> @@ -22,7 +22,15 @@ git bisect replay \n> >>  git bisect log\n> >>        show bisect log.\n> >>  git bisect run ...\n> >> -       use ... to automatically bisect.\n> >> +       use ... to automatically bisect\n> >\n> > Why this change?\n>\n> To end a sentence with a full-stop?\n\nI think you read this backwards. This change removes the full-stop.\n\nPhil\n"},{"id":"193518","messageId":"1339541765-2078-1-git-send-email-Valentin.Duperray@ensimag.imag.fr","threadId":"30775","inReplyTo":"1339466625-17461-1-git-send-email-Valentin.Duperray@ensimag.imag.fr","subject":"[PATCHv2] git bisect old/new","fromName":"Valentin Duperray","fromEmail":"valentin.duperray@ensimag.imag.fr","sentAt":"2012-06-12T22:56:05Z","receivedAt":"2012-06-12T22:56:05Z","isPatch":false,"sender":{"key":"valentin.duperray@ensimag.imag.fr","avatar":null},"body":"When not looking for a regression during a bisect but for a fix or a\nchange in another given property, it can be confusing to use 'good'\nand 'bad'.\n\nThis patch introduce `git bisect new` and `git bisect old` as an\nalternative to 'bad' and good': the commits which have the most\nrecent version of the property must be marked as `new` and the ones\nwith the older version as `old`.\n\nThe output will be the first commit after the change in the property.\nDuring a new/old bisect session you cannot use bad/good commands and\nvice-versa.\n\n`git bisect replay` works fine for old/new bisect sessions.\n\nSome commands are still not available for old/new:\n\n * git bisect start [ [...]] is not possible: the\n commits will be treated as bad and good.\n * git rev-list --bisect does not treat the revs/bisect/new and\n revs/bisect/old-SHA1 files.\n * thus, git bisect run is not available for new/old.\n * git bisect visualize seem to work partially: the tags are\n displayed correctly but the tree is not limited to the bisect\n section.\n\nRelated discussions:\n\n\t- http://thread.gmane.org/gmane.comp.version-control.git/86063\n\t\tintroduced bisect fix unfixed to find fix.\n\t- http://thread.gmane.org/gmane.comp.version-control.git/182398\n\t\tdiscussion around bisect yes/no or old/new.\n\nSigned-off-by: Valentin Duperray \nSigned-off-by: Franck Jonas \nSigned-off-by: Lucien Kong \nSigned-off-by: Thomas Nguy \nSigned-off-by: Huynh Khoi Nguyen Nguyen \nSigned-off-by: Matthieu Moy \n---\n Documentation/git-bisect.txt | 43 ++++++++++++++-\n bisect.c | 84 +++++++++++++++++++++--------\n git-bisect.sh | 121 ++++++++++++++++++++++++++++++++----------\n t/t6030-bisect-porcelain.sh | 40 ++++++++++++++\n 4 files changed, 236 insertions(+), 52 deletions(-)\n\ndiff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt\nindex e4f46bc..fc63894 100644\n--- a/Documentation/git-bisect.txt\n+++ b/Documentation/git-bisect.txt\n@@ -18,8 +18,10 @@ on the subcommand:\n \n git bisect help\n git bisect start [--no-checkout] [ [...]] [--] [...]\n- git bisect bad []\n- git bisect good [...]\n+ git bisect (bad|new) []\n+ git bisect (good|old) [...]\n+ git bisect new []\n+ git bisect old [...]\n git bisect skip [(|)...]\n git bisect reset []\n git bisect visualize\n@@ -104,6 +106,31 @@ For example, `git bisect reset HEAD` will leave you on the current\n bisection commit and avoid switching commits at all, while `git bisect\n reset bisect/bad` will check out the first bad revision.\n \n+\n+Alternative terms: bisect new and bisect old\n+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n+\n+If you are not at ease with the terms \"bad\" and \"good\", perhaps\n+because you are looking for the commit that introduced a fix, you can\n+alternatively use \"new\" and \"old\" instead.\n+But note that you cannot mix \"bad\" and good\" with \"new\" and \"old\".\n+\n+------------------------------------------------\n+git bisect new []\n+------------------------------------------------\n+\n+Same as \"git bisect bad []\".\n+\n+------------------------------------------------\n+git bisect old [...]\n+------------------------------------------------\n+\n+Same as \"git bisect good [...]\".\n+\n+You must run `git bisect start` without commits as argument and run\n+`git bisect new `/`git bisect old ...` after to add the\n+commits.\n+\n Bisect visualize\n ~~~~~~~~~~~~~~~~\n \n@@ -374,6 +401,18 @@ In this case, when 'git bisect run' finishes, bisect/bad will refer to a commit\n has at least one parent whose reachable graph is fully traversable in the sense\n required by 'git pack objects'.\n \n+* Look for a fix instead of a regression in the code\n++\n+------------\n+$ git bisect start\n+$ git bisect new\t\t# Current version is fixed\n+$ git bisect old bugged_version\t# bugged_version was the last version\n+\t\t\t\t# known to be unfixed\n+------------\n++\n+The \"new\" commits are the fixed ones and the \"old\" commits are the unfixed ones.\n+At the end of the commit session, you will have the first fixed commit.\n+\n \n SEE ALSO\n --------\ndiff --git a/bisect.c b/bisect.c\nindex 48acf73..38de2d5 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -21,6 +21,9 @@ static const char *argv_checkout[] = {\"checkout\", \"-q\", NULL, \"--\", NULL};\n static const char *argv_show_branch[] = {\"show-branch\", NULL, NULL};\n static const char *argv_update_ref[] = {\"update-ref\", \"--no-deref\", \"BISECT_HEAD\", NULL, NULL};\n \n+static const char *bisect_bad;\n+static const char *bisect_good;\n+\n /* bits #0-15 in revision.h */\n \n #define COUNTED\t\t(1u<<16)\n@@ -403,9 +406,10 @@ struct commit_list *find_bisection(struct commit_list *list,\n static int register_ref(const char *refname, const unsigned char *sha1,\n \t\t\tint flags, void *cb_data)\n {\n-\tif (!strcmp(refname, \"bad\")) {\n+\tif (!strcmp(refname, bisect_bad)) {\n \t\tcurrent_bad_sha1 = sha1;\n-\t} else if (!prefixcmp(refname, \"good-\")) {\n+\t} else if (!prefixcmp(refname, \"good-\") ||\n+\t\t\t!prefixcmp(refname, \"old-\")) {\n \t\tsha1_array_append(&good_revs, sha1);\n \t} else if (!prefixcmp(refname, \"skip-\")) {\n \t\tsha1_array_append(&skipped_revs, sha1);\n@@ -633,7 +637,7 @@ static void exit_if_skipped_commits(struct commit_list *tried,\n \t\treturn;\n \n \tprintf(\"There are only 'skip'ped commits left to test.\\n\"\n-\t \"The first bad commit could be any of:\\n\");\n+\t \"The first %s commit could be any of:\\n\", bisect_bad);\n \tprint_commit_list(tried, \"%s\\n\", \"%s\\n\");\n \tif (bad)\n \t\tprintf(\"%s\\n\", sha1_to_hex(bad));\n@@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)\n \tif (is_expected_rev(current_bad_sha1)) {\n \t\tchar *bad_hex = sha1_to_hex(current_bad_sha1);\n \t\tchar *good_hex = join_sha1_array_hex(&good_revs, ' ');\n-\n-\t\tfprintf(stderr, \"The merge base %s is bad.\\n\"\n-\t\t\t\"This means the bug has been fixed \"\n-\t\t\t\"between %s and [%s].\\n\",\n-\t\t\tbad_hex, bad_hex, good_hex);\n-\n+\t\tif (!strcmp(bisect_bad,\"bad\")) {\n+\t\t\tfprintf(stderr, \"The merge base %s is bad.\\n\"\n+\t\t\t\t\"This means the bug has been fixed \"\n+\t\t\t\t\"between %s and [%s].\\n\",\n+\t\t\t\tbad_hex, bad_hex, good_hex);\n+\t\t} else {\n+\t\t\tfprintf(stderr, \"The merge base %s is new.\\n\"\n+\t\t\t\t\"The property has changed \"\n+\t\t\t\t\"between %s and [%s].\\n\",\n+\t\t\t\tbad_hex, bad_hex, good_hex);\n+\t\t}\n \t\texit(3);\n \t}\n \n-\tfprintf(stderr, \"Some good revs are not ancestor of the bad rev.\\n\"\n+\tfprintf(stderr, \"Some %s revs are not ancestor of the %s rev.\\n\"\n \t\t\"git bisect cannot work properly in this case.\\n\"\n-\t\t\"Maybe you mistake good and bad revs?\\n\");\n+\t\t\"Maybe you mistake %s and %s revs?\\n\",\n+\t\tbisect_good, bisect_bad, bisect_good,\n+\t\tbisect_bad);\n \texit(1);\n }\n \n@@ -754,19 +765,19 @@ static void handle_skipped_merge_base(const unsigned char *mb)\n \n \twarning(\"the merge base between %s and [%s] \"\n \t\t\"must be skipped.\\n\"\n-\t\t\"So we cannot be sure the first bad commit is \"\n+\t\t\"So we cannot be sure the first %s commit is \"\n \t\t\"between %s and %s.\\n\"\n \t\t\"We continue anyway.\",\n-\t\tbad_hex, good_hex, mb_hex, bad_hex);\n+\t\tbad_hex, good_hex, bisect_bad, mb_hex, bad_hex);\n \tfree(good_hex);\n }\n \n /*\n- * \"check_merge_bases\" checks that merge bases are not \"bad\".\n+ * \"check_merge_bases\" checks that merge bases are not \"bad\" (or \"new\").\n *\n- * - If one is \"bad\", it means the user assumed something wrong\n+ * - If one is \"bad\" (or \"new\"), it means the user assumed something wrong\n * and we must exit with a non 0 error code.\n- * - If one is \"good\", that's good, we have nothing to do.\n+ * - If one is \"good\" (or \"old\"), that's good, we have nothing to do.\n * - If one is \"skipped\", we can't know but we should warn.\n * - If we don't know, we should check it out and ask the user to test.\n */\n@@ -838,7 +849,7 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)\n \tint fd;\n \n \tif (!current_bad_sha1)\n-\t\tdie(\"a bad revision is needed\");\n+\t\tdie(\"a %s revision is needed\", bisect_bad);\n \n \t/* Check if file BISECT_ANCESTORS_OK exists. */\n \tif (!stat(filename, &st) && S_ISREG(st.st_mode))\n@@ -889,6 +900,31 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n }\n \n /*\n+ * The terms used for this bisect session are stocked in\n+ * BISECT_TERMS: it can be bad/good or new/old.\n+ * We read them and stock them to adapt the messages\n+ * accordingly. Default is bad/good.\n+ */\n+void read_bisect_terms(void)\n+{\n+\tstruct strbuf str = STRBUF_INIT;\n+\tconst char *filename = git_path(\"BISECT_TERMS\");\n+\tFILE *fp = fopen(filename, \"r\");\n+\n+\tif (!fp) {\n+\t\tbisect_bad = \"bad\";\n+\t\tbisect_good = \"good\";\n+\t} else {\n+\tstrbuf_getline(&str, fp, '\\n');\n+\tbisect_bad = strbuf_detach(&str, NULL);\n+\tstrbuf_getline(&str, fp, '\\n');\n+\tbisect_good = strbuf_detach(&str, NULL);\n+\t}\n+\tstrbuf_release(&str);\n+\tfclose(fp);\n+}\n+\n+/*\n * We use the convention that exiting with an exit code 10 means that\n * the bisection process finished successfully.\n * In this case the calling shell script should exit 0.\n@@ -903,7 +939,7 @@ int bisect_next_all(const char *prefix, int no_checkout)\n \tint reaches = 0, all = 0, nr, steps;\n \tconst unsigned char *bisect_rev;\n \tchar bisect_rev_hex[41];\n-\n+\tread_bisect_terms();\n \tif (read_bisect_refs())\n \t\tdie(\"reading bisect refs failed\");\n \n@@ -925,8 +961,9 @@ int bisect_next_all(const char *prefix, int no_checkout)\n \t\t */\n \t\texit_if_skipped_commits(tried, NULL);\n \n-\t\tprintf(\"%s was both good and bad\\n\",\n-\t\t sha1_to_hex(current_bad_sha1));\n+\t\tprintf(\"%s was both %s and %s\\n\",\n+\t\t sha1_to_hex(current_bad_sha1), bisect_good,\n+\t\t bisect_bad);\n \t\texit(1);\n \t}\n \n@@ -941,7 +978,8 @@ int bisect_next_all(const char *prefix, int no_checkout)\n \n \tif (!hashcmp(bisect_rev, current_bad_sha1)) {\n \t\texit_if_skipped_commits(tried, current_bad_sha1);\n-\t\tprintf(\"%s is the first bad commit\\n\", bisect_rev_hex);\n+\t\tprintf(\"%s is the first %s commit\\n\", bisect_rev_hex,\n+\t\t\tbisect_bad);\n \t\tshow_diff_tree(prefix, revs.commits->item);\n \t\t/* This means the bisection process succeeded. */\n \t\texit(10);\n@@ -953,6 +991,8 @@ int bisect_next_all(const char *prefix, int no_checkout)\n \t \"(roughly %d step%s)\\n\", nr, (nr == 1 ? \"\" : \"s\"),\n \t steps, (steps == 1 ? \"\" : \"s\"));\n \n+\tfree((char*)bisect_bad);\n+\tfree((char*)bisect_good);\n+\n \treturn bisect_checkout(bisect_rev_hex, no_checkout);\n }\n-\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 99efbe8..6ba87e5 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -1,14 +1,16 @@\n #!/bin/sh\n \n-USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run]'\n+USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run|new|old]'\n LONG_USAGE='git bisect help\n \tprint this long help message.\n git bisect start [--no-checkout] [ [...]] [--] [...]\n \treset bisect state and start bisection.\n-git bisect bad []\n-\tmark a known-bad revision.\n-git bisect good [...]\n-\tmark ... known-good revisions.\n+git bisect (bad|new) []\n+\tmark a known-bad revision/\n+\t\ta revision after change in a given property.\n+git bisect (good|old) [...]\n+\tmark ... known-good revisions/\n+\t\trevisions before change in a given property.\n git bisect skip [(|)...]\n \tmark ... untestable revisions.\n git bisect next\n@@ -32,6 +34,8 @@ OPTIONS_SPEC=\n \n _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'\n _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n+BISECT_BAD=\"bad\"\n+BISECT_GOOD=\"good\"\n \n bisect_head()\n {\n@@ -75,6 +79,7 @@ bisect_start() {\n \torig_args=$(git rev-parse --sq-quote \"$@\")\n \tbad_seen=0\n \teval=''\n+\tstart_bad_good=0\n \tif test \"z$(git rev-parse --is-bare-repository)\" != zfalse\n \tthen\n \t\tmode=--no-checkout\n@@ -99,6 +104,16 @@ bisect_start() {\n \t\t\t\tdie \"$(eval_gettext \"'\\$arg' does not appear to be a valid revision\")\"\n \t\t\t\tbreak\n \t\t\t}\n+\n+\t\t\tif test -s \"$GIT_DIR/BISECT_TERMS\"\n+\t\t\tthen\n+\t\t\t\tif $(sed -n 1p \"$GIT_DIR/BISECT_TERMS\") != 'bad'\n+\t\t\t\tthen\n+\t\t\t\t\tdie \"$(gettext \"you are already bisecting in old/new mode\")\"\n+\t\t\t\tfi\n+\t\t\tfi\n+\t\t\tstart_bad_good=1\n+\n \t\t\tcase $bad_seen in\n \t\t\t0) state='bad' ; bad_seen=1 ;;\n \t\t\t*) state='good' ;;\n@@ -170,6 +185,11 @@ bisect_start() {\n \t} &&\n \tgit rev-parse --sq-quote \"$@\" >\"$GIT_DIR/BISECT_NAMES\" &&\n \teval \"$eval true\" &&\n+\tif test $start_bad_good -eq 1 -a ! -s \"$GIT_DIR/BISECT_TERMS\"\n+\tthen\n+\t\techo \"bad\" >\"$GIT_DIR/BISECT_TERMS\" &&\n+\t\techo \"good\" >>\"$GIT_DIR/BISECT_TERMS\"\n+\tfi &&\n \techo \"git bisect start$orig_args\" >>\"$GIT_DIR/BISECT_LOG\" || exit\n \t#\n \t# Check if we can proceed to the next bisect state.\n@@ -184,9 +204,12 @@ bisect_write() {\n \trev=\"$2\"\n \tnolog=\"$3\"\n \tcase \"$state\" in\n-\t\tbad)\t\ttag=\"$state\" ;;\n-\t\tgood|skip)\ttag=\"$state\"-\"$rev\" ;;\n-\t\t*)\t\tdie \"$(eval_gettext \"Bad bisect_write argument: \\$state\")\" ;;\n+\t\t\"$BISECT_BAD\")\n+\t\t\ttag=\"$state\" ;;\n+\t\t\"$BISECT_GOOD\"|skip)\n+\t\t\ttag=\"$state\"-\"$rev\" ;;\n+\t\t*)\n+\t\t\tdie \"$(eval_gettext \"Bad bisect_write argument: \\$state\")\" ;;\n \tesac\n \tgit update-ref \"refs/bisect/$tag\" \"$rev\" || exit\n \techo \"# $state: $(git show-branch $rev)\" >>\"$GIT_DIR/BISECT_LOG\"\n@@ -230,12 +253,12 @@ bisect_state() {\n \tcase \"$#,$state\" in\n \t0,*)\n \t\tdie \"$(gettext \"Please call 'bisect_state' with at least one argument.\")\" ;;\n-\t1,bad|1,good|1,skip)\n+\t1,\"$BISECT_BAD\"|1,\"$BISECT_GOOD\"|1,skip)\n \t\trev=$(git rev-parse --verify $(bisect_head)) ||\n \t\t\tdie \"$(gettext \"Bad rev input: $(bisect_head)\")\"\n \t\tbisect_write \"$state\" \"$rev\"\n \t\tcheck_expected_revs \"$rev\" ;;\n-\t2,bad|*,good|*,skip)\n+\t2,\"$BISECT_BAD\"|*,\"$BISECT_GOOD\"|*,skip)\n \t\tshift\n \t\teval=''\n \t\tfor rev in \"$@\"\n@@ -246,8 +269,8 @@ bisect_state() {\n \t\tdone\n \t\teval \"$eval\"\n \t\tcheck_expected_revs \"$@\" ;;\n-\t*,bad)\n-\t\tdie \"$(gettext \"'git bisect bad' can take only one argument.\")\" ;;\n+\t*,\"$BISECT_BAD\")\n+\t\tdie \"$(gettext \"'git bisect $BISECT_BAD' can take only one argument.\")\" ;;\n \t*)\n \t\tusage ;;\n \tesac\n@@ -256,21 +279,21 @@ bisect_state() {\n \n bisect_next_check() {\n \tmissing_good= missing_bad=\n-\tgit show-ref -q --verify refs/bisect/bad || missing_bad=t\n-\ttest -n \"$(git for-each-ref \"refs/bisect/good-*\")\" || missing_good=t\n+\tgit show-ref -q --verify refs/bisect/$BISECT_BAD || missing_bad=t\n+\ttest -n \"$(git for-each-ref \"refs/bisect/$BISECT_GOOD-*\")\" || missing_good=t\n \n \tcase \"$missing_good,$missing_bad,$1\" in\n \t,,*)\n-\t\t: have both good and bad - ok\n+\t\t: have both good and bad or old and new - ok\n \t\t;;\n \t*,)\n \t\t# do not have both but not asked to fail - just report.\n \t\tfalse\n \t\t;;\n-\tt,,good)\n-\t\t# have bad but not good. we could bisect although\n+\tt,,\"$BISECT_GOOD\")\n+\t\t# have bad (or new) but not good (or old). we could bisect although\n \t\t# this is less optimum.\n-\t\tgettextln \"Warning: bisecting only with a bad commit.\" >&2\n+\t\tgettextln \"Warning: bisecting only with a $BISECT_BAD commit.\" >&2\n \t\tif test -t 0\n \t\tthen\n \t\t\t# TRANSLATORS: Make sure to include [Y] and [n] in your\n@@ -280,17 +303,17 @@ bisect_next_check() {\n \t\t\tread yesno\n \t\t\tcase \"$yesno\" in [Nn]*) exit 1 ;; esac\n \t\tfi\n-\t\t: bisect without good...\n+\t\t: bisect without $BISECT_GOOD...\n \t\t;;\n \t*)\n \n \t\tif test -s \"$GIT_DIR/BISECT_START\"\n \t\tthen\n-\t\t\tgettextln \"You need to give me at least one good and one bad revisions.\n+\t\t\tgettextln \"You need to give me at least one good and one bad (or one old and one new) revisions.\n (You can use \\\"git bisect bad\\\" and \\\"git bisect good\\\" for that.)\" >&2\n \t\telse\n \t\t\tgettextln \"You need to start by \\\"git bisect start\\\".\n-You then need to give me at least one good and one bad revisions.\n+You then need to give me at least one good and one bad (or one old and one new) revisions.\n (You can use \\\"git bisect bad\\\" and \\\"git bisect good\\\" for that.)\" >&2\n \t\tfi\n \t\texit 1 ;;\n@@ -304,7 +327,7 @@ bisect_auto_next() {\n bisect_next() {\n \tcase \"$#\" in 0) ;; *) usage ;; esac\n \tbisect_autostart\n-\tbisect_next_check good\n+\tbisect_next_check $BISECT_GOOD\n \n \t# Perform all bisection computation, display and checkout\n \tgit bisect--helper --next-all $(test -f \"$GIT_DIR/BISECT_HEAD\" && echo --no-checkout)\n@@ -378,6 +401,7 @@ bisect_clean_state() {\n \trm -f \"$GIT_DIR/BISECT_LOG\" &&\n \trm -f \"$GIT_DIR/BISECT_NAMES\" &&\n \trm -f \"$GIT_DIR/BISECT_RUN\" &&\n+\trm -f \"$GIT_DIR/BISECT_TERMS\" &&\n \t# Cleanup head-name if it got left by an old version of git-bisect\n \trm -f \"$GIT_DIR/head-name\" &&\n \tgit update-ref -d --no-deref BISECT_HEAD &&\n@@ -398,11 +422,13 @@ bisect_replay () {\n \t\t\trev=\"$command\"\n \t\t\tcommand=\"$bisect\"\n \t\tfi\n+\t\tget_terms\n+\t\tcheck_and_set_terms \"$command\"\n \t\tcase \"$command\" in\n \t\tstart)\n \t\t\tcmd=\"bisect_start $rev\"\n \t\t\teval \"$cmd\" ;;\n-\t\tgood|bad|skip)\n+\t\tgood|bad|skip|old|new)\n \t\t\tbisect_write \"$command\" \"$rev\" ;;\n \t\t*)\n \t\t\tdie \"$(gettext \"?? what are you talking about?\")\" ;;\n@@ -436,9 +462,9 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n \t\t\tstate='skip'\n \t\telif [ $res -gt 0 ]\n \t\tthen\n-\t\t\tstate='bad'\n+\t\t\tstate=\"$BISECT_BAD\"\n \t\telse\n-\t\t\tstate='good'\n+\t\t\tstate=\"$BISECT_GOOD\"\n \t\tfi\n \n \t\t# We have to use a subshell because \"bisect_state\" can exit.\n@@ -447,7 +473,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n \n \t\tcat \"$GIT_DIR/BISECT_RUN\"\n \n-\t\tif sane_grep \"first bad commit could be any of\" \"$GIT_DIR/BISECT_RUN\" \\\n+\t\tif sane_grep \"first $BISECT_BAD commit could be any of\" \"$GIT_DIR/BISECT_RUN\" \\\n \t\t\t> /dev/null\n \t\tthen\n \t\t\tgettextln \"bisect run cannot continue any more\" >&2\n@@ -461,7 +487,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n \t\t\texit $res\n \t\tfi\n \n-\t\tif sane_grep \"is the first bad commit\" \"$GIT_DIR/BISECT_RUN\" > /dev/null\n+\t\tif sane_grep \"is the first $BISECT_BAD commit\" \"$GIT_DIR/BISECT_RUN\" > /dev/null\n \t\tthen\n \t\t\tgettextln \"bisect run success\"\n \t\t\texit 0;\n@@ -475,18 +501,57 @@ bisect_log () {\n \tcat \"$GIT_DIR/BISECT_LOG\"\n }\n \n+get_terms () {\n+\tif test -s \"$GIT_DIR/BISECT_TERMS\"\n+\tthen\n+\t\tBISECT_BAD=\"$(sed -n 1p \"$GIT_DIR/BISECT_TERMS\")\"\n+\t\tBISECT_GOOD=\"$(sed -n 2p \"$GIT_DIR/BISECT_TERMS\")\"\n+\tfi\n+}\n+\n+check_and_set_terms () {\n+\tcmd=\"$1\"\n+\tcase \"$cmd\" in\n+\tbad|good|new|old)\n+\t\tif test -s \"$GIT_DIR/BISECT_TERMS\" -a \"$cmd\" != \"$BISECT_BAD\" -a \"$cmd\" != \"$BISECT_GOOD\"\n+\t\tthen\n+\t\t\tdie \"$(eval_gettext \"Invalid command : you're currently in a \\$BISECT_BAD/\\$BISECT_GOOD bisect.\")\"\n+\t\tfi\n+\t\tcase \"$cmd\" in\n+\t\tbad|good)\n+\t\t\tif test ! -s \"$GIT_DIR/BISECT_TERMS\"\n+\t\t\tthen\n+\t\t\t\techo \"bad\" >\"$GIT_DIR/BISECT_TERMS\" &&\n+\t\t\t\techo \"good\" >>\"$GIT_DIR/BISECT_TERMS\"\n+\t\t\tfi\n+\t\t\tBISECT_BAD=\"bad\"\n+\t\t\tBISECT_GOOD=\"good\" ;;\n+\t\tnew|old)\n+\t\t\tif test ! -s \"$GIT_DIR/BISECT_TERMS\"\n+\t\t\tthen\n+\t\t\t\techo \"new\" >\"$GIT_DIR/BISECT_TERMS\" &&\n+\t\t\t\techo \"old\" >>\"$GIT_DIR/BISECT_TERMS\"\n+\t\t\tfi\n+\t\t\tBISECT_BAD=\"new\"\n+\t\t\tBISECT_GOOD=\"old\" ;;\n+\t\tesac ;;\n+\tesac\n+}\n+\n case \"$#\" in\n 0)\n \tusage ;;\n *)\n \tcmd=\"$1\"\n+\tget_terms\n+\tcheck_and_set_terms \"$cmd\"\n \tshift\n \tcase \"$cmd\" in\n \thelp)\n \t\tgit bisect -h ;;\n \tstart)\n \t\tbisect_start \"$@\" ;;\n-\tbad|good)\n+\tbad|good|new|old)\n \t\tbisect_state \"$cmd\" \"$@\" ;;\n \tskip)\n \t\tbisect_skip \"$@\" ;;\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 72e28ee..c4a5d6b 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -741,6 +741,46 @@ test_expect_success 'bisect: demonstrate identification of damage boundary' \"\n \t\ttest \\$rc = 0' &&\n \tcheck_same BROKEN_HASH6 bisect/bad &&\n \tgit bisect reset\n+\tgit checkout master\n+\n \"\n \n+test_expect_success 'bisect starts with only one new' '\n+\tgit bisect reset &&\n+\tgit bisect start &&\n+\tgit bisect new $HASH4 &&\n+\tgit bisect next\n+'\n+test_expect_success 'bisect does not start with only one old' '\n+\tgit bisect reset &&\n+\tgit bisect start &&\n+\tgit bisect old $HASH1 || return 1\n+\ttest_must_fail git bisect next\n+\n+'\n+\n+test_expect_success 'bisect start with one new and old' '\n+\tgit bisect reset &&\n+\tgit bisect start &&\n+\tgit bisect old $HASH1 &&\n+\tgit bisect new $HASH4 &&\n+\tgit bisect new &&\n+\tgit bisect new >bisect_result &&\n+\tgrep \"$HASH2 is the first new commit\" bisect_result &&\n+\tgit bisect log > log_to_replay.txt &&\n+\tgit bisect reset\n+'\n+\n+test_expect_success 'bisect replay with old and new' '\n+\tgit bisect replay log_to_replay.txt > bisect_result &&\n+\tgrep \"$HASH2 is the first new commit\" bisect_result &&\n+\tgit bisect reset\n+'\n+\n+test_expect_success 'bisect cannot mix old/new and good/bad' '\n+\tgit bisect start &&\n+\tgit bisect bad $HASH4 &&\n+\ttest_must_fail git bisect old $HASH1\n+'\n+\n test_done\n-- \n1.7.8\n"},{"id":"193520","messageId":"7vr4tkhys5.fsf@alter.siamese.dyndns.org","threadId":"30775","inReplyTo":"1339541765-2078-1-git-send-email-Valentin.Duperray@ensimag.imag.fr","subject":"Re: [PATCHv2] git bisect old/new","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-12T23:54:18Z","receivedAt":"2012-06-12T23:54:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Valentin Duperray writes:\n\nPlease make sure that you have Christian who took the time to review\nthe previous round on Cc:, and have addressed the issues raised in\nthe review.\n\n> Some commands are still not available for old/new:\n>\n> * git bisect start [ [...]] is not possible: the\n> commits will be treated as bad and good.\n> * git rev-list --bisect does not treat the revs/bisect/new and\n> revs/bisect/old-SHA1 files.\n> * thus, git bisect run is not available for new/old.\n> * git bisect visualize seem to work partially: the tags are\n> displayed correctly but the tree is not limited to the bisect\n> section.\n\nWould be easier to review if the subject is marked as RFC while\nthese todo items are still there.\n\nAlso before going too far into the implementation, I think it is a\ngood idea to think how you are going to address the above issues. I\nsuspect the changes to bisect.c will have to be vastly different\ndepending on that plan.\n\n> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt\n> index e4f46bc..fc63894 100644\n> --- a/Documentation/git-bisect.txt\n> +++ b/Documentation/git-bisect.txt\n> @@ -18,8 +18,10 @@ on the subcommand:\n> \n> git bisect help\n> git bisect start [--no-checkout] [ [...]] [--] [...]\n> - git bisect bad []\n> - git bisect good [...]\n> + git bisect (bad|new) []\n> + git bisect (good|old) [...]\n> + git bisect new []\n> + git bisect old [...]\n\nHuh?\n\n> +If you are not at ease with the terms \"bad\" and \"good\", perhaps\n> +because you are looking for the commit that introduced a fix, you can\n> +alternatively use \"new\" and \"old\" instead.\n> +But note that you cannot mix \"bad\" and good\" with \"new\" and \"old\".\n> +\n> +------------------------------------------------\n> +git bisect new []\n> +------------------------------------------------\n> +\n> +Same as \"git bisect bad []\".\n\nIt somewhat makes me feel uneasy to see a sentence without any verb.\n\n> +------------------------------------------------\n> +git bisect old [...]\n> +------------------------------------------------\n> +\n> +Same as \"git bisect good [...]\".\n\nLikewise.\n\n> +You must run `git bisect start` without commits as argument and run\n> +`git bisect new `/`git bisect old ...` after to add the\n> +commits.\n\nWhat happens when you do:\n\n\tgit bisect start\n git bisect new HEAD\n git bisect old v1.0.0\n\nand then\n\n git bisect bad v1.2.0\n\nDoes it error out? For that matter, what happens if you do this?\n\n\tgit bisect start\n git bisect new HEAD\n\tgit bisect good v1.0.0\n\nNote that I am not suggesting to document the behaviour here.\n\n> @@ -374,6 +401,18 @@ In this case, when 'git bisect run' finishes, bisect/bad will refer to a commit\n> has at least one parent whose reachable graph is fully traversable in the sense\n> required by 'git pack objects'.\n> \n> +* Look for a fix instead of a regression in the code\n> ++\n> +------------\n> +$ git bisect start\n> +$ git bisect new\t\t# Current version is fixed\n> +$ git bisect old bugged_version\t# bugged_version was the last version\n> +\t\t\t\t# known to be unfixed\n\nWe do not usually use \"bug\" as a transitive verb that means \"to\nbreak\". Perhaps \"buggy_version\" is easier to read and gramatically\nmore correct.\n\n> ++\n> +The \"new\" commits are the fixed ones and the \"old\" commits are the unfixed ones.\n> +At the end of the commit session, you will have the first fixed commit.\n> +\n\ncommit session? Is it a bisect session?\n\n> diff --git a/bisect.c b/bisect.c\n> index 48acf73..38de2d5 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -21,6 +21,9 @@ static const char *argv_checkout[] = {\"checkout\", \"-q\", NULL, \"--\", NULL};\n> static const char *argv_show_branch[] = {\"show-branch\", NULL, NULL};\n> static const char *argv_update_ref[] = {\"update-ref\", \"--no-deref\", \"BISECT_HEAD\", NULL, NULL};\n> \n> +static const char *bisect_bad;\n> +static const char *bisect_good;\n> +\n> /* bits #0-15 in revision.h */\n> \n> #define COUNTED\t\t(1u<<16)\n> @@ -403,9 +406,10 @@ struct commit_list *find_bisection(struct commit_list *list,\n> static int register_ref(const char *refname, const unsigned char *sha1,\n> \t\t\tint flags, void *cb_data)\n> {\n> -\tif (!strcmp(refname, \"bad\")) {\n> +\tif (!strcmp(refname, bisect_bad)) {\n> \t\tcurrent_bad_sha1 = sha1;\n> -\t} else if (!prefixcmp(refname, \"good-\")) {\n> +\t} else if (!prefixcmp(refname, \"good-\") ||\n> +\t\t\t!prefixcmp(refname, \"old-\")) {\n\nIt feels kind of strange that only one of \"bad\" or \"new\" is allowed,\nwhile \"good-X\" and \"old-Y\" can coexist and be used interchangeably.\n\n> @@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)\n> \tif (is_expected_rev(current_bad_sha1)) {\n> \t\tchar *bad_hex = sha1_to_hex(current_bad_sha1);\n> \t\tchar *good_hex = join_sha1_array_hex(&good_revs, ' ');\n> +\t\tif (!strcmp(bisect_bad,\"bad\")) {\n\ns/,/, /;\n\nBut see below. It feels wrong to always running string comparison\nwhen we know there are either good/bad mode or old/new mode.\n\n> -\tfprintf(stderr, \"Some good revs are not ancestor of the bad rev.\\n\"\n> +\tfprintf(stderr, \"Some %s revs are not ancestor of the %s rev.\\n\"\n> \t\t\"git bisect cannot work properly in this case.\\n\"\n> -\t\t\"Maybe you mistake good and bad revs?\\n\");\n> +\t\t\"Maybe you mistake %s and %s revs?\\n\",\n> +\t\tbisect_good, bisect_bad, bisect_good,\n> +\t\tbisect_bad);\n\nYou merely inherited this from the original, but shouldn't it be\n\"mistook\" in the past tense?\n\n> @@ -889,6 +900,31 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n> }\n> \n> /*\n> + * The terms used for this bisect session are stocked in\n> + * BISECT_TERMS: it can be bad/good or new/old.\n> + * We read them and stock them to adapt the messages\n> + * accordingly. Default is bad/good.\n> + */\n> +void read_bisect_terms(void)\n> +{\n> +\tstruct strbuf str = STRBUF_INIT;\n> +\tconst char *filename = git_path(\"BISECT_TERMS\");\n> +\tFILE *fp = fopen(filename, \"r\");\n> +\n> +\tif (!fp) {\n> +\t\tbisect_bad = \"bad\";\n> +\t\tbisect_good = \"good\";\n> +\t} else {\n> +\tstrbuf_getline(&str, fp, '\\n');\n> +\tbisect_bad = strbuf_detach(&str, NULL);\n> +\tstrbuf_getline(&str, fp, '\\n');\n> +\tbisect_good = strbuf_detach(&str, NULL);\n> +\t}\n> +\tstrbuf_release(&str);\n> +\tfclose(fp);\n> +}\n\nWhile this is not wrong per-se, I am not sure if storing and reading\ntwo lines from this file is really worth the trouble.\n\nWouldn't it be easier to change the convention so that the presense\nof BISECT_OLDNEW file signals that the program is working in the\nold/new mode as opposed to the traditional good/bad mode, or perhaps\na single line \"true\" or \"false\" in the file tells us if we are in\nOLDNEW mode, or something?\n"},{"id":"193533","messageId":"CAP8UFD1A+GggOOb4-GYn5O1vFWpCUZ8b98Z-WOZc4gorC4p=yg@mail.gmail.com","threadId":"30775","inReplyTo":"1339541765-2078-1-git-send-email-Valentin.Duperray@ensimag.imag.fr","subject":"Re: [PATCHv2] git bisect old/new","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2012-06-13T10:05:11Z","receivedAt":"2012-06-13T10:05:11Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jun 13, 2012 at 12:56 AM, Valentin Duperray\n wrote:\n>\n> Related discussions:\n>\n>        - http://thread.gmane.org/gmane.comp.version-control.git/86063\n>                introduced bisect fix unfixed to find fix.\n>        - http://thread.gmane.org/gmane.comp.version-control.git/182398\n>                discussion around bisect yes/no or old/new.\n\nThanks!\n\n>  /*\n> + * The terms used for this bisect session are stocked in\n> + * BISECT_TERMS: it can be bad/good or new/old.\n> + * We read them and stock them to adapt the messages\n> + * accordingly. Default is bad/good.\n> + */\n> +void read_bisect_terms(void)\n> +{\n> +       struct strbuf str = STRBUF_INIT;\n> +       const char *filename = git_path(\"BISECT_TERMS\");\n> +       FILE *fp = fopen(filename, \"r\");\n> +\n> +       if (!fp) {\n> +               bisect_bad = \"bad\";\n> +               bisect_good = \"good\";\n\nOk, but...\n\n> +       } else {\n> +       strbuf_getline(&str, fp, '\\n');\n> +       bisect_bad = strbuf_detach(&str, NULL);\n> +       strbuf_getline(&str, fp, '\\n');\n> +       bisect_good = strbuf_detach(&str, NULL);\n> +       }\n> +       strbuf_release(&str);\n> +       fclose(fp);\n> +}\n\n\n> @@ -953,6 +991,8 @@ int bisect_next_all(const char *prefix, int no_checkout)\n>               \"(roughly %d step%s)\\n\", nr, (nr == 1 ? \"\" : \"s\"),\n>               steps, (steps == 1 ? \"\" : \"s\"));\n>\n> +       free((char*)bisect_bad);\n> +       free((char*)bisect_good);\n\n...it is not a good thing to free these variables if they were not allocated.\n\n> +\n>        return bisect_checkout(bisect_rev_hex, no_checkout);\n>  }\n>        case \"$missing_good,$missing_bad,$1\" in\n>        ,,*)\n> -               : have both good and bad - ok\n> +               : have both good and bad or old and new - ok\n\nMaybe \": have both $BISECT_GOOD and $BISECT_BAD - ok\".\n\nThanks,\nChristian.\n"},{"id":"193534","messageId":"CAP8UFD1=phEzzwRBmwsLqnbM0_yAS+uMXma_uyqfdq3xo4fTGw@mail.gmail.com","threadId":"30775","inReplyTo":"CABURp0rWu0GtmSrLV+qEQJVYOt8p=B4bQtG3fzKh2f0fTGHwOQ@mail.gmail.com","subject":"Re: [PATCH] git bisect old/new","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2012-06-13T10:12:21Z","receivedAt":"2012-06-13T10:12:21Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Jun 12, 2012 at 9:41 PM, Phil Hord wrote:\n> On Tue, Jun 12, 2012 at 1:43 AM, Junio C Hamano wrote:\n>>\n>> >> @@ -403,9 +406,10 @@ struct commit_list *find_bisection(struct\n>> >> commit_list *list,\n>> >>  static int register_ref(const char *refname, const unsigned char\n>> >> *sha1,\n>> >>                        int flags, void *cb_data)\n>> >>  {\n>> >> -       if (!strcmp(refname, \"bad\")) {\n>> >> +       if (!strcmp(refname, bisect_term_bad)) {\n>> >>                current_bad_sha1 = sha1;\n>> >> -       } else if (!prefixcmp(refname, \"good-\")) {\n>> >> +       } else if (!prefixcmp(refname, \"good-\") ||\n>> >> +                       !prefixcmp(refname, \"old-\")) {\n>> >\n>> > I don't like very much \"good\" and \"old\" to be hardcoded here.\n>>\n>> Really?\n>\n> I tend to agree.  But I like more generic code and less hard-coded in\n> almost all cases.\n\nYeah, I prefer generic code too.\n\n>> >> @@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)\n>> >>        if (is_expected_rev(current_bad_sha1)) {\n>> >>                char *bad_hex = sha1_to_hex(current_bad_sha1);\n>> >>                char *good_hex = join_sha1_array_hex(&good_revs, ' ');\n>> >> +               if (!strcmp(bisect_term_bad,\"bad\")) {\n>> >> +                       fprintf(stderr, \"The merge base %s is bad.\\n\"\n>> >> +                               \"This means the bug has been fixed \"\n>> >> +                               \"between %s and [%s].\\n\",\n>> >> +                               bad_hex, bad_hex, good_hex);\n>> >> +               } else {\n>> >> +                       fprintf(stderr, \"The merge base %s is new.\\n\"\n>> >> +                               \"The property has changed \"\n>> >> +                               \"between %s and [%s].\\n\",\n>> >> +                               bad_hex, bad_hex, good_hex);\n>> >> +               }\n>> >\n>> > I don't like very much \"new\" to be harcoded here too.\n>>\n>> Why not?  It is not like we will be adding any more synonym pair\n>> beyond good/bad, so...\n>\n> Previously we discussed using yes/no, among others.\n> http://permalink.gmane.org/gmane.comp.version-control.git/182496\n\nAnd even if we add \"yes/no\" maybe people will still want to use\n\"good/bad\" instead of \"bad/good\", \"new/old\" or \"yes/no\".\n\nThanks,\nChristian.\n"},{"id":"193556","messageId":"7vzk87glwn.fsf@alter.siamese.dyndns.org","threadId":"30775","inReplyTo":"CABURp0rWu0GtmSrLV+qEQJVYOt8p=B4bQtG3fzKh2f0fTGHwOQ@mail.gmail.com","subject":"Re: [PATCH] git bisect old/new","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-13T17:30:00Z","receivedAt":"2012-06-13T17:30:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord writes:\n\n> On Tue, Jun 12, 2012 at 1:43 AM, Junio C Hamano wrote:\n>>\n>> Why not? It is not like we will be adding any more synonym pair\n>> beyond good/bad, so...\n>\n> Previously we discussed using yes/no, among others.\n> http://permalink.gmane.org/gmane.comp.version-control.git/182496\n\nYes, but didn't we dismiss yes/no and others as no better than\nbad/good after discussing?\n"},{"id":"193562","messageId":"20120613200606.Horde.QkenYnwdC4BP2NaOTf8gvnA@webmail.minatec.grenoble-inp.fr","threadId":"30775","inReplyTo":"7vr4tkhys5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2] git bisect old/new","fromName":"","fromEmail":"duperrav@minatec.inpg.fr","sentAt":"2012-06-13T18:06:06Z","receivedAt":"2012-06-13T18:06:06Z","isPatch":false,"sender":{"key":"duperrav@minatec.inpg.fr","avatar":null},"body":"\nJunio C Hamano a écrit :\n\n>> Some commands are still not available for old/new:\n>>\n>> * git bisect start [ [...]] is not possible: the\n>> commits will be treated as bad and good.\n>> * git rev-list --bisect does not treat the revs/bisect/new and\n>> revs/bisect/old-SHA1 files.\n>> * thus, git bisect run is not available for new/old.\n>> * git bisect visualize seem to work partially: the tags are\n>> displayed correctly but the tree is not limited to the bisect\n>> section.\n>\n> Would be easier to review if the subject is marked as RFC while\n> these todo items are still there.\n>\n> Also before going too far into the implementation, I think it is a\n> good idea to think how you are going to address the above issues. I\n> suspect the changes to bisect.c will have to be vastly different\n> depending on that plan.\n\n * git bisect start [ [...]]:\n\nThe idea would be to add a \"--new\" option to start in new/old mode.\n\n * git rev-list --bisect:\n\nI see two solutions for this:\n\n - read revisions from both refs/bisect/bad and refs/bisect/new\n (resp. refs/bisect/good and refs/bisect/old).\n\n - read revisions only from refs/bisect/bad and refs/bisect/good\n when the BISECT_TERMS doesn't exist or contains bad/good\n and\n read revisions only from refs/bisect/new and refs/bisect/old\n when the BISECT_TERMS exists and contains new/old.\n\nI prefer the latter because I don't really know how reading all files\nwill affect the calls of \"git rev-list\" outside of a bisect session and\nthe two types of files should not be present simultaneously anyway.\n\n> What happens when you do:\n>\n> \tgit bisect start\n> git bisect new HEAD\n> git bisect old v1.0.0\n>\n> and then\n>\n> git bisect bad v1.2.0\n>\n> Does it error out? For that matter, what happens if you do this?\n>\n> \tgit bisect start\n> git bisect new HEAD\n> \tgit bisect good v1.0.0\n>\n\nIn both cases, the `git bisect good`/`git bisect bad`command is\nconsidered invalid. The message\n\"Invalid command : you're currently in a new/old bisect.\"\nis displayed and the bisect section is not reseted.\nSame thing happens if you try to use new/old in a bad/good\nbisect session.\n\n\n>> @@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)\n>> \tif (is_expected_rev(current_bad_sha1)) {\n>> \t\tchar *bad_hex = sha1_to_hex(current_bad_sha1);\n>> \t\tchar *good_hex = join_sha1_array_hex(&good_revs, ' ');\n>> +\t\tif (!strcmp(bisect_bad,\"bad\")) {\n>\n> s/,/, /;\n>\n> But see below. It feels wrong to always running string comparison\n> when we know there are either good/bad mode or old/new mode.\n>\n>> @@ -889,6 +900,31 @@ static void show_diff_tree(const char *prefix, \n>> struct commit *commit)\n>> }\n>>\n>> /*\n>> + * The terms used for this bisect session are stocked in\n>> + * BISECT_TERMS: it can be bad/good or new/old.\n>> + * We read them and stock them to adapt the messages\n>> + * accordingly. Default is bad/good.\n>> + */\n>> +void read_bisect_terms(void)\n>> +{\n>> +\tstruct strbuf str = STRBUF_INIT;\n>> +\tconst char *filename = git_path(\"BISECT_TERMS\");\n>> +\tFILE *fp = fopen(filename, \"r\");\n>> +\n>> +\tif (!fp) {\n>> +\t\tbisect_bad = \"bad\";\n>> +\t\tbisect_good = \"good\";\n>> +\t} else {\n>> +\tstrbuf_getline(&str, fp, '\\n');\n>> +\tbisect_bad = strbuf_detach(&str, NULL);\n>> +\tstrbuf_getline(&str, fp, '\\n');\n>> +\tbisect_good = strbuf_detach(&str, NULL);\n>> +\t}\n>> +\tstrbuf_release(&str);\n>> +\tfclose(fp);\n>> +}\n>\n> While this is not wrong per-se, I am not sure if storing and reading\n> two lines from this file is really worth the trouble.\n>\n> Wouldn't it be easier to change the convention so that the presense\n> of BISECT_OLDNEW file signals that the program is working in the\n> old/new mode as opposed to the traditional good/bad mode, or perhaps\n> a single line \"true\" or \"false\" in the file tells us if we are in\n> OLDNEW mode, or something?\n\nIf there is consensus around the fact that no other terms will be added\nafter old/new, only checking if the file is present would be easier\nindeed.\n\nThanks,\n\nValentin\n"},{"id":"193642","messageId":"CAP8UFD2LBKXuey2w_-0Zy9Thi1DWCgekjCL3RULmHWVK5DXCog@mail.gmail.com","threadId":"30775","inReplyTo":"20120613200606.Horde.QkenYnwdC4BP2NaOTf8gvnA@webmail.minatec.grenoble-inp.fr","subject":"Re: [PATCHv2] git bisect old/new","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2012-06-14T09:56:02Z","receivedAt":"2012-06-14T09:56:02Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jun 13, 2012 at 8:06 PM, wrote:\n>\n> Junio C Hamano a écrit :\n>\n>\n>>> Some commands are still not available for old/new:\n>>>\n>>>     * git bisect start [ [...]] is not possible: the\n>>>       commits will be treated as bad and good.\n>>>     * git rev-list --bisect does not treat the revs/bisect/new and\n>>>       revs/bisect/old-SHA1 files.\n>>>     * thus, git bisect run is not available for new/old.\n>>>     * git bisect visualize seem to work partially: the tags are\n>>>       displayed correctly but the tree is not limited to the bisect\n>>>       section.\n>>\n>>\n>> Would be easier to review if the subject is marked as RFC while\n>> these todo items are still there.\n>>\n>> Also before going too far into the implementation, I think it is a\n>> good idea to think how you are going to address the above issues. I\n>> suspect the changes to bisect.c will have to be vastly different\n>> depending on that plan.\n>\n>\n>        * git bisect start [ [...]]:\n>\n> The idea would be to add a \"--new\" option to start in new/old mode.\n\nI am ok with that.\n\n>        * git rev-list --bisect:\n>\n> I see two solutions for this:\n>\n>        - read revisions from both refs/bisect/bad and refs/bisect/new\n>          (resp. refs/bisect/good and refs/bisect/old).\n>\n>        - read revisions only from refs/bisect/bad and refs/bisect/good\n>          when the BISECT_TERMS doesn't exist or contains bad/good\n>          and\n>          read revisions only from refs/bisect/new and refs/bisect/old\n>          when the BISECT_TERMS exists and contains new/old.\n>\n> I prefer the latter because I don't really know how reading all files\n> will affect the calls of \"git rev-list\" outside of a bisect session and\n> the two types of files should not be present simultaneously anyway.\n\nWhy didn't you consider adding another option: \"--bisect-terms-new\" or\n\"--bisect-terms=new,old\" or \"--bisect-refs=new,old\"?\n\nBy the way, I just looked at the doc for \"--bisect-vars\". This outputs\ntext ready to be eval'ed by the shell, and, among the variables it\noutputs, there are \"bisect_bad\" and \"bisect_good\".\nSo maybe we should avoid using BISECT_BAD and BISECT_GOOD shell\nvariables in git-bisect.sh to avoid confusion.\n\n>> While this is not wrong per-se, I am not sure if storing and reading\n>> two lines from this file is really worth the trouble.\n>>\n>> Wouldn't it be easier to change the convention so that the presense\n>> of BISECT_OLDNEW file signals that the program is working in the\n>> old/new mode as opposed to the traditional good/bad mode, or perhaps\n>> a single line \"true\" or \"false\" in the file tells us if we are in\n>> OLDNEW mode, or something?\n>\n>\n> If there is consensus around the fact that no other terms will be added\n> after old/new, only checking if the file is present would be easier\n> indeed.\n\nHere is the end of the previous thread where old/new was originaly discussed:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/182398/focus=183410\n\nPeff said:\n\n-------\nHmm. I think this is not quite as nice, but it is way simpler. It may be\nworth trying for a bit to see how people like it. If they don't, the\ncost of failure is that we have to maintain \"old/new\" forever, even\nafter we implement a yes/no reversible scheme. But maintaining the\nold/new mapping from yes/no would not be any harder than the good/bad\nmapping, which we would need to do anyway.\n\nSo it sounds like a reasonable first step.\n-------\n\nSo in my opinion, there was a consensus that if old/new is not enough\nit should not be a big deal to maintain anyway.\nAnd I agree with this provided that we indeed implement old/new so\nthat it's not a big deal to maintain if we change our mind later.\n(For example we might later want \"yes/no\", or \"good/bad\" with a\nmeaning reversed, or perhaps something else.)\n\nSo I'd rather have a file with a generic name like \"BISECT_TERMS\", but\nit may contain just one line like for example \"new/old\".\nWe could just check that the content of the line is \"new/old\" and\ndie(\"Only 'new/old' is supported in $GIT_DIR/BISECT_TERMS\") if it is\nsomething else.\n\nThanks,\nChristian.\n"},{"id":"193667","messageId":"7vlijper0e.fsf@alter.siamese.dyndns.org","threadId":"30775","inReplyTo":"CAP8UFD2LBKXuey2w_-0Zy9Thi1DWCgekjCL3RULmHWVK5DXCog@mail.gmail.com","subject":"Re: [PATCHv2] git bisect old/new","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-14T17:34:57Z","receivedAt":"2012-06-14T17:34:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Peff said:\n>\n> -------\n> Hmm. I think this is not quite as nice, but it is way simpler. It may be\n> worth trying for a bit to see how people like it. If they don't, the\n> cost of failure is that we have to maintain \"old/new\" forever, even\n> after we implement a yes/no reversible scheme. But maintaining the\n> old/new mapping from yes/no would not be any harder than the good/bad\n> mapping, which we would need to do anyway.\n>\n> So it sounds like a reasonable first step.\n> -------\n\nThe above is a very reasonable stand. But I do not think it leads\nto the following at all.\n\n> So I'd rather have a file with a generic name like \"BISECT_TERMS\", but\n> it may contain just one line like for example \"new/old\".\n\nThat is forcing an unnecessary complexity, when we do not even know\nif we need anything more than old/new. We can start simple and the\nresult may be sufficient and in which case we can stop there.\n\nIn fact, Peff's advice you quoted is against doing what you just\nsaid, which is to do more than we know that we need right now.\n\nIf we end up needing arbitrary pair of words, then at that point we\nmay add a mechanism that records the pair of words in use, be they\n, or . And when that happens, will\ncontinue to be supported for free--there is no extra work to support\n in addition to the work needed to support and\n that we need to support anyway.\n"},{"id":"193732","messageId":"CABURp0qGUvjtOBp8vQ4LqWsWp-5oi+-2GwVq-edQbUnKzXHR-A@mail.gmail.com","threadId":"30775","inReplyTo":"7vlijper0e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2] git bisect old/new","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2012-06-15T20:20:51Z","receivedAt":"2012-06-15T20:20:51Z","isPatch":false,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Thu, Jun 14, 2012 at 1:34 PM, Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > Peff said:\n> >\n> > -------\n> > Hmm. I think this is not quite as nice, but it is way simpler. It may be\n> > worth trying for a bit to see how people like it. If they don't, the\n> > cost of failure is that we have to maintain \"old/new\" forever, even\n> > after we implement a yes/no reversible scheme. But maintaining the\n> > old/new mapping from yes/no would not be any harder than the good/bad\n> > mapping, which we would need to do anyway.\n> >\n> > So it sounds like a reasonable first step.\n> > -------\n>\n> The above is a very reasonable stand.  But I do not think it leads\n> to the following at all.\n>\n> > So I'd rather have a file with a generic name like \"BISECT_TERMS\", but\n> > it may contain just one line like for example \"new/old\".\n>\n> That is forcing an unnecessary complexity, when we do not even know\n> if we need anything more than old/new.  We can start simple and the\n> result may be sufficient and in which case we can stop there.\n>\n> In fact, Peff's advice you quoted is against doing what you just\n> said, which is to do more than we know that we need right now.\n>\n> If we end up needing arbitrary pair of words, then at that point we\n> may add a mechanism that records the pair of words in use, be they\n> , or .  And when that happens, will\n> continue to be supported for free--there is no extra work to support\n> in addition to the work needed to support and\n> that we need to support anyway.\n\nExcept that most of the work to support is\nalready being done in this patch to add , but it will need\nre-doing or un-doing to move to arbitrary terms. Supporting arbitrary\nterms today would save work (the re-doing and the un-doring), if we\nwent that way in the future.\n\nBut perhaps we won't.\n\nPhil\n"},{"id":"193739","messageId":"7v62as9tex.fsf@alter.siamese.dyndns.org","threadId":"30775","inReplyTo":"CABURp0qGUvjtOBp8vQ4LqWsWp-5oi+-2GwVq-edQbUnKzXHR-A@mail.gmail.com","subject":"Re: [PATCHv2] git bisect old/new","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-15T21:06:30Z","receivedAt":"2012-06-15T21:06:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord writes:\n\n> Except that most of the work to support is\n> already being done in this patch to add , but it will need\n> re-doing or un-doing to move to arbitrary terms. Supporting arbitrary\n> terms today would save work (the re-doing and the un-doring), if we\n> went that way in the future.\n\nYou are forgetting that \"somebody wrote the code\" does not matter\nmuch. What counds more is that the code is more complex than\nnecessary to support the case we are immediately interested in,\nwhich means it is much more likely to have undiscovered bugs and\nwill take more effort to update if we need to change the aspects\nother than \"arbitrary two tokens\" later. \"Somebody wrote the code\nalready\" matters only when we are sure that we want to build on top\nof it. In this case, we don't.\n"}]}