Add the usage of the submodules.recurse parameter on clone #695
Conversation
Welcome to GitGitGadgetHi @Masmiseim36, 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. 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 is an issue in commit 4f574cf4b3f8ade0aa928613034157fca90b9696: |
|
There is an issue in commit 8aabd4c9ecd36f3919c3263dfce092d5466672bc: |
|
There is an issue in commit bb5b2efc847fd402dfc2399686f8bcb2f5a7d662: |
|
/allow |
|
User Masmiseim36 is now allowed to use GitGitGadget. WARNING: Masmiseim36 has no public email address set on GitHub |
There was a problem hiding this comment.
These commits should all be squashed into a single one. The commit message should be improved, in accordance with https://git-scm.com/docs/SubmittingPatches#describe-changes, and the sign-off is required (look at the existing commit history to see the commit message style preferred in the Git project).
t/t7407-submodule-foreach.sh
Outdated
| git rev-parse --resolve-git-dir nested1/nested2/.git && | ||
| git rev-parse --resolve-git-dir nested1/nested2/nested3/.git && | ||
| git rev-parse --resolve-git-dir nested1/nested2/nested3/submodule/.git | ||
| ) |
There was a problem hiding this comment.
It would be better to do this in a single invocation, and then to compare it to the expected values. Besides, I don't think that it needs to be exhaustive. So I would do this instead:
git -C clone5 rev-parse \
--resolve-git-dir .git \
--resolve-git-dir nested1/nested2/nested3/submodule/.git >actual &&
cat >expect <<-EOF &&
.git
$(pwd)/nested1/nested2/nested3/submodule/.git
EOF
test_cmp expect actualThere was a problem hiding this comment.
Sorry, don’t get it. I just copied the test for ‘git clone –recursive’. This command should do the identical job like git clone -c submodule.recurse=true’. Why do different tests?
There was a problem hiding this comment.
Because you can improve the style? You also do not need to be so extensive in an essentially slightly repeated test. Testing whether one nested submodule was cloned should be plenty enough.
There was a problem hiding this comment.
OK, I will take a closer look. I like to understand the test regression code a bit better first
builtin/clone.c
Outdated
| { | ||
| if (!strcmp(var, "submodule.recurse")) { | ||
| option_recurse_submodules.nr = git_config_bool(var, value) ? | ||
| 1 : option_recurse_submodules.nr; |
There was a problem hiding this comment.
This is most likely incorrect, as option_recurse_submodules is a string_list, and increasing the nr without even allocating a corresponding item, let alone initializing it, is most likely causing a crash.
There was a problem hiding this comment.
True, the Size is used as an indication of submodules active or not.
There was a problem hiding this comment.
But what we really want to do is to stay as close to what passing the --recurse-submodules option would have accomplished, which is to add ".", not "true"...
builtin/clone.c
Outdated
| { | ||
| if (!strcmp(var, "submodule.recurse")) { | ||
| option_recurse_submodules.nr = git_config_bool(var, value) ? | ||
| RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF; |
There was a problem hiding this comment.
This does way more than just rename the function. And it still modifies nr without allocating/initializing the item. The correct thing to do here is more likely to be this:
string_list_append(&option_recurse_submodules, ".");(compare to what recurse_submodules_cb() does when no arg is provided)
Thank you for taking on this task! The changes already look pretty good, but they should be squashed into a single one (use the Do make sure to describe the motivation for this change well in the commit message ;-) |
Masmiseim36
force-pushed
the
dev/CloneWithSubmodule
branch
from
bb5b2ef
to
332fd22
Compare
|
There are issues in commit 332fd226d543d3cdfb8924c271a26994ca3cbc89: |
|
Please ignore the failing Visual Studio build for now. Your next push should "fix" this by virtue of It would be good to fix the commit message, though, as the GitGitGadget bot pointed out. Please wrap the paragraphs at <=76 columns/line and leave empty lines between them. Also, instead of "clone: respect submodules.recurse config option for automatically clone submodules" you could say "clone: respect the Please note that I suggested the |
Masmiseim36
force-pushed
the
dev/CloneWithSubmodule
branch
from
332fd22
to
6ca650a
Compare
|
There are issues in commit 6ca650a55da1c509a186eec0eb26921d6ef0c7bc: |
I see that you broke the first line of the commit message (you really have to look at the commit locally, it is not obvious in GitHub's UI): Maybe you can think of another way to phrase it so that it gets shorter? Also, I really think that you are asking to be questioned why the documentation explicitly excludes Finally, you will still need to sign-off on the patch (easy: call |
|
@Masmiseim36 ping? |
|
Masmiseim36
force-pushed
the
dev/CloneWithSubmodule
branch
from
6ca650a
to
64a7040
Compare
|
There is an issue in commit 64a70401aaed298029a2f69319d83fbe6399f034: |
|
I restarted the failed test (it failed for unrelated reasons in a flaky test, the re-run should "fix" it). You will still be required to add your Sign-off, GitGitGadget won't let you send the patch otherwise. |
Masmiseim36
force-pushed
the
dev/CloneWithSubmodule
branch
from
64a7040
to
fba8f9c
Compare
|
Hmm. I think the Signed-off-by: line is still incomplete, as it only says "Markus", not "Markus Klein". The reviewers on the Git mailing list won't like that, https://git-scm.com/docs/SubmittingPatches#sign-off specifically states:
|
Masmiseim36
force-pushed
the
dev/CloneWithSubmodule
branch
from
fba8f9c
to
7fa8d19
Compare
|
Hello dscho, I’ve extended the “Signed-off-by:” with my full name. Thanks for the guidance and the feedback. I really appreciate that. |
|
Looks good to me. I think it's |
|
Error: Could not determine full name of Masmiseim36 |
|
/submit |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
Masmiseim36
force-pushed
the
dev/CloneWithSubmodule
branch
from
7fa8d19
to
a59ca4e
Compare
|
Hello Johannes,
Von: gitgitgadget-git[bot] <notifications@github.com>
Gesendet: Samstag, 1. Februar 2020 22:24
An: git/git <git@noreply.github.com>
Cc: Markus Klein <masmiseim@gmx.de>; Mention <mention@noreply.github.com>
Betreff: Re: [git/git] Add the usage ot he submodules.recurse parameter on clone (#695)
On the Git mailing list <https://public-inbox.org/git/nycvar.QRO.7.76.6.2002012218410.46@tvgsbejvaqbjf.bet> , Johannes Schindelin wrote (reply to this <https://github.com/gitgitgadget/gitgitgadget/wiki/ReplyToThis> ):
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
--8323328-864764858-1580591979=:46
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Hi Markus,
On Fri, 31 Jan 2020, Markus Klein via GitGitGadget wrote:
From: Markus ***@***.*** ***@***.***> >
This line should probably match…
Simplify cloning repositories with submodules when the option
submodules.recurse ot he by the user. This makes it transparent ot he
user if submodules are used. The user doesn=E2=80=99t have to know if he=
ot he add
an extra parameter to get the full project including the used submodules=
..
This makes clone behave identical to other commands like fetch, pull,
checkout, … which include the submodules automatically if this option =
is
set.
It is implemented analog ot he pull command by using an own config
function instead of using just the default config. In contrast ot he pu=
ll
command, the submodule.recurse state is ot hes an array of strings as =
it
can take an optionally pathspec argument which describes which submodule=
s
should be recursively initialized and cloned. To recursively initialize =
and
clone all submodules a pathspec of „.“ ot he be used.
The regression test is simplified compared ot he test for „git clone
--recursive“ as the general functionality is already checked there.
Signed-off-by: Markus Klein ***@***.*** ***@***.***> >
… this line. I.e. you will want to run `git config –global user.name
„Markus Klein“` and then `git commit –amend –reset-author`.
Ciao,
Johannes
Thanks for the hint. I’ve fixed this
Best Regards
Markus
---
Add the usage of the submodules.recurse parameter on clone
I try to finish the pullrequest #573 from Maddimax. This adds the us=
age
of the submodules.recurse parameter on clone
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-69=
5%2FMasmiseim36%2Fdev%2FCloneWithSubmodule-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-695/M=
asmiseim36/dev/CloneWithSubmodule-v1
Pull-Request: #695
builtin/clone.c | 16 +++++++++++++++-
t/t7407-submodule-foreach.sh | 11 +++++++++++
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 0fc89ae2b9..21b9d927a2 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -26,6 +26,8 @@
#include "dir-iterator.h"
#include "iterator.h"
#include "sigchain.h"
+#include "submodule-config.h"
+#include "submodule.h"
#include "branch.h"
#include "remote.h"
#include "run-command.h"
@@ -929,6 +931,18 @@ static int path_exists(const char *path)
return !stat(path, &sb);
}
+/**
+ * Read config variables.
+ */
+static int git_clone_config(const char *var, const char *value, void *c=
b)
+{
+ if (!strcmp(var, "submodule.recurse") && git_config_bool(var, value)) =
{
+ string_list_append(&option_recurse_submodules, "true");
+ return 0;
+ }
+ return git_default_config(var, value, cb);
+}
+
int cmd_clone(int argc, const char **argv, const char *prefix)
{
int is_bundle =3D 0, is_local;
@@ -1103,7 +1117,7 @@ int cmd_clone(int argc, const char **argv, const c=
har *prefix)
write_config(&option_config);
- git_config(git_default_config, NULL);
+ git_config(git_clone_config, NULL);
if (option_bare) {
if (option_mirror)
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6b2aa917e1..44b32f7b27 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -383,6 +383,17 @@ test_expect_success 'use "update --recursive nested=
1" to checkout all submodules
git rev-parse --resolve-git-dir nested1/nested2/nested3/submodule/.gi=
t
)
'
+test_expect_success 'use "git clone" with submodule.recurse=3Dtrue to c=
heckout all submodules' '
+ git clone -c submodule.recurse=3Dtrue super clone7 &&
+ (
+ git -C clone7 rev-parse --resolve-git-dir .git --resolve-git-dir nest=
ed1/nested2/nested3/submodule/.git >actual &&
+ cat >expect <<-EOF &&
+ .git
+ $(pwd)/clone7/.git/modules/nested1/modules/nested2/modules/nested3/mo=
dules/submodule
+ EOF
+ test_cmp expect actual
+ )
+'
test_expect_success 'command passed to foreach retains notion of stdin'=
'
… (
base-commit: d0654dc
--
gitgitgadget
--8323328-864764858-1580591979=:46--
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#695?email_source=notifications&email_token=ALDBVOSIGDINAXRXDYFRL3DRAXR57A5CNFSM4KHJN67KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKRG7IQ#issuecomment-581070754> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALDBVOT7TDLOJ23SYN22OELRAXR57ANCNFSM4KHJN67A> . <https://github.com/notifications/beacon/ALDBVOSZHJTJCFFZBUUGFITRAXR57A5CNFSM4KHJN67KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKRG7IQ.gif>
|
This was the only information in that otherwise garbled reply, right? |
I was able to verify this by looking at this: https://github.com/git/git/commit/a59ca4e27b7879f07d99d56cb972761e59714731.patch
If so, please note that nobody on the Git mailing list saw your reply nor your change. To send another iteration of this patch, please use GitGitGadget again: add a comment consisting of Before that, you will want to edit the initial comment (which will be sent as a sort of cover letter, embedded within the single-patch mail) to say something along these lines:
|
Masmiseim36
force-pushed
the
dev/CloneWithSubmodule
branch
from
a59ca4e
to
c758352
Compare
Sorry for the confusion, Maybe I should try a different E-Mail client. |
|
/submit |
|
For what it's worth: by "initial comment", I meant the first comment on the PR, i.e. the PR description, not the commit message... |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
Masmiseim36
force-pushed
the
dev/CloneWithSubmodule
branch
from
c758352
to
f71ca26
Compare
|
On the Git mailing list, wrote (reply to this): |
|
/submit |
|
@Masmiseim36 I saw that Junio offered a demo patch; did you end up using that? In any case, what is the status of this patch? |
Simplify cloning repositories with submodules when the option submodule.recurse is set by the user. This makes it transparent to the user if submodules are used. The user doesn’t have to know if he has to add an extra parameter to get the full project including the used submodules. This makes clone behave identical to other commands like fetch, pull, checkout, ... which include the submodules automatically if this option is set. It is implemented analog to the pull command by using an own config function instead of using just the default config. In contrast to the pull command, the submodule.recurse state is saved in addition as an array of strings as it can take an optionally pathspec argument which describes which submodules should be recursively initialized and cloned. To recursively initialize and clone all submodules a pathspec of "." has to be used. The regression test is simplified compared to the test for "git clone --recursive" as the general functionality is already checked there. Signed-off-by: Markus Klein <masmiseim@gmx.de>
Masmiseim36
force-pushed
the
dev/CloneWithSubmodule
branch
from
f71ca26
to
e502f37
Compare
|
/submit |
This adds the usage of the submodule.recurse parameter on clone
Changes since v1:
Changes since v2:
Changes since v3: