Re: [PATCH] prune: --expire=time

From: Junio C Hamano <junkio@cox.net>
Date: Jan 21, 2007, 11:17 UTC
Message-ID: <7vejpo39zg.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20070121103724.GA23256@moooo.ath.cx>

Matthias Lederhofer <matled@gmx.net> writes:

Show 8 quoted lines

> Junio C Hamano <junkio@cox.net> wrote:
>> I am considering to commit the attached instead.
> Looks fine.  Just one question:  You said normally unsigned long would
> be used for time_t but time_t itself seems to be signed.  Using
> unsigned long instead of int for prune_grace_period (which is used as
> time_t here) results in 'warning: comparison between signed and
> unsigned'.  Perhaps you want to change it here anyway to be consistent
> with the rest of the code (approxidate returns unsigned long too).

Although I've merged this and pushed out v1.5.0-rc2, I am starting to think this whole implementation of grace period is unfortunately busted and does not buy us much. Running git-prune in an uncontrolled way from a cron job is still not safe.

Suppose there is a repository that has one old blob that is not referenced from any existing ref (in other words, its been more than the grace period since 'prune' was run in the repository, and one of its heads were rewound which lost the last reference to the blob). You are pushing a new commit into it, whose tree has that blob as one of the files.

You construct a pack, and unpack-objects starts to run to extract the objects you send in the said repository. The pack you are sending does contain the blob (because no refs reached it in the repository), but unpack-objects safety measure means the blob is not re-extracted to overwrite the existing old blob.

Now, an automated prune runs and finishes reading the available refs before your push concludes and updates the ref with your new commit.

What happens?

If we wanted to apply this grace period conservatively, protecting young objects is not enough. You need to protect everything they refer to as well. In the above scenario, you would protect the new commit object and probably the tree objects contained within, but the code happily will lose the blob that was already sitting there.