[PATCH v3 0/3] Create stronger guard rails on replace refs

["Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>]

(This series is based on tb/pack-bitmap-traversal-with-boundary due to
wanting to modify prepare_repo_settings() in a similar way.)

The replace-refs can be ignored via the core.useReplaceRefs=false config
setting. This setting is possible to miss in some Git commands if they do
not load default config at the appropriate time. See [1] for a recent
example of this.

[1]
https://lore.kernel.org/git/pull.1530.git.1683745654800.gitgitgadget@gmail.com/

This series aims to avoid this kind of error from happening in the future.
The idea is to encapsulate the setting in such a way that we can guarantee
that config has been checked before using the in-memory value.

Further, we must be careful that some Git commands want to disable replace
refs unconditionally, as if GIT_NO_REPLACE_REFS was enabled in the
environment.

The approach taken here is to split the global into two different sources.
First, read_replace_refs is kept (but moved to replace-objects.c scope) and
reflects whether or not the feature is permitted by the environment and the
current command. Second, a new value is added to repo-settings and this is
checked after using prepare_repo_settings() to guarantee the config has been
read.

This presents a potential behavior change, in that now core.useReplaceRefs
is specific to each in-memory repository instead of applying the
superproject value to all submodules. I could not find a Git command that
has multiple in-memory repositories and follows OIDs to object contents, so
I'm not sure how to demonstrate it in a test.

Here is the breakdown of the series:

 * Patch 1 creates disable_replace_refs() to encapsulate the global
   disabling of the feature.
 * Patch 2 creates replace_refs_enabled() to check if the feature is enabled
   (with respect to a given repository).
 * Patch 3 creates the value in repo-settings as well as ensures that the
   repo settings have been prepared before accessing the value within
   replace_refs_enabled(). A test is added to demonstrate how the config
   value is now scoped on a per-repository basis.


Updates in v3
=============

Thanks for the review on v2!

 * The removal of the global from environment.c is delayed to patch 3
   because config.c still assigns the value in patch 2.
 * The comment for the member in the repo_settings struct is modified for
   better grammar.


Updates in v2
=============

Thanks for the careful review on v1!

 * disable_replace_refs() now replaces "read_replace_refs = 0" in the exact
   same line to avoid possible behavior change.
 * Stale comments, include headers, and commit messages are updated to
   include the latest status.
 * Patch 3 contains a test of the repo-scoped value using 'git grep'.

Thanks, -Stolee

Derrick Stolee (3):
  repository: create disable_replace_refs()
  replace-objects: create wrapper around setting
  repository: create read_replace_refs setting

 builtin/cat-file.c                 |  2 +-
 builtin/commit-graph.c             |  2 +-
 builtin/fsck.c                     |  2 +-
 builtin/index-pack.c               |  2 +-
 builtin/pack-objects.c             |  2 +-
 builtin/prune.c                    |  2 +-
 builtin/replace.c                  |  2 +-
 builtin/unpack-objects.c           |  2 +-
 builtin/upload-pack.c              |  2 +-
 commit-graph.c                     |  4 +--
 config.c                           |  5 ----
 environment.c                      |  3 +--
 git.c                              |  2 +-
 log-tree.c                         |  2 +-
 replace-object.c                   | 28 ++++++++++++++++++++-
 replace-object.h                   | 30 +++++++++++++++-------
 repo-settings.c                    |  1 +
 repository.h                       |  9 +++++++
 t/t7814-grep-recurse-submodules.sh | 40 ++++++++++++++++++++++++++++++
 19 files changed, 111 insertions(+), 31 deletions(-)


base-commit: b0afdce5dab61f224fd66c13768facc36a7f8705
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1537%2Fderrickstolee%2Freplace-refs-safety-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1537/derrickstolee/replace-refs-safety-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1537

Range-diff vs v2:

 1:  0616fdbf303 = 1:  0616fdbf303 repository: create disable_replace_refs()
 2:  0831e7f8b5e ! 2:  4e75a76f5dd replace-objects: create wrapper around setting
     @@ Commit message
          set accidentally in other places, wrap it in a replace_refs_enabled()
          method.
      
     -    This allows us to remove the global from being recognized outside of
     -    replace-objects.c.
     +    Since we still assign this global in config.c, we are not able to remove
     +    the global scope of this variable and make it a static within
     +    replace-object.c. This will happen in a later change which will also
     +    prevent the variable from being read before it is initialized.
      
     -    Further, a future change will help prevent the variable from being read
     -    before it is initialized. Centralizing its access is an important first
     -    step.
     +    Centralizing read access to the variable is an important first step.
      
          Signed-off-by: Derrick Stolee <derrickstolee@github.com>
      
     @@ commit-graph.c: static struct commit_graph *alloc_commit_graph(void)
       		if (hashmap_get_size(&r->objects->replace_map->map))
       			return 0;
      
     - ## environment.c ##
     -@@ environment.c: const char *editor_program;
     - const char *askpass_program;
     - const char *excludes_file;
     - enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
     --int read_replace_refs = 1;
     - enum eol core_eol = EOL_UNSET;
     - int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
     - char *check_roundtrip_encoding = "SHIFT-JIS";
     -
       ## log-tree.c ##
      @@ log-tree.c: static int add_ref_decoration(const char *refname, const struct object_id *oid,
       
     @@ log-tree.c: static int add_ref_decoration(const char *refname, const struct obje
       				&original_oid)) {
      
       ## replace-object.c ##
     -@@ replace-object.c: const struct object_id *do_lookup_replace_object(struct repository *r,
     - 	die(_("replace depth too high for object %s"), oid_to_hex(oid));
     - }
     - 
     -+static int read_replace_refs = 1;
     -+
     - void disable_replace_refs(void)
     +@@ replace-object.c: void disable_replace_refs(void)
       {
       	read_replace_refs = 0;
       }
     @@ replace-object.h: void prepare_replace_object(struct repository *r);
       const struct object_id *do_lookup_replace_object(struct repository *r,
       						 const struct object_id *oid);
       
     -+
      +/*
      + * Some commands disable replace-refs unconditionally, and otherwise each
      + * repository could alter the core.useReplaceRefs config value.
 3:  4c7dbeb8c6d ! 3:  8b7c7714c8c repository: create read_replace_refs setting
     @@ Commit message
          method, if necessary. This solves the problem of forgetting to check the
          config, as we will check it before returning this value.
      
     +    Due to this encapsulation, the global can move to be static within
     +    replace-object.c.
     +
          There is an interesting behavior change possible here: we now have a
          repository-scoped understanding of this config value. Thus, if there was
          a command that recurses into submodules and might follow replace refs,
     @@ config.c: static int git_default_core_config(const char *var, const char *value,
       	return platform_core_config(var, value, cb);
       }
      
     + ## environment.c ##
     +@@ environment.c: const char *editor_program;
     + const char *askpass_program;
     + const char *excludes_file;
     + enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
     +-int read_replace_refs = 1;
     + enum eol core_eol = EOL_UNSET;
     + int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
     + char *check_roundtrip_encoding = "SHIFT-JIS";
     +
       ## replace-object.c ##
      @@ replace-object.c: void prepare_replace_object(struct repository *r)
        * replacement object's name (replaced recursively, if necessary).
     @@ replace-object.c: void prepare_replace_object(struct repository *r)
        */
       const struct object_id *do_lookup_replace_object(struct repository *r,
       						 const struct object_id *oid)
     +@@ replace-object.c: const struct object_id *do_lookup_replace_object(struct repository *r,
     + 	die(_("replace depth too high for object %s"), oid_to_hex(oid));
     + }
     + 
     ++/*
     ++ * This indicator determines whether replace references should be
     ++ * respected process-wide, regardless of which repository is being
     ++ * using at the time.
     ++ */
     ++static int read_replace_refs = 1;
     ++
     + void disable_replace_refs(void)
     + {
     + 	read_replace_refs = 0;
      @@ replace-object.c: void disable_replace_refs(void)
       
       int replace_refs_enabled(struct repository *r)
     @@ repository.h: struct repo_settings {
       	int pack_use_bitmap_boundary_traversal;
       
      +	/*
     -+	 * Has this repository have core.useReplaceRefs=true (on by
     ++	 * Does this repository have core.useReplaceRefs=true (on by
      +	 * default)? This provides a repository-scoped version of this
      +	 * config, though it could be disabled process-wide via some Git
      +	 * builtins or the --no-replace-objects option. See

-- 
gitgitgadget