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

Matthias Lederhofer [off-list ref] writes:

Junio C Hamano [off-list ref] wrote:

quoted

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.

On Sun, Jan 21, 2007 at 03:17:07AM -0800, Junio C Hamano wrote:

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

That's not sufficient either. You might not _have_ the young objects
yet, think the blob is dangling, and delete it. Meanwhile, the tree that
references it arrives. IOW,
  1. blob B arrives, but already exists
  2. prune deletes unreference and old blob B
  3. tree T arrives, referencing blob B
I think this might be safe if you add objects in a top-down way (i.e., T
before B). However, that doesn't make sense for the commit operation, in
which you add blobs (with git-add), and then eventually construct a
tree.

-Peff

Jeff King wrote:

That's not sufficient either. You might not _have_ the young objects
yet, think the blob is dangling, and delete it. Meanwhile, the tree that
references it arrives. IOW,
  1. blob B arrives, but already exists
  2. prune deletes unreference and old blob B
  3. tree T arrives, referencing blob B
I think this might be safe if you add objects in a top-down way (i.e., T
before B). However, that doesn't make sense for the commit operation, in
which you add blobs (with git-add), and then eventually construct a
tree.
  

Shouldn't the repository be locked against operations like prune while a 
commit is in progress anyway? That seems like it's pretty prudent and 
reasonable to me -- doing otherwise is just asking for a zillion little 
race conditions. Prune should be a rare enough operation that having it 
abort (or better, block) while a commit is going on wouldn't be a big 
problem, I'd think.

-Steve

On Sun, Jan 21, 2007 at 05:38:57PM -0800, Steven Grimm wrote:

quoted

before B). However, that doesn't make sense for the commit operation, in
which you add blobs (with git-add), and then eventually construct a
tree.
 

Shouldn't the repository be locked against operations like prune while a 
commit is in progress anyway? That seems like it's pretty prudent and 
reasonable to me -- doing otherwise is just asking for a zillion little 
race conditions. Prune should be a rare enough operation that having it 
abort (or better, block) while a commit is going on wouldn't be a big 
problem, I'd think.

I was a bit loose with my phrase 'commit operation'. What I really mean
is:

$ git add file   ;# (1)
$ hack hack hack ;# (2)
$ git commit     ;# (3)

After step (1), you have a blob in your db. If you already had that
blob, then you have the old blob. You don't get the updated tree and
commit until step (3). Step (2) can be hours or days. Do you really want
to lock the repository that long?

Potentially we could 'touch' the blob in step (1) to update its
timestamp. But if we update timestamps for things like commit, then that
might mean 'touch'ing tens of thousands of objects for a commit which
_should_ only require making a few objects.

-Peff