Re: [PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special
On Fri, Mar 10, 2017 at 11:38:10AM -0800, Junio C Hamano wrote: > Jeff King writes: > > > I think we ended up deciding that it would be better to just disallow > > symlink .gitattributes (and .git*) from entering the index, the way we > > disallow ".git". > > Hmph, I thought we would need both, though. Or do we specifically > want to honor untracked .gitattributes that is left as a symlink > pointing to elsewhere in the filesystem or something like that? I wasn't going to worry about an untracked .gitattributes. The reasons for disallowing symlinked .gitattributes are: - it doesn't behave the same when accessed internally via the tree objects - malicious symlinks that try to leave the repository Neither of those issues is at play if your symlink .gitattributes file isn't tracked. So there's some inconsistency in the sense that it "works" until you try to "git add" it. But either you aren't going to add it (in which case it's a feature that it works), or you are going to add it, and you'll get notified then that it's disallowed. -Peff
Re: [PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special
Jeff King writes: > I think we ended up deciding that it would be better to just disallow > symlink .gitattributes (and .git*) from entering the index, the way we > disallow ".git". Hmph, I thought we would need both, though. Or do we specifically want to honor untracked .gitattributes that is left as a symlink pointing to elsewhere in the filesystem or something like that?
Re: [PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special
On Fri, Mar 10, 2017 at 09:58:23AM -0800, Brandon Williams wrote: > > A while back when we discussed whether to allow symlinks for > > .gitattributes, etc, I think the consensus was to treat the whole > > ".git*" namespace consistently. I haven't followed up with patches yet, > > but my plan was to go that route. > > Well if I remember correctly you sent out some patches for > .gitattributes but I got in the way with the refactoring work! :) True. :) But those were the old method that tries to treat .gitattributes specially, by using O_NOFOLLOW in the attribute code (but only for in-tree files, naturally). I think we ended up deciding that it would be better to just disallow symlink .gitattributes (and .git*) from entering the index, the way we disallow ".git". -Peff
Re: [PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special
On 03/09, Jeff King wrote: > On Wed, Mar 08, 2017 at 03:21:11PM -0500, Jeff Hostetler wrote: > > > > And not ."gitmodules"? > > > > > > What happens when we later add ".gitsomethingelse"? > > > > > > Do we have to worry about the case where the set of git "special > > > files" (can we have a better name for them please, by the way?) > > > understood by the sending side and the receiving end is different? > > > > > > I have a feeling that a mode that makes anything whose name begins > > > with ".git" excempt from the size based cutoff may generally be > > > easier to handle. > > > > I forgot about ".gitmodules". The more I think about it, maybe > > we should always include them (or anything starting with ".git*") > > and ignore the size, since they are important for correct behavior. > > I'm also in favor of staking out ".git*" as "this is special and belongs > to Git". I agree, .git* files should probably be the bare minimum of files included. Especially since things like .gitattributes can effect things like checkout. > > A while back when we discussed whether to allow symlinks for > .gitattributes, etc, I think the consensus was to treat the whole > ".git*" namespace consistently. I haven't followed up with patches yet, > but my plan was to go that route. Well if I remember correctly you sent out some patches for .gitattributes but I got in the way with the refactoring work! :) -- Brandon Williams
Re: [PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special
On 3/9/2017 2:31 AM, Jeff King wrote: On Wed, Mar 08, 2017 at 05:37:57PM +, Jeff Hostetler wrote: From: Jeff Hostetler Teach pack-objects to omit blobs from the generated packfile. When the --partial-by-size=n[kmg] argument is used, only blobs smaller than the requested size are included. When n is zero, no blobs are included. When the --partial-special argument is used, git special files, such as ".gitattributes" and ".gitignores" are included. When both are given, the union of two are included. I understand why one would want to do: --partial-by-size=100 --partial-special and get the union. The first one restricts, and the second one adds back in. But I don't understand why "--partial-special" by itself makes any sense. Wouldn't we already be including all blobs, and it would be a noop? My thought was that the "--partial-special" when used by itself would *only* give you the .git* files (and if we had something like a .gitsparse/ directory, everything under it). The client could then do a "special" clone -- mainly to get the sparse checkout templates under .gitsparse/ and then come back for a sparse fetch using one of them. Somewhat of a chicken-n-egg problem, unless the user knows the template names in advance. Also, I was thinking a bit on Junio's comment elsewhere on whether read_object_list_from_stdin() should do the same limiting. I think the answer is "probably not", because whoever is generating that object list can cull the set. You could do it today with something like: git rev-list --objects HEAD | git cat-file --batch-check='%(objectsize) %(objecttype) %(objectname) %(rest)' | perl -lne 's/^(\d+) (\S+) //; print if $2 ne "blob" || $1 < 100' | git pack-objects But if we are going to add this --partial-by-size for the pack-objects traversal, shouldn't we just add it to rev-list? Then: git rev-list --objects --partial-by-size=100 --partial-special | git pack-objects works, and you should get it in the pack-objects basically for free (I think you'd have to allow through the "--partial" arguments on stdin, and make sure the rev-list implementation is done via traverse_commit_list). As a bonus, I suspect it would make the --partial-special path-handling easier, because you'd see each tree entry rather than the fully constructed path (so no more monkeying around with "/"). Interesting. Let me give that a try and see what it looks like. diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 7e052bb..2df2f49 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -77,6 +77,10 @@ static unsigned long cache_max_small_delta_size = 1000; static unsigned long window_memory_limit = 0; +static signed long partial_by_size = -1; I would have expected this to be an off_t, though I think OPT_MAGNITUDE() forces you into "unsigned long". I guess it is nothing new for Git; we use "unsigned long" for single object sizes elsewhere, so systems with a 32-bit long are out of luck anyway until we fix that. The signed "long" here is unfortunate, as it limits us to 2G on such systems. Maybe it is not worth worrying too much about. The "big object" threshold is usually around 500MB. I think the failure behavior is not great, though (asking for "3G" would go negative and effectively be ignored). I think handling all cases would involve swapping out OPT_MAGNITUDE() for a special callback that writes the "yes, the user set this" bit in a separate variable. Yeah, there is a bit of confusion there. I used OPT_MAGNITUDE in one place (for the argument checking), but couldn't in another place. And I tried to pass the original string across the wire for sanity. And I had to fight with the types a little. It would probably be simpler to replace that with a custom handler (or a uint64_t version of magnitude) that would do the right thing and then use that numeric value elsewhere. Thanks, Jeff
Re: [PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special
On Wed, Mar 08, 2017 at 05:37:57PM +, Jeff Hostetler wrote: > From: Jeff Hostetler > > Teach pack-objects to omit blobs from the generated packfile. > > When the --partial-by-size=n[kmg] argument is used, only blobs > smaller than the requested size are included. When n is zero, > no blobs are included. > > When the --partial-special argument is used, git special files, > such as ".gitattributes" and ".gitignores" are included. > > When both are given, the union of two are included. I understand why one would want to do: --partial-by-size=100 --partial-special and get the union. The first one restricts, and the second one adds back in. But I don't understand why "--partial-special" by itself makes any sense. Wouldn't we already be including all blobs, and it would be a noop? Also, I was thinking a bit on Junio's comment elsewhere on whether read_object_list_from_stdin() should do the same limiting. I think the answer is "probably not", because whoever is generating that object list can cull the set. You could do it today with something like: git rev-list --objects HEAD | git cat-file --batch-check='%(objectsize) %(objecttype) %(objectname) %(rest)' | perl -lne 's/^(\d+) (\S+) //; print if $2 ne "blob" || $1 < 100' | git pack-objects But if we are going to add this --partial-by-size for the pack-objects traversal, shouldn't we just add it to rev-list? Then: git rev-list --objects --partial-by-size=100 --partial-special | git pack-objects works, and you should get it in the pack-objects basically for free (I think you'd have to allow through the "--partial" arguments on stdin, and make sure the rev-list implementation is done via traverse_commit_list). As a bonus, I suspect it would make the --partial-special path-handling easier, because you'd see each tree entry rather than the fully constructed path (so no more monkeying around with "/"). > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c > index 7e052bb..2df2f49 100644 > --- a/builtin/pack-objects.c > +++ b/builtin/pack-objects.c > @@ -77,6 +77,10 @@ static unsigned long cache_max_small_delta_size = 1000; > > static unsigned long window_memory_limit = 0; > > +static signed long partial_by_size = -1; I would have expected this to be an off_t, though I think OPT_MAGNITUDE() forces you into "unsigned long". I guess it is nothing new for Git; we use "unsigned long" for single object sizes elsewhere, so systems with a 32-bit long are out of luck anyway until we fix that. The signed "long" here is unfortunate, as it limits us to 2G on such systems. Maybe it is not worth worrying too much about. The "big object" threshold is usually around 500MB. I think the failure behavior is not great, though (asking for "3G" would go negative and effectively be ignored). I think handling all cases would involve swapping out OPT_MAGNITUDE() for a special callback that writes the "yes, the user set this" bit in a separate variable. -Peff
Re: [PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special
On Wed, Mar 08, 2017 at 03:21:11PM -0500, Jeff Hostetler wrote: > > And not ."gitmodules"? > > > > What happens when we later add ".gitsomethingelse"? > > > > Do we have to worry about the case where the set of git "special > > files" (can we have a better name for them please, by the way?) > > understood by the sending side and the receiving end is different? > > > > I have a feeling that a mode that makes anything whose name begins > > with ".git" excempt from the size based cutoff may generally be > > easier to handle. > > I forgot about ".gitmodules". The more I think about it, maybe > we should always include them (or anything starting with ".git*") > and ignore the size, since they are important for correct behavior. I'm also in favor of staking out ".git*" as "this is special and belongs to Git". A while back when we discussed whether to allow symlinks for .gitattributes, etc, I think the consensus was to treat the whole ".git*" namespace consistently. I haven't followed up with patches yet, but my plan was to go that route. -Peff
[PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special
From: Jeff Hostetler
Teach pack-objects to omit blobs from the generated packfile.
When the --partial-by-size=n[kmg] argument is used, only blobs
smaller than the requested size are included. When n is zero,
no blobs are included.
When the --partial-special argument is used, git special files,
such as ".gitattributes" and ".gitignores" are included.
When both are given, the union of two are included.
This is intended to be used in a partial clone or fetch.
(This has also been called sparse- or lazy-clone.)
Signed-off-by: Jeff Hostetler
---
builtin/pack-objects.c | 62 +-
1 file changed, 61 insertions(+), 1 deletion(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 7e052bb..2df2f49 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -77,6 +77,10 @@ static unsigned long cache_max_small_delta_size = 1000;
static unsigned long window_memory_limit = 0;
+static signed long partial_by_size = -1;
+static int partial_special = 0;
+static struct trace_key trace_partial = TRACE_KEY_INIT(PARTIAL);
+
/*
* stats
*/
@@ -2532,6 +2536,54 @@ static void show_object(struct object *obj, const char
*name, void *data)
obj->flags |= OBJECT_ADDED;
}
+/*
+ * If ANY --partial-* option was given, we want to OMIT all
+ * blobs UNLESS they match one of our patterns. We treat
+ * the options as OR's so that we get the resulting UNION.
+ */
+static void show_object_partial(struct object *obj, const char *name, void
*data)
+{
+ unsigned long s = 0;
+
+ if (obj->type != OBJ_BLOB)
+ goto include_it;
+
+ /*
+* When (partial_by_size == 0), we want to OMIT all blobs.
+* When (partial_by_size > 0), we want blobs smaller than that.
+*/
+ if (partial_by_size > 0) {
+ enum object_type t = sha1_object_info(obj->oid.hash, &s);
+ assert(t == OBJ_BLOB);
+ if (s < partial_by_size)
+ goto include_it;
+ }
+
+ /*
+* When (partial_special), we want the .git* special files.
+*/
+ if (partial_special) {
+ if (strcmp(name, GITATTRIBUTES_FILE) == 0 ||
+ strcmp(name, ".gitignore") == 0)
+ goto include_it;
+ else {
+ const char *last_slash = strrchr(name, '/');
+ if (last_slash)
+ if (strcmp(last_slash+1, GITATTRIBUTES_FILE) ==
0 ||
+ strcmp(last_slash+1, ".gitignore") == 0)
+ goto include_it;
+ }
+ }
+
+ trace_printf_key(
+ &trace_partial, "omitting blob '%s' %"PRIuMAX" '%s'\n",
+ oid_to_hex(&obj->oid), (uintmax_t)s, name);
+ return;
+
+include_it:
+ show_object(obj, name, data);
+}
+
static void show_edge(struct commit *commit)
{
add_preferred_base(commit->object.oid.hash);
@@ -2794,7 +2846,11 @@ static void get_object_list(int ac, const char **av)
if (prepare_revision_walk(&revs))
die("revision walk setup failed");
mark_edges_uninteresting(&revs, show_edge);
- traverse_commit_list(&revs, show_commit, show_object, NULL);
+
+ if (partial_by_size >= 0 || partial_special)
+ traverse_commit_list(&revs, show_commit, show_object_partial,
NULL);
+ else
+ traverse_commit_list(&revs, show_commit, show_object, NULL);
if (unpack_unreachable_expiration) {
revs.ignore_missing_links = 1;
@@ -2930,6 +2986,10 @@ int cmd_pack_objects(int argc, const char **argv, const
char *prefix)
N_("use a bitmap index if available to speed up
counting objects")),
OPT_BOOL(0, "write-bitmap-index", &write_bitmap_index,
N_("write a bitmap index together with the pack
index")),
+ OPT_MAGNITUDE(0, "partial-by-size", (unsigned long
*)&partial_by_size,
+N_("only include blobs smaller than size in result")),
+ OPT_BOOL(0, "partial-special", &partial_special,
+N_("only include blobs for git special files")),
OPT_END(),
};
--
2.7.4
Re: [PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special
On 3/8/2017 1:47 PM, Junio C Hamano wrote: Jeff Hostetler writes: From: Jeff Hostetler Teach pack-objects to omit blobs from the generated packfile. When the --partial-by-size=n[kmg] argument is used, only blobs smaller than the requested size are included. When n is zero, no blobs are included. Does this interact with a more traditional way of feeding output of an external "rev-list --objects" to pack-objects via its standard input, and if so, should it (and if not, shouldn't it)? It is perfectly OK if the answer is "this applies only to the case where we generate the list of objects with internal traversal." but that needs to be documented and discussed in the proposed log message. Let me study that and see. I'm still thinking thru ways and options for doing the sparse-checkout like filtering. When the --partial-special argument is used, git special files, such as ".gitattributes" and ".gitignores" are included. And not ."gitmodules"? What happens when we later add ".gitsomethingelse"? Do we have to worry about the case where the set of git "special files" (can we have a better name for them please, by the way?) understood by the sending side and the receiving end is different? I have a feeling that a mode that makes anything whose name begins with ".git" excempt from the size based cutoff may generally be easier to handle. I forgot about ".gitmodules". The more I think about it, maybe we should always include them (or anything starting with ".git*") and ignore the size, since they are important for correct behavior. I am not sure how "back-filling" of a resulting narrow clone would safely be done and how this impacts "git fsck" at this point, but if they are solved within this effort, that would be a very welcome change. Thanks.
Re: [PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special
Jeff Hostetler writes: > From: Jeff Hostetler > > Teach pack-objects to omit blobs from the generated packfile. > > When the --partial-by-size=n[kmg] argument is used, only blobs > smaller than the requested size are included. When n is zero, > no blobs are included. Does this interact with a more traditional way of feeding output of an external "rev-list --objects" to pack-objects via its standard input, and if so, should it (and if not, shouldn't it)? It is perfectly OK if the answer is "this applies only to the case where we generate the list of objects with internal traversal." but that needs to be documented and discussed in the proposed log message. > When the --partial-special argument is used, git special files, > such as ".gitattributes" and ".gitignores" are included. And not ."gitmodules"? What happens when we later add ".gitsomethingelse"? Do we have to worry about the case where the set of git "special files" (can we have a better name for them please, by the way?) understood by the sending side and the receiving end is different? I have a feeling that a mode that makes anything whose name begins with ".git" excempt from the size based cutoff may generally be easier to handle. I am not sure how "back-filling" of a resulting narrow clone would safely be done and how this impacts "git fsck" at this point, but if they are solved within this effort, that would be a very welcome change. Thanks.
[PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special
From: Jeff Hostetler
Teach pack-objects to omit blobs from the generated packfile.
When the --partial-by-size=n[kmg] argument is used, only blobs
smaller than the requested size are included. When n is zero,
no blobs are included.
When the --partial-special argument is used, git special files,
such as ".gitattributes" and ".gitignores" are included.
When both are given, the union of two are included.
This is intended to be used in a partial clone or fetch.
(This has also been called sparse- or lazy-clone.)
Signed-off-by: Jeff Hostetler
---
builtin/pack-objects.c | 62 +-
1 file changed, 61 insertions(+), 1 deletion(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 7e052bb..2df2f49 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -77,6 +77,10 @@ static unsigned long cache_max_small_delta_size = 1000;
static unsigned long window_memory_limit = 0;
+static signed long partial_by_size = -1;
+static int partial_special = 0;
+static struct trace_key trace_partial = TRACE_KEY_INIT(PARTIAL);
+
/*
* stats
*/
@@ -2532,6 +2536,54 @@ static void show_object(struct object *obj, const char
*name, void *data)
obj->flags |= OBJECT_ADDED;
}
+/*
+ * If ANY --partial-* option was given, we want to OMIT all
+ * blobs UNLESS they match one of our patterns. We treat
+ * the options as OR's so that we get the resulting UNION.
+ */
+static void show_object_partial(struct object *obj, const char *name, void
*data)
+{
+ unsigned long s = 0;
+
+ if (obj->type != OBJ_BLOB)
+ goto include_it;
+
+ /*
+* When (partial_by_size == 0), we want to OMIT all blobs.
+* When (partial_by_size > 0), we want blobs smaller than that.
+*/
+ if (partial_by_size > 0) {
+ enum object_type t = sha1_object_info(obj->oid.hash, &s);
+ assert(t == OBJ_BLOB);
+ if (s < partial_by_size)
+ goto include_it;
+ }
+
+ /*
+* When (partial_special), we want the .git* special files.
+*/
+ if (partial_special) {
+ if (strcmp(name, GITATTRIBUTES_FILE) == 0 ||
+ strcmp(name, ".gitignore") == 0)
+ goto include_it;
+ else {
+ const char *last_slash = strrchr(name, '/');
+ if (last_slash)
+ if (strcmp(last_slash+1, GITATTRIBUTES_FILE) ==
0 ||
+ strcmp(last_slash+1, ".gitignore") == 0)
+ goto include_it;
+ }
+ }
+
+ trace_printf_key(
+ &trace_partial, "omitting blob '%s' %"PRIuMAX" '%s'\n",
+ oid_to_hex(&obj->oid), (uintmax_t)s, name);
+ return;
+
+include_it:
+ show_object(obj, name, data);
+}
+
static void show_edge(struct commit *commit)
{
add_preferred_base(commit->object.oid.hash);
@@ -2794,7 +2846,11 @@ static void get_object_list(int ac, const char **av)
if (prepare_revision_walk(&revs))
die("revision walk setup failed");
mark_edges_uninteresting(&revs, show_edge);
- traverse_commit_list(&revs, show_commit, show_object, NULL);
+
+ if (partial_by_size >= 0 || partial_special)
+ traverse_commit_list(&revs, show_commit, show_object_partial,
NULL);
+ else
+ traverse_commit_list(&revs, show_commit, show_object, NULL);
if (unpack_unreachable_expiration) {
revs.ignore_missing_links = 1;
@@ -2930,6 +2986,10 @@ int cmd_pack_objects(int argc, const char **argv, const
char *prefix)
N_("use a bitmap index if available to speed up
counting objects")),
OPT_BOOL(0, "write-bitmap-index", &write_bitmap_index,
N_("write a bitmap index together with the pack
index")),
+ OPT_MAGNITUDE(0, "partial-by-size", (unsigned long
*)&partial_by_size,
+N_("only include blobs smaller than size in result")),
+ OPT_BOOL(0, "partial-special", &partial_special,
+N_("only include blobs for git special files")),
OPT_END(),
};
--
2.7.4
