Re: [PATCH] entry: check for fstat() errors after checkout

[Junio C Hamano <gitster@pobox.com>]

Matheus Tavares <matheus.bernardino@usp.br> writes:

> In 11179eb311 ("entry.c: check if file exists after checkout",
> 2017-10-05) we started checking the result of the lstat() call done
> after writing a file, to avoid writing garbage to the corresponding
> cache entry. However, the code skips calling lstat() if it's possible
> to use fstat() when it still has the file descriptor open. And when
> calling fstat() we don't do the same error checking. To fix that, let
> the callers of fstat_output() know when fstat() fails. In this case,
> write_entry() will try to use lstat() and properly report an error if
> that fails as well.

The original is not correct as you point out, as it loses the error
return from fstat(), but I do not think this is right, either.

The returned value from fstat_output() is suppsed to be "have we
done fstat() so that we do not need to do a lstat()?"  Don't you
instead want to extend it to "0 means we didn't, 1 means we did
successfully, and -1 means we did and failed"?  At least, the way
_this_ function is modified by this patch is in line with that.

Which means that we'd need to update the caller(s) to match, to
avoid risking this change to be just half a change, very similarly
to how the change in 11179eb311 was just half a change.

Perhaps like this?

 entry.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/entry.c b/entry.c
index 53380bb614..f48507ca42 100644
--- a/entry.c
+++ b/entry.c
@@ -108,14 +108,21 @@ static int open_output_fd(char *path, const struct cache_entry *ce, int to_tempf
 	}
 }
 
+/*
+ * We have an open fd to a file that we may use lstat() on later. 
+ * When able, try doing a fstat(fd) instead and tell the caller it
+ * does not have to do an extra lstat()
+ *
+ * Return 1 if we successfully ran fstat() and *st is valid.
+ * Return 0 if we did not do fstat() and the caller should do lstat().
+ * Return -1 if we got failure from fstat()---the caller can skip lstat().
+ */
 static int fstat_output(int fd, const struct checkout *state, struct stat *st)
 {
 	/* use fstat() only when path == ce->name */
 	if (fstat_is_reliable() &&
-	    state->refresh_cache && !state->base_dir_len) {
-		fstat(fd, st);
-		return 1;
-	}
+	    state->refresh_cache && !state->base_dir_len)
+		return (fstat(fd, st) < 0) ? -1 : 1;
 	return 0;
 }
 
@@ -369,10 +376,10 @@ static int write_entry(struct cache_entry *ce,
 finish:
 	if (state->refresh_cache) {
 		assert(state->istate);
-		if (!fstat_done)
-			if (lstat(ce->name, &st) < 0)
-				return error_errno("unable to stat just-written file %s",
-						   ce->name);
+		if (fstat_done < 0 ||
+		    (!fstat_done && lstat(ce->name, &st) < 0))
+			return error_errno("unable to stat just-written file %s",
+					   ce->name);
 		fill_stat_cache_info(state->istate, ce, &st);
 		ce->ce_flags |= CE_UPDATE_IN_BASE;
 		mark_fsmonitor_invalid(state->istate, ce);