From: Junio C Hamano <gitster@pobox•com>
To: Antoine Delaite <antoine.delaite@ensimag•grenoble-inp.fr>
Cc: git@vger•kernel.org, remi.lespinet@ensimag•grenoble-inp.fr,
louis--alexandre.stuber@ensimag•grenoble-inp.fr,
remi.galan-alfonso@ensimag•grenoble-inp.fr,
guillaume.pages@ensimag•grenoble-inp.fr,
Matthieu.Moy@grenoble-inp•fr, chriscool@tuxfamily•org,
thomasxnguy@gmail•com, valentinduperray@gmail•com
Subject: Re: [PATCH 4/4] bisect: add the terms old/new
Date: Mon, 08 Jun 2015 14:21:18 -0700 [thread overview]
Message-ID: <xmqqmw0999rl.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1433794930-5158-4-git-send-email-antoine.delaite@ensimag.grenoble-inp.fr> (Antoine Delaite's message of "Mon, 8 Jun 2015 22:22:10 +0200")
Antoine Delaite <antoine.delaite@ensimag•grenoble-inp.fr> writes:
> When not looking for a regression during a bisect but for a fix or a
> change in another given property, it can be confusing to use 'good'
> and 'bad'.
>
> This patch introduce `git bisect new` and `git bisect old` as an
> alternative to 'bad' and good': the commits which have a certain
> property must be marked as `new` and the ones which do not as `old`.
>
> The output will be the first commit after the change in the property.
> During a new/old bisect session you cannot use bad/good commands and
> vice-versa.
>
> `git bisect replay` works fine for old/new bisect sessions.
>
> Some commands are still not available for old/new:
>
> * git bisect start [<new> [<old>...]] is not possible: the
> commits will be treated as bad and good.
Just throwing a suggestion. You could perhaps add a new verb to be
used before starting to do anything, e.g.
$ git bisect terms new old
(where the default mode is "git bisect terms bad good") to set up
the "terms", and then after that
$ git bisect start $new $old1 $old2...
would be treated as you would naturally expect, no?
> * git rev-list --bisect does not treat the revs/bisect/new and
> revs/bisect/old-SHA1 files.
That shouldn't be hard to add, I would imagine, either by
git rev-list --bisect=new,old
or teaching "git rev-list --bisect" to read the "terms" itself, as a
follow-up patch.
> * git bisect visualize seem to work partially: the tags are
> displayed correctly but the tree is not limited to the bisect
> section.
This is directly related to the lack of "git rev-list --bisect"
support, I would imagine. If you make the command read "terms", I
suspect that it would solve itself.
> @@ -379,6 +408,21 @@ 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 HEAD # current commit is marked as new
> +$ git bisect old HEAD~10 # the tenth commit from now is marked as old
> +------------
> ++
> +If the last commit has a given property, and we are looking for the commit
> +which introduced this property.
Hmph, that is not a complete sentence and I cannot understand what
it is trying to say.
> +For each commit the bisection guide us to we
s/guide us to we/guides us to, we/;
> +will test if the property is present, if it is we will mark the commit as new
> +with 'git bisect new', otherwise we will mark it as old.
It would be easier to read as separate sentences, i.e.
s/is present, if it is/is present. If it is/;
> diff --git a/bisect.c b/bisect.c
> index 3b7df85..511b905 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -409,7 +409,8 @@ static int register_ref(const char *refname, const unsigned char *sha1,
> if (!strcmp(refname, name_bad)) {
> current_bad_oid = xmalloc(sizeof(*current_bad_oid));
> hashcpy(current_bad_oid->hash, sha1);
> - } else if (starts_with(refname, "good-")) {
> + } else if (starts_with(refname, "good-") ||
> + starts_with(refname, "old-")) {
> sha1_array_append(&good_revs, sha1);
> } else if (starts_with(refname, "skip-")) {
> sha1_array_append(&skipped_revs, sha1);
> @@ -741,6 +742,12 @@ static void handle_bad_merge_base(void)
> "between %s and [%s].\n",
> bad_hex, bad_hex, good_hex);
> }
> + else if (!strcmp(name_bad, "new")) {
> + fprintf(stderr, "The merge base %s is new.\n"
> + "The property has changed "
> + "between %s and [%s].\n",
> + bad_hex, bad_hex, good_hex);
> + }
After reading the previous patches in the series, I expected that
this new code would know to read "terms" and does not limit us only
to "new" and "old". Somewhat disappointing.
> @@ -439,7 +441,7 @@ bisect_replay () {
> start)
> cmd="bisect_start $rev"
> eval "$cmd" ;;
> - good|bad|skip)
> + good|bad|skip|old|new)
Not NAME_GOOD / NAME_BAD?
To support replay, you may want to consider the "bisect terms"
approach and when the bisection is using non-standard terms record
that as the first entry to the bisect log.
next prev parent reply other threads:[~2015-06-08 21:21 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-08 20:22 [PATCH 1/4] bisect : correction of typo Antoine Delaite
2015-06-08 20:22 ` [PATCH 2/4] bisect: replace hardcoded "bad|good" by variables Antoine Delaite
2015-06-08 20:30 ` Junio C Hamano
2015-06-09 6:45 ` Matthieu Moy
2015-06-09 8:12 ` Christian Couder
2015-06-09 12:39 ` Matthieu Moy
2015-06-09 19:18 ` Junio C Hamano
2015-06-08 20:22 ` [PATCH 3/4] bisect: simplify the add of new bisect terms Antoine Delaite
2015-06-08 20:42 ` Junio C Hamano
2015-06-09 18:17 ` Antoine Delaite
2015-06-10 16:30 ` Matthieu Moy
2015-06-09 7:01 ` Matthieu Moy
2015-06-09 8:39 ` Christian Couder
2015-06-09 20:17 ` Louis-Alexandre Stuber
2015-06-10 0:39 ` Junio C Hamano
2015-06-10 7:15 ` Louis-Alexandre Stuber
2015-06-10 8:03 ` Matthieu Moy
2015-06-10 9:41 ` Louis-Alexandre Stuber
2015-06-10 15:24 ` Matthieu Moy
2015-06-10 15:10 ` Junio C Hamano
2015-06-10 15:25 ` Matthieu Moy
2015-06-10 16:11 ` Junio C Hamano
2015-06-08 20:22 ` [PATCH 4/4] bisect: add the terms old/new Antoine Delaite
2015-06-08 21:21 ` Junio C Hamano [this message]
[not found] <78277223.323387.1433874176840.JavaMail.zimbra@ensimag.grenoble-inp.fr>
2015-06-10 7:11 ` Antoine Delaite
2015-06-10 8:09 ` Matthieu Moy
2015-06-10 15:24 ` Junio C Hamano
2015-06-10 15:47 ` Christian Couder
2015-06-10 16:08 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqmw0999rl.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=Matthieu.Moy@grenoble-inp$(echo .)fr \
--cc=antoine.delaite@ensimag$(echo .)grenoble-inp.fr \
--cc=chriscool@tuxfamily$(echo .)org \
--cc=git@vger$(echo .)kernel.org \
--cc=guillaume.pages@ensimag$(echo .)grenoble-inp.fr \
--cc=louis--alexandre.stuber@ensimag$(echo .)grenoble-inp.fr \
--cc=remi.galan-alfonso@ensimag$(echo .)grenoble-inp.fr \
--cc=remi.lespinet@ensimag$(echo .)grenoble-inp.fr \
--cc=thomasxnguy@gmail$(echo .)com \
--cc=valentinduperray@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox