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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx [2] https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?pli=1# "Heather Lapointe via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > 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.
