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