[PATCH 0/2] packfile URIs: support concurrent downloads

[PATCH 0/2] packfile URIs: support concurrent downloads

[Ted Nyman]

Packfile URI downloads currently stage a pack at
objects/pack/pack-<hash>.pack.temp. Two Git processes fetching the same
pack into one object database can append to that file concurrently,
which can corrupt the temporary pack or cause a resume request at EOF.

The first patch gives each direct packfile URI download a private
temporary file. Ordinary dumb HTTP pack requests retain their existing
resumable staging behavior. A later packfile URI retry starts a new
download.

The second patch handles the related .keep race. When another process
has already created the keep file, index-pack reports "pack<TAB><hash>"
instead of "keep<TAB><hash>". Accept both successful forms and remove
only keep files created by the current process.

Each patch adds a regression test for its respective race.

Ted Nyman (2):
  http: use unique tempfiles for packfile URI downloads
  fetch-pack: accept "pack" output for packfile URIs

 Documentation/git-http-fetch.adoc |  5 +-
 fetch-pack.c                      | 36 ++++++++-------
 http.c                            | 77 +++++++++++++++++++++----------
 http.h                            |  1 +
 t/t5550-http-fetch-dumb.sh        | 72 ++++++++++++++++++++++++++++-
 t/t5702-protocol-v2.sh            | 31 +++++++++++++
 6 files changed, 177 insertions(+), 45 deletions(-)


base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
-- 
2.55.0

[PATCH 1/2] http: use unique tempfiles for packfile URI downloads

[Ted Nyman]

Since 8d5d2a34df (http-fetch: support fetching packfiles by URL,
2020-06-10), packfile URI downloads have been staged at
objects/pack/pack-<hash>.pack.temp.

The path is derived from the advertised pack hash. Two processes
fetching the same pack into a shared object database therefore open the
same file for append. Their writes can corrupt the temporary pack. If
one process arrives after the other has completed the download, it may
instead try to resume at EOF, which some HTTP servers reject with 416.

Use the tempfile API to give direct packfile URI downloads unique
temporary files. Keep the deterministic path for ordinary dumb HTTP
pack requests, which use it to resume a partial download left by an
earlier invocation.

This means that a packfile URI download cannot be resumed by a later
invocation. A retry starts with an empty temporary file instead.

Add a test which pauses one process after downloading the pack and
starts another process using the same object database.

Signed-off-by: Ted Nyman <tnyman@openai.com>
---
 Documentation/git-http-fetch.adoc |  5 +-
 http.c                            | 77 +++++++++++++++++++++----------
 http.h                            |  1 +
 t/t5550-http-fetch-dumb.sh        | 72 ++++++++++++++++++++++++++++-
 4 files changed, 126 insertions(+), 29 deletions(-)

diff --git a/Documentation/git-http-fetch.adoc b/Documentation/git-http-fetch.adoc
index 2200f073c4..533bf381c4 100644
--- a/Documentation/git-http-fetch.adoc
+++ b/Documentation/git-http-fetch.adoc
@@ -48,9 +48,8 @@ commit-id::
 	line (which is not expected in
 	this case), 'git http-fetch' fetches the packfile directly at the given
 	URL and uses index-pack to generate corresponding .idx and .keep files.
-	The hash is used to determine the name of the temporary file and is
-	arbitrary. The output of index-pack is printed to stdout. Requires
-	--index-pack-args.
+	The hash is arbitrary. The output of index-pack is printed to stdout.
+	Requires --index-pack-args.
 
 --index-pack-args=<args>::
 	For internal use only. The command to run on the contents of the
diff --git a/http.c b/http.c
index b4e7b8d00b..5a46e7c65c 100644
--- a/http.c
+++ b/http.c
@@ -2668,7 +2668,10 @@ int http_get_info_packs(const char *base_url, struct packfile_list *packs)
 
 void release_http_pack_request(struct http_pack_request *preq)
 {
-	if (preq->packfile) {
+	if (preq->tempfile) {
+		delete_tempfile(&preq->tempfile);
+		preq->packfile = NULL;
+	} else if (preq->packfile) {
 		fclose(preq->packfile);
 		preq->packfile = NULL;
 	}
@@ -2688,7 +2691,10 @@ int finish_http_pack_request(struct http_pack_request *preq)
 	int tmpfile_fd;
 	int ret = 0;
 
-	fclose(preq->packfile);
+	if (preq->tempfile)
+		close_tempfile_gently(preq->tempfile);
+	else
+		fclose(preq->packfile);
 	preq->packfile = NULL;
 
 	tmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);
@@ -2711,7 +2717,10 @@ int finish_http_pack_request(struct http_pack_request *preq)
 
 cleanup:
 	close(tmpfile_fd);
-	unlink(preq->tmpfile.buf);
+	if (preq->tempfile)
+		delete_tempfile(&preq->tempfile);
+	else
+		unlink(preq->tmpfile.buf);
 	return ret;
 }
 
@@ -2723,20 +2732,8 @@ void http_install_packfile(struct packed_git *p,
 	packfile_store_add_pack(files->packed, p);
 }
 
-struct http_pack_request *new_http_pack_request(
-	const unsigned char *packed_git_hash, const char *base_url) {
-
-	struct strbuf buf = STRBUF_INIT;
-
-	end_url_with_slash(&buf, base_url);
-	strbuf_addf(&buf, "objects/pack/pack-%s.pack",
-		hash_to_hex(packed_git_hash));
-	return new_direct_http_pack_request(packed_git_hash,
-					    strbuf_detach(&buf, NULL));
-}
-
-struct http_pack_request *new_direct_http_pack_request(
-	const unsigned char *packed_git_hash, char *url)
+static struct http_pack_request *new_http_pack_request_for_url(
+	const unsigned char *packed_git_hash, char *url, int resumable)
 {
 	off_t prev_posn = 0;
 	struct http_pack_request *preq;
@@ -2746,9 +2743,22 @@ struct http_pack_request *new_direct_http_pack_request(
 
 	preq->url = url;
 
-	odb_pack_name(the_repository, &preq->tmpfile, packed_git_hash, "pack");
-	strbuf_addstr(&preq->tmpfile, ".temp");
-	preq->packfile = fopen(preq->tmpfile.buf, "a");
+	if (resumable) {
+		odb_pack_name(the_repository, &preq->tmpfile,
+			      packed_git_hash, "pack");
+		strbuf_addstr(&preq->tmpfile, ".temp");
+		preq->packfile = fopen(preq->tmpfile.buf, "a");
+	} else {
+		strbuf_addf(&preq->tmpfile, "%s/pack/tmp_pack_XXXXXX",
+			    repo_get_object_directory(the_repository));
+		preq->tempfile = mks_tempfile_m(preq->tmpfile.buf, 0444);
+		if (preq->tempfile) {
+			strbuf_reset(&preq->tmpfile);
+			strbuf_addstr(&preq->tmpfile,
+				      get_tempfile_path(preq->tempfile));
+			preq->packfile = fdopen_tempfile(preq->tempfile, "w");
+		}
+	}
 	if (!preq->packfile) {
 		error("Unable to open local file %s for pack",
 		      preq->tmpfile.buf);
@@ -2766,8 +2776,9 @@ struct http_pack_request *new_direct_http_pack_request(
 	 * If there is data present from a previous transfer attempt,
 	 * resume where it left off
 	 */
-	prev_posn = ftello(preq->packfile);
-	if (prev_posn>0) {
+	if (resumable)
+		prev_posn = ftello(preq->packfile);
+	if (prev_posn > 0) {
 		if (http_is_verbose)
 			fprintf(stderr,
 				"Resuming fetch of pack %s at byte %"PRIuMAX"\n",
@@ -2779,12 +2790,28 @@ struct http_pack_request *new_direct_http_pack_request(
 	return preq;
 
 abort:
-	strbuf_release(&preq->tmpfile);
-	free(preq->url);
-	free(preq);
+	release_http_pack_request(preq);
 	return NULL;
 }
 
+struct http_pack_request *new_http_pack_request(
+	const unsigned char *packed_git_hash, const char *base_url)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	end_url_with_slash(&buf, base_url);
+	strbuf_addf(&buf, "objects/pack/pack-%s.pack",
+		hash_to_hex(packed_git_hash));
+	return new_http_pack_request_for_url(packed_git_hash,
+					     strbuf_detach(&buf, NULL), 1);
+}
+
+struct http_pack_request *new_direct_http_pack_request(
+	const unsigned char *packed_git_hash, char *url)
+{
+	return new_http_pack_request_for_url(packed_git_hash, url, 0);
+}
+
 /* Helpers for fetching objects (loose) */
 static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,
 			       void *data)
diff --git a/http.h b/http.h
index 729c51904d..2c900779f5 100644
--- a/http.h
+++ b/http.h
@@ -224,6 +224,7 @@ struct http_pack_request {
 
 	FILE *packfile;
 	struct strbuf tmpfile;
+	struct tempfile *tempfile;
 	struct active_request_slot *slot;
 	struct curl_slist *headers;
 };
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index b0080bf204..314a74c433 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -293,6 +293,74 @@ test_expect_success 'http-fetch --packfile' '
 	git -C packfileclient cat-file -e "$HASH"
 '
 
+test_expect_success PIPE 'concurrent http-fetch --packfile' '
+	git init packfileclient-concurrent &&
+	HASH=$(git -C "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git rev-parse HEAD) &&
+	p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+		ls objects/pack/pack-*.pack) &&
+	packhash=$(basename "$p" .pack) &&
+	packhash=${packhash#pack-} &&
+
+	mkfifo first-ready first-continue &&
+	exec 8<>first-ready &&
+	exec 9<>first-continue &&
+	write_script git-wait-index-pack <<-\EOF &&
+	echo ready >"$GIT_TEST_WAIT_READY" &&
+	read continue <"$GIT_TEST_WAIT_CONTINUE" &&
+	exec git index-pack "$@"
+	EOF
+
+	# Hold the first download before it is indexed, so that the second
+	# download installs the pack first.
+	{
+		(
+			if ! PATH="$TRASH_DIRECTORY:$PATH" \
+			GIT_TEST_WAIT_READY="$TRASH_DIRECTORY/first-ready" \
+			GIT_TEST_WAIT_CONTINUE="$TRASH_DIRECTORY/first-continue" \
+			git -C packfileclient-concurrent http-fetch \
+				--packfile="$packhash" \
+				--index-pack-arg=wait-index-pack \
+				--index-pack-arg=--stdin \
+				--index-pack-arg=--keep \
+				"$HTTPD_URL/dumb/repo_pack.git/$p" >first.out
+			then
+				echo failed >"$TRASH_DIRECTORY/first-ready" &&
+				exit 1
+			fi
+		) &
+		first_pid=$!
+	} &&
+	test_when_finished "
+		echo continue >&9
+		wait $first_pid 2>/dev/null || :
+		exec 8>&-
+		exec 9>&-
+		rm -f first-ready first-continue git-wait-index-pack
+	" &&
+
+	read ready <&8 &&
+	test "$ready" = ready &&
+	git -C packfileclient-concurrent http-fetch \
+		--packfile="$packhash" \
+		--index-pack-arg=index-pack \
+		--index-pack-arg=--stdin \
+		--index-pack-arg=--keep \
+		"$HTTPD_URL/dumb/repo_pack.git/$p" >second.out &&
+	echo continue >&9 &&
+	wait "$first_pid" &&
+
+	printf "pack\t%s\n" "$packhash" >expect &&
+	test_cmp expect first.out &&
+	printf "keep\t%s\n" "$packhash" >expect &&
+	test_cmp expect second.out &&
+	test_path_is_missing \
+		"packfileclient-concurrent/.git/objects/pack/pack-$packhash.pack.temp" &&
+	find packfileclient-concurrent/.git/objects/pack \
+		-name "tmp_pack_*" -print >tmpfiles &&
+	test_must_be_empty tmpfiles &&
+	git -C packfileclient-concurrent cat-file -e "$HASH"
+'
+
 test_expect_success 'fetch notices corrupt pack' '
 	cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
 	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
@@ -313,7 +381,9 @@ test_expect_success 'http-fetch --packfile with corrupt pack' '
 	git init packfileclient &&
 	p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git && ls objects/pack/pack-*.pack) &&
 	test_must_fail git -C packfileclient http-fetch --packfile \
-		"$HTTPD_URL"/dumb/repo_bad1.git/$p
+		"$HTTPD_URL"/dumb/repo_bad1.git/$p &&
+	find packfileclient/.git/objects/pack -name "tmp_pack_*" -print >tmpfiles &&
+	test_must_be_empty tmpfiles
 '
 
 test_expect_success 'fetch notices corrupt idx' '
-- 
2.55.0

Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads

[Junio C Hamano]

Ted Nyman <tnyman@openai.com> writes:

> Since 8d5d2a34df (http-fetch: support fetching packfiles by URL,
> 2020-06-10), packfile URI downloads have been staged at
> objects/pack/pack-<hash>.pack.temp.
>
> The path is derived from the advertised pack hash. Two processes
> fetching the same pack into a shared object database therefore open the
> same file for append. Their writes can corrupt the temporary pack. If
> one process arrives after the other has completed the download, it may
> instead try to resume at EOF, which some HTTP servers reject with 416.
>
> Use the tempfile API to give direct packfile URI downloads unique
> temporary files. Keep the deterministic path for ordinary dumb HTTP
> pack requests, which use it to resume a partial download left by an
> earlier invocation.
>
> This means that a packfile URI download cannot be resumed by a later
> invocation. A retry starts with an empty temporary file instead.

While that does sound like a safe and correct approach, stepping
back briefly, would it not be wasteful for the second process to
download the same packfile that the first has already started
downloading?

Are there better ways for these processes to coordinate with each
other?  Instead of appending to the file, what if the second process
uses a predictable temporary name (which we already use) to open a
new file with O_CREAT | O_EXCL to avoid this redundant work?  If the
open call fails because the file already exists, the second process
can detect that another process is active and wait for it to finish
rather than initiating its own network request.

Doing so might require setting up a trigger or polling mechanism to
wait for the first process's download to complete (and detecting if
the other process dies without cleaning up), though that may open a
can of worms.

Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads

[Ted Nyman]

> While that does sound like a safe and correct approach, stepping
> back briefly, would it not be wasteful for the second process to
> download the same packfile that the first has already started
> downloading?

Yes. If two fetches overlap, the second download is redundant.

> Are there better ways for these processes to coordinate with each
> other? Instead of appending to the file, what if the second process
> uses a predictable temporary name (which we already use) to open a
> new file with O_CREAT | O_EXCL to avoid this redundant work?

Using the existing pack-<hash>.pack.temp name with O_CREAT | O_EXCL
would prevent concurrent writes, but EEXIST alone would not
distinguish an in-progress download from one left by an earlier
failed or interrupted invocation. The existing .pack.temp name is not
covered by the tmp_* pruning path, so simply waiting for it to
disappear could leave a fetch stuck after a crash.

The waiting case would also need a complete handoff. If the first
process finishes, the second would need to notice the installed pack
and account for the expected index-pack result and keep state. If the
first process fails and removes its temporary file, the second would
need to retry as the downloader. That is possible, but introduces
cross-process coordination and a timeout policy in http-fetch.

The unique tempfile preserves the existing "download, index, then
install" behavior for each invocation and fixes both the
concurrent-append and EOF-resume failures. Avoiding the duplicate
transfer would be useful for large packs, but I would prefer to keep
that as a follow-up unless you think it is necessary for this
correctness fix.

Thanks,
Ted

Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads

[Taylor Blau]

On Mon, Jul 13, 2026 at 06:58:24PM -0700, Ted Nyman wrote:
> > While that does sound like a safe and correct approach, stepping
> > back briefly, would it not be wasteful for the second process to
> > download the same packfile that the first has already started
> > downloading?
>
> Yes. If two fetches overlap, the second download is redundant.
>
> > Are there better ways for these processes to coordinate with each
> > other? Instead of appending to the file, what if the second process
> > uses a predictable temporary name (which we already use) to open a
> > new file with O_CREAT | O_EXCL to avoid this redundant work?
>
> Using the existing pack-<hash>.pack.temp name with O_CREAT | O_EXCL
> would prevent concurrent writes, but EEXIST alone would not
> distinguish an in-progress download from one left by an earlier
> failed or interrupted invocation. The existing .pack.temp name is not
> covered by the tmp_* pruning path, so simply waiting for it to
> disappear could leave a fetch stuck after a crash.

Exactly. If two processes are downloading the same pack at the same time
to different locations, the effort is of course redundant. But I don't
think we can reliably distinguish between that case and one where an
earlier process died in the middle of downloading a pack but was unable
to clean up after itself.

Thanks,
Taylor

Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads

[Jeff King]

On Mon, Jul 13, 2026 at 06:58:24PM -0700, Ted Nyman wrote:

> > Are there better ways for these processes to coordinate with each
> > other? Instead of appending to the file, what if the second process
> > uses a predictable temporary name (which we already use) to open a
> > new file with O_CREAT | O_EXCL to avoid this redundant work?
> 
> Using the existing pack-<hash>.pack.temp name with O_CREAT | O_EXCL
> would prevent concurrent writes, but EEXIST alone would not
> distinguish an in-progress download from one left by an earlier
> failed or interrupted invocation. The existing .pack.temp name is not
> covered by the tmp_* pruning path, so simply waiting for it to
> disappear could leave a fetch stuck after a crash.

A few thoughts:

  - Using O_EXCL makes this essentially a lockfile. So we could apply
    the logic used elsewhere for lockfiles, like auto-removing files
    with ancient mtimes. Or we could even go all-in with a pid check for
    liveness; most of Git's lockfiles don't do that, but at least one
    does (the background auto-gc lock).

  - If we're not already using a name which is auto-cleaned during
    maintenance, we probably ought to be. Leaving aside concurrency
    issues, nobody would ever clean up the on-disk cruft.

    But of course the original code here is intentionally _not_ using a
    name we'd clean up, because it wants to be able to resume an
    interrupted transfer.  And you're explicitly breaking that for the
    packfile URI case.

    Is that a cost we're OK with paying? Fixing it opens up that same
    coordination can of worms. You have to tell the difference a
    concurrent writer and a previous dead one (whose work you can
    resume).

    It does feel weird that we'd do one thing for dumb-http and another
    for packfile URIs. Wouldn't they suffer from the same concurrency
    and resumption problems?

> The unique tempfile preserves the existing "download, index, then
> install" behavior for each invocation and fixes both the
> concurrent-append and EOF-resume failures. Avoiding the duplicate
> transfer would be useful for large packs, but I would prefer to keep
> that as a follow-up unless you think it is necessary for this
> correctness fix.

If we're OK with killing the ability to resume, then yeah, I think it
would make sense to start simple and un-break things. And then put a
coordination layer on top later (or never if nobody cares enough).

-Peff

Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads

[Taylor Blau]

On Mon, Jul 13, 2026 at 03:34:33PM -0700, Ted Nyman wrote:
> Since 8d5d2a34df (http-fetch: support fetching packfiles by URL,
> 2020-06-10), packfile URI downloads have been staged at
> objects/pack/pack-<hash>.pack.temp.
>
> The path is derived from the advertised pack hash. Two processes
> fetching the same pack into a shared object database therefore open the
> same file for append. Their writes can corrupt the temporary pack. If
> one process arrives after the other has completed the download, it may
> instead try to resume at EOF, which some HTTP servers reject with 416.
>
> Use the tempfile API to give direct packfile URI downloads unique
> temporary files. Keep the deterministic path for ordinary dumb HTTP
> pack requests, which use it to resume a partial download left by an
> earlier invocation.
>
> This means that a packfile URI download cannot be resumed by a later
> invocation. A retry starts with an empty temporary file instead.
>
> Add a test which pauses one process after downloading the pack and
> starts another process using the same object database.
>
> Signed-off-by: Ted Nyman <tnyman@openai.com>
> ---
>  Documentation/git-http-fetch.adoc |  5 +-
>  http.c                            | 77 +++++++++++++++++++++----------
>  http.h                            |  1 +
>  t/t5550-http-fetch-dumb.sh        | 72 ++++++++++++++++++++++++++++-
>  4 files changed, 126 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/git-http-fetch.adoc b/Documentation/git-http-fetch.adoc
> index 2200f073c4..533bf381c4 100644
> --- a/Documentation/git-http-fetch.adoc
> +++ b/Documentation/git-http-fetch.adoc
> @@ -48,9 +48,8 @@ commit-id::
>  	line (which is not expected in
>  	this case), 'git http-fetch' fetches the packfile directly at the given
>  	URL and uses index-pack to generate corresponding .idx and .keep files.
> -	The hash is used to determine the name of the temporary file and is
> -	arbitrary. The output of index-pack is printed to stdout. Requires
> -	--index-pack-args.
> +	The hash is arbitrary. The output of index-pack is printed to stdout.
> +	Requires --index-pack-args.
>
>  --index-pack-args=<args>::
>  	For internal use only. The command to run on the contents of the
> diff --git a/http.c b/http.c
> index b4e7b8d00b..5a46e7c65c 100644
> --- a/http.c
> +++ b/http.c
> @@ -2668,7 +2668,10 @@ int http_get_info_packs(const char *base_url, struct packfile_list *packs)
>
>  void release_http_pack_request(struct http_pack_request *preq)
>  {
> -	if (preq->packfile) {
> +	if (preq->tempfile) {
> +		delete_tempfile(&preq->tempfile);
> +		preq->packfile = NULL;

We should be able to drop the assignment to NULL on the second line,
since `delete_tempfile()` takes a double pointer to the 'struct
packfile' and NULL's it out for us.

(The other callers appear to avoid explicitly setting `preq->tempfile`
to NULL.)

The rest of the patch looks good to me.

> diff --git a/http.h b/http.h
> index 729c51904d..2c900779f5 100644
> --- a/http.h
> +++ b/http.h
> @@ -224,6 +224,7 @@ struct http_pack_request {
>
>  	FILE *packfile;
>  	struct strbuf tmpfile;
> +	struct tempfile *tempfile;
>  	struct active_request_slot *slot;
>  	struct curl_slist *headers;
>  };
> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> index b0080bf204..314a74c433 100755
> --- a/t/t5550-http-fetch-dumb.sh
> +++ b/t/t5550-http-fetch-dumb.sh
> @@ -293,6 +293,74 @@ test_expect_success 'http-fetch --packfile' '
>  	git -C packfileclient cat-file -e "$HASH"
>  '
>
> +test_expect_success PIPE 'concurrent http-fetch --packfile' '

Phew ;-).

This is definitely tricky to test, but what you wrote here looks
plausibly correct to me.

Thanks,
Taylor

Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads

[Jeff King]

On Mon, Jul 13, 2026 at 09:06:12PM -0700, Taylor Blau wrote:

> >  void release_http_pack_request(struct http_pack_request *preq)
> >  {
> > -	if (preq->packfile) {
> > +	if (preq->tempfile) {
> > +		delete_tempfile(&preq->tempfile);
> > +		preq->packfile = NULL;
> 
> We should be able to drop the assignment to NULL on the second line,
> since `delete_tempfile()` takes a double pointer to the 'struct
> packfile' and NULL's it out for us.
> 
> (The other callers appear to avoid explicitly setting `preq->tempfile`
> to NULL.)

It takes a double-pointer to the "struct tempfile"; the NULL assignment
is to the "packfile" member, which is the FILE handle.

I thought at first this was buggy; we still call fdopen() on the
tempfile and assign the result to preq->packfile, even in the new
non-resumable case. Don't we need to fclose() it? But the answer is no:
fdopen_tempfile() retains ownership of the result, storing it in
tempfile.fp. So it will be correctly closed during delete_tempfile(),
and in fact we must _not_ fclose it again.

But assigning NULL can happen with either style. So doing it
unconditionally like:

  if (preq->tempfile)
	delete_tempfile(&preq->tempfile);
  else if (preq->packfile)
	fclose(preq->packfile);
  preq->packfile = NULL;

makes more sense, as it is done in finish_http_pack_request(). It might
even make sense to add a comment explaining why we don't need to
fclose() in the first part of the conditional.

All that said, I do not think setting it to NULL matters at all here,
since the function ends with free(preq). So just dropping the NULL would
perhaps be more clear.

-Peff

Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads

[Jeff King]

On Mon, Jul 13, 2026 at 03:34:33PM -0700, Ted Nyman wrote:

> The path is derived from the advertised pack hash. Two processes
> fetching the same pack into a shared object database therefore open the
> same file for append. Their writes can corrupt the temporary pack. If
> one process arrives after the other has completed the download, it may
> instead try to resume at EOF, which some HTTP servers reject with 416.

Yuck. In theory they're writing the same thing, but I think the source
of the corruption is append mode. Two concurrent writers will keep
auto-seeking to the end of the file, rather than keeping their own file
pointers. There's no way to ask for O_APPEND without O_TRUNC via stdio,
but we can drop down a level like this:

diff --git a/http.c b/http.c
index b4e7b8d00b..d7362c99a2 100644
--- a/http.c
+++ b/http.c
@@ -2740,6 +2740,7 @@ struct http_pack_request *new_direct_http_pack_request(
 {
 	off_t prev_posn = 0;
 	struct http_pack_request *preq;
+	int fd;
 
 	CALLOC_ARRAY(preq, 1);
 	strbuf_init(&preq->tmpfile, 0);
@@ -2748,12 +2749,13 @@ struct http_pack_request *new_direct_http_pack_request(
 
 	odb_pack_name(the_repository, &preq->tmpfile, packed_git_hash, "pack");
 	strbuf_addstr(&preq->tmpfile, ".temp");
-	preq->packfile = fopen(preq->tmpfile.buf, "a");
-	if (!preq->packfile) {
+	fd = open(preq->tmpfile.buf, O_WRONLY|O_CREAT, 0666);
+	if (fd < 0) {
 		error("Unable to open local file %s for pack",
 		      preq->tmpfile.buf);
 		goto abort;
 	}
+	preq->packfile = xfdopen(fd, "w");
 
 	preq->slot = get_active_slot();
 	preq->headers = object_request_headers();

That patch (with no other code changes) passes your test.

I suspect it could cause us to racily send an http range of "N-" to the
server, where N is the total number of bytes in the file (because we
don't know how many bytes there are supposed to be). I don't know if
that would cause an HTTP 416 or not. I think possibly not, and the 416
you saw (and that I see when running the test without any code changes)
might be from sending a range that starts _past_ N. We end up with a
too-long when both processes are appending.

I can't say I love the overall notion of "two processes are writing the
same data, it will probably be fine!". There might be portability
issues, and I'm not sure what would happen if we ever did get
conflicting data. If we're just feeding this to "index-pack --stdin"
we'd at least notice the problem (rather than quietly corrupting the
indexed file!).

So I'm offering this as a point for further discussion, and not
necessarily a counter-proposal. ;)

> Use the tempfile API to give direct packfile URI downloads unique
> temporary files. Keep the deterministic path for ordinary dumb HTTP
> pack requests, which use it to resume a partial download left by an
> earlier invocation.
> 
> This means that a packfile URI download cannot be resumed by a later
> invocation. A retry starts with an empty temporary file instead.

Arguably losing the ability to retry is a regression. In general, I
think we should prefer correctness to efficiency. But I wonder if this
is a case where the user might want to make the choice to say "I am not
going to fetch two packfiles at once; please enable resumable fetches".
Especially because one of the selling points of packfile URIs is that
they are resumable.

One other thought on resumable transfers: if we are not going to resume
the transfer, then why spool the pack to disk at all? In other words,
why not just send it straight to "index-pack --stdin". That fixes your
concurrency issue (because it uses its own tempfiles behind the scene),
but has two other big advantages:

  1. It halves the number of disk writes, and lowers the peak disk usage
     (with the current code, there is a moment where both the tempfile
     and the indexed pack are present on disk).

  2. It pipelines the data processing. The current code bottlenecks on
     the network while the CPU sits idle, and then bottlenecks on the
     CPU once we have the whole file. We could be doing useful CPU work
     during the network transfer, just like a regular pack code does.


So I'm not quite sold on losing the ability to resume entirely. And in
cases where we do lose it, I think it opens up other improvements.

But I'll reader over the rest of the patch with the notion that this is
the direction we want to go in.

> diff --git a/Documentation/git-http-fetch.adoc b/Documentation/git-http-fetch.adoc
> index 2200f073c4..533bf381c4 100644
> --- a/Documentation/git-http-fetch.adoc
> +++ b/Documentation/git-http-fetch.adoc
> @@ -48,9 +48,8 @@ commit-id::
>  	line (which is not expected in
>  	this case), 'git http-fetch' fetches the packfile directly at the given
>  	URL and uses index-pack to generate corresponding .idx and .keep files.
> -	The hash is used to determine the name of the temporary file and is
> -	arbitrary. The output of index-pack is printed to stdout. Requires
> -	--index-pack-args.
> +	The hash is arbitrary. The output of index-pack is printed to stdout.
> +	Requires --index-pack-args.

Do we even need to provide a hash anymore? After your patch I don't
think we even use it. It might be worth keeping around, though, as it
would be a unique key for de-duping or resuming, if we ever did
implement those on top.

>  void release_http_pack_request(struct http_pack_request *preq)
>  {
> -	if (preq->packfile) {
> +	if (preq->tempfile) {
> +		delete_tempfile(&preq->tempfile);
> +		preq->packfile = NULL;
> +	} else if (preq->packfile) {
>  		fclose(preq->packfile);
>  		preq->packfile = NULL;
>  	}

OK. I think this is correct, though see my comments elsewhere in the
thread.

> @@ -2688,7 +2691,10 @@ int finish_http_pack_request(struct http_pack_request *preq)
>  	int tmpfile_fd;
>  	int ret = 0;
>  
> -	fclose(preq->packfile);
> +	if (preq->tempfile)
> +		close_tempfile_gently(preq->tempfile);
> +	else
> +		fclose(preq->packfile);
>  	preq->packfile = NULL;

OK, and this is correct because preq->packfile is just an alias for
preq->tempfile.fp when the tempfile is valid. The NULL assignment is
important here so that the release() function doesn't double-free.

> -struct http_pack_request *new_http_pack_request(
> -	const unsigned char *packed_git_hash, const char *base_url) {
> -
> -	struct strbuf buf = STRBUF_INIT;
> -
> -	end_url_with_slash(&buf, base_url);
> -	strbuf_addf(&buf, "objects/pack/pack-%s.pack",
> -		hash_to_hex(packed_git_hash));
> -	return new_direct_http_pack_request(packed_git_hash,
> -					    strbuf_detach(&buf, NULL));
> -}

This hunk puzzled me at first, but it's because we used to just be a
wrapper for the "direct" variant, and now the two will share a single
static helper. That might have been a little more clear as a preparatory
patch, but OK.

> +	if (resumable) {
> +		odb_pack_name(the_repository, &preq->tmpfile,
> +			      packed_git_hash, "pack");
> +		strbuf_addstr(&preq->tmpfile, ".temp");
> +		preq->packfile = fopen(preq->tmpfile.buf, "a");
> +	} else {
> +		strbuf_addf(&preq->tmpfile, "%s/pack/tmp_pack_XXXXXX",
> +			    repo_get_object_directory(the_repository));
> +		preq->tempfile = mks_tempfile_m(preq->tmpfile.buf, 0444);
> +		if (preq->tempfile) {
> +			strbuf_reset(&preq->tmpfile);
> +			strbuf_addstr(&preq->tmpfile,
> +				      get_tempfile_path(preq->tempfile));
> +			preq->packfile = fdopen_tempfile(preq->tempfile, "w");
> +		}
> +	}
>  	if (!preq->packfile) {
>  		error("Unable to open local file %s for pack",
>  		      preq->tmpfile.buf);

OK, and this is the meat of the change. We usually use odb_mkstemp() for
tmp_pack_* files, but that annoyingly doesn't give you a tempfile
struct. So setting up your own filename and using mks_tempfile_m() makes
sense here.

The error path is a little funny, but we catch it in the context when
preq->packfile is NULL. Good.

> @@ -2766,8 +2776,9 @@ struct http_pack_request *new_direct_http_pack_request(
>  	 * If there is data present from a previous transfer attempt,
>  	 * resume where it left off
>  	 */
> -	prev_posn = ftello(preq->packfile);
> -	if (prev_posn>0) {
> +	if (resumable)
> +		prev_posn = ftello(preq->packfile);
> +	if (prev_posn > 0) {

I think this is not technically necessary, as ftello() would just return
"0" for our newly-created file. But it does make the intent clear.

> @@ -2779,12 +2790,28 @@ struct http_pack_request *new_direct_http_pack_request(
>  	return preq;
>  
>  abort:
> -	strbuf_release(&preq->tmpfile);
> -	free(preq->url);
> -	free(preq);
> +	release_http_pack_request(preq);
>  	return NULL;
>  }

OK, now we have potentially more to free, so we rely on the release
function. That could cause problems if we jump to this abort label when
the struct isn't fully initialized. I think it is OK, though. We zero
the whole thing, so the extra fields that the release() function
considers will just be ignored.

> diff --git a/http.h b/http.h
> index 729c51904d..2c900779f5 100644
> --- a/http.h
> +++ b/http.h
> @@ -224,6 +224,7 @@ struct http_pack_request {
>  
>  	FILE *packfile;
>  	struct strbuf tmpfile;
> +	struct tempfile *tempfile;
>  	struct active_request_slot *slot;
>  	struct curl_slist *headers;

Yuck, now we have "tempfile" and "tmpfile" with two different types and
totally different semantics (and even when "tempfile" is in use,
"tmpfile" is still meaningful!).

Can we even just call the second one non_resumable_tempfile or
something? It's a mouthful, but it makes it less likely to confuse the
two.

> +	# Hold the first download before it is indexed, so that the second
> +	# download installs the pack first.
> +	{
> +		(
> +			if ! PATH="$TRASH_DIRECTORY:$PATH" \
> +			GIT_TEST_WAIT_READY="$TRASH_DIRECTORY/first-ready" \
> +			GIT_TEST_WAIT_CONTINUE="$TRASH_DIRECTORY/first-continue" \
> +			git -C packfileclient-concurrent http-fetch \
> +				--packfile="$packhash" \
> +				--index-pack-arg=wait-index-pack \
> +				--index-pack-arg=--stdin \
> +				--index-pack-arg=--keep \
> +				"$HTTPD_URL/dumb/repo_pack.git/$p" >first.out
> +			then
> +				echo failed >"$TRASH_DIRECTORY/first-ready" &&
> +				exit 1
> +			fi
> +		) &
> +		first_pid=$!
> +	} &&

OK. I wonder if it would be simpler and a more robust test if rather
than writing the correct bytes (and then waiting), the first process
just wrote total garbage. Then we'd be sure the other process is not
reading it, because it would definitely corrupt their input.

I dunno. This is a more realistic scenario, so in that sense maybe it is
more interesting.

> +	test_when_finished "
> +		echo continue >&9
> +		wait $first_pid 2>/dev/null || :
> +		exec 8>&-
> +		exec 9>&-
> +		rm -f first-ready first-continue git-wait-index-pack
> +	" &&
> [...]

The rest of the fifo handling looks plausibly correct. This is a tricky
area and it's common to introduce funky races, but I didn't see anything
wrong, and it passed a few dozen rounds of --stress.

> @@ -313,7 +381,9 @@ test_expect_success 'http-fetch --packfile with corrupt pack' '
>  	git init packfileclient &&
>  	p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git && ls objects/pack/pack-*.pack) &&
>  	test_must_fail git -C packfileclient http-fetch --packfile \
> -		"$HTTPD_URL"/dumb/repo_bad1.git/$p
> +		"$HTTPD_URL"/dumb/repo_bad1.git/$p &&
> +	find packfileclient/.git/objects/pack -name "tmp_pack_*" -print >tmpfiles &&
> +	test_must_be_empty tmpfiles
>  '

OK, so here we just detect that we cleaned up after ourselves. Makes
sense.

-Peff

[PATCH 2/2] fetch-pack: accept "pack" output for packfile URIs

[Ted Nyman]

When "index-pack --keep" creates a .keep file, it reports
"keep<TAB><hash>". If the file already exists, index-pack leaves it
untouched and reports "pack<TAB><hash>" instead.

Since dd4b732df7 (upload-pack: send part of packfile response as uri,
2020-06-10), fetch-pack has accepted only the "keep" form for packs
downloaded through packfile URIs. A concurrent fetch can install the
same pack and create its .keep file before another process reaches
index-pack. The latter process then fails even though index-pack
completed successfully.

Accept both successful forms. Add a path to pack_lockfiles only for the
"keep" form, so cleanup removes only a keep file created by the current
process and preserves a pre-existing one.

Add a regression test which pre-creates a keep file and verifies that a
fetch succeeds without changing it.

Signed-off-by: Ted Nyman <tnyman@openai.com>
---
 fetch-pack.c           | 36 ++++++++++++++++++++----------------
 t/t5702-protocol-v2.sh | 31 +++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 16 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 120e01f3cf..a16b80177a 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1887,9 +1887,12 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	}
 
 	for (i = 0; i < packfile_uris.nr; i++) {
+		int created_keep = 0;
 		int j;
 		struct child_process cmd = CHILD_PROCESS_INIT;
-		char packname[GIT_MAX_HEXSZ + 1];
+		char packname[GIT_MAX_HEXSZ + 6];
+		const char *packhash;
+		const int packname_len = the_hash_algo->hexsz + 6;
 		const char *uri = packfile_uris.items[i].string +
 			the_hash_algo->hexsz + 1;
 
@@ -1907,16 +1910,16 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		if (start_command(&cmd))
 			die("fetch-pack: unable to spawn http-fetch");
 
-		if (read_in_full(cmd.out, packname, 5) < 0 ||
-		    memcmp(packname, "keep\t", 5))
-			die("fetch-pack: expected keep then TAB at start of http-fetch output");
-
-		if (read_in_full(cmd.out, packname,
-				 the_hash_algo->hexsz + 1) < 0 ||
-		    packname[the_hash_algo->hexsz] != '\n')
-			die("fetch-pack: expected hash then LF at end of http-fetch output");
-
-		packname[the_hash_algo->hexsz] = '\0';
+		if (read_in_full(cmd.out, packname, packname_len) != packname_len ||
+		    packname[packname_len - 1] != '\n')
+			die("fetch-pack: expected pack or keep, TAB, hash, "
+			    "then LF in http-fetch output");
+		packname[packname_len - 1] = '\0';
+		if (skip_prefix(packname, "keep\t", &packhash))
+			created_keep = 1;
+		else if (!skip_prefix(packname, "pack\t", &packhash))
+			die("fetch-pack: expected pack or keep, TAB, hash, "
+			    "then LF in http-fetch output");
 
 		parse_gitmodules_oids(cmd.out, &fsck_options.gitmodules_found);
 
@@ -1925,16 +1928,17 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		if (finish_command(&cmd))
 			die("fetch-pack: unable to finish http-fetch");
 
-		if (memcmp(packfile_uris.items[i].string, packname,
+		if (memcmp(packfile_uris.items[i].string, packhash,
 			   the_hash_algo->hexsz))
 			die("fetch-pack: pack downloaded from %s does not match expected hash %.*s",
 			    uri, (int) the_hash_algo->hexsz,
 			    packfile_uris.items[i].string);
 
-		string_list_append_nodup(pack_lockfiles,
-					 xstrfmt("%s/pack/pack-%s.keep",
-						 repo_get_object_directory(the_repository),
-						 packname));
+		if (created_keep)
+			string_list_append_nodup(pack_lockfiles,
+						 xstrfmt("%s/pack/pack-%s.keep",
+							 repo_get_object_directory(the_repository),
+							 packhash));
 	}
 	string_list_clear(&packfile_uris, 0);
 	strvec_clear(&index_pack_args);
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 9f6cf4142d..1861eb7d7c 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -1291,6 +1291,37 @@ test_expect_success 'packfile URIs with fetch instead of clone' '
 		fetch "$HTTPD_URL/smart/http_parent"
 '
 
+test_expect_success 'packfile URI preserves an existing keep file' '
+	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
+	rm -rf "$P" http_child keep.expect &&
+
+	git init "$P" &&
+	git -C "$P" config uploadpack.allowsidebandall true &&
+
+	echo my-blob >"$P/my-blob" &&
+	git -C "$P" add my-blob &&
+	git -C "$P" commit -m x &&
+	configure_exclusion "$P" my-blob >h &&
+
+	git init http_child &&
+	packhash=$(cat packh) &&
+	keep="http_child/.git/objects/pack/pack-$packhash.keep" &&
+	echo pre-existing >"$keep" &&
+	cp "$keep" keep.expect &&
+
+	GIT_TEST_SIDEBAND_ALL=1 \
+	git -C http_child -c protocol.version=2 \
+		-c fetch.uriprotocols=http,https \
+		fetch "$HTTPD_URL/smart/http_parent" &&
+
+	test_path_is_file \
+		"http_child/.git/objects/pack/pack-$packhash.pack" &&
+	test_path_is_file \
+		"http_child/.git/objects/pack/pack-$packhash.idx" &&
+	test_cmp keep.expect "$keep" &&
+	git -C http_child cat-file -e "$(cat h)"
+'
+
 test_expect_success 'fetching with valid packfile URI but invalid hash fails' '
 	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
 	rm -rf "$P" http_child log &&
-- 
2.55.0

Re: [PATCH 2/2] fetch-pack: accept "pack" output for packfile URIs

[Jeff King]

On Mon, Jul 13, 2026 at 03:34:43PM -0700, Ted Nyman wrote:

> When "index-pack --keep" creates a .keep file, it reports
> "keep<TAB><hash>". If the file already exists, index-pack leaves it
> untouched and reports "pack<TAB><hash>" instead.
> 
> Since dd4b732df7 (upload-pack: send part of packfile response as uri,
> 2020-06-10), fetch-pack has accepted only the "keep" form for packs
> downloaded through packfile URIs. A concurrent fetch can install the
> same pack and create its .keep file before another process reaches
> index-pack. The latter process then fails even though index-pack
> completed successfully.
> 
> Accept both successful forms. Add a path to pack_lockfiles only for the
> "keep" form, so cleanup removes only a keep file created by the current
> process and preserves a pre-existing one.

OK, that all makes sense.

>  	for (i = 0; i < packfile_uris.nr; i++) {
> +		int created_keep = 0;
>  		int j;
>  		struct child_process cmd = CHILD_PROCESS_INIT;
> -		char packname[GIT_MAX_HEXSZ + 1];
> +		char packname[GIT_MAX_HEXSZ + 6];
> +		const char *packhash;
> +		const int packname_len = the_hash_algo->hexsz + 6;

The "+ 6" here is gross, but not really any more than the bare "5" in
the original code.

Calling it "packhash" made me wonder about this line of code:

> -		if (memcmp(packfile_uris.items[i].string, packname,
> +		if (memcmp(packfile_uris.items[i].string, packhash,

Surely we need to change more than this if we now have the hash rather
than the whole packname? But no, the original code was really just
storing the hash in packname.

Which was rather misleading, but it is not much better after your patch.
Now packname still just has the packhash, along with the extra keep/pack
marker.

Would a more generic name like "cmd_output" or something make sense? I
also think this would all be much nicer with a strbuf (which would let
us get rid of the magic numbers), but that is a slightly larger
refactor:

diff --git a/fetch-pack.c b/fetch-pack.c
index 1e8461d07e..5f94f35c30 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1890,9 +1890,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		int created_keep = 0;
 		int j;
 		struct child_process cmd = CHILD_PROCESS_INIT;
-		char packname[GIT_MAX_HEXSZ + 6];
+		struct strbuf cmd_output = STRBUF_INIT;
 		const char *packhash;
-		const int packname_len = the_hash_algo->hexsz + 6;
 		const char *uri = packfile_uris.items[i].string +
 			the_hash_algo->hexsz + 1;
 
@@ -1910,14 +1909,11 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		if (start_command(&cmd))
 			die("fetch-pack: unable to spawn http-fetch");
 
-		if (read_in_full(cmd.out, packname, packname_len) != packname_len ||
-		    packname[packname_len - 1] != '\n')
-			die("fetch-pack: expected pack or keep, TAB, hash, "
-			    "then LF in http-fetch output");
-		packname[packname_len - 1] = '\0';
-		if (skip_prefix(packname, "keep\t", &packhash))
+		if (strbuf_read(&cmd_output, cmd.out, 0) < 0)
+			die("failed to read http-fetch output");
+		if (skip_prefix(cmd_output.buf, "keep\t", &packhash))
 			created_keep = 1;
-		else if (!skip_prefix(packname, "pack\t", &packhash))
+		else if (!skip_prefix(cmd_output.buf, "pack\t", &packhash))
 			die("fetch-pack: expected pack or keep, TAB, hash, "
 			    "then LF in http-fetch output");
 


BTW, two things that puzzled me while poking at your patch (but neither
I think are new or the fault of your patch):

  1. git-http-fetch documents --index-pack-args, but the actual option
     is the singular --index-pack-arg. The caller in fetch-pack
     obviously uses the one that works.

  2. The code you're touching insists on reading "keep" in the output,
     but wouldn't that depend on feeding "--keep" via index_pack_args? I
     didn't see immediately where we set it, but I certainly don't think
     your patch could be making anything worse here (since the existing
     code would have just died upon seeing a "pack" line). I think it
     happens as a side effect in get_pack(), which is...subtle. But
     again, not anything new.

-Peff

Re: [PATCH 2/2] fetch-pack: accept "pack" output for packfile URIs

[Jeff King]

On Tue, Jul 14, 2026 at 03:12:31AM -0400, Jeff King wrote:

> Would a more generic name like "cmd_output" or something make sense? I
> also think this would all be much nicer with a strbuf (which would let
> us get rid of the magic numbers), but that is a slightly larger
> refactor:
> 
> diff --git a/fetch-pack.c b/fetch-pack.c

In case anybody does pursue this, it is obviously missing this bit:

diff --git a/fetch-pack.c b/fetch-pack.c
index 5f94f35c30..359740f231 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1935,6 +1935,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 						 xstrfmt("%s/pack/pack-%s.keep",
 							 repo_get_object_directory(the_repository),
 							 packhash));
+
+		strbuf_release(&cmd_output);
 	}
 	string_list_clear(&packfile_uris, 0);
 	strvec_clear(&index_pack_args);

to avoid a leak.

-Peff

Re: [PATCH 0/2] packfile URIs: support concurrent downloads

[Taylor Blau]

On Mon, Jul 13, 2026 at 03:37:58PM -0700, Ted Nyman wrote:
> Ted Nyman (2):
>   http: use unique tempfiles for packfile URI downloads
>   fetch-pack: accept "pack" output for packfile URIs

I left one pretty minor style-nit on the first patch, but otherwise this
looks good to me.

Thanks,
Taylor