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'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:
> @@ -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 ....
> + 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.
