Migrate bisect to C (was: [PATCH 1/7] strbuf: add "include_delim" parameter to "strbuf_split")

From: Christian Couder <chriscool@tuxfamily.org>
Date: Mar 14, 2009, 07:46 UTC
Message-ID: <200903140846.17599.chriscool@tuxfamily.org>
In-Reply-To: <7vfxhhj4mh.fsf@gitster.siamese.dyndns.org>

Le vendredi 13 mars 2009, Junio C Hamano a écrit :

Show 24 quoted lines

>
> I also do not agree that you have to keep list of skip both in shell and
> rev-list when you go the route I suggested.  I think a separate bisect.c
> you did is a good first step to make not just the bisect machinery but
> the whole bisect command into a built-in, and even if we do not do the
> full rewrite in C in one go, moving these "shell script reads from
> refs/bisect only to feed the result to rev-list --bisect" pattern to
> "shell script updates refs/bisect and let rev-list --bisect read from
> there" pattern would be a good initial step.  Oh, and I did not mean it
> only for "skip", but also doing this for "good" and "bad" as well.
>
> For example, you read "refs/bisect/skip-*" and keep that in $skip to:
>
>  - feed it to filter_skipped() which you are making built-in with this
>    series;
>
>  - feed it to check_good_are_ancestors_of_bad that in turn calls
>    check_merge_bases;
>
> and its use is contained in bisect_next() alone.  After this series is
> done, we can move the logic in check_good_are... to bisect.c and you do
> not have to read refs/bisect/skip-* in the shell anymore.  IOW, we can
> migrate away from the "shell reads from refs/bisect/ and feeds that to
> rev-list --bisect" pattern incrementally.

Do you mean that you want this series to migrate both "filter_skipped" and "check_good_are_ancestors_of_bad" to C? Or is it ok if "check_good_are_ancestors_of_bad" migrates later?

If it is ok to migrate "check_good_are_ancestors_of_bad" later, then I think something like the 8/7 patch I posted yesterday might be a good way, because I think a "--bisect-read-refs" option that read refs from "refs/bisect/*" would not fit well in "git rev-list".

Because, the "git rev-list" usage is:

git rev-list [OPTION] <commit-id>... [ -- paths... ]

That means that at least one <commit-id> should always be passed to "git rev-list".

So it would be strange to have to pass a commit on the command line when using the "--bisect-read-refs" option. And I think it would not be very consistent to change the usage like this:

git rev-list [OPTION] [ --bisect-read-refs | <commit-id>... ] [ -- paths... ]

Also when we migrate "check_good_are_ancestors_of_bad" to C, we will probably have to move the code that checks out the source code ("bisect_checkout" shell function), because "check_good_are_ancestors_of_bad" can call "bisect_checkout". And I don't think that the checkout behavior would fit well in "git rev-list".

That's why I suggested to add a new "git rev-bisect" plumbing command that would read refs from "refs/bisect/*" and that could later be fitted with the "bisect_checkout" and "check_good_are_ancestors_of_bad" behavior.

Best regards, Christian.