[PATCH] shallow: use stat_validity to check for up-to-date file

[Jeff King <peff@peff.net>]

On Thu, Feb 27, 2014 at 05:18:58PM +0700, Duy Nguyen wrote:

> On Thu, Feb 27, 2014 at 4:22 PM, Jeff King <peff@peff.net> wrote:
> > On Thu, Feb 27, 2014 at 04:10:12AM -0500, Jeff King wrote:
> >
> >> I also notice that check_shallow_file_for_update returns early if
> >> !is_shallow. Is that safe? Is it possible for another process to have
> >> made us shallow since the program began? In that case, we would have to
> >> stat() the file always, then complain if it exists and !is_shallow.
> 
> I think it's safer to do it your way.

Yeah, I played around a bit and found that using "git fetch --depth" in
a non-shallow repo could run into this case.

> >         if (stat(git_path("shallow"), &st))
> >                 die("shallow file was removed during fetch");
> > +       else if (!is_shallow)
> > +               die("shallow file appeared during fetch");

Note that this is wrong; when the file is missing (the first part of the
conditional), we need to check "is_shallow" before dying. Otherwise we
erroneously complain when creating the file for the first time.

As I was fixing it, though, I recalled that we had to write a similar
system for the packed-refs file. Fortunately, it was easy to reuse, and
I ended up with the patch below.

-- >8 --
Subject: shallow: use stat_validity to check for up-to-date file

When we are about to write the shallow file, we check that
it has not changed since we last read it. Instead of
hand-rolling this, we can use stat_validity. This is built
around the index stat-check, so it is more robust than just
checking the mtime, as we do now (it uses the same check as
we do for index files).

The new code also handles the case of a shallow file
appearing unexpectedly. With the current code, two
simultaneous processes making us shallow (e.g., two "git
fetch --depth=1" running at the same time in a non-shallow
repository) can race to overwrite each other.

As a bonus, we also remove a race in determining the stat
information of what we read (we stat and then open, leaving
a race window; instead we should open and then fstat the
descriptor).

Signed-off-by: Jeff King <peff@peff.net>
---
 shallow.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/shallow.c b/shallow.c
index 75da07a..9668b39 100644
--- a/shallow.c
+++ b/shallow.c
@@ -10,7 +10,7 @@
 #include "commit-slab.h"
 
 static int is_shallow = -1;
-static struct stat shallow_stat;
+static struct stat_validity shallow_stat;
 static char *alternate_shallow_file;
 
 void set_alternate_shallow_file(const char *path, int override)
@@ -52,12 +52,12 @@ int is_repository_shallow(void)
 	 * shallow file should be used. We could just open it and it
 	 * will likely fail. But let's do an explicit check instead.
 	 */
-	if (!*path ||
-	    stat(path, &shallow_stat) ||
-	    (fp = fopen(path, "r")) == NULL) {
+	if (!*path || (fp = fopen(path, "r")) == NULL) {
+		stat_validity_clear(&shallow_stat);
 		is_shallow = 0;
 		return is_shallow;
 	}
+	stat_validity_update(&shallow_stat, fileno(fp));
 	is_shallow = 1;
 
 	while (fgets(buf, sizeof(buf), fp)) {
@@ -137,21 +137,11 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 
 void check_shallow_file_for_update(void)
 {
-	struct stat st;
-
-	if (!is_shallow)
-		return;
-	else if (is_shallow == -1)
+	if (is_shallow == -1)
 		die("BUG: shallow must be initialized by now");
 
-	if (stat(git_path("shallow"), &st))
-		die("shallow file was removed during fetch");
-	else if (st.st_mtime != shallow_stat.st_mtime
-#ifdef USE_NSEC
-		 || ST_MTIME_NSEC(st) != ST_MTIME_NSEC(shallow_stat)
-#endif
-		   )
-		die("shallow file was changed during fetch");
+	if (!stat_validity_check(&shallow_stat, git_path("shallow")))
+		die("shallow file has changed since we read it");
 }
 
 #define SEEN_ONLY 1
-- 
1.8.5.2.500.g8060133