| Message ID | pull.1779.git.1725472799637.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remote: prefetch config | expand |
On 9/4/24 1:59 PM, Shubham Kanodia via GitGitGadget wrote: > From: Shubham Kanodia <shubham.kanodia10@gmail.com> > > Large repositories often contain numerous branches and refs, many of > which individual users may not need. This commit introduces a new > configuration option (`remote.<remote>.prefetch`) to allow > users to specify which remotes to prefetch during > the maintenance task. > > Key behaviors: > 1. If `remote.<remote>.prefetch` is unset or true, running > `git-maintenance` will prefetch all refs for the remote. > 2. If `remote.<remote>.prefetch` is set to false, the remote > will be ignored for prefetching. Thanks for this contribution. I think this is a good idea for extra flexibility of the prefetch task. > In a future change, we could also allow restricting the refs that are > prefetched per remote using the `prefetchref` config option per remote. I agree that this would also be of interest, but more complicated. Thanks for starting with this simpler modification. > + if (remote->prefetch == 0) > + return 0; In the Git codebase, this would normally be written as if (!remote->prefetch) return 0; > + # Run maintenance prefetch task > + GIT_TRACE2_EVENT="$(pwd)/prefetch.txt" git maintenance run --task=prefetch 2>/dev/null && > + > + # Check that remote1 was not fetched (prefetch=false) > + test_subcommand ! git fetch remote1 --prefetch --prune --no-tags \ > + --no-write-fetch-head --recurse-submodules=no --quiet \ > + <prefetch.txt && I'm happy to see this use of test_subcommand to validate the behavior of this patch! This is a very good patch and I only have the one style nit. Thanks, -Stolee
Derrick Stolee <stolee@gmail.com> writes: > I agree that this would also be of interest, but more complicated. Thanks > for starting with this simpler modification. > >> + if (remote->prefetch == 0) >> + return 0; > > In the Git codebase, this would normally be written as > > if (!remote->prefetch) > return 0; ;-) >> + # Run maintenance prefetch task >> + GIT_TRACE2_EVENT="$(pwd)/prefetch.txt" git maintenance run --task=prefetch 2>/dev/null && >> + >> + # Check that remote1 was not fetched (prefetch=false) >> + test_subcommand ! git fetch remote1 --prefetch --prune --no-tags \ >> + --no-write-fetch-head --recurse-submodules=no --quiet \ >> + <prefetch.txt && > > I'm happy to see this use of test_subcommand to validate the behavior > of this patch! I found it a bit disturbing that the pattern is overly specific. The only thing we are interested in is that we are not fetching from remote1, so it _should_ suffice if we could write test_subcommand ! git fetch remote1 <prefetch.txt && to avoid being tied to how the current version of Git happens to pass these command line option flags and the order it does so. Looking at the implementation of test_subcommand, it seems that we cannot quite do that (it assumes that the pattern it assembles out of the parameters are to match the full argument list used in invocation, enclosing them in a single [] pair and without giving the caller an easy way to sneak wildcards like ".*" in), which is sad. So, the expected command line being too strit is *not* a fault of this patch, and with the style fix, I think this half of the solution is a good one. Thanks.
On 9/4/24 4:55 PM, Junio C Hamano wrote: > Derrick Stolee <stolee@gmail.com> writes: >>> + # Run maintenance prefetch task >>> + GIT_TRACE2_EVENT="$(pwd)/prefetch.txt" git maintenance run --task=prefetch 2>/dev/null && >>> + >>> + # Check that remote1 was not fetched (prefetch=false) >>> + test_subcommand ! git fetch remote1 --prefetch --prune --no-tags \ >>> + --no-write-fetch-head --recurse-submodules=no --quiet \ >>> + <prefetch.txt && >> >> I'm happy to see this use of test_subcommand to validate the behavior >> of this patch! > > I found it a bit disturbing that the pattern is overly specific. > > The only thing we are interested in is that we are not fetching from > remote1, so it _should_ suffice if we could write > > test_subcommand ! git fetch remote1 <prefetch.txt && > > to avoid being tied to how the current version of Git happens to > pass these command line option flags and the order it does so. > > Looking at the implementation of test_subcommand, it seems that we > cannot quite do that (it assumes that the pattern it assembles out > of the parameters are to match the full argument list used in > invocation, enclosing them in a single [] pair and without giving > the caller an easy way to sneak wildcards like ".*" in), which is > sad. I agree the ergonomics of the test_subcommand helper is a bit poor (and not this patch author's fault). The trickiest part is the negative case, as in this highlighted one. It's hard to read from this if the subcommand wasn't found because the argument list is too specific and doesn't match the exact arguments. It helps that the same options are given for the other, positive tests. But maybe that could be a hint as to how to make this test a bit cleaner: make a variable describing the "uninteresting" arguments. Something like... args="--prefetch --prune --no-tags \ --no-write-fetch-head --recurse-submodules=no --quiet" && test_subcommand ! git fetch remote1 $args <prefetch.txt && test_subcommand git fetch remote2 $args <prefetch.txt && test_subcommand git fetch remote3 $args <prefetch.txt && Thanks, -Stolee
On Thu, Sep 5, 2024 at 7:38 AM Derrick Stolee <stolee@gmail.com> wrote: > > On 9/4/24 4:55 PM, Junio C Hamano wrote: > > Derrick Stolee <stolee@gmail.com> writes: > > >>> + # Run maintenance prefetch task > >>> + GIT_TRACE2_EVENT="$(pwd)/prefetch.txt" git maintenance run --task=prefetch 2>/dev/null && > >>> + > >>> + # Check that remote1 was not fetched (prefetch=false) > >>> + test_subcommand ! git fetch remote1 --prefetch --prune --no-tags \ > >>> + --no-write-fetch-head --recurse-submodules=no --quiet \ > >>> + <prefetch.txt && > >> > >> I'm happy to see this use of test_subcommand to validate the behavior > >> of this patch! > > > > I found it a bit disturbing that the pattern is overly specific. > > > > The only thing we are interested in is that we are not fetching from > > remote1, so it _should_ suffice if we could write > > > > test_subcommand ! git fetch remote1 <prefetch.txt && > > > > to avoid being tied to how the current version of Git happens to > > pass these command line option flags and the order it does so. > > > > Looking at the implementation of test_subcommand, it seems that we > > cannot quite do that (it assumes that the pattern it assembles out > > of the parameters are to match the full argument list used in > > invocation, enclosing them in a single [] pair and without giving > > the caller an easy way to sneak wildcards like ".*" in), which is > > sad. > I agree the ergonomics of the test_subcommand helper is a bit poor > (and not this patch author's fault). The trickiest part is the > negative case, as in this highlighted one. It's hard to read from > this if the subcommand wasn't found because the argument list is > too specific and doesn't match the exact arguments. > > It helps that the same options are given for the other, positive > tests. But maybe that could be a hint as to how to make this test > a bit cleaner: make a variable describing the "uninteresting" > arguments. Something like... > > args="--prefetch --prune --no-tags \ > --no-write-fetch-head --recurse-submodules=no --quiet" && > > test_subcommand ! git fetch remote1 $args <prefetch.txt && > test_subcommand git fetch remote2 $args <prefetch.txt && > test_subcommand git fetch remote3 $args <prefetch.txt && > > Thanks, > -Stolee > Agree with both the suggestions here. Updated my patch.
Derrick Stolee <stolee@gmail.com> writes: > On 9/4/24 4:55 PM, Junio C Hamano wrote: >> Derrick Stolee <stolee@gmail.com> writes: > >>>> + # Run maintenance prefetch task >>>> + GIT_TRACE2_EVENT="$(pwd)/prefetch.txt" git maintenance run --task=prefetch 2>/dev/null && >>>> + >>>> + # Check that remote1 was not fetched (prefetch=false) >>>> + test_subcommand ! git fetch remote1 --prefetch --prune --no-tags \ >>>> + --no-write-fetch-head --recurse-submodules=no --quiet \ >>>> + <prefetch.txt && >>> >>> I'm happy to see this use of test_subcommand to validate the behavior >>> of this patch! >> I found it a bit disturbing that the pattern is overly specific. >> The only thing we are interested in is that we are not fetching from >> remote1, so it _should_ suffice if we could write >> test_subcommand ! git fetch remote1 <prefetch.txt && >> to avoid being tied to how the current version of Git happens to >> pass these command line option flags and the order it does so. >> Looking at the implementation of test_subcommand, it seems that we >> cannot quite do that (it assumes that the pattern it assembles out >> of the parameters are to match the full argument list used in >> invocation, enclosing them in a single [] pair and without giving >> the caller an easy way to sneak wildcards like ".*" in), which is >> sad. > I agree the ergonomics of the test_subcommand helper is a bit poor > (and not this patch author's fault). I suspect that we could do test_subcommand ! git fetch remote1 --prefetch '.*' <prefetch.txt which would be rewritten to this pattern \["git", "fetch", "remote1", "--prefetch", ".*"\] if I am reading how the expr given to grep is built by the test_subcommand implementation. As long a there is at least one actual argument after the "--prefetch" one, .* would slurp everything. But it is ugly. In any case, this is a tangent unrelated to the topic of the patch on this thread.
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..88b8d80aff6 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 == 0) + 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..57d21a7bfe7 100644 --- a/remote.h +++ b/remote.h @@ -77,6 +77,15 @@ struct remote { struct refspec fetch; + /* + * This 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..7bc349ec546 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -245,6 +245,57 @@ 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 remote1 was not fetched (prefetch=false) + test_subcommand ! git fetch remote1 --prefetch --prune --no-tags \ + --no-write-fetch-head --recurse-submodules=no --quiet \ + <prefetch.txt && + + # Check that remote2 was fetched (prefetch=true) + test_subcommand git fetch remote2 --prefetch --prune --no-tags \ + --no-write-fetch-head --recurse-submodules=no --quiet \ + <prefetch.txt && + + # Check that remote3 was fetched (prefetch unset, default to true) + test_subcommand git fetch remote3 --prefetch --prune --no-tags \ + --no-write-fetch-head --recurse-submodules=no --quiet \ + <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 &&