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

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

[David Steven Tweed]

Write errors when repacking (eg, due to out-of-space conditions)
can leave temporary packs (and possibly other files beginning
with "tmp_") lying around which no existing
codepath removes and which aren't obvious to the casual user.
These can also be multi-megabyte files wasting noticeable space.
Unfortunately there's no way to definitely tell in builtin-prune
that a tmp_ file is not being used by a concurrent process.
However, it is documented that pruning should only be done
on a quiet repository. The names of removed files are printed.

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

Per discussion of previous version, this now unconditionally
removes any tmp_ file existing when prune is run.

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

diff --git a/builtin-prune.c b/builtin-prune.c
index b5e7684..9db3cf0 100644
--- a/builtin-prune.c
+++ b/builtin-prune.c
@@ -83,6 +83,30 @@ static void prune_object_dir(const char *path)
  	}
  }

+/*
+ * Write errors (particularly out of space) can result in
+ * failed temporary packs (and more rarely indexes and other
+ * files begining with "tmp_") accumulating in the
+ * object directory.
+ */
+static void remove_temporary_files(void)
+{
+	DIR *dir;
+	struct dirent *de;
+	char* dirname=get_object_directory();
+
+	dir = opendir(dirname);
+	while ((de = readdir(dir)) != NULL) {
+		if (strncmp(de->d_name, "tmp_", 4) == 0) {
+			char name[4096];
+			sprintf(name, "%s/%s", dirname, de->d_name);
+			printf("Removing abandoned pack %s\n", name);
+			unlink(name);
+		}
+	}
+	closedir(dir);
+}
+
  int cmd_prune(int argc, const char **argv, const char *prefix)
  {
  	int i;
@@ -115,5 +139,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix)

  	sync();
  	prune_packed_objects(show_only);
+	remove_temporary_files();
  	return 0;
  }
-- 
1.5.4.19.g40d1a-dirty

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

[Nicolas Pitre]

On Tue, 5 Feb 2008, David Steven Tweed wrote:

> @@ -115,5 +139,6 @@ int cmd_prune(int argc, const char **argv, const char
> *prefix)
> 
>  	sync();
>  	prune_packed_objects(show_only);
> +	remove_temporary_files();

Maybe you could implement the "show_only" mode for 
remove_temporary_files() as well?  Otherwise the -n option would not be 
respected.

Also you should consider honoring the --expire option as well.


Nicolas

[PATCH] prune: heed --expire for stale packs, add a test

[Johannes Schindelin]

Follow the same logic as for loose objects when removing stale packs: they
might be in use (for example when fetching, or repacking in a cron job),
so give the user a chance to say (via --expire) what is considered too
young an age to die for stale packs.

Also add a simple test to verify that the stale packs are actually
expired.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---


	On Tue, 5 Feb 2008, Nicolas Pitre wrote:

	> On Tue, 5 Feb 2008, David Steven Tweed wrote:
	> 
	> > @@ -115,5 +139,6 @@ int cmd_prune(int argc, const char **argv, 
	> > const char *prefix)
	> > 
	> >  	sync();
	> >  	prune_packed_objects(show_only);
	> > +	remove_temporary_files();
	> 
	> Maybe you could implement the "show_only" mode for 
	> remove_temporary_files() as well?  Otherwise the -n option would 
	> not be respected.
	> 
	> Also you should consider honoring the --expire option as well.

	How about this on top of David's patch?

 builtin-prune.c  |    9 ++++++++-
 t/t5304-prune.sh |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletions(-)
 create mode 100644 t/t5304-prune.sh

diff --git a/builtin-prune.c b/builtin-prune.c
index 9152984..d5a3b60 100644
--- a/builtin-prune.c
+++ b/builtin-prune.c
@@ -100,7 +100,14 @@ static void remove_temporary_files(void)
 		if (strncmp(de->d_name, "tmp_", 4) == 0) {
 			char name[4096];
 			sprintf(name, "%s/%s", dirname, de->d_name);
-			printf("Removing abandoned pack %s\n", name);
+			if (expire) {
+				struct stat st;
+				if (stat(name, &st) || st.st_mtime >= expire)
+					continue;
+			}
+			printf("Removing stale pack %s\n", name);
+			if (show_only)
+				continue;
 			unlink(name);
 		}
 	}
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
new file mode 100644
index 0000000..6560af7
--- /dev/null
+++ b/t/t5304-prune.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Johannes E. Schindelin
+#
+
+test_description='prune'
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	: > file &&
+	git add file &&
+	test_tick &&
+	git commit -m initial &&
+	git gc
+
+'
+
+test_expect_success 'prune stale packs' '
+
+	orig_pack=$(echo .git/objects/pack/*.pack) &&
+	: > .git/objects/tmp_1.pack &&
+	: > .git/objects/tmp_2.pack &&
+	test-chmtime -86501 .git/objects/tmp_1.pack &&
+	git prune --expire 1.day &&
+	test -f $orig_pack &&
+	test -f .git/objects/tmp_2.pack &&
+	! test -f .git/objects/tmp_1.pack
+
+'
+
+test_done
-- 
1.5.4.1230.g4ecf8

Re: [PATCH] prune: heed --expire for stale packs, add a test

[Nicolas Pitre]

On Tue, 5 Feb 2008, Johannes Schindelin wrote:

> 
> Follow the same logic as for loose objects when removing stale packs: they
> might be in use (for example when fetching, or repacking in a cron job),
> so give the user a chance to say (via --expire) what is considered too
> young an age to die for stale packs.
> 
> Also add a simple test to verify that the stale packs are actually
> expired.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Acked-by: Nicolas Pitre <nico@cam.org>


Nicolas

Re: [PATCH] prune: heed --expire for stale packs, add a test

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

> On Tue, 5 Feb 2008, Johannes Schindelin wrote:
>
>> Follow the same logic as for loose objects when removing stale packs: they
>> might be in use (for example when fetching, or repacking in a cron job),
>> so give the user a chance to say (via --expire) what is considered too
>> young an age to die for stale packs.
>> 
>> Also add a simple test to verify that the stale packs are actually
>> expired.
>> 
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Acked-by: Nicolas Pitre <nico@cam.org>
>
> Nicolas

They are not "stale packs", but temporary files that wanted to
become pack but did not succeed.  Perhaps "stale temporary
packs"?

Shouldn't we do something similar to objects/pack/pack-*.temp
files and objects/??/*.temp that http walker leaves?

Re: [PATCH] prune: heed --expire for stale packs, add a test

[Johannes Schindelin]

Hi,

On Tue, 5 Feb 2008, Junio C Hamano wrote:

> Nicolas Pitre <nico@cam.org> writes:
> 
> > On Tue, 5 Feb 2008, Johannes Schindelin wrote:
> >
> >> Follow the same logic as for loose objects when removing stale packs: they
> >> might be in use (for example when fetching, or repacking in a cron job),
> >> so give the user a chance to say (via --expire) what is considered too
> >> young an age to die for stale packs.
> >> 
> >> Also add a simple test to verify that the stale packs are actually
> >> expired.
> >> 
> >> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Acked-by: Nicolas Pitre <nico@cam.org>
> >
> > Nicolas
> 
> They are not "stale packs", but temporary files that wanted to
> become pack but did not succeed.  Perhaps "stale temporary
> packs"?
> 
> Shouldn't we do something similar to objects/pack/pack-*.temp
> files and objects/??/*.temp that http walker leaves?

Yep.

Ciao,
Dscho

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

[Junio C Hamano]

David Steven Tweed <d.s.tweed@reading.ac.uk> writes:

> Write errors when repacking (eg, due to out-of-space conditions)
> can leave temporary packs (and possibly other files beginning
> with "tmp_") lying around which no existing
> codepath removes and which aren't obvious to the casual user.
> These can also be multi-megabyte files wasting noticeable space.
> Unfortunately there's no way to definitely tell in builtin-prune
> that a tmp_ file is not being used by a concurrent process.
> However, it is documented that pruning should only be done
> on a quiet repository. The names of removed files are printed.
>
> Signed-off-by: David Tweed (david.tweed@gmail.com)
> ---
>
> Per discussion of previous version, this now unconditionally
> removes any tmp_ file existing when prune is run.

Sorry, can't apply

	Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed

Re: [PATCH] prune: heed --expire for stale packs, add a test

[Nicolas Pitre]

On Tue, 5 Feb 2008, Junio C Hamano wrote:

> They are not "stale packs", but temporary files that wanted to
> become pack but did not succeed.  Perhaps "stale temporary
> packs"?
> 
> Shouldn't we do something similar to objects/pack/pack-*.temp
> files and objects/??/*.temp that http walker leaves?

Instead, I think http walker should be made to use the same location and 
filename pattern for its temporary files as the rest of the code.


Nicolas

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

[Brandon Casey]

Nicolas Pitre wrote:
> On Tue, 5 Feb 2008, David Steven Tweed wrote:
> 
>> @@ -115,5 +139,6 @@ int cmd_prune(int argc, const char **argv, const char
>> *prefix)
>>
>>  	sync();
>>  	prune_packed_objects(show_only);
>> +	remove_temporary_files();
> 
> Maybe you could implement the "show_only" mode for 
> remove_temporary_files() as well?  Otherwise the -n option would not be 
> respected.

I also suggest taking a look at the functions in builtin-prune-packed.c to see
how similar functions are implemented there.

Use strlcpy instead of sprintf.
Use prefixcmp instead of strncmp.
Use same messages as prune_dir() for show_only path and unlink failure.
You could also check the opendir call and print a suitable message on failure.

-brandon

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

[David Tweed]

On Feb 6, 2008 4:48 PM, Brandon Casey <casey@nrlssc.navy.mil> wrote:
> I also suggest taking a look at the functions in builtin-prune-packed.c to see
> how similar functions are implemented there.
>
> Use strlcpy instead of sprintf.
> Use prefixcmp instead of strncmp.
> Use same messages as prune_dir() for show_only path and unlink failure.
> You could also check the opendir call and print a suitable message on failure.

All the other path creation in builtin-prune.c is using sprintf; is
doing 3 strlcpy's much better? (I backed off from using snprintf when
the other element in the if that tested it vanished; I probably ought
to put that back.) I'll use prefixcmp and check the opendir call
(although if get_object_directory() doesn't return something sensible
presumably bigger problems are in the mix.)

-- 
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

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

[David Tweed]

On Feb 5, 2008 7:02 PM, Nicolas Pitre <nico@cam.org> wrote:
> On Tue, 5 Feb 2008, David Steven Tweed wrote:
>
> > @@ -115,5 +139,6 @@ int cmd_prune(int argc, const char **argv, const char
> > *prefix)
> >
> >       sync();
> >       prune_packed_objects(show_only);
> > +     remove_temporary_files();
>
> Maybe you could implement the "show_only" mode for
> remove_temporary_files() as well?  Otherwise the -n option would not be
> respected.
>
> Also you should consider honoring the --expire option as well.

I guess the -n ought to be honoured. However, unless I'm missing
something, the case of expiring objects is different. The primary
reason is that objects can get orphaned by "semantic" decisions
(delete this branch, rewind, etc) so they contain valid content that
you might want to later rescue (using low-level command like git cat
if necessary). In contrast, the only way to get a temporary pack when
the repository is quiescent is resulting from a _write error_ and thus
is a corrupt entity which it would take a great deal of work to
extract any valid data from. (To be honest, I wouldn't be bothering to
delete them if it weren't for the fact that they can be quite big
files, and once you've got one from out-of-space you're more likely to
get another in future because you've got even less space.) So it's not
obvious that the same conditions should apply as to valid objects.

Does it really make sense to apply expire to these?

-- 
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

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

[Nicolas Pitre]

On Wed, 6 Feb 2008, David Tweed wrote:

> I guess the -n ought to be honoured. However, unless I'm missing
> something, the case of expiring objects is different. The primary
> reason is that objects can get orphaned by "semantic" decisions
> (delete this branch, rewind, etc) so they contain valid content that
> you might want to later rescue (using low-level command like git cat
> if necessary).

You can also get loose unconnected objects when fetching and the number 
of objects is lower than the transfer.unpackLimit value.

> In contrast, the only way to get a temporary pack when
> the repository is quiescent is resulting from a _write error_ and thus
> is a corrupt entity which it would take a great deal of work to
> extract any valid data from.

Or when a fetch is in progress, just like the case above, but with the 
number of objects greater than transfer.unpackLimit.

This is uncommon to have a prune occurring at the same time as a fetch, 
but the --expire argument is there if for example you do a prune from a 
cron job but still want to be safe by giving a grace period to garbage 
files which might not be so after all.


Nicolas

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

[Brandon Casey]

David Tweed wrote:
> On Feb 6, 2008 4:48 PM, Brandon Casey <casey@nrlssc.navy.mil> wrote:
>> I also suggest taking a look at the functions in builtin-prune-packed.c to see
>> how similar functions are implemented there.
>>
>> Use strlcpy instead of sprintf.
>> Use prefixcmp instead of strncmp.
>> Use same messages as prune_dir() for show_only path and unlink failure.
>> You could also check the opendir call and print a suitable message on failure.
> 
> All the other path creation in builtin-prune.c is using sprintf; is
> doing 3 strlcpy's much better? (I backed off from using snprintf when
> the other element in the if that tested it vanished; I probably ought
> to put that back.)

They use sprintf for the "%02x" part, but they use memcpy to copy the return
of get_object_directory() into a fixed string and then append onto that,
rather than repeatedly writing the same string over and over. Ok, there is one
instance in builtin-prune.c that repeatedly writes path, but builtin-prune-packed.c
does the memcpy thing.

Something like:

	char pathname[PATH_MAX];
	...

	/* check length of dirname not too long */

	memcpy(pathname, dirname, len);

	if (len && pathname[len-1] != '/')
		pathname[len++] = '/';

	...

	while ((de = readdir(dir)...) {
		if (!prefixcmp(...
			if (strlcpy(pathname + len, de->d_name, PATH_MAX - len)
			    >= PATH_MAX - len) {
				warning("too long path encountered: %s%s",
					pathname, de->d_name);
				continue;
			}

			...


-brandon

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

[David Tweed]

On Feb 6, 2008 7:41 PM, Brandon Casey <casey@nrlssc.navy.mil> wrote:
> They use sprintf for the "%02x" part, but they use memcpy to copy the return
> of get_object_directory() into a fixed string and then append onto that,
> rather than repeatedly writing the same string over and over. Ok, there is one
> instance in builtin-prune.c that repeatedly writes path, but builtin-prune-packed.c
> does the memcpy thing.

Given I'm the only person (AFAICS from the list archives) who's ever
talked about failed temporary packs, I assume that almost everyone
using git is using filesystems with sufficient space that they don't
get write errors, so the path building will generally be done 0 times
per --prune. (Using a USB disk that's almost full, along with
occasionally filling up my /home with experiment output, is the
occasions I get them.) So I don't think efficiency is an issue. So the
big (genuine) question is: is a properly checked snprintf more or less
readable/canonical git style than doing more complicated strlcpy
things? (I know I need to really think about what the below is doing,
and what the correctness conditions are.)

> Something like:
>
>         char pathname[PATH_MAX];
>         ...
>
>         /* check length of dirname not too long */
>
>         memcpy(pathname, dirname, len);
>
>         if (len && pathname[len-1] != '/')
>                 pathname[len++] = '/';
>
>         ...
>
>         while ((de = readdir(dir)...) {
>                 if (!prefixcmp(...
>                         if (strlcpy(pathname + len, de->d_name, PATH_MAX - len)
>                             >= PATH_MAX - len) {
>                                 warning("too long path encountered: %s%s",
>                                         pathname, de->d_name);
>                                 continue;
>                         }
>
>                         ...

-- 
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

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

[David Tweed]

On Feb 6, 2008 7:31 PM, Nicolas Pitre <nico@cam.org> wrote:
> On Wed, 6 Feb 2008, David Tweed wrote:
>
> > I guess the -n ought to be honoured. However, unless I'm missing
> > something, the case of expiring objects is different. The primary
> > reason is that objects can get orphaned by "semantic" decisions
> > (delete this branch, rewind, etc) so they contain valid content that
> > you might want to later rescue (using low-level command like git cat
> > if necessary).
>
> You can also get loose unconnected objects when fetching and the number
> of objects is lower than the transfer.unpackLimit value.

I didn't know that, but the key point is that these are still
well-formed objects, so rescuing data from them makes sense. In
contrast, I'm concerned with packs which are ill-formed from a
write-error, which I don't believe git will even read. (I've just
tried truncating a finished pack file and none of the git commands
I've tried will read any objects in it. So you'll have to do low-level
hacking to get data out of a write-error pack. WARNING: be aware of
how to do a non-hardlinked clone before trying this.)

> > In contrast, the only way to get a temporary pack when
> > the repository is quiescent is resulting from a _write error_ and thus
> > is a corrupt entity which it would take a great deal of work to
> > extract any valid data from.
>
> Or when a fetch is in progress, just like the case above, but with the
> number of objects greater than transfer.unpackLimit.

Being "in progress" contradicts the presumption of "being quiescent".

> This is uncommon to have a prune occurring at the same time as a fetch,
> but the --expire argument is there if for example you do a prune from a
> cron job but still want to be safe by giving a grace period to garbage
> files which might not be so after all.

Ah, I hadn't realised this was an intended usage of --expire. Since as
you note there's no way to tell an abandoned tmp pack from one that's
in the process of being written, following expire is probably
necessary for safety. I'll look at adding support for that.

Many thanks for the advice,

-- 
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

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

[Nicolas Pitre]

On Wed, 6 Feb 2008, David Tweed wrote:

> Given I'm the only person (AFAICS from the list archives) who's ever
> talked about failed temporary packs, I assume that almost everyone
> using git is using filesystems with sufficient space that they don't
> get write errors,

Note that doing ^C during a fetch will also leave a temporary pack file 
there.  So your patch is good even for people with plenty of disk space.

> so the path building will generally be done 0 times
> per --prune. (Using a USB disk that's almost full, along with
> occasionally filling up my /home with experiment output, is the
> occasions I get them.) So I don't think efficiency is an issue.

Clearly not a hot path indeed.


Nicolas

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

[Nicolas Pitre]

On Wed, 6 Feb 2008, David Tweed wrote:
> On Feb 6, 2008 7:31 PM, Nicolas Pitre <nico@cam.org> wrote:
> > This is uncommon to have a prune occurring at the same time as a fetch,
> > but the --expire argument is there if for example you do a prune from a
> > cron job but still want to be safe by giving a grace period to garbage
> > files which might not be so after all.
> 
> Ah, I hadn't realised this was an intended usage of --expire. Since as
> you note there's no way to tell an abandoned tmp pack from one that's
> in the process of being written, following expire is probably
> necessary for safety. I'll look at adding support for that.

Did you miss Johannes ' patch?  He posted it yesterday and you were even 
CC'd. It did exactly that on top of yours already.


Nicolas

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

[David Tweed]

On Feb 6, 2008 8:16 PM, Nicolas Pitre <nico@cam.org> wrote:
> On Wed, 6 Feb 2008, David Tweed wrote:
> > Ah, I hadn't realised this was an intended usage of --expire. Since as
> > you note there's no way to tell an abandoned tmp pack from one that's
> > in the process of being written, following expire is probably
> > necessary for safety. I'll look at adding support for that.
>
> Did you miss Johannes ' patch?  He posted it yesterday and you were even
> CC'd. It did exactly that on top of yours already.

I had missed it (I've only just started reading email). Thanks for
pointing it out.



-- 
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

Re: [PATCH] prune: heed --expire for stale packs, add a test

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

> On Tue, 5 Feb 2008, Junio C Hamano wrote:
>
>> They are not "stale packs", but temporary files that wanted to
>> become pack but did not succeed.  Perhaps "stale temporary
>> packs"?
>> 
>> Shouldn't we do something similar to objects/pack/pack-*.temp
>> files and objects/??/*.temp that http walker leaves?
>
> Instead, I think http walker should be made to use the same location and 
> filename pattern for its temporary files as the rest of the code.

I concur.  That's much cleaner.

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

[Brandon Casey]

David Tweed wrote:
> On Feb 6, 2008 7:41 PM, Brandon Casey <casey@nrlssc.navy.mil> wrote:
>> They use sprintf for the "%02x" part, but they use memcpy to copy the return
>> of get_object_directory() into a fixed string and then append onto that,
>> rather than repeatedly writing the same string over and over. Ok, there is one
>> instance in builtin-prune.c that repeatedly writes path, but builtin-prune-packed.c
>> does the memcpy thing.
> 

>So I don't think efficiency is an issue.

Yeah not performance critical, readability is definitely more important.

I just don't like repeatedly doing:

   sprintf(dst, "static string %s", s)

when the "static string" part doesn't change.

Either way it doesn't make much difference here.

-brandon