Valentin Duperray <Valentin.Duperray@xxxxxxxxxxxxxxx> writes:
Please make sure that you have Christian who took the time to review
the previous round on Cc:, and have addressed the issues raised in
the review.
> Some commands are still not available for old/new:
>
> * git bisect start [<new> [<old>...]] is not possible: the
> commits will be treated as bad and good.
> * git rev-list --bisect does not treat the revs/bisect/new and
> revs/bisect/old-SHA1 files.
> * thus, git bisect run <cmd> is not available for new/old.
> * git bisect visualize seem to work partially: the tags are
> displayed correctly but the tree is not limited to the bisect
> section.
Would be easier to review if the subject is marked as RFC while
these todo items are still there.
Also before going too far into the implementation, I think it is a
good idea to think how you are going to address the above issues. I
suspect the changes to bisect.c will have to be vastly different
depending on that plan.
> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
> index e4f46bc..fc63894 100644
> --- a/Documentation/git-bisect.txt
> +++ b/Documentation/git-bisect.txt
> @@ -18,8 +18,10 @@ on the subcommand:
>
> git bisect help
> git bisect start [--no-checkout] [<bad> [<good>...]] [--] [<paths>...]
> - git bisect bad [<rev>]
> - git bisect good [<rev>...]
> + git bisect (bad|new) [<rev>]
> + git bisect (good|old) [<rev>...]
> + git bisect new [<rev>]
> + git bisect old [<rev>...]
Huh?
> +If you are not at ease with the terms "bad" and "good", perhaps
> +because you are looking for the commit that introduced a fix, you can
> +alternatively use "new" and "old" instead.
> +But note that you cannot mix "bad" and good" with "new" and "old".
> +
> +------------------------------------------------
> +git bisect new [<rev>]
> +------------------------------------------------
> +
> +Same as "git bisect bad [<rev>]".
It somewhat makes me feel uneasy to see a sentence without any verb.
> +------------------------------------------------
> +git bisect old [<rev>...]
> +------------------------------------------------
> +
> +Same as "git bisect good [<rev>...]".
Likewise.
> +You must run `git bisect start` without commits as argument and run
> +`git bisect new <rev>`/`git bisect old <rev>...` after to add the
> +commits.
What happens when you do:
git bisect start
git bisect new HEAD
git bisect old v1.0.0
and then
git bisect bad v1.2.0
Does it error out? For that matter, what happens if you do this?
git bisect start
git bisect new HEAD
git bisect good v1.0.0
Note that I am not suggesting to document the behaviour here.
> @@ -374,6 +401,18 @@ In this case, when 'git bisect run' finishes, bisect/bad will refer to a commit
> has at least one parent whose reachable graph is fully traversable in the sense
> required by 'git pack objects'.
>
> +* Look for a fix instead of a regression in the code
> ++
> +------------
> +$ git bisect start
> +$ git bisect new # Current version is fixed
> +$ git bisect old bugged_version # bugged_version was the last version
> + # known to be unfixed
We do not usually use "bug" as a transitive verb that means "to
break". Perhaps "buggy_version" is easier to read and gramatically
more correct.
> ++
> +The "new" commits are the fixed ones and the "old" commits are the unfixed ones.
> +At the end of the commit session, you will have the first fixed commit.
> +
commit session? Is it a bisect session?
> diff --git a/bisect.c b/bisect.c
> index 48acf73..38de2d5 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -21,6 +21,9 @@ static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
> static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
> static const char *argv_update_ref[] = {"update-ref", "--no-deref", "BISECT_HEAD", NULL, NULL};
>
> +static const char *bisect_bad;
> +static const char *bisect_good;
> +
> /* bits #0-15 in revision.h */
>
> #define COUNTED (1u<<16)
> @@ -403,9 +406,10 @@ struct commit_list *find_bisection(struct commit_list *list,
> static int register_ref(const char *refname, const unsigned char *sha1,
> int flags, void *cb_data)
> {
> - if (!strcmp(refname, "bad")) {
> + if (!strcmp(refname, bisect_bad)) {
> current_bad_sha1 = sha1;
> - } else if (!prefixcmp(refname, "good-")) {
> + } else if (!prefixcmp(refname, "good-") ||
> + !prefixcmp(refname, "old-")) {
It feels kind of strange that only one of "bad" or "new" is allowed,
while "good-X" and "old-Y" can coexist and be used interchangeably.
> @@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)
> if (is_expected_rev(current_bad_sha1)) {
> char *bad_hex = sha1_to_hex(current_bad_sha1);
> char *good_hex = join_sha1_array_hex(&good_revs, ' ');
> + if (!strcmp(bisect_bad,"bad")) {
s/,/, /;
But see below. It feels wrong to always running string comparison
when we know there are either good/bad mode or old/new mode.
> - fprintf(stderr, "Some good revs are not ancestor of the bad rev.\n"
> + fprintf(stderr, "Some %s revs are not ancestor of the %s rev.\n"
> "git bisect cannot work properly in this case.\n"
> - "Maybe you mistake good and bad revs?\n");
> + "Maybe you mistake %s and %s revs?\n",
> + bisect_good, bisect_bad, bisect_good,
> + bisect_bad);
You merely inherited this from the original, but shouldn't it be
"mistook" in the past tense?
> @@ -889,6 +900,31 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
> }
>
> /*
> + * The terms used for this bisect session are stocked in
> + * BISECT_TERMS: it can be bad/good or new/old.
> + * We read them and stock them to adapt the messages
> + * accordingly. Default is bad/good.
> + */
> +void read_bisect_terms(void)
> +{
> + struct strbuf str = STRBUF_INIT;
> + const char *filename = git_path("BISECT_TERMS");
> + FILE *fp = fopen(filename, "r");
> +
> + if (!fp) {
> + bisect_bad = "bad";
> + bisect_good = "good";
> + } else {
> + strbuf_getline(&str, fp, '\n');
> + bisect_bad = strbuf_detach(&str, NULL);
> + strbuf_getline(&str, fp, '\n');
> + bisect_good = strbuf_detach(&str, NULL);
> + }
> + strbuf_release(&str);
> + fclose(fp);
> +}
While this is not wrong per-se, I am not sure if storing and reading
two lines from this file is really worth the trouble.
Wouldn't it be easier to change the convention so that the presense
of BISECT_OLDNEW file signals that the program is working in the
old/new mode as opposed to the traditional good/bad mode, or perhaps
a single line "true" or "false" in the file tells us if we are in
OLDNEW mode, or something?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html