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>
Subject: Re: [PATCH 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities
Date: Sun, 29 Jul 2018 15:13:10 +0200 [thread overview]
Message-ID: <86h8ki89sp.fsf@gmail.com> (raw)
In-Reply-To: <pull.11.git.gitgitgadget@gmail.com> (Derrick Stolee via GitGitGadget's message of "Wed, 18 Jul 2018 08:15:34 -0700 (PDT)")
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
Sidenote: the GitGitGadget doesn't fold/wrap lines properly in the
cover-letter. Not that it is much of a problem.
> One unresolved issue with the commit-graph feature is that it can
> cause issues when combined with replace objects, commit grafts, or
> shallow clones. These are not 100% incompatible, as one could be
> reasonably successful writing a commit-graph after replacing some
> objects and not have issues. The problems happen when commits that are
> already in the commit-graph file are replaced, or when git is run with
> the `--no-replace-objects` option; this can cause incorrect parents or
> incorrect generation numbers. Similar things occur with commit grafts
> and shallow clones, especially when running `git fetch --unshallow` in
> a shallow repo.
This means that these features can change the view of the history, and
if the commit-graph was created before the change, it can have stale
incorrect information -- which would lead to incorrect results if the
commit-graph feature is used.
>
> Instead of trying (and probably failing) to make these features work
> together, default to making the commit-graph feature unavailable in
> these situations. Create a new method 'commit_graph_compatible(r)'
> that checks if the repository 'r' has any of these features enabled.
Yes, the alternative would be trying to keep the commit-graph file
up-to-date with the current view of history. But there are problems
with this approach.
With grafts (which is a deprecated feature), there is no automated entry
point. Best what one could do is to store some kind of fingerprint of
graft file, and invalidate / recreate commit-graft file if it does not
match what is in graft file at the time of running git command that may
use commit-graft feature.
With replace objects we have two separate entry points: the git-replace
command and fetching references in refs/replace/* namespace (and
equivalent). While the first should be not difficult to do, the second
one seems to be harder. Additionally, there is '--no-replace-objects'
option to turn off the feature (fetch and push ignore replacement
objects automatically); so we might want to have two versions of
commit-graph: one with replacement objects feature and one without (or
something like that).
The shallow clone seems easiest, with only one automated entry points
for changing the view of history this way, and no option to disable this
feature -- but it is also the least interesting, as the intent of
shallow clone is to have less history, so the commit-graph is less
necessary, as you wrote.
We might want to revisit it later, but I agree that starting from this
simple approach would be best.
One thing I would like to see added is for the user to know when
commit-graph is not available (in manpages), and maybe even a way to see
if it is on (e.g. with 'git commit-graph verify' and/or maybe in
'git status' output). But this is a separate issue.
>
> I will send a follow-up patch that shows how I tested these
> interactions by computing the commit-graph on every 'git commit'.
Wouldn't it be enough to create commit-graph file, change the view of
the history (via grafts, via replace objects, via shallow clone), and
check that you still get correct results?
[cut]
>
> This approach is very different from the previous RFC on the subject [1].
The previous RFC was in my opinion needlessly complicated. This one is
much simpler, and better.
[...]
> Thanks,
> -Stolee
>
> [1] https://public-inbox.org/git/20180531174024.124488-1-dstolee@microsoft.com/
> [RFC PATCH 0/6] Fix commit-graph/graft/replace/shallow combo
>
> [2] https://public-inbox.org/git/20180717224935.96397-1-sbeller@google.com/T/#t
> [PATCH 0/2] RFC ref store to repository migration
>
> [3] https://public-inbox.org/git/20180717224935.96397-1-sbeller@google.com/T/#m966eac85fd58c66523654ddaf0bec72877d3295a
> [PATCH] TO-SQUASH: replace the_repository with arbitrary r
>
> Based-On: jt/commit-graph-per-object-store
> Cc: jonathantanmy@google.com
> Cc: sbeller@google.com
>
> Derrick Stolee (6):
> commit-graph: update design document
> test-repository: properly init repo
> commit-graph: not compatible with replace objects
> commit-graph: not compatible with grafts
> commit-graph: not compatible with uninitialized repo
> commit-graph: close_commit_graph before shallow walk
>
> Stefan Beller (2):
> refs.c: migrate internal ref iteration to pass thru repository
> argument
> refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback
>
> Documentation/technical/commit-graph.txt | 18 ++++++--
> builtin/replace.c | 8 ++--
> commit-graph.c | 34 ++++++++++++--
> commit-graph.h | 1 +
> commit.c | 2 +-
> commit.h | 1 +
> refs.c | 48 +++++++++++++++++---
> refs.h | 12 ++++-
> refs/iterator.c | 6 +--
> refs/refs-internal.h | 5 +-
> replace-object.c | 7 +--
> replace-object.h | 2 +
> t/helper/test-repository.c | 10 +++-
> t/t5318-commit-graph.sh | 58 ++++++++++++++++++++++++
> upload-pack.c | 2 +
> 15 files changed, 184 insertions(+), 30 deletions(-)
>
>
> base-commit: dade47c06cf849b0ca180a8e6383b55ea6f75812
> Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-11%2Fderrickstolee%2Fshallow%2Fupstream-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-11/derrickstolee/shallow/upstream-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/11
next prev parent reply other threads:[~2018-07-29 13:14 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 ` Jakub Narebski [this message]
2018-07-29 19:27 ` [PATCH 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities 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=86h8ki89sp.fsf@gmail.com \
--to=jnareb@gmail.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).