From: Heiko Voigt <hvoigt@hvoigt•net>
To: "W. Trevor King" <wking@tremily•us>
Cc: Git <git@vger•kernel.org>, Francesco Pretto <ceztko@gmail•com>,
Junio C Hamano <gitster@pobox•com>,
Jens Lehmann <Jens.Lehmann@web•de>
Subject: Re: [RFC v2] submodule: Respect requested branch on all clones
Date: Sun, 5 Jan 2014 20:48:50 +0100 [thread overview]
Message-ID: <20140105194850.GA2994@book.hvoigt.net> (raw)
In-Reply-To: <d0de817dfc687fd943349c9d3e1d410161a0f01e.1388938473.git.wking@tremily.us>
On Sun, Jan 05, 2014 at 08:17:00AM -0800, W. Trevor King wrote:
> From: "W. Trevor King" <wking@tremily•us>
>
> The previous code only checked out the requested branch in cmd_add.
> This commit moves the branch-checkout logic into module_clone, where
> it can be shared by cmd_add and cmd_update. I also update the initial
> checkout command to use 'rebase' to preserve branches setup during
> module_clone.
>
> Signed-off-by: W. Trevor King <wking@tremily•us>
> ---
> Changes since v1:
> * Fix a 'reqested' -> 'requested' typo in the subject/summary.
> * Restore a post-clone 'git checkout -f -q' for the empty-branch case
> in module_clone().
> * Distinguish between $branch (which defaults to 'master') and a new
> $config_branch (which defaults to empty) in cmd_update
>
> After these fixes, all the existing submodule tests pass. If we want
> to merge this, we'd still want new tests that demonstrate the new
> functionality.
I think this patch is going in the right direction. Making add() and
update() do the same is the right thing to do.
I would still like a complete description of Francesco's use case
though. Francesco: Could you give us a short description about what
exactly is your use-case? And please ignore all technical details how we
are going to implement this. I would like to know how, in an ideal
world, you would expect git to behave. Do you really only care about
git submodule add
and the *initial*
git submodule update
?
What happens if a developer already has the submodule and wants to work
on a feature?
> On Sun, Jan 05, 2014 at 04:53:12AM +0100, Francesco Pretto wrote:
> > If I understand it correctly, looking at your intervention in
> > module_clone and cmd_update, when "submodule.<module>.branch" is set
> > during "update" the resulting first clone will always be a branch
> > checkout (cause $branch is filled with "branch" property).
>
> That's correct.
>
> > I believe this will break a lot of tests,
>
> Thanks for prompting me to run the tests. This v2 now passes all of
> the current submodule tests, and the functionality actually matches my
> earlier descriptions of it ;).
>
> > as the the documentation says that in this configuration the HEAD
> > should be detached.
>
> The current Documentation/git-submodule.txt has:
>
> update::
> Update the registered submodules, i.e. clone missing submodules
> and checkout the commit specified in the index of the containing
> repository. This will make the submodules HEAD be detached unless
> `--rebase` or `--merge` is specified or the key
> `submodule.$name.update` is set to `rebase`, `merge` or `none`.
>
> It's not clear if this refers to the initial-clone update, future
> post-clone updates, or both. Ideally, the behavior should be the
> same, but in the initial-clone case we don't have an existing
> checked-out branch to work with.
I do not think that its actually important to end up with a detached
HEAD. The documentation just states it because in most cases there is no
other option. But I do not think anything will break if a branch points
to the exact sha1 we would checkout and we checkout the branch instead.
> > Also it could break some users that rely on the current behavior.
>
> The current code always has a detached HEAD after an initial-clone
> update, regardless of submodule.<name>.update, which doesn't match
> those docs either. Adding a check to only checkout
> submodule.<name>.branch if submodule.<name>.update was 'rebase',
> 'merge', or 'none' would be easy, but I don't think that makes much
> sense. I can't see any reason for folks who specify
> submodule.<name>.branch to prefer a detached HEAD over a local branch
> matching the remote branch's name. If they prefer checkout updates,
> the first such update will return them to a detached HEAD. If they
> prefer merge/rebase updates, future updates will keep them on the same
> branch. All my commit does is setup that initial branch for folks who
> get the submodule via 'update', in the same way it's currently setup
> for folks who get the submodule via 'add'.
I like your goal of putting this logic into one place. But a few things
still could be improved IMO.
> git-submodule.sh | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 2979197..167d4fa 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -253,6 +253,7 @@ module_clone()
> url=$3
> reference="$4"
> depth="$5"
> + branch="$6"
> quiet=
> if test -n "$GIT_QUIET"
> then
> @@ -306,7 +307,15 @@ module_clone()
> echo "gitdir: $rel/$a" >"$sm_path/.git"
>
> rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
> - (clear_local_git_env; cd "$sm_path" && GIT_WORK_TREE=. git config core.worktree "$rel/$b")
> + (
> + clear_local_git_env
> + cd "$sm_path" &&
> + GIT_WORK_TREE=. git config core.worktree "$rel/$b" &&
> + case "$branch" in
> + '') git checkout -f -q ;;
> + ?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
> + esac
> + ) || die "$(eval_gettext "Unable to setup cloned submodule '\$sm_path'")"
> }
>
> isnumber()
> @@ -469,16 +478,7 @@ Use -f if you really want to add it." >&2
> echo "$(eval_gettext "Reactivating local git directory for submodule '\$sm_name'.")"
> fi
> fi
> - module_clone "$sm_path" "$sm_name" "$realrepo" "$reference" "$depth" || exit
> - (
> - clear_local_git_env
> - cd "$sm_path" &&
> - # ash fails to wordsplit ${branch:+-b "$branch"...}
> - case "$branch" in
> - '') git checkout -f -q ;;
> - ?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
> - esac
> - ) || die "$(eval_gettext "Unable to checkout submodule '\$sm_path'")"
> + module_clone "$sm_path" "$sm_name" "$realrepo" "$reference" "$depth" "$branch" || exit
> fi
> git config submodule."$sm_name".url "$realrepo"
>
> @@ -787,7 +787,8 @@ cmd_update()
> fi
> name=$(module_name "$sm_path") || exit
> url=$(git config submodule."$name".url)
> - branch=$(get_submodule_config "$name" branch master)
> + config_branch=$(get_submodule_config "$name" branch)
> + branch="${config_branch:-master}"
> if ! test -z "$update"
> then
> update_module=$update
> @@ -815,7 +816,7 @@ Maybe you want to use 'update --init'?")"
>
> if ! test -d "$sm_path"/.git -o -f "$sm_path"/.git
> then
> - module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit
> + module_clone "$sm_path" "$name" "$url" "$reference" "$depth" "$config_branch" || exit
In the simple case (update=checkout, no branch specified) with the new
checkout branch stuff in module_clone() this code here ends up calling
checkout twice. First for master and then here later with the sha1.
This feels a little bit double. I would prefer if we skip the checkout
in module_clone() if its not necessary.
How about we move the whole "what to checkout"-decision into one place
instead of having it in update() and moving it from add() into
module_clone() ?
Previously there was not much in add() regarding checkout but since it
seems to grow (and now parts need to be shared between add() and
update()). I would like it if we could move that code into one central
location.
> cloned_modules="$cloned_modules;$name"
> subsha1=
> else
> @@ -861,7 +862,12 @@ Maybe you want to use 'update --init'?")"
> case ";$cloned_modules;" in
> *";$name;"*)
> # then there is no local change to integrate
> - update_module= ;;
> + if test -n "$config_branch"; then
> + update_module="!git reset --hard -q"
If we get here the checkout has already been done. Shouldn't this rather
specify a noop. I.E. like
update_module="!true"
?
Cheers Heiko
next prev parent reply other threads:[~2014-01-05 19:49 UTC|newest]
Thread overview: 144+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-30 1:49 [PATCH/RFC] Introduce git submodule add|update --attach Francesco Pretto
2013-12-31 20:05 ` Phil Hord
2014-01-02 18:48 ` Francesco Pretto
2014-01-13 17:31 ` Junio C Hamano
2014-01-02 20:07 ` Junio C Hamano
2014-01-02 23:42 ` Francesco Pretto
2014-01-03 0:26 ` Francesco Pretto
2014-01-03 8:49 ` Francesco Pretto
2014-01-03 18:06 ` [PATCH] submodule: Respect reqested branch on all clones W. Trevor King
2014-01-04 22:09 ` Heiko Voigt
2014-01-04 22:54 ` W. Trevor King
2014-01-05 0:39 ` Heiko Voigt
2014-01-05 1:08 ` W. Trevor King
2014-01-05 3:53 ` Francesco Pretto
2014-01-05 16:17 ` [RFC v2] submodule: Respect requested " W. Trevor King
2014-01-05 19:48 ` Heiko Voigt [this message]
2014-01-05 21:24 ` W. Trevor King
2014-01-05 22:57 ` Heiko Voigt
2014-01-05 23:39 ` W. Trevor King
2014-01-06 0:33 ` W. Trevor King
2014-01-06 1:12 ` W. Trevor King
2014-01-06 16:02 ` Heiko Voigt
2014-01-06 23:10 ` Francesco Pretto
2014-01-06 23:32 ` Francesco Pretto
2014-01-07 18:27 ` Junio C Hamano
2014-01-07 19:19 ` Francesco Pretto
2014-01-07 19:45 ` W. Trevor King
2014-01-07 19:48 ` Francesco Pretto
2014-01-07 21:37 ` W. Trevor King
2014-01-07 21:51 ` Francesco Pretto
2014-01-07 22:38 ` Heiko Voigt
2014-01-08 0:17 ` Francesco Pretto
2014-01-08 1:05 ` W. Trevor King
2014-01-08 2:12 ` Francesco Pretto
2014-01-08 23:07 ` Francesco Pretto
2014-01-09 0:03 ` W. Trevor King
2014-01-09 1:09 ` Francesco Pretto
2014-01-09 2:22 ` W. Trevor King
2014-01-09 8:31 ` Jens Lehmann
2014-01-09 17:32 ` W. Trevor King
2014-01-09 19:23 ` Jens Lehmann
2014-01-09 19:55 ` W. Trevor King
2014-01-09 21:40 ` Jens Lehmann
2014-01-09 22:18 ` W. Trevor King
2014-01-14 10:24 ` Heiko Voigt
2014-01-14 16:57 ` W. Trevor King
2014-01-14 20:58 ` Heiko Voigt
2014-01-14 21:42 ` W. Trevor King
2014-01-14 22:19 ` Heiko Voigt
2014-01-14 22:39 ` W. Trevor King
2014-01-14 21:46 ` Re: " Heiko Voigt
2014-01-14 22:22 ` W. Trevor King
2014-01-14 22:42 ` Heiko Voigt
2014-01-15 0:02 ` Francesco Pretto
2014-01-16 4:09 ` [PATCH v4 0/6] submodule: Local branch creation in module_clone W. Trevor King
2014-01-16 4:10 ` [PATCH v4 1/6] submodule: Make 'checkout' update_module explicit W. Trevor King
2014-01-16 18:46 ` Junio C Hamano
2014-01-16 19:22 ` W. Trevor King
2014-01-16 20:07 ` Francesco Pretto
2014-01-16 20:19 ` W. Trevor King
2014-01-16 4:10 ` [PATCH v4 2/6] submodule: Document module_clone arguments in comments W. Trevor King
2014-01-16 4:10 ` [PATCH v4 3/6] submodule: Explicit local branch creation in module_clone W. Trevor King
2014-01-16 19:18 ` Junio C Hamano
2014-01-16 19:29 ` W. Trevor King
2014-01-16 19:43 ` Junio C Hamano
2014-01-16 21:12 ` W. Trevor King
2014-01-16 4:10 ` [PATCH v4 4/6] t7406: Just-cloned checkouts update to the gitlinked hash with 'reset' W. Trevor King
2014-01-16 19:22 ` Junio C Hamano
2014-01-16 19:32 ` W. Trevor King
2014-01-16 20:24 ` Junio C Hamano
2014-01-16 4:10 ` [PATCH v4 5/6] t7406: Add explicit tests for head attachement after cloning updates W. Trevor King
2014-01-16 4:10 ` [PATCH v4 6/6] Documentation: Describe 'submodule update' modes in detail W. Trevor King
2014-01-16 20:21 ` Junio C Hamano
2014-01-16 20:55 ` W. Trevor King
2014-01-16 21:02 ` John Keeping
2014-01-16 21:16 ` W. Trevor King
2014-01-16 21:55 ` Junio C Hamano
2014-01-17 2:37 ` W. Trevor King
2014-01-26 20:45 ` [PATCH v5 0/4] submodule: Local branch creation in module_clone W. Trevor King
2014-01-26 20:45 ` [PATCH v5 1/4] submodule: Make 'checkout' update_module explicit W. Trevor King
2014-01-27 1:32 ` Eric Sunshine
2014-01-27 1:59 ` W. Trevor King
2014-01-26 20:45 ` [PATCH v5 2/4] submodule: Document module_clone arguments in comments W. Trevor King
2014-01-26 20:45 ` [PATCH v5 3/4] submodule: Explicit local branch creation in module_clone W. Trevor King
2014-01-26 20:45 ` [PATCH v5 4/4] Documentation: Describe 'submodule update --remote' use case W. Trevor King
2014-01-16 22:18 ` [PATCH v4 6/6] Documentation: Describe 'submodule update' modes in detail Philip Oakley
2014-01-16 22:35 ` W. Trevor King
2014-01-08 23:54 ` Re: [RFC v2] submodule: Respect requested branch on all clones Francesco Pretto
2014-01-09 0:23 ` W. Trevor King
2014-01-07 19:52 ` Francesco Pretto
2014-01-06 15:47 ` Re: " Heiko Voigt
2014-01-06 17:22 ` W. Trevor King
2014-01-05 21:27 ` Francesco Pretto
2014-01-05 21:47 ` W. Trevor King
2014-01-05 22:01 ` W. Trevor King
2014-01-06 14:47 ` Heiko Voigt
2014-01-06 16:56 ` Junio C Hamano
2014-01-06 17:37 ` W. Trevor King
2014-01-06 21:32 ` Junio C Hamano
2014-01-07 0:55 ` Francesco Pretto
2014-01-07 18:15 ` Junio C Hamano
2014-01-07 18:47 ` W. Trevor King
2014-01-07 19:21 ` Junio C Hamano
2014-01-07 19:50 ` W. Trevor King
2014-01-05 2:50 ` [PATCH 1/2] git-submodule.sh: Support 'checkout' as a valid update command Francesco Pretto
2014-01-05 2:50 ` [PATCH 2/2] Introduce git submodule attached update Francesco Pretto
2014-01-05 19:55 ` Francesco Pretto
2014-01-05 20:33 ` Heiko Voigt
2014-01-05 21:46 ` Francesco Pretto
2014-01-06 14:06 ` Heiko Voigt
2014-01-06 17:47 ` Francesco Pretto
2014-01-06 19:21 ` David Engster
2014-01-07 19:27 ` W. Trevor King
2014-01-07 4:10 ` W. Trevor King
2014-01-07 22:36 ` Preferred local submodule branches (was: Introduce git submodule attached update) W. Trevor King
2014-01-07 23:52 ` W. Trevor King
2014-01-08 3:47 ` Preferred local submodule branches W. Trevor King
2014-01-08 4:06 ` W. Trevor King
2014-01-09 6:17 ` [RFC v3 0/4] " W. Trevor King
2014-01-09 6:17 ` [RFC v3 1/4] submodule: Add helpers for configurable local branches W. Trevor King
2014-01-09 6:17 ` [RFC v3 2/4] submodule: Teach 'update' to preserve " W. Trevor King
2014-01-09 6:17 ` [RFC v3 3/4] submodule: Teach 'add' about a configurable local-branch W. Trevor King
2014-01-15 0:18 ` Francesco Pretto
2014-01-15 1:02 ` W. Trevor King
2014-01-09 6:17 ` [RFC v3 4/4] submodule: Add a new 'checkout' command W. Trevor King
2014-01-12 1:08 ` Tight submodule bindings (was: Preferred local submodule branches) W. Trevor King
2014-01-13 19:37 ` Tight submodule bindings Jens Lehmann
2014-01-13 20:07 ` W. Trevor King
2014-01-13 22:13 ` Junio C Hamano
2014-01-14 2:44 ` W. Trevor King
2014-01-07 22:51 ` Re: [PATCH 2/2] Introduce git submodule attached update Heiko Voigt
2014-01-07 23:14 ` W. Trevor King
2014-01-07 18:56 ` Junio C Hamano
2014-01-07 19:44 ` Francesco Pretto
2014-01-07 19:07 ` Junio C Hamano
2014-01-07 19:25 ` Francesco Pretto
2014-01-05 23:22 ` Francesco Pretto
2014-01-06 14:18 ` Heiko Voigt
2014-01-06 15:58 ` W. Trevor King
2014-01-05 20:20 ` [PATCH 1/2] git-submodule.sh: Support 'checkout' as a valid update command Heiko Voigt
2014-01-05 20:44 ` W. Trevor King
2014-01-06 16:20 ` Junio C Hamano
2014-01-06 17:42 ` Junio C Hamano
2014-01-06 17:52 ` Francesco Pretto
Reply instructions:
You may reply publicly 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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140105194850.GA2994@book.hvoigt.net \
--to=hvoigt@hvoigt$(echo .)net \
--cc=Jens.Lehmann@web$(echo .)de \
--cc=ceztko@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=wking@tremily$(echo .)us \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox