rerere: error out on autoupdate failure

We have been silently tolerating errors by returning early with an error that the caller ignores since rerere.autoupdate was introduced in v1.6.0-rc0~120^2 (2008-06-22). So on error (for example if the index is already locked), rerere can return success silently without updating the index or with only some items in the index updated.

Better to treat such failures as a fatal error so the operator can figure out what is wrong and fix it.

---
I ran into this while auditing hold_locked_index callers. Tests still pass after this change. Sensible?

 rerere.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

index 1b0555f..195f663 100644
@@ -477,27 +477,23 @@ out:
 
 static struct lock_file index_lock;
 
-static int update_paths(struct string_list *update)
+static void update_paths(struct string_list *update)
 {
 	int i;
-	int fd = hold_locked_index(&index_lock, 0);
-	int status = 0;
 
-	if (fd < 0)
-		return -1;
+	hold_locked_index(&index_lock, 1);
 
 	for (i = 0; i < update->nr; i++) {
 		struct string_list_item *item = &update->items[i];
 		if (add_file_to_cache(item->string, ADD_CACHE_IGNORE_ERRORS))
-			status = -1;
+			die("staging updated %s failed", item->string);
 	}
 
-	if (!status && active_cache_changed) {
+	if (active_cache_changed) {
 		if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
 			die("Unable to write new index file");
-	} else if (fd >= 0)
+	} else
 		rollback_lock_file(&index_lock);
-	return status;
 }
 
 static int do_plain_rerere(struct string_list *rr, int fd)

I think what the change wants to do is sensible, but "tests still pass" probably does not tell us very much. You'd need a competing writers (e.g. in real life a human user may open two terminals and try to do things in the same repository from both terminals without realizing that she is stomping on herself), which is expected to break in unexpected ways ;-), which is not something the existing tests would try to catch anyway.

	if (add_file_to_cache(item->string, 0))
        	return -1;

We have been tolerating errors by returning early with an error that the caller ignores since rerere.autoupdate was introduced in v1.6.0-rc0~120^2 (2008-06-22). So on error (for example if the index is already locked), rerere can return success without updating the index or with only some items in the index updated.

Better to treat such failures as a fatal error so the operator can figure out what is wrong and fix it.

---

Good catch. It turns out that add_file_to_index does not even pay attention to the IGNORE_ERRORS flag. On error it unconditionally prints a message using 'return error()'.

builtin/add.c uses the IGNORE_ERRORS to flag to decide whether to avoid die()-ing on error (and it still lets add_file_to_index print the error to stderr in that case, so --ignore-errors is a bit of a misnomer).

	if (add_file_to_cache(dir->entries[i]->name, flags)) {
		if (!ignore_add_errors)
			die(_("adding files failed"));
		exit_status = 1;
	}

The message always says which file it failed to index, so I agree that it makes sense to simply exit(128) here.

 rerere.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

index 1b0555f..dbb2545 100644
@@ -477,27 +477,26 @@ out:
 
 static struct lock_file index_lock;
 
-static int update_paths(struct string_list *update)
+static void update_paths(struct string_list *update)
 {
 	int i;
-	int fd = hold_locked_index(&index_lock, 0);
-	int status = 0;
 
-	if (fd < 0)
-		return -1;
+	hold_locked_index(&index_lock, 1);
 
 	for (i = 0; i < update->nr; i++) {
-		struct string_list_item *item = &update->items[i];
-		if (add_file_to_cache(item->string, ADD_CACHE_IGNORE_ERRORS))
-			status = -1;
+		if (add_file_to_cache(update->items[i].string, 0))
+			/*
+			 * add_file_to_cache() printed an error
+			 * with the pathname, so we don't have to.
+			 */
+			exit(128);
 	}
 
-	if (!status && active_cache_changed) {
+	if (active_cache_changed) {
 		if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
 			die("Unable to write new index file");
-	} else if (fd >= 0)
+	} else
 		rollback_lock_file(&index_lock);
-	return status;
 }
 
 static int do_plain_rerere(struct string_list *rr, int fd)

Err, that should be exit(128) to mimic die().