Re: git-svn should default to --repack

Karl Hasselström [off-list ref] writes:

On 2008-01-18 07:17:55 -0500, Kevin Ballard wrote:

quoted

I was very surprised to find that git-svn does not in fact default
to --repack. I firmly believe it should.

I believe so too. And nowadays there's "git gc --auto", which was made
for occasions such as this, so it should be a breeze to implement. The
overhead might be low enough that it can be called after _every_
imported revision.

Careful.  I made the same mistake and it had to be corrected
with e0cd252eb0ba6453acd64762625b004aa4cc162b.

"gc --auto" after every 1000 or so feels like a good default and
I would agree that would be a real fix to a real usability bug.

Patches?

On 2008-01-18 12:44:08 -0800, Junio C Hamano wrote:

Karl Hasselström [off-list ref] writes:

quoted

I believe so too. And nowadays there's "git gc --auto", which was
made for occasions such as this, so it should be a breeze to
implement. The overhead might be low enough that it can be called
after _every_ imported revision.

Careful. I made the same mistake and it had to be corrected with
e0cd252eb0ba6453acd64762625b004aa4cc162b.

"gc --auto" after every 1000 or so feels like a good default and I
would agree that would be a real fix to a real usability bug.

I think 1000 might be too high; considering that (at least in my
experience) it takes on the order of 250-500 ms to import a commit,
the gc --auto overhead of maybe 10 ms isn't so bad.

A good compromise might be to run gc --auto after every 10-100
commits, _and_ when the import is done.

However, if gc --auto always takes a lot of time without accomplishing
anything in the presence of too many unreachable loose objects it
might not be a good idea to run it at all, since the use of git-svn
involves frequent rebasing.

Patches?

Just hot air and noise for now from my end. Sorry.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Note: CC list pruned as, once again, my Mail client decided to send  
the original message as HTML and it got bounced from the list.
Original CC list: kha@treskal.com, gitster@pobox.com

On Jan 19, 2008, at 7:35 AM, Karl Hasselström wrote:

On 2008-01-18 12:44:08 -0800, Junio C Hamano wrote:

quoted

Karl Hasselström [off-list ref] writes:

quoted

I believe so too. And nowadays there's "git gc --auto", which was
made for occasions such as this, so it should be a breeze to
implement. The overhead might be low enough that it can be called
after _every_ imported revision.

Careful. I made the same mistake and it had to be corrected with
e0cd252eb0ba6453acd64762625b004aa4cc162b.

"gc --auto" after every 1000 or so feels like a good default and I
would agree that would be a real fix to a real usability bug.

I think 1000 might be too high; considering that (at least in my
experience) it takes on the order of 250-500 ms to import a commit,
the gc --auto overhead of maybe 10 ms isn't so bad.

A good compromise might be to run gc --auto after every 10-100
commits, _and_ when the import is done.

However, if gc --auto always takes a lot of time without accomplishing
anything in the presence of too many unreachable loose objects it
might not be a good idea to run it at all, since the use of git-svn
involves frequent rebasing.

I don't know much about how this works, so if git gc --auto might have  
a problem, it seems the simplest fix for now would be to default git- 
svn to having --repack=1000 on.

quoted

Patches?

Just hot air and noise for now from my end. Sorry.

Same. I don't know Perl. Sorry.

-Kevin Ballard

-- 
Kevin Ballard
http://kevin.sb.org
kevin@sb.org
http://www.tildesoft.com

Let "git svn" run "git gc --auto" every 100 imported commits, to
reduce the number of loose objects.

To handle the common use case of frequent imports, where each
invocation typically fetches less than 100 commits, randomly set the
counter to something in the range 1-100 on initialization. It's almost
as good as saving the counter, and much less of a hassle.

Oh, and 100 is just my best guess at a reasonable number. It could
conceivably need tweaking.

Signed-off-by: Karl Hasselström <redacted>

---

On 2008-01-19 13:35:57 +0100, Karl Hasselström wrote:

On 2008-01-18 12:44:08 -0800, Junio C Hamano wrote:

quoted

Patches?

Just hot air and noise for now from my end. Sorry.

OK, it didn't feel good saying that. So here's my attempt at being a
model citizen. (It's not hard with a change this small ...)

I'm not quite sure how this should interact with the --repack flag.
Right now they just coexist, except for never running right after one
another, but conceivably we should do something cleverer. Eric?

 git-svn.perl |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 9f2b587..89e1d61 100755
--- a/git-svn.perl
+++ b/git-svn.perl

@@ -1247,7 +1247,7 @@ use File::Path qw/mkpath/;
 use File::Copy qw/copy/;
 use IPC::Open3;
 
-my $_repack_nr;
+my ($_repack_nr, $_gc_nr, $_gc_period);
 # properties that we do not log:
 my %SKIP_PROP;
 BEGIN {

@@ -1413,6 +1413,8 @@ sub init_vars {
 		$_repack_nr = $_repack;
 		$_repack_flags ||= '-d';
 	}
+	$_gc_period = 100;
+	$_gc_nr = int(rand($_gc_period)) + 1;
 }
 
 sub verify_remotes_sanity {

@@ -2157,6 +2159,9 @@ sub do_git_commit {
 		print "Running git repack $_repack_flags ...\n";
 		command_noisy('repack', split(/\s+/, $_repack_flags));
 		print "Done repacking\n";
+	} elsif (--$_gc_nr == 0) {
+		$_gc_nr = $_gc_period;
+		command_noisy('gc', '--auto');
 	}
 	return $commit;
 }

On Sat, 2008-01-19 at 23:36 +0100, Karl Hasselström wrote:

Let "git svn" run "git gc --auto" every 100 imported commits, to
reduce the number of loose objects.

I found 100 was a bit too low when doing some large repos, I've
been using 1000.  I'd argue that --repack=1000 should be done by
default.

I'm not quite sure how this should interact with the --repack flag.
Right now they just coexist, except for never running right after one
another, but conceivably we should do something cleverer. Eric?

How about git gc always gets run at the very end of a git svn fetch?

Just a thought.

Harvey

Harvey Harrison [off-list ref] wrote:

On Sat, 2008-01-19 at 23:36 +0100, Karl Hasselström wrote:

quoted

Let "git svn" run "git gc --auto" every 100 imported commits, to
reduce the number of loose objects.

I found 100 was a bit too low when doing some large repos, I've
been using 1000.  I'd argue that --repack=1000 should be done by
default.

I've found 100 for repack too low in the past, too, which is why
repack defaults to 1000 if no number is specified.  I think it
should hold for gc --auto, too.

quoted

I'm not quite sure how this should interact with the --repack flag.
Right now they just coexist, except for never running right after one
another, but conceivably we should do something cleverer. Eric?

I consider --repack is out-of-date now that we have gc --auto.  I'm in
favor of ripping out repack support in git-svn and just using gc --auto.

How about git gc always gets run at the very end of a git svn fetch?

I'd much prefer that we run gc --auto at the end of every fetch instead
of doing so randomly for small fetches.

-- 
Eric Wong

On 2008-01-19 19:37:37 -0800, Eric Wong wrote:

Harvey Harrison [off-list ref] wrote:

quoted

I found 100 was a bit too low when doing some large repos, I've
been using 1000. I'd argue that --repack=1000 should be done by
default.

I've found 100 for repack too low in the past, too, which is why
repack defaults to 1000 if no number is specified. I think it should
hold for gc --auto, too.

OK, I'll change it. But remember, gc --auto doesn't do _anything_
unless it's deemed necessary, so it should behave much better than
just plain repack. In theory at least.

I consider --repack is out-of-date now that we have gc --auto. I'm
in favor of ripping out repack support in git-svn and just using gc
--auto.

Will do. What should I do with the repack commadline options? Keep
them for backwards compatibility but ignore them?

quoted

How about git gc always gets run at the very end of a git svn
fetch?

I'd much prefer that we run gc --auto at the end of every fetch
instead of doing so randomly for small fetches.

OK, will do. I'll just have to find a good spot to call it from. Hints
welcome.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

In a moment, we'll start calling git-gc --auto instead, since it is a
better fit to what we're trying to accomplish.

The command line options are still accepted, but don't have any
effect, and we warn the user about that.

Signed-off-by: Karl Hasselström <redacted>

---

Is this close enough to what you intended?

 git-svn.perl |   14 ++------------
 1 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 9f2b587..988d8f6 100755
--- a/git-svn.perl
+++ b/git-svn.perl

@@ -1247,7 +1247,6 @@ use File::Path qw/mkpath/;
 use File::Copy qw/copy/;
 use IPC::Open3;
 
-my $_repack_nr;
 # properties that we do not log:
 my %SKIP_PROP;
 BEGIN {

@@ -1408,10 +1407,8 @@ sub read_all_remotes {
 }
 
 sub init_vars {
-	if (defined $_repack) {
-		$_repack = 1000 if ($_repack <= 0);
-		$_repack_nr = $_repack;
-		$_repack_flags ||= '-d';
+	if (defined $_repack || defined $_repack_flags) {
+               warn "Repack options are obsolete; they have no effect.\n";
 	}
 }
 

@@ -2151,13 +2148,6 @@ sub do_git_commit {
 		                   0, $self->svm_uuid);
 	}
 	print " = $commit ($self->{ref_id})\n";
-	if (defined $_repack && (--$_repack_nr == 0)) {
-		$_repack_nr = $_repack;
-		# repack doesn't use any arguments with spaces in them, does it?
-		print "Running git repack $_repack_flags ...\n";
-		command_noisy('repack', split(/\s+/, $_repack_flags));
-		print "Done repacking\n";
-	}
 	return $commit;
 }
 

Let "git svn" run "git gc --auto" every 1000 imported commits to
reduce the number of loose objects.

To handle the common use case of frequent imports, where each
invocation typically fetches much less than 1000 commits, also run gc
unconditionally at the end of the import.

"1000" is the same number that was used by default when we called
git-repack. It isn't necessarily still the best choice.

Signed-off-by: Karl Hasselström <redacted>

---

 git-svn.perl |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 988d8f6..be4105c 100755
--- a/git-svn.perl
+++ b/git-svn.perl

@@ -1247,6 +1247,8 @@ use File::Path qw/mkpath/;
 use File::Copy qw/copy/;
 use IPC::Open3;
 
+my ($_gc_nr, $_gc_period);
+
 # properties that we do not log:
 my %SKIP_PROP;
 BEGIN {

@@ -1407,6 +1409,7 @@ sub read_all_remotes {
 }
 
 sub init_vars {
+	$_gc_nr = $_gc_period = 1000;
 	if (defined $_repack || defined $_repack_flags) {
                warn "Repack options are obsolete; they have no effect.\n";
 	}

@@ -2095,6 +2098,10 @@ sub restore_commit_header_env {
 	}
 }
 
+sub gc {
+	command_noisy('gc', '--auto');
+};
+
 sub do_git_commit {
 	my ($self, $log_entry) = @_;
 	my $lr = $self->last_rev;

@@ -2148,6 +2155,10 @@ sub do_git_commit {
 		                   0, $self->svm_uuid);
 	}
 	print " = $commit ($self->{ref_id})\n";
+	if (--$_gc_nr == 0) {
+		$_gc_nr = $_gc_period;
+		gc();
+	}
 	return $commit;
 }
 

@@ -3975,6 +3986,7 @@ sub gs_fetch_loop_common {
 		$max += $inc;
 		$max = $head if ($max > $head);
 	}
+	Git::SVN::gc();
 }
 
 sub match_globs {