Re: [PATCH 1/5] fast-import: Let importers retrieve blobs

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> Note to self: I didn't notice write_in_full in wrapper.c
> earlier. Returns the actual size written to the fd from the buffer.

Yeah, write_in_full() is write() without the partial-read semantics
when interrupted by a signal, on Windows, or encountering an empty
pipe.

>> +static void option_cat_blob_fd(const char *fd)
>> +{
>> +	unsigned long n = strtoul(fd, NULL, 0);
>> +	if (n > (unsigned long) INT_MAX)
>> +		die("--cat-blob-fd cannot exceed %d", INT_MAX);
>> +	cat_blob_fd = (int) n;
>> +}
>> +
>
> You don't display an appropriate error when n < 0.

How can n be < 0?  strtoul returns an unsigned long.

But more to the point, yes, this does not return an appropriate
error when "--cat-blob-fd=" is not followed by an unsigned
integer.  At least it's consistent with --depth=nonsense et al.

Rough patch below (needs tests).

> Ok. I'm eager to see this go through to `master`.
> Reviewed-by: Ramkumar Ramachandra <artagnon@gmail.com>

Thanks.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
diff --git a/fast-import.c b/fast-import.c
index eb6860d..20023c1 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2822,16 +2822,25 @@ static void option_date_format(const char *fmt)
 		die("unknown --date-format argument %s", fmt);
 }
 
+static unsigned long ulong_arg(const char *arg)
+{
+	char *endptr;
+	unsigned long rv = strtoul(arg, &endptr, 0);
+	if (endptr == arg || *endptr)
+		die("%s: argument must be an unsigned integer", arg);
+	return rv;
+}
+
 static void option_depth(const char *depth)
 {
-	max_depth = strtoul(depth, NULL, 0);
+	max_depth = ulong_arg(depth);
 	if (max_depth > MAX_DEPTH)
 		die("--depth cannot exceed %u", MAX_DEPTH);
 }
 
 static void option_active_branches(const char *branches)
 {
-	max_active_branches = strtoul(branches, NULL, 0);
+	max_active_branches = ulong_arg(branches);
 }
 
 static void option_export_marks(const char *marks)
@@ -2842,7 +2851,7 @@ static void option_export_marks(const char *marks)
 
 static void option_cat_blob_fd(const char *fd)
 {
-	unsigned long n = strtoul(fd, NULL, 0);
+	unsigned long n = ulong_arg(fd);
 	if (n > (unsigned long) INT_MAX)
 		die("--cat-blob-fd cannot exceed %d", INT_MAX);
 	cat_blob_fd = (int) n;