Re: [PATCH 5/8] commit-graph: not compatible with replace objects

[Derrick Stolee <stolee@gmail.com>]

On 7/18/2018 3:46 PM, Jeff King wrote:
> On Wed, Jul 18, 2018 at 08:15:41AM -0700, Derrick Stolee via GitGitGadget wrote:
>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> Create new method commit_graph_compatible(r) to check if a given
>> repository r is compatible with the commit-graph feature. Fill the
>> method with a check to see if replace-objects exist. Test this
>> interaction succeeds, including ignoring an existing commit-graph and
>> failing to write a new commit-graph.
> I think this approach is sensible. These are optimizations, and it's not
> a big deal to just punt no cases we can't handle.
>
> I do have one complaint about the implementation, though:
>
>> +static int commit_graph_compatible(struct repository *r)
>> +{
>> +	prepare_replace_object(r);
>> +	if (hashmap_get_size(&r->objects->replace_map->map))
>> +		return 0;
>> +
>> +	return 1;
>> +}
> If I'm reading the code correctly, this will predicate the decision
> entirely on the presence of refs in refs/replace/. But you may have a
> situation where those refs exist, but you are not respecting them in
> this run.
>
> For instance, imagine[1] a server that hosts arbitrary repositories, but
> wants to use commit graphs to speed up server-side operations (e.g.,
> serving fetches, but also perhaps a web interface doing --contains,
> etc). If it runs all of the server-side commands with
> GIT_NO_REPLACE_OBJECTS=1, then there should be no problem. But if a user
> pushes anything to refs/replace (because they _do_ use replace refs
> locally, and want to share them with other clients), that would disable
> graphs on the server.
>
> So I think this should at least be loosened to:
>
>    if (check_replace_refs) {
> 	prepare_replace_object(r);
> 	if (...)
>    }
>
> which would make this case work. I'd even go so far as to say that for
> writing, we should just always ignore replace refs and generate the full
> graph (just like pack-objects does so when writing a pack). Then the
> resulting graph can be used selectively by disabling replace refs for
> particular commands. But for the scenario I described above, the
> distinction is mostly academic, as replacements would be disabled anyway
> during the write command anyway.
>
> -Peff
>
> [1] You can probably guess that this is how GitHub handles replace refs.
>      We ran into this long ago because replacements and grafts mess up
>      any other caches external to Git that rely on the immutability of
>      the hash.
>
>      We do it with a config option, though, which requires a trivial
>      patch. I'll post that momentarily.

Thanks for the details! I never considered that someone would have these 
refs around, but would ignore them most of the time.

The biggest reason I wanted to punt here was that it is easy to toggle 
between using replace refs and not using them. Writing and reading as 
long as we are ignoring those refs is a good idea, and I'll use that 
approach in v2.

Thanks,

-Stolee