threads / rfc / 47993

patchRe: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default

Subject: Re: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default

## tl;dr

29 messages between Mar 6, 2018 and Apr 19, 2018. Diffs are folded; open one to read it.

replies: 28people: 6as markdown or json

Jonathan Nieder· Mar 6, 2018, 20:21 UTC · lore

(cc list snipped) Hi,

Eddy Petrișor wrote:
> Cc: [a lot of people]

Can you say a little about how this cc list was created? E.g. should "git send-email" get a feature to warn about long cc lists?

Show 14 quoted lines
> Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
> ---
>
> There are projects such as llvm/clang which use several repositories, and they
> might be forked for providing support for various features such as adding Redox
> awareness to the toolchain. This typically means the superproject will use
> another branch than master, occasionally even use an old commit from that
> non-master branch.
>
> Combined with the fact that when incorporating such a hierachy of repositories
> usually the user is interested in just the exact commit specified in the
> submodule info, it follows that a desireable usecase is to be also able to
> provide '--depth 1' to avoid waiting for ages for the clone operation to
> finish.

Some previous discussion is at https://public-inbox.org/git/CAGZ79ka6UXKyVLmdLg_M5-sB1x96g8FRzRZy=ENy5aJBQf9_QA@mail.gmail.com/.

In theory this should be straightforward: Git protocol allows fetching an arbitrary commit, so "git submodule update" and similar commands could fetch the submodule commit by SHA-1 instead of by refname. Poof! Problem gone.

In practice, some complications:
 - some servers do not permit fetch-by-sha1.  For example, github does
   not permit it.  This is governed by the
   uploadpack.allowReachableSHA1InWant / uploadpack.allowAnySHA1InWant
   configuration items.
   That should be surmountable by making the behavior conditional, but
   it's a complication.
 - When the user passes --depth=<num>, do they mean that to apply to
   the superproject, to the submodules, or both?  Documentation should
   make the behavior clear.
   Fortunately I believe this complication has been takencare of using
   the --shallow-submodules option.
> Git submodule seems to be very stubborn and cloning master, although the
> wrapper script and the gitmodules-helper could work together to clone directly
> the branch specified in the .gitmodules file, if specified.

This could make sense. For the same reason as --depth in the superproject gives ambiguous signals about what should happen in submodules, --single-branch in the superproject gives ambiguous signals about what branch to fetch in submodules.

> Another wrinkle is that when the commit is not the tip of the branch, the depth
> parameter should somehow be stored in the .gitmodules info, but any change in
> the submodule will break the supermodule submodule depth info sooner or later,
> which is definitly frigile.

Hm, this seems to go in another direction. I don't think we should store the depth parameter in the .gitmodules file, since different users are likely to have different preferences about what to make shallow. If we make --depth easy enough to use at the superproject level then the user can specify what they want there.

Show 16 quoted lines
> I tried digging into this section of the code and debugging with bashdb to see
> where --depth might fit, but I got stuck on the shell-to-helper interaction and
> the details of the submodule implementation, so I want to lay out this first
> patch as starting point for the discussion in the hope somebody else picks it
> up or can provide some inputs. I have the feeling there are multiple code paths
> that are being ran, depending on the moment (initial clone, submodule
> recursive, post-clone update etc.) and I have a gut feeling there shouldn't be
> any code duplication just because the operation is different.
>
> This first patch is only trying to use a non-master branch, I have some changes
> for the --depth part, but I stopped working on it due to the "default depth"
> issue above.
>
> Does any of this sound reasonable?
> Is this patch idea usable or did I managed to touch the part of the code that
> should not be touched?

I agree with the goal. As mentioned above, I'm not confident about the particular mechanism --- e.g. something using fetch-by-sha1 seems likely to be more intuitive.

Today, the 'branch' setting in .gitmodules is only for "git submodule update --remote". This patch would be a significant expansion in scope for it. Hopefully others on the list can talk more about how that fits into various workflows and whether it would work out well.

Thanks and hope that helps, Jonathan

Show 20 quoted lines
>  git-submodule.sh | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 2491496..370f19e 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -589,8 +589,11 @@ cmd_update()
>  			branch=$(git submodule--helper remote-branch "$sm_path")
>  			if test -z "$nofetch"
>  			then
> +				# non-default branch
> +				rbranch=$(git config -f .gitmodules submodule.$sm_path.branch)
> +				br_refspec=${rbanch:+"refs/heads/$rbranch:refs/heads/$rbranch"}
>  				# Fetch remote before determining tracking $sha1
> -				fetch_in_submodule "$sm_path" $depth ||
> +				fetch_in_submodule "$sm_path" $depth $br_refspec ||
>  				die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
>  			fi
>  			remote_name=$(sanitize_submodule_env; cd "$sm_path" && get_default_remote)
Eddy Petrișor· Mar 8, 2018, 19:29 UTC · re: Jonathan Nieder · lore
2018-03-06 22:21 GMT+02:00 Jonathan Nieder <jrnieder@gmail.com>:
Show 10 quoted lines
>
> (cc list snipped)
> Hi,
>
> Eddy Petrișor wrote:
>
> > Cc: [a lot of people]
>
> Can you say a little about how this cc list was created?  E.g. should
> "git send-email" get a feature to warn about long cc lists?
I did it as advised by the  documentation, git blame:
https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L264

I was suprised that there is no specialized script that does this, as it is for the kernel or u-boot, so I ran first

    git log --pretty=format:'%an <%ae>' git-submodule.sh | sort -u >
mail-list-submodule

then configure git to use that custom output and ignore the patch since it was trying to convert every line of it into a email address:

    git config sendemail.ccCmd 'cat mail-list-submodule # '
Then "git send-email 0001..." did the rest.
Show 30 quoted lines
>
> > Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
> > ---
> >
> > There are projects such as llvm/clang which use several repositories, and they
> > might be forked for providing support for various features such as adding Redox
> > awareness to the toolchain. This typically means the superproject will use
> > another branch than master, occasionally even use an old commit from that
> > non-master branch.
> >
> > Combined with the fact that when incorporating such a hierachy of repositories
> > usually the user is interested in just the exact commit specified in the
> > submodule info, it follows that a desireable usecase is to be also able to
> > provide '--depth 1' to avoid waiting for ages for the clone operation to
> > finish.
>
> Some previous discussion is at
> https://public-inbox.org/git/CAGZ79ka6UXKyVLmdLg_M5-sB1x96g8FRzRZy=ENy5aJBQf9_QA@mail.gmail.com/.
>
> In theory this should be straightforward: Git protocol allows fetching
> an arbitrary commit, so "git submodule update" and similar commands
> could fetch the submodule commit by SHA-1 instead of by refname.  Poof!
> Problem gone.
>
> In practice, some complications:
>
>  - some servers do not permit fetch-by-sha1.  For example, github does
>    not permit it.  This is governed by the
>    uploadpack.allowReachableSHA1InWant / uploadpack.allowAnySHA1InWant
>    configuration items.

That is the problem I have faced since I tried to clone this repo which has at lest 2 levels of submodules: https://github.com/redox-os/redox

The problematic modules are:
rust @ https://github.com/redox-os/rust/tree/81c2bf4e51647295d3d92952dbb0464b460df0c3
- first level
and

rust/src/llvm @ https://github.com/rust-lang/llvm/tree/ba2edd794c7def715007931fcd1b4ce62aa711c8

>
>    That should be surmountable by making the behavior conditional, but
>    it's a complication.

Which forced me to try to fetch a branch on which that commit exists, but also consider providing --depth for the submodule checkout, effectively minimizing the amount of brought in history.

Show 5 quoted lines
>
>
>  - When the user passes --depth=<num>, do they mean that to apply to
>    the superproject, to the submodules, or both?  Documentation should
>    make the behavior clear.

The supermodule clone already exists and that works OK; I was after providing something like 'git submodule update --depth 20 --recursive' or evn providing that 'depth' info via the .gitmodules parameters since 'shallow' is already used somehow, yet that is a bool value, not an integer, like depth expects:

eddy@feodora:~/usr/src/redox/rust-eddy$ git config -f .gitmodules --list | egrep '(depth|shallow)' submodule.src/llvm.shallow=true submodule.src/rt/hoedown.shallow=true submodule.src/jemalloc.shallow=true submodule.src/liblibc.shallow=true submodule.src/doc/nomicon.shallow=true submodule.src/tools/cargo.shallow=true submodule.src/doc/reference.shallow=true submodule.src/doc/book.shallow=true submodule.src/tools/rls.shallow=true submodule.src/libcompiler_builtins.shallow=true submodule.src/tools/clippy.shallow=true submodule.src/tools/rustfmt.shallow=true submodule.src/tools/miri.shallow=true submodule.src/dlmalloc.shallow=true submodule.src/binaryen.shallow=true submodule.src/doc/rust-by-example.shallow=true submodule.src/llvm-emscripten.shallow=true submodule.src/tools/rust-installer.shallow=true

Show 8 quoted lines
> > Git submodule seems to be very stubborn and cloning master, although the
> > wrapper script and the gitmodules-helper could work together to clone directly
> > the branch specified in the .gitmodules file, if specified.
>
> This could make sense.  For the same reason as --depth in the
> superproject gives ambiguous signals about what should happen in
> submodules, --single-branch in the superproject gives ambiguous
> signals about what branch to fetch in submodules.

I never thought of providing the branch in the command line, since that's not versionable info, but to provide git-submodule a hint in the .gitsubmodule config about on which branch the hash in question should be found, like this:

eddy@feodora:~/usr/src/redox/rust-eddy$ git config -f .gitmodules --list | egrep branch submodule.src/llvm.branch=rust-llvm-release-4-0-1 submodule.src/rt/hoedown.branch=rust-2015-09-21-do-not-delete

Show 11 quoted lines
>
> > Another wrinkle is that when the commit is not the tip of the branch, the depth
> > parameter should somehow be stored in the .gitmodules info, but any change in
> > the submodule will break the supermodule submodule depth info sooner or later,
> > which is definitly frigile.
>
> Hm, this seems to go in another direction.  I don't think we should
> store the depth parameter in the .gitmodules file, since different
> users are likely to have different preferences about what to make
> shallow.  If we make --depth easy enough to use at the superproject
> level then the user can specify what they want there.

Yes, but a) if the commit is not on the tip of that branch, b) the server does not support sha1 fetching and c) it is not expected to typically work in the submodules from the superproject checkout (e.g submodule is under another team's control)

the typical usecase could be that the code is question is read-only and it should be OK to typically fetch a shallow clone of the submodule (by default). The only compromise at that point is to either choose a afe margin for the defalt depth in case the module branch is being modified.

I solved a similar problem at work where having 2 shallow branches which had to be merged; since when trying to merge two branches without apprent common history git will do a merge commit, when a simple ff woudl wor, I wanted to find the merge-base. SInce the brances were shallow and they had to have a merge-base, the solution was to was to simply increase the --depth parameter in predifined steps (of 10 commits) and fetch both branches, check if the 'git merge-base' command manged to find a commit; if not the depthc parameter woudl be increased and the whole process would be started until that merge-base was found.

I wonder if the same couldn't be done for the submodules that have submodule.<dir>.shallow setting, making the depth parameter hardcoding unnecessary, and guranteeing the submodule could be cloned at any given time, as long as the branch specified by submodule.<dir>.branch is not removed from the submodule repo.

Show 26 quoted lines
>
> > I tried digging into this section of the code and debugging with bashdb to see
> > where --depth might fit, but I got stuck on the shell-to-helper interaction and
> > the details of the submodule implementation, so I want to lay out this first
> > patch as starting point for the discussion in the hope somebody else picks it
> > up or can provide some inputs. I have the feeling there are multiple code paths
> > that are being ran, depending on the moment (initial clone, submodule
> > recursive, post-clone update etc.) and I have a gut feeling there shouldn't be
> > any code duplication just because the operation is different.
> >
> > This first patch is only trying to use a non-master branch, I have some changes
> > for the --depth part, but I stopped working on it due to the "default depth"
> > issue above.
> >
> > Does any of this sound reasonable?
> > Is this patch idea usable or did I managed to touch the part of the code that
> > should not be touched?
>
> I agree with the goal.  As mentioned above, I'm not confident about
> the particular mechanism --- e.g. something using fetch-by-sha1 seems
> likely to be more intuitive.
>
> Today, the 'branch' setting in .gitmodules is only for "git submodule
> update --remote".  This patch would be a significant expansion in
> scope for it.  Hopefully others on the list can talk more about how
> that fits into various workflows and whether it would work out well.
Thanks for the input, it was helpful anyway.

Now that I think of it, I think the "fetch more and more from the branch until we find the sha1" could work; in the worst case you would end up going to the beginning of the repo in case the submodule history was rewritten or if the branch specified in the supermodule submodule.<path>.branch was removed.

I am aware this is not ellegant at all (what is the default "step"?), yet the submodule part of the code already struggles in that area because it clearly looks like an afterthought, anyway.

>
> Thanks and hope that helps,
> Jonathan
-- 
Eddy Petrișor
Eddy Petrișor· Mar 8, 2018, 19:41 UTC · re: Eddy Petrișor · lore
2018-03-08 21:29 GMT+02:00 Eddy Petrișor <eddy.petrisor@gmail.com>:
Show 177 quoted lines
> 2018-03-06 22:21 GMT+02:00 Jonathan Nieder <jrnieder@gmail.com>:
>>
>> (cc list snipped)
>> Hi,
>>
>> Eddy Petrișor wrote:
>>
>> > Cc: [a lot of people]
>>
>> Can you say a little about how this cc list was created?  E.g. should
>> "git send-email" get a feature to warn about long cc lists?
>
>
> I did it as advised by the  documentation, git blame:
>
> https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L264
>
> I was suprised that there is no specialized script that does this, as
> it is for the kernel or u-boot, so I ran first
>
>     git log --pretty=format:'%an <%ae>' git-submodule.sh | sort -u >
> mail-list-submodule
>
> then configure git to use that custom output and ignore the patch
> since it was trying to convert every line of it into a email address:
>
>     git config sendemail.ccCmd 'cat mail-list-submodule # '
>
> Then "git send-email 0001..." did the rest.
>
>
>>
>> > Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
>> > ---
>> >
>> > There are projects such as llvm/clang which use several repositories, and they
>> > might be forked for providing support for various features such as adding Redox
>> > awareness to the toolchain. This typically means the superproject will use
>> > another branch than master, occasionally even use an old commit from that
>> > non-master branch.
>> >
>> > Combined with the fact that when incorporating such a hierachy of repositories
>> > usually the user is interested in just the exact commit specified in the
>> > submodule info, it follows that a desireable usecase is to be also able to
>> > provide '--depth 1' to avoid waiting for ages for the clone operation to
>> > finish.
>>
>> Some previous discussion is at
>> https://public-inbox.org/git/CAGZ79ka6UXKyVLmdLg_M5-sB1x96g8FRzRZy=ENy5aJBQf9_QA@mail.gmail.com/.
>>
>> In theory this should be straightforward: Git protocol allows fetching
>> an arbitrary commit, so "git submodule update" and similar commands
>> could fetch the submodule commit by SHA-1 instead of by refname.  Poof!
>> Problem gone.
>>
>> In practice, some complications:
>>
>>  - some servers do not permit fetch-by-sha1.  For example, github does
>>    not permit it.  This is governed by the
>>    uploadpack.allowReachableSHA1InWant / uploadpack.allowAnySHA1InWant
>>    configuration items.
>
>
> That is the problem I have faced since I tried to clone this repo
> which has at lest 2 levels of submodules:
> https://github.com/redox-os/redox
>
> The problematic modules are:
> rust @ https://github.com/redox-os/rust/tree/81c2bf4e51647295d3d92952dbb0464b460df0c3
> - first level
>
> and
>
> rust/src/llvm @
> https://github.com/rust-lang/llvm/tree/ba2edd794c7def715007931fcd1b4ce62aa711c8
>
>
>>
>>    That should be surmountable by making the behavior conditional, but
>>    it's a complication.
>
>
> Which forced me to try to fetch a branch on which that commit exists,
> but also consider providing --depth for the submodule checkout,
> effectively minimizing the amount of brought in history.
>
>>
>>
>>  - When the user passes --depth=<num>, do they mean that to apply to
>>    the superproject, to the submodules, or both?  Documentation should
>>    make the behavior clear.
>
>
> The supermodule clone already exists and that works OK; I was after
> providing something like 'git submodule update --depth 20 --recursive'
> or evn providing that 'depth' info via the .gitmodules parameters
> since 'shallow' is already used somehow, yet that is a bool value, not
> an integer, like depth expects:
>
>
> eddy@feodora:~/usr/src/redox/rust-eddy$ git config -f .gitmodules
> --list | egrep '(depth|shallow)'
> submodule.src/llvm.shallow=true
> submodule.src/rt/hoedown.shallow=true
> submodule.src/jemalloc.shallow=true
> submodule.src/liblibc.shallow=true
> submodule.src/doc/nomicon.shallow=true
> submodule.src/tools/cargo.shallow=true
> submodule.src/doc/reference.shallow=true
> submodule.src/doc/book.shallow=true
> submodule.src/tools/rls.shallow=true
> submodule.src/libcompiler_builtins.shallow=true
> submodule.src/tools/clippy.shallow=true
> submodule.src/tools/rustfmt.shallow=true
> submodule.src/tools/miri.shallow=true
> submodule.src/dlmalloc.shallow=true
> submodule.src/binaryen.shallow=true
> submodule.src/doc/rust-by-example.shallow=true
> submodule.src/llvm-emscripten.shallow=true
> submodule.src/tools/rust-installer.shallow=true
>
>
>> > Git submodule seems to be very stubborn and cloning master, although the
>> > wrapper script and the gitmodules-helper could work together to clone directly
>> > the branch specified in the .gitmodules file, if specified.
>>
>> This could make sense.  For the same reason as --depth in the
>> superproject gives ambiguous signals about what should happen in
>> submodules, --single-branch in the superproject gives ambiguous
>> signals about what branch to fetch in submodules.
>
>
> I never thought of providing the branch in the command line, since
> that's not versionable info, but to provide git-submodule a hint in
> the .gitsubmodule config about on which branch the hash in question
> should be found, like this:
>
> eddy@feodora:~/usr/src/redox/rust-eddy$ git config -f .gitmodules
> --list | egrep branch
> submodule.src/llvm.branch=rust-llvm-release-4-0-1
> submodule.src/rt/hoedown.branch=rust-2015-09-21-do-not-delete
>
>>
>> > Another wrinkle is that when the commit is not the tip of the branch, the depth
>> > parameter should somehow be stored in the .gitmodules info, but any change in
>> > the submodule will break the supermodule submodule depth info sooner or later,
>> > which is definitly frigile.
>>
>> Hm, this seems to go in another direction.  I don't think we should
>> store the depth parameter in the .gitmodules file, since different
>> users are likely to have different preferences about what to make
>> shallow.  If we make --depth easy enough to use at the superproject
>> level then the user can specify what they want there.
>
>
> Yes, but
> a) if the commit is not on the tip of that branch,
> b) the server does not support sha1 fetching and
> c) it is not expected to typically work in the submodules from the
> superproject checkout (e.g submodule is under another team's control)
>
> the typical usecase could be that the code is question is read-only
> and it should be OK to typically fetch a shallow clone of the
> submodule (by default). The only compromise at that point is to either
> choose a afe margin for the defalt depth in case the module branch is
> being modified.
>
> I solved a similar problem at work where having 2 shallow branches
> which had to be merged; since when trying to merge two branches
> without apprent common history git will do a merge commit, when a
> simple ff woudl wor, I wanted to find the merge-base.
> SInce the brances were shallow and they had to have a merge-base, the
> solution was to was to simply increase the --depth parameter in
> predifined steps (of 10 commits) and fetch both branches, check if the
> 'git merge-base'  command manged to find a commit; if not the depthc
> parameter woudl be increased and the whole process would be started
> until that merge-base was found.

Another optimization could be for submodule.<path>.shallow to imply --single-branch, too.

Show 41 quoted lines
> I wonder if the same couldn't be done for the submodules that have
> submodule.<dir>.shallow setting, making the depth parameter hardcoding
> unnecessary, and guranteeing the submodule could be cloned at any
> given time, as long as the branch specified by submodule.<dir>.branch
> is not removed from the submodule repo.
>
>>
>> > I tried digging into this section of the code and debugging with bashdb to see
>> > where --depth might fit, but I got stuck on the shell-to-helper interaction and
>> > the details of the submodule implementation, so I want to lay out this first
>> > patch as starting point for the discussion in the hope somebody else picks it
>> > up or can provide some inputs. I have the feeling there are multiple code paths
>> > that are being ran, depending on the moment (initial clone, submodule
>> > recursive, post-clone update etc.) and I have a gut feeling there shouldn't be
>> > any code duplication just because the operation is different.
>> >
>> > This first patch is only trying to use a non-master branch, I have some changes
>> > for the --depth part, but I stopped working on it due to the "default depth"
>> > issue above.
>> >
>> > Does any of this sound reasonable?
>> > Is this patch idea usable or did I managed to touch the part of the code that
>> > should not be touched?
>>
>> I agree with the goal.  As mentioned above, I'm not confident about
>> the particular mechanism --- e.g. something using fetch-by-sha1 seems
>> likely to be more intuitive.
>>
>> Today, the 'branch' setting in .gitmodules is only for "git submodule
>> update --remote".  This patch would be a significant expansion in
>> scope for it.  Hopefully others on the list can talk more about how
>> that fits into various workflows and whether it would work out well.
>
>
> Thanks for the input, it was helpful anyway.
>
> Now that I think of it, I think the "fetch more and more from the
> branch until we find the sha1" could work; in the worst case you would
> end up going to the beginning of the repo in case the submodule
> history was rewritten or if the branch specified in the supermodule
> submodule.<path>.branch was removed.

Actually, the branch removal in the submodule would lead to a failure to fetch from the get go, so the worst case is if history is rewritten because the sha1 would be missing fromt he branch. Antoher non-fatal perfomance issue would be if the desired commit is very close to the initial commit of the repo.

> I am aware this is not ellegant at all (what is the default "step"?),
> yet the submodule part of the code already struggles in that area
> because it clearly looks like an afterthought, anyway.
-- 
Eddy Petrișor
Thomas Gummerer· Mar 16, 2018, 21:33 UTC · re: Eddy Petrișor · lore
On 03/08, Eddy Petrișor wrote:
Show 29 quoted lines
> 2018-03-06 22:21 GMT+02:00 Jonathan Nieder <jrnieder@gmail.com>:
> >
> > (cc list snipped)
> > Hi,
> >
> > Eddy Petrișor wrote:
> >
> > > Cc: [a lot of people]
> >
> > Can you say a little about how this cc list was created?  E.g. should
> > "git send-email" get a feature to warn about long cc lists?
> 
> 
> I did it as advised by the  documentation, git blame:
> 
> https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L264
> 
> I was suprised that there is no specialized script that does this, as
> it is for the kernel or u-boot, so I ran first
> 
>     git log --pretty=format:'%an <%ae>' git-submodule.sh | sort -u >
> mail-list-submodule
> 
> then configure git to use that custom output and ignore the patch
> since it was trying to convert every line of it into a email address:
> 
>     git config sendemail.ccCmd 'cat mail-list-submodule # '
> 
> Then "git send-email 0001..." did the rest.

There's a 'git contacts' command which we've been carrying in "contrib/" for a while. Maybe we could advertise that in "Documentation/SubmittingPatches" instead of 'git blame' and 'git shortlog'?

Maybe something like the patch below?
--- >8 ---
Subject: [PATCH] SubmittingPatches: mention the git contacts command

Instead of just mentioning 'git blame' and 'git shortlog', which make it harder than necessary for new contributors to pick out the appropriate list of people to cc on their patch series, mention the 'git contacts' utility, which should make it much easier to get a reasonable list of contacts for a change.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 Documentation/SubmittingPatches | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to Documentation/SubmittingPatches +2 −2
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index a1d0feca36..945f8edb46 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -260,8 +260,8 @@ that starts with `-----BEGIN PGP SIGNED MESSAGE-----`.  That is
 not a text/plain, it's something else.
 
 Send your patch with "To:" set to the mailing list, with "cc:" listing
-people who are involved in the area you are touching (the output from
-`git blame $path` and `git shortlog --no-merges $path` would help to
+people who are involved in the area you are touching (the `git
+contacts` command in `contrib/contacts/` can help to
 identify them), to solicit comments and reviews.
 
 :1: footnote:[The current maintainer: gitster@pobox.com]
-- 
2.16.2.804.g6dcf76e11
Eric Sunshine· Mar 16, 2018, 21:44 UTC · re: Thomas Gummerer · lore
On Fri, Mar 16, 2018 at 5:33 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
Show 18 quoted lines
> Subject: [PATCH] SubmittingPatches: mention the git contacts command
>
> Instead of just mentioning 'git blame' and 'git shortlog', which make
> it harder than necessary for new contributors to pick out the
> appropriate list of people to cc on their patch series, mention the
> 'git contacts' utility, which should make it much easier to get a
> reasonable list of contacts for a change.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> @@ -260,8 +260,8 @@ that starts with `-----BEGIN PGP SIGNED MESSAGE-----`.  That is
>  Send your patch with "To:" set to the mailing list, with "cc:" listing
> -people who are involved in the area you are touching (the output from
> -`git blame $path` and `git shortlog --no-merges $path` would help to
> +people who are involved in the area you are touching (the `git
> +contacts` command in `contrib/contacts/` can help to
>  identify them), to solicit comments and reviews.

It may be a disservice to remove mention of git-blame and git-shortlog since git-contacts may not be suitable for everyone. Instead, perhaps advertise git-contacts as a potentially simpler alternative to the already-mentioned git-blamd & git-shortlog?

Also, would it make sense to mention Felipe's git-related[1] which is the original (and now more configurable) script from which git-contacts functionality was derived?

[1]: https://github.com/felipec/git-related
Thomas Gummerer· Mar 17, 2018, 19:11 UTC · lore
On 03/17, Eddy Petrișor wrote:
Show 18 quoted lines
> vin., 16 mar. 2018, 23:44 Eric Sunshine <sunshine@sunshineco.com> a scris:
> 
> > On Fri, Mar 16, 2018 at 5:33 PM, Thomas Gummerer <t.gummerer@gmail.com>
> > wrote:
> > > a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> > > @@ -260,8 +260,8 @@ that starts with `-----BEGIN PGP SIGNED
> > MESSAGE-----`.  That is
> > >  Send your patch with "To:" set to the mailing list, with "cc:" listing
> > > -people who are involved in the area you are touching (the output from
> > > -`git blame $path` and `git shortlog --no-merges $path` would help to
> > > +people who are involved in the area you are touching (the `git
> > > +contacts` command in `contrib/contacts/` can help to
> > >  identify them), to solicit comments and reviews.
> >
> > It may be a disservice to remove mention of git-blame and git-shortlog
> > since git-contacts may not be suitable for everyone. Instead, perhaps
> > advertise git-contacts as a potentially simpler alternative to the
> > already-mentioned git-blamd & git-shortlog?

Not sure how much of a disservice it would be. I think of SubmittingPatches as mostly a document for new git contributors, for who I think we should make it as easy as possible to start contributing. Interpreting the output of 'git blame' and 'git shortlog' feels like an extra hurdle for new contributors, especially if someone is not familiar with the mailing list workflow. I do remember wondering exactly how I should interpret this when I sent my first patches.

Show 9 quoted lines
> As a "victim" of the current documentation, I would advise to have the
> order reversed. For a new contributor, judging if git blame and shortlog
> are "more suitable" than git contracts  or git related is definitely over
> the reasonable knowledge expectation. Why is that more suitable than this?
> How is suitability determined?
> 
> A new person needs a straight forward way to focus on submitting the patch
> in the right form. With experience adding more people in cc will come
> naturally and those contacts might be aware of the contributor, too.

This is also my experience, as I am getting involved with longer with the project I get more of an intuition who is involved where, and 'blame' and 'shortlog' start helping me confirm that and come up with a reasonable list of contacts (although I'm still not always sure whether I got the correct people or not).

But I don't know if people that are getting involved for longer read this document much anymore. So I feel like having the commands mentioned here comes at the expense of new contributors, so I'm not sure it's worth it.

> > Also, would it make sense to mention Felipe's git-related[1] which is
> > the original (and now more configurable) script from which
> > git-contacts functionality was derived?

The reason I chose 'git contacts' over git-related is mainly that it comes available with git. Mentioning both again makes things harder on new contributors who already have enough to think about when submitting the patch.

I guess in the end it comes down to who we think the target of the document is. To me it was always people new to the project, which is why I think the single command there makes sense.

> > [1]: https://github.com/felipec/git-related
> >
Eric Sunshine· Mar 18, 2018, 01:43 UTC · re: Thomas Gummerer · lore
On Sat, Mar 17, 2018 at 3:11 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
Show 15 quoted lines
> On 03/17, Eddy Petrișor wrote:
>> vin., 16 mar. 2018, 23:44 Eric Sunshine <sunshine@sunshineco.com> a scris:
>> > It may be a disservice to remove mention of git-blame and git-shortlog
>> > since git-contacts may not be suitable for everyone. Instead, perhaps
>> > advertise git-contacts as a potentially simpler alternative to the
>> > already-mentioned git-blamd & git-shortlog?
>
> Not sure how much of a disservice it would be.  I think of
> SubmittingPatches as mostly a document for new git contributors, for
> who I think we should make it as easy as possible to start
> contributing.  Interpreting the output of 'git blame' and 'git
> shortlog' feels like an extra hurdle for new contributors, especially
> if someone is not familiar with the mailing list workflow.  I do
> remember wondering exactly how I should interpret this when I sent my
> first patches.

Okay. Mentioning those commands (in addition to git-contacts) is an opportunity to educate newcomers to Git the tool (not just to git.git the project) about additional ways to engage in project spelunking. By "disservice", I meant that that educational opportunity is lost. Eddy's suggestion of reversing the order, thus mentioning git-contacts first is a good alternative.

However, perhaps this idea of educating newcomers to Git is misplaced in this context; such spelunking advice may be better suited to a general Git tutorial than to SubmittingPatches which is indeed specific to git.git. Given that reasoning, then my "disservice" view may be wrong.

Show 12 quoted lines
>> > Also, would it make sense to mention Felipe's git-related[1] which is
>> > the original (and now more configurable) script from which
>> > git-contacts functionality was derived?
>
> The reason I chose 'git contacts' over git-related is mainly that it
> comes available with git.  Mentioning both again makes things harder
> on new contributors who already have enough to think about when
> submitting the patch.
>
> I guess in the end it comes down to who we think the target of the
> document is.  To me it was always people new to the project, which is
> why I think the single command there makes sense.
Fair enough.
Stefan Beller· Mar 26, 2018, 23:06 UTC · lore
[snipped the cc list as well]

On Tue, Mar 6, 2018 at 12:06 PM Eddy Petrișor <eddy.petrisor@gmail.com> wrote:

> Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
> ---

Did this go anywhere? (I just came back from a longer vacation, sorry for the delay on my site)

> There are projects such as llvm/clang which use several repositories, and
they
> might be forked for providing support for various features such as adding
Redox
> awareness to the toolchain. This typically means the superproject will use
> another branch than master, occasionally even use an old commit from that
> non-master branch.
> Combined with the fact that when incorporating such a hierachy of
repositories
> usually the user is interested in just the exact commit specified in the
> submodule info, it follows that a desireable usecase is to be also able to
> provide '--depth 1' to avoid waiting for ages for the clone operation to
> finish.
Very sensible.
> Git submodule seems to be very stubborn and cloning master, although the
> wrapper script and the gitmodules-helper could work together to clone
directly
> the branch specified in the .gitmodules file, if specified.
Also very sensible.

So far so good, could you move these paragraphs before the triple dashed line and sign off so we record it as the commit message?

> Another wrinkle is that when the commit is not the tip of the branch, the
depth
> parameter should somehow be stored in the .gitmodules info, but any
change in
> the submodule will break the supermodule submodule depth info sooner or
later,
> which is definitly frigile.
... which is why I would not include that.

git-fetch knows about --shallow-since or even better shallow-exclude which could be set to the (depth+1)-th commit (the boundary commit) recorded in the shallow information.

> I tried digging into this section of the code and debugging with bashdb
to see
> where --depth might fit, but I got stuck on the shell-to-helper
interaction and
> the details of the submodule implementation, so I want to lay out this
first
> patch as starting point for the discussion in the hope somebody else
picks it
> up or can provide some inputs. I have the feeling there are multiple code
paths
> that are being ran, depending on the moment (initial clone, submodule
> recursive, post-clone update etc.) and I have a gut feeling there
shouldn't be
> any code duplication just because the operation is different.
> This first patch is only trying to use a non-master branch, I have some
changes
> for the --depth part, but I stopped working on it due to the "default
depth"
> issue above.
> Does any of this sound reasonable?
> Is this patch idea usable or did I managed to touch the part of the code
that
> should not be touched?
This sounds reasonable. Thanks for writing the patch!
Show 6 quoted lines
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 2491496..370f19e 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -589,8 +589,11 @@ cmd_update()
>                          branch=$(git submodule--helper remote-branch
"$sm_path")
>                          if test -z "$nofetch"
>                          then
> +                               # non-default branch
> +                               rbranch=$(git config -f .gitmodules
submodule.$sm_path.branch)
> +
br_refspec=${rbanch:+"refs/heads/$rbranch:refs/heads/$rbranch"}

Wouldn't we want to fetch into a remote tracking branch instead? Instead of computing all this by yourself, these two lines could be

     br_refspec=$(git submodule--helper remote-branch $sm_path)
I would think.
>                                  # Fetch remote before determining
tracking $sha1
> -                               fetch_in_submodule "$sm_path" $depth ||
> +                               fetch_in_submodule "$sm_path" $depth
$br_refspec ||
>                                  die "$(eval_gettext "Unable to fetch in
submodule path '\$sm_path'")"
>                          fi
>                          remote_name=$(sanitize_submodule_env; cd
"$sm_path" && get_default_remote)

It would be awesome if you could write a little test for this feature, too. Look for the tests in regarding --remote in t7406 (in the t/ directory) as a starting point, please.

Thanks! Stefan

Eddy Petrișor· Mar 29, 2018, 20:54 UTC · lore

Fwd: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default

mar., 27 mar. 2018, 02:07 Stefan Beller <sbeller@google.com> a scris:
Show 11 quoted lines
>
> [snipped the cc list as well]
>
> On Tue, Mar 6, 2018 at 12:06 PM Eddy Petrișor <eddy.petrisor@gmail.com>
> wrote:
>
> > Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
> > ---
>
> Did this go anywhere?
> (I just came back from a longer vacation, sorry for the delay on my site)
Not really. I am still unsure how is best to proceed. Details below.
Show 16 quoted lines
> > There are projects such as llvm/clang which use several repositories, and
> they
> > might be forked for providing support for various features such as adding
> Redox
> > awareness to the toolchain. This typically means the superproject will use
> > another branch than master, occasionally even use an old commit from that
> > non-master branch.
>
> > Combined with the fact that when incorporating such a hierachy of
> repositories
> > usually the user is interested in just the exact commit specified in the
> > submodule info, it follows that a desireable usecase is to be also able to
> > provide '--depth 1' to avoid waiting for ages for the clone operation to
> > finish.
>
> Very sensible.

The only change is that I realized that hard coding the depth is not necessary because the client can fetch more and more from the branch until the commit hash is found or the entire history was fetched and it wasn't found.

This is more robust but has a variable performance penalty and is probably slower than single branch fetching from the start.

Show 10 quoted lines
> > Git submodule seems to be very stubborn and cloning master, although the
> > wrapper script and the gitmodules-helper could work together to clone
> directly
> > the branch specified in the .gitmodules file, if specified.
>
> Also very sensible.
>
> So far so good, could you move these paragraphs before the triple dashed
> line
> and sign off so we record it as the commit message?
Sure, as long as the implementation and design makes sense.
Show 13 quoted lines
> > Another wrinkle is that when the commit is not the tip of the branch, the
> depth
> > parameter should somehow be stored in the .gitmodules info, but any
> change in
> > the submodule will break the supermodule submodule depth info sooner or
> later,
> > which is definitly frigile.
>
> ... which is why I would not include that.
>
> git-fetch knows about --shallow-since or even better
> shallow-exclude which could be set to the (depth+1)-th commit
> (the boundary commit) recorded in the shallow information.

I am unsure what that means. Without yet looking in the docs, would this --shallow-since be better than the try-until-found algorithm explained above?

Show 27 quoted lines
> > I tried digging into this section of the code and debugging with bashdb
> to see
> > where --depth might fit, but I got stuck on the shell-to-helper
> interaction and
> > the details of the submodule implementation, so I want to lay out this
> first
> > patch as starting point for the discussion in the hope somebody else
> picks it
> > up or can provide some inputs. I have the feeling there are multiple code
> paths
> > that are being ran, depending on the moment (initial clone, submodule
> > recursive, post-clone update etc.) and I have a gut feeling there
> shouldn't be
> > any code duplication just because the operation is different.
>
> > This first patch is only trying to use a non-master branch, I have some
> changes
> > for the --depth part, but I stopped working on it due to the "default
> depth"
> > issue above.
>
> > Does any of this sound reasonable?
> > Is this patch idea usable or did I managed to touch the part of the code
> that
> > should not be touched?
>
> This sounds reasonable. Thanks for writing the patch!
OK. Now I need to make it good, which is the hard part :)
Show 21 quoted lines
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index 2491496..370f19e 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -589,8 +589,11 @@ cmd_update()
> >                          branch=$(git submodule--helper remote-branch
> "$sm_path")
> >                          if test -z "$nofetch"
> >                          then
> > +                               # non-default branch
> > +                               rbranch=$(git config -f .gitmodules
> submodule.$sm_path.branch)
> > +
> br_refspec=${rbanch:+"refs/heads/$rbranch:refs/heads/$rbranch"}
>
> Wouldn't we want to fetch into a remote tracking branch instead?
> Instead of computing all this by yourself, these two lines could be
>
>      br_refspec=$(git submodule--helper remote-branch $sm_path)
>
> I would think.
I wasn't aware of this, will implement I  the next version and see what happens.
Show 16 quoted lines
>
>
> >                                  # Fetch remote before determining
> tracking $sha1
> > -                               fetch_in_submodule "$sm_path" $depth ||
> > +                               fetch_in_submodule "$sm_path" $depth
> $br_refspec ||
> >                                  die "$(eval_gettext "Unable to fetch in
> submodule path '\$sm_path'")"
> >                          fi
> >                          remote_name=$(sanitize_submodule_env; cd
> "$sm_path" && get_default_remote)
>
> It would be awesome if you could write a little test for this feature, too.
> Look for the tests in regarding --remote in t7406 (in the t/ directory) as
> a starting point, please.

Coming up with a test case is probably a better way to explain what I want the behaviour to be. Thanks for pointing out the test case area to look into.

> Thanks!
> Stefan
-- 
Eddy Petrișor
Eddy Petrișor· Mar 29, 2018, 22:59 UTC · lore

Re: [RFC PATCH v2] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default

(+list)
2018-03-30 1:55 GMT+03:00  <eddy.petrisor@gmail.com>:
Show 46 quoted lines
> From: Eddy Petrișor <eddy.petrisor@gmail.com>
>
> There are projects such as llvm/clang which use several repositories, and they
> might be forked for providing support for various features such as adding Redox
> awareness to the toolchain. This typically means the superproject will use
> another branch than master, occasionally even use an old commit from that
> non-master branch.
>
> Combined with the fact that when incorporating such a hierachy of repositories
> usually the user is interested in just the exact commit specified in the
> submodule info, it follows that a desireable usecase is to be also able to
> provide '--depth 1' or at least have a shallow clone to avoid waiting for ages
> for the clone operation to finish.
>
> In theory, this should be straightforward since the git protocol allows
> fetching an arbitary commit, but, in practice, some servers do not permit
> fetch-by-sha1.
>
> Git submodule seems to be very stubborn and cloning master, although the
> wrapper script and the gitmodules-helper could work together to clone directly
> the branch specified in the .gitmodules file, if specified.
>
> Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
> ---
>  git-submodule.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 24914963c..65e3af08b 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -589,8 +589,10 @@ cmd_update()
>                         branch=$(git submodule--helper remote-branch "$sm_path")
>                         if test -z "$nofetch"
>                         then
> +                               # non-default branch refspec
> +                               br_refspec=$(git submodule-helper remote-branch $sm_path)
>                                 # Fetch remote before determining tracking $sha1
> -                               fetch_in_submodule "$sm_path" $depth ||
> +                               fetch_in_submodule "$sm_path" $depth $br_refspec ||
>                                 die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
>                         fi
>                         remote_name=$(sanitize_submodule_env; cd "$sm_path" && get_default_remote)
> --
> 2.16.2
>

I am planning to write a test case in the next few days, between the few drops of free time I have. I am still trying to figure out my way through the test code.

-- 
Eddy Petrișor
Eddy Petrișor· Apr 3, 2018, 22:20 UTC · re: Eddy Petrișor · lore

[RFC PATCH v3 1/2] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default

From: Eddy Petrișor <eddy.petrisor@gmail.com>

There are projects such as llvm/clang which use several repositories, and they might be forked for providing support for various features such as adding Redox awareness to the toolchain. This typically means the superproject will use another branch than master, occasionally even use an old commit from that non-master branch.

Combined with the fact that when incorporating such a hierachy of repositories usually the user is interested in just the exact commit specified in the submodule info, it follows that a desireable usecase is to be also able to provide '--depth 1' or at least have a shallow clone to avoid waiting for ages for the clone operation to finish.

In theory, this should be straightforward since the git protocol allows fetching an arbitary commit, but, in practice, some servers do not permit fetch-by-sha1.

Git submodule seems to be very stubborn and cloning master, although the wrapper script and the gitmodules-helper could work together to clone directly the branch specified in the .gitmodules file, if specified.

Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
---
 git-submodule.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to git-submodule.sh +3 −1
diff --git a/git-submodule.sh b/git-submodule.sh
index 24914963c..65e3af08b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -589,8 +589,10 @@ cmd_update()
 			branch=$(git submodule--helper remote-branch "$sm_path")
 			if test -z "$nofetch"
 			then
+				# non-default branch refspec
+				br_refspec=$(git submodule-helper remote-branch $sm_path)
 				# Fetch remote before determining tracking $sha1
-				fetch_in_submodule "$sm_path" $depth ||
+				fetch_in_submodule "$sm_path" $depth $br_refspec ||
 				die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
 			fi
 			remote_name=$(sanitize_submodule_env; cd "$sm_path" && get_default_remote)
-- 
2.16.2
Eddy Petrișor· Apr 3, 2018, 22:20 UTC · re: Eddy Petrișor · lore

[RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule

From: Eddy Petrișor <eddy.petrisor@gmail.com>

If a submodule uses a non-default branch and the branch info is versioned, on submodule update --recursive --init the correct branch should be checked out.

Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
---
 t/t7406-submodule-update.sh | 54 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
Show changes to t/t7406-submodule-update.sh +54 −0
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 6f083c4d6..7b65f1dd1 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -259,6 +259,60 @@ test_expect_success 'submodule update --remote should fetch upstream changes wit
 	)
 '
 
+test_expect_success 'submodule update --remote --recursive --init should fetch module branch from .gitmodules' '
+	git clone . super5 &&
+	git clone super5 submodl2b2 &&
+	git clone super5 submodl1b1 &&
+	cd submodl2b2 &&
+	echo linel2b2 > l2b2 &&
+	git checkout -b b2 &&
+	git add l2b2 &&
+	test_tick &&
+	git commit -m "commit on b2 branch in l2" &&
+	git rev-parse --verify HEAD >../expectl2 &&
+	git checkout master &&
+	cd ../submodl1b1 &&
+	git checkout -b b1 &&
+	echo linel1b1 > l1b1 &&
+	git add l1b1 &&
+	test_tick &&
+	git commit -m "commit on b1 branch in l1" &&
+	git submodule add ../submodl2b2 submodl2b2 &&
+	git config -f .gitmodules submodule."submodl2b2".branch b2 &&
+	git add .gitmodules &&
+	test_tick &&
+	git commit -m "add l2 module with branch b2 in l1 module in branch b1" &&
+	git submodule init submodl2b2 &&
+	git rev-parse --verify HEAD >../expectl1 &&
+	git checkout master &&
+	cd ../super5 &&
+	echo super_with_2_chained_modules > super5 &&
+	git add super5 &&
+	test_tick &&
+	git commit -m "commit on default branch in super5" &&
+	git submodule add ../submodl1b1 submodl1b1 &&
+	git config -f .gitmodules submodule."submodl1b1".branch b1 &&
+	git add .gitmodules &&
+	test_tick &&
+	git commit -m "add l1 module with branch b1 in super5" &&
+	git submodule init submodl1b1 &&
+	git clone super5 super &&
+	(
+		cd super &&
+		git submodule update --recursive --init
+	) &&
+	(
+		cd submodl1b1 &&
+		git rev-parse --verify HEAD >../../actuall1 &&
+		test_cmp ../../expectl1 ../../actuall1
+	) &&
+	(
+		cd submodl2b2 &&
+		git rev-parse --verify HEAD >../../../actuall2 &&
+		test_cmp ../../../expectl2 ../../../actuall2
+	)
+'
+
 test_expect_success 'local config should override .gitmodules branch' '
 	(cd submodule &&
 	 git checkout test-branch &&
-- 
2.16.2
Eddy Petrișor· Apr 3, 2018, 22:26 UTC · re: Eddy Petrișor · lore

Re: [RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule

Notes:
    I am aware this test is not probably the best one and maybe I
should have first one test that does a one level non-default, before
trying a test with 2 levels of submodules, but I wanted to express the
goal of the patch.
    Currently the test fails, so I am obviously missing something.
Help would be appreciated.
2018-04-04 1:20 GMT+03:00 Eddy Petrișor <eddy.petrisor@codeaurora.org>:
Show 78 quoted lines
> From: Eddy Petrișor <eddy.petrisor@gmail.com>
>
> If a submodule uses a non-default branch and the branch info is versioned, on
> submodule update --recursive --init the correct branch should be checked out.
>
> Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
> ---
>  t/t7406-submodule-update.sh | 54 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 6f083c4d6..7b65f1dd1 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -259,6 +259,60 @@ test_expect_success 'submodule update --remote should fetch upstream changes wit
>         )
>  '
>
> +test_expect_success 'submodule update --remote --recursive --init should fetch module branch from .gitmodules' '
> +       git clone . super5 &&
> +       git clone super5 submodl2b2 &&
> +       git clone super5 submodl1b1 &&
> +       cd submodl2b2 &&
> +       echo linel2b2 > l2b2 &&
> +       git checkout -b b2 &&
> +       git add l2b2 &&
> +       test_tick &&
> +       git commit -m "commit on b2 branch in l2" &&
> +       git rev-parse --verify HEAD >../expectl2 &&
> +       git checkout master &&
> +       cd ../submodl1b1 &&
> +       git checkout -b b1 &&
> +       echo linel1b1 > l1b1 &&
> +       git add l1b1 &&
> +       test_tick &&
> +       git commit -m "commit on b1 branch in l1" &&
> +       git submodule add ../submodl2b2 submodl2b2 &&
> +       git config -f .gitmodules submodule."submodl2b2".branch b2 &&
> +       git add .gitmodules &&
> +       test_tick &&
> +       git commit -m "add l2 module with branch b2 in l1 module in branch b1" &&
> +       git submodule init submodl2b2 &&
> +       git rev-parse --verify HEAD >../expectl1 &&
> +       git checkout master &&
> +       cd ../super5 &&
> +       echo super_with_2_chained_modules > super5 &&
> +       git add super5 &&
> +       test_tick &&
> +       git commit -m "commit on default branch in super5" &&
> +       git submodule add ../submodl1b1 submodl1b1 &&
> +       git config -f .gitmodules submodule."submodl1b1".branch b1 &&
> +       git add .gitmodules &&
> +       test_tick &&
> +       git commit -m "add l1 module with branch b1 in super5" &&
> +       git submodule init submodl1b1 &&
> +       git clone super5 super &&
> +       (
> +               cd super &&
> +               git submodule update --recursive --init
> +       ) &&
> +       (
> +               cd submodl1b1 &&
> +               git rev-parse --verify HEAD >../../actuall1 &&
> +               test_cmp ../../expectl1 ../../actuall1
> +       ) &&
> +       (
> +               cd submodl2b2 &&
> +               git rev-parse --verify HEAD >../../../actuall2 &&
> +               test_cmp ../../../expectl2 ../../../actuall2
> +       )
> +'
> +
>  test_expect_success 'local config should override .gitmodules branch' '
>         (cd submodule &&
>          git checkout test-branch &&
> --
> 2.16.2
>
-- 
Eddy Petrișor
Stefan Beller· Apr 4, 2018, 18:36 UTC · re: Eddy Petrișor · lore

Re: [RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule

On Tue, Apr 3, 2018 at 3:26 PM, Eddy Petrișor <eddy.petrisor@gmail.com> wrote:
Show 5 quoted lines
> Notes:
>     I am aware this test is not probably the best one and maybe I
> should have first one test that does a one level non-default, before
> trying a test with 2 levels of submodules, but I wanted to express the
> goal of the patch.

This patch only contains the test, I presume this goes on top of https://public-inbox.org/git/20180403222053.23132-1-eddy.petrisor@codeaurora.org/ which you plan to later submit as one patch including both the change as well as the test.

Show 25 quoted lines
>     Currently the test fails, so I am obviously missing something.
> Help would be appreciated.
>
>
> 2018-04-04 1:20 GMT+03:00 Eddy Petrișor <eddy.petrisor@codeaurora.org>:
>> From: Eddy Petrișor <eddy.petrisor@gmail.com>
>>
>> If a submodule uses a non-default branch and the branch info is versioned, on
>> submodule update --recursive --init the correct branch should be checked out.
>>
>> Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
>> ---
>>  t/t7406-submodule-update.sh | 54 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 54 insertions(+)
>>
>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>> index 6f083c4d6..7b65f1dd1 100755
>> --- a/t/t7406-submodule-update.sh
>> +++ b/t/t7406-submodule-update.sh
>> @@ -259,6 +259,60 @@ test_expect_success 'submodule update --remote should fetch upstream changes wit
>>         )
>>  '
>>
>> +test_expect_success 'submodule update --remote --recursive --init should fetch module branch from .gitmodules' '
>> +       git clone . super5 &&

I found adding "test_pause &&" to be a great helper as it will drop you in a shell where you can inspect the repository.

>> +       git clone super5 submodl2b2 &&
>> +       git clone super5 submodl1b1 &&
We may want to cleanup after the test is done:
  test_when_finished "rm submodl2b2" &&
  test_when_finished "rm submodl1b2" &&
>> +       cd submodl2b2 &&
I'd think the test suite prefers subshells to operate in other dirs
instead of direct cd's, just like at the end of the test.
For short parts, we make heavy use of the -C option,
So for example the code below
       (
               cd super &&
               git submodule update --recursive --init
       ) &&
can be written as
    git -C super submodule update --recursive --init
which is shorter.
>> +       echo linel2b2 > l2b2 &&
style: The Git test suite prefers to have the redirect adjacent to the
file name:
  echo hello >world
Show 5 quoted lines
>> +       git checkout -b b2 &&
>> +       git add l2b2 &&
>> +       test_tick &&
>> +       git commit -m "commit on b2 branch in l2" &&
>> +       git rev-parse --verify HEAD >../expectl2 &&

So until now we made a commit in a submodule on branch b2 and wrote it out to an expect file.

Show 7 quoted lines
>> +       git checkout master &&
>> +       cd ../submodl1b1 &&
>> +       git checkout -b b1 &&
>> +       echo linel1b1 > l1b1 &&
>> +       git add l1b1 &&
>> +       test_tick &&
>> +       git commit -m "commit on b1 branch in l1" &&
very similar to above, just in another repo
instead of making a commit yourself, you may want to use
    test_commit <name>
then you don't need to call echo/add/commit yourself.
Show 5 quoted lines
>> +       git submodule add ../submodl2b2 submodl2b2 &&
>> +       git config -f .gitmodules submodule."submodl2b2".branch b2 &&
>> +       git add .gitmodules &&
>> +       test_tick &&
>> +       git commit -m "add l2 module with branch b2 in l1 module in branch b1" &&

So one submodule is made to be a submodule of the other with a specific branch (b2)

>> +       git submodule init submodl2b2 &&

git submodule add ought to have initialized that submodule already, I am not sure we need the explicit init here.

>> +       git rev-parse --verify HEAD >../expectl1 &&
>> +       git checkout master &&
We go back to master, which doesn't have the nested submodule?
Show 10 quoted lines
>> +       cd ../super5 &&
>> +       echo super_with_2_chained_modules > super5 &&
>> +       git add super5 &&
>> +       test_tick &&
>> +       git commit -m "commit on default branch in super5" &&
>> +       git submodule add ../submodl1b1 submodl1b1 &&
>> +       git config -f .gitmodules submodule."submodl1b1".branch b1 &&
>> +       git add .gitmodules &&
>> +       test_tick &&
>> +       git commit -m "add l1 module with branch b1 in super5" &&

now we do this again without the nested submodule, just one repo as a submodule?

>> +       git submodule init submodl1b1 &&
>> +       git clone super5 super &&

does super exist here already? (I did not check, but IIRC super and super{1-4} are there as we count upwards to find a name that is ok.

Show 14 quoted lines
>> +       (
>> +               cd super &&
>> +               git submodule update --recursive --init
>> +       ) &&
>> +       (
>> +               cd submodl1b1 &&
>> +               git rev-parse --verify HEAD >../../actuall1 &&
>> +               test_cmp ../../expectl1 ../../actuall1
>> +       ) &&
>> +       (
>> +               cd submodl2b2 &&
>> +               git rev-parse --verify HEAD >../../../actuall2 &&
>> +               test_cmp ../../../expectl2 ../../../actuall2
>> +       )
Stefan
Eddy Petrișor· Apr 4, 2018, 20:37 UTC · re: Stefan Beller · lore

Re: [RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule

2018-04-04 21:36 GMT+03:00 Stefan Beller <sbeller@google.com>:
Show 11 quoted lines
> On Tue, Apr 3, 2018 at 3:26 PM, Eddy Petrișor <eddy.petrisor@gmail.com> wrote:
>> Notes:
>>     I am aware this test is not probably the best one and maybe I
>> should have first one test that does a one level non-default, before
>> trying a test with 2 levels of submodules, but I wanted to express the
>> goal of the patch.
>
> This patch only contains the test, I presume this goes on top of
> https://public-inbox.org/git/20180403222053.23132-1-eddy.petrisor@codeaurora.org/
> which you plan to later submit as one patch including both the change as well as
> the test.

Yes, not sure why the emails were not linked together, I specified the in-reply-to mesage id when I "git format-patch"-ed the patches.

I wasn't actually planning to squash them in a single patch, will do, but I guess for now it helps to focus the discussion around the test since the implementation is still lacking, see below 2 lines comment.

Show 6 quoted lines
>>     Currently the test fails, so I am obviously missing something.
>> Help would be appreciated.
>>
>>
>> 2018-04-04 1:20 GMT+03:00 Eddy Petrișor <eddy.petrisor@codeaurora.org>:
>>> From: Eddy Petrișor <eddy.petrisor@gmail.com>
[..]
Show 12 quoted lines
>>> --- a/t/t7406-submodule-update.sh
>>> +++ b/t/t7406-submodule-update.sh
>>> @@ -259,6 +259,60 @@ test_expect_success 'submodule update --remote should fetch upstream changes wit
>>>         )
>>>  '
>>>
>>> +test_expect_success 'submodule update --remote --recursive --init should fetch module branch from .gitmodules' '
>>> +       git clone . super5 &&
>
> I found adding "test_pause &&"
> to be a great helper as it will drop you in a shell where
> you can inspect the repository.

Thanks for the pointer, I only looked at the post-failure state after adding --debug -i --verbose, but having "test_pause" to stop and inspect the interim stage should help a lot with debugging.

Show 8 quoted lines
>
>>> +       git clone super5 submodl2b2 &&
>>> +       git clone super5 submodl1b1 &&
>
> We may want to cleanup after the test is done:
>
>   test_when_finished "rm submodl2b2" &&
>   test_when_finished "rm submodl1b2" &&
Sure, will do.
>>> +       cd submodl2b2 &&
>
> I'd think the test suite prefers subshells to operate in other dirs
> instead of direct cd's, just like at the end of the test.

After looking at other tests I was under the impression that the setup phase (e.g. creating the "server" side repos) of the test was done in the main context, while the test case (i.e. the client side, or what the user would do) is in subshells.

Show 12 quoted lines
> For short parts, we make heavy use of the -C option,
> So for example the code below
>        (
>                cd super &&
>                git submodule update --recursive --init
>        ) &&
>
> can be written as
>
>     git -C super submodule update --recursive --init
>
> which is shorter.
Nice.
Show 5 quoted lines
>>> +       echo linel2b2 > l2b2 &&
>
> style: The Git test suite prefers to have the redirect adjacent to the
> file name:
>   echo hello >world

I wasn't aware of that, it seems there are some inconsistencies, including in the modified test:

eddy@feodora:~/usr/src/git/t$ grep '> ' -c t* 2>/dev/null | grep -v ':0$' | grep 7406 t7406-submodule-update.sh:24 eddy@feodora:~/usr/src/git/t$ grep '> ' -c t* 2>/dev/null | grep -v ':0$' | wc -l 325

I suspect that a minor patch correcting these inconsistencies would be faily fast reviewed adn accepted, of course, disconnected from this modification.

Show 8 quoted lines
>>> +       git checkout -b b2 &&
>>> +       git add l2b2 &&
>>> +       test_tick &&
>>> +       git commit -m "commit on b2 branch in l2" &&
>>> +       git rev-parse --verify HEAD >../expectl2 &&
>
> So until now we made a commit in a submodule on branch b2
> and wrote it out to an expect file.
Yes, I was trying to replicate the redox-os case which has this structure:
redox-os (master branch)
   + rust (@some commit on a non-default)
          + src/llvm (@some commit on a non-default branch)

This section is making sure the b2 branch has other content than the default one and setting the expectation for level2 submodule branch, later.

Show 12 quoted lines
>>> +       git checkout master &&
>>> +       cd ../submodl1b1 &&
>>> +       git checkout -b b1 &&
>>> +       echo linel1b1 > l1b1 &&
>>> +       git add l1b1 &&
>>> +       test_tick &&
>>> +       git commit -m "commit on b1 branch in l1" &&
>
> very similar to above, just in another repo
> instead of making a commit yourself, you may want to use
>     test_commit <name>
> then you don't need to call echo/add/commit yourself.

thanks for the pointer, will change that since my purpose for the commit was to make sure default branch (master) and b1 branch are different

Show 8 quoted lines
>>> +       git submodule add ../submodl2b2 submodl2b2 &&
>>> +       git config -f .gitmodules submodule."submodl2b2".branch b2 &&
>>> +       git add .gitmodules &&
>>> +       test_tick &&
>>> +       git commit -m "add l2 module with branch b2 in l1 module in branch b1" &&
>
> So one submodule is made to be a submodule of the other
> with a specific branch (b2)
correct
>>> +       git submodule init submodl2b2 &&
>
> git submodule add ought to have initialized that submodule
> already, I am not sure we need the explicit init here.

I think that from my tests I saw that without it the submodule wasn't there, but I might be mistaken, will check.

>>> +       git rev-parse --verify HEAD >../expectl1 &&
>>> +       git checkout master &&
>
> We go back to master, which doesn't have the nested submodule?

exactly, since the desired behaviour is to have in the nested submodule the b2 branch. I guess I could have added the level2 submodule@master on l1's master branch, but I didn't see much point in that since it should be enough now.

It might make more sense if master and b2, and, respectively master and l1 have diverging histories, but for now that is probably overkill and it will make sense in the context of shallow cloning of submodules, which I haven't yet tackled except presenting the idea.

Show 13 quoted lines
>>> +       cd ../super5 &&
>>> +       echo super_with_2_chained_modules > super5 &&
>>> +       git add super5 &&
>>> +       test_tick &&
>>> +       git commit -m "commit on default branch in super5" &&
>>> +       git submodule add ../submodl1b1 submodl1b1 &&
>>> +       git config -f .gitmodules submodule."submodl1b1".branch b1 &&
>>> +       git add .gitmodules &&
>>> +       test_tick &&
>>> +       git commit -m "add l1 module with branch b1 in super5" &&
>
> now we do this again without the nested submodule, just one repo
> as a submodule?

My intention was to have super5 -> submodl1b1@b1 -> submodl2b2@b2 on the "server" side. But are you saying I just implemented super5 -> sbmodl1b1@master due to the previous master checkout in submodl1b1?

Show 6 quoted lines
>>> +       git submodule init submodl1b1 &&
>>> +       git clone super5 super &&
>
> does super exist here already? (I did not check, but IIRC
> super and super{1-4} are there as we count upwards to
> find a name that is ok.

I created it in the first step of the test with the intention to have super5 as the "server" and "super" as the client clone.

Show 15 quoted lines
>
>>> +       (
>>> +               cd super &&
>>> +               git submodule update --recursive --init
>>> +       ) &&
>>> +       (
>>> +               cd submodl1b1 &&
>>> +               git rev-parse --verify HEAD >../../actuall1 &&
>>> +               test_cmp ../../expectl1 ../../actuall1
>>> +       ) &&
>>> +       (
>>> +               cd submodl2b2 &&
>>> +               git rev-parse --verify HEAD >../../../actuall2 &&
>>> +               test_cmp ../../../expectl2 ../../../actuall2
>>> +       )
As a general idea for a test, does it look sane?

Do you think I should I start with a just one level of submodule with a non-default branch (super -> l1@b1), or it this OK? In my view, having 2 levels makes sure the recursive part is also addressed and verified.

-- 
Eddy Petrișor
Stefan Beller· Apr 4, 2018, 21:41 UTC · re: Eddy Petrișor · lore

Re: [RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule

On Wed, Apr 4, 2018 at 1:37 PM, Eddy Petrișor <eddy.petrisor@gmail.com> wrote:
Show 7 quoted lines
>> This patch only contains the test, I presume this goes on top of
>> https://public-inbox.org/git/20180403222053.23132-1-eddy.petrisor@codeaurora.org/
>> which you plan to later submit as one patch including both the change as well as
>> the test.
>
> Yes, not sure why the emails were not linked together, I specified the
> in-reply-to mesage id when I "git format-patch"-ed the patches.

They are linked in public-inbox, but I noticed too late. (in gmail they were not)

> Thanks for the pointer, I only looked at the post-failure state after
> adding --debug -i --verbose, but having "test_pause" to stop and
> inspect the interim stage should help a lot with debugging.

I also like the -x flag that stops at the failing test, but as this is the last test of this script it is of limited use here.

> After looking at other tests I was under the impression that the setup
> phase (e.g. creating the "server" side repos) of the test was done in
> the main context, while the test case (i.e. the client side, or what
> the user would do) is in subshells.
This roughly coincides, as the client is usually in a new clone. :)
Show 13 quoted lines
>> style: The Git test suite prefers to have the redirect adjacent to the
>> file name:
>>   echo hello >world
>
> I wasn't aware of that, it seems there are some inconsistencies,
> including in the modified test:
>
> eddy@feodora:~/usr/src/git/t$ grep '> ' -c t* 2>/dev/null | grep -v
> ':0$' | grep 7406
> t7406-submodule-update.sh:24
> eddy@feodora:~/usr/src/git/t$ grep '> ' -c t* 2>/dev/null | grep -v
> ':0$' | wc -l
> 325
Thanks for digging up numbers!
> I suspect that a minor patch correcting these inconsistencies would be
> faily fast reviewed adn accepted, of course, disconnected from this
> modification.

There are two schools of thought, one of them is to fix things up whenever you see fit. I am very sympathetic to this one.

The other is found in Documentation/CodingGuidelines:
 - Fixing style violations while working on a real change as a
   preparatory clean-up step is good, but otherwise avoid useless code
   churn for the sake of conforming to the style.
   "Once it _is_ in the tree, it's not really worth the patch noise to
   go and fix it up."
   Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html
Show 9 quoted lines
> Yes, I was trying to replicate the redox-os case which has this structure:
>
> redox-os (master branch)
>    + rust (@some commit on a non-default)
>           + src/llvm (@some commit on a non-default branch)
>
> This section is making sure the b2 branch has other content than the
> default one and setting the expectation for level2 submodule branch,
> later.
Oh, cool!
Show 8 quoted lines
>
>>>> +       git rev-parse --verify HEAD >../expectl1 &&
>>>> +       git checkout master &&
>>
>> We go back to master, which doesn't have the nested submodule?
>
> exactly, since the desired behaviour is to have in the nested
> submodule the b2 branch.

Well if the nested submodule doesn't exist at HEAD, then git should not change branches in there, only on checking out/ updating to a state that has the nested sub? (I may missunderstand things here)

Show 8 quoted lines
> I guess I could have added the level2 submodule@master on l1's master
> branch, but I didn't see much point in that since it should be enough
> now.
>
> It might make more sense if master and b2, and, respectively master
> and l1 have diverging histories, but for now that is probably overkill
> and it will make sense in the context of shallow cloning of
> submodules, which I haven't yet tackled except presenting the idea.
ok.
Show 19 quoted lines
>
>>>> +       cd ../super5 &&
>>>> +       echo super_with_2_chained_modules > super5 &&
>>>> +       git add super5 &&
>>>> +       test_tick &&
>>>> +       git commit -m "commit on default branch in super5" &&
>>>> +       git submodule add ../submodl1b1 submodl1b1 &&
>>>> +       git config -f .gitmodules submodule."submodl1b1".branch b1 &&
>>>> +       git add .gitmodules &&
>>>> +       test_tick &&
>>>> +       git commit -m "add l1 module with branch b1 in super5" &&
>>
>> now we do this again without the nested submodule, just one repo
>> as a submodule?
>
> My intention was to have super5 -> submodl1b1@b1 -> submodl2b2@b2 on
> the "server" side.
> But are you saying I just implemented super5 -> sbmodl1b1@master due
> to the previous master checkout in submodl1b1?
No. I was a little confused about the code.
Show 10 quoted lines
>
>>>> +       git submodule init submodl1b1 &&
>>>> +       git clone super5 super &&
>>
>> does super exist here already? (I did not check, but IIRC
>> super and super{1-4} are there as we count upwards to
>> find a name that is ok.
>
> I created it in the first step of the test with the intention to have
> super5 as the "server" and "super" as the client clone.
oh, ok.
Show 18 quoted lines
>
>>
>>>> +       (
>>>> +               cd super &&
>>>> +               git submodule update --recursive --init
>>>> +       ) &&
>>>> +       (
>>>> +               cd submodl1b1 &&
>>>> +               git rev-parse --verify HEAD >../../actuall1 &&
>>>> +               test_cmp ../../expectl1 ../../actuall1
>>>> +       ) &&
>>>> +       (
>>>> +               cd submodl2b2 &&
>>>> +               git rev-parse --verify HEAD >../../../actuall2 &&
>>>> +               test_cmp ../../../expectl2 ../../../actuall2
>>>> +       )
>
> As a general idea for a test, does it look sane?
Yes, I think it is a sane approach. Thanks for writing such a test!
> Do you think I should I start with a just one level of submodule with
> a non-default branch (super -> l1@b1), or it this OK?
> In my view, having 2 levels makes sure the recursive part is also
> addressed and verified.
I totally agree.
Stefan
Eddy Petrișor· Apr 18, 2018, 22:35 UTC · re: Stefan Beller · lore

[RFC PATCH v4 1/9] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default

From: Eddy Petrișor <eddy.petrisor@gmail.com>

There are projects such as llvm/clang which use several repositories, and they might be forked for providing support for various features such as adding Redox awareness to the toolchain. This typically means the superproject will use another branch than master, occasionally even use an old commit from that non-master branch.

Combined with the fact that when incorporating such a hierachy of repositories usually the user is interested in just the exact commit specified in the submodule info, it follows that a desireable usecase is to be also able to provide '--depth 1' or at least have a shallow clone to avoid waiting for ages for the clone operation to finish.

In theory, this should be straightforward since the git protocol allows fetching an arbitary commit, but, in practice, some servers do not permit fetch-by-sha1.

Git submodule seems to be very stubborn and cloning master, although the wrapper script and the gitmodules-helper could work together to clone directly the branch specified in the .gitmodules file, if specified.

Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
---
 git-submodule.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to git-submodule.sh +3 −1
diff --git a/git-submodule.sh b/git-submodule.sh
index 24914963c..65e3af08b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -589,8 +589,10 @@ cmd_update()
 			branch=$(git submodule--helper remote-branch "$sm_path")
 			if test -z "$nofetch"
 			then
+				# non-default branch refspec
+				br_refspec=$(git submodule-helper remote-branch $sm_path)
 				# Fetch remote before determining tracking $sha1
-				fetch_in_submodule "$sm_path" $depth ||
+				fetch_in_submodule "$sm_path" $depth $br_refspec ||
 				die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
 			fi
 			remote_name=$(sanitize_submodule_env; cd "$sm_path" && get_default_remote)
-- 
2.16.2
Eddy Petrișor· Apr 18, 2018, 22:35 UTC · re: Eddy Petrișor · lore

[RFC PATCH v4 2/9] t7406: add test for non-default branch in submodule

From: Eddy Petrișor <eddy.petrisor@gmail.com>

If a submodule uses a non-default branch and the branch info is versioned, on submodule update --recursive --init the correct branch should be checked out.

Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
---
 t/t7406-submodule-update.sh | 54 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
Show changes to t/t7406-submodule-update.sh +54 −0
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 6f083c4d6..7b65f1dd1 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -259,6 +259,60 @@ test_expect_success 'submodule update --remote should fetch upstream changes wit
 	)
 '
 
+test_expect_success 'submodule update --remote --recursive --init should fetch module branch from .gitmodules' '
+	git clone . super5 &&
+	git clone super5 submodl2b2 &&
+	git clone super5 submodl1b1 &&
+	cd submodl2b2 &&
+	echo linel2b2 > l2b2 &&
+	git checkout -b b2 &&
+	git add l2b2 &&
+	test_tick &&
+	git commit -m "commit on b2 branch in l2" &&
+	git rev-parse --verify HEAD >../expectl2 &&
+	git checkout master &&
+	cd ../submodl1b1 &&
+	git checkout -b b1 &&
+	echo linel1b1 > l1b1 &&
+	git add l1b1 &&
+	test_tick &&
+	git commit -m "commit on b1 branch in l1" &&
+	git submodule add ../submodl2b2 submodl2b2 &&
+	git config -f .gitmodules submodule."submodl2b2".branch b2 &&
+	git add .gitmodules &&
+	test_tick &&
+	git commit -m "add l2 module with branch b2 in l1 module in branch b1" &&
+	git submodule init submodl2b2 &&
+	git rev-parse --verify HEAD >../expectl1 &&
+	git checkout master &&
+	cd ../super5 &&
+	echo super_with_2_chained_modules > super5 &&
+	git add super5 &&
+	test_tick &&
+	git commit -m "commit on default branch in super5" &&
+	git submodule add ../submodl1b1 submodl1b1 &&
+	git config -f .gitmodules submodule."submodl1b1".branch b1 &&
+	git add .gitmodules &&
+	test_tick &&
+	git commit -m "add l1 module with branch b1 in super5" &&
+	git submodule init submodl1b1 &&
+	git clone super5 super &&
+	(
+		cd super &&
+		git submodule update --recursive --init
+	) &&
+	(
+		cd submodl1b1 &&
+		git rev-parse --verify HEAD >../../actuall1 &&
+		test_cmp ../../expectl1 ../../actuall1
+	) &&
+	(
+		cd submodl2b2 &&
+		git rev-parse --verify HEAD >../../../actuall2 &&
+		test_cmp ../../../expectl2 ../../../actuall2
+	)
+'
+
 test_expect_success 'local config should override .gitmodules branch' '
 	(cd submodule &&
 	 git checkout test-branch &&
-- 
2.16.2
Eddy Petrișor· Apr 18, 2018, 22:35 UTC · re: Eddy Petrișor · lore

[RFC PATCH v4 3/9] fixup:t7406: use test_commit instead of echo/add/commit as suggested by Stefan Beller

From: Eddy Petrișor <eddy.petrisor@gmail.com>
Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
---
 t/t7406-submodule-update.sh | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)
Show changes to t/t7406-submodule-update.sh +3 −12
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 7b65f1dd1..7fb370991 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -264,19 +264,13 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m
 	git clone super5 submodl2b2 &&
 	git clone super5 submodl1b1 &&
 	cd submodl2b2 &&
-	echo linel2b2 > l2b2 &&
 	git checkout -b b2 &&
-	git add l2b2 &&
-	test_tick &&
-	git commit -m "commit on b2 branch in l2" &&
+	test_commit "l2_on_b2" &&
 	git rev-parse --verify HEAD >../expectl2 &&
 	git checkout master &&
 	cd ../submodl1b1 &&
 	git checkout -b b1 &&
-	echo linel1b1 > l1b1 &&
-	git add l1b1 &&
-	test_tick &&
-	git commit -m "commit on b1 branch in l1" &&
+	test_commit "l1_on_b1" &&
 	git submodule add ../submodl2b2 submodl2b2 &&
 	git config -f .gitmodules submodule."submodl2b2".branch b2 &&
 	git add .gitmodules &&
@@ -286,10 +280,7 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m
 	git rev-parse --verify HEAD >../expectl1 &&
 	git checkout master &&
 	cd ../super5 &&
-	echo super_with_2_chained_modules > super5 &&
-	git add super5 &&
-	test_tick &&
-	git commit -m "commit on default branch in super5" &&
+	test_commit super5_with_2_chained_modules_on_default_branch &&
 	git submodule add ../submodl1b1 submodl1b1 &&
 	git config -f .gitmodules submodule."submodl1b1".branch b1 &&
 	git add .gitmodules &&
-- 
2.16.2
Eddy Petrișor· Apr 18, 2018, 22:35 UTC · re: Eddy Petrișor · lore

[RFC PATCH v4 5/9] fixup:t7406:cleanup chained submodules after test is done

From: Eddy Petrișor <eddy.petrisor@gmail.com>
Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
---
 t/t7406-submodule-update.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to t/t7406-submodule-update.sh +3 −1
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 974f949df..c5b412c46 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -298,7 +298,9 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m
 		cd submodl2b2 &&
 		git rev-parse --verify HEAD >../../../actuall2 &&
 		test_cmp ../../../expectl2 ../../../actuall2
-	)
+	) &&
+	test_when_finished "rm submodl2b2" &&
+	test_when_finished "rm submodl1b1"
 '
 
 test_expect_success 'local config should override .gitmodules branch' '
-- 
2.16.2
Eddy Petrișor· Apr 18, 2018, 22:35 UTC · re: Eddy Petrișor · lore

[RFC PATCH v4 4/9] fixup:t7404:use 'git -C' instead of cd .. && git

From: Eddy Petrișor <eddy.petrisor@gmail.com>
Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
---
 t/t7406-submodule-update.sh | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
Show changes to t/t7406-submodule-update.sh +1 −4
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 7fb370991..974f949df 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -288,10 +288,7 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m
 	git commit -m "add l1 module with branch b1 in super5" &&
 	git submodule init submodl1b1 &&
 	git clone super5 super &&
-	(
-		cd super &&
-		git submodule update --recursive --init
-	) &&
+	git -C super submodule update --recursive --init &&
 	(
 		cd submodl1b1 &&
 		git rev-parse --verify HEAD >../../actuall1 &&
-- 
2.16.2
Eddy Petrișor· Apr 18, 2018, 22:35 UTC · re: Eddy Petrișor · lore

[RFC PATCH v4 6/9] fixup:t7406:don't call init after add, is redundant

From: Eddy Petrișor <eddy.petrisor@gmail.com>
Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
---
 t/t7406-submodule-update.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Show changes to t/t7406-submodule-update.sh +1 −2
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index c5b412c46..32995e272 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -275,8 +275,7 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m
 	git config -f .gitmodules submodule."submodl2b2".branch b2 &&
 	git add .gitmodules &&
 	test_tick &&
-	git commit -m "add l2 module with branch b2 in l1 module in branch b1" &&
-	git submodule init submodl2b2 &&
+	git commit -m "add l2 (on b2) in l1 (on b1)" &&
 	git rev-parse --verify HEAD >../expectl1 &&
 	git checkout master &&
 	cd ../super5 &&
-- 
2.16.2
Eddy Petrișor· Apr 18, 2018, 22:35 UTC · re: Eddy Petrișor · lore

[RFC PATCH v4 7/9] fixup:t7406:supr5 commit is done before submodules chaining

From: Eddy Petrișor <eddy.petrisor@gmail.com>
Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
---
 t/t7406-submodule-update.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t7406-submodule-update.sh +1 −1
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 32995e272..18328be73 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -279,7 +279,7 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m
 	git rev-parse --verify HEAD >../expectl1 &&
 	git checkout master &&
 	cd ../super5 &&
-	test_commit super5_with_2_chained_modules_on_default_branch &&
+	test_commit super5_commit_before_2_chained_modules_on_default_branch &&
 	git submodule add ../submodl1b1 submodl1b1 &&
 	git config -f .gitmodules submodule."submodl1b1".branch b1 &&
 	git add .gitmodules &&
-- 
2.16.2
Eddy Petrișor· Apr 18, 2018, 22:35 UTC · re: Eddy Petrișor · lore

[RFC PATCH v4 9/9] fixup:t7406:change branches in submodules after the link is done

From: Eddy Petrișor <eddy.petrisor@gmail.com>
Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
---
 t/t7406-submodule-update.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t7406-submodule-update.sh +2 −2
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f44872143..68c25ce0f 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -267,17 +267,16 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m
 	git checkout -b b2 &&
 	test_commit "l2_on_b2" &&
 	git rev-parse --verify HEAD >../expectl2 &&
-	git checkout master &&
 	cd ../submodl1b1 &&
 	git checkout -b b1 &&
 	test_commit "l1_on_b1" &&
 	git submodule add ../submodl2b2 submodl2b2 &&
 	git config -f .gitmodules submodule."submodl2b2".branch b2 &&
 	git add .gitmodules &&
+	git -C ../submodl2b2 checkout master &&
 	test_tick &&
 	git commit -m "add l2 (on b2) in l1 (on b1)" &&
 	git rev-parse --verify HEAD >../expectl1 &&
-	git checkout master &&
 	cd ../super5 &&
 	test_commit super5_commit_before_2_chained_modules_on_default_branch &&
 	git submodule add ../submodl1b1 submodl1b1 &&
@@ -285,6 +284,7 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m
 	git add .gitmodules &&
 	test_tick &&
 	git commit -m "add l1 module with branch b1 in super5" &&
+	git -C ../submodl1b1 checkout master &&
 	git submodule init submodl1b1 &&
 	cd .. &&
 	git clone super5 super_w &&
-- 
2.16.2
Eddy Petrișor· Apr 18, 2018, 22:35 UTC · re: Eddy Petrișor · lore

[RFC PATCH v4 8/9] fixup:t7406:use super_w instead of the existing super

From: Eddy Petrișor <eddy.petrisor@gmail.com>
Signed-off-by: Eddy Petrișor <eddy.petrisor@gmail.com>
---
 t/t7406-submodule-update.sh | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Show changes to t/t7406-submodule-update.sh +5 −3
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 18328be73..f44872143 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -286,10 +286,11 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m
 	test_tick &&
 	git commit -m "add l1 module with branch b1 in super5" &&
 	git submodule init submodl1b1 &&
-	git clone super5 super &&
-	git -C super submodule update --recursive --init &&
+	cd .. &&
+	git clone super5 super_w &&
+	git -C super_w submodule update --recursive --init &&
 	(
-		cd submodl1b1 &&
+		cd super_w/submodl1b1 &&
 		git rev-parse --verify HEAD >../../actuall1 &&
 		test_cmp ../../expectl1 ../../actuall1
 	) &&
@@ -298,6 +299,7 @@ test_expect_success 'submodule update --remote --recursive --init should fetch m
 		git rev-parse --verify HEAD >../../../actuall2 &&
 		test_cmp ../../../expectl2 ../../../actuall2
 	) &&
+	test_when_finished "rm super_w" &&
 	test_when_finished "rm submodl2b2" &&
 	test_when_finished "rm submodl1b1"
 '
-- 
2.16.2
Stefan Beller· Apr 18, 2018, 23:53 UTC · re: Eddy Petrișor · lore

Re: [RFC PATCH v4 1/9] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default

Hi Eddy,

all the following patches 3-9 are touching the test as added in patch 2, which would go best with this patch. Could you squash all commits into one?

There are a couple ways to do it:
  git reset --soft
  git commit -a --reuse-commit-message=<...>
or using
    git rebase --interactive origin/master
    # and then marking all but the first as "fixup"

I think the end result looks good, but that is best reviewed as one piece instead of 9 patches.

Thanks, Stefan

Eddy Petrișor· Apr 19, 2018, 05:43 UTC · re: Stefan Beller · lore

Re: [RFC PATCH v4 1/9] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default

2018-04-19 2:53 GMT+03:00 Stefan Beller <sbeller@google.com>:
Show 5 quoted lines
> Hi Eddy,
>
> all the following patches 3-9 are touching the test as added in patch
> 2, which would go best with this patch.
> Could you squash all commits into one?
Yes,

I did not have time yesterday to put all changes into a single commit with an associated note (because note managment seems to be a huge pain), so I preferred small commits.

But I wanted to get your feedback on something, I'll reply in thread arm where you actually suspected the problem.

Show 9 quoted lines
> There are a couple ways to do it:
>
>   git reset --soft
>   git commit -a --reuse-commit-message=<...>
>
> or using
>
>     git rebase --interactive origin/master
>     # and then marking all but the first as "fixup"

I am aware of git rebase -i and use it regularly, that's why patches 3-9 have the 'fixup' prefix.

> I think the end result looks good, but that is best reviewed as one
> piece instead of 9 patches.
-- 
Eddy Petrișor
Eddy Petrișor· Apr 19, 2018, 06:07 UTC · re: Stefan Beller · lore

Re: [RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule

2018-04-05 0:41 GMT+03:00 Stefan Beller <sbeller@google.com>:
Show 6 quoted lines
> On Wed, Apr 4, 2018 at 1:37 PM, Eddy Petrișor <eddy.petrisor@gmail.com> wrote:
>
>>> you plan to later submit as one patch including both the change as well as
>>> the test.
>>
>> Yes,

I did not forget about having a single patch. Will do it once the details are ironed out.

Show 20 quoted lines
>>>>> +       cd ../super5 &&
>>>>> +       echo super_with_2_chained_modules > super5 &&
>>>>> +       git add super5 &&
>>>>> +       test_tick &&
>>>>> +       git commit -m "commit on default branch in super5" &&
>>>>> +       git submodule add ../submodl1b1 submodl1b1 &&
>>>>> +       git config -f .gitmodules submodule."submodl1b1".branch b1 &&
>>>>> +       git add .gitmodules &&
>>>>> +       test_tick &&
>>>>> +       git commit -m "add l1 module with branch b1 in super5" &&
>>>
>>> now we do this again without the nested submodule, just one repo
>>> as a submodule?
>>
>> My intention was to have super5 -> submodl1b1@b1 -> submodl2b2@b2 on
>> the "server" side.
>> But are you saying I just implemented super5 -> sbmodl1b1@master due
>> to the previous master checkout in submodl1b1?
>
> No. I was a little confused about the code.

Actually you were 100% correct. In order to link to submodl1b1@b1 I had to move the master branch checkout after the submobl2@b2 is added.

Otherwise the submodule is added with the last commit on master, not the last one on b1 an b2, respectively.

I suspect that in the tests, because the "server side" repos are local, the git fetch-by-sha1/cloning by hash will be done correctly, without the need of a branch hint, but the problem will still exist for servers such as github which do not support fetch-by-sha1. In case I realize that a server-side repo that doesn't support fetch-by-sha1 is needed, is there a mechanism to set that up in the test case, or do I have to rethink my approach?

Show 11 quoted lines
>>>>> +       git submodule init submodl1b1 &&
>>>>> +       git clone super5 super &&
>>>
>>> does super exist here already? (I did not check, but IIRC
>>> super and super{1-4} are there as we count upwards to
>>> find a name that is ok.
>>
>> I created it in the first step of the test with the intention to have
>> super5 as the "server" and "super" as the client clone.
>
> oh, ok.

After using test_pause I realized 'super' is left over by some other test cases, so in my v4 (unjustifibly) long series I switch to using super_w because I was getting all sorts of issues and wanted to not interfere with the other tests.

>> As a general idea for a test, does it look sane?
>
> Yes, I think it is a sane approach. Thanks for writing such a test!
OK, thanks for the feedback.
Show 6 quoted lines
>> Do you think I should I start with a just one level of submodule with
>> a non-default branch (super -> l1@b1), or it this OK?
>> In my view, having 2 levels makes sure the recursive part is also
>> addressed and verified.
>
> I totally agree.
-- 
Eddy Petrișor
Stefan Beller· Apr 19, 2018, 17:52 UTC · re: Eddy Petrișor · lore

Re: [RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule

Hi Eddy,
Show 7 quoted lines
> I suspect that in the tests, because the "server side" repos are
> local, the git fetch-by-sha1/cloning by hash will be done correctly,
> without the need of a branch hint, but the problem will still exist
> for servers such as github which do not support fetch-by-sha1.
> In case I realize that a server-side repo that doesn't support
> fetch-by-sha1 is needed, is there a mechanism to set that up in the
> test case, or do I have to rethink my approach?

You can force a clone (at least, not sure about fetch) to use the git protocol by --no-local, and then you can set uploadpack.{allowTipSHA1InWant, allowReachableSHA1InWant, allowAnySHA1InWant} as neded by the test.

From the fetch man page: For local repositories, also supported by Git natively, the following syntaxes may be used:

       ·   /path/to/repo.git/
       ·   file:///path/to/repo.git/

These two syntaxes are mostly equivalent, except when cloning, when the former implies --local option.

Thanks, Stefan

← back to recent threads