git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: gitster@pobox.com, vdye@github.com,
	Derrick Stolee <derrickstolee@github.com>
Subject: [PATCH v2 0/2] scalar: make unregister idempotent
Date: Thu, 22 Sep 2022 13:37:15 +0000	[thread overview]
Message-ID: <pull.1358.v2.git.1663853837.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1358.git.1663635732095.gitgitgadget@gmail.com>

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 v2
=============

 * This is now a two-patch series.
 * I rebased onto v2.38.0-rc1 for two reasons: Scalar is now merged, and the
   usage for 'git maintenance unregister' removed its translation markers.
 * Instead of making git maintenance unregister idempotent, add a --force
   option for those who do not want to require that the repository is
   already registered.
 * Make scalar unregister idempotent, with reasons argued in patch 2.

Thanks, -Stolee

Derrick Stolee (2):
  maintenance: add 'unregister --force'
  scalar: make 'unregister' idempotent

 Documentation/git-maintenance.txt |  6 +++++-
 builtin/gc.c                      | 31 +++++++++++++++++++++++++------
 scalar.c                          |  5 ++++-
 t/t7900-maintenance.sh            |  6 +++++-
 t/t9210-scalar.sh                 |  5 ++++-
 5 files changed, 43 insertions(+), 10 deletions(-)


base-commit: 1b3d6e17fe83eb6f79ffbac2f2c61bbf1eaef5f8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1358%2Fderrickstolee%2Fmaintenance-unregister-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1358/derrickstolee/maintenance-unregister-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1358

Range-diff vs v1:

 1:  f028208a3ab ! 1:  69c74f52eef maintenance: make unregister idempotent
     @@ Metadata
      Author: Derrick Stolee <derrickstolee@github.com>
      
       ## Commit message ##
     -    maintenance: make unregister idempotent
     +    maintenance: add 'unregister --force'
      
          The 'git maintenance unregister' subcommand has a step that removes the
          current repository from the multi-valued maitenance.repo config key.
     @@ Commit message
          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.
     +    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.
      
          Signed-off-by: Derrick Stolee <derrickstolee@github.com>
      
     + ## Documentation/git-maintenance.txt ##
     +@@ Documentation/git-maintenance.txt: SYNOPSIS
     + [verse]
     + 'git maintenance' run [<options>]
     + 'git maintenance' start [--scheduler=<scheduler>]
     +-'git maintenance' (stop|register|unregister)
     ++'git maintenance' (stop|register|unregister) [<options>]
     + 
     + 
     + DESCRIPTION
     +@@ Documentation/git-maintenance.txt: 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
     + -----
     +
       ## builtin/gc.c ##
     -@@ builtin/gc.c: static int maintenance_unregister(int argc, const char **argv, const char *prefi
     +@@ builtin/gc.c: 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;
     @@ builtin/gc.c: static int maintenance_unregister(int argc, const char **argv, con
      +			     "--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);
     @@ t/t7900-maintenance.sh: test_expect_success 'register and unregister' '
      -	test_cmp before actual
      +	test_cmp before actual &&
      +
     -+	# Expect unregister to be idempotent.
     -+	git maintenance unregister
     ++	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' '
 -:  ----------- > 2:  f5d8d6e4901 scalar: make 'unregister' idempotent

-- 
gitgitgadget

  parent reply	other threads:[~2022-09-22 13:39 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 ` Derrick Stolee via GitGitGadget [this message]
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
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=pull.1358.v2.git.1663853837.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.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).