From: Tom Saeger <tom.saeger@oracle.com>
Date: Fri, 16 Apr 2021 14:33:17 GMT
Subject: Re: [PATCH v4 4/4] maintenance: respect remote.*.skipFetchAll
Message-ID: <20210416143317.hoswhvk7ivdg6z7v@dhcp-10-154-163-120.vpn.oracle.com>
In-Reply-To: <87zgxytjwa.fsf@evledraar.gmail.com>

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