diff mbox series

[v3,12/12] clone: fail gracefully when cloning filtered bundle

Message ID 805e1d1172210c6a39b33edcb2cd6d21b754f821.1646750359.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Partial bundles | expand

Commit Message

Derrick Stolee March 8, 2022, 2:39 p.m. UTC 
From: Derrick Stolee <derrickstolee@github.com>

Users can create a new repository using 'git clone <bundle-file>'. The
new "@filter" capability for bundles means that we can generate a bundle
that does not contain all reachable objects, even if the header has no
negative commit OIDs.

It is feasible to think that we could make a filtered bundle work with
the command

  git clone --filter=$filter --bare <bundle-file>

or possibly replacing --bare with --no-checkout. However, this requires
having some repository-global config that specifies the specified object
filter and notifies Git about the existence of promisor pack-files.
Without a remote, that is currently impossible.

As a stop-gap, parse the bundle header during 'git clone' and die() with
a helpful error message instead of the current behavior of failing due
to "missing objects".

Most of the existing logic for handling bundle clones actually happens
in fetch-pack.c, but that logic is the same as if the user specified
'git fetch <bundle>', so we want to avoid failing to fetch a filtered
bundle when in an existing repository that has the proper config set up
for at least one remote.

Carefully comment around the test that this is not the desired long-term
behavior of 'git clone' in this case, but instead that we need to do
more work before that is possible.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 builtin/clone.c        | 13 +++++++++++++
 t/t6020-bundle-misc.sh | 12 ++++++++++++
 2 files changed, 25 insertions(+)

Comments

Derrick Stolee March 8, 2022, 4:10 p.m. UTC | #1 
On 3/8/2022 9:39 AM, Derrick Stolee via GitGitGadget wrote:

> +	if (is_bundle) {
> +		struct bundle_header header = { 0 };
> +		int fd = read_bundle_header(path, &header);
> +		int has_filter = header.filter.choice != LOFC_DISABLED;

Of course, as I was sending an email replying to What's Cooking, I
realized that I missed one of the suggestions, which is fixed with
this diff:

--- >8 ---

diff --git a/builtin/clone.c b/builtin/clone.c
index 623a5040b1..e57504c2aa 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1140,7 +1140,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	transport->cloning = 1;
 
 	if (is_bundle) {
-		struct bundle_header header = { 0 };
+		struct bundle_header header = BUNDLE_HEADER_INIT;
 		int fd = read_bundle_header(path, &header);
 		int has_filter = header.filter.choice != LOFC_DISABLED;
Junio C Hamano March 8, 2022, 5:19 p.m. UTC | #2 
Derrick Stolee <derrickstolee@github.com> writes:

> On 3/8/2022 9:39 AM, Derrick Stolee via GitGitGadget wrote:
>
>> +	if (is_bundle) {
>> +		struct bundle_header header = { 0 };
>> +		int fd = read_bundle_header(path, &header);
>> +		int has_filter = header.filter.choice != LOFC_DISABLED;
>
> Of course, as I was sending an email replying to What's Cooking, I
> realized that I missed one of the suggestions, which is fixed with
> this diff:
>
> --- >8 ---
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 623a5040b1..e57504c2aa 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1140,7 +1140,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	transport->cloning = 1;
>  
>  	if (is_bundle) {
> -		struct bundle_header header = { 0 };
> +		struct bundle_header header = BUNDLE_HEADER_INIT;
>  		int fd = read_bundle_header(path, &header);
>  		int has_filter = header.filter.choice != LOFC_DISABLED;

Let me squash it into 12/12, then.

Thanks.
diff mbox series

Patch

diff --git a/builtin/clone.c b/builtin/clone.c
index 9c29093b352..623a5040b1c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -33,6 +33,7 @@ 
 #include "packfile.h"
 #include "list-objects-filter-options.h"
 #include "hook.h"
+#include "bundle.h"
 
 /*
  * Overall FIXMEs:
@@ -1138,6 +1139,18 @@  int cmd_clone(int argc, const char **argv, const char *prefix)
 		warning(_("--local is ignored"));
 	transport->cloning = 1;
 
+	if (is_bundle) {
+		struct bundle_header header = { 0 };
+		int fd = read_bundle_header(path, &header);
+		int has_filter = header.filter.choice != LOFC_DISABLED;
+
+		if (fd > 0)
+			close(fd);
+		bundle_header_release(&header);
+		if (has_filter)
+			die(_("cannot clone from filtered bundle"));
+	}
+
 	transport_set_option(transport, TRANS_OPT_KEEP, "yes");
 
 	if (reject_shallow)
diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
index 42e8cf2eb29..5160cb0a75c 100755
--- a/t/t6020-bundle-misc.sh
+++ b/t/t6020-bundle-misc.sh
@@ -537,4 +537,16 @@  do
 	'
 done
 
+# NEEDSWORK: 'git clone --bare' should be able to clone from a filtered
+# bundle, but that requires a change to promisor/filter config options.
+# For now, we fail gracefully with a helpful error. This behavior can be
+# changed in the future to succeed as much as possible.
+test_expect_success 'cloning from filtered bundle has useful error' '
+	git bundle create partial.bdl \
+		--all \
+		--filter=blob:none &&
+	test_must_fail git clone --bare partial.bdl partial 2>err &&
+	grep "cannot clone from filtered bundle" err
+'
+
 test_done