{"thread":{"id":"18275","subject":"[PATCH 1/7] strbuf: add \"include_delim\" parameter to \"strbuf_split\"","startedAt":"2009-03-12T07:51:03Z","lastAt":"2009-03-14T12:09:30Z","messageCount":9,"participants":["Christian Couder","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"107772","messageId":"20090312085103.e83b34a5.chriscool@tuxfamily.org","threadId":"18275","inReplyTo":null,"subject":"[PATCH 1/7] strbuf: add \"include_delim\" parameter to \"strbuf_split\"","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-03-12T07:51:03Z","receivedAt":"2009-03-12T07:51:03Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"The \"strbuf_split\" function used to include the delimiter character\nat the end of the splited strbufs it produced.\n\nThis behavior is not wanted in many cases, so this patch adds a new\n\"include_delim\" parameter to the function to let us switch it on or\noff as we want.\n\nSigned-off-by: Christian Couder \n---\n builtin-mailinfo.c | 2 +-\n imap-send.c | 2 +-\n strbuf.c | 8 +++++---\n strbuf.h | 2 +-\n 4 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex 2789ccd..b96ea1a 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -814,7 +814,7 @@ static void handle_body(void)\n \t\t\t * multiple new lines. Pass only one chunk\n \t\t\t * at a time to handle_filter()\n \t\t\t */\n-\t\t\tlines = strbuf_split(&line, '\\n');\n+\t\t\tlines = strbuf_split(&line, '\\n', 1);\n \t\t\tfor (it = lines; (sb = *it); it++) {\n \t\t\t\tif (*(it + 1) == NULL) /* The last line */\n \t\t\t\t\tif (sb->buf[sb->len - 1] != '\\n') {\ndiff --git a/imap-send.c b/imap-send.c\nindex cb518eb..a27f744 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1289,7 +1289,7 @@ static void wrap_in_html(struct msg_data *msg)\n \tint added_header = 0;\n \n \tstrbuf_attach(&buf, msg->data, msg->len, msg->len);\n-\tlines = strbuf_split(&buf, '\\n');\n+\tlines = strbuf_split(&buf, '\\n', 1);\n \tstrbuf_release(&buf);\n \tfor (p = lines; *p; p++) {\n \t\tif (! added_header) {\ndiff --git a/strbuf.c b/strbuf.c\nindex 6ed0684..4ef9084 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -97,7 +97,9 @@ void strbuf_tolower(struct strbuf *sb)\n \t\tsb->buf[i] = tolower(sb->buf[i]);\n }\n \n-struct strbuf **strbuf_split(const struct strbuf *sb, int delim)\n+struct strbuf **strbuf_split(const struct strbuf *sb,\n+\t\t\t int delim,\n+\t\t\t int include_delim)\n {\n \tint alloc = 2, pos = 0;\n \tchar *n, *p;\n@@ -114,8 +116,8 @@ struct strbuf **strbuf_split(const struct strbuf *sb, int delim)\n \t\t\tret = xrealloc(ret, sizeof(struct strbuf *) * alloc);\n \t\t}\n \t\tif (!n)\n-\t\t\tn = sb->buf + sb->len - 1;\n-\t\tlen = n - p + 1;\n+\t\t\tn = sb->buf + sb->len - (include_delim ? 1 : 0);\n+\t\tlen = n - p + (include_delim ? 1 : 0);\n \t\tt = xmalloc(sizeof(struct strbuf));\n \t\tstrbuf_init(t, len);\n \t\tstrbuf_add(t, p, len);\ndiff --git a/strbuf.h b/strbuf.h\nindex 89bd36e..2202d28 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -83,7 +83,7 @@ extern void strbuf_ltrim(struct strbuf *);\n extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);\n extern void strbuf_tolower(struct strbuf *);\n \n-extern struct strbuf **strbuf_split(const struct strbuf *, int delim);\n+extern struct strbuf **strbuf_split(const struct strbuf *, int delim, int include_delim);\n extern void strbuf_list_free(struct strbuf **);\n \n /*----- add data in your buffer -----*/\n-- \n1.6.2.83.g012a16.dirty\n"},{"id":"107900","messageId":"200903130548.30370.chriscool@tuxfamily.org","threadId":"18275","inReplyTo":"20090312190846.6117@nanako3.lavabit.com","subject":"Re: [PATCH 1/7] strbuf: add \"include_delim\" parameter to \"strbuf_split\"","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-03-13T04:48:30Z","receivedAt":"2009-03-13T04:48:30Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le jeudi 12 mars 2009, Nanako Shiraishi a écrit :\n> Quoting Christian Couder :\n> > The \"strbuf_split\" function used to include the delimiter character\n> > at the end of the splited strbufs it produced.\n> >\n> > This behavior is not wanted in many cases, so this patch adds a new\n> > \"include_delim\" parameter to the function to let us switch it on or\n> > off as we want.\n>\n> Sorry, but I don't understand the above claim. You say \"not wanted in\n> many cases\" but your patch updates the existing callers, all of which do\n> want to include the delimiter.\n\nIn many programming languages, like Perl and Python for example, there is \na \"split\" function that splits strings, and by default the resulting \nstrings don't include the delimiter.\n\nIn Git there are only 2 existing callers and I think this function could be \nused a lot more if there was a way not to include the delimiter.\n\nIn my patch series I add one caller that don't want the delimiter so after \nmy patch series there are already half as many callers that don't want the \ndelimiter.\n\nAnd by the way, when I mentored the GSoC sequencer project I suggested to \nStephan to use strbuf_split, but we also had the problem that the delimiter \nwas included.\n\n> The patch would easily justify itself if it made the callers pass 0 to\n> the function to decline the delimiter, and as the result it made the\n> codepaths that use the result simpler. But I don't think that is what\n> your patch does.\n\nYes, my patch does not do that, because I think including the delimiter is a \nspecial case of the more general and useful behavior of not including it.\n\nBest regards,\nChristian.\n"},{"id":"107901","messageId":"7v1vt2j91x.fsf@gitster.siamese.dyndns.org","threadId":"18275","inReplyTo":"200903130548.30370.chriscool@tuxfamily.org","subject":"Re: [PATCH 1/7] strbuf: add \"include_delim\" parameter to \"strbuf_split\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-13T05:17:30Z","receivedAt":"2009-03-13T05:17:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Yes, my patch does not do that, because I think including the delimiter is a \n> special case of the more general and useful behavior of not including it.\n\nYou got it backwards.\n\nWith the way the returned string is used by the single caller that your\npatch adds (which splits at \",\"), I would agree that lack of delimiter\nallows the result to get used directly in the further processing.\n\nBut even in that codepath, I have to say that it is just lazy programming\nthat the caller does not postprocess the returned value from the splitter\nfunction. If it wants not just accept input such as \"a,b,c\" but also\nwants to tolerate things like \"a, b, c\", it will have to look at the\nresulting string, and ignoring the delimiter at the end becomes just a\nsmall part of the general clean-up process [*1*].\n\nOnce you start allowing \"split at one of these characters\" and/or \"split\nat delimiter that matches this pattern\", you cannot just discard the\ndelimiter if you want to support non-simplistic callers, because they\nwould want to know what the delimiter was.\n\nStripping out the delimiter is the special case for simplistic callers\n(think \"gets()\" that strips, and \"fgets()\" that doesn't). A more general\nsolution should be by default not to strip it, and I do not think your new\ncaller, if it were written correctly, needs stripping behaviour either.\nThat means there is no need for the \"optionally strip\" flag to the\nfunction in order to support the rest of the series [*2*].\n\nAlso comparing this with Perl/Python split() forgets that you are working\nin C, where truncating an existing string is quite cheap (just assign '\\0').\nThere is a different trade-off to be made in these language environments.\n\n[Footnote]\n\n*1* and this is not a made-up feature enhancement request. If you tell\npeople that they can give more than one value with comma separated, some\nof them _will_ feed you --option=\"a, b, c\". Your parser can error out by\nsaying \"I do not understand ' b'\", but you will be told \"What other\npossible interpretation is there for that thing, other than 'b'!\", and\nwould grudgingly have to change your code to accept such a list.\n\n*2* In any case, as I told you in a separate review comments, I think\npassing a huge list as a command line parameter, be it separated with\ncomma or whatever, is not an appropriate way to solve the issue of\nfilter_skipped() your primary topic seems to be trying to address, so I\nexpect your series would not need strbuf_split() at all. You would most\nlikely be calling for_each_ref(), looking at the refs under \"refs/bisect\"\nhierarchy, instead of having shell feed you the list.\n"},{"id":"107906","messageId":"200903130702.01039.chriscool@tuxfamily.org","threadId":"18275","inReplyTo":"7v1vt2j91x.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/7] strbuf: add \"include_delim\" parameter to \"strbuf_split\"","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-03-13T06:02:00Z","receivedAt":"2009-03-13T06:02:00Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le vendredi 13 mars 2009, Junio C Hamano a écrit :\n> Christian Couder writes:\n> > Yes, my patch does not do that, because I think including the delimiter\n> > is a special case of the more general and useful behavior of not\n> > including it.\n>\n> You got it backwards.\n>\n> With the way the returned string is used by the single caller that your\n> patch adds (which splits at \",\"), I would agree that lack of delimiter\n> allows the result to get used directly in the further processing.\n>\n> But even in that codepath, I have to say that it is just lazy programming\n> that the caller does not postprocess the returned value from the splitter\n> function. If it wants not just accept input such as \"a,b,c\" but also\n> wants to tolerate things like \"a, b, c\", it will have to look at the\n> resulting string, and ignoring the delimiter at the end becomes just a\n> small part of the general clean-up process [*1*].\n\nI think talking about \"lazy programming\" in this case is a bit strong, \nbecause first \"git rev-list --bisect-skip\" is plumbing and will be used \nmostly by porcelain and second because there are much more common shell \nutilities that don't tolerate things like \"a, b, c\". Try using \"cut\" \nwith -f'1, 2' (instead of -f1,2) for example.\n\n> Once you start allowing \"split at one of these characters\" and/or \"split\n> at delimiter that matches this pattern\", you cannot just discard the\n> delimiter if you want to support non-simplistic callers, because they\n> would want to know what the delimiter was.\n\nBut I let non simplistic callers use \"1\" as the last parameter if they want \nthe delimiter. I just give one more way to use strbuf_split. I don't remove \nanything.\n\n> Stripping out the delimiter is the special case for simplistic callers\n> (think \"gets()\" that strips, and \"fgets()\" that doesn't). \n\nAren't gets and fgets an example that having the choice to strip out the \ndelimiter or not is good?\n\n> A more general \n> solution should be by default not to strip it, and I do not think your\n> new caller, if it were written correctly, needs stripping behaviour\n> either. That means there is no need for the \"optionally strip\" flag to\n> the function in order to support the rest of the series [*2*].\n\nI think my patch 8/7, that I just sent, is a good solution and it still uses \nthe new behavior of strbuf_split introduced in 1/7.\n\n> Also comparing this with Perl/Python split() forgets that you are working\n> in C, where truncating an existing string is quite cheap (just assign\n> '\\0'). There is a different trade-off to be made in these language\n> environments.\n\nSorry but I think the goal of the strbuf API is to be quite high level, so I \nthink comparing this with Perl/Python is ok.\n\nBest regards,\nChristian.\n"},{"id":"107907","messageId":"7vfxhhj4mh.fsf@gitster.siamese.dyndns.org","threadId":"18275","inReplyTo":"200903130702.01039.chriscool@tuxfamily.org","subject":"Re: [PATCH 1/7] strbuf: add \"include_delim\" parameter to \"strbuf_split\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-13T06:53:10Z","receivedAt":"2009-03-13T06:53:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Aren't gets and fgets an example that having the choice to strip out the \n> delimiter or not is good?\n\nI consider it is an example that an earlier simplistic API (gets()) later\ngets corrected when it is redone right (fgets()) with other changes like a\nproper bounds check.\n\n>> A more general \n>> solution should be by default not to strip it, and I do not think your\n>> new caller, if it were written correctly, needs stripping behaviour\n>> either. That means there is no need for the \"optionally strip\" flag to\n>> the function in order to support the rest of the series [*2*].\n\nI also do not agree that you have to keep list of skip both in shell and\nrev-list when you go the route I suggested. I think a separate bisect.c\nyou did is a good first step to make not just the bisect machinery but the\nwhole bisect command into a built-in, and even if we do not do the full\nrewrite in C in one go, moving these \"shell script reads from refs/bisect\nonly to feed the result to rev-list --bisect\" pattern to \"shell script\nupdates refs/bisect and let rev-list --bisect read from there\" pattern\nwould be a good initial step. Oh, and I did not mean it only for \"skip\",\nbut also doing this for \"good\" and \"bad\" as well.\n\nFor example, you read \"refs/bisect/skip-*\" and keep that in $skip to:\n\n - feed it to filter_skipped() which you are making built-in with this\n series;\n\n - feed it to check_good_are_ancestors_of_bad that in turn calls\n check_merge_bases;\n\nand its use is contained in bisect_next() alone. After this series is\ndone, we can move the logic in check_good_are... to bisect.c and you do\nnot have to read refs/bisect/skip-* in the shell anymore. IOW, we can\nmigrate away from the \"shell reads from refs/bisect/ and feeds that to\nrev-list --bisect\" pattern incrementally.\n"},{"id":"107908","messageId":"7v7i2tj411.fsf@gitster.siamese.dyndns.org","threadId":"18275","inReplyTo":"200903130702.01039.chriscool@tuxfamily.org","subject":"Re: [PATCH 1/7] strbuf: add \"include_delim\" parameter to \"strbuf_split\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-13T07:06:02Z","receivedAt":"2009-03-13T07:06:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n>> Also comparing this with Perl/Python split() forgets that you are working\n>> in C, where truncating an existing string is quite cheap (just assign\n>> '\\0'). There is a different trade-off to be made in these language\n>> environments.\n>\n> Sorry but I think the goal of the strbuf API is to be quite high level, so I \n> think comparing this with Perl/Python is ok.\n\nI think you are mistaken again.\n\nThe highlevel-ness of strbuf holds true only while you live in the strbuf\nworld, but you should consider what happens when its end product finally\ngets used in the calling function written in C. It will be a good-old NUL\nterminated string or just a chunk of memory with known length.\n\nFor a user written in C, it is far easier and cheaper to trim excess at\nthe end, especially when the length of the string is known (strbuf_detach\ngives you the length of the string exactly for this reason) than having to\nextend it (in order to recover the lost delimiter because the splitter\nremoved it). Trimming from the front is also cheap (just move the head\npointer and start consuming from the middle).\n"},{"id":"108001","messageId":"200903140846.17599.chriscool@tuxfamily.org","threadId":"18275","inReplyTo":"7vfxhhj4mh.fsf@gitster.siamese.dyndns.org","subject":"Migrate bisect to C (was: [PATCH 1/7] strbuf: add \"include_delim\" parameter to \"strbuf_split\")","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-03-14T07:46:17Z","receivedAt":"2009-03-14T07:46:17Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le vendredi 13 mars 2009, Junio C Hamano a écrit :\n>\n> I also do not agree that you have to keep list of skip both in shell and\n> rev-list when you go the route I suggested. I think a separate bisect.c\n> you did is a good first step to make not just the bisect machinery but\n> the whole bisect command into a built-in, and even if we do not do the\n> full rewrite in C in one go, moving these \"shell script reads from\n> refs/bisect only to feed the result to rev-list --bisect\" pattern to\n> \"shell script updates refs/bisect and let rev-list --bisect read from\n> there\" pattern would be a good initial step. Oh, and I did not mean it\n> only for \"skip\", but also doing this for \"good\" and \"bad\" as well.\n>\n> For example, you read \"refs/bisect/skip-*\" and keep that in $skip to:\n>\n> - feed it to filter_skipped() which you are making built-in with this\n> series;\n>\n> - feed it to check_good_are_ancestors_of_bad that in turn calls\n> check_merge_bases;\n>\n> and its use is contained in bisect_next() alone. After this series is\n> done, we can move the logic in check_good_are... to bisect.c and you do\n> not have to read refs/bisect/skip-* in the shell anymore. IOW, we can\n> migrate away from the \"shell reads from refs/bisect/ and feeds that to\n> rev-list --bisect\" pattern incrementally.\n\nDo you mean that you want this series to migrate both \"filter_skipped\" and\n\"check_good_are_ancestors_of_bad\" to C? Or is it ok \nif \"check_good_are_ancestors_of_bad\" migrates later?\n\nIf it is ok to migrate \"check_good_are_ancestors_of_bad\" later, then I think \nsomething like the 8/7 patch I posted yesterday might be a good way, \nbecause I think a \"--bisect-read-refs\" option that read refs \nfrom \"refs/bisect/*\" would not fit well in \"git rev-list\".\n\nBecause, the \"git rev-list\" usage is:\n\ngit rev-list [OPTION] ... [ -- paths... ]\n\nThat means that at least one should always be passed to \"git \nrev-list\".\n\nSo it would be strange to have to pass a commit on the command line when \nusing the \"--bisect-read-refs\" option. And I think it would not be very \nconsistent to change the usage like this:\n\ngit rev-list [OPTION] [ --bisect-read-refs | ... ] [ -- \npaths... ]\n\nAlso when we migrate \"check_good_are_ancestors_of_bad\" to C, we will \nprobably have to move the code that checks out the source code \n(\"bisect_checkout\" shell function), \nbecause \"check_good_are_ancestors_of_bad\" can call \"bisect_checkout\".\nAnd I don't think that the checkout behavior would fit well in \"git \nrev-list\".\n\nThat's why I suggested to add a new \"git rev-bisect\" plumbing command that \nwould read refs from \"refs/bisect/*\" and that could later be fitted with \nthe \"bisect_checkout\" and \"check_good_are_ancestors_of_bad\" behavior.\n\nBest regards,\nChristian.\n"},{"id":"108003","messageId":"7v63iccydg.fsf@gitster.siamese.dyndns.org","threadId":"18275","inReplyTo":"200903140846.17599.chriscool@tuxfamily.org","subject":"Re: Migrate bisect to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-14T08:16:59Z","receivedAt":"2009-03-14T08:16:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Do you mean that you want this series to migrate both \"filter_skipped\" and\n> \"check_good_are_ancestors_of_bad\" to C? Or is it ok \n> if \"check_good_are_ancestors_of_bad\" migrates later?\n\nOne small step at a time. That's the only sane way we can get there.\n\n> If it is ok to migrate \"check_good_are_ancestors_of_bad\" later, then I think \n> something like the 8/7 patch I posted yesterday might be a good way, \n\nIn the final shape, you will be reading from refs/bisect namespace using\nfor_each_ref(), and at that point you won't have anybody feeding the\nskipped from the standard input. The code you would add in [8/7] would\nhave to be removed if you go that route. If you make the filter_skipped\ncodepath to read from for_each_ref() during this round, you can still keep\nthat codepath even after you fully migrate everybody to C, no?\n\n> ...\n> That means that at least one should always be passed to \"git \n> rev-list\".\n\nBut you do not have to even be tied to rev-list. After all, the partial\nmigration of filter_skipped is not \"git bisect in C\", but more like the\nfirst subcommand to \"git bisect--helper\" command that is written in C and\ncan be called from shell. The next subcommand might be check_good_are...\nand eventually you will have all the necessary and complex pieces the\nshell version of \"git bisect\" currently implements as shell function as\nthe subcommands of \"git bisect--helper\". Finally, \"git bisect in C\" will\nthen make direct calls to the functions that would implement that \"git\nbisect--helper\" command, and gets rid of the \"helper\" command altogether.\n\n\tSide note. That was how git-fetch--tool was started; it was a\n\thelper to partially migrate slower parts of git-fetch.sh to C. I\n\tsuspect we can almost remove it but not quite yet...\n"},{"id":"108006","messageId":"alpine.DEB.1.00.0903141308180.10279@pacific.mpi-cbg.de","threadId":"18275","inReplyTo":"7v63iccydg.fsf@gitster.siamese.dyndns.org","subject":"fetch--tool, was Re: Migrate bisect to C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-03-14T12:09:30Z","receivedAt":"2009-03-14T12:09:30Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 14 Mar 2009, Junio C Hamano wrote:\n\n> \tSide note. That was how git-fetch--tool was started; it was a \n> \thelper to partially migrate slower parts of git-fetch.sh to C. I \n> \tsuspect we can almost remove it but not quite yet...\n\nFunny. I was looking at that last week, and yes, there is still a user. \nThrough several functions in parse-remote.sh, git-pull.sh uses \nfetch--tool.\n\nNow, git-pull.sh is not all that large/complicated...\n\nCiao,\nDscho\n"}]}