{"thread":{"id":"60650","subject":"[PATCH] sideband.c: replace int with size_t for clarity","startedAt":"2023-12-22T17:01:12Z","lastAt":"2024-01-02T22:31:19Z","messageCount":12,"participants":["Chandra Pratap via GitGitGadget","Torsten Bögershausen","Chandra Pratap","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"485980","messageId":"pull.1625.git.1703264469238.gitgitgadget@gmail.com","threadId":"60650","inReplyTo":null,"subject":"[PATCH] sideband.c: replace int with size_t for clarity","fromName":"Chandra Pratap via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-12-22T17:01:09Z","receivedAt":"2023-12-22T17:01:12Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"From: Chandra Pratap \n\nReplace int with size_t for clarity and remove the\n'NEEDSWORK' tag associated with it.\n\nSigned-off-by: Chandra Pratap \n---\n sideband.c: replace int with size_t for clarity\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1625%2FChand-ra%2Fdusra-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1625/Chand-ra/dusra-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1625\n\n sideband.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/sideband.c b/sideband.c\nindex 6cbfd391c47..1599e408d1b 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -69,9 +69,8 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref\n * of the line. This should be called for a single line only, which is\n * passed as the first N characters of the SRC array.\n *\n- * NEEDSWORK: use \"size_t n\" instead for clarity.\n */\n-static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n+static void maybe_colorize_sideband(struct strbuf *dest, const char *src, size_t n)\n {\n \tint i;\n \n\nbase-commit: 1a87c842ece327d03d08096395969aca5e0a6996\n-- \ngitgitgadget\n"},{"id":"485981","messageId":"20231222175747.GA2880@tb-raspi4","threadId":"60650","inReplyTo":"pull.1625.git.1703264469238.gitgitgadget@gmail.com","subject":"Re: [PATCH] sideband.c: replace int with size_t for clarity","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2023-12-22T17:57:47Z","receivedAt":"2023-12-22T17:57:50Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Fri, Dec 22, 2023 at 05:01:09PM +0000, Chandra Pratap via GitGitGadget wrote:\n> From: Chandra Pratap \n>\n> Replace int with size_t for clarity and remove the\n> 'NEEDSWORK' tag associated with it.\n>\n> Signed-off-by: Chandra Pratap \n> ---\n> sideband.c: replace int with size_t for clarity\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1625%2FChand-ra%2Fdusra-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1625/Chand-ra/dusra-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1625\n>\n> sideband.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n>\n> diff --git a/sideband.c b/sideband.c\n> index 6cbfd391c47..1599e408d1b 100644\n> --- a/sideband.c\n> +++ b/sideband.c\n> @@ -69,9 +69,8 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref\n> * of the line. This should be called for a single line only, which is\n> * passed as the first N characters of the SRC array.\n> *\n> - * NEEDSWORK: use \"size_t n\" instead for clarity.\n> */\n> -static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> +static void maybe_colorize_sideband(struct strbuf *dest, const char *src, size_t n)\n> {\n> \tint i;\n>\n\nThanks for working on this.\nThere is, however, more potential for improvements.\nFirst of all: the \"int i\" from above.\nFurther down, we read\n\tfor (i = 0; i < ARRAY_SIZE(keywords); i++) {\n\nHowever, a size of an array can never be negative, so that\nan unsigned data type is a better choice than a signed.\nAnd, arrays can have more elements than an int can address,\nat least in theory.\nFor a reader it makes more sense, to replace\nint i;\nwith\nsize_t i;\n\n\nAnd further down, there is another place for improvments:\n\n\t\tint len = strlen(p->keyword);\n\t\tif (n < len)\n\t\t\tcontinue;\n\nEven here, a strlen is never negative. And a size_t is the choice for len,\nsince all \"modern\" implementations declare strlen() to return size_t\n\nDo you think that you can have a look at these changes ?\n\nI will be happy to do a review, and possibly other people as well.\n"},{"id":"485982","messageId":"CA+J6zkQ=+bCsi7URtpTfgawTrmVVALvMi3kdnMi=mUjB_Yxxwg@mail.gmail.com","threadId":"60650","inReplyTo":"20231222175747.GA2880@tb-raspi4","subject":"Re: [PATCH] sideband.c: replace int with size_t for clarity","fromName":"Chandra Pratap","fromEmail":"chandrapratap3519@gmail.com","sentAt":"2023-12-22T18:27:25Z","receivedAt":"2023-12-22T18:27:37Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"Thanks for the feedback, Torsten. I was working on improving the rest of\nsideband.c as you suggested when I encountered this snippet on line 82:\n\nwhile (0 < n && isspace(*src)) {\nstrbuf_addch(dest, *src);\nsrc++;\nn--;\n}\n\nHere, we are decreasing the value of an unsigned type to potentially below\n0 which may lead to overflow and result in some nasty bug. In this case,\nis it wise to continue with replacing 'int n' with 'size_t n' as the\nNEEDSWORK tag suggests or should we improve upon the rest of the file\nand revert 'size_t n' to 'int n' ?\n\nOn Fri, 22 Dec 2023 at 23:27, Torsten Bögershausen wrote:\n>\n> On Fri, Dec 22, 2023 at 05:01:09PM +0000, Chandra Pratap via GitGitGadget wrote:\n> > From: Chandra Pratap \n> >\n> > Replace int with size_t for clarity and remove the\n> > 'NEEDSWORK' tag associated with it.\n> >\n> > Signed-off-by: Chandra Pratap \n> > ---\n> > sideband.c: replace int with size_t for clarity\n> >\n> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1625%2FChand-ra%2Fdusra-v1\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1625/Chand-ra/dusra-v1\n> > Pull-Request: https://github.com/gitgitgadget/git/pull/1625\n> >\n> > sideband.c | 3 +--\n> > 1 file changed, 1 insertion(+), 2 deletions(-)\n> >\n> > diff --git a/sideband.c b/sideband.c\n> > index 6cbfd391c47..1599e408d1b 100644\n> > --- a/sideband.c\n> > +++ b/sideband.c\n> > @@ -69,9 +69,8 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref\n> > * of the line. This should be called for a single line only, which is\n> > * passed as the first N characters of the SRC array.\n> > *\n> > - * NEEDSWORK: use \"size_t n\" instead for clarity.\n> > */\n> > -static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> > +static void maybe_colorize_sideband(struct strbuf *dest, const char *src, size_t n)\n> > {\n> > int i;\n> >\n>\n> Thanks for working on this.\n> There is, however, more potential for improvements.\n> First of all: the \"int i\" from above.\n> Further down, we read\n> for (i = 0; i < ARRAY_SIZE(keywords); i++) {\n>\n> However, a size of an array can never be negative, so that\n> an unsigned data type is a better choice than a signed.\n> And, arrays can have more elements than an int can address,\n> at least in theory.\n> For a reader it makes more sense, to replace\n> int i;\n> with\n> size_t i;\n>\n>\n> And further down, there is another place for improvments:\n>\n> int len = strlen(p->keyword);\n> if (n < len)\n> continue;\n>\n> Even here, a strlen is never negative. And a size_t is the choice for len,\n> since all \"modern\" implementations declare strlen() to return size_t\n>\n> Do you think that you can have a look at these changes ?\n>\n> I will be happy to do a review, and possibly other people as well.\n"},{"id":"485983","messageId":"20231222183958.GA3355@tb-raspi4","threadId":"60650","inReplyTo":"CA+J6zkQ=+bCsi7URtpTfgawTrmVVALvMi3kdnMi=mUjB_Yxxwg@mail.gmail.com","subject":"Re: [PATCH] sideband.c: replace int with size_t for clarity","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2023-12-22T18:39:58Z","receivedAt":"2023-12-22T18:40:01Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\n(PLease, avoid top-posting on this list)\nOn Fri, Dec 22, 2023 at 11:57:25PM +0530, Chandra Pratap wrote:\n> Thanks for the feedback, Torsten. I was working on improving the rest of\n> sideband.c as you suggested when I encountered this snippet on line 82:\n>\n> while (0 < n && isspace(*src)) {\n> strbuf_addch(dest, *src);\n> src++;\n> n--;\n> }\n>\n> Here, we are decreasing the value of an unsigned type to potentially below\n> 0 which may lead to overflow and result in some nasty bug. In this case,\n> is it wise to continue with replacing 'int n' with 'size_t n' as the\n> NEEDSWORK tag suggests or should we improve upon the rest of the file\n> and revert 'size_t n' to 'int n' ?\n\nYes, that could be a solution.\nBut.\nIn general, we are are striving to use size_t for all objects that live in\nmemory, and that is a long term thing.\nCareful review is needed, and that is what you just did.\n\nIf we look at this code again:\nwhile (0 < n && isspace(*src)) {\n strbuf_addch(dest, *src);\n src++;\n n--;\n}\n\nWhen n is signed, it makes sense to use \"0 < n\".\nHowever, if I think about it, it should work for unsigned as well,\nwouldn't it ?\nWe can leave it as is, or replace it by\n\nwhile (n && isspace(*src)) {\n strbuf_addch(dest, *src);\n src++;\n n--;\n}\n\n\n\n\n>\n> On Fri, 22 Dec 2023 at 23:27, Torsten Bögershausen wrote:\n> >\n> > On Fri, Dec 22, 2023 at 05:01:09PM +0000, Chandra Pratap via GitGitGadget wrote:\n> > > From: Chandra Pratap \n> > >\n> > > Replace int with size_t for clarity and remove the\n> > > 'NEEDSWORK' tag associated with it.\n> > >\n> > > Signed-off-by: Chandra Pratap \n> > > ---\n> > > sideband.c: replace int with size_t for clarity\n> > >\n> > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1625%2FChand-ra%2Fdusra-v1\n> > > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1625/Chand-ra/dusra-v1\n> > > Pull-Request: https://github.com/gitgitgadget/git/pull/1625\n> > >\n> > > sideband.c | 3 +--\n> > > 1 file changed, 1 insertion(+), 2 deletions(-)\n> > >\n> > > diff --git a/sideband.c b/sideband.c\n> > > index 6cbfd391c47..1599e408d1b 100644\n> > > --- a/sideband.c\n> > > +++ b/sideband.c\n> > > @@ -69,9 +69,8 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref\n> > > * of the line. This should be called for a single line only, which is\n> > > * passed as the first N characters of the SRC array.\n> > > *\n> > > - * NEEDSWORK: use \"size_t n\" instead for clarity.\n> > > */\n> > > -static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> > > +static void maybe_colorize_sideband(struct strbuf *dest, const char *src, size_t n)\n> > > {\n> > > int i;\n> > >\n> >\n> > Thanks for working on this.\n> > There is, however, more potential for improvements.\n> > First of all: the \"int i\" from above.\n> > Further down, we read\n> > for (i = 0; i < ARRAY_SIZE(keywords); i++) {\n> >\n> > However, a size of an array can never be negative, so that\n> > an unsigned data type is a better choice than a signed.\n> > And, arrays can have more elements than an int can address,\n> > at least in theory.\n> > For a reader it makes more sense, to replace\n> > int i;\n> > with\n> > size_t i;\n> >\n> >\n> > And further down, there is another place for improvments:\n> >\n> > int len = strlen(p->keyword);\n> > if (n < len)\n> > continue;\n> >\n> > Even here, a strlen is never negative. And a size_t is the choice for len,\n> > since all \"modern\" implementations declare strlen() to return size_t\n> >\n> > Do you think that you can have a look at these changes ?\n> >\n> > I will be happy to do a review, and possibly other people as well.\n>\n"},{"id":"485984","messageId":"xmqqedfejc32.fsf@gitster.g","threadId":"60650","inReplyTo":"20231222175747.GA2880@tb-raspi4","subject":"Re: [PATCH] sideband.c: replace int with size_t for clarity","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-22T19:01:37Z","receivedAt":"2023-12-22T19:01:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\nJust this part.\n\n> Further down, we read\n> \tfor (i = 0; i < ARRAY_SIZE(keywords); i++) {\n>\n> However, a size of an array can never be negative, so that\n> an unsigned data type is a better choice than a signed.\n> And, arrays can have more elements than an int can address,\n> at least in theory.\n> For a reader it makes more sense, to replace\n> int i;\n> with\n> size_t i;\n\nIt is a very good discipline to use size_t to index into an array\nwhose size is externally controled (e.g., we slurp what the end user\nor the server on the other end of the connection gave us into a\npiece of memory we allocate) to avoid integer overflows as \"int\" is\noften narrower than \"size_t\". But this particular one is a Meh; the\nkeywords[] is a small hardcoded array whose size and contents are\ntotally under our control.\n"},{"id":"485990","messageId":"pull.1625.v2.git.1703351016486.gitgitgadget@gmail.com","threadId":"60650","inReplyTo":"pull.1625.git.1703264469238.gitgitgadget@gmail.com","subject":"[PATCH v2] sideband.c: replace int with size_t for clarity","fromName":"Chandra Pratap via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-12-23T17:03:36Z","receivedAt":"2023-12-23T17:03:40Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"From: Chandra Pratap \n\nReplace int with size_t for non-negative values to improve\nclarity and remove the 'NEEDSWORK' tag associated with it.wq\n\nSigned-off-by: Chandra Pratap \n---\n sideband.c: replace int with size_t for clarity\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1625%2FChand-ra%2Fdusra-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1625/Chand-ra/dusra-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1625\n\nRange-diff vs v1:\n\n 1: be67e1bca38 ! 1: fdacc69ae3b sideband.c: replace int with size_t for clarity\n @@ Metadata\n ## Commit message ##\n sideband.c: replace int with size_t for clarity\n \n - Replace int with size_t for clarity and remove the\n - 'NEEDSWORK' tag associated with it.\n + Replace int with size_t for non-negative values to improve\n + clarity and remove the 'NEEDSWORK' tag associated with it.wq\n \n Signed-off-by: Chandra Pratap \n \n @@ sideband.c: void list_config_color_sideband_slots(struct string_list *list, cons\n {\n \tint i;\n \n +@@ sideband.c: static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n + \n + \tfor (i = 0; i < ARRAY_SIZE(keywords); i++) {\n + \t\tstruct keyword_entry *p = keywords + i;\n +-\t\tint len = strlen(p->keyword);\n ++\t\tsize_t len = strlen(p->keyword);\n + \n + \t\tif (n < len)\n + \t\t\tcontinue;\n\n\n sideband.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/sideband.c b/sideband.c\nindex 6cbfd391c47..7c3ec0ece29 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -69,9 +69,8 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref\n * of the line. This should be called for a single line only, which is\n * passed as the first N characters of the SRC array.\n *\n- * NEEDSWORK: use \"size_t n\" instead for clarity.\n */\n-static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n+static void maybe_colorize_sideband(struct strbuf *dest, const char *src, size_t n)\n {\n \tint i;\n \n@@ -88,7 +87,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n \n \tfor (i = 0; i < ARRAY_SIZE(keywords); i++) {\n \t\tstruct keyword_entry *p = keywords + i;\n-\t\tint len = strlen(p->keyword);\n+\t\tsize_t len = strlen(p->keyword);\n \n \t\tif (n < len)\n \t\t\tcontinue;\n\nbase-commit: 1a87c842ece327d03d08096395969aca5e0a6996\n-- \ngitgitgadget\n"},{"id":"486027","messageId":"xmqqo7ecj37x.fsf@gitster.g","threadId":"60650","inReplyTo":"pull.1625.git.1703264469238.gitgitgadget@gmail.com","subject":"Re: [PATCH] sideband.c: replace int with size_t for clarity","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-26T17:14:26Z","receivedAt":"2023-12-26T17:14:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chandra Pratap via GitGitGadget\" writes:\n\n> -static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> +static void maybe_colorize_sideband(struct strbuf *dest, const char *src, size_t n)\n\nChanging the type of the paramter alone might be a good start, but\ndoes not really help anybody, as (1) the callers are not taught to\nhandle wider integral types for the values they pass and (2) the\nfunction uses \"int len\" it computes internally to compare with \"n\".\n\nThere are three callers in demultiplex_sideband(), one of whose\nparamters is \"int len\" and is passed to this function in one of\nthese calls. Among the other two, one uses \"int linelen\" derived\nfrom scanning the piece of memory read from sideband via strpbrk(),\nand the other uses strlen(b) which is the length of the \"remainder\"\nof the same buffer after the loop that processes one line at a time\nusing the said strpbrk() is done with the complete lines in the\nearly part.\n\nThe buffer involved in all of the above stores bytes taken from a\npacket received via the pkt-line interface, which is capable of\ntransferring only 64kB at a time.\n\nI _think_ the most productive use of our time is to replace the\nNEEDSWORK with a comment saying why it is fine to use \"int\" here to\navoid tempting the next developer to come up with this patch\nagain---they will waste their time to do so without thinking it\nthrough if we leave the (incomplete) NEEDSWORK comment, I am afraid.\n\n"},{"id":"486053","messageId":"pull.1625.v3.git.1703672407895.gitgitgadget@gmail.com","threadId":"60650","inReplyTo":"pull.1625.v2.git.1703351016486.gitgitgadget@gmail.com","subject":"[PATCH v3] sideband.c: remove redundant 'NEEDSWORK' tag","fromName":"Chandra Pratap via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-12-27T10:20:07Z","receivedAt":"2023-12-27T10:20:11Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"From: Chandra Pratap \n\nSigned-off-by: Chandra Pratap \n---\n sideband.c: replace int with size_t for clarity\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1625%2FChand-ra%2Fdusra-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1625/Chand-ra/dusra-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1625\n\nRange-diff vs v2:\n\n 1: fdacc69ae3b ! 1: 273415aa6a4 sideband.c: replace int with size_t for clarity\n @@ Metadata\n Author: Chandra Pratap \n \n ## Commit message ##\n - sideband.c: replace int with size_t for clarity\n -\n - Replace int with size_t for non-negative values to improve\n - clarity and remove the 'NEEDSWORK' tag associated with it.wq\n + sideband.c: remove redundant 'NEEDSWORK' tag\n \n Signed-off-by: Chandra Pratap \n \n @@ sideband.c: void list_config_color_sideband_slots(struct string_list *list, cons\n * passed as the first N characters of the SRC array.\n *\n - * NEEDSWORK: use \"size_t n\" instead for clarity.\n ++ * It is fine to use \"int n\" here instead of \"size_t n\" as all calls to this\n ++ * function pass an 'int' parameter.\n */\n --static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n -+static void maybe_colorize_sideband(struct strbuf *dest, const char *src, size_t n)\n + static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n {\n - \tint i;\n - \n -@@ sideband.c: static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n - \n - \tfor (i = 0; i < ARRAY_SIZE(keywords); i++) {\n - \t\tstruct keyword_entry *p = keywords + i;\n --\t\tint len = strlen(p->keyword);\n -+\t\tsize_t len = strlen(p->keyword);\n - \n - \t\tif (n < len)\n - \t\t\tcontinue;\n\n\n sideband.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sideband.c b/sideband.c\nindex 6cbfd391c47..a89201d52ac 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -69,7 +69,8 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref\n * of the line. This should be called for a single line only, which is\n * passed as the first N characters of the SRC array.\n *\n- * NEEDSWORK: use \"size_t n\" instead for clarity.\n+ * It is fine to use \"int n\" here instead of \"size_t n\" as all calls to this\n+ * function pass an 'int' parameter.\n */\n static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n {\n\nbase-commit: 1a87c842ece327d03d08096395969aca5e0a6996\n-- \ngitgitgadget\n"},{"id":"486062","messageId":"xmqqmstvl1z8.fsf@gitster.g","threadId":"60650","inReplyTo":"pull.1625.v3.git.1703672407895.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] sideband.c: remove redundant 'NEEDSWORK' tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-27T22:22:51Z","receivedAt":"2023-12-27T22:22:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chandra Pratap via GitGitGadget\" writes:\n\n> - * NEEDSWORK: use \"size_t n\" instead for clarity.\n> ++ * It is fine to use \"int n\" here instead of \"size_t n\" as all calls to this\n> ++ * function pass an 'int' parameter.\n\nThis does not sound like a sufficient justification, though.\n\nWe should also explain why \"int\" is good enough for these callers.\nOtherwise, using size_t throughout the callchain would become\nanother viable solution.\n"},{"id":"486088","messageId":"pull.1625.v4.git.1703750460527.gitgitgadget@gmail.com","threadId":"60650","inReplyTo":"pull.1625.v3.git.1703672407895.gitgitgadget@gmail.com","subject":"[PATCH v4] sideband.c: remove redundant 'NEEDSWORK' tag","fromName":"Chandra Pratap via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-12-28T08:01:00Z","receivedAt":"2023-12-28T08:01:04Z","isPatch":true,"sender":{"key":"chandrapratap3519@gmail.com","avatar":null},"body":"From: Chandra Pratap \n\nSigned-off-by: Chandra Pratap \n---\n sideband.c: replace int with size_t for clarity\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1625%2FChand-ra%2Fdusra-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1625/Chand-ra/dusra-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1625\n\nRange-diff vs v3:\n\n 1: 273415aa6a4 ! 1: 8c003256e5b sideband.c: remove redundant 'NEEDSWORK' tag\n @@ sideband.c: void list_config_color_sideband_slots(struct string_list *list, cons\n *\n - * NEEDSWORK: use \"size_t n\" instead for clarity.\n + * It is fine to use \"int n\" here instead of \"size_t n\" as all calls to this\n -+ * function pass an 'int' parameter.\n ++ * function pass an 'int' parameter. Additionally, the buffer involved in\n ++ * storing these 'int' values takes input from a packet via the pkt-line\n ++ * interface, which is capable of transferring only 64kB at a time.\n */\n static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n {\n\n\n sideband.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/sideband.c b/sideband.c\nindex 6cbfd391c47..266a67342be 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -69,7 +69,10 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref\n * of the line. This should be called for a single line only, which is\n * passed as the first N characters of the SRC array.\n *\n- * NEEDSWORK: use \"size_t n\" instead for clarity.\n+ * It is fine to use \"int n\" here instead of \"size_t n\" as all calls to this\n+ * function pass an 'int' parameter. Additionally, the buffer involved in\n+ * storing these 'int' values takes input from a packet via the pkt-line\n+ * interface, which is capable of transferring only 64kB at a time.\n */\n static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n {\n\nbase-commit: 1a87c842ece327d03d08096395969aca5e0a6996\n-- \ngitgitgadget\n"},{"id":"486143","messageId":"xmqq8r5egj85.fsf@gitster.g","threadId":"60650","inReplyTo":"pull.1625.v4.git.1703750460527.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] sideband.c: remove redundant 'NEEDSWORK' tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-28T20:33:46Z","receivedAt":"2023-12-28T20:33:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chandra Pratap via GitGitGadget\" writes:\n\n> Subject: Re: [PATCH v4] sideband.c: remove redundant 'NEEDSWORK' tag\n\nThe reason for removal is not that it was redundant and we said the\nsame thing elsewhere. Rather, what it claimed to be necessary has\nturned to be unwanted. So something like\n\n Subject: sideband.c: update stale NEEDSWORK comment\n\n If we really wanted to change the type of the parameter to this\n function to \"size_t\", we should also update its callers to hold\n the values they use to compute the parameter also in \"size_t\".\n\n But in this callchain, \"int\" is wide enough. Avoid tempting\n future developers into wasting their time on using \"size_t\"\n around this function.\n\nor along that line would be more appropriate, perhaps?\n\nThanks.\n\n> From: Chandra Pratap \n>\n> Signed-off-by: Chandra Pratap \n> ---\n> sideband.c: replace int with size_t for clarity\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1625%2FChand-ra%2Fdusra-v4\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1625/Chand-ra/dusra-v4\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1625\n>\n> Range-diff vs v3:\n>\n> 1: 273415aa6a4 ! 1: 8c003256e5b sideband.c: remove redundant 'NEEDSWORK' tag\n> @@ sideband.c: void list_config_color_sideband_slots(struct string_list *list, cons\n> *\n> - * NEEDSWORK: use \"size_t n\" instead for clarity.\n> + * It is fine to use \"int n\" here instead of \"size_t n\" as all calls to this\n> -+ * function pass an 'int' parameter.\n> ++ * function pass an 'int' parameter. Additionally, the buffer involved in\n> ++ * storing these 'int' values takes input from a packet via the pkt-line\n> ++ * interface, which is capable of transferring only 64kB at a time.\n> */\n> static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> {\n>\n>\n> sideband.c | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/sideband.c b/sideband.c\n> index 6cbfd391c47..266a67342be 100644\n> --- a/sideband.c\n> +++ b/sideband.c\n> @@ -69,7 +69,10 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref\n> * of the line. This should be called for a single line only, which is\n> * passed as the first N characters of the SRC array.\n> *\n> - * NEEDSWORK: use \"size_t n\" instead for clarity.\n> + * It is fine to use \"int n\" here instead of \"size_t n\" as all calls to this\n> + * function pass an 'int' parameter. Additionally, the buffer involved in\n> + * storing these 'int' values takes input from a packet via the pkt-line\n> + * interface, which is capable of transferring only 64kB at a time.\n> */\n> static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)\n> {\n>\n> base-commit: 1a87c842ece327d03d08096395969aca5e0a6996\n"},{"id":"486225","messageId":"ZZSOtiZSPSthcDgp@nand.local","threadId":"60650","inReplyTo":"xmqqedfejc32.fsf@gitster.g","subject":"Re: [PATCH] sideband.c: replace int with size_t for clarity","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-02T22:31:18Z","receivedAt":"2024-01-02T22:31:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Dec 22, 2023 at 11:01:37AM -0800, Junio C Hamano wrote:\n> Torsten Bögershausen writes:\n>\n> Just this part.\n>\n> > Further down, we read\n> > \tfor (i = 0; i < ARRAY_SIZE(keywords); i++) {\n> >\n> > However, a size of an array can never be negative, so that\n> > an unsigned data type is a better choice than a signed.\n> > And, arrays can have more elements than an int can address,\n> > at least in theory.\n> > For a reader it makes more sense, to replace\n> > int i;\n> > with\n> > size_t i;\n>\n> It is a very good discipline to use size_t to index into an array\n> whose size is externally controled (e.g., we slurp what the end user\n> or the server on the other end of the connection gave us into a\n> piece of memory we allocate) to avoid integer overflows as \"int\" is\n> often narrower than \"size_t\". But this particular one is a Meh; the\n> keywords[] is a small hardcoded array whose size and contents are\n> totally under our control.\n\nI certainly agree in theory, though I've always erred on the side of\nalways using size_t for indexing into arrays, even if they're small. It\nremoves a potential pitfall if you are working with an\nexternally-controlled array and happen to forget to use size_t.\n\nBut if there is an existing index variable with type \"int\", and we can\neasily validate that it's small, I probably wouldn't bother changing it\nif I was editing nearby code.\n\nThanks,\nTaylor\n"}]}