Partial bundles

From: Derrick Stolee <[email protected]>

Since 'git bundle' uses setup_revisions() to specify the object walk, some options do not make sense to include during the pack-objects child process. Further, these options are used for a call to traverse_commit_list() which would then require a callback which is currently NULL.

By populating the callback we prevent a segfault in the case of adding the --objects flag. This is really a redundant statement because the

bundles are constructing a pack-file containing all objects in the discovered commit range.

Adding --objects to a 'git bundle' command might cause a slower command, but at least it will not have a hard failure when the user supplies this option. We can also disable walking trees and blobs in advance of this walk.

Signed-off-by Derrick Stolee <[email protected]>

---
  bundle.c               | 10 +++++++++-
  t/t6020-bundle-misc.sh | 12 ++++++++++++
  2 files changed, 21 insertions(+), 1 deletion(-)

bundle.c +9 -1

index a0bb687b0f4..dc56db9a50a 100644
@@ -451,6 +451,12 @@ struct bundle_prerequisites_info {
  	int fd;
  };
 
+
+static void ignore_object(struct object *obj, const char *v, void *data)
+{
+	/* Do nothing. */
+}
+
  static void write_bundle_prerequisites(struct commit *commit, void *data)
  {
  	struct bundle_prerequisites_info *bpi = data;

@@ -544,7 +550,9 @@ int create_bundle(struct repository *r, const char *path,
  		die("revision walk setup failed");
  	bpi.fd = bundle_fd;
  	bpi.pending = &revs_copy.pending;
-	traverse_commit_list(&revs, write_bundle_prerequisites, NULL, &bpi);
+
+	revs.blob_objects = revs.tree_objects = 0;
+	traverse_commit_list(&revs, write_bundle_prerequisites, ignore_object, &bpi);
  	object_array_remove_duplicates(&revs_copy.pending);
 
  	/* write bundle refs */

t/t6020-bundle-misc.sh +12 -0

index b13e8a52a93..6522401617d 100755
@@ -475,4 +475,16 @@ test_expect_success 'clone from bundle' '
  	test_cmp expect actual
  '
 
+test_expect_success 'unfiltered bundle with --objects' '
+	git bundle create all-objects.bdl \
+		--all --objects &&
+	git bundle create all.bdl \
+		--all &&
+
+	# Compare the headers of these files.
+	head -11 all.bdl >expect &&
+	head -11 all-objects.bdl >actual &&
+	test_cmp expect actual
+'
+
  test_done

While discussing bundle-URIs [1], it came to my attention that bundles have no way to specify an object filter, so bundles cannot be used with partial clones.

[1]
https://lore.kernel.org/git/[email protected]/

This series provides a way to fix that by adding a 'filter' capability to the bundle file format and allowing one to create a partial bundle with 'git bundle create --filter=blob:none '.

There are a couple things that I want to point out about this implementation that could use some high-level feedback:

1. I moved the '--filter' parsing into setup_revisions() instead of adding another place to parse it. This works for 'git bundle' but it also allows it to be parsed successfully in commands such as 'git diff' which doesn't make sense. Options such as '--objects' are already being parsed there, and they don't make sense either, so I want some thoughts on this.

2. If someone uses 'git clone partial.bdl partial' where 'partial.bdl' is a filtered bundle, then the clone will fail with a message such as

fatal: missing blob object '9444604d515c0b162e37e59accd54a0bac50ed2e' fatal:
remote did not send all necessary objects

This might be fine. We don't expect users to clone partial bundles or fetch partial bundles into an unfiltered repo and these failures are expected. It is possible that we could put in custom logic to fail faster by reading the bundle header for a filter.

Generally, the idea is to open this up as a potential way to bootstrap a clone of a partial clone using a set of precomputed partial bundles.

Thanks, -Stolee

Derrick Stolee (11):
index-pack: document and test the --promisor option
revision: put object filter into struct rev_info
pack-objects: use rev.filter when possible
pack-bitmap: drop filter in prepare_bitmap_walk()
list-objects: consolidate traverse_commit_list[_filtered]
MyFirstObjectWalk: update recommended usage
bundle: safely handle --objects option
bundle: parse filter capability
rev-list: move --filter parsing into revision.c
bundle: create filtered bundles
bundle: unbundle promisor packs

  Documentation/MyFirstObjectWalk.txt | 44 ++++++---------
  Documentation/git-index-pack.txt    |  8 +++
  builtin/pack-objects.c              |  9 +--
  builtin/rev-list.c                  | 29 +++-------
  bundle.c                            | 87 ++++++++++++++++++++++++-----
  bundle.h                            |  3 +
  list-objects-filter-options.c       |  2 +-
  list-objects-filter-options.h       |  5 ++
  list-objects.c                      | 25 +++------
  list-objects.h                      | 12 +++-
  pack-bitmap.c                       | 24 ++++----
  pack-bitmap.h                       |  2 -
  reachable.c                         |  2 +-
  revision.c                          | 11 ++++
  revision.h                          |  4 ++
  t/t5300-pack-object.sh              |  4 +-
  t/t6020-bundle-misc.sh              | 48 ++++++++++++++++
  17 files changed, 215 insertions(+), 104 deletions(-)

base-commit: 45fe28c951c3e70666ee4ef8379772851a8e4d32
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1159%2Fderrickstolee%2Fbundle%2Fpartial-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1159/derrickstolee/bundle/partial-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1159

Quoted 10 lines · Derrick Stolee via GitGitGadget

While discussing bundle-URIs [1], it came to my attention that bundles have no way to specify an object filter, so bundles cannot be used with partial clones.

[1]
https://lore.kernel.org/git/[email protected]/

This series provides a way to fix that by adding a 'filter' capability to the bundle file format and allowing one to create a partial bundle with 'git bundle create --filter=blob:none '.

Nicely done.  There's a lot of refactoring here to move the filtering code into a more usable place and get rid of some of the awkward limitations of my original code.  Sorry that you had to slog thru all of that.

Quoted 10 lines

There are a couple things that I want to point out about this implementation that could use some high-level feedback:

  1. I moved the '--filter' parsing into setup_revisions() instead of adding another place to parse it. This works for 'git bundle' but it also allows it to be parsed successfully in commands such as 'git diff' which doesn't make sense. Options such as '--objects' are already being parsed there, and they don't make sense either, so I want some thoughts on this.

This feels like something that can wait for another task. Let's keep this series focused on adding partial bundles.

Quoted 6 lines

  2. If someone uses 'git clone partial.bdl partial' where 'partial.bdl' is a filtered bundle, then the clone will fail with a message such as

fatal: missing blob object '9444604d515c0b162e37e59accd54a0bac50ed2e' fatal:
remote did not send all necessary objects

Quoted 7 lines

This might be fine. We don't expect users to clone partial bundles or fetch partial bundles into an unfiltered repo and these failures are expected. It is possible that we could put in custom logic to fail faster by reading the bundle header for a filter.

Generally, the idea is to open this up as a potential way to bootstrap a clone of a partial clone using a set of precomputed partial bundles.

I think this is to be expected.

Would it help to have Git do a no-checkout clone when cloning from a partial bundle?  Maybe that would give the user a chance to set a real remote (and maybe set the partial clone/fetch config settings) and then backfill their local clone??   (That might be functional, but not very user-friendly....)

Or should we just consider this limitation as a placeholder while we wait for the Bundle URI effort?

Quoted 28 lines · Jeff Hostetler

Quoted 10 lines · Derrick Stolee via GitGitGadget

While discussing bundle-URIs [1], it came to my attention that bundles have no way to specify an object filter, so bundles cannot be used with partial clones.

[1]
https://lore.kernel.org/git/[email protected]/

This series provides a way to fix that by adding a 'filter' capability to the bundle file format and allowing one to create a partial bundle with 'git bundle create --filter=blob:none '.

Nicely done.  There's a lot of refactoring here to move the filtering code into a more usable place and get rid of some of the awkward limitations of my original code.  Sorry that you had to slog thru all of that.

Quoted 10 lines

There are a couple things that I want to point out about this implementation that could use some high-level feedback:

  1. I moved the '--filter' parsing into setup_revisions() instead of adding another place to parse it. This works for 'git bundle' but it also allows it to be parsed successfully in commands such as 'git diff' which doesn't make sense. Options such as '--objects' are already being parsed there, and they don't make sense either, so I want some thoughts on this.

Quoted 2 lines

This feels like something that can wait for another task. Let's keep this series focused on adding partial bundles.

What do you mean "can wait"? Do you recommend that I _don't_ do this refactor and instead implement filter parsing directly in bundles?

Or, are you saying that we should not worry about these potential side-effects of allowing (then ignoring) certain options in other commands, at least until a later series?

Quoted 17 lines

Quoted 7 lines

  2. If someone uses 'git clone partial.bdl partial' where 'partial.bdl' is a filtered bundle, then the clone will fail with a message such as

fatal: missing blob object '9444604d515c0b162e37e59accd54a0bac50ed2e' fatal:
remote did not send all necessary objects

Quoted 7 lines

This might be fine. We don't expect users to clone partial bundles or fetch partial bundles into an unfiltered repo and these failures are expected. It is possible that we could put in custom logic to fail faster by reading the bundle header for a filter.

Generally, the idea is to open this up as a potential way to bootstrap a clone of a partial clone using a set of precomputed partial bundles.

I think this is to be expected.

Quoted 8 lines

Would it help to have Git do a no-checkout clone when cloning from a partial bundle?  Maybe that would give the user a chance to set a real remote (and maybe set the partial clone/fetch config settings) and then backfill their local clone??   (That might be functional, but not very user-friendly....)

Or should we just consider this limitation as a placeholder while we wait for the Bundle URI effort?

It would be interesting to have another application of partial bundles, such as cloning directly from a bundle, then allowing a remote to be configured. It provides a "build-it-yourself" approach to bundle URIs for partial clones.

I'm not sure if such an application is required for this series, or if it could be delayed until after. I'm open to suggestions.

While discussing bundle-URIs [1], it came to my attention that bundles have no way to specify an object filter, so bundles cannot be used with partial clones.

[1]
https://lore.kernel.org/git/[email protected]/

This series provides a way to fix that by adding a 'filter' capability to the bundle file format and allowing one to create a partial bundle with 'git bundle create --filter=blob:none '.

There are a couple things that I want to point out about this implementation that could use some high-level feedback:

1. I moved the '--filter' parsing into setup_revisions() instead of adding another place to parse it. This works for 'git bundle' but it also allows it to be parsed successfully in commands such as 'git diff' which doesn't make sense. Options such as '--objects' are already being parsed there, and they don't make sense either, so I want some thoughts on this.

2. If someone uses 'git clone partial.bdl partial' where 'partial.bdl' is a filtered bundle, then the clone will fail with a message such as

fatal: missing blob object '9444604d515c0b162e37e59accd54a0bac50ed2e' fatal:
remote did not send all necessary objects

This might be fine. We don't expect users to clone partial bundles or fetch partial bundles into an unfiltered repo and these failures are expected. It is possible that we could put in custom logic to fail faster by reading the bundle header for a filter.

Generally, the idea is to open this up as a potential way to bootstrap a clone of a partial clone using a set of precomputed partial bundles.

revision.h +3 -0

 struct rev_info {
 	/* Starting list */

@@ -94,6 +95,9 @@ struct rev_info {
 	/* The end-points specified by the end user */
 	struct rev_cmdline_info cmdline;
 
+	/* Object filter options. NULL for no filtering. */
+	struct list_objects_filter_options *filter;
+
 	/* excluding from --branches, --refs, etc. expansion */
 	struct string_list *ref_excludes;

From: Derrick Stolee <[email protected]>

In builtin/pack-objects.c, we use a 'filter_options' global to populate the --filter=<X> argument. The previous change created a pointer to a filter option in 'struct rev_info', so we can use that pointer here as a start to simplifying some usage of object filters.

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/pack-objects.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

builtin/pack-objects.c +1 -1

index ba2006f2212..256d9b1798f 100644
@@ -3651,7 +3651,7 @@ static int pack_options_allow_reuse(void)
 
 static int get_object_list_from_bitmap(struct rev_info *revs)
 {
-	if (!(bitmap_git = prepare_bitmap_walk(revs, &filter_options, 0)))
+	if (!(bitmap_git = prepare_bitmap_walk(revs, revs->filter, 0)))
 		return -1;
 
 	if (pack_options_allow_reuse() &&

builtin/pack-objects.c +1 -1

 	/* make sure shallows are read */
 	is_repository_shallow(the_repository);

@@ -3777,7 +3778,7 @@ static void get_object_list(int ac, const char **av)
 
 	if (!fn_show_object)
 		fn_show_object = show_object;
-	traverse_commit_list_filtered(&filter_options, &revs,
+	traverse_commit_list_filtered(revs.filter, &revs,
 				      show_commit, fn_show_object, NULL,
 				      NULL);

From: Derrick Stolee <[email protected]>

Now that all consumers of prepare_bitmap_walk() have populated the 'filter' member of 'struct rev_info', we can drop that extra parameter from the method and access it directly from the 'struct rev_info'.

From: Derrick Stolee <[email protected]>

Now that all consumers of traverse_commit_list_filtered() populate the 'filter' member of 'struct rev_info', we can drop that parameter from the method prototype to simplify things. In addition, the only thing different now between traverse_commit_list_filtered() and traverse_commit_list() is the presence of the 'omitted' parameter, which is only non-NULL for one caller. We can consolidate these two methods by having one call the other and use the simpler form everywhere the 'omitted' paramter would be NULL.

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/pack-objects.c |  6 +++---
 builtin/rev-list.c     |  2 +-
 list-objects.c         | 25 ++++++++-----------------
 list-objects.h         | 12 ++++++++++--
 pack-bitmap.c          |  6 +++---
 5 files changed, 25 insertions(+), 26 deletions(-)

builtin/pack-objects.c +3 -3

index 57f2cf49696..0432ae1e499 100644
@@ -3778,9 +3778,9 @@ static void get_object_list(int ac, const char **av)
 
 	if (!fn_show_object)
 		fn_show_object = show_object;
-	traverse_commit_list_filtered(revs.filter, &revs,
-				      show_commit, fn_show_object, NULL,
-				      NULL);
+	traverse_commit_list(&revs,
+			     show_commit, fn_show_object,
+			     NULL);
 
 	if (unpack_unreachable_expiration) {
 		revs.ignore_missing_links = 1;

builtin/rev-list.c +1 -1

index 556e78aebb9..3ab727817fd 100644
@@ -733,7 +733,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 		oidset_init(&missing_objects, DEFAULT_OIDSET_SIZE);
 
 	traverse_commit_list_filtered(
-		revs.filter, &revs, show_commit, show_object, &info,
+		&revs, show_commit, show_object, &info,
 		(arg_print_omitted ? &omitted_objects : NULL));
 
 	if (arg_print_omitted) {

list-objects.c +8 -17

index 2f623f82115..9422625b39e 100644
@@ -416,22 +416,7 @@ static void do_traverse(struct traversal_context *ctx)
 	strbuf_release(&csp);
 }
 
-void traverse_commit_list(struct rev_info *revs,
-			  show_commit_fn show_commit,
-			  show_object_fn show_object,
-			  void *show_data)
-{
-	struct traversal_context ctx;
-	ctx.revs = revs;
-	ctx.show_commit = show_commit;
-	ctx.show_object = show_object;
-	ctx.show_data = show_data;
-	ctx.filter = NULL;
-	do_traverse(&ctx);
-}
-
 void traverse_commit_list_filtered(
-	struct list_objects_filter_options *filter_options,
 	struct rev_info *revs,
 	show_commit_fn show_commit,
 	show_object_fn show_object,

@@ -444,7 +429,13 @@ void traverse_commit_list_filtered(
 	ctx.show_object = show_object;
 	ctx.show_commit = show_commit;
 	ctx.show_data = show_data;
-	ctx.filter = list_objects_filter__init(omitted, filter_options);
+	if (revs->filter)
+		ctx.filter = list_objects_filter__init(omitted, revs->filter);
+	else
+		ctx.filter = NULL;
+
 	do_traverse(&ctx);
-	list_objects_filter__free(ctx.filter);
+
+	if (ctx.filter)
+		list_objects_filter__free(ctx.filter);
 }

list-objects.h +10 -2

index a952680e466..9eaf4de8449 100644
@@ -7,7 +7,6 @@ struct rev_info;
 
 typedef void (*show_commit_fn)(struct commit *, void *);
 typedef void (*show_object_fn)(struct object *, const char *, void *);
-void traverse_commit_list(struct rev_info *, show_commit_fn, show_object_fn, void *);
 
 typedef void (*show_edge_fn)(struct commit *);
 void mark_edges_uninteresting(struct rev_info *revs,

@@ -18,11 +17,20 @@ struct oidset;
 struct list_objects_filter_options;
 
 void traverse_commit_list_filtered(
-	struct list_objects_filter_options *filter_options,
 	struct rev_info *revs,
 	show_commit_fn show_commit,
 	show_object_fn show_object,
 	void *show_data,
 	struct oidset *omitted);
 
+static inline void traverse_commit_list(
+	struct rev_info *revs,
+	show_commit_fn show_commit,
+	show_object_fn show_object,
+	void *show_data)
+{
+	traverse_commit_list_filtered(revs, show_commit,
+				      show_object, show_data, NULL);
+}
+
 #endif /* LIST_OBJECTS_H */

pack-bitmap.c +3 -3

index 613f2797cdf..cbefaedbf43 100644
@@ -822,9 +822,9 @@ static struct bitmap *find_objects(struct bitmap_index *bitmap_git,
 		show_data.bitmap_git = bitmap_git;
 		show_data.base = base;
 
-		traverse_commit_list_filtered(revs->filter, revs,
-					      show_commit, show_object,
-					      &show_data, NULL);
+		traverse_commit_list(revs,
+				     show_commit, show_object,
+				     &show_data);
 
 		revs->include_check = NULL;
 		revs->include_check_obj = NULL;

From: Derrick Stolee <[email protected]>

The previous change consolidated traverse_commit_list() and traverse_commit_list_filtered(). This allows us to simplify the recommended usage in MyFirstObjectWalk.txt to use this new set of values.

While here, add some clarification on the difference between the two methods.

Signed-off-by Derrick Stolee <[email protected]>

---
 Documentation/MyFirstObjectWalk.txt | 44 +++++++++++------------------
 1 file changed, 16 insertions(+), 28 deletions(-)

Documentation/MyFirstObjectWalk.txt +0 -0

index ca267941f3e..8ec83185b8a 100644
@@ -522,24 +522,25 @@ function shows that the all-object walk is being performed by
 `traverse_commit_list()` or `traverse_commit_list_filtered()`. Those two
 functions reside in `list-objects.c`; examining the source shows that, despite
 the name, these functions traverse all kinds of objects. Let's have a look at

Documentation/MyFirstObjectWalk.txt +6 -8

 `traverse_commit_list()` with the following, which will remind us which kind of
 walk we've just performed:
 

@@ -733,19 +723,17 @@ walk we've just performed:
 	if (0) {
 		/* Unfiltered: */
 		trace_printf(_("Unfiltered object walk.\n"));
-		traverse_commit_list(rev, walken_show_commit,
-				walken_show_object, NULL);
 	} else {
 		trace_printf(
 			_("Filtered object walk with filterspec 'tree:1'.\n"));
-		parse_list_objects_filter(&filter_options, "tree:1");
-
-		traverse_commit_list_filtered(&filter_options, rev,
-			walken_show_commit, walken_show_object, NULL, NULL);
+		CALLOC_ARRAY(rev->filter, 1);
+		parse_list_objects_filter(rev->filter, "tree:1");
 	}
+	traverse_commit_list(rev, walken_show_commit,
+			     walken_show_object, NULL);
 ----
 
-`struct list_objects_filter_options` is usually built directly from a command
+The `rev->filter` member is usually built directly from a command
 line argument, so the module provides an easy way to build one from a string.
 Even though we aren't taking user input right now, we can still build one with
 a hardcoded string using `parse_list_objects_filter()`.

@@ -784,7 +772,7 @@ object:
 ----
...
 
-		traverse_commit_list_filtered(&filter_options, rev,
+		traverse_commit_list_filtered(rev,
 			walken_show_commit, walken_show_object, NULL, &omitted);
 
...

From: Derrick Stolee <[email protected]>

Since 'git bundle' uses setup_revisions() to specify the object walk, some options do not make sense to include during the pack-objects child process. Further, these options are used for a call to traverse_commit_list() which would then require a callback which is currently NULL.

By populating the callback we prevent a segfault in the case of adding the --objects flag. This is really a redundant statement because the bundles are constructing a pack-file containing all objects in the discovered commit range.

Adding --objects to a 'git bundle' command might cause a slower command, but at least it will not have a hard failure when the user supplies this option. We can also disable walking trees and blobs in advance of this walk.

Quoted 10 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>
Since 'git bundle' uses setup_revisions() to specify the object walk,
some options do not make sense to include during the pack-objects child process. Further, these options are used for a call to traverse_commit_list() which would then require a callback which is currently NULL.
By populating the callback we prevent a segfault in the case of adding
the --objects flag. This is really a redundant statement because the

Nit: I stumbled over "...because the bundles are constructing..." Is there a better wording here??

From: Derrick Stolee <[email protected]>

A previous change allowed Git to parse bundles with the 'filter' capability. Now, teach Git to create bundles with this option.

Some rearranging of code is required to get the option parsing in the correct spot. There are now two reasons why we might need capabilities (a new hash algorithm or an object filter) so that is pulled out into a place where we can check both at the same time.

The --filter option is parsed as part of setup_revisions(), but it expected the --objects flag, too. That flag is somewhat implied by 'git bundle' because it creates a pack-file walking objects, but there is also a walk that walks the revision range expecting only commits. Make this parsing work by setting 'revs.tree_objects' and 'revs.blob_objects' before the call to setup_revisions().

Signed-off-by Derrick Stolee <[email protected]>

---

bundle.c +1 -1

@@ -507,10 +510,38 @@ int create_bundle(struct repository *r, const char *path,
 	int bundle_to_stdout;
 	int ref_count = 0;
 	struct rev_info revs, revs_copy;
-	int min_version = the_hash_algo == &hash_algos[GIT_HASH_SHA1] ? 2 : 3;
+	int min_version = 2;
 	struct bundle_prerequisites_info bpi;
 	int i;

t/t6020-bundle-misc.sh +16 -0

+	test_expect_success 'filtered bundle: $filter' '
+		test_when_finished rm -rf .git/objects/pack &&
+		git bundle create partial.bdl \
+			--all \
+			--filter=$filter &&
+
+		git bundle verify partial.bdl >unfiltered &&
+		make_user_friendly_and_stable_output <unfiltered >actual &&
+
+		cat >expect <<-EOF &&
+		The bundle contains these 10 refs:
+		<COMMIT-P> refs/heads/main
+		<COMMIT-N> refs/heads/release
+		<COMMIT-D> refs/heads/topic/1
+		<COMMIT-H> refs/heads/topic/2
+		<COMMIT-D> refs/pull/1/head

t/t6020-bundle-misc.sh +2 -0

+done
+
 test_done

From: Derrick Stolee <[email protected]>

In order to have a valid pack-file after unbundling a bundle that has the 'filter' capability, we need to generate a .promisor file. The bundle does not promise _where_ the objects can be found, but we can expect that these bundles will be unbundled in repositories with appropriate promisor remotes that can find those missing objects.

Use the 'git index-pack --promisor=<message>' option to create this .promisor file. Add "from-bundle" as the message to help anyone diagnose issues with these promisor packs.

Signed-off-by Derrick Stolee <[email protected]>

---
 bundle.c               | 4 ++++
 t/t6020-bundle-misc.sh | 8 +++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

bundle.c +4 -0

index e284ef63062..3d97de40ef0 100644
@@ -631,6 +631,10 @@ int unbundle(struct repository *r, struct bundle_header *header,
 	struct child_process ip = CHILD_PROCESS_INIT;
 	strvec_pushl(&ip.args, "index-pack", "--fix-thin", "--stdin", NULL);
 
+	/* If there is a filter, then we need to create the promisor pack. */
+	if (header->filter)
+		strvec_push(&ip.args, "--promisor=from-bundle");
+
 	if (extra_index_pack_args) {
 		strvec_pushv(&ip.args, extra_index_pack_args->v);
 		strvec_clear(extra_index_pack_args);

t/t6020-bundle-misc.sh +0 -0

index 39cfefafb65..344af34db1e 100755
@@ -513,7 +513,13 @@ do
 		The bundle uses this filter: $filter

Review

Quoted 3 lines · Derrick Stolee via GitGitGadget

static int try_bitmap_count(struct rev_info *revs,

(quoted diff) +0 -1

-			    struct list_objects_filter_options *filter,
 			    int filter_provided_objects)

This makes quite a lot of sense as filter is now available as revs->filter.

builtin/rev-list.c static int try_bitmap_count(struct rev_info *revs, · line 438

Quoted 9 lines

{

builtin/rev-list.c +2 -1

 	uint32_t commit_count = 0,

@@ -436,7 +434,8 @@ static int try_bitmap_count(struct rev_info *revs,
 	 */
 	max_count = revs->max_count;
 
-	bitmap_git = prepare_bitmap_walk(revs, filter, filter_provided_objects);
+	bitmap_git = prepare_bitmap_walk(revs, revs->filter,
+					 filter_provided_objects);

And we should be able to do the same to prepare_bitmap_walk(). It is OK if such a change comes later and not as part of this commit.

Perhaps it is deliberate. Unlike the helpers this step touches, namely, try_bitmap_count(), try_bitmap_traversal(), and try_bitmap_disk_usage(), prepare_bitmap_walk() is not a file-scope static helper and updating it will need touching many more places.

builtin/rev-list.c int cmd_rev_list(int argc, const char **argv, const char *prefix) · line 597

Quoted 4 lines

builtin/rev-list.c +0 -0

@@ -597,13 +595,17 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 		}
 
 		if (skip_prefix(arg, ("--" CL_ARG__FILTER "="), &arg)) {

#leftoverbit. We need to remember to clean this up, now "--filter" is well established (I am assuming this literal-string pasting is because we didn't know what the right and final word to be used as the option name back when this code was originally written), when the code around here is quiescent.

Quoted 7 lines

builtin/rev-list.c +4 -2

-			parse_list_objects_filter(&filter_options, arg);
-			if (filter_options.choice && !revs.blob_objects)
+			if (!revs.filter)
+				CALLOC_ARRAY(revs.filter, 1);
+			parse_list_objects_filter(revs.filter, arg);
+			if (revs.filter->choice && !revs.blob_objects)
 				die(_("object filtering requires --objects"));

Review int cmd_rev_list(int argc, const char **argv, const char *prefix) · line 603

Quoted 1 line

(quoted diff) +0 -0

 			continue;

OK. The original "filter_options" was a structure and not a pointer to a structure; now we have a pointer to a structure in revs as a member so we need an on-demand allocation. CALLOC_ARRAY() instead of xcalloc(), when we know we are creating one element and not an array of elements whose size happens to be one, is not wrong but it does look strange. Was there a reason why we avoid xcalloc()?

Makes me also wonder how big the filter_options structure is; because we will not use unbounded many revs structure, it may have been a simpler conversion to turn a static struct into an embedded struct member in a struct (instead of a member of a struct that is a pointer to a struct). That way, we did not have to ...

Quoted 1 line

(quoted diff) +0 -0

 >  		}

builtin/rev-list.c int cmd_rev_list(int argc, const char **argv, const char *prefix) · line 607

Quoted 4 lines

builtin/rev-list.c +2 -1

 		if (!strcmp(arg, ("--no-" CL_ARG__FILTER))) {
-			list_objects_filter_set_no_filter(&filter_options);
+			if (!revs.filter)
+				CALLOC_ARRAY(revs.filter, 1);

... repeat the on-demand allocation. If some code used to pass &filter_options in a parameter to helper functions, and such calling sites get rewritten to pass the value in the revs.filter pointer, and if revs hasn't gone through this codepath, these helper functions will start receiving NULL in their filter_options parameter, which they may or may not be prepared to take. This "we get rid of a global struct and replace it with an on-demand allocated structure, pointer to which is stored in the revs structure" rewrite somehow makes me nervous.

Documentation/MyFirstObjectWalk.txt function shows that the all-object walk is being performed by · line 529

Quoted 11 lines · Derrick Stolee via GitGitGadget

the name, these functions traverse all kinds of objects. Let's have a look at

Documentation/MyFirstObjectWalk.txt +4 -5

-the arguments to `traverse_commit_list_filtered()`, which are a superset of the
-arguments to the unfiltered version.
+the arguments to `traverse_commit_list()`.
 
-- `struct list_objects_filter_options *filter_options`: This is a struct which
-  stores a filter-spec as outlined in `Documentation/rev-list-options.txt`.
-- `struct rev_info *revs`: This is the `rev_info` used for the walk.
+- `struct rev_info *revs`: This is the `rev_info` used for the walk. It
+  includes a `filter` member which contains information for how to filter
+  the object list.

Perhaps,

"When its filter member is not NULL, it contains ..."

implying that it is valid for filter member to be NULL and none of the following things will happen in such a case.

Quoted 12 lines · Jeff Hostetler

Quoted 10 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>
Since 'git bundle' uses setup_revisions() to specify the object walk,
some options do not make sense to include during the pack-objects child process. Further, these options are used for a call to traverse_commit_list() which would then require a callback which is currently NULL.
By populating the callback we prevent a segfault in the case of adding
the --objects flag. This is really a redundant statement because the

Quoted 2 lines

Nit: I stumbled over "...because the bundles are constructing..." Is there a better wording here??

"... because the command is constructing ..." should be sufficient, I hope?

Quoted 17 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

A previous change allowed Git to parse bundles with the 'filter' capability. Now, teach Git to create bundles with this option.

Some rearranging of code is required to get the option parsing in the correct spot. There are now two reasons why we might need capabilities (a new hash algorithm or an object filter) so that is pulled out into a place where we can check both at the same time.

The --filter option is parsed as part of setup_revisions(), but it expected the --objects flag, too. That flag is somewhat implied by 'git bundle' because it creates a pack-file walking objects, but there is also a walk that walks the revision range expecting only commits. Make this parsing work by setting 'revs.tree_objects' and 'revs.blob_objects' before the call to setup_revisions().

Quoted 2 lines

Signed-off-by Derrick Stolee <[email protected]>

---

Now, the gem of the series ;-)

bundle.c int create_bundle(struct repository *r, const char *path, · line 537

Quoted 2 lines

bundle.c +2 -0

+	if (the_hash_algo != &hash_algos[GIT_HASH_SHA1] ||
+	    revs.filter)

Did we need to wrap? With these on a single line, the line is way shorter than the line with "because our hash algorithm is not" on it.

bundle.c int create_bundle(struct repository *r, const char *path, · line 605

Quoted 9 lines

bundle.c +6 -0

+	/*
+	 * Nullify the filter here, and any object walking. We only care
+	 * about commits and tags here. The revs_copy has the right
+	 * instances of these values.
+	 */
+	revs.filter = NULL;
 	revs.blob_objects = revs.tree_objects = 0;
 	traverse_commit_list(&revs, write_bundle_prerequisites, ignore_object, &bpi);
 	object_array_remove_duplicates(&revs_copy.pending);

OK. We prepare revs, and we save it to revs_copy, because we perform two traversals, one to determine which bottom commits are required to unbundle the bundle (which is done with the instance "revs"), and then later to actually enumerate the objects to place in the bundle (using "revs_copy"). Is there a reason why we need to remove .filter in order to perform the first traversal?

This is a tangent, but I wish we could reliably determine when we can optimize the first traversal away, by inspecting revs. If there are any pending objects with UNINTERESTING bit, or members like max_count, max_age, min_age are set, we'd end up traversing down to all roots and the prerequisites list would be empty.

Quoted 1 line

t/t6020-bundle-misc.sh +1 -0

+	test_expect_success 'filtered bundle: $filter' '

OK.

It is somewhat curious why our bundle tests do not unbundle and check the resulting contents of the repository we unbundle it in.

Quoted 7 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

In order to have a valid pack-file after unbundling a bundle that has the 'filter' capability, we need to generate a .promisor file. The bundle does not promise _where_ the objects can be found, but we can expect that these bundles will be unbundled in repositories with appropriate promisor remotes that can find those missing objects.

That sounds like a lot of wishful thinking, but I do not think of a better way to phrase the idea. Taking a bundle out of a repository and unbundling it elsewhere is "git fetch" that could be done to send objects from the former to the latter repository, so I am OK with the assumption that the original repository will stay available for such users who took its contents over sneaker-net instead of over the wire.

Quoted 31 lines

Use the 'git index-pack --promisor=<message>' option to create this .promisor file. Add "from-bundle" as the message to help anyone diagnose issues with these promisor packs.

Signed-off-by Derrick Stolee <[email protected]>

---
 bundle.c               | 4 ++++
 t/t6020-bundle-misc.sh | 8 +++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

bundle.c +4 -0

index e284ef63062..3d97de40ef0 100644
@@ -631,6 +631,10 @@ int unbundle(struct repository *r, struct bundle_header *header,
 	struct child_process ip = CHILD_PROCESS_INIT;
 	strvec_pushl(&ip.args, "index-pack", "--fix-thin", "--stdin", NULL);
 
+	/* If there is a filter, then we need to create the promisor pack. */
+	if (header->filter)
+		strvec_push(&ip.args, "--promisor=from-bundle");
+
 	if (extra_index_pack_args) {
 		strvec_pushv(&ip.args, extra_index_pack_args->v);
 		strvec_clear(extra_index_pack_args);

t/t6020-bundle-misc.sh +0 -0

index 39cfefafb65..344af34db1e 100755
@@ -513,7 +513,13 @@ do
 		The bundle uses this filter: $filter

t/t6020-bundle-misc.sh do · line 522

Quoted 10 lines

t/t6020-bundle-misc.sh +7 -1

 		The bundle records a complete history.
 		EOF
-		test_cmp expect actual
+		test_cmp expect actual &&
+
+		# This creates the first pack-file in the
+		# .git/objects/pack directory. Look for a .promisor.
+		git bundle unbundle partial.bdl &&
+		ls .git/objects/pack/pack-*.promisor >promisor &&
+		test_line_count = 1 promisor

OK. Do we also want to inspect the contents of the resulting repository to make sure that the bundle had the right contents?

One idea to do so would probably be

- prepare a test repository (you already have it)
- prepare a partial.bdl (you already do this)

- clone the test repository into a new repository, with the same filter
- create an empty repository, unbundle the partial.bdl

- take "for-each-ref" and list of objects available in these two "partial copies" from the test repository, and compare

 Documentation/MyFirstObjectWalk.txt | 44 ++++++---------
 Documentation/git-index-pack.txt    |  8 +++
 builtin/pack-objects.c              |  9 +--
 builtin/rev-list.c                  | 29 +++-------
 bundle.c                            | 87 ++++++++++++++++++++++++-----
 bundle.h                            |  3 +
 list-objects-filter-options.c       |  2 +-
 list-objects-filter-options.h       |  5 ++
 list-objects.c                      | 25 +++------
 list-objects.h                      | 12 +++-

 pack-bitmap.c                       | 24 ++++----
 pack-bitmap.h                       |  2 -
 reachable.c                         |  2 +-
 revision.c                          | 11 ++++
 revision.h                          |  4 ++
 t/t5300-pack-object.sh              |  4 +-
 t/t6020-bundle-misc.sh              | 48 ++++++++++++++++
 17 files changed, 215 insertions(+), 104 deletions(-)

On Wed, Feb 23 2022, Derrick Stolee via GitGitGadget wrote:

Just on:

Quoted 10 lines

 Documentation/MyFirstObjectWalk.txt | 44 ++++++---------
 Documentation/git-index-pack.txt    |  8 +++
 builtin/pack-objects.c              |  9 +--
 builtin/rev-list.c                  | 29 +++-------
 bundle.c                            | 87 ++++++++++++++++++++++++-----
 bundle.h                            |  3 +
 list-objects-filter-options.c       |  2 +-
 list-objects-filter-options.h       |  5 ++
 list-objects.c                      | 25 +++------
 list-objects.h                      | 12 +++-

Quoted 8 lines

 pack-bitmap.c                       | 24 ++++----
 pack-bitmap.h                       |  2 -
 reachable.c                         |  2 +-
 revision.c                          | 11 ++++
 revision.h                          |  4 ++
 t/t5300-pack-object.sh              |  4 +-
 t/t6020-bundle-misc.sh              | 48 ++++++++++++++++
 17 files changed, 215 insertions(+), 104 deletions(-)

This is missing a corresponding change to
Documentation/technical/bundle-format.txt.

From: Derrick Stolee <[email protected]>

Since 'git bundle' uses setup_revisions() to specify the object walk, some options do not make sense to include during the pack-objects child process. Further, these options are used for a call to traverse_commit_list() which would then require a callback which is currently NULL.

By populating the callback we prevent a segfault in the case of adding the --objects flag. This is really a redundant statement because the bundles are constructing a pack-file containing all objects in the discovered commit range.

Adding --objects to a 'git bundle' command might cause a slower command, but at least it will not have a hard failure when the user supplies this option. We can also disable walking trees and blobs in advance of this walk.

Signed-off-by Derrick Stolee <[email protected]>

---
 bundle.c               | 10 +++++++++-
 t/t6020-bundle-misc.sh | 12 ++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

bundle.c +6 -0

index a0bb687b0f4..dc56db9a50a 100644
@@ -451,6 +451,12 @@ struct bundle_prerequisites_info {
 	int fd;
 };
 
+
+static void ignore_object(struct object *obj, const char *v, void *data)
+{
+	/* Do nothing. */
+}
+
 static void write_bundle_prerequisites(struct commit *commit, void *data)
 {
 	struct bundle_prerequisites_info *bpi = data;

From: Derrick Stolee <[email protected]>

The v3 bundle format has capabilities, allowing newer versions of Git to create bundles with newer features. Older versions that do not understand these new capabilities will fail with a helpful warning.

Create a new capability allowing Git to understand that the contained pack-file is filtered according to some object filter. Typically, this filter will be "blob:none" for a blobless partial clone.

This change teaches Git to parse this capability, place its value in the bundle header, and demonstrate this understanding by adding a message to 'git bundle verify'.

Signed-off-by Derrick Stolee <[email protected]>

---
 bundle.c                      | 17 ++++++++++++++++-
 bundle.h                      |  3 +++
 list-objects-filter-options.c |  2 +-
 list-objects-filter-options.h |  5 +++++
 4 files changed, 25 insertions(+), 2 deletions(-)

bundle.c +16 -1

index dc56db9a50a..2afced4d991 100644
@@ -11,7 +11,7 @@
 #include "run-command.h"
 #include "refs.h"
 #include "strvec.h"
-
+#include "list-objects-filter-options.h"
 
 static const char v2_bundle_signature[] = "# v2 git bundle\n";
 static const char v3_bundle_signature[] = "# v3 git bundle\n";

@@ -33,6 +33,8 @@ void bundle_header_release(struct bundle_header *header)
 {
 	string_list_clear(&header->prerequisites, 1);
 	string_list_clear(&header->references, 1);
+	list_objects_filter_release(header->filter);
+	free(header->filter);
 }
 
 static int parse_capability(struct bundle_header *header, const char *capability)

@@ -45,6 +47,11 @@ static int parse_capability(struct bundle_header *header, const char *capability
 		header->hash_algo = &hash_algos[algo];
 		return 0;
 	}
+	if (skip_prefix(capability, "filter=", &arg)) {
+		CALLOC_ARRAY(header->filter, 1);
+		parse_list_objects_filter(header->filter, arg);
+		return 0;
+	}
 	return error(_("unknown capability '%s'"), capability);
 }
 

@@ -220,6 +227,8 @@ int verify_bundle(struct repository *r,
 	req_nr = revs.pending.nr;
 	setup_revisions(2, argv, &revs, NULL);
 
+	revs.filter = header->filter;
+
 	if (prepare_revision_walk(&revs))
 		die(_("revision walk setup failed"));
 

@@ -259,6 +268,12 @@ int verify_bundle(struct repository *r,
 			     r->nr),
 			  r->nr);
 		list_refs(r, 0, NULL);
+
+		if (header->filter) {
+			printf_ln("The bundle uses this filter: %s",
+				  list_objects_filter_spec(header->filter));
+		}
+
 		r = &header->prerequisites;
 		if (!r->nr) {
 			printf_ln(_("The bundle records a complete history."));

bundle.h +0 -0

index 06009fe6b1f..eb026153d56 100644

list-objects-filter-options.h +5 -0

+int gently_parse_list_objects_filter(
+	struct list_objects_filter_options *filter_options,
+	const char *arg,
+	struct strbuf *errbuf);
+

From: Derrick Stolee <[email protected]>

t/t6020-bundle-misc.sh +18 -0

+		test_when_finished rm -rf .git/objects/pack &&
+		git bundle create partial.bdl \
+			--all \
+			--filter=$filter &&
+
+		git bundle verify partial.bdl >unfiltered &&
+		make_user_friendly_and_stable_output <unfiltered >actual &&
+
+		cat >expect <<-EOF &&
+		The bundle contains these 10 refs:
+		<COMMIT-P> refs/heads/main
+		<COMMIT-N> refs/heads/release
+		<COMMIT-D> refs/heads/topic/1
+		<COMMIT-H> refs/heads/topic/2
+		<COMMIT-D> refs/pull/1/head
+		<COMMIT-G> refs/pull/2/head
+		<TAG-1> refs/tags/v1
+		<TAG-2> refs/tags/v2

From: Derrick Stolee <[email protected]>

In order to have a valid pack-file after unbundling a bundle that has the 'filter' capability, we need to generate a .promisor file. The bundle does not promise _where_ the objects can be found, but we can expect that these bundles will be unbundled in repositories with appropriate promisor remotes that can find those missing objects.

Use the 'git index-pack --promisor=<message>' option to create this .promisor file. Add "from-bundle" as the message to help anyone diagnose issues with these promisor packs.

Signed-off-by Derrick Stolee <[email protected]>

---
 bundle.c               | 4 ++++
 t/t6020-bundle-misc.sh | 8 +++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

bundle.c +4 -0

index e284ef63062..3d97de40ef0 100644
@@ -631,6 +631,10 @@ int unbundle(struct repository *r, struct bundle_header *header,
 	struct child_process ip = CHILD_PROCESS_INIT;
 	strvec_pushl(&ip.args, "index-pack", "--fix-thin", "--stdin", NULL);
 
+	/* If there is a filter, then we need to create the promisor pack. */
+	if (header->filter)
+		strvec_push(&ip.args, "--promisor=from-bundle");
+
 	if (extra_index_pack_args) {
 		strvec_pushv(&ip.args, extra_index_pack_args->v);
 		strvec_clear(extra_index_pack_args);

t/t6020-bundle-misc.sh +0 -0

index 39cfefafb65..344af34db1e 100755
@@ -513,7 +513,13 @@ do
 		The bundle uses this filter: $filter
 		The bundle records a complete history.
 		EOF

From: Derrick Stolee <[email protected]>
[...]
static int parse_capability(struct bundle_header *header, const char *capability)

Quoted 47 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

In order to have a valid pack-file after unbundling a bundle that has the 'filter' capability, we need to generate a .promisor file. The bundle does not promise _where_ the objects can be found, but we can expect that these bundles will be unbundled in repositories with appropriate promisor remotes that can find those missing objects.

Use the 'git index-pack --promisor=<message>' option to create this .promisor file. Add "from-bundle" as the message to help anyone diagnose issues with these promisor packs.

Signed-off-by Derrick Stolee <[email protected]>

---
 bundle.c               | 4 ++++
 t/t6020-bundle-misc.sh | 8 +++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

bundle.c +4 -0

index e284ef63062..3d97de40ef0 100644
@@ -631,6 +631,10 @@ int unbundle(struct repository *r, struct bundle_header *header,
 	struct child_process ip = CHILD_PROCESS_INIT;
 	strvec_pushl(&ip.args, "index-pack", "--fix-thin", "--stdin", NULL);
 
+	/* If there is a filter, then we need to create the promisor pack. */
+	if (header->filter)
+		strvec_push(&ip.args, "--promisor=from-bundle");
+
 	if (extra_index_pack_args) {
 		strvec_pushv(&ip.args, extra_index_pack_args->v);
 		strvec_clear(extra_index_pack_args);

t/t6020-bundle-misc.sh +5 -1

index 39cfefafb65..344af34db1e 100755
@@ -513,7 +513,13 @@ do
 		The bundle uses this filter: $filter
 		The bundle records a complete history.
 		EOF
-		test_cmp expect actual
+		test_cmp expect actual &&
+
+		# This creates the first pack-file in the
+		# .git/objects/pack directory. Look for a .promisor.
+		git bundle unbundle partial.bdl &&

t/t6020-bundle-misc.sh do · line 524

Quoted 4 lines

t/t6020-bundle-misc.sh +2 -0

+		ls .git/objects/pack/pack-*.promisor >promisor &&
+		test_line_count = 1 promisor
 	'
 done

Aside from what Junio mentioned, the preceding commit seems to be incomplete here. I.e. I'd expect to see this replace a case where we died or whatever before. What happened if we invoked "unbundle" before?

Quoted 1 line · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

Quoted 8 lines

bundle.h +0 -0

index 06009fe6b1f..eb026153d56 100644
@@ -5,11 +5,14 @@
 #include "cache.h"
 #include "string-list.h"
 

bundle.h line 9

Quoted 2 lines

bundle.h +2 -0

+struct list_objects_filter_options;
+

For the other ones we include the relevant header, do the same here (or if there's a need to not do it, do we need it for the rest too?)

bundle.h line 16

Quoted 7 lines

bundle.h +1 -0

 struct bundle_header {
 	unsigned version;
 	struct string_list prerequisites;
 	struct string_list references;
 	const struct git_hash_algo *hash_algo;
+	struct list_objects_filter_options *filter;
 };

I haven't tried, but any reason this needs to be a *filter v.s. embedding it in the struct?

Then we'd just need list_objects_filter_release() and not the free() as well.

Is it because you're piggy-backing on "if (header->filter)" as "do we have it" state, better to check .nr?

bundle.h const char *list_object_filter_config_name(enum list_objects_filter_choice c) · line 57

Quoted 4 lines

bundle.h +0 -0

@@ -55,7 +55,7 @@ const char *list_object_filter_config_name(enum list_objects_filter_choice c)
  * expand_list_objects_filter_spec() first).  We also "intern" the arg for the
  * convenience of the current command.
  */

These API docs....

list-objects-filter-options.h struct list_objects_filter_options { · line 73

Quoted 12 lines

bundle.h +1 -1

-static int gently_parse_list_objects_filter(
+int gently_parse_list_objects_filter(
 	struct list_objects_filter_options *filter_options,
 	const char *arg,
 	struct strbuf *errbuf)

list-objects-filter-options.h +0 -0

index da5b6737e27..347a99c28cf 100644
@@ -72,6 +72,11 @@ struct list_objects_filter_options {
 /* Normalized command line arguments */
 #define CL_ARG__FILTER "filter"

...should be moved here, presumably.

Quoted 4 lines · Ævar Arnfjörð Bjarmason

Quoted 1 line · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

...

Quoted 8 lines

Quoted 8 lines

bundle.h +0 -0

index 06009fe6b1f..eb026153d56 100644
@@ -5,11 +5,14 @@
 #include "cache.h"
 #include "string-list.h"
 

Quoted 5 lines

bundle.h line 9

Quoted 2 lines

bundle.h +2 -0

+struct list_objects_filter_options;
+

For the other ones we include the relevant header, do the same here (or if there's a need to not do it, do we need it for the rest too?)

The others are .c files that require looking into the struct. This declaration is all that's required for this header file.

Quoted 14 lines

bundle.h line 16

Quoted 7 lines

bundle.h +1 -0

 struct bundle_header {
 	unsigned version;
 	struct string_list prerequisites;
 	struct string_list references;
 	const struct git_hash_algo *hash_algo;
+	struct list_objects_filter_options *filter;
 };

I haven't tried, but any reason this needs to be a *filter v.s. embedding it in the struct?

Then we'd just need list_objects_filter_release() and not the free() as well.

Quoted 2 lines

Is it because you're piggy-backing on "if (header->filter)" as "do we have it" state, better to check .nr?

Yes. I replied to Junio before that there is some assumption in the filtering code that the .nr == 0 case is listed as a BUG() so we would possibly be breaking expectations in a different way doing the embedded version.

Quoted 37 lines · Derrick Stolee

Quoted 4 lines · Ævar Arnfjörð Bjarmason

Quoted 1 line · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

...

Quoted 8 lines

Quoted 8 lines

bundle.h +0 -0

index 06009fe6b1f..eb026153d56 100644
@@ -5,11 +5,14 @@
 #include "cache.h"
 #include "string-list.h"
 

Quoted 5 lines

bundle.h line 9

Quoted 2 lines

bundle.h +2 -0

+struct list_objects_filter_options;
+

For the other ones we include the relevant header, do the same here (or if there's a need to not do it, do we need it for the rest too?)

The others are .c files that require looking into the struct. This declaration is all that's required for this header file.

Quoted 14 lines

bundle.h line 16

Quoted 7 lines

bundle.h +1 -0

 struct bundle_header {
 	unsigned version;
 	struct string_list prerequisites;
 	struct string_list references;
 	const struct git_hash_algo *hash_algo;
+	struct list_objects_filter_options *filter;
 };

I haven't tried, but any reason this needs to be a *filter v.s. embedding it in the struct?

Then we'd just need list_objects_filter_release() and not the free() as well.

Quoted 7 lines

Quoted 2 lines

Is it because you're piggy-backing on "if (header->filter)" as "do we have it" state, better to check .nr?

Yes. I replied to Junio before that there is some assumption in the filtering code that the .nr == 0 case is listed as a BUG() so we would possibly be breaking expectations in a different way doing the embedded version.

Having an "unsigned int using_filter:1" or whatever IMO makes that much clearer than needing to carefully eyeball code that's already using embedded structs & see why the one exception that's malloc'd is because of that or some other reason...

Quoted 4 lines

member so we need an on-demand allocation. CALLOC_ARRAY() instead of xcalloc(), when we know we are creating one element and not an array of elements whose size happens to be one, is not wrong but it does look strange. Was there a reason why we avoid xcalloc()?

I think I've been using CALLOC_ARRAY(..., 1) over "... = xcalloc()" for simplicity's sake for a while. I see quite a few across the codebase, too, but I can swap the usage here if you feel that is important.

Quoted 4 lines

they may or may not be prepared to take. This "we get rid of a global struct and replace it with an on-demand allocated structure, pointer to which is stored in the revs structure" rewrite somehow makes me nervous.

I think the main idea is that the filter being NULL indicates "no filter is used. Do a full object walk." If we use a static struct, then we need to instead use revs->filter.filter_spec.nr, but that is already being used as a BUG() statement:

While we _could_ make that switch to using a static struct and change our checks to allow empty specs, that would be a more involved change. Maybe we can leave it for a follow up?

Definitely room for improvement here. I got hung up on the two uses of "it" so here is another attempt:

If its filter member is not NULL, then filter contains information for how to filter the object list.

What do you think?

Perhaps I was thinking about having one line per "reason", which might be extended in the future.

Of course, looking closer at it... "git bundle unbundle" doesn't actually store the refs directly in the refspace, but instead only outputs the refs that it used.

'
done

--- >8 ---

I also attempted doing a "git clone --bare partial.bdl unbundled.git" to get the 'git clone' command to actually place the refs. However, 'git clone' does not set up the repository filter based on the bundle, so it reports missing blobs (even though there is no checkout).

Quoted 3 lines · Derrick Stolee

Of course, looking closer at it... "git bundle unbundle" doesn't actually store the refs directly in the refspace, but instead only outputs the refs that it used.

True. I was more thinking about equivalence between

    cd src_repo
    git clone --no-local --filter=... . ../partial.network.cloned
    git bundle create --filter=... partial.bndl
    git clone partial.bndl ../partial.bundle.cloned

The two resulting repositories should look very similar except for that the remote.origin.* of the former would expect that it pushes back to where it was cloned from, while the latter would not.

t/t6020-bundle-misc.sh do · line 525

Quoted 8 lines

t/t6020-bundle-misc.sh +8 -0

+		git init unbundled &&
+		(
+			cd unbundled &&
+			# This creates the first pack-file in the
+			# .git/objects/pack directory. Look for a .promisor.
+			git bundle unbundle ../partial.bdl >ref-list.txt &&
+			ls .git/objects/pack/pack-*.promisor >promisor &&
+			test_line_count = 1 promisor

And can we enumerate the objects we have in .git/objects, both loose and packed?

t/t6020-bundle-misc.sh do · line 532

Quoted 7 lines

t/t6020-bundle-misc.sh +7 -0

+		) &&
+
+		git clone --filter=blob:none --mirror "file://$(pwd)" cloned &&
+		git -C cloned for-each-ref \
+			--format="%(objectname) %(refname)" >cloned-refs.txt &&
+		echo "$(git -C cloned rev-parse HEAD) HEAD" >>cloned-refs.txt &&
+		test_cmp cloned-refs.txt unbundled/ref-list.txt

Likewise here? I think the two should match, and that was what I was wondering if we should enforce.

Quoted 9 lines

'
done

--- >8 ---

I also attempted doing a "git clone --bare partial.bdl unbundled.git" to get the 'git clone' command to actually place the refs. However, 'git clone' does not set up the repository filter based on the bundle, so it reports missing blobs (even though there is no checkout).

Understandable, as cloning from a bundle, if I recall correctly, was done as yet another special case in "git clone", differently from the main "over the network" code path. And from end-user's point of view, I think updating it is part of introducing the filtered bundle.

I also had some struggles getting this to work since local clones were actually ignoring the filter. I didn't think it was worth setting up an HTTP or SSH server just for this test.

Quoted 10 lines

Quoted 4 lines

I also attempted doing a "git clone --bare partial.bdl unbundled.git" to get the 'git clone' command to actually place the refs. However, 'git clone' does not set up the repository filter based on the bundle, so it reports missing blobs (even though there is no checkout).

Understandable, as cloning from a bundle, if I recall correctly, was done as yet another special case in "git clone", differently from the main "over the network" code path. And from end-user's point of view, I think updating it is part of introducing the filtered bundle.

The reason I did not include that here is because of the lack of repository-global promisor/filter config. I do want to loop back and make those updates, but perhaps for this series we should add an error condition into 'git clone' to say "Cannot currently clone from a filtered bundle" to help users understand the issue?

Quoted 3 lines · Derrick Stolee

I also had some struggles getting this to work since local clones were actually ignoring the filter. I didn't think it was worth setting up an HTTP or SSH server just for this test.

Does "clone --no-local $path" work as a workaround? It should do the same thing as "ssh" codepath except that it uses "sh" instead as the process on the other side is running locally.

Quoted 11 lines

Quoted 10 lines

Quoted 4 lines

I also attempted doing a "git clone --bare partial.bdl unbundled.git" to get the 'git clone' command to actually place the refs. However, 'git clone' does not set up the repository filter based on the bundle, so it reports missing blobs (even though there is no checkout).

Understandable, as cloning from a bundle, if I recall correctly, was done as yet another special case in "git clone", differently from the main "over the network" code path. And from end-user's point of view, I think updating it is part of introducing the filtered bundle.

Quoted 5 lines

The reason I did not include that here is because of the lack of repository-global promisor/filter config. I do want to loop back and make those updates, but perhaps for this series we should add an error condition into 'git clone' to say "Cannot currently clone from a filtered bundle" to help users understand the issue?

It would be a workable stepwise solution, I would think. It is not like we are robbing an existing feature from users---it merely is that the support of partial cloning over different "transport" is uneven, which is to be expected, especially in earlier phase of introducing a new feature.

I've been trying this:

git clone --no-local --filter=$filter "file://$(pwd)" cloned &&

which "succeeds" with this in the stderr:

warning: filtering not recognized by server, ignoring

Quoted 8 lines

Quoted 2 lines

an error condition into 'git clone' to say "Cannot currently clone from a filtered bundle" to help users understand the issue?

It would be a workable stepwise solution, I would think. It is not like we are robbing an existing feature from users---it merely is that the support of partial cloning over different "transport" is uneven, which is to be expected, especially in earlier phase of introducing a new feature.

That was my understanding, too.

Quoted 2 lines · Derrick Stolee

I've been trying this:

Quoted 5 lines

git clone --no-local --filter=$filter "file://$(pwd)" cloned &&

which "succeeds" with this in the stderr:

warning: filtering not recognized by server, ignoring

Hmph, and we won't see it when going over ssh to the same repository? That is puzzling.

Quoted 2 lines

Hmph, and we won't see it when going over ssh to the same repository? That is puzzling.

Of course, it's not an issue with file://, but an issue with the defaults. In order to test partial clones, I need to enable these config options in the server repo:

		test_config uploadpack.allowfilter 1 &&
		test_config uploadpack.allowanysha1inwant 1 &&

Sorry for taking so long to realize this.