"Derrick Stolee via GitGitGadget" <gitgitgad...@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-dsto...@microsoft.com/ > [RFC PATCH 0/6] Fix commit-graph/graft/replace/shallow combo > > [2] > https://public-inbox.org/git/20180717224935.96397-1-sbel...@google.com/T/#t > [PATCH 0/2] RFC ref store to repository migration > > [3] > https://public-inbox.org/git/20180717224935.96397-1-sbel...@google.com/T/#m966eac85fd58c66523654ddaf0bec72877d3295a > [PATCH] TO-SQUASH: replace the_repository with arbitrary r > > Based-On: jt/commit-graph-per-object-store > Cc: jonathanta...@google.com > Cc: sbel...@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
