{"thread":{"id":"64329","subject":"[Outreachy PATCH v2 0/2] gpg-interface.c: use string_list_split() instead of strbuf_split_max()","startedAt":"2025-10-16T01:04:55Z","lastAt":"2025-10-17T18:47:19Z","messageCount":10,"participants":["Olamide Caleb Bello","Junio C Hamano","Christian Couder","Bello Olamide"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"528923","messageId":"cover.1760571220.git.belkid98@gmail.com","threadId":"64329","inReplyTo":null,"subject":"[Outreachy PATCH v2 0/2] gpg-interface.c: use string_list_split() instead of strbuf_split_max()","fromName":"Olamide Caleb Bello","fromEmail":"belkid98@gmail.com","sentAt":"2025-10-16T01:03:52Z","receivedAt":"2025-10-16T01:04:55Z","isPatch":true,"sender":{"key":"belkid98@gmail.com","avatar":"https://avatars.githubusercontent.com/u/73387291?v=4"},"body":"Commit 2efe707054 (wt-status: avoid strbuf_split*(), 2025-07-31) noticed\nthat the array of strbufs that calls to strbuf_split*() provides are merely\nused to store the strings gotten from the split and no edit are\ndone on these resulting strings making the strbuf_split*() unideal\nfor this usecase, with the string_list_split*() being a more suitable option.\n\nThe patch series by Junio Hamano can be seen in the link below.\n\nhttps://public-inbox.org/git/20250731225433.4028872-1-gitster@pobox.com/\n\nThis series continues on this path by replacing instances of\nstrbuf_split_max() with string_list_split() where the string from the\nsplit is merely returned as char * and no edits are done on them.\n\nChanges since v1\n================\n - Added commit reference and link to patch series for previous work\n done on the subject\n\n\nOlamide Caleb Bello (2):\n  gpg-interface: replace strbuf_split_max() with string_list_split()\n  gpg-interface: use string_list_split() instead of strbuf_split_max()\n\n gpg-interface.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\n-- \n2.51.0.463.g79cf913ea9\n\n"},{"id":"528924","messageId":"818ca6b104cf25ebe4c60145d046029f057f4db1.1760571220.git.belkid98@gmail.com","threadId":"64329","inReplyTo":"cover.1760571220.git.belkid98@gmail.com","subject":"[Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split()","fromName":"Olamide Caleb Bello","fromEmail":"belkid98@gmail.com","sentAt":"2025-10-16T01:03:53Z","receivedAt":"2025-10-16T01:04:59Z","isPatch":true,"sender":{"key":"belkid98@gmail.com","avatar":"https://avatars.githubusercontent.com/u/73387291?v=4"},"body":"In get_ssh_finger_print(), the output of the `ssh-keygen` command is\nput into `fingerprint_stdout\n\nThe string in fingerprint_stdout is then split into 3 strbufs using\nstrbuf_split_max(), however they are not modified after the split thereby\nnot making use of the strbuf API as the fingerprint token is merely\nreturned as a char * and not a strbuf, hence they do not need to be strbufs.\n\nUse string_list_split() instead for simplicity.\n\nNote that strbuf_split_max() uses 3 to specify the number of tokens to\nextract from the string, while string_list_split() uses 2 because it specifies\nthe number of times the split will be done on the string, so 2 gives 3 tokens\nas it is in the original instance.\n\nSigned-off-by: Olamide Caleb Bello <belkid98@gmail.com>\n---\n Changes in v2\n   - Reword the commit message for clarity\n   - Add () to the function names\n   - Change the test to ensure the number of tokens appended to the\n    list by string_list_split() equals 3\n gpg-interface.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 2f4f0e32cb..989dca7d14 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -14,6 +14,7 @@\n #include \"sigchain.h\"\n #include \"tempfile.h\"\n #include \"alias.h\"\n+#include \"string-list.h\"\n \n static int git_gpg_config(const char *, const char *,\n \t\t\t  const struct config_context *, void *);\n@@ -821,7 +822,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)\n \tstruct child_process ssh_keygen = CHILD_PROCESS_INIT;\n \tint ret = -1;\n \tstruct strbuf fingerprint_stdout = STRBUF_INIT;\n-\tstruct strbuf **fingerprint;\n+\tstruct string_list split = STRING_LIST_INIT_DUP;\n \tchar *fingerprint_ret;\n \tconst char *literal_key = NULL;\n \n@@ -845,13 +846,12 @@ static char *get_ssh_key_fingerprint(const char *signing_key)\n \t\tdie_errno(_(\"failed to get the ssh fingerprint for key '%s'\"),\n \t\t\t  signing_key);\n \n-\tfingerprint = strbuf_split_max(&fingerprint_stdout, ' ', 3);\n-\tif (!fingerprint[1])\n+\tif (string_list_split(&split, fingerprint_stdout.buf, \" \", 2) != 3)\n \t\tdie_errno(_(\"failed to get the ssh fingerprint for key '%s'\"),\n \t\t\t  signing_key);\n \n-\tfingerprint_ret = strbuf_detach(fingerprint[1], NULL);\n-\tstrbuf_list_free(fingerprint);\n+\tfingerprint_ret = xstrdup(split.items[1].string);\n+\tstring_list_clear(&split, 0);\n \tstrbuf_release(&fingerprint_stdout);\n \treturn fingerprint_ret;\n }\n-- \n2.51.0.463.g79cf913ea9\n\n"},{"id":"528925","messageId":"024a44a242394f5a067f17ec97a99c61232898fe.1760571220.git.belkid98@gmail.com","threadId":"64329","inReplyTo":"cover.1760571220.git.belkid98@gmail.com","subject":"[Outreachy PATCH v2 2/2] gpg-interface: use string_list_split() instead of strbuf_split_max()","fromName":"Olamide Caleb Bello","fromEmail":"belkid98@gmail.com","sentAt":"2025-10-16T01:03:54Z","receivedAt":"2025-10-16T01:05:03Z","isPatch":true,"sender":{"key":"belkid98@gmail.com","avatar":"https://avatars.githubusercontent.com/u/73387291?v=4"},"body":"In get_default_ssh_signing_key(), the default ssh signing key is\nretrieved in `key_stdout`, which is then split using\nstrbuf_split_max() into two tokens\n\nThe string in `key_stdout` is then split using strbuf_split_max() into\ntwo tokens at a new line and the first token is returned as a `char *`\nand not a strbuf.\nThis makes the function lack the use of strbuf API as no edits are\nperformed on the split tokens.\n\nReplace strbuf_split_max() with string_list_split() for simplicity.\n\nNote that strbuf_split_max() uses `2` to indicate the number of tokens\nto extract from the string, while string_list_split() uses `1` to specify\nthe number of times the split will be done on the string, so 1 gives 2\ntokens as it is in the original instance.\n\nSigned-off-by: Olamide Caleb Bello <belkid98@gmail.com>\n---\nChanges in v2\n - Reword the commit message for clarity\n - Replace the comment removed in v1\n - Change the test to ensure the number of tokens added to the list by\n   string_list_split() equals 2\n - Add () to function names\n\n gpg-interface.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 989dca7d14..ab7b8b46e5 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -862,7 +862,7 @@ static char *get_default_ssh_signing_key(void)\n \tstruct child_process ssh_default_key = CHILD_PROCESS_INIT;\n \tint ret = -1;\n \tstruct strbuf key_stdout = STRBUF_INIT, key_stderr = STRBUF_INIT;\n-\tstruct strbuf **keys;\n+\tstruct string_list keys = STRING_LIST_INIT_DUP;\n \tchar *key_command = NULL;\n \tconst char **argv;\n \tint n;\n@@ -884,19 +884,19 @@ static char *get_default_ssh_signing_key(void)\n \t\t\t   &key_stderr, 0);\n \n \tif (!ret) {\n-\t\tkeys = strbuf_split_max(&key_stdout, '\\n', 2);\n-\t\tif (keys[0] && is_literal_ssh_key(keys[0]->buf, &literal_key)) {\n+\t\tif (string_list_split(&keys, key_stdout.buf, \"\\n\", 1) == 2 &&\n+\t\t\tis_literal_ssh_key(keys.items[0].string, &literal_key)) {\n \t\t\t/*\n \t\t\t * We only use `is_literal_ssh_key` here to check validity\n \t\t\t * The prefix will be stripped when the key is used.\n \t\t\t */\n-\t\t\tdefault_key = strbuf_detach(keys[0], NULL);\n+\t\t\tdefault_key = xstrdup(keys.items[0].string);\n \t\t} else {\n \t\t\twarning(_(\"gpg.ssh.defaultKeyCommand succeeded but returned no keys: %s %s\"),\n \t\t\t\tkey_stderr.buf, key_stdout.buf);\n \t\t}\n \n-\t\tstrbuf_list_free(keys);\n+\t\tstring_list_clear(&keys, 0);\n \t} else {\n \t\twarning(_(\"gpg.ssh.defaultKeyCommand failed: %s %s\"),\n \t\t\tkey_stderr.buf, key_stdout.buf);\n-- \n2.51.0.463.g79cf913ea9\n\n"},{"id":"528977","messageId":"xmqqms5q4v0r.fsf@gitster.g","threadId":"64329","inReplyTo":"818ca6b104cf25ebe4c60145d046029f057f4db1.1760571220.git.belkid98@gmail.com","subject":"Re: [Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-16T17:27:16Z","receivedAt":"2025-10-16T17:27:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Olamide Caleb Bello <belkid98@gmail.com> writes:\n\n> @@ -821,7 +822,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)\n>  \tstruct child_process ssh_keygen = CHILD_PROCESS_INIT;\n>  \tint ret = -1;\n>  \tstruct strbuf fingerprint_stdout = STRBUF_INIT;\n> -\tstruct strbuf **fingerprint;\n> +\tstruct string_list split = STRING_LIST_INIT_DUP;\n>  \tchar *fingerprint_ret;\n>  \tconst char *literal_key = NULL;\n>  \n> @@ -845,13 +846,12 @@ static char *get_ssh_key_fingerprint(const char *signing_key)\n>  \t\tdie_errno(_(\"failed to get the ssh fingerprint for key '%s'\"),\n>  \t\t\t  signing_key);\n>  \n> -\tfingerprint = strbuf_split_max(&fingerprint_stdout, ' ', 3);\n> -\tif (!fingerprint[1])\n> +\tif (string_list_split(&split, fingerprint_stdout.buf, \" \", 2) != 3)\n\nThe original splits the thing into upto 3 pieces, but only complains\nif the second piece is NULL (i.e. we said \"up to 3\", but there was\nnot even one place to split, and the request to split_max gave the\none thing as one piece).  IOW, the original code will happily accept\nif the finterprint_stdout is split only into two, not three.\n\nThe updated code asks to split at at most two places (yes, it is a\nconfusing API, but if you split at two places, you will end up with\nthree pieces), and insists that the split results in three pieces. \n\nSo the rewrite tightens the error condition.\n\nWas the original code too loose in detecting an error, and does this\npatch tightens the condition \"while at it\"?  Or was the original\ncode correct to expect that there are legitimate cases where the\npayload in finterprint_stdout only contains two pieces, and it was\nthe right thing to do to accept when fingerprint[1] is not NULL but\nfingerprint[2] is NULL?\n\nThis is a genuine question.  I haven't studied the code path to\nreach this point in the code flow, I don't know what the data in\nfingerprint_stdout is supposed to look like, so I do not know the\nanswer to the question (in other words, it cannot be an oblique way\nto point out that the updated code is wrong or anything like that).\n\n>  \t\tdie_errno(_(\"failed to get the ssh fingerprint for key '%s'\"),\n>  \t\t\t  signing_key);\n>  \n> -\tfingerprint_ret = strbuf_detach(fingerprint[1], NULL);\n> -\tstrbuf_list_free(fingerprint);\n> +\tfingerprint_ret = xstrdup(split.items[1].string);\n> +\tstring_list_clear(&split, 0);\n>  \tstrbuf_release(&fingerprint_stdout);\n\nSince this code releases fingerprint_stdout before leaving, and\nreturns a fresh copy of a split piece, it may make more sense to use\nstring_list_split_in_place(), which does not have to allocate extra\nstrings while it does its work, unlike string_list_split().\n\n>  \treturn fingerprint_ret;\n>  }\n"},{"id":"529045","messageId":"CAP8UFD0Lzmt5ayQ3iSXtmJohDawmCEiu1r8Gq2mKTMy-1gX_fw@mail.gmail.com","threadId":"64329","inReplyTo":"cover.1760571220.git.belkid98@gmail.com","subject":"Re: [Outreachy PATCH v2 0/2] gpg-interface.c: use string_list_split() instead of strbuf_split_max()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2025-10-17T07:58:18Z","receivedAt":"2025-10-17T07:58:32Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Oct 16, 2025 at 3:04 AM Olamide Caleb Bello <belkid98@gmail.com> wrote:\n>\n> Commit 2efe707054 (wt-status: avoid strbuf_split*(), 2025-07-31) noticed\n> that the array of strbufs that calls to strbuf_split*() provides are merely\n> used to store the strings gotten from the split and no edit are\n> done on these resulting strings making the strbuf_split*() unideal\n> for this usecase, with the string_list_split*() being a more suitable option.\n>\n> The patch series by Junio Hamano can be seen in the link below.\n>\n> https://public-inbox.org/git/20250731225433.4028872-1-gitster@pobox.com/\n\nThis description is probably good enough as-is, but here are some\ncomments that might help improve it if you want if you send a v3.\n\nThe way you explain things up to here, it might seem only one commit\nin that series is about avoiding strbuf_split*(). But in fact the\ncommit you mention is the first one in that series which is named \"do\nnot overuse strbuf_split*()\" and contains 11 patches.\n\nSo I think it would be a bit better if, instead of speaking about that\ncommit first, this cover letter started with a link to that patch\nseries and explained the purpose of the whole series. You may then\nmention one or more commits in the series as examples of commits where\nstrbuf_split*() is replaced with string_list_split*() though if you\nwant.\n\n> This series continues on this path by replacing instances of\n> strbuf_split_max() with string_list_split() where the string from the\n> split is merely returned as char * and no edits are done on them.\n\nYeah, some commits in the series do that, but not all.\n\n> Changes since v1\n> ================\n>  - Added commit reference and link to patch series for previous work\n>  done on the subject\n\nIn a cover letter we are interested in the changes in the patch series\nsince the previous version, not the changes in the cover letter. This\nis because the cover letter itself is not merged (except perhaps its\nfirst few sentences that might be reused in the merge commit, but this\nis more advanced, so don't take this into account for now) when a\npatch series is merged. So a cover letter is more about giving context\nto reviewers and inviting them to review the patch series.\n\nSo here it would be nice if there were things like:\n\n  - a summary of the changes in the patch series since v1,\n  - a range diff between v1 and v2,\n  - a link to a CI platform where the v2 has been pushed and the CI\ntests have been performed.\n\nThis would help make reviewer confident that the series is in a much\nbetter shape compared to v1 and reviewers' comments on v1 have been\ntaken into account.\n\nThanks.\n"},{"id":"529050","messageId":"CAP8UFD1FMwrBxbZ6Ck8JwBPBOAhB039US6pHSTusb6qzW=crpg@mail.gmail.com","threadId":"64329","inReplyTo":"xmqqms5q4v0r.fsf@gitster.g","subject":"Re: [Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2025-10-17T09:15:37Z","receivedAt":"2025-10-17T09:15:52Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Oct 16, 2025 at 7:27 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Olamide Caleb Bello <belkid98@gmail.com> writes:\n>\n> > @@ -821,7 +822,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)\n> >       struct child_process ssh_keygen = CHILD_PROCESS_INIT;\n> >       int ret = -1;\n> >       struct strbuf fingerprint_stdout = STRBUF_INIT;\n> > -     struct strbuf **fingerprint;\n> > +     struct string_list split = STRING_LIST_INIT_DUP;\n> >       char *fingerprint_ret;\n> >       const char *literal_key = NULL;\n> >\n> > @@ -845,13 +846,12 @@ static char *get_ssh_key_fingerprint(const char *signing_key)\n> >               die_errno(_(\"failed to get the ssh fingerprint for key '%s'\"),\n> >                         signing_key);\n> >\n> > -     fingerprint = strbuf_split_max(&fingerprint_stdout, ' ', 3);\n> > -     if (!fingerprint[1])\n> > +     if (string_list_split(&split, fingerprint_stdout.buf, \" \", 2) != 3)\n>\n> The original splits the thing into upto 3 pieces, but only complains\n> if the second piece is NULL (i.e. we said \"up to 3\", but there was\n> not even one place to split, and the request to split_max gave the\n> one thing as one piece).  IOW, the original code will happily accept\n> if the finterprint_stdout is split only into two, not three.\n>\n> The updated code asks to split at at most two places (yes, it is a\n> confusing API, but if you split at two places, you will end up with\n> three pieces), and insists that the split results in three pieces.\n>\n> So the rewrite tightens the error condition.\n>\n> Was the original code too loose in detecting an error, and does this\n> patch tightens the condition \"while at it\"?  Or was the original\n> code correct to expect that there are legitimate cases where the\n> payload in finterprint_stdout only contains two pieces, and it was\n> the right thing to do to accept when fingerprint[1] is not NULL but\n> fingerprint[2] is NULL?\n>\n> This is a genuine question.  I haven't studied the code path to\n> reach this point in the code flow, I don't know what the data in\n> fingerprint_stdout is supposed to look like, so I do not know the\n> answer to the question (in other words, it cannot be an oblique way\n> to point out that the updated code is wrong or anything like that).\n\nYeah, I think the problem is that the commit message should explain\nhow this whole issue is dealt with. Currently it talks about passing\n\"2\" to string_list_split() instead of passing \"3\" to\nstrbuf_split_max(), but it says nothing about dealing with the return\nvalue from string_list_split().\n\nIt seems to me that fingerprint_stdout should contain something like:\n\n4096 SHA256:PelI2esT2xZlv20wJJyYOkQsli5RMK79oJ2VxqYb2PA\nchristian.couder@gmail.com (RSA)\n\nand it looks like the 'key_size fingerprint_hash:fingerprint comment\n(key_type)' format is the standard `ssh-keygen -l` output.\n\nSo I think it's safe to say that we should expect\n`string_list_split(..., 2)` to return 3. That should work even if\nthere is no comment in the key file.\n\nBut on the other hand, I think it's also acceptable to say that we\njust want to keep the same behavior as the original code and check its\nreturn value with `> 1` since we only need the second element\nresulting from the split.\n\n> >               die_errno(_(\"failed to get the ssh fingerprint for key '%s'\"),\n> >                         signing_key);\n> >\n"},{"id":"529053","messageId":"CAD=f0L9OFO5N=_3iiqMh33WK+AV-zQPhpdTLn2JusNf4AwnYrg@mail.gmail.com","threadId":"64329","inReplyTo":"CAP8UFD0Lzmt5ayQ3iSXtmJohDawmCEiu1r8Gq2mKTMy-1gX_fw@mail.gmail.com","subject":"Re: [Outreachy PATCH v2 0/2] gpg-interface.c: use string_list_split() instead of strbuf_split_max()","fromName":"Bello Olamide","fromEmail":"belkid98@gmail.com","sentAt":"2025-10-17T09:54:05Z","receivedAt":"2025-10-17T09:54:05Z","isPatch":true,"sender":{"key":"belkid98@gmail.com","avatar":"https://avatars.githubusercontent.com/u/73387291?v=4"},"body":"On Fri, 17 Oct 2025 at 08:58, Christian Couder\n<christian.couder@gmail.com> wrote:\n>\n> On Thu, Oct 16, 2025 at 3:04 AM Olamide Caleb Bello <belkid98@gmail.com> wrote:\n> >\n> > Commit 2efe707054 (wt-status: avoid strbuf_split*(), 2025-07-31) noticed\n> > that the array of strbufs that calls to strbuf_split*() provides are merely\n> > used to store the strings gotten from the split and no edit are\n> > done on these resulting strings making the strbuf_split*() unideal\n> > for this usecase, with the string_list_split*() being a more suitable option.\n> >\n> > The patch series by Junio Hamano can be seen in the link below.\n> >\n> > https://public-inbox.org/git/20250731225433.4028872-1-gitster@pobox.com/\n>\n> This description is probably good enough as-is, but here are some\n> comments that might help improve it if you want if you send a v3.\n>\n> The way you explain things up to here, it might seem only one commit\n> in that series is about avoiding strbuf_split*(). But in fact the\n> commit you mention is the first one in that series which is named \"do\n> not overuse strbuf_split*()\" and contains 11 patches.\n>\n> So I think it would be a bit better if, instead of speaking about that\n> commit first, this cover letter started with a link to that patch\n> series and explained the purpose of the whole series. You may then\n> mention one or more commits in the series as examples of commits where\n> strbuf_split*() is replaced with string_list_split*() though if you\n> want.\n\nYes, this approach would be better.\nThank you\n>\n> > This series continues on this path by replacing instances of\n> > strbuf_split_max() with string_list_split() where the string from the\n> > split is merely returned as char * and no edits are done on them.\n>\n> Yeah, some commits in the series do that, but not all.\n\nYes I will reword this too.\n\n>\n> > Changes since v1\n> > ================\n> >  - Added commit reference and link to patch series for previous work\n> >  done on the subject\n>\n> In a cover letter we are interested in the changes in the patch series\n> since the previous version, not the changes in the cover letter. This\n> is because the cover letter itself is not merged (except perhaps its\n> first few sentences that might be reused in the merge commit, but this\n> is more advanced, so don't take this into account for now) when a\n> patch series is merged. So a cover letter is more about giving context\n> to reviewers and inviting them to review the patch series.\n>\n> So here it would be nice if there were things like:\n>\n>   - a summary of the changes in the patch series since v1,\n>   - a range diff between v1 and v2,\n>   - a link to a CI platform where the v2 has been pushed and the CI\n> tests have been performed.\n>\n> This would help make reviewer confident that the series is in a much\n> better shape compared to v1 and reviewers' comments on v1 have been\n> taken into account.\n>\n\nThank you very much for the review and suggestions.\nI will reorganise and send a corrected v3.\n\nBello.\n"},{"id":"529055","messageId":"CAD=f0L-EEWe7T72iXwA26N_7rA4gaBktzxJPzCku1s8UCj=bwg@mail.gmail.com","threadId":"64329","inReplyTo":"CAP8UFD1FMwrBxbZ6Ck8JwBPBOAhB039US6pHSTusb6qzW=crpg@mail.gmail.com","subject":"Re: [Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split()","fromName":"Bello Olamide","fromEmail":"belkid98@gmail.com","sentAt":"2025-10-17T10:07:34Z","receivedAt":"2025-10-17T10:07:33Z","isPatch":true,"sender":{"key":"belkid98@gmail.com","avatar":"https://avatars.githubusercontent.com/u/73387291?v=4"},"body":"On Fri, 17 Oct 2025 at 10:15, Christian Couder\n<christian.couder@gmail.com> wrote:\n>\n> On Thu, Oct 16, 2025 at 7:27 PM Junio C Hamano <gitster@pobox.com> wrote:\n> >\n> > Olamide Caleb Bello <belkid98@gmail.com> writes:\n> >\n> > > @@ -821,7 +822,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)\n> > >       struct child_process ssh_keygen = CHILD_PROCESS_INIT;\n> > >       int ret = -1;\n> > >       struct strbuf fingerprint_stdout = STRBUF_INIT;\n> > > -     struct strbuf **fingerprint;\n> > > +     struct string_list split = STRING_LIST_INIT_DUP;\n> > >       char *fingerprint_ret;\n> > >       const char *literal_key = NULL;\n> > >\n> > > @@ -845,13 +846,12 @@ static char *get_ssh_key_fingerprint(const char *signing_key)\n> > >               die_errno(_(\"failed to get the ssh fingerprint for key '%s'\"),\n> > >                         signing_key);\n> > >\n> > > -     fingerprint = strbuf_split_max(&fingerprint_stdout, ' ', 3);\n> > > -     if (!fingerprint[1])\n> > > +     if (string_list_split(&split, fingerprint_stdout.buf, \" \", 2) != 3)\n> >\n> > The original splits the thing into upto 3 pieces, but only complains\n> > if the second piece is NULL (i.e. we said \"up to 3\", but there was\n> > not even one place to split, and the request to split_max gave the\n> > one thing as one piece).  IOW, the original code will happily accept\n> > if the finterprint_stdout is split only into two, not three.\n> >\n> > The updated code asks to split at at most two places (yes, it is a\n> > confusing API, but if you split at two places, you will end up with\n> > three pieces), and insists that the split results in three pieces.\n> >\n> > So the rewrite tightens the error condition.\n> >\n> > Was the original code too loose in detecting an error, and does this\n> > patch tightens the condition \"while at it\"?  Or was the original\n> > code correct to expect that there are legitimate cases where the\n> > payload in finterprint_stdout only contains two pieces, and it was\n> > the right thing to do to accept when fingerprint[1] is not NULL but\n> > fingerprint[2] is NULL?\n> >\n> > This is a genuine question.  I haven't studied the code path to\n> > reach this point in the code flow, I don't know what the data in\n> > fingerprint_stdout is supposed to look like, so I do not know the\n> > answer to the question (in other words, it cannot be an oblique way\n> > to point out that the updated code is wrong or anything like that).\n>\n> Yeah, I think the problem is that the commit message should explain\n> how this whole issue is dealt with. Currently it talks about passing\n> \"2\" to string_list_split() instead of passing \"3\" to\n> strbuf_split_max(), but it says nothing about dealing with the return\n> value from string_list_split().\n\nYes, I will add this to the commit message too.\n>\n> It seems to me that fingerprint_stdout should contain something like:\n>\n> 4096 SHA256:PelI2esT2xZlv20wJJyYOkQsli5RMK79oJ2VxqYb2PA\n> christian.couder@gmail.com (RSA)\n>\n> and it looks like the 'key_size fingerprint_hash:fingerprint comment\n> (key_type)' format is the standard `ssh-keygen -l` output.\n>\n> So I think it's safe to say that we should expect\n> `string_list_split(..., 2)` to return 3. That should work even if\n> there is no comment in the key file.\n\nYes, in this case fingerprint_stdout will contain something like:\n\n2048 SHA256:BoQYpZX7zNZFOc9BSprkHoIgPhMTT1D5d5tqbwXZ1hA\nno comment (RSA)\n>\n> But on the other hand, I think it's also acceptable to say that we\n> just want to keep the same behavior as the original code and check its\n> return value with `> 1` since we only need the second element\n> resulting from the split.\n\nOkay this is noted.\nThank you.\n\nBello\n"},{"id":"529059","messageId":"CAD=f0L8-+Ut4eTbd4Mh-vPJV7KwNxcVUY1-xCrB+m8mT0xkkow@mail.gmail.com","threadId":"64329","inReplyTo":"xmqqms5q4v0r.fsf@gitster.g","subject":"Re: [Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split()","fromName":"Bello Olamide","fromEmail":"belkid98@gmail.com","sentAt":"2025-10-17T10:53:52Z","receivedAt":"2025-10-17T10:53:54Z","isPatch":true,"sender":{"key":"belkid98@gmail.com","avatar":"https://avatars.githubusercontent.com/u/73387291?v=4"},"body":"On Thu, 16 Oct 2025 at 18:27, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Olamide Caleb Bello <belkid98@gmail.com> writes:\n>\n> > @@ -821,7 +822,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)\n> >       struct child_process ssh_keygen = CHILD_PROCESS_INIT;\n> >       int ret = -1;\n> >       struct strbuf fingerprint_stdout = STRBUF_INIT;\n> > -     struct strbuf **fingerprint;\n> > +     struct string_list split = STRING_LIST_INIT_DUP;\n> >       char *fingerprint_ret;\n> >       const char *literal_key = NULL;\n> >\n> > @@ -845,13 +846,12 @@ static char *get_ssh_key_fingerprint(const char *signing_key)\n> >               die_errno(_(\"failed to get the ssh fingerprint for key '%s'\"),\n> >                         signing_key);\n> >\n> > -     fingerprint = strbuf_split_max(&fingerprint_stdout, ' ', 3);\n> > -     if (!fingerprint[1])\n> > +     if (string_list_split(&split, fingerprint_stdout.buf, \" \", 2) != 3)\n>\n> The original splits the thing into upto 3 pieces, but only complains\n> if the second piece is NULL (i.e. we said \"up to 3\", but there was\n> not even one place to split, and the request to split_max gave the\n> one thing as one piece).  IOW, the original code will happily accept\n> if the finterprint_stdout is split only into two, not three.\n>\n> The updated code asks to split at at most two places (yes, it is a\n> confusing API, but if you split at two places, you will end up with\n> three pieces), and insists that the split results in three pieces.\n>\n> So the rewrite tightens the error condition.\n>\n> Was the original code too loose in detecting an error, and does this\n> patch tightens the condition \"while at it\"?  Or was the original\n> code correct to expect that there are legitimate cases where the\n> payload in finterprint_stdout only contains two pieces, and it was\n> the right thing to do to accept when fingerprint[1] is not NULL but\n> fingerprint[2] is NULL?\n>\n> This is a genuine question.  I haven't studied the code path to\n> reach this point in the code flow, I don't know what the data in\n> fingerprint_stdout is supposed to look like, so I do not know the\n> answer to the question (in other words, it cannot be an oblique way\n> to point out that the updated code is wrong or anything like that).\n>\n> >               die_errno(_(\"failed to get the ssh fingerprint for key '%s'\"),\n> >                         signing_key);\n> >\n> > -     fingerprint_ret = strbuf_detach(fingerprint[1], NULL);\n> > -     strbuf_list_free(fingerprint);\n> > +     fingerprint_ret = xstrdup(split.items[1].string);\n> > +     string_list_clear(&split, 0);\n> >       strbuf_release(&fingerprint_stdout);\n>\n> Since this code releases fingerprint_stdout before leaving, and\n> returns a fresh copy of a split piece, it may make more sense to use\n> string_list_split_in_place(), which does not have to allocate extra\n> strings while it does its work, unlike string_list_split().\n\nThank you Junio for the review.\n\nI will take a look at string_list_split_in_place().\n\nBello\n"},{"id":"529088","messageId":"xmqqv7kdz7pn.fsf@gitster.g","threadId":"64329","inReplyTo":"CAP8UFD1FMwrBxbZ6Ck8JwBPBOAhB039US6pHSTusb6qzW=crpg@mail.gmail.com","subject":"Re: [Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-17T18:47:16Z","receivedAt":"2025-10-17T18:47:19Z","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> It seems to me that fingerprint_stdout should contain something like:\n>\n> 4096 SHA256:PelI2esT2xZlv20wJJyYOkQsli5RMK79oJ2VxqYb2PA\n> christian.couder@gmail.com (RSA)\n>\n> and it looks like the 'key_size fingerprint_hash:fingerprint comment\n> (key_type)' format is the standard `ssh-keygen -l` output.\n>\n> So I think it's safe to say that we should expect\n> `string_list_split(..., 2)` to return 3. That should work even if\n> there is no comment in the key file.\n\nI do not know why you or anybody can think it is safe from what you\nobserved above, though.  Who or which page of what manual told us\nthat the key owner's identity part cannot be missing?\n\n> But on the other hand, I think it's also acceptable to say that we\n> just want to keep the same behavior as the original code and check its\n> return value with `> 1` since we only need the second element\n> resulting from the split.\n\nYes, as a code clean-up topic, I think it is the *only* acceptable\nthing to do.  Finding the source to justify tightening of the rule\nto validate data to insist we have to see 3 pieces can be left to a\nseparate topic once this series settles.\n\nThanks.\n"}]}