Re: [PATCH v4 3/6] submodule: Explicit local branch creation in module_clone
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:59:40
"W. Trevor King" [off-list ref] writes:
quoted hunk
The previous code only checked out branches 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 'reset' to preserve branches setup during module_clone. With this change, folks cloning submodules for the first time via: $ git submodule update ... will get a local branch instead of a detached HEAD, unless they are using the default checkout-mode updates. This is a change from the previous situation where cmd_update always used checkout-mode logic (regardless of the requested update mode) for updates that triggered an initial clone, which always resulted in a detached HEAD. This commit does not change the logic for updates after the initial clone, which will continue to create detached HEADs for checkout-mode updates, and integrate remote work with the local HEAD (detached or not) in other modes. The motivation for the change is that developers doing local work inside the submodule are likely to select a non-checkout-mode for updates so their local work is integrated with upstream work. Developers who are not doing local submodule work stick with checkout-mode updates so any apparently local work is blown away during updates. For example, if upstream rolls back the remote branch or gitlinked commit to an earlier version, the checkout-mode developer wants their old submodule checkout to be rolled back as well, instead of getting a no-op merge/rebase with the rolled-back reference. By using the update mode to distinguish submodule developers from black-box submodule consumers, we can setup local branches for the developers who will want local branches, and stick with detached HEADs for the developers that don't care. Signed-off-by: W. Trevor King <redacted> --- git-submodule.sh | 53 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 17 deletions(-)diff --git a/git-submodule.sh b/git-submodule.sh index 68dcbe1..4a09951 100755 --- a/git-submodule.sh +++ b/git-submodule.sh@@ -246,6 +246,9 @@ module_name() # $3 = URL to clone # $4 = reference repository to reuse (empty for independent) # $5 = depth argument for shallow clones (empty for deep) +# $6 = (remote-tracking) starting point for the local branch (empty for HEAD) +# $7 = local branch to create (empty for a detached HEAD, unless $6 is +# also empty, in which case the local branch is left unchanged) # # Prior to calling, cmd_update checks that a possibly existing # path is not a git repository.@@ -259,6 +262,8 @@ module_clone() url=$3 reference="$4" depth="$5" + start_point="$6" + local_branch="$7" quiet= if test -n "$GIT_QUIET" then@@ -312,7 +317,16 @@ 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" && + # ash fails to wordsplit ${local_branch:+-B "$local_branch"...}
Interesting...
+ case "$local_branch" in
+ '') git checkout -f -q ${start_point:+"$start_point"} ;;
+ ?*) git checkout -f -q -B "$local_branch" ${start_point:+"$start_point"} ;;
+ esac
I am wondering if it makes more sense if you did this instead:
git checkout -f -q ${start_point:+"$start_point"} &&
if test -n "$local_branch"
then
git checkout -B "$local_branch" HEAD
fi
The optional "re-attaching to the local_branch" done with the second
"checkout" would be a non-destructive no-op to the working tree and
to the index, but it does distinguish between creating the branch
anew and resetting the existing branch in its output (note that
there is no "-q" to squelch it). By doing it this way, when we
later teach "git branch -f" and "git checkout -B" to report more
about what commits have been lost by such a resetting, you will get
the safety for free if you made the switching with "-B" run without
"-q" here.
quoted hunk
+ ) || die "$(eval_gettext "Unable to setup cloned submodule '\$sm_path'")" } isnumber()@@ -475,16 +489,14 @@ 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'")" + if test -n "$branch" + then + start_point="origin/$branch" + local_branch="$branch" + else + start_point="" + fi
I'd feel safer if the "else" clause explicitly cleared $local_branch by assigning an empty string to it; it would make it a lot clearer that "when $branch is an empty string here, we do not want to trigger the new codepath to run checkout with "-B $local_branch" in module_clone" is what you mean.
+ module_clone "$sm_path" "$sm_name" "$realrepo" "$reference" "$depth" "$start_point" "$local_branch" || exit