Git Mailing List Archive mirror
 help / color / mirror / Atom feed

From: Christian Couder <christian.couder@gmail.com>
To: mohitmarathe@proton.me
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
	"gitster@pobox.com" <gitster@pobox.com>,
	 "britton.kerin@gmail.com" <britton.kerin@gmail.com>,
	"peff@peff.net" <peff@peff.net>
Subject: Re: [GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject
Date: Tue, 9 Jan 2024 10:56:34 +0100	[thread overview]
Message-ID: <CAP8UFD1d7FSa=mUzzUA5e3eSEcCVfaymxWewo5GjdDBi4GyE-g@mail.gmail.com> (raw)
In-Reply-To: <OqD4xQ_p-jcftCbAw0ovvrBztfiuoMGcTonCc0i6x7Ziy-hx3uA-Hoz4-3tfRI39KMj-V5OZIGgOe66b1eyX3YrKZNThMYjjMkn6g4-Ww8c=@proton.me>

Hi,

On Mon, Jan 8, 2024 at 6:38 PM <mohitmarathe@proton.me> 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.

next prev parent reply	other threads:[~2024-01-09  9:56 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-08 17:34 [GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject mohitmarathe
2024-01-09  9:56 ` Christian Couder [this message]
2024-01-10 17:38   ` Mohit Marathe
2024-01-10 17:55     ` rsbecker
2024-01-10 18:46       ` Junio C Hamano
2024-01-10 18:44     ` Junio C Hamano
2024-01-12 13:04       ` Mohit Marathe
2024-01-17  5:28       ` Mohit Marathe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAP8UFD1d7FSa=mUzzUA5e3eSEcCVfaymxWewo5GjdDBi4GyE-g@mail.gmail.com' \
    --to=christian.couder@gmail.com \
    --cc=britton.kerin@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=mohitmarathe@proton.me \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).