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

>
> "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,