| Message ID | pull.1782.v3.git.1726741439445.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] remote: introduce config to set prefetch refs | expand |
"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.
Sounds good. Submitted a patch at: https://lore.kernel.org/git/pull.1782.v4.git.1728073292874.gitgitgadget@gmail.com/ Which seems to be detached from this thread now — perhaps because I re-wrote the commit message. Or perhaps I'm reading the threads wrong. Apologies for the break. On Tue, Sep 24, 2024 at 2:54 AM Junio C Hamano <gitster@pobox.com> wrote: > > "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.
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. + remote.<name>.push:: The default set of "refspec" for linkgit:git-push[1]. See linkgit:git-push[1]. 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" #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000) @@ -485,6 +486,49 @@ static void filter_prefetch_refspec(struct refspec *rs) } } +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++) { + 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; + } + } + + if (!has_positive) + return !matched_negative; + + return matched_positive && !matched_negative; +} + + +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; + } + } +} + static struct ref *get_ref_map(struct remote *remote, const struct ref *remote_refs, struct refspec *rs, @@ -502,6 +546,7 @@ static struct ref *get_ref_map(struct remote *remote, int existing_refs_populated = 0; filter_prefetch_refspec(rs); + if (remote) filter_prefetch_refspec(&remote->fetch); @@ -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); + } + } + } + ref_map = ref_remove_duplicates(ref_map); for (rm = ref_map; rm; rm = rm->next) { diff --git a/remote.c b/remote.c index 8f3dee13186..b46d62b2c47 100644 --- a/remote.c +++ b/remote.c @@ -141,6 +141,7 @@ static struct remote *make_remote(struct remote_state *remote_state, ret->prune = -1; /* unspecified */ ret->prune_tags = -1; /* unspecified */ ret->name = xstrndup(name, len); + string_list_init_dup(&ret->prefetch_refs); refspec_init(&ret->push, REFSPEC_PUSH); refspec_init(&ret->fetch, REFSPEC_FETCH); @@ -166,6 +167,7 @@ static void remote_clear(struct remote *remote) free((char *)remote->uploadpack); FREE_AND_NULL(remote->http_proxy); FREE_AND_NULL(remote->http_proxy_authmethod); + string_list_clear(&remote->prefetch_refs, 0); } static void add_merge(struct branch *branch, const char *name) @@ -456,6 +458,12 @@ static int handle_config(const char *key, const char *value, remote->prune = git_config_bool(key, value); else if (!strcmp(subkey, "prunetags")) remote->prune_tags = git_config_bool(key, value); + else if (!strcmp(subkey, "prefetchref")) { + if (!value) + return config_error_nonbool(key); + string_list_split(&remote->prefetch_refs, value, ' ', -1); + return 0; + } else if (!strcmp(subkey, "url")) { if (!value) return config_error_nonbool(key); diff --git a/remote.h b/remote.h index b901b56746d..c18e68e0d8d 100644 --- a/remote.h +++ b/remote.h @@ -5,6 +5,7 @@ #include "hashmap.h" #include "refspec.h" #include "strvec.h" +#include "string-list.h" struct option; struct transport_ls_refs_options; @@ -77,6 +78,8 @@ struct remote { struct refspec fetch; + struct string_list prefetch_refs; + /* * The setting for whether to fetch tags (as a separate rule from the * configured refspecs); 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 @@ -245,6 +245,84 @@ test_expect_success 'prefetch multiple remotes' ' test_subcommand git fetch remote2 $fetchargs <skip-remote1.txt ' +test_expect_success 'prefetch with positive prefetch ref patterns' ' + test_create_repo filter-prefetch-positive && + ( + cd filter-prefetch-positive && + test_commit initial && + git clone . clone2 && + git remote add remote2 "file://$(pwd)/clone2" && + + cd clone2 && + git checkout -b feature && test_commit feature-commit-2 && + git checkout -b wip/test && test_commit wip-test-commit-2 && + git checkout -b topic/x && test_commit topic-x-commit-2 && + git push -f origin feature wip/test topic/x&& + cd .. && + + git config remote.remote2.prefetchref "refs/heads/feature" && + fetchargs="--prefetch --prune --no-tags --no-write-fetch-head --recurse-submodules=no --quiet" && + GIT_TRACE2_EVENT="$(pwd)/prefetch-positive.txt" git maintenance run --task=prefetch 2>/dev/null && + test_subcommand git fetch remote2 $fetchargs <prefetch-positive.txt && + + git rev-parse refs/prefetch/remotes/remote2/feature && + test_must_fail git rev-parse refs/prefetch/remotes/remote2/wip/test && + test_must_fail git rev-parse refs/prefetch/remotes/remote2/topic/x + ) +' + +test_expect_success 'prefetch with negative prefetch ref patterns' ' + test_create_repo filter-prefetch-negative && + ( + cd filter-prefetch-negative && + test_commit initial && + git clone . clone3 && + git remote add remote3 "file://$(pwd)/clone3" && + cat .git/config && + + cd clone3 && + git checkout -b feature && test_commit feature-commit-3 && + git checkout -b wip/test && test_commit wip-test-commit-3 && + git checkout -b topic/x && test_commit topic-x-commit-3 && + git push -f origin feature wip/test topic/x && + cd .. && + + git config remote.remote3.prefetchref "!refs/heads/wip/*" && + fetchargs="--prefetch --prune --no-tags --no-write-fetch-head --recurse-submodules=no --quiet" && + GIT_TRACE2_EVENT="$(pwd)/prefetch-negative.txt" git maintenance run --task=prefetch 2>/dev/null && + test_subcommand git fetch remote3 $fetchargs <prefetch-negative.txt && + git rev-parse refs/prefetch/remotes/remote3/feature && + git rev-parse refs/prefetch/remotes/remote3/topic/x && + test_must_fail git rev-parse refs/prefetch/remotes/remote3/wip/test + ) +' + +test_expect_success 'prefetch with positive & negative prefetch ref patterns' ' + test_create_repo filter-prefetch-mixed && + ( + cd filter-prefetch-mixed && + test_commit initial && + git clone . clone4 && + git remote add remote4 "file://$(pwd)/clone4" && + + cd clone4 && + git checkout -b feature && test_commit feature-commit-4 && + git checkout -b topic/x && test_commit topic-x-commit-4 && + git checkout -b topic/y && test_commit topic-y-commit-4 && + git push -f origin feature topic/x topic/y && + cd .. && + + git config remote.remote4.prefetchref "refs/heads/topic/* !refs/heads/topic/y" && + fetchargs="--prefetch --prune --no-tags --no-write-fetch-head --recurse-submodules=no --quiet" && + GIT_TRACE2_EVENT="$(pwd)/prefetch-mixed.txt" git maintenance run --task=prefetch 2>/dev/null && + test_subcommand git fetch remote4 $fetchargs <prefetch-mixed.txt && + + test_must_fail git rev-parse refs/prefetch/remotes/remote4/feature && + test_must_fail git rev-parse refs/prefetch/remotes/remote4/topic/y && + git rev-parse refs/prefetch/remotes/remote4/topic/x + ) +' + test_expect_success 'loose-objects task' ' # Repack everything so we know the state of the object dir git repack -adk &&