Re: [PATCH v2 1/2] maintenance: add 'unregister --force'

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 26, 2022, 15:39 UTC
Message-ID: <220926.86mtamyvo7.gmgdl@evledraar.gmail.com>
In-Reply-To: <fc6739c3-2f7c-5dab-e4a2-8243deafde50@github.com>

On Mon, Sep 26 2022, Derrick Stolee wrote:

Show 18 quoted lines

> On 9/23/2022 9:08 AM, SZEDER Gábor wrote:
>> On Thu, Sep 22, 2022 at 01:37:16PM +0000, Derrick Stolee via GitGitGadget wrote:
>>>  static int maintenance_unregister(int argc, const char **argv, const char *prefix)
>>>  {
>>> +	int force = 0;
>>>  	struct option options[] = {
>>> +		OPT_BOOL(0, "force", &force,
>>> +			 N_("return success even if repository was not registered")),
>> 
>> This could be shortened a bit by using OPT__FORCE() instead of
>> OPT_BOOL().  OTOH, please make it a bit longer, and declare the option
>> with the PARSE_OPT_NOCOMPLETE flag to hide it from completion:
>
> Looks like I can do both like this:
>
> 		OPT__FORCE(&force,
> 			   N_("return success even if repository was not registered"),
> 			   PARSE_OPT_NOCOMPLETE),

I don't think PARSE_OPT_NOCOMPLETE is appropriate here. Yes we use it for most of --force, but in some non-destructive cases (e.g. "add") we don't.

This seems to be such a case, we'll destroy no data or do anything irrecoverable. It's really just a --do-not-be-so-anal-about-your-exit-code option.

I'm guessing that you wanted to be able to error check this more strictly in some cases. For "git remote" I post-hoc filled in this use-case by exiting with a code of 2 on missing remotes on e.g. "git remote remove", see: 9144ba4cf52 (remote: add meaningful exit code on missing/existing, 2020-10-27).

In this case we would return exit code 5 for this in most case before,
as we wouldn't be able to find the key, wouldn't we? I.e. the return
value from "git config". Seems so:
	
	$ GIT_TRACE=1 git maintenance unregister; echo $?
	17:48:59.984529 exec-cmd.c:90           trace: resolved executable path from procfs: /home/avar/local/bin/git
	17:48:59.984566 exec-cmd.c:237          trace: resolved executable dir: /home/avar/local/bin
	17:48:59.986795 git.c:509               trace: built-in: git maintenance unregister
	17:48:59.986817 run-command.c:654       trace: run_command: git config --global --unset --fixed-value maintenance.repo /home/avar/g/git
	17:48:59.987532 exec-cmd.c:90           trace: resolved executable path from procfs: /home/avar/local/bin/git
	17:48:59.987561 exec-cmd.c:237          trace: resolved executable dir: /home/avar/local/bin
	17:48:59.988733 git.c:509               trace: built-in: git config --global --unset --fixed-value maintenance.repo /home/avar/g/git
	5

Maybe we want to just define an exit code here too? I think doing so is a better interface, the user can then pipe the STDERR to /dev/null themselves (or equivalent).

Aside from anything else, I think this would be much clearer if it were split up so that:

 * We first test what we do now without --force, which is clearly
   untested and undocumented (you are adding tests for it here
   while-at-it)

 * This commit, which adds a --force.

Also:

Show 13 quoted lines

> @@ -1538,11 +1545,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>  		usage_with_options(builtin_maintenance_unregister_usage,
>  				   options);
>  
> -	config_unset.git_cmd = 1;
> -	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
> -		     "--fixed-value", "maintenance.repo", maintpath, NULL);
> +	for_each_string_list_item(item, list) {
> +		if (!strcmp(maintpath, item->string)) {
> +			found = 1;
> +			break;
> +		}
> +	}

This code now has a race condition it didn't before. Before we just did a "git config --unset" which would have locked the config file, so if we didn't have a key we'd return 5.

> +	if (found) {

But here we looked for the key *earlier*, so in that window we could have raced and had the key again, so ....

Show 6 quoted lines

> +		config_unset.git_cmd = 1;
> +		strvec_pushl(&config_unset.args, "config", "--global", "--unset",
> +			     "--fixed-value", key, maintpath, NULL);
> +
> +		rc = run_command(&config_unset);
> +	} else if (!force) {

...found would not be true, and if you you didn't have --force...

> +		die(_("repository '%s' is not registered"), maintpath);
> +	}
>  
> -	rc = run_command(&config_unset);

...this removal would cause us to still have the key in the end, no? I.e.:

 1. We check if the key is there
 2. Another process LOCKS config
 3. Another process SETS the key
 4. Another process UNLOCKS config
 5. We act with the assumption that the key isn't set

Maybe it's not racy, or it doesn't matter.