git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Derrick Stolee <stolee@gmail.com>
To: Jeff King <peff@peff.net>,
	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:52:54 -0400	[thread overview]
Message-ID: <f5d2f06d-1736-57e4-edbd-aa638ae34238@gmail.com> (raw)
In-Reply-To: <20180718194657.GC7778@sigill.intra.peff.net>

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


  parent reply	other threads:[~2018-07-18 19:52 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
2018-07-18 19:48     ` Jeff King
2018-07-18 19:52     ` Derrick Stolee [this message]
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=f5d2f06d-1736-57e4-edbd-aa638ae34238@gmail.com \
    --to=stolee@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=jnareb@gmail.com \
    --cc=peff@peff.net \
    /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).