From: Ted Nyman <tnyman@openai.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v5 2/3] http: avoid concurrent appends to partial packs
Date: Sun, 26 Jul 2026 03:04:21 -0700 [thread overview]
Message-ID: <20260726100421.12648-1-tnyman@openai.com> (raw)
In-Reply-To: <20260726092027.GA3529827@coredump.intra.peff.net>
On Sun, Jul 26, 2026 at 05:20:27AM -0400, Jeff King wrote:
> I wonder if we still need this or not.
I think so, but wouldn't bet the farm on it. A concurrent downloader can
complete the staging file before another downloader issues its Range
request. That request then starts exactly at EOF, so the server can
respond with 416. The existing regression test exercises that case, and
we still need to let index-pack validate the completed local pack.
> More importantly, why don't we need to close tmpfile_fd anymore? We hand
> it off to run_command(), which will always close it. So I _think_ it was
> always wrong to close it ourselves here. If so, then could this hunk
> become a preparatory commit on its own?
You're right: run_command() already closes ip.in, so the old
close(tmpfile_fd) was a double-close. That cleanup is independent, and
I can pull it into a preparatory patch if that would make the series
easier to follow.
> That is...subtle as hell. I really wonder if it would be worth
> introducing the basic form of this (just opening once with O_RDWR) and
> then doing the Windows hackery on top as a separate commit.
I'm certainly not an expert on the Windows side, so I had to track this
down in the MinGW open() wrapper. The existing-file O_RDWR path includes
FILE_SHARE_DELETE, while creating a new file falls back to _wopen()
without it. The loop creates the file with O_EXCL if needed, closes that
descriptor, and retries through the existing-file path; a racing creator
that sees EEXIST also retries.
I kept those pieces together to avoid an intermediate state without the
required sharing behavior on MinGW, but I'm happy to split them if you
think it would be clearer.
> Hopefully this perl script (and the accompanying fifo monstrosities)
> can sit here for eternity un-looked-at by human eyes, just quietly
> doing their job until the heat death of the universe.
I thought you, of all people, might appreciate a little more Perl. ;-)
Thanks,
Ted
next prev parent reply other threads:[~2026-07-26 10:04 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-07-13 22:37 [PATCH 0/2] packfile URIs: support concurrent downloads Ted Nyman
2026-07-13 22:34 ` [PATCH 1/2] http: use unique tempfiles for packfile URI downloads Ted Nyman
2026-07-14 1:00 ` Junio C Hamano
2026-07-14 1:58 ` Ted Nyman
2026-07-14 4:07 ` Taylor Blau
2026-07-14 5:28 ` Jeff King
2026-07-14 18:10 ` Junio C Hamano
2026-07-14 18:31 ` Ted Nyman
2026-07-14 4:06 ` Taylor Blau
2026-07-14 5:44 ` Jeff King
2026-07-14 6:46 ` Jeff King
2026-07-13 22:34 ` [PATCH 2/2] fetch-pack: accept "pack" output for packfile URIs Ted Nyman
2026-07-14 7:12 ` Jeff King
2026-07-14 7:13 ` Jeff King
2026-07-14 18:38 ` Ted Nyman
2026-07-14 21:47 ` Jeff King
2026-07-14 4:13 ` [PATCH 0/2] packfile URIs: support concurrent downloads Taylor Blau
2026-07-20 22:33 ` [PATCH v2 " Ted Nyman
2026-07-20 22:33 ` [PATCH v2 1/2] http: avoid concurrent appends to partial packs Ted Nyman
2026-07-21 19:56 ` Junio C Hamano
2026-07-20 22:34 ` [PATCH v2 2/2] fetch-pack: accept "pack" output for packfile URIs Ted Nyman
2026-07-21 23:29 ` [PATCH v3 0/3] packfile URIs: support concurrent downloads Ted Nyman
2026-07-21 23:29 ` [PATCH v3 1/3] http-fetch: correct --index-pack-arg documentation Ted Nyman
2026-07-21 23:29 ` [PATCH v3 2/3] http: avoid concurrent appends to partial packs Ted Nyman
2026-07-21 23:29 ` [PATCH v3 3/3] fetch-pack: accept "pack" output for packfile URIs Ted Nyman
2026-07-24 4:43 ` [PATCH v3 0/3] packfile URIs: support concurrent downloads Junio C Hamano
2026-07-25 9:09 ` Jeff King
2026-07-25 9:21 ` Jeff King
2026-07-25 10:02 ` Jeff King
2026-07-25 10:10 ` Jeff King
2026-07-25 16:20 ` Junio C Hamano
2026-07-24 8:14 ` [PATCH v4 " Ted Nyman
2026-07-24 8:14 ` [PATCH v4 1/3] http-fetch: correct --index-pack-arg documentation Ted Nyman
2026-07-24 21:38 ` Taylor Blau
2026-07-24 8:14 ` [PATCH v4 2/3] http: avoid concurrent appends to partial packs Ted Nyman
2026-07-24 8:14 ` [PATCH v4 3/3] fetch-pack: accept "pack" output for packfile URIs Ted Nyman
2026-07-24 21:46 ` Taylor Blau
2026-07-26 6:44 ` [PATCH v5 0/3] packfile URIs: support concurrent downloads Ted Nyman
2026-07-26 6:44 ` [PATCH v5 1/3] http-fetch: correct --index-pack-arg documentation Ted Nyman
2026-07-26 6:44 ` [PATCH v5 2/3] http: avoid concurrent appends to partial packs Ted Nyman
2026-07-26 9:20 ` Jeff King
2026-07-26 10:04 ` Ted Nyman [this message]
2026-07-26 10:27 ` Jeff King
2026-07-26 6:44 ` [PATCH v5 3/3] fetch-pack: accept "pack" output for packfile URIs Ted Nyman
2026-07-26 9:21 ` [PATCH v5 0/3] packfile URIs: support concurrent downloads Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260726100421.12648-1-tnyman@openai.com \
--to=tnyman@openai.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.