Hi, On Mon, Jan 8, 2024 at 6:38 PM <mohitmarathe@xxxxxxxxx> wrote: > > Hello, > > 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/. > I want to work on this task (if it is not taken up already) as a microproject for GSoC. Thanks for your interest in Git and the GSoC! > Approach: > 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/. > 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: > > > static inline int strtol_i2(char const *s, int base, int *result, char **endp) > > { > > long ul; > > char *dummy = NULL; > > > > if (!endp) > > endp = &dummy; > > errno = 0; > > ul = strtol(s, &endp, base); > > if (errno || > > /* > > * if we are told to parse to the end of the string by > > * passing NULL to endp, it is an error to have any > > * remaining character after the digits. > > */ > > (dummy && *dummy) || > > endp == s || (int) ul != ul) > > return -1; > > *result = ul; > > return 0; > > } > > > 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? In https://public-inbox.org/git/xmqqjzpjsbjl.fsf@gitster.g/ Junio says: "Some places use atoi() immediately followed by strspn() to skip over digits, which means they are parsing an integer and want to continue reading after the integer, which is incompatible with what strtol_i() wants to do. They need either a separate helper or an updated strtol_i() that optionally allows you to parse the prefix and report where the integer ended, e.g., something like:" and then he suggests the above helper. So it seems that the two instances you found look like good places where Junio says the new helper could be useful. Now if you want to continue further on this, I think you would need to take a closer look at those two instances to see if replacing atoi() there with the new helper would improve something there or not. If you find it would improve something, be sure to explain what would be improved in the commit message. > 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? I think that adding a new helper in one .c file and its declaration in the corresponding .h file, and then using it in another .c file would change around 3 files and would be Ok size wise for a microproject. Thanks.
