[PATCH v2 0/6] vcs-svn: housekeeping

[PATCH v2 0/6] vcs-svn: housekeeping

[David Barr]

I've begun to upstream the changes made in the git fork
of svn-dump-fast-export in vcs-svn back to the original
project. This series contains the fixups I needed to
compile without errors or warnings and pass static
analysis.

This second attempt addresses Jonathan's comments.
I've tidied the upstream history in preparation to
merge this series back if it is accepted.

--
David Barr

[PATCH v2 1/6] vcs-svn: fix clang-analyzer error

[David Barr]

vcs-svn/svndiff.c:298:3: warning: Assigned value is garbage or undefined
                off_t pre_off = pre_off; /* stupid GCC... */
                ^               ~~~~~~~

Signed-off-by: David Barr <davidbarr@google.com>
---
 vcs-svn/svndiff.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vcs-svn/svndiff.c b/vcs-svn/svndiff.c
index 1647c1a..57d647d 100644
--- a/vcs-svn/svndiff.c
+++ b/vcs-svn/svndiff.c
@@ -295,7 +295,7 @@ int svndiff0_apply(struct line_buffer *delta, off_t delta_len,
 	if (read_magic(delta, &delta_len))
 		return -1;
 	while (delta_len) {	/* For each window: */
-		off_t pre_off = pre_off; /* stupid GCC... */
+		off_t pre_off = 0; /* stupid GCC and clang-analyzer... */
 		size_t pre_len;
 
 		if (read_offset(delta, &pre_off, &delta_len) ||
-- 
1.7.10.2

[PATCH v2 2/6] vcs-svn: simplify cleanup in apply_one_window()

[David Barr]

As a side-effect, fix clang-analyzer warning:

vcs-svn/svndiff.c:278:3: warning: expression result unused [-Wunused-value]
                error("invalid delta: incorrect postimage length");
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This warning caused by an insanely concise error() upstream.

Signed-off-by: David Barr <davidbarr@google.com>
---
 vcs-svn/svndiff.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/vcs-svn/svndiff.c b/vcs-svn/svndiff.c
index 57d647d..11a0e38 100644
--- a/vcs-svn/svndiff.c
+++ b/vcs-svn/svndiff.c
@@ -258,6 +258,7 @@ static int apply_window_in_core(struct window *ctx)
 static int apply_one_window(struct line_buffer *delta, off_t *delta_len,
 			    struct sliding_view *preimage, FILE *out)
 {
+	int rv = -1;
 	struct window ctx = WINDOW_INIT(preimage);
 	size_t out_len;
 	size_t instructions_len;
@@ -275,16 +276,15 @@ static int apply_one_window(struct line_buffer *delta, off_t *delta_len,
 	if (apply_window_in_core(&ctx))
 		goto error_out;
 	if (ctx.out.len != out_len) {
-		error("invalid delta: incorrect postimage length");
+		rv = error("invalid delta: incorrect postimage length");
 		goto error_out;
 	}
 	if (write_strbuf(&ctx.out, out))
 		goto error_out;
-	window_release(&ctx);
-	return 0;
+	rv = 0;
 error_out:
 	window_release(&ctx);
-	return -1;
+	return rv;
 }
 
 int svndiff0_apply(struct line_buffer *delta, off_t delta_len,
-- 
1.7.10.2

[PATCH v2 3/6] vcs-svn: prefer constcmp to prefixcmp

[David Barr]

Comparisons in svndump.c are always guarded by length.
As a bonus, elimate dependency on prefixcmp for upstream.

Signed-off-by: David Barr <davidbarr@google.com>
---
 vcs-svn/svndump.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 0899790..8d0ae9c 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -361,7 +361,7 @@ void svndump_read(const char *url)
 			reset_rev_ctx(atoi(val));
 			break;
 		case sizeof("Node-path"):
-			if (prefixcmp(t, "Node-"))
+			if (constcmp(t, "Node-"))
 				continue;
 			if (!constcmp(t + strlen("Node-"), "path")) {
 				if (active_ctx == NODE_CTX)
-- 
1.7.10.2

Re: [PATCH v2 3/6] vcs-svn: prefer constcmp to prefixcmp

[Junio C Hamano]

David Barr <davidbarr@google.com> writes:

> Comparisons in svndump.c are always guarded by length.
> As a bonus, elimate dependency on prefixcmp for upstream.
>
> Signed-off-by: David Barr <davidbarr@google.com>

It feels suboptimal, from cross-project maintenance point of view,
that "do not use prefixcmp() in the source in this directory" has to
be an unwritten rule.  Is there something we can do better to avoid
having to apply a patch like this in the future?

>  vcs-svn/svndump.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
> index 0899790..8d0ae9c 100644
> --- a/vcs-svn/svndump.c
> +++ b/vcs-svn/svndump.c
> @@ -361,7 +361,7 @@ void svndump_read(const char *url)
>  			reset_rev_ctx(atoi(val));
>  			break;
>  		case sizeof("Node-path"):
> -			if (prefixcmp(t, "Node-"))
> +			if (constcmp(t, "Node-"))
>  				continue;
>  			if (!constcmp(t + strlen("Node-"), "path")) {
>  				if (active_ctx == NODE_CTX)

Re: [PATCH v2 3/6] vcs-svn: prefer constcmp to prefixcmp

[David Michael Barr]

On Sat, Jun 2, 2012 at 2:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
> David Barr <davidbarr@google.com> writes:
>
>> Comparisons in svndump.c are always guarded by length.
>> As a bonus, elimate dependency on prefixcmp for upstream.
>>
>> Signed-off-by: David Barr <davidbarr@google.com>
>
> It feels suboptimal, from cross-project maintenance point of view,
> that "do not use prefixcmp() in the source in this directory" has to
> be an unwritten rule.  Is there something we can do better to avoid
> having to apply a patch like this in the future?

I should note that constcmp() is local to vcs-svn/svndump.c.
In this particular case, using prefixcmp() was a departure from the
style of the surrounding code.

Should prefixcmp() be referenced outside svndump_read() and
handle_property(), I'll handle the dependency upstream.

>>  vcs-svn/svndump.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
>> index 0899790..8d0ae9c 100644
>> --- a/vcs-svn/svndump.c
>> +++ b/vcs-svn/svndump.c
>> @@ -361,7 +361,7 @@ void svndump_read(const char *url)
>>                       reset_rev_ctx(atoi(val));
>>                       break;
>>               case sizeof("Node-path"):
>> -                     if (prefixcmp(t, "Node-"))
>> +                     if (constcmp(t, "Node-"))
>>                               continue;
>>                       if (!constcmp(t + strlen("Node-"), "path")) {
>>                               if (active_ctx == NODE_CTX)

--
David Barr

[PATCH v2 4/6] vcs-svn: prefer strstr over memmem

[David Barr]

The common pattern is to use strstr to match a fixed needle.
As a bonus, elimate dependency on memmem for upstream.

Signed-off-by: David Barr <davidbarr@google.com>
---
 vcs-svn/fast_export.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index b823b85..cda37dd 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -163,7 +163,7 @@ static int parse_cat_response_line(const char *header, off_t *len)
 
 	if (ends_with(header, headerlen, " missing"))
 		return error("cat-blob reports missing blob: %s", header);
-	type = memmem(header, headerlen, " blob ", strlen(" blob "));
+	type = strstr(header, " blob ");
 	if (!type)
 		return error("cat-blob header has wrong object type: %s", header);
 	n = strtoumax(type + strlen(" blob "), (char **) &end, 10);
-- 
1.7.10.2

[PATCH v2 5/6] vcs-svn: fix signedness warnings

[David Barr]

Stop mixing off_t and size_t.

Signed-off-by: David Barr <davidbarr@google.com>
---
 vcs-svn/fast_export.c    |    4 ++--
 vcs-svn/sliding_window.c |    2 +-
 vcs-svn/svndiff.c        |    2 +-
 vcs-svn/svndump.c        |    4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index cda37dd..6ded98b 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -259,7 +259,7 @@ static int parse_ls_response(const char *response, uint32_t *mode,
 	}
 
 	/* Mode. */
-	if (response_end - response < strlen("100644") ||
+	if (response_end - response < (signed) strlen("100644") ||
 	    response[strlen("100644")] != ' ')
 		die("invalid ls response: missing mode: %s", response);
 	*mode = 0;
@@ -272,7 +272,7 @@ static int parse_ls_response(const char *response, uint32_t *mode,
 	}
 
 	/* ' blob ' or ' tree ' */
-	if (response_end - response < strlen(" blob ") ||
+	if (response_end - response < (signed) strlen(" blob ") ||
 	    (response[1] != 'b' && response[1] != 't'))
 		die("unexpected ls response: not a tree or blob: %s", response);
 	response += strlen(" blob ");
diff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c
index ec2707c..f11d490 100644
--- a/vcs-svn/sliding_window.c
+++ b/vcs-svn/sliding_window.c
@@ -54,7 +54,7 @@ int move_window(struct sliding_view *view, off_t off, size_t width)
 		return -1;
 	if (off < view->off || off + width < view->off + view->width)
 		return error("invalid delta: window slides left");
-	if (view->max_off >= 0 && view->max_off < off + width)
+	if (view->max_off >= 0 && view->max_off < off + (off_t) width)
 		return error("delta preimage ends early");
 
 	file_offset = view->off + view->buf.len;
diff --git a/vcs-svn/svndiff.c b/vcs-svn/svndiff.c
index 11a0e38..f061fae 100644
--- a/vcs-svn/svndiff.c
+++ b/vcs-svn/svndiff.c
@@ -78,7 +78,7 @@ static int read_chunk(struct line_buffer *delta, off_t *delta_len,
 		      struct strbuf *buf, size_t len)
 {
 	strbuf_reset(buf);
-	if (len > *delta_len ||
+	if ((off_t) len > *delta_len ||
 	    buffer_read_binary(delta, buf, len) != len)
 		return error_short_read(delta);
 	*delta_len -= buf->len;
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 8d0ae9c..6d1e3cd 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -40,8 +40,8 @@
 static struct line_buffer input = LINE_BUFFER_INIT;
 
 static struct {
-	uint32_t action, propLength, srcRev, type;
-	off_t text_length;
+	uint32_t action, srcRev, type;
+	off_t propLength, text_length;
 	struct strbuf src, dst;
 	uint32_t text_delta, prop_delta;
 } node_ctx;
-- 
1.7.10.2

[PATCH v2 6/6] vcs-svn: drop no-op reset methods

[David Barr]

Since v1.7.5~42^2~6 (vcs-svn: remove buffer_read_string)
buffer_reset() does nothing thus fast_export_reset() also.

Signed-off-by: David Barr <davidbarr@google.com>
---
 vcs-svn/fast_export.c |    5 -----
 vcs-svn/fast_export.h |    1 -
 vcs-svn/line_buffer.c |    4 ----
 vcs-svn/line_buffer.h |    1 -
 vcs-svn/svndump.c     |    2 --
 5 files changed, 13 deletions(-)

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 6ded98b..1f04697 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -42,11 +42,6 @@ void fast_export_deinit(void)
 		die_errno("error closing fast-import feedback stream");
 }
 
-void fast_export_reset(void)
-{
-	buffer_reset(&report_buffer);
-}
-
 void fast_export_delete(const char *path)
 {
 	putchar('D');
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index aa629f5..8823aca 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -6,7 +6,6 @@ struct line_buffer;
 
 void fast_export_init(int fd);
 void fast_export_deinit(void);
-void fast_export_reset(void);
 
 void fast_export_delete(const char *path);
 void fast_export_modify(const char *path, uint32_t mode, const char *dataref);
diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c
index 01fcb84..57cc1ce 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -124,7 +124,3 @@ off_t buffer_skip_bytes(struct line_buffer *buf, off_t nbytes)
 	}
 	return done;
 }
-
-void buffer_reset(struct line_buffer *buf)
-{
-}
diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h
index 8901f21..ee23b4f 100644
--- a/vcs-svn/line_buffer.h
+++ b/vcs-svn/line_buffer.h
@@ -14,7 +14,6 @@ struct line_buffer {
 int buffer_init(struct line_buffer *buf, const char *filename);
 int buffer_fdinit(struct line_buffer *buf, int fd);
 int buffer_deinit(struct line_buffer *buf);
-void buffer_reset(struct line_buffer *buf);
 
 int buffer_tmpfile_init(struct line_buffer *buf);
 FILE *buffer_tmpfile_rewind(struct line_buffer *buf);	/* prepare to write. */
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 6d1e3cd..f1705f3 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -499,8 +499,6 @@ void svndump_deinit(void)
 
 void svndump_reset(void)
 {
-	fast_export_reset();
-	buffer_reset(&input);
 	strbuf_release(&dump_ctx.uuid);
 	strbuf_release(&dump_ctx.url);
 	strbuf_release(&rev_ctx.log);
-- 
1.7.10.2

Re: [PATCH v2 6/6] vcs-svn: drop no-op reset methods

[David Michael Barr]

On Fri, Jun 1, 2012 at 12:41 AM, David Barr <davidbarr@google.com> wrote:
> Since v1.7.5~42^2~6 (vcs-svn: remove buffer_read_string)
> buffer_reset() does nothing thus fast_export_reset() also.
>
> Signed-off-by: David Barr <davidbarr@google.com>
> ---
>  vcs-svn/fast_export.c |    5 -----
>  vcs-svn/fast_export.h |    1 -
>  vcs-svn/line_buffer.c |    4 ----
>  vcs-svn/line_buffer.h |    1 -
>  vcs-svn/svndump.c     |    2 --
>  5 files changed, 13 deletions(-)
>
> diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
> index 6ded98b..1f04697 100644
> --- a/vcs-svn/fast_export.c
> +++ b/vcs-svn/fast_export.c
> @@ -42,11 +42,6 @@ void fast_export_deinit(void)
>                die_errno("error closing fast-import feedback stream");
>  }
>
> -void fast_export_reset(void)
> -{
> -       buffer_reset(&report_buffer);
> -}
> -
>  void fast_export_delete(const char *path)
>  {
>        putchar('D');
> diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
> index aa629f5..8823aca 100644
> --- a/vcs-svn/fast_export.h
> +++ b/vcs-svn/fast_export.h
> @@ -6,7 +6,6 @@ struct line_buffer;
>
>  void fast_export_init(int fd);
>  void fast_export_deinit(void);
> -void fast_export_reset(void);
>
>  void fast_export_delete(const char *path);
>  void fast_export_modify(const char *path, uint32_t mode, const char *dataref);
> diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c
> index 01fcb84..57cc1ce 100644
> --- a/vcs-svn/line_buffer.c
> +++ b/vcs-svn/line_buffer.c
> @@ -124,7 +124,3 @@ off_t buffer_skip_bytes(struct line_buffer *buf, off_t nbytes)
>        }
>        return done;
>  }
> -
> -void buffer_reset(struct line_buffer *buf)
> -{
> -}
> diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h
> index 8901f21..ee23b4f 100644
> --- a/vcs-svn/line_buffer.h
> +++ b/vcs-svn/line_buffer.h
> @@ -14,7 +14,6 @@ struct line_buffer {
>  int buffer_init(struct line_buffer *buf, const char *filename);
>  int buffer_fdinit(struct line_buffer *buf, int fd);
>  int buffer_deinit(struct line_buffer *buf);
> -void buffer_reset(struct line_buffer *buf);
>
>  int buffer_tmpfile_init(struct line_buffer *buf);
>  FILE *buffer_tmpfile_rewind(struct line_buffer *buf);  /* prepare to write. */
> diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
> index 6d1e3cd..f1705f3 100644
> --- a/vcs-svn/svndump.c
> +++ b/vcs-svn/svndump.c
> @@ -499,8 +499,6 @@ void svndump_deinit(void)
>
>  void svndump_reset(void)
>  {
> -       fast_export_reset();
> -       buffer_reset(&input);
>        strbuf_release(&dump_ctx.uuid);
>        strbuf_release(&dump_ctx.url);
>        strbuf_release(&rev_ctx.log);
> --
> 1.7.10.2
>

It' a little embarrassing that I didn't catch this before submitting.
Please squash in this fix-up when queueing.

--
diff --git a/test-line-buffer.c b/test-line-buffer.c
index 7ec9b13..ef1d7ba 100644
--- a/test-line-buffer.c
+++ b/test-line-buffer.c
@@ -87,6 +87,5 @@ int main(int argc, char *argv[])
 		die("input error");
 	if (ferror(stdout))
 		die("output error");
-	buffer_reset(&stdin_buf);
 	return 0;
 }
diff --git a/test-svn-fe.c b/test-svn-fe.c
index 332a5f7..83633a2 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -31,9 +31,7 @@ static int apply_delta(int argc, char *argv[])
 		die_errno("cannot close preimage");
 	if (buffer_deinit(&delta))
 		die_errno("cannot close delta");
-	buffer_reset(&preimage);
 	strbuf_release(&preimage_view.buf);
-	buffer_reset(&delta);
 	return 0;
 }

--
David Barr