{"thread":{"id":"35442","subject":"[PATCH 1/5] replace_object: don't check read_replace_refs twice","startedAt":"2013-11-30T13:51:18Z","lastAt":"2013-12-03T20:46:17Z","messageCount":11,"participants":["Christian Couder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"231303","messageId":"20131130133934.2697.75781.chriscool@tuxfamily.org","threadId":"35442","inReplyTo":null,"subject":"[PATCH 0/5] teach replace objects to sha1_object_info_extended()","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2013-11-30T13:51:18Z","receivedAt":"2013-11-30T13:51:18Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Here is a patch series to improve the way sha1_object_info_extended()\nbehaves when it is passed a replaced object.\n\nThis patch series was inspired by a sub thread in this discussion:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/238118\n\nPatches 1/5 and 2/5 are cleanups.\nPatch 4/5 adds a test that fails.\nPatch 5/5 adds a call to lookup_replace_object_extended() in\nsha1_object_info_extended() and makes the previous test pass.\n\nChristian Couder (5):\n  replace_object: don't check read_replace_refs twice\n  introduce lookup_replace_object_extended() to pass flags\n  Add an \"unsigned flags\" parameter to sha1_object_info_extended()\n  t6050: show that git cat-file --batch fails with replace objects\n  sha1_file: perform object replacement in sha1_object_info_extended()\n\n builtin/cat-file.c |  2 +-\n cache.h            |  8 +++++++-\n replace_object.c   |  3 ---\n sha1_file.c        | 20 ++++++++++----------\n streaming.c        |  2 +-\n t/t6050-replace.sh |  5 +++++\n 6 files changed, 24 insertions(+), 16 deletions(-)\n\n-- \n1.8.4.1.561.g12affca\n"},{"id":"231301","messageId":"20131130135124.2697.81789.chriscool@tuxfamily.org","threadId":"35442","inReplyTo":"20131130133934.2697.75781.chriscool@tuxfamily.org","subject":"[PATCH 1/5] replace_object: don't check read_replace_refs twice","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2013-11-30T13:51:19Z","receivedAt":"2013-11-30T13:51:19Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Since e1111cef (inline lookup_replace_object() calls,\nMay 15 2011) the read_replace_refs global variable is\nchecked twice, once in lookup_replace_object() and\nonce again in do_lookup_replace_object().\n\nAs do_lookup_replace_object() is called only from\nlookup_replace_object(), we can remove the check in\ndo_lookup_replace_object().\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n replace_object.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/replace_object.c b/replace_object.c\nindex d0b1548..cdcaf8c 100644\n--- a/replace_object.c\n+++ b/replace_object.c\n@@ -97,9 +97,6 @@ const unsigned char *do_lookup_replace_object(const unsigned char *sha1)\n \tint pos, depth = MAXREPLACEDEPTH;\n \tconst unsigned char *cur = sha1;\n \n-\tif (!read_replace_refs)\n-\t\treturn sha1;\n-\n \tprepare_replace_object();\n \n \t/* Try to recursively replace the object */\n-- \n1.8.4.1.561.g12affca\n"},{"id":"231302","messageId":"20131130135124.2697.42059.chriscool@tuxfamily.org","threadId":"35442","inReplyTo":"20131130133934.2697.75781.chriscool@tuxfamily.org","subject":"[PATCH 2/5] introduce lookup_replace_object_extended() to pass flags","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2013-11-30T13:51:20Z","receivedAt":"2013-11-30T13:51:20Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Currently, there is only one caller to lookup_replace_object()\nthat can benefit from passing it some flags, but we expect\nthat there could be more.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n cache.h     | 6 ++++++\n sha1_file.c | 3 +--\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex ce377e1..b845485 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -773,6 +773,12 @@ static inline const unsigned char *lookup_replace_object(const unsigned char *sh\n \t\treturn sha1;\n \treturn do_lookup_replace_object(sha1);\n }\n+static inline const unsigned char *lookup_replace_object_extended(const unsigned char *sha1, unsigned flag)\n+{\n+\tif (! (flag & READ_SHA1_FILE_REPLACE))\n+\t\treturn sha1;\n+\treturn lookup_replace_object(sha1);\n+}\n \n /* Read and unpack a sha1 file into memory, write memory to a sha1 file */\n extern int sha1_object_info(const unsigned char *, unsigned long *);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 7dadd04..b0a3964 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2662,8 +2662,7 @@ void *read_sha1_file_extended(const unsigned char *sha1,\n \tvoid *data;\n \tchar *path;\n \tconst struct packed_git *p;\n-\tconst unsigned char *repl = (flag & READ_SHA1_FILE_REPLACE)\n-\t\t? lookup_replace_object(sha1) : sha1;\n+\tconst unsigned char *repl = lookup_replace_object_extended(sha1, flag);\n \n \terrno = 0;\n \tdata = read_object(repl, type, size);\n-- \n1.8.4.1.561.g12affca\n"},{"id":"231306","messageId":"20131130135124.2697.9050.chriscool@tuxfamily.org","threadId":"35442","inReplyTo":"20131130133934.2697.75781.chriscool@tuxfamily.org","subject":"[PATCH 3/5] Add an \"unsigned flags\" parameter to sha1_object_info_extended()","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2013-11-30T13:51:21Z","receivedAt":"2013-11-30T13:51:21Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This parameter is not used yet, but it will be used to tell\nsha1_object_info_extended() if it should perform object\nreplacement or not.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/cat-file.c | 2 +-\n cache.h            | 2 +-\n sha1_file.c        | 6 +++---\n streaming.c        | 2 +-\n 4 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex b2ca775..a2d3a9b 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -238,7 +238,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n \t\treturn 0;\n \t}\n \n-\tif (sha1_object_info_extended(data->sha1, &data->info) < 0) {\n+\tif (sha1_object_info_extended(data->sha1, &data->info, READ_SHA1_FILE_REPLACE) < 0) {\n \t\tprintf(\"%s missing\\n\", obj_name);\n \t\tfflush(stdout);\n \t\treturn 0;\ndiff --git a/cache.h b/cache.h\nindex b845485..3be03dc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1104,7 +1104,7 @@ struct object_info {\n \t\t} packed;\n \t} u;\n };\n-extern int sha1_object_info_extended(const unsigned char *, struct object_info *);\n+extern int sha1_object_info_extended(const unsigned char *, struct object_info *, unsigned flags);\n \n /* Dumb servers support */\n extern int update_server_info(int);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b0a3964..09e56ef 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2514,7 +2514,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \treturn 0;\n }\n \n-int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n+int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi, unsigned flags)\n {\n \tstruct cached_object *co;\n \tstruct pack_entry e;\n@@ -2548,7 +2548,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n \trtype = packed_object_info(e.p, e.offset, oi);\n \tif (rtype < 0) {\n \t\tmark_bad_packed_object(e.p, sha1);\n-\t\treturn sha1_object_info_extended(sha1, oi);\n+\t\treturn sha1_object_info_extended(sha1, oi, 0);\n \t} else if (in_delta_base_cache(e.p, e.offset)) {\n \t\toi->whence = OI_DBCACHED;\n \t} else {\n@@ -2570,7 +2570,7 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)\n \n \toi.typep = &type;\n \toi.sizep = sizep;\n-\tif (sha1_object_info_extended(sha1, &oi) < 0)\n+\tif (sha1_object_info_extended(sha1, &oi, READ_SHA1_FILE_REPLACE) < 0)\n \t\treturn -1;\n \treturn type;\n }\ndiff --git a/streaming.c b/streaming.c\nindex debe904..9659f18 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -113,7 +113,7 @@ static enum input_source istream_source(const unsigned char *sha1,\n \n \toi->typep = type;\n \toi->sizep = &size;\n-\tstatus = sha1_object_info_extended(sha1, oi);\n+\tstatus = sha1_object_info_extended(sha1, oi, 0);\n \tif (status < 0)\n \t\treturn stream_error;\n \n-- \n1.8.4.1.561.g12affca\n"},{"id":"231305","messageId":"20131130135124.2697.79815.chriscool@tuxfamily.org","threadId":"35442","inReplyTo":"20131130133934.2697.75781.chriscool@tuxfamily.org","subject":"[PATCH 4/5] t6050: show that git cat-file --batch fails with replace objects","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2013-11-30T13:51:22Z","receivedAt":"2013-11-30T13:51:22Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"When --batch is passed to git cat-file, the sha1_object_info_extended()\nfunction is used to get information about the objects passed to\ngit cat-file.\n\nUnfortunately sha1_object_info_extended() doesn't take care of\nobject replacement properly, so it will often fail with a\nmessage like this:\n\n$ echo a3fb2e1845a1aaf129b7975048973414dc172173 | git cat-file --batch\na3fb2e1845a1aaf129b7975048973414dc172173 commit 231\nfatal: object a3fb2e1845a1aaf129b7975048973414dc172173 change size!?\n\nThe goal of this patch is to show this breakage.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n t/t6050-replace.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex 7d47984..b90dbdc 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -276,6 +276,11 @@ test_expect_success '-f option bypasses the type check' '\n \tgit replace -f HEAD^ $BLOB\n '\n \n+test_expect_failure 'git cat-file --batch works on replace objects' '\n+\tgit replace | grep $PARA3 &&\n+\techo $PARA3 | git cat-file --batch\n+'\n+\n test_expect_success 'replace ref cleanup' '\n \ttest -n \"$(git replace)\" &&\n \tgit replace -d $(git replace) &&\n-- \n1.8.4.1.561.g12affca\n"},{"id":"231304","messageId":"20131130135124.2697.62573.chriscool@tuxfamily.org","threadId":"35442","inReplyTo":"20131130133934.2697.75781.chriscool@tuxfamily.org","subject":"[PATCH 5/5] sha1_file: perform object replacement in sha1_object_info_extended()","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2013-11-30T13:51:23Z","receivedAt":"2013-11-30T13:51:23Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"sha1_object_info_extended() should perform object replacement\nif it is needed.\n\nThe simplest way to do that is to make it call\nlookup_replace_object_extended().\n\nAnd now its \"unsigned flags\" parameter is used as it is passed\nto lookup_replace_object_extended().\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n sha1_file.c        | 13 +++++++------\n t/t6050-replace.sh |  2 +-\n 2 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 09e56ef..d715553 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2519,8 +2519,9 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,\n \tstruct cached_object *co;\n \tstruct pack_entry e;\n \tint rtype;\n+\tconst unsigned char *real = lookup_replace_object_extended(sha1, flags);\n \n-\tco = find_cached_object(sha1);\n+\tco = find_cached_object(real);\n \tif (co) {\n \t\tif (oi->typep)\n \t\t\t*(oi->typep) = co->type;\n@@ -2532,23 +2533,23 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,\n \t\treturn 0;\n \t}\n \n-\tif (!find_pack_entry(sha1, &e)) {\n+\tif (!find_pack_entry(real, &e)) {\n \t\t/* Most likely it's a loose object. */\n-\t\tif (!sha1_loose_object_info(sha1, oi)) {\n+\t\tif (!sha1_loose_object_info(real, oi)) {\n \t\t\toi->whence = OI_LOOSE;\n \t\t\treturn 0;\n \t\t}\n \n \t\t/* Not a loose object; someone else may have just packed it. */\n \t\treprepare_packed_git();\n-\t\tif (!find_pack_entry(sha1, &e))\n+\t\tif (!find_pack_entry(real, &e))\n \t\t\treturn -1;\n \t}\n \n \trtype = packed_object_info(e.p, e.offset, oi);\n \tif (rtype < 0) {\n-\t\tmark_bad_packed_object(e.p, sha1);\n-\t\treturn sha1_object_info_extended(sha1, oi, 0);\n+\t\tmark_bad_packed_object(e.p, real);\n+\t\treturn sha1_object_info_extended(real, oi, 0);\n \t} else if (in_delta_base_cache(e.p, e.offset)) {\n \t\toi->whence = OI_DBCACHED;\n \t} else {\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex b90dbdc..bb785ec 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -276,7 +276,7 @@ test_expect_success '-f option bypasses the type check' '\n \tgit replace -f HEAD^ $BLOB\n '\n \n-test_expect_failure 'git cat-file --batch works on replace objects' '\n+test_expect_success 'git cat-file --batch works on replace objects' '\n \tgit replace | grep $PARA3 &&\n \techo $PARA3 | git cat-file --batch\n '\n-- \n1.8.4.1.561.g12affca\n"},{"id":"231369","messageId":"20131202145225.GA12457@sigill.intra.peff.net","threadId":"35442","inReplyTo":"20131130133934.2697.75781.chriscool@tuxfamily.org","subject":"Re: [PATCH 0/5] teach replace objects to sha1_object_info_extended()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-02T14:52:25Z","receivedAt":"2013-12-02T14:52:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 30, 2013 at 02:51:18PM +0100, Christian Couder wrote:\n\n> Here is a patch series to improve the way sha1_object_info_extended()\n> behaves when it is passed a replaced object.\n> [...]\n> Christian Couder (5):\n>   replace_object: don't check read_replace_refs twice\n>   introduce lookup_replace_object_extended() to pass flags\n>   Add an \"unsigned flags\" parameter to sha1_object_info_extended()\n>   t6050: show that git cat-file --batch fails with replace objects\n>   sha1_file: perform object replacement in sha1_object_info_extended()\n\nOverall looks OK to me.\n\nI find it a little funny that we reuse the READ_SHA1_FILE_REPLACE flag\ndirectly in lookup_replace_object. That means that it is now a\nmeaningful flag for sha1_object_info_extended, even though the name does\nnot say so. It also means that the two may have to coordinate further\nflags (since a portion of their flag namespace is shared by\nlookup_replace_object). I don't foresee adding a lot of new flags,\nthough, so it probably isn't a huge deal.\n\nI also would have expected sha1_object_info_extended to simply receive\nthe new flag via the struct object_info. Again, probably not a big deal,\nbecause there aren't many callsites that needed updating. But if we were\nnot sharing flags with read_sha1_file, I think doing it as a flag in the\nstruct would be nicer.\n\nI checked the resulting behavior against the list I made earlier; looks\nlike all of the _extended callsites are doing the right thing.\n\nI do think the checks you added in 277336a (replace: forbid replacing an\nobject with one of a different type, 2013-09-06) need updating. I\nstarted on that, but I wonder if all of cmd_replace should simply turn\noff read_replace_refs. I'd think it would always want to be dealing with\nthe true objects.\n\n-Peff\n"},{"id":"231371","messageId":"20131202150634.GA20416@sigill.intra.peff.net","threadId":"35442","inReplyTo":"20131202145225.GA12457@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] teach replace objects to sha1_object_info_extended()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-02T15:06:34Z","receivedAt":"2013-12-02T15:06:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 02, 2013 at 09:52:25AM -0500, Jeff King wrote:\n\n> I find it a little funny that we reuse the READ_SHA1_FILE_REPLACE flag\n> directly in lookup_replace_object. That means that it is now a\n> meaningful flag for sha1_object_info_extended, even though the name does\n> not say so. It also means that the two may have to coordinate further\n> flags (since a portion of their flag namespace is shared by\n> lookup_replace_object). I don't foresee adding a lot of new flags,\n> though, so it probably isn't a huge deal.\n> \n> I also would have expected sha1_object_info_extended to simply receive\n> the new flag via the struct object_info. Again, probably not a big deal,\n> because there aren't many callsites that needed updating. But if we were\n> not sharing flags with read_sha1_file, I think doing it as a flag in the\n> struct would be nicer.\n\nCurious what this would look like, I wrote the patch. If you drop your\npatches 2 and 3, then your final patch (actually fixing the problem)\nwould look like the one below:\n\nWe may be getting into bikeshed territory, and I don't feel\nsuper-strongly about it, so I won't say anything more. Now we've seen\nboth alternatives, and you or Junio can pick. :)\n\n---\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex b2ca775..a2e3e26 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -270,6 +270,7 @@ static int batch_objects(struct batch_options *opt)\n \t * object.\n \t */\n \tmemset(&data, 0, sizeof(data));\n+\tdata.info.respect_replace = 1;\n \tdata.mark_query = 1;\n \tstrbuf_expand(&buf, opt->format, expand_format, &data);\n \tdata.mark_query = 0;\ndiff --git a/cache.h b/cache.h\nindex ce377e1..0ad262f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1074,6 +1074,7 @@ struct object_info {\n \tenum object_type *typep;\n \tunsigned long *sizep;\n \tunsigned long *disk_sizep;\n+\tunsigned respect_replace:1;\n \n \t/* Response */\n \tenum {\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 7dadd04..b6ddad0 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2519,8 +2519,11 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n \tstruct cached_object *co;\n \tstruct pack_entry e;\n \tint rtype;\n+\tconst unsigned char *real = oi->respect_replace ?\n+\t\t\t\t    lookup_replace_object(sha1) :\n+\t\t\t\t    sha1;\n \n-\tco = find_cached_object(sha1);\n+\tco = find_cached_object(real);\n \tif (co) {\n \t\tif (oi->typep)\n \t\t\t*(oi->typep) = co->type;\n@@ -2532,16 +2535,16 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n \t\treturn 0;\n \t}\n \n-\tif (!find_pack_entry(sha1, &e)) {\n+\tif (!find_pack_entry(real, &e)) {\n \t\t/* Most likely it's a loose object. */\n-\t\tif (!sha1_loose_object_info(sha1, oi)) {\n+\t\tif (!sha1_loose_object_info(real, oi)) {\n \t\t\toi->whence = OI_LOOSE;\n \t\t\treturn 0;\n \t\t}\n \n \t\t/* Not a loose object; someone else may have just packed it. */\n \t\treprepare_packed_git();\n-\t\tif (!find_pack_entry(sha1, &e))\n+\t\tif (!find_pack_entry(real, &e))\n \t\t\treturn -1;\n \t}\n \n@@ -2570,6 +2573,7 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)\n \n \toi.typep = &type;\n \toi.sizep = sizep;\n+\toi.respect_replace = 1;\n \tif (sha1_object_info_extended(sha1, &oi) < 0)\n \t\treturn -1;\n \treturn type;\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex b90dbdc..bb785ec 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -276,7 +276,7 @@ test_expect_success '-f option bypasses the type check' '\n \tgit replace -f HEAD^ $BLOB\n '\n \n-test_expect_failure 'git cat-file --batch works on replace objects' '\n+test_expect_success 'git cat-file --batch works on replace objects' '\n \tgit replace | grep $PARA3 &&\n \techo $PARA3 | git cat-file --batch\n '\n"},{"id":"231399","messageId":"xmqqtxeqq0qs.fsf@gitster.dls.corp.google.com","threadId":"35442","inReplyTo":"20131202150634.GA20416@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] teach replace objects to sha1_object_info_extended()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-02T23:41:47Z","receivedAt":"2013-12-02T23:41:47Z","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> On Mon, Dec 02, 2013 at 09:52:25AM -0500, Jeff King wrote:\n>\n>> I find it a little funny that we reuse the READ_SHA1_FILE_REPLACE flag\n>> directly in lookup_replace_object. That means that it is now a\n>> meaningful flag for sha1_object_info_extended, even though the name does\n>> not say so. It also means that the two may have to coordinate further\n>> flags (since a portion of their flag namespace is shared by\n>> lookup_replace_object). I don't foresee adding a lot of new flags,\n>> though, so it probably isn't a huge deal.\n>> \n>> I also would have expected sha1_object_info_extended to simply receive\n>> the new flag via the struct object_info. Again, probably not a big deal,\n>> because there aren't many callsites that needed updating. But if we were\n>> not sharing flags with read_sha1_file, I think doing it as a flag in the\n>> struct would be nicer.\n>\n> Curious what this would look like, I wrote the patch. If you drop your\n> patches 2 and 3, then your final patch (actually fixing the problem)\n> would look like the one below:\n>\n> We may be getting into bikeshed territory, and I don't feel\n> super-strongly about it, so I won't say anything more. Now we've seen\n> both alternatives, and you or Junio can pick. :)\n\nFWIW, I shared that \"a little funny\" feeling ;-)\n\n>\n> ---\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index b2ca775..a2e3e26 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -270,6 +270,7 @@ static int batch_objects(struct batch_options *opt)\n>  \t * object.\n>  \t */\n>  \tmemset(&data, 0, sizeof(data));\n> +\tdata.info.respect_replace = 1;\n>  \tdata.mark_query = 1;\n>  \tstrbuf_expand(&buf, opt->format, expand_format, &data);\n>  \tdata.mark_query = 0;\n> diff --git a/cache.h b/cache.h\n> index ce377e1..0ad262f 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1074,6 +1074,7 @@ struct object_info {\n>  \tenum object_type *typep;\n>  \tunsigned long *sizep;\n>  \tunsigned long *disk_sizep;\n> +\tunsigned respect_replace:1;\n>  \n>  \t/* Response */\n>  \tenum {\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 7dadd04..b6ddad0 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2519,8 +2519,11 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n>  \tstruct cached_object *co;\n>  \tstruct pack_entry e;\n>  \tint rtype;\n> +\tconst unsigned char *real = oi->respect_replace ?\n> +\t\t\t\t    lookup_replace_object(sha1) :\n> +\t\t\t\t    sha1;\n>  \n> -\tco = find_cached_object(sha1);\n> +\tco = find_cached_object(real);\n>  \tif (co) {\n>  \t\tif (oi->typep)\n>  \t\t\t*(oi->typep) = co->type;\n> @@ -2532,16 +2535,16 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n>  \t\treturn 0;\n>  \t}\n>  \n> -\tif (!find_pack_entry(sha1, &e)) {\n> +\tif (!find_pack_entry(real, &e)) {\n>  \t\t/* Most likely it's a loose object. */\n> -\t\tif (!sha1_loose_object_info(sha1, oi)) {\n> +\t\tif (!sha1_loose_object_info(real, oi)) {\n>  \t\t\toi->whence = OI_LOOSE;\n>  \t\t\treturn 0;\n>  \t\t}\n>  \n>  \t\t/* Not a loose object; someone else may have just packed it. */\n>  \t\treprepare_packed_git();\n> -\t\tif (!find_pack_entry(sha1, &e))\n> +\t\tif (!find_pack_entry(real, &e))\n>  \t\t\treturn -1;\n>  \t}\n>  \n> @@ -2570,6 +2573,7 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)\n>  \n>  \toi.typep = &type;\n>  \toi.sizep = sizep;\n> +\toi.respect_replace = 1;\n>  \tif (sha1_object_info_extended(sha1, &oi) < 0)\n>  \t\treturn -1;\n>  \treturn type;\n> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\n> index b90dbdc..bb785ec 100755\n> --- a/t/t6050-replace.sh\n> +++ b/t/t6050-replace.sh\n> @@ -276,7 +276,7 @@ test_expect_success '-f option bypasses the type check' '\n>  \tgit replace -f HEAD^ $BLOB\n>  '\n>  \n> -test_expect_failure 'git cat-file --batch works on replace objects' '\n> +test_expect_success 'git cat-file --batch works on replace objects' '\n>  \tgit replace | grep $PARA3 &&\n>  \techo $PARA3 | git cat-file --batch\n>  '\n"},{"id":"231470","messageId":"20131203.214209.1698497614731424957.chriscool@tuxfamily.org","threadId":"35442","inReplyTo":"xmqqtxeqq0qs.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/5] teach replace objects to sha1_object_info_extended()","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2013-12-03T20:42:09Z","receivedAt":"2013-12-03T20:42:09Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\n>\n> Jeff King <peff@peff.net> writes:\n> \n>> On Mon, Dec 02, 2013 at 09:52:25AM -0500, Jeff King wrote:\n>>\n>>> I find it a little funny that we reuse the READ_SHA1_FILE_REPLACE flag\n>>> directly in lookup_replace_object. That means that it is now a\n>>> meaningful flag for sha1_object_info_extended, even though the name does\n>>> not say so. It also means that the two may have to coordinate further\n>>> flags (since a portion of their flag namespace is shared by\n>>> lookup_replace_object). I don't foresee adding a lot of new flags,\n>>> though, so it probably isn't a huge deal.\n>>> \n>>> I also would have expected sha1_object_info_extended to simply receive\n>>> the new flag via the struct object_info. Again, probably not a big deal,\n>>> because there aren't many callsites that needed updating. But if we were\n>>> not sharing flags with read_sha1_file, I think doing it as a flag in the\n>>> struct would be nicer.\n>>\n>> Curious what this would look like, I wrote the patch. If you drop your\n>> patches 2 and 3, then your final patch (actually fixing the problem)\n>> would look like the one below:\n>>\n>> We may be getting into bikeshed territory, and I don't feel\n>> super-strongly about it, so I won't say anything more. Now we've seen\n>> both alternatives, and you or Junio can pick. :)\n\nThanks for doing that.\n\nI still prefer a flag used by sha1_object_info_extended(),\nread_sha1_file_extended() and lookup_replace_object_extended().\nI think it is more coherent this way.\n\n> FWIW, I shared that \"a little funny\" feeling ;-)\n\nYeah, it's true that I should have renamed the flag.\nREAD_SHA1_FILE_REPLACE is too much related to the read_sha1_file*()\nfunctions.\n\nAs it is related to lookup_replace_object*() functions, what\nabout LOOKUP_REPLACE_OBJECT?\n\nThanks,\nChristian.\n"},{"id":"231471","messageId":"20131203.214617.1435733880353938948.chriscool@tuxfamily.org","threadId":"35442","inReplyTo":"20131202145225.GA12457@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] teach replace objects to sha1_object_info_extended()","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2013-12-03T20:46:17Z","receivedAt":"2013-12-03T20:46:17Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Jeff King <peff@peff.net>\n>\n> On Sat, Nov 30, 2013 at 02:51:18PM +0100, Christian Couder wrote:\n> \n>> Here is a patch series to improve the way sha1_object_info_extended()\n>> behaves when it is passed a replaced object.\n> \n> Overall looks OK to me.\n\nThanks for reviewing it.\n\n[...]\n \n> I checked the resulting behavior against the list I made earlier; looks\n> like all of the _extended callsites are doing the right thing.\n> \n> I do think the checks you added in 277336a (replace: forbid replacing an\n> object with one of a different type, 2013-09-06) need updating. I\n> started on that, but I wonder if all of cmd_replace should simply turn\n> off read_replace_refs. I'd think it would always want to be dealing with\n> the true objects.\n\nYeah, I think you are right. I will have a look and probably turn off\nread_replace_ref in cmd_replace() as you suggest.\n\nThanks,\nChristian.\n"}]}