Re: [PATCH 1/7] strbuf: add "include_delim" parameter to "strbuf_split"

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 13, 2009, 06:53 UTC
Message-ID: <7vfxhhj4mh.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <200903130702.01039.chriscool@tuxfamily.org>

Christian Couder <chriscool@tuxfamily.org> 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.

Show 5 quoted lines

>> 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.