[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]

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

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