On Fri, Nov 22, 2013 at 10:58 AM, Jeff King <peff@xxxxxxxx> wrote: > On Thu, Nov 21, 2013 at 09:19:25PM +0100, Christian Couder wrote: > >> Yeah, I think it might report wrong size in case of replaced objects >> for example. >> I looked at that following Junio's comment about the >> sha1_object_info() API, which, >> unlike read_sha1_file() API, does not interact with the "replace" mechanism: >> >> http://thread.gmane.org/gmane.comp.version-control.git/234023/ >> >> I started to work on a patch about this but didn't take the time to >> finish and post it. > > That seems kind of crazy. Would the fix be as simple as this: > > diff --git a/sha1_file.c b/sha1_file.c > index 10676ba..a051d6c 100644 > --- a/sha1_file.c > +++ b/sha1_file.c > @@ -2529,6 +2529,8 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi) > struct pack_entry e; > int rtype; > > + sha1 = lookup_replace_object(sha1); > + > co = find_cached_object(sha1); > if (co) { > if (oi->typep) > > or do we need some way for callers to turn off replacement? I notice > that read_sha1_file has such a feature, but it is only used in one > place. Yeah, indeed, I asked myself such a question and that's why it is not so simple unfortunately. In "sha1_file.c", there is: void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type, unsigned long *size, unsigned flag) { void *data; char *path; const struct packed_git *p; const unsigned char *repl = (flag & READ_SHA1_FILE_REPLACE) ? lookup_replace_object(sha1) : sha1; errno = 0; data = read_object(repl, type, size); ... And in cache.h, there is: #define READ_SHA1_FILE_REPLACE 1 static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size) { return read_sha1_file_extended(sha1, type, size, READ_SHA1_FILE_REPLACE); } So the READ_SHA1_FILE_REPLACE is a way to disable replacement at compile time. But in my opinion if we want such a knob, we should use it when we set the "read_replace_refs" global variable. For example with something like this: diff --git a/environment.c b/environment.c index 0a15349..7c99af8 100644 --- a/environment.c +++ b/environment.c @@ -44,7 +44,7 @@ const char *editor_program; const char *askpass_program; const char *excludes_file; enum auto_crlf auto_crlf = AUTO_CRLF_FALSE; -int read_replace_refs = 1; /* NEEDSWORK: rename to use_replace_refs */ +int read_replace_refs = READ_SHA1_FILE_REPLACE; /* NEEDSWORK: rename to use_replace_refs */ enum eol core_eol = EOL_UNSET; enum safe_crlf safe_crlf = SAFE_CRLF_WARN; unsigned whitespace_rule_cfg = WS_DEFAULT_RULE; @Junio what would you think about such a change? > I guess we would need to audit all the sha1_object_info callers. Yeah but when I looked at them, there were not many that looked dangerous. Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html