Re: [PATCH 3/5] checkout --to: no auto-detach if the ref is already checked out

[Michael J Gruber <git@drmicha.warpmail.net>]

Nguyễn Thái Ngọc Duy venit, vidit, dixit 23.07.2014 13:43:
> Give the user a choice in this case. If they want to detach, they can go
> with '--detach --to ...', or they could switch branch of the checkout
> that's holding the ref in question. Or they could just create a new
> branch with '-b xxx --to yyy'
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/config.txt |  3 ++
>  advice.c                 |  2 ++
>  advice.h                 |  1 +
>  builtin/checkout.c       | 73 ++++++++++++++++++++++++++++--------------------
>  t/t2025-checkout-to.sh   | 16 +++++------
>  5 files changed, 56 insertions(+), 39 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 57999fa..4a41202 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -201,6 +201,9 @@ advice.*::
>  	rmHints::
>  		In case of failure in the output of linkgit:git-rm[1],
>  		show directions on how to proceed from the current state.
> +	checkoutTo::
> +		In case of failure in the output of "git checkout --to",
> +		show directions on how to proceed from the current state.
>  --
>  
>  core.fileMode::
> diff --git a/advice.c b/advice.c
> index 9b42033..b1fb524 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -15,6 +15,7 @@ int advice_detached_head = 1;
>  int advice_set_upstream_failure = 1;
>  int advice_object_name_warning = 1;
>  int advice_rm_hints = 1;
> +int advice_checkout_to = 1;
>  
>  static struct {
>  	const char *name;
> @@ -35,6 +36,7 @@ static struct {
>  	{ "setupstreamfailure", &advice_set_upstream_failure },
>  	{ "objectnamewarning", &advice_object_name_warning },
>  	{ "rmhints", &advice_rm_hints },
> +	{ "checkoutto", &advice_checkout_to },
>  
>  	/* make this an alias for backward compatibility */
>  	{ "pushnonfastforward", &advice_push_update_rejected }
> diff --git a/advice.h b/advice.h
> index 5ecc6c1..a288219 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -18,6 +18,7 @@ extern int advice_detached_head;
>  extern int advice_set_upstream_failure;
>  extern int advice_object_name_warning;
>  extern int advice_rm_hints;
> +extern int advice_checkout_to;
>  
>  int git_default_advice_config(const char *var, const char *value);
>  __attribute__((format (printf, 1, 2)))
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index c83f476..d35245a 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1006,31 +1006,52 @@ static const char *unique_tracking_name(const char *name, unsigned char *sha1)
>  	return NULL;
>  }
>  
> -static int check_linked_checkout(struct branch_info *new,
> -				  const char *name, const char *path)
> +static void check_linked_checkout(struct branch_info *new, const char *id)
>  {
>  	struct strbuf sb = STRBUF_INIT;
> +	struct strbuf path = STRBUF_INIT;
> +	struct strbuf gitdir = STRBUF_INIT;
>  	const char *start, *end;
> -	if (strbuf_read_file(&sb, path, 0) < 0 ||
> -	    !skip_prefix(sb.buf, "ref:", &start)) {
> -		strbuf_release(&sb);
> -		return 0;
> -	}
>  
> +	if (id)
> +		strbuf_addf(&path, "%s/repos/%s/HEAD", get_git_common_dir(), id);
> +	else
> +		strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
> +
> +	if (strbuf_read_file(&sb, path.buf, 0) <= 0 ||
> +	    !skip_prefix(sb.buf, "ref:", &start))
> +		goto done;
>  	while (isspace(*start))
>  		start++;
>  	end = start;
>  	while (*end && !isspace(*end))
>  		end++;
> -	if (!strncmp(start, new->path, end - start) &&
> -	    new->path[end - start] == '\0') {
> -		strbuf_release(&sb);
> -		new->path = NULL; /* detach */
> -		new->checkout = xstrdup(name); /* reason */
> -		return 1;
> -	}
> +	if (strncmp(start, new->path, end - start) ||
> +	    new->path[end - start] != '\0')
> +		goto done;
> +	if (id) {
> +		strbuf_reset(&path);
> +		strbuf_addf(&path, "%s/repos/%s/gitdir",
> +			    get_git_common_dir(), id);
> +		if (strbuf_read_file(&gitdir, path.buf, 0) <= 0)
> +			goto done;
> +		while (gitdir.len && (gitdir.buf[gitdir.len - 1] == '\n' ||
> +				      gitdir.buf[gitdir.len - 1] == '\r'))
> +			gitdir.buf[--gitdir.len] = '\0';
> +	} else
> +		strbuf_addstr(&gitdir, get_git_common_dir());
> +	if (advice_checkout_to)
> +		die(_("%s is already checked out at %s.\n"
> +		      "Either use --detach or -b together with --to "
> +		      "or switch branch in the the other checkout."),

"or switch to a different branch in the other checkout". But maybe we
can be even more helpful, like:

"%s is already checked out at %s.\n"
"Either checkout the detached head of branch %s using\n"
"    git checkout --detach --to %s %s\n"
"or checkout a new branch based off %s using\n"
"    git checkout --to %s -b %s newbranch %s\n"
"or switch to a different branch in the other checkout."

if we can figure out the appropriate arguments at this point in the code.

> +		    new->path, gitdir.buf);
> +	else
> +		die(_("%s is already checked out at %s."),
> +		    new->path, gitdir.buf);
> +done:
> +	strbuf_release(&path);
>  	strbuf_release(&sb);
> -	return 0;
> +	strbuf_release(&gitdir);
>  }
>  
>  static void check_linked_checkouts(struct branch_info *new)
> @@ -1045,27 +1066,17 @@ static void check_linked_checkouts(struct branch_info *new)
>  		return;
>  	}
>  
> -	strbuf_reset(&path);
> -	strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
>  	/*
>  	 * $GIT_COMMON_DIR/HEAD is practically outside
>  	 * $GIT_DIR so resolve_ref_unsafe() won't work (it
>  	 * uses git_path). Parse the ref ourselves.
>  	 */
> -	if (check_linked_checkout(new, "", path.buf)) {
> -		strbuf_release(&path);
> -		closedir(dir);
> -		return;
> -	}
> +	check_linked_checkout(new, NULL);
>  
>  	while ((d = readdir(dir)) != NULL) {
>  		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
>  			continue;
> -		strbuf_reset(&path);
> -		strbuf_addf(&path, "%s/repos/%s/HEAD",
> -			    get_git_common_dir(), d->d_name);
> -		if (check_linked_checkout(new, d->d_name, path.buf))
> -			break;
> +		check_linked_checkout(new, d->d_name);
>  	}
>  	strbuf_release(&path);
>  	closedir(dir);
> @@ -1076,7 +1087,8 @@ static int parse_branchname_arg(int argc, const char **argv,
>  				struct branch_info *new,
>  				struct tree **source_tree,
>  				unsigned char rev[20],
> -				const char **new_branch)
> +				const char **new_branch,
> +				int force_detach)
>  {
>  	int argcount = 0;
>  	unsigned char branch_rev[20];
> @@ -1198,7 +1210,7 @@ static int parse_branchname_arg(int argc, const char **argv,
>  	else
>  		new->path = NULL; /* not an existing branch */
>  
> -	if (new->path) {
> +	if (new->path && !force_detach && !*new_branch) {
>  		unsigned char sha1[20];
>  		int flag;
>  		char *head_ref = resolve_refdup("HEAD", sha1, 0, &flag);
> @@ -1416,7 +1428,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  			!opts.new_branch;
>  		int n = parse_branchname_arg(argc, argv, dwim_ok,
>  					     &new, &opts.source_tree,
> -					     rev, &opts.new_branch);
> +					     rev, &opts.new_branch,
> +					     opts.force_detach);
>  		argv += n;
>  		argc -= n;
>  	}
> diff --git a/t/t2025-checkout-to.sh b/t/t2025-checkout-to.sh
> index b0d97a0..c6601a4 100755
> --- a/t/t2025-checkout-to.sh
> +++ b/t/t2025-checkout-to.sh
> @@ -14,7 +14,7 @@ test_expect_success 'checkout --to not updating paths' '
>  
>  test_expect_success 'checkout --to a new worktree' '
>  	git rev-parse HEAD >expect &&
> -	git checkout --to here master &&
> +	git checkout --detach --to here master &&
>  	(
>  		cd here &&
>  		test_cmp ../init.t init.t &&
> @@ -28,7 +28,7 @@ test_expect_success 'checkout --to a new worktree' '
>  test_expect_success 'checkout --to from a linked checkout' '
>  	(
>  		cd here &&
> -		git checkout --to nested-here master
> +		git checkout --detach --to nested-here master
>  		cd nested-here &&
>  		git fsck
>  	)
> @@ -46,19 +46,17 @@ test_expect_success 'checkout --to a new worktree creating new branch' '
>  	)
>  '
>  
> -test_expect_success 'detach if the same branch is already checked out' '
> +test_expect_success 'die the same branch is already checked out' '
>  	(
>  		cd here &&
> -		git checkout newmaster &&
> -		test_must_fail git symbolic-ref HEAD
> +		test_must_fail git checkout newmaster
>  	)
>  '
>  
> -test_expect_success 'not detach on re-checking out current branch' '
> +test_expect_success 'not die on re-checking out current branch' '
>  	(
>  		cd there &&
> -		git checkout newmaster &&
> -		git symbolic-ref HEAD
> +		git checkout newmaster
>  	)
>  '
>  
> @@ -66,7 +64,7 @@ test_expect_success 'checkout --to from a bare repo' '
>  	(
>  		git clone --bare . bare &&
>  		cd bare &&
> -		git checkout --to ../there2 master
> +		git checkout --to ../there2 -b bare-master master
>  	)
>  '
>  
>