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

[Taylor Blau <me@ttaylorr.com>]

On Sat, Jun 08, 2024 at 06:48:21AM -0400, Jeff King wrote:
> 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.

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

> 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