"Shubham Kanodia via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> +static void apply_prefetch_refspec(struct remote *remote, struct refspec *rs)
> +{
> + if (remote->prefetch_refs.nr > 0) {
> + int i;
> + for (i = 0; i < remote->prefetch_refs.nr; i++) {
> + const char *src = remote->prefetch_refs.items[i].string;
> + struct strbuf dst = STRBUF_INIT;
> +
> + strbuf_addf(&dst, "refs/prefetch/%s/", remote->name);
> + if (starts_with(src, "refs/heads/")) {
> + strbuf_addstr(&dst, src + 11);
> + } else if (starts_with(src, "refs/")) {
> + strbuf_addstr(&dst, src + 5);
> + } else {
> + strbuf_addstr(&dst, src);
> + }
> +
> + refspec_appendf(rs, "%s:%s", src, dst.buf);
> + strbuf_release(&dst);
> + }
> + }
> +}
> static void filter_prefetch_refspec(struct refspec *rs)
> {
> int i;
> @@ -502,8 +526,11 @@ static struct ref *get_ref_map(struct remote *remote,
> int existing_refs_populated = 0;
>
> filter_prefetch_refspec(rs);
> - if (remote)
> + if (remote) {
> filter_prefetch_refspec(&remote->fetch);
> + if (prefetch)
> + apply_prefetch_refspec(remote, rs);
> + }
Hmph, a separate helper function with a separate loop was something
I did not expect to see. Looking at the filter_prefetch_refspec(),
it already limits what it prefetched to what we usually fetch from
the remote, and filteres out the tag namespace. I was hoping that
this will _extend_ that existing mechanism, as if by default we
have prefetch refspec "!refs/tags/*", which can be replaced by the
configuration to say "!refs/tags/* !refs/changes/*", or positive
ones like "refs/heads/*". The filtering semantics should be
* a refspec whose src matches negated pattern (like !refs/tags/*)
is rejected.
* if the prefetch_refs has *only* positive patterns, then a refspec
whose src does not match *any* of the pattern is rejected.
* a refspec that is not rejected is prefetched.
But the above still allows what filter_prefetch_refspec() does by
default, without any way to narrow it down, and then adds its own
entries.
This is a half-tangent, but while studying for this topic, I noticed
that filter_prefetch_refspec() triggers O(n) loop every time a fetch
refspec is skipped.
It all comes from 2e03115d (fetch: add --prefetch option,
2021-04-16) but rewriting the loop to use two pointers into the
array seemed trivial and the result seemed a bit more readable.
Your "further limit the prefetched refs with configuration" feature
would probably replace this part of the updated code:
+ /* skip ones that do not store, or store in refs/tags */
+ if (!rs->items[scan].dst ||
+ (rs->items[scan].src &&
+ starts_with(rs->items[scan].src,
+ ref_namespace[NAMESPACE_TAGS].ref))) {
That is, instead of "skip ones that do not store, or store in refs/tags",
filtering using the configured value (probably implemented as a helper
function) would be used as the condition of the if statement.
Thoughts?
builtin/fetch.c | 46 ++++++++++++++++++++++++----------------------
1 file changed, 24 insertions(+), 22 deletions(-)
diff --git c/builtin/fetch.c w/builtin/fetch.c
index 693f02b958..219302ed67 100644
--- c/builtin/fetch.c
+++ w/builtin/fetch.c
@@ -436,37 +436,38 @@ static void find_non_local_tags(const struct ref *refs,
static void filter_prefetch_refspec(struct refspec *rs)
{
- int i;
+ int scan, store;
if (!prefetch)
return;
- for (i = 0; i < rs->nr; i++) {
+ /*
+ * scan refspec items with 'scan', and decide to either
+ * mangle and store it in 'store', or omit it from the result.
+ */
+ for (scan = store = 0; scan < rs->nr; scan++, store++) {
struct strbuf new_dst = STRBUF_INIT;
char *old_dst;
const char *sub = NULL;
- if (rs->items[i].negative)
- continue;
- if (!rs->items[i].dst ||
- (rs->items[i].src &&
- starts_with(rs->items[i].src,
- ref_namespace[NAMESPACE_TAGS].ref))) {
- int j;
-
- free(rs->items[i].src);
- free(rs->items[i].dst);
-
- for (j = i + 1; j < rs->nr; j++) {
- rs->items[j - 1] = rs->items[j];
- rs->raw[j - 1] = rs->raw[j];
- }
- rs->nr--;
- i--;
+ /* negative ones are kept as-is */
+ if (rs->items[scan].negative) {
+ if (store != scan)
+ rs->items[store] = rs->items[scan];
continue;
}
- old_dst = rs->items[i].dst;
+ /* skip ones that do not store, or store in refs/tags */
+ if (!rs->items[scan].dst ||
+ (rs->items[scan].src &&
+ starts_with(rs->items[scan].src,
+ ref_namespace[NAMESPACE_TAGS].ref))) {
+ refspec_item_clear(&rs->items[scan]);
+ store--; /* compensate for loop's auto increment */
+ continue;
+ }
+
+ old_dst = rs->items[scan].dst;
strbuf_addstr(&new_dst, ref_namespace[NAMESPACE_PREFETCH].ref);
/*
@@ -478,11 +479,12 @@ static void filter_prefetch_refspec(struct refspec *rs)
sub = old_dst;
strbuf_addstr(&new_dst, sub);
- rs->items[i].dst = strbuf_detach(&new_dst, NULL);
- rs->items[i].force = 1;
+ rs->items[store].dst = strbuf_detach(&new_dst, NULL);
+ rs->items[store].force = 1;
free(old_dst);
}
+ rs->nr = store;
}
static struct ref *get_ref_map(struct remote *remote,
