{"thread":{"id":"60954","subject":"[PATCH 0/2] apply: add unit tests for parse_range","startedAt":"2024-02-19T04:45:41Z","lastAt":"2024-06-07T16:59:15Z","messageCount":12,"participants":["Philip Peterson via GitGitGadget","Junio C Hamano","Kristoffer Haugsbakk","Philip","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"488900","messageId":"pull.1677.git.git.1708317938.gitgitgadget@gmail.com","threadId":"60954","inReplyTo":null,"subject":"[PATCH 0/2] apply: add unit tests for parse_range","fromName":"Philip Peterson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-19T04:45:36Z","receivedAt":"2024-02-19T04:45:41Z","isPatch":true,"sender":{"key":"name:Philip Peterson","avatar":null},"body":"Hello all,\n\nThis patchset adds unit tests for apply's parse_range function. Also renames\nparse_range to parse_fragment_range.\n\nIt was necessary to make the function be non-internal linkage in order to\nexpose it to the unit testing framework. Because there is another function\nin the codebase also called parse_range, I changed this one to a more\nspecific name as well: parse_fragment_range. There are also several test\ncases added (both positive and negative) for the function.\n\nThank you for your help,\n\nPhilip\n\nPhilip Peterson (2):\n apply: add unit tests for parse_range and rename to\n parse_fragment_range\n apply: rewrite unit tests with structured cases\n\n Makefile | 1 +\n apply.c | 8 ++--\n apply.h | 4 ++\n t/unit-tests/t-apply.c | 100 +++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 109 insertions(+), 4 deletions(-)\n create mode 100644 t/unit-tests/t-apply.c\n\n\nbase-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1677%2Fphilip-peterson%2Fpeterson%2Funit-tests-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1677/philip-peterson/peterson/unit-tests-v1\nPull-Request: https://github.com/git/git/pull/1677\n-- \ngitgitgadget\n"},{"id":"488901","messageId":"2c60c4406d4eb1307a32f23604f3ef8e34ad56d6.1708317938.git.gitgitgadget@gmail.com","threadId":"60954","inReplyTo":"pull.1677.git.git.1708317938.gitgitgadget@gmail.com","subject":"[PATCH 1/2] apply: add unit tests for parse_range and rename to parse_fragment_range","fromName":"Philip Peterson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-19T04:45:37Z","receivedAt":"2024-02-19T04:45:42Z","isPatch":true,"sender":{"key":"name:Philip Peterson","avatar":null},"body":"From: Philip Peterson \n\nThis patchset makes the parse_range function in apply be non-internal\nlinkage in order to expose to the unit testing framework. In so doing,\nbecause there is another function called parse_range, I gave this one a more\nspecific name, parse_fragment_range. Other than that, this commit adds\nseveral test cases (positive and negative) for the function.\n\nSigned-off-by: Philip Peterson \n---\n Makefile | 1 +\n apply.c | 8 ++---\n apply.h | 4 +++\n t/unit-tests/t-apply.c | 67 ++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 76 insertions(+), 4 deletions(-)\n create mode 100644 t/unit-tests/t-apply.c\n\ndiff --git a/Makefile b/Makefile\nindex 15990ff3122..369092aedfe 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1339,6 +1339,7 @@ THIRD_PARTY_SOURCES += compat/regex/%\n THIRD_PARTY_SOURCES += sha1collisiondetection/%\n THIRD_PARTY_SOURCES += sha1dc/%\n \n+UNIT_TEST_PROGRAMS += t-apply\n UNIT_TEST_PROGRAMS += t-basic\n UNIT_TEST_PROGRAMS += t-mem-pool\n UNIT_TEST_PROGRAMS += t-strbuf\ndiff --git a/apply.c b/apply.c\nindex 7608e3301ca..199a1150df6 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1430,8 +1430,8 @@ static int parse_num(const char *line, unsigned long *p)\n \treturn ptr - line;\n }\n \n-static int parse_range(const char *line, int len, int offset, const char *expect,\n-\t\t unsigned long *p1, unsigned long *p2)\n+int parse_fragment_range(const char *line, int len, int offset, const char *expect,\n+\t\t\t unsigned long *p1, unsigned long *p2)\n {\n \tint digits, ex;\n \n@@ -1530,8 +1530,8 @@ static int parse_fragment_header(const char *line, int len, struct fragment *fra\n \t\treturn -1;\n \n \t/* Figure out the number of lines in a fragment */\n-\toffset = parse_range(line, len, 4, \" +\", &fragment->oldpos, &fragment->oldlines);\n-\toffset = parse_range(line, len, offset, \" @@\", &fragment->newpos, &fragment->newlines);\n+\toffset = parse_fragment_range(line, len, 4, \" +\", &fragment->oldpos, &fragment->oldlines);\n+\toffset = parse_fragment_range(line, len, offset, \" @@\", &fragment->newpos, &fragment->newlines);\n \n \treturn offset;\n }\ndiff --git a/apply.h b/apply.h\nindex 7cd38b1443c..bbc5e3caeb5 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -187,3 +187,7 @@ int apply_all_patches(struct apply_state *state,\n \t\t int options);\n \n #endif\n+\n+\n+int parse_fragment_range(const char *line, int len, int offset, const char *expect,\n+\t\t unsigned long *p1, unsigned long *p2);\ndiff --git a/t/unit-tests/t-apply.c b/t/unit-tests/t-apply.c\nnew file mode 100644\nindex 00000000000..ff0abfb2e0b\n--- /dev/null\n+++ b/t/unit-tests/t-apply.c\n@@ -0,0 +1,67 @@\n+#include \"test-lib.h\"\n+#include \"apply.h\"\n+\n+#define FAILURE -1\n+\n+static void setup_static(const char *line, int len, int offset,\n+\t\t\t\t\t\t const char *expect, int assert_result,\n+\t\t\t\t\t\t unsigned long assert_p1,\n+\t\t\t\t\t\t unsigned long assert_p2)\n+{\n+\tunsigned long p1 = 9999;\n+\tunsigned long p2 = 9999;\n+\tint result = parse_fragment_range(line, len, offset, expect, &p1, &p2);\n+\tcheck_int(result, ==, assert_result);\n+\tcheck_int(p1, ==, assert_p1);\n+\tcheck_int(p2, ==, assert_p2);\n+}\n+\n+int cmd_main(int argc, const char **argv)\n+{\n+\tchar* text;\n+\tint expected_result;\n+\n+\t/* Success */\n+\ttext = \"@@ -4,4 +\";\n+\texpected_result = 9;\n+\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 4, 4),\n+\t\t \"well-formed range\");\n+\n+\ttext = \"@@ -4 +8 @@\";\n+\texpected_result = 7;\n+\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 4, 1),\n+\t\t \"non-comma range\");\n+\n+\t/* Failure */\n+\ttext = \"@@ -X,4 +\";\n+\texpected_result = FAILURE;\n+\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 9999, 9999),\n+\t\t \"non-digit range (first coordinate)\");\n+\n+\ttext = \"@@ -4,X +\";\n+\texpected_result = FAILURE;\n+\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 4, 1), // p2 is 1, a little strange but not catastrophic\n+\t\t \"non-digit range (second coordinate)\");\n+\n+\ttext = \"@@ -4,4 -\";\n+\texpected_result = FAILURE;\n+\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 4, 4),\n+\t\t \"non-expected trailing text\");\n+\n+\ttext = \"@@ -4,4\";\n+\texpected_result = FAILURE;\n+\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 4, 4),\n+\t\t \"not long enough for expected trailing text\");\n+\n+\ttext = \"@@ -4,4\";\n+\texpected_result = FAILURE;\n+\tTEST(setup_static(text, strlen(text), 7, \" +\", expected_result, 9999, 9999),\n+\t\t \"not long enough for offset\");\n+\n+\ttext = \"@@ -4,4\";\n+\texpected_result = FAILURE;\n+\tTEST(setup_static(text, strlen(text), -1, \" +\", expected_result, 9999, 9999),\n+\t\t \"negative offset\");\n+\n+\treturn test_done();\n+}\n-- \ngitgitgadget\n\n"},{"id":"488902","messageId":"7dab12ab7b8af3e6a0778fc1a01dd1479990bcff.1708317938.git.gitgitgadget@gmail.com","threadId":"60954","inReplyTo":"pull.1677.git.git.1708317938.gitgitgadget@gmail.com","subject":"[PATCH 2/2] apply: rewrite unit tests with structured cases","fromName":"Philip Peterson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-19T04:45:38Z","receivedAt":"2024-02-19T04:45:43Z","isPatch":true,"sender":{"key":"name:Philip Peterson","avatar":null},"body":"From: Philip Peterson \n\nThe imperative format was a little hard to read, so I rewrote the test cases\nin a declarative style by defining a common structure for each test case and\nits assertions.\n\nSigned-off-by: Philip Peterson \n---\n t/unit-tests/t-apply.c | 123 ++++++++++++++++++++++++++---------------\n 1 file changed, 78 insertions(+), 45 deletions(-)\n\ndiff --git a/t/unit-tests/t-apply.c b/t/unit-tests/t-apply.c\nindex ff0abfb2e0b..2b78624b690 100644\n--- a/t/unit-tests/t-apply.c\n+++ b/t/unit-tests/t-apply.c\n@@ -3,65 +3,98 @@\n \n #define FAILURE -1\n \n-static void setup_static(const char *line, int len, int offset,\n-\t\t\t\t\t\t const char *expect, int assert_result,\n-\t\t\t\t\t\t unsigned long assert_p1,\n-\t\t\t\t\t\t unsigned long assert_p2)\n+typedef struct test_case {\n+\tconst char *line;\n+\tconst char *expect_suffix;\n+\tint offset;\n+\tunsigned long expect_p1;\n+\tunsigned long expect_p2;\n+\tint expect_result;\n+} test_case;\n+\n+static void setup_static(struct test_case t)\n {\n \tunsigned long p1 = 9999;\n \tunsigned long p2 = 9999;\n-\tint result = parse_fragment_range(line, len, offset, expect, &p1, &p2);\n-\tcheck_int(result, ==, assert_result);\n-\tcheck_int(p1, ==, assert_p1);\n-\tcheck_int(p2, ==, assert_p2);\n+\tint result = parse_fragment_range(t.line, strlen(t.line), t.offset, t.expect_suffix, &p1, &p2);\n+\tcheck_int(result, ==, t.expect_result);\n+\tcheck_int(p1, ==, t.expect_p1);\n+\tcheck_int(p2, ==, t.expect_p2);\n }\n \n int cmd_main(int argc, const char **argv)\n {\n-\tchar* text;\n-\tint expected_result;\n-\n-\t/* Success */\n-\ttext = \"@@ -4,4 +\";\n-\texpected_result = 9;\n-\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 4, 4),\n-\t\t \"well-formed range\");\n+\tTEST(setup_static((struct test_case) {\n+\t\t.line = \"@@ -4,4 +\",\n+\t\t.offset = 4,\n+\t\t.expect_suffix = \" +\",\n+\t\t.expect_result = 9,\n+\t\t.expect_p1 = 4,\n+\t\t.expect_p2 = 4\n+\t}), \"well-formed range\");\n \n-\ttext = \"@@ -4 +8 @@\";\n-\texpected_result = 7;\n-\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 4, 1),\n-\t\t \"non-comma range\");\n+\tTEST(setup_static((struct test_case) {\n+\t\t.line = \"@@ -4 +8 @@\",\n+\t\t.offset = 4,\n+\t\t.expect_suffix = \" +\",\n+\t\t.expect_result = 7,\n+\t\t.expect_p1 = 4,\n+\t\t.expect_p2 = 1\n+\t}), \"non-comma range\");\n \n-\t/* Failure */\n-\ttext = \"@@ -X,4 +\";\n-\texpected_result = FAILURE;\n-\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 9999, 9999),\n-\t\t \"non-digit range (first coordinate)\");\n+\tTEST(setup_static((struct test_case) {\n+\t\t.line = \"@@ -X,4 +\",\n+\t\t.offset = 4,\n+\t\t.expect_suffix = \" +\",\n+\t\t.expect_result = FAILURE,\n+\t\t.expect_p1 = 9999,\n+\t\t.expect_p2 = 9999\n+\t}), \"non-digit range (first coordinate)\");\n \n-\ttext = \"@@ -4,X +\";\n-\texpected_result = FAILURE;\n-\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 4, 1), // p2 is 1, a little strange but not catastrophic\n-\t\t \"non-digit range (second coordinate)\");\n+\tTEST(setup_static((struct test_case) {\n+\t\t.line = \"@@ -4,X +\",\n+\t\t.offset = 4,\n+\t\t.expect_suffix = \" +\",\n+\t\t.expect_result = FAILURE,\n+\t\t.expect_p1 = 4,\n+\t\t.expect_p2 = 1 // A little strange this is 1, but not end of the world\n+\t}), \"non-digit range (second coordinate)\");\n \n-\ttext = \"@@ -4,4 -\";\n-\texpected_result = FAILURE;\n-\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 4, 4),\n-\t\t \"non-expected trailing text\");\n+\tTEST(setup_static((struct test_case) {\n+\t\t.line = \"@@ -4,4 -\",\n+\t\t.offset = 4,\n+\t\t.expect_suffix = \" +\",\n+\t\t.expect_result = FAILURE,\n+\t\t.expect_p1 = 4,\n+\t\t.expect_p2 = 4\n+\t}), \"non-expected trailing text\");\n \n-\ttext = \"@@ -4,4\";\n-\texpected_result = FAILURE;\n-\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 4, 4),\n-\t\t \"not long enough for expected trailing text\");\n+\tTEST(setup_static((struct test_case) {\n+\t\t.line = \"@@ -4,4\",\n+\t\t.offset = 4,\n+\t\t.expect_suffix = \" +\",\n+\t\t.expect_result = FAILURE,\n+\t\t.expect_p1 = 4,\n+\t\t.expect_p2 = 4\n+\t}), \"not long enough for expected trailing text\");\n \n-\ttext = \"@@ -4,4\";\n-\texpected_result = FAILURE;\n-\tTEST(setup_static(text, strlen(text), 7, \" +\", expected_result, 9999, 9999),\n-\t\t \"not long enough for offset\");\n+\tTEST(setup_static((struct test_case) {\n+\t\t.line = \"@@ -4,4\",\n+\t\t.offset = 7,\n+\t\t.expect_suffix = \" +\",\n+\t\t.expect_result = FAILURE,\n+\t\t.expect_p1 = 9999,\n+\t\t.expect_p2 = 9999\n+\t}), \"not long enough for offset\");\n \n-\ttext = \"@@ -4,4\";\n-\texpected_result = FAILURE;\n-\tTEST(setup_static(text, strlen(text), -1, \" +\", expected_result, 9999, 9999),\n-\t\t \"negative offset\");\n+\tTEST(setup_static((struct test_case) {\n+\t\t.line = \"@@ -4,4\",\n+\t\t.offset = -1,\n+\t\t.expect_suffix = \" +\",\n+\t\t.expect_result = FAILURE,\n+\t\t.expect_p1 = 9999,\n+\t\t.expect_p2 = 9999\n+\t}), \"negative offset\");\n \n \treturn test_done();\n }\n-- \ngitgitgadget\n"},{"id":"488932","messageId":"xmqqil2k5e8u.fsf@gitster.g","threadId":"60954","inReplyTo":"2c60c4406d4eb1307a32f23604f3ef8e34ad56d6.1708317938.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] apply: add unit tests for parse_range and rename to parse_fragment_range","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-19T21:35:29Z","receivedAt":"2024-02-19T21:35:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Peterson via GitGitGadget\" writes:\n\n> From: Philip Peterson \n>\n> This patchset makes the parse_range function in apply be non-internal\n> linkage in order to expose to the unit testing framework. In so doing,\n> because there is another function called parse_range, I gave this one a more\n> specific name, parse_fragment_range. Other than that, this commit adds\n> several test cases (positive and negative) for the function.\n\nWe do not write \"I did this, I did that\" in our proposed log\nmessage. In addition, guidance on the proposed commit log in a\nhandful of sections in Documentation/SubmittingPatches would be\nhelpful.\n\nIt may probably be a good idea to split this into a preliminary\npatch that makes a symbol extern (and doing nothing else), and\nthe main patch that adds external caller(s) to the function from\na new unit test.\n\nIt certainly is better than doing nothing and just make it extern,\nbut I am not sure \"fragment\" is specific enough to make the symbol\nclearly belong to \"apply\" API.\n\n> diff --git a/apply.c b/apply.c\n> index 7608e3301ca..199a1150df6 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -1430,8 +1430,8 @@ static int parse_num(const char *line, unsigned long *p)\n> \treturn ptr - line;\n> }\n> \n> -static int parse_range(const char *line, int len, int offset, const char *expect,\n> -\t\t unsigned long *p1, unsigned long *p2)\n> +int parse_fragment_range(const char *line, int len, int offset, const char *expect,\n> +\t\t\t unsigned long *p1, unsigned long *p2)\n> {\n> \tint digits, ex;\n\nAlternatively we could do something like this to make the blast\nradius of this patch smaller.\n\n -static int parse_range(const char *line, int len, int offset, const char *expect,\n +#define apply_parse_fragment_range parse_range\n +int parse_range(const char *line, int len, int offset, const char *expect,\n unsigned long *p1, unsigned long *p2)\n\nIf not for unit-test, this function has no reason to be extern with\nsuch a long name, so it is better to allow internal callers to refer\nto it with the name that has been good enough for them for the past\n19 years since it was introduced in fab2c257 (git-apply: make the\ndiffstat output happen for \"--stat\" only., 2005-05-26).\n\n> diff --git a/apply.h b/apply.h\n> index 7cd38b1443c..bbc5e3caeb5 100644\n> --- a/apply.h\n> +++ b/apply.h\n> @@ -187,3 +187,7 @@ int apply_all_patches(struct apply_state *state,\n> \t\t int options);\n> \n> #endif\n> +\n> +\n> +int parse_fragment_range(const char *line, int len, int offset, const char *expect,\n> +\t\t unsigned long *p1, unsigned long *p2);\n\nThis is wrong. The #endif is about avoiding double inclusion of\nthis header file and any new declaration must go before it.\n\n> diff --git a/t/unit-tests/t-apply.c b/t/unit-tests/t-apply.c\n> new file mode 100644\n\nThis should go to the next patch.\n"},{"id":"488935","messageId":"xmqqcyss5dm3.fsf@gitster.g","threadId":"60954","inReplyTo":"7dab12ab7b8af3e6a0778fc1a01dd1479990bcff.1708317938.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] apply: rewrite unit tests with structured cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-19T21:49:08Z","receivedAt":"2024-02-19T21:49:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Peterson via GitGitGadget\" writes:\n\n> From: Philip Peterson \n>\n> The imperative format was a little hard to read, so I rewrote the test cases\n> in a declarative style by defining a common structure for each test case and\n> its assertions.\n>\n> Signed-off-by: Philip Peterson \n> ---\n> t/unit-tests/t-apply.c | 123 ++++++++++++++++++++++++++---------------\n> 1 file changed, 78 insertions(+), 45 deletions(-)\n\nIn this project, we do not add a version of a known-to-be-bad file\nin patch [1/2], only to be immediately improved in patch [2/2].\nUnless, of course, there is a good reason to do so. And \"to\npreserve the true history of what happened in the developer's\nworking tree\" is not a good reason.\n\nWe give our developers \"rebase -i\" and other means to rewrite their\nGit history, not just because we want them to be able to pretend\nthat they are a better developer who never make such a mistake or\nmisdesign in the first place, but because a polished history is\neasier to review in the shorter term and to maintain in the longer\nterm.\n\nThanks.\n"},{"id":"488936","messageId":"423c4144-56dd-4897-9b81-badf67e22420@app.fastmail.com","threadId":"60954","inReplyTo":"7dab12ab7b8af3e6a0778fc1a01dd1479990bcff.1708317938.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] apply: rewrite unit tests with structured cases","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-19T22:04:19Z","receivedAt":"2024-02-19T22:04:44Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Mon, Feb 19, 2024, at 05:45, Philip Peterson via GitGitGadget wrote:\n> From: Philip Peterson \n>\n> The imperative format was a little hard to read, so I rewrote the test cases\n> in a declarative style by defining a common structure for each test case and\n> its assertions.\n>\n> Signed-off-by: Philip Peterson \n\nIMO in general you can just assert that X and Y in the commit message.\n\n “ The imperative format is hard to read. Rewrite the test cases …\n\nIf your patch passes review and is merged then that’s the truth as\ndetermined by you and the reviewers.\n\nMore subjective-sounding “This was hard to read” and maybe anecdotes\nlike “this tripped me up when reading” can go outside the commit message\nlike the cover letter or the free-form space between the commit message\nand the patch (after the three-hyphen/three-dash lines).\n\n-- \nKristoffer Haugsbakk\n"},{"id":"492196","messageId":"CAJ6X7_Uc0OdzYToJSs15+vbydraKAB8x4DPj7UsL1PKLzyY0dQ@mail.gmail.com","threadId":"60954","inReplyTo":"xmqqil2k5e8u.fsf@gitster.g","subject":"Re: [PATCH 1/2] apply: add unit tests for parse_range and rename to parse_fragment_range","fromName":"Philip","fromEmail":"philip.c.peterson@gmail.com","sentAt":"2024-04-04T03:53:27Z","receivedAt":"2024-04-04T03:54:04Z","isPatch":true,"sender":{"key":"philip.c.peterson@gmail.com","avatar":"https://gravatar.com/avatar/2dfa9764cb8e5ce07b180670c62dc3fc643e4f8e2f2afafcb7220ac10ea4b4ca?d=mp&s=160"},"body":"Hi,\n\nThanks for the tips. I am confused about the change request though:\n\n> Alternatively we could do something like this to make the blast\n> radius of this patch smaller.\n>\n> -static int parse_range(const char *line, int len, int offset, const char *expect,\n> +#define apply_parse_fragment_range parse_range\n> +int parse_range(const char *line, int len, int offset, const char *expect,\n> unsigned long *p1, unsigned long *p2)\n\nFrom what I understand, this still creates a new extern symbol\ncalled parse_range. The hope was to avoid creating a new symbol\nwith a generic name that someone might start to consume, because\nif they did start consuming that symbol, it would be a burden on\nthem when we eventually have to rename it due to the conflict with\nthe other symbol currently called parse_range in add-patch.c, which\nwe might also want to add unit tests for later. Is it not\npreferable to just rename it now, before it becomes extern?\n\nCheers,\nPhil\n\nOn Mon, Feb 19, 2024 at 4:35 PM Junio C Hamano wrote:\n>\n> \"Philip Peterson via GitGitGadget\" writes:\n>\n> > From: Philip Peterson \n> >\n> > This patchset makes the parse_range function in apply be non-internal\n> > linkage in order to expose to the unit testing framework. In so doing,\n> > because there is another function called parse_range, I gave this one a more\n> > specific name, parse_fragment_range. Other than that, this commit adds\n> > several test cases (positive and negative) for the function.\n>\n> We do not write \"I did this, I did that\" in our proposed log\n> message. In addition, guidance on the proposed commit log in a\n> handful of sections in Documentation/SubmittingPatches would be\n> helpful.\n>\n> It may probably be a good idea to split this into a preliminary\n> patch that makes a symbol extern (and doing nothing else), and\n> the main patch that adds external caller(s) to the function from\n> a new unit test.\n>\n> It certainly is better than doing nothing and just make it extern,\n> but I am not sure \"fragment\" is specific enough to make the symbol\n> clearly belong to \"apply\" API.\n>\n> > diff --git a/apply.c b/apply.c\n> > index 7608e3301ca..199a1150df6 100644\n> > --- a/apply.c\n> > +++ b/apply.c\n> > @@ -1430,8 +1430,8 @@ static int parse_num(const char *line, unsigned long *p)\n> > return ptr - line;\n> > }\n> >\n> > -static int parse_range(const char *line, int len, int offset, const char *expect,\n> > - unsigned long *p1, unsigned long *p2)\n> > +int parse_fragment_range(const char *line, int len, int offset, const char *expect,\n> > + unsigned long *p1, unsigned long *p2)\n> > {\n> > int digits, ex;\n>\n> Alternatively we could do something like this to make the blast\n> radius of this patch smaller.\n>\n> -static int parse_range(const char *line, int len, int offset, const char *expect,\n> +#define apply_parse_fragment_range parse_range\n> +int parse_range(const char *line, int len, int offset, const char *expect,\n> unsigned long *p1, unsigned long *p2)\n>\n> If not for unit-test, this function has no reason to be extern with\n> such a long name, so it is better to allow internal callers to refer\n> to it with the name that has been good enough for them for the past\n> 19 years since it was introduced in fab2c257 (git-apply: make the\n> diffstat output happen for \"--stat\" only., 2005-05-26).\n>\n> > diff --git a/apply.h b/apply.h\n> > index 7cd38b1443c..bbc5e3caeb5 100644\n> > --- a/apply.h\n> > +++ b/apply.h\n> > @@ -187,3 +187,7 @@ int apply_all_patches(struct apply_state *state,\n> > int options);\n> >\n> > #endif\n> > +\n> > +\n> > +int parse_fragment_range(const char *line, int len, int offset, const char *expect,\n> > + unsigned long *p1, unsigned long *p2);\n>\n> This is wrong. The #endif is about avoiding double inclusion of\n> this header file and any new declaration must go before it.\n>\n> > diff --git a/t/unit-tests/t-apply.c b/t/unit-tests/t-apply.c\n> > new file mode 100644\n>\n> This should go to the next patch.\n"},{"id":"492263","messageId":"xmqqsf01yllb.fsf@gitster.g","threadId":"60954","inReplyTo":"CAJ6X7_Uc0OdzYToJSs15+vbydraKAB8x4DPj7UsL1PKLzyY0dQ@mail.gmail.com","subject":"Re: [PATCH 1/2] apply: add unit tests for parse_range and rename to parse_fragment_range","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-04T19:27:44Z","receivedAt":"2024-04-04T19:27:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philip writes:\n\n> On Mon, Feb 19, 2024 at 4:35 PM Junio C Hamano wrote:\n>>\n>> \"Philip Peterson via GitGitGadget\" writes:\n\nIt's quite a blast from a long time ago that I no longer remember.\n\n>> Alternatively we could do something like this to make the blast\n>> radius of this patch smaller.\n>>\n>> -static int parse_range(const char *line, int len, int offset, const char *expect,\n>> +#define apply_parse_fragment_range parse_range\n>> +int parse_range(const char *line, int len, int offset, const char *expect,\n>> unsigned long *p1, unsigned long *p2)\n>\n> From what I understand, this still creates a new extern symbol\n> called parse_range.\n\nSorry, I misspoke. The direction of #define is the other way\naround. That is, we may have to give the function a name that is\noverly long because it needs to be externally linkable only to\nsupport for your test, but we would want to locally rename that long\nname down to the name currently used by the primary callers of that\nfunction, so that the patch does not have to touch these existing\ncalling sites. After all, this function is a small implementation\ndetail and not a part of the official apply.c API, and the only\nreason why we are making it extern is because some new tests want to\nlink with it from the side.\n\nSo, in the header file you'll do\n\n\t/* \n\t * exposed only for tests; do not call this as it not\n\t * a part of the API\n\t */\n\textern int apply_parse_fragment_range(...);\n\nand then in the original file that used to have \"static int\nparse_range(...)\", you'd add\n\n\t#define parse_range apply_parse_fragment_range\n\nnear the top, after the header files are included.\n\nThanks.\n"},{"id":"495648","messageId":"pull.1677.v2.git.git.1716710073910.gitgitgadget@gmail.com","threadId":"60954","inReplyTo":"pull.1677.git.git.1708317938.gitgitgadget@gmail.com","subject":"[PATCH v2] apply: add unit tests for parse_range","fromName":"Philip Peterson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-26T07:54:33Z","receivedAt":"2024-05-26T07:54:38Z","isPatch":true,"sender":{"key":"name:Philip Peterson","avatar":null},"body":"From: Philip Peterson \n\nAlso rename parse_range to parse_fragment_range for external linkage.\n\nSigned-off-by: Philip Peterson \n---\n apply: add unit tests for parse_range\n \n Add unit tests for apply's parse_range function. Also rename parse_range\n to parse_fragment_range and expose it externally for use by the unit\n tests. Internal callers may continue to refer to it by the name\n parse_range.\n \n It is necessary to make the function be non-internal linkage in order to\n expose it to the unit testing framework. Because there is another\n function in the codebase also called parse_range, change this one to a\n more specific name as well: parse_fragment_range. Also add several test\n cases (both positive and negative) for the function.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1677%2Fphilip-peterson%2Fpeterson%2Funit-tests-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1677/philip-peterson/peterson/unit-tests-v2\nPull-Request: https://github.com/git/git/pull/1677\n\nRange-diff vs v1:\n\n 1: 2c60c4406d4 < -: ----------- apply: add unit tests for parse_range and rename to parse_fragment_range\n 2: 7dab12ab7b8 ! 1: c0d7b93e383 apply: rewrite unit tests with structured cases\n @@ Metadata\n Author: Philip Peterson \n \n ## Commit message ##\n - apply: rewrite unit tests with structured cases\n + apply: add unit tests for parse_range\n \n - The imperative format was a little hard to read, so I rewrote the test cases\n - in a declarative style by defining a common structure for each test case and\n - its assertions.\n + Also rename parse_range to parse_fragment_range for external linkage.\n \n Signed-off-by: Philip Peterson \n \n - ## t/unit-tests/t-apply.c ##\n + ## Makefile ##\n +@@ Makefile: THIRD_PARTY_SOURCES += compat/regex/%\n + THIRD_PARTY_SOURCES += sha1collisiondetection/%\n + THIRD_PARTY_SOURCES += sha1dc/%\n + \n ++UNIT_TEST_PROGRAMS += t-apply\n + UNIT_TEST_PROGRAMS += t-ctype\n + UNIT_TEST_PROGRAMS += t-mem-pool\n + UNIT_TEST_PROGRAMS += t-prio-queue\n +\n + ## apply.c ##\n @@\n + #include \"wildmatch.h\"\n + #include \"ws.h\"\n \n - #define FAILURE -1\n ++#define parse_range apply_parse_fragment_range\n ++\n + struct gitdiff_data {\n + \tstruct strbuf *root;\n + \tint linenr;\n +@@ apply.c: static int parse_num(const char *line, unsigned long *p)\n + \treturn ptr - line;\n + }\n \n --static void setup_static(const char *line, int len, int offset,\n --\t\t\t\t\t\t const char *expect, int assert_result,\n --\t\t\t\t\t\t unsigned long assert_p1,\n --\t\t\t\t\t\t unsigned long assert_p2)\n +-static int parse_range(const char *line, int len, int offset, const char *expect,\n +-\t\t unsigned long *p1, unsigned long *p2)\n ++int apply_parse_fragment_range(const char *line, int len, int offset, const char *expect,\n ++\t\t\t unsigned long *p1, unsigned long *p2)\n + {\n + \tint digits, ex;\n + \n +\n + ## apply.h ##\n +@@ apply.h: int apply_all_patches(struct apply_state *state,\n + \t\t int argc, const char **argv,\n + \t\t int options);\n + \n ++/*\n ++ * exposed only for tests; do not call this as it not\n ++ * a part of the API\n ++ */\n ++extern int apply_parse_fragment_range(const char *line, int len, int offset,\n ++\t\t\t\t const char *expect, unsigned long *p1,\n ++\t\t\t\t unsigned long *p2);\n ++\n + #endif\n +\n + ## t/unit-tests/t-apply.c (new) ##\n +@@\n ++#include \"test-lib.h\"\n ++#include \"apply.h\"\n ++\n ++#define FAILURE -1\n ++\n +typedef struct test_case {\n +\tconst char *line;\n +\tconst char *expect_suffix;\n @@ t/unit-tests/t-apply.c\n +} test_case;\n +\n +static void setup_static(struct test_case t)\n - {\n - \tunsigned long p1 = 9999;\n - \tunsigned long p2 = 9999;\n --\tint result = parse_fragment_range(line, len, offset, expect, &p1, &p2);\n --\tcheck_int(result, ==, assert_result);\n --\tcheck_int(p1, ==, assert_p1);\n --\tcheck_int(p2, ==, assert_p2);\n -+\tint result = parse_fragment_range(t.line, strlen(t.line), t.offset, t.expect_suffix, &p1, &p2);\n ++{\n ++\tunsigned long p1 = 9999;\n ++\tunsigned long p2 = 9999;\n ++\tint result = apply_parse_fragment_range(t.line, strlen(t.line), t.offset,\n ++\t\t\t\t\t\tt.expect_suffix, &p1, &p2);\n +\tcheck_int(result, ==, t.expect_result);\n +\tcheck_int(p1, ==, t.expect_p1);\n +\tcheck_int(p2, ==, t.expect_p2);\n - }\n - \n - int cmd_main(int argc, const char **argv)\n - {\n --\tchar* text;\n --\tint expected_result;\n --\n --\t/* Success */\n --\ttext = \"@@ -4,4 +\";\n --\texpected_result = 9;\n --\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 4, 4),\n --\t\t \"well-formed range\");\n ++}\n ++\n ++int cmd_main(int argc, const char **argv)\n ++{\n +\tTEST(setup_static((struct test_case) {\n +\t\t.line = \"@@ -4,4 +\",\n +\t\t.offset = 4,\n @@ t/unit-tests/t-apply.c\n +\t\t.expect_p1 = 4,\n +\t\t.expect_p2 = 4\n +\t}), \"well-formed range\");\n - \n --\ttext = \"@@ -4 +8 @@\";\n --\texpected_result = 7;\n --\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 4, 1),\n --\t\t \"non-comma range\");\n ++\n +\tTEST(setup_static((struct test_case) {\n +\t\t.line = \"@@ -4 +8 @@\",\n +\t\t.offset = 4,\n @@ t/unit-tests/t-apply.c\n +\t\t.expect_p1 = 4,\n +\t\t.expect_p2 = 1\n +\t}), \"non-comma range\");\n - \n --\t/* Failure */\n --\ttext = \"@@ -X,4 +\";\n --\texpected_result = FAILURE;\n --\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 9999, 9999),\n --\t\t \"non-digit range (first coordinate)\");\n ++\n +\tTEST(setup_static((struct test_case) {\n +\t\t.line = \"@@ -X,4 +\",\n +\t\t.offset = 4,\n @@ t/unit-tests/t-apply.c\n +\t\t.expect_p1 = 9999,\n +\t\t.expect_p2 = 9999\n +\t}), \"non-digit range (first coordinate)\");\n - \n --\ttext = \"@@ -4,X +\";\n --\texpected_result = FAILURE;\n --\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 4, 1), // p2 is 1, a little strange but not catastrophic\n --\t\t \"non-digit range (second coordinate)\");\n ++\n +\tTEST(setup_static((struct test_case) {\n +\t\t.line = \"@@ -4,X +\",\n +\t\t.offset = 4,\n @@ t/unit-tests/t-apply.c\n +\t\t.expect_p1 = 4,\n +\t\t.expect_p2 = 1 // A little strange this is 1, but not end of the world\n +\t}), \"non-digit range (second coordinate)\");\n - \n --\ttext = \"@@ -4,4 -\";\n --\texpected_result = FAILURE;\n --\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 4, 4),\n --\t\t \"non-expected trailing text\");\n ++\n +\tTEST(setup_static((struct test_case) {\n +\t\t.line = \"@@ -4,4 -\",\n +\t\t.offset = 4,\n @@ t/unit-tests/t-apply.c\n +\t\t.expect_p1 = 4,\n +\t\t.expect_p2 = 4\n +\t}), \"non-expected trailing text\");\n - \n --\ttext = \"@@ -4,4\";\n --\texpected_result = FAILURE;\n --\tTEST(setup_static(text, strlen(text), 4, \" +\", expected_result, 4, 4),\n --\t\t \"not long enough for expected trailing text\");\n ++\n +\tTEST(setup_static((struct test_case) {\n +\t\t.line = \"@@ -4,4\",\n +\t\t.offset = 4,\n @@ t/unit-tests/t-apply.c\n +\t\t.expect_p1 = 4,\n +\t\t.expect_p2 = 4\n +\t}), \"not long enough for expected trailing text\");\n - \n --\ttext = \"@@ -4,4\";\n --\texpected_result = FAILURE;\n --\tTEST(setup_static(text, strlen(text), 7, \" +\", expected_result, 9999, 9999),\n --\t\t \"not long enough for offset\");\n ++\n +\tTEST(setup_static((struct test_case) {\n +\t\t.line = \"@@ -4,4\",\n +\t\t.offset = 7,\n @@ t/unit-tests/t-apply.c\n +\t\t.expect_p1 = 9999,\n +\t\t.expect_p2 = 9999\n +\t}), \"not long enough for offset\");\n - \n --\ttext = \"@@ -4,4\";\n --\texpected_result = FAILURE;\n --\tTEST(setup_static(text, strlen(text), -1, \" +\", expected_result, 9999, 9999),\n --\t\t \"negative offset\");\n ++\n +\tTEST(setup_static((struct test_case) {\n +\t\t.line = \"@@ -4,4\",\n +\t\t.offset = -1,\n @@ t/unit-tests/t-apply.c\n +\t\t.expect_p1 = 9999,\n +\t\t.expect_p2 = 9999\n +\t}), \"negative offset\");\n - \n - \treturn test_done();\n - }\n ++\n ++\treturn test_done();\n ++}\n\n\n Makefile | 1 +\n apply.c | 6 ++-\n apply.h | 8 ++++\n t/unit-tests/t-apply.c | 101 +++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 114 insertions(+), 2 deletions(-)\n create mode 100644 t/unit-tests/t-apply.c\n\ndiff --git a/Makefile b/Makefile\nindex 8f4432ae57c..1615de69e6c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1334,6 +1334,7 @@ THIRD_PARTY_SOURCES += compat/regex/%\n THIRD_PARTY_SOURCES += sha1collisiondetection/%\n THIRD_PARTY_SOURCES += sha1dc/%\n \n+UNIT_TEST_PROGRAMS += t-apply\n UNIT_TEST_PROGRAMS += t-ctype\n UNIT_TEST_PROGRAMS += t-mem-pool\n UNIT_TEST_PROGRAMS += t-prio-queue\ndiff --git a/apply.c b/apply.c\nindex 901b67e6255..8cdf7e7d77f 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -36,6 +36,8 @@\n #include \"wildmatch.h\"\n #include \"ws.h\"\n \n+#define parse_range apply_parse_fragment_range\n+\n struct gitdiff_data {\n \tstruct strbuf *root;\n \tint linenr;\n@@ -1438,8 +1440,8 @@ static int parse_num(const char *line, unsigned long *p)\n \treturn ptr - line;\n }\n \n-static int parse_range(const char *line, int len, int offset, const char *expect,\n-\t\t unsigned long *p1, unsigned long *p2)\n+int apply_parse_fragment_range(const char *line, int len, int offset, const char *expect,\n+\t\t\t unsigned long *p1, unsigned long *p2)\n {\n \tint digits, ex;\n \ndiff --git a/apply.h b/apply.h\nindex 7cd38b1443c..283aba77495 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -186,4 +186,12 @@ int apply_all_patches(struct apply_state *state,\n \t\t int argc, const char **argv,\n \t\t int options);\n \n+/*\n+ * exposed only for tests; do not call this as it not\n+ * a part of the API\n+ */\n+extern int apply_parse_fragment_range(const char *line, int len, int offset,\n+\t\t\t\t const char *expect, unsigned long *p1,\n+\t\t\t\t unsigned long *p2);\n+\n #endif\ndiff --git a/t/unit-tests/t-apply.c b/t/unit-tests/t-apply.c\nnew file mode 100644\nindex 00000000000..0eb0c22fc0d\n--- /dev/null\n+++ b/t/unit-tests/t-apply.c\n@@ -0,0 +1,101 @@\n+#include \"test-lib.h\"\n+#include \"apply.h\"\n+\n+#define FAILURE -1\n+\n+typedef struct test_case {\n+\tconst char *line;\n+\tconst char *expect_suffix;\n+\tint offset;\n+\tunsigned long expect_p1;\n+\tunsigned long expect_p2;\n+\tint expect_result;\n+} test_case;\n+\n+static void setup_static(struct test_case t)\n+{\n+\tunsigned long p1 = 9999;\n+\tunsigned long p2 = 9999;\n+\tint result = apply_parse_fragment_range(t.line, strlen(t.line), t.offset,\n+\t\t\t\t\t\tt.expect_suffix, &p1, &p2);\n+\tcheck_int(result, ==, t.expect_result);\n+\tcheck_int(p1, ==, t.expect_p1);\n+\tcheck_int(p2, ==, t.expect_p2);\n+}\n+\n+int cmd_main(int argc, const char **argv)\n+{\n+\tTEST(setup_static((struct test_case) {\n+\t\t.line = \"@@ -4,4 +\",\n+\t\t.offset = 4,\n+\t\t.expect_suffix = \" +\",\n+\t\t.expect_result = 9,\n+\t\t.expect_p1 = 4,\n+\t\t.expect_p2 = 4\n+\t}), \"well-formed range\");\n+\n+\tTEST(setup_static((struct test_case) {\n+\t\t.line = \"@@ -4 +8 @@\",\n+\t\t.offset = 4,\n+\t\t.expect_suffix = \" +\",\n+\t\t.expect_result = 7,\n+\t\t.expect_p1 = 4,\n+\t\t.expect_p2 = 1\n+\t}), \"non-comma range\");\n+\n+\tTEST(setup_static((struct test_case) {\n+\t\t.line = \"@@ -X,4 +\",\n+\t\t.offset = 4,\n+\t\t.expect_suffix = \" +\",\n+\t\t.expect_result = FAILURE,\n+\t\t.expect_p1 = 9999,\n+\t\t.expect_p2 = 9999\n+\t}), \"non-digit range (first coordinate)\");\n+\n+\tTEST(setup_static((struct test_case) {\n+\t\t.line = \"@@ -4,X +\",\n+\t\t.offset = 4,\n+\t\t.expect_suffix = \" +\",\n+\t\t.expect_result = FAILURE,\n+\t\t.expect_p1 = 4,\n+\t\t.expect_p2 = 1 // A little strange this is 1, but not end of the world\n+\t}), \"non-digit range (second coordinate)\");\n+\n+\tTEST(setup_static((struct test_case) {\n+\t\t.line = \"@@ -4,4 -\",\n+\t\t.offset = 4,\n+\t\t.expect_suffix = \" +\",\n+\t\t.expect_result = FAILURE,\n+\t\t.expect_p1 = 4,\n+\t\t.expect_p2 = 4\n+\t}), \"non-expected trailing text\");\n+\n+\tTEST(setup_static((struct test_case) {\n+\t\t.line = \"@@ -4,4\",\n+\t\t.offset = 4,\n+\t\t.expect_suffix = \" +\",\n+\t\t.expect_result = FAILURE,\n+\t\t.expect_p1 = 4,\n+\t\t.expect_p2 = 4\n+\t}), \"not long enough for expected trailing text\");\n+\n+\tTEST(setup_static((struct test_case) {\n+\t\t.line = \"@@ -4,4\",\n+\t\t.offset = 7,\n+\t\t.expect_suffix = \" +\",\n+\t\t.expect_result = FAILURE,\n+\t\t.expect_p1 = 9999,\n+\t\t.expect_p2 = 9999\n+\t}), \"not long enough for offset\");\n+\n+\tTEST(setup_static((struct test_case) {\n+\t\t.line = \"@@ -4,4\",\n+\t\t.offset = -1,\n+\t\t.expect_suffix = \" +\",\n+\t\t.expect_result = FAILURE,\n+\t\t.expect_p1 = 9999,\n+\t\t.expect_p2 = 9999\n+\t}), \"negative offset\");\n+\n+\treturn test_done();\n+}\n\nbase-commit: b9cfe4845cb2562584837bc0101c0ab76490a239\n-- \ngitgitgadget\n"},{"id":"496547","messageId":"xmqqle3im1bx.fsf@gitster.g","threadId":"60954","inReplyTo":"pull.1677.v2.git.git.1716710073910.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] apply: add unit tests for parse_range","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-06T17:24:18Z","receivedAt":"2024-06-06T17:24:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Peterson via GitGitGadget\" writes:\n\n> From: Philip Peterson \n>\n> Also rename parse_range to parse_fragment_range for external linkage.\n>\n> Signed-off-by: Philip Peterson \n> ---\n\nThis version saw no reviews (unfortunately).\n\nI just gave another quick glance, did not spot anything glaringly\nwrong, and the way the tests are organized as a series of ...\n\n> +\tTEST(setup_static((struct test_case) {\n> +\t\t.line = \"@@ -4,4\",\n> +\t\t.offset = -1,\n> +\t\t.expect_suffix = \" +\",\n> +\t\t.expect_result = FAILURE,\n> +\t\t.expect_p1 = 9999,\n> +\t\t.expect_p2 = 9999\n> +\t}), \"negative offset\");\n\n... rather pleasant to read. I'd admit that it does not count as a\nproper review, though.\n\nThanks.\n\n\n\n"},{"id":"496647","messageId":"b7eca313-9ea8-4132-ba1d-ed9236e07095@gmail.com","threadId":"60954","inReplyTo":"pull.1677.v2.git.git.1716710073910.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] apply: add unit tests for parse_range","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-07T15:00:07Z","receivedAt":"2024-06-07T15:00:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Philip\n\nThanks for re-rolling, I've left a few comments below\n\nOn 26/05/2024 08:54, Philip Peterson via GitGitGadget wrote:\n> From: Philip Peterson \n> \n> Also rename parse_range to parse_fragment_range for external linkage.\n> \n> Signed-off-by: Philip Peterson \n> ---\n> apply: add unit tests for parse_range\n> \n> Add unit tests for apply's parse_range function. Also rename parse_range\n> to parse_fragment_range and expose it externally for use by the unit\n> tests. Internal callers may continue to refer to it by the name\n> parse_range.\n> \n> It is necessary to make the function be non-internal linkage in order to\n> expose it to the unit testing framework. Because there is another\n> function in the codebase also called parse_range, change this one to a\n> more specific name as well: parse_fragment_range. Also add several test\n> cases (both positive and negative) for the function.\n\nThis description is useful and should be part of the commit message \nabove the \"---\" line\n\n> +/*\n> + * exposed only for tests; do not call this as it not\n> + * a part of the API\n> + */\n> +extern int apply_parse_fragment_range(const char *line, int len, int offset,\n> +\t\t\t\t const char *expect, unsigned long *p1,\n> +\t\t\t\t unsigned long *p2);\n\nWe don't bother with extern on function declarations as can be seen from \nall the other declarations in this header file.\n\n> +\n> #endif\n> diff --git a/t/unit-tests/t-apply.c b/t/unit-tests/t-apply.c\n> new file mode 100644\n> index 00000000000..0eb0c22fc0d\n> --- /dev/null\n> +++ b/t/unit-tests/t-apply.c\n> @@ -0,0 +1,101 @@\n> +#include \"test-lib.h\"\n> +#include \"apply.h\"\n> +\n> +#define FAILURE -1\n> +\n> +typedef struct test_case {\n> +\tconst char *line;\n> +\tconst char *expect_suffix;\n> +\tint offset;\n> +\tunsigned long expect_p1;\n> +\tunsigned long expect_p2;\n> +\tint expect_result;\n> +} test_case;\n\nWe don't use typedef's in this code base\n\n> +static void setup_static(struct test_case t)\n> +{\n> +\tunsigned long p1 = 9999;\n> +\tunsigned long p2 = 9999;\n> +\tint result = apply_parse_fragment_range(t.line, strlen(t.line), t.offset,\n> +\t\t\t\t\t\tt.expect_suffix, &p1, &p2);\n> +\tcheck_int(result, ==, t.expect_result);\n\nIf we expect apply_parse_fragment_range() to return an error then I'm \nnot sure we want to bother checking p1 and p2 as I don't think we make \nany guarantees about their values in that case. If we promised not to \ntouch them unless the fragment was successfully parsed we would want to \ncheck that but we don't make that promise.\n\n> +\tcheck_int(p1, ==, t.expect_p1);\n> +\tcheck_int(p2, ==, t.expect_p2);\n> +}\n> +\n> +int cmd_main(int argc, const char **argv)\n> +{\n> +\tTEST(setup_static((struct test_case) {\n> +\t\t.line = \"@@ -4,4 +\",\n> +\t\t.offset = 4,\n> +\t\t.expect_suffix = \" +\",\n> +\t\t.expect_result = 9,\n> +\t\t.expect_p1 = 4,\n> +\t\t.expect_p2 = 4\n> +\t}), \"well-formed range\");\n\nThis is a nice use of compound literals. It would be good to have a test \nthat checks .expect_result for a longer input string to ensure we're not \njust stopping at the end of the string.\n\n> +\tTEST(setup_static((struct test_case) {\n> +\t\t.line = \"@@ -4 +8 @@\",\n\nI would be nice to vary \"-4\" a bit, all the tests where the parse is \nsuccessful expect p1 == 4\n\n> +\t\t.offset = 4,\n> +\t\t.expect_suffix = \" +\",\n> +\t\t.expect_result = 7,\n> +\t\t.expect_p1 = 4,\n> +\t\t.expect_p2 = 1\n> +\t}), \"non-comma range\");\n> +\n> +\tTEST(setup_static((struct test_case) {\n> +\t\t.line = \"@@ -X,4 +\",\n> +\t\t.offset = 4,\n> +\t\t.expect_suffix = \" +\",\n> +\t\t.expect_result = FAILURE,\n> +\t\t.expect_p1 = 9999,\n> +\t\t.expect_p2 = 9999\n> +\t}), \"non-digit range (first coordinate)\");\n> +\n> +\tTEST(setup_static((struct test_case) {\n> +\t\t.line = \"@@ -4,X +\",\n> +\t\t.offset = 4,\n> +\t\t.expect_suffix = \" +\",\n> +\t\t.expect_result = FAILURE,\n> +\t\t.expect_p1 = 4,\n> +\t\t.expect_p2 = 1 // A little strange this is 1, but not end of the world\n\nThis is an example of why I don't think we should check p1 and p2 when \nwe're expecting the parse to fail. Also please note we don't use \"//\" \ncomments in the code base.\n\nThere is a good range of failing non-digit inputs. It would be nice to \nsee a test for a hunk header where the number is too large to parse. \nIdeally we'd also change the parsing code to return an error in that \ncase rather than waiting to error out later on as the apply code does \ncurrently. To do that You'd need to explicitly set errno to zero before \ncalling strtoul() and check it afterwards.\n\nOverall this is looking pretty good, thanks for working on it\n\nBest Wishes\n\nPhillip\n\n\n> +\t}), \"non-digit range (second coordinate)\");\n> +\n> +\tTEST(setup_static((struct test_case) {\n> +\t\t.line = \"@@ -4,4 -\",\n> +\t\t.offset = 4,\n> +\t\t.expect_suffix = \" +\",\n> +\t\t.expect_result = FAILURE,\n> +\t\t.expect_p1 = 4,\n> +\t\t.expect_p2 = 4\n> +\t}), \"non-expected trailing text\");\n> +\n> +\tTEST(setup_static((struct test_case) {\n> +\t\t.line = \"@@ -4,4\",\n> +\t\t.offset = 4,\n> +\t\t.expect_suffix = \" +\",\n> +\t\t.expect_result = FAILURE,\n> +\t\t.expect_p1 = 4,\n> +\t\t.expect_p2 = 4\n> +\t}), \"not long enough for expected trailing text\");\n> +\n> +\tTEST(setup_static((struct test_case) {\n> +\t\t.line = \"@@ -4,4\",\n> +\t\t.offset = 7,\n> +\t\t.expect_suffix = \" +\",\n> +\t\t.expect_result = FAILURE,\n> +\t\t.expect_p1 = 9999,\n> +\t\t.expect_p2 = 9999\n> +\t}), \"not long enough for offset\");\n> +\n> +\tTEST(setup_static((struct test_case) {\n> +\t\t.line = \"@@ -4,4\",\n> +\t\t.offset = -1,\n> +\t\t.expect_suffix = \" +\",\n> +\t\t.expect_result = FAILURE,\n> +\t\t.expect_p1 = 9999,\n> +\t\t.expect_p2 = 9999\n> +\t}), \"negative offset\");\n> +\n> +\treturn test_done();\n> +}\n> \n> base-commit: b9cfe4845cb2562584837bc0101c0ab76490a239\n"},{"id":"496657","messageId":"xmqqo78celjy.fsf@gitster.g","threadId":"60954","inReplyTo":"b7eca313-9ea8-4132-ba1d-ed9236e07095@gmail.com","subject":"Re: [PATCH v2] apply: add unit tests for parse_range","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T16:59:13Z","receivedAt":"2024-06-07T16:59:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Thanks for re-rolling, I've left a few comments below\n> ...\n> This is an example of why I don't think we should check p1 and p2 when\n> we're expecting the parse to fail. Also please note we don't use \"//\"\n> comments in the code base.\n>\n> There is a good range of failing non-digit inputs. It would be nice to\n> see a test for a hunk header where the number is too large to\n> parse. Ideally we'd also change the parsing code to return an error in\n> that case rather than waiting to error out later on as the apply code\n> does currently. To do that You'd need to explicitly set errno to zero\n> before calling strtoul() and check it afterwards.\n\nThanks. All points you raised make quite a lot of sense to me.\n\n"}]}