git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: eddy.petrisor@gmail.com
Cc: git <git@vger.kernel.org>, Heiko Voigt <hvoigt@hvoigt.net>,
	Jacob Keller <jacob.keller@gmail.com>,
	Jonathan Nieder <jrnieder@gmail.com>
Subject: Re: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default
Date: Mon, 26 Mar 2018 23:06:54 +0000
Message-ID: <CAGZ79kb4Ea7t5j9XA0key1f99w5xRDwyRhMder1FMgdiZot3Tg@mail.gmail.com> (raw)
In-Reply-To: <1520366804-28233-1-git-send-email-eddy.petrisor@gmail.com>

[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

  parent reply index

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <1520366804-28233-1-git-send-email-eddy.petrisor@gmail.com>
2018-03-06 20:21 ` Jonathan Nieder
2018-03-08 19:29   ` Eddy Petrișor
2018-03-08 19:41     ` Eddy Petrișor
2018-03-16 21:33     ` Thomas Gummerer
2018-03-16 21:44       ` Eric Sunshine
     [not found]         ` <CAK0XTWcNySGgwgFgCPDnZ+m=2hfEgswHbJKYeu+LQfuQ9_=shQ@mail.gmail.com>
2018-03-17 19:11           ` Thomas Gummerer
2018-03-18  1:43             ` Eric Sunshine
2018-03-26 23:06 ` Stefan Beller [this message]
     [not found]   ` <CAK0XTWd7QGtVDwm8FDXejZfbgVH6-1NprGY0xxAnC33QH8aCCQ@mail.gmail.com>
2018-03-29 20:54     ` Fwd: " Eddy Petrișor
     [not found] ` <20180329225502.20112-1-eddy.petrisor@gmail.com>
2018-03-29 22:59   ` [RFC PATCH v2] " Eddy Petrișor
2018-04-03 22:20     ` [RFC PATCH v3 1/2] " Eddy Petrișor
2018-04-03 22:20     ` [RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule Eddy Petrișor
2018-04-03 22:26       ` Eddy Petrișor
2018-04-04 18:36         ` Stefan Beller
2018-04-04 20:37           ` Eddy Petrișor
2018-04-04 21:41             ` Stefan Beller
2018-04-18 22:35               ` [RFC PATCH v4 1/9] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default Eddy Petrișor
2018-04-18 23:53                 ` Stefan Beller
2018-04-19  5:43                   ` Eddy Petrișor
2018-04-18 22:35               ` [RFC PATCH v4 2/9] t7406: add test for non-default branch in submodule Eddy Petrișor
2018-04-18 22:35               ` [RFC PATCH v4 3/9] fixup:t7406: use test_commit instead of echo/add/commit as suggested by Stefan Beller Eddy Petrișor
2018-04-18 22:35               ` [RFC PATCH v4 4/9] fixup:t7404:use 'git -C' instead of cd .. && git Eddy Petrișor
2018-04-18 22:35               ` [RFC PATCH v4 5/9] fixup:t7406:cleanup chained submodules after test is done Eddy Petrișor
2018-04-18 22:35               ` [RFC PATCH v4 6/9] fixup:t7406:don't call init after add, is redundant Eddy Petrișor
2018-04-18 22:35               ` [RFC PATCH v4 7/9] fixup:t7406:supr5 commit is done before submodules chaining Eddy Petrișor
2018-04-18 22:35               ` [RFC PATCH v4 8/9] fixup:t7406:use super_w instead of the existing super Eddy Petrișor
2018-04-18 22:35               ` [RFC PATCH v4 9/9] fixup:t7406:change branches in submodules after the link is done Eddy Petrișor
2018-04-19  6:07               ` [RFC PATCH v3 2/2] t7406: add test for non-default branch in submodule Eddy Petrișor
2018-04-19 17:52                 ` Stefan Beller

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGZ79kb4Ea7t5j9XA0key1f99w5xRDwyRhMder1FMgdiZot3Tg@mail.gmail.com \
    --to=sbeller@google.com \
    --cc=eddy.petrisor@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=hvoigt@hvoigt.net \
    --cc=jacob.keller@gmail.com \
    --cc=jrnieder@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox