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 6/8] commit-graph: not compatible with grafts
Date: Mon, 30 Jul 2018 00:04:25 +0200 [thread overview]
Message-ID: <86bmap7l7a.fsf@gmail.com> (raw)
In-Reply-To: <94dd91ac35006ebee3c8808af1fce4ad69975234.1531926932.git.gitgitgadget@gmail.com> (Derrick Stolee via GitGitGadget's message of "Wed, 18 Jul 2018 08:15:43 -0700 (PDT)")
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Augment commit_graph_compatible(r) to return false when the given
> repository r has commit grafts or is a shallow clone. Test that in these
> situations we ignore existing commit-graph files and we do not write new
> commit-graph files.
Tests for grafts are wrong, as they test replace objects.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> commit-graph.c | 6 ++++++
> commit.c | 2 +-
> commit.h | 1 +
> t/t5318-commit-graph.sh | 36 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 711099858..5097c7c12 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -60,6 +60,12 @@ static struct commit_graph *alloc_commit_graph(void)
>
> static int commit_graph_compatible(struct repository *r)
> {
> + prepare_commit_graft(r);
> + if (r->parsed_objects && r->parsed_objects->grafts_nr)
> + return 0;
> + if (is_repository_shallow(r))
> + return 0;
Do I assume correctly that is_repository_shallow(r) needs
prepare_commit_graph(r) done earlier? If it is so, all right.
If it is not so, then why put two separate history-"altering" features,
namely grafts and shallow clones in single commit instead of in separate
commits?
> +
> prepare_replace_object(r);
> if (hashmap_get_size(&r->objects->replace_map->map))
> return 0;
> diff --git a/commit.c b/commit.c
> index 39b80bd21..609adaf8a 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -209,7 +209,7 @@ static int read_graft_file(struct repository *r, const char *graft_file)
> return 0;
> }
>
> -static void prepare_commit_graft(struct repository *r)
> +void prepare_commit_graft(struct repository *r)
> {
> char *graft_file;
>
> diff --git a/commit.h b/commit.h
> index da0db36eb..5459e279f 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -202,6 +202,7 @@ typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);
>
> struct commit_graft *read_graft_line(struct strbuf *line);
> int register_commit_graft(struct repository *r, struct commit_graft *, int);
> +void prepare_commit_graft(struct repository *r);
> struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);
>
I guess that prepare_commit_graft() was not needed outside commit.c
before.
> extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2);
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index c90626f5d..1d9f49af5 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -281,6 +281,42 @@ test_expect_success 'replace-objects invalidates commit-graph' '
> )
> '
>
> +test_expect_success 'commit grafts invalidate commit-graph' '
> + cd "$TRASH_DIRECTORY" &&
> + test_when_finished rm -rf graft &&
> + git clone full graft &&
> + (
> + cd graft &&
> + git commit-graph write --reachable &&
> + test_path_is_file .git/objects/info/commit-graph &&
> + git replace --graft HEAD~1 HEAD~3 &&
Errr... this uses *replace objects* mechanism, not graft file!! The
`--graft` option of git-remote command is here to make it easier to use
replace objects to alter the view of history, and make if easy to
convert from deprected grafts feature to replace objects.
Grafts file structure is described in gitrepository-layout(5) manpage.
You would need to do something like the following:
+ H1=$(git rev-parse --verify HEAD~1) &&
+ H3=$(git rev-parse --verify HEAD~3) &&
+ echo '$H1 $H3' >.git/info/grafts &&
You could have used "git replace --graft HEAD~1 HEAD~3" in previous
commit test, like I wrote in reply to it.
> + git -c core.commitGraph=false log >expect &&
> + git -c core.commitGraph=true log >actual &&
> + test_cmp expect actual &&
All right, we test that we get the same result (when graft-file is
written before altering the view of history) with and without the
commit-graph feature.
Sidenote: shouldn't we use rev-list instead? Though here git-log is
actually safe to use...
> + git commit-graph write --reachable &&
> + git -c core.commitGraph=false --no-replace-objects log >expect &&
> + git -c core.commitGraph=true --no-replace-objects log >actual &&
The `--no-replace-objects` does not affect the graph file!! You would
want to remove graft file instead:
+ git commit-graph write --reachable &&
+ rm -f .git/info/grafts &&
+ git -c core.commitGraph=false log >expect &&
+ git -c core.commitGraph=true log >actual &&
And then check the results.
> + 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.
Though, if it is the same for grafts and for replacements... nah.
Adding a feature and deleting a feature is different, and it is only a
single repetition.
> + )
> +'
> +
> +test_expect_success 'replace-objects invalidates commit-graph' '
> + cd "$TRASH_DIRECTORY" &&
> + test_when_finished rm -rf shallow &&
> + git clone --depth 2 "file://$TRASH_DIRECTORY/full" shallow &&
Why do we need to use "file://..." URL, instead of simply "full"?
> + (
> + cd shallow &&
> + git commit-graph write --reachable &&
> + test_path_is_missing .git/objects/info/commit-graph &&
> + git fetch origin --unshallow &&
> + git commit-graph write --reachable &&
> + test_path_is_file .git/objects/info/commit-graph
All right, so in this case because you need to start (clone) with
shallow feature, the test could be simplified.
> + )
> +'
> +
> # 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 22:04 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 [this message]
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=86bmap7l7a.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).