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

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

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.

Overly long lines.

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.

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).

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.

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.

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

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().

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.