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

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 14, 2024, 19:35 UTC
Message-ID: <ZmybaokdVIXOX+6D@nand.local>
In-Reply-To: <20240608104821.GF2659849@coredump.intra.peff.net>

On Sat, Jun 08, 2024 at 06:48:21AM -0400, Jeff King wrote:

Show 14 quoted lines

> On Thu, Jun 06, 2024 at 06:19:21PM -0400, Taylor Blau wrote:
>
> > 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).

Heh, yes. Those were intentionally omitted from this list ;-).

I agree that having the content stick around in failed packfile and loose object writes is useful as a last-resort recovery mechanism. I suspect that it is often difficult or impossible to recover the contents of an object/pack from a broken write, but I think it's better than the alternative of just throwing it away up front.

Show 7 quoted lines

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

Yup.

Show 5 quoted lines

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

I dunno... I don't disagree with what you're saying, but it does seem a little scary to delete files containing data that we might have been able to recover.

I think the current series ends at a reasonable stopping point... I'd honestly have to think more about whether I agree with what you're saying here or not ;-).

Thanks, Taylor