[RFC,PATCH] Make git prune remove temporary packs that look like write failures

Make git prune remove temporary packs that look like write failures

Write errors when repacking (eg, due to out-of-space conditions)
can leave temporary packs lying around which no existing
codepath removes and  which aren't obvious to the casual user.
Unfortunately the only way to tell in builtin-prune that a tmp_pack file is
of this sort is that it hasn't been modified recently. We assume a pack
which hasn't been modified within 10 minutes is of this sort and
delete it, printing a notification to help debugging. (Nicolas Pitre
suggested this functionality should be activated only by --prune.)

Signed-off-by: David Tweed (david.tweed@gmail.com)
---

I KNOW this initial RFC is mailer whitespace damaged. Finally version won't be.

In principle this is a really trivial patch, but I'm being cautious because I
don't like the fact that I don't know (and AFAICS can't reliably check) that
the files being deleting are definitely dead. An alternative would
be to make prune just print out that the suspicious packs are
there and let the user delete them manually. (My itch is that once
a write-failure pack gets created, nothing in git operations tells the
user that a generally multimegabyte file hidden in .git occupying space.)

 builtin-prune.c |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/builtin-prune.c b/builtin-prune.c
index b5e7684..90111ab 100644
--- a/builtin-prune.c
+++ b/builtin-prune.c

@@ -83,6 +83,39 @@ static void prune_object_dir(const char *path)
        }
 }

+/*
+ * Write errors (particularly out of space) can result in
+ * failed temporary packs accumulating in the object directory.
+ * This removes anything in the object directory beginning
+ * with tmp_ using the heuristic that anything
+ * that was last modified more than 10 minutes
+ * ago is the abandoned result of a write failure.
+ */
+static void remove_temporary_files(void)
+{
+       DIR *dir;
+       struct stat status;
+       time_t now;
+       struct dirent *de;
+       char* dirname=get_object_directory();
+
+       now = time(NULL);
+       dir = opendir(dirname);
+       while ((de = readdir(dir)) != NULL) {
+               if(strncmp(de->d_name, "tmp_", 4) == 0){
+                       char name[4096];
+                       int c=snprintf(name, 4095, "%s/%s", dirname,

de->d_name);
+                       if(c>0 && c<4096 && stat(name, &status) == 0
+                          && status.st_mtime < now - 600){
+                               printf("Removing apparently abandoned
%s\n",name);
+                               unlink(name);
+                       }
+               }
+       }
+       closedir(dir);
+}
+
+
 int cmd_prune(int argc, const char **argv, const char *prefix)
 {
        int i;

@@ -115,5 +148,6 @@ int cmd_prune(int argc, const char **argv, const

char *prefix)

        sync();
        prune_packed_objects(show_only);
+       remove_temporary_files();
        return 0;
 }

On Mon, 4 Feb 2008, David Tweed wrote:

In principle this is a really trivial patch, but I'm being cautious because I
don't like the fact that I don't know (and AFAICS can't reliably check) that
the files being deleting are definitely dead. An alternative would
be to make prune just print out that the suspicious packs are
there and let the user delete them manually. (My itch is that once
a write-failure pack gets created, nothing in git operations tells the
user that a generally multimegabyte file hidden in .git occupying space.)

The lifelessness of a temporary pack is the same as for loose objects, 
hence the same rule should apply in both cases.  Just asking the user to 
delete them manually isn't too nice either.  A prune operation is 
already said to be dangerous and should be performed only when no other 
activities are occurring in the same repository.  That should cover the 
case of dead temporary pack files just as well.


Nicolas

Hi,

On Mon, 4 Feb 2008, David Tweed wrote:

+                       if(c>0 && c<4096 && stat(name, &status) == 0
+                          && status.st_mtime < now - 600){

Please have spaces after the "if" and before the "{" (just imitate the 
style of the rest of the file).

Also, 10 minutes grace period for any ongoing fetch or repack seems a bit 
arbitrary.  Maybe default to 10 minutes, and introduce 
prune.packGracePeriod?

(Which reminds me that it might be useful to add a 
prune.looseObjectsGracePeriod to avoid having to type --expire= all the 
time?)

Ciao,
Dscho

On Feb 4, 2008 3:16 PM, Johannes Schindelin [off-list ref] wrote:

Hi,

On Mon, 4 Feb 2008, David Tweed wrote:

quoted

+                       if(c>0 && c<4096 && stat(name, &status) == 0
+                          && status.st_mtime < now - 600){

Please have spaces after the "if" and before the "{" (just imitate the
style of the rest of the file).

Also, 10 minutes grace period for any ongoing fetch or repack seems a bit
arbitrary.  Maybe default to 10 minutes, and introduce
prune.packGracePeriod?

In response to this and to Nico's earlier mail, I _think_ the usage
with repack is completely safe. What I'm not sure about is that other
things like git-svn create temporary packs with usage/semantics I'm
not sure about. I'm happy to delete immediately if those who
understand the interactions in the whole of git say that's acceptable
when the user specifically calls git-prune.

-- 
cheers, dave tweed__________________________
david.tweed@gmail.com
Rm 124, School of Systems Engineering, University of Reading.
"while having code so boring anyone can maintain it, use Python." --
attempted insult seen on slashdot

On Mon, 4 Feb 2008, Johannes Schindelin wrote:

Hi,

On Mon, 4 Feb 2008, David Tweed wrote:

quoted

+                       if(c>0 && c<4096 && stat(name, &status) == 0
+                          && status.st_mtime < now - 600){

Please have spaces after the "if" and before the "{" (just imitate the 
style of the rest of the file).

Also, 10 minutes grace period for any ongoing fetch or repack seems a bit 
arbitrary.  Maybe default to 10 minutes, and introduce 
prune.packGracePeriod?

(Which reminds me that it might be useful to add a 
prune.looseObjectsGracePeriod to avoid having to type --expire= all the 
time?)

Please use the same parameter for both.  There is no need to have 
separate settings.


Nicolas

Hi,

On Mon, 4 Feb 2008, Nicolas Pitre wrote:

On Mon, 4 Feb 2008, Johannes Schindelin wrote:

quoted

On Mon, 4 Feb 2008, David Tweed wrote:

quoted

+                       if(c>0 && c<4096 && stat(name, &status) == 0
+                          && status.st_mtime < now - 600){

Please have spaces after the "if" and before the "{" (just imitate the 
style of the rest of the file).

Also, 10 minutes grace period for any ongoing fetch or repack seems a 
bit arbitrary.  Maybe default to 10 minutes, and introduce 
prune.packGracePeriod?

(Which reminds me that it might be useful to add a 
prune.looseObjectsGracePeriod to avoid having to type --expire= all 
the time?)

Please use the same parameter for both.  There is no need to have 
separate settings.

Right.

Ciao,
Dscho

Hi,

On Mon, 4 Feb 2008, David Tweed wrote:

In response to this and to Nico's earlier mail, I _think_ the usage with 
repack is completely safe.

It would have been nicer of you to defend that, instead of sending me off 
to look for myself.  Having looked for myself, I am not convinced at all.

And it would have been surprising: if your patch would play nicely with a 
repack in progress, then it would fail to remove the temporary packs left 
by a crashed repack.

Ciao,
Dscho

Ugh:
On Feb 4, 2008 5:39 PM, David Tweed [off-list ref] wrote:

You're right (and I didn't intend to suggest otherwise) that it would
be safe when running a "git prune" concurrently with a separate "git

s/safe/unsafe/

repack".

-- 
cheers, dave tweed__________________________
david.tweed@gmail.com
Rm 124, School of Systems Engineering, University of Reading.
"while having code so boring anyone can maintain it, use Python." --
attempted insult seen on slashdot

On Feb 4, 2008 5:21 PM, Johannes Schindelin [off-list ref] wrote:

On Mon, 4 Feb 2008, David Tweed wrote:

quoted

In response to this and to Nico's earlier mail, I _think_ the usage with
repack is completely safe.

It would have been nicer of you to defend that, instead of sending me off
to look for myself.  Having looked for myself, I am not convinced at all.

I probably ought to have put the underlines around the "I". I'm
convinced, but since this is deleting things I'm more cautious than I
would be, say, parsing options.

And it would have been surprising: if your patch would play nicely with a
repack in progress, then it would fail to remove the temporary packs left
by a crashed repack.

I should been more careful what I said: I only use repack via "git gc"
which calls the repack as a subcommand. If the repack fails then the
whole process dies and you've got a dead tmp pack. The _next_ time you
call "git gc" it will do the repack, finish and then call "git prune"
(assuming --prune) and delete the temporary pack. Used in this way, I
have tried and I cannot see an execution path where this can go wrong.
You're right (and I didn't intend to suggest otherwise) that it would
be safe when running a "git prune" concurrently with a separate "git
repack".

However, I'm not familiar with what things like git-svn, cvs, etc, do.
Given that I've seen patches adding "git gc" periodically during
various imports, I wanted to someone who knows that area to confirm
the patch isn't violating any assumptions.

-- 
cheers, dave tweed__________________________
david.tweed@gmail.com
Rm 124, School of Systems Engineering, University of Reading.
"while having code so boring anyone can maintain it, use Python." --
attempted insult seen on slashdot

On Mon, 4 Feb 2008, David Tweed wrote:

However, I'm not familiar with what things like git-svn, cvs, etc, do.
Given that I've seen patches adding "git gc" periodically during
various imports, I wanted to someone who knows that area to confirm
the patch isn't violating any assumptions.

If they're prunning old objects already, they can prune old temporary 
pack files assuming the same level of (non) risk.


Nicolas

On Feb 4, 2008 5:47 PM, Nicolas Pitre [off-list ref] wrote:

On Mon, 4 Feb 2008, David Tweed wrote:
If they're prunning old objects already, they can prune old temporary
pack files assuming the same level of (non) risk.

Thanks. I'll leave it a day or so, then post a final patch without the
modification time check.

-- 
cheers, dave tweed__________________________
david.tweed@gmail.com
Rm 124, School of Systems Engineering, University of Reading.
"while having code so boring anyone can maintain it, use Python." --
attempted insult seen on slashdot