Re: [PATCH] drop support for "experimental" loose objects

From: Christian Couder <christian.couder@gmail.com>
Date: Nov 22, 2013, 11:04 UTC
Message-ID: <CAP8UFD1fMTrJGo9Z4+jdWqc-=UmPG1jQjwTij4962WDoh_a1DA@mail.gmail.com>
In-Reply-To: <20131122095801.GB12042@sigill.intra.peff.net>

On Fri, Nov 22, 2013 at 10:58 AM, Jeff King <peff@peff.net> wrote:

Show 32 quoted lines

> 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.