Re: ds/maintenance-part-3 (was Re: What's cooking in git.git (Nov 2020, #02; Mon, 9))

From: Emily Shaffer <emilyshaffer@google.com>
Date: Nov 16, 2020, 23:56 UTC
Message-ID: <20201116235642.GA15562@google.com>
In-Reply-To: <d0123439-236c-1a62-294b-a3373465eadb@gmail.com>

On Tue, Nov 10, 2020 at 08:31:35AM -0500, Derrick Stolee wrote:

Show 27 quoted lines

> 
> On 11/9/2020 6:42 PM, Junio C Hamano wrote:
> > * ds/maintenance-part-3 (2020-10-16) 8 commits
> >  - maintenance: add troubleshooting guide to docs
> >  - maintenance: use 'incremental' strategy by default
> >  - maintenance: create maintenance.strategy config
> >  - maintenance: add start/stop subcommands
> >  - maintenance: add [un]register subcommands
> >  - for-each-repo: run subcommands on configured repos
> >  - maintenance: add --schedule option and config
> >  - maintenance: optionally skip --auto process
> >  (this branch is used by ds/maintenance-part-4.)
> > 
> >  Parts of "git maintenance" to ease writing crontab entries (and
> >  other scheduling system configuration) for it.
> > 
> >  Will merge to 'next'?
> 
> This hasn't changed for a while, and part-4 is also sitting
> after some initial feedback. I'd love for others to test
> these background maintenance activities in their home forks.
> I certainly have on all three platforms.
> 
> Perhaps part-3 could merge to 'next' soonish? I know we
> discussed only merging part-3 and part-4 to 'master' at the
> same time, but it would be good to have part-3 start cooking
> in 'next' sooner than later, right?

Hiya, we found a bug in part-3 today!

Because 'git maintenance unregister' spins a child process to call 'git config --unset maintenance.repo <cwd>', it actually fails if "cwd" contains a POSIX regular expression special character:

  git config [<file-option>] --unset name [value_regex]

You can demo it for yourself like so:

  git init repro+for+maintenance
  git maintenance register
  git maintenance unregister
  echo $?	# returns '5'
  git config --list --global

I see two paths forward:

1. Teach 'git config' to learn either which regex parser to use
(including fixed), or at least to learn "value isn't a regex", or

2. Don't spin a child process in 'git maintenance [un]register' and
instead just call the config API.

I'd suggest #2. The config API is very nice, and seems to have a simple way to add or remove configs to your global file in just a couple of lines. If there's a reason why it's not simpler to do it that way, it's my fault for missing the review :)

 - Emily