Re: [PATCH v2 0/2] archive: Add --recurse-submodules to git-archive command
- From
René Scharfe <l.s.r@web.de>
- Date
- Oct 13, 2022, 17:53 UTC
- Message-ID
- <97a82675-22bb-b996-deac-3f13a91f3df4@web.de>
- In-Reply-To
- <pull.1359.v2.git.git.1665660960.gitgitgadget@gmail.com>
Am 13.10.22 um 13:35 schrieb Heather Lapointe via GitGitGadget:
> This makes it possible to include submodule contents in an archive command.
Great!
Show 19 quoted lines
> 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 :) > > TODO:
This list confuses me:
> * working implementation
What exactly is not working, yet?
> * valgrind
What's up with it? Does is report errors or leaks?
> * add regression tests
This series adds a new test script. Do you plan to add more checks?
> * update documentation with new flag
That I can understand: Indeed Documentation/git-archive.txt would need an update.
> * submit to mailing list
But you already did submit two iterations of this series to the Git mailing list!?
> > Alphadelta14 (2): > archive: add --recurse-submodules to git-archive command > archive: fix a case of submodule in submodule traversal
We prefer to keep known bugs out of the repo. It helps when bisecting, for example. So it would be better to squash the fix into the patch that adds the feature. But...
Show 18 quoted lines
> archive-tar.c | 14 +++-- > archive-zip.c | 14 ++--- > archive.c | 99 ++++++++++++++++++++++++----------- > archive.h | 8 +-- > builtin/checkout.c | 2 +- > builtin/log.c | 2 +- > builtin/ls-files.c | 10 ++-- > builtin/ls-tree.c | 16 +++--- > list-objects.c | 2 +- > merge-recursive.c | 2 +- > revision.c | 4 +- > sparse-index.c | 2 +- > t/t5005-archive-submodules.sh | 84 +++++++++++++++++++++++++++++ > tree.c | 93 ++++++++++++++++++++++---------- > tree.h | 11 ++-- > wt-status.c | 2 +- > 16 files changed, 269 insertions(+), 96 deletions(-) > create mode 100755 t/t5005-archive-submodules.sh
... this is all a bit much for a single patch, I feel. Giving parse_tree_gently() a repo parameter, adding repo_parse_tree(), using it in read_tree_at(), adding a repo parameter to read_tree_fn_t, letting read_tree_at() recurse into submodules and adding the new option to git archive all seem like topics worth their own patch and rationale.
You probably have all of that in your head right now, but at least my attention span and working memory capacity requires smaller morsels.
Show 12 quoted lines
> > > base-commit: e85701b4af5b7c2a9f3a1b07858703318dce365d > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1359%2FAlphadelta14%2Farchive-recurse-submodules-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1359/Alphadelta14/archive-recurse-submodules-v2 > Pull-Request: https://github.com/git/git/pull/1359 > > Range-diff vs v1: > > 1: 41664a59029 = 1: 41664a59029 archive: add --recurse-submodules to git-archive command > -: ----------- > 2: 68f7830c6d9 archive: fix a case of submodule in submodule traversal >