Re: [PATCH 0/5] teach replace objects to sha1_object_info_extended()
On Mon, Dec 02, 2013 at 09:52:25AM -0500, Jeff King wrote:
Show 13 quoted lines
> 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. :)
---
Show changes to 4 files +11 −5
builtin/cat-file.c, cache.h, sha1_file.c, t/t6050-replace.sh
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
'
Christian Couder
