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

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

[Ramsay Jones]

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(-)

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

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

[Joachim Schmitz]

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 

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

[Erik Faye-Lund]

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

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

[Junio C Hamano]

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?

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

[Ramsay Jones]

Erik Faye-Lund wrote:
> 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

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

[Ramsay Jones]

Junio C Hamano wrote:
> 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