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
