From: Junio C Hamano <gitster@pobox.com>
To: Derrick Stolee <dstolee@microsoft.com>
Cc: "git\@vger.kernel.org" <git@vger.kernel.org>,
"jnareb\@gmail.com" <jnareb@gmail.com>,
"sbeller\@google.com" <sbeller@google.com>
Subject: Re: [PATCH v2 5/8] commit-graph: not compatible with replace objects
Date: Tue, 21 Aug 2018 10:45:26 -0700 [thread overview]
Message-ID: <xmqqa7pfaa15.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20180820182359.165929-6-dstolee@microsoft.com> (Derrick Stolee's message of "Mon, 20 Aug 2018 18:24:27 +0000")
Derrick Stolee <dstolee@microsoft.com> writes:
> 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. However, we do ensure that
> we write a new commit-graph by setting read_replace_refs to 0, thereby
> ignoring the replace refs.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> builtin/commit-graph.c | 4 ++++
> commit-graph.c | 21 +++++++++++++++++++++
> replace-object.c | 2 +-
> replace-object.h | 2 ++
> t/t5318-commit-graph.sh | 22 ++++++++++++++++++++++
> 5 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 0bf0c48657..da737df321 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -120,6 +120,8 @@ static int graph_read(int argc, const char **argv)
> return 0;
> }
>
> +extern int read_replace_refs;
> +
Why do you need this (and also in commit-graph.c)? I thought
cache.h includes it, which you can just make use of it.
> +static int commit_graph_compatible(struct repository *r)
> +{
> + if (read_replace_refs) {
> + prepare_replace_object(r);
> + if (hashmap_get_size(&r->objects->replace_map->map))
> + return 0;
> + }
> +
> + return 1;
> +}
> diff --git a/replace-object.c b/replace-object.c
> index 3c17864eb7..9821f1477e 100644
> --- a/replace-object.c
> +++ b/replace-object.c
> @@ -32,7 +32,7 @@ static int register_replace_ref(struct repository *r,
> return 0;
> }
>
> -static void prepare_replace_object(struct repository *r)
> +void prepare_replace_object(struct repository *r)
> {
> if (r->objects->replace_map)
> return;
The way the new caller is written, I am wondering if this function
should return "we are (or, are not) using the replace object
feature", making it unnecessary for callers on the reading side to
know about "read-replace-refs" external variable, for example.
/*
* To be called on-demand from codepaths that want to make
* sure that replacement objects can be found as necessary.
*
* Return number of replacement defined for the repository, or
* -1 when !read_replace_refs tells us not to use replacement
* mechanism at all.
*/
int prepare_replace_object(struct repository *r)
{
if (!read_replace_refs)
return -1;
if (!r->objects->replace_map) {
r->objects->replace_map =
xmalloc(...);
oidmap_init(r->objects->replace_map, 0);
for_each_refplace_ref(r, register_...);
}
return hashmap_get_size(&r->objects->replace_map->map);
}
Then, the caller side can simply become something like:
#define cgraph_compat(r) (prepare_replace_object(r) <= 0)
There are various writers to read_replace_refs variable, but I think
they should first be replaced with calls to something like:
void use_replace_refs(struct repository *r, int enable);
which allows us to hide the global variable and later make it per
repository if we wanted to.
next prev parent reply other threads:[~2018-08-21 17:45 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
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 [this message]
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=xmqqa7pfaa15.fsf@gitster-ct.c.googlers.com \
--to=gitster@pobox.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=jnareb@gmail.com \
--cc=sbeller@google.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).