Re: [PATCH v2 1/2] archive: add --recurse-submodules to git-archive command

[Heather Lapointe <alpha@alphaservcomputing.solutions>]

---- On Thu, 13 Oct 2022 10:53:46 -0700 René Scharfe  wrote ---

 > Am 13.10.22 um 13:35 schrieb Alphadelta14 via GitGitGadget: 
 > > From: Alphadelta14 alpha@alphaservcomputing.solutions> 
 > > 
 >  
 > > diff --git a/tree.c b/tree.c 
 > > index 410e3b477e5..c5b5a0ac08f 100644 
 > > --- a/tree.c 
 > > +++ b/tree.c 
 > > @@ -8,6 +8,7 @@ 
 > >  #include "alloc.h" 
 > >  #include "tree-walk.h" 
 > >  #include "repository.h" 
 > > +#include "pathspec.h" 
 > > 
 > >  const char *tree_type = "tree"; 
 > > 
 > > @@ -22,8 +23,8 @@ int read_tree_at(struct repository *r, 
 > >      int len, oldlen = base->len; 
 > >      enum interesting retval = entry_not_interesting; 
 > > 
 > > -    if (parse_tree(tree)) 
 > > -        return -1; 
 > > +    if (repo_parse_tree(r, tree)) 
 > > +        die("Failed to parse tree"); 
 > > 
 > >      init_tree_desc(&desc, tree->buffer, tree->size); 
 > > 
 > > @@ -37,7 +38,7 @@ int read_tree_at(struct repository *r, 
 > >                  continue; 
 > >          } 
 > > 
 > > -        switch (fn(&entry.oid, base, 
 > > +        switch (fn(r, &entry.oid, base, 
 > >                 entry.path, entry.mode, context)) { 
 > >          case 0: 
 > >              continue; 
 > > @@ -47,36 +48,57 @@ int read_tree_at(struct repository *r, 
 > >              return -1; 
 > >          } 
 > > 
 > > -        if (S_ISDIR(entry.mode)) 
 > > +        if (S_ISDIR(entry.mode)) { 
 > >              oidcpy(&oid, &entry.oid); 
 > > -        else if (S_ISGITLINK(entry.mode)) { 
 >  
 > So you remove the non-recursive handling of submodules here... 

The original logic looked like it had not been thoroughly used in the past years.
It was performing a commit lookup within the submodule using the superproject
repository, which would not actually work unless there was global state
being contextually pushed where `the_repository` was being replaced
with a submodule repository instance (it appeared to not be the case currently).

 >  
 > > +            len = tree_entry_len(&entry); 
 > > +            strbuf_add(base, entry.path, len); 
 > > +            strbuf_addch(base, '/'); 
 > > +            retval = read_tree_at(r, lookup_tree(r, &oid), 
 > > +                        base, pathspec, 
 > > +                        fn, context); 
 > > +            strbuf_setlen(base, oldlen); 
 > > +            if (retval) 
 > > +                return -1; 
 > > +        } else if (pathspec->recurse_submodules && S_ISGITLINK(entry.mode)) { 
 >  
 > ... and add recursive handling here, and there is no further else 
 > branch.  Why do we no longer need the non-recursive variant? 

The else case is that we either have submodules, but we don't have pathspec->recurse_submodules.
Both the dir case and submodule cases handle recursion
(but with enough difference that it didn't make sense to use the same logic).
The other else case we simply not recurse (for files).

 >  
 > >              struct commit *commit; 
 > > +            struct repository subrepo; 
 > > +            struct repository* subrepo_p = &subrepo; 
 > > +            struct tree* submodule_tree; 
 > > 
 > > -            commit = lookup_commit(r, &entry.oid); 
 > > +            if (repo_submodule_init(subrepo_p, r, entry.path, null_oid())) 
 > > +                die("couldn't init submodule %s%s", base->buf, entry.path); 
 > > + 
 > > +            if (repo_read_index(subrepo_p) < 0) 
 > > +                die("index file corrupt"); 
 > > + 
 > > +            commit = lookup_commit(subrepo_p, &entry.oid); 
 > >              if (!commit) 
 > >                  die("Commit %s in submodule path %s%s not found", 
 > >                      oid_to_hex(&entry.oid), 
 > >                      base->buf, entry.path); 
 > > 
 > > -            if (parse_commit(commit)) 
 > > +            if (repo_parse_commit(subrepo_p, commit)) 
 > >                  die("Invalid commit %s in submodule path %s%s", 
 > >                      oid_to_hex(&entry.oid), 
 > >                      base->buf, entry.path); 
 > > 
 > > -            oidcpy(&oid, get_commit_tree_oid(commit)); 
 > > +            submodule_tree = repo_get_commit_tree(subrepo_p, commit); 
 > > +            oidcpy(&oid, submodule_tree ? &submodule_tree->object.oid : NULL); 
 > > + 
 > > +            len = tree_entry_len(&entry); 
 > > +            strbuf_add(base, entry.path, len); 
 > > +            strbuf_addch(base, '/'); 
 > > +            retval = read_tree_at(subrepo_p, lookup_tree(subrepo_p, &oid), 
 > > +                        base, pathspec, 
 > > +                        fn, context); 
 > > +            if (retval) { 
 > > +                die("failed to read tree for %s%s", base->buf, entry.path); 
 > > +                return -1; 
 > > +            } 
 > > +            strbuf_setlen(base, oldlen); 
 > > +            repo_clear(subrepo_p); 
 > >          } 
 > > -        else 
 > > -            continue; 
 > > 
 > > -        len = tree_entry_len(&entry); 
 > > -        strbuf_add(base, entry.path, len); 
 > > -        strbuf_addch(base, '/'); 
 > > -        retval = read_tree_at(r, lookup_tree(r, &oid), 
 > > -                      base, pathspec, 
 > > -                      fn, context); 
 > > -        strbuf_setlen(base, oldlen); 
 > > -        if (retval) 
 > > -            return -1; 
 > >      } 
 > >      return 0; 
 > >  } 
 > > @@ -121,7 +143,7 @@ int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size) 
 > >      return 0; 
 > >  } 
 > > 
 > > -int parse_tree_gently(struct tree *item, int quiet_on_missing) 
 > > +int parse_tree_gently(struct repository *r, struct tree *item, int quiet_on_missing) 
 > >  { 
 > >       enum object_type type; 
 > >       void *buffer; 
 > > @@ -129,7 +151,7 @@ int parse_tree_gently(struct tree *item, int quiet_on_missing) 
 > > 
 > >      if (item->object.parsed) 
 > >          return 0; 
 > > -    buffer = read_object_file(&item->object.oid, &type, &size); 
 > > +    buffer = repo_read_object_file(r, &item->object.oid, &type, &size); 
 > >      if (!buffer) 
 > >          return quiet_on_missing ? -1 : 
 > >              error("Could not read %s", 
 >  
 >