{"thread":{"id":"64413","subject":"[PATCH] fsck: use starts_with() in fsck_commit()","startedAt":"2025-10-31T22:00:46Z","lastAt":"2025-11-02T03:58:36Z","messageCount":3,"participants":["keita","Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"530053","messageId":"q4heOh8stc94r_P5mX-tucCyqQ1JdGP2dJ9Dot3WTyDboRWsboWMAIoCQIXuyuRvNRmN5AGQYjhmAnjuxgOXNooUDjPAEEvKPc2k_DCDEtY=@proton.me","threadId":"64413","inReplyTo":null,"subject":"[PATCH] fsck: use starts_with() in fsck_commit()","fromName":"keita","fromEmail":"rudykeita@proton.me","sentAt":"2025-10-31T22:00:27Z","receivedAt":"2025-10-31T22:00:46Z","isPatch":true,"sender":{"key":"rudykeita@proton.me","avatar":null},"body":"\n\n\nFrom 30136adebaffb97edacae2c58c4ea491e39e3f5b Mon Sep 17 00:00:00 2001From: Songiso Cooper Lyambai <rudykeita@proton.me>\nDate: Fri, 31 Oct 2025 23:45:23 +0200\nSubject: [PATCH] fsck: use starts_with() in fsck_commit()\n\nReplace manual buffer checks with starts_with() for safety and clarity.\n\nThis avoids buffer overreads and follows Git's idiomatic style used \n\n\nSigned-off-by: Songiso Cooper Lyambai <rudykeita@proton.me>\n---\n fsck.c | 124 +++++++++++++++++++++++++++++++--------------------------\n 1 file changed, 67 insertions(+), 57 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 341e100d24..7172c4ff1c 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -921,67 +921,77 @@ static int fsck_ident(const char **ident,\n }\n\n static int fsck_commit(const struct object_id *oid,\n-       const char *buffer, unsigned long size,\n-       struct fsck_options *options)\n+               const char *buffer, unsigned long size,\n+               struct fsck_options *options)\n {\n- struct object_id tree_oid, parent_oid;\n- unsigned author_count;\n- int err;\n- const char *buffer_begin = buffer;\n- const char *buffer_end = buffer + size;\n- const char *p;\n+    struct object_id tree_oid, parent_oid;\n+    unsigned author_count = 0;\n+    int err = 0;\n+    const char *buffer_end = buffer + size;\n+    const char *p;\n\n- /*\n- * We _must_ stop parsing immediately if this reports failure, as the\n- * memory safety of the rest of the function depends on it. See the\n- * comment above the definition of verify_headers() for more details.\n- */\n- if (verify_headers(buffer, size, oid, OBJ_COMMIT, options))\n- return -1;\n\n- if (buffer >= buffer_end || !skip_prefix(buffer, \"tree \", &buffer))\n- return report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_TREE, \"invalid format - expected 'tree' line\");\n- if (parse_oid_hex(buffer, &tree_oid, &p) || *p != '\\n') {\n- err = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_TREE_SHA1, \"invalid 'tree' line format - bad sha1\");\n- if (err)\n- return err;\n- }\n- buffer = p + 1;\n- while (buffer < buffer_end && skip_prefix(buffer, \"parent \", &buffer)) {\n- if (parse_oid_hex(buffer, &parent_oid, &p) || *p != '\\n') {\n- err = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_PARENT_SHA1, \"invalid 'parent' line format - bad sha1\");\n- if (err)\n- return err;\n- }\n- buffer = p + 1;\n- }\n- author_count = 0;\n- while (buffer < buffer_end && skip_prefix(buffer, \"author \", &buffer)) {\n- author_count++;\n- err = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n- if (err)\n- return err;\n- }\n- if (author_count < 1)\n- err = report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_AUTHOR, \"invalid format - expected 'author' line\");\n- else if (author_count > 1)\n- err = report(options, oid, OBJ_COMMIT, FSCK_MSG_MULTIPLE_AUTHORS, \"invalid format - multiple 'author' lines\");\n- if (err)\n- return err;\n- if (buffer >= buffer_end || !skip_prefix(buffer, \"committer \", &buffer))\n- return report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_COMMITTER, \"invalid format - expected 'committer' line\");\n- err = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n- if (err)\n- return err;\n- if (memchr(buffer_begin, '\\0', size)) {\n- err = report(options, oid, OBJ_COMMIT, FSCK_MSG_NUL_IN_COMMIT,\n-     \"NUL byte in the commit object body\");\n- if (err)\n- return err;\n- }\n- return 0;\n+ /*\n+ * We _must_ stop parsing immediately if this reports failure, as the\n+ * memory safety of the rest of the function depends on it. See the\n+ * comment above the definition of verify_headers() for more details.\n+ */\n+\n+    if (verify_headers(buffer, size, oid, OBJ_COMMIT, options))\n+        return -1;\n+\n+    \n+    if (!skip_prefix(buffer, \"tree \", &buffer))\n+        return report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_TREE,\n+                  \"invalid format - expected 'tree' line\");\n+    if (parse_oid_hex(buffer, &tree_oid, &p) || *p != '\\n') {\n+        return report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_TREE_SHA1,\n+                  \"invalid 'tree' line format - bad sha1\");\n+    }\n+    buffer = p + 1;\n+\n+    while (starts_with(buffer, \"parent \")) {\n+        if (!skip_prefix(buffer, \"parent \", &buffer) ||\n+            parse_oid_hex(buffer, &parent_oid, &p) || *p != '\\n') {\n+            return report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_PARENT_SHA1,\n+                      \"invalid 'parent' line format - bad sha1\");\n+        }\n+        buffer = p + 1;\n+    }\n+\n+    while (starts_with(buffer, \"author \")) {\n+        author_count++;\n+        if (!skip_prefix(buffer, \"author \", &buffer))\n+            return report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_AUTHOR,\n+                      \"invalid format - expected 'author' line\");\n+        if ((err = fsck_ident(&buffer, oid, OBJ_COMMIT, options)))\n+            return err;\n+    }\n+\n+    if (author_count < 1)\n+        return report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_AUTHOR,\n+                  \"invalid format - expected 'author' line\");\n+    if (author_count > 1)\n+        return report(options, oid, OBJ_COMMIT, FSCK_MSG_MULTIPLE_AUTHORS,\n+                  \"invalid format - multiple 'author' lines\");\n+\n+    if (!starts_with(buffer, \"committer \"))\n+        return report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_COMMITTER,\n+                  \"invalid format - expected 'committer' line\");\n+\n+    if (!skip_prefix(buffer, \"committer \", &buffer))\n+        return report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_COMMITTER,\n+                  \"invalid format - expected 'committer' line\");\n+\n+    if ((err = fsck_ident(&buffer, oid, OBJ_COMMIT, options)))\n+        return err;\n+\n+    if (memchr(buffer, '\\0', buffer_end - buffer))\n+        return report(options, oid, OBJ_COMMIT, FSCK_MSG_NUL_IN_COMMIT,\n+                  \"NUL byte in the commit object body\");\n+\n+    return 0;\n }\n"},{"id":"530061","messageId":"CAP8UFD0CqC2tgERkPHuoOPO2ON9NNw_C1R-6UpBXjpgYEM8yxQ@mail.gmail.com","threadId":"64413","inReplyTo":"q4heOh8stc94r_P5mX-tucCyqQ1JdGP2dJ9Dot3WTyDboRWsboWMAIoCQIXuyuRvNRmN5AGQYjhmAnjuxgOXNooUDjPAEEvKPc2k_DCDEtY=@proton.me","subject":"Re: [PATCH] fsck: use starts_with() in fsck_commit()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2025-11-01T14:59:43Z","receivedAt":"2025-11-01T14:59:57Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Fri, Oct 31, 2025 at 11:01 PM keita <rudykeita@proton.me> wrote:\n\n> From 30136adebaffb97edacae2c58c4ea491e39e3f5b Mon Sep 17 00:00:00 2001From: Songiso Cooper Lyambai <rudykeita@proton.me>\n> Date: Fri, 31 Oct 2025 23:45:23 +0200\n> Subject: [PATCH] fsck: use starts_with() in fsck_commit()\n\nIf this is related to Outreachy, it would be better to put\n\"[Outreachy]\" at the start of the subject.\n\n> Replace manual buffer checks with starts_with() for safety and clarity.\n>\n> This avoids buffer overreads and follows Git's idiomatic style used\n\nIt looks like the above sentence is not finished. Maybe s/ used/./ ?\n\n> Signed-off-by: Songiso Cooper Lyambai <rudykeita@proton.me>\n> ---\n>  fsck.c | 124 +++++++++++++++++++++++++++++++--------------------------\n>  1 file changed, 67 insertions(+), 57 deletions(-)\n\nA lot of lines seem to be changed for a patch that wants to use\nstarts_with() in fsck_commit(). Let's see below.\n\n> diff --git a/fsck.c b/fsck.c\n> index 341e100d24..7172c4ff1c 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -921,67 +921,77 @@ static int fsck_ident(const char **ident,\n>  }\n>\n>  static int fsck_commit(const struct object_id *oid,\n> -       const char *buffer, unsigned long size,\n> -       struct fsck_options *options)\n> +               const char *buffer, unsigned long size,\n> +               struct fsck_options *options)\n\nIf a patch does other things than it's main goal, it should be\nmentioned in the commit message that it's doing those other things\n\"while at it\".\n\nAnyway when I look at fsck_commit() in \"fsck.c\" on 'master', it seems\nto me that the function is properly indented. So I suspect that your\npatch changes its indentation for no good reason.\n\n>  {\n> - struct object_id tree_oid, parent_oid;\n> - unsigned author_count;\n> - int err;\n> - const char *buffer_begin = buffer;\n> - const char *buffer_end = buffer + size;\n> - const char *p;\n> +    struct object_id tree_oid, parent_oid;\n> +    unsigned author_count = 0;\n> +    int err = 0;\n> +    const char *buffer_end = buffer + size;\n> +    const char *p;\n\nHere also I suspect that the indentation changes are not necessary.\nThey are also making it more difficult to spot actual changes like\n'buffer_begin' being removed and 'author_count' and 'err' being\ninitialized to 0.\n\nSorry I am stopping my review here as there seems to be too many\nindentation changes that are masking the actual changes.\n\nPlease make sure you send patches that don't change the indentation\nfor no good reason.\n\nThanks.\n"},{"id":"530073","messageId":"xmqqseexw0eu.fsf@gitster.g","threadId":"64413","inReplyTo":"CAP8UFD0CqC2tgERkPHuoOPO2ON9NNw_C1R-6UpBXjpgYEM8yxQ@mail.gmail.com","subject":"Re: [PATCH] fsck: use starts_with() in fsck_commit()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-02T03:58:33Z","receivedAt":"2025-11-02T03:58:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> Hi,\n>\n> On Fri, Oct 31, 2025 at 11:01 PM keita <rudykeita@proton.me> wrote:\n>\n>> From 30136adebaffb97edacae2c58c4ea491e39e3f5b Mon Sep 17 00:00:00 2001From: Songiso Cooper Lyambai <rudykeita@proton.me>\n>> Date: Fri, 31 Oct 2025 23:45:23 +0200\n>> Subject: [PATCH] fsck: use starts_with() in fsck_commit()\n>\n> If this is related to Outreachy, it would be better to put\n> \"[Outreachy]\" at the start of the subject.\n\nPlus there is a lot more important thing to be said for this part fo\nthe lines that you forgot to point out.  They should *NOT* be part\nof the e-mail body.  The Subject: header of the e-mail seems to be\nset to the same as this line, so the sender only needs to delete all\nthese four lines from the e-mail body and correct the subject.\n\n>>  {\n>> - struct object_id tree_oid, parent_oid;\n>> - unsigned author_count;\n>> - int err;\n>> - const char *buffer_begin = buffer;\n>> - const char *buffer_end = buffer + size;\n>> - const char *p;\n>> +    struct object_id tree_oid, parent_oid;\n>> +    unsigned author_count = 0;\n>> +    int err = 0;\n>> +    const char *buffer_end = buffer + size;\n>> +    const char *p;\n>\n> Here also I suspect that the indentation changes are not necessary.\n> They are also making it ...\n\nDoes the preimage even match our code?  There is no C code in our\ncodebase that uses a single space indent, so I would not expect\nthese 7 lines of preimage to be found in fsck.c or anywhere in our\ncodebase.\n\nHence, another more important thing to point out is that the patch\nwould not apply.  A suggestion to the author (and other aspiring\nfolks who want to become Git developers) is to\n\n - Send the patch you are planning to submit, but not to the list\n   but only to yourself.\n\n - Subscribe to the list and then observe the traffic for a day or\n   two to find patch e-mails from others.  Find other patch e-mails\n   from each of these people at https://lore.kernel.org/git and pick\n   the author who is highly regarded.\n\n - Compare the e-mailed patch you received from yourself, and the\n   one you received from the list written by that highly regarded\n   author you picked.  I am reasonably sure that they do not have\n   the e-mail headers repeated.\n\n - In your clone of Git, check out an appropriate target; if you are\n   fixing something, you want to use a tag that corresponds to a\n   released version, if you are proposing a new feature, you want to\n   build on the latest released version.  Use \"git am\" to apply the\n   e-mailed patch you received from yourself.\n\nWe may want to #leftoverbits add something like the above to the\nMyFirstContribution document near the part that it teachs to send\npatches.\n\n> Please make sure you send patches that don't change the indentation\n> for no good reason.\n\nA good suggestion that applies much wider than the indentation.\nYour patch should not do anything related to the theme of the patch,\nwhcih you explained in your proposed log message.\n"}]}