threads / patch / 30613

patch, 7 partsvcs-svn: housekeeping

Subject: [PATCH 0/7] vcs-svn: housekeeping

## tl;dr

14 messages between May 24, 2012 and May 31, 2012. Diffs are folded; open one to read it.

replies: 13people: 2as markdown or json

David Barr· May 24, 2012, 14:04 UTC · lore

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

David Barr· May 24, 2012, 14:04 UTC · re: David Barr · lore

[PATCH 1/7] vcs-svn: prefer constcmp to prefixcmp

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(-)
Show changes to vcs-svn/svndump.c +1 −1
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
David Barr· May 24, 2012, 14:04 UTC · re: David Barr · lore

[PATCH 2/7] vcs-svn: prefer strstr over memmem

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(-)
Show changes to vcs-svn/fast_export.c +1 −1
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
David Barr· May 24, 2012, 14:04 UTC · re: David Barr · lore

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

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(-)
Show changes to 7 files +19 −20

vcs-svn/fast_export.c, vcs-svn/line_buffer.c, vcs-svn/line_buffer.h, vcs-svn/sliding_window.c, vcs-svn/sliding_window.h, vcs-svn/svndiff.c, vcs-svn/svndump.c

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
Jonathan Nieder· May 24, 2012, 14:48 UTC · re: David Barr · lore

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

David Barr wrote:
Show 8 quoted lines
> --- 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.
[...]
Show 12 quoted lines
> --- 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.

[...]
Show 9 quoted lines
> --- 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

David Michael Barr· May 31, 2012, 13:14 UTC · re: Jonathan Nieder · lore

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

On Fri, May 25, 2012 at 12:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 18 quoted lines
> 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.
Show 17 quoted lines
> [...]
>> --- 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.
Show 16 quoted lines
> [...]
>> --- 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

David Barr· May 24, 2012, 14:04 UTC · re: David Barr · lore

[PATCH 4/7] vcs-svn: drop no-op reset methods

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(-)
Show changes to 5 files +0 −13

vcs-svn/fast_export.c, vcs-svn/fast_export.h, vcs-svn/line_buffer.c, vcs-svn/line_buffer.h, vcs-svn/svndump.c

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
David Barr· May 24, 2012, 14:04 UTC · re: David Barr · lore

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

[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(-)
Show changes to vcs-svn/fast_export.c +1 −1
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
Jonathan Nieder· May 24, 2012, 14:20 UTC · re: David Barr · lore

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

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

David Michael Barr· May 31, 2012, 11:17 UTC · re: Jonathan Nieder · lore

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

On Fri, May 25, 2012 at 12:20 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 29 quoted lines
> 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

David Barr· May 24, 2012, 14:04 UTC · re: David Barr · lore

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

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(-)
Show changes to vcs-svn/svndiff.c +1 −1
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
David Barr· May 24, 2012, 14:04 UTC · re: David Barr · lore

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

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(-)
Show changes to vcs-svn/svndiff.c +4 −4
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
Jonathan Nieder· May 24, 2012, 14:33 UTC · re: David Barr · lore

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

David Barr wrote:
Show 7 quoted lines
> 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)
[...]
Show 17 quoted lines
> @@ -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
David Michael Barr· May 31, 2012, 11:34 UTC · re: Jonathan Nieder · lore

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

On Fri, May 25, 2012 at 12:33 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 56 quoted lines
> 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.

← back to recent threads