From: Jakub Narebski <jnareb@gmail.com>
To: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 5/8] commit-graph: not compatible with replace objects
Date: Sun, 29 Jul 2018 23:00:19 +0200 [thread overview]
Message-ID: <86wotd7o64.fsf@gmail.com> (raw)
In-Reply-To: <7f596c1718d35539f02828edbf933c8e660f123b.1531926932.git.gitgitgadget@gmail.com> (Derrick Stolee via GitGitGadget's message of "Wed, 18 Jul 2018 08:15:41 -0700 (PDT)")
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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.
All right, looks sensible. Did you start with the history-"altering"
feature with simplest detection mechanism?
> Test this
> interaction succeeds, including ignoring an existing commit-graph and
> failing to write a new commit-graph.
I wonder if we should test the implementation, or just that you get
correct answer when history view is altered after writing commit-graph
file... oh, wait, you do that (well, except for testing that
`git commit-graph write` does not create commit-graph file instead of
testing that the commit-graph file stores correct information even after
the feature is removed (with `git replace --delete`) or turned off (with
`--no-replace-objects` or equivalent). Good.
Sidenote: the inconsistency between command options and
subcommands... ehh... compare e.g. git-replace and git-remote.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> commit-graph.c | 17 +++++++++++++++++
> replace-object.c | 2 +-
> replace-object.h | 2 ++
> t/t5318-commit-graph.sh | 22 ++++++++++++++++++++++
> 4 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index b0a55ad12..711099858 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -13,6 +13,8 @@
> #include "commit-graph.h"
> #include "object-store.h"
> #include "alloc.h"
> +#include "hashmap.h"
> +#include "replace-object.h"
>
> #define GRAPH_SIGNATURE 0x43475048 /* "CGPH" */
> #define GRAPH_CHUNKID_OIDFANOUT 0x4f494446 /* "OIDF" */
> @@ -56,6 +58,15 @@ static struct commit_graph *alloc_commit_graph(void)
> return g;
> }
>
> +static int commit_graph_compatible(struct repository *r)
> +{
> + prepare_replace_object(r);
> + if (hashmap_get_size(&r->objects->replace_map->map))
A very minor nitpick, feel free to ignore. I think that the condition
would be more readable written as
+ if (hashmap_get_size(&r->objects->replace_map->map) > 0)
Another issue is that the condition may be too strict (but it is also
the easiest to test for). You might have replacement objects for
non-commit objects (e.g. replacing contents of configuration files with
one with API keys and/or other secrets - not that I know any tool that
does that), or replacing commit-objects without changing their history
(e.g. to fix some annoying error in demo repository, where you want to
show reflog). But I think that would be rare, so easier if maybe
over-eager test is fine for me.
> + return 0;
> +
> + return 1;
> +}
> +
> struct commit_graph *load_commit_graph_one(const char *graph_file)
> {
> void *graph_map;
> @@ -223,6 +234,9 @@ static int prepare_commit_graph(struct repository *r)
> */
> return 0;
>
> + if (!commit_graph_compatible(r))
> + return 0;
> +
> obj_dir = r->objects->objectdir;
> prepare_commit_graph_one(r, obj_dir);
> prepare_alt_odb(r);
> @@ -693,6 +707,9 @@ void write_commit_graph(const char *obj_dir,
> int num_extra_edges;
> struct commit_list *parent;
>
> + if (!commit_graph_compatible(the_repository))
> + return;
> +
Hmmm... we might want to write commit-graph for immutable history; but
not all history-"altering" feature can be easily turned off. So if it
is to stay universal, without aadditional complications, it is good
enough.
> oids.nr = 0;
> oids.alloc = approximate_object_count() / 4;
>
> diff --git a/replace-object.c b/replace-object.c
> index 017f02f8e..4d2d84cf4 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;
> diff --git a/replace-object.h b/replace-object.h
> index f996de3d6..c7a99fc35 100644
> --- a/replace-object.h
> +++ b/replace-object.h
> @@ -10,6 +10,8 @@ struct replace_object {
> struct object_id replacement;
> };
>
> +void prepare_replace_object(struct repository *r);
> +
Hmmm... how it is that this function didn't need to be exported before,
I do wonder?
> /*
> * This internal function is only declared here for the benefit of
> * lookup_replace_object(). Please do not call it directly.
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 4f17d7701..c90626f5d 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -259,6 +259,28 @@ test_expect_success 'check that gc computes commit-graph' '
> test_cmp commit-graph-after-gc $objdir/info/commit-graph
> '
>
> +test_expect_success 'replace-objects invalidates commit-graph' '
> + cd "$TRASH_DIRECTORY" &&
> + test_when_finished rm -rf replace &&
> + git clone full replace &&
> + (
> + cd replace &&
> + git commit-graph write --reachable &&
> + test_path_is_file .git/objects/info/commit-graph &&
All right, this is preparation / setup; create commit-graph file, and
check that it was created.
> + git replace HEAD~1 HEAD~2 &&
All right, so before replacement the history looks like this:
... <--- A <--- B <--- C <--- D <--- master <=== HEAD
~3 ~2 ~1 HEAD
While after the replacement the history view looks like this, if I
understand it correctly:
... <--- A <--- B
\
\---[B] <--- D <--- master <=== HEAD
Wouldn't it be better (and more similar to the grafts test) to use
`git replace --graft`, e.g.:
+ git replace --graft HEAD~1 HEAD~3 &&
But that might be just a matter of taste.
> + git -c core.commitGraph=false log >expect &&
> + git -c core.commitGraph=true log >actual &&
> + test_cmp expect actual &&
All right, so instead of testing if the output is what is expected, we
test that the commit-graph feature did not change it. Good.
> + git commit-graph write --reachable &&
Is this necessary [see below]?
> + git -c core.commitGraph=false --no-replace-objects log >expect &&
> + git -c core.commitGraph=true --no-replace-objects log >actual &&
All right, and here we test the same for `--no-replace-objects` case.
Good.
By the way, is this `git commit-graph write --reachable` preceding this
part of test necessary?
> + test_cmp expect actual &&
> + rm -rf .git/objects/info/commit-graph &&
> + git commit-graph write --reachable &&
> + test_path_is_missing .git/objects/info/commit-graph
All right, so here we test that we wont write commit-graph file with
altered view of history; which would store wrong information if the
replace feature is turned off or if replace object is deleted (or if
commit graph gets transferred in the furure, but the replacement refs do
not).
Checking that the commit-graph file ws not created might be seen as
testing implementation details, but full test would require testing with
`--no-replace-objects` and after `git replace -d`; the above is simpler,
so good enough.
> + )
> +'
> +
> # the verify tests below expect the commit-graph to contain
> # exactly the commits reachable from the commits/8 branch.
> # If the file changes the set of commits in the list, then the
next prev parent reply other threads:[~2018-07-29 21:00 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 [this message]
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=86wotd7o64.fsf@gmail.com \
--to=jnareb@gmail.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.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).