{"thread":{"id":"60702","subject":"[GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject","startedAt":"2024-01-08T17:34:35Z","lastAt":"2024-01-17T05:29:07Z","messageCount":8,"participants":["mohitmarathe@proton.me","Christian Couder","Mohit Marathe","rsbecker@nexbridge.com","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"486399","messageId":"OqD4xQ_p-jcftCbAw0ovvrBztfiuoMGcTonCc0i6x7Ziy-hx3uA-Hoz4-3tfRI39KMj-V5OZIGgOe66b1eyX3YrKZNThMYjjMkn6g4-Ww8c=@proton.me","threadId":"60702","inReplyTo":null,"subject":"[GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject","fromName":"","fromEmail":"mohitmarathe@proton.me","sentAt":"2024-01-08T17:34:01Z","receivedAt":"2024-01-08T17:34:35Z","isPatch":false,"sender":{"key":"mohitmarathe@proton.me","avatar":"https://gravatar.com/avatar/3bf18f96e6d36cef960077143b9880e8b22468d960da18bf0b34c4e87d4df7e9?d=mp&s=160"},"body":"Hello,\n\nI'm Mohit, an undergrad from India, and I want to start contributing to the Git project. I have already built Git from source and finished `git psuh` tutorial. And I must say the \"Hacking Git\" documentation is great (very detailed and maybe exhaustive) and easy to follow. So I also read the topic on \"microprojects\", and while searching for one, I came across this message: https://public-inbox.org/git/xmqqjzpjsbjl.fsf@gitster.g/.\nI want to work on this task (if it is not taken up already) as a microproject for GSoC.\n\nApproach:\nFrom what I understood, the idea is to *not* allow non-integer characters in the arguments by printing an error message. So we have to replace `atoi` with `strtol_i`, like it is done in this patch: https://public-inbox.org/git/xmqq5y181fx0.fsf_-_@gitster.g/.\nThere are some places where we want to continue to parse after the end of the integer (where `strspn` is used as mentioned by Junio), and based on Junio's suggestion, a new helper can be created like this:\n\n> static inline int strtol_i2(char const *s, int base, int *result, char **endp)\n> {\n> \tlong ul;\n>         char *dummy = NULL;\n> \n>         if (!endp)\n> \t\tendp = &dummy;\n> \terrno = 0;\n> \tul = strtol(s, &endp, base);\n> \tif (errno ||\n> \t    /*\n> \t     * if we are told to parse to the end of the string by\n> \t     * passing NULL to endp, it is an error to have any\n> \t     * remaining character after the digits.\n> \t     */\n>             (dummy && *dummy) ||\n> \t    endp == s || (int) ul != ul)\n> \t\treturn -1;\n> \t*result = ul;\n> \treturn 0;\n> }\n\n\nSo I searched for all the occurrences of `atoi(` (as suggested by Junio), and I found only two instances (both in `builtin/patch-id.c`) where it is followed by `strspn`. Is it safe to assume that this is the only place where we cannot directly replace `atoi` with `strtol_i`, or should I keep digging?\n\nAlso, this seems like a large change due to the number of files involved, while the documentation about the microproject emphasizes keeping the change small. Does it mean a small change per commit or a small change per Pull Request?\n\nThanks!\n"},{"id":"486423","messageId":"CAP8UFD1d7FSa=mUzzUA5e3eSEcCVfaymxWewo5GjdDBi4GyE-g@mail.gmail.com","threadId":"60702","inReplyTo":"OqD4xQ_p-jcftCbAw0ovvrBztfiuoMGcTonCc0i6x7Ziy-hx3uA-Hoz4-3tfRI39KMj-V5OZIGgOe66b1eyX3YrKZNThMYjjMkn6g4-Ww8c=@proton.me","subject":"Re: [GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-01-09T09:56:34Z","receivedAt":"2024-01-09T09:56:48Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Mon, Jan 8, 2024 at 6:38 PM <mohitmarathe@proton.me> wrote:\n>\n> Hello,\n>\n> I'm Mohit, an undergrad from India, and I want to start contributing to the Git project. I have already built Git from source and finished `git psuh` tutorial. And I must say the \"Hacking Git\" documentation is great (very detailed and maybe exhaustive) and easy to follow. So I also read the topic on \"microprojects\", and while searching for one, I came across this message: https://public-inbox.org/git/xmqqjzpjsbjl.fsf@gitster.g/.\n> I want to work on this task (if it is not taken up already) as a microproject for GSoC.\n\nThanks for your interest in Git and the GSoC!\n\n> Approach:\n> From what I understood, the idea is to *not* allow non-integer characters in the arguments by printing an error message. So we have to replace `atoi` with `strtol_i`, like it is done in this patch: https://public-inbox.org/git/xmqq5y181fx0.fsf_-_@gitster.g/.\n> There are some places where we want to continue to parse after the end of the integer (where `strspn` is used as mentioned by Junio), and based on Junio's suggestion, a new helper can be created like this:\n>\n> > static inline int strtol_i2(char const *s, int base, int *result, char **endp)\n> > {\n> >       long ul;\n> >         char *dummy = NULL;\n> >\n> >         if (!endp)\n> >               endp = &dummy;\n> >       errno = 0;\n> >       ul = strtol(s, &endp, base);\n> >       if (errno ||\n> >           /*\n> >            * if we are told to parse to the end of the string by\n> >            * passing NULL to endp, it is an error to have any\n> >            * remaining character after the digits.\n> >            */\n> >             (dummy && *dummy) ||\n> >           endp == s || (int) ul != ul)\n> >               return -1;\n> >       *result = ul;\n> >       return 0;\n> > }\n>\n>\n> So I searched for all the occurrences of `atoi(` (as suggested by Junio), and I found only two instances (both in `builtin/patch-id.c`) where it is followed by `strspn`. Is it safe to assume that this is the only place where we cannot directly replace `atoi` with `strtol_i`, or should I keep digging?\n\nIn https://public-inbox.org/git/xmqqjzpjsbjl.fsf@gitster.g/ Junio says:\n\n\"Some places use atoi() immediately followed by strspn() to skip over\ndigits, which means they are parsing an integer and want to continue\nreading after the integer, which is incompatible with what\nstrtol_i() wants to do.  They need either a separate helper or an\nupdated strtol_i() that optionally allows you to parse the prefix\nand report where the integer ended, e.g., something like:\"\n\nand then he suggests the above helper.\n\nSo it seems that the two instances you found look like good places\nwhere Junio says the new helper could be useful.\n\nNow if you want to continue further on this, I think you would need to\ntake a closer look at those two instances to see if replacing atoi()\nthere with the new helper would improve something there or not. If you\nfind it would improve something, be sure to explain what would be\nimproved in the commit message.\n\n> Also, this seems like a large change due to the number of files involved, while the documentation about the microproject emphasizes keeping the change small. Does it mean a small change per commit or a small change per Pull Request?\n\nI think that adding a new helper in one .c file and its declaration in\nthe corresponding .h file, and then using it in another .c file would\nchange around 3 files and would be Ok size wise for a microproject.\n\nThanks.\n"},{"id":"486544","messageId":"F6ejgAfr2IMRNR3Tq0CDTHeT9xMWzJ9ley8M_fnSX97ayRNRp_CEgA62WdtOooi9bha1WJPGB53ptJYQFII2lCbIflwgNvbIaefw7nK8w7M=@proton.me","threadId":"60702","inReplyTo":"CAP8UFD1d7FSa=mUzzUA5e3eSEcCVfaymxWewo5GjdDBi4GyE-g@mail.gmail.com","subject":"Re: [GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject","fromName":"Mohit Marathe","fromEmail":"mohitmarathe@proton.me","sentAt":"2024-01-10T17:38:07Z","receivedAt":"2024-01-10T17:38:19Z","isPatch":false,"sender":{"key":"mohitmarathe@proton.me","avatar":"https://gravatar.com/avatar/3bf18f96e6d36cef960077143b9880e8b22468d960da18bf0b34c4e87d4df7e9?d=mp&s=160"},"body":">In https://public-inbox.org/git/xmqqjzpjsbjl.fsf@gitster.g/ Junio says:\n>\n>\"Some places use atoi() immediately followed by strspn() to skip over\n>digits, which means they are parsing an integer and want to continue\n>reading after the integer, which is incompatible with what\n>strtol_i() wants to do.  They need either a separate helper or an\n>updated strtol_i() that optionally allows you to parse the prefix\n>and report where the integer ended, e.g., something like:\"\n>\n>and then he suggests the above helper.\n>\n>So it seems that the two instances you found look like good places\n>where Junio says the new helper could be useful.\n>\n>Now if you want to continue further on this, I think you would need to\n>take a closer look at those two instances to see if replacing atoi()\n>there with the new helper would improve something there or not. If you\n>find it would improve something, be sure to explain what would be\n>improved in the commit message.\n\nI took a closer look at `builtin/patch-id.c` and it seems replacing \n`atoi()` (which is used to parse numbers in the hunk header) wouldn't\nimprove anything, unless I'm missing something.\n\nSo then I tried finding other places where `atoi()` can be replaced\nbut I find it difficult to find any reason that would justify the change.\nSo far I've only looked at few of the MANY occurrences of `atoi()`.\nAs far as I understand, the only advantage of `strtol_i()`\nover `atoi()` is better error handling. And most of places I've seen either\nalready takes care of that or does not need that at all.\n\nThanks!\n"},{"id":"486552","messageId":"004601da43ee$2b6cd0c0$82467240$@nexbridge.com","threadId":"60702","inReplyTo":"F6ejgAfr2IMRNR3Tq0CDTHeT9xMWzJ9ley8M_fnSX97ayRNRp_CEgA62WdtOooi9bha1WJPGB53ptJYQFII2lCbIflwgNvbIaefw7nK8w7M=@proton.me","subject":"RE: [GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-01-10T17:55:11Z","receivedAt":"2024-01-10T18:04:07Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Wednesday, January 10, 2024 12:38 PM, Mohit Marathe wrote:\n>>In https://public-inbox.org/git/xmqqjzpjsbjl.fsf@gitster.g/ Junio says:\n>>\n>>\"Some places use atoi() immediately followed by strspn() to skip over\n>>digits, which means they are parsing an integer and want to continue\n>>reading after the integer, which is incompatible with what\n>>strtol_i() wants to do.  They need either a separate helper or an\n>>updated strtol_i() that optionally allows you to parse the prefix and\n>>report where the integer ended, e.g., something like:\"\n>>\n>>and then he suggests the above helper.\n>>\n>>So it seems that the two instances you found look like good places\n>>where Junio says the new helper could be useful.\n>>\n>>Now if you want to continue further on this, I think you would need to\n>>take a closer look at those two instances to see if replacing atoi()\n>>there with the new helper would improve something there or not. If you\n>>find it would improve something, be sure to explain what would be\n>>improved in the commit message.\n>\n>I took a closer look at `builtin/patch-id.c` and it seems replacing `atoi()` (which is\n>used to parse numbers in the hunk header) wouldn't improve anything, unless I'm\n>missing something.\n>\n>So then I tried finding other places where `atoi()` can be replaced but I find it\n>difficult to find any reason that would justify the change.\n>So far I've only looked at few of the MANY occurrences of `atoi()`.\n>As far as I understand, the only advantage of `strtol_i()` over `atoi()` is better error\n>handling. And most of places I've seen either already takes care of that or does not\n>need that at all.\n\nI am not sure this is a good idea. The error detection inside strtol_i() reports a -1 if the supplied text value is invalid. This does not differentiate between an invalid value and a valid \"-1\" supplied. Replacing all instances of atoi() with strtol_i() will likely cause breakages as the error semantics are different between the two.\n--Randall\n\n"},{"id":"486554","messageId":"xmqqa5pdav04.fsf@gitster.g","threadId":"60702","inReplyTo":"F6ejgAfr2IMRNR3Tq0CDTHeT9xMWzJ9ley8M_fnSX97ayRNRp_CEgA62WdtOooi9bha1WJPGB53ptJYQFII2lCbIflwgNvbIaefw7nK8w7M=@proton.me","subject":"Re: [GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-10T18:44:27Z","receivedAt":"2024-01-10T18:44:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mohit Marathe <mohitmarathe@proton.me> writes:\n\n> I took a closer look at `builtin/patch-id.c` and it seems replacing \n> `atoi()` (which is used to parse numbers in the hunk header) wouldn't\n> improve anything, unless I'm missing something.\n\nYou can of course notice an invalid patch that places non-digit to\nthe hunk header and error out with such a change.  If we are reading\noutput from Git that we are invoking, hopefully we will not see such\nan invalid patch, but the command is designed to read arbitrary\ninput like a patch e-mail you received over the network, so I do not\nthink it is fair to say there is no merit to such a change, even\nthough I agree that it may not matter all that much.\n\nA corrupt patch may be getting a nonsense patch-ID with the current\ncode and hopefully is not matching other patches that are not\ncorrupt, but with such a change, a corrupt patch may not be getting\nany patch-ID and a loop that computes patch-ID for many files and\ntry to match them up might need to be rewritten to take the new\nfailure case into account.\n"},{"id":"486555","messageId":"xmqq5y01auw2.fsf@gitster.g","threadId":"60702","inReplyTo":"004601da43ee$2b6cd0c0$82467240$@nexbridge.com","subject":"Re: [GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-10T18:46:53Z","receivedAt":"2024-01-10T18:47:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"<rsbecker@nexbridge.com> writes:\n\n> I am not sure this is a good idea. The error detection inside strtol_i() reports a -1 if the supplied text value is invalid. This does not differentiate between an invalid value and a valid \"-1\" supplied. Replacing all instances of atoi() with strtol_i() will likely cause breakages as the error semantics are different between the two.\n\natoi() and strtol_i() have totally different function signatures, so\nit won't be a straight replacement.  The report of \"-1\" you mention\nis only about strtol_i() reporting \"have we successfully read out a\nnumber [yes/no]?\".  The value we parsed goes to a separate location,\nso there is no risk of confusion as long as the caller knows what it\nis doing.\n\nThanks.\n\n"},{"id":"486708","messageId":"N4StHAGzqHVGHJxcPqm6LviErWK3vILDzmedr0WVcNyjprnEh4PgVUXk3LufOjO90wMZKqAeujSdzKa2OJx_uTvEzK0bSk_QdFDwggZAjnk=@proton.me","threadId":"60702","inReplyTo":"xmqqa5pdav04.fsf@gitster.g","subject":"Re: [GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject","fromName":"Mohit Marathe","fromEmail":"mohitmarathe@proton.me","sentAt":"2024-01-12T13:04:11Z","receivedAt":"2024-01-12T13:04:31Z","isPatch":false,"sender":{"key":"mohitmarathe@proton.me","avatar":"https://gravatar.com/avatar/3bf18f96e6d36cef960077143b9880e8b22468d960da18bf0b34c4e87d4df7e9?d=mp&s=160"},"body":" > > I took a closer look at `builtin/patch-id.c` and it seems replacing\n> > `atoi()` (which is used to parse numbers in the hunk header) wouldn't\n> > improve anything, unless I'm missing something.\n> \n> \n> You can of course notice an invalid patch that places non-digit to\n> the hunk header and error out with such a change. If we are reading\n> output from Git that we are invoking, hopefully we will not see such\n> an invalid patch, but the command is designed to read arbitrary\n> input like a patch e-mail you received over the network, so I do not\n> think it is fair to say there is no merit to such a change, even\n> though I agree that it may not matter all that much.\n> \n> A corrupt patch may be getting a nonsense patch-ID with the current\n> code and hopefully is not matching other patches that are not\n> corrupt, but with such a change, a corrupt patch may not be getting\n> any patch-ID and a loop that computes patch-ID for many files and\n> try to match them up might need to be rewritten to take the new\n> failure case into account.\n\nOh, I didn't know corrupt patches were a thing. I will start working \non the patch for this.\n"},{"id":"486903","messageId":"Ec-xbd5ISDrcz8ybM8oJkVEhX7hgEmENoeek1n4K5M9buOfQ1SwOgGqAZRyMnYGYY1AFG1ppUKwXi3V63dndTzAjnJj7Cs0cVrtqwROg6oc=@proton.me","threadId":"60702","inReplyTo":"xmqqa5pdav04.fsf@gitster.g","subject":"Re: [GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject","fromName":"Mohit Marathe","fromEmail":"mohitmarathe@proton.me","sentAt":"2024-01-17T05:28:36Z","receivedAt":"2024-01-17T05:29:07Z","isPatch":false,"sender":{"key":"mohitmarathe@proton.me","avatar":"https://gravatar.com/avatar/3bf18f96e6d36cef960077143b9880e8b22468d960da18bf0b34c4e87d4df7e9?d=mp&s=160"},"body":"> A corrupt patch may be getting a nonsense patch-ID with the current\n> code and hopefully is not matching other patches that are not\n> corrupt, but with such a change, a corrupt patch may not be getting\n> any patch-ID and a loop that computes patch-ID for many files and\n> try to match them up might need to be rewritten to take the new\n> failure case into account.\n\nAre you talking about the loop in `get_one_patchid()`? And how should\nthe failure case be handled? Should it give a warning or an error?\n"}]}