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

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

[Nguyễn Thái Ngọc Duy]

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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/receive-pack.c   |  2 +-
 commit.h                 |  2 +-
 fetch-pack.c             |  2 +-
 shallow.c                | 13 +++++++++++--
 t/t5537-fetch-shallow.sh | 13 +++++++++++++
 upload-pack.c            |  2 +-
 6 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 85bba35..9d96f2c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -936,7 +936,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 			ntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));
 
 	if (si->nr_ours || si->nr_theirs) {
-		alt_shallow_file = setup_temporary_shallow(si->shallow);
+		alt_shallow_file = setup_temporary_shallow(si->shallow, 0);
 		argv_array_pushl(&av, "--shallow-file", alt_shallow_file, NULL);
 	}
 
diff --git a/commit.h b/commit.h
index 16d9c43..44a40ad 100644
--- a/commit.h
+++ b/commit.h
@@ -209,7 +209,7 @@ extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 extern void setup_alternate_shallow(struct lock_file *shallow_lock,
 				    const char **alternate_shallow_file,
 				    const struct sha1_array *extra);
-extern char *setup_temporary_shallow(const struct sha1_array *extra);
+extern char *setup_temporary_shallow(const struct sha1_array *extra, int read_only);
 extern void advertise_shallow_grafts(int);
 
 struct shallow_info {
diff --git a/fetch-pack.c b/fetch-pack.c
index 90fdd49..382b825 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -853,7 +853,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
 					NULL);
 	else if (si->nr_ours || si->nr_theirs)
-		alternate_shallow_file = setup_temporary_shallow(si->shallow);
+		alternate_shallow_file = setup_temporary_shallow(si->shallow, 0);
 	else
 		alternate_shallow_file = NULL;
 	if (get_pack(args, fd, pack_lockfile))
diff --git a/shallow.c b/shallow.c
index bbc98b5..3adfbd5 100644
--- a/shallow.c
+++ b/shallow.c
@@ -216,7 +216,7 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 	return write_shallow_commits_1(out, use_pack_protocol, extra, 0);
 }
 
-char *setup_temporary_shallow(const struct sha1_array *extra)
+char *setup_temporary_shallow(const struct sha1_array *extra, int read_only)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int fd;
@@ -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);
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
 			die_errno("failed to write to %s",
 				  path.buf);
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index b0fa738..171db88 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -173,6 +173,19 @@ EOF
 	)
 '
 
+test_expect_success POSIXPERM 'shallow fetch from a read-only repo' '
+	cp -R .git read-only.git &&
+	find read-only.git -print | xargs chmod -w &&
+	test_when_finished "find read-only.git -type d -print | xargs chmod +w" &&
+	git clone --no-local --depth=2 read-only.git from-read-only &&
+	git --git-dir=from-read-only/.git log --format=%s >actual &&
+	cat >expect <<EOF &&
+add-1-back
+4
+EOF
+	test_cmp expect actual
+'
+
 if test -n "$NO_CURL" -o -z "$GIT_TEST_HTTPD"; then
 	say 'skipping remaining tests, git built without http support'
 	test_done
diff --git a/upload-pack.c b/upload-pack.c
index 0c44f6b..11404b4 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -84,7 +84,7 @@ static void create_pack_file(void)
 	char *shallow_file = NULL;
 
 	if (shallow_nr) {
-		shallow_file = setup_temporary_shallow(NULL);
+		shallow_file = setup_temporary_shallow(NULL, 1);
 		argv[arg++] = "--shallow-file";
 		argv[arg++] = shallow_file;
 	}
-- 
1.9.0.66.g14f785a

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

[Jeff King]

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) {

[PATCH] shallow: verify shallow file after taking lock

[Jeff King]

Before writing the shallow file, we stat() the existing file
to make sure it has not been updated since our operation
began. However, we do not do so under a lock, so there is a
possible race:

  1. Process A takes the lock.

  2. Process B calls check_shallow_file_for_update and finds
     no update.

  3. Process A commits the lockfile.

  4. Process B takes the lock, then overwrite's process A's
     changes.

We can fix this by doing our check while we hold the lock.

Signed-off-by: Jeff King <peff@peff.net>
---
On Thu, Feb 27, 2014 at 04:04:26AM -0500, Jeff King wrote:

> by the way, shouldn't we do that check under the lock? I think
> setup_alternate_shallow has a race condition).

Here is the fix. I didn't actually reproduce the race experimentally,
but it seems pretty straightforward.

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.

 shallow.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/shallow.c b/shallow.c
index bbc98b5..75da07a 100644
--- a/shallow.c
+++ b/shallow.c
@@ -246,9 +246,9 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
 	struct strbuf sb = STRBUF_INIT;
 	int fd;
 
-	check_shallow_file_for_update();
 	fd = hold_lock_file_for_update(shallow_lock, git_path("shallow"),
 				       LOCK_DIE_ON_ERROR);
+	check_shallow_file_for_update();
 	if (write_shallow_commits(&sb, 0, extra)) {
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
 			die_errno("failed to write to %s",
@@ -293,9 +293,9 @@ void prune_shallow(int show_only)
 		strbuf_release(&sb);
 		return;
 	}
-	check_shallow_file_for_update();
 	fd = hold_lock_file_for_update(&shallow_lock, git_path("shallow"),
 				       LOCK_DIE_ON_ERROR);
+	check_shallow_file_for_update();
 	if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
 			die_errno("failed to write to %s",
-- 
1.8.5.2.500.g8060133

Re: [PATCH] shallow: verify shallow file after taking lock

[Jeff King]

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.

That patch would look like this:

diff --git a/shallow.c b/shallow.c
index 75da07a..e05a241 100644
--- a/shallow.c
+++ b/shallow.c
@@ -139,13 +139,13 @@ 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 (!is_shallow)
+		die("shallow file appeared during fetch");
 	else if (st.st_mtime != shallow_stat.st_mtime
 #ifdef USE_NSEC
 		 || ST_MTIME_NSEC(st) != ST_MTIME_NSEC(shallow_stat)

but again, I'm not really clear on whether this is possible.

-Peff

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

[Duy Nguyen]

On Thu, Feb 27, 2014 at 4:04 PM, Jeff King <peff@peff.net> wrote:
> 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).

For writable cases (fetch-pack and receive-pack), yes I think locking
earlier is better or multiple fetches/pushes will race to update
shallow file. Chances of racing fetches are practically near zero, I
think. We need to do something about push.

We only update shallow file in these cases: clone --depth, fetch
--update-shallow, fetch --depth, and push when receive.shallowupdate
is set. All of them may end up not updating shallow file though. The
last case is the most troublesome because receive.shallowupdate is set
at server side and we don't want any shallow push to block all other
shallow pushes..

For read-only case in upload-file, locking only reduces the
availability of clone/fetch.

> 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).

Looks like a good thing to do anyway.

>
> 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) {



-- 
Duy

Re: [PATCH] shallow: verify shallow file after taking lock

[Duy Nguyen]

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.

>
> That patch would look like this:
>
> diff --git a/shallow.c b/shallow.c
> index 75da07a..e05a241 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -139,13 +139,13 @@ 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 (!is_shallow)
> +               die("shallow file appeared during fetch");
>         else if (st.st_mtime != shallow_stat.st_mtime
>  #ifdef USE_NSEC
>                  || ST_MTIME_NSEC(st) != ST_MTIME_NSEC(shallow_stat)
>
> but again, I'm not really clear on whether this is possible.
>
> -Peff



-- 
Duy

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

[Jeff King]

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

[PATCH] shallow: automatically clean up shallow tempfiles

[Jeff King]

On Thu, Feb 27, 2014 at 05:11:41PM +0700, Duy Nguyen wrote:

> We only update shallow file in these cases: clone --depth, fetch
> --update-shallow, fetch --depth, and push when receive.shallowupdate
> is set. All of them may end up not updating shallow file though.

OK, that last sentence is what I wasn't sure of. If they might sometimes
not update the shallow file, then I think locking early is wrong. It
means we create lock contention where it might not exist.

> For read-only case in upload-file, locking only reduces the
> availability of clone/fetch.

Right, those should never lock. So even if we did want to tweak the
locking, we would still need a separate tempfile cleanup for those.

Here it is as a completed patch. It conflicts textually but not
semantically with the patch that started this thread (I think one of my
earlier patches has a minor textual conflict, too). Should we roll them
into a single series to help Junio? If so, do you want to do it, or
should I?

-- >8 --
Subject: shallow: automatically clean up shallow tempfiles

We sometimes write tempfiles of the form "shallow_XXXXXX"
during fetch/push operations with shallow repositories.
Under normal circumstances, we clean up the result when we
are done. However, we do no take steps to clean up after
ourselves when we exit due to die() or signal death.

This patch teaches the tempfile creation code to register
handlers to clean up after ourselves. To handle this, we
change the ownership semantics of the filename returned by
setup_temporary_shallow. It now keeps a copy of the filename
itself, and returns only a const pointer to it.

We can also do away with explicit tempfile removal in the
callers. They all exit not long after finishing with the
file, so they can rely on the auto-cleanup, simplifying the
code.

Note that we keep things simple and maintain only a single
filename to be cleaned. This is sufficient for the current
caller, but we future-proof it with a die("BUG").

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/receive-pack.c | 16 ++++------------
 commit.h               |  2 +-
 fetch-pack.c           | 11 -----------
 shallow.c              | 41 ++++++++++++++++++++++++++++++++++-------
 upload-pack.c          |  7 +------
 5 files changed, 40 insertions(+), 37 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 85bba35..c323081 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -828,14 +828,10 @@ static void execute_commands(struct command *commands,
 		}
 	}
 
-	if (shallow_update) {
-		if (!checked_connectivity)
-			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);
-	}
+	if (shallow_update && !checked_connectivity)
+		error("BUG: run 'git fsck' for safety.\n"
+		      "If there are errors, try to remove "
+		      "the reported refs above");
 }
 
 static struct command *read_head_info(struct sha1_array *shallow)
@@ -1087,10 +1083,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/commit.h b/commit.h
index 16d9c43..55631f1 100644
--- a/commit.h
+++ b/commit.h
@@ -209,7 +209,7 @@ extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 extern void setup_alternate_shallow(struct lock_file *shallow_lock,
 				    const char **alternate_shallow_file,
 				    const struct sha1_array *extra);
-extern char *setup_temporary_shallow(const struct sha1_array *extra);
+extern const char *setup_temporary_shallow(const struct sha1_array *extra);
 extern void advertise_shallow_grafts(int);
 
 struct shallow_info {
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 9668b39..0b267b6 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_validity shallow_stat;
@@ -206,27 +207,53 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 	return write_shallow_commits_1(out, use_pack_protocol, extra, 0);
 }
 
-char *setup_temporary_shallow(const struct sha1_array *extra)
+static struct strbuf temporary_shallow = STRBUF_INIT;
+
+static void remove_temporary_shallow(void)
+{
+	if (temporary_shallow.len) {
+		unlink_or_warn(temporary_shallow.buf);
+		strbuf_reset(&temporary_shallow);
+	}
+}
+
+static void remove_temporary_shallow_on_signal(int signo)
+{
+	remove_temporary_shallow();
+	sigchain_pop(signo);
+	raise(signo);
+}
+
+const char *setup_temporary_shallow(const struct sha1_array *extra)
 {
+	static int installed_handler;
 	struct strbuf sb = STRBUF_INIT;
 	int fd;
 
+	if (temporary_shallow.len)
+		die("BUG: attempt to create two temporary shallow files");
+
 	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(&temporary_shallow, git_path("shallow_XXXXXX"));
+		fd = xmkstemp(temporary_shallow.buf);
+
+		if (!installed_handler) {
+			atexit(remove_temporary_shallow);
+			sigchain_push_common(remove_temporary_shallow_on_signal);
+		}
+
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
 			die_errno("failed to write to %s",
-				  path.buf);
+				  temporary_shallow.buf);
 		close(fd);
 		strbuf_release(&sb);
-		return strbuf_detach(&path, NULL);
+		return temporary_shallow.buf;
 	}
 	/*
 	 * is_repository_shallow() sees empty string as "no shallow
 	 * file".
 	 */
-	return xstrdup("");
+	return temporary_shallow.buf;
 }
 
 void setup_alternate_shallow(struct lock_file *shallow_lock,
diff --git a/upload-pack.c b/upload-pack.c
index 0c44f6b..a3c52f6 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -81,7 +81,7 @@ static void create_pack_file(void)
 	const char *argv[12];
 	int i, arg = 0;
 	FILE *pipe_fd;
-	char *shallow_file = NULL;
+	const char *shallow_file = NULL;
 
 	if (shallow_nr) {
 		shallow_file = setup_temporary_shallow(NULL);
@@ -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) {
-- 
1.8.5.2.500.g8060133

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

[Nguyễn Thái Ngọc Duy]

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.

git:// servers do not need write access to repos and usually don't,
which mean cdab485 breaks shallow clone over git://

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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Rebased on top of jk/shallow-update-fix

 builtin/receive-pack.c   |  2 +-
 commit.h                 |  2 +-
 fetch-pack.c             |  2 +-
 shallow.c                | 13 +++++++++++--
 t/t5537-fetch-shallow.sh | 13 +++++++++++++
 upload-pack.c            |  2 +-
 6 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..d723099 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -932,7 +932,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 			ntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));
 
 	if (si->nr_ours || si->nr_theirs) {
-		alt_shallow_file = setup_temporary_shallow(si->shallow);
+		alt_shallow_file = setup_temporary_shallow(si->shallow, 0);
 		argv_array_pushl(&av, "--shallow-file", alt_shallow_file, NULL);
 	}
 
diff --git a/commit.h b/commit.h
index 55631f1..d38e996 100644
--- a/commit.h
+++ b/commit.h
@@ -209,7 +209,7 @@ extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 extern void setup_alternate_shallow(struct lock_file *shallow_lock,
 				    const char **alternate_shallow_file,
 				    const struct sha1_array *extra);
-extern const char *setup_temporary_shallow(const struct sha1_array *extra);
+extern const char *setup_temporary_shallow(const struct sha1_array *extra, int read_only);
 extern void advertise_shallow_grafts(int);
 
 struct shallow_info {
diff --git a/fetch-pack.c b/fetch-pack.c
index ae8550e..b71d186 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -853,7 +853,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
 					NULL);
 	else if (si->nr_ours || si->nr_theirs)
-		alternate_shallow_file = setup_temporary_shallow(si->shallow);
+		alternate_shallow_file = setup_temporary_shallow(si->shallow, 0);
 	else
 		alternate_shallow_file = NULL;
 	if (get_pack(args, fd, pack_lockfile))
diff --git a/shallow.c b/shallow.c
index c7602ce..ad28af6 100644
--- a/shallow.c
+++ b/shallow.c
@@ -224,7 +224,8 @@ static void remove_temporary_shallow_on_signal(int signo)
 	raise(signo);
 }
 
-const char *setup_temporary_shallow(const struct sha1_array *extra)
+const char *setup_temporary_shallow(const struct sha1_array *extra,
+				    int read_only)
 {
 	static int installed_handler;
 	struct strbuf sb = STRBUF_INIT;
@@ -235,7 +236,15 @@ const char *setup_temporary_shallow(const struct sha1_array *extra)
 
 	if (write_shallow_commits(&sb, 0, extra)) {
 		strbuf_addstr(&temporary_shallow, git_path("shallow_XXXXXX"));
-		fd = xmkstemp(temporary_shallow.buf);
+		fd = mkstemp(temporary_shallow.buf);
+		if (read_only && fd < 0) {
+			strbuf_grow(&temporary_shallow, PATH_MAX);
+			fd = git_mkstemp(temporary_shallow.buf, PATH_MAX,
+					 "shallow_XXXXXX");
+		}
+		if (fd < 0)
+			die_errno("Unable to create temporary file '%s'",
+				  temporary_shallow.buf);
 
 		if (!installed_handler) {
 			atexit(remove_temporary_shallow);
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index b0fa738..171db88 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -173,6 +173,19 @@ EOF
 	)
 '
 
+test_expect_success POSIXPERM 'shallow fetch from a read-only repo' '
+	cp -R .git read-only.git &&
+	find read-only.git -print | xargs chmod -w &&
+	test_when_finished "find read-only.git -type d -print | xargs chmod +w" &&
+	git clone --no-local --depth=2 read-only.git from-read-only &&
+	git --git-dir=from-read-only/.git log --format=%s >actual &&
+	cat >expect <<EOF &&
+add-1-back
+4
+EOF
+	test_cmp expect actual
+'
+
 if test -n "$NO_CURL" -o -z "$GIT_TEST_HTTPD"; then
 	say 'skipping remaining tests, git built without http support'
 	test_done
diff --git a/upload-pack.c b/upload-pack.c
index a3c52f6..b538f32 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -84,7 +84,7 @@ static void create_pack_file(void)
 	const char *shallow_file = NULL;
 
 	if (shallow_nr) {
-		shallow_file = setup_temporary_shallow(NULL);
+		shallow_file = setup_temporary_shallow(NULL, 1);
 		argv[arg++] = "--shallow-file";
 		argv[arg++] = shallow_file;
 	}
-- 
1.9.0.40.gaa8c3ea

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> 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.
>
> git:// servers do not need write access to repos and usually don't,
> which mean cdab485 breaks shallow clone over git://
>
> 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.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Rebased on top of jk/shallow-update-fix

Hmph.

I notice that the original code, with or without this change, allows
upload-pack spawned by daemon to attempt to write into GIT_DIR.
As upload-pack is supposed to be a read-only operation, this is
quite bad.

Perhaps we should give server operators an option to run their
daemon -> upload-pack chain to always write to a throw-away
directory of their choice, without ever attempting to write to
GIT_DIR it serves?

How well is the access to the temporary shallow file controlled in
your code (sorry, but I do not recall carefully reading your patch
that added the mechanism with security issues in mind, so now I am
asking)?  When it is redirected to TMPDIR (let's forget GIT_DIR for
now---if an attacker can write into there, the repository is already
lost), can an attacker race with us to cause us to overwrite we do
not expect to?

Even if it turns out that this patch is secure enough as-is, we
definitely need to make sure that server operators, who want to keep
their upload-pack truly a read-only operation, know that it is
necessary to (1) keep the system user they run git-daemon under
incapable of writing into GIT_DIR, and (2) make sure TMPDIR points
at somewhere only git-daemon user and nobody else can write into,
somewhere in the documentation.

> diff --git a/fetch-pack.c b/fetch-pack.c
> index ae8550e..b71d186 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -853,7 +853,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>  		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
>  					NULL);
>  	else if (si->nr_ours || si->nr_theirs)
> -		alternate_shallow_file = setup_temporary_shallow(si->shallow);
> +		alternate_shallow_file = setup_temporary_shallow(si->shallow, 0);
>  	else
>  		alternate_shallow_file = NULL;
>  	if (get_pack(args, fd, pack_lockfile))
> diff --git a/shallow.c b/shallow.c
> index c7602ce..ad28af6 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -224,7 +224,8 @@ static void remove_temporary_shallow_on_signal(int signo)
>  	raise(signo);
>  }
>  
> -const char *setup_temporary_shallow(const struct sha1_array *extra)
> +const char *setup_temporary_shallow(const struct sha1_array *extra,
> +				    int read_only)
>  {
>  	static int installed_handler;
>  	struct strbuf sb = STRBUF_INIT;
> @@ -235,7 +236,15 @@ const char *setup_temporary_shallow(const struct sha1_array *extra)
>  
>  	if (write_shallow_commits(&sb, 0, extra)) {
>  		strbuf_addstr(&temporary_shallow, git_path("shallow_XXXXXX"));
> -		fd = xmkstemp(temporary_shallow.buf);
> +		fd = mkstemp(temporary_shallow.buf);
> +		if (read_only && fd < 0) {
> +			strbuf_grow(&temporary_shallow, PATH_MAX);
> +			fd = git_mkstemp(temporary_shallow.buf, PATH_MAX,
> +					 "shallow_XXXXXX");
> +		}
> +		if (fd < 0)
> +			die_errno("Unable to create temporary file '%s'",
> +				  temporary_shallow.buf);
>  
>  		if (!installed_handler) {
>  			atexit(remove_temporary_shallow);
> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
> index b0fa738..171db88 100755
> --- a/t/t5537-fetch-shallow.sh
> +++ b/t/t5537-fetch-shallow.sh
> @@ -173,6 +173,19 @@ EOF
>  	)
>  '
>  
> +test_expect_success POSIXPERM 'shallow fetch from a read-only repo' '

s/POSIXPERM/&,SANITY/, perhaps?

Thinking of it again, perhaps POSIXPERM should imply SANITY is required?

> +	cp -R .git read-only.git &&
> +	find read-only.git -print | xargs chmod -w &&
> +	test_when_finished "find read-only.git -type d -print | xargs chmod +w" &&
> +	git clone --no-local --depth=2 read-only.git from-read-only &&
> +	git --git-dir=from-read-only/.git log --format=%s >actual &&
> +	cat >expect <<EOF &&
> +add-1-back
> +4
> +EOF
> +	test_cmp expect actual
> +'
> +
>  if test -n "$NO_CURL" -o -z "$GIT_TEST_HTTPD"; then
>  	say 'skipping remaining tests, git built without http support'
>  	test_done
> diff --git a/upload-pack.c b/upload-pack.c
> index a3c52f6..b538f32 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -84,7 +84,7 @@ static void create_pack_file(void)
>  	const char *shallow_file = NULL;
>  
>  	if (shallow_nr) {
> -		shallow_file = setup_temporary_shallow(NULL);
> +		shallow_file = setup_temporary_shallow(NULL, 1);
>  		argv[arg++] = "--shallow-file";
>  		argv[arg++] = shallow_file;
>  	}

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

[Duy Nguyen]

On Wed, Mar 5, 2014 at 1:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I notice that the original code, with or without this change, allows
> upload-pack spawned by daemon to attempt to write into GIT_DIR.
> As upload-pack is supposed to be a read-only operation, this is
> quite bad.
>
> Perhaps we should give server operators an option to run their
> daemon -> upload-pack chain to always write to a throw-away
> directory of their choice, without ever attempting to write to
> GIT_DIR it serves?

That would be setting TMPDIR before running git-daemon, I think.
Except places that we ignore TMPDIR like this one.

> How well is the access to the temporary shallow file controlled in
> your code (sorry, but I do not recall carefully reading your patch
> that added the mechanism with security issues in mind, so now I am
> asking)?  When it is redirected to TMPDIR (let's forget GIT_DIR for
> now---if an attacker can write into there, the repository is already
> lost), can an attacker race with us to cause us to overwrite we do
> not expect to?

I'm sorry to say that attackers were simply not a concern when I wrote
the patch. Not even that upload-pack is a read-only operation (so
obvious now that I think about this). I think racing is possible, yes.

> Even if it turns out that this patch is secure enough as-is, we
> definitely need to make sure that server operators, who want to keep
> their upload-pack truly a read-only operation, know that it is
> necessary to (1) keep the system user they run git-daemon under
> incapable of writing into GIT_DIR, and (2) make sure TMPDIR points
> at somewhere only git-daemon user and nobody else can write into,
> somewhere in the documentation.

If only there is a way to pass this info without a temporary
file. Multiplexing it to pack-objects' stdin should work. It may be
ugly, but it's probably the safest way.

Wait it does not look that ugly. We can feed "--shallow <SHA1>" lines
before sending want/have/edge objects. Something like this seems to
work (just ran a few shallow-related tests, not the whole test suite)

-- 8< --
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..130097c 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2467,6 +2467,14 @@ static void get_object_list(int ac, const char **av)
 				write_bitmap_index = 0;
 				continue;
 			}
+			if (starts_with(line, "--shallow ")) {
+				unsigned char sha1[20];
+				if (get_sha1_hex(line + 10, sha1))
+					die("not an SHA-1 '%s'", line + 10);
+				register_shallow(sha1);
+				/* XXX: set shallow.c:is_shallow = 1 ? */
+				continue;
+			}
 			die("not a rev '%s'", line);
 		}
 		if (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))
diff --git a/upload-pack.c b/upload-pack.c
index 0c44f6b..a5c50e4 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -70,6 +70,14 @@ static ssize_t send_client_data(int fd, const char *data, ssize_t sz)
 	return sz;
 }
 
+static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
+{
+	FILE *fp = cb_data;
+	if (graft->nr_parent == -1)
+		fprintf(fp, "--shallow %s\n", sha1_to_hex(graft->sha1));
+	return 0;
+}
+
 static void create_pack_file(void)
 {
 	struct child_process pack_objects;
@@ -81,12 +89,10 @@ static void create_pack_file(void)
 	const char *argv[12];
 	int i, arg = 0;
 	FILE *pipe_fd;
-	char *shallow_file = NULL;
 
 	if (shallow_nr) {
-		shallow_file = setup_temporary_shallow(NULL);
 		argv[arg++] = "--shallow-file";
-		argv[arg++] = shallow_file;
+		argv[arg++] = "";
 	}
 	argv[arg++] = "pack-objects";
 	argv[arg++] = "--revs";
@@ -114,6 +120,9 @@ static void create_pack_file(void)
 
 	pipe_fd = xfdopen(pack_objects.in, "w");
 
+	if (shallow_nr)
+		for_each_commit_graft(write_one_shallow, pipe_fd);
+
 	for (i = 0; i < want_obj.nr; i++)
 		fprintf(pipe_fd, "%s\n",
 			sha1_to_hex(want_obj.objects[i].item->sha1));
@@ -242,12 +251,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) {
 		data[0] = buffered;
-- 8< --
-- 
Duy

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> If only there is a way to pass this info without a temporary
> file. Multiplexing it to pack-objects' stdin should work. It may be
> ugly, but it's probably the safest way.
>
> Wait it does not look that ugly. We can feed "--shallow <SHA1>" lines
> before sending want/have/edge objects. Something like this seems to
> work (just ran a few shallow-related tests, not the whole test suite)

Sounds like it is a much better approach to the issue.

[PATCH v3] upload-pack: send shallow info over stdin to pack-objects

[Nguyễn Thái Ngọc Duy]

From: Duy Nguyen <pclouds@gmail.com>

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.

git:// servers do not need write access to repos and usually don't
have it, which means cdab485 breaks shallow clone over git://

Instead of using a temporary file as the media for shallow points, we
can send them over stdin to pack-objects as well. Prepend shallow
SHA-1 with --shallow so pack-objects knows what is
what.

read_object_list_from_stdin() does not accept --shallow lines because
upload-pack never uses that code. When upload-pack does, then it can
learn about --shallow lines.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 OK new approach, stop creating shallow_XXXXXX in upload-pack.

 builtin/pack-objects.c   |  7 +++++++
 shallow.c                |  2 ++
 t/t5537-fetch-shallow.sh | 13 +++++++++++++
 upload-pack.c            | 21 ++++++++++++---------
 4 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..79e848e 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2467,6 +2467,13 @@ static void get_object_list(int ac, const char **av)
 				write_bitmap_index = 0;
 				continue;
 			}
+			if (starts_with(line, "--shallow ")) {
+				unsigned char sha1[20];
+				if (get_sha1_hex(line + 10, sha1))
+					die("not an SHA-1 '%s'", line + 10);
+				register_shallow(sha1);
+				continue;
+			}
 			die("not a rev '%s'", line);
 		}
 		if (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))
diff --git a/shallow.c b/shallow.c
index bbc98b5..41ff4a0 100644
--- a/shallow.c
+++ b/shallow.c
@@ -33,6 +33,8 @@ int register_shallow(const unsigned char *sha1)
 	graft->nr_parent = -1;
 	if (commit && commit->object.parsed)
 		commit->parents = NULL;
+	if (is_shallow == -1)
+		is_shallow = 1;
 	return register_commit_graft(graft, 0);
 }
 
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 3ae9092..a980574 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -173,4 +173,17 @@ EOF
 	)
 '
 
+test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '
+	cp -R .git read-only.git &&
+	find read-only.git -print | xargs chmod -w &&
+	test_when_finished "find read-only.git -type d -print | xargs chmod +w" &&
+	git clone --no-local --depth=2 read-only.git from-read-only &&
+	git --git-dir=from-read-only/.git log --format=%s >actual &&
+	cat >expect <<EOF &&
+add-1-back
+4
+EOF
+	test_cmp expect actual
+'
+
 test_done
diff --git a/upload-pack.c b/upload-pack.c
index 0c44f6b..a5c50e4 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -70,6 +70,14 @@ static ssize_t send_client_data(int fd, const char *data, ssize_t sz)
 	return sz;
 }
 
+static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
+{
+	FILE *fp = cb_data;
+	if (graft->nr_parent == -1)
+		fprintf(fp, "--shallow %s\n", sha1_to_hex(graft->sha1));
+	return 0;
+}
+
 static void create_pack_file(void)
 {
 	struct child_process pack_objects;
@@ -81,12 +89,10 @@ static void create_pack_file(void)
 	const char *argv[12];
 	int i, arg = 0;
 	FILE *pipe_fd;
-	char *shallow_file = NULL;
 
 	if (shallow_nr) {
-		shallow_file = setup_temporary_shallow(NULL);
 		argv[arg++] = "--shallow-file";
-		argv[arg++] = shallow_file;
+		argv[arg++] = "";
 	}
 	argv[arg++] = "pack-objects";
 	argv[arg++] = "--revs";
@@ -114,6 +120,9 @@ static void create_pack_file(void)
 
 	pipe_fd = xfdopen(pack_objects.in, "w");
 
+	if (shallow_nr)
+		for_each_commit_graft(write_one_shallow, pipe_fd);
+
 	for (i = 0; i < want_obj.nr; i++)
 		fprintf(pipe_fd, "%s\n",
 			sha1_to_hex(want_obj.objects[i].item->sha1));
@@ -242,12 +251,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) {
 		data[0] = buffered;
-- 
1.9.0.66.g14f785a

Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> From: Duy Nguyen <pclouds@gmail.com>
>
> 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.
>
> git:// servers do not need write access to repos and usually don't
> have it, which means cdab485 breaks shallow clone over git://
>
> Instead of using a temporary file as the media for shallow points, we
> can send them over stdin to pack-objects as well. Prepend shallow
> SHA-1 with --shallow so pack-objects knows what is
> what.
>
> read_object_list_from_stdin() does not accept --shallow lines because
> upload-pack never uses that code. When upload-pack does, then it can
> learn about --shallow lines.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  OK new approach, stop creating shallow_XXXXXX in upload-pack.

Thanks.

I like what I see in this patch, but I wonder if we can essentially
revert that "temporary shallow file" patch and replace it with the
same (or a similar) mechanism uniformly?

On the receive-pack side, the comment at the bottom of
preprare_shallow_update() makes it clear that, if we wanted to use
hooks, we cannot avoid having the proposed new shallow-file in a
temporary file, which is unfortunate.  Do we have a similar issue on
hooks on the upload-pack side?


>  builtin/pack-objects.c   |  7 +++++++
>  shallow.c                |  2 ++
>  t/t5537-fetch-shallow.sh | 13 +++++++++++++
>  upload-pack.c            | 21 ++++++++++++---------
>  4 files changed, 34 insertions(+), 9 deletions(-)

Nothing for Documentation/ anywhere?

> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index c733379..79e848e 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -2467,6 +2467,13 @@ static void get_object_list(int ac, const char **av)
>  				write_bitmap_index = 0;
>  				continue;
>  			}
> +			if (starts_with(line, "--shallow ")) {
> +				unsigned char sha1[20];
> +				if (get_sha1_hex(line + 10, sha1))
> +					die("not an SHA-1 '%s'", line + 10);
> +				register_shallow(sha1);
> +				continue;
> +			}
>  			die("not a rev '%s'", line);
>  		}
>  		if (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))
> diff --git a/shallow.c b/shallow.c
> index bbc98b5..41ff4a0 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -33,6 +33,8 @@ int register_shallow(const unsigned char *sha1)
>  	graft->nr_parent = -1;
>  	if (commit && commit->object.parsed)
>  		commit->parents = NULL;
> +	if (is_shallow == -1)
> +		is_shallow = 1;
>  	return register_commit_graft(graft, 0);
>  }
>  
> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
> index 3ae9092..a980574 100755
> --- a/t/t5537-fetch-shallow.sh
> +++ b/t/t5537-fetch-shallow.sh
> @@ -173,4 +173,17 @@ EOF
>  	)
>  '
>  
> +test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '
> +	cp -R .git read-only.git &&
> +	find read-only.git -print | xargs chmod -w &&
> +	test_when_finished "find read-only.git -type d -print | xargs chmod +w" &&
> +	git clone --no-local --depth=2 read-only.git from-read-only &&
> +	git --git-dir=from-read-only/.git log --format=%s >actual &&
> +	cat >expect <<EOF &&
> +add-1-back
> +4
> +EOF
> +	test_cmp expect actual
> +'
> +
>  test_done
> diff --git a/upload-pack.c b/upload-pack.c
> index 0c44f6b..a5c50e4 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -70,6 +70,14 @@ static ssize_t send_client_data(int fd, const char *data, ssize_t sz)
>  	return sz;
>  }
>  
> +static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
> +{
> +	FILE *fp = cb_data;
> +	if (graft->nr_parent == -1)
> +		fprintf(fp, "--shallow %s\n", sha1_to_hex(graft->sha1));
> +	return 0;
> +}
> +
>  static void create_pack_file(void)
>  {
>  	struct child_process pack_objects;
> @@ -81,12 +89,10 @@ static void create_pack_file(void)
>  	const char *argv[12];
>  	int i, arg = 0;
>  	FILE *pipe_fd;
> -	char *shallow_file = NULL;
>  
>  	if (shallow_nr) {
> -		shallow_file = setup_temporary_shallow(NULL);
>  		argv[arg++] = "--shallow-file";
> -		argv[arg++] = shallow_file;
> +		argv[arg++] = "";
>  	}
>  	argv[arg++] = "pack-objects";
>  	argv[arg++] = "--revs";
> @@ -114,6 +120,9 @@ static void create_pack_file(void)
>  
>  	pipe_fd = xfdopen(pack_objects.in, "w");
>  
> +	if (shallow_nr)
> +		for_each_commit_graft(write_one_shallow, pipe_fd);
> +
>  	for (i = 0; i < want_obj.nr; i++)
>  		fprintf(pipe_fd, "%s\n",
>  			sha1_to_hex(want_obj.objects[i].item->sha1));
> @@ -242,12 +251,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) {
>  		data[0] = buffered;

Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects

[Duy Nguyen]

On Fri, Mar 7, 2014 at 1:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I like what I see in this patch, but I wonder if we can essentially
> revert that "temporary shallow file" patch and replace it with the
> same (or a similar) mechanism uniformly?

Using --shallow-file is uniform.The only downside is temporary file.
Without it you'll need to think of a way (probably different way for
each command) to feed shallow info to.

> On the receive-pack side, the comment at the bottom of
> preprare_shallow_update() makes it clear that, if we wanted to use
> hooks, we cannot avoid having the proposed new shallow-file in a
> temporary file, which is unfortunate.  Do we have a similar issue on
> hooks on the upload-pack side?

No. I don't think we have hooks on upload-pack.

>>  builtin/pack-objects.c   |  7 +++++++
>>  shallow.c                |  2 ++
>>  t/t5537-fetch-shallow.sh | 13 +++++++++++++
>>  upload-pack.c            | 21 ++++++++++++---------
>>  4 files changed, 34 insertions(+), 9 deletions(-)
>
> Nothing for Documentation/ anywhere?

Heh git-pack-objects.txt never described stdin format. At least I
searched for --not in it and found none. So I gladly accepted the
situation and skipped doc update :D
-- 
Duy

Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects

[Duy Nguyen]

On Thu, Mar 6, 2014 at 3:49 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
> index 3ae9092..a980574 100755
> --- a/t/t5537-fetch-shallow.sh
> +++ b/t/t5537-fetch-shallow.sh
> @@ -173,4 +173,17 @@ EOF
>         )
>  '
>
> +test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '
> +       cp -R .git read-only.git &&
> +       find read-only.git -print | xargs chmod -w &&
> +       test_when_finished "find read-only.git -type d -print | xargs chmod +w" &&
> +       git clone --no-local --depth=2 read-only.git from-read-only &&
> +       git --git-dir=from-read-only/.git log --format=%s >actual &&
> +       cat >expect <<EOF &&
> +add-1-back
> +4
> +EOF
> +       test_cmp expect actual
> +'
> +
>  test_done

It's a separate issue, but maybe we should add a similar test case for
non-shallow clone from a read-only repo too. Are there any other
operations that should work well on read-only repos?
-- 
Duy

Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Fri, Mar 7, 2014 at 1:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> I like what I see in this patch, but I wonder if we can essentially
>> revert that "temporary shallow file" patch and replace it with the
>> same (or a similar) mechanism uniformly?
>
> Using --shallow-file is uniform.  The only downside is temporary file.
> Without it you'll need to think of a way (probably different way for
> each command) to feed shallow info to.

Yes, that is what I meant to say by the "we need a way to tell hooks
in some cases" below; we are in agreement.

>> On the receive-pack side, the comment at the bottom of
>> preprare_shallow_update() makes it clear that, if we wanted to use
>> hooks, we cannot avoid having the proposed new shallow-file in a
>> temporary file, which is unfortunate.  Do we have a similar issue on
>> hooks on the upload-pack side?
>
> No. I don't think we have hooks on upload-pack.

The question was not only about "do we happen to work OK with the
current code?" but about "are we closing the door for the future?"

If we ever need to add hooks to upload-pack, with the updated
approach, its operation will not be affected by the temporary
shallow file tailored for this specific customer.  Which I think is
a good thing in general.

But at the same time, it means that its operation cannot be
customized for the specific customer, taking into account what they
lack (which can be gleaned by looking at the temporary shallow
information).  I do think that it is not a big downside, but that is
merely my knee-jerk reaction.

>> Nothing for Documentation/ anywhere?
>
> Heh git-pack-objects.txt never described stdin format. At least I
> searched for --not in it and found none. So I gladly accepted the
> situation and skipped doc update :D

To pile new technical debt on top of existing ones is to make things
worse, which we would rather not to see.

Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Thu, Mar 6, 2014 at 3:49 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
>> index 3ae9092..a980574 100755
>> --- a/t/t5537-fetch-shallow.sh
>> +++ b/t/t5537-fetch-shallow.sh
>> @@ -173,4 +173,17 @@ EOF
>>         )
>>  '
>>
>> +test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '
>> +       cp -R .git read-only.git &&
>> +       find read-only.git -print | xargs chmod -w &&
>> +       test_when_finished "find read-only.git -type d -print | xargs chmod +w" &&
>> +       git clone --no-local --depth=2 read-only.git from-read-only &&
>> +       git --git-dir=from-read-only/.git log --format=%s >actual &&
>> +       cat >expect <<EOF &&
>> +add-1-back
>> +4
>> +EOF
>> +       test_cmp expect actual
>> +'
>> +
>>  test_done
>
> It's a separate issue, but maybe we should add a similar test case for
> non-shallow clone from a read-only repo too. Are there any other
> operations that should work well on read-only repos?

Any read-only operation ;-)?

On the object-transfer front, that would mean fetching from,
archive-r from, and perhaps creating bundle from.

Locally, log, grep, blame, gitk (but only the browsing part), etc.

If a read-write borrower borrows from a read-only location via the
objects/info/alternates mechanism, anying operation to the borrower
should also work without modifying the borrowee.

Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects

[Duy Nguyen]

On Sat, Mar 8, 2014 at 1:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> On the receive-pack side, the comment at the bottom of
>>> preprare_shallow_update() makes it clear that, if we wanted to use
>>> hooks, we cannot avoid having the proposed new shallow-file in a
>>> temporary file, which is unfortunate.  Do we have a similar issue on
>>> hooks on the upload-pack side?
>>
>> No. I don't think we have hooks on upload-pack.
>
> The question was not only about "do we happen to work OK with the
> current code?" but about "are we closing the door for the future?"
>
> If we ever need to add hooks to upload-pack, with the updated
> approach, its operation will not be affected by the temporary
> shallow file tailored for this specific customer.  Which I think is
> a good thing in general.
>
> But at the same time, it means that its operation cannot be
> customized for the specific customer, taking into account what they
> lack (which can be gleaned by looking at the temporary shallow
> information).  I do think that it is not a big downside, but that is
> merely my knee-jerk reaction.

When upload-pack learns about hooks, I think we'll need to go back
with --shallow-file, perhaps we a secure temporary place to write in.
I don't see another way out. Not really sure why upload-pack needs
customization though. The only case I can think of is to prevent most
users from fetching certain objects, but that does not sound
realistic..

>>> Nothing for Documentation/ anywhere?
>>
>> Heh git-pack-objects.txt never described stdin format. At least I
>> searched for --not in it and found none. So I gladly accepted the
>> situation and skipped doc update :D
>
> To pile new technical debt on top of existing ones is to make things
> worse, which we would rather not to see.

Of course.. So what should we do with this? Go with this "no temp
file" approach and deal with hooks when they come, or prepare now and
introduce a secure temp. area?
-- 
Duy

Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Sat, Mar 8, 2014 at 1:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> On the receive-pack side, the comment at the bottom of
>>>> preprare_shallow_update() makes it clear that, if we wanted to use
>>>> hooks, we cannot avoid having the proposed new shallow-file in a
>>>> temporary file, which is unfortunate.  Do we have a similar issue on
>>>> hooks on the upload-pack side?
>>>
>>> No. I don't think we have hooks on upload-pack.
>>
>> The question was not only about "do we happen to work OK with the
>> current code?" but about "are we closing the door for the future?"
>>
>> If we ever need to add hooks to upload-pack, with the updated
>> approach, its operation will not be affected by the temporary
>> shallow file tailored for this specific customer.  Which I think is
>> a good thing in general.
>>
>> But at the same time, it means that its operation cannot be
>> customized for the specific customer, taking into account what they
>> lack (which can be gleaned by looking at the temporary shallow
>> information).  I do think that it is not a big downside, but that is
>> merely my knee-jerk reaction.
>
> When upload-pack learns about hooks, I think we'll need to go back
> with --shallow-file, perhaps we a secure temporary place to write in.
> I don't see another way out. Not really sure why upload-pack needs
> customization though. The only case I can think of is to prevent most
> users from fetching certain objects, but that does not sound
> realistic..

I was more thinking along the lines of logging.

But you are right that we can easily revisit --shallow-file
interface later.

> Of course.. So what should we do with this? Go with this "no temp
> file" approach and deal with hooks when they come, or prepare now and
> introduce a secure temp. area?

I was rather hoping that we did not have to worry about temporary
files.  This patch solves the issue for the service people would
expect to be read-only the most, and it is a good step in the right
direction.  So let's follow through with the approach and not worry
about "secure temporary" for now.

[PATCH v4] upload-pack: send shallow info over stdin to pack-objects

[Nguyễn Thái Ngọc Duy]

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.

git:// servers do not need write access to repos and usually don't
have it, which means cdab485 breaks shallow clone over git://

Instead of using a temporary file as the media for shallow points, we
can send them over stdin to pack-objects as well. Prepend shallow
SHA-1 with --shallow so pack-objects knows what is what.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation update and a minor tweak.

 Documentation/git-pack-objects.txt |  2 ++
 builtin/pack-objects.c             | 10 ++++++++++
 t/t5537-fetch-shallow.sh           | 13 +++++++++++++
 upload-pack.c                      | 21 ++++++++++++---------
 4 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index cdab9ed..d2d8f47 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -64,6 +64,8 @@ base-name::
 	the same way as 'git rev-list' with the `--objects` flag
 	uses its `commit` arguments to build the list of objects it
 	outputs.  The objects on the resulting list are packed.
+	Besides revisions, `--not` or `--shallow <SHA-1>` lines are
+	also accepted.
 
 --unpacked::
 	This implies `--revs`.  When processing the list of
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..358f9a3 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2455,6 +2455,9 @@ static void get_object_list(int ac, const char **av)
 	save_commit_buffer = 0;
 	setup_revisions(ac, av, &revs, NULL);
 
+	/* make sure shallows are read */
+	is_repository_shallow();
+
 	while (fgets(line, sizeof(line), stdin) != NULL) {
 		int len = strlen(line);
 		if (len && line[len - 1] == '\n')
@@ -2467,6 +2470,13 @@ static void get_object_list(int ac, const char **av)
 				write_bitmap_index = 0;
 				continue;
 			}
+			if (starts_with(line, "--shallow ")) {
+				unsigned char sha1[20];
+				if (get_sha1_hex(line + 10, sha1))
+					die("not an SHA-1 '%s'", line + 10);
+				register_shallow(sha1);
+				continue;
+			}
 			die("not a rev '%s'", line);
 		}
 		if (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 3ae9092..a980574 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -173,4 +173,17 @@ EOF
 	)
 '
 
+test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '
+	cp -R .git read-only.git &&
+	find read-only.git -print | xargs chmod -w &&
+	test_when_finished "find read-only.git -type d -print | xargs chmod +w" &&
+	git clone --no-local --depth=2 read-only.git from-read-only &&
+	git --git-dir=from-read-only/.git log --format=%s >actual &&
+	cat >expect <<EOF &&
+add-1-back
+4
+EOF
+	test_cmp expect actual
+'
+
 test_done
diff --git a/upload-pack.c b/upload-pack.c
index 0c44f6b..a5c50e4 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -70,6 +70,14 @@ static ssize_t send_client_data(int fd, const char *data, ssize_t sz)
 	return sz;
 }
 
+static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
+{
+	FILE *fp = cb_data;
+	if (graft->nr_parent == -1)
+		fprintf(fp, "--shallow %s\n", sha1_to_hex(graft->sha1));
+	return 0;
+}
+
 static void create_pack_file(void)
 {
 	struct child_process pack_objects;
@@ -81,12 +89,10 @@ static void create_pack_file(void)
 	const char *argv[12];
 	int i, arg = 0;
 	FILE *pipe_fd;
-	char *shallow_file = NULL;
 
 	if (shallow_nr) {
-		shallow_file = setup_temporary_shallow(NULL);
 		argv[arg++] = "--shallow-file";
-		argv[arg++] = shallow_file;
+		argv[arg++] = "";
 	}
 	argv[arg++] = "pack-objects";
 	argv[arg++] = "--revs";
@@ -114,6 +120,9 @@ static void create_pack_file(void)
 
 	pipe_fd = xfdopen(pack_objects.in, "w");
 
+	if (shallow_nr)
+		for_each_commit_graft(write_one_shallow, pipe_fd);
+
 	for (i = 0; i < want_obj.nr; i++)
 		fprintf(pipe_fd, "%s\n",
 			sha1_to_hex(want_obj.objects[i].item->sha1));
@@ -242,12 +251,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) {
 		data[0] = buffered;
-- 
1.9.0.40.gaa8c3ea