diff mbox series

[v4,4/4] maintenance: respect remote.*.skipFetchAll

Message ID 92652fd9e6e17654abdb30625c85937b6b56c38e.1618577399.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 32f67888d85bd0af30b857a29ca25a55999b6d01
Headers show
Series Maintenance: adapt custom refspecs | expand

Commit Message

Derrick Stolee April 16, 2021, 12:49 p.m. UTC 
From: Derrick Stolee <dstolee@microsoft.com>

If a remote has the skipFetchAll setting enabled, then that remote is
not intended for frequent fetching. It makes sense to not fetch that
data during the 'prefetch' maintenance task. Skip that remote in the
iteration without error. The skip_default_update member is initialized
in remote.c:handle_config() as part of initializing the 'struct remote'.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/gc.c           | 3 +++
 t/t7900-maintenance.sh | 8 +++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

Comments

Ævar Arnfjörð Bjarmason April 16, 2021, 1:54 p.m. UTC | #1 
On Fri, Apr 16 2021, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> If a remote has the skipFetchAll setting enabled, then that remote is
> not intended for frequent fetching. It makes sense to not fetch that
> data during the 'prefetch' maintenance task. Skip that remote in the
> iteration without error. The skip_default_update member is initialized
> in remote.c:handle_config() as part of initializing the 'struct remote'.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/gc.c           | 3 +++
>  t/t7900-maintenance.sh | 8 +++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 9d35f7da50d8..98a803196b88 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -878,6 +878,9 @@ static int fetch_remote(struct remote *remote, void *cbdata)
>  	struct maintenance_run_opts *opts = cbdata;
>  	struct child_process child = CHILD_PROCESS_INIT;
>  
> +	if (remote->skip_default_update)
> +		return 0;
> +
>  	child.git_cmd = 1;
>  	strvec_pushl(&child.args, "fetch", remote->name,
>  		     "--prefetch", "--prune", "--no-tags",
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index eadb800c08cc..b93ae014ee58 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -153,7 +153,13 @@ test_expect_success 'prefetch multiple remotes' '
>  
>  	test_cmp_config refs/prefetch/ log.excludedecoration &&
>  	git log --oneline --decorate --all >log &&
> -	! grep "prefetch" log
> +	! grep "prefetch" log &&
> +
> +	test_when_finished git config --unset remote.remote1.skipFetchAll &&
> +	git config remote.remote1.skipFetchAll true &&
> +	GIT_TRACE2_EVENT="$(pwd)/skip-remote1.txt" git maintenance run --task=prefetch 2>/dev/null &&
> +	test_subcommand ! git fetch remote1 $fetchargs <skip-remote1.txt &&
> +	test_subcommand git fetch remote2 $fetchargs <skip-remote1.txt
>  '
>  
>  test_expect_success 'prefetch and existing log.excludeDecoration values' '

Without having read the code I'd have very much expected a
"remote.*.skipFetchAll" to impact:

    git fetch --all

Or:

    git remote update --all # --all does not exist yet

As e.g. remote.<name>.skipDefaultUpdate would do (i.e. impact "git
remote update" ...).

I suspect naming it like this started as a hack around the lack of
4-level .ini config keys, i.e. so we could do:

    maintenance.remote.<name>.skipFetchAll = true

But I wonder if we couldn't give this a less confusing name still, even
the pseudo-command form of:

    maintenanceRemote.<name>.skipFetchAll = true

Or something...
Tom Saeger April 16, 2021, 2:33 p.m. UTC | #2 
On Fri, Apr 16, 2021 at 03:54:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Apr 16 2021, Derrick Stolee via GitGitGadget wrote:
> 
> > From: Derrick Stolee <dstolee@microsoft.com>
> >
> > If a remote has the skipFetchAll setting enabled, then that remote is
> > not intended for frequent fetching. It makes sense to not fetch that
> > data during the 'prefetch' maintenance task. Skip that remote in the
> > iteration without error. The skip_default_update member is initialized
> > in remote.c:handle_config() as part of initializing the 'struct remote'.
> >
> > Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> > ---
> >  builtin/gc.c           | 3 +++
> >  t/t7900-maintenance.sh | 8 +++++++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index 9d35f7da50d8..98a803196b88 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -878,6 +878,9 @@ static int fetch_remote(struct remote *remote, void *cbdata)
> >  	struct maintenance_run_opts *opts = cbdata;
> >  	struct child_process child = CHILD_PROCESS_INIT;
> >  
> > +	if (remote->skip_default_update)
> > +		return 0;
> > +
> >  	child.git_cmd = 1;
> >  	strvec_pushl(&child.args, "fetch", remote->name,
> >  		     "--prefetch", "--prune", "--no-tags",
> > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> > index eadb800c08cc..b93ae014ee58 100755
> > --- a/t/t7900-maintenance.sh
> > +++ b/t/t7900-maintenance.sh
> > @@ -153,7 +153,13 @@ test_expect_success 'prefetch multiple remotes' '
> >  
> >  	test_cmp_config refs/prefetch/ log.excludedecoration &&
> >  	git log --oneline --decorate --all >log &&
> > -	! grep "prefetch" log
> > +	! grep "prefetch" log &&
> > +
> > +	test_when_finished git config --unset remote.remote1.skipFetchAll &&
> > +	git config remote.remote1.skipFetchAll true &&
> > +	GIT_TRACE2_EVENT="$(pwd)/skip-remote1.txt" git maintenance run --task=prefetch 2>/dev/null &&
> > +	test_subcommand ! git fetch remote1 $fetchargs <skip-remote1.txt &&
> > +	test_subcommand git fetch remote2 $fetchargs <skip-remote1.txt
> >  '
> >  
> >  test_expect_success 'prefetch and existing log.excludeDecoration values' '
> 
> Without having read the code I'd have very much expected a
> "remote.*.skipFetchAll" to impact:
> 
>     git fetch --all

'skipFetchAll' indeed impacts 'git fetch --all'

But this patch doesn't add "skipFetchAll", instead it just honors that
config if set during 'git maintenance --task=prefetch'

See v3 discussion: https://lore.kernel.org/git/2f4fa2b5-0d8b-b368-ab4d-411740595a4f@gmail.com/

> 
> Or:
> 
>     git remote update --all # --all does not exist yet
> 
> As e.g. remote.<name>.skipDefaultUpdate would do (i.e. impact "git
> remote update" ...).
> 
> I suspect naming it like this started as a hack around the lack of
> 4-level .ini config keys, i.e. so we could do:
> 
>     maintenance.remote.<name>.skipFetchAll = true
> 
> But I wonder if we couldn't give this a less confusing name still, even
> the pseudo-command form of:
> 
>     maintenanceRemote.<name>.skipFetchAll = true
> 
> Or something...
> 

Whether or not additional maintenance specific configs are desired, that might be
something to consider.  I've thought about this for a few of my repos
which have remotes which require a VPN connection.  Perhaps
I want to skip those during 'prefetch'?  Or maybe instead define a
'remotes.prefetch' group and only prefetch remotes listed?

That all said - I wouldn't hold-up this patch series for it.

--Tom
Tom Saeger April 16, 2021, 6:31 p.m. UTC | #3 
On Fri, Apr 16, 2021 at 12:49:59PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> If a remote has the skipFetchAll setting enabled, then that remote is
> not intended for frequent fetching. It makes sense to not fetch that
> data during the 'prefetch' maintenance task. Skip that remote in the
> iteration without error. The skip_default_update member is initialized
> in remote.c:handle_config() as part of initializing the 'struct remote'.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

Reviewed-by: Tom Saeger <tom.saeger@oracle.com>
> ---
>  builtin/gc.c           | 3 +++
>  t/t7900-maintenance.sh | 8 +++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 9d35f7da50d8..98a803196b88 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -878,6 +878,9 @@ static int fetch_remote(struct remote *remote, void *cbdata)
>  	struct maintenance_run_opts *opts = cbdata;
>  	struct child_process child = CHILD_PROCESS_INIT;
>  
> +	if (remote->skip_default_update)
> +		return 0;
> +

Well that is way easier than doing it in 'fetch'.


>  	child.git_cmd = 1;
>  	strvec_pushl(&child.args, "fetch", remote->name,
>  		     "--prefetch", "--prune", "--no-tags",
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index eadb800c08cc..b93ae014ee58 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -153,7 +153,13 @@ test_expect_success 'prefetch multiple remotes' '
>  
>  	test_cmp_config refs/prefetch/ log.excludedecoration &&
>  	git log --oneline --decorate --all >log &&
> -	! grep "prefetch" log
> +	! grep "prefetch" log &&
> +
> +	test_when_finished git config --unset remote.remote1.skipFetchAll &&
> +	git config remote.remote1.skipFetchAll true &&
> +	GIT_TRACE2_EVENT="$(pwd)/skip-remote1.txt" git maintenance run --task=prefetch 2>/dev/null &&
> +	test_subcommand ! git fetch remote1 $fetchargs <skip-remote1.txt &&
> +	test_subcommand git fetch remote2 $fetchargs <skip-remote1.txt
>  '
>  
>  test_expect_success 'prefetch and existing log.excludeDecoration values' '
> -- 
> gitgitgadget
diff mbox series

Patch

diff --git a/builtin/gc.c b/builtin/gc.c
index 9d35f7da50d8..98a803196b88 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -878,6 +878,9 @@  static int fetch_remote(struct remote *remote, void *cbdata)
 	struct maintenance_run_opts *opts = cbdata;
 	struct child_process child = CHILD_PROCESS_INIT;
 
+	if (remote->skip_default_update)
+		return 0;
+
 	child.git_cmd = 1;
 	strvec_pushl(&child.args, "fetch", remote->name,
 		     "--prefetch", "--prune", "--no-tags",
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index eadb800c08cc..b93ae014ee58 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -153,7 +153,13 @@  test_expect_success 'prefetch multiple remotes' '
 
 	test_cmp_config refs/prefetch/ log.excludedecoration &&
 	git log --oneline --decorate --all >log &&
-	! grep "prefetch" log
+	! grep "prefetch" log &&
+
+	test_when_finished git config --unset remote.remote1.skipFetchAll &&
+	git config remote.remote1.skipFetchAll true &&
+	GIT_TRACE2_EVENT="$(pwd)/skip-remote1.txt" git maintenance run --task=prefetch 2>/dev/null &&
+	test_subcommand ! git fetch remote1 $fetchargs <skip-remote1.txt &&
+	test_subcommand git fetch remote2 $fetchargs <skip-remote1.txt
 '
 
 test_expect_success 'prefetch and existing log.excludeDecoration values' '