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:

quoted

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