Jeff King <peff@xxxxxxxx> writes: > On Tue, Mar 16, 2021 at 02:53:36AM +0000, SURA via GitGitGadget wrote: > >> In Gitee.com, I often use scripts to start a time-limited > > Not related to your patch, but I think this name falls afoul of Git's > trademark policy. See: > > https://git-scm.com/trademark > > There's also some discussion in this thread: > > https://lore.kernel.org/git/20170202022655.2jwvudhvo4hmueaw@xxxxxxxxxxxxxxxxxxxxx/ Thanks. On somewhat related to this patch, we also ask contributors to use their real names so that we do not render the Signed-off-by: procedure meaningless. > This isn't quite true. "git gc" will clean up the temporary files, but > only if the mtime is sufficiently old. The purpose here is to give a > grace period to avoid deleting a file that is actively being written to. > However, we use the same grace period that we use for deleting > unreachable objects, which is absurdly long for this purpose: 2 weeks. > Probably something like an hour would be more appropriate (since the > mtime is updated on each write, this would imply a process not making > forward progress). I agree that for temporaries the two-week default is way too long, and I am OK if we decide to shorten the expiration for them separately from the known-to-be-good-but-unreferenced objects. > Likewise, we have a tempfile cleanup system already. > > I think this hunk: > >> @@ -336,6 +339,7 @@ static const char *open_pack_file(const char *pack_name) >> output_fd = odb_mkstemp(&tmp_file, >> "pack/tmp_pack_XXXXXX"); >> pack_name = strbuf_detach(&tmp_file, NULL); >> + tmp_pack_name = pack_name; > > ...can just call register_tempfile(). It should also record the result > so that we don't try to unlink() it after we've already moved it away > from its temporary name (though it's fairly unlikely for somebody else > to have used the name in the interim). > > I think you'd want to do the same for the tmp_idx_* files, too. Likewise > for ".rev" files we create starting in v2.31. > > I think it would also make sense in create_tmp_packfile(), which is used > during repacking (a different problem space, but really the same thing: > if repacking fails for some reason, we probably shouldn't leave a > useless gigantic half-finished packfile on disk). > > We should possibly also do so for tmp_obj_* files. Those can be written > for a fetch or push via unpack-objects (as well as normal local > commands). They're not usually as big as a pack, obviously, but I think > the same principle applies. > >> [...] > > It would be nice to see some tests covering this functionality, too. > Reproducing it with signals is likely to be racy and not worth it. But I > think that right now index-pack reading a bogus pack (say, one that > fails fsck checks) will leave the tmp_pack_* on disk. And it would not > if we cleanup tempfiles (again, this would be on any exit, not just > signal death, but I think that is what we'd want, and also what > register_tempfile() will do). Sounds like a good medium difficulty leftover bit. Thanks.
