From: "René Scharfe" <l.s.r@web.de>
To: Heather Lapointe via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Cc: Heather Lapointe <alpha@alphaservcomputing.solutions>
Subject: Re: [PATCH v2 0/2] archive: Add --recurse-submodules to git-archive command
Date: Thu, 13 Oct 2022 19:53:44 +0200 [thread overview]
Message-ID: <97a82675-22bb-b996-deac-3f13a91f3df4@web.de> (raw)
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!
> 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...
> 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.
>
>
> 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
>
next prev parent reply other threads:[~2022-10-13 17:55 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-12 17:52 [PATCH] archive: add --recurse-submodules to git-archive command Heather Lapointe via GitGitGadget
2022-10-13 11:35 ` [PATCH v2 0/2] archive: Add " Heather Lapointe via GitGitGadget
2022-10-13 11:35 ` [PATCH v2 1/2] archive: add " Alphadelta14 via GitGitGadget
2022-10-13 17:53 ` René Scharfe
2022-10-13 21:37 ` Heather Lapointe
2022-10-13 11:36 ` [PATCH v2 2/2] archive: fix a case of submodule in submodule traversal Alphadelta14 via GitGitGadget
2022-10-13 17:53 ` René Scharfe [this message]
2022-10-13 21:23 ` [PATCH v2 0/2] archive: Add --recurse-submodules to git-archive command Heather Lapointe
2022-10-14 9:47 ` René Scharfe
2022-10-17 2:23 ` [PATCH v3 0/9] " Heather Lapointe via GitGitGadget
2022-10-17 2:23 ` [PATCH v3 1/9] tree: do not use the_repository for tree traversal methods Alphadelta14 via GitGitGadget
2022-10-17 13:26 ` Junio C Hamano
2022-10-26 22:33 ` Glen Choo
2022-10-27 18:09 ` Jonathan Tan
2022-10-27 18:50 ` Junio C Hamano
2022-10-17 2:23 ` [PATCH v3 2/9] tree: update cases to use repo_ tree methods Heather Lapointe via GitGitGadget
2022-10-17 2:23 ` [PATCH v3 3/9] tree: increase test coverage for tree.c Heather Lapointe via GitGitGadget
2022-10-17 13:34 ` Phillip Wood
2022-10-17 13:36 ` Junio C Hamano
2022-10-27 18:28 ` Jonathan Tan
2022-10-17 2:23 ` [PATCH v3 4/9] tree: handle submodule case for read_tree_at properly Heather Lapointe via GitGitGadget
2022-10-17 13:48 ` Phillip Wood
2022-10-17 13:56 ` Junio C Hamano
2022-10-26 22:48 ` Glen Choo
2022-10-27 18:43 ` Jonathan Tan
2022-10-17 2:23 ` [PATCH v3 5/9] tree: add repository parameter to read_tree_fn_t Heather Lapointe via GitGitGadget
2022-10-17 2:23 ` [PATCH v3 6/9] archive: pass repo objects to write_archive handlers Heather Lapointe via GitGitGadget
2022-10-17 13:50 ` Phillip Wood
2022-10-17 2:23 ` [PATCH v3 7/9] archive: remove global repository from archive_args Heather Lapointe via GitGitGadget
2022-10-17 2:23 ` [PATCH v3 8/9] archive: add --recurse-submodules to git-archive command Heather Lapointe via GitGitGadget
2022-10-26 23:34 ` Glen Choo
2022-10-27 7:09 ` René Scharfe
2022-10-27 17:29 ` Glen Choo
2022-10-27 17:30 ` Glen Choo
2022-10-27 17:33 ` Glen Choo
2022-10-17 2:23 ` [PATCH v3 9/9] archive: add tests for git archive --recurse-submodules Heather Lapointe via GitGitGadget
2022-10-27 18:54 ` Jonathan Tan
2022-10-27 23:30 ` Glen Choo
2022-10-28 0:17 ` Ævar Arnfjörð Bjarmason
2022-10-17 13:57 ` [PATCH v3 0/9] archive: Add --recurse-submodules to git-archive command Phillip Wood
2022-10-18 18:34 ` Junio C Hamano
2022-10-18 18:48 ` Heather Lapointe
2022-10-19 16:16 ` Junio C Hamano
2022-10-19 20:44 ` Junio C Hamano
2022-10-20 1:21 ` Junio C Hamano
2022-10-21 1:43 ` Junio C Hamano
2022-10-26 22:14 ` Glen Choo
2022-10-28 18:18 ` Heather Lapointe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=97a82675-22bb-b996-deac-3f13a91f3df4@web.de \
--to=l.s.r@web.de \
--cc=alpha@alphaservcomputing.solutions \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).