On Thu, Feb 21, 2019 at 10:49:48AM -0800, Jonathan Tan wrote:
> > > - if (finalize_object_file(preq->tmpfile.buf, sha1_pack_name(p->sha1))
> > > - || finalize_object_file(tmp_idx, sha1_pack_index_name(p->sha1))) {
> > > - free(tmp_idx);
> > > - return -1;
> > > + if (finish_command(&ip)) {
> > > + ret = -1;
> > > + goto cleanup;
> > > }
> >
> > If the command fails but we got something in *lockfile, should we clean
> > it up? Likewise, do we need to be installing a signal handler to clean
> > it up in case we die in other code paths (or by a signal)?
>
> My inclination is not to do it - as far as I can tell, we don't have
> cleanup or signal handlers in fetch-pack.c either.
Hmm, yeah, you're right. Nor do we do it on fetch. At first I was
surprised that this doesn't cause more problems (we'd leave it in place
after seeing a pre-receive hook fail, for instance), but in practice it
gets cleaned up along with the rest of the quarantine area.
There's still a window where we may leave it around, though (and we
don't quarantine fetches at all), so it might be worth addressing at
some point. But I agree it is totally out of scope for this patch.
-Peff
