scalar: make unregister idempotent #1358
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.1358.git.1663635732095.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Derrick Stolee <derrickstolee@github.com>
>
> The 'git maintenance unregister' subcommand has a step that removes the
> current repository from the multi-valued maitenance.repo config key.
> This fails if the repository is not listed in that key. This makes
> running 'git maintenance unregister' twice result in a failure in the
> second instance.
>
> Make this task idempotent, since the end result is the same in both
> cases: maintenance will no longer run on this repository.
I am not sure if this is a good idea. What is the ultimate reason
why we want to allow running it blindly without knowing if it is
necessary? Is it because there is no easy way to tell if unregister
is needed in the first place?
If this were "unregister X" that takes a parameter that tells what
to unregister, I would be vehemently opposed to the idea, because
I'd expect people make typos and want to be reminded of them when
they happen, but ...
> git maintenance unregister &&
> git config --global --get-all maintenance.repo >actual &&
> - test_cmp before actual
> + test_cmp before actual &&
> +
> + # Expect unregister to be idempotent.
> + git maintenance unregister
> '
... given that the user does not have any control over what to
unregister from the command line (it is decided implicitly by where
the user is), I am halfway OK with it.
A user with two repositories may still be upset after running
"unregister" in the one they did not mean to, and not getting told
about the mistake, though, e.g.
$ ls *.git
a.git b.git
$ cd a.git
$ git maintenance unregister
$ cd b.git
... this would give you an error message ...
$ git maintenance unregister
... this is silent with the change ...
Surely, the second "cd" should be to ../b.git instead of b.git and
surely the user should have noticed that "cd" gave them an error,
but the unregister that complains would be an extra chance for them
to notice the mistake when they wanted to unregister the maintenance
tasks from all their repositories.
I wonder if something like the "--force" option, i.e.
$ >cruft
$ rm curft; echo $?
rm: cannot remove 'curft': No such file or directory
1
$ rm cruft; echo $?
0
$ rm cruft; echo $?
rm: cannot remove 'cruft': No such file or directory
1
$ rm -f cruft; echo $?
0
which gives us both typo detection while supporting blind removal,
is a usable workaround? I dunno.
But as I said, I am halfway OK with the change. I just think it is
a bit unfriendly to the users.
Thanks. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@f982f31. |
|
This patch series was integrated into seen via git@4bd3fd6. |
|
This patch series was integrated into seen via git@47ab142. |
|
There was a status update in the "New Topics" section about the branch "git maintenance unregister" in a repository that is already been unregistered reported an error. Somewhat dubious? cf. <xmqqpmfo4pc7.fsf@gitster.g> source: <pull.1358.git.1663635732095.gitgitgadget@gmail.com> |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 9/21/2022 1:19 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> The 'git maintenance unregister' subcommand has a step that removes the
>> current repository from the multi-valued maitenance.repo config key.
>> This fails if the repository is not listed in that key. This makes
>> running 'git maintenance unregister' twice result in a failure in the
>> second instance.
>>
>> Make this task idempotent, since the end result is the same in both
>> cases: maintenance will no longer run on this repository.
>
> I am not sure if this is a good idea. What is the ultimate reason
> why we want to allow running it blindly without knowing if it is
> necessary? Is it because there is no easy way to tell if unregister
> is needed in the first place?
We want to leave the internal details of what it means to be
registered as hidden to the user. They could look for the repo in
the global config, but that seems like a hassle when they just
want to make sure they are not currently registered.
>> + # Expect unregister to be idempotent.
>> + git maintenance unregister
>> '
>
> ... given that the user does not have any control over what to
> unregister from the command line (it is decided implicitly by where
> the user is), I am halfway OK with it.
>
> A user with two repositories may still be upset after running
> "unregister" in the one they did not mean to, and not getting told
> about the mistake, though, e.g.
...
> I wonder if something like the "--force" option,
I like the --force option. I'll add it in v2 and then update Scalar
to use that option.
Thanks,
-Stolee |
derrickstolee
force-pushed
the
maintenance-unregister
branch
from
f028208
to
f5d8d6e
Compare
derrickstolee
changed the title
|
/submit |
|
Submitted as pull.1358.v2.git.1663853837.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Derrick Stolee <derrickstolee@github.com> writes:
>> I am not sure if this is a good idea. What is the ultimate reason
>> why we want to allow running it blindly without knowing if it is
>> necessary? Is it because there is no easy way to tell if unregister
>> is needed in the first place?
>
> We want to leave the internal details of what it means to be
> registered as hidden to the user. They could look for the repo in
> the global config, but that seems like a hassle when they just
> want to make sure they are not currently registered.
OK, so there is no published officially sanctioned way to ask "is
this repository under maintenance's control and cron jobs run in
it?" or "give me the list of such repositories".
Then I can see why you want to allow users to blindly run
"unregister", with or without "--force".
But doesn't it point at a more fundamental problem?
Is there a reason why we want to hide the list of repositories
(enlistments?) from the users?
|
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 9/22/2022 3:31 PM, Junio C Hamano wrote:
> Derrick Stolee <derrickstolee@github.com> writes:
>
>>> I am not sure if this is a good idea. What is the ultimate reason
>>> why we want to allow running it blindly without knowing if it is
>>> necessary? Is it because there is no easy way to tell if unregister
>>> is needed in the first place?
>>
>> We want to leave the internal details of what it means to be
>> registered as hidden to the user. They could look for the repo in
>> the global config, but that seems like a hassle when they just
>> want to make sure they are not currently registered.
>
> OK, so there is no published officially sanctioned way to ask "is
> this repository under maintenance's control and cron jobs run in
> it?" or "give me the list of such repositories".
>
> Then I can see why you want to allow users to blindly run
> "unregister", with or without "--force".
>
> But doesn't it point at a more fundamental problem?
>
> Is there a reason why we want to hide the list of repositories
> (enlistments?) from the users?
I don't think we want to hide it, but we've never needed to present
the list in a canonical way before. It's been sufficient to let
users run 'git config --global --get-all maintenance.repo', assuming
they know that config key is the important one.
Adding a 'git maintenance list-registered' or something would solve
that problem, but I'm not sure it is a super high priority.
Thanks,
-Stolee |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Derrick Stolee <derrickstolee@github.com> writes:
> Adding a 'git maintenance list-registered' or something would solve
> that problem, but I'm not sure it is a super high priority.
Is that "git maintenance" or "scalar" that list-registered would
hang below?
In any case, I tend to think the word "idempotent" is used as a
rough synonym to "sloppy", but unregistering from the automatic
maintenance (but still known as part of enlistment?) would probably
be a rare event that would not be a huge deal if the user failed to
do so without getting reminded, so I would not oppose to the step
[2/2] of the updated series.
Thanks, queued. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@0859faa. |
|
This patch series was integrated into seen via git@7f4916a. |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -11,7 +11,7 @@ SYNOPSIS | |||
| [verse] | |||
There was a problem hiding this comment.
On the Git mailing list, SZEDER Gábor wrote (reply to this):
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.)
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
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.
-StoleeThere was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
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.There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
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,
-StoleeThere was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
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.
|
User |
|
This patch series was integrated into seen via git@aee1ad6. |
|
This patch series was integrated into seen via git@05e0c92. |
|
There was a status update in the "New Topics" section about the branch "git maintenance unregister" in a repository that is already been unregistered reported an error. Will merge to 'next'? source: <pull.1358.v2.git.1663853837.gitgitgadget@gmail.com> |
derrickstolee
force-pushed
the
maintenance-unregister
branch
from
f5d8d6e
to
ea53500
Compare
|
User |
derrickstolee
force-pushed
the
maintenance-unregister
branch
2 times, most recently
from
043f0b5
to
260d7be
Compare
|
/submit |
|
Submitted as pull.1358.v3.git.1664218087.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
| @@ -11,7 +11,7 @@ SYNOPSIS | |||
| [verse] | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -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>]
An unrelated tangent, but should register complain when given in a
repository that is already registered as well? Just being curious.There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 9/26/2022 3:23 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> @@ -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>]
>
> An unrelated tangent, but should register complain when given in a
> repository that is already registered as well? Just being curious.
Let's leave that as a #leftoverbits and perhaps as something to
consider next to something like 'git maintenance list' to list the
currently-registered repositories.
Thanks,
-Stolee
| @@ -1470,11 +1470,12 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) | |||
| struct option options[] = { | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Derrick Stolee <derrickstolee@github.com>
>
> The 'git maintenance [un]register' commands set or unset the multi-
> valued maintenance.repo config key with the absolute path of the current
> repository. These are set in the global config file.
OK. This step is new but it looks reasonable.
Embarrassingly sadly, we open, rewrite, and close the configuration
file for each of these "proper API calls", so the IO load is not
reduced, even though we do not have to spawn extra processes ;-)
All three patches queued. Thanks.
|
This patch series was integrated into seen via git@df8740d. |
| @@ -11,7 +11,7 @@ SYNOPSIS | |||
| [verse] | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -1538,11 +1546,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;
> + }
> + }
Just out of curiosity, I ran this in a fresh repository and got a
segfault. An attached patch obviously fixes it, but I am wondering
if a better "fix" is to teach for_each_string_list_item() that it is
perfectly reasonable to see a NULL passed to it as the list, which
is a mere special case that the caller has a string list with 0
items on it.
Thoughts?
Thanks.
diff --git a/builtin/gc.c b/builtin/gc.c
index 4c59235950..67f41fad4b 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1549,6 +1549,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
usage_with_options(builtin_maintenance_unregister_usage,
options);
+ if (list)
for_each_string_list_item(item, list) {
if (!strcmp(maintpath, item->string)) {
found = 1;
--
2.38.0-rc1-123-g02867222b8
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 9/26/2022 5:55 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> @@ -1538,11 +1546,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;
>> + }
>> + }
>
> Just out of curiosity, I ran this in a fresh repository and got a
> segfault.
Yikes! Thanks for catching. I think there was another instance in
the 'register' code that I caught by tests, but I appreciate you
catching this one.
> An attached patch obviously fixes it, but I am wondering
> if a better "fix" is to teach for_each_string_list_item() that it is
> perfectly reasonable to see a NULL passed to it as the list, which
> is a mere special case that the caller has a string list with 0
> items on it.
>
> Thoughts?
I agree that for_each_string_list_item() could handle NULL lists
better, especially because it looks like a method and hides some
details. Plus, wrapping the for-ish loop with an if statement is
particularly ugly.
I think this might be more confusing that
git_configset_get_value_multi() can return a NULL list instead of
an empty list. It boils down to this diff:
diff --git a/config.c b/config.c
index cbb5a3bab74..39cc0534170 100644
--- a/config.c
+++ b/config.c
@@ -2416,8 +2416,9 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char *
const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)
{
+ static struct string_list empty_list = STRING_LIST_INIT_NODUP;
struct config_set_element *e = configset_find_element(cs, key);
- return e ? &e->value_list : NULL;
+ return e ? &e->value_list : &empty_list;
}
int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
However, this change causes a lot of cascading failures that
are probably not worth tracking down.
I'll get a patch put together that changes the behavior of
for_each_string_list_item() and adds the missing 'unregister' test
so we can avoid this problem.
Thanks,
-StoleeThere was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 9/27/2022 7:38 AM, Derrick Stolee wrote:
> On 9/26/2022 5:55 PM, Junio C Hamano wrote:
>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> @@ -1538,11 +1546,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;
>>> + }
>>> + }
>>
>> Just out of curiosity, I ran this in a fresh repository and got a
>> segfault.
>
> Yikes! Thanks for catching. I think there was another instance in
> the 'register' code that I caught by tests, but I appreciate you
> catching this one.
>
>> An attached patch obviously fixes it, but I am wondering
>> if a better "fix" is to teach for_each_string_list_item() that it is
>> perfectly reasonable to see a NULL passed to it as the list, which
>> is a mere special case that the caller has a string list with 0
>> items on it.
>>
>> Thoughts?
>
> I agree that for_each_string_list_item() could handle NULL lists
> better, especially because it looks like a method and hides some
> details. Plus, wrapping the for-ish loop with an if statement is
> particularly ugly.
...
> I'll get a patch put together that changes the behavior of
> for_each_string_list_item() and adds the missing 'unregister' test
> so we can avoid this problem.
Of course, there is a reason why we don't check for NULL here,
and it's because -Werror=address complains when we use a non-pointer
value in the macro:
string-list.h:146:28: error: the address of ‘friendly_ref_names’ will always evaluate as ‘true’ [-Werror=address]
146 | for (item = (list) ? (list)->items : NULL; \
|
I tried searching for a way to suppress this error in a particular
case like this (perhaps using something like an attribute?), but I
couldn't find anything.
Thanks,
-StoleeThere was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Tue, Sep 27 2022, Derrick Stolee wrote:
> On 9/27/2022 7:38 AM, Derrick Stolee wrote:
>> On 9/26/2022 5:55 PM, Junio C Hamano wrote:
>>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>>
>>>> @@ -1538,11 +1546,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;
>>>> + }
>>>> + }
>>>
>>> Just out of curiosity, I ran this in a fresh repository and got a
>>> segfault.
>>
>> Yikes! Thanks for catching. I think there was another instance in
>> the 'register' code that I caught by tests, but I appreciate you
>> catching this one.
>>
>>> An attached patch obviously fixes it, but I am wondering
>>> if a better "fix" is to teach for_each_string_list_item() that it is
>>> perfectly reasonable to see a NULL passed to it as the list, which
>>> is a mere special case that the caller has a string list with 0
>>> items on it.
>>>
>>> Thoughts?
>>
>> I agree that for_each_string_list_item() could handle NULL lists
>> better, especially because it looks like a method and hides some
>> details. Plus, wrapping the for-ish loop with an if statement is
>> particularly ugly.
> ...
>> I'll get a patch put together that changes the behavior of
>> for_each_string_list_item() and adds the missing 'unregister' test
>> so we can avoid this problem.
>
> Of course, there is a reason why we don't check for NULL here,
> and it's because -Werror=address complains when we use a non-pointer
> value in the macro:
>
> string-list.h:146:28: error: the address of ‘friendly_ref_names’ will always evaluate as ‘true’ [-Werror=address]
> 146 | for (item = (list) ? (list)->items : NULL; \
> |
>
> I tried searching for a way to suppress this error in a particular
> case like this (perhaps using something like an attribute?), but I
> couldn't find anything.
We discussed this exact issue just a few months ago, see:
https://lore.kernel.org/git/220614.86czfcytlz.gmgdl@evledraar.gmail.com/
In general I don't think we should be teaching
for_each_string_list_item() to handle NULL.
Instead most callers that need to deal with a "NULL" list should
probably just use a list that's never NULL. See:
https://lore.kernel.org/git/220616.86bkuswuh5.gmgdl@evledraar.gmail.com/
In this case however it seems perfectly reasonable to return a valid
pointer or NULL, and the function documents as much:
/**
* Finds and returns the value list, sorted in order of increasing priority
* for the configuration variable `key`. When the configuration variable
* `key` is not found, returns NULL. The caller should not free or modify
* the returned pointer, as it is owned by the cache.
*/
const struct string_list *git_config_get_value_multi(const char *key);
You also have code in 3/3 that uses that API in the correct way, I think
just adjusting this callsite in 1/3 would be the right move here.
This also gives the reader & compiler more information to e.g. eliminate
dead code. You're calling maintpath() unconditionally, but if you have
no config & the user provided --force we'll never end up using it, so we
can avoid allocating it in the first place.
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 9/27/2022 9:36 AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Sep 27 2022, Derrick Stolee wrote:
>> Of course, there is a reason why we don't check for NULL here,
>> and it's because -Werror=address complains when we use a non-pointer
>> value in the macro:
>>
>> string-list.h:146:28: error: the address of ‘friendly_ref_names’ will always evaluate as ‘true’ [-Werror=address]
>> 146 | for (item = (list) ? (list)->items : NULL; \
>> |
>>
>> I tried searching for a way to suppress this error in a particular
>> case like this (perhaps using something like an attribute?), but I
>> couldn't find anything.
>
> We discussed this exact issue just a few months ago, see:
> https://lore.kernel.org/git/220614.86czfcytlz.gmgdl@evledraar.gmail.com/
Thanks for finding this thread. I knew it was vaguely familiar.
> In general I don't think we should be teaching
> for_each_string_list_item() to handle NULL.
>
> Instead most callers that need to deal with a "NULL" list should
> probably just use a list that's never NULL. See:
> https://lore.kernel.org/git/220616.86bkuswuh5.gmgdl@evledraar.gmail.com/
>
> In this case however it seems perfectly reasonable to return a valid
> pointer or NULL, and the function documents as much:
>
> /**
> * Finds and returns the value list, sorted in order of increasing priority
> * for the configuration variable `key`. When the configuration variable
> * `key` is not found, returns NULL. The caller should not free or modify
> * the returned pointer, as it is owned by the cache.
> */
> const struct string_list *git_config_get_value_multi(const char *key);
It documents that it will never return an empty list, and instead will
return NULL. There are several places that check that condition explicitly.
Converting them is not terribly hard, though, and I'll send an RFC soon
that performs that conversion.
> This also gives the reader & compiler more information to e.g. eliminate
> dead code. You're calling maintpath() unconditionally, but if you have
> no config & the user provided --force we'll never end up using it, so we
> can avoid allocating it in the first place.
While you're correct that we could avoid that allocation, it makes the
code look terrible and hard to reason about, so I won't make that change.
Thanks,
-Stolee
|
This patch series was integrated into seen via git@519cdb8. |
derrickstolee
force-pushed
the
maintenance-unregister
branch
from
260d7be
to
105f0f6
Compare
The 'git maintenance unregister' subcommand has a step that removes the current repository from the multi-valued maitenance.repo config key. This fails if the repository is not listed in that key. This makes running 'git maintenance unregister' twice result in a failure in the second instance. This failure exit code is helpful, but its message is not. Add a new die() message that explicitly calls out the failure due to the repository not being registered. In some cases, users may want to run 'git maintenance unregister' just to make sure that background jobs will not start on this repository, but they do not want to check to see if it is registered first. Add a new '--force' option that will siltently succeed if the repository is not already registered. Also add an extra test of 'git maintenance unregister' at a point where there are no registered repositories. This should fail without --force. Signed-off-by: Derrick Stolee <derrickstolee@github.com>
The 'scalar unregister' command removes a repository from the list of registered Scalar repositories and removes it from the list of repositories registered for background maintenance. If the repository was not already registered for background maintenance, then the command fails, even if the repository was still registered as a Scalar repository. After using 'scalar clone' or 'scalar register', the repository would be enrolled in background maintenance since those commands run 'git maintenance start'. If the user runs 'git maintenance unregister' on that repository, then it is still in the list of repositories which get new config updates from 'scalar reconfigure'. The 'scalar unregister' command would fail since 'git maintenance unregister' would fail. Further, the add_or_remove_enlistment() method in scalar.c already has this idempotent nature built in as an expectation since it returns zero when the scalar.repo list already has the proper containment of the repository. The previous change added the 'git maintenance unregister --force' option, so use it within 'scalar unregister' to make it idempotent. Signed-off-by: Derrick Stolee <derrickstolee@github.com>
The 'git maintenance [un]register' commands set or unset the multi- valued maintenance.repo config key with the absolute path of the current repository. These are set in the global config file. Instead of calling a subcommand and creating a new process, create the proper API calls to git_config_set_multivar_in_file_gently(). It requires loading the filename for the global config file (and erroring out if now $HOME value is set). We also need to be careful about using CONFIG_REGEX_NONE when adding the value and using CONFIG_FLAGS_FIXED_VALUE when removing the value. In both cases, we check that the value already exists (this check already existed for 'unregister'). Also, remove the transparent translation of the error code from the config API to the exit code of 'git maintenance'. Instead, use die() to recover from failures at that level. In the case of 'unregister --force', allow the CONFIG_NOTHING_SET error code to be a success. This allows a possible race where another process removes the config value. The end result is that the config value is not set anymore, so we can treat this as a success. Reported-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
The for_each_string_list_item() macro takes a string_list and automatically constructs a for loop to iterate over its contents. This macro will segfault if the list is non-NULL. We cannot change the macro to be careful around NULL values because there are many callers that use the address of a local variable, which will never be NULL and will cause compile errors with -Werror=address. For now, leave a documentation comment to try to avoid mistakes in the future where a caller does not check for a NULL list. Signed-off-by: Derrick Stolee <derrickstolee@github.com>
derrickstolee
force-pushed
the
maintenance-unregister
branch
from
105f0f6
to
73a262c
Compare
|
/submit |
|
Submitted as pull.1358.v4.git.1664287021.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> I noticed this while we were updating the microsoft/git fork to include
> v2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent
> before, but instead some change in 'scalar unregister' led to it relying on
> the return code of 'git maintenance unregister'. Our functional tests expect
> 'scalar unregister' to be idempotent, and I think that's a good pattern for
> 'git maintenance unregister', so I'm fixing it at that layer.
>
> Despite finding this during the 2.38.0-rc0 integration, this isn't critical
> to the release.
>
> Perhaps an argument could be made that "failure means it wasn't registered
> before", but I think that isn't terribly helpful.
I happen _not_ to share the idea that "unregister is expected to be
idempotent" is a good pattern at all, and it is a better pattern to
make failure mean that the object specified to be acted upon did not
even exist (cf. "rm no-such-file"). But that aside, does what the
above paragraphs mention still true for this round, namely, you are
"fixing" it at that (i.e. "maintenance unregister") layer?
The cover letter does not become part of the permanent history, so
it is not a huge deal as long as the reviewers know what they are
looking at ;-). Just leaving a note in case somebody who missed the
iterations up to v3 is now interested in the topic.
Thanks for a quick iteration. |
| @@ -141,7 +141,12 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c | |||
| int for_each_string_list(struct string_list *list, | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Derrick Stolee <derrickstolee@github.com>
>
> The for_each_string_list_item() macro takes a string_list and
> automatically constructs a for loop to iterate over its contents. This
> macro will segfault if the list is non-NULL.
>
> We cannot change the macro to be careful around NULL values because
> there are many callers that use the address of a local variable, which
> will never be NULL and will cause compile errors with -Werror=address.
>
> For now, leave a documentation comment to try to avoid mistakes in the
> future where a caller does not check for a NULL list.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
> string-list.h | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
For exactly the -Werror=address reason, any other way that checks
list for NULL-ness cannot be done here (other than with Peff's
inline helper that returns the first item or NULL), which is a bit
of shame but this is totally outside the topic, and an additional
comment is a good thing to have here.
Thanks. All four patches look reasonable. Will queue (but after I
make sure I can tag and push out -rc2 today).
> diff --git a/string-list.h b/string-list.h
> index d5a744e1438..c7b0d5d0008 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -141,7 +141,12 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c
> int for_each_string_list(struct string_list *list,
> string_list_each_func_t func, void *cb_data);
>
> -/** Iterate over each item, as a macro. */
> +/**
> + * Iterate over each item, as a macro.
> + *
> + * Be sure that 'list' is non-NULL. The macro cannot perform NULL
> + * checks due to -Werror=address errors.
> + */
> #define for_each_string_list_item(item,list) \
> for (item = (list)->items; \
> item && item < (list)->items + (list)->nr; \
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 9/27/2022 12:31 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> I noticed this while we were updating the microsoft/git fork to include
>> v2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent
>> before, but instead some change in 'scalar unregister' led to it relying on
>> the return code of 'git maintenance unregister'. Our functional tests expect
>> 'scalar unregister' to be idempotent, and I think that's a good pattern for
>> 'git maintenance unregister', so I'm fixing it at that layer.
>>
>> Despite finding this during the 2.38.0-rc0 integration, this isn't critical
>> to the release.
>>
>> Perhaps an argument could be made that "failure means it wasn't registered
>> before", but I think that isn't terribly helpful.
>
> I happen _not_ to share the idea that "unregister is expected to be
> idempotent" is a good pattern at all, and it is a better pattern to
> make failure mean that the object specified to be acted upon did not
> even exist (cf. "rm no-such-file"). But that aside, does what the
> above paragraphs mention still true for this round, namely, you are
> "fixing" it at that (i.e. "maintenance unregister") layer?
Sorry, I forgot to update the cover letter when updating the title.
"git maintenance unregister" will fail if the repository is not already
registered (unless --force is given). Now, "scalar unregister" _is_
idempotent (it uses "git maintenance unregister --force").
> The cover letter does not become part of the permanent history, so
> it is not a huge deal as long as the reviewers know what they are
> looking at ;-). Just leaving a note in case somebody who missed the
> iterations up to v3 is now interested in the topic.
>
> Thanks for a quick iteration.
Thank you for your very careful review!
-Stolee |
|
There was a status update in the "Cooking" section about the branch "git maintenance unregister" in a repository that is already been unregistered reported an error. Will merge to 'next'. source: <pull.1358.v4.git.1664287021.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@25d321d. |
|
This patch series was integrated into seen via git@48fceda. |
|
This patch series was integrated into next via git@a09a78d. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed this while we were updating the microsoft/git fork to include v2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent before, but instead some change in 'scalar unregister' led to it relying on the return code of 'git maintenance unregister'. Our functional tests expect 'scalar unregister' to be idempotent, and I think that's a good pattern for 'git maintenance unregister', so I'm fixing it at that layer.
Despite finding this during the 2.38.0-rc0 integration, this isn't critical to the release.
Perhaps an argument could be made that "failure means it wasn't registered before", but I think that isn't terribly helpful.
Our functional tests are running the unregister subcommand to disable maintenance in order to run tests on the object store (such as running maintenance commands in the foreground and checking the object store afterwards). This is a form of automation using 'unregister' as a check that maintenance will not run at the same time, and it doesn't care if maintenance was already disabled. I can imagine other scripting scenarios wanting that kind of guarantee.
Updates in v4
Updates in v3
Updates in v2
git maintenance unregisteridempotent, add a--forceoption for those who do not want to require that the repository is already registered.scalar unregisteridempotent, with reasons argued in patch 2.Thanks,
-Stolee
cc: gitster@pobox.com
cc: vdye@github.com
cc: SZEDER Gábor szeder.dev@gmail.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com