Re: [PATCH 0/2] commit-graph/server-info: use tempfile.h in more places

From: Jeff King <peff@peff.net>
Date: Jun 8, 2024, 10:48 UTC
Message-ID: <20240608104821.GF2659849@coredump.intra.peff.net>
In-Reply-To: <cover.1717712358.git.me@ttaylorr.com>

On Thu, Jun 06, 2024 at 06:19:21PM -0400, Taylor Blau wrote:

Show 6 quoted lines

> Looking at the remaining uses of mkstemp(), the remaining class of
> callers that don't use the tempfile.h API are for creating temporary
> .idx, .rev files, and similar. My personal feeling is that we should
> apply similar treatment there, since these files are generated based on
> .pack data, and thus keeping around temporary copies is unnecessary when
> they can be regenerated.

And actual loose object and pack files themselves, I think. I think it was a deliberate choice long ago to keep those files around, since in the worst case they could be used to recover actual repo content (e.g., a failed fetch will leave its tmp_pack_* file around, and you could probably extract _some_ objects from it).

And the philosophy is that we'd leave them sitting around until gc ran and found tmp_* in objects/, check their mtimes, and remove them then.

In practice, I don't think I have really seen anybody recover anything from a temporary file. You're better off looking at whatever was feeding the temporary file (if it was "git add", then look at the filesystem, and if it was index-pack, look at the fetch/push source, etc).

And leaving them around is a minor nuisance, since degenerate cases (like a script retrying a failed operation over and over) can let them pile up.

And indeed, for push we don't keep the contents around anymore. Even though we don't clean up the .pack files, we stick them in a quarantine directory which does get cleaned up. This solved what used to be an operational headache for GitHub, and I don't think anybody has ever complained about not having their partial failed packfile available.

So I'd argue that we should just treat object/pack tempfiles like the rest, and delete them if they don't make it all the way to the rename step. If we really want to support debugging, we could perhaps provide a run-time knob to leave them in place (and maybe even have it apply to _all_ tempfiles).

But that is all way beyond your series, and I don't think there is any urgent need to tackle it. IIRC, GitHub's fork does sprinkle some register_tempfile() calls around the odb code, but I don't have access to that code anymore.

-Peff