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

On Thu, Jan 16, 2014 at 11:18:00AM -0800, Junio C Hamano wrote:

"W. Trevor King" [off-list ref] writes:

quoted

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

quoted

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

quoted

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