Re: [PATCH v4 3/6] submodule: Explicit local branch creation in module_clone

> "W. Trevor King" <wking@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...

> > +		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.

> > @@ -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.