[PATCH 0/5] teach replace objects to sha1_object_info_extended()

[PATCH 0/5] teach replace objects to sha1_object_info_extended()

[Christian Couder]

Here is a patch series to improve the way sha1_object_info_extended()
behaves when it is passed a replaced object.

This patch series was inspired by a sub thread in this discussion:

http://thread.gmane.org/gmane.comp.version-control.git/238118

Patches 1/5 and 2/5 are cleanups.
Patch 4/5 adds a test that fails.
Patch 5/5 adds a call to lookup_replace_object_extended() in
sha1_object_info_extended() and makes the previous test pass.

Christian Couder (5):
  replace_object: don't check read_replace_refs twice
  introduce lookup_replace_object_extended() to pass flags
  Add an "unsigned flags" parameter to sha1_object_info_extended()
  t6050: show that git cat-file --batch fails with replace objects
  sha1_file: perform object replacement in sha1_object_info_extended()

 builtin/cat-file.c |  2 +-
 cache.h            |  8 +++++++-
 replace_object.c   |  3 ---
 sha1_file.c        | 20 ++++++++++----------
 streaming.c        |  2 +-
 t/t6050-replace.sh |  5 +++++
 6 files changed, 24 insertions(+), 16 deletions(-)

-- 
1.8.4.1.561.g12affca

[PATCH 1/5] replace_object: don't check read_replace_refs twice

[Christian Couder]

Since e1111cef (inline lookup_replace_object() calls,
May 15 2011) the read_replace_refs global variable is
checked twice, once in lookup_replace_object() and
once again in do_lookup_replace_object().

As do_lookup_replace_object() is called only from
lookup_replace_object(), we can remove the check in
do_lookup_replace_object().

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 replace_object.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/replace_object.c b/replace_object.c
index d0b1548..cdcaf8c 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -97,9 +97,6 @@ const unsigned char *do_lookup_replace_object(const unsigned char *sha1)
 	int pos, depth = MAXREPLACEDEPTH;
 	const unsigned char *cur = sha1;
 
-	if (!read_replace_refs)
-		return sha1;
-
 	prepare_replace_object();
 
 	/* Try to recursively replace the object */
-- 
1.8.4.1.561.g12affca

[PATCH 2/5] introduce lookup_replace_object_extended() to pass flags

[Christian Couder]

Currently, there is only one caller to lookup_replace_object()
that can benefit from passing it some flags, but we expect
that there could be more.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 cache.h     | 6 ++++++
 sha1_file.c | 3 +--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index ce377e1..b845485 100644
--- a/cache.h
+++ b/cache.h
@@ -773,6 +773,12 @@ static inline const unsigned char *lookup_replace_object(const unsigned char *sh
 		return sha1;
 	return do_lookup_replace_object(sha1);
 }
+static inline const unsigned char *lookup_replace_object_extended(const unsigned char *sha1, unsigned flag)
+{
+	if (! (flag & READ_SHA1_FILE_REPLACE))
+		return sha1;
+	return lookup_replace_object(sha1);
+}
 
 /* Read and unpack a sha1 file into memory, write memory to a sha1 file */
 extern int sha1_object_info(const unsigned char *, unsigned long *);
diff --git a/sha1_file.c b/sha1_file.c
index 7dadd04..b0a3964 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2662,8 +2662,7 @@ void *read_sha1_file_extended(const unsigned char *sha1,
 	void *data;
 	char *path;
 	const struct packed_git *p;
-	const unsigned char *repl = (flag & READ_SHA1_FILE_REPLACE)
-		? lookup_replace_object(sha1) : sha1;
+	const unsigned char *repl = lookup_replace_object_extended(sha1, flag);
 
 	errno = 0;
 	data = read_object(repl, type, size);
-- 
1.8.4.1.561.g12affca

[PATCH 3/5] Add an "unsigned flags" parameter to sha1_object_info_extended()

[Christian Couder]

This parameter is not used yet, but it will be used to tell
sha1_object_info_extended() if it should perform object
replacement or not.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/cat-file.c | 2 +-
 cache.h            | 2 +-
 sha1_file.c        | 6 +++---
 streaming.c        | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index b2ca775..a2d3a9b 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -238,7 +238,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,
 		return 0;
 	}
 
-	if (sha1_object_info_extended(data->sha1, &data->info) < 0) {
+	if (sha1_object_info_extended(data->sha1, &data->info, READ_SHA1_FILE_REPLACE) < 0) {
 		printf("%s missing\n", obj_name);
 		fflush(stdout);
 		return 0;
diff --git a/cache.h b/cache.h
index b845485..3be03dc 100644
--- a/cache.h
+++ b/cache.h
@@ -1104,7 +1104,7 @@ struct object_info {
 		} packed;
 	} u;
 };
-extern int sha1_object_info_extended(const unsigned char *, struct object_info *);
+extern int sha1_object_info_extended(const unsigned char *, struct object_info *, unsigned flags);
 
 /* Dumb servers support */
 extern int update_server_info(int);
diff --git a/sha1_file.c b/sha1_file.c
index b0a3964..09e56ef 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2514,7 +2514,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,
 	return 0;
 }
 
-int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)
+int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi, unsigned flags)
 {
 	struct cached_object *co;
 	struct pack_entry e;
@@ -2548,7 +2548,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)
 	rtype = packed_object_info(e.p, e.offset, oi);
 	if (rtype < 0) {
 		mark_bad_packed_object(e.p, sha1);
-		return sha1_object_info_extended(sha1, oi);
+		return sha1_object_info_extended(sha1, oi, 0);
 	} else if (in_delta_base_cache(e.p, e.offset)) {
 		oi->whence = OI_DBCACHED;
 	} else {
@@ -2570,7 +2570,7 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)
 
 	oi.typep = &type;
 	oi.sizep = sizep;
-	if (sha1_object_info_extended(sha1, &oi) < 0)
+	if (sha1_object_info_extended(sha1, &oi, READ_SHA1_FILE_REPLACE) < 0)
 		return -1;
 	return type;
 }
diff --git a/streaming.c b/streaming.c
index debe904..9659f18 100644
--- a/streaming.c
+++ b/streaming.c
@@ -113,7 +113,7 @@ static enum input_source istream_source(const unsigned char *sha1,
 
 	oi->typep = type;
 	oi->sizep = &size;
-	status = sha1_object_info_extended(sha1, oi);
+	status = sha1_object_info_extended(sha1, oi, 0);
 	if (status < 0)
 		return stream_error;
 
-- 
1.8.4.1.561.g12affca

[PATCH 4/5] t6050: show that git cat-file --batch fails with replace objects

[Christian Couder]

When --batch is passed to git cat-file, the sha1_object_info_extended()
function is used to get information about the objects passed to
git cat-file.

Unfortunately sha1_object_info_extended() doesn't take care of
object replacement properly, so it will often fail with a
message like this:

$ echo a3fb2e1845a1aaf129b7975048973414dc172173 | git cat-file --batch
a3fb2e1845a1aaf129b7975048973414dc172173 commit 231
fatal: object a3fb2e1845a1aaf129b7975048973414dc172173 change size!?

The goal of this patch is to show this breakage.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6050-replace.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 7d47984..b90dbdc 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -276,6 +276,11 @@ test_expect_success '-f option bypasses the type check' '
 	git replace -f HEAD^ $BLOB
 '
 
+test_expect_failure 'git cat-file --batch works on replace objects' '
+	git replace | grep $PARA3 &&
+	echo $PARA3 | git cat-file --batch
+'
+
 test_expect_success 'replace ref cleanup' '
 	test -n "$(git replace)" &&
 	git replace -d $(git replace) &&
-- 
1.8.4.1.561.g12affca

[PATCH 5/5] sha1_file: perform object replacement in sha1_object_info_extended()

[Christian Couder]

sha1_object_info_extended() should perform object replacement
if it is needed.

The simplest way to do that is to make it call
lookup_replace_object_extended().

And now its "unsigned flags" parameter is used as it is passed
to lookup_replace_object_extended().

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 sha1_file.c        | 13 +++++++------
 t/t6050-replace.sh |  2 +-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 09e56ef..d715553 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2519,8 +2519,9 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
 	struct cached_object *co;
 	struct pack_entry e;
 	int rtype;
+	const unsigned char *real = lookup_replace_object_extended(sha1, flags);
 
-	co = find_cached_object(sha1);
+	co = find_cached_object(real);
 	if (co) {
 		if (oi->typep)
 			*(oi->typep) = co->type;
@@ -2532,23 +2533,23 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
 		return 0;
 	}
 
-	if (!find_pack_entry(sha1, &e)) {
+	if (!find_pack_entry(real, &e)) {
 		/* Most likely it's a loose object. */
-		if (!sha1_loose_object_info(sha1, oi)) {
+		if (!sha1_loose_object_info(real, oi)) {
 			oi->whence = OI_LOOSE;
 			return 0;
 		}
 
 		/* Not a loose object; someone else may have just packed it. */
 		reprepare_packed_git();
-		if (!find_pack_entry(sha1, &e))
+		if (!find_pack_entry(real, &e))
 			return -1;
 	}
 
 	rtype = packed_object_info(e.p, e.offset, oi);
 	if (rtype < 0) {
-		mark_bad_packed_object(e.p, sha1);
-		return sha1_object_info_extended(sha1, oi, 0);
+		mark_bad_packed_object(e.p, real);
+		return sha1_object_info_extended(real, oi, 0);
 	} else if (in_delta_base_cache(e.p, e.offset)) {
 		oi->whence = OI_DBCACHED;
 	} else {
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index b90dbdc..bb785ec 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -276,7 +276,7 @@ test_expect_success '-f option bypasses the type check' '
 	git replace -f HEAD^ $BLOB
 '
 
-test_expect_failure 'git cat-file --batch works on replace objects' '
+test_expect_success 'git cat-file --batch works on replace objects' '
 	git replace | grep $PARA3 &&
 	echo $PARA3 | git cat-file --batch
 '
-- 
1.8.4.1.561.g12affca

Re: [PATCH 0/5] teach replace objects to sha1_object_info_extended()

[Jeff King]

On Sat, Nov 30, 2013 at 02:51:18PM +0100, Christian Couder wrote:

> Here is a patch series to improve the way sha1_object_info_extended()
> behaves when it is passed a replaced object.
> [...]
> Christian Couder (5):
>   replace_object: don't check read_replace_refs twice
>   introduce lookup_replace_object_extended() to pass flags
>   Add an "unsigned flags" parameter to sha1_object_info_extended()
>   t6050: show that git cat-file --batch fails with replace objects
>   sha1_file: perform object replacement in sha1_object_info_extended()

Overall looks OK to me.

I find it a little funny that we reuse the READ_SHA1_FILE_REPLACE flag
directly in lookup_replace_object. That means that it is now a
meaningful flag for sha1_object_info_extended, even though the name does
not say so. It also means that the two may have to coordinate further
flags (since a portion of their flag namespace is shared by
lookup_replace_object). I don't foresee adding a lot of new flags,
though, so it probably isn't a huge deal.

I also would have expected sha1_object_info_extended to simply receive
the new flag via the struct object_info. Again, probably not a big deal,
because there aren't many callsites that needed updating. But if we were
not sharing flags with read_sha1_file, I think doing it as a flag in the
struct would be nicer.

I checked the resulting behavior against the list I made earlier; looks
like all of the _extended callsites are doing the right thing.

I do think the checks you added in 277336a (replace: forbid replacing an
object with one of a different type, 2013-09-06) need updating. I
started on that, but I wonder if all of cmd_replace should simply turn
off read_replace_refs. I'd think it would always want to be dealing with
the true objects.

-Peff

Re: [PATCH 0/5] teach replace objects to sha1_object_info_extended()

[Jeff King]

On Mon, Dec 02, 2013 at 09:52:25AM -0500, Jeff King wrote:

> I find it a little funny that we reuse the READ_SHA1_FILE_REPLACE flag
> directly in lookup_replace_object. That means that it is now a
> meaningful flag for sha1_object_info_extended, even though the name does
> not say so. It also means that the two may have to coordinate further
> flags (since a portion of their flag namespace is shared by
> lookup_replace_object). I don't foresee adding a lot of new flags,
> though, so it probably isn't a huge deal.
> 
> I also would have expected sha1_object_info_extended to simply receive
> the new flag via the struct object_info. Again, probably not a big deal,
> because there aren't many callsites that needed updating. But if we were
> not sharing flags with read_sha1_file, I think doing it as a flag in the
> struct would be nicer.

Curious what this would look like, I wrote the patch. If you drop your
patches 2 and 3, then your final patch (actually fixing the problem)
would look like the one below:

We may be getting into bikeshed territory, and I don't feel
super-strongly about it, so I won't say anything more. Now we've seen
both alternatives, and you or Junio can pick. :)

---
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index b2ca775..a2e3e26 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -270,6 +270,7 @@ static int batch_objects(struct batch_options *opt)
 	 * object.
 	 */
 	memset(&data, 0, sizeof(data));
+	data.info.respect_replace = 1;
 	data.mark_query = 1;
 	strbuf_expand(&buf, opt->format, expand_format, &data);
 	data.mark_query = 0;
diff --git a/cache.h b/cache.h
index ce377e1..0ad262f 100644
--- a/cache.h
+++ b/cache.h
@@ -1074,6 +1074,7 @@ struct object_info {
 	enum object_type *typep;
 	unsigned long *sizep;
 	unsigned long *disk_sizep;
+	unsigned respect_replace:1;
 
 	/* Response */
 	enum {
diff --git a/sha1_file.c b/sha1_file.c
index 7dadd04..b6ddad0 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2519,8 +2519,11 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)
 	struct cached_object *co;
 	struct pack_entry e;
 	int rtype;
+	const unsigned char *real = oi->respect_replace ?
+				    lookup_replace_object(sha1) :
+				    sha1;
 
-	co = find_cached_object(sha1);
+	co = find_cached_object(real);
 	if (co) {
 		if (oi->typep)
 			*(oi->typep) = co->type;
@@ -2532,16 +2535,16 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)
 		return 0;
 	}
 
-	if (!find_pack_entry(sha1, &e)) {
+	if (!find_pack_entry(real, &e)) {
 		/* Most likely it's a loose object. */
-		if (!sha1_loose_object_info(sha1, oi)) {
+		if (!sha1_loose_object_info(real, oi)) {
 			oi->whence = OI_LOOSE;
 			return 0;
 		}
 
 		/* Not a loose object; someone else may have just packed it. */
 		reprepare_packed_git();
-		if (!find_pack_entry(sha1, &e))
+		if (!find_pack_entry(real, &e))
 			return -1;
 	}
 
@@ -2570,6 +2573,7 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)
 
 	oi.typep = &type;
 	oi.sizep = sizep;
+	oi.respect_replace = 1;
 	if (sha1_object_info_extended(sha1, &oi) < 0)
 		return -1;
 	return type;
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index b90dbdc..bb785ec 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -276,7 +276,7 @@ test_expect_success '-f option bypasses the type check' '
 	git replace -f HEAD^ $BLOB
 '
 
-test_expect_failure 'git cat-file --batch works on replace objects' '
+test_expect_success 'git cat-file --batch works on replace objects' '
 	git replace | grep $PARA3 &&
 	echo $PARA3 | git cat-file --batch
 '

Re: [PATCH 0/5] teach replace objects to sha1_object_info_extended()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Dec 02, 2013 at 09:52:25AM -0500, Jeff King wrote:
>
>> I find it a little funny that we reuse the READ_SHA1_FILE_REPLACE flag
>> directly in lookup_replace_object. That means that it is now a
>> meaningful flag for sha1_object_info_extended, even though the name does
>> not say so. It also means that the two may have to coordinate further
>> flags (since a portion of their flag namespace is shared by
>> lookup_replace_object). I don't foresee adding a lot of new flags,
>> though, so it probably isn't a huge deal.
>> 
>> I also would have expected sha1_object_info_extended to simply receive
>> the new flag via the struct object_info. Again, probably not a big deal,
>> because there aren't many callsites that needed updating. But if we were
>> not sharing flags with read_sha1_file, I think doing it as a flag in the
>> struct would be nicer.
>
> Curious what this would look like, I wrote the patch. If you drop your
> patches 2 and 3, then your final patch (actually fixing the problem)
> would look like the one below:
>
> We may be getting into bikeshed territory, and I don't feel
> super-strongly about it, so I won't say anything more. Now we've seen
> both alternatives, and you or Junio can pick. :)

FWIW, I shared that "a little funny" feeling ;-)

>
> ---
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index b2ca775..a2e3e26 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -270,6 +270,7 @@ static int batch_objects(struct batch_options *opt)
>  	 * object.
>  	 */
>  	memset(&data, 0, sizeof(data));
> +	data.info.respect_replace = 1;
>  	data.mark_query = 1;
>  	strbuf_expand(&buf, opt->format, expand_format, &data);
>  	data.mark_query = 0;
> diff --git a/cache.h b/cache.h
> index ce377e1..0ad262f 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1074,6 +1074,7 @@ struct object_info {
>  	enum object_type *typep;
>  	unsigned long *sizep;
>  	unsigned long *disk_sizep;
> +	unsigned respect_replace:1;
>  
>  	/* Response */
>  	enum {
> diff --git a/sha1_file.c b/sha1_file.c
> index 7dadd04..b6ddad0 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2519,8 +2519,11 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)
>  	struct cached_object *co;
>  	struct pack_entry e;
>  	int rtype;
> +	const unsigned char *real = oi->respect_replace ?
> +				    lookup_replace_object(sha1) :
> +				    sha1;
>  
> -	co = find_cached_object(sha1);
> +	co = find_cached_object(real);
>  	if (co) {
>  		if (oi->typep)
>  			*(oi->typep) = co->type;
> @@ -2532,16 +2535,16 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)
>  		return 0;
>  	}
>  
> -	if (!find_pack_entry(sha1, &e)) {
> +	if (!find_pack_entry(real, &e)) {
>  		/* Most likely it's a loose object. */
> -		if (!sha1_loose_object_info(sha1, oi)) {
> +		if (!sha1_loose_object_info(real, oi)) {
>  			oi->whence = OI_LOOSE;
>  			return 0;
>  		}
>  
>  		/* Not a loose object; someone else may have just packed it. */
>  		reprepare_packed_git();
> -		if (!find_pack_entry(sha1, &e))
> +		if (!find_pack_entry(real, &e))
>  			return -1;
>  	}
>  
> @@ -2570,6 +2573,7 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)
>  
>  	oi.typep = &type;
>  	oi.sizep = sizep;
> +	oi.respect_replace = 1;
>  	if (sha1_object_info_extended(sha1, &oi) < 0)
>  		return -1;
>  	return type;
> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
> index b90dbdc..bb785ec 100755
> --- a/t/t6050-replace.sh
> +++ b/t/t6050-replace.sh
> @@ -276,7 +276,7 @@ test_expect_success '-f option bypasses the type check' '
>  	git replace -f HEAD^ $BLOB
>  '
>  
> -test_expect_failure 'git cat-file --batch works on replace objects' '
> +test_expect_success 'git cat-file --batch works on replace objects' '
>  	git replace | grep $PARA3 &&
>  	echo $PARA3 | git cat-file --batch
>  '

Re: [PATCH 0/5] teach replace objects to sha1_object_info_extended()

[Christian Couder]

From: Junio C Hamano <gitster@pobox.com>
>
> Jeff King <peff@peff.net> writes:
> 
>> On Mon, Dec 02, 2013 at 09:52:25AM -0500, Jeff King wrote:
>>
>>> I find it a little funny that we reuse the READ_SHA1_FILE_REPLACE flag
>>> directly in lookup_replace_object. That means that it is now a
>>> meaningful flag for sha1_object_info_extended, even though the name does
>>> not say so. It also means that the two may have to coordinate further
>>> flags (since a portion of their flag namespace is shared by
>>> lookup_replace_object). I don't foresee adding a lot of new flags,
>>> though, so it probably isn't a huge deal.
>>> 
>>> I also would have expected sha1_object_info_extended to simply receive
>>> the new flag via the struct object_info. Again, probably not a big deal,
>>> because there aren't many callsites that needed updating. But if we were
>>> not sharing flags with read_sha1_file, I think doing it as a flag in the
>>> struct would be nicer.
>>
>> Curious what this would look like, I wrote the patch. If you drop your
>> patches 2 and 3, then your final patch (actually fixing the problem)
>> would look like the one below:
>>
>> We may be getting into bikeshed territory, and I don't feel
>> super-strongly about it, so I won't say anything more. Now we've seen
>> both alternatives, and you or Junio can pick. :)

Thanks for doing that.

I still prefer a flag used by sha1_object_info_extended(),
read_sha1_file_extended() and lookup_replace_object_extended().
I think it is more coherent this way.

> FWIW, I shared that "a little funny" feeling ;-)

Yeah, it's true that I should have renamed the flag.
READ_SHA1_FILE_REPLACE is too much related to the read_sha1_file*()
functions.

As it is related to lookup_replace_object*() functions, what
about LOOKUP_REPLACE_OBJECT?

Thanks,
Christian.

Re: [PATCH 0/5] teach replace objects to sha1_object_info_extended()

[Christian Couder]

From: Jeff King <peff@peff.net>
>
> On Sat, Nov 30, 2013 at 02:51:18PM +0100, Christian Couder wrote:
> 
>> Here is a patch series to improve the way sha1_object_info_extended()
>> behaves when it is passed a replaced object.
> 
> Overall looks OK to me.

Thanks for reviewing it.

[...]
 
> I checked the resulting behavior against the list I made earlier; looks
> like all of the _extended callsites are doing the right thing.
> 
> I do think the checks you added in 277336a (replace: forbid replacing an
> object with one of a different type, 2013-09-06) need updating. I
> started on that, but I wonder if all of cmd_replace should simply turn
> off read_replace_refs. I'd think it would always want to be dealing with
> the true objects.

Yeah, I think you are right. I will have a look and probably turn off
read_replace_ref in cmd_replace() as you suggest.

Thanks,
Christian.