On Thu, Jan 16, 2014 at 11:18:00AM -0800, Junio C Hamano wrote:
> "W. Trevor King" <wk...@tremily.us> writes:
> > @@ -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...
That's copied out of the old cmd_add code ;). I don't have ash
intalled to actually confirm the comment.
> > + 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.
This is immediately post-non-checkout-clone. There are no local
branches to clobber yet.
> > @@ -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.
Ok. Will add in v5.
Cheers,
Trevor
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
signature.asc
Description: OpenPGP digital signature
