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

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

"Shubham Kanodia via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Shubham Kanodia <shubham.kanodia10@gmail.com>
>
> This commit introduces a new configuration option, ...

The usual way to compose a log message of this project is to

 - Give an observation on how the current system work in the present
   tense (so no need to say "Currently X is Y", just "X is Y"), and
   discuss what you perceive as a problem in it.

 - Propose a solution (optional---often, problem description
   trivially leads to an obvious solution in reader's minds).

 - Give commands to the codebase to "become like so".

in this order.

To those who have been intimately following the discussion, it often
is understandable without some of the above, but we are not writing
for those who review the patches.  We are primarily writing for future
readers of "git log" who are not aware of the review discussion we
have on list, so we should give something to prepare them by setting
the stage and stating the objective first, before going into how the
patch solved it.

> diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt
> index 8efc53e836d..b04ee0c4c22 100644
> --- a/Documentation/config/remote.txt
> +++ b/Documentation/config/remote.txt
> @@ -33,6 +33,11 @@ remote.<name>.fetch::
>  	The default set of "refspec" for linkgit:git-fetch[1]. See
>  	linkgit:git-fetch[1].
>  
> +remote.<name>.prefetchref::
> +    Specify the refs to be prefetched when fetching from this remote.
> +    The value is a space-separated list of ref patterns (e.g., "refs/heads/master !refs/heads/develop*").
> +    This can be used to optimize fetch operations by specifying exactly which refs should be prefetched.

Overly long lines.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b2b5aee5bf2..16c8a31c2e1 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -38,6 +38,7 @@
>  #include "trace.h"
>  #include "trace2.h"
>  #include "bundle-uri.h"
> +#include "wildmatch.h"

The match done by the wildmatch function is a way too loose to be
used in the context of parsing and matching the src half of an
refspec_item (e.g. a globbing refspec element can have at most one
asterisk in it, but wildmatch does not have any sanity check to
ensure it).  The refspec logic to accept a fetch refspec
"refs/heads/*:refs/remotes/origin/*" and rewrite a ref the remote
side advertised as refs/heads/foo into refs/remotes/origin/foo, the
remote-tracking ref on the local side, is in
remote.c:match_name_with_pattern(), and it does not use wildmatch.

> +static int matches_prefetch_refs(const char *refname, const struct string_list *prefetch_refs)
> +{
> +	int i;
> +	int has_positive = 0;
> +	int matched_positive = 0;
> +	int matched_negative = 0;
> +
> +	for (i = 0; i < prefetch_refs->nr; i++) {

Just FYI, in modern Git codebase, loop control variable 'i' can be
declared in the initialization part of the for() statement, i.e.

    for (int i = 0; i < ...) {

which might make it easier to see that there are three variables of
importance to determine the outcome of the computation (and 'i' is
merely a loop counter).

> +		const char *pattern = prefetch_refs->items[i].string;
> +		int is_negative = (*pattern == '!');
> +
> +		if (is_negative)
> +			pattern++;
> +		else
> +			has_positive = 1;
> +
> +		if (wildmatch(pattern, refname, 0) == 0) {
> +			if (is_negative)
> +				matched_negative = 1;
> +			else
> +				matched_positive = 1;
> +		}
> +	}

I suspect that the function can be exposed to external callers as-is,
possibly after renaming it to make it clear that the helper function
is about refspec.  Then you can call it from the code below in stead
of wildmatch(), by passing the pattern as "key", and the refname as
"name", with NULL for both "value" and "result", I think.

> +
> +	if (!has_positive)
> +		return !matched_negative;
> +
> +	return matched_positive && !matched_negative;
> +}

And the implementation of the logic looks correct.  If the pattern
says "!A !B !C" (I don't want any of these), we only care that none
of these negative patterns trigger, but if the pattern has even one
positive one "X Y !A !B !C" (it must match X or Y, but it must not
match any of the A B C), we also make sure it matched at least one
positive pattern.

> +static void ref_remove(struct ref **head, struct ref *to_remove)
> +{
> +	struct ref **pp, *p;
> +
> +	for (pp = head; (p = *pp) != NULL; pp = &p->next) {
> +		if (p == to_remove) {
> +			*pp = p->next;
> +			return;
> +		}
> +	}
> +}

We remote from a linked list of "struct ref" one such element
to_remove.  OK.

> @@ -610,6 +655,22 @@ static struct ref *get_ref_map(struct remote *remote,
>  	else
>  		ref_map = apply_negative_refspecs(ref_map, &remote->fetch);
>  
> +	/**
> +	 * Filter out advertised refs that we don't want to fetch during
> +	 * prefetch if a prefetchref config is set
> +	 */
> +	if (prefetch && remote->prefetch_refs.nr) {
> +		struct ref *ref, *next;
> +		for (ref = ref_map; ref; ref = next) {
> +			next = ref->next;
> +
> +			if (!matches_prefetch_refs(ref->name, &remote->prefetch_refs)) {
> +					ref_remove(&ref_map, ref);
> +					free_one_ref(ref);

It is curious why such an overly deep indentation is used only in
this if() statement.

It is good that you made sure we do not leak the ref that we
removed, but it looks verly inefficient to repeatedly call
ref_remove (the function becomes more inefficient as the loop
progresses as it is more costly to remove later elements on a
linearly linked list).

Wouldn't it be more efficient to iterate over the original list, sift
each element into "surviving" and "discarded" bin as we go?  Something
along the lines of ...

	struct ref *new_list = NULL, **tail = &new_list;

	while (ref_map) {
		struct ref *ref = ref_map;

		ref_map = ref_map->next;
		ref->next = NULL;

        	if (matches(...)) {
			*tail = ref;
			tail = &ref->next;
		} else {
			free_one_ref(ref);
		}
	}
        ref_map = new_list;

..., which I imitated code structure of ref_remove_duplicates().

>  	ref_map = ref_remove_duplicates(ref_map);

> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index abae7a97546..054f1f06f95 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh

Avoid overly long lines in the test script, too.

In any case, I very much agree with you that get_ref_map().is the
best place to add this logic to.  Looking much better.

Thanks.