Re: [PATCH] drop support for "experimental" loose objects
From: Christian Couder <hidden>
Date: 2016-06-15 22:59:18
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Fri, Nov 22, 2013 at 10:58 AM, Jeff King [off-list ref] wrote:
quoted hunk ↗ jump to hunk
On Thu, Nov 21, 2013 at 09:19:25PM +0100, Christian Couder wrote:quoted
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.