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
