index: modernize error handling of index_insert

The current error hanling of the function `index_insert` is currently
very fragile. Instead of erroring out in case an error has happened, it
will instead verify that no error has happened for each statement. This
makes adding new code to that function an adventurous task.

Improve the situation by converting the function to use our typical
`goto out` pattern.

Author: pks-t

src/index.c

@@ -1337,57 +1337,58 @@ static int index_insert(
 	bool trust_mode,
 	bool trust_id)
 {
-	int error = 0;
-	size_t path_length, position;
 	git_index_entry *existing, *best, *entry;
+	size_t path_length, position;
+	int error;
 
 	assert(index && entry_ptr);
 
 	entry = *entry_ptr;
 
-	/* make sure that the path length flag is correct */
+	/* Make sure that the path length flag is correct */
 	path_length = ((struct entry_internal *)entry)->pathlen;
 	index_entry_adjust_namemask(entry, path_length);
 
-	/* this entry is now up-to-date and should not be checked for raciness */
+	/* This entry is now up-to-date and should not be checked for raciness */
 	entry->flags_extended |= GIT_IDXENTRY_UPTODATE;
 
 	git_vector_sort(&index->entries);
 
-	/* look if an entry with this path already exists, either staged, or (if
+	/*
+	 * Look if an entry with this path already exists, either staged, or (if
 	 * this entry is a regular staged item) as the "ours" side of a conflict.
 	 */
 	index_existing_and_best(&existing, &position, &best, index, entry);
 
-	/* update the file mode */
+	/* Update the file mode */
 	entry->mode = trust_mode ?
 		git_index__create_mode(entry->mode) :
 		index_merge_mode(index, best, entry->mode);
 
-	/* canonicalize the directory name */
-	if (!trust_path)
-		error = canonicalize_directory_path(index, entry, best);
+	/* Canonicalize the directory name */
+	if (!trust_path && (error = canonicalize_directory_path(index, entry, best)) < 0)
+		goto out;
 
-	/* ensure that the given id exists (unless it's a submodule) */
-	if (!error && !trust_id && INDEX_OWNER(index) &&
-		(entry->mode & GIT_FILEMODE_COMMIT) != GIT_FILEMODE_COMMIT) {
+	/* Ensure that the given id exists (unless it's a submodule) */
+	if (!trust_id && INDEX_OWNER(index) &&
+	    (entry->mode & GIT_FILEMODE_COMMIT) != GIT_FILEMODE_COMMIT) {
 
 		if (!git_object__is_valid(INDEX_OWNER(index), &entry->id,
-			git_object__type_from_filemode(entry->mode)))
+					  git_object__type_from_filemode(entry->mode))) {
 			error = -1;
+			goto out;
+		}
 	}
 
-	/* look for tree / blob name collisions, removing conflicts if requested */
-	if (!error)
-		error = check_file_directory_collision(index, entry, position, replace);
-
-	if (error < 0)
-		/* skip changes */;
+	/* Look for tree / blob name collisions, removing conflicts if requested */
+	if ((error = check_file_directory_collision(index, entry, position, replace)) < 0)
+		goto out;
 
-	/* if we are replacing an existing item, overwrite the existing entry
+	/*
+	 * If we are replacing an existing item, overwrite the existing entry
 	 * and return it in place of the passed in one.
 	 */
-	else if (existing) {
+	if (existing) {
 		if (replace) {
 			index_entry_cpy(existing, entry);
 
@@ -1396,25 +1397,25 @@ static int index_insert(
 		}
 
 		index_entry_free(entry);
-		*entry_ptr = entry = existing;
-	}
-	else {
-		/* if replace is not requested or no existing entry exists, insert
+		*entry_ptr = existing;
+	} else {
+		/*
+		 * If replace is not requested or no existing entry exists, insert
 		 * at the sorted position.  (Since we re-sort after each insert to
 		 * check for dups, this is actually cheaper in the long run.)
 		 */
-		error = git_vector_insert_sorted(&index->entries, entry, index_no_dups);
+		if ((error = git_vector_insert_sorted(&index->entries, entry, index_no_dups)) < 0)
+			goto out;
 
-		if (error == 0) {
-			INSERT_IN_MAP(index, entry, &error);
-		}
+		INSERT_IN_MAP(index, entry, &error);
 	}
 
+	index->dirty = 1;
+
+out:
 	if (error < 0) {
 		index_entry_free(*entry_ptr);
 		*entry_ptr = NULL;
-	} else {
-		index->dirty = 1;
 	}
 
 	return error;