[v3,10/11] bundle-uri: download bundles from an advertised list

Commit Message

Derrick Stolee Dec. 5, 2022, 5:50 p.m. UTC

From: Derrick Stolee <[e-mail]>

The logic in fetch_bundle_uri() is useful for the --bundle-uri option of
'git clone', but is not helpful when the clone operation discovers a
list of URIs from the bundle-uri protocol v2 command. To actually
download and unbundle the advertised bundles, we need a different
mechanism.

Create the new fetch_bundle_list() method which is very similar to
fetch_bundle_uri() except that it relies on download_bundle_list()
instead of fetch_bundle_uri_internal(). The download_bundle_list()
method will recursively call fetch_bundle_uri_internal() if any of the
advertised URIs serve a bundle list instead of a bundle. This will also
follow the bundle.list.mode setting from the input list: "any" will
download only one such URI while "all" will download data from all of
the URIs.

In an identical way to fetch_bundle_uri(), the bundles are unbundled
after all of the bundle lists have been expanded and all necessary URIs.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 bundle-uri.c | 21 +++++++++++++++++++++
 bundle-uri.h | 11 +++++++++++
 2 files changed, 32 insertions(+)

Comments

Jeff King Dec. 7, 2022, 12:57 p.m. UTC | #1

On Mon, Dec 05, 2022 at 05:50:38PM +0000, Derrick Stolee via GitGitGadget wrote:

> +int fetch_bundle_list(struct repository *r, const char *uri, struct bundle_list *list)
> +{
> +	int result;
> +	struct bundle_list global_list;
> +
> +	init_bundle_list(&global_list);
> +
> +	/* If a bundle is added to this global list, then it is required. */
> +	global_list.mode = BUNDLE_MODE_ALL;
> +
> +	if ((result = download_bundle_list(r, list, &global_list, 0)))
> +		goto cleanup;
> +
> +	result = unbundle_all_bundles(r, &global_list);
> +
> +cleanup:
> +	for_all_bundles_in_list(&global_list, unlink_bundle, NULL);
> +	clear_bundle_list(&global_list);
> +	return result;
> +}

The "uri" parameter in this function is unused. I'm not sure if that's
indicative of a bug or missing feature (e.g., could it be the base for a
relative url?), or if it's just a leftover from development.

If the latter, I'm happy to add it to my list of cleanups.

There are a couple other unused parameters in this series, too, but they
are all in virtual functions and must be kept. I'll add them to my list
of annotations.

-Peff

Derrick Stolee Dec. 7, 2022, 3:27 p.m. UTC | #2

On 12/7/2022 7:57 AM, Jeff King wrote:
> On Mon, Dec 05, 2022 at 05:50:38PM +0000, Derrick Stolee via GitGitGadget wrote:
> 
>> +int fetch_bundle_list(struct repository *r, const char *uri, struct bundle_list *list)
>> +{
>> +	int result;
>> +	struct bundle_list global_list;
>> +
>> +	init_bundle_list(&global_list);
>> +
>> +	/* If a bundle is added to this global list, then it is required. */
>> +	global_list.mode = BUNDLE_MODE_ALL;
>> +
>> +	if ((result = download_bundle_list(r, list, &global_list, 0)))
>> +		goto cleanup;
>> +
>> +	result = unbundle_all_bundles(r, &global_list);
>> +
>> +cleanup:
>> +	for_all_bundles_in_list(&global_list, unlink_bundle, NULL);
>> +	clear_bundle_list(&global_list);
>> +	return result;
>> +}
> 
> The "uri" parameter in this function is unused. I'm not sure if that's
> indicative of a bug or missing feature (e.g., could it be the base for a
> relative url?), or if it's just a leftover from development.

Thanks for your careful eye. This 'uri' is indeed not needed. I think it
was initially there for relative URIs, but the given 'list' is expected
to have that value initialized. I'll make it clear in the doc comment.
 
> If the latter, I'm happy to add it to my list of cleanups.
> 
> There are a couple other unused parameters in this series, too, but they
> are all in virtual functions and must be kept. I'll add them to my list
> of annotations.

Your UNUSED annotations exist in my tree, so I'll try my best to update
them in the next version.

Thanks,
-Stolee

Derrick Stolee Dec. 7, 2022, 3:54 p.m. UTC | #3

On 12/7/2022 10:27 AM, Derrick Stolee wrote:
> On 12/7/2022 7:57 AM, Jeff King wrote:

>> There are a couple other unused parameters in this series, too, but they
>> are all in virtual functions and must be kept. I'll add them to my list
>> of annotations.
> 
> Your UNUSED annotations exist in my tree, so I'll try my best to update
> them in the next version.

One of these showed an example where I should have been using
repo_config_...() instead of git_config_...(). Thanks!

-Stolee

Jeff King Dec. 8, 2022, 6:36 a.m. UTC | #4

On Wed, Dec 07, 2022 at 10:27:06AM -0500, Derrick Stolee wrote:

> > The "uri" parameter in this function is unused. I'm not sure if that's
> > indicative of a bug or missing feature (e.g., could it be the base for a
> > relative url?), or if it's just a leftover from development.
> 
> Thanks for your careful eye. This 'uri' is indeed not needed. I think it
> was initially there for relative URIs, but the given 'list' is expected
> to have that value initialized. I'll make it clear in the doc comment.

That makes sense. I've queued a patch locally to remove it (since
locally I build with -Wunused-parameters), which will eventually make
its way to the list.

> > If the latter, I'm happy to add it to my list of cleanups.
> > 
> > There are a couple other unused parameters in this series, too, but they
> > are all in virtual functions and must be kept. I'll add them to my list
> > of annotations.
> 
> Your UNUSED annotations exist in my tree, so I'll try my best to update
> them in the next version.

Sounds good (and again, I've queued something locally, but if you beat
me to it, it's easy to drop mine).

Note that your series hit 'next' (which is how I noticed it), so there
usually would not be a "next version". Though we will rewind
post-release, so there may still be an opportunity (I didn't follow the
topic closely enough to know if you might want to re-roll for other
reasons).

-Peff

Jeff King Dec. 8, 2022, 6:40 a.m. UTC | #5

On Wed, Dec 07, 2022 at 10:54:31AM -0500, Derrick Stolee wrote:

> On 12/7/2022 10:27 AM, Derrick Stolee wrote:
> > On 12/7/2022 7:57 AM, Jeff King wrote:
> 
> >> There are a couple other unused parameters in this series, too, but they
> >> are all in virtual functions and must be kept. I'll add them to my list
> >> of annotations.
> > 
> > Your UNUSED annotations exist in my tree, so I'll try my best to update
> > them in the next version.
> 
> One of these showed an example where I should have been using
> repo_config_...() instead of git_config_...(). Thanks!

Oh good. It is always satisfying when the warning finds a potential bug,
as it sometimes feels a bit like annotation make-work. ;)

In this instance we're in server-side v2 protocol code, which is already
very global-heavy in its world-view. So I don't think it's a real bug
here, but just a nice-to-have.

I have seen this "oops, we don't really use our repository parameter"
issue in a few places. And while I do think it's best to use it if you
have it, I suspect it's the tip of the iceberg in terms of functions
using the_repository. In the long run, I think we'll really smoke those
out from the bottom up, as more functions insist on taking a repository
parameter (and then their callers will have to switch or face the
embarrassment of passing the_repository themselves, and so on).

All of which is to say that yes, that is a fine change to make. But I
don't consider at all urgent in this instance.

-Peff

Derrick Stolee Dec. 8, 2022, 2:58 p.m. UTC | #6

On 12/8/2022 1:36 AM, Jeff King wrote:
> On Wed, Dec 07, 2022 at 10:27:06AM -0500, Derrick Stolee wrote:
> 
>>> The "uri" parameter in this function is unused. I'm not sure if that's
>>> indicative of a bug or missing feature (e.g., could it be the base for a
>>> relative url?), or if it's just a leftover from development.
>>
>> Thanks for your careful eye. This 'uri' is indeed not needed. I think it
>> was initially there for relative URIs, but the given 'list' is expected
>> to have that value initialized. I'll make it clear in the doc comment.
> 
> That makes sense. I've queued a patch locally to remove it (since
> locally I build with -Wunused-parameters), which will eventually make
> its way to the list.
> 
>>> If the latter, I'm happy to add it to my list of cleanups.
>>>
>>> There are a couple other unused parameters in this series, too, but they
>>> are all in virtual functions and must be kept. I'll add them to my list
>>> of annotations.
>>
>> Your UNUSED annotations exist in my tree, so I'll try my best to update
>> them in the next version.
> 
> Sounds good (and again, I've queued something locally, but if you beat
> me to it, it's easy to drop mine).
> 
> Note that your series hit 'next' (which is how I noticed it), so there
> usually would not be a "next version". Though we will rewind
> post-release, so there may still be an opportunity (I didn't follow the
> topic closely enough to know if you might want to re-roll for other
> reasons).

I noticed that after reading this round of review, so I'll be preparing
some fixes on top. I noticed some UNUSED that would be necessary from
earlier parts of the bundle URI work, and you've probably already queued
those changes.

Since I no longer plan to re-roll this series, I'd be happy to review
your queued annotations, and I'll focus on the other fixups.

Thanks,
-Stolee

Patch

diff --git a/bundle-uri.c b/bundle-uri.c
index 80370992773..c411b871bdd 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -577,6 +577,27 @@  cleanup:
 	return result;
 }
 
+int fetch_bundle_list(struct repository *r, const char *uri, struct bundle_list *list)
+{
+	int result;
+	struct bundle_list global_list;
+
+	init_bundle_list(&global_list);
+
+	/* If a bundle is added to this global list, then it is required. */
+	global_list.mode = BUNDLE_MODE_ALL;
+
+	if ((result = download_bundle_list(r, list, &global_list, 0)))
+		goto cleanup;
+
+	result = unbundle_all_bundles(r, &global_list);
+
+cleanup:
+	for_all_bundles_in_list(&global_list, unlink_bundle, NULL);
+	clear_bundle_list(&global_list);
+	return result;
+}
+
 /**
  * API for serve.c.
  */
diff --git a/bundle-uri.h b/bundle-uri.h
index e7e90a5f088..b2c9c160a52 100644
--- a/bundle-uri.h
+++ b/bundle-uri.h
@@ -107,6 +107,17 @@  int bundle_uri_parse_config_format(const char *uri,
  */
 int fetch_bundle_uri(struct repository *r, const char *uri);
 
+/**
+ * Given a bundle list that was already advertised (likely by the
+ * bundle-uri protocol v2 verb) at the given uri, fetch and unbundle the
+ * bundles according to the bundle strategy of that list.
+ *
+ * Returns non-zero if no bundle information is found at the given 'uri'.
+ */
+int fetch_bundle_list(struct repository *r,
+		      const char *uri,
+		      struct bundle_list *list);
+
 /**
  * API for serve.c.
  */