On Fri, Nov 22, 2013 at 10:58 AM, Jeff King <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
