On Mon, Sep 9, 2024 at 10:12 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "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,
How should we handle the related `remote.<remote-name>.fetch` config?
In an earlier discussion, it was discussed that —
`.prefetchref` should override `.fetch` completely (instead of
patching it) which makes sense to me.
At the moment my reading is that `filter_prefetch_refspec` still
filters / modifies `remote->fetch`.
```
if (remote) {
filter_prefetch_refspec(&remote->fetch);
}
```
So we'll need to clear refspecs and re-populate perhaps?
