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

[Derrick Stolee <derrickstolee@github.com>]

On 9/26/2022 11:39 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Sep 26 2022, Derrick Stolee wrote:
> 
>> 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 agree, so I wasn't completely sold on PARSE_OPT_NOCOMPLETE. I'll use
your vote to not add that 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).

Generally, I'm not terribly interested in the exit code value other
than 0, 128, and <otherwise non-zero> being three categories. I definitely
don't want to create a strict list of exit code modes here.

> 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".

We definitely inherited an exit code from 'git config' here, but
I should probably convert it into a die() message to make it clearer.

> 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 ....
> 
>> +		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

I think this list of events is fine, since we check the value and then
do not try to modify state if it isn't there.

Instead if we had this scenario:

 1. We check and see that it _is_there.
 2. Another process modifies config to remove the value.
 3. We try to remove the value and fail.

In this case, the --force option would still fail even though the end
result is the same. We could ignore a "not found" case during a --force
option to avoid failing due to such concurrency.
 
> Maybe it's not racy, or it doesn't matter.
Generally, I think in this case it doesn't matter, but we can be a bit
more careful about handling races.

Thanks,
-Stolee