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

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

[Jonathan Nieder]

(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?

> 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.

> 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

>  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)

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

[Eddy Petrișor]

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.

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.

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

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

[Eddy Petrișor]

2018-03-08 21:29 GMT+02:00 Eddy Petrișor <eddy.petrisor@gmail.com>:
> 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.

> 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

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

[Thomas Gummerer]

On 03/08, Eddy Petrișor wrote:
> 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(-)

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

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

[Eric Sunshine]

On Fri, Mar 16, 2018 at 5:33 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> 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

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

[Thomas Gummerer]

On 03/17, Eddy Petrișor wrote:
> 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.

> 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
> >

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

[Eric Sunshine]

On Sat, Mar 17, 2018 at 3:11 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> 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.

>> > 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.

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

[Stefan Beller]

[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!

> 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

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

[Eddy Petrișor]

mar., 27 mar. 2018, 02:07 Stefan Beller <sbeller@google.com> a scris:
>
> [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.

> > 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.

> > 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.

> > 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?

> > 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 :)

> > 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.
>
>
> >                                  # 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

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

[Eddy Petrișor]

(+list)

2018-03-30 1:55 GMT+03:00  <eddy.petrisor@gmail.com>:
> 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

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

[Eddy Petrișor]

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

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

[Eddy Petrișor]

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

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

[Eddy Petrișor]

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>:
> 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

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

[Stefan Beller]

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.

>     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

>> +       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.


>> +       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.

>> +       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?

>> +       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.

>> +       (
>> +               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

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

[Eddy Petrișor]

2018-04-04 21:36 GMT+03:00 Stefan Beller <sbeller@google.com>:
> 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.

>>     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>
[..]
>>> --- 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.

>
>>> +       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.

> 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.

>>> +       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.

>>> +       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.

>>> +       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

>>> +       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.

>>> +       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?

>>> +       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.

>
>>> +       (
>>> +               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

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

[Stefan Beller]

On Wed, Apr 4, 2018 at 1:37 PM, Eddy Petrișor <eddy.petrisor@gmail.com> wrote:

>> 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. :)

>> 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

> 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!

>
>>>> +       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)

> 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.

>
>>>> +       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.

>
>>>> +       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.

>
>>
>>>> +       (
>>>> +               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

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

[Eddy Petrișor]

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

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

[Eddy Petrișor]

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

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

[Eddy Petrișor]

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(-)

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

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

[Eddy Petrișor]

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(-)

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

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

[Eddy Petrișor]

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(-)

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

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

[Eddy Petrișor]

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(-)

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

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

[Eddy Petrișor]

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(-)

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

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

[Eddy Petrișor]

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(-)

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

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

[Eddy Petrișor]

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(-)

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

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

[Stefan Beller]

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

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

[Eddy Petrișor]

2018-04-19 2:53 GMT+03:00 Stefan Beller <sbeller@google.com>:
> 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.

> 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

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

[Eddy Petrișor]

2018-04-05 0:41 GMT+03:00 Stefan Beller <sbeller@google.com>:
> 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.

>>>>> +       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?

>>>>> +       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.

>> 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

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

[Stefan Beller]

Hi Eddy,

> 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