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

[Jeff King <peff@peff.net>]

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

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