threads / patch / 41212

v2, 6 partsKill and replace update_linked_gitdir()

Subject: [PATCH v2 0/6] Kill and replace update_linked_gitdir()

## tl;dr

15 messages between Jan 18, 2016 and Jan 22, 2016. Diffs are folded; open one to read it.

replies: 14people: 5as markdown or json

Nguyễn Thái Ngọc Duy· Jan 18, 2016, 11:21 UTC · lore
There are a couple of problems with this function:
 - premature design
 - create "gitdir" file outside multi-worktree context
 - update the file's content with relative path, with breaks "worktree list"
The first three patches kill it, as a result. They are relatively safe.

The last three re-implement it in a new form, "worktree refresh", inspired by "update-index --refresh". For now the user can use this command to correct some internal data after moving a worktree. In future, we might do automatic refresh like we do with the index.

The last three try out new design, so it will probably take more time to graduate than the first three, which may end up in the next release as worktree bug fix.

Eric Sunshine (1):
  worktree.txt: how to fix up after moving a worktree
Nguyễn Thái Ngọc Duy (5):
  worktree.c: fix indentation
  worktree: stop supporting moving worktrees manually
  abspath.c: add and use real_path_dup()
  setup.c: record the location of .git file
  worktree: new command to fix up worktree's info after moving
 Documentation/git-worktree.txt | 21 ++++++++++++++++-----
 abspath.c                      |  5 +++++
 builtin/clone.c                |  2 +-
 builtin/init-db.c              |  6 +++---
 builtin/worktree.c             | 18 ++++++++++++++++++
 cache.h                        |  2 ++
 setup.c                        | 26 +++++++++++---------------
 t/t1501-worktree.sh            |  9 +++++++++
 worktree.c                     |  8 ++++----
 9 files changed, 69 insertions(+), 28 deletions(-)
-- 
2.7.0.96.g5373197
Nguyễn Thái Ngọc Duy· Jan 18, 2016, 11:21 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 1/6] worktree.c: fix indentation

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 worktree.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to worktree.c +4 −4
diff --git a/worktree.c b/worktree.c
index 981f810..6181a66 100644
--- a/worktree.c
+++ b/worktree.c
@@ -176,10 +176,10 @@ struct worktree **get_worktrees(void)
 			if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
 				continue;
 
-				if ((linked = get_linked_worktree(d->d_name))) {
-					ALLOC_GROW(list, counter + 1, alloc);
-					list[counter++] = linked;
-				}
+			if ((linked = get_linked_worktree(d->d_name))) {
+				ALLOC_GROW(list, counter + 1, alloc);
+				list[counter++] = linked;
+			}
 		}
 		closedir(dir);
 	}
-- 
2.7.0.96.g5373197
Nguyễn Thái Ngọc Duy· Jan 18, 2016, 11:21 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 2/6] worktree: stop supporting moving worktrees manually

The current update_linked_gitdir() has a bug that can create "gitdir" file in non-multi-worktree setup. Worse, sometimes it can write relative path to "gitdir" file, which will not work (e.g. "git worktree list" will display the worktree's location incorrectly)

Instead of fixing this, we step back a bit. The original design was probably not well thought out. For now, if the user manually moves a worktree, they have to fix up "gitdir" file manually or the worktree will get pruned.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-worktree.txt |  6 ++----
 setup.c                        | 12 ------------
 2 files changed, 2 insertions(+), 16 deletions(-)
Show changes to 2 files +2 −16

Documentation/git-worktree.txt, setup.c

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 5b9ad04..4814f48 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -33,10 +33,8 @@ The working tree's administrative files in the repository (see
 clean up any stale administrative files.
 
 If you move a linked working tree to another file system, or
-within a file system that does not support hard links, you need to run
-at least one git command inside the linked working tree
-(e.g. `git status`) in order to update its administrative files in the
-repository so that they do not get automatically pruned.
+within a file system that does not support hard links, you need to update
+$GIT_DIR/worktrees/<id>/gitdir so that they do not get automatically pruned.
 
 If a linked working tree is stored on a portable device or network share
 which is not always mounted, you can prevent its administrative files from
diff --git a/setup.c b/setup.c
index d343725..6ee2b23 100644
--- a/setup.c
+++ b/setup.c
@@ -434,17 +434,6 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)
 	return ret;
 }
 
-static void update_linked_gitdir(const char *gitfile, const char *gitdir)
-{
-	struct strbuf path = STRBUF_INIT;
-	struct stat st;
-
-	strbuf_addf(&path, "%s/gitdir", gitdir);
-	if (stat(path.buf, &st) || st.st_mtime + 24 * 3600 < time(NULL))
-		write_file(path.buf, "%s", gitfile);
-	strbuf_release(&path);
-}
-
 /*
  * Try to read the location of the git directory from the .git file,
  * return path to git directory if found.
@@ -514,7 +503,6 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 		error_code = READ_GITFILE_ERR_NOT_A_REPO;
 		goto cleanup_return;
 	}
-	update_linked_gitdir(path, dir);
 	path = real_path(dir);
 
 cleanup_return:
-- 
2.7.0.96.g5373197
Eric Sunshine· Jan 18, 2016, 18:24 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 2/6] worktree: stop supporting moving worktrees manually

On Mon, Jan 18, 2016 at 6:21 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 21 quoted lines
> The current update_linked_gitdir() has a bug that can create "gitdir"
> file in non-multi-worktree setup. Worse, sometimes it can write relative
> path to "gitdir" file, which will not work (e.g. "git worktree list"
> will display the worktree's location incorrectly)
>
> Instead of fixing this, we step back a bit. The original design was
> probably not well thought out. For now, if the user manually moves a
> worktree, they have to fix up "gitdir" file manually or the worktree
> will get pruned.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -33,10 +33,8 @@ The working tree's administrative files in the repository (see
>  If you move a linked working tree to another file system, or
> -within a file system that does not support hard links, you need to run
> -at least one git command inside the linked working tree
> -(e.g. `git status`) in order to update its administrative files in the
> -repository so that they do not get automatically pruned.
> +within a file system that does not support hard links, you need to update
> +$GIT_DIR/worktrees/<id>/gitdir so that they do not get automatically pruned.

It seems kind of sad to change this text in this patch and then immediately change it again in the next patch. You could instead combine the two patches (and add a "Helped-by: Eric" if you want to credit me).

Nguyễn Thái Ngọc Duy· Jan 18, 2016, 11:21 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 3/6] worktree.txt: how to fix up after moving a worktree

From: Eric Sunshine <ericsunshine@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-worktree.txt | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
Show changes to Documentation/git-worktree.txt +10 −3
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 4814f48..62c76c1 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -32,9 +32,9 @@ The working tree's administrative files in the repository (see
 `git worktree prune` in the main or any linked working tree to
 clean up any stale administrative files.
 
-If you move a linked working tree to another file system, or
-within a file system that does not support hard links, you need to update
-$GIT_DIR/worktrees/<id>/gitdir so that they do not get automatically pruned.
+If you move a linked working tree, you need to manually update the
+administrative files so that they do not get pruned automatically. See
+section "DETAILS" for more information.
 
 If a linked working tree is stored on a portable device or network share
 which is not always mounted, you can prevent its administrative files from
@@ -135,6 +135,13 @@ thumb is do not make any assumption about whether a path belongs to
 $GIT_DIR or $GIT_COMMON_DIR when you need to directly access something
 inside $GIT_DIR. Use `git rev-parse --git-path` to get the final path.
 
+If you move a linked working tree, you need to update the 'gitdir' file
+in the entry's directory. For example, if a linked working tree is moved
+to `/newpath/test-next` and its `.git` file points to
+`/path/main/.git/worktrees/test-next`, then update
+`/path/main/.git/worktrees/test-next/gitdir` to reference `/newpath/test-next`
+instead.
+
 To prevent a $GIT_DIR/worktrees entry from being pruned (which
 can be useful in some situations, such as when the
 entry's working tree is stored on a portable device), add a file named
-- 
2.7.0.96.g5373197
Eric Sunshine· Jan 18, 2016, 18:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 3/6] worktree.txt: how to fix up after moving a worktree

On Mon, Jan 18, 2016 at 6:21 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> From: Eric Sunshine <ericsunshine@gmail.com>
Let's use sunshine@sunshineco.com instead.

If you want to keep this patch separate from the previous patch, then perhaps add a commit message here saying something like:

    Following the example of af189b4 (Documentation/git-worktree:
    split technical info from general description, 2015-07-06), keep the
    high-level overview free of low-level details about updating
    administrative files when moving a worktree, and instead mention
    $GIT_DIR/worktrees/<id>/gitdir in the "DETAILS" section.
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Show 38 quoted lines
> ---
>  Documentation/git-worktree.txt | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 4814f48..62c76c1 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -32,9 +32,9 @@ The working tree's administrative files in the repository (see
>  `git worktree prune` in the main or any linked working tree to
>  clean up any stale administrative files.
>
> -If you move a linked working tree to another file system, or
> -within a file system that does not support hard links, you need to update
> -$GIT_DIR/worktrees/<id>/gitdir so that they do not get automatically pruned.
> +If you move a linked working tree, you need to manually update the
> +administrative files so that they do not get pruned automatically. See
> +section "DETAILS" for more information.
>
>  If a linked working tree is stored on a portable device or network share
>  which is not always mounted, you can prevent its administrative files from
> @@ -135,6 +135,13 @@ thumb is do not make any assumption about whether a path belongs to
>  $GIT_DIR or $GIT_COMMON_DIR when you need to directly access something
>  inside $GIT_DIR. Use `git rev-parse --git-path` to get the final path.
>
> +If you move a linked working tree, you need to update the 'gitdir' file
> +in the entry's directory. For example, if a linked working tree is moved
> +to `/newpath/test-next` and its `.git` file points to
> +`/path/main/.git/worktrees/test-next`, then update
> +`/path/main/.git/worktrees/test-next/gitdir` to reference `/newpath/test-next`
> +instead.
> +
>  To prevent a $GIT_DIR/worktrees entry from being pruned (which
>  can be useful in some situations, such as when the
>  entry's working tree is stored on a portable device), add a file named
> --
> 2.7.0.96.g5373197
>
Duy Nguyen· Jan 19, 2016, 05:20 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 3/6] worktree.txt: how to fix up after moving a worktree

On Tue, Jan 19, 2016 at 1:30 AM, Eric Sunshine <ericsunshine@gmail.com> wrote:
Show 6 quoted lines
> On Mon, Jan 18, 2016 at 6:21 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> From: Eric Sunshine <ericsunshine@gmail.com>
>
> Let's use sunshine@sunshineco.com instead.
>
> If you want to keep this patch separate from the previous patch, then

I don't want to make you feel sad. So I'll combine the two patches into one. Noted the email address for helped-by though.

Show 7 quoted lines
> perhaps add a commit message here saying something like:
>
>     Following the example of af189b4 (Documentation/git-worktree:
>     split technical info from general description, 2015-07-06), keep the
>     high-level overview free of low-level details about updating
>     administrative files when moving a worktree, and instead mention
>     $GIT_DIR/worktrees/<id>/gitdir in the "DETAILS" section.
-- 
Duy
Nguyễn Thái Ngọc Duy· Jan 18, 2016, 11:21 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 4/6] abspath.c: add and use real_path_dup()

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 abspath.c         | 5 +++++
 builtin/clone.c   | 2 +-
 builtin/init-db.c | 6 +++---
 cache.h           | 1 +
 setup.c           | 2 +-
 5 files changed, 11 insertions(+), 5 deletions(-)
Show changes to 5 files +11 −5

abspath.c, builtin/clone.c, builtin/init-db.c, cache.h, setup.c

diff --git a/abspath.c b/abspath.c
index 5edb4e7..ca44eb9 100644
--- a/abspath.c
+++ b/abspath.c
@@ -135,6 +135,11 @@ const char *real_path(const char *path)
 	return real_path_internal(path, 1);
 }
 
+char *real_path_dup(const char *path)
+{
+	return xstrdup(real_path(path));
+}
+
 const char *real_path_if_valid(const char *path)
 {
 	return real_path_internal(path, 0);
diff --git a/builtin/clone.c b/builtin/clone.c
index a0b3cd9..7652e03 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -280,7 +280,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
 	struct strbuf alternate = STRBUF_INIT;
 
 	/* Beware: read_gitfile(), real_path() and mkpath() return static buffer */
-	ref_git = xstrdup(real_path(item->string));
+	ref_git = real_path_dup(item->string);
 
 	repo = read_gitfile(ref_git);
 	if (!repo)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 07229d6..65c95fd 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -315,7 +315,7 @@ int set_git_dir_init(const char *git_dir, const char *real_git_dir,
 		 * make sure symlinks are resolved because we'll be
 		 * moving the target repo later on in separate_git_dir()
 		 */
-		git_link = xstrdup(real_path(git_dir));
+		git_link = real_path_dup(git_dir);
 		set_git_dir(real_path(real_git_dir));
 	}
 	else {
@@ -480,7 +480,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);
 
 	if (real_git_dir && !is_absolute_path(real_git_dir))
-		real_git_dir = xstrdup(real_path(real_git_dir));
+		real_git_dir = real_path_dup(real_git_dir);
 
 	if (argc == 1) {
 		int mkdir_tried = 0;
@@ -551,7 +551,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 		const char *git_dir_parent = strrchr(git_dir, '/');
 		if (git_dir_parent) {
 			char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
-			git_work_tree_cfg = xstrdup(real_path(rel));
+			git_work_tree_cfg = real_path_dup(rel);
 			free(rel);
 		}
 		if (!git_work_tree_cfg)
diff --git a/cache.h b/cache.h
index c63fcc1..fbe29ac 100644
--- a/cache.h
+++ b/cache.h
@@ -960,6 +960,7 @@ static inline int is_absolute_path(const char *path)
 int is_directory(const char *);
 const char *real_path(const char *path);
 const char *real_path_if_valid(const char *path);
+char *real_path_dup(const char *path);
 const char *absolute_path(const char *path);
 const char *remove_leading_path(const char *in, const char *prefix);
 const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);
diff --git a/setup.c b/setup.c
index 6ee2b23..8b02429 100644
--- a/setup.c
+++ b/setup.c
@@ -643,7 +643,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
 	/* --work-tree is set without --git-dir; use discovered one */
 	if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
 		if (offset != cwd->len && !is_absolute_path(gitdir))
-			gitdir = xstrdup(real_path(gitdir));
+			gitdir = real_path_dup(gitdir);
 		if (chdir(cwd->buf))
 			die_errno("Could not come back to cwd");
 		return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
-- 
2.7.0.96.g5373197
Nguyễn Thái Ngọc Duy· Jan 18, 2016, 11:21 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 5/6] setup.c: record the location of .git file

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache.h |  1 +
 setup.c | 12 ++++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)
Show changes to 2 files +11 −2

cache.h, setup.c

diff --git a/cache.h b/cache.h
index fbe29ac..c912afb 100644
--- a/cache.h
+++ b/cache.h
@@ -1728,6 +1728,7 @@ const char *split_cmdline_strerror(int cmdline_errno);
 struct startup_info {
 	int have_repository;
 	const char *prefix;
+	const char *first_gitfile;
 };
 extern struct startup_info *startup_info;
 
diff --git a/setup.c b/setup.c
index 8b02429..0489f54 100644
--- a/setup.c
+++ b/setup.c
@@ -550,6 +550,8 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 	gitfile = (char*)read_gitfile(gitdirenv);
 	if (gitfile) {
 		gitfile = xstrdup(gitfile);
+		if (startup_info && !startup_info->first_gitfile)
+			startup_info->first_gitfile = real_path_dup(gitdirenv);
 		gitdirenv = gitfile;
 	}
 
@@ -834,9 +836,12 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 		current_device = get_device_or_die(".", NULL, 0);
 	for (;;) {
 		gitfile = (char*)read_gitfile(DEFAULT_GIT_DIR_ENVIRONMENT);
-		if (gitfile)
+		if (gitfile) {
 			gitdirenv = gitfile = xstrdup(gitfile);
-		else {
+			if (startup_info && !startup_info->first_gitfile)
+				startup_info->first_gitfile =
+					real_path_dup(DEFAULT_GIT_DIR_ENVIRONMENT);
+		} else {
 			if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
 				gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
 		}
@@ -885,6 +890,9 @@ const char *setup_git_directory_gently(int *nongit_ok)
 {
 	const char *prefix;
 
+	if (startup_info)
+		startup_info->first_gitfile = NULL;
+
 	prefix = setup_git_directory_gently_1(nongit_ok);
 	if (prefix)
 		setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
-- 
2.7.0.96.g5373197
Nguyễn Thái Ngọc Duy· Jan 18, 2016, 11:21 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 6/6] worktree: new command to fix up worktree's info after moving

This is a low-level command that can be used to correct worktree information after a worktree is moved. The idea is like 'index refresh'. In future we may do "worktree refresh" automatically to keep it from being pruned.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-worktree.txt |  8 +++++++-
 builtin/worktree.c             | 18 ++++++++++++++++++
 t/t1501-worktree.sh            |  9 +++++++++
 3 files changed, 34 insertions(+), 1 deletion(-)
Show changes to 3 files +34 −1

Documentation/git-worktree.txt, builtin/worktree.c, t/t1501-worktree.sh

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 62c76c1..306aeec 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -12,6 +12,7 @@ SYNOPSIS
 'git worktree add' [-f] [--detach] [-b <new-branch>] <path> [<branch>]
 'git worktree prune' [-n] [-v] [--expire <expire>]
 'git worktree list' [--porcelain]
+'git worktree refresh'
 
 DESCRIPTION
 -----------
@@ -65,6 +66,11 @@ each of the linked worktrees.  The output details include if the worktree is
 bare, the revision currently checked out, and the branch currently checked out
 (or 'detached HEAD' if none).
 
+refresh::
+
+This command is required to update worktree's information after it's moved.
+Executed from inside the moved worktree.
+
 OPTIONS
 -------
 
@@ -140,7 +146,7 @@ in the entry's directory. For example, if a linked working tree is moved
 to `/newpath/test-next` and its `.git` file points to
 `/path/main/.git/worktrees/test-next`, then update
 `/path/main/.git/worktrees/test-next/gitdir` to reference `/newpath/test-next`
-instead.
+instead. Alternatively you can run "git worktree refresh".
 
 To prevent a $GIT_DIR/worktrees entry from being pruned (which
 can be useful in some situations, such as when the
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 475b958..0183ce0 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -445,6 +445,22 @@ static int list(int ac, const char **av, const char *prefix)
 	return 0;
 }
 
+static int refresh(int ac, const char **av, const char *prefix)
+{
+	const char *gitdir;
+
+	if (ac != 1)
+		die(_("Arguments not expected"));
+
+	gitdir = git_pathdup("gitdir");
+	if (access(gitdir, F_OK))
+		return 0;
+	if (!startup_info->first_gitfile)
+		die("BUG: .git file's location not found");
+	write_file(gitdir, "%s", startup_info->first_gitfile);
+	return 0;
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
 	struct option options[] = {
@@ -459,5 +475,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
 		return prune(ac - 1, av + 1, prefix);
 	if (!strcmp(av[1], "list"))
 		return list(ac - 1, av + 1, prefix);
+	if (!strcmp(av[1], "refresh"))
+		return refresh(ac - 1, av + 1, prefix);
 	usage_with_options(worktree_usage, options);
 }
diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index cc5b870..821831b 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -423,4 +423,13 @@ test_expect_success '$GIT_WORK_TREE overrides $GIT_DIR/common' '
 	)
 '
 
+test_expect_success 'worktree refresh corrects gitdir file' '
+	git worktree add test-refresh &&
+	P=repo.git/worktrees/test-refresh/gitdir &&
+	echo corrupt >$P &&
+	git -C test-refresh worktree refresh &&
+	echo "$TRASH_DIRECTORY/test-refresh/.git" >expected &&
+	test_cmp expected $P
+'
+
 test_done
-- 
2.7.0.96.g5373197
Philip Oakley· Jan 18, 2016, 11:21 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 6/6] worktree: new command to fix up worktree's info after moving

From: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Show 48 quoted lines
> This is a low-level command that can be used to correct worktree
> information after a worktree is moved. The idea is like 'index refresh'.
> In future we may do "worktree refresh" automatically to keep it from
> being pruned.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> Documentation/git-worktree.txt |  8 +++++++-
> builtin/worktree.c             | 18 ++++++++++++++++++
> t/t1501-worktree.sh            |  9 +++++++++
> 3 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-worktree.txt 
> b/Documentation/git-worktree.txt
> index 62c76c1..306aeec 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -12,6 +12,7 @@ SYNOPSIS
> 'git worktree add' [-f] [--detach] [-b <new-branch>] <path> [<branch>]
> 'git worktree prune' [-n] [-v] [--expire <expire>]
> 'git worktree list' [--porcelain]
> +'git worktree refresh'
>
> DESCRIPTION
> -----------
> @@ -65,6 +66,11 @@ each of the linked worktrees.  The output details 
> include if the worktree is
> bare, the revision currently checked out, and the branch currently checked 
> out
> (or 'detached HEAD' if none).
>
> +refresh::
> +
> +This command is required to update worktree's information after it's 
> moved.
> +Executed from inside the moved worktree.
> +
> OPTIONS
> -------
>
> @@ -140,7 +146,7 @@ in the entry's directory. For example, if a linked 
> working tree is moved
> to `/newpath/test-next` and its `.git` file points to
> `/path/main/.git/worktrees/test-next`, then update
> `/path/main/.git/worktrees/test-next/gitdir` to reference 
> `/newpath/test-next`
> -instead.
> +instead. Alternatively you can run "git worktree refresh".
Shouldn't this note also include the caveat about the run location?
...run "git worktree refresh" from inside the moved worktree.
Show 62 quoted lines
>
> To prevent a $GIT_DIR/worktrees entry from being pruned (which
> can be useful in some situations, such as when the
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 475b958..0183ce0 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -445,6 +445,22 @@ static int list(int ac, const char **av, const char 
> *prefix)
>  return 0;
> }
>
> +static int refresh(int ac, const char **av, const char *prefix)
> +{
> + const char *gitdir;
> +
> + if (ac != 1)
> + die(_("Arguments not expected"));
> +
> + gitdir = git_pathdup("gitdir");
> + if (access(gitdir, F_OK))
> + return 0;
> + if (!startup_info->first_gitfile)
> + die("BUG: .git file's location not found");
> + write_file(gitdir, "%s", startup_info->first_gitfile);
> + return 0;
> +}
> +
> int cmd_worktree(int ac, const char **av, const char *prefix)
> {
>  struct option options[] = {
> @@ -459,5 +475,7 @@ int cmd_worktree(int ac, const char **av, const char 
> *prefix)
>  return prune(ac - 1, av + 1, prefix);
>  if (!strcmp(av[1], "list"))
>  return list(ac - 1, av + 1, prefix);
> + if (!strcmp(av[1], "refresh"))
> + return refresh(ac - 1, av + 1, prefix);
>  usage_with_options(worktree_usage, options);
> }
> diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
> index cc5b870..821831b 100755
> --- a/t/t1501-worktree.sh
> +++ b/t/t1501-worktree.sh
> @@ -423,4 +423,13 @@ test_expect_success '$GIT_WORK_TREE overrides 
> $GIT_DIR/common' '
>  )
> '
>
> +test_expect_success 'worktree refresh corrects gitdir file' '
> + git worktree add test-refresh &&
> + P=repo.git/worktrees/test-refresh/gitdir &&
> + echo corrupt >$P &&
> + git -C test-refresh worktree refresh &&
> + echo "$TRASH_DIRECTORY/test-refresh/.git" >expected &&
> + test_cmp expected $P
> +'
> +
> test_done
> -- 
> 2.7.0.96.g5373197
>

-- Philip

Junio C Hamano· Jan 19, 2016, 18:25 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 6/6] worktree: new command to fix up worktree's info after moving

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 7 quoted lines
> This is a low-level command that can be used to correct worktree
> information after a worktree is moved. The idea is like 'index refresh'.
> In future we may do "worktree refresh" automatically to keep it from
> being pruned.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
I cannot help but think this is backwards.

Before this step, the users were essentially forbidden from using "mv" to move a worktree, as doing so without telling Git can make things inconsistent. Why not keep that prohibition, and give one and only official supported way to move a worktree, e.g. "worktree mv", that explicitly tells Git what is being moved from where to where?

It is prudent to avoid having to do things automatically, especially that you already were burned once to be overly clever by doing things automatically, no? And telling users not to "mv" and instead to use "worktree mv" would be one way to avoid having to "refresh" automatically.

Show 91 quoted lines
>  Documentation/git-worktree.txt |  8 +++++++-
>  builtin/worktree.c             | 18 ++++++++++++++++++
>  t/t1501-worktree.sh            |  9 +++++++++
>  3 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 62c76c1..306aeec 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -12,6 +12,7 @@ SYNOPSIS
>  'git worktree add' [-f] [--detach] [-b <new-branch>] <path> [<branch>]
>  'git worktree prune' [-n] [-v] [--expire <expire>]
>  'git worktree list' [--porcelain]
> +'git worktree refresh'
>  
>  DESCRIPTION
>  -----------
> @@ -65,6 +66,11 @@ each of the linked worktrees.  The output details include if the worktree is
>  bare, the revision currently checked out, and the branch currently checked out
>  (or 'detached HEAD' if none).
>  
> +refresh::
> +
> +This command is required to update worktree's information after it's moved.
> +Executed from inside the moved worktree.
> +
>  OPTIONS
>  -------
>  
> @@ -140,7 +146,7 @@ in the entry's directory. For example, if a linked working tree is moved
>  to `/newpath/test-next` and its `.git` file points to
>  `/path/main/.git/worktrees/test-next`, then update
>  `/path/main/.git/worktrees/test-next/gitdir` to reference `/newpath/test-next`
> -instead.
> +instead. Alternatively you can run "git worktree refresh".
>  
>  To prevent a $GIT_DIR/worktrees entry from being pruned (which
>  can be useful in some situations, such as when the
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 475b958..0183ce0 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -445,6 +445,22 @@ static int list(int ac, const char **av, const char *prefix)
>  	return 0;
>  }
>  
> +static int refresh(int ac, const char **av, const char *prefix)
> +{
> +	const char *gitdir;
> +
> +	if (ac != 1)
> +		die(_("Arguments not expected"));
> +
> +	gitdir = git_pathdup("gitdir");
> +	if (access(gitdir, F_OK))
> +		return 0;
> +	if (!startup_info->first_gitfile)
> +		die("BUG: .git file's location not found");
> +	write_file(gitdir, "%s", startup_info->first_gitfile);
> +	return 0;
> +}
> +
>  int cmd_worktree(int ac, const char **av, const char *prefix)
>  {
>  	struct option options[] = {
> @@ -459,5 +475,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
>  		return prune(ac - 1, av + 1, prefix);
>  	if (!strcmp(av[1], "list"))
>  		return list(ac - 1, av + 1, prefix);
> +	if (!strcmp(av[1], "refresh"))
> +		return refresh(ac - 1, av + 1, prefix);
>  	usage_with_options(worktree_usage, options);
>  }
> diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
> index cc5b870..821831b 100755
> --- a/t/t1501-worktree.sh
> +++ b/t/t1501-worktree.sh
> @@ -423,4 +423,13 @@ test_expect_success '$GIT_WORK_TREE overrides $GIT_DIR/common' '
>  	)
>  '
>  
> +test_expect_success 'worktree refresh corrects gitdir file' '
> +	git worktree add test-refresh &&
> +	P=repo.git/worktrees/test-refresh/gitdir &&
> +	echo corrupt >$P &&
> +	git -C test-refresh worktree refresh &&
> +	echo "$TRASH_DIRECTORY/test-refresh/.git" >expected &&
> +	test_cmp expected $P
> +'
> +
>  test_done
Nguyễn Thái Ngọc Duy· Jan 22, 2016, 08:35 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 0/2] Kill and replace update_linked_gitdir()

v3 kills the update_linked_gitdir's replacement in v2. I'll come back to "worktree mv" (and think more about hardlink support) once I'm done with worktree's config split.

Nguyễn Thái Ngọc Duy (2):
  worktree.c: fix indentation
  worktree: stop supporting moving worktrees manually
 Documentation/git-worktree.txt | 15 ++++++++++-----
 setup.c                        | 12 ------------
 worktree.c                     |  8 ++++----
 3 files changed, 14 insertions(+), 21 deletions(-)
-- 
2.7.0.125.g9eec362
Nguyễn Thái Ngọc Duy· Jan 22, 2016, 08:35 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 1/2] worktree.c: fix indentation

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 worktree.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to worktree.c +4 −4
diff --git a/worktree.c b/worktree.c
index 981f810..6181a66 100644
--- a/worktree.c
+++ b/worktree.c
@@ -176,10 +176,10 @@ struct worktree **get_worktrees(void)
 			if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
 				continue;
 
-				if ((linked = get_linked_worktree(d->d_name))) {
-					ALLOC_GROW(list, counter + 1, alloc);
-					list[counter++] = linked;
-				}
+			if ((linked = get_linked_worktree(d->d_name))) {
+				ALLOC_GROW(list, counter + 1, alloc);
+				list[counter++] = linked;
+			}
 		}
 		closedir(dir);
 	}
-- 
2.7.0.125.g9eec362
Nguyễn Thái Ngọc Duy· Jan 22, 2016, 08:35 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 2/2] worktree: stop supporting moving worktrees manually

The current update_linked_gitdir() has a bug that can create "gitdir" file in non-multi-worktree setup. Worse, sometimes it can write relative path to "gitdir" file, which will not work (e.g. "git worktree list" will display the worktree's location incorrectly)

Instead of fixing this, we step back a bit. The original design was probably not well thought out. For now, if the user manually moves a worktree, they have to fix up "gitdir" file manually or the worktree will get pruned.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-worktree.txt | 15 ++++++++++-----
 setup.c                        | 12 ------------
 2 files changed, 10 insertions(+), 17 deletions(-)
Show changes to 2 files +10 −17

Documentation/git-worktree.txt, setup.c

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 5b9ad04..62c76c1 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -32,11 +32,9 @@ The working tree's administrative files in the repository (see
 `git worktree prune` in the main or any linked working tree to
 clean up any stale administrative files.
 
-If you move a linked working tree to another file system, or
-within a file system that does not support hard links, you need to run
-at least one git command inside the linked working tree
-(e.g. `git status`) in order to update its administrative files in the
-repository so that they do not get automatically pruned.
+If you move a linked working tree, you need to manually update the
+administrative files so that they do not get pruned automatically. See
+section "DETAILS" for more information.
 
 If a linked working tree is stored on a portable device or network share
 which is not always mounted, you can prevent its administrative files from
@@ -137,6 +135,13 @@ thumb is do not make any assumption about whether a path belongs to
 $GIT_DIR or $GIT_COMMON_DIR when you need to directly access something
 inside $GIT_DIR. Use `git rev-parse --git-path` to get the final path.
 
+If you move a linked working tree, you need to update the 'gitdir' file
+in the entry's directory. For example, if a linked working tree is moved
+to `/newpath/test-next` and its `.git` file points to
+`/path/main/.git/worktrees/test-next`, then update
+`/path/main/.git/worktrees/test-next/gitdir` to reference `/newpath/test-next`
+instead.
+
 To prevent a $GIT_DIR/worktrees entry from being pruned (which
 can be useful in some situations, such as when the
 entry's working tree is stored on a portable device), add a file named
diff --git a/setup.c b/setup.c
index d343725..6ee2b23 100644
--- a/setup.c
+++ b/setup.c
@@ -434,17 +434,6 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)
 	return ret;
 }
 
-static void update_linked_gitdir(const char *gitfile, const char *gitdir)
-{
-	struct strbuf path = STRBUF_INIT;
-	struct stat st;
-
-	strbuf_addf(&path, "%s/gitdir", gitdir);
-	if (stat(path.buf, &st) || st.st_mtime + 24 * 3600 < time(NULL))
-		write_file(path.buf, "%s", gitfile);
-	strbuf_release(&path);
-}
-
 /*
  * Try to read the location of the git directory from the .git file,
  * return path to git directory if found.
@@ -514,7 +503,6 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 		error_code = READ_GITFILE_ERR_NOT_A_REPO;
 		goto cleanup_return;
 	}
-	update_linked_gitdir(path, dir);
 	path = real_path(dir);
 
 cleanup_return:
-- 
2.7.0.125.g9eec362

← back to recent threads