Re: [PATCH] repack.c: rename and unlink pack file if it exists

From: Jeff King <peff@peff.net>
Date: Feb 5, 2014, 21:01 UTC
Message-ID: <20140205210129.GA24314@sigill.intra.peff.net>
In-Reply-To: <xmqqfvnx2s7p.fsf@gitster.dls.corp.google.com>

On Wed, Feb 05, 2014 at 12:57:14PM -0800, Junio C Hamano wrote:

Show 6 quoted lines

> > ...does not seem to fail, and it does not seem to leave any cruft in
> > place. So maybe I am misunderstanding the thing the patch is meant to
> > fix. Is it that we simply do not replace the pack in this instance?
> 
> Yes.  Not just the command finishing OK, but the packfile left by
> the first repack needs to be left intact. [...]

Thanks for the explanation. Having looked at this now, I'm thinking a test may not be worth the trouble. Due to 1190a1ac, we effectively don't care whether we get the old pack or the new one. So the fact that this bug exists doesn't really produce any user-visible behavior, and hopefully post-release we would drop the code entirely, and the test would have no reason to exist.

> We could use test-chmtime to reset the timestamp of the packfile
> generated by the first repack to somewhere reasonably old and then
> rerun the repack to see that it is a different file, which may be
> more portable than inspecting the inum of the packfile.

Yeah, I think that would work. But it sounds like we also need a filesystem in which rename() does not overwrite. So the test would not be portable.

-Peff