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

[Derrick Stolee <stolee@gmail.com>]

On 8/21/2018 1:51 PM, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Mon, Aug 20, 2018 at 11:24 AM Derrick Stolee <dstolee@microsoft.com> wrote:
>>> Because of the change of base, it is hard to provide a side-by-side diff
>>> from v1.
>> I thought that was the whole point of range-diff. ;-)
> I thought so, too.  Was there any change that confused range-diff
> machinery?

I've just been out of the loop and didn't realize what range-diff did. 
Here is the range-diff:

1:  43ddcc9ef9 = 1:  c8edae4179 refs.c: migrate internal ref iteration 
to pass thru repository argument
2:  22dc9ce836 = 2:  f89451e884 refs.c: upgrade for_each_replace_ref to 
be a each_repo_ref_fn callback
3:  5e90d36f84 ! 3:  d95aa3472c commit-graph: update design document
     @@ -19,7 +19,7 @@
         so a future change of hash algorithm does not require a change 
in format.

      +- Commit grafts and replace objects can change the shape of the 
commit
     -+  history. These can also be enabled/disabled on the fly using
     ++  history. The latter can also be enabled/disabled on the fly using
      +  `--no-replace-objects`. This leads to difficultly storing both 
possible
      +  interpretations of a commit id, especially when computing 
generation
      +  numbers. The commit-graph will not be read or written when
4:  88711a3cf4 = 4:  ec89c88e14 test-repository: properly init repo
5:  7f596c1718 ! 5:  0321a3cf10 commit-graph: not compatible with 
replace objects
     @@ -6,10 +6,34 @@
          repository r is compatible with the commit-graph feature. Fill the
          method with a check to see if replace-objects exist. Test this
          interaction succeeds, including ignoring an existing 
commit-graph and
     -    failing to write a new commit-graph.
     +    failing to write a new commit-graph. However, we do ensure that
     +    we write a new commit-graph by setting read_replace_refs to 0, 
thereby
     +    ignoring the replace refs.

          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

     +diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
     +--- a/builtin/commit-graph.c
     ++++ b/builtin/commit-graph.c
     +@@
     +   return 0;
     + }
     +
     ++extern int read_replace_refs;
     ++
     + static int graph_write(int argc, const char **argv)
     + {
     +   struct string_list *pack_indexes = NULL;
     +@@
     +   if (!opts.obj_dir)
     +           opts.obj_dir = get_object_directory();
     +
     ++  read_replace_refs = 0;
     ++
     +   if (opts.reachable) {
     +           write_commit_graph_reachable(opts.obj_dir, opts.append);
     +           return 0;
     +
      diff --git a/commit-graph.c b/commit-graph.c
      --- a/commit-graph.c
      +++ b/commit-graph.c
     @@ -26,11 +50,15 @@
         return g;
       }

     ++extern int read_replace_refs;
     ++
      +static int commit_graph_compatible(struct repository *r)
      +{
     -+  prepare_replace_object(r);
     -+  if (hashmap_get_size(&r->objects->replace_map->map))
     -+          return 0;
     ++  if (read_replace_refs) {
     ++          prepare_replace_object(r);
     ++          if (hashmap_get_size(&r->objects->replace_map->map))
     ++                  return 0;
     ++  }
      +
      +  return 1;
      +}
     @@ -110,7 +138,7 @@
      +          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
     ++          test_path_is_file .git/objects/info/commit-graph
      +  )
      +'
      +
6:  94dd91ac35 ! 6:  d18ecc0124 commit-graph: not compatible with grafts
     @@ -7,24 +7,25 @@
          situations we ignore existing commit-graph files and we do not 
write new
          commit-graph files.

     +    Helped-by: Jakub Narebski <jnareb@gmail.com>
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

      diff --git a/commit-graph.c b/commit-graph.c
      --- a/commit-graph.c
      +++ b/commit-graph.c
      @@
     +                   return 0;
     +   }

     - 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;
      +
     -   prepare_replace_object(r);
     -   if (hashmap_get_size(&r->objects->replace_map->map))
     -           return 0;
     +   return 1;
     + }
     +

      diff --git a/commit.c b/commit.c
      --- a/commit.c
     @@ -66,7 +67,9 @@
      +          cd graft &&
      +          git commit-graph write --reachable &&
      +          test_path_is_file .git/objects/info/commit-graph &&
     -+          git replace --graft HEAD~1 HEAD~3 &&
     ++          H1=$(git rev-parse --verify HEAD~1) &&
     ++          H3=$(git rev-parse --verify HEAD~3) &&
     ++          echo "$H1 $H3" >.git/info/grafts &&
      +          git -c core.commitGraph=false log >expect &&
      +          git -c core.commitGraph=true log >actual &&
      +          test_cmp expect actual &&
7:  5314a5a93d ! 7:  87d3397700 commit-graph: not compatible with 
uninitialized repo
     @@ -14,6 +14,6 @@
      +  if (!r->gitdir)
      +          return 0;
      +
     -   prepare_commit_graft(r);
     -   if (r->parsed_objects && r->parsed_objects->grafts_nr)
     -           return 0;
     +   if (read_replace_refs) {
     +           prepare_replace_object(r);
     +           if (hashmap_get_size(&r->objects->replace_map->map))
8:  f4ab234ed2 ! 8:  a26c175897 commit-graph: close_commit_graph before 
shallow walk
     @@ -2,8 +2,6 @@

          commit-graph: close_commit_graph before shallow walk

     -    Make close_commit_graph() work for arbitrary repositories.
     -
          Call close_commit_graph() when about to start a rev-list walk that
          includes shallow commits. This is necessary in code paths that 
"fake"
          shallow commits for the sake of fetch. Specifically, test 351 in