archive: Add --recurse-submodules to git-archive command #1359
Conversation
Welcome to GitGitGadgetHi @Alphadelta14, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that your Pull Request has a good description, as it will be used as cover letter. You can CC potential reviewers by adding a footer to the PR description with the following syntax: Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
There are issues in commit b8e745f: |
fd3e3b2 to
9c43294
Compare
|
There are issues in commit e957a74: |
|
There are issues in commit acd92e2: |
acd92e2 to
41664a5
Compare
|
/allow |
|
User Alphadelta14 is now allowed to use GitGitGadget. WARNING: Alphadelta14 has no public email address set on GitHub; |
|
/submit |
|
Submitted as pull.1359.git.git.1665597148042.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
/submit |
|
Submitted as pull.1359.v2.git.git.1665660960.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, René Scharfe wrote (reply to this): 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
>
|
|
User |
|
On the Git mailing list, Heather Lapointe wrote (reply to this): ---- On Thu, 13 Oct 2022 10:53:44 -0700 René Scharfe wrote ---
> > I am open to feedback since this is all quite new to me :)
> >
> > TODO:
>
> This list confuses me:
I apologize. I'm new to this repo and workflow.
I had been using checkboxes in github, which look like `- [x]` for ones that I have completed.
They all got converted into items that look like they needed doing via GitGitGadget.
The only remaining one was to update documentation.
>
> >
> > 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...
Absolutely can do.
>
> > 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.
Does this mean I should create multiple PRs?
Or should they just be split up into individual commits.
I will work off assuming the latter.
I am comfortable rewriting history as long as I understand the direction to go in.
Should each individual patch be completely standalone?
(To the point where each set, with the previous patches should produce a working application?
Or is having the patch broken up by groups of changes, with some level of expecting the final
functionality good?)
But thanks so far. I will get working on this (and review your next set of messages).
|
|
On the Git mailing list, René Scharfe wrote (reply to this): Am 13.10.22 um 23:23 schrieb Heather Lapointe:
> I had been using checkboxes in github, which look like `- [x]` for
> ones that I have completed. They all got converted into items that
> look like they needed doing via GitGitGadget.
>
> The only remaining one was to update documentation.
I opened https://github.com/html-to-text/node-html-to-text/issues/260 to
see if they are willing to support this kind of to-do lists better.
> Does this mean I should create multiple PRs? Or should they just be
> split up into individual commits. I will work off assuming the
> latter.
Right, I meant multiple commits.
> Should each individual patch be completely standalone? (To the point
> where each set, with the previous patches should produce a working
> application? Or is having the patch broken up by groups of changes,
> with some level of expecting the final functionality good?)
Yes, each commit should leave Git in a working state. Adding new global
functions that are only used by later commits in the series is fine.
René |
Expect that tree walking may switch repository contexts for cases such as submodules. Added compatibility macros for existing cases. Annotate an existing issue where repo is wrong when traversing. Signed-off-by: Heather Lapointe <alpha@alphaservcomputing.solutions>
68f7830 to
654758e
Compare
|
There are issues in commit 6b2d0c9: |
|
There are issues in commit 4d88d18: |
For cases which had already had a repository instance, update those to use the repo_parse_tree* methods. Leave the remaining invocations that were already using the_repository alone. Signed-off-by: Heather Lapointe <alpha@alphaservcomputing.solutions>
654758e to
6a54a48
Compare
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Junio C Hamano <gitster@pobox.com> writes:
>> I personally doubt it is generally a good idea, as it sets a bad
>> pattern that tempts unsuspecting users to blindly copy and paste it
>> to their $HOME/.gitconfig without realizing what its ramifications
>> are, but the easiest workaround may be to mimic what was done in
>> t/lib-submodule-update.sh that sets protocol.file.allow
>> configuration knob globally.
>
> I'll queue this at the tip of your topic when I rebuild 'seen' for
> today's integration run.
>
> t/t1023-tree-read-tree-at.sh | 4 +++-
> t/t5005-archive-submodules.sh | 7 ++++---
> 2 files changed, 7 insertions(+), 4 deletions(-)
It seems to have cleared the "submodule tests no longer can use
submodules with file:// without tweaking the config" issue I saw
earlier. It seems to give us a segfault in win+VS test, though.
https://github.com/git/git/actions/runs/3285647856/jobs/5413033844#step:5:245
Thanks.
|
|
There was a status update in the "Cooking" section about the branch "git archive" has been taught "--recurse-submodules" option to create a tarball that includes contents from submodules. Expecting a reroll. source: <pull.1359.v3.git.git.1665973401.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch "git archive" has been taught "--recurse-submodules" option to create a tarball that includes contents from submodules. Expecting a reroll. source: <pull.1359.v3.git.git.1665973401.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via aea3d0c. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Junio C Hamano <gitster@pobox.com> writes:
> It seems to have cleared the "submodule tests no longer can use
> submodules with file:// without tweaking the config" issue I saw
> earlier. It seems to give us a segfault in win+VS test, though.
>
> https://github.com/git/git/actions/runs/3285647856/jobs/5413033844#step:5:245
Here is a pair of CI run that attributes the breakage to this topic:
https://github.com/git/git/actions/runs/3293333066
is one CI run on 'seen' that has this topic and everything else in
flight. This topic is at the tip of 'seen' when this CI run was
done, and win+VS test (8) seems to be failing the same way as the
previous one I reported earlier above.
Dropping the merge of this topic (i.e. "git reset --hard HEAD^") out
of 'seen' and running CI again:
https://github.com/git/git/actions/runs/3293553109
we can see that all tests pass there, which unfortunately is a rare
event these days (well, the segfaulting code is something this topic
adds, so it is not surprising that the rest of the topics in flight
would not segfault the same way).
Do you need help from somebody equipped with Windows knowledge and
build/test environment? As I do not do Windows or macOS, I cannot
offer to be one myself, but the development community is full of
capable folks and help is often a request away.
Thanks. |
|
This patch series was integrated into seen via efab989. |
|
There was a status update in the "Cooking" section about the branch "git archive" has been taught "--recurse-submodules" option to create a tarball that includes contents from submodules. Expecting a reroll. Seems to break win+VS test(8). cf. https://github.com/git/git/actions/runs/3293333066 whose only difference from https://github.com/git/git/actions/runs/3293553109 is the inclusion of this topic. source: <pull.1359.v3.git.git.1665973401.gitgitgadget@gmail.com> |
|
On the Git mailing list, Glen Choo wrote (reply to this): 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:
> 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. |
|
User |
|
On the Git mailing list, Heather Lapointe wrote (reply to this): On Wed, Oct 26, 2022 at 6:15 PM Glen Choo <chooglen@google.com> wrote:
> 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.
Thank you! I've started looking through a lot of these!
I have been a bit swamped with my own work or I would have contributed
another patch series by now. |
|
User |
|
There was a status update in the "Cooking" section about the branch "git archive" has been taught "--recurse-submodules" option to create a tarball that includes contents from submodules. Expecting a reroll. Seems to break win+VS test(8). cf. https://github.com/git/git/actions/runs/3293333066 whose only difference from https://github.com/git/git/actions/runs/3293553109 is the inclusion of this topic. source: <pull.1359.v3.git.git.1665973401.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via ce948a5. |
|
This patch series was integrated into seen via 599b7ae. |
|
This patch series was integrated into seen via b3fdd3a. |
|
This patch series was integrated into seen via f6d8d35. |
|
There was a status update in the "Stalled" section about the branch "git archive" has been taught "--recurse-submodules" option to create a tarball that includes contents from submodules. Expecting a reroll. Seems to break win+VS test(8). cf. https://github.com/git/git/actions/runs/3293333066 whose only difference from https://github.com/git/git/actions/runs/3293553109 is the inclusion of this topic. source: <pull.1359.v3.git.git.1665973401.gitgitgadget@gmail.com> |
|
There was a status update in the "Stalled" section about the branch "git archive" has been taught "--recurse-submodules" option to create a tarball that includes contents from submodules. Expecting a reroll. Seems to break win+VS test(8). cf. https://github.com/git/git/actions/runs/3293333066 whose only difference from https://github.com/git/git/actions/runs/3293553109 is the inclusion of this topic. source: <pull.1359.v3.git.git.1665973401.gitgitgadget@gmail.com> |
|
There was a status update in the "Stalled" section about the branch "git archive" has been taught "--recurse-submodules" option to create a tarball that includes contents from submodules. Expecting a reroll. Seems to break win+VS test(8). cf. https://github.com/git/git/actions/runs/3293333066 whose only difference from https://github.com/git/git/actions/runs/3293553109 is the inclusion of this topic. source: <pull.1359.v3.git.git.1665973401.gitgitgadget@gmail.com> |
|
There was a status update in the "Stalled" section about the branch "git archive" has been taught "--recurse-submodules" option to create a tarball that includes contents from submodules. Expecting a reroll. Seems to break win+VS test(8). cf. https://github.com/git/git/actions/runs/3293333066 whose only difference from https://github.com/git/git/actions/runs/3293553109 is the inclusion of this topic. source: <pull.1359.v3.git.git.1665973401.gitgitgadget@gmail.com> |
|
There was a status update in the "Stalled" section about the branch "git archive" has been taught "--recurse-submodules" option to create a tarball that includes contents from submodules. Expecting a reroll. Seems to break win+VS test(8). cf. https://github.com/git/git/actions/runs/3293333066 whose only difference from https://github.com/git/git/actions/runs/3293553109 is the inclusion of this topic. source: <pull.1359.v3.git.git.1665973401.gitgitgadget@gmail.com> |
|
There was a status update in the "Discarded" section about the branch "git archive" has been taught "--recurse-submodules" option to create a tarball that includes contents from submodules. Expecting a reroll. Seems to break win+VS test(8). cf. https://github.com/git/git/actions/runs/3293333066 whose only difference from https://github.com/git/git/actions/runs/3293553109 is the inclusion of this topic. source: <pull.1359.v3.git.git.1665973401.gitgitgadget@gmail.com> |
|
There was a status update in the "Discarded" section about the branch "git archive" has been taught "--recurse-submodules" option to create a tarball that includes contents from submodules. Expecting a reroll. Seems to break win+VS test(8). cf. https://github.com/git/git/actions/runs/3293333066 whose only difference from https://github.com/git/git/actions/runs/3293553109 is the inclusion of this topic. source: <pull.1359.v3.git.git.1665973401.gitgitgadget@gmail.com> |
|
There was a status update in the "Discarded" section about the branch "git archive" has been taught "--recurse-submodules" option to create a tarball that includes contents from submodules. Expecting a reroll. Seems to break win+VS test(8). cf. https://github.com/git/git/actions/runs/3293333066 whose only difference from https://github.com/git/git/actions/runs/3293553109 is the inclusion of this topic. source: <pull.1359.v3.git.git.1665973401.gitgitgadget@gmail.com> |
This fixes #377 and #445. See also discussion in #332. I would have rather not put 18k lines of code back into this repo but the lack of progress in git/git#1359 and the upcoming 2.0 release made me think this is the best option forward.
|
Me being a humble git user who had high hopes this would make his life easier, may I ask: was this abandoned just because there was no-one who would be willing and able to look into and fix a segfault on the win+VS build? If so, that would be really sad. |
This makes it possible to include submodule contents in an archive command.
The inspiration for this change comes from this Github thread, dear-github/dear-github#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_repositoryreferences to look up commit or tree or oid information.I ended up deciding that
read_tree_fn_twould probably be best off having a concrete repo referencesince 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 :)
cc: René Scharfe l.s.r@web.de
cc: Phillip Wood phillip.wood123@gmail.com
cc: Glen Choo chooglen@google.com
cc: Heather Lapointe hthralpha@gmail.com