[PATCH 0/5] nd/multiple-work-trees follow-ups

[PATCH 0/5] nd/multiple-work-trees follow-ups

[Nguyễn Thái Ngọc Duy]

The series has entered 'next' so I can't replace patches any more.
Besides the brown paper bag fixes, checkout now rejects if a branch is
already checked out elsewhere.

Nguyễn Thái Ngọc Duy (5):
  gitrepository-layout.txt: s/ignored/ignored if/
  prune --repos: fix uninitialized access
  checkout --to: no auto-detach if the ref is already checked out
  checkout --to: fix dangling pointers in remove_junk()
  environment.c: fix incorrect git_graft_file initialization

 Documentation/config.txt               |  3 ++
 Documentation/gitrepository-layout.txt |  6 +--
 advice.c                               |  2 +
 advice.h                               |  1 +
 builtin/checkout.c                     | 88 ++++++++++++++++++++--------------
 builtin/prune.c                        | 16 +++----
 environment.c                          |  2 +-
 t/t0060-path-utils.sh                  |  1 +
 t/t2025-checkout-to.sh                 | 40 ++++++++++++----
 t/t2026-prune-linked-checkouts.sh      |  2 +-
 10 files changed, 102 insertions(+), 59 deletions(-)

-- 
1.9.1.346.ga2b5940

[PATCH 1/5] gitrepository-layout.txt: s/ignored/ignored if/

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/gitrepository-layout.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/gitrepository-layout.txt b/Documentation/gitrepository-layout.txt
index bed4f1a..6bd82af 100644
--- a/Documentation/gitrepository-layout.txt
+++ b/Documentation/gitrepository-layout.txt
@@ -139,7 +139,7 @@ is often called 'detached HEAD.'  See linkgit:git-checkout[1]
 for details.
 
 config::
-	Repository specific configuration file. This file is ignored
+	Repository specific configuration file. This file is ignored if
 	$GIT_COMMON_DIR is set and "$GIT_COMMON_DIR/config" will be
 	used instead.
 
@@ -220,7 +220,7 @@ remotes::
 logs::
 	Records of changes made to refs are stored in this
 	directory.  See linkgit:git-update-ref[1]
-	for more information. This directory is ignored
+	for more information. This directory is ignored if
 	$GIT_COMMON_DIR is set and "$GIT_COMMON_DIR/logs" will be used
 	instead.
 
@@ -252,7 +252,7 @@ modules::
 repos::
 	Contains worktree specific information of linked
 	checkouts. Each subdirectory contains the worktree-related
-	part of a linked checkout. This directory is ignored
+	part of a linked checkout. This directory is ignored if
 	$GIT_COMMON_DIR is set and "$GIT_COMMON_DIR/repos" will be
 	used instead.
 
-- 
1.9.1.346.ga2b5940

[PATCH 2/5] prune --repos: fix uninitialized access

[Nguyễn Thái Ngọc Duy]

There's a code path in prune_repo_dir() that does not initialize 'st'
buffer, which is checked by the caller, prune_repos_dir(). Instead
of leaking some prune logic out to prune_repos_dir(), move 'st' into
prune_repo_dir().

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/prune.c                   | 16 ++++++----------
 t/t2026-prune-linked-checkouts.sh |  2 +-
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/builtin/prune.c b/builtin/prune.c
index 28b7adf..e72c391 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -112,8 +112,9 @@ static void prune_object_dir(const char *path)
 	}
 }
 
-static int prune_repo_dir(const char *id, struct stat *st, struct strbuf *reason)
+static int prune_repo_dir(const char *id, struct strbuf *reason)
 {
+	struct stat st;
 	char *path;
 	int fd, len;
 
@@ -123,26 +124,23 @@ static int prune_repo_dir(const char *id, struct stat *st, struct strbuf *reason
 	}
 	if (file_exists(git_path("repos/%s/locked", id)))
 		return 0;
-	if (stat(git_path("repos/%s/gitdir", id), st)) {
-		st->st_mtime = expire;
+	if (stat(git_path("repos/%s/gitdir", id), &st)) {
 		strbuf_addf(reason, _("Removing repos/%s: gitdir file does not exist"), id);
 		return 1;
 	}
 	fd = open(git_path("repos/%s/gitdir", id), O_RDONLY);
 	if (fd < 0) {
-		st->st_mtime = expire;
 		strbuf_addf(reason, _("Removing repos/%s: unable to read gitdir file (%s)"),
 			    id, strerror(errno));
 		return 1;
 	}
-	len = st->st_size;
+	len = st.st_size;
 	path = xmalloc(len + 1);
 	read_in_full(fd, path, len);
 	close(fd);
 	while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
 		len--;
 	if (!len) {
-		st->st_mtime = expire;
 		strbuf_addf(reason, _("Removing repos/%s: invalid gitdir file"), id);
 		free(path);
 		return 1;
@@ -162,7 +160,7 @@ static int prune_repo_dir(const char *id, struct stat *st, struct strbuf *reason
 		return 1;
 	}
 	free(path);
-	return 0;
+	return st.st_mtime <= expire;
 }
 
 static void prune_repos_dir(void)
@@ -172,15 +170,13 @@ static void prune_repos_dir(void)
 	DIR *dir = opendir(git_path("repos"));
 	struct dirent *d;
 	int ret;
-	struct stat st;
 	if (!dir)
 		return;
 	while ((d = readdir(dir)) != NULL) {
 		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
 			continue;
 		strbuf_reset(&reason);
-		if (!prune_repo_dir(d->d_name, &st, &reason) ||
-		    st.st_mtime > expire)
+		if (!prune_repo_dir(d->d_name, &reason))
 			continue;
 		if (show_only || verbose)
 			printf("%s\n", reason.buf);
diff --git a/t/t2026-prune-linked-checkouts.sh b/t/t2026-prune-linked-checkouts.sh
index 4ccfa4e..79d84cb 100755
--- a/t/t2026-prune-linked-checkouts.sh
+++ b/t/t2026-prune-linked-checkouts.sh
@@ -77,7 +77,7 @@ test_expect_success 'not prune recent checkouts' '
 	mkdir zz &&
 	mkdir -p .git/repos/jlm &&
 	echo "$TRASH_DIRECTORY"/zz >.git/repos/jlm/gitdir &&
-	git prune --repos --verbose &&
+	git prune --repos --verbose --expire=2.days.ago &&
 	test -d .git/repos/jlm
 '
 
-- 
1.9.1.346.ga2b5940

Re: [PATCH 2/5] prune --repos: fix uninitialized access

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> There's a code path in prune_repo_dir() that does not initialize 'st'
> buffer, which is checked by the caller, prune_repos_dir(). Instead
> of leaking some prune logic out to prune_repos_dir(), move 'st' into
> prune_repo_dir().
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

The return value from the function used to be "should .git/repos/$x
be removed if it is stale enough?" and now it is just "should it be
removed?", so the change makes sense.

It does not explain what the change to the test is about, though.

Thanks.

>  builtin/prune.c                   | 16 ++++++----------
>  t/t2026-prune-linked-checkouts.sh |  2 +-
>  2 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/prune.c b/builtin/prune.c
> index 28b7adf..e72c391 100644
> --- a/builtin/prune.c
> +++ b/builtin/prune.c
> @@ -112,8 +112,9 @@ static void prune_object_dir(const char *path)
>  	}
>  }
>  
> -static int prune_repo_dir(const char *id, struct stat *st, struct strbuf *reason)
> +static int prune_repo_dir(const char *id, struct strbuf *reason)
>  {
> +	struct stat st;
>  	char *path;
>  	int fd, len;
>  
> @@ -123,26 +124,23 @@ static int prune_repo_dir(const char *id, struct stat *st, struct strbuf *reason
>  	}
>  	if (file_exists(git_path("repos/%s/locked", id)))
>  		return 0;
> -	if (stat(git_path("repos/%s/gitdir", id), st)) {
> -		st->st_mtime = expire;
> +	if (stat(git_path("repos/%s/gitdir", id), &st)) {
>  		strbuf_addf(reason, _("Removing repos/%s: gitdir file does not exist"), id);
>  		return 1;
>  	}
>  	fd = open(git_path("repos/%s/gitdir", id), O_RDONLY);
>  	if (fd < 0) {
> -		st->st_mtime = expire;
>  		strbuf_addf(reason, _("Removing repos/%s: unable to read gitdir file (%s)"),
>  			    id, strerror(errno));
>  		return 1;
>  	}
> -	len = st->st_size;
> +	len = st.st_size;
>  	path = xmalloc(len + 1);
>  	read_in_full(fd, path, len);
>  	close(fd);
>  	while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
>  		len--;
>  	if (!len) {
> -		st->st_mtime = expire;
>  		strbuf_addf(reason, _("Removing repos/%s: invalid gitdir file"), id);
>  		free(path);
>  		return 1;
> @@ -162,7 +160,7 @@ static int prune_repo_dir(const char *id, struct stat *st, struct strbuf *reason
>  		return 1;
>  	}
>  	free(path);
> -	return 0;
> +	return st.st_mtime <= expire;
>  }
>  
>  static void prune_repos_dir(void)
> @@ -172,15 +170,13 @@ static void prune_repos_dir(void)
>  	DIR *dir = opendir(git_path("repos"));
>  	struct dirent *d;
>  	int ret;
> -	struct stat st;
>  	if (!dir)
>  		return;
>  	while ((d = readdir(dir)) != NULL) {
>  		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
>  			continue;
>  		strbuf_reset(&reason);
> -		if (!prune_repo_dir(d->d_name, &st, &reason) ||
> -		    st.st_mtime > expire)
> +		if (!prune_repo_dir(d->d_name, &reason))
>  			continue;
>  		if (show_only || verbose)
>  			printf("%s\n", reason.buf);
> diff --git a/t/t2026-prune-linked-checkouts.sh b/t/t2026-prune-linked-checkouts.sh
> index 4ccfa4e..79d84cb 100755
> --- a/t/t2026-prune-linked-checkouts.sh
> +++ b/t/t2026-prune-linked-checkouts.sh
> @@ -77,7 +77,7 @@ test_expect_success 'not prune recent checkouts' '
>  	mkdir zz &&
>  	mkdir -p .git/repos/jlm &&
>  	echo "$TRASH_DIRECTORY"/zz >.git/repos/jlm/gitdir &&
> -	git prune --repos --verbose &&
> +	git prune --repos --verbose --expire=2.days.ago &&
>  	test -d .git/repos/jlm
>  '

Re: [PATCH 2/5] prune --repos: fix uninitialized access

[Duy Nguyen]

On Thu, Jul 24, 2014 at 2:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
> It does not explain what the change to the test is about, though.

The old test and code are wrong. We want to verify repos are not
pruned if they're new (like, just created). But "git prune" without
--expire sets expire time to 0xffffffff (everything will be pruned) so
today's repos are pruned as well. But the old code is wrong. It
returns 0 at the end of prune_repo_dir instead of1, so st.st_mtime is
never checked, the repos are not pruned. The end result is an
incorrect passed test :(
-- 
Duy

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

[Nguyễn Thái Ngọc Duy]

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."),
+		    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
 	)
 '
 
-- 
1.9.1.346.ga2b5940

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

[Michael J Gruber]

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
>  	)
>  '
>  
> 

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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

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

Another possible alternative is "go and work there instead of
creating yet another checkout", but is that too obvious without
saying?

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

[Duy Nguyen]

On Wed, Jul 23, 2014 at 8:48 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> Nguyễn Thái Ngọc Duy venit, vidit, dixit 23.07.2014 13:43:
>> +     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.

We would not be able to construct the exact command that the user has
entered, so perhaps

  git checkout --detach <more options> %s
  git checkout -b <new branch> <more options> %s

?

Note that this does not only occur when --to is given. If you have two
checkouts associated to the same repo, "git checkout foo" on one
checkout when "foo" is held by another checkout would cause the same
error. I'll need to think of a better name than advice.checkoutTo.
-- 
Duy

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Jul 23, 2014 at 8:48 PM, Michael J Gruber
> <git@drmicha.warpmail.net> wrote:
>> Nguyễn Thái Ngọc Duy venit, vidit, dixit 23.07.2014 13:43:
>>> +     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.
>
> We would not be able to construct the exact command that the user has
> entered, so perhaps
>
>   git checkout --detach <more options> %s
>   git checkout -b <new branch> <more options> %s
>
> ?
>
> Note that this does not only occur when --to is given. If you have two
> checkouts associated to the same repo, "git checkout foo" on one
> checkout when "foo" is held by another checkout would cause the same
> error. I'll need to think of a better name than advice.checkoutTo.

I am not sure if we need to say anything more than the first line of
the message you have in your patch.  To fork a new branch at the
commit the user is interested in to check it out, or not bothering
with the branch and detach, are both very normal parts of user's Git
toolchest, nothing particularly special.  As long as the most
important point, i.e. "in the new world order, unlike the
contrib/workdir hack, you cannot check out the same branch at two
different places", is clearly conveyed and understood, everything
else should follow naturally, no?

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 24.07.2014 23:30:
> Duy Nguyen <pclouds@gmail.com> writes:
> 
>> On Wed, Jul 23, 2014 at 8:48 PM, Michael J Gruber
>> <git@drmicha.warpmail.net> wrote:
>>> Nguyễn Thái Ngọc Duy venit, vidit, dixit 23.07.2014 13:43:
>>>> +     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.
>>
>> We would not be able to construct the exact command that the user has
>> entered, so perhaps
>>
>>   git checkout --detach <more options> %s
>>   git checkout -b <new branch> <more options> %s
>>
>> ?
>>
>> Note that this does not only occur when --to is given. If you have two
>> checkouts associated to the same repo, "git checkout foo" on one
>> checkout when "foo" is held by another checkout would cause the same
>> error. I'll need to think of a better name than advice.checkoutTo.
> 
> I am not sure if we need to say anything more than the first line of
> the message you have in your patch.  To fork a new branch at the
> commit the user is interested in to check it out, or not bothering
> with the branch and detach, are both very normal parts of user's Git
> toolchest, nothing particularly special.  As long as the most
> important point, i.e. "in the new world order, unlike the
> contrib/workdir hack, you cannot check out the same branch at two
> different places", is clearly conveyed and understood, everything
> else should follow naturally, no?

As an error message that is completely sufficient.

The advice messages are meant to teach the user about the normal parts
of the toolchest to use in a situation of "conflict", aren't they? Maybe
we should ask someone who hasn't turned them off...

Michael

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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> As an error message that is completely sufficient.
>
> The advice messages are meant to teach the user about the normal parts
> of the toolchest to use in a situation of "conflict", aren't they?

Not really.  They are to remind (to those who learned but forgot)
and to hint (to those who haven't realized they have things to learn
in this area).  Wall of text that tries to do more than that, like
"teaching", risks not getting read by anybody.

> Maybe
> we should ask someone who hasn't turned them off...

Actually, I run without most of the 'advice.*' turned off.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> As an error message that is completely sufficient.
>>
>> The advice messages are meant to teach the user about the normal parts
>> of the toolchest to use in a situation of "conflict", aren't they?
>
> Not really.  They are to remind (to those who learned but forgot)
> and to hint (to those who haven't realized they have things to learn
> in this area).  Wall of text that tries to do more than that, like
> "teaching", risks not getting read by anybody.
>
>> Maybe
>> we should ask someone who hasn't turned them off...
>
> Actually, I run without most of the 'advice.*' turned off.

Ehh, what I meant was that I do not have "advice.foo = false" for
most of 'foo', i.e. I run with most of them still active.

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

[Duy Nguyen]

On Thu, Jul 31, 2014 at 1:03 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> As an error message that is completely sufficient.
>>
>> The advice messages are meant to teach the user about the normal parts
>> of the toolchest to use in a situation of "conflict", aren't they?
>
> Not really.  They are to remind (to those who learned but forgot)
> and to hint (to those who haven't realized they have things to learn
> in this area).  Wall of text that tries to do more than that, like
> "teaching", risks not getting read by anybody.

Last call to all. Keep this 'advice' or drop it?
-- 
Duy

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Thu, Jul 31, 2014 at 1:03 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>>
>>> As an error message that is completely sufficient.
>>>
>>> The advice messages are meant to teach the user about the normal parts
>>> of the toolchest to use in a situation of "conflict", aren't they?
>>
>> Not really.  They are to remind (to those who learned but forgot)
>> and to hint (to those who haven't realized they have things to learn
>> in this area).  Wall of text that tries to do more than that, like
>> "teaching", risks not getting read by anybody.
>
> Last call to all. Keep this 'advice' or drop it?

I think you quoted what I needed to say already so I won't repeat
myself.  Earlier you said:

    If you have two checkouts associated to the same repo, "git
    checkout foo" on one checkout when "foo" is held by another
    checkout would cause the same error. I'll need to think of a
    better name than advice.checkoutTo.

and I agree; the patch needs rerolling in any case.

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

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

Not new in this round of update, and may not even be an issue, but:

 - Earlier, the code returned early on a negative return value from
   read-file (i.e., an error), but this round it also does so for
   zero.  Intended?

 - The above duplicates the logic in resolve_ref_unsafe() and
   resolve_gitlink_ref_recursive(); three places now knows what a
   textual symref looks like (i.e. begins with "ref:", zero or more
   whitespaces, the target ref and then zero or more trailing
   whitespaces).  Perhaps we need to consolidate the code further,
   so that this knowledge does not leak out of refs.c?

> +	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';

Accepting arbitrary numbers of '\r' and '\n' sounds as if the code
is allowing it, but text editors would not end their files with a
nonsense sequence like "\r\r\n\r" unless the end-user works to do
so, and if you are prepared to be lenient to noisy human input, not
trimming trailing whitespaces does not look it is going far enough
to help them.

I do not see a good reason to allow random text editors to edit this
file in the first place, so my preference is:

	if (strbuf_read_file(...) < 0 ||
	    gitdir.len == 0 ||
            gitdir.buf[gitdir.len - 1] != '\n')
            goto error_return;
	gitdir.buf[--gitdir.len] = '\0';

Alternatively, if you are trying to be lenient to human input, I
would understand:

	if (strbuf_read_file(...) < 0)
        	goto error_return;
	strbuf_rtrim(&gitdir);

The code in the patch, which is something in between, does not make
much sense to me.

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

[Duy Nguyen]

On Thu, Jul 24, 2014 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> +     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++;
>
> Not new in this round of update, and may not even be an issue, but:
>
>  - Earlier, the code returned early on a negative return value from
>    read-file (i.e., an error), but this round it also does so for
>    zero.  Intended?

Yes. But it does not make any difference. strbuf_read_file returns
sb.len, if it's empty, the next skip_prefix would fail anyway.

>  - The above duplicates the logic in resolve_ref_unsafe() and
>    resolve_gitlink_ref_recursive(); three places now knows what a
>    textual symref looks like (i.e. begins with "ref:", zero or more
>    whitespaces, the target ref and then zero or more trailing
>    whitespaces).  Perhaps we need to consolidate the code further,
>    so that this knowledge does not leak out of refs.c?

OK. Will do (unless Mike has a different opinion about this).

>
>> +     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';
>
> Accepting arbitrary numbers of '\r' and '\n' sounds as if the code
> is allowing it, but text editors would not end their files with a
> nonsense sequence like "\r\r\n\r" unless the end-user works to do
> so, and if you are prepared to be lenient to noisy human input, not
> trimming trailing whitespaces does not look it is going far enough
> to help them.
>
> I do not see a good reason to allow random text editors to edit this
> file in the first place, so my preference is:
>
>         if (strbuf_read_file(...) < 0 ||
>             gitdir.len == 0 ||
>             gitdir.buf[gitdir.len - 1] != '\n')
>             goto error_return;
>         gitdir.buf[--gitdir.len] = '\0';
>
> Alternatively, if you are trying to be lenient to human input, I
> would understand:
>
>         if (strbuf_read_file(...) < 0)
>                 goto error_return;
>         strbuf_rtrim(&gitdir);
>
> The code in the patch, which is something in between, does not make
> much sense to me.

I think more about "echo abc > $this_file" where the echo command may
output '\r\n' on Windows (wild guess though, I don't use Windows
much). I think I'm going with _rtrim.
-- 
Duy

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Thu, Jul 24, 2014 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> +     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++;
>>
>> Not new in this round of update, and may not even be an issue, but:
>>
>>  - Earlier, the code returned early on a negative return value from
>>    read-file (i.e., an error), but this round it also does so for
>>    zero.  Intended?
>
> Yes. But it does not make any difference. strbuf_read_file returns
> sb.len, if it's empty, the next skip_prefix would fail anyway.

Yes but changing < 0 to <= 0 is inconsistent with that; I would
understand if you changed it to <= 4, which would be consistent with
the reasoning, though.

>> The code in the patch, which is something in between, does not make
>> much sense to me.
>
> I think more about "echo abc > $this_file" where the echo command may
> output '\r\n' on Windows (wild guess though, I don't use Windows
> much). I think I'm going with _rtrim.

To expect 'echo' into the file is to expect and encourage that
people muck with the internal implementation details by hand, which
we do not generally do for things inside .git [*1*].

If we consider the contents of $this_file not an implementation
detail but a part of the published API (i.e. "writing this string
into the file is a valid way to make Git do this"), rtrim would at
least be consistent with how the existing code deals with symrefs,
so I wouldn't say "does not make much sense" if you are going in
that direction ;-)


[Footnote]

*1* ... except for .git/config, to which we say "It's a simple text
file; don't be afraied to edit it away!".

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Thu, Jul 24, 2014 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> +     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++;
>>>
>>> Not new in this round of update, and may not even be an issue, but:
>>>
>>>  - Earlier, the code returned early on a negative return value from
>>>    read-file (i.e., an error), but this round it also does so for
>>>    zero.  Intended?
>>
>> Yes. But it does not make any difference. strbuf_read_file returns
>> sb.len, if it's empty, the next skip_prefix would fail anyway.
>
> Yes but changing < 0 to <= 0 is inconsistent with that; I would
> understand if you changed it to <= 4, which would be consistent with
> the reasoning, though.

Just to make sure.  I am not saying changing < 0  to <= 4 is a good
idea.  I think checking for strbuf_read_file() failure with < 0 and
then checking for malformatted input (or detached head perhaps?)
with !skip_prefix(), i.e. testing two logically separate things with
two separate conditions concatenated together with ||, would be the
most natural way to express it.

[PATCH 4/5] checkout --to: fix dangling pointers in remove_junk()

[Nguyễn Thái Ngọc Duy]

junk_git_dir is set to sb_repo.buf. By the end of prepare_linked_checkout(),
sb_repo is freed and so junk_git_dir points to nowhere. If the second
checkout command fails, is_junk remains non-zero, remove_junk() will
be called and try to clean junk_git_dir, which could be anything now
(if it does not crash the program).

The new test may pass even without this patch. But it does fail under
valgrind (without this patch) with "Invalid read of size 8" at the
right line.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout.c     | 15 ++++++++++-----
 t/t2025-checkout-to.sh |  6 ++++++
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index d35245a..e62c084 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -825,8 +825,8 @@ static int switch_branches(const struct checkout_opts *opts,
 	return ret || writeout_error;
 }
 
-static const char *junk_work_tree;
-static const char *junk_git_dir;
+static char *junk_work_tree;
+static char *junk_git_dir;
 static int is_junk;
 static pid_t junk_pid;
 
@@ -895,7 +895,7 @@ static int prepare_linked_checkout(const struct checkout_opts *opts,
 
 	if (mkdir(sb_repo.buf, 0777))
 		die_errno(_("could not create directory of '%s'"), sb_repo.buf);
-	junk_git_dir = sb_repo.buf;
+	junk_git_dir = xstrdup(sb_repo.buf);
 	is_junk = 1;
 
 	/*
@@ -909,7 +909,7 @@ static int prepare_linked_checkout(const struct checkout_opts *opts,
 	if (safe_create_leading_directories_const(sb_git.buf))
 		die_errno(_("could not create leading directories of '%s'"),
 			  sb_git.buf);
-	junk_work_tree = path;
+	junk_work_tree = xstrdup(path);
 
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/gitdir", sb_repo.buf);
@@ -939,8 +939,13 @@ static int prepare_linked_checkout(const struct checkout_opts *opts,
 	cp.git_cmd = 1;
 	cp.argv = opts->saved_argv;
 	ret = run_command(&cp);
-	if (!ret)
+	if (!ret) {
 		is_junk = 0;
+		free(junk_work_tree);
+		free(junk_git_dir);
+		junk_work_tree = NULL;
+		junk_git_dir = NULL;
+	}
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/locked", sb_repo.buf);
 	unlink_or_warn(sb.buf);
diff --git a/t/t2025-checkout-to.sh b/t/t2025-checkout-to.sh
index c6601a4..8a00310 100755
--- a/t/t2025-checkout-to.sh
+++ b/t/t2025-checkout-to.sh
@@ -12,6 +12,12 @@ test_expect_success 'checkout --to not updating paths' '
 	test_must_fail git checkout --to -- init.t
 '
 
+test_expect_success 'checkout --to refuses to checkout locked branch' '
+	test_must_fail git checkout --to zere master &&
+	! test -d zere &&
+	! test -d .git/repos/zere
+'
+
 test_expect_success 'checkout --to a new worktree' '
 	git rev-parse HEAD >expect &&
 	git checkout --detach --to here master &&
-- 
1.9.1.346.ga2b5940

[PATCH 5/5] environment.c: fix incorrect git_graft_file initialization

[Nguyễn Thái Ngọc Duy]

"info/grafts" should be part of the "common repository" when accessed
from a linked checkout (iow $GIT_COMMON_DIR/info/grafts, not
$GIT_DIR/info/grafts).

git_path("info/grafts") returns correctly, even without this fix,
because it detects that $GIT_GRAFT_FILE is not set, so it goes with the
common rule: anything except sparse-checkout in 'info' belongs to common
repo. But get_graft_file() would return a wrong value and that one is
used for setting grafts up.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 environment.c          |  2 +-
 t/t0060-path-utils.sh  |  1 +
 t/t2025-checkout-to.sh | 18 ++++++++++++++++++
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/environment.c b/environment.c
index 50ed40a..d5b0788 100644
--- a/environment.c
+++ b/environment.c
@@ -157,7 +157,7 @@ static void setup_git_env(void)
 					   "objects", &git_db_env);
 	git_index_file = git_path_from_env(INDEX_ENVIRONMENT, git_dir,
 					   "index", &git_index_env);
-	git_graft_file = git_path_from_env(GRAFT_ENVIRONMENT, git_dir,
+	git_graft_file = git_path_from_env(GRAFT_ENVIRONMENT, git_common_dir,
 					   "info/grafts", &git_graft_env);
 	if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
 		check_replace_refs = 0;
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index da82aab..93605f4 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -269,6 +269,7 @@ test_git_path GIT_COMMON_DIR=bar logs/HEAD                .git/logs/HEAD
 test_git_path GIT_COMMON_DIR=bar objects                  bar/objects
 test_git_path GIT_COMMON_DIR=bar objects/bar              bar/objects/bar
 test_git_path GIT_COMMON_DIR=bar info/exclude             bar/info/exclude
+test_git_path GIT_COMMON_DIR=bar info/grafts              bar/info/grafts
 test_git_path GIT_COMMON_DIR=bar info/sparse-checkout     .git/info/sparse-checkout
 test_git_path GIT_COMMON_DIR=bar remotes/bar              bar/remotes/bar
 test_git_path GIT_COMMON_DIR=bar branches/bar             bar/branches/bar
diff --git a/t/t2025-checkout-to.sh b/t/t2025-checkout-to.sh
index 8a00310..508993f 100755
--- a/t/t2025-checkout-to.sh
+++ b/t/t2025-checkout-to.sh
@@ -81,4 +81,22 @@ test_expect_success 'checkout from a bare repo without --to' '
 	)
 '
 
+test_expect_success 'checkout with grafts' '
+	test_when_finished rm .git/info/grafts &&
+	test_commit abc &&
+	SHA1=`git rev-parse HEAD` &&
+	test_commit def &&
+	test_commit xyz &&
+	echo "`git rev-parse HEAD` $SHA1" >.git/info/grafts &&
+	cat >expected <<-\EOF &&
+	xyz
+	abc
+	EOF
+	git log --format=%s -2 >actual &&
+	test_cmp expected actual &&
+	git checkout --detach --to grafted master &&
+	git --git-dir=grafted/.git log --format=%s -2 >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
1.9.1.346.ga2b5940

Re: [PATCH 5/5] environment.c: fix incorrect git_graft_file initialization

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> "info/grafts" should be part of the "common repository" when accessed
> from a linked checkout (iow $GIT_COMMON_DIR/info/grafts, not
> $GIT_DIR/info/grafts).
>
> git_path("info/grafts") returns correctly, even without this fix,
> because it detects that $GIT_GRAFT_FILE is not set, so it goes with the
> common rule: anything except sparse-checkout in 'info' belongs to common
> repo. But get_graft_file() would return a wrong value and that one is
> used for setting grafts up.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Thanks Eric for sharp eyes and Duy for a quick fix.

Will queue all five.

>  environment.c          |  2 +-
>  t/t0060-path-utils.sh  |  1 +
>  t/t2025-checkout-to.sh | 18 ++++++++++++++++++
>  3 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/environment.c b/environment.c
> index 50ed40a..d5b0788 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -157,7 +157,7 @@ static void setup_git_env(void)
>  					   "objects", &git_db_env);
>  	git_index_file = git_path_from_env(INDEX_ENVIRONMENT, git_dir,
>  					   "index", &git_index_env);
> -	git_graft_file = git_path_from_env(GRAFT_ENVIRONMENT, git_dir,
> +	git_graft_file = git_path_from_env(GRAFT_ENVIRONMENT, git_common_dir,
>  					   "info/grafts", &git_graft_env);
>  	if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
>  		check_replace_refs = 0;
> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> index da82aab..93605f4 100755
> --- a/t/t0060-path-utils.sh
> +++ b/t/t0060-path-utils.sh
> @@ -269,6 +269,7 @@ test_git_path GIT_COMMON_DIR=bar logs/HEAD                .git/logs/HEAD
>  test_git_path GIT_COMMON_DIR=bar objects                  bar/objects
>  test_git_path GIT_COMMON_DIR=bar objects/bar              bar/objects/bar
>  test_git_path GIT_COMMON_DIR=bar info/exclude             bar/info/exclude
> +test_git_path GIT_COMMON_DIR=bar info/grafts              bar/info/grafts
>  test_git_path GIT_COMMON_DIR=bar info/sparse-checkout     .git/info/sparse-checkout
>  test_git_path GIT_COMMON_DIR=bar remotes/bar              bar/remotes/bar
>  test_git_path GIT_COMMON_DIR=bar branches/bar             bar/branches/bar
> diff --git a/t/t2025-checkout-to.sh b/t/t2025-checkout-to.sh
> index 8a00310..508993f 100755
> --- a/t/t2025-checkout-to.sh
> +++ b/t/t2025-checkout-to.sh
> @@ -81,4 +81,22 @@ test_expect_success 'checkout from a bare repo without --to' '
>  	)
>  '
>  
> +test_expect_success 'checkout with grafts' '
> +	test_when_finished rm .git/info/grafts &&
> +	test_commit abc &&
> +	SHA1=`git rev-parse HEAD` &&
> +	test_commit def &&
> +	test_commit xyz &&
> +	echo "`git rev-parse HEAD` $SHA1" >.git/info/grafts &&
> +	cat >expected <<-\EOF &&
> +	xyz
> +	abc
> +	EOF
> +	git log --format=%s -2 >actual &&
> +	test_cmp expected actual &&
> +	git checkout --detach --to grafted master &&
> +	git --git-dir=grafted/.git log --format=%s -2 >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_done

[PATCH v2 0/8] nd/multiple-work-trees follow-ups

[Nguyễn Thái Ngọc Duy]

This fixes all comments from v1. I still keep the long advice when
'git checkout' fails because another branch is locked, because it's
easy(*) to disable the advice. Two more fixes found today. The test
breakages on Windows have to wait until somebody on Windows jumps in.

(*) although I start to want an advice.nothing config that disables
all advice..

Nguyễn Thái Ngọc Duy (8):
  gitrepository-layout.txt: s/ignored/ignored if/
  checkout: no need to call check_linked_checkouts if head_ref is NULL
  prune --repos: fix uninitialized access
  checkout: no auto-detach if the ref is already checked out
  checkout --to: fix dangling pointers in remove_junk()
  environment.c: fix incorrect git_graft_file initialization
  checkout: prefix --to argument properly when cwd is moved
  checkout --to: do not touch existing target directory

 Documentation/config.txt               |  4 ++
 Documentation/gitrepository-layout.txt |  6 +--
 advice.c                               |  2 +
 advice.h                               |  1 +
 builtin/checkout.c                     | 98 +++++++++++++++++++---------------
 builtin/prune.c                        | 16 +++---
 environment.c                          |  2 +-
 t/t0060-path-utils.sh                  |  1 +
 t/t2025-checkout-to.sh                 | 40 ++++++++++----
 t/t2026-prune-linked-checkouts.sh      |  2 +-
 10 files changed, 106 insertions(+), 66 deletions(-)

-- 
2.1.0.rc0.78.gc0d8480

[PATCH v2 1/8] gitrepository-layout.txt: s/ignored/ignored if/

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/gitrepository-layout.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/gitrepository-layout.txt b/Documentation/gitrepository-layout.txt
index bed4f1a..6bd82af 100644
--- a/Documentation/gitrepository-layout.txt
+++ b/Documentation/gitrepository-layout.txt
@@ -139,7 +139,7 @@ is often called 'detached HEAD.'  See linkgit:git-checkout[1]
 for details.
 
 config::
-	Repository specific configuration file. This file is ignored
+	Repository specific configuration file. This file is ignored if
 	$GIT_COMMON_DIR is set and "$GIT_COMMON_DIR/config" will be
 	used instead.
 
@@ -220,7 +220,7 @@ remotes::
 logs::
 	Records of changes made to refs are stored in this
 	directory.  See linkgit:git-update-ref[1]
-	for more information. This directory is ignored
+	for more information. This directory is ignored if
 	$GIT_COMMON_DIR is set and "$GIT_COMMON_DIR/logs" will be used
 	instead.
 
@@ -252,7 +252,7 @@ modules::
 repos::
 	Contains worktree specific information of linked
 	checkouts. Each subdirectory contains the worktree-related
-	part of a linked checkout. This directory is ignored
+	part of a linked checkout. This directory is ignored if
 	$GIT_COMMON_DIR is set and "$GIT_COMMON_DIR/repos" will be
 	used instead.
 
-- 
2.1.0.rc0.78.gc0d8480

[PATCH v2 2/8] checkout: no need to call check_linked_checkouts if head_ref is NULL

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index c83f476..6ac89eb 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1202,7 +1202,8 @@ static int parse_branchname_arg(int argc, const char **argv,
 		unsigned char sha1[20];
 		int flag;
 		char *head_ref = resolve_refdup("HEAD", sha1, 0, &flag);
-		if (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path))
+		if (head_ref &&
+		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)))
 			check_linked_checkouts(new);
 		free(head_ref);
 	}
-- 
2.1.0.rc0.78.gc0d8480

[PATCH v2 3/8] prune --repos: fix uninitialized access

[Nguyễn Thái Ngọc Duy]

There's a code path in prune_repo_dir() that does not initialize 'st'
buffer, which is checked by the caller, prune_repos_dir(). Instead
of leaking some prune logic out to prune_repos_dir(), move 'st' into
prune_repo_dir().

Another bug that is fixed while at there is the "return 0" at the end
of prune_repo_dir() instead of '1', meaning "keep the checkout" while
we want "keep the checkout _unless_ its last update is older than
expire limit". Set correct expire limit in the test as well.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/prune.c                   | 16 ++++++----------
 t/t2026-prune-linked-checkouts.sh |  2 +-
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/builtin/prune.c b/builtin/prune.c
index 28b7adf..e72c391 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -112,8 +112,9 @@ static void prune_object_dir(const char *path)
 	}
 }
 
-static int prune_repo_dir(const char *id, struct stat *st, struct strbuf *reason)
+static int prune_repo_dir(const char *id, struct strbuf *reason)
 {
+	struct stat st;
 	char *path;
 	int fd, len;
 
@@ -123,26 +124,23 @@ static int prune_repo_dir(const char *id, struct stat *st, struct strbuf *reason
 	}
 	if (file_exists(git_path("repos/%s/locked", id)))
 		return 0;
-	if (stat(git_path("repos/%s/gitdir", id), st)) {
-		st->st_mtime = expire;
+	if (stat(git_path("repos/%s/gitdir", id), &st)) {
 		strbuf_addf(reason, _("Removing repos/%s: gitdir file does not exist"), id);
 		return 1;
 	}
 	fd = open(git_path("repos/%s/gitdir", id), O_RDONLY);
 	if (fd < 0) {
-		st->st_mtime = expire;
 		strbuf_addf(reason, _("Removing repos/%s: unable to read gitdir file (%s)"),
 			    id, strerror(errno));
 		return 1;
 	}
-	len = st->st_size;
+	len = st.st_size;
 	path = xmalloc(len + 1);
 	read_in_full(fd, path, len);
 	close(fd);
 	while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
 		len--;
 	if (!len) {
-		st->st_mtime = expire;
 		strbuf_addf(reason, _("Removing repos/%s: invalid gitdir file"), id);
 		free(path);
 		return 1;
@@ -162,7 +160,7 @@ static int prune_repo_dir(const char *id, struct stat *st, struct strbuf *reason
 		return 1;
 	}
 	free(path);
-	return 0;
+	return st.st_mtime <= expire;
 }
 
 static void prune_repos_dir(void)
@@ -172,15 +170,13 @@ static void prune_repos_dir(void)
 	DIR *dir = opendir(git_path("repos"));
 	struct dirent *d;
 	int ret;
-	struct stat st;
 	if (!dir)
 		return;
 	while ((d = readdir(dir)) != NULL) {
 		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
 			continue;
 		strbuf_reset(&reason);
-		if (!prune_repo_dir(d->d_name, &st, &reason) ||
-		    st.st_mtime > expire)
+		if (!prune_repo_dir(d->d_name, &reason))
 			continue;
 		if (show_only || verbose)
 			printf("%s\n", reason.buf);
diff --git a/t/t2026-prune-linked-checkouts.sh b/t/t2026-prune-linked-checkouts.sh
index 4ccfa4e..79d84cb 100755
--- a/t/t2026-prune-linked-checkouts.sh
+++ b/t/t2026-prune-linked-checkouts.sh
@@ -77,7 +77,7 @@ test_expect_success 'not prune recent checkouts' '
 	mkdir zz &&
 	mkdir -p .git/repos/jlm &&
 	echo "$TRASH_DIRECTORY"/zz >.git/repos/jlm/gitdir &&
-	git prune --repos --verbose &&
+	git prune --repos --verbose --expire=2.days.ago &&
 	test -d .git/repos/jlm
 '
 
-- 
2.1.0.rc0.78.gc0d8480

[PATCH v2 4/8] checkout: no auto-detach if the ref is already checked out

[Nguyễn Thái Ngọc Duy]

Give the user a choice in this case: detach, make a new branch,
release current branch in the other checkout or simply 'cd' there and
continue to work.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt |  4 +++
 advice.c                 |  2 ++
 advice.h                 |  1 +
 builtin/checkout.c       | 76 ++++++++++++++++++++++++++----------------------
 t/t2025-checkout-to.sh   | 16 +++++-----
 5 files changed, 55 insertions(+), 44 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 57999fa..b2c3388 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -201,6 +201,10 @@ advice.*::
 	rmHints::
 		In case of failure in the output of linkgit:git-rm[1],
 		show directions on how to proceed from the current state.
+	checkoutLocked::
+		In multiple checkout setup, attempting to checkout a
+		branch already checked out elsewhere will fail. Show
+		some useful options to proceed.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index 9b42033..cf3b0f7 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_locked = 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 },
+	{ "checkoutlocked", &advice_checkout_locked },
 
 	/* make this an alias for backward compatibility */
 	{ "pushnonfastforward", &advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index 5ecc6c1..935631d 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_locked;
 
 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 6ac89eb..0714856 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -645,11 +645,6 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 			if (old->path && advice_detached_head)
 				detach_advice(new->name);
 			describe_detached_head(_("HEAD is now at"), new->commit);
-			if (new->checkout && !*new->checkout)
-				fprintf(stderr, _("hint: the main checkout is holding this branch\n"));
-			else if (new->checkout)
-				fprintf(stderr, _("hint: the linked checkout %s is holding this branch\n"),
-					new->checkout);
 		}
 	} else if (new->path) {	/* Switch branches. */
 		create_symref("HEAD", new->path, msg.buf);
@@ -1006,31 +1001,50 @@ 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;
+		strbuf_rtrim(&gitdir);
+	} else
+		strbuf_addstr(&gitdir, get_git_common_dir());
+	if (advice_checkout_locked)
+		die(_("'%s' is already checked out at %s\n"
+		      "Either go there and continue working, or detach HEAD using\n"
+		      "    git checkout --detach [more options] %s\n"
+		      "or create a new branch based off '%s' using\n"
+		      "    git checkout -b <branch name> [more options] %s\n"
+		      "or switch to another branch at the other checkout and retry."),
+		    new->name, gitdir.buf, new->name, new->name, new->name);
+	else
+		die(_("'%s' is already checked out at %s"), new->name, 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 +1059,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 +1080,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 +1203,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);
@@ -1417,7 +1422,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
 	)
 '
 
-- 
2.1.0.rc0.78.gc0d8480

[PATCH v2 5/8] checkout --to: fix dangling pointers in remove_junk()

[Nguyễn Thái Ngọc Duy]

junk_git_dir is set to sb_repo.buf. By the end of prepare_linked_checkout(),
sb_repo is freed and so junk_git_dir points to nowhere. If the second
checkout command fails, is_junk remains non-zero, remove_junk() will
be called and try to clean junk_git_dir, which could be anything now
(if it does not crash the program).

The new test may pass even without this patch. But it does fail under
valgrind (without this patch) with "Invalid read of size 8" at the
right line.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout.c     | 15 ++++++++++-----
 t/t2025-checkout-to.sh |  6 ++++++
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0714856..173aab1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -820,8 +820,8 @@ static int switch_branches(const struct checkout_opts *opts,
 	return ret || writeout_error;
 }
 
-static const char *junk_work_tree;
-static const char *junk_git_dir;
+static char *junk_work_tree;
+static char *junk_git_dir;
 static int is_junk;
 static pid_t junk_pid;
 
@@ -890,7 +890,7 @@ static int prepare_linked_checkout(const struct checkout_opts *opts,
 
 	if (mkdir(sb_repo.buf, 0777))
 		die_errno(_("could not create directory of '%s'"), sb_repo.buf);
-	junk_git_dir = sb_repo.buf;
+	junk_git_dir = xstrdup(sb_repo.buf);
 	is_junk = 1;
 
 	/*
@@ -904,7 +904,7 @@ static int prepare_linked_checkout(const struct checkout_opts *opts,
 	if (safe_create_leading_directories_const(sb_git.buf))
 		die_errno(_("could not create leading directories of '%s'"),
 			  sb_git.buf);
-	junk_work_tree = path;
+	junk_work_tree = xstrdup(path);
 
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/gitdir", sb_repo.buf);
@@ -934,8 +934,13 @@ static int prepare_linked_checkout(const struct checkout_opts *opts,
 	cp.git_cmd = 1;
 	cp.argv = opts->saved_argv;
 	ret = run_command(&cp);
-	if (!ret)
+	if (!ret) {
 		is_junk = 0;
+		free(junk_work_tree);
+		free(junk_git_dir);
+		junk_work_tree = NULL;
+		junk_git_dir = NULL;
+	}
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/locked", sb_repo.buf);
 	unlink_or_warn(sb.buf);
diff --git a/t/t2025-checkout-to.sh b/t/t2025-checkout-to.sh
index c6601a4..8a00310 100755
--- a/t/t2025-checkout-to.sh
+++ b/t/t2025-checkout-to.sh
@@ -12,6 +12,12 @@ test_expect_success 'checkout --to not updating paths' '
 	test_must_fail git checkout --to -- init.t
 '
 
+test_expect_success 'checkout --to refuses to checkout locked branch' '
+	test_must_fail git checkout --to zere master &&
+	! test -d zere &&
+	! test -d .git/repos/zere
+'
+
 test_expect_success 'checkout --to a new worktree' '
 	git rev-parse HEAD >expect &&
 	git checkout --detach --to here master &&
-- 
2.1.0.rc0.78.gc0d8480

[PATCH v2 6/8] environment.c: fix incorrect git_graft_file initialization

[Nguyễn Thái Ngọc Duy]

"info/grafts" should be part of the "common repository" when accessed
from a linked checkout (iow $GIT_COMMON_DIR/info/grafts, not
$GIT_DIR/info/grafts).

git_path("info/grafts") returns correctly, even without this fix,
because it detects that $GIT_GRAFT_FILE is not set, so it goes with the
common rule: anything except sparse-checkout in 'info' belongs to common
repo. But get_graft_file() would return a wrong value and that one is
used for setting grafts up.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 environment.c          |  2 +-
 t/t0060-path-utils.sh  |  1 +
 t/t2025-checkout-to.sh | 18 ++++++++++++++++++
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/environment.c b/environment.c
index 50ed40a..d5b0788 100644
--- a/environment.c
+++ b/environment.c
@@ -157,7 +157,7 @@ static void setup_git_env(void)
 					   "objects", &git_db_env);
 	git_index_file = git_path_from_env(INDEX_ENVIRONMENT, git_dir,
 					   "index", &git_index_env);
-	git_graft_file = git_path_from_env(GRAFT_ENVIRONMENT, git_dir,
+	git_graft_file = git_path_from_env(GRAFT_ENVIRONMENT, git_common_dir,
 					   "info/grafts", &git_graft_env);
 	if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
 		check_replace_refs = 0;
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index da82aab..93605f4 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -269,6 +269,7 @@ test_git_path GIT_COMMON_DIR=bar logs/HEAD                .git/logs/HEAD
 test_git_path GIT_COMMON_DIR=bar objects                  bar/objects
 test_git_path GIT_COMMON_DIR=bar objects/bar              bar/objects/bar
 test_git_path GIT_COMMON_DIR=bar info/exclude             bar/info/exclude
+test_git_path GIT_COMMON_DIR=bar info/grafts              bar/info/grafts
 test_git_path GIT_COMMON_DIR=bar info/sparse-checkout     .git/info/sparse-checkout
 test_git_path GIT_COMMON_DIR=bar remotes/bar              bar/remotes/bar
 test_git_path GIT_COMMON_DIR=bar branches/bar             bar/branches/bar
diff --git a/t/t2025-checkout-to.sh b/t/t2025-checkout-to.sh
index 8a00310..508993f 100755
--- a/t/t2025-checkout-to.sh
+++ b/t/t2025-checkout-to.sh
@@ -81,4 +81,22 @@ test_expect_success 'checkout from a bare repo without --to' '
 	)
 '
 
+test_expect_success 'checkout with grafts' '
+	test_when_finished rm .git/info/grafts &&
+	test_commit abc &&
+	SHA1=`git rev-parse HEAD` &&
+	test_commit def &&
+	test_commit xyz &&
+	echo "`git rev-parse HEAD` $SHA1" >.git/info/grafts &&
+	cat >expected <<-\EOF &&
+	xyz
+	abc
+	EOF
+	git log --format=%s -2 >actual &&
+	test_cmp expected actual &&
+	git checkout --detach --to grafted master &&
+	git --git-dir=grafted/.git log --format=%s -2 >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.1.0.rc0.78.gc0d8480

[PATCH v2 7/8] checkout: prefix --to argument properly when cwd is moved

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 173aab1..4fbb9c1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1342,7 +1342,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 			 N_("do not limit pathspecs to sparse entries only")),
 		OPT_HIDDEN_BOOL(0, "guess", &dwim_new_local_branch,
 				N_("second guess 'git checkout no-such-branch'")),
-		OPT_STRING(0, "to", &opts.new_worktree, N_("path"),
+		OPT_FILENAME(0, "to", &opts.new_worktree,
 			   N_("check a branch out in a separate working directory")),
 		OPT_END(),
 	};
-- 
2.1.0.rc0.78.gc0d8480

Re: [PATCH v2 7/8] checkout: prefix --to argument properly when cwd is moved

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/checkout.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 173aab1..4fbb9c1 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1342,7 +1342,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  			 N_("do not limit pathspecs to sparse entries only")),
>  		OPT_HIDDEN_BOOL(0, "guess", &dwim_new_local_branch,
>  				N_("second guess 'git checkout no-such-branch'")),
> -		OPT_STRING(0, "to", &opts.new_worktree, N_("path"),
> +		OPT_FILENAME(0, "to", &opts.new_worktree,
>  			   N_("check a branch out in a separate working directory")),
>  		OPT_END(),
>  	};

Good thinking.  Otherwise this would not work from within a
subdirectory.  Perhaps you would want a test for it?

An unrelated tangent, but would we want to further enhance
OPT_FILENAME() to understand ~/path and ~user/path perhaps?
It is easy to rely on users' shells to do so, so it is not a big
deal and it certainly is unrelated to this particular topic.

Re: [PATCH v2 7/8] checkout: prefix --to argument properly when cwd is moved

[Duy Nguyen]

On Wed, Jul 30, 2014 at 3:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  builtin/checkout.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/checkout.c b/builtin/checkout.c
>> index 173aab1..4fbb9c1 100644
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -1342,7 +1342,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>>                        N_("do not limit pathspecs to sparse entries only")),
>>               OPT_HIDDEN_BOOL(0, "guess", &dwim_new_local_branch,
>>                               N_("second guess 'git checkout no-such-branch'")),
>> -             OPT_STRING(0, "to", &opts.new_worktree, N_("path"),
>> +             OPT_FILENAME(0, "to", &opts.new_worktree,
>>                          N_("check a branch out in a separate working directory")),
>>               OPT_END(),
>>       };
>
> Good thinking.  Otherwise this would not work from within a
> subdirectory.  Perhaps you would want a test for it?

Will do at the next round.

> An unrelated tangent, but would we want to further enhance
> OPT_FILENAME() to understand ~/path and ~user/path perhaps?
> It is easy to rely on users' shells to do so, so it is not a big
> deal and it certainly is unrelated to this particular topic.

Yeah, the only user group that probably wants this is cmd.exe users.
We already have expand_user_path for this, all needed is some glue.
I'll let Windows users do this if they really want to.
-- 
Duy

[PATCH v2 8/8] checkout --to: do not touch existing target directory

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 4fbb9c1..3dc416c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -861,6 +861,8 @@ static int prepare_linked_checkout(const struct checkout_opts *opts,
 
 	if (!new->commit)
 		die(_("no branch specified"));
+	if (file_exists(path))
+		die(_("%s already exists"), path);
 
 	len = strlen(path);
 	while (len && is_dir_sep(path[len - 1]))
-- 
2.1.0.rc0.78.gc0d8480

Re: [PATCH 0/5] nd/multiple-work-trees follow-ups

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> The series has entered 'next' so I can't replace patches any more.
> Besides the brown paper bag fixes, checkout now rejects if a branch is
> already checked out elsewhere.

I do not think we would want to rush the entire series to 'master'
before the upcoming release, so we could squash these fixes into the
original when we rewind 'next' post release, if you wanted to.

The fix-ups are easier to review than wholesale replacements; keep
them coming as needed.

Thanks.

Re: [PATCH 0/5] nd/multiple-work-trees follow-ups

[Duy Nguyen]

On Thu, Jul 31, 2014 at 12:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> The series has entered 'next' so I can't replace patches any more.
>> Besides the brown paper bag fixes, checkout now rejects if a branch is
>> already checked out elsewhere.
>
> I do not think we would want to rush the entire series to 'master'
> before the upcoming release, so we could squash these fixes into the
> original when we rewind 'next' post release, if you wanted to.

Great. Please remove it from next at the next rewind.

> The fix-ups are easier to review than wholesale replacements; keep
> them coming as needed.

Will do.
-- 
Duy

Re: [PATCH 0/5] nd/multiple-work-trees follow-ups

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Thu, Jul 31, 2014 at 12:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>
>>> The series has entered 'next' so I can't replace patches any more.
>>> Besides the brown paper bag fixes, checkout now rejects if a branch is
>>> already checked out elsewhere.
>>
>> I do not think we would want to rush the entire series to 'master'
>> before the upcoming release, so we could squash these fixes into the
>> original when we rewind 'next' post release, if you wanted to.
>
> Great. Please remove it from next at the next rewind.

I'd likely forget so please remind me when the time comes ;-)  In
the meantime, I made a note in the "What's cooking" report to hold
the topic in 'next' and not advance it to 'master'.