threads / patch / 31356

patch, 3 partsremote-testsvn.c: Avoid the getline() GNU extension function

Subject: [PATCH 1/3] remote-testsvn.c: Avoid the getline() GNU extension function

## tl;dr

6 messages between Aug 25, 2012 and Aug 30, 2012. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Ramsay Jones· Aug 25, 2012, 17:13 UTC · lore

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>
---
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(-)
Show changes to remote-testsvn.c +4 −4
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· Aug 25, 2012, 19:03 UTC · re: Ramsay Jones · lore

Re: [PATCH 1/3] remote-testsvn.c: Avoid the getline() GNU extension function

Ramsay Jones wrote:
Show 13 quoted lines
> 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
Show 47 quoted lines
> ---
>
> 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· Aug 25, 2012, 19:20 UTC · re: Ramsay Jones · lore

Re: [PATCH 1/3] remote-testsvn.c: Avoid the getline() GNU extension function

On Sat, Aug 25, 2012 at 7:13 PM, Ramsay Jones <ramsay@ramsay1.demon.co.uk> 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· Aug 26, 2012, 17:31 UTC · re: Erik Faye-Lund · lore

Re: [PATCH 1/3] remote-testsvn.c: Avoid the getline() GNU extension function

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 10 quoted lines
> On Sat, Aug 25, 2012 at 7:13 PM, Ramsay Jones
> <ramsay@ramsay1.demon.co.uk> 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· Aug 30, 2012, 17:25 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] remote-testsvn.c: Avoid the getline() GNU extension function

Junio C Hamano wrote:
Show 24 quoted lines
> Erik Faye-Lund <kusmabite@gmail.com> writes:
> 
>> On Sat, Aug 25, 2012 at 7:13 PM, Ramsay Jones
>> <ramsay@ramsay1.demon.co.uk> 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

Ramsay Jones· Aug 30, 2012, 17:19 UTC · re: Erik Faye-Lund · lore

Re: [PATCH 1/3] remote-testsvn.c: Avoid the getline() GNU extension function

Erik Faye-Lund wrote:
Show 10 quoted lines
> On Sat, Aug 25, 2012 at 7:13 PM, Ramsay Jones
> <ramsay@ramsay1.demon.co.uk> 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

← back to recent threads