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

> On Thu, Feb 27, 2014 at 4:22 PM, Jeff King <p...@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

Signed-off-by: Jeff King <p...@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)
-                  )
-               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

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to