Re: [PATCH v3 0/9] archive: Add --recurse-submodules to git-archive command

From: Glen Choo <chooglen@google.com>
Date: Oct 26, 2022, 22:14 UTC
Message-ID: <kl6lr0yuqlk0.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <pull.1359.v3.git.git.1665973401.gitgitgadget@gmail.com>

Hi Heather!

We covered this series in Review Club [1]. We will leave review on this thread, though you may find the notes [2] useful.

[1] https://lore.kernel.org/git/kl6l35bbsubq.fsf@chooglen-macbookpro.roam.corp.google.com [2] https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?pli=1#

"Heather Lapointe via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 19 quoted lines

> This makes it possible to include submodule contents in an archive command.
>
> The inspiration for this change comes from this Github thread,
> https://github.com/dear-github/dear-github/issues/214, with at least 160
> 👍🏻 's at the time of writing. (I stumbled upon it because I wanted it as
> well).
>
> I figured the underlying implementation wouldn't be too difficult with most
> of the plumbing already in place, so I decided to add the relevant logic to
> the client git-archive command.
>
> One of the trickier parts of this implementation involved teaching read_tree
> about submodules. Some of the troublesome areas were still using the
> the_repository references to look up commit or tree or oid information. I
> ended up deciding that read_tree_fn_t would probably be best off having a
> concrete repo reference since it allows changing the context to a subrepo
> where needed (even though some of the usages did not need it specifically).
>
> I am open to feedback since this is all quite new to me :)

The Review Club participants generally agreed that this is a really well-structured and easy-to-follow series :) As far as new contributions go, this is really good.

I think this series broadly makes sense, i.e.:

- the implementation of plumbing "struct repository" through read_tree()
  (this might also be really helpful for future work)
- the interface (using "--recurse-submodules")
- the expected behavior

So I can see this going through with a bit of polish. The others have covered style issues quite thoroughly, so I won't comment on those.