Re: [PATCH] remote: introduce config to set prefetch refs

[Junio C Hamano <gitster@pobox.com>]

"Shubham Kanodia via GitGitGadget" <gitgitgadget@gmail.com> 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,