From: Jeff King <peff@peff.net>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, jnareb@gmail.com,
Junio C Hamano <gitster@pobox.com>,
Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 5/8] commit-graph: not compatible with replace objects
Date: Wed, 18 Jul 2018 15:46:57 -0400 [thread overview]
Message-ID: <20180718194657.GC7778@sigill.intra.peff.net> (raw)
In-Reply-To: <7f596c1718d35539f02828edbf933c8e660f123b.1531926932.git.gitgitgadget@gmail.com>
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.
next prev parent reply other threads:[~2018-07-18 19:47 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-18 15:15 [PATCH 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities Derrick Stolee via GitGitGadget
2018-07-18 15:15 ` [PATCH 1/8] refs.c: migrate internal ref iteration to pass thru repository argument Stefan Beller via GitGitGadget
2018-07-18 15:15 ` [PATCH 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback Stefan Beller via GitGitGadget
2018-07-18 18:32 ` Junio C Hamano
2018-07-18 19:19 ` Stefan Beller
2018-07-18 20:13 ` Junio C Hamano
2018-07-18 15:15 ` [PATCH 3/8] commit-graph: update design document Derrick Stolee via GitGitGadget
2018-07-29 13:44 ` Jakub Narebski
2018-07-18 15:15 ` [PATCH 4/8] test-repository: properly init repo Derrick Stolee via GitGitGadget
2018-07-29 21:07 ` Jakub Narebski
2018-07-18 15:15 ` [PATCH 5/8] commit-graph: not compatible with replace objects Derrick Stolee via GitGitGadget
2018-07-18 19:46 ` Jeff King [this message]
2018-07-18 19:48 ` Jeff King
2018-07-18 19:52 ` Derrick Stolee
2018-07-20 16:45 ` Derrick Stolee
2018-07-20 16:49 ` Stefan Beller
2018-07-20 16:57 ` Derrick Stolee
2018-07-29 21:00 ` Jakub Narebski
2018-07-18 15:15 ` [PATCH 6/8] commit-graph: not compatible with grafts Derrick Stolee via GitGitGadget
2018-07-29 22:04 ` Jakub Narebski
2018-07-18 15:15 ` [PATCH 7/8] commit-graph: not compatible with uninitialized repo Derrick Stolee via GitGitGadget
2018-07-29 22:46 ` Jakub Narebski
2018-07-18 15:15 ` [PATCH 8/8] commit-graph: close_commit_graph before shallow walk Derrick Stolee via GitGitGadget
2018-07-30 19:24 ` Jakub Narebski
2018-07-18 15:22 ` [PATCH] DO-NOT-MERGE: write and read commit-graph always Derrick Stolee
2018-07-30 14:39 ` Jakub Narebski
2018-07-30 17:06 ` Stefan Beller
2018-07-31 16:54 ` Jakub Narebski
2018-07-31 17:40 ` Elijah Newren
2018-07-29 13:13 ` [PATCH 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities Jakub Narebski
2018-07-29 19:27 ` Eric Sunshine
2018-08-20 18:24 ` [PATCH v2 " Derrick Stolee
2018-08-20 18:24 ` [PATCH v2 1/8] refs.c: migrate internal ref iteration to pass thru repository argument Derrick Stolee
2018-08-20 18:24 ` [PATCH v2 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback Derrick Stolee
2018-08-20 18:24 ` [PATCH v2 3/8] commit-graph: update design document Derrick Stolee
2018-08-20 18:24 ` [PATCH v2 4/8] test-repository: properly init repo Derrick Stolee
2018-08-20 18:24 ` [PATCH v2 5/8] commit-graph: not compatible with replace objects Derrick Stolee
2018-08-21 17:45 ` Junio C Hamano
2018-08-21 18:39 ` Derrick Stolee
2018-08-20 18:24 ` [PATCH v2 6/8] commit-graph: not compatible with grafts Derrick Stolee
2018-08-20 18:24 ` [PATCH v2 7/8] commit-graph: not compatible with uninitialized repo Derrick Stolee
2018-08-20 18:24 ` [PATCH v2 8/8] commit-graph: close_commit_graph before shallow walk Derrick Stolee
2018-08-20 19:06 ` [PATCH v2 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities Stefan Beller
2018-08-21 17:51 ` Junio C Hamano
2018-08-21 18:35 ` Derrick Stolee
2018-08-20 19:37 ` Stefan Beller
2018-08-20 19:54 ` Derrick Stolee
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180718194657.GC7778@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=jnareb@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).