[PATCH 0/7] vcs-svn: housekeeping

[PATCH 0/7] 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.

--
David Barr

[PATCH 1/7] 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

[PATCH 2/7] 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 3/7] 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/line_buffer.c    |    3 +--
 vcs-svn/line_buffer.h    |    2 +-
 vcs-svn/sliding_window.c |    6 +++---
 vcs-svn/sliding_window.h |    4 ++--
 vcs-svn/svndiff.c        |   18 +++++++++---------
 vcs-svn/svndump.c        |    2 +-
 7 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index cda37dd..7396a91 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 < (off_t) 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 < (off_t) 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/line_buffer.c b/vcs-svn/line_buffer.c
index 01fcb84..b4104af 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -91,8 +91,7 @@ char *buffer_read_line(struct line_buffer *buf)
 	return buf->line_buffer;
 }
 
-size_t buffer_read_binary(struct line_buffer *buf,
-				struct strbuf *sb, size_t size)
+off_t buffer_read_binary(struct line_buffer *buf, struct strbuf *sb, off_t size)
 {
 	return strbuf_fread(sb, size, buf->infile);
 }
diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h
index 8901f21..bf9a053 100644
--- a/vcs-svn/line_buffer.h
+++ b/vcs-svn/line_buffer.h
@@ -23,7 +23,7 @@ long buffer_tmpfile_prepare_to_read(struct line_buffer *buf);
 int buffer_ferror(struct line_buffer *buf);
 char *buffer_read_line(struct line_buffer *buf);
 int buffer_read_char(struct line_buffer *buf);
-size_t buffer_read_binary(struct line_buffer *buf, struct strbuf *sb, size_t len);
+off_t buffer_read_binary(struct line_buffer *buf, struct strbuf *sb, off_t len);
 /* Returns number of bytes read (not necessarily written). */
 off_t buffer_copy_bytes(struct line_buffer *buf, off_t len);
 off_t buffer_skip_bytes(struct line_buffer *buf, off_t len);
diff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c
index ec2707c..e67e85a 100644
--- a/vcs-svn/sliding_window.c
+++ b/vcs-svn/sliding_window.c
@@ -43,11 +43,11 @@ static int check_offset_overflow(off_t offset, uintmax_t len)
 	return 0;
 }
 
-int move_window(struct sliding_view *view, off_t off, size_t width)
+int move_window(struct sliding_view *view, off_t off, off_t width)
 {
 	off_t file_offset;
 	assert(view);
-	assert(view->width <= view->buf.len);
+	assert(view->width <= (off_t)(view->buf.len));
 	assert(!check_offset_overflow(view->off, view->buf.len));
 
 	if (check_offset_overflow(off, width))
@@ -68,7 +68,7 @@ int move_window(struct sliding_view *view, off_t off, size_t width)
 		strbuf_setlen(&view->buf, 0);
 	}
 
-	if (view->buf.len > width)
+	if ((off_t)(view->buf.len) > width)
 		; /* Already read. */
 	else if (read_to_fill_or_whine(view->file, &view->buf, width))
 		return -1;
diff --git a/vcs-svn/sliding_window.h b/vcs-svn/sliding_window.h
index b43a825..d21473a 100644
--- a/vcs-svn/sliding_window.h
+++ b/vcs-svn/sliding_window.h
@@ -6,13 +6,13 @@
 struct sliding_view {
 	struct line_buffer *file;
 	off_t off;
-	size_t width;
+	off_t width;
 	off_t max_off;	/* -1 means unlimited */
 	struct strbuf buf;
 };
 
 #define SLIDING_VIEW_INIT(input, len)	{ (input), 0, 0, (len), STRBUF_INIT }
 
-extern int move_window(struct sliding_view *view, off_t off, size_t width);
+extern int move_window(struct sliding_view *view, off_t off, off_t width);
 
 #endif
diff --git a/vcs-svn/svndiff.c b/vcs-svn/svndiff.c
index 1647c1a..97d6967 100644
--- a/vcs-svn/svndiff.c
+++ b/vcs-svn/svndiff.c
@@ -75,7 +75,7 @@ static int error_short_read(struct line_buffer *input)
 }
 
 static int read_chunk(struct line_buffer *delta, off_t *delta_len,
-		      struct strbuf *buf, size_t len)
+		      struct strbuf *buf, off_t len)
 {
 	strbuf_reset(buf);
 	if (len > *delta_len ||
@@ -123,7 +123,7 @@ static int read_int(struct line_buffer *in, uintmax_t *result, off_t *len)
 	return error_short_read(in);
 }
 
-static int parse_int(const char **buf, size_t *result, const char *end)
+static int parse_int(const char **buf, off_t *result, const char *end)
 {
 	size_t rv = 0;
 	const char *pos;
@@ -165,9 +165,9 @@ static int read_length(struct line_buffer *in, size_t *result, off_t *len)
 }
 
 static int copyfrom_source(struct window *ctx, const char **instructions,
-			   size_t nbytes, const char *insns_end)
+			   off_t nbytes, const char *insns_end)
 {
-	size_t offset;
+	off_t offset;
 	if (parse_int(instructions, &offset, insns_end))
 		return -1;
 	if (unsigned_add_overflows(offset, nbytes) ||
@@ -180,10 +180,10 @@ static int copyfrom_source(struct window *ctx, const char **instructions,
 static int copyfrom_target(struct window *ctx, const char **instructions,
 			   size_t nbytes, const char *instructions_end)
 {
-	size_t offset;
+	off_t offset;
 	if (parse_int(instructions, &offset, instructions_end))
 		return -1;
-	if (offset >= ctx->out.len)
+	if (offset >= (off_t)(ctx->out.len))
 		return error("invalid delta: copies from the future");
 	for (; nbytes > 0; nbytes--)
 		strbuf_addch(&ctx->out, ctx->out.buf[offset++]);
@@ -201,9 +201,9 @@ static int copyfrom_data(struct window *ctx, size_t *data_pos, size_t nbytes)
 	return 0;
 }
 
-static int parse_first_operand(const char **buf, size_t *out, const char *end)
+static int parse_first_operand(const char **buf, off_t *out, const char *end)
 {
-	size_t result = (unsigned char) *(*buf)++ & OPERAND_MASK;
+	off_t result = (unsigned char) *(*buf)++ & OPERAND_MASK;
 	if (result) {	/* immediate operand */
 		*out = result;
 		return 0;
@@ -216,7 +216,7 @@ static int execute_one_instruction(struct window *ctx,
 {
 	unsigned int instruction;
 	const char *insns_end = ctx->instructions.buf + ctx->instructions.len;
-	size_t nbytes;
+	off_t nbytes;
 	assert(ctx);
 	assert(instructions && *instructions);
 	assert(data_pos);
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 8d0ae9c..73706ae 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -34,7 +34,7 @@
 #define NODE_CTX 2	/* node metadata */
 #define INTERNODE_CTX 3	/* between nodes */
 
-#define LENGTH_UNKNOWN (~0)
+#define LENGTH_UNKNOWN (~0u)
 #define DATE_RFC2822_LEN 31
 
 static struct line_buffer input = LINE_BUFFER_INIT;
-- 
1.7.10.2

[PATCH 4/7] 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 7396a91..0b2b7b9 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 b4104af..89f5dc2 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -123,7 +123,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 bf9a053..bbe20ad 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 73706ae..dd55cb7 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

[PATCH 5/7] vcs-svn: fix cppcheck warning

[David Barr]

[vcs-svn/fast_export.c:211]: (warning) Using sizeof with a numeric constant as function argument might not be what you intended.

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 0b2b7b9..d06a81c 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -208,7 +208,7 @@ static long apply_delta(off_t len, struct line_buffer *input,
 		die("cannot apply delta");
 	if (old_data) {
 		/* Read the remainder of preimage and trailing newline. */
-		assert(!signed_add_overflows(preimage.max_off, 1));
+		assert(!signed_add_overflows(preimage.max_off, (off_t) 1));
 		preimage.max_off++;	/* room for newline */
 		if (move_window(&preimage, preimage.max_off - 1, 1))
 			die("cannot seek to end of input");
-- 
1.7.10.2

[PATCH 6/7] 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 97d6967..7a71833 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 7/7] vcs-svn: fix clang-analyzer warning

[David Barr]

vcs-svn/svndiff.c:278:3: warning: expression result unused [-Wunused-value]
                error("invalid delta: incorrect postimage length");
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from vcs-svn/svndiff.c:6:
vcs-svn/compat-util.h:18:61: note: instantiated from:
#define error(...) (fprintf(stderr, "error: " __VA_ARGS__), -1)
                                                            ^~

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 7a71833..9cd2ba3 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

Re: [PATCH 5/7] vcs-svn: fix cppcheck warning

[Jonathan Nieder]

Hi,

First, thanks much for this series!  I especially like patch #4.

David Barr wrote:

> [vcs-svn/fast_export.c:211]: (warning) Using sizeof with a numeric constant
> as function argument might not be what you intended.
[...]
> -		assert(!signed_add_overflows(preimage.max_off, 1));
> +		assert(!signed_add_overflows(preimage.max_off, (off_t) 1));

I think this is due to a typo in the compat-util.h compatibility layer
you are using.  In git.git there is

	#define signed_add_overflows(a, b) \
	    ((b) > maximum_signed_value_of_type(a) - (a))

so the sizeof() operator is applied to preimage.max_off and all is well.
By contrast, in svn-dump-fast-export.git it says

	#define signed_add_overflows(a, b) \
		((a) > maximum_signed_value_of_type(b) - (b))

Though the comment describing signed_add_overflows() does say that "a"
and "b" should have the same type, I like being able to ask if
signed_add_overflows(n, 1) in a non-fussy way that does not introduce
subtle bugs when the type of "n" changes, so I'd prefer not to take
this patch.

Hope that helps,
Jonathan

Re: [PATCH 7/7] vcs-svn: fix clang-analyzer warning

[Jonathan Nieder]

David Barr wrote:

> vcs-svn/svndiff.c:278:3: warning: expression result unused [-Wunused-value]
>                 error("invalid delta: incorrect postimage length");
>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from vcs-svn/svndiff.c:6:
> vcs-svn/compat-util.h:18:61: note: instantiated from:
> #define error(...) (fprintf(stderr, "error: " __VA_ARGS__), -1)
>                                                             ^~

Yuck.  Would you be ok with an inline variadic function?

 static inline int error(const char *fmt, ...)
 {
	va_list ap;

	fprintf(stderr, "error: ");

	va_start(ap, fmt);
	vfprintf(stderr, fmt, ap)
	va_end(ap);

	fprintf(stderr, "\n");

	return -1;
 }

The error() macro above also seems to leave out a newline.

> --- a/vcs-svn/svndiff.c
> +++ b/vcs-svn/svndiff.c
> @@ -258,6 +258,7 @@ static int apply_window_in_core(struct window *ctx)
[...]
> @@ -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;

That said, if this change is justified by saying that it avoids having
to repeat the cleanup code, it already looks like a good change.  The
commit message could mention that the original motivation and a
side-benefit is to help the standalone version that has a slightly
crazier definition of error().

Jonathan

Re: [PATCH 3/7] vcs-svn: fix signedness warnings

[Jonathan Nieder]

David Barr wrote:

> --- 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 < (off_t) strlen("100644") ||

I wish the static analyzer could notice that "response_end - response"
is always nonnegative and stop worrying.  If we want to appease it,
I guess I'd mildly prefer something like

	if (response_end - response < (signed) strlen("100644") ||

which expresses the intent more directly.

[...]
> --- a/vcs-svn/line_buffer.c
> +++ b/vcs-svn/line_buffer.c
> @@ -91,8 +91,7 @@ char *buffer_read_line(struct line_buffer *buf)
>  	return buf->line_buffer;
>  }
>  
> -size_t buffer_read_binary(struct line_buffer *buf,
> -				struct strbuf *sb, size_t size)
> +off_t buffer_read_binary(struct line_buffer *buf, struct strbuf *sb, off_t size)
>  {
>  	return strbuf_fread(sb, size, buf->infile);
>  }

On systems with larger off_t than size_t (think "typical 32-bit PC,
since file offsets tend to be 64 bits"), this silently throws away
bits.  I think the cure is worse than the disease.

[...]
> --- a/vcs-svn/sliding_window.c
> +++ b/vcs-svn/sliding_window.c
> @@ -43,11 +43,11 @@ static int check_offset_overflow(off_t offset, uintmax_t len)
>  	return 0;
>  }
>  
> -int move_window(struct sliding_view *view, off_t off, size_t width)
> +int move_window(struct sliding_view *view, off_t off, off_t width)
>  {

Likewise.  I'd rather the caller know that the window has to fit in an
address space which can be smaller than the maximum file size.

Is this to avoid having two different functions that parse a
variable-length integer, or is there some other reason?

Hope that helps,
Jonathan

Re: [PATCH 5/7] vcs-svn: fix cppcheck warning

[David Michael Barr]

On Fri, May 25, 2012 at 12:20 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> First, thanks much for this series!  I especially like patch #4.
>
> David Barr wrote:
>
>> [vcs-svn/fast_export.c:211]: (warning) Using sizeof with a numeric constant
>> as function argument might not be what you intended.
> [...]
>> -             assert(!signed_add_overflows(preimage.max_off, 1));
>> +             assert(!signed_add_overflows(preimage.max_off, (off_t) 1));
>
> I think this is due to a typo in the compat-util.h compatibility layer
> you are using.  In git.git there is
>
>        #define signed_add_overflows(a, b) \
>            ((b) > maximum_signed_value_of_type(a) - (a))
>
> so the sizeof() operator is applied to preimage.max_off and all is well.
> By contrast, in svn-dump-fast-export.git it says
>
>        #define signed_add_overflows(a, b) \
>                ((a) > maximum_signed_value_of_type(b) - (b))
>
> Though the comment describing signed_add_overflows() does say that "a"
> and "b" should have the same type, I like being able to ask if
> signed_add_overflows(n, 1) in a non-fussy way that does not introduce
> subtle bugs when the type of "n" changes, so I'd prefer not to take
> this patch.

Thank you for the review. I will queue the typo fix upstream and drop
this patch.

--
David Barr

Re: [PATCH 7/7] vcs-svn: fix clang-analyzer warning

[David Michael Barr]

On Fri, May 25, 2012 at 12:33 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> David Barr wrote:
>
>> vcs-svn/svndiff.c:278:3: warning: expression result unused [-Wunused-value]
>>                 error("invalid delta: incorrect postimage length");
>>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> In file included from vcs-svn/svndiff.c:6:
>> vcs-svn/compat-util.h:18:61: note: instantiated from:
>> #define error(...) (fprintf(stderr, "error: " __VA_ARGS__), -1)
>>                                                             ^~
>
> Yuck.  Would you be ok with an inline variadic function?
>
>  static inline int error(const char *fmt, ...)
>  {
>        va_list ap;
>
>        fprintf(stderr, "error: ");
>
>        va_start(ap, fmt);
>        vfprintf(stderr, fmt, ap)
>        va_end(ap);
>
>        fprintf(stderr, "\n");
>
>        return -1;
>  }
>
> The error() macro above also seems to leave out a newline.
>
>> --- a/vcs-svn/svndiff.c
>> +++ b/vcs-svn/svndiff.c
>> @@ -258,6 +258,7 @@ static int apply_window_in_core(struct window *ctx)
> [...]
>> @@ -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;
>
> That said, if this change is justified by saying that it avoids having
> to repeat the cleanup code, it already looks like a good change.  The
> commit message could mention that the original motivation and a
> side-benefit is to help the standalone version that has a slightly
> crazier definition of error().

I'll rework the commit message and requeue.

--
David Barr.

Re: [PATCH 3/7] vcs-svn: fix signedness warnings

[David Michael Barr]

On Fri, May 25, 2012 at 12:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> David Barr wrote:
>
>> --- 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 < (off_t) strlen("100644") ||
>
> I wish the static analyzer could notice that "response_end - response"
> is always nonnegative and stop worrying.  If we want to appease it,
> I guess I'd mildly prefer something like
>
>        if (response_end - response < (signed) strlen("100644") ||
>
> which expresses the intent more directly.

Noted.

> [...]
>> --- a/vcs-svn/line_buffer.c
>> +++ b/vcs-svn/line_buffer.c
>> @@ -91,8 +91,7 @@ char *buffer_read_line(struct line_buffer *buf)
>>       return buf->line_buffer;
>>  }
>>
>> -size_t buffer_read_binary(struct line_buffer *buf,
>> -                             struct strbuf *sb, size_t size)
>> +off_t buffer_read_binary(struct line_buffer *buf, struct strbuf *sb, off_t size)
>>  {
>>       return strbuf_fread(sb, size, buf->infile);
>>  }
>
> On systems with larger off_t than size_t (think "typical 32-bit PC,
> since file offsets tend to be 64 bits"), this silently throws away
> bits.  I think the cure is worse than the disease.

Agreed, I'll implement a better approach when I update the series.

> [...]
>> --- a/vcs-svn/sliding_window.c
>> +++ b/vcs-svn/sliding_window.c
>> @@ -43,11 +43,11 @@ static int check_offset_overflow(off_t offset, uintmax_t len)
>>       return 0;
>>  }
>>
>> -int move_window(struct sliding_view *view, off_t off, size_t width)
>> +int move_window(struct sliding_view *view, off_t off, off_t width)
>>  {
>
> Likewise.  I'd rather the caller know that the window has to fit in an
> address space which can be smaller than the maximum file size.
>
> Is this to avoid having two different functions that parse a
> variable-length integer, or is there some other reason?

Nope, just me taking the wrong approach.

I'll submit an alternate patch, which much shorter and less worrying.

--
David Barr