Re: [PATCH] builtin/fetch.c: clean tmp pack after receive signal

Jeff King [off-list ref] writes:

On Tue, Mar 16, 2021 at 02:53:36AM +0000, SURA via GitGitGadget wrote:

quoted

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@sigill.intra.peff.net/ (local)

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:

quoted

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

quoted

[...]

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.