threads / rfc / 11869

RFC patchMake git prune remove temporary packs that look like write failures

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

## tl;dr

11 messages between Feb 4, 2008 and Feb 4, 2008. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

David Tweed· Feb 4, 2008, 14:10 UTC · lore
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(-)
Show changes to builtin-prune.c +34 −0
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;
 }
Nicolas Pitre· Feb 4, 2008, 14:52 UTC · re: David Tweed · lore

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

On Mon, 4 Feb 2008, David Tweed wrote:
Show 7 quoted lines
> 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
Johannes Schindelin· Feb 4, 2008, 15:16 UTC · re: David Tweed · lore

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

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

David Tweed· Feb 4, 2008, 15:24 UTC · re: Johannes Schindelin · lore

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

On Feb 4, 2008 3:16 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 13 quoted lines
> 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?

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
Johannes Schindelin· Feb 4, 2008, 17:21 UTC · re: David Tweed · lore

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

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

David Tweed· Feb 4, 2008, 17:39 UTC · re: Johannes Schindelin · lore

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

On Feb 4, 2008 5:21 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Mon, 4 Feb 2008, David Tweed wrote:
Show 5 quoted lines
> > 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
David Tweed· Feb 4, 2008, 17:42 UTC · re: David Tweed · lore

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

Ugh: On Feb 4, 2008 5:39 PM, David Tweed <david.tweed@gmail.com> 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
Nicolas Pitre· Feb 4, 2008, 17:47 UTC · re: David Tweed · lore

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

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
David Tweed· Feb 4, 2008, 17:54 UTC · re: Nicolas Pitre · lore

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

On Feb 4, 2008 5:47 PM, Nicolas Pitre <nico@cam.org> 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
Nicolas Pitre· Feb 4, 2008, 16:06 UTC · re: Johannes Schindelin · lore

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

On Mon, 4 Feb 2008, Johannes Schindelin wrote:
Show 17 quoted lines
> 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?)

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

Nicolas
Johannes Schindelin· Feb 4, 2008, 16:33 UTC · re: Nicolas Pitre · lore

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

Hi,
On Mon, 4 Feb 2008, Nicolas Pitre wrote:
Show 20 quoted lines
> On Mon, 4 Feb 2008, Johannes Schindelin wrote:
> 
> > 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?)
> 
> Please use the same parameter for both.  There is no need to have 
> separate settings.
Right.

Ciao, Dscho

← back to recent threads