| Message ID | 69c74f52eefd906c38494759a02e137e4d7c01d8.1663853837.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scalar: make unregister idempotent | expand |
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: '--force' is usually used to enable something potentially dangerous, and therefore should be used with care, so our completion script in general doesn't offer it, giving the users more opportunity to think things through while typing it out. Though, arguably in this particular case it seems there is not much danger to be afraid of. > @@ -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; > + } > + } > + > + if (found) { > + config_unset.git_cmd = 1; > + strvec_pushl(&config_unset.args, "config", "--global", "--unset", > + "--fixed-value", key, maintpath, NULL); > + > + rc = run_command(&config_unset); It seems a bit heavy-handed to fork() an extra git process just to unset a config variable. Wouldn't a properly parametrized git_config_set_multivar_in_file_gently() call be sufficient? Though TBH I don't offhand know what those parameters should be, in particular whether there is a convenient way to find out the path of the global config file in order to pass it to this function. (Not an issue with this patch, as the context shows that this has been done with the extra process before; it just caught my eye.)
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), >> @@ -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; >> + } >> + } >> + >> + if (found) { >> + config_unset.git_cmd = 1; >> + strvec_pushl(&config_unset.args, "config", "--global", "--unset", >> + "--fixed-value", key, maintpath, NULL); >> + >> + rc = run_command(&config_unset); > > It seems a bit heavy-handed to fork() an extra git process just to > unset a config variable. Wouldn't a properly parametrized > git_config_set_multivar_in_file_gently() call be sufficient? Though > TBH I don't offhand know what those parameters should be, in > particular whether there is a convenient way to find out the path of > the global config file in order to pass it to this function. > > (Not an issue with this patch, as the context shows that this has been > done with the extra process before; it just caught my eye.) Thanks. I'll look into it. -Stolee
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.
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
Derrick Stolee <derrickstolee@github.com> writes: > 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 am perfectly OK with that. Given that "git reset --hard" is not given nocomplete option, I do not see much point in "destructive ones are not completed" guideline in practice anyway. After all, "add --force" would be destructively removing the object name recorded for the path previously. Thanks for carefully thinking the UI remifications through.
diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt index 9c630efe19c..bb888690e4d 100644 --- a/Documentation/git-maintenance.txt +++ b/Documentation/git-maintenance.txt @@ -11,7 +11,7 @@ SYNOPSIS [verse] 'git maintenance' run [<options>] 'git maintenance' start [--scheduler=<scheduler>] -'git maintenance' (stop|register|unregister) +'git maintenance' (stop|register|unregister) [<options>] DESCRIPTION @@ -79,6 +79,10 @@ unregister:: Remove the current repository from background maintenance. This only removes the repository from the configured list. It does not stop the background maintenance processes from running. ++ +The `unregister` subcommand will report an error if the current repository +is not already registered. Use the `--force` option to return success even +when the current repository is not registered. TASKS ----- diff --git a/builtin/gc.c b/builtin/gc.c index 2753bd15a5e..3189b4ba2b1 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1519,18 +1519,25 @@ done: } static char const * const builtin_maintenance_unregister_usage[] = { - "git maintenance unregister", + "git maintenance unregister [--force]", NULL }; 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")), OPT_END(), }; - int rc; + const char *key = "maintenance.repo"; + int rc = 0; struct child_process config_unset = CHILD_PROCESS_INIT; char *maintpath = get_maintpath(); + int found = 0; + struct string_list_item *item; + const struct string_list *list = git_config_get_value_multi(key); argc = parse_options(argc, argv, prefix, options, builtin_maintenance_unregister_usage, 0); @@ -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; + } + } + + if (found) { + 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) { + die(_("repository '%s' is not registered"), maintpath); + } - rc = run_command(&config_unset); free(maintpath); return rc; } diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 2724a44fe3e..cfe3236567a 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -493,7 +493,11 @@ test_expect_success 'register and unregister' ' git maintenance unregister && git config --global --get-all maintenance.repo >actual && - test_cmp before actual + test_cmp before actual && + + test_must_fail git maintenance unregister 2>err && + grep "is not registered" err && + git maintenance unregister --force ' test_expect_success !MINGW 'register and unregister with regex metacharacters' '