Christian Couder [off-list ref] writes:
Aren't gets and fgets an example that having the choice to strip out the
delimiter or not is good?
I consider it is an example that an earlier simplistic API (gets()) later
gets corrected when it is redone right (fgets()) with other changes like a
proper bounds check.
quoted
A more general
solution should be by default not to strip it, and I do not think your
new caller, if it were written correctly, needs stripping behaviour
either. That means there is no need for the "optionally strip" flag to
the function in order to support the rest of the series [*2*].
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.
Le vendredi 13 mars 2009, Junio C Hamano a écrit :
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.