On 9/26/2022 11:39 AM, Ævar Arnfjörð Bjarmason wrote:

On Mon, Sep 26 2022, Derrick Stolee wrote:

quoted

On 9/23/2022 9:08 AM, SZEDER Gábor wrote:

quoted

On Thu, Sep 22, 2022 at 01:37:16PM +0000, Derrick Stolee via GitGitGadget wrote:

quoted

 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.

quoted

+	if (found) {

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

quoted

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

quoted

+		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