{"thread":{"id":"35966","subject":"[PATCH] upload-pack: allow shallow fetching from a read-only repository","startedAt":"2014-02-27T07:13:03Z","lastAt":"2014-03-11T12:59:46Z","messageCount":21,"participants":["Nguyễn Thái Ngọc Duy","Jeff King","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"235439","messageId":"1393485183-20100-1-git-send-email-pclouds@gmail.com","threadId":"35966","inReplyTo":null,"subject":"[PATCH] upload-pack: allow shallow fetching from a read-only repository","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-27T07:13:03Z","receivedAt":"2014-02-27T07:13:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Before cdab485 (upload-pack: delegate rev walking in shallow fetch to\npack-objects - 2013-08-16) upload-pack does not write to the source\nrepository. cdab485 starts to write $GIT_DIR/shallow_XXXXXX if it's a\nshallow fetch, so the source repo must be writable.\n\nFall back to $TMPDIR if $GIT_DIR/shallow_XXXXXX cannot be created in\nthis case. Note that in other cases that write $GIT_DIR/shallow_XXXXXX\nand eventually rename it to $GIT_DIR/shallow, there is no fallback to\n$TMPDIR.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/receive-pack.c | 2 +-\n commit.h | 2 +-\n fetch-pack.c | 2 +-\n shallow.c | 13 +++++++++++--\n t/t5537-fetch-shallow.sh | 13 +++++++++++++\n upload-pack.c | 2 +-\n 6 files changed, 28 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 85bba35..9d96f2c 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -936,7 +936,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\t\tntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));\n \n \tif (si->nr_ours || si->nr_theirs) {\n-\t\talt_shallow_file = setup_temporary_shallow(si->shallow);\n+\t\talt_shallow_file = setup_temporary_shallow(si->shallow, 0);\n \t\targv_array_pushl(&av, \"--shallow-file\", alt_shallow_file, NULL);\n \t}\n \ndiff --git a/commit.h b/commit.h\nindex 16d9c43..44a40ad 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -209,7 +209,7 @@ extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol,\n extern void setup_alternate_shallow(struct lock_file *shallow_lock,\n \t\t\t\t const char **alternate_shallow_file,\n \t\t\t\t const struct sha1_array *extra);\n-extern char *setup_temporary_shallow(const struct sha1_array *extra);\n+extern char *setup_temporary_shallow(const struct sha1_array *extra, int read_only);\n extern void advertise_shallow_grafts(int);\n \n struct shallow_info {\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 90fdd49..382b825 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -853,7 +853,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\tsetup_alternate_shallow(&shallow_lock, &alternate_shallow_file,\n \t\t\t\t\tNULL);\n \telse if (si->nr_ours || si->nr_theirs)\n-\t\talternate_shallow_file = setup_temporary_shallow(si->shallow);\n+\t\talternate_shallow_file = setup_temporary_shallow(si->shallow, 0);\n \telse\n \t\talternate_shallow_file = NULL;\n \tif (get_pack(args, fd, pack_lockfile))\ndiff --git a/shallow.c b/shallow.c\nindex bbc98b5..3adfbd5 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -216,7 +216,7 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol,\n \treturn write_shallow_commits_1(out, use_pack_protocol, extra, 0);\n }\n \n-char *setup_temporary_shallow(const struct sha1_array *extra)\n+char *setup_temporary_shallow(const struct sha1_array *extra, int read_only)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fd;\n@@ -224,7 +224,16 @@ char *setup_temporary_shallow(const struct sha1_array *extra)\n \tif (write_shallow_commits(&sb, 0, extra)) {\n \t\tstruct strbuf path = STRBUF_INIT;\n \t\tstrbuf_addstr(&path, git_path(\"shallow_XXXXXX\"));\n-\t\tfd = xmkstemp(path.buf);\n+\t\tif (read_only) {\n+\t\t\tfd = mkstemp(path.buf);\n+\t\t\tif (fd < 0) {\n+\t\t\t\tchar buf[PATH_MAX];\n+\t\t\t\tfd = git_mkstemp(buf, sizeof(buf), \"shallow_XXXXXX\");\n+\t\t\t}\n+\t\t\tif (fd < 0)\n+\t\t\t\tdie_errno(\"Unable to create temporary shallow file\");\n+\t\t} else\n+\t\t\tfd = xmkstemp(path.buf);\n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n \t\t\tdie_errno(\"failed to write to %s\",\n \t\t\t\t path.buf);\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex b0fa738..171db88 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -173,6 +173,19 @@ EOF\n \t)\n '\n \n+test_expect_success POSIXPERM 'shallow fetch from a read-only repo' '\n+\tcp -R .git read-only.git &&\n+\tfind read-only.git -print | xargs chmod -w &&\n+\ttest_when_finished \"find read-only.git -type d -print | xargs chmod +w\" &&\n+\tgit clone --no-local --depth=2 read-only.git from-read-only &&\n+\tgit --git-dir=from-read-only/.git log --format=%s >actual &&\n+\tcat >expect < Before cdab485 (upload-pack: delegate rev walking in shallow fetch to\n> pack-objects - 2013-08-16) upload-pack does not write to the source\n> repository. cdab485 starts to write $GIT_DIR/shallow_XXXXXX if it's a\n> shallow fetch, so the source repo must be writable.\n> \n> Fall back to $TMPDIR if $GIT_DIR/shallow_XXXXXX cannot be created in\n> this case. Note that in other cases that write $GIT_DIR/shallow_XXXXXX\n> and eventually rename it to $GIT_DIR/shallow, there is no fallback to\n> $TMPDIR.\n\nThat makes sense.\n\n> @@ -224,7 +224,16 @@ char *setup_temporary_shallow(const struct sha1_array *extra)\n> \tif (write_shallow_commits(&sb, 0, extra)) {\n> \t\tstruct strbuf path = STRBUF_INIT;\n> \t\tstrbuf_addstr(&path, git_path(\"shallow_XXXXXX\"));\n> -\t\tfd = xmkstemp(path.buf);\n> +\t\tif (read_only) {\n> +\t\t\tfd = mkstemp(path.buf);\n> +\t\t\tif (fd < 0) {\n> +\t\t\t\tchar buf[PATH_MAX];\n> +\t\t\t\tfd = git_mkstemp(buf, sizeof(buf), \"shallow_XXXXXX\");\n> +\t\t\t}\n> +\t\t\tif (fd < 0)\n> +\t\t\t\tdie_errno(\"Unable to create temporary shallow file\");\n> +\t\t} else\n> +\t\t\tfd = xmkstemp(path.buf);\n\nThis is not introduced by your patch, but I notice that we do not seem\nto do anything with the tempfiles when the program dies prematurely.\nWe've started collecting stale shallow_XXXXXX files in our server repos.\n\nFor the writable cases, should we be using the lockfile API to take\nshallow.lock? It looks like we do take a lock on it, but only after the\nfetch, and then we have to do a manual check for it having moved (by the\nway, shouldn't we do that check under the lock? I think\nsetup_alternate_shallow has a race condition).\n\nI guess the reason to take the lock at the last minute is that the whole\nfetch is heavyweight. But if the fetches are going to conflict, perhaps\nit is better to have lock contention at the beginning, rather than\nfailing only at the end. I don't know very much about the shallow\nsystem; does each operation rewrite the shallow file, or is it often\nleft untouched (in which case two simultaneous fetches could coexist\nmost of the time).\n\nAt any rate, if we used the lockfile API, it handles tempfile cleanup\nautomatically. Otherwise, I think we need something like the patch\nbelow (in the interest of simplicity, I just drop all of the explicit\nunlinks and let them use the atexit handler to clean up; you could also\nadd a function to explicitly unlink the tempfile and clear the handler).\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 85bba35..ac1d9b5 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -833,8 +833,6 @@ static void execute_commands(struct command *commands,\n \t\t\terror(\"BUG: run 'git fsck' for safety.\\n\"\n \t\t\t \"If there are errors, try to remove \"\n \t\t\t \"the reported refs above\");\n-\t\tif (alt_shallow_file && *alt_shallow_file)\n-\t\t\tunlink(alt_shallow_file);\n \t}\n }\n \n@@ -1087,10 +1085,6 @@ static void update_shallow_info(struct command *commands,\n \t\t\tcmd->skip_update = 1;\n \t\t}\n \t}\n-\tif (alt_shallow_file && *alt_shallow_file) {\n-\t\tunlink(alt_shallow_file);\n-\t\talt_shallow_file = NULL;\n-\t}\n \tfree(ref_status);\n }\n \ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 90fdd49..ae8550e 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -947,17 +947,6 @@ static void update_shallow(struct fetch_pack_args *args,\n \tif (!si->shallow || !si->shallow->nr)\n \t\treturn;\n \n-\tif (alternate_shallow_file) {\n-\t\t/*\n-\t\t * The temporary shallow file is only useful for\n-\t\t * index-pack and unpack-objects because it may\n-\t\t * contain more roots than we want. Delete it.\n-\t\t */\n-\t\tif (*alternate_shallow_file)\n-\t\t\tunlink(alternate_shallow_file);\n-\t\tfree((char *)alternate_shallow_file);\n-\t}\n-\n \tif (args->cloning) {\n \t\t/*\n \t\t * remote is shallow, but this is a clone, there are\ndiff --git a/shallow.c b/shallow.c\nindex bbc98b5..0f2bb48 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -8,6 +8,7 @@\n #include \"diff.h\"\n #include \"revision.h\"\n #include \"commit-slab.h\"\n+#include \"sigchain.h\"\n \n static int is_shallow = -1;\n static struct stat shallow_stat;\n@@ -216,27 +217,49 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol,\n \treturn write_shallow_commits_1(out, use_pack_protocol, extra, 0);\n }\n \n+static struct strbuf shallow_tmpfile = STRBUF_INIT;\n+\n+static void remove_shallow_tmpfile(void)\n+{\n+\tif (shallow_tmpfile.len) {\n+\t\tunlink_or_warn(shallow_tmpfile.buf);\n+\t\tstrbuf_reset(&shallow_tmpfile);\n+\t}\n+}\n+\n+static void remove_shallow_tmpfile_on_signal(int signo)\n+{\n+\tremove_shallow_tmpfile();\n+\tsigchain_pop(signo);\n+\traise(signo);\n+}\n+\n char *setup_temporary_shallow(const struct sha1_array *extra)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fd;\n \n \tif (write_shallow_commits(&sb, 0, extra)) {\n-\t\tstruct strbuf path = STRBUF_INIT;\n-\t\tstrbuf_addstr(&path, git_path(\"shallow_XXXXXX\"));\n-\t\tfd = xmkstemp(path.buf);\n+\t\tstrbuf_addstr(&shallow_tmpfile, git_path(\"shallow_XXXXXX\"));\n+\t\tfd = xmkstemp(shallow_tmpfile.buf);\n+\n+\t\t/* XXX can there be multiple shallow tempfiles in one program?\n+\t\t * In that case, we would need to maintain a list */\n+\t\tatexit(remove_shallow_tmpfile);\n+\t\tsigchain_push_common(remove_shallow_tmpfile_on_signal);\n+\n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n \t\t\tdie_errno(\"failed to write to %s\",\n-\t\t\t\t path.buf);\n+\t\t\t\t shallow_tmpfile.buf);\n \t\tclose(fd);\n \t\tstrbuf_release(&sb);\n-\t\treturn strbuf_detach(&path, NULL);\n+\t\treturn shallow_tmpfile.buf;\n \t}\n \t/*\n \t * is_repository_shallow() sees empty string as \"no shallow\n \t * file\".\n \t */\n-\treturn xstrdup(\"\");\n+\treturn shallow_tmpfile.buf;\n }\n \n void setup_alternate_shallow(struct lock_file *shallow_lock,\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 0c44f6b..55c1f71 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -242,11 +242,6 @@ static void create_pack_file(void)\n \t\terror(\"git upload-pack: git-pack-objects died with error.\");\n \t\tgoto fail;\n \t}\n-\tif (shallow_file) {\n-\t\tif (*shallow_file)\n-\t\t\tunlink(shallow_file);\n-\t\tfree(shallow_file);\n-\t}\n \n \t/* flush the data */\n \tif (0 <= buffered) {\n"},{"id":"235447","messageId":"20140227091012.GB21892@sigill.intra.peff.net","threadId":"35966","inReplyTo":"20140227090426.GA21892@sigill.intra.peff.net","subject":"[PATCH] shallow: verify shallow file after taking lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-27T09:10:13Z","receivedAt":"2014-02-27T09:10:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Before writing the shallow file, we stat() the existing file\nto make sure it has not been updated since our operation\nbegan. However, we do not do so under a lock, so there is a\npossible race:\n\n 1. Process A takes the lock.\n\n 2. Process B calls check_shallow_file_for_update and finds\n no update.\n\n 3. Process A commits the lockfile.\n\n 4. Process B takes the lock, then overwrite's process A's\n changes.\n\nWe can fix this by doing our check while we hold the lock.\n\nSigned-off-by: Jeff King \n---\nOn Thu, Feb 27, 2014 at 04:04:26AM -0500, Jeff King wrote:\n\n> by the way, shouldn't we do that check under the lock? I think\n> setup_alternate_shallow has a race condition).\n\nHere is the fix. I didn't actually reproduce the race experimentally,\nbut it seems pretty straightforward.\n\nI also notice that check_shallow_file_for_update returns early if\n!is_shallow. Is that safe? Is it possible for another process to have\nmade us shallow since the program began? In that case, we would have to\nstat() the file always, then complain if it exists and !is_shallow.\n\n shallow.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/shallow.c b/shallow.c\nindex bbc98b5..75da07a 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -246,9 +246,9 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fd;\n \n-\tcheck_shallow_file_for_update();\n \tfd = hold_lock_file_for_update(shallow_lock, git_path(\"shallow\"),\n \t\t\t\t LOCK_DIE_ON_ERROR);\n+\tcheck_shallow_file_for_update();\n \tif (write_shallow_commits(&sb, 0, extra)) {\n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n \t\t\tdie_errno(\"failed to write to %s\",\n@@ -293,9 +293,9 @@ void prune_shallow(int show_only)\n \t\tstrbuf_release(&sb);\n \t\treturn;\n \t}\n-\tcheck_shallow_file_for_update();\n \tfd = hold_lock_file_for_update(&shallow_lock, git_path(\"shallow\"),\n \t\t\t\t LOCK_DIE_ON_ERROR);\n+\tcheck_shallow_file_for_update();\n \tif (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {\n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n \t\t\tdie_errno(\"failed to write to %s\",\n-- \n1.8.5.2.500.g8060133\n"},{"id":"235448","messageId":"20140227092227.GA28551@sigill.intra.peff.net","threadId":"35966","inReplyTo":"20140227091012.GB21892@sigill.intra.peff.net","subject":"Re: [PATCH] shallow: verify shallow file after taking lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-27T09:22:27Z","receivedAt":"2014-02-27T09:22:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 27, 2014 at 04:10:12AM -0500, Jeff King wrote:\n\n> I also notice that check_shallow_file_for_update returns early if\n> !is_shallow. Is that safe? Is it possible for another process to have\n> made us shallow since the program began? In that case, we would have to\n> stat() the file always, then complain if it exists and !is_shallow.\n\nThat patch would look like this:\n\ndiff --git a/shallow.c b/shallow.c\nindex 75da07a..e05a241 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -139,13 +139,13 @@ void check_shallow_file_for_update(void)\n {\n \tstruct stat st;\n \n-\tif (!is_shallow)\n-\t\treturn;\n-\telse if (is_shallow == -1)\n+\tif (is_shallow == -1)\n \t\tdie(\"BUG: shallow must be initialized by now\");\n \n \tif (stat(git_path(\"shallow\"), &st))\n \t\tdie(\"shallow file was removed during fetch\");\n+\telse if (!is_shallow)\n+\t\tdie(\"shallow file appeared during fetch\");\n \telse if (st.st_mtime != shallow_stat.st_mtime\n #ifdef USE_NSEC\n \t\t || ST_MTIME_NSEC(st) != ST_MTIME_NSEC(shallow_stat)\n\nbut again, I'm not really clear on whether this is possible.\n\n-Peff\n"},{"id":"235450","messageId":"CACsJy8Di7vVhZzzHhavTPupbgeiKr70psq_U33C8i4c+auJxUA@mail.gmail.com","threadId":"35966","inReplyTo":"20140227090426.GA21892@sigill.intra.peff.net","subject":"Re: [PATCH] upload-pack: allow shallow fetching from a read-only repository","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-27T10:11:41Z","receivedAt":"2014-02-27T10:11:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Feb 27, 2014 at 4:04 PM, Jeff King wrote:\n> This is not introduced by your patch, but I notice that we do not seem\n> to do anything with the tempfiles when the program dies prematurely.\n> We've started collecting stale shallow_XXXXXX files in our server repos.\n>\n> For the writable cases, should we be using the lockfile API to take\n> shallow.lock? It looks like we do take a lock on it, but only after the\n> fetch, and then we have to do a manual check for it having moved (by the\n> way, shouldn't we do that check under the lock? I think\n> setup_alternate_shallow has a race condition).\n>\n> I guess the reason to take the lock at the last minute is that the whole\n> fetch is heavyweight. But if the fetches are going to conflict, perhaps\n> it is better to have lock contention at the beginning, rather than\n> failing only at the end. I don't know very much about the shallow\n> system; does each operation rewrite the shallow file, or is it often\n> left untouched (in which case two simultaneous fetches could coexist\n> most of the time).\n\nFor writable cases (fetch-pack and receive-pack), yes I think locking\nearlier is better or multiple fetches/pushes will race to update\nshallow file. Chances of racing fetches are practically near zero, I\nthink. We need to do something about push.\n\nWe only update shallow file in these cases: clone --depth, fetch\n--update-shallow, fetch --depth, and push when receive.shallowupdate\nis set. All of them may end up not updating shallow file though. The\nlast case is the most troublesome because receive.shallowupdate is set\nat server side and we don't want any shallow push to block all other\nshallow pushes..\n\nFor read-only case in upload-file, locking only reduces the\navailability of clone/fetch.\n\n> At any rate, if we used the lockfile API, it handles tempfile cleanup\n> automatically. Otherwise, I think we need something like the patch\n> below (in the interest of simplicity, I just drop all of the explicit\n> unlinks and let them use the atexit handler to clean up; you could also\n> add a function to explicitly unlink the tempfile and clear the handler).\n\nLooks like a good thing to do anyway.\n\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 85bba35..ac1d9b5 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -833,8 +833,6 @@ static void execute_commands(struct command *commands,\n> error(\"BUG: run 'git fsck' for safety.\\n\"\n> \"If there are errors, try to remove \"\n> \"the reported refs above\");\n> - if (alt_shallow_file && *alt_shallow_file)\n> - unlink(alt_shallow_file);\n> }\n> }\n>\n> @@ -1087,10 +1085,6 @@ static void update_shallow_info(struct command *commands,\n> cmd->skip_update = 1;\n> }\n> }\n> - if (alt_shallow_file && *alt_shallow_file) {\n> - unlink(alt_shallow_file);\n> - alt_shallow_file = NULL;\n> - }\n> free(ref_status);\n> }\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 90fdd49..ae8550e 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -947,17 +947,6 @@ static void update_shallow(struct fetch_pack_args *args,\n> if (!si->shallow || !si->shallow->nr)\n> return;\n>\n> - if (alternate_shallow_file) {\n> - /*\n> - * The temporary shallow file is only useful for\n> - * index-pack and unpack-objects because it may\n> - * contain more roots than we want. Delete it.\n> - */\n> - if (*alternate_shallow_file)\n> - unlink(alternate_shallow_file);\n> - free((char *)alternate_shallow_file);\n> - }\n> -\n> if (args->cloning) {\n> /*\n> * remote is shallow, but this is a clone, there are\n> diff --git a/shallow.c b/shallow.c\n> index bbc98b5..0f2bb48 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -8,6 +8,7 @@\n> #include \"diff.h\"\n> #include \"revision.h\"\n> #include \"commit-slab.h\"\n> +#include \"sigchain.h\"\n>\n> static int is_shallow = -1;\n> static struct stat shallow_stat;\n> @@ -216,27 +217,49 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol,\n> return write_shallow_commits_1(out, use_pack_protocol, extra, 0);\n> }\n>\n> +static struct strbuf shallow_tmpfile = STRBUF_INIT;\n> +\n> +static void remove_shallow_tmpfile(void)\n> +{\n> + if (shallow_tmpfile.len) {\n> + unlink_or_warn(shallow_tmpfile.buf);\n> + strbuf_reset(&shallow_tmpfile);\n> + }\n> +}\n> +\n> +static void remove_shallow_tmpfile_on_signal(int signo)\n> +{\n> + remove_shallow_tmpfile();\n> + sigchain_pop(signo);\n> + raise(signo);\n> +}\n> +\n> char *setup_temporary_shallow(const struct sha1_array *extra)\n> {\n> struct strbuf sb = STRBUF_INIT;\n> int fd;\n>\n> if (write_shallow_commits(&sb, 0, extra)) {\n> - struct strbuf path = STRBUF_INIT;\n> - strbuf_addstr(&path, git_path(\"shallow_XXXXXX\"));\n> - fd = xmkstemp(path.buf);\n> + strbuf_addstr(&shallow_tmpfile, git_path(\"shallow_XXXXXX\"));\n> + fd = xmkstemp(shallow_tmpfile.buf);\n> +\n> + /* XXX can there be multiple shallow tempfiles in one program?\n> + * In that case, we would need to maintain a list */\n> + atexit(remove_shallow_tmpfile);\n> + sigchain_push_common(remove_shallow_tmpfile_on_signal);\n> +\n> if (write_in_full(fd, sb.buf, sb.len) != sb.len)\n> die_errno(\"failed to write to %s\",\n> - path.buf);\n> + shallow_tmpfile.buf);\n> close(fd);\n> strbuf_release(&sb);\n> - return strbuf_detach(&path, NULL);\n> + return shallow_tmpfile.buf;\n> }\n> /*\n> * is_repository_shallow() sees empty string as \"no shallow\n> * file\".\n> */\n> - return xstrdup(\"\");\n> + return shallow_tmpfile.buf;\n> }\n>\n> void setup_alternate_shallow(struct lock_file *shallow_lock,\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 0c44f6b..55c1f71 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -242,11 +242,6 @@ static void create_pack_file(void)\n> error(\"git upload-pack: git-pack-objects died with error.\");\n> goto fail;\n> }\n> - if (shallow_file) {\n> - if (*shallow_file)\n> - unlink(shallow_file);\n> - free(shallow_file);\n> - }\n>\n> /* flush the data */\n> if (0 <= buffered) {\n\n\n\n-- \nDuy\n"},{"id":"235451","messageId":"CACsJy8AHwyy0wwFD3fu+Aak+k=bFM1NAWzVSs1G4389UWqZptg@mail.gmail.com","threadId":"35966","inReplyTo":"20140227092227.GA28551@sigill.intra.peff.net","subject":"Re: [PATCH] shallow: verify shallow file after taking lock","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-27T10:18:58Z","receivedAt":"2014-02-27T10:18:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Feb 27, 2014 at 4:22 PM, Jeff King wrote:\n> On Thu, Feb 27, 2014 at 04:10:12AM -0500, Jeff King wrote:\n>\n>> I also notice that check_shallow_file_for_update returns early if\n>> !is_shallow. Is that safe? Is it possible for another process to have\n>> made us shallow since the program began? In that case, we would have to\n>> stat() the file always, then complain if it exists and !is_shallow.\n\nI think it's safer to do it your way.\n\n>\n> That patch would look like this:\n>\n> diff --git a/shallow.c b/shallow.c\n> index 75da07a..e05a241 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -139,13 +139,13 @@ void check_shallow_file_for_update(void)\n> {\n> struct stat st;\n>\n> - if (!is_shallow)\n> - return;\n> - else if (is_shallow == -1)\n> + if (is_shallow == -1)\n> die(\"BUG: shallow must be initialized by now\");\n>\n> if (stat(git_path(\"shallow\"), &st))\n> die(\"shallow file was removed during fetch\");\n> + else if (!is_shallow)\n> + die(\"shallow file appeared during fetch\");\n> else if (st.st_mtime != shallow_stat.st_mtime\n> #ifdef USE_NSEC\n> || ST_MTIME_NSEC(st) != ST_MTIME_NSEC(shallow_stat)\n>\n> but again, I'm not really clear on whether this is possible.\n>\n> -Peff\n\n\n\n-- \nDuy\n"},{"id":"235454","messageId":"20140227105630.GA29668@sigill.intra.peff.net","threadId":"35966","inReplyTo":"CACsJy8AHwyy0wwFD3fu+Aak+k=bFM1NAWzVSs1G4389UWqZptg@mail.gmail.com","subject":"[PATCH] shallow: use stat_validity to check for up-to-date file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-27T10:56:31Z","receivedAt":"2014-02-27T10:56:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 27, 2014 at 05:18:58PM +0700, Duy Nguyen wrote:\n\n> On Thu, Feb 27, 2014 at 4:22 PM, Jeff King wrote:\n> > On Thu, Feb 27, 2014 at 04:10:12AM -0500, Jeff King wrote:\n> >\n> >> I also notice that check_shallow_file_for_update returns early if\n> >> !is_shallow. Is that safe? Is it possible for another process to have\n> >> made us shallow since the program began? In that case, we would have to\n> >> stat() the file always, then complain if it exists and !is_shallow.\n> \n> I think it's safer to do it your way.\n\nYeah, I played around a bit and found that using \"git fetch --depth\" in\na non-shallow repo could run into this case.\n\n> > if (stat(git_path(\"shallow\"), &st))\n> > die(\"shallow file was removed during fetch\");\n> > + else if (!is_shallow)\n> > + die(\"shallow file appeared during fetch\");\n\nNote that this is wrong; when the file is missing (the first part of the\nconditional), we need to check \"is_shallow\" before dying. Otherwise we\nerroneously complain when creating the file for the first time.\n\nAs I was fixing it, though, I recalled that we had to write a similar\nsystem for the packed-refs file. Fortunately, it was easy to reuse, and\nI ended up with the patch below.\n\n-- >8 --\nSubject: shallow: use stat_validity to check for up-to-date file\n\nWhen we are about to write the shallow file, we check that\nit has not changed since we last read it. Instead of\nhand-rolling this, we can use stat_validity. This is built\naround the index stat-check, so it is more robust than just\nchecking the mtime, as we do now (it uses the same check as\nwe do for index files).\n\nThe new code also handles the case of a shallow file\nappearing unexpectedly. With the current code, two\nsimultaneous processes making us shallow (e.g., two \"git\nfetch --depth=1\" running at the same time in a non-shallow\nrepository) can race to overwrite each other.\n\nAs a bonus, we also remove a race in determining the stat\ninformation of what we read (we stat and then open, leaving\na race window; instead we should open and then fstat the\ndescriptor).\n\nSigned-off-by: Jeff King \n---\n shallow.c | 24 +++++++-----------------\n 1 file changed, 7 insertions(+), 17 deletions(-)\n\ndiff --git a/shallow.c b/shallow.c\nindex 75da07a..9668b39 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -10,7 +10,7 @@\n #include \"commit-slab.h\"\n \n static int is_shallow = -1;\n-static struct stat shallow_stat;\n+static struct stat_validity shallow_stat;\n static char *alternate_shallow_file;\n \n void set_alternate_shallow_file(const char *path, int override)\n@@ -52,12 +52,12 @@ int is_repository_shallow(void)\n \t * shallow file should be used. We could just open it and it\n \t * will likely fail. But let's do an explicit check instead.\n \t */\n-\tif (!*path ||\n-\t stat(path, &shallow_stat) ||\n-\t (fp = fopen(path, \"r\")) == NULL) {\n+\tif (!*path || (fp = fopen(path, \"r\")) == NULL) {\n+\t\tstat_validity_clear(&shallow_stat);\n \t\tis_shallow = 0;\n \t\treturn is_shallow;\n \t}\n+\tstat_validity_update(&shallow_stat, fileno(fp));\n \tis_shallow = 1;\n \n \twhile (fgets(buf, sizeof(buf), fp)) {\n@@ -137,21 +137,11 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,\n \n void check_shallow_file_for_update(void)\n {\n-\tstruct stat st;\n-\n-\tif (!is_shallow)\n-\t\treturn;\n-\telse if (is_shallow == -1)\n+\tif (is_shallow == -1)\n \t\tdie(\"BUG: shallow must be initialized by now\");\n \n-\tif (stat(git_path(\"shallow\"), &st))\n-\t\tdie(\"shallow file was removed during fetch\");\n-\telse if (st.st_mtime != shallow_stat.st_mtime\n-#ifdef USE_NSEC\n-\t\t || ST_MTIME_NSEC(st) != ST_MTIME_NSEC(shallow_stat)\n-#endif\n-\t\t )\n-\t\tdie(\"shallow file was changed during fetch\");\n+\tif (!stat_validity_check(&shallow_stat, git_path(\"shallow\")))\n+\t\tdie(\"shallow file has changed since we read it\");\n }\n \n #define SEEN_ONLY 1\n-- \n1.8.5.2.500.g8060133\n"},{"id":"235458","messageId":"20140227112519.GB29668@sigill.intra.peff.net","threadId":"35966","inReplyTo":"CACsJy8Di7vVhZzzHhavTPupbgeiKr70psq_U33C8i4c+auJxUA@mail.gmail.com","subject":"[PATCH] shallow: automatically clean up shallow tempfiles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-27T11:25:20Z","receivedAt":"2014-02-27T11:25:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 27, 2014 at 05:11:41PM +0700, Duy Nguyen wrote:\n\n> We only update shallow file in these cases: clone --depth, fetch\n> --update-shallow, fetch --depth, and push when receive.shallowupdate\n> is set. All of them may end up not updating shallow file though.\n\nOK, that last sentence is what I wasn't sure of. If they might sometimes\nnot update the shallow file, then I think locking early is wrong. It\nmeans we create lock contention where it might not exist.\n\n> For read-only case in upload-file, locking only reduces the\n> availability of clone/fetch.\n\nRight, those should never lock. So even if we did want to tweak the\nlocking, we would still need a separate tempfile cleanup for those.\n\nHere it is as a completed patch. It conflicts textually but not\nsemantically with the patch that started this thread (I think one of my\nearlier patches has a minor textual conflict, too). Should we roll them\ninto a single series to help Junio? If so, do you want to do it, or\nshould I?\n\n-- >8 --\nSubject: shallow: automatically clean up shallow tempfiles\n\nWe sometimes write tempfiles of the form \"shallow_XXXXXX\"\nduring fetch/push operations with shallow repositories.\nUnder normal circumstances, we clean up the result when we\nare done. However, we do no take steps to clean up after\nourselves when we exit due to die() or signal death.\n\nThis patch teaches the tempfile creation code to register\nhandlers to clean up after ourselves. To handle this, we\nchange the ownership semantics of the filename returned by\nsetup_temporary_shallow. It now keeps a copy of the filename\nitself, and returns only a const pointer to it.\n\nWe can also do away with explicit tempfile removal in the\ncallers. They all exit not long after finishing with the\nfile, so they can rely on the auto-cleanup, simplifying the\ncode.\n\nNote that we keep things simple and maintain only a single\nfilename to be cleaned. This is sufficient for the current\ncaller, but we future-proof it with a die(\"BUG\").\n\nSigned-off-by: Jeff King \n---\n builtin/receive-pack.c | 16 ++++------------\n commit.h | 2 +-\n fetch-pack.c | 11 -----------\n shallow.c | 41 ++++++++++++++++++++++++++++++++++-------\n upload-pack.c | 7 +------\n 5 files changed, 40 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 85bba35..c323081 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -828,14 +828,10 @@ static void execute_commands(struct command *commands,\n \t\t}\n \t}\n \n-\tif (shallow_update) {\n-\t\tif (!checked_connectivity)\n-\t\t\terror(\"BUG: run 'git fsck' for safety.\\n\"\n-\t\t\t \"If there are errors, try to remove \"\n-\t\t\t \"the reported refs above\");\n-\t\tif (alt_shallow_file && *alt_shallow_file)\n-\t\t\tunlink(alt_shallow_file);\n-\t}\n+\tif (shallow_update && !checked_connectivity)\n+\t\terror(\"BUG: run 'git fsck' for safety.\\n\"\n+\t\t \"If there are errors, try to remove \"\n+\t\t \"the reported refs above\");\n }\n \n static struct command *read_head_info(struct sha1_array *shallow)\n@@ -1087,10 +1083,6 @@ static void update_shallow_info(struct command *commands,\n \t\t\tcmd->skip_update = 1;\n \t\t}\n \t}\n-\tif (alt_shallow_file && *alt_shallow_file) {\n-\t\tunlink(alt_shallow_file);\n-\t\talt_shallow_file = NULL;\n-\t}\n \tfree(ref_status);\n }\n \ndiff --git a/commit.h b/commit.h\nindex 16d9c43..55631f1 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -209,7 +209,7 @@ extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol,\n extern void setup_alternate_shallow(struct lock_file *shallow_lock,\n \t\t\t\t const char **alternate_shallow_file,\n \t\t\t\t const struct sha1_array *extra);\n-extern char *setup_temporary_shallow(const struct sha1_array *extra);\n+extern const char *setup_temporary_shallow(const struct sha1_array *extra);\n extern void advertise_shallow_grafts(int);\n \n struct shallow_info {\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 90fdd49..ae8550e 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -947,17 +947,6 @@ static void update_shallow(struct fetch_pack_args *args,\n \tif (!si->shallow || !si->shallow->nr)\n \t\treturn;\n \n-\tif (alternate_shallow_file) {\n-\t\t/*\n-\t\t * The temporary shallow file is only useful for\n-\t\t * index-pack and unpack-objects because it may\n-\t\t * contain more roots than we want. Delete it.\n-\t\t */\n-\t\tif (*alternate_shallow_file)\n-\t\t\tunlink(alternate_shallow_file);\n-\t\tfree((char *)alternate_shallow_file);\n-\t}\n-\n \tif (args->cloning) {\n \t\t/*\n \t\t * remote is shallow, but this is a clone, there are\ndiff --git a/shallow.c b/shallow.c\nindex 9668b39..0b267b6 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -8,6 +8,7 @@\n #include \"diff.h\"\n #include \"revision.h\"\n #include \"commit-slab.h\"\n+#include \"sigchain.h\"\n \n static int is_shallow = -1;\n static struct stat_validity shallow_stat;\n@@ -206,27 +207,53 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol,\n \treturn write_shallow_commits_1(out, use_pack_protocol, extra, 0);\n }\n \n-char *setup_temporary_shallow(const struct sha1_array *extra)\n+static struct strbuf temporary_shallow = STRBUF_INIT;\n+\n+static void remove_temporary_shallow(void)\n+{\n+\tif (temporary_shallow.len) {\n+\t\tunlink_or_warn(temporary_shallow.buf);\n+\t\tstrbuf_reset(&temporary_shallow);\n+\t}\n+}\n+\n+static void remove_temporary_shallow_on_signal(int signo)\n+{\n+\tremove_temporary_shallow();\n+\tsigchain_pop(signo);\n+\traise(signo);\n+}\n+\n+const char *setup_temporary_shallow(const struct sha1_array *extra)\n {\n+\tstatic int installed_handler;\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fd;\n \n+\tif (temporary_shallow.len)\n+\t\tdie(\"BUG: attempt to create two temporary shallow files\");\n+\n \tif (write_shallow_commits(&sb, 0, extra)) {\n-\t\tstruct strbuf path = STRBUF_INIT;\n-\t\tstrbuf_addstr(&path, git_path(\"shallow_XXXXXX\"));\n-\t\tfd = xmkstemp(path.buf);\n+\t\tstrbuf_addstr(&temporary_shallow, git_path(\"shallow_XXXXXX\"));\n+\t\tfd = xmkstemp(temporary_shallow.buf);\n+\n+\t\tif (!installed_handler) {\n+\t\t\tatexit(remove_temporary_shallow);\n+\t\t\tsigchain_push_common(remove_temporary_shallow_on_signal);\n+\t\t}\n+\n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n \t\t\tdie_errno(\"failed to write to %s\",\n-\t\t\t\t path.buf);\n+\t\t\t\t temporary_shallow.buf);\n \t\tclose(fd);\n \t\tstrbuf_release(&sb);\n-\t\treturn strbuf_detach(&path, NULL);\n+\t\treturn temporary_shallow.buf;\n \t}\n \t/*\n \t * is_repository_shallow() sees empty string as \"no shallow\n \t * file\".\n \t */\n-\treturn xstrdup(\"\");\n+\treturn temporary_shallow.buf;\n }\n \n void setup_alternate_shallow(struct lock_file *shallow_lock,\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 0c44f6b..a3c52f6 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -81,7 +81,7 @@ static void create_pack_file(void)\n \tconst char *argv[12];\n \tint i, arg = 0;\n \tFILE *pipe_fd;\n-\tchar *shallow_file = NULL;\n+\tconst char *shallow_file = NULL;\n \n \tif (shallow_nr) {\n \t\tshallow_file = setup_temporary_shallow(NULL);\n@@ -242,11 +242,6 @@ static void create_pack_file(void)\n \t\terror(\"git upload-pack: git-pack-objects died with error.\");\n \t\tgoto fail;\n \t}\n-\tif (shallow_file) {\n-\t\tif (*shallow_file)\n-\t\t\tunlink(shallow_file);\n-\t\tfree(shallow_file);\n-\t}\n \n \t/* flush the data */\n \tif (0 <= buffered) {\n-- \n1.8.5.2.500.g8060133\n"},{"id":"235989","messageId":"1393936205-15953-1-git-send-email-pclouds@gmail.com","threadId":"35966","inReplyTo":"1393485183-20100-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2] upload-pack: allow shallow fetching from a read-only repository","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-04T12:30:05Z","receivedAt":"2014-03-04T12:30:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Before cdab485 (upload-pack: delegate rev walking in shallow fetch to\npack-objects - 2013-08-16) upload-pack does not write to the source\nrepository. cdab485 starts to write $GIT_DIR/shallow_XXXXXX if it's a\nshallow fetch, so the source repo must be writable.\n\ngit:// servers do not need write access to repos and usually don't,\nwhich mean cdab485 breaks shallow clone over git://\n\nFall back to $TMPDIR if $GIT_DIR/shallow_XXXXXX cannot be created in\nthis case. Note that in other cases that write $GIT_DIR/shallow_XXXXXX\nand eventually rename it to $GIT_DIR/shallow, there is no fallback to\n$TMPDIR.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Rebased on top of jk/shallow-update-fix\n\n builtin/receive-pack.c | 2 +-\n commit.h | 2 +-\n fetch-pack.c | 2 +-\n shallow.c | 13 +++++++++++--\n t/t5537-fetch-shallow.sh | 13 +++++++++++++\n upload-pack.c | 2 +-\n 6 files changed, 28 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c323081..d723099 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -932,7 +932,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\t\tntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));\n \n \tif (si->nr_ours || si->nr_theirs) {\n-\t\talt_shallow_file = setup_temporary_shallow(si->shallow);\n+\t\talt_shallow_file = setup_temporary_shallow(si->shallow, 0);\n \t\targv_array_pushl(&av, \"--shallow-file\", alt_shallow_file, NULL);\n \t}\n \ndiff --git a/commit.h b/commit.h\nindex 55631f1..d38e996 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -209,7 +209,7 @@ extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol,\n extern void setup_alternate_shallow(struct lock_file *shallow_lock,\n \t\t\t\t const char **alternate_shallow_file,\n \t\t\t\t const struct sha1_array *extra);\n-extern const char *setup_temporary_shallow(const struct sha1_array *extra);\n+extern const char *setup_temporary_shallow(const struct sha1_array *extra, int read_only);\n extern void advertise_shallow_grafts(int);\n \n struct shallow_info {\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex ae8550e..b71d186 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -853,7 +853,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\tsetup_alternate_shallow(&shallow_lock, &alternate_shallow_file,\n \t\t\t\t\tNULL);\n \telse if (si->nr_ours || si->nr_theirs)\n-\t\talternate_shallow_file = setup_temporary_shallow(si->shallow);\n+\t\talternate_shallow_file = setup_temporary_shallow(si->shallow, 0);\n \telse\n \t\talternate_shallow_file = NULL;\n \tif (get_pack(args, fd, pack_lockfile))\ndiff --git a/shallow.c b/shallow.c\nindex c7602ce..ad28af6 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -224,7 +224,8 @@ static void remove_temporary_shallow_on_signal(int signo)\n \traise(signo);\n }\n \n-const char *setup_temporary_shallow(const struct sha1_array *extra)\n+const char *setup_temporary_shallow(const struct sha1_array *extra,\n+\t\t\t\t int read_only)\n {\n \tstatic int installed_handler;\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -235,7 +236,15 @@ const char *setup_temporary_shallow(const struct sha1_array *extra)\n \n \tif (write_shallow_commits(&sb, 0, extra)) {\n \t\tstrbuf_addstr(&temporary_shallow, git_path(\"shallow_XXXXXX\"));\n-\t\tfd = xmkstemp(temporary_shallow.buf);\n+\t\tfd = mkstemp(temporary_shallow.buf);\n+\t\tif (read_only && fd < 0) {\n+\t\t\tstrbuf_grow(&temporary_shallow, PATH_MAX);\n+\t\t\tfd = git_mkstemp(temporary_shallow.buf, PATH_MAX,\n+\t\t\t\t\t \"shallow_XXXXXX\");\n+\t\t}\n+\t\tif (fd < 0)\n+\t\t\tdie_errno(\"Unable to create temporary file '%s'\",\n+\t\t\t\t temporary_shallow.buf);\n \n \t\tif (!installed_handler) {\n \t\t\tatexit(remove_temporary_shallow);\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex b0fa738..171db88 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -173,6 +173,19 @@ EOF\n \t)\n '\n \n+test_expect_success POSIXPERM 'shallow fetch from a read-only repo' '\n+\tcp -R .git read-only.git &&\n+\tfind read-only.git -print | xargs chmod -w &&\n+\ttest_when_finished \"find read-only.git -type d -print | xargs chmod +w\" &&\n+\tgit clone --no-local --depth=2 read-only.git from-read-only &&\n+\tgit --git-dir=from-read-only/.git log --format=%s >actual &&\n+\tcat >expect < writes:\n\n> Before cdab485 (upload-pack: delegate rev walking in shallow fetch to\n> pack-objects - 2013-08-16) upload-pack does not write to the source\n> repository. cdab485 starts to write $GIT_DIR/shallow_XXXXXX if it's a\n> shallow fetch, so the source repo must be writable.\n>\n> git:// servers do not need write access to repos and usually don't,\n> which mean cdab485 breaks shallow clone over git://\n>\n> Fall back to $TMPDIR if $GIT_DIR/shallow_XXXXXX cannot be created in\n> this case. Note that in other cases that write $GIT_DIR/shallow_XXXXXX\n> and eventually rename it to $GIT_DIR/shallow, there is no fallback to\n> $TMPDIR.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> Rebased on top of jk/shallow-update-fix\n\nHmph.\n\nI notice that the original code, with or without this change, allows\nupload-pack spawned by daemon to attempt to write into GIT_DIR.\nAs upload-pack is supposed to be a read-only operation, this is\nquite bad.\n\nPerhaps we should give server operators an option to run their\ndaemon -> upload-pack chain to always write to a throw-away\ndirectory of their choice, without ever attempting to write to\nGIT_DIR it serves?\n\nHow well is the access to the temporary shallow file controlled in\nyour code (sorry, but I do not recall carefully reading your patch\nthat added the mechanism with security issues in mind, so now I am\nasking)? When it is redirected to TMPDIR (let's forget GIT_DIR for\nnow---if an attacker can write into there, the repository is already\nlost), can an attacker race with us to cause us to overwrite we do\nnot expect to?\n\nEven if it turns out that this patch is secure enough as-is, we\ndefinitely need to make sure that server operators, who want to keep\ntheir upload-pack truly a read-only operation, know that it is\nnecessary to (1) keep the system user they run git-daemon under\nincapable of writing into GIT_DIR, and (2) make sure TMPDIR points\nat somewhere only git-daemon user and nobody else can write into,\nsomewhere in the documentation.\n\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index ae8550e..b71d186 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -853,7 +853,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n> \t\tsetup_alternate_shallow(&shallow_lock, &alternate_shallow_file,\n> \t\t\t\t\tNULL);\n> \telse if (si->nr_ours || si->nr_theirs)\n> -\t\talternate_shallow_file = setup_temporary_shallow(si->shallow);\n> +\t\talternate_shallow_file = setup_temporary_shallow(si->shallow, 0);\n> \telse\n> \t\talternate_shallow_file = NULL;\n> \tif (get_pack(args, fd, pack_lockfile))\n> diff --git a/shallow.c b/shallow.c\n> index c7602ce..ad28af6 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -224,7 +224,8 @@ static void remove_temporary_shallow_on_signal(int signo)\n> \traise(signo);\n> }\n> \n> -const char *setup_temporary_shallow(const struct sha1_array *extra)\n> +const char *setup_temporary_shallow(const struct sha1_array *extra,\n> +\t\t\t\t int read_only)\n> {\n> \tstatic int installed_handler;\n> \tstruct strbuf sb = STRBUF_INIT;\n> @@ -235,7 +236,15 @@ const char *setup_temporary_shallow(const struct sha1_array *extra)\n> \n> \tif (write_shallow_commits(&sb, 0, extra)) {\n> \t\tstrbuf_addstr(&temporary_shallow, git_path(\"shallow_XXXXXX\"));\n> -\t\tfd = xmkstemp(temporary_shallow.buf);\n> +\t\tfd = mkstemp(temporary_shallow.buf);\n> +\t\tif (read_only && fd < 0) {\n> +\t\t\tstrbuf_grow(&temporary_shallow, PATH_MAX);\n> +\t\t\tfd = git_mkstemp(temporary_shallow.buf, PATH_MAX,\n> +\t\t\t\t\t \"shallow_XXXXXX\");\n> +\t\t}\n> +\t\tif (fd < 0)\n> +\t\t\tdie_errno(\"Unable to create temporary file '%s'\",\n> +\t\t\t\t temporary_shallow.buf);\n> \n> \t\tif (!installed_handler) {\n> \t\t\tatexit(remove_temporary_shallow);\n> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\n> index b0fa738..171db88 100755\n> --- a/t/t5537-fetch-shallow.sh\n> +++ b/t/t5537-fetch-shallow.sh\n> @@ -173,6 +173,19 @@ EOF\n> \t)\n> '\n> \n> +test_expect_success POSIXPERM 'shallow fetch from a read-only repo' '\n\ns/POSIXPERM/&,SANITY/, perhaps?\n\nThinking of it again, perhaps POSIXPERM should imply SANITY is required?\n\n> +\tcp -R .git read-only.git &&\n> +\tfind read-only.git -print | xargs chmod -w &&\n> +\ttest_when_finished \"find read-only.git -type d -print | xargs chmod +w\" &&\n> +\tgit clone --no-local --depth=2 read-only.git from-read-only &&\n> +\tgit --git-dir=from-read-only/.git log --format=%s >actual &&\n> +\tcat >expect < +add-1-back\n> +4\n> +EOF\n> +\ttest_cmp expect actual\n> +'\n> +\n> if test -n \"$NO_CURL\" -o -z \"$GIT_TEST_HTTPD\"; then\n> \tsay 'skipping remaining tests, git built without http support'\n> \ttest_done\n> diff --git a/upload-pack.c b/upload-pack.c\n> index a3c52f6..b538f32 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -84,7 +84,7 @@ static void create_pack_file(void)\n> \tconst char *shallow_file = NULL;\n> \n> \tif (shallow_nr) {\n> -\t\tshallow_file = setup_temporary_shallow(NULL);\n> +\t\tshallow_file = setup_temporary_shallow(NULL, 1);\n> \t\targv[arg++] = \"--shallow-file\";\n> \t\targv[arg++] = shallow_file;\n> \t}\n"},{"id":"236083","messageId":"20140305124304.GA27214@lanh","threadId":"35966","inReplyTo":"xmqqr46hltrl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] upload-pack: allow shallow fetching from a read-only repository","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-05T12:43:04Z","receivedAt":"2014-03-05T12:43:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 5, 2014 at 1:10 AM, Junio C Hamano wrote:\n> I notice that the original code, with or without this change, allows\n> upload-pack spawned by daemon to attempt to write into GIT_DIR.\n> As upload-pack is supposed to be a read-only operation, this is\n> quite bad.\n>\n> Perhaps we should give server operators an option to run their\n> daemon -> upload-pack chain to always write to a throw-away\n> directory of their choice, without ever attempting to write to\n> GIT_DIR it serves?\n\nThat would be setting TMPDIR before running git-daemon, I think.\nExcept places that we ignore TMPDIR like this one.\n\n> How well is the access to the temporary shallow file controlled in\n> your code (sorry, but I do not recall carefully reading your patch\n> that added the mechanism with security issues in mind, so now I am\n> asking)? When it is redirected to TMPDIR (let's forget GIT_DIR for\n> now---if an attacker can write into there, the repository is already\n> lost), can an attacker race with us to cause us to overwrite we do\n> not expect to?\n\nI'm sorry to say that attackers were simply not a concern when I wrote\nthe patch. Not even that upload-pack is a read-only operation (so\nobvious now that I think about this). I think racing is possible, yes.\n\n> Even if it turns out that this patch is secure enough as-is, we\n> definitely need to make sure that server operators, who want to keep\n> their upload-pack truly a read-only operation, know that it is\n> necessary to (1) keep the system user they run git-daemon under\n> incapable of writing into GIT_DIR, and (2) make sure TMPDIR points\n> at somewhere only git-daemon user and nobody else can write into,\n> somewhere in the documentation.\n\nIf only there is a way to pass this info without a temporary\nfile. Multiplexing it to pack-objects' stdin should work. It may be\nugly, but it's probably the safest way.\n\nWait it does not look that ugly. We can feed \"--shallow \" lines\nbefore sending want/have/edge objects. Something like this seems to\nwork (just ran a few shallow-related tests, not the whole test suite)\n\n-- 8< --\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c733379..130097c 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2467,6 +2467,14 @@ static void get_object_list(int ac, const char **av)\n \t\t\t\twrite_bitmap_index = 0;\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (starts_with(line, \"--shallow \")) {\n+\t\t\t\tunsigned char sha1[20];\n+\t\t\t\tif (get_sha1_hex(line + 10, sha1))\n+\t\t\t\t\tdie(\"not an SHA-1 '%s'\", line + 10);\n+\t\t\t\tregister_shallow(sha1);\n+\t\t\t\t/* XXX: set shallow.c:is_shallow = 1 ? */\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tdie(\"not a rev '%s'\", line);\n \t\t}\n \t\tif (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 0c44f6b..a5c50e4 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -70,6 +70,14 @@ static ssize_t send_client_data(int fd, const char *data, ssize_t sz)\n \treturn sz;\n }\n \n+static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n+{\n+\tFILE *fp = cb_data;\n+\tif (graft->nr_parent == -1)\n+\t\tfprintf(fp, \"--shallow %s\\n\", sha1_to_hex(graft->sha1));\n+\treturn 0;\n+}\n+\n static void create_pack_file(void)\n {\n \tstruct child_process pack_objects;\n@@ -81,12 +89,10 @@ static void create_pack_file(void)\n \tconst char *argv[12];\n \tint i, arg = 0;\n \tFILE *pipe_fd;\n-\tchar *shallow_file = NULL;\n \n \tif (shallow_nr) {\n-\t\tshallow_file = setup_temporary_shallow(NULL);\n \t\targv[arg++] = \"--shallow-file\";\n-\t\targv[arg++] = shallow_file;\n+\t\targv[arg++] = \"\";\n \t}\n \targv[arg++] = \"pack-objects\";\n \targv[arg++] = \"--revs\";\n@@ -114,6 +120,9 @@ static void create_pack_file(void)\n \n \tpipe_fd = xfdopen(pack_objects.in, \"w\");\n \n+\tif (shallow_nr)\n+\t\tfor_each_commit_graft(write_one_shallow, pipe_fd);\n+\n \tfor (i = 0; i < want_obj.nr; i++)\n \t\tfprintf(pipe_fd, \"%s\\n\",\n \t\t\tsha1_to_hex(want_obj.objects[i].item->sha1));\n@@ -242,12 +251,6 @@ static void create_pack_file(void)\n \t\terror(\"git upload-pack: git-pack-objects died with error.\");\n \t\tgoto fail;\n \t}\n-\tif (shallow_file) {\n-\t\tif (*shallow_file)\n-\t\t\tunlink(shallow_file);\n-\t\tfree(shallow_file);\n-\t}\n-\n \t/* flush the data */\n \tif (0 <= buffered) {\n \t\tdata[0] = buffered;\n-- 8< --\n-- \nDuy\n"},{"id":"236121","messageId":"xmqqha7ch1bi.fsf@gitster.dls.corp.google.com","threadId":"35966","inReplyTo":"20140305124304.GA27214@lanh","subject":"Re: [PATCH v2] upload-pack: allow shallow fetching from a read-only repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-05T19:50:41Z","receivedAt":"2014-03-05T19:50:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> If only there is a way to pass this info without a temporary\n> file. Multiplexing it to pack-objects' stdin should work. It may be\n> ugly, but it's probably the safest way.\n>\n> Wait it does not look that ugly. We can feed \"--shallow \" lines\n> before sending want/have/edge objects. Something like this seems to\n> work (just ran a few shallow-related tests, not the whole test suite)\n\nSounds like it is a much better approach to the issue.\n"},{"id":"236151","messageId":"1394095783-24402-1-git-send-email-pclouds@gmail.com","threadId":"35966","inReplyTo":"1393936205-15953-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v3] upload-pack: send shallow info over stdin to pack-objects","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-06T08:49:43Z","receivedAt":"2014-03-06T08:49:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Duy Nguyen \n\nBefore cdab485 (upload-pack: delegate rev walking in shallow fetch to\npack-objects - 2013-08-16) upload-pack does not write to the source\nrepository. cdab485 starts to write $GIT_DIR/shallow_XXXXXX if it's a\nshallow fetch, so the source repo must be writable.\n\ngit:// servers do not need write access to repos and usually don't\nhave it, which means cdab485 breaks shallow clone over git://\n\nInstead of using a temporary file as the media for shallow points, we\ncan send them over stdin to pack-objects as well. Prepend shallow\nSHA-1 with --shallow so pack-objects knows what is\nwhat.\n\nread_object_list_from_stdin() does not accept --shallow lines because\nupload-pack never uses that code. When upload-pack does, then it can\nlearn about --shallow lines.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n OK new approach, stop creating shallow_XXXXXX in upload-pack.\n\n builtin/pack-objects.c | 7 +++++++\n shallow.c | 2 ++\n t/t5537-fetch-shallow.sh | 13 +++++++++++++\n upload-pack.c | 21 ++++++++++++---------\n 4 files changed, 34 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c733379..79e848e 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2467,6 +2467,13 @@ static void get_object_list(int ac, const char **av)\n \t\t\t\twrite_bitmap_index = 0;\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (starts_with(line, \"--shallow \")) {\n+\t\t\t\tunsigned char sha1[20];\n+\t\t\t\tif (get_sha1_hex(line + 10, sha1))\n+\t\t\t\t\tdie(\"not an SHA-1 '%s'\", line + 10);\n+\t\t\t\tregister_shallow(sha1);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tdie(\"not a rev '%s'\", line);\n \t\t}\n \t\tif (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))\ndiff --git a/shallow.c b/shallow.c\nindex bbc98b5..41ff4a0 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -33,6 +33,8 @@ int register_shallow(const unsigned char *sha1)\n \tgraft->nr_parent = -1;\n \tif (commit && commit->object.parsed)\n \t\tcommit->parents = NULL;\n+\tif (is_shallow == -1)\n+\t\tis_shallow = 1;\n \treturn register_commit_graft(graft, 0);\n }\n \ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 3ae9092..a980574 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -173,4 +173,17 @@ EOF\n \t)\n '\n \n+test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '\n+\tcp -R .git read-only.git &&\n+\tfind read-only.git -print | xargs chmod -w &&\n+\ttest_when_finished \"find read-only.git -type d -print | xargs chmod +w\" &&\n+\tgit clone --no-local --depth=2 read-only.git from-read-only &&\n+\tgit --git-dir=from-read-only/.git log --format=%s >actual &&\n+\tcat >expect <nr_parent == -1)\n+\t\tfprintf(fp, \"--shallow %s\\n\", sha1_to_hex(graft->sha1));\n+\treturn 0;\n+}\n+\n static void create_pack_file(void)\n {\n \tstruct child_process pack_objects;\n@@ -81,12 +89,10 @@ static void create_pack_file(void)\n \tconst char *argv[12];\n \tint i, arg = 0;\n \tFILE *pipe_fd;\n-\tchar *shallow_file = NULL;\n \n \tif (shallow_nr) {\n-\t\tshallow_file = setup_temporary_shallow(NULL);\n \t\targv[arg++] = \"--shallow-file\";\n-\t\targv[arg++] = shallow_file;\n+\t\targv[arg++] = \"\";\n \t}\n \targv[arg++] = \"pack-objects\";\n \targv[arg++] = \"--revs\";\n@@ -114,6 +120,9 @@ static void create_pack_file(void)\n \n \tpipe_fd = xfdopen(pack_objects.in, \"w\");\n \n+\tif (shallow_nr)\n+\t\tfor_each_commit_graft(write_one_shallow, pipe_fd);\n+\n \tfor (i = 0; i < want_obj.nr; i++)\n \t\tfprintf(pipe_fd, \"%s\\n\",\n \t\t\tsha1_to_hex(want_obj.objects[i].item->sha1));\n@@ -242,12 +251,6 @@ static void create_pack_file(void)\n \t\terror(\"git upload-pack: git-pack-objects died with error.\");\n \t\tgoto fail;\n \t}\n-\tif (shallow_file) {\n-\t\tif (*shallow_file)\n-\t\t\tunlink(shallow_file);\n-\t\tfree(shallow_file);\n-\t}\n-\n \t/* flush the data */\n \tif (0 <= buffered) {\n \t\tdata[0] = buffered;\n-- \n1.9.0.66.g14f785a\n"},{"id":"236177","messageId":"xmqqfvmvyxzv.fsf@gitster.dls.corp.google.com","threadId":"35966","inReplyTo":"1394095783-24402-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-06T18:37:24Z","receivedAt":"2014-03-06T18:37:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> From: Duy Nguyen \n>\n> Before cdab485 (upload-pack: delegate rev walking in shallow fetch to\n> pack-objects - 2013-08-16) upload-pack does not write to the source\n> repository. cdab485 starts to write $GIT_DIR/shallow_XXXXXX if it's a\n> shallow fetch, so the source repo must be writable.\n>\n> git:// servers do not need write access to repos and usually don't\n> have it, which means cdab485 breaks shallow clone over git://\n>\n> Instead of using a temporary file as the media for shallow points, we\n> can send them over stdin to pack-objects as well. Prepend shallow\n> SHA-1 with --shallow so pack-objects knows what is\n> what.\n>\n> read_object_list_from_stdin() does not accept --shallow lines because\n> upload-pack never uses that code. When upload-pack does, then it can\n> learn about --shallow lines.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> OK new approach, stop creating shallow_XXXXXX in upload-pack.\n\nThanks.\n\nI like what I see in this patch, but I wonder if we can essentially\nrevert that \"temporary shallow file\" patch and replace it with the\nsame (or a similar) mechanism uniformly?\n\nOn the receive-pack side, the comment at the bottom of\npreprare_shallow_update() makes it clear that, if we wanted to use\nhooks, we cannot avoid having the proposed new shallow-file in a\ntemporary file, which is unfortunate. Do we have a similar issue on\nhooks on the upload-pack side?\n\n\n> builtin/pack-objects.c | 7 +++++++\n> shallow.c | 2 ++\n> t/t5537-fetch-shallow.sh | 13 +++++++++++++\n> upload-pack.c | 21 ++++++++++++---------\n> 4 files changed, 34 insertions(+), 9 deletions(-)\n\nNothing for Documentation/ anywhere?\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index c733379..79e848e 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -2467,6 +2467,13 @@ static void get_object_list(int ac, const char **av)\n> \t\t\t\twrite_bitmap_index = 0;\n> \t\t\t\tcontinue;\n> \t\t\t}\n> +\t\t\tif (starts_with(line, \"--shallow \")) {\n> +\t\t\t\tunsigned char sha1[20];\n> +\t\t\t\tif (get_sha1_hex(line + 10, sha1))\n> +\t\t\t\t\tdie(\"not an SHA-1 '%s'\", line + 10);\n> +\t\t\t\tregister_shallow(sha1);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> \t\t\tdie(\"not a rev '%s'\", line);\n> \t\t}\n> \t\tif (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))\n> diff --git a/shallow.c b/shallow.c\n> index bbc98b5..41ff4a0 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -33,6 +33,8 @@ int register_shallow(const unsigned char *sha1)\n> \tgraft->nr_parent = -1;\n> \tif (commit && commit->object.parsed)\n> \t\tcommit->parents = NULL;\n> +\tif (is_shallow == -1)\n> +\t\tis_shallow = 1;\n> \treturn register_commit_graft(graft, 0);\n> }\n> \n> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\n> index 3ae9092..a980574 100755\n> --- a/t/t5537-fetch-shallow.sh\n> +++ b/t/t5537-fetch-shallow.sh\n> @@ -173,4 +173,17 @@ EOF\n> \t)\n> '\n> \n> +test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '\n> +\tcp -R .git read-only.git &&\n> +\tfind read-only.git -print | xargs chmod -w &&\n> +\ttest_when_finished \"find read-only.git -type d -print | xargs chmod +w\" &&\n> +\tgit clone --no-local --depth=2 read-only.git from-read-only &&\n> +\tgit --git-dir=from-read-only/.git log --format=%s >actual &&\n> +\tcat >expect < +add-1-back\n> +4\n> +EOF\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 0c44f6b..a5c50e4 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -70,6 +70,14 @@ static ssize_t send_client_data(int fd, const char *data, ssize_t sz)\n> \treturn sz;\n> }\n> \n> +static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n> +{\n> +\tFILE *fp = cb_data;\n> +\tif (graft->nr_parent == -1)\n> +\t\tfprintf(fp, \"--shallow %s\\n\", sha1_to_hex(graft->sha1));\n> +\treturn 0;\n> +}\n> +\n> static void create_pack_file(void)\n> {\n> \tstruct child_process pack_objects;\n> @@ -81,12 +89,10 @@ static void create_pack_file(void)\n> \tconst char *argv[12];\n> \tint i, arg = 0;\n> \tFILE *pipe_fd;\n> -\tchar *shallow_file = NULL;\n> \n> \tif (shallow_nr) {\n> -\t\tshallow_file = setup_temporary_shallow(NULL);\n> \t\targv[arg++] = \"--shallow-file\";\n> -\t\targv[arg++] = shallow_file;\n> +\t\targv[arg++] = \"\";\n> \t}\n> \targv[arg++] = \"pack-objects\";\n> \targv[arg++] = \"--revs\";\n> @@ -114,6 +120,9 @@ static void create_pack_file(void)\n> \n> \tpipe_fd = xfdopen(pack_objects.in, \"w\");\n> \n> +\tif (shallow_nr)\n> +\t\tfor_each_commit_graft(write_one_shallow, pipe_fd);\n> +\n> \tfor (i = 0; i < want_obj.nr; i++)\n> \t\tfprintf(pipe_fd, \"%s\\n\",\n> \t\t\tsha1_to_hex(want_obj.objects[i].item->sha1));\n> @@ -242,12 +251,6 @@ static void create_pack_file(void)\n> \t\terror(\"git upload-pack: git-pack-objects died with error.\");\n> \t\tgoto fail;\n> \t}\n> -\tif (shallow_file) {\n> -\t\tif (*shallow_file)\n> -\t\t\tunlink(shallow_file);\n> -\t\tfree(shallow_file);\n> -\t}\n> -\n> \t/* flush the data */\n> \tif (0 <= buffered) {\n> \t\tdata[0] = buffered;\n"},{"id":"236218","messageId":"CACsJy8B8=N6R6nVa12jjhxdqxMA2eGXOV6jR-XqRRbb-6Xppdg@mail.gmail.com","threadId":"35966","inReplyTo":"xmqqfvmvyxzv.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-06T23:13:05Z","receivedAt":"2014-03-06T23:13:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 7, 2014 at 1:37 AM, Junio C Hamano wrote:\n> I like what I see in this patch, but I wonder if we can essentially\n> revert that \"temporary shallow file\" patch and replace it with the\n> same (or a similar) mechanism uniformly?\n\nUsing --shallow-file is uniform.The only downside is temporary file.\nWithout it you'll need to think of a way (probably different way for\neach command) to feed shallow info to.\n\n> On the receive-pack side, the comment at the bottom of\n> preprare_shallow_update() makes it clear that, if we wanted to use\n> hooks, we cannot avoid having the proposed new shallow-file in a\n> temporary file, which is unfortunate. Do we have a similar issue on\n> hooks on the upload-pack side?\n\nNo. I don't think we have hooks on upload-pack.\n\n>> builtin/pack-objects.c | 7 +++++++\n>> shallow.c | 2 ++\n>> t/t5537-fetch-shallow.sh | 13 +++++++++++++\n>> upload-pack.c | 21 ++++++++++++---------\n>> 4 files changed, 34 insertions(+), 9 deletions(-)\n>\n> Nothing for Documentation/ anywhere?\n\nHeh git-pack-objects.txt never described stdin format. At least I\nsearched for --not in it and found none. So I gladly accepted the\nsituation and skipped doc update :D\n-- \nDuy\n"},{"id":"236228","messageId":"CACsJy8AYWBKwUCB=pC0istGvNCNor0i-Ntp1nY5NbJGW+c2fVw@mail.gmail.com","threadId":"35966","inReplyTo":"1394095783-24402-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-07T01:24:38Z","receivedAt":"2014-03-07T01:24:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 6, 2014 at 3:49 PM, Nguyễn Thái Ngọc Duy wrote:\n> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\n> index 3ae9092..a980574 100755\n> --- a/t/t5537-fetch-shallow.sh\n> +++ b/t/t5537-fetch-shallow.sh\n> @@ -173,4 +173,17 @@ EOF\n> )\n> '\n>\n> +test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '\n> + cp -R .git read-only.git &&\n> + find read-only.git -print | xargs chmod -w &&\n> + test_when_finished \"find read-only.git -type d -print | xargs chmod +w\" &&\n> + git clone --no-local --depth=2 read-only.git from-read-only &&\n> + git --git-dir=from-read-only/.git log --format=%s >actual &&\n> + cat >expect < +add-1-back\n> +4\n> +EOF\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n\nIt's a separate issue, but maybe we should add a similar test case for\nnon-shallow clone from a read-only repo too. Are there any other\noperations that should work well on read-only repos?\n-- \nDuy\n"},{"id":"236267","messageId":"xmqq8uslvp86.fsf@gitster.dls.corp.google.com","threadId":"35966","inReplyTo":"CACsJy8B8=N6R6nVa12jjhxdqxMA2eGXOV6jR-XqRRbb-6Xppdg@mail.gmail.com","subject":"Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-07T18:27:21Z","receivedAt":"2014-03-07T18:27:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Fri, Mar 7, 2014 at 1:37 AM, Junio C Hamano wrote:\n>> I like what I see in this patch, but I wonder if we can essentially\n>> revert that \"temporary shallow file\" patch and replace it with the\n>> same (or a similar) mechanism uniformly?\n>\n> Using --shallow-file is uniform. The only downside is temporary file.\n> Without it you'll need to think of a way (probably different way for\n> each command) to feed shallow info to.\n\nYes, that is what I meant to say by the \"we need a way to tell hooks\nin some cases\" below; we are in agreement.\n\n>> On the receive-pack side, the comment at the bottom of\n>> preprare_shallow_update() makes it clear that, if we wanted to use\n>> hooks, we cannot avoid having the proposed new shallow-file in a\n>> temporary file, which is unfortunate. Do we have a similar issue on\n>> hooks on the upload-pack side?\n>\n> No. I don't think we have hooks on upload-pack.\n\nThe question was not only about \"do we happen to work OK with the\ncurrent code?\" but about \"are we closing the door for the future?\"\n\nIf we ever need to add hooks to upload-pack, with the updated\napproach, its operation will not be affected by the temporary\nshallow file tailored for this specific customer. Which I think is\na good thing in general.\n\nBut at the same time, it means that its operation cannot be\ncustomized for the specific customer, taking into account what they\nlack (which can be gleaned by looking at the temporary shallow\ninformation). I do think that it is not a big downside, but that is\nmerely my knee-jerk reaction.\n\n>> Nothing for Documentation/ anywhere?\n>\n> Heh git-pack-objects.txt never described stdin format. At least I\n> searched for --not in it and found none. So I gladly accepted the\n> situation and skipped doc update :D\n\nTo pile new technical debt on top of existing ones is to make things\nworse, which we would rather not to see.\n"},{"id":"236268","messageId":"xmqq4n39vox4.fsf@gitster.dls.corp.google.com","threadId":"35966","inReplyTo":"CACsJy8AYWBKwUCB=pC0istGvNCNor0i-Ntp1nY5NbJGW+c2fVw@mail.gmail.com","subject":"Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-07T18:33:59Z","receivedAt":"2014-03-07T18:33:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Thu, Mar 6, 2014 at 3:49 PM, Nguyễn Thái Ngọc Duy wrote:\n>> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\n>> index 3ae9092..a980574 100755\n>> --- a/t/t5537-fetch-shallow.sh\n>> +++ b/t/t5537-fetch-shallow.sh\n>> @@ -173,4 +173,17 @@ EOF\n>> )\n>> '\n>>\n>> +test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '\n>> + cp -R .git read-only.git &&\n>> + find read-only.git -print | xargs chmod -w &&\n>> + test_when_finished \"find read-only.git -type d -print | xargs chmod +w\" &&\n>> + git clone --no-local --depth=2 read-only.git from-read-only &&\n>> + git --git-dir=from-read-only/.git log --format=%s >actual &&\n>> + cat >expect <> +add-1-back\n>> +4\n>> +EOF\n>> + test_cmp expect actual\n>> +'\n>> +\n>> test_done\n>\n> It's a separate issue, but maybe we should add a similar test case for\n> non-shallow clone from a read-only repo too. Are there any other\n> operations that should work well on read-only repos?\n\nAny read-only operation ;-)?\n\nOn the object-transfer front, that would mean fetching from,\narchive-r from, and perhaps creating bundle from.\n\nLocally, log, grep, blame, gitk (but only the browsing part), etc.\n\nIf a read-write borrower borrows from a read-only location via the\nobjects/info/alternates mechanism, anying operation to the borrower\nshould also work without modifying the borrowee.\n"},{"id":"236284","messageId":"CACsJy8AcYBXi6LjyJDeEnogPTXfqYXqijXaLY=bUgNnd4cT_Fg@mail.gmail.com","threadId":"35966","inReplyTo":"xmqq8uslvp86.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-08T00:08:05Z","receivedAt":"2014-03-08T00:08:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 8, 2014 at 1:27 AM, Junio C Hamano wrote:\n>>> On the receive-pack side, the comment at the bottom of\n>>> preprare_shallow_update() makes it clear that, if we wanted to use\n>>> hooks, we cannot avoid having the proposed new shallow-file in a\n>>> temporary file, which is unfortunate. Do we have a similar issue on\n>>> hooks on the upload-pack side?\n>>\n>> No. I don't think we have hooks on upload-pack.\n>\n> The question was not only about \"do we happen to work OK with the\n> current code?\" but about \"are we closing the door for the future?\"\n>\n> If we ever need to add hooks to upload-pack, with the updated\n> approach, its operation will not be affected by the temporary\n> shallow file tailored for this specific customer. Which I think is\n> a good thing in general.\n>\n> But at the same time, it means that its operation cannot be\n> customized for the specific customer, taking into account what they\n> lack (which can be gleaned by looking at the temporary shallow\n> information). I do think that it is not a big downside, but that is\n> merely my knee-jerk reaction.\n\nWhen upload-pack learns about hooks, I think we'll need to go back\nwith --shallow-file, perhaps we a secure temporary place to write in.\nI don't see another way out. Not really sure why upload-pack needs\ncustomization though. The only case I can think of is to prevent most\nusers from fetching certain objects, but that does not sound\nrealistic..\n\n>>> Nothing for Documentation/ anywhere?\n>>\n>> Heh git-pack-objects.txt never described stdin format. At least I\n>> searched for --not in it and found none. So I gladly accepted the\n>> situation and skipped doc update :D\n>\n> To pile new technical debt on top of existing ones is to make things\n> worse, which we would rather not to see.\n\nOf course.. So what should we do with this? Go with this \"no temp\nfile\" approach and deal with hooks when they come, or prepare now and\nintroduce a secure temp. area?\n-- \nDuy\n"},{"id":"236409","messageId":"xmqqeh2arsbi.fsf@gitster.dls.corp.google.com","threadId":"35966","inReplyTo":"CACsJy8AcYBXi6LjyJDeEnogPTXfqYXqijXaLY=bUgNnd4cT_Fg@mail.gmail.com","subject":"Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-10T15:23:13Z","receivedAt":"2014-03-10T15:23:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sat, Mar 8, 2014 at 1:27 AM, Junio C Hamano wrote:\n>>>> On the receive-pack side, the comment at the bottom of\n>>>> preprare_shallow_update() makes it clear that, if we wanted to use\n>>>> hooks, we cannot avoid having the proposed new shallow-file in a\n>>>> temporary file, which is unfortunate. Do we have a similar issue on\n>>>> hooks on the upload-pack side?\n>>>\n>>> No. I don't think we have hooks on upload-pack.\n>>\n>> The question was not only about \"do we happen to work OK with the\n>> current code?\" but about \"are we closing the door for the future?\"\n>>\n>> If we ever need to add hooks to upload-pack, with the updated\n>> approach, its operation will not be affected by the temporary\n>> shallow file tailored for this specific customer. Which I think is\n>> a good thing in general.\n>>\n>> But at the same time, it means that its operation cannot be\n>> customized for the specific customer, taking into account what they\n>> lack (which can be gleaned by looking at the temporary shallow\n>> information). I do think that it is not a big downside, but that is\n>> merely my knee-jerk reaction.\n>\n> When upload-pack learns about hooks, I think we'll need to go back\n> with --shallow-file, perhaps we a secure temporary place to write in.\n> I don't see another way out. Not really sure why upload-pack needs\n> customization though. The only case I can think of is to prevent most\n> users from fetching certain objects, but that does not sound\n> realistic..\n\nI was more thinking along the lines of logging.\n\nBut you are right that we can easily revisit --shallow-file\ninterface later.\n\n> Of course.. So what should we do with this? Go with this \"no temp\n> file\" approach and deal with hooks when they come, or prepare now and\n> introduce a secure temp. area?\n\nI was rather hoping that we did not have to worry about temporary\nfiles. This patch solves the issue for the service people would\nexpect to be read-only the most, and it is a good step in the right\ndirection. So let's follow through with the approach and not worry\nabout \"secure temporary\" for now.\n"},{"id":"236502","messageId":"1394542786-2812-1-git-send-email-pclouds@gmail.com","threadId":"35966","inReplyTo":"1394095783-24402-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v4] upload-pack: send shallow info over stdin to pack-objects","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-11T12:59:46Z","receivedAt":"2014-03-11T12:59:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Before cdab485 (upload-pack: delegate rev walking in shallow fetch to\npack-objects - 2013-08-16) upload-pack does not write to the source\nrepository. cdab485 starts to write $GIT_DIR/shallow_XXXXXX if it's a\nshallow fetch, so the source repo must be writable.\n\ngit:// servers do not need write access to repos and usually don't\nhave it, which means cdab485 breaks shallow clone over git://\n\nInstead of using a temporary file as the media for shallow points, we\ncan send them over stdin to pack-objects as well. Prepend shallow\nSHA-1 with --shallow so pack-objects knows what is what.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation update and a minor tweak.\n\n Documentation/git-pack-objects.txt | 2 ++\n builtin/pack-objects.c | 10 ++++++++++\n t/t5537-fetch-shallow.sh | 13 +++++++++++++\n upload-pack.c | 21 ++++++++++++---------\n 4 files changed, 37 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex cdab9ed..d2d8f47 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -64,6 +64,8 @@ base-name::\n \tthe same way as 'git rev-list' with the `--objects` flag\n \tuses its `commit` arguments to build the list of objects it\n \toutputs. The objects on the resulting list are packed.\n+\tBesides revisions, `--not` or `--shallow ` lines are\n+\talso accepted.\n \n --unpacked::\n \tThis implies `--revs`. When processing the list of\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c733379..358f9a3 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2455,6 +2455,9 @@ static void get_object_list(int ac, const char **av)\n \tsave_commit_buffer = 0;\n \tsetup_revisions(ac, av, &revs, NULL);\n \n+\t/* make sure shallows are read */\n+\tis_repository_shallow();\n+\n \twhile (fgets(line, sizeof(line), stdin) != NULL) {\n \t\tint len = strlen(line);\n \t\tif (len && line[len - 1] == '\\n')\n@@ -2467,6 +2470,13 @@ static void get_object_list(int ac, const char **av)\n \t\t\t\twrite_bitmap_index = 0;\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (starts_with(line, \"--shallow \")) {\n+\t\t\t\tunsigned char sha1[20];\n+\t\t\t\tif (get_sha1_hex(line + 10, sha1))\n+\t\t\t\t\tdie(\"not an SHA-1 '%s'\", line + 10);\n+\t\t\t\tregister_shallow(sha1);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tdie(\"not a rev '%s'\", line);\n \t\t}\n \t\tif (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 3ae9092..a980574 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -173,4 +173,17 @@ EOF\n \t)\n '\n \n+test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '\n+\tcp -R .git read-only.git &&\n+\tfind read-only.git -print | xargs chmod -w &&\n+\ttest_when_finished \"find read-only.git -type d -print | xargs chmod +w\" &&\n+\tgit clone --no-local --depth=2 read-only.git from-read-only &&\n+\tgit --git-dir=from-read-only/.git log --format=%s >actual &&\n+\tcat >expect <nr_parent == -1)\n+\t\tfprintf(fp, \"--shallow %s\\n\", sha1_to_hex(graft->sha1));\n+\treturn 0;\n+}\n+\n static void create_pack_file(void)\n {\n \tstruct child_process pack_objects;\n@@ -81,12 +89,10 @@ static void create_pack_file(void)\n \tconst char *argv[12];\n \tint i, arg = 0;\n \tFILE *pipe_fd;\n-\tchar *shallow_file = NULL;\n \n \tif (shallow_nr) {\n-\t\tshallow_file = setup_temporary_shallow(NULL);\n \t\targv[arg++] = \"--shallow-file\";\n-\t\targv[arg++] = shallow_file;\n+\t\targv[arg++] = \"\";\n \t}\n \targv[arg++] = \"pack-objects\";\n \targv[arg++] = \"--revs\";\n@@ -114,6 +120,9 @@ static void create_pack_file(void)\n \n \tpipe_fd = xfdopen(pack_objects.in, \"w\");\n \n+\tif (shallow_nr)\n+\t\tfor_each_commit_graft(write_one_shallow, pipe_fd);\n+\n \tfor (i = 0; i < want_obj.nr; i++)\n \t\tfprintf(pipe_fd, \"%s\\n\",\n \t\t\tsha1_to_hex(want_obj.objects[i].item->sha1));\n@@ -242,12 +251,6 @@ static void create_pack_file(void)\n \t\terror(\"git upload-pack: git-pack-objects died with error.\");\n \t\tgoto fail;\n \t}\n-\tif (shallow_file) {\n-\t\tif (*shallow_file)\n-\t\t\tunlink(shallow_file);\n-\t\tfree(shallow_file);\n-\t}\n-\n \t/* flush the data */\n \tif (0 <= buffered) {\n \t\tdata[0] = buffered;\n-- \n1.9.0.40.gaa8c3ea\n"}]}