| Message ID | pull.1359.v3.git.git.1665973401.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | archive: Add --recurse-submodules to git-archive command | expand |
Hi Heather On 17/10/2022 03:23, Heather Lapointe via GitGitGadget wrote: > 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 >
Today I was scheduled to be offline, so I won't dig further on the issues this topic has now, but the new tests this series introduces, namely t1023 and t5005, both relies on being able to clone a nested submodule via file:// transport, which no longer is allowed. The patches need to be updated to adjust to the new world order, of course, but we probably should take it as an example of what the most recent update may be breaking for real world users. Thanks.
Thanks for taking a look. On Tue, Oct 18, 2022 at 2:34 PM Junio C Hamano <gitster@pobox.com> wrote: > > Today I was scheduled to be offline, so I won't dig further on the > issues this topic has now, but the new tests this series introduces, > namely t1023 and t5005, both relies on being able to clone a nested > submodule via file:// transport, which no longer is allowed. I was following the patterns of t/lib-submodule-update.sh. Are there better examples that I can follow? > The patches need to be updated to adjust to the new world order, of > course, but we probably should take it as an example of what the > most recent update may be breaking for real world users. > > Thanks.
Heather Lapointe <alpha@alphaservcomputing.solutions> writes: > Thanks for taking a look. > > On Tue, Oct 18, 2022 at 2:34 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Today I was scheduled to be offline, so I won't dig further on the >> issues this topic has now, but the new tests this series introduces, >> namely t1023 and t5005, both relies on being able to clone a nested >> submodule via file:// transport, which no longer is allowed. > > I was following the patterns of t/lib-submodule-update.sh. Are there > better examples > that I can follow? Mimic what Taylor did to adjust to the new world order that was introduced in the 2.38.1 update. Look at 9c32cfb4 (Sync with v2.38.1, 2022-10-17), which merges 2.38.1 and updates the tests to adjust to the new world order, by comparing the t/ directory of its first parent and the result of the merge. It shows what Taylor did to adjust the tests to adjust. $ git diff 9c32cfb4^ 9c32cfb4 t/ 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.
Junio C Hamano <gitster@pobox.com> writes: > Mimic what Taylor did to adjust to the new world order that was > introduced in the 2.38.1 update. > > Look at 9c32cfb4 (Sync with v2.38.1, 2022-10-17), which merges > 2.38.1 and updates the tests to adjust to the new world order, by > comparing the t/ directory of its first parent and the result of the > merge. It shows what Taylor did to adjust the tests to adjust. > > $ git diff 9c32cfb4^ 9c32cfb4 t/ > > 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(-) diff --git a/t/t1023-tree-read-tree-at.sh b/t/t1023-tree-read-tree-at.sh index 9e5ce3abb4..cfe6c867e3 100755 --- a/t/t1023-tree-read-tree-at.sh +++ b/t/t1023-tree-read-tree-at.sh @@ -32,7 +32,8 @@ test_expect_success 'read_tree basic' ' ' test_expect_success 'read_tree submodules' ' - rm -rf walk_tree_submodules && + git config --global protocol.file.allow always && + rm -rf submodule1 && git init submodule1 && ( cd submodule1 && @@ -42,6 +43,7 @@ test_expect_success 'read_tree submodules' ' git add file1.txt dir1/dirA/file1.txt && git commit -m "initial commit" ) && + rm -rf walk_tree_submodules && git init walk_tree_submodules && ( cd walk_tree_submodules && diff --git a/t/t5005-archive-submodules.sh b/t/t5005-archive-submodules.sh index aad6cfd108..e1413e08a2 100755 --- a/t/t5005-archive-submodules.sh +++ b/t/t5005-archive-submodules.sh @@ -4,7 +4,7 @@ test_description='git archive --recurse-submodules test' . ./test-lib.sh -check_tar() { +check_tar () { tarfile=$1.tar listfile=$1.lst dir=$1 @@ -15,7 +15,7 @@ check_tar() { ' } -check_added() { +check_added () { dir=$1 path_in_fs=$2 path_in_archive=$3 @@ -26,7 +26,7 @@ check_added() { ' } -check_not_added() { +check_not_added () { dir=$1 path_in_archive=$2 @@ -37,6 +37,7 @@ check_not_added() { } test_expect_success 'setup' ' + git config --global protocol.file.allow always && rm -rf repo_with_submodules submodule1 uninited_repo_with_submodules && git init repo_with_submodules && git init submodule1 &&
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.
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.
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 >
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.