Re: [PATCH 0/5] teach replace objects to sha1_object_info_extended()
From: Junio C Hamano gits...@pobox.com Jeff King p...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] teach replace objects to sha1_object_info_extended()
From: Jeff King p...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] teach replace objects to sha1_object_info_extended()
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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:
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
'
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] teach replace objects to sha1_object_info_extended()
Jeff King p...@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
'
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/5] teach replace objects to sha1_object_info_extended()
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
