git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Derrick Stolee <derrickstolee@github.com>
Cc: "SZEDER Gábor" <szeder.dev@gmail.com>,
	"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>,
	git@vger.kernel.org, gitster@pobox.com, vdye@github.com
Subject: Re: [PATCH v2 1/2] maintenance: add 'unregister --force'
Date: Mon, 26 Sep 2022 17:39:50 +0200	[thread overview]
Message-ID: <220926.86mtamyvo7.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <fc6739c3-2f7c-5dab-e4a2-8243deafde50@github.com>


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.

  reply	other threads:[~2022-09-26 17:01 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-20  1:02 [PATCH] maintenance: make unregister idempotent Derrick Stolee via GitGitGadget
2022-09-21 17:19 ` Junio C Hamano
2022-09-22 12:37   ` Derrick Stolee
2022-09-22 19:31     ` Junio C Hamano
2022-09-22 19:46       ` Derrick Stolee
2022-09-22 20:44         ` Junio C Hamano
2022-09-22 13:37 ` [PATCH v2 0/2] scalar: " Derrick Stolee via GitGitGadget
2022-09-22 13:37   ` [PATCH v2 1/2] maintenance: add 'unregister --force' Derrick Stolee via GitGitGadget
2022-09-23 13:08     ` SZEDER Gábor
2022-09-26 13:32       ` Derrick Stolee
2022-09-26 15:39         ` Ævar Arnfjörð Bjarmason [this message]
2022-09-26 17:25           ` Derrick Stolee
2022-09-26 19:17             ` Junio C Hamano
2022-09-22 13:37   ` [PATCH v2 2/2] scalar: make 'unregister' idempotent Derrick Stolee via GitGitGadget
2022-09-26 18:48   ` [PATCH v3 0/3] scalar: make unregister idempotent Derrick Stolee via GitGitGadget
2022-09-26 18:48     ` [PATCH v3 1/3] maintenance: add 'unregister --force' Derrick Stolee via GitGitGadget
2022-09-26 19:23       ` Junio C Hamano
2022-09-26 20:49         ` Derrick Stolee
2022-09-26 21:55       ` Junio C Hamano
2022-09-27 11:38         ` Derrick Stolee
2022-09-27 11:54           ` Derrick Stolee
2022-09-27 13:36             ` Ævar Arnfjörð Bjarmason
2022-09-27 13:55               ` Derrick Stolee
2022-09-26 18:48     ` [PATCH v3 2/3] scalar: make 'unregister' idempotent Derrick Stolee via GitGitGadget
2022-09-26 18:48     ` [PATCH v3 3/3] gc: replace config subprocesses with API calls Derrick Stolee via GitGitGadget
2022-09-26 19:27       ` Junio C Hamano
2022-09-27 13:56     ` [PATCH v4 0/4] scalar: make unregister idempotent Derrick Stolee via GitGitGadget
2022-09-27 13:56       ` [PATCH v4 1/4] maintenance: add 'unregister --force' Derrick Stolee via GitGitGadget
2022-09-27 13:56       ` [PATCH v4 2/4] scalar: make 'unregister' idempotent Derrick Stolee via GitGitGadget
2022-09-27 13:57       ` [PATCH v4 3/4] gc: replace config subprocesses with API calls Derrick Stolee via GitGitGadget
2022-09-27 13:57       ` [PATCH v4 4/4] string-list: document iterator behavior on NULL input Derrick Stolee via GitGitGadget
2022-09-27 16:39         ` Junio C Hamano
2022-09-27 16:31       ` [PATCH v4 0/4] scalar: make unregister idempotent Junio C Hamano
2022-09-27 16:54         ` Derrick Stolee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=220926.86mtamyvo7.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=szeder.dev@gmail.com \
    --cc=vdye@github.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).