[PATCH] add core.usereplacerefs config option

[PATCH] add core.usereplacerefs config option

[Jeff King]

We can already disable replace refs using a command line
option or environment variable, but those are awkward to
apply universally. Let's add a config option to do the same
thing.

That raises the question of why one might want to do so
universally. The answer is that replace refs violate the
immutability of objects. For instance, if you wanted to
cache the diff between commit XYZ and its parent, then in
theory that never changes; the hash XYZ represents the total
state. But replace refs violate that; pushing up a new ref
may create a completely new diff.

The obvious "if it hurts, don't do it" answer is not to
create replace refs if you're doing this kind of caching.
But for a site hosting arbitrary repositories, they may want
to allow users to share replace refs with each other, but
not actually respect them on the site (because the caching
is more important than the replace feature).

Signed-off-by: Jeff King <peff@peff.net>
---
We've been using this patch for about 4 years at GitHub. I'm not sure
why I never sent it upstream until now, since it's pretty trivial.

I think this should interact OK with Stefan's recent c3c36d7de2
(replace-object: check_replace_refs is safe in multi repo environment,
2018-04-11), because we still consider check_replace_refs before doing
anything in lookup_replace_object(). So even if you accidentally
_loaded_ the replace refs due to a timing issue (e.g., looking at them
before reading config) we still wouldn't respect them as long as you've
loaded config by the time you're actually looking at objects.

I followed the existing style of t6050 here, but it looks like it could
use some modernization (indent with spaces, and piping git command
output losing the exit codes).

 Documentation/config.txt | 5 +++++
 config.c                 | 5 +++++
 t/t6050-replace.sh       | 6 ++++++
 3 files changed, 16 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1cc18a828c..92b277d27b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -908,6 +908,11 @@ core.commitGraph::
 	Enable git commit graph feature. Allows reading from the
 	commit-graph file.
 
+core.useReplaceRefs::
+	If set to `false`, behave as if the `--no-replace-objects`
+	option was given on the command line. See linkgit:git[1] and
+	linkgit:git-replace[1] for more information.
+
 core.sparseCheckout::
 	Enable "sparse checkout" feature. See section "Sparse checkout" in
 	linkgit:git-read-tree[1] for more information.
diff --git a/config.c b/config.c
index f4a208a166..ce103ebc20 100644
--- a/config.c
+++ b/config.c
@@ -1346,6 +1346,11 @@ static int git_default_core_config(const char *var, const char *value)
 					 var, value);
 	}
 
+	if (!strcmp(var, "core.usereplacerefs")) {
+		check_replace_refs = git_config_bool(var, value);
+		return 0;
+	}
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index aa3e249639..86374a9c52 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -113,6 +113,12 @@ test_expect_success 'test GIT_NO_REPLACE_OBJECTS env variable' '
      GIT_NO_REPLACE_OBJECTS=1 git show $HASH2 | grep "A U Thor"
 '
 
+test_expect_success 'test core.usereplacerefs config option' '
+	test_config core.usereplacerefs false &&
+	git cat-file commit $HASH2 | grep "author A U Thor" &&
+	git show $HASH2 | grep "A U Thor"
+'
+
 cat >tag.sig <<EOF
 object $HASH2
 type commit
-- 
2.18.0.433.gb9621797ee

Re: [PATCH] add core.usereplacerefs config option

[Derrick Stolee]

On 7/18/2018 4:17 PM, Jeff King wrote:
> We can already disable replace refs using a command line
> option or environment variable, but those are awkward to
> apply universally. Let's add a config option to do the same
> thing.
>
> That raises the question of why one might want to do so
> universally. The answer is that replace refs violate the
> immutability of objects. For instance, if you wanted to
> cache the diff between commit XYZ and its parent, then in
> theory that never changes; the hash XYZ represents the total
> state. But replace refs violate that; pushing up a new ref
> may create a completely new diff.
>
> The obvious "if it hurts, don't do it" answer is not to
> create replace refs if you're doing this kind of caching.
> But for a site hosting arbitrary repositories, they may want
> to allow users to share replace refs with each other, but
> not actually respect them on the site (because the caching
> is more important than the replace feature).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> We've been using this patch for about 4 years at GitHub. I'm not sure
> why I never sent it upstream until now, since it's pretty trivial.
>
> I think this should interact OK with Stefan's recent c3c36d7de2
> (replace-object: check_replace_refs is safe in multi repo environment,
> 2018-04-11), because we still consider check_replace_refs before doing
> anything in lookup_replace_object(). So even if you accidentally
> _loaded_ the replace refs due to a timing issue (e.g., looking at them
> before reading config) we still wouldn't respect them as long as you've
> loaded config by the time you're actually looking at objects.
>
> I followed the existing style of t6050 here, but it looks like it could
> use some modernization (indent with spaces, and piping git command
> output losing the exit codes).
>
>   Documentation/config.txt | 5 +++++
>   config.c                 | 5 +++++
>   t/t6050-replace.sh       | 6 ++++++
>   3 files changed, 16 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 1cc18a828c..92b277d27b 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -908,6 +908,11 @@ core.commitGraph::
>   	Enable git commit graph feature. Allows reading from the
>   	commit-graph file.
>   
> +core.useReplaceRefs::
> +	If set to `false`, behave as if the `--no-replace-objects`
> +	option was given on the command line. See linkgit:git[1] and
> +	linkgit:git-replace[1] for more information.
> +
>   core.sparseCheckout::
>   	Enable "sparse checkout" feature. See section "Sparse checkout" in
>   	linkgit:git-read-tree[1] for more information.
> diff --git a/config.c b/config.c
> index f4a208a166..ce103ebc20 100644
> --- a/config.c
> +++ b/config.c
> @@ -1346,6 +1346,11 @@ static int git_default_core_config(const char *var, const char *value)
>   					 var, value);
>   	}
>   
> +	if (!strcmp(var, "core.usereplacerefs")) {
> +		check_replace_refs = git_config_bool(var, value);
> +		return 0;
> +	}
> +
>   	/* Add other config variables here and to Documentation/config.txt. */
>   	return 0;
>   }
> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
> index aa3e249639..86374a9c52 100755
> --- a/t/t6050-replace.sh
> +++ b/t/t6050-replace.sh
> @@ -113,6 +113,12 @@ test_expect_success 'test GIT_NO_REPLACE_OBJECTS env variable' '
>        GIT_NO_REPLACE_OBJECTS=1 git show $HASH2 | grep "A U Thor"
>   '
>   
> +test_expect_success 'test core.usereplacerefs config option' '
> +	test_config core.usereplacerefs false &&
> +	git cat-file commit $HASH2 | grep "author A U Thor" &&
> +	git show $HASH2 | grep "A U Thor"
> +'
> +
>   cat >tag.sig <<EOF
>   object $HASH2
>   type commit

This patch looks good to me. The only thing I saw was when I ran 'git 
grep check_replace_refs' and saw the following in environment.c:

     int check_replace_refs = 1; /* NEEDSWORK: rename to 
read_replace_refs */

This does help me feel confident that the case where the config value is 
missing will default to 'yes, please check replace refs', but also the 
NEEDSWORK could be something to either (1) do, or (2) remove the 
comment. Neither needs to happen as part of this patch.

Reviewed-by: Derrick Stolee <dstolee@microsoft.com>

Re: [PATCH] add core.usereplacerefs config option

[Jeff King]

On Wed, Jul 18, 2018 at 04:23:20PM -0400, Derrick Stolee wrote:

> This patch looks good to me. The only thing I saw was when I ran 'git grep
> check_replace_refs' and saw the following in environment.c:
> 
>     int check_replace_refs = 1; /* NEEDSWORK: rename to read_replace_refs */
> 
> This does help me feel confident that the case where the config value is
> missing will default to 'yes, please check replace refs', but also the
> NEEDSWORK could be something to either (1) do, or (2) remove the comment.
> Neither needs to happen as part of this patch.

Yeah, it was actually that comment that led me to Stefan's recent
c3c36d7de2 (replace-object: check_replace_refs is safe in multi repo
environment, 2018-04-11).

And ironically, back when I originally wrote this patch, it _was_ called
read_replace_refs. That changed in afc711b8e1 (rename read_replace_refs
to check_replace_refs, 2014-02-18), which was in turn picking up a
leftover from e1111cef23 (inline lookup_replace_object() calls,
2011-05-15).

Since Stefan's patch logically undoes e1111cef23, I think that's why he
put in the comment to move back to the old name.

Personally, I do not find one name any more informative than the other,
and would be happy to leave it as-is (dropping the comment).

But I'm also fine with following through on the "do". According to
c3c36d7de2, that was waiting for a calmer time in the code base. I guess
the best way to find out is to write the patch and see how terribly it
conflicts with pu. :)

> Reviewed-by: Derrick Stolee <dstolee@microsoft.com>

Thanks!

-Peff

Re: [PATCH] add core.usereplacerefs config option

[Stefan Beller]

On Wed, Jul 18, 2018 at 1:31 PM Jeff King <peff@peff.net> wrote:
>
> On Wed, Jul 18, 2018 at 04:23:20PM -0400, Derrick Stolee wrote:
>
> > This patch looks good to me. The only thing I saw was when I ran 'git grep
> > check_replace_refs' and saw the following in environment.c:
> >
> >     int check_replace_refs = 1; /* NEEDSWORK: rename to read_replace_refs */
> >
> > This does help me feel confident that the case where the config value is
> > missing will default to 'yes, please check replace refs', but also the
> > NEEDSWORK could be something to either (1) do, or (2) remove the comment.
> > Neither needs to happen as part of this patch.
>
> Yeah, it was actually that comment that led me to Stefan's recent
> c3c36d7de2 (replace-object: check_replace_refs is safe in multi repo
> environment, 2018-04-11).
>
> And ironically, back when I originally wrote this patch, it _was_ called
> read_replace_refs. That changed in afc711b8e1 (rename read_replace_refs
> to check_replace_refs, 2014-02-18), which was in turn picking up a
> leftover from e1111cef23 (inline lookup_replace_object() calls,
> 2011-05-15).
>
> Since Stefan's patch logically undoes e1111cef23, I think that's why he
> put in the comment to move back to the old name.

I think so, too

> Personally, I do not find one name any more informative than the other,
> and would be happy to leave it as-is (dropping the comment).
>
> But I'm also fine with following through on the "do". According to
> c3c36d7de2, that was waiting for a calmer time in the code base. I guess
> the best way to find out is to write the patch and see how terribly it
> conflicts with pu. :)

As someone burned by coming up with renaming (or rather lack thereof),
I'd happily watch from the sideline this time.

I think this patch is good; ship it. :-)

Thanks,
Stefan

Re: [PATCH] add core.usereplacerefs config option

[Jeff King]

On Wed, Jul 18, 2018 at 04:31:52PM -0400, Jeff King wrote:

> Since Stefan's patch logically undoes e1111cef23, I think that's why he
> put in the comment to move back to the old name.
> 
> Personally, I do not find one name any more informative than the other,
> and would be happy to leave it as-is (dropping the comment).
> 
> But I'm also fine with following through on the "do". According to
> c3c36d7de2, that was waiting for a calmer time in the code base. I guess
> the best way to find out is to write the patch and see how terribly it
> conflicts with pu. :)

It turns out there are no conflicts right now, aside from the patch I
just sent. And perhaps your commit-graph work is going to add another
reference, if you take my suggestion in the other thread. ;)

So I remain ambivalent, but here is the patch to do so, with mine now on
top (the only difference is s/read/check/ in the variable name).

But note that while changing this, I noticed a leftover bit from
c3c36d7de2 that should be dealt with in either case. I put that at the
front of the series.

  [1/3]: check_replace_refs: fix outdated comment
  [2/3]: check_replace_refs: rename to read_replace_refs
  [3/3]: add core.usereplacerefs config option

 Documentation/config.txt | 5 +++++
 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 +-
 cache.h                  | 6 ++----
 config.c                 | 5 +++++
 environment.c            | 4 ++--
 git.c                    | 2 +-
 log-tree.c               | 2 +-
 replace-object.c         | 2 +-
 replace-object.h         | 2 +-
 t/t6050-replace.sh       | 6 ++++++
 16 files changed, 31 insertions(+), 17 deletions(-)

-Peff

[PATCH 1/3] check_replace_refs: fix outdated comment

[Jeff King]

Commit afc711b8e1 (rename read_replace_refs to
check_replace_refs, 2014-02-18) added a comment mentioning
that check_replace_refs is set in two ways:

  - from user intent via --no-replace-objects, etc

  - after seeing there are no replace refs to respect

Since c3c36d7de2 (replace-object: check_replace_refs is safe
in multi repo environment, 2018-04-11) the second is no
longer true. Let's drop that part of the comment.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index d49092d94d..6365fd6c0f 100644
--- a/cache.h
+++ b/cache.h
@@ -804,9 +804,7 @@ void reset_shared_repository(void);
  * Do replace refs need to be checked this run?  This variable is
  * initialized to true unless --no-replace-object is used or
  * $GIT_NO_REPLACE_OBJECTS is set, but is set to false by some
- * commands that do not want replace references to be active.  As an
- * optimization it is also set to false if replace references have
- * been sought but there were none.
+ * commands that do not want replace references to be active.
  */
 extern int check_replace_refs;
 extern char *git_replace_ref_base;
-- 
2.18.0.433.gb9621797ee

[PATCH 2/3] check_replace_refs: rename to read_replace_refs

[Jeff King]

This was added as a NEEDSWORK in 3c36d7de2 (replace-object:
check_replace_refs is safe in multi repo environment,
2018-04-11), waiting for a calmer period. Since doing so now
doesn't conflict with anything in 'pu', it seems as good a
time as any.

Signed-off-by: Jeff King <peff@peff.net>
---
 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 +-
 cache.h                  | 2 +-
 environment.c            | 4 ++--
 git.c                    | 2 +-
 log-tree.c               | 2 +-
 replace-object.c         | 2 +-
 replace-object.h         | 2 +-
 13 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 3ad4f160f9..0c3cbb86fc 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -689,7 +689,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 	fetch_if_missing = 0;
 
 	errors_found = 0;
-	check_replace_refs = 0;
+	read_replace_refs = 0;
 
 	argc = parse_options(argc, argv, prefix, fsck_opts, fsck_usage, 0);
 
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 74fe2973e1..a24faa0e51 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1679,7 +1679,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage(index_pack_usage);
 
-	check_replace_refs = 0;
+	read_replace_refs = 0;
 	fsck_options.walk = mark_link;
 
 	reset_pack_idx_option(&opts);
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 71056d8294..63ee2c14c9 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3185,7 +3185,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	if (DFS_NUM_STATES > (1 << OE_DFS_STATE_BITS))
 		BUG("too many dfs states, increase OE_DFS_STATE_BITS");
 
-	check_replace_refs = 0;
+	read_replace_refs = 0;
 
 	reset_pack_idx_option(&pack_idx_opts);
 	git_config(git_pack_config, NULL);
diff --git a/builtin/prune.c b/builtin/prune.c
index 518ffbea13..a1307b2f0d 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -117,7 +117,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
 
 	expire = TIME_MAX;
 	save_commit_buffer = 0;
-	check_replace_refs = 0;
+	read_replace_refs = 0;
 	ref_paranoia = 1;
 	init_revisions(&revs, prefix);
 
diff --git a/builtin/replace.c b/builtin/replace.c
index 6da2411e14..9f4a616750 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -544,7 +544,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	check_replace_refs = 0;
+	read_replace_refs = 0;
 	git_config(git_default_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 6e81ca8ca2..5f76e6fe4e 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -512,7 +512,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)
 	int i;
 	struct object_id oid;
 
-	check_replace_refs = 0;
+	read_replace_refs = 0;
 
 	git_config(git_default_config, NULL);
 
diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index decde5a3b1..42dc4da5a1 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -31,7 +31,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 	};
 
 	packet_trace_identity("upload-pack");
-	check_replace_refs = 0;
+	read_replace_refs = 0;
 
 	argc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0);
 
diff --git a/cache.h b/cache.h
index 6365fd6c0f..f98eb09441 100644
--- a/cache.h
+++ b/cache.h
@@ -806,7 +806,7 @@ void reset_shared_repository(void);
  * $GIT_NO_REPLACE_OBJECTS is set, but is set to false by some
  * commands that do not want replace references to be active.
  */
-extern int check_replace_refs;
+extern int read_replace_refs;
 extern char *git_replace_ref_base;
 
 extern int fsync_object_files;
diff --git a/environment.c b/environment.c
index 2a6de2330b..05d0d469b4 100644
--- a/environment.c
+++ b/environment.c
@@ -51,7 +51,7 @@ const char *editor_program;
 const char *askpass_program;
 const char *excludes_file;
 enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
-int check_replace_refs = 1; /* NEEDSWORK: rename to read_replace_refs */
+int read_replace_refs = 1;
 char *git_replace_ref_base;
 enum eol core_eol = EOL_UNSET;
 int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
@@ -183,7 +183,7 @@ void setup_git_env(const char *git_dir)
 	argv_array_clear(&to_free);
 
 	if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
-		check_replace_refs = 0;
+		read_replace_refs = 0;
 	replace_ref_base = getenv(GIT_REPLACE_REF_BASE_ENVIRONMENT);
 	free(git_replace_ref_base);
 	git_replace_ref_base = xstrdup(replace_ref_base ? replace_ref_base
diff --git a/git.c b/git.c
index 9dbe6ffaa7..c304a0626b 100644
--- a/git.c
+++ b/git.c
@@ -164,7 +164,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--no-replace-objects")) {
-			check_replace_refs = 0;
+			read_replace_refs = 0;
 			setenv(NO_REPLACE_OBJECTS_ENVIRONMENT, "1", 1);
 			if (envchanged)
 				*envchanged = 1;
diff --git a/log-tree.c b/log-tree.c
index d3a43e29cd..95fa51a652 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -90,7 +90,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,
 
 	if (starts_with(refname, git_replace_ref_base)) {
 		struct object_id original_oid;
-		if (!check_replace_refs)
+		if (!read_replace_refs)
 			return 0;
 		if (get_oid_hex(refname + strlen(git_replace_ref_base),
 				&original_oid)) {
diff --git a/replace-object.c b/replace-object.c
index 801b5c1678..4162df6324 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -51,7 +51,7 @@ static void prepare_replace_object(struct repository *r)
  * replacement object's name (replaced recursively, if necessary).
  * The return value is either oid or a pointer to a
  * permanently-allocated value.  This function always respects replace
- * references, regardless of the value of check_replace_refs.
+ * references, regardless of the value of read_replace_refs.
  */
 const struct object_id *do_lookup_replace_object(struct repository *r,
 						 const struct object_id *oid)
diff --git a/replace-object.h b/replace-object.h
index f996de3d62..9345e105dd 100644
--- a/replace-object.h
+++ b/replace-object.h
@@ -26,7 +26,7 @@ extern const struct object_id *do_lookup_replace_object(struct repository *r,
 static inline const struct object_id *lookup_replace_object(struct repository *r,
 							    const struct object_id *oid)
 {
-	if (!check_replace_refs ||
+	if (!read_replace_refs ||
 	    (r->objects->replace_map &&
 	     r->objects->replace_map->map.tablesize == 0))
 		return oid;
-- 
2.18.0.433.gb9621797ee

[PATCH 3/3] add core.usereplacerefs config option

[Jeff King]

We can already disable replace refs using a command line
option or environment variable, but those are awkward to
apply universally. Let's add a config option to do the same
thing.

That raises the question of why one might want to do so
universally. The answer is that replace refs violate the
immutability of objects. For instance, if you wanted to
cache the diff between commit XYZ and its parent, then in
theory that never changes; the hash XYZ represents the total
state. But replace refs violate that; pushing up a new ref
may create a completely new diff.

The obvious "if it hurts, don't do it" answer is not to
create replace refs if you're doing this kind of caching.
But for a site hosting arbitrary repositories, they may want
to allow users to share replace refs with each other, but
not actually respect them on the site (because the caching
is more important than the replace feature).

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/config.txt | 5 +++++
 config.c                 | 5 +++++
 t/t6050-replace.sh       | 6 ++++++
 3 files changed, 16 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1cc18a828c..92b277d27b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -908,6 +908,11 @@ core.commitGraph::
 	Enable git commit graph feature. Allows reading from the
 	commit-graph file.
 
+core.useReplaceRefs::
+	If set to `false`, behave as if the `--no-replace-objects`
+	option was given on the command line. See linkgit:git[1] and
+	linkgit:git-replace[1] for more information.
+
 core.sparseCheckout::
 	Enable "sparse checkout" feature. See section "Sparse checkout" in
 	linkgit:git-read-tree[1] for more information.
diff --git a/config.c b/config.c
index f4a208a166..49170c4556 100644
--- a/config.c
+++ b/config.c
@@ -1346,6 +1346,11 @@ static int git_default_core_config(const char *var, const char *value)
 					 var, value);
 	}
 
+	if (!strcmp(var, "core.usereplacerefs")) {
+		read_replace_refs = git_config_bool(var, value);
+		return 0;
+	}
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index aa3e249639..86374a9c52 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -113,6 +113,12 @@ test_expect_success 'test GIT_NO_REPLACE_OBJECTS env variable' '
      GIT_NO_REPLACE_OBJECTS=1 git show $HASH2 | grep "A U Thor"
 '
 
+test_expect_success 'test core.usereplacerefs config option' '
+	test_config core.usereplacerefs false &&
+	git cat-file commit $HASH2 | grep "author A U Thor" &&
+	git show $HASH2 | grep "A U Thor"
+'
+
 cat >tag.sig <<EOF
 object $HASH2
 type commit
-- 
2.18.0.433.gb9621797ee

Re: [PATCH 1/3] check_replace_refs: fix outdated comment

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Commit afc711b8e1 (rename read_replace_refs to
> check_replace_refs, 2014-02-18) added a comment mentioning
> that check_replace_refs is set in two ways:
>
>   - from user intent via --no-replace-objects, etc
>
>   - after seeing there are no replace refs to respect
>
> Since c3c36d7de2 (replace-object: check_replace_refs is safe
> in multi repo environment, 2018-04-11) the second is no
> longer true. Let's drop that part of the comment.
>

I wonder if c3c36d7de2 should be corrected so that we would have
this bit per in-core repository instance?  When the superproject and
its three submodules all have an in-core repository instance each,
and only one of the submodules uses replace ref, the original
optimization disabled by that patch would be an obvious thing to
have per repository.

But that is a tangent.  What this patch does is correct without
any doubt.


> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  cache.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index d49092d94d..6365fd6c0f 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -804,9 +804,7 @@ void reset_shared_repository(void);
>   * Do replace refs need to be checked this run?  This variable is
>   * initialized to true unless --no-replace-object is used or
>   * $GIT_NO_REPLACE_OBJECTS is set, but is set to false by some
> - * commands that do not want replace references to be active.  As an
> - * optimization it is also set to false if replace references have
> - * been sought but there were none.
> + * commands that do not want replace references to be active.
>   */
>  extern int check_replace_refs;
>  extern char *git_replace_ref_base;

Re: [PATCH 2/3] check_replace_refs: rename to read_replace_refs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This was added as a NEEDSWORK in 3c36d7de2 (replace-object:

I'll do s/3c36d7de2/c&/; while queuing.

Like you, I do not think one is vastly better than the other between
these two names, but since a patch has been written, so ...

> check_replace_refs is safe in multi repo environment,
> 2018-04-11), waiting for a calmer period. Since doing so now
> doesn't conflict with anything in 'pu', it seems as good a
> time as any.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  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 +-
>  cache.h                  | 2 +-
>  environment.c            | 4 ++--
>  git.c                    | 2 +-
>  log-tree.c               | 2 +-
>  replace-object.c         | 2 +-
>  replace-object.h         | 2 +-
>  13 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 3ad4f160f9..0c3cbb86fc 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -689,7 +689,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
>  	fetch_if_missing = 0;
>  
>  	errors_found = 0;
> -	check_replace_refs = 0;
> +	read_replace_refs = 0;
>  
>  	argc = parse_options(argc, argv, prefix, fsck_opts, fsck_usage, 0);
>  
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 74fe2973e1..a24faa0e51 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1679,7 +1679,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
>  	if (argc == 2 && !strcmp(argv[1], "-h"))
>  		usage(index_pack_usage);
>  
> -	check_replace_refs = 0;
> +	read_replace_refs = 0;
>  	fsck_options.walk = mark_link;
>  
>  	reset_pack_idx_option(&opts);
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 71056d8294..63ee2c14c9 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3185,7 +3185,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  	if (DFS_NUM_STATES > (1 << OE_DFS_STATE_BITS))
>  		BUG("too many dfs states, increase OE_DFS_STATE_BITS");
>  
> -	check_replace_refs = 0;
> +	read_replace_refs = 0;
>  
>  	reset_pack_idx_option(&pack_idx_opts);
>  	git_config(git_pack_config, NULL);
> diff --git a/builtin/prune.c b/builtin/prune.c
> index 518ffbea13..a1307b2f0d 100644
> --- a/builtin/prune.c
> +++ b/builtin/prune.c
> @@ -117,7 +117,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
>  
>  	expire = TIME_MAX;
>  	save_commit_buffer = 0;
> -	check_replace_refs = 0;
> +	read_replace_refs = 0;
>  	ref_paranoia = 1;
>  	init_revisions(&revs, prefix);
>  
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 6da2411e14..9f4a616750 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -544,7 +544,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
>  		OPT_END()
>  	};
>  
> -	check_replace_refs = 0;
> +	read_replace_refs = 0;
>  	git_config(git_default_config, NULL);
>  
>  	argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);
> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
> index 6e81ca8ca2..5f76e6fe4e 100644
> --- a/builtin/unpack-objects.c
> +++ b/builtin/unpack-objects.c
> @@ -512,7 +512,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)
>  	int i;
>  	struct object_id oid;
>  
> -	check_replace_refs = 0;
> +	read_replace_refs = 0;
>  
>  	git_config(git_default_config, NULL);
>  
> diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
> index decde5a3b1..42dc4da5a1 100644
> --- a/builtin/upload-pack.c
> +++ b/builtin/upload-pack.c
> @@ -31,7 +31,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
>  	};
>  
>  	packet_trace_identity("upload-pack");
> -	check_replace_refs = 0;
> +	read_replace_refs = 0;
>  
>  	argc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0);
>  
> diff --git a/cache.h b/cache.h
> index 6365fd6c0f..f98eb09441 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -806,7 +806,7 @@ void reset_shared_repository(void);
>   * $GIT_NO_REPLACE_OBJECTS is set, but is set to false by some
>   * commands that do not want replace references to be active.
>   */
> -extern int check_replace_refs;
> +extern int read_replace_refs;
>  extern char *git_replace_ref_base;
>  
>  extern int fsync_object_files;
> diff --git a/environment.c b/environment.c
> index 2a6de2330b..05d0d469b4 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -51,7 +51,7 @@ const char *editor_program;
>  const char *askpass_program;
>  const char *excludes_file;
>  enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
> -int check_replace_refs = 1; /* NEEDSWORK: rename to read_replace_refs */
> +int read_replace_refs = 1;
>  char *git_replace_ref_base;
>  enum eol core_eol = EOL_UNSET;
>  int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
> @@ -183,7 +183,7 @@ void setup_git_env(const char *git_dir)
>  	argv_array_clear(&to_free);
>  
>  	if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
> -		check_replace_refs = 0;
> +		read_replace_refs = 0;
>  	replace_ref_base = getenv(GIT_REPLACE_REF_BASE_ENVIRONMENT);
>  	free(git_replace_ref_base);
>  	git_replace_ref_base = xstrdup(replace_ref_base ? replace_ref_base
> diff --git a/git.c b/git.c
> index 9dbe6ffaa7..c304a0626b 100644
> --- a/git.c
> +++ b/git.c
> @@ -164,7 +164,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  			if (envchanged)
>  				*envchanged = 1;
>  		} else if (!strcmp(cmd, "--no-replace-objects")) {
> -			check_replace_refs = 0;
> +			read_replace_refs = 0;
>  			setenv(NO_REPLACE_OBJECTS_ENVIRONMENT, "1", 1);
>  			if (envchanged)
>  				*envchanged = 1;
> diff --git a/log-tree.c b/log-tree.c
> index d3a43e29cd..95fa51a652 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -90,7 +90,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,
>  
>  	if (starts_with(refname, git_replace_ref_base)) {
>  		struct object_id original_oid;
> -		if (!check_replace_refs)
> +		if (!read_replace_refs)
>  			return 0;
>  		if (get_oid_hex(refname + strlen(git_replace_ref_base),
>  				&original_oid)) {
> diff --git a/replace-object.c b/replace-object.c
> index 801b5c1678..4162df6324 100644
> --- a/replace-object.c
> +++ b/replace-object.c
> @@ -51,7 +51,7 @@ static void prepare_replace_object(struct repository *r)
>   * replacement object's name (replaced recursively, if necessary).
>   * The return value is either oid or a pointer to a
>   * permanently-allocated value.  This function always respects replace
> - * references, regardless of the value of check_replace_refs.
> + * references, regardless of the value of read_replace_refs.
>   */
>  const struct object_id *do_lookup_replace_object(struct repository *r,
>  						 const struct object_id *oid)
> diff --git a/replace-object.h b/replace-object.h
> index f996de3d62..9345e105dd 100644
> --- a/replace-object.h
> +++ b/replace-object.h
> @@ -26,7 +26,7 @@ extern const struct object_id *do_lookup_replace_object(struct repository *r,
>  static inline const struct object_id *lookup_replace_object(struct repository *r,
>  							    const struct object_id *oid)
>  {
> -	if (!check_replace_refs ||
> +	if (!read_replace_refs ||
>  	    (r->objects->replace_map &&
>  	     r->objects->replace_map->map.tablesize == 0))
>  		return oid;

Re: [PATCH 1/3] check_replace_refs: fix outdated comment

[Jeff King]

On Wed, Jul 18, 2018 at 03:41:10PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Commit afc711b8e1 (rename read_replace_refs to
> > check_replace_refs, 2014-02-18) added a comment mentioning
> > that check_replace_refs is set in two ways:
> >
> >   - from user intent via --no-replace-objects, etc
> >
> >   - after seeing there are no replace refs to respect
> >
> > Since c3c36d7de2 (replace-object: check_replace_refs is safe
> > in multi repo environment, 2018-04-11) the second is no
> > longer true. Let's drop that part of the comment.
> >
> 
> I wonder if c3c36d7de2 should be corrected so that we would have
> this bit per in-core repository instance?  When the superproject and
> its three submodules all have an in-core repository instance each,
> and only one of the submodules uses replace ref, the original
> optimization disabled by that patch would be an obvious thing to
> have per repository.

I think c3c36d7de2 represents that bit by checking the non-emptiness of
the refs/replace map.

What it doesn't do is represent a per-repository notion of "are we even
even interested in the replace feature". Right now there are probably
few cases where that would matter. It comes from the environment, so
it's automatically passed down to children anyway. You could tweak the
environment, but if you did so you'd probably be in a separate process
anyway, so none of this multi-repo-in-one-process stuff would matter
either way.  But there may be cases with programs which manually tweak
the value and then recurse into a submodule.

It would be made slightly worse with my config patch, if you expected:

  git config core.usereplacerefs true
  git -C submodule core.usereplacerefs false
  git log --submodule=log

to respect replace refs in the outer repo, but not the inner one.

-Peff

Re: [PATCH 2/3] check_replace_refs: rename to read_replace_refs

[Jeff King]

On Wed, Jul 18, 2018 at 03:44:36PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This was added as a NEEDSWORK in 3c36d7de2 (replace-object:
> 
> I'll do s/3c36d7de2/c&/; while queuing.

Oops, vi strikes again, I think.

> Like you, I do not think one is vastly better than the other between
> these two names, but since a patch has been written, so ...

:)

-Peff