{"thread":{"id":"39702","subject":"[RFC/PATCH 0/2] bumping repository format version","startedAt":"2015-06-23T10:50:42Z","lastAt":"2015-06-25T10:07:37Z","messageCount":13,"participants":["Jeff King","Duy Nguyen","Junio C Hamano","David Turner"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"264620","messageId":"20150623105042.GA10888@peff.net","threadId":"39702","inReplyTo":null,"subject":"[RFC/PATCH 0/2] bumping repository format version","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-23T10:50:42Z","receivedAt":"2015-06-23T10:50:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We've managed to avoid bumping core.repositoryformatversion for the past\n10 years, which is great. But I think there are some looming features\nthat are going to need it. The most obvious one is changing the ref\nstorage, where we need some way to tell older gits \"no, please don't\nthink that taking 'refs/heads/foo.lock' is sufficient to actually lock\".\n\nThe first patch in this series is an attempt to pave the way for version\nbumps like this in as painless a way as possible, by letting us mark\nincompatible \"extensions\" by name. That way we can version things like\n\"how do you lock a ref\" independent of the main repositoryformatversion\nsetting (just like we do for index version, for example).\n\nSee the explanation in the first patch for more details. The second\npatch shows another use of the \"extension\" feature to provide safety\nin shared-object repos against older versions of git.\n\n  [1/2]: introduce \"extensions\" form of core.repositoryformatversion\n  [2/2]: introduce \"preciousObjects\" repository extension\n\n-Peff\n"},{"id":"264623","messageId":"20150623105357.GA12518@peff.net","threadId":"39702","inReplyTo":"20150623105042.GA10888@peff.net","subject":"[PATCH 1/2] introduce \"extensions\" form of core.repositoryformatversion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-23T10:53:58Z","receivedAt":"2015-06-23T10:53:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Normally we try to avoid bumps of the whole-repository\ncore.repositoryformatversion field. However, it is\nunavoidable if we want to safely change certain aspects of\ngit in a backwards-incompatible way (e.g., modifying the set\nof ref tips that we must traverse to generate a list of\nunreachable, safe-to-prune objects).\n\nIf we were to bump the repository version for every such\nchange, then any implementation understanding version `X`\nwould also have to understand `X-1`, `X-2`, and so forth,\neven though the incompatibilities may be in orthogonal parts\nof the system, and there is otherwise no reason we cannot\nimplement one without the other (or more importantly, that\nthe user cannot choose to use one feature without the other,\nweighing the tradeoff in compatibility only for that\nparticular feature).\n\nThis patch documents the existing repositoryformatversion\nstrategy and introduces a new format, \"1\", which lets a\nrepository specify that it must run with an arbitrary set of\nextensions. This can be used, for example:\n\n - to inform git that the objects should not be pruned based\n   only on the reachability of the ref tips (e.g, because it\n   has \"clone --shared\" children)\n\n - that the refs are stored in a format besides the usual\n   \"refs\" and \"packed-refs\" directories\n\nBecause we bump to format \"1\", and because format \"1\"\nrequires that a running git knows about any extensions\nmentioned, we know that older versions of the code will not\ndo something dangerous when confronted with these new\nformats.\n\nFor example, if the user chooses to use database storage for\nrefs, they may set the \"extensions.refbackend\" config to\n\"db\". Older versions of git will not understand format \"1\"\nand bail. Versions of git which understand \"1\" but do not\nknow about \"refbackend\", or which know about \"refbackend\"\nbut not about the \"db\" backend, will refuse to run. This is\nannoying, of course, but much better than the alternative of\nclaiming that there are no refs in the repository, or\nwriting to a location that other implementations will not\nread.\n\nNote that we are only defining the rules for format 1 here.\nWe do not ever write format 1 ourselves; it is a tool that\nis meant to be used by users and future extensions to\nprovide safety with older implementations.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/technical/repository-version.txt | 81 ++++++++++++++++++++++++++\n cache.h                                        |  6 ++\n setup.c                                        | 37 +++++++++++-\n t/t1302-repo-version.sh                        | 38 ++++++++++++\n 4 files changed, 159 insertions(+), 3 deletions(-)\n create mode 100644 Documentation/technical/repository-version.txt\n\ndiff --git a/Documentation/technical/repository-version.txt b/Documentation/technical/repository-version.txt\nnew file mode 100644\nindex 0000000..3d7106d\n--- /dev/null\n+++ b/Documentation/technical/repository-version.txt\n@@ -0,0 +1,81 @@\n+Git Repository Format Versions\n+==============================\n+\n+Every git repository is marked with a numeric version in the\n+`core.repositoryformatversion` key of its `config` file. This version\n+specifies the rules for operating on the on-disk repository data. An\n+implementation of git which does not understand a particular version\n+advertised by an on-disk repository MUST NOT operate on that repository;\n+doing so risks not only producing wrong results, but actually losing\n+data.\n+\n+Because of this rule, version bumps should be kept to an absolute\n+minimum. Instead, we generally prefer these strategies:\n+\n+  - bumping format version numbers of individual data files (e.g.,\n+    index, packfiles, etc). This restricts the incompatibilities only to\n+    those files.\n+\n+  - introducing new data that gracefully degrades when used by older\n+    clients (e.g., pack bitmap files are ignored by older clients, which\n+    simply do not take advantage of the optimization they provide).\n+\n+A whole-repository format version bump should only be part of a change\n+that cannot be independently versioned. For instance, if one were to\n+change the reachability rules for objects, or the rules for locking\n+refs, that would require a bump of the repository format version.\n+\n+Note that this applies only to accessing the repository's disk contents\n+directly. An older client which understands only format `0` may still\n+connect via `git://` to a repository using format `1`, as long as the\n+server process understands format `1`.\n+\n+The preferred strategy for rolling out a version bump (whether whole\n+repository or for a single file) is to teach git to read the new format,\n+and allow writing the new format with a config switch or command line\n+option (for experimentation or for those who do not care about backwards\n+compatibility with older gits). Then after a long period to allow the\n+reading capability to become common, we may switch to writing the new\n+format by default.\n+\n+The currently defined format versions are:\n+\n+Version `0`\n+-----------\n+\n+This is the format defined by the initial version of git, including but\n+not limited to the format of the repository directory, the repository\n+configuration file, and the object and ref storage. Specifying the\n+complete behavior of git is beyond the scope of this document.\n+\n+Version `1`\n+-----------\n+\n+This format is identical to version `0`, with the following exceptions:\n+\n+  1. When reading the `core.repositoryformatversion` variable, a git\n+     implementation which supports version 1 MUST also read any\n+     configuration keys found in the `extensions` section of the\n+     configuration file.\n+\n+  2. If a version-1 repository specifies any `extensions.*` keys that\n+     the running git has not implemented, the operation MUST NOT\n+     proceed. Similarly, if the value of any known key is not understood\n+     by the implementation, the operation MUST NOT proceed.\n+\n+Note that if no extensions are specified in the config file, then\n+`core.repositoryformatversion` SHOULD be set to `0` (setting it to `1`\n+provides no benefit, and makes the repository incompatible with older\n+implementations of git).\n+\n+This document will serve as the master list for extensions. Any\n+implementation wishing to define a new extension should make a note of\n+it here, in order to claim the name.\n+\n+The defined extensions are:\n+\n+`noop`\n+~~~~~~\n+\n+This extension does not change git's behavior at all. It is useful only\n+for testing format-1 compatibility.\ndiff --git a/cache.h b/cache.h\nindex 571c98f..bee425b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -686,7 +686,13 @@ extern char *notes_ref_name;\n \n extern int grafts_replace_parents;\n \n+/*\n+ * GIT_REPO_VERSION is the version we write by default. The\n+ * _READ variant is the highest number we know how to\n+ * handle.\n+ */\n #define GIT_REPO_VERSION 0\n+#define GIT_REPO_VERSION_READ 1\n extern int repository_format_version;\n extern int check_repository_format(void);\n \ndiff --git a/setup.c b/setup.c\nindex 82c0cc2..0d53846 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -5,6 +5,7 @@\n static int inside_git_dir = -1;\n static int inside_work_tree = -1;\n static int work_tree_config_is_bogus;\n+static struct string_list unknown_extensions = STRING_LIST_INIT_DUP;\n \n /*\n  * The input parameter must contain an absolute path, and it must already be\n@@ -352,10 +353,23 @@ void setup_work_tree(void)\n \n static int check_repo_format(const char *var, const char *value, void *cb)\n {\n+\tconst char *ext;\n+\n \tif (strcmp(var, \"core.repositoryformatversion\") == 0)\n \t\trepository_format_version = git_config_int(var, value);\n \telse if (strcmp(var, \"core.sharedrepository\") == 0)\n \t\tshared_repository = git_config_perm(var, value);\n+\telse if (skip_prefix(var, \"extensions.\", &ext)) {\n+\t\t/*\n+\t\t * record any known extensions here; otherwise,\n+\t\t * we fall through to recording it as unknown, and\n+\t\t * check_repository_format will complain\n+\t\t */\n+\t\tif (!strcmp(ext, \"noop\"))\n+\t\t\t;\n+\t\telse\n+\t\t\tstring_list_append(&unknown_extensions, ext);\n+\t}\n \treturn 0;\n }\n \n@@ -366,6 +380,8 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)\n \tconfig_fn_t fn;\n \tint ret = 0;\n \n+\tstring_list_clear(&unknown_extensions, 0);\n+\n \tif (get_common_dir(&sb, gitdir))\n \t\tfn = check_repo_format;\n \telse\n@@ -383,16 +399,31 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)\n \t * is a good one.\n \t */\n \tgit_config_early(fn, NULL, repo_config);\n-\tif (GIT_REPO_VERSION < repository_format_version) {\n+\tif (GIT_REPO_VERSION_READ < repository_format_version) {\n \t\tif (!nongit_ok)\n \t\t\tdie (\"Expected git repo version <= %d, found %d\",\n-\t\t\t     GIT_REPO_VERSION, repository_format_version);\n+\t\t\t     GIT_REPO_VERSION_READ, repository_format_version);\n \t\twarning(\"Expected git repo version <= %d, found %d\",\n-\t\t\tGIT_REPO_VERSION, repository_format_version);\n+\t\t\tGIT_REPO_VERSION_READ, repository_format_version);\n \t\twarning(\"Please upgrade Git\");\n \t\t*nongit_ok = -1;\n \t\tret = -1;\n \t}\n+\n+\tif (repository_format_version >= 1 && unknown_extensions.nr) {\n+\t\tint i;\n+\n+\t\tif (!nongit_ok)\n+\t\t\tdie(\"unknown repository extension: %s\",\n+\t\t\t    unknown_extensions.items[0].string);\n+\n+\t\tfor (i = 0; i < unknown_extensions.nr; i++)\n+\t\t\twarning(\"unknown repository extension: %s\",\n+\t\t\t\tunknown_extensions.items[i].string);\n+\t\t*nongit_ok = -1;\n+\t\tret = -1;\n+\t}\n+\n \tstrbuf_release(&sb);\n \treturn ret;\n }\ndiff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh\nindex 0d9388a..8dd6fd7 100755\n--- a/t/t1302-repo-version.sh\n+++ b/t/t1302-repo-version.sh\n@@ -67,4 +67,42 @@ test_expect_success 'gitdir required mode' '\n \t)\n '\n \n+check_allow () {\n+\tgit rev-parse --git-dir >actual &&\n+\techo .git >expect &&\n+\ttest_cmp expect actual\n+}\n+\n+check_abort () {\n+\ttest_must_fail git rev-parse --git-dir\n+}\n+\n+# avoid git-config, since it cannot be trusted to run\n+# in a repository with a broken version\n+mkconfig () {\n+\techo '[core]' &&\n+\techo \"repositoryformatversion = $1\" &&\n+\tshift &&\n+\n+\tif test $# -gt 0; then\n+\t\techo '[extensions]' &&\n+\t\tfor i in \"$@\"; do\n+\t\t\techo \"$i\"\n+\t\tdone\n+\tfi\n+}\n+\n+while read outcome version extensions; do\n+\ttest_expect_success \"$outcome version=$version $extensions\" \"\n+\t\tmkconfig $version $extensions >.git/config &&\n+\t\tcheck_${outcome}\n+\t\"\n+done <<\\EOF\n+allow 0\n+allow 1\n+allow 1 noop\n+abort 1 no-such-extension\n+allow 0 no-such-extension\n+EOF\n+\n test_done\n-- \n2.4.4.719.g3984bc6\n"},{"id":"264622","messageId":"20150623105411.GB12518@peff.net","threadId":"39702","inReplyTo":"20150623105042.GA10888@peff.net","subject":"[PATCH 2/2] introduce \"preciousObjects\" repository extension","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-23T10:54:11Z","receivedAt":"2015-06-23T10:54:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If this extension is used in a repository, then no\noperations should run which may drop objects from the object\nstorage. This can be useful if you are sharing that storage\nwith other repositories whose refs you cannot see.\n\nFor instance, if you do:\n\n  $ git clone -s parent child\n  $ git -C parent config extensions.preciousObjects true\n  $ git -C parent config core.repositoryformatversion 1\n\nyou now have additional safety when running git in the\nparent repository. Prunes and repacks will bail with an\nerror, and `git gc` will skip those operations (it will\ncontinue to pack refs and do other non-object operations).\nOlder versions of git, when run in the repository, will\nfail on every operation.\n\nNote that we do not set the preciousObjects extension by\ndefault when doing a \"clone -s\", as doing so breaks\nbackwards compatibility. It is a decision the user should\nmake explicitly.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/technical/repository-version.txt |  7 +++++++\n builtin/gc.c                                   | 20 +++++++++++---------\n builtin/prune.c                                |  3 +++\n builtin/repack.c                               |  3 +++\n cache.h                                        |  1 +\n environment.c                                  |  1 +\n setup.c                                        |  2 ++\n t/t1302-repo-version.sh                        | 22 ++++++++++++++++++++++\n 8 files changed, 50 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/technical/repository-version.txt b/Documentation/technical/repository-version.txt\nindex 3d7106d..00ad379 100644\n--- a/Documentation/technical/repository-version.txt\n+++ b/Documentation/technical/repository-version.txt\n@@ -79,3 +79,10 @@ The defined extensions are:\n \n This extension does not change git's behavior at all. It is useful only\n for testing format-1 compatibility.\n+\n+`preciousObjects`\n+~~~~~~~~~~~~~~~~~\n+\n+When the config key `extensions.preciousObjects` is set to `true`,\n+objects in the repository MUST NOT be deleted (e.g., by `git-prune` or\n+`git repack -d`).\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 36fe333..8b8dc6b 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -352,15 +352,17 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tif (gc_before_repack())\n \t\treturn -1;\n \n-\tif (run_command_v_opt(repack.argv, RUN_GIT_CMD))\n-\t\treturn error(FAILED_RUN, repack.argv[0]);\n-\n-\tif (prune_expire) {\n-\t\targv_array_push(&prune, prune_expire);\n-\t\tif (quiet)\n-\t\t\targv_array_push(&prune, \"--no-progress\");\n-\t\tif (run_command_v_opt(prune.argv, RUN_GIT_CMD))\n-\t\t\treturn error(FAILED_RUN, prune.argv[0]);\n+\tif (!repository_format_precious_objects) {\n+\t\tif (run_command_v_opt(repack.argv, RUN_GIT_CMD))\n+\t\t\treturn error(FAILED_RUN, repack.argv[0]);\n+\n+\t\tif (prune_expire) {\n+\t\t\targv_array_push(&prune, prune_expire);\n+\t\t\tif (quiet)\n+\t\t\t\targv_array_push(&prune, \"--no-progress\");\n+\t\t\tif (run_command_v_opt(prune.argv, RUN_GIT_CMD))\n+\t\t\t\treturn error(FAILED_RUN, prune.argv[0]);\n+\t\t}\n \t}\n \n \tif (prune_worktrees_expire) {\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 0c73246..fc0c8e8 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -218,6 +218,9 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n \n+\tif (repository_format_precious_objects)\n+\t\tdie(\"cannot prune in a precious-objects repo\");\n+\n \twhile (argc--) {\n \t\tunsigned char sha1[20];\n \t\tconst char *name = *argv++;\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex af7340c..8ae7fe5 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -193,6 +193,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, builtin_repack_options,\n \t\t\t\tgit_repack_usage, 0);\n \n+\tif (delete_redundant && repository_format_precious_objects)\n+\t\tdie(\"cannot repack in a precious-objects repo\");\n+\n \tif (pack_kept_objects < 0)\n \t\tpack_kept_objects = write_bitmaps;\n \ndiff --git a/cache.h b/cache.h\nindex bee425b..9b59a63 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -694,6 +694,7 @@ extern int grafts_replace_parents;\n #define GIT_REPO_VERSION 0\n #define GIT_REPO_VERSION_READ 1\n extern int repository_format_version;\n+extern int repository_format_precious_objects;\n extern int check_repository_format(void);\n \n #define MTIME_CHANGED\t0x0001\ndiff --git a/environment.c b/environment.c\nindex 61c685b..da66e82 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -26,6 +26,7 @@ int warn_ambiguous_refs = 1;\n int warn_on_object_refname_ambiguity = 1;\n int ref_paranoia = -1;\n int repository_format_version;\n+int repository_format_precious_objects;\n const char *git_commit_encoding;\n const char *git_log_output_encoding;\n int shared_repository = PERM_UMASK;\ndiff --git a/setup.c b/setup.c\nindex 0d53846..8b8dca9 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -367,6 +367,8 @@ static int check_repo_format(const char *var, const char *value, void *cb)\n \t\t */\n \t\tif (!strcmp(ext, \"noop\"))\n \t\t\t;\n+\t\telse if (!strcmp(ext, \"preciousobjects\"))\n+\t\t\trepository_format_precious_objects = git_config_bool(var, value);\n \t\telse\n \t\t\tstring_list_append(&unknown_extensions, ext);\n \t}\ndiff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh\nindex 8dd6fd7..9bcd349 100755\n--- a/t/t1302-repo-version.sh\n+++ b/t/t1302-repo-version.sh\n@@ -105,4 +105,26 @@ abort 1 no-such-extension\n allow 0 no-such-extension\n EOF\n \n+test_expect_success 'precious-objects allowed' '\n+\tmkconfig 1 preciousObjects >.git/config &&\n+\tcheck_allow\n+'\n+\n+test_expect_success 'precious-objects blocks destructive repack' '\n+\ttest_must_fail git repack -ad\n+'\n+\n+test_expect_success 'other repacks are OK' '\n+\ttest_commit foo &&\n+\tgit repack\n+'\n+\n+test_expect_success 'precious-objects blocks prune' '\n+\ttest_must_fail git prune\n+'\n+\n+test_expect_success 'gc runs without complaint' '\n+\tgit gc\n+'\n+\n test_done\n-- \n2.4.4.719.g3984bc6\n"},{"id":"264625","messageId":"CACsJy8AR_vZLAAKf4P+aEtKu1u35N8kAYtYLNOg55ET8DWeZUA@mail.gmail.com","threadId":"39702","inReplyTo":"20150623105411.GB12518@peff.net","subject":"Re: [PATCH 2/2] introduce \"preciousObjects\" repository extension","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-23T11:14:22Z","receivedAt":"2015-06-23T11:14:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jun 23, 2015 at 5:54 PM, Jeff King <peff@peff.net> wrote:\n> diff --git a/builtin/prune.c b/builtin/prune.c\n> index 0c73246..fc0c8e8 100644\n> --- a/builtin/prune.c\n> +++ b/builtin/prune.c\n> @@ -218,6 +218,9 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n>                 return 0;\n>         }\n>\n> +       if (repository_format_precious_objects)\n> +               die(\"cannot prune in a precious-objects repo\");\n> +\n>         while (argc--) {\n>                 unsigned char sha1[20];\n>                 const char *name = *argv++;\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index af7340c..8ae7fe5 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -193,6 +193,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>         argc = parse_options(argc, argv, prefix, builtin_repack_options,\n>                                 git_repack_usage, 0);\n>\n> +       if (delete_redundant && repository_format_precious_objects)\n> +               die(\"cannot repack in a precious-objects repo\");\n> +\n>         if (pack_kept_objects < 0)\n>                 pack_kept_objects = write_bitmaps;\n>\n\nI know both commands have translatable strings that are not marked\nwith _(). But if you reroll, maybe you could add _() for these new\nstrings. It's even better if you mark all others in these commands\ntoo, if you have time.\n-- \nDuy\n"},{"id":"264627","messageId":"20150623114745.GA3845@peff.net","threadId":"39702","inReplyTo":"CACsJy8AR_vZLAAKf4P+aEtKu1u35N8kAYtYLNOg55ET8DWeZUA@mail.gmail.com","subject":"Re: [PATCH 2/2] introduce \"preciousObjects\" repository extension","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-23T11:47:45Z","receivedAt":"2015-06-23T11:47:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 23, 2015 at 06:14:22PM +0700, Duy Nguyen wrote:\n\n> > +       if (delete_redundant && repository_format_precious_objects)\n> > +               die(\"cannot repack in a precious-objects repo\");\n> > +\n> >         if (pack_kept_objects < 0)\n> >                 pack_kept_objects = write_bitmaps;\n> >\n> \n> I know both commands have translatable strings that are not marked\n> with _(). But if you reroll, maybe you could add _() for these new\n> strings. It's even better if you mark all others in these commands\n> too, if you have time.\n\nYeah, I agree these should be marked for translation. Thanks.\n\n-Peff\n"},{"id":"264691","messageId":"xmqq1th2cezr.fsf@gitster.dls.corp.google.com","threadId":"39702","inReplyTo":"20150623105411.GB12518@peff.net","subject":"Re: [PATCH 2/2] introduce \"preciousObjects\" repository extension","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-23T21:05:28Z","receivedAt":"2015-06-23T21:05:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>  This extension does not change git's behavior at all. It is useful only\n>  for testing format-1 compatibility.\n> +\n> +`preciousObjects`\n> +~~~~~~~~~~~~~~~~~\n> +\n> +When the config key `extensions.preciousObjects` is set to `true`,\n> +objects in the repository MUST NOT be deleted (e.g., by `git-prune` or\n> +`git repack -d`).\n\nOK.  In essense, the 'extension' on the disk is like 'capability' on\nthe wire, in that you are not supposed to ask for capability they do\nnot understand, and you are not supposed to touch a repository you\ndo not understand.\n\nAnd the above looks like a reasonable sample \"feature\" to protect by\nthe 'extension' system.\n\n> +\tif (delete_redundant && repository_format_precious_objects)\n> +\t\tdie(\"cannot repack in a precious-objects repo\");\n\nThis message initially threw me off during my cursory reading, but\nthe code tells me that this is only about \"repack -d\".\n\nUnfortunately the users do not get the chance to read the code;\nperhaps s/cannot repack/& -d/; or something?\n"},{"id":"264693","messageId":"1435095074.28466.74.camel@twopensource.com","threadId":"39702","inReplyTo":"20150623105411.GB12518@peff.net","subject":"Re: [PATCH 2/2] introduce \"preciousObjects\" repository extension","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-06-23T21:31:14Z","receivedAt":"2015-06-23T21:31:14Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2015-06-23 at 06:54 -0400, Jeff King wrote:\n> +\tmkconfig 1 preciousObjects >.git/config &&\n\nnit: I think it's better to use git config rather than manually writing\nconfig files.  git config is more future-proof if we up switching config\nbackends; it's also more composable with other configs (making this test\neasier to copy and manipulate), and more explicit.\n\nOther than that it looks good to me.\n"},{"id":"264708","messageId":"20150624075019.GA827@peff.net","threadId":"39702","inReplyTo":"xmqq1th2cezr.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] introduce \"preciousObjects\" repository extension","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-24T07:50:20Z","receivedAt":"2015-06-24T07:50:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 23, 2015 at 02:05:28PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >  This extension does not change git's behavior at all. It is useful only\n> >  for testing format-1 compatibility.\n> > +\n> > +`preciousObjects`\n> > +~~~~~~~~~~~~~~~~~\n> > +\n> > +When the config key `extensions.preciousObjects` is set to `true`,\n> > +objects in the repository MUST NOT be deleted (e.g., by `git-prune` or\n> > +`git repack -d`).\n> \n> OK.  In essense, the 'extension' on the disk is like 'capability' on\n> the wire, in that you are not supposed to ask for capability they do\n> not understand, and you are not supposed to touch a repository you\n> do not understand.\n\nYeah, I think that is a good analogy.\n\n> > +\tif (delete_redundant && repository_format_precious_objects)\n> > +\t\tdie(\"cannot repack in a precious-objects repo\");\n> \n> This message initially threw me off during my cursory reading, but\n> the code tells me that this is only about \"repack -d\".\n> \n> Unfortunately the users do not get the chance to read the code;\n> perhaps s/cannot repack/& -d/; or something?\n\nI agree that would be better. I originally just blocked all use of\ngit-repack, but at the last minute softened it to just \"repack -d\". I'm\nnot sure if that would actually help anyone in practice. Sure, doing\n\"git repack\" without any options is not destructive, but I wonder if\nanybody actually does it. They either run `git gc`, or they probably do\nsomething more exotic and disable the safety check during that run[1].\n\nSo I think we could squash in the patch below (which also marks the\nstrings for translation). But I'd also be OK with the rule covering all\nof `git repack`.\n\n-Peff\n\n[1] One of my proposed uses for this is to revamp the way we handle\n    shared objects on GitHub servers. Right now objects get pushed to\n    individual forks, and then migrate to a shared repository that is\n    accessed via the alternates mechanism. I would like to move to\n    symlinking the `objects/` directory to write directly into the\n    shared space. But the destruction from accidentally running\n    something like `git gc` in a fork is very high. With this patch, we\n    can bump the forks to the v1 format and mark their objects as\n    precious.\n\n---\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex fc0c8e8..6a58e75 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -219,7 +219,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (repository_format_precious_objects)\n-\t\tdie(\"cannot prune in a precious-objects repo\");\n+\t\tdie(_(\"cannot prune in a precious-objects repo\"));\n \n \twhile (argc--) {\n \t\tunsigned char sha1[20];\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 8ae7fe5..3beda2c 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -194,7 +194,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tgit_repack_usage, 0);\n \n \tif (delete_redundant && repository_format_precious_objects)\n-\t\tdie(\"cannot repack in a precious-objects repo\");\n+\t\tdie(_(\"cannot delete packs in a precious-objects repo\"));\n \n \tif (pack_kept_objects < 0)\n \t\tpack_kept_objects = write_bitmaps;\n"},{"id":"264710","messageId":"20150624075514.GB827@peff.net","threadId":"39702","inReplyTo":"1435095074.28466.74.camel@twopensource.com","subject":"Re: [PATCH 2/2] introduce \"preciousObjects\" repository extension","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-24T07:55:14Z","receivedAt":"2015-06-24T07:55:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 23, 2015 at 05:31:14PM -0400, David Turner wrote:\n\n> On Tue, 2015-06-23 at 06:54 -0400, Jeff King wrote:\n> > +\tmkconfig 1 preciousObjects >.git/config &&\n> \n> nit: I think it's better to use git config rather than manually writing\n> config files.  git config is more future-proof if we up switching config\n> backends; it's also more composable with other configs (making this test\n> easier to copy and manipulate), and more explicit.\n\nI would have preferred that, but it makes the tests very fragile. You\nare depending on \"git config\" running even when the current directory is\nnot a valid git repo (and we walk up to the surround git.git directory\nand change the config there!).\n\nI guess we could use \"git config -f .git/config\", though that is\npartially defeating the purpose of your suggestion.\n\nI dunno. I kind of figured we would cross that bridge if and when we\ncome to it. I imagine you are pretty sensitive to it, though, having\njust waded through all the tests that assume various things about\n.git/refs. :)\n\n-Peff\n"},{"id":"264711","messageId":"20150624081224.GA4781@peff.net","threadId":"39702","inReplyTo":"20150623105411.GB12518@peff.net","subject":"Re: [PATCH 2/2] introduce \"preciousObjects\" repository extension","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-24T08:12:24Z","receivedAt":"2015-06-24T08:12:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 23, 2015 at 06:54:11AM -0400, Jeff King wrote:\n\n> diff --git a/builtin/prune.c b/builtin/prune.c\n> index 0c73246..fc0c8e8 100644\n> --- a/builtin/prune.c\n> +++ b/builtin/prune.c\n> @@ -218,6 +218,9 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n>  \t\treturn 0;\n>  \t}\n>  \n> +\tif (repository_format_precious_objects)\n> +\t\tdie(\"cannot prune in a precious-objects repo\");\n> +\n\nBy the way, I originally wrote this patch on an older version of git,\nand was surprised that `git gc` broke when I brought it forward. The\nproblem was that gc now runs `git prune --worktree`, and my die() was\naffecting that case.\n\nIt really seems misplaced to me to make worktree-pruning part of\ngit-prune. I imagine the rationale was \"well, we are pruning things, so\nlet's add an option to this command rather than make a new one\". But I\ndo not think that follows our usual pattern with gc, which is:\n\n  1. Individual areas of code handle their own cleanup. E.g., \"reflog\n     expire\", \"rerere gc\".\n\n  2. We tie them together with \"git gc\", not with \"git prune\".\n\nSo it seems weird that \"git prune --worktree\" is a fundamentally\ndifferent command than \"git prune\". I think by (1), it should be a\nseparate \"git prune-worktree\" (or better yet, \"git worktree prune\", as\nthe start of a command for manipulating the list of multiple-worktrees).\n\nNot a _huge_ deal, but if we want to change it, it would be nice to do so\nnow before it is part of a release. Thoughts?\n\n-Peff\n"},{"id":"264718","messageId":"CACsJy8Ca4J4Q4zi2i-HmySK6WXcNLo=oUZ9H=nPpms7VWhHe1Q@mail.gmail.com","threadId":"39702","inReplyTo":"20150624081224.GA4781@peff.net","subject":"Re: [PATCH 2/2] introduce \"preciousObjects\" repository extension","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-24T10:29:42Z","receivedAt":"2015-06-24T10:29:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jun 24, 2015 at 3:12 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Jun 23, 2015 at 06:54:11AM -0400, Jeff King wrote:\n>\n>> diff --git a/builtin/prune.c b/builtin/prune.c\n>> index 0c73246..fc0c8e8 100644\n>> --- a/builtin/prune.c\n>> +++ b/builtin/prune.c\n>> @@ -218,6 +218,9 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n>>               return 0;\n>>       }\n>>\n>> +     if (repository_format_precious_objects)\n>> +             die(\"cannot prune in a precious-objects repo\");\n>> +\n>\n> By the way, I originally wrote this patch on an older version of git,\n> and was surprised that `git gc` broke when I brought it forward. The\n> problem was that gc now runs `git prune --worktree`, and my die() was\n> affecting that case.\n>\n> It really seems misplaced to me to make worktree-pruning part of\n> git-prune. I imagine the rationale was \"well, we are pruning things, so\n> let's add an option to this command rather than make a new one\". But I\n> do not think that follows our usual pattern with gc, which is:\n>\n>   1. Individual areas of code handle their own cleanup. E.g., \"reflog\n>      expire\", \"rerere gc\".\n>\n>   2. We tie them together with \"git gc\", not with \"git prune\".\n>\n> So it seems weird that \"git prune --worktree\" is a fundamentally\n> different command than \"git prune\". I think by (1), it should be a\n> separate \"git prune-worktree\" (or better yet, \"git worktree prune\", as\n> the start of a command for manipulating the list of multiple-worktrees).\n>\n> Not a _huge_ deal, but if we want to change it, it would be nice to do so\n> now before it is part of a release. Thoughts?\n\nI was misled by the generic name \"prune\" :) (OK my bad, the document\nclearly says it's about object database). Maybe we should make an\nalias prune-objects.. And you caught me too late, I also added\nprune_shallow() in there.\n\nMultiple worktree feature is not released yet so I still have some\ntime to move it out. Yeah \"git worktree prune\" makes sense. I think I\nneed a way to list worktrees anyway. I didn't find good enough reason\nto create \"git worktree\" back then just for listing..\n-- \nDuy\n"},{"id":"264739","messageId":"xmqqvbed9gf7.fsf@gitster.dls.corp.google.com","threadId":"39702","inReplyTo":"20150624075019.GA827@peff.net","subject":"Re: [PATCH 2/2] introduce \"preciousObjects\" repository extension","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-24T17:15:08Z","receivedAt":"2015-06-24T17:15:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> > +\tif (delete_redundant && repository_format_precious_objects)\n>> > +\t\tdie(\"cannot repack in a precious-objects repo\");\n>> \n>> This message initially threw me off during my cursory reading, but\n>> the code tells me that this is only about \"repack -d\".\n>> \n>> Unfortunately the users do not get the chance to read the code;\n>> perhaps s/cannot repack/& -d/; or something?\n>\n> I agree that would be better. I originally just blocked all use of\n> git-repack, but at the last minute softened it to just \"repack -d\". I'm\n> not sure if that would actually help anyone in practice. Sure, doing\n> \"git repack\" without any options is not destructive, but I wonder if\n> anybody actually does it.\n\nHmph, if you cannot afford to lose objects that are unreachable from\nyour refs (because you know your repository has borrowers) but are\nsuffering from too many packs, wouldn't \"repack -a\" be the most\nnatural thing to do?  Maybe I am biased, but \"git gc\" is not the\nfirst thing that comes to my mind in that situation.\n\n> So I think we could squash in the patch below (which also marks the\n> strings for translation). But I'd also be OK with the rule covering all\n> of `git repack`.\n\nOK, will squash it in.\n\n> [1] One of my proposed uses for this is to revamp the way we handle\n>     shared objects on GitHub servers. Right now objects get pushed to\n>     individual forks, and then migrate to a shared repository that is\n>     accessed via the alternates mechanism. I would like to move to\n>     symlinking the `objects/` directory to write directly into the\n>     shared space. But the destruction from accidentally running\n>     something like `git gc` in a fork is very high. With this patch, we\n>     can bump the forks to the v1 format and mark their objects as\n>     precious.\n>\n> ---\n> diff --git a/builtin/prune.c b/builtin/prune.c\n> index fc0c8e8..6a58e75 100644\n> --- a/builtin/prune.c\n> +++ b/builtin/prune.c\n> @@ -219,7 +219,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \n>  \tif (repository_format_precious_objects)\n> -\t\tdie(\"cannot prune in a precious-objects repo\");\n> +\t\tdie(_(\"cannot prune in a precious-objects repo\"));\n>  \n>  \twhile (argc--) {\n>  \t\tunsigned char sha1[20];\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 8ae7fe5..3beda2c 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -194,7 +194,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \t\t\t\tgit_repack_usage, 0);\n>  \n>  \tif (delete_redundant && repository_format_precious_objects)\n> -\t\tdie(\"cannot repack in a precious-objects repo\");\n> +\t\tdie(_(\"cannot delete packs in a precious-objects repo\"));\n>  \n>  \tif (pack_kept_objects < 0)\n>  \t\tpack_kept_objects = write_bitmaps;\n"},{"id":"264827","messageId":"20150625100737.GA26795@peff.net","threadId":"39702","inReplyTo":"xmqqvbed9gf7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] introduce \"preciousObjects\" repository extension","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-25T10:07:37Z","receivedAt":"2015-06-25T10:07:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 24, 2015 at 10:15:08AM -0700, Junio C Hamano wrote:\n\n> > I agree that would be better. I originally just blocked all use of\n> > git-repack, but at the last minute softened it to just \"repack -d\". I'm\n> > not sure if that would actually help anyone in practice. Sure, doing\n> > \"git repack\" without any options is not destructive, but I wonder if\n> > anybody actually does it.\n> \n> Hmph, if you cannot afford to lose objects that are unreachable from\n> your refs (because you know your repository has borrowers) but are\n> suffering from too many packs, wouldn't \"repack -a\" be the most\n> natural thing to do?  Maybe I am biased, but \"git gc\" is not the\n> first thing that comes to my mind in that situation.\n\nMy assumption was that people fall into one of two categories:\n\n  - people who just run `git gc`\n\n  - people who are doing something clever, and will use `git\n    repack` to do a full repack after making sure it is safe to do so.\n\n    E.g., after \"clone -s\", it might be OK to do:\n\n      for i in ../*.git; do\n        git fetch $i +refs/*:refs/remotes/$i/*\n      done\n      git -c extensions.preciousObjects=false repack -ad\n\n    But only the user can make that decision; git does not know whether\n    \"../*.git\" is the complete set of children.\n\nCertainly \"git repack -a\" is a safe stopgap in the shared-object parent,\nbut eventually you will want to do the clever thing. :)\n\nI think it is OK to use this patch as a starting point, and for people\nto loosen the rules later if there is a combination of repack flags that\nare safe to run but not covered by the current logic (there is no\nregression, since preciousObjects is a new extension, and going forward\nit is OK to allow new safe things to open up workflows, but not the\nother way around).\n\nIt may even be that the current patch even allows any sane workflow; I\nam only claiming that I did not think too hard on it, and tried to err\non the side of safety, and allowing the workflow above.\n\n-Peff\n"}]}