Update git_blob_create_fromchunks callback behavr

The callback to supply data chunks could return a negative value to stop creation of the blob, but we were neither using GIT_EUSER nor propagating the return value. This makes things use the new behavior of returning the negative value back to the user.
libgit2 · vmg · Dec 13, 2013 · Nov 25, 2013 · Dec 4, 2013 · Dec 5, 2013
commit 19853bdd97e006b6e4519bc352c3e8fd7586e9c3
diff --git a/include/git2/blob.h b/include/git2/blob.h
@@ -159,37 +159,32 @@ typedef int (*git_blob_chunk_cb)(char *content, size_t max_length, void *payload
  * Write a loose blob to the Object Database from a
  * provider of chunks of data.
  *
- * Provided the `hintpath` parameter is filled, its value
- * will help to determine what git filters should be applied
- * to the object before it can be placed to the object database.
+ * If the `hintpath` parameter is filled, it will be used to determine
+ * what git filters should be applied to the object before it is written
+ * to the object database.
  *
+ * The implementation of the callback MUST respect the following rules:
  *
- * The implementation of the callback has to respect the
- * following rules:
+ *  - `content` must be filled by the callback. The maximum number of
+ *    bytes that the buffer can accept per call is defined by the
+ *    `max_length` parameter. Allocation and freeing of the buffer will
+ *    be taken care of by libgit2.
  *
- *  - `content` will have to be filled by the consumer. The maximum number
- * of bytes that the buffer can accept per call is defined by the
- * `max_length` parameter. Allocation and freeing of the buffer will be taken
- * care of by the function.
+ *  - The `callback` must return the number of bytes that have been
+ *    written to the `content` buffer.
  *
- *  - The callback is expected to return the number of bytes
- * that `content` have been filled with.
- *
- *  - When there is no more data to stream, the callback should
- * return 0. This will prevent it from being invoked anymore.
- *
- *  - When an error occurs, the callback should return -1.
+ *  - When there is no more data to stream, `callback` should return
+ *    0. This will prevent it from being invoked anymore.
  *
+ *  - If an error occurs, the callback should return a negative value.
+ *    This value will be returned to the caller.
  *
  * @param id Return the id of the written blob
- *
- * @param repo repository where the blob will be written.
- * This repository can be bare or not.
- *
- * @param hintpath if not NULL, will help selecting the filters
- * to apply onto the content of the blob to be created.
- *
- * @return 0 or an error code
+ * @param repo Repository where the blob will be written.
+ *        This repository can be bare or not.
+ * @param hintpath If not NULL, will be used to select data filters
+ *        to apply onto the content of the blob to be created.
+ * @return 0 or error code (from either libgit2 or callback function)
  */
 GIT_EXTERN(int) git_blob_create_fromchunks(
 	git_oid *id,

diff --git a/src/blob.c b/src/blob.c
@@ -272,37 +272,44 @@ int git_blob_create_fromchunks(
 	int (*source_cb)(char *content, size_t max_length, void *payload),
 	void *payload)
 {
-	int error = -1, read_bytes;
+	int error;
 	char *content = NULL;
 	git_filebuf file = GIT_FILEBUF_INIT;
 	git_buf path = GIT_BUF_INIT;
 
-	if (git_buf_joinpath(
-			&path, git_repository_path(repo), GIT_OBJECTS_DIR "streamed") < 0)
+	assert(oid && repo && source_cb);
+
+	if ((error = git_buf_joinpath(
+			&path, git_repository_path(repo), GIT_OBJECTS_DIR "streamed")) < 0)
 		goto cleanup;
 
 	content = git__malloc(BUFFER_SIZE);
 	GITERR_CHECK_ALLOC(content);
 
-	if (git_filebuf_open(&file, git_buf_cstr(&path), GIT_FILEBUF_TEMPORARY, 0666) < 0)
+	if ((error = git_filebuf_open(
+			&file, git_buf_cstr(&path), GIT_FILEBUF_TEMPORARY, 0666)) < 0)
 		goto cleanup;
 
 	while (1) {
-		read_bytes = source_cb(content, BUFFER_SIZE, payload);
-
-		assert(read_bytes <= BUFFER_SIZE);
+		int read_bytes = source_cb(content, BUFFER_SIZE, payload);
 
-		if (read_bytes <= 0)
+		if (!read_bytes)
 			break;
 
-		if (git_filebuf_write(&file, content, read_bytes) < 0)
+		if (read_bytes > BUFFER_SIZE) {
+			giterr_set(GITERR_OBJECT, "Invalid chunk size while creating blob");
+			error = GIT_EBUFS;
+		} else if (read_bytes < 0) {
+			error = giterr_set_after_callback(read_bytes);
+		} else {
+			error = git_filebuf_write(&file, content, read_bytes);
+		}
+
+		if (error < 0)
 			goto cleanup;
 	}
 
-	if (read_bytes < 0)
-		goto cleanup;
-
-	if (git_filebuf_flush(&file) < 0)
+	if ((error = git_filebuf_flush(&file)) < 0)
 		goto cleanup;
 
 	error = git_blob__create_from_paths(
@@ -312,6 +319,7 @@ int git_blob_create_fromchunks(
 	git_buf_free(&path);
 	git_filebuf_cleanup(&file);
 	git__free(content);
+
 	return error;
 }
 

diff --git a/tests/object/blob/fromchunks.c b/tests/object/blob/fromchunks.c
@@ -59,7 +59,7 @@ void test_object_blob_fromchunks__doesnot_overwrite_an_already_existing_object(v
 	git_buf content = GIT_BUF_INIT;
 	git_oid expected_oid, oid;
 	int howmany = 7;
-	
+
 	cl_git_pass(git_oid_fromstr(&expected_oid, "321cbdf08803c744082332332838df6bd160f8f9"));
 
 	cl_git_pass(git_blob_create_fromchunks(&oid, repo, NULL, text_chunked_source_cb, &howmany));
@@ -117,3 +117,40 @@ void tes
8000
t_object_blob_fromchunks__creating_a_blob_from_chunks_honors_the_attribu
 	assert_named_chunked_blob("e9671e138a780833cb689753570fd10a55be84fb", "dummy.txt");
 	assert_named_chunked_blob("e9671e138a780833cb689753570fd10a55be84fb", "dummy.dunno");
 }
+
+static int failing_chunked_source_cb(
+	char *content, size_t max_length, void *payload)
+{
+	int *count = (int *)payload;
+
+	GIT_UNUSED(max_length);
+
+	(*count)--;
+	if (*count == 0)
+		return -1234;
+
+	strcpy(content, textual_content);
+	return (int)strlen(textual_content);
+}
+
+void test_object_blob_fromchunks__can_stop_with_error(void)
+{
+	git_oid expected_oid, oid;
+	git_object *blob;
+	int howmany = 7;
+
+	cl_git_pass(git_oid_fromstr(
+		&expected_oid, "321cbdf08803c744082332332838df6bd160f8f9"));
+
+	cl_git_fail_with(
+		git_object_lookup(&blob, repo, &expected_oid, GIT_OBJ_ANY),
+		GIT_ENOTFOUND);
+
+	cl_git_fail_with(git_blob_create_fromchunks(
+		&oid, repo, NULL, failing_chunked_source_cb, &howmany), -1234);
+
+	cl_git_fail_with(
+		git_object_lookup(&blob, repo, &expected_oid, GIT_OBJ_ANY),
+		GIT_ENOTFOUND);
+}
+