| Message ID | pull.1779.v2.git.1725504725976.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] remote: prefetch config | expand |
"Shubham Kanodia via GitGitGadget" <gitgitgadget@gmail.com> writes: > diff --git a/builtin/gc.c b/builtin/gc.c > index 427faf1cfe1..2ca3a3e7d6a 100644 > --- a/builtin/gc.c > +++ b/builtin/gc.c > @@ -1027,6 +1027,9 @@ static int fetch_remote(struct remote *remote, void *cbdata) > if (remote->skip_default_update) > return 0; > > + if (!remote->prefetch) > + return 0; This, while better than ane xplicit comparison with "== 0", is a bit tricky in this patch, as it is not saying "if we are told to prefetch, fall through to the rest of the function". It is saying "leave if and only if we are explicitly configured not to prefetch". It might warrant a comment. > diff --git a/remote.c b/remote.c > index 8f3dee13186..05edb3a5f40 100644 > --- a/remote.c > +++ b/remote.c > @@ -140,6 +140,7 @@ static struct remote *make_remote(struct remote_state *remote_state, > CALLOC_ARRAY(ret, 1); > ret->prune = -1; /* unspecified */ > ret->prune_tags = -1; /* unspecified */ > + ret->prefetch = -1; /* unspecified */ Or, we can just assign "1" (and drop "unspecified" comment). ret->prefetch = 1; /* enabled by default */ If I understand it correctly, we want this to default to true... > ret->name = xstrndup(name, len); > refspec_init(&ret->push, REFSPEC_PUSH); > refspec_init(&ret->fetch, REFSPEC_FETCH); > @@ -456,6 +457,8 @@ 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, "prefetch")) > + remote->prefetch = git_config_bool(key, value); ... with a way for the user to turn it off. > diff --git a/remote.h b/remote.h > index b901b56746d..4522fdec354 100644 > --- a/remote.h > +++ b/remote.h > @@ -77,6 +77,15 @@ struct remote { > > struct refspec fetch; > > + /* > + * The setting for whether to prefetch from a remote > + * when a fetch is invoked with a prefetch flag. > + * -1 = unset > + * 0 = don't prefetch from this remote > + * 1 = prefetch from this remote > + */ > + int prefetch; And then we can get rid of "-1 unset" from this list. The comment can become a lot more brief, as such a change would make it a simple Boolean flag that everybody would understand immediately. "prefetch" in the comment is superfluous, as that is the name of the member anyway. "from this remote" is superfluous, as that is the point of having the member in "struct remote" that gives settings that are per-remote. int prefetch; /* is prefetch enabled? */ If we really want to have "unspecified yet" state, what we commonly do is * to initialize the variable to -1 to signal "unspecified yet", which you did in this patch. * after the configuration reader returns, check if the variable is still -1, and then explicitly reset it to the default value, which your patch does not do. * the code that uses the variable assumes it is either 0 or 1 and there shoudl be no "unspecified yet" value. It indeed is a bug that the ariable is left unspecified as it is a sign that the code to do previous step was somehow skipped. But I do not think it is needed in this case; initializing the .prefetch member to whichever is the default should be sufficient. Thanks.
On Thu, Sep 5, 2024 at 9:36 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Shubham Kanodia via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > diff --git a/builtin/gc.c b/builtin/gc.c > > index 427faf1cfe1..2ca3a3e7d6a 100644 > > --- a/builtin/gc.c > > +++ b/builtin/gc.c > > @@ -1027,6 +1027,9 @@ static int fetch_remote(struct remote *remote, void *cbdata) > > if (remote->skip_default_update) > > return 0; > > > > + if (!remote->prefetch) > > + return 0; > > This, while better than ane xplicit comparison with "== 0", is a bit > tricky in this patch, as it is not saying "if we are told to prefetch, > fall through to the rest of the function". It is saying "leave if > and only if we are explicitly configured not to prefetch". > > It might warrant a comment. > > > diff --git a/remote.c b/remote.c > > index 8f3dee13186..05edb3a5f40 100644 > > --- a/remote.c > > +++ b/remote.c > > @@ -140,6 +140,7 @@ static struct remote *make_remote(struct remote_state *remote_state, > > CALLOC_ARRAY(ret, 1); > > ret->prune = -1; /* unspecified */ > > ret->prune_tags = -1; /* unspecified */ > > + ret->prefetch = -1; /* unspecified */ > > Or, we can just assign "1" (and drop "unspecified" comment). > > ret->prefetch = 1; /* enabled by default */ > > If I understand it correctly, we want this to default to true... > > > ret->name = xstrndup(name, len); > > refspec_init(&ret->push, REFSPEC_PUSH); > > refspec_init(&ret->fetch, REFSPEC_FETCH); > > @@ -456,6 +457,8 @@ 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, "prefetch")) > > + remote->prefetch = git_config_bool(key, value); > > ... with a way for the user to turn it off. > > > diff --git a/remote.h b/remote.h > > index b901b56746d..4522fdec354 100644 > > --- a/remote.h > > +++ b/remote.h > > @@ -77,6 +77,15 @@ struct remote { > > > > struct refspec fetch; > > > > + /* > > + * The setting for whether to prefetch from a remote > > + * when a fetch is invoked with a prefetch flag. > > + * -1 = unset > > + * 0 = don't prefetch from this remote > > + * 1 = prefetch from this remote > > + */ > > + int prefetch; > > And then we can get rid of "-1 unset" from this list. The comment > can become a lot more brief, as such a change would make it a simple > Boolean flag that everybody would understand immediately. > > "prefetch" in the comment is superfluous, as that is the name of the > member anyway. "from this remote" is superfluous, as that is the > point of having the member in "struct remote" that gives settings > that are per-remote. > > int prefetch; /* is prefetch enabled? */ > > If we really want to have "unspecified yet" state, what we commonly > do is > > * to initialize the variable to -1 to signal "unspecified yet", > which you did in this patch. > > * after the configuration reader returns, check if the variable is > still -1, and then explicitly reset it to the default value, > which your patch does not do. > > * the code that uses the variable assumes it is either 0 or 1 and > there shoudl be no "unspecified yet" value. It indeed is a bug > that the ariable is left unspecified as it is a sign that the > code to do previous step was somehow skipped. > > But I do not think it is needed in this case; initializing the > .prefetch member to whichever is the default should be sufficient. > > Thanks. Fair. I kept the initial value as `unset` as that could be interpreted as a special case to do something else in the future — but I agree that keeping it initialized to default keeps things clearer for now since such a case doesn't arise. Updating my patch — please let me know if there's anything else I can improve here.
Shubham Kanodia <shubham.kanodia10@gmail.com> writes: >> >> int prefetch; /* is prefetch enabled? */ >> > ... > Updating my patch — please let me know if there's anything else I can > improve here. Renaming the .prefetch member to .prefetch_enabled would eliminate the need to add any comment on the member in the header file. Thanks.
On Thu, Sep 5, 2024 at 10:22 PM Junio C Hamano <gitster@pobox.com> wrote: > > Shubham Kanodia <shubham.kanodia10@gmail.com> writes: > > >> > >> int prefetch; /* is prefetch enabled? */ > >> > > ... > > Updating my patch — please let me know if there's anything else I can > > improve here. > > Renaming the .prefetch member to .prefetch_enabled would eliminate > the need to add any comment on the member in the header file. Do you mean for the struct member here or also the config? For the config, it'll probably be clearer to keep `prefetch` still as it aligns nicely with the boolean `--prefetch` command line flag. I can name the struct member `prefetch_enabled` — though I don't see other boolean remote properties (`prune`, `prune_tags`) add suffixes to indicate they are booleans. > Thanks.
Shubham Kanodia <shubham.kanodia10@gmail.com> writes: > On Thu, Sep 5, 2024 at 10:22 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Shubham Kanodia <shubham.kanodia10@gmail.com> writes: >> >> >> >> >> int prefetch; /* is prefetch enabled? */ >> >> >> > ... >> > Updating my patch — please let me know if there's anything else I can >> > improve here. >> >> Renaming the .prefetch member to .prefetch_enabled would eliminate >> the need to add any comment on the member in the header file. > > Do you mean for the struct member here or also the config? I do not think I mentioned anything about the name of the configuration variable, but if I did that was a mistake. End-user facing configuration variables are often named after a feature that it enables or disables, so it can use the name without "enable". An int variable on the other hand can mean many other things, ranging from "how many times have we prefetched from here" to "does this remote allow prefetching?", so a more explicit name would often help. Thanks.
diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt index 8efc53e836d..c2b3876192c 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>.prefetch:: + If false, refs from the remote would not be prefetched for + the prefetch task in linkgit:git-maintenance[1]. If not set, + the value is assumed to be true. + remote.<name>.push:: The default set of "refspec" for linkgit:git-push[1]. See linkgit:git-push[1]. diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt index 51d0f7e94b6..2fd38706ea2 100644 --- a/Documentation/git-maintenance.txt +++ b/Documentation/git-maintenance.txt @@ -97,9 +97,10 @@ commit-graph:: prefetch:: The `prefetch` task updates the object directory with the latest - objects from all registered remotes. For each remote, a `git fetch` - command is run. The configured refspec is modified to place all - requested refs within `refs/prefetch/`. Also, tags are not updated. + objects from all registered remotes unless they've disabled prefetch + using `remote.<remote>.prefetch` set to `false`. For each such remote, + a `git fetch` command is run. The configured refspec is modified to place + all requested refs within `refs/prefetch/`. Also, tags are not updated. + This is done to avoid disrupting the remote-tracking branches. The end users expect these refs to stay unmoved unless they initiate a fetch. However, diff --git a/builtin/gc.c b/builtin/gc.c index 427faf1cfe1..2ca3a3e7d6a 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1027,6 +1027,9 @@ static int fetch_remote(struct remote *remote, void *cbdata) if (remote->skip_default_update) return 0; + if (!remote->prefetch) + return 0; + child.git_cmd = 1; strvec_pushl(&child.args, "fetch", remote->name, "--prefetch", "--prune", "--no-tags", diff --git a/remote.c b/remote.c index 8f3dee13186..05edb3a5f40 100644 --- a/remote.c +++ b/remote.c @@ -140,6 +140,7 @@ static struct remote *make_remote(struct remote_state *remote_state, CALLOC_ARRAY(ret, 1); ret->prune = -1; /* unspecified */ ret->prune_tags = -1; /* unspecified */ + ret->prefetch = -1; /* unspecified */ ret->name = xstrndup(name, len); refspec_init(&ret->push, REFSPEC_PUSH); refspec_init(&ret->fetch, REFSPEC_FETCH); @@ -456,6 +457,8 @@ 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, "prefetch")) + remote->prefetch = git_config_bool(key, value); else if (!strcmp(subkey, "url")) { if (!value) return config_error_nonbool(key); diff --git a/remote.h b/remote.h index b901b56746d..4522fdec354 100644 --- a/remote.h +++ b/remote.h @@ -77,6 +77,15 @@ struct remote { struct refspec fetch; + /* + * The setting for whether to prefetch from a remote + * when a fetch is invoked with a prefetch flag. + * -1 = unset + * 0 = don't prefetch from this remote + * 1 = prefetch from this remote + */ + int prefetch; + /* * 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..7484e1f1d46 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -245,6 +245,48 @@ test_expect_success 'prefetch multiple remotes' ' test_subcommand git fetch remote2 $fetchargs <skip-remote1.txt ' +test_expect_success 'prefetch respects remote.*.prefetch config' ' + test_create_repo prefetch-test-config && + ( + cd prefetch-test-config && + test_commit initial && + test_create_repo clone1 && + test_create_repo clone2 && + test_create_repo clone3 && + + git remote add remote1 "file://$(pwd)/clone1" && + git remote add remote2 "file://$(pwd)/clone2" && + git remote add remote3 "file://$(pwd)/clone3" && + + git config remote.remote1.prefetch false && + git config remote.remote2.prefetch true && + # remote3 is left unset + + # Make changes in all clones + git -C clone1 switch -c one && + git -C clone2 switch -c two && + git -C clone3 switch -c three && + test_commit -C clone1 one && + test_commit -C clone2 two && + test_commit -C clone3 three && + + # Run maintenance prefetch task + GIT_TRACE2_EVENT="$(pwd)/prefetch.txt" git maintenance run --task=prefetch 2>/dev/null && + + # Check that if remotes were prefetched properly + fetchargs="--prefetch --prune --no-tags --no-write-fetch-head --recurse-submodules=no --quiet" && + test_subcommand ! git fetch remote1 $fetchargs <prefetch.txt && + test_subcommand git fetch remote2 $fetchargs <prefetch.txt && + test_subcommand git fetch remote3 $fetchargs <prefetch.txt && + + # Verify that changes are in the prefetch refs for remote2 and remote3, but not remote1 + test_must_fail git rev-parse refs/prefetch/remotes/remote1/one && + git fetch --all && + test_cmp_rev refs/remotes/remote2/two refs/prefetch/remotes/remote2/two && + test_cmp_rev refs/remotes/remote3/three refs/prefetch/remotes/remote3/three + ) +' + test_expect_success 'loose-objects task' ' # Repack everything so we know the state of the object dir git repack -adk &&