remote: prefetch config
PATCH 1/1 remote: prefetch config
From: Shubham Kanodia <[email protected]>
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.
In a future change, we could also allow restricting the refs that are prefetched per remote using the prefetchref config option per remote.
Both of these options in unison would allow users to optimize their prefetch operations, reducing network traffic and disk usage.
---
remote: prefetch config
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1779%2Fpastelsky%2Fsk%2Fmaintenance-prefetch-remote-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1779/pastelsky/sk/maintenance-prefetch-remote-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1779
Documentation/config/remote.txt | 5 +++ Documentation/git-maintenance.txt | 7 +++-- builtin/gc.c | 3 ++ remote.c | 3 ++ remote.h | 9 ++++++ t/t7900-maintenance.sh | 51 +++++++++++++++++++++++++++++++ 6 files changed, 75 insertions(+), 3 deletions(-)
Documentation/config/remote.txt +5 -0
@@ -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].
Documentation/git-maintenance.txt +4 -3
@@ -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,
builtin/gc.c +3 -0
@@ -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",
remote.c +3 -0
@@ -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);
remote.h +9 -0
@@ -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);
t/t7900-maintenance.sh +51 -0
57 lines not on this page · raw message
base-commit: 2e7b89e038c0c888acf61f1b4ee5a43d4dd5e94c
Signature · 1 line
gitgitgadget
Quoted 8 lines · Shubham Kanodia via GitGitGadget
From: Shubham Kanodia <[email protected]>
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.
Quoted 5 lines
Key behaviors:
1. Ifremote.<remote>.prefetchis unset or true, runninggit-maintenancewill prefetch all refs for the remote.
2. Ifremote.<remote>.prefetchis 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.
Quoted 2 lines
In a future change, we could also allow restricting the refs that are prefetched per remote using the
prefetchrefconfig option per remote.
I agree that this would also be of interest, but more complicated. Thanks for starting with this simpler modification.
builtin/gc.c static int fetch_remote(struct remote *remote, void *cbdata) · line 1031
Quoted 2 lines
builtin/gc.c +2
-0+if (remote->prefetch ==0 ) +return 0 ;
In the Git codebase, this would normally be written as
if (!remote->prefetch) return 0;
t/t7900-maintenance.sh test_expect_success 'prefetch multiple remotes' ' · line 279
Quoted 7 lines
t/t7900-maintenance.sh +7
-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
Quoted 10 lines · Derrick Stolee
I agree that this would also be of interest, but more complicated. Thanks for starting with this simpler modification.
builtin/gc.c static int fetch_remote(struct remote *remote, void *cbdata) · line 1031
Quoted 2 lines
builtin/gc.c +2
-0+if (remote->prefetch ==0 ) +return 0 ;In the Git codebase, this would normally be written as
if (!remote->prefetch) return 0;
;-)
Quoted 10 lines
t/t7900-maintenance.sh test_expect_success 'prefetch multiple remotes' ' · line 279
Quoted 7 lines
t/t7900-maintenance.sh +7
-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_subcommandto 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.
Quoted 23 lines · Junio C Hamano
Quoted 10 lines · Derrick Stolee
t/t7900-maintenance.sh test_expect_success 'prefetch multiple remotes' ' · line 279
Quoted 7 lines
t/t7900-maintenance.sh +7
-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_subcommandto 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.
Quoted 6 lines
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
Quoted 49 lines · Derrick Stolee
Quoted 23 lines · Junio C Hamano
Quoted 10 lines · Derrick Stolee
t/t7900-maintenance.sh test_expect_success 'prefetch multiple remotes' ' · line 279
Quoted 7 lines
t/t7900-maintenance.sh +7
-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_subcommandto 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.
Quoted 6 lines
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_subcommandhelper 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 &&
Quoted 3 lines
Thanks,
-Stolee
Agree with both the suggestions here. Updated my patch.
Quoted 25 lines · Derrick Stolee
Quoted 16 lines · Junio C Hamano
Quoted 10 lines · Derrick Stolee
t/t7900-maintenance.sh test_expect_success 'prefetch multiple remotes' ' · line 279
Quoted 7 lines
t/t7900-maintenance.sh +7
-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_subcommandto 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 &&Quoted 8 lines
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.
Quoted 2 lines
I agree the ergonomics of the
test_subcommandhelper 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.
PATCH v2 1/1 remote: prefetch config
In reply to Shubham Kanodia via GitGitGadgetFrom: Shubham Kanodia <[email protected]>
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.
In a future change, we could also allow restricting the refs that are prefetched per remote using the prefetchref config option per remote.
Both of these options in unison would allow users to optimize their prefetch operations, reducing network traffic and disk usage.
---
remote: prefetch config
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1779%2Fpastelsky%2Fsk%2Fmaintenance-prefetch-remote-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1779/pastelsky/sk/maintenance-prefetch-remote-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1779
Range-diff vs v1:
1: 667553b114b ! 1: 1d58b782e22 remote: prefetch config
@@ builtin/gc.c: static int fetch_remote(struct remote *remote, void *cbdata)
if (remote->skip_default_update)
return 0;
-+ if (remote->prefetch == 0)
++ if (!remote->prefetch)
+ return 0;
+
child.git_cmd = 1;
@@ remote.h: struct remote {
struct refspec fetch;
+ /*
-+ * This setting for whether to prefetch from a remote
++ * 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
@@ t/t7900-maintenance.sh: test_expect_success 'prefetch multiple remotes' '
+ # 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 &&
++ # 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 &&Documentation/config/remote.txt | 5 ++++ Documentation/git-maintenance.txt | 7 +++--- builtin/gc.c | 3 +++ remote.c | 3 +++ remote.h | 9 +++++++ t/t7900-maintenance.sh | 42 +++++++++++++++++++++++++++++++ 6 files changed, 66 insertions(+), 3 deletions(-)
Documentation/config/remote.txt +5 -0
@@ -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].
Documentation/git-maintenance.txt +4 -3
@@ -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,
builtin/gc.c +3 -0
@@ -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",
remote.c +3 -0
@@ -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);
remote.h +9 -0
@@ -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);
t/t7900-maintenance.sh +42 -0
48 lines not on this page · raw message
base-commit: 2e7b89e038c0c888acf61f1b4ee5a43d4dd5e94c
Signature · 1 line
gitgitgadget
builtin/gc.c static int fetch_remote(struct remote *remote, void *cbdata) · line 1031
Quoted 10 lines · Shubham Kanodia via GitGitGadget
builtin/gc.c +2
-0@@ -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.
remote.c static struct remote *make_remote(struct remote_state *remote_state, · line 143
Quoted 9 lines
remote.c +1
-0@@ -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...
remote.c static int handle_config(const char *key, const char *value, · line 461
Quoted 9 lines
remote.c +2
-0ret->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.
Quoted 6 lines
remote.h +0
-0@@ -77,6 +77,15 @@ struct remote {
remote.h struct remote { · line 87
Quoted 10 lines
remote.h +8
-0struct 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.
Quoted 92 lines · Junio C Hamano
builtin/gc.c static int fetch_remote(struct remote *remote, void *cbdata) · line 1031
Quoted 10 lines · Shubham Kanodia via GitGitGadget
builtin/gc.c +2
-0@@ -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.
remote.c static struct remote *make_remote(struct remote_state *remote_state, · line 143
Quoted 9 lines
remote.c +1
-0@@ -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...
remote.c static int handle_config(const char *key, const char *value, · line 461
Quoted 9 lines
remote.c +2
-0ret->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.
Quoted 6 lines
remote.h +0
-0@@ -77,6 +77,15 @@ struct remote {remote.h struct remote { · line 87
Quoted 10 lines
remote.h +8
-0struct 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.
Quoted 4 lines
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.
Quoted 6 lines · Shubham Kanodia
Quoted 3 lines
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.
Quoted 4 lines · Junio C Hamano
Quoted 1 line · Shubham Kanodia
Quoted 1 line
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 1 line
Thanks.
Quoted 7 lines · Shubham Kanodia
Quoted 6 lines · Junio C Hamano
Quoted 3 lines · Shubham Kanodia
Quoted 3 lines
int prefetch; /* is prefetch enabled? */
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
...
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.
PATCH v3 1/1 remote: prefetch config
In reply to Shubham Kanodia via GitGitGadgetFrom: Shubham Kanodia <[email protected]>
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.
In a future change, we could also allow restricting the refs that are prefetched per remote using the prefetchref config option per remote.
Both of these options in unison would allow users to optimize their prefetch operations, reducing network traffic and disk usage.
---
remote: prefetch config
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1779%2Fpastelsky%2Fsk%2Fmaintenance-prefetch-remote-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1779/pastelsky/sk/maintenance-prefetch-remote-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1779
Range-diff vs v2:
1: 1d58b782e22 ! 1: c348f8efd33 remote: prefetch config
@@ remote.c: 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->prefetch = 1;
ret->name = xstrndup(name, len);
refspec_init(&ret->push, REFSPEC_PUSH);
refspec_init(&ret->fetch, REFSPEC_FETCH);
@@ remote.h: 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;
+
/*Documentation/config/remote.txt | 5 ++++ Documentation/git-maintenance.txt | 7 +++--- builtin/gc.c | 3 +++ remote.c | 3 +++ remote.h | 2 ++ t/t7900-maintenance.sh | 42 +++++++++++++++++++++++++++++++ 6 files changed, 59 insertions(+), 3 deletions(-)
Documentation/config/remote.txt +5 -0
@@ -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].
Documentation/git-maintenance.txt +4 -3
@@ -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,
builtin/gc.c +3 -0
@@ -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",
remote.c +3 -0
@@ -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 ; 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);
remote.h +2 -0
@@ -77,6 +77,8 @@ struct remote {struct refspec fetch; +int prefetch; +/* * The setting for whether to fetch tags (as a separate rule from the * configured refspecs);
t/t7900-maintenance.sh +42 -0
48 lines not on this page · raw message
base-commit: 2e7b89e038c0c888acf61f1b4ee5a43d4dd5e94c
Signature · 1 line
gitgitgadget
PATCH v4 1/1 remote: prefetch config
From: Shubham Kanodia <[email protected]>
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.
In a future change, we could also allow restricting the refs that are prefetched per remote using the prefetchref config option per remote.
Both of these options in unison would allow users to optimize their prefetch operations, reducing network traffic and disk usage.
---
remote: prefetch config
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1779%2Fpastelsky%2Fsk%2Fmaintenance-prefetch-remote-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1779/pastelsky/sk/maintenance-prefetch-remote-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1779
Range-diff vs v3:
1: c348f8efd33 ! 1: 80af121f835 remote: prefetch config
@@ builtin/gc.c: static int fetch_remote(struct remote *remote, void *cbdata)
if (remote->skip_default_update)
return 0;
-+ if (!remote->prefetch)
++ if (!remote->prefetch_enabled)
+ return 0;
+
child.git_cmd = 1;
@@ remote.c: 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;
++ ret->prefetch_enabled = 1;
ret->name = xstrndup(name, len);
refspec_init(&ret->push, REFSPEC_PUSH);
refspec_init(&ret->fetch, REFSPEC_FETCH);
@@ remote.c: static int handle_config(const char *key, const char *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);
++ remote->prefetch_enabled = git_config_bool(key, value);
else if (!strcmp(subkey, "url")) {
if (!value)
return config_error_nonbool(key);
@@ remote.h: struct remote {
struct refspec fetch;
-+ int prefetch;
++ int prefetch_enabled;
+
/*
* The setting for whether to fetch tags (as a separate rule from theDocumentation/config/remote.txt | 5 ++++ Documentation/git-maintenance.txt | 7 +++--- builtin/gc.c | 3 +++ remote.c | 3 +++ remote.h | 2 ++ t/t7900-maintenance.sh | 42 +++++++++++++++++++++++++++++++ 6 files changed, 59 insertions(+), 3 deletions(-)
Documentation/config/remote.txt +5 -0
@@ -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].
Documentation/git-maintenance.txt +4 -3
@@ -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,
builtin/gc.c +3 -0
@@ -1027,6 +1027,9 @@ static int fetch_remote(struct remote *remote, void *cbdata)if (remote->skip_default_update)return 0 ; +if (!remote->prefetch_enabled) +return 0 ; + child.git_cmd =1 ; strvec_pushl(&child.args, "fetch", remote->name, "--prefetch", "--prune", "--no-tags",
remote.c +3 -0
@@ -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_enabled =1 ; 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_enabled =git_config_bool (key, value); else if (!strcmp(subkey, "url")) {if (!value)return config_error_nonbool (key);
remote.h +2 -0
@@ -77,6 +77,8 @@ struct remote {struct refspec fetch; +int prefetch_enabled; +/* * The setting for whether to fetch tags (as a separate rule from the * configured refspecs);
t/t7900-maintenance.sh +42 -0
48 lines not on this page · raw message
base-commit: 2e7b89e038c0c888acf61f1b4ee5a43d4dd5e94c
Signature · 1 line
gitgitgadget
Quoted 10 lines · Shubham Kanodia via GitGitGadget
From: Shubham Kanodia <[email protected]>
...
In a future change, we could also allow restricting the refs that are prefetched per remote using theprefetchrefconfig option per remote.Both of these options in unison would allow users to optimize their prefetch operations, reducing network traffic and disk usage.
Signed-off-by Shubham Kanodia <[email protected]>---
Looking good. Thanks.
Quoted 8 lines · Junio C Hamano
Quoted 5 lines · Shubham Kanodia via GitGitGadget
From: Shubham Kanodia <[email protected]>
...
In a future change, we could also allow restricting the refs that are prefetched per remote using theprefetchrefconfig option per remote.
Quoted 7 lines
Quoted 5 lines
Both of these options in unison would allow users to optimize their prefetch operations, reducing network traffic and disk usage.
Signed-off-by Shubham Kanodia <[email protected]>---
Looking good. Thanks.
How long do you reckon changes like this typically remain in "seen" until merged upstream?
I'm preparing part-2 of this change separately — so would be good to know when I can submit that.