Re: [PATCH] upload-pack: allow shallow fetching from a read-only repository

[Jeff King <peff@peff.net>]

On Thu, Feb 27, 2014 at 02:13:03PM +0700, Nguyễn Thái Ngọc Duy wrote:

> Before cdab485 (upload-pack: delegate rev walking in shallow fetch to
> pack-objects - 2013-08-16) upload-pack does not write to the source
> repository. cdab485 starts to write $GIT_DIR/shallow_XXXXXX if it's a
> shallow fetch, so the source repo must be writable.
> 
> Fall back to $TMPDIR if $GIT_DIR/shallow_XXXXXX cannot be created in
> this case. Note that in other cases that write $GIT_DIR/shallow_XXXXXX
> and eventually rename it to $GIT_DIR/shallow, there is no fallback to
> $TMPDIR.

That makes sense.

> @@ -224,7 +224,16 @@ char *setup_temporary_shallow(const struct sha1_array *extra)
>  	if (write_shallow_commits(&sb, 0, extra)) {
>  		struct strbuf path = STRBUF_INIT;
>  		strbuf_addstr(&path, git_path("shallow_XXXXXX"));
> -		fd = xmkstemp(path.buf);
> +		if (read_only) {
> +			fd = mkstemp(path.buf);
> +			if (fd < 0) {
> +				char buf[PATH_MAX];
> +				fd = git_mkstemp(buf, sizeof(buf), "shallow_XXXXXX");
> +			}
> +			if (fd < 0)
> +				die_errno("Unable to create temporary shallow file");
> +		} else
> +			fd = xmkstemp(path.buf);

This is not introduced by your patch, but I notice that we do not seem
to do anything with the tempfiles when the program dies prematurely.
We've started collecting stale shallow_XXXXXX files in our server repos.

For the writable cases, should we be using the lockfile API to take
shallow.lock? It looks like we do take a lock on it, but only after the
fetch, and then we have to do a manual check for it having moved (by the
way, shouldn't we do that check under the lock? I think
setup_alternate_shallow has a race condition).

I guess the reason to take the lock at the last minute is that the whole
fetch is heavyweight. But if the fetches are going to conflict, perhaps
it is better to have lock contention at the beginning, rather than
failing only at the end. I don't know very much about the shallow
system; does each operation rewrite the shallow file, or is it often
left untouched (in which case two simultaneous fetches could coexist
most of the time).

At any rate, if we used the lockfile API, it handles tempfile cleanup
automatically. Otherwise, I think we need something like the patch
below (in the interest of simplicity, I just drop all of the explicit
unlinks and let them use the atexit handler to clean up; you could also
add a function to explicitly unlink the tempfile and clear the handler).

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 85bba35..ac1d9b5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -833,8 +833,6 @@ static void execute_commands(struct command *commands,
 			error("BUG: run 'git fsck' for safety.\n"
 			      "If there are errors, try to remove "
 			      "the reported refs above");
-		if (alt_shallow_file && *alt_shallow_file)
-			unlink(alt_shallow_file);
 	}
 }
 
@@ -1087,10 +1085,6 @@ static void update_shallow_info(struct command *commands,
 			cmd->skip_update = 1;
 		}
 	}
-	if (alt_shallow_file && *alt_shallow_file) {
-		unlink(alt_shallow_file);
-		alt_shallow_file = NULL;
-	}
 	free(ref_status);
 }
 
diff --git a/fetch-pack.c b/fetch-pack.c
index 90fdd49..ae8550e 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -947,17 +947,6 @@ static void update_shallow(struct fetch_pack_args *args,
 	if (!si->shallow || !si->shallow->nr)
 		return;
 
-	if (alternate_shallow_file) {
-		/*
-		 * The temporary shallow file is only useful for
-		 * index-pack and unpack-objects because it may
-		 * contain more roots than we want. Delete it.
-		 */
-		if (*alternate_shallow_file)
-			unlink(alternate_shallow_file);
-		free((char *)alternate_shallow_file);
-	}
-
 	if (args->cloning) {
 		/*
 		 * remote is shallow, but this is a clone, there are
diff --git a/shallow.c b/shallow.c
index bbc98b5..0f2bb48 100644
--- a/shallow.c
+++ b/shallow.c
@@ -8,6 +8,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "commit-slab.h"
+#include "sigchain.h"
 
 static int is_shallow = -1;
 static struct stat shallow_stat;
@@ -216,27 +217,49 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 	return write_shallow_commits_1(out, use_pack_protocol, extra, 0);
 }
 
+static struct strbuf shallow_tmpfile = STRBUF_INIT;
+
+static void remove_shallow_tmpfile(void)
+{
+	if (shallow_tmpfile.len) {
+		unlink_or_warn(shallow_tmpfile.buf);
+		strbuf_reset(&shallow_tmpfile);
+	}
+}
+
+static void remove_shallow_tmpfile_on_signal(int signo)
+{
+	remove_shallow_tmpfile();
+	sigchain_pop(signo);
+	raise(signo);
+}
+
 char *setup_temporary_shallow(const struct sha1_array *extra)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int fd;
 
 	if (write_shallow_commits(&sb, 0, extra)) {
-		struct strbuf path = STRBUF_INIT;
-		strbuf_addstr(&path, git_path("shallow_XXXXXX"));
-		fd = xmkstemp(path.buf);
+		strbuf_addstr(&shallow_tmpfile, git_path("shallow_XXXXXX"));
+		fd = xmkstemp(shallow_tmpfile.buf);
+
+		/* XXX can there be multiple shallow tempfiles in one program?
+		 * In that case, we would need to maintain a list */
+		atexit(remove_shallow_tmpfile);
+		sigchain_push_common(remove_shallow_tmpfile_on_signal);
+
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
 			die_errno("failed to write to %s",
-				  path.buf);
+				  shallow_tmpfile.buf);
 		close(fd);
 		strbuf_release(&sb);
-		return strbuf_detach(&path, NULL);
+		return shallow_tmpfile.buf;
 	}
 	/*
 	 * is_repository_shallow() sees empty string as "no shallow
 	 * file".
 	 */
-	return xstrdup("");
+	return shallow_tmpfile.buf;
 }
 
 void setup_alternate_shallow(struct lock_file *shallow_lock,
diff --git a/upload-pack.c b/upload-pack.c
index 0c44f6b..55c1f71 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -242,11 +242,6 @@ static void create_pack_file(void)
 		error("git upload-pack: git-pack-objects died with error.");
 		goto fail;
 	}
-	if (shallow_file) {
-		if (*shallow_file)
-			unlink(shallow_file);
-		free(shallow_file);
-	}
 
 	/* flush the data */
 	if (0 <= buffered) {