{"thread":{"id":"24980","subject":"[PATCH] Add test-string-list.c","startedAt":"2010-09-05T02:34:21Z","lastAt":"2010-09-07T12:04:45Z","messageCount":6,"participants":["Thiago Farina","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"149925","messageId":"4f798daaf4631e00927b455b77919d5bb8fade03.1283653854.git.tfransosi@gmail.com","threadId":"24980","inReplyTo":null,"subject":"[PATCH] Add test-string-list.c","fromName":"Thiago Farina","fromEmail":"tfransosi@gmail.com","sentAt":"2010-09-05T02:34:21Z","receivedAt":"2010-09-05T02:34:21Z","isPatch":true,"sender":{"key":"tfransosi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/970071?v=4"},"body":"Add a simple test that demonstrates how to create and manipulate\na list of strings using the string-list.h API.\n\nTo see the test, call it by:\n./bin-wrappers/test-string-list.c\n\nSigned-off-by: Thiago Farina \n---\n Makefile | 1 +\n test-string-list.c | 29 +++++++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 0 deletions(-)\n create mode 100644 test-string-list.c\n\ndiff --git a/Makefile b/Makefile\nindex 40fbcae..287bc2c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -422,6 +422,7 @@ TEST_PROGRAMS_NEED_X += test-string-pool\n TEST_PROGRAMS_NEED_X += test-svn-fe\n TEST_PROGRAMS_NEED_X += test-treap\n TEST_PROGRAMS_NEED_X += test-index-version\n+TEST_PROGRAMS_NEED_X += test-string-list\n \n TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n \ndiff --git a/test-string-list.c b/test-string-list.c\nnew file mode 100644\nindex 0000000..11b8a7f\n--- /dev/null\n+++ b/test-string-list.c\n@@ -0,0 +1,29 @@\n+#include \n+#include \n+#include \"string-list.h\"\n+\n+int main(int argc, const char **argv)\n+{\n+\tstruct string_list list;\n+\tint i;\n+\n+\tmemset(&list, 0, sizeof(struct string_list));\n+\n+\tstring_list_append(&list, \"foo\");\n+\tstring_list_append(&list, \"bar\");\n+\n+\tfor (i = 0; i < list.nr; i++)\n+\t\tprintf(\"%s\\n\", list.items[i].string);\n+\n+\tprint_string_list(&list, \"\");\n+\n+ int has_foo = string_list_has_string(&list, \"foo\");\n+\tif (has_foo != 1)\n+\t\terror(\"List doesn't have foo.\");\n+\n+\tstring_list_clear(&list, 0);\n+\tif (list.nr > 0)\n+\t\terror(\"List is not clear.\");\n+\n+\treturn 0;\n+}\n-- \n1.7.2.3.313.gcd15\n"},{"id":"149936","messageId":"20100905050254.GA6134@burratino","threadId":"24980","inReplyTo":"4f798daaf4631e00927b455b77919d5bb8fade03.1283653854.git.tfransosi@gmail.com","subject":"Re: [PATCH] Add test-string-list.c","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-09-05T05:02:58Z","receivedAt":"2010-09-05T05:02:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Thiago,\n\nThiago Farina wrote:\n\n> Add a simple test that demonstrates how to create and manipulate\n> a list of strings using the string-list.h API.\n\nQuick thoughts:\n\n> --- /dev/null\n> +++ b/test-string-list.c\n> @@ -0,0 +1,29 @@\n> +#include \n\nGit programs tend to start with \n\n #include \"cache.h\"\n\nor\n\n #include \"git-compat-util.h\"\n\nto get all the portability niceties.\n\n> +\tprint_string_list(&list, \"\");\n> +\n> + int has_foo = string_list_has_string(&list, \"foo\");\n\nWhitespace, declaration after statement... (see\nDocumentation/CodingGuidelines).\n\n> +\tif (has_foo != 1)\n> +\t\terror(\"List doesn't have foo.\");\n\nThis does not exit with nonzero status when it fails. You probably\nwanted\n\n\tif (bad things)\n\t\treturn error(\"problems!\");\n\n> +\tstring_list_clear(&list, 0);\n> +\tif (list.nr > 0)\n> +\t\terror(\"List is not clear.\");\n\nTo make sure this example remains valid, wouldn't you want to include\na caller in the t/ directory so it can be automatically run? (See\nt/README.)\n\nThoughts separate from the code:\n\n * it is probably worth mentioning Documentation/technical/api-string-list.txt\n for people who do not know about it.\n\n * for this to be useful as a test I think one has to sort of believe\n that it can break. That is, a test of something this basic (which\n is already demonstrated and exercised by code throughout git, after\n all) would tend to be especially devious.\n\n * api-string-list.txt does not mention the STRING_LIST_INIT macros\n you introduced. Maybe that would be worth improving.\n\nRegards,\nJonathan\n"},{"id":"150043","messageId":"7v7hizwgc0.fsf@alter.siamese.dyndns.org","threadId":"24980","inReplyTo":"20100905050254.GA6134@burratino","subject":"Re: [PATCH] Add test-string-list.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-06T06:12:31Z","receivedAt":"2010-09-06T06:12:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Git programs tend to start with \n>\n> #include \"cache.h\"\n>\n> or\n>\n> #include \"git-compat-util.h\"\n>\n> to get all the portability niceties.\n\nQuote Documentation/CodingGuidelines here, too. And use of these headers\nis not merely \"niceties\"--some exotic platforms tend to want standard\nsystem headers in particular inclusion order and what git-compat-util.h\ndoes is the result of us painfully finding these rules out over years.\n\n> Thoughts separate from the code:\n>\n> * it is probably worth mentioning Documentation/technical/api-string-list.txt\n> for people who do not know about it.\n>\n> * for this to be useful as a test I think one has to sort of believe\n> that it can break. That is, a test of something this basic (which\n> is already demonstrated and exercised by code throughout git, after\n> all) would tend to be especially devious.\n>\n> * api-string-list.txt does not mention the STRING_LIST_INIT macros\n> you introduced. Maybe that would be worth improving.\n\n * this may be built but nothing exercises it. Is it worth adding\n tNNNN-run-string-list-test.sh for some value of NNNN? \n\nI tend to agree with your second point and have a moderately negative\nfeeling against the patch. An addition to API documentation like you\nsuggested would be more useful for people, and what Thiago did perhaps\nmight be useful in its sample code section, but then we already have\nplenty of working samples in live code and they are single \"git grep\"\naway, so...\n"},{"id":"150154","messageId":"AANLkTi=J_0XaVYUt8Umu3uW15W86wbWv-xCMuVGA7cUE@mail.gmail.com","threadId":"24980","inReplyTo":"20100905050254.GA6134@burratino","subject":"Re: [PATCH] Add test-string-list.c","fromName":"Thiago Farina","fromEmail":"tfransosi@gmail.com","sentAt":"2010-09-06T23:48:29Z","receivedAt":"2010-09-06T23:48:29Z","isPatch":true,"sender":{"key":"tfransosi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/970071?v=4"},"body":"On Sun, Sep 5, 2010 at 2:02 AM, Jonathan Nieder wrote:>\n>\n> Git programs tend to start with\n>\n>  #include \"cache.h\"\n>\n> or\n>\n>  #include \"git-compat-util.h\"\n>\n> to get all the portability niceties.\n>\nIncluding it now.\n\n>> +     print_string_list(&list, \"\");\n>> +\n>> +        int has_foo = string_list_has_string(&list, \"foo\");\n>\n> Whitespace, declaration after statement... (see\n> Documentation/CodingGuidelines).\n>\nFixed.\n\n>> +     if (has_foo != 1)\n>> +             error(\"List doesn't have foo.\");\n>\n> This does not exit with nonzero status when it fails.  You probably\n> wanted\n>\n>        if (bad things)\n>                return error(\"problems!\");\n>\nFixed.\n\n>> +     string_list_clear(&list, 0);\n>> +     if (list.nr > 0)\n>> +             error(\"List is not clear.\");\n>\n> To make sure this example remains valid, wouldn't you want to include\n> a caller in the t/ directory so it can be automatically run?  (See\n> t/README.)\n>\nI read it, but I'm not sure how to do this. Maybe you could point me\nto an example?\n\n> Thoughts separate from the code:\n>\n>  * for this to be useful as a test I think one has to sort of believe\n>   that it can break.  That is, a test of something this basic (which\n>   is already demonstrated and exercised by code throughout git, after\n>   all) would tend to be especially devious.\n>\nIt is basic, so anyone can read it, and say \"Oh, I can do this.\".\nLooking through the code maybe not so easy.\n\nIt can be expanded later by anyone to test many other things though.\nSo, why not? (Is it so bad to not have it at all?).\n"},{"id":"150160","messageId":"20100907000703.GB32717@burratino","threadId":"24980","inReplyTo":"AANLkTi=J_0XaVYUt8Umu3uW15W86wbWv-xCMuVGA7cUE@mail.gmail.com","subject":"Re: [PATCH] Add test-string-list.c","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-09-07T00:07:03Z","receivedAt":"2010-09-07T00:07:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Thiago,\n\nThiago Farina wrote:\n> On Sun, Sep 5, 2010 at 2:02 AM, Jonathan Nieder wrote:>\n\n>> To make sure this example remains valid, wouldn't you want to include\n>> a caller in the t/ directory so it can be automatically run?  (See\n>> t/README.)\n>\n> I read it, but I'm not sure how to do this. Maybe you could point me\n> to an example?\n\nt0070-fundamental.sh might be a good place to add it.\n\n> It can be expanded later by anyone to test many other things though.\n\nI suppose. Edge cases for arguments, sorting, and _DUP versus _NODUP\nsemantics would be the main thing I would be interested in testing.\nMaybe another day.\n\nThanks,\nJonathan\n"},{"id":"150209","messageId":"AANLkTi=1B6GaWZ0d6uiHWWjP7EvQLsaO0ZQDgWuBofDN@mail.gmail.com","threadId":"24980","inReplyTo":"20100907000703.GB32717@burratino","subject":"Re: [PATCH] Add test-string-list.c","fromName":"Thiago Farina","fromEmail":"tfransosi@gmail.com","sentAt":"2010-09-07T12:04:45Z","receivedAt":"2010-09-07T12:04:45Z","isPatch":true,"sender":{"key":"tfransosi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/970071?v=4"},"body":"On Mon, Sep 6, 2010 at 9:07 PM, Jonathan Nieder wrote:\n> Hi Thiago,\n>\n> Thiago Farina wrote:\n>> On Sun, Sep 5, 2010 at 2:02 AM, Jonathan Nieder wrote:>\n>\n>>> To make sure this example remains valid, wouldn't you want to include\n>>> a caller in the t/ directory so it can be automatically run?  (See\n>>> t/README.)\n>>\n>> I read it, but I'm not sure how to do this. Maybe you could point me\n>> to an example?\n>\n> t0070-fundamental.sh might be a good place to add it.\n>\nThanks, added it to there.\n\n>> It can be expanded later by anyone to test many other things though.\n>\n> I suppose.  Edge cases for arguments, sorting, and _DUP versus _NODUP\n> semantics would be the main thing I would be interested in testing.\n\nReworked into small functions to test string_list functions separated\n(using assert to check if the return value of the function is what is\nexpected).\n\nPlease, take another look.\n"}]}