Improve error propagation in stash

Stash was sometimes obscuring the actual error code, replacing it
with a -1 when there was more descriptive value.  This updates
stash to preserve the original error code more reliably along
with a variety of other error handling tweaks.

I believe this is an improvement, but arguably, preserving the
underlying error code may result in values that are harder to
interpret by the caller who does not understand the internals.
Discussion is welcome!

src/stash.c

@@ -22,15 +22,6 @@ static int create_error(int error, const char *msg)
 	return error;
 }
 
-static int ensure_non_bare_repository(git_repository *repo)
-{
-	if (!git_repository_is_bare(repo))
-		return 0;
-
-	return create_error(GIT_EBAREREPO,
-		"Stash related operations require a working directory.");
-}
-
 static int retrieve_head(git_reference **out, git_repository *repo)
 {
 	int error = git_repository_head(out, repo);
@@ -89,23 +80,22 @@ static int retrieve_base_commit_and_message(
 	if ((error = retrieve_head(&head, repo)) < 0)
 		return error;
 
-	error = -1;
-
 	if (strcmp("HEAD", git_reference_name(head)) == 0)
-		git_buf_puts(stash_message, "(no branch): ");
+		error = git_buf_puts(stash_message, "(no branch): ");
 	else
-		git_buf_printf(
+		error = git_buf_printf(
 			stash_message,
 			"%s: ",
 			git_reference_name(head) + strlen(GIT_REFS_HEADS_DIR));
-
-	if (git_commit_lookup(b_commit, repo, git_reference_target(head)) < 0)
+	if (error < 0)
 		goto cleanup;
 
-	if (append_commit_description(stash_message, *b_commit) < 0)
+	if ((error = git_commit_lookup(
+			 b_commit, repo, git_reference_target(head))) < 0)
 		goto cleanup;
 
-	error = 0;
+	if ((error = append_commit_description(stash_message, *b_commit)) < 0)
+		goto cleanup;
 
 cleanup:
 	git_reference_free(head);
@@ -114,9 +104,10 @@ cleanup:
 
 static int build_tree_from_index(git_tree **out, git_index *index)
 {
+	int error;
 	git_oid i_tree_oid;
 
-	if (git_index_write_tree(&i_tree_oid, index) < 0)
+	if ((error = git_index_write_tree(&i_tree_oid, index)) < 0)
 		return -1;
 
 	return git_tree_lookup(out, git_index_owner(index), &i_tree_oid);
@@ -132,15 +123,15 @@ static int commit_index(
 	git_tree *i_tree = NULL;
 	git_oid i_commit_oid;
 	git_buf msg = GIT_BUF_INIT;
-	int error = -1;
+	int error;
 
-	if (build_tree_from_index(&i_tree, index) < 0)
+	if ((error = build_tree_from_index(&i_tree, index)) < 0)
 		goto cleanup;
 
-	if (git_buf_printf(&msg, "index on %s\n", message) < 0)
+	if ((error = git_buf_printf(&msg, "index on %s\n", message)) < 0)
 		goto cleanup;
 
-	if (git_commit_create(
+	if ((error = git_commit_create(
 		&i_commit_oid,
 		git_index_owner(index),
 		NULL,
@@ -150,8 +141,8 @@ static int commit_index(
 		git_buf_cstr(&msg),
 		i_tree,
 		1,
-		&parent) < 0)
-			goto cleanup;
+		&parent)) < 0)
+		goto cleanup;
 
 	error = git_commit_lookup(i_commit, git_index_owner(index), &i_commit_oid);
 
@@ -164,6 +155,8 @@ cleanup:
 struct cb_data {
 	git_index *index;
 
+	int error;
+
 	bool include_changed;
 	bool include_untracked;
 	bool include_ignored;
@@ -174,52 +167,50 @@ static int update_index_cb(
 	float progress,
 	void *payload)
 {
-	int pos;
 	struct cb_data *data = (struct cb_data *)payload;
+	const char *add_path = NULL;
 
 	GIT_UNUSED(progress);
 
 	switch (delta->status) {
 	case GIT_DELTA_IGNORED:
-		if (!data->include_ignored)
-			break;
-
-		return git_index_add_from_workdir(data->index, delta->new_file.path);
+		if (data->include_ignored)
+			add_path = delta->new_file.path;
+		break;
 
 	case GIT_DELTA_UNTRACKED:
-		if (!data->include_untracked)
-			break;
-
-		return git_index_add_from_workdir(data->index, delta->new_file.path);
+		if (data->include_untracked)
+			add_path = delta->new_file.path;
+		break;
 
 	case GIT_DELTA_ADDED:
-		/* Fall through */
 	case GIT_DELTA_MODIFIED:
-		if (!data->include_changed)
-			break;
-
-		return git_index_add_from_workdir(data->index, delta->new_file.path);
+		if (data->include_changed)
+			add_path = delta->new_file.path;
+		break;
 
 	case GIT_DELTA_DELETED:
 		if (!data->include_changed)
 			break;
-
-		if ((pos = git_index_find(data->index, delta->new_file.path)) < 0)
-			return -1;
-
-		if (git_index_remove(data->index, delta->new_file.path, 0) < 0)
-			return -1;
+		if (git_index_find(data->index, delta->old_file.path) == 0)
+			data->error = git_index_remove(
+				data->index, delta->old_file.path, 0);
+		break;
 
 	default:
 		/* Unimplemented */
 		giterr_set(
 			GITERR_INVALID,
-			"Cannot update index. Unimplemented status kind (%d)",
+			"Cannot update index. Unimplemented status (%d)",
 			delta->status);
-		return -1;
+		data->error = -1;
+		break;
 	}
 
-	return 0;
+	if (add_path != NULL)
+		data->error = git_index_add_from_workdir(data->index, add_path);
+
+	return data->error;
 }
 
 static int build_untracked_tree(
@@ -232,14 +223,15 @@ static int build_untracked_tree(
 	git_diff_list *diff = NULL;
 	git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
 	struct cb_data data = {0};
-	int error = -1;
+	int error;
 
 	git_index_clear(index);
 
 	data.index = index;
 
 	if (flags & GIT_STASH_INCLUDE_UNTRACKED) {
-		opts.flags |= GIT_DIFF_INCLUDE_UNTRACKED | GIT_DIFF_RECURSE_UNTRACKED_DIRS;
+		opts.flags |= GIT_DIFF_INCLUDE_UNTRACKED |
+			GIT_DIFF_RECURSE_UNTRACKED_DIRS;
 		data.include_untracked = true;
 	}
 
@@ -248,19 +240,22 @@ static int build_untracked_tree(
 		data.include_ignored = true;
 	}
 
-	if (git_commit_tree(&i_tree, i_commit) < 0)
-		goto cleanup;
-
-	if (git_diff_tree_to_workdir(&diff, git_index_owner(index), i_tree, &opts) < 0)
+	if ((error = git_commit_tree(&i_tree, i_commit)) < 0)
 		goto cleanup;
 
-	if (git_diff_foreach(diff, update_index_cb, NULL, NULL, &data) < 0)
+	if ((error = git_diff_tree_to_workdir(
+			&diff, git_index_owner(index), i_tree, &opts)) < 0)
 		goto cleanup;
 
-	if (build_tree_from_index(tree_out, index) < 0)
+	if ((error = git_diff_foreach(
+			diff, update_index_cb, NULL, NULL, &data)) < 0)
+	{
+		if (error == GIT_EUSER)
+			error = data.error;
 		goto cleanup;
+	}
 
-	error = 0;
+	error = build_tree_from_index(tree_out, index);
 
 cleanup:
 	git_diff_list_free(diff);
@@ -279,15 +274,15 @@ static int commit_untracked(
 	git_tree *u_tree = NULL;
 	git_oid u_commit_oid;
 	git_buf msg = GIT_BUF_INIT;
-	int error = -1;
+	int error;
 
-	if (build_untracked_tree(&u_tree, index, i_commit, flags) < 0)
+	if ((error = build_untracked_tree(&u_tree, index, i_commit, flags)) < 0)
 		goto cleanup;
 
-	if (git_buf_printf(&msg, "untracked files on %s\n", message) < 0)
+	if ((error = git_buf_printf(&msg, "untracked files on %s\n", message)) < 0)
 		goto cleanup;
 
-	if (git_commit_create(
+	if ((error = git_commit_create(
 		&u_commit_oid,
 		git_index_owner(index),
 		NULL,
@@ -297,8 +292,8 @@ static int commit_untracked(
 		git_buf_cstr(&msg),
 		u_tree,
 		0,
-		NULL) < 0)
-			goto cleanup;
+		NULL)) < 0)
+		goto cleanup;
 
 	error = git_commit_lookup(u_commit, git_index_owner(index), &u_commit_oid);
 
@@ -318,35 +313,40 @@ static int build_workdir_tree(
 	git_diff_list *diff = NULL, *diff2 = NULL;
 	git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
 	struct cb_data data = {0};
-	int error = -1;
+	int error;
 
-	if (git_commit_tree(&b_tree, b_commit) < 0)
+	if ((error = git_commit_tree(&b_tree, b_commit)) < 0)
 		goto cleanup;
 
-	if (git_diff_tree_to_index(&diff, repo, b_tree, NULL, &opts) < 0)
+	if ((error = git_diff_tree_to_index(&diff, repo, b_tree, NULL, &opts)) < 0)
 		goto cleanup;
 
-	if (git_diff_index_to_workdir(&diff2, repo, NULL, &opts) < 0)
+	if ((error = git_diff_index_to_workdir(&diff2, repo, NULL, &opts)) < 0)
 		goto cleanup;
 
-	if (git_diff_merge(diff, diff2) < 0)
+	if ((error = git_diff_merge(diff, diff2)) < 0)
 		goto cleanup;
 
 	data.index = index;
 	data.include_changed = true;
 
-	if (git_diff_foreach(diff, update_index_cb, NULL, NULL, &data) < 0)
+	if ((error = git_diff_foreach(
+			diff, update_index_cb, NULL, NULL, &data)) < 0)
+	{
+		if (error == GIT_EUSER)
+			error = data.error;
 		goto cleanup;
+	}
 
-	if (build_tree_from_index(tree_out, index) < 0)
-		goto cleanup;
 
-	error = 0;
+	if ((error = build_tree_from_index(tree_out, index)) < 0)
+		goto cleanup;
 
 cleanup:
 	git_diff_list_free(diff);
 	git_diff_list_free(diff2);
 	git_tree_free(b_tree);
+
 	return error;
 }
 
@@ -359,25 +359,24 @@ static int commit_worktree(
 	git_commit *b_commit,
 	git_commit *u_commit)
 {
+	int error = 0;
 	git_tree *w_tree = NULL, *i_tree = NULL;
-	int error = -1;
-
 	const git_commit *parents[] = {	NULL, NULL,	NULL };
 
 	parents[0] = b_commit;
 	parents[1] = i_commit;
 	parents[2] = u_commit;
 
-	if (git_commit_tree(&i_tree, i_commit) < 0)
-		return -1;
+	if ((error = git_commit_tree(&i_tree, i_commit)) < 0)
+		goto cleanup;
 
-	if (git_index_read_tree(index, i_tree) < 0)
+	if ((error = git_index_read_tree(index, i_tree)) < 0)
 		goto cleanup;
 
-	if (build_workdir_tree(&w_tree, index, b_commit) < 0)
+	if ((error = build_workdir_tree(&w_tree, index, b_commit)) < 0)
 		goto cleanup;
 
-	if (git_commit_create(
+	error = git_commit_create(
 		w_commit_oid,
 		git_index_owner(index),
 		NULL,
@@ -386,10 +385,8 @@ static int commit_worktree(
 		NULL,
 		message,
 		w_tree,
-		u_commit ? 3 : 2, parents) < 0)
-			goto cleanup;
-
-	error = 0;
+		u_commit ? 3 : 2,
+		parents);
 
 cleanup:
 	git_tree_free(i_tree);
@@ -402,9 +399,11 @@ static int prepare_worktree_commit_message(
 	const char *user_message)
 {
 	git_buf buf = GIT_BUF_INIT;
-	int error = -1;
+	int error;
+
+	if (git_buf_set(&buf, git_buf_cstr(msg), git_buf_len(msg)) < 0)
+		return -1;
 
-	git_buf_set(&buf, git_buf_cstr(msg), git_buf_len(msg));
 	git_buf_clear(msg);
 
 	if (!user_message)
@@ -424,6 +423,7 @@ static int prepare_worktree_commit_message(
 
 cleanup:
 	git_buf_free(&buf);
+
 	return error;
 }
 
@@ -449,8 +449,6 @@ static int update_reflog(
 	if ((error = git_reflog_write(reflog)) < 0)
 		goto cleanup;
 
-	error = 0;
-
 cleanup:
 	git_reference_free(stash);
 	git_reflog_free(reflog);
@@ -522,7 +520,7 @@ int git_stash_save(
 
 	assert(out && repo && stasher);
 
-	if ((error = ensure_non_bare_repository(repo)) < 0)
+	if ((error = git_repository__ensure_not_bare(repo, "stash save")) < 0)
 		return error;
 
 	if ((error = retrieve_base_commit_and_message(&b_commit, &msg, repo)) < 0)
@@ -530,47 +528,50 @@ int git_stash_save(
 
 	if ((error = ensure_there_are_changes_to_stash(
 		repo,
-		(flags & GIT_STASH_INCLUDE_UNTRACKED) == GIT_STASH_INCLUDE_UNTRACKED,
-		(flags & GIT_STASH_INCLUDE_IGNORED) == GIT_STASH_INCLUDE_IGNORED)) < 0)
+		(flags & GIT_STASH_INCLUDE_UNTRACKED) != 0,
+		(flags & GIT_STASH_INCLUDE_IGNORED) != 0)) < 0)
 		goto cleanup;
 
-	error = -1;
-
-	if (git_repository_index(&index, repo) < 0)
+	if ((error = git_repository_index(&index, repo)) < 0)
 		goto cleanup;
 
-	if (commit_index(&i_commit, index, stasher, git_buf_cstr(&msg), b_commit) < 0)
+	if ((error = commit_index(
+			&i_commit, index, stasher, git_buf_cstr(&msg), b_commit)) < 0)
 		goto cleanup;
 
-	if ((flags & GIT_STASH_INCLUDE_UNTRACKED || flags & GIT_STASH_INCLUDE_IGNORED)
-		&& commit_untracked(&u_commit, index, stasher, git_buf_cstr(&msg), i_commit, flags) < 0)
+	if ((flags & (GIT_STASH_INCLUDE_UNTRACKED | GIT_STASH_INCLUDE_IGNORED)) &&
+		(error = commit_untracked(
+			&u_commit, index, stasher, git_buf_cstr(&msg),
+			i_commit, flags)) < 0)
 		goto cleanup;
 
-	if (prepare_worktree_commit_message(&msg, message) < 0)
+	if ((error = prepare_worktree_commit_message(&msg, message)) < 0)
 		goto cleanup;
 
-	if (commit_worktree(out, index, stasher, git_buf_cstr(&msg), i_commit, b_commit, u_commit) < 0)
+	if ((error = commit_worktree(
+			out, index, stasher, git_buf_cstr(&msg),
+			i_commit, b_commit, u_commit)) < 0)
 		goto cleanup;
 
 	git_buf_rtrim(&msg);
-	if (update_reflog(out, repo, stasher, git_buf_cstr(&msg)) < 0)
+
+	if ((error = update_reflog(out, repo, stasher, git_buf_cstr(&msg))) < 0)
 		goto cleanup;
 
-	if (reset_index_and_workdir(
+	if ((error = reset_index_and_workdir(
 		repo,
-		((flags & GIT_STASH_KEEP_INDEX) == GIT_STASH_KEEP_INDEX) ?
-			i_commit : b_commit,
-		(flags & GIT_STASH_INCLUDE_UNTRACKED) == GIT_STASH_INCLUDE_UNTRACKED) < 0)
+		((flags & GIT_STASH_KEEP_INDEX) != 0) ? i_commit : b_commit,
+		(flags & GIT_STASH_INCLUDE_UNTRACKED) != 0)) < 0)
 		goto cleanup;
 
-	error = 0;
-
 cleanup:
+
 	git_buf_free(&msg);
 	git_commit_free(i_commit);
 	git_commit_free(b_commit);
 	git_commit_free(u_commit);
 	git_index_free(index);
+
 	return error;
 }
 
@@ -588,7 +589,6 @@ int git_stash_foreach(
 	error = git_reference_lookup(&stash, repo, GIT_REFS_STASH_FILE);
 	if (error == GIT_ENOTFOUND)
 		return 0;
-
 	if (error < 0)
 		goto cleanup;
 
@@ -598,18 +598,16 @@ int git_stash_foreach(
 	max = git_reflog_entrycount(reflog);
 	for (i = 0; i < max; i++) {
 		entry = git_reflog_entry_byindex(reflog, i);
-		
+
 		if (callback(i,
 			git_reflog_entry_message(entry),
 			git_reflog_entry_id_new(entry),
 			payload)) {
 				error = GIT_EUSER;
-				goto cleanup;
+				break;
 		}
 	}
 
-	error = 0;
-
 cleanup:
 	git_reference_free(stash);
 	git_reflog_free(reflog);

tests-clar/stash/drop.c

@@ -36,15 +36,22 @@ static void push_three_states(void)
 	cl_git_pass(git_repository_index(&index, repo));
 	cl_git_pass(git_index_add_from_workdir(index, "zero.txt"));
 	commit_staged_files(&oid, index, signature);
+	cl_assert(git_path_exists("stash/zero.txt"));
 
 	cl_git_mkfile("stash/one.txt", "content\n");
 	cl_git_pass(git_stash_save(&oid, repo, signature, "First", GIT_STASH_INCLUDE_UNTRACKED));
+	cl_assert(!git_path_exists("stash/one.txt"));
+	cl_assert(git_path_exists("stash/zero.txt"));
 
 	cl_git_mkfile("stash/two.txt", "content\n");
 	cl_git_pass(git_stash_save(&oid, repo, signature, "Second", GIT_STASH_INCLUDE_UNTRACKED));
+	cl_assert(!git_path_exists("stash/two.txt"));
+	cl_assert(git_path_exists("stash/zero.txt"));
 
 	cl_git_mkfile("stash/three.txt", "content\n");
 	cl_git_pass(git_stash_save(&oid, repo, signature, "Third", GIT_STASH_INCLUDE_UNTRACKED));
+	cl_assert(!git_path_exists("stash/three.txt"));
+	cl_assert(git_path_exists("stash/zero.txt"));
 
 	git_index_free(index);
 }

tests-clar/stash/foreach.c

@@ -38,10 +38,10 @@ void test_stash_foreach__cleanup(void)
 }
 
 static int callback_cb(
-		size_t index,
-		const char* message,
-		const git_oid *stash_oid,
-		void *payload)
+	size_t index,
+	const char* message,
+	const git_oid *stash_oid,
+	void *payload)
 {
 	struct callback_data *data = (struct callback_data *)payload;