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

From: Jeff King <peff@peff.net>
Date: Nov 22, 2013, 16:15 UTC
Message-ID: <20131122161558.GA4170@sigill.intra.peff.net>
In-Reply-To: <CAP8UFD1z4NsmgzrnPmqHo7CkNRkgg24qT2SGnFjhjrzckdKoTQ@mail.gmail.com>

On Fri, Nov 22, 2013 at 03:23:31PM +0100, Christian Couder wrote:

Show 9 quoted lines

> > 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().
> 
> Yeah, you are right. Sorry for overlooking this.
> 
> But anyway it looks redundant to me to have both this REPLACE flag and
> the read_replace_refs global variable, so I think a proper solution
> would involve some significant refactoring.

I don't think it is redundant. The global variable is about "does the whole operation want the replace feature turned on" and the flag is about "does this particular callsite want the replace featured turned on". We use the feature iff both are true.

We could implement the callsite flag by tweaking the global right before the call to read_sha1_file, but then we would have to remember to turn it back on afterwards. If this were a language with dynamic scopes like Perl, that would be easy. But in C you have to remember to reset it in all code paths. :)

In some cases it does make sense to turn the feature off for a whole command (like pack-objects); using the global makes sense there. And indeed, we seem to do it already in things like fsck, index-pack, etc. So that answers my question of why I did not see more of case (1) in my previous email: they do not need per-callsite disabling, because they do it for the whole command.

> And if we decide to keep a REPLACE flag we might need to add one to
> sha1_object_info_extended() too.

Yes, but somebody needs to look at all of the callsites and decide which form they want. :)

I did a brief skim, and the ones I noticed were:

  - several spots in index-pack, pack-objects, etc. But these are
    already covered by unsetting read_replace_refs.

  - replace_object looks at both the original and new object to compare
    their types (due to your recent patches); it would obviously want to
    get the true type of the original object

  - When creating tags and trees, we care about the type of the object
    (the former for the "type" line of the tag, the latter to set the
    mode). What should they do with replace objects? As above, it is
    probably insane to switch types, so it may not matter for practical
    purposes.

  - istream_source in streaming.c would probably want to turn it off for
    the same reason it uses read_sha1_file_extended

So I think most sites would be unaffected, but due to the second and fourth item in my list above, we would need a flag for sha1_object_info_extended.

-Peff