# [PATCH 1/3] remote-testsvn.c: Avoid the getline() GNU extension function 6 messages from 2012-08-25 to 2012-08-30. Participants: Ramsay Jones, Joachim Schmitz, Erik Faye-Lund, Junio C Hamano. Thread: https://gitlist.dev/t/31356 ## Ramsay Jones, 2012-08-25 17:13 Subject: [PATCH 1/3] remote-testsvn.c: Avoid the getline() GNU extension function Message-ID: <503907B3.9040101@ramsay1.demon.co.uk> URL: https://gitlist.dev/e/503907B3.9040101%40ramsay1.demon.co.uk ``` The getline() function is a GNU extension (you need to define _GNU_SOURCE before including stdio.h) and is, therefore, not portable. In particular, getline() is not available on MinGW. In order to support non-GNU systems, we replace the call to getline() with (almost) equivalent code using strbuf_getline(). Note that, unlike getline(), strbuf_getline() removes the newline terminator from the returned string. This difference in semantics does not matter at this call-site. Also, we note that the original code was leaking the memory allocated to 'line' by getline(). Signed-off-by: Ramsay Jones --- Hi Florian, Could you please squash this into commit 0320cef0 ("remote-svn: add marks-file regeneration", 22-08-2012). ATB, Ramsay Jones remote-testsvn.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/remote-testsvn.c b/remote-testsvn.c index 09dc304..d0b81d5 100644 --- a/remote-testsvn.c +++ b/remote-testsvn.c @@ -121,9 +121,8 @@ static void regenerate_marks(void) static void check_or_regenerate_marks(int latestrev) { FILE *marksfile; - char *line = NULL; - size_t linelen = 0; struct strbuf sb = STRBUF_INIT; + struct strbuf line = STRBUF_INIT; int found = 0; if (latestrev < 1) @@ -139,8 +138,8 @@ static void check_or_regenerate_marks(int latestrev) { fclose(marksfile); } else { strbuf_addf(&sb, ":%d ", latestrev); - while (getline(&line, &linelen, marksfile) != -1) { - if (!prefixcmp(line, sb.buf)) { + while (strbuf_getline(&line, marksfile, '\n') != EOF) { + if (!prefixcmp(line.buf, sb.buf)) { found++; break; } @@ -151,6 +150,7 @@ static void check_or_regenerate_marks(int latestrev) { } free_notes(NULL); strbuf_release(&sb); + strbuf_release(&line); } static int cmd_import(const char *line) -- 1.7.12 ``` ## Joachim Schmitz, 2012-08-25 19:03 Subject: Re: [PATCH 1/3] remote-testsvn.c: Avoid the getline() GNU extension function Message-ID: URL: https://gitlist.dev/e/k1b7ij%24p8v%241%40ger.gmane.org In-Reply-To: <503907B3.9040101@ramsay1.demon.co.uk> ``` Ramsay Jones wrote: > The getline() function is a GNU extension (you need to define > _GNU_SOURCE before including stdio.h) and is, therefore, not > portable. In particular, getline() is not available on MinGW. > > In order to support non-GNU systems, we replace the call to > getline() with (almost) equivalent code using strbuf_getline(). > Note that, unlike getline(), strbuf_getline() removes the > newline terminator from the returned string. This difference > in semantics does not matter at this call-site. Also, we note > that the original code was leaking the memory allocated to > 'line' by getline(). > > Signed-off-by: Ramsay Jones ramsay@ramsay1.demon.co.uk Tested-by: Joachim Schmitz jojo@schmitz-digital.de > --- > > Hi Florian, > > Could you please squash this into commit 0320cef0 ("remote-svn: add > marks-file regeneration", 22-08-2012). > > ATB, > Ramsay Jones > > remote-testsvn.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/remote-testsvn.c b/remote-testsvn.c > index 09dc304..d0b81d5 100644 > --- a/remote-testsvn.c > +++ b/remote-testsvn.c > @@ -121,9 +121,8 @@ static void regenerate_marks(void) > > static void check_or_regenerate_marks(int latestrev) { > FILE *marksfile; > - char *line = NULL; > - size_t linelen = 0; > struct strbuf sb = STRBUF_INIT; > + struct strbuf line = STRBUF_INIT; > int found = 0; > > if (latestrev < 1) > @@ -139,8 +138,8 @@ static void check_or_regenerate_marks(int > latestrev) { fclose(marksfile); > } else { > strbuf_addf(&sb, ":%d ", latestrev); > - while (getline(&line, &linelen, marksfile) != -1) { > - if (!prefixcmp(line, sb.buf)) { > + while (strbuf_getline(&line, marksfile, '\n') != EOF) { > + if (!prefixcmp(line.buf, sb.buf)) { > found++; > break; > } > @@ -151,6 +150,7 @@ static void check_or_regenerate_marks(int > latestrev) { } > free_notes(NULL); > strbuf_release(&sb); > + strbuf_release(&line); > } > > static int cmd_import(const char *line) I'd like to second this request, having the same problem on HP NonStop and this patch fixes it for me too. Bye, Jojo ``` ## Erik Faye-Lund, 2012-08-25 19:20 Subject: Re: [PATCH 1/3] remote-testsvn.c: Avoid the getline() GNU extension function Message-ID: URL: https://gitlist.dev/e/CABPQNSZfUO-vjVmBU2%2BCMbxTXMj_HT%3DcTFfVbnehUjqOL-vkxw%40mail.gmail.com In-Reply-To: <503907B3.9040101@ramsay1.demon.co.uk> ``` On Sat, Aug 25, 2012 at 7:13 PM, Ramsay Jones wrote: > > The getline() function is a GNU extension (you need to define > _GNU_SOURCE before including stdio.h) and is, therefore, not > portable. In particular, getline() is not available on MinGW. Actually, getline is a POSIX-2008 feature, so it's not (simply) a GNU extension: http://pubs.opengroup.org/onlinepubs/9699919799/functions/getline.html ``` ## Junio C Hamano, 2012-08-26 17:31 Subject: Re: [PATCH 1/3] remote-testsvn.c: Avoid the getline() GNU extension function Message-ID: <7v3939wofb.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v3939wofb.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Erik Faye-Lund writes: > On Sat, Aug 25, 2012 at 7:13 PM, Ramsay Jones > wrote: >> >> The getline() function is a GNU extension (you need to define >> _GNU_SOURCE before including stdio.h) and is, therefore, not >> portable. In particular, getline() is not available on MinGW. > > Actually, getline is a POSIX-2008 feature, so it's not (simply) a GNU extension: > > http://pubs.opengroup.org/onlinepubs/9699919799/functions/getline.html True, thanks for pointing it out. Justify the change with something like this instead, perhaps? The getline() function, even though is in POSIX.1-2008, is not available on some platforms. Use strbuf_getline() for portability. By the way, the remainder of the proposed log message talks about the difference between getline() that keeps the terminating LF vs strbuf_getline() that drops it. Would strbuf_getwholeline() be a better alternative? ``` ## Ramsay Jones, 2012-08-30 17:19 Subject: Re: [PATCH 1/3] remote-testsvn.c: Avoid the getline() GNU extension function Message-ID: <503FA0B4.6010401@ramsay1.demon.co.uk> URL: https://gitlist.dev/e/503FA0B4.6010401%40ramsay1.demon.co.uk In-Reply-To: ``` Erik Faye-Lund wrote: > On Sat, Aug 25, 2012 at 7:13 PM, Ramsay Jones > wrote: >> >> The getline() function is a GNU extension (you need to define >> _GNU_SOURCE before including stdio.h) and is, therefore, not >> portable. In particular, getline() is not available on MinGW. > > Actually, getline is a POSIX-2008 feature, so it's not (simply) a GNU extension: > > http://pubs.opengroup.org/onlinepubs/9699919799/functions/getline.html Thanks for the reference. I hadn't noticed this being included into POSIX. Well, it's only been four years ... :-D Thanks again. ATB, Ramsay Jones ``` ## Ramsay Jones, 2012-08-30 17:25 Subject: Re: [PATCH 1/3] remote-testsvn.c: Avoid the getline() GNU extension function Message-ID: <503FA216.8070704@ramsay1.demon.co.uk> URL: https://gitlist.dev/e/503FA216.8070704%40ramsay1.demon.co.uk In-Reply-To: <7v3939wofb.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: > Erik Faye-Lund writes: > >> On Sat, Aug 25, 2012 at 7:13 PM, Ramsay Jones >> wrote: >>> >>> The getline() function is a GNU extension (you need to define >>> _GNU_SOURCE before including stdio.h) and is, therefore, not >>> portable. In particular, getline() is not available on MinGW. >> >> Actually, getline is a POSIX-2008 feature, so it's not (simply) a GNU extension: >> >> http://pubs.opengroup.org/onlinepubs/9699919799/functions/getline.html > > True, thanks for pointing it out. Justify the change with something > like this instead, perhaps? > > The getline() function, even though is in POSIX.1-2008, is > not available on some platforms. Use strbuf_getline() for > portability. > > By the way, the remainder of the proposed log message talks about > the difference between getline() that keeps the terminating LF vs > strbuf_getline() that drops it. Would strbuf_getwholeline() be a > better alternative? I don't think it matters; I was just making sure Florian was aware of the difference (it will not appear in the commit message anyway), including the memory leak in the original code. ATB, Ramsay Jones ```