From: Jon Seymour <jon.seymour@gmail.com>
Date: Wed, 03 Aug 2011 21:37:20 GMT
Subject: Re: [PATCH v15 7/7] bisect: add documentation for --no-checkout option.
Message-ID: <CAH3Anrq_xAUDN5UW6XB+VvGbCB3OrM0WdKRUKTAufVx5xCa+MA@mail.gmail.com>
In-Reply-To: <7v62meb4a3.fsf@alter.siamese.dyndns.org>

On Thu, Aug 4, 2011 at 6:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jon Seymour <jon.seymour@gmail.com> writes:
>
>> +This option is useful in circumstances in which checkout is either not
>> +possible (because the repository is damaged) or is otherwise not required.
>
> I do not think "because the repository is damaged" is a particularly good
> example to single out here. For one thing, you may not be able to bisect
> the history in a damaged repository due to unreadable commits. There are
> other valid and more plausible reasons that the users do not want to
> update the working tree while bisecting, e.g. they have local changes,
> they do not want to rebuild everything after bisection is done, etc.
>
> Also the above description muddies what is prerequisite and what is user's
> wish.
>
>  - "is otherwise not required" is the prerequisite: "if and only if the
>   test you would perform during bisection does not need a checkout of the
>   version being tested, you can use this option";
>
>  - Even if the user wishes to use this option for various reasons (e.g. so
>   that the next "make" wouldn't have to rebuild everything unnecessarily;
>   or the repository has corrupt blobs and checkout of some versions may
>   fail), if the test _needs_ a checkout, it is not an option to use this
>   feature.
>
> So I would suggest toning it down perhaps like this:
>
>    This option may be useful when the test you would perform in each step
>    does not require a checked out tree.
>

Sounds good.

>> +$ git bisect run eval '
>> +rc=1;
>> +if git rev-list --objects BISECT_HEAD >tmp.$$; then
>> +   git pack-objects --stdout >/dev/null < tmp.$$ && rc=0;
>> +fi;
>> +rm tmp.$$;
>> +test $rc -eq 0;'
>
> Hmm, "eval"?
>
> More importantly, aren't these excessive and unnecessary semicolons
> unsightly?
>
>        $ git bisect run sh -c '
>                git rev-list --objects BISECT_HEAD >tmp.$$ &&
>                git pack-objects --stdout >/dev/null <tmp.$$
>                rc=$?
>                rm -f tmp.$$
>                test $rc = 0'
>
>> +In this case, when 'git bisect run' finishes, bisect/bad will refer to a commit that
>> +has at least one parent whose reachable graph is fully traversable in the sense
>> +required by 'git pack objects'.
>
> Also "everything reachable from BISECT_HEAD" feels unnecessarily
> wasteful. By the time you invoke "bisect run", you already know one good
> commit, everything reachable from which are known to be good.
>

How about?

git bisect run sh -c '
        GOOD=$(git for-each-ref "--format=%(objectname)" refs/bisect/good-*) &&
        git rev-list --objects BISECT_HEAD --not $GOOD >tmp.$$ &&
        git pack-objects --stdout >/dev/null <tmp.$$
        rc=$?
        rm -f tmp.$$
        test $rc = 0'