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

mar., 27 mar. 2018, 02:07 Stefan Beller [off-list ref] a scris:

[snipped the cc list as well]

On Tue, Mar 6, 2018 at 12:06 PM Eddy Petrișor [off-list ref]
wrote:

quoted

Signed-off-by: Eddy Petrișor <redacted>
---

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.

quoted

There are projects such as llvm/clang which use several repositories, and

they

quoted

might be forked for providing support for various features such as adding

Redox

quoted

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.

quoted

Combined with the fact that when incorporating such a hierachy of

repositories

quoted

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.

quoted

Git submodule seems to be very stubborn and cloning master, although the
wrapper script and the gitmodules-helper could work together to clone

directly

quoted

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.

quoted

Another wrinkle is that when the commit is not the tip of the branch, the

depth

quoted

parameter should somehow be stored in the .gitmodules info, but any

change in

quoted

the submodule will break the supermodule submodule depth info sooner or

later,

quoted

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?

quoted

I tried digging into this section of the code and debugging with bashdb

to see

quoted

where --depth might fit, but I got stuck on the shell-to-helper

interaction and

quoted

the details of the submodule implementation, so I want to lay out this

first

quoted

patch as starting point for the discussion in the hope somebody else

picks it

quoted

up or can provide some inputs. I have the feeling there are multiple code

paths

quoted

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

quoted

any code duplication just because the operation is different.

quoted

This first patch is only trying to use a non-master branch, I have some

changes

quoted

for the --depth part, but I stopped working on it due to the "default

depth"

quoted

issue above.

quoted

Does any of this sound reasonable?
Is this patch idea usable or did I managed to touch the part of the code

that

quoted

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

quoted

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

quoted

                         if test -z "$nofetch"
                         then
+                               # non-default branch
+                               rbranch=$(git config -f .gitmodules

submodule.$sm_path.branch)

quoted

+

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.

quoted

                                 # Fetch remote before determining

tracking $sha1

quoted

-                               fetch_in_submodule "$sm_path" $depth ||
+                               fetch_in_submodule "$sm_path" $depth

$br_refspec ||

quoted

                                 die "$(eval_gettext "Unable to fetch in

submodule path '\$sm_path'")"

quoted

                         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