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

From: Jeff King <peff@peff.net>
Date: Nov 22, 2013, 11:24 UTC
Message-ID: <20131122112429.GA16172@sigill.intra.peff.net>
In-Reply-To: <CAP8UFD1fMTrJGo9Z4+jdWqc-=UmPG1jQjwTij4962WDoh_a1DA@mail.gmail.com>

On Fri, Nov 22, 2013 at 12:04:01PM +0100, Christian Couder wrote:

Show 28 quoted lines

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

Is it? I did not have the impression anyone would ever redefine READ_SHA1_FILE_REPLACE at compile time, but that it was a flag that individual callsites would pass to read_sha1_file_extended to tell them whether they were interested in replacements. And the obvious reasons to not be are:

  1. You are doing some operation which needs real objects, like fsck or
     generating a packfile.

  2. You have already resolved any replacements, and want to make sure
     you are getting the same object used elsewhere (e.g., because you
     already printed its size :) ).

The only site which calls read_sha1_file_extended directly and does not pass the REPLACE flag is in streaming.c. And that looks to be a case of (2), since we resolve the replacement at the start in open_istream().

I am kind of surprised we do not need to do so for (1) in places like pack-objects.c. Most of that code does not use read_sha1_file, preferring instead to find the individual pack entries (for reuse). But there are some calls to read_sha1_file, and I wonder if there is a bug lurking there.

-Peff