threads / patch / 46142

patchfast-import: Increase the default pack depth to 50

Subject: [PATCH] fast-import: Increase the default pack depth to 50

## tl;dr

4 messages between Jun 8, 2017 and Jun 8, 2017. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Mike Hommey· Jun 8, 2017, 05:34 UTC · lore

In 618e613a70, 10 years ago, the default for pack depth used for git-pack-objects and git-repack was changed from 10 to 50, while leaving fast-import's default to 10.

There doesn't seem to be a reason besides oversight for the change not having happened in fast-import as well.

Interestingly, fast-import uses pack.depth when it's set, and the git-config manual says the default for pack.depth is 50. While the git-fast-import manual does say the default depth is 10, the inconsistency is also confusing.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 Documentation/git-fast-import.txt | 2 +-
 fast-import.c                     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Show changes to 2 files +2 −2

Documentation/git-fast-import.txt, fast-import.c

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 2b762654bf..3d3d219e58 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -121,7 +121,7 @@ Performance and Compression Tuning
 
 --depth=<n>::
 	Maximum delta depth, for blob and tree deltification.
-	Default is 10.
+	Default is 50.
 
 --export-pack-edges=<file>::
 	After creating a packfile, print a line of data to
diff --git a/fast-import.c b/fast-import.c
index e69d219682..05c73858b0 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -280,7 +280,7 @@ struct recent_command {
 };
 
 /* Configured limits on output */
-static unsigned long max_depth = 10;
+static unsigned long max_depth = 50;
 static off_t max_packsize;
 static int unpack_limit = 100;
 static int force_update;

-- 
2.13.0

Jeff King· Jun 8, 2017, 07:05 UTC · re: Mike Hommey · lore

Re: [PATCH] fast-import: Increase the default pack depth to 50

On Thu, Jun 08, 2017 at 02:34:36PM +0900, Mike Hommey wrote:

Show 11 quoted lines

> In 618e613a70, 10 years ago, the default for pack depth used for
> git-pack-objects and git-repack was changed from 10 to 50, while
> leaving fast-import's default to 10.
> 
> There doesn't seem to be a reason besides oversight for the change not
> having happened in fast-import as well.
> 
> Interestingly, fast-import uses pack.depth when it's set, and the
> git-config manual says the default for pack.depth is 50. While the
> git-fast-import manual does say the default depth is 10, the
> inconsistency is also confusing.

Makes sense. If anything, fast-import would want to allow a deeper depth than normal, since (IIRC) its delta chains are always completely linear. Whereas in a real pack, if we decide not to make a delta off the 50th item in a chain, we usually find the 48th or 49th, and end up with a bushier graph.

It probably doesn't matter that much, though, as you'd really want to `repack -f` afterwards if you care about getting good deltas. And one base object every 50 versions is probably fine for keeping the initial pack manageable.

> ---
>  Documentation/git-fast-import.txt | 2 +-
>  fast-import.c                     | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Patch itself looks obviously correct.

-Peff

Mike Hommey· Jun 8, 2017, 07:11 UTC · re: Jeff King · lore

Re: [PATCH] fast-import: Increase the default pack depth to 50

On Thu, Jun 08, 2017 at 03:05:37AM -0400, Jeff King wrote:

Show 24 quoted lines

> On Thu, Jun 08, 2017 at 02:34:36PM +0900, Mike Hommey wrote:
> 
> > In 618e613a70, 10 years ago, the default for pack depth used for
> > git-pack-objects and git-repack was changed from 10 to 50, while
> > leaving fast-import's default to 10.
> > 
> > There doesn't seem to be a reason besides oversight for the change not
> > having happened in fast-import as well.
> > 
> > Interestingly, fast-import uses pack.depth when it's set, and the
> > git-config manual says the default for pack.depth is 50. While the
> > git-fast-import manual does say the default depth is 10, the
> > inconsistency is also confusing.
> 
> Makes sense. If anything, fast-import would want to allow a deeper depth
> than normal, since (IIRC) its delta chains are always completely linear.
> Whereas in a real pack, if we decide not to make a delta off the 50th
> item in a chain, we usually find the 48th or 49th, and end up with a
> bushier graph.
> 
> It probably doesn't matter that much, though, as you'd really want to
> `repack -f` afterwards if you care about getting good deltas. And one
> base object every 50 versions is probably fine for keeping the initial
> pack manageable.

It actually is possible to have non-linear delta chains with fast-import, because the cat-blob command resets the delta base when storing a blob. See https://github.com/git/git/blob/v2.13.1/fast-import.c#L2984-L2987

As a side effect of that, git-cinnabar[1] ends up with decent-ish delta chains out of the box, without having to go with repack -f, when the mercurial server gives out changegroup version 2.

Mike

1. https://github.com/glandium/git-cinnabar/

Jeff King· Jun 8, 2017, 07:21 UTC · re: Mike Hommey · lore

Re: [PATCH] fast-import: Increase the default pack depth to 50

On Thu, Jun 08, 2017 at 04:11:52PM +0900, Mike Hommey wrote:

Show 13 quoted lines

> > It probably doesn't matter that much, though, as you'd really want to
> > `repack -f` afterwards if you care about getting good deltas. And one
> > base object every 50 versions is probably fine for keeping the initial
> > pack manageable.
> 
> It actually is possible to have non-linear delta chains with
> fast-import, because the cat-blob command resets the delta base when
> storing a blob. See
> https://github.com/git/git/blob/v2.13.1/fast-import.c#L2984-L2987
> 
> As a side effect of that, git-cinnabar[1] ends up with decent-ish delta
> chains out of the box, without having to go with repack -f, when the
> mercurial server gives out changegroup version 2.

Interesting. Today I learned.

I agree that bumping the "10" to "50" is doubly important then.

-Peff