Re: [PATCH 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities

[Jakub Narebski <jnareb@gmail.com>]

"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