git worktree (re)move

PATCH Superseded Nguyễn Thái Ngọc Duy 4 participants 39 messages Nov 12, 2016 – Dec 19, 2016 2 messages not in the archive Documentation

lore

Share & tools

lore

Nguyễn Thái Ngọc Duy

author

2016-11-12 02:24 UTC #0 lore raw

cover git worktree (re)move

This is mostly a resend from last time [1]. The main difference is patch 10/11 to prevent moving a worktree that contains submodules because these .git files may need rewritten to point to the right place. I'm leaving the rewriting .git files for future (preferably done by "submodules guys").

[1] https://public-inbox.org/git/[email protected]/

Nguyễn Thái Ngọc Duy (11):
copy.c: import copy_file() from busybox copy.c: delete unused code in copy_file() copy.c: convert bb_(p)error_msg to error(_errno) copy.c: style fix
copy.c: convert copy_file() to copy_dir_recursively() worktree.c: add validate_worktree() worktree.c: add update_worktree_location() worktree move: new command
worktree move: accept destination as directory worktree move: refuse to move worktrees with submodules worktree remove: new command

 Documentation/git-worktree.txt         |  28 ++-
 builtin/worktree.c                     | 179 ++++++++++++++++
 cache.h                                |   1 +
 contrib/completion/git-completion.bash |   5 +-
 copy.c                                 | 369 +++++++++++++++++++++++++++++++++
 t/t2028-worktree-move.sh               |  56 +++++
 worktree.c                             |  84 ++++++++
 worktree.h                             |  11 +
 8 files changed, 722 insertions(+), 11 deletions(-)

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-12 02:25 UTC #1 lore raw

PATCH 1/11 copy.c: import copy_file() from busybox

This is busybox's unmodified copy_file() in libbb/copy_file.c from the GPL2+ commit f2c043acfcf9dad9fd3d65821b81f89986bbe54e (busybox: fix uninitialized memory when displaying IPv6 addresses - 2016-01-18). This is a no-op commit. More changes are needed before this new code can compile.

This will be needed for implementing "git worktree move" where we have to move a directory recursively. We can implement it from scratch, but then we will have to deal with corner cases (failure to move, circular symlinks...). And delegating the task to "/bin/mv" takes a way the ability to clean things up properly when things fail and we may have to deal with "mv" differences between platforms.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 copy.c | 331 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 331 insertions(+)

copy.c +331 -0

index 4de6a11..79623ac 100644
@@ -65,3 +65,334 @@ int copy_file_with_time(const char *dst, const char *src, int mode)
 		return copy_times(dst, src);
 	return status;
 }
+
+#if 0
+/* Return:
+ * -1 error, copy not made
+ *  0 copy is made or user answered "no" in interactive mode
+ *    (failures to preserve mode/owner/times are not reported in exit code)
+ */
+int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
+{
+	/* This is a recursive function, try to minimize stack usage */
+	/* NB: each struct stat is ~100 bytes */
+	struct stat source_stat;
+	struct stat dest_stat;
+	smallint retval = 0;
+	smallint dest_exists = 0;
+	smallint ovr;
+
+/* Inverse of cp -d ("cp without -d") */
+#define FLAGS_DEREF (flags & (FILEUTILS_DEREFERENCE + FILEUTILS_DEREFERENCE_L0))
+
+	if ((FLAGS_DEREF ? stat : lstat)(source, &source_stat) < 0) {
+		/* This may be a dangling symlink.
+		 * Making [sym]links to dangling symlinks works, so... */
+		if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK))
+			goto make_links;
+		bb_perror_msg("can't stat '%s'", source);
+		return -1;
+	}
+
+	if (lstat(dest, &dest_stat) < 0) {
+		if (errno != ENOENT) {
+			bb_perror_msg("can't stat '%s'", dest);
+			return -1;
+		}
+	} else {
+		if (source_stat.st_dev == dest_stat.st_dev
+		 && source_stat.st_ino == dest_stat.st_ino
+		) {
+			bb_error_msg("'%s' and '%s' are the same file", source, dest);
+			return -1;
+		}
+		dest_exists = 1;
+	}
+
+#if ENABLE_SELINUX
+	if ((flags & FILEUTILS_PRESERVE_SECURITY_CONTEXT) && is_selinux_enabled() > 0) {
+		security_context_t con;
+		if (lgetfilecon(source, &con) >= 0) {
+			if (setfscreatecon(con) < 0) {
+				bb_perror_msg("can't set setfscreatecon %s", con);
+				freecon(con);
+				return -1;
+			}
+		} else if (errno == ENOTSUP || errno == ENODATA) {
+			setfscreatecon_or_die(NULL);
+		} else {
+			bb_perror_msg("can't lgetfilecon %s", source);
+			return -1;
+		}
+	}
+#endif
+
+	if (S_ISDIR(source_stat.st_mode)) {
+		DIR *dp;
+		const char *tp;
+		struct dirent *d;
+		mode_t saved_umask = 0;
+
+		if (!(flags & FILEUTILS_RECUR)) {
+			bb_error_msg("omitting directory '%s'", source);
+			return -1;
+		}
+
+		/* Did we ever create source ourself before? */
+		tp = is_in_ino_dev_hashtable(&source_stat);
+		if (tp) {
+			/* We did! it's a recursion! man the lifeboats... */
+			bb_error_msg("recursion detected, omitting directory '%s'",
+					source);
+			return -1;
+		}
+
+		if (dest_exists) {
+			if (!S_ISDIR(dest_stat.st_mode)) {
+				bb_error_msg("target '%s' is not a directory", dest);
+				return -1;
+			}
+			/* race here: user can substitute a symlink between
+			 * this check and actual creation of files inside dest */
+		} else {
+			/* Create DEST */
+			mode_t mode;
+			saved_umask = umask(0);
+
+			mode = source_stat.st_mode;
+			if (!(flags & FILEUTILS_PRESERVE_STATUS))
+				mode = source_stat.st_mode & ~saved_umask;
+			/* Allow owner to access new dir (at least for now) */
+			mode |= S_IRWXU;
+			if (mkdir(dest, mode) < 0) {
+				umask(saved_umask);
+				bb_perror_msg("can't create directory '%s'", dest);
+				return -1;
+			}
+			umask(saved_umask);
+			/* need stat info for add_to_ino_dev_hashtable */
+			if (lstat(dest, &dest_stat) < 0) {
+				bb_perror_msg("can't stat '%s'", dest);
+				return -1;
+			}
+		}
+		/* remember (dev,inode) of each created dir.
+		 * NULL: name is not remembered */
+		add_to_ino_dev_hashtable(&dest_stat, NULL);
+
+		/* Recursively copy files in SOURCE */
+		dp = opendir(source);
+		if (dp == NULL) {
+			retval = -1;
+			goto preserve_mode_ugid_time;
+		}
+
+		while ((d = readdir(dp)) != NULL) {
+			char *new_source, *new_dest;
+
+			new_source = concat_subpath_file(source, d->d_name);
+			if (new_source == NULL)
+				continue;
+			new_dest = concat_path_file(dest, d->d_name);
+			if (copy_file(new_source, new_dest, flags & ~FILEUTILS_DEREFERENCE_L0) < 0)
+				retval = -1;
+			free(new_source);
+			free(new_dest);
+		}
+		closedir(dp);
+
+		if (!dest_exists
+		 && chmod(dest, source_stat.st_mode & ~saved_umask) < 0
+		) {
+			bb_perror_msg("can't preserve %s of '%s'", "permissions", dest);
+			/* retval = -1; - WRONG! copy *WAS* made */
+		}
+		goto preserve_mode_ugid_time;
+	}
+
+	if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK)) {
+		int (*lf)(const char *oldpath, const char *newpath);
+ make_links:
+		/* Hmm... maybe
+		 * if (DEREF && MAKE_SOFTLINK) source = realpath(source) ?
+		 * (but realpath returns NULL on dangling symlinks...) */
+		lf = (flags & FILEUTILS_MAKE_SOFTLINK) ? symlink : link;
+		if (lf(source, dest) < 0) {
+			ovr = ask_and_unlink(dest, flags);
+			if (ovr <= 0)
+				return ovr;
+			if (lf(source, dest) < 0) {
+				bb_perror_msg("can't create link '%s'", dest);
+				return -1;
+			}
+		}
+		/* _Not_ jumping to preserve_mode_ugid_time:
+		 * (sym)links don't have those */
+		return 0;
+	}
+
+	if (/* "cp thing1 thing2" without -R: just open and read() from thing1 */
+	    !(flags & FILEUTILS_RECUR)
+	    /* "cp [-opts] regular_file thing2" */
+	 || S_ISREG(source_stat.st_mode)
+	 /* DEREF uses stat, which never returns S_ISLNK() == true.
+	  * So the below is never true: */
+	 /* || (FLAGS_DEREF && S_ISLNK(source_stat.st_mode)) */
+	) {
+		int src_fd;
+		int dst_fd;
+		mode_t new_mode;
+
+		if (!FLAGS_DEREF && S_ISLNK(source_stat.st_mode)) {
+			/* "cp -d symlink dst": create a link */
+			goto dont_cat;
+		}
+
+		if (ENABLE_FEATURE_PRESERVE_HARDLINKS && !FLAGS_DEREF) {
+			const char *link_target;
+			link_target = is_in_ino_dev_hashtable(&source_stat);
+			if (link_target) {
+				if (link(link_target, dest) < 0) {
+					ovr = ask_and_unlink(dest, flags);
+					if (ovr <= 0)
+						return ovr;
+					if (link(link_target, dest) < 0) {
+						bb_perror_msg("can't create link '%s'", dest);
+						return -1;
+					}
+				}
+				return 0;
+			}
+			add_to_ino_dev_hashtable(&source_stat, dest);
+		}
+
+		src_fd = open_or_warn(source, O_RDONLY);
+		if (src_fd < 0)
+			return -1;
+
+		/* Do not try to open with weird mode fields */
+		new_mode = source_stat.st_mode;
+		if (!S_ISREG(source_stat.st_mode))
+			new_mode = 0666;
+
+		// POSIX way is a security problem versus (sym)link attacks
+		if (!ENABLE_FEATURE_NON_POSIX_CP) {
+			dst_fd = open(dest, O_WRONLY|O_CREAT|O_TRUNC, new_mode);
+		} else { /* safe way: */
+			dst_fd = open(dest, O_WRONLY|O_CREAT|O_EXCL, new_mode);
+		}
+		if (dst_fd == -1) {
+			ovr = ask_and_unlink(dest, flags);
+			if (ovr <= 0) {
+				close(src_fd);
+				return ovr;
+			}
+			/* It shouldn't exist. If it exists, do not open (symlink attack?) */
+			dst_fd = open3_or_warn(dest, O_WRONLY|O_CREAT|O_EXCL, new_mode);
+			if (dst_fd < 0) {
+				close(src_fd);
+				return -1;
+			}
+		}
+
+#if ENABLE_SELINUX
+		if ((flags & (FILEUTILS_PRESERVE_SECURITY_CONTEXT|FILEUTILS_SET_SECURITY_CONTEXT))
+		 && is_selinux_enabled() > 0
+		) {
+			security_context_t con;
+			if (getfscreatecon(&con) == -1) {
+				bb_perror_msg("getfscreatecon");
+				return -1;
+			}
+			if (con) {
+				if (setfilecon(dest, con) == -1) {
+					bb_perror_msg("setfilecon:%s,%s", dest, con);
+					freecon(con);
+					return -1;
+				}
+				freecon(con);
+			}
+		}
+#endif
+		if (bb_copyfd_eof(src_fd, dst_fd) == -1)
+			retval = -1;
+		/* Careful with writing... */
+		if (close(dst_fd) < 0) {
+			bb_perror_msg("error writing to '%s'", dest);
+			retval = -1;
+		}
+		/* ...but read size is already checked by bb_copyfd_eof */
+		close(src_fd);
+		/* "cp /dev/something new_file" should not
+		 * copy mode of /dev/something */
+		if (!S_ISREG(source_stat.st_mode))
+			return retval;
+		goto preserve_mode_ugid_time;
+	}
+ dont_cat:
+
+	/* Source is a symlink or a special file */
+	/* We are lazy here, a bit lax with races... */
+	if (dest_exists) {
+		errno = EEXIST;
+		ovr = ask_and_unlink(dest, flags);
+		if (ovr <= 0)
+			return ovr;
+	}
+	if (S_ISLNK(source_stat.st_mode)) {
+		char *lpath = xmalloc_readlink_or_warn(source);
+		if (lpath) {
+			int r = symlink(lpath, dest);
+			free(lpath);
+			if (r < 0) {
+				bb_perror_msg("can't create symlink '%s'", dest);
+				return -1;
+			}
+			if (flags & FILEUTILS_PRESERVE_STATUS)
+				if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
+					bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
+		}
+		/* _Not_ jumping to preserve_mode_ugid_time:
+		 * symlinks don't have those */
+		return 0;
+	}
+	if (S_ISBLK(source_stat.st_mode) || S_ISCHR(source_stat.st_mode)
+	 || S_ISSOCK(source_stat.st_mode) || S_ISFIFO(source_stat.st_mode)
+	) {
+		if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0) {
+			bb_perror_msg("can't create '%s'", dest);
+			return -1;
+		}
+	} else {
+		bb_error_msg("unrecognized file '%s' with mode %x", source, source_stat.st_mode);
+		return -1;
+	}
+
+ preserve_mode_ugid_time:
+
+	if (flags & FILEUTILS_PRESERVE_STATUS
+	/* Cannot happen: */
+	/* && !(flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK)) */
+	) {
+		struct timeval times[2];
+
+		times[1].tv_sec = times[0].tv_sec = source_stat.st_mtime;
+		times[1].tv_usec = times[0].tv_usec = 0;
+		/* BTW, utimes sets usec-precision time - just FYI */
+		if (utimes(dest, times) < 0)
+			bb_perror_msg("can't preserve %s of '%s'", "times", dest);
+		if (chown(dest, source_stat.st_uid, source_stat.st_gid) < 0) {
+			source_stat.st_mode &= ~(S_ISUID | S_ISGID);
+			bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
+		}
+		if (chmod(dest, source_stat.st_mode) < 0)
+			bb_perror_msg("can't preserve %s of '%s'", "permissions", dest);
+	}
+
+	if (flags & FILEUTILS_VERBOSE) {
+		printf("'%s' -> '%s'\n", source, dest);
+	}
+
+	return retval;
+}
+#endif

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-12 02:24 UTC #2 lore raw

PATCH 2/11 copy.c: delete unused code in copy_file()

In reply to Nguyễn Thái Ngọc Duy

- selinux preservation code
- make-link code
- delete link dereference code
- non-recursive copy code
- stat no preservation code
- verbose printing code

Some of these are "cp" features that we don't need (for "git worktree move"). Some do not make sense in source-control context (SELinux). Some probably need a reimplementation to better fit in (verbose printing code).

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 copy.c | 101 +++++------------------------------------------------------------
 1 file changed, 7 insertions(+), 94 deletions(-)

copy.c +7 ~~-94~~

index 79623ac..b7a87f1 100644
@@ -82,14 +82,7 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 	smallint dest_exists = 0;
 	smallint ovr;
 
-/* Inverse of cp -d ("cp without -d") */
-#define FLAGS_DEREF (flags & (FILEUTILS_DEREFERENCE + FILEUTILS_DEREFERENCE_L0))
-
-	if ((FLAGS_DEREF ? stat : lstat)(source, &source_stat) < 0) {
-		/* This may be a dangling symlink.
-		 * Making [sym]links to dangling symlinks works, so... */
-		if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK))
-			goto make_links;
+	if (lstat(source, &source_stat) < 0) {
 		bb_perror_msg("can't stat '%s'", source);
 		return -1;
 	}

@@ -109,35 +102,12 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		dest_exists = 1;
 	}
 
-#if ENABLE_SELINUX
-	if ((flags & FILEUTILS_PRESERVE_SECURITY_CONTEXT) && is_selinux_enabled() > 0) {
-		security_context_t con;
-		if (lgetfilecon(source, &con) >= 0) {
-			if (setfscreatecon(con) < 0) {
-				bb_perror_msg("can't set setfscreatecon %s", con);
-				freecon(con);
-				return -1;
-			}
-		} else if (errno == ENOTSUP || errno == ENODATA) {
-			setfscreatecon_or_die(NULL);
-		} else {
-			bb_perror_msg("can't lgetfilecon %s", source);
-			return -1;
-		}
-	}
-#endif
-
 	if (S_ISDIR(source_stat.st_mode)) {
 		DIR *dp;
 		const char *tp;
 		struct dirent *d;
 		mode_t saved_umask = 0;
 
-		if (!(flags & FILEUTILS_RECUR)) {
-			bb_error_msg("omitting directory '%s'", source);
-			return -1;
-		}
-
 		/* Did we ever create source ourself before? */
 		tp = is_in_ino_dev_hashtable(&source_stat);
 		if (tp) {

@@ -160,8 +130,6 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 			saved_umask = umask(0);
 
 			mode = source_stat.st_mode;
-			if (!(flags & FILEUTILS_PRESERVE_STATUS))
-				mode = source_stat.st_mode & ~saved_umask;
 			/* Allow owner to access new dir (at least for now) */
 			mode |= S_IRWXU;
 			if (mkdir(dest, mode) < 0) {

@@ -210,45 +178,17 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		goto preserve_mode_ugid_time;
 	}
 
-	if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK)) {
-		int (*lf)(const char *oldpath, const char *newpath);
- make_links:
-		/* Hmm... maybe
-		 * if (DEREF && MAKE_SOFTLINK) source = realpath(source) ?
-		 * (but realpath returns NULL on dangling symlinks...) */
-		lf = (flags & FILEUTILS_MAKE_SOFTLINK) ? symlink : link;
-		if (lf(source, dest) < 0) {
-			ovr = ask_and_unlink(dest, flags);
-			if (ovr <= 0)
-				return ovr;
-			if (lf(source, dest) < 0) {
-				bb_perror_msg("can't create link '%s'", dest);
-				return -1;
-			}
-		}
-		/* _Not_ jumping to preserve_mode_ugid_time:
-		 * (sym)links don't have those */
-		return 0;
-	}
-
-	if (/* "cp thing1 thing2" without -R: just open and read() from thing1 */
-	    !(flags & FILEUTILS_RECUR)
-	    /* "cp [-opts] regular_file thing2" */
-	 || S_ISREG(source_stat.st_mode)
-	 /* DEREF uses stat, which never returns S_ISLNK() == true.
-	  * So the below is never true: */
-	 /* || (FLAGS_DEREF && S_ISLNK(source_stat.st_mode)) */
-	) {
+	if (S_ISREG(source_stat.st_mode) ) { /* "cp [-opts] regular_file thing2" */
 		int src_fd;
 		int dst_fd;
 		mode_t new_mode;
 
-		if (!FLAGS_DEREF && S_ISLNK(source_stat.st_mode)) {
+		if (S_ISLNK(source_stat.st_mode)) {
 			/* "cp -d symlink dst": create a link */
 			goto dont_cat;
 		}
 
-		if (ENABLE_FEATURE_PRESERVE_HARDLINKS && !FLAGS_DEREF) {
+		if (ENABLE_FEATURE_PRESERVE_HARDLINKS) {
 			const char *link_target;
 			link_target = is_in_ino_dev_hashtable(&source_stat);
 			if (link_target) {

@@ -295,25 +235,6 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 			}
 		}
 
-#if ENABLE_SELINUX
-		if ((flags & (FILEUTILS_PRESERVE_SECURITY_CONTEXT|FILEUTILS_SET_SECURITY_CONTEXT))
-		 && is_selinux_enabled() > 0
-		) {
-			security_context_t con;
-			if (getfscreatecon(&con) == -1) {
-				bb_perror_msg("getfscreatecon");
-				return -1;
-			}
-			if (con) {
-				if (setfilecon(dest, con) == -1) {
-					bb_perror_msg("setfilecon:%s,%s", dest, con);
-					freecon(con);
-					return -1;
-				}
-				freecon(con);
-			}
-		}
-#endif
 		if (bb_copyfd_eof(src_fd, dst_fd) == -1)
 			retval = -1;
 		/* Careful with writing... */

@@ -348,9 +269,8 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 				bb_perror_msg("can't create symlink '%s'", dest);
 				return -1;
 			}
-			if (flags & FILEUTILS_PRESERVE_STATUS)
-				if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
-					bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
+			if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
+				bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
 		}
 		/* _Not_ jumping to preserve_mode_ugid_time:
 		 * symlinks don't have those */

@@ -370,10 +290,7 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 
  preserve_mode_ugid_time:
 
-	if (flags & FILEUTILS_PRESERVE_STATUS
-	/* Cannot happen: */
-	/* && !(flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK)) */
-	) {
+	if (1 /*FILEUTILS_PRESERVE_STATUS*/) {
 		struct timeval times[2];
 
 		times[1].tv_sec = times[0].tv_sec = source_stat.st_mtime;

@@ -389,10 +306,6 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 			bb_perror_msg("can't preserve %s of '%s'", "permissions", dest);
 	}
 
-	if (flags & FILEUTILS_VERBOSE) {
-		printf("'%s' -> '%s'\n", source, dest);
-	}
-
 	return retval;
 }
 #endif

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-12 02:24 UTC #3 lore raw

PATCH 3/11 copy.c: convert bb_(p)error_msg to error(_errno)

In reply to Nguyễn Thái Ngọc Duy

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 copy.c | 85 ++++++++++++++++++++++++------------------------------------------
 1 file changed, 31 insertions(+), 54 deletions(-)

copy.c +31 ~~-54~~

index b7a87f1..074b609 100644
@@ -82,23 +82,16 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 	smallint dest_exists = 0;
 	smallint ovr;
 
-	if (lstat(source, &source_stat) < 0) {
-		bb_perror_msg("can't stat '%s'", source);
-		return -1;
-	}
+	if (lstat(source, &source_stat) < 0)
+		return error_errno(_("can't stat '%s'"), source);
 
 	if (lstat(dest, &dest_stat) < 0) {
-		if (errno != ENOENT) {
-			bb_perror_msg("can't stat '%s'", dest);
-			return -1;
-		}
+		if (errno != ENOENT)
+			return error_errno(_("can't stat '%s'"), dest);
 	} else {
-		if (source_stat.st_dev == dest_stat.st_dev
-		 && source_stat.st_ino == dest_stat.st_ino
-		) {
-			bb_error_msg("'%s' and '%s' are the same file", source, dest);
-			return -1;
-		}
+		if (source_stat.st_dev == dest_stat.st_dev &&
+		    source_stat.st_ino == dest_stat.st_ino)
+			return error(_("'%s' and '%s' are the same file"), source, dest);
 		dest_exists = 1;
 	}

@@ -110,18 +103,14 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 
 		/* Did we ever create source ourself before? */
 		tp = is_in_ino_dev_hashtable(&source_stat);
-		if (tp) {
+		if (tp)
 			/* We did! it's a recursion! man the lifeboats... */
-			bb_error_msg("recursion detected, omitting directory '%s'",
-					source);
-			return -1;
-		}
+			return error(_("recursion detected, omitting directory '%s'"),
+				     source);
 
 		if (dest_exists) {
-			if (!S_ISDIR(dest_stat.st_mode)) {
-				bb_error_msg("target '%s' is not a directory", dest);
-				return -1;
-			}
+			if (!S_ISDIR(dest_stat.st_mode))
+				return error(_("target '%s' is not a directory"), dest);
 			/* race here: user can substitute a symlink between
 			 * this check and actual creation of files inside dest */
 		} else {

@@ -134,15 +123,12 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 			mode |= S_IRWXU;
 			if (mkdir(dest, mode) < 0) {
 				umask(saved_umask);
-				bb_perror_msg("can't create directory '%s'", dest);
-				return -1;
+				return error_errno(_("can't create directory '%s'"), dest);
 			}
 			umask(saved_umask);
 			/* need stat info for add_to_ino_dev_hashtable */
-			if (lstat(dest, &dest_stat) < 0) {
-				bb_perror_msg("can't stat '%s'", dest);
-				return -1;
-			}
+			if (lstat(dest, &dest_stat) < 0)
+				return error_errno(_("can't stat '%s'"), dest);
 		}
 		/* remember (dev,inode) of each created dir.
 		 * NULL: name is not remembered */

@@ -172,7 +158,7 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		if (!dest_exists
 		 && chmod(dest, source_stat.st_mode & ~saved_umask) < 0
 		) {
-			bb_perror_msg("can't preserve %s of '%s'", "permissions", dest);
+			error_errno(_("can't preserve permissions of '%s'"), dest);
 			/* retval = -1; - WRONG! copy *WAS* made */
 		}
 		goto preserve_mode_ugid_time;

@@ -196,10 +182,8 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 					ovr = ask_and_unlink(dest, flags);
 					if (ovr <= 0)
 						return ovr;
-					if (link(link_target, dest) < 0) {
-						bb_perror_msg("can't create link '%s'", dest);
-						return -1;
-					}
+					if (link(link_target, dest) < 0)
+						return error_errno(_("can't create link '%s'"), dest);
 				}
 				return 0;
 			}

@@ -238,10 +222,8 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		if (bb_copyfd_eof(src_fd, dst_fd) == -1)
 			retval = -1;
 		/* Careful with writing... */
-		if (close(dst_fd) < 0) {
-			bb_perror_msg("error writing to '%s'", dest);
-			retval = -1;
-		}
+		if (close(dst_fd) < 0)
+			retval = error_errno(_("error writing to '%s'"), dest);
 		/* ...but read size is already checked by bb_copyfd_eof */
 		close(src_fd);
 		/* "cp /dev/something new_file" should not

@@ -265,12 +247,10 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		if (lpath) {
 			int r = symlink(lpath, dest);
 			free(lpath);
-			if (r < 0) {
-				bb_perror_msg("can't create symlink '%s'", dest);
-				return -1;
-			}
+			if (r < 0)
+				return error_errno(_("can't create symlink '%s'"), dest);
 			if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
-				bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
+				error_errno(_("can't preserve %s of '%s'"), "ownership", dest);
 		}
 		/* _Not_ jumping to preserve_mode_ugid_time:
 		 * symlinks don't have those */

@@ -279,14 +259,11 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 	if (S_ISBLK(source_stat.st_mode) || S_ISCHR(source_stat.st_mode)
 	 || S_ISSOCK(source_stat.st_mode) || S_ISFIFO(source_stat.st_mode)
 	) {
-		if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0) {
-			bb_perror_msg("can't create '%s'", dest);
-			return -1;
-		}
-	} else {
-		bb_error_msg("unrecognized file '%s' with mode %x", source, source_stat.st_mode);
-		return -1;
-	}
+		if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0)
+			return error_errno(_("can't create '%s'"), dest);
+	} else
+		return error(_("unrecognized file '%s' with mode %x"),
+			     source, source_stat.st_mode);
 
  preserve_mode_ugid_time:

@@ -297,13 +274,13 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		times[1].tv_usec = times[0].tv_usec = 0;
 		/* BTW, utimes sets usec-precision time - just FYI */
 		if (utimes(dest, times) < 0)
-			bb_perror_msg("can't preserve %s of '%s'", "times", dest);
+			error_errno(_("can't preserve %s of '%s'"), "times", dest);
 		if (chown(dest, source_stat.st_uid, source_stat.st_gid) < 0) {
 			source_stat.st_mode &= ~(S_ISUID | S_ISGID);
-			bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
+			error_errno(_("can't preserve %s of '%s'"), "ownership", dest);
 		}
 		if (chmod(dest, source_stat.st_mode) < 0)
-			bb_perror_msg("can't preserve %s of '%s'", "permissions", dest);
+			error_errno(_("can't preserve %s of '%s'"), "permissions", dest);
 	}
 
 	return retval;

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-12 02:25 UTC #4 lore raw

PATCH 4/11 copy.c: style fix

In reply to Nguyễn Thái Ngọc Duy

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 copy.c | 50 +++++++++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 21 deletions(-)

copy.c +29 ~~-21~~

index 074b609..60c7d8a 100644
@@ -111,8 +111,10 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		if (dest_exists) {
 			if (!S_ISDIR(dest_stat.st_mode))
 				return error(_("target '%s' is not a directory"), dest);
-			/* race here: user can substitute a symlink between
-			 * this check and actual creation of files inside dest */
+			/*
+			 * race here: user can substitute a symlink between
+			 * this check and actual creation of files inside dest
+			 */
 		} else {
 			/* Create DEST */
 			mode_t mode;

@@ -130,22 +132,24 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 			if (lstat(dest, &dest_stat) < 0)
 				return error_errno(_("can't stat '%s'"), dest);
 		}
-		/* remember (dev,inode) of each created dir.
-		 * NULL: name is not remembered */
+		/*
+		 * remember (dev,inode) of each created dir. name is
+		 * not remembered
+		 */
 		add_to_ino_dev_hashtable(&dest_stat, NULL);
 
 		/* Recursively copy files in SOURCE */
 		dp = opendir(source);
-		if (dp == NULL) {
+		if (!dp) {
 			retval = -1;
 			goto preserve_mode_ugid_time;
 		}
 
-		while ((d = readdir(dp)) != NULL) {
+		while ((d = readdir(dp))) {
 			char *new_source, *new_dest;
 
 			new_source = concat_subpath_file(source, d->d_name);
-			if (new_source == NULL)
+			if (!new_source)
 				continue;
 			new_dest = concat_path_file(dest, d->d_name);
 			if (copy_file(new_source, new_dest, flags & ~FILEUTILS_DEREFERENCE_L0) < 0)

@@ -155,16 +159,15 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		}
 		closedir(dp);
 
-		if (!dest_exists
-		 && chmod(dest, source_stat.st_mode & ~saved_umask) < 0
-		) {
+		if (!dest_exists &&
+		    chmod(dest, source_stat.st_mode & ~saved_umask) < 0) {
 			error_errno(_("can't preserve permissions of '%s'"), dest);
 			/* retval = -1; - WRONG! copy *WAS* made */
 		}
 		goto preserve_mode_ugid_time;
 	}
 
-	if (S_ISREG(source_stat.st_mode) ) { /* "cp [-opts] regular_file thing2" */
+	if (S_ISREG(source_stat.st_mode)) { /* "cp [-opts] regular_file thing2" */
 		int src_fd;
 		int dst_fd;
 		mode_t new_mode;

@@ -199,7 +202,7 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		if (!S_ISREG(source_stat.st_mode))
 			new_mode = 0666;
 
-		// POSIX way is a security problem versus (sym)link attacks
+		/* POSIX way is a security problem versus (sym)link attacks */
 		if (!ENABLE_FEATURE_NON_POSIX_CP) {
 			dst_fd = open(dest, O_WRONLY|O_CREAT|O_TRUNC, new_mode);
 		} else { /* safe way: */

@@ -226,13 +229,15 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 			retval = error_errno(_("error writing to '%s'"), dest);
 		/* ...but read size is already checked by bb_copyfd_eof */
 		close(src_fd);
-		/* "cp /dev/something new_file" should not
-		 * copy mode of /dev/something */
+		/*
+		 * "cp /dev/something new_file" should not
+		 * copy mode of /dev/something
+		 */
 		if (!S_ISREG(source_stat.st_mode))
 			return retval;
 		goto preserve_mode_ugid_time;
 	}
- dont_cat:
+dont_cat:
 
 	/* Source is a symlink or a special file */
 	/* We are lazy here, a bit lax with races... */

@@ -252,20 +257,23 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 			if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
 				error_errno(_("can't preserve %s of '%s'"), "ownership", dest);
 		}
-		/* _Not_ jumping to preserve_mode_ugid_time:
-		 * symlinks don't have those */
+		/*
+		 * _Not_ jumping to preserve_mode_ugid_time: symlinks
+		 * don't have those
+		 */
 		return 0;
 	}
-	if (S_ISBLK(source_stat.st_mode) || S_ISCHR(source_stat.st_mode)
-	 || S_ISSOCK(source_stat.st_mode) || S_ISFIFO(source_stat.st_mode)
-	) {
+	if (S_ISBLK(source_stat.st_mode) ||
+	    S_ISCHR(source_stat.st_mode) ||
+	    S_ISSOCK(source_stat.st_mode) ||
+	    S_ISFIFO(source_stat.st_mode)) {
 		if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0)
 			return error_errno(_("can't create '%s'"), dest);
 	} else
 		return error(_("unrecognized file '%s' with mode %x"),
 			     source, source_stat.st_mode);
 
- preserve_mode_ugid_time:
+preserve_mode_ugid_time:
 
 	if (1 /*FILEUTILS_PRESERVE_STATUS*/) {
 		struct timeval times[2];

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-12 02:25 UTC #5 lore raw

PATCH 5/11 copy.c: convert copy_file() to copy_dir_recursively()

In reply to Nguyễn Thái Ngọc Duy

This finally enables busybox's copy_file() code under a new name (because "copy_file" is already taken in Git code base). Because this comes from busybox, POSIXy (or even Linuxy) behavior is expected. More changes may be needed for Windows support.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 cache.h |   1 +
 copy.c  | 216 ++++++++++++++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 179 insertions(+), 38 deletions(-)

cache.h +1 -0

index a50a61a..a9a72f8 100644
@@ -1857,6 +1857,7 @@ extern void fprintf_or_die(FILE *, const char *fmt, ...);
 extern int copy_fd(int ifd, int ofd);
 extern int copy_file(const char *dst, const char *src, int mode);
 extern int copy_file_with_time(const char *dst, const char *src, int mode);
+extern int copy_dir_recursively(const char *source, const char *dest);
 
 extern void write_or_die(int fd, const void *buf, size_t count);
 extern void fsync_or_die(int fd, const char *);

copy.c +178 ~~-38~~

304 lines not on this page · raw message

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-12 02:24 UTC #6 lore raw

PATCH 6/11 worktree.c: add validate_worktree()

In reply to Nguyễn Thái Ngọc Duy

This function is later used by "worktree move" and "worktree remove" to ensure that we have a good connection between the repository and the worktree. For example, if a worktree is moved manually, the worktree location recorded in $GIT_DIR/worktrees/.../gitdir is incorrect and we should not move that one.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 worktree.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 worktree.h |  5 +++++
 2 files changed, 68 insertions(+)

worktree.c +63 -0

index f7869f8..7e15ec7 100644
@@ -291,6 +291,69 @@ const char *is_worktree_locked(struct worktree *wt)
 	return wt->lock_reason;
 }
 
+static int report(int quiet, const char *fmt, ...)
+{
+	va_list params;
+
+	if (quiet)
+		return -1;
+
+	va_start(params, fmt);
+	vfprintf(stderr, fmt, params);
+	fputc('\n', stderr);
+	va_end(params);
+	return -1;
+}
+
+int validate_worktree(const struct worktree *wt, int quiet)
+{
+	struct strbuf sb = STRBUF_INIT;
+	const char *path;
+	int err;
+
+	if (is_main_worktree(wt)) {
+		/*
+		 * Main worktree using .git file to point to the
+		 * repository would make it impossible to know where
+		 * the actual worktree is if this function is executed
+		 * from another worktree. No .git file support for now.
+		 */
+		strbuf_addf(&sb, "%s/.git", wt->path);
+		if (!is_directory(sb.buf)) {
+			strbuf_release(&sb);
+			return report(quiet, _("'%s/.git' at main worktree is not the repository directory"),
+				      wt->path);
+		}
+		return 0;
+	}
+
+	/*
+	 * Make sure "gitdir" file points to a real .git file and that
+	 * file points back here.
+	 */
+	if (!is_absolute_path(wt->path))
+		return report(quiet, _("'%s' file does not contain absolute path to the worktree location"),
+			      git_common_path("worktrees/%s/gitdir", wt->id));
+
+	strbuf_addf(&sb, "%s/.git", wt->path);
+	if (!file_exists(sb.buf)) {
+		strbuf_release(&sb);
+		return report(quiet, _("'%s/.git' does not exist"), wt->path);
+	}
+
+	path = read_gitfile_gently(sb.buf, &err);
+	strbuf_release(&sb);
+	if (!path)
+		return report(quiet, _("'%s/.git' is not a .git file, error code %d"),
+			      wt->path, err);
+
+	if (fspathcmp(path, real_path(git_common_path("worktrees/%s", wt->id))))
+		return report(quiet, _("'%s' does not point back to"),
+			      wt->path, git_common_path("worktrees/%s", wt->id));
+
+	return 0;
+}
+
 int is_worktree_being_rebased(const struct worktree *wt,
 			      const char *target)
 {

worktree.h +5 -0

index 90e1311..e782ae5 100644
@@ -50,6 +50,11 @@ extern int is_main_worktree(const struct worktree *wt);
  */
 extern const char *is_worktree_locked(struct worktree *wt);
 
+/*
+ * Return zero if the worktree is in good condition.
+ */
+extern int validate_worktree(const struct worktree *wt, int quiet);
+
 /*
  * Free up the memory for worktree(s)
  */

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-12 02:24 UTC #7 lore raw

PATCH 7/11 worktree.c: add update_worktree_location()

In reply to Nguyễn Thái Ngọc Duy

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 worktree.c | 21 +++++++++++++++++++++
 worktree.h |  6 ++++++
 2 files changed, 27 insertions(+)

worktree.c +21 -0

index 7e15ec7..db63758 100644
@@ -354,6 +354,27 @@ int validate_worktree(const struct worktree *wt, int quiet)
 	return 0;
 }
 
+int update_worktree_location(struct worktree *wt, const char *path_)
+{
+	struct strbuf path = STRBUF_INIT;
+	int ret = 0;
+
+	if (is_main_worktree(wt))
+		return 0;
+
+	strbuf_add_absolute_path(&path, path_);
+	if (fspathcmp(wt->path, path.buf)) {
+		write_file(git_common_path("worktrees/%s/gitdir",
+					   wt->id),
+			   "%s/.git", real_path(path.buf));
+		free(wt->path);
+		wt->path = strbuf_detach(&path, NULL);
+		ret = 0;
+	}
+	strbuf_release(&path);
+	return ret;
+}
+
 int is_worktree_being_rebased(const struct worktree *wt,
 			      const char *target)
 {

worktree.h +6 -0

index e782ae5..0477a3d 100644
@@ -55,6 +55,12 @@ extern const char *is_worktree_locked(struct worktree *wt);
  */
 extern int validate_worktree(const struct worktree *wt, int quiet);
 
+/*
+ * Update worktrees/xxx/gitdir with the new path.
+ */
+extern int update_worktree_location(struct worktree *wt,
+				    const char *path_);
+
 /*
  * Free up the memory for worktree(s)
  */

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-12 02:25 UTC #8 lore raw

PATCH 8/11 worktree move: new command

In reply to Nguyễn Thái Ngọc Duy

There are two options to move the main worktree, but both have complications, so it's not implemented yet. Anyway the options are:

- convert the main worktree to a linked one and move it away, leave the git repository where it is. The repo essentially becomes bare after this move.

- move the repository with the main worktree. The tricky part is make sure all file descriptors to the repository are closed, or it may fail on Windows.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 Documentation/git-worktree.txt         |  7 +++-
 builtin/worktree.c                     | 61 ++++++++++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |  2 +-
 t/t2028-worktree-move.sh               | 30 +++++++++++++++++
 4 files changed, 98 insertions(+), 2 deletions(-)

Documentation/git-worktree.txt +6 -1

index 0aeb020..81f4fee 100644
@@ -12,6 +12,7 @@ SYNOPSIS
 'git worktree add' [-f] [--detach] [--checkout] [-b <new-branch>] <path> [<branch>]
 'git worktree list' [--porcelain]
 'git worktree lock' [--reason <string>] <worktree>
+'git worktree move' <worktree> <new-path>
 'git worktree prune' [-n] [-v] [--expire <expire>]
 'git worktree unlock' <worktree>

@@ -71,6 +72,11 @@ files from being pruned automatically. This also prevents it from
 being moved or deleted. Optionally, specify a reason for the lock
 with `--reason`.
 
+move::
+
+Move a working tree to a new location. Note that the main working tree
+cannot be moved yet.
+
 prune::
 
 Prune working tree information in $GIT_DIR/worktrees.

@@ -252,7 +258,6 @@ performed manually, such as:
 
 - `remove` to remove a linked working tree and its administrative files (and
   warn if the working tree is dirty)
-- `mv` to move or rename a working tree and update its administrative files
 
 GIT
 ---

builtin/worktree.c +61 -0

78 lines not on this page · raw message

contrib/completion/git-completion.bash +1 -1

index 21016bf..613e03b 100644
@@ -2715,7 +2715,7 @@ _git_whatchanged ()
 
 _git_worktree ()
 {
-	local subcommands="add list lock prune unlock"
+	local subcommands="add list lock move prune unlock"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"

t/t2028-worktree-move.sh +30 -0

index 8298aaf..74070bd 100755
@@ -59,4 +59,34 @@ test_expect_success 'unlock worktree twice' '
 	test_path_is_missing .git/worktrees/source/locked
 '
 
+test_expect_success 'move non-worktree' '
+	mkdir abc &&
+	test_must_fail git worktree move abc def
+'
+
+test_expect_success 'move locked worktree' '
+	git worktree lock source &&
+	test_must_fail git worktree move source destination &&
+	git worktree unlock source
+'
+
+test_expect_success 'move worktree' '
+	git worktree move source destination &&
+	test_path_is_missing source &&
+	git worktree list --porcelain | grep "^worktree" >actual &&
+	cat <<-EOF >expected &&
+	worktree $TRASH_DIRECTORY
+	worktree $TRASH_DIRECTORY/destination
+	worktree $TRASH_DIRECTORY/elsewhere
+	EOF
+	test_cmp expected actual &&
+	git -C destination log --format=%s >actual2 &&
+	echo init >expected2 &&
+	test_cmp expected2 actual2
+'
+
+test_expect_success 'move main worktree' '
+	test_must_fail git worktree move . def
+'
+
 test_done

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-12 02:25 UTC #9 lore raw

PATCH 9/11 worktree move: accept destination as directory

In reply to Nguyễn Thái Ngọc Duy

Similar to "mv a b/", which is actually "mv a b/a", we extract basename of source worktree and create a directory of the same name at destination if dst path is a directory.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 builtin/worktree.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

builtin/worktree.c +18 -1

index c0d4a73..307019c 100644
@@ -539,7 +539,13 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	strbuf_addstr(&dst, prefix_filename(prefix,
 					    strlen(prefix),
 					    av[1]));
-	if (file_exists(dst.buf))
+	if (is_directory(dst.buf))
+		/*
+		 * keep going, dst will be appended after we get the
+		 * source's absolute path
+		 */
+		;
+	else if (file_exists(dst.buf))
 		die(_("target '%s' already exists"), av[1]);
 
 	worktrees = get_worktrees();

@@ -556,6 +562,17 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	if (validate_worktree(wt, 0))
 		return -1;
 
+	if (is_directory(dst.buf)) {
+		const char *sep = find_last_dir_sep(wt->path);
+
+		if (!sep)
+			die(_("could not figure out destination name from '%s'"),
+			    wt->path);
+		strbuf_addstr(&dst, sep);
+		if (file_exists(dst.buf))
+			die(_("target '%s' already exists"), dst.buf);
+	}
+
 	/*
 	 * First try. Atomically move, and probably cheaper, if both
 	 * source and target are on the same file system.

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-12 02:25 UTC #10 lore raw

PATCH 10/11 worktree move: refuse to move worktrees with submodules

In reply to Nguyễn Thái Ngọc Duy

Submodules contains .git files with relative paths. After a worktree move, these files need to be updated or they may point to nowhere.

This is a bandage patch to make sure "worktree move" don't break people's worktrees by accident. When .git file update code is in place, this validate_no_submodules() could be removed.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 builtin/worktree.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

builtin/worktree.c +23 -0

index 307019c..11be345 100644
@@ -523,6 +523,27 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
 	return ret;
 }
 
+static void validate_no_submodules(const struct worktree *wt)
+{
+	struct index_state istate = {0};
+	int i, found_submodules = 0;
+
+	if (read_index_from(&istate, worktree_git_path(wt, "index")) > 0) {
+		for (i = 0; i < istate.cache_nr; i++) {
+			struct cache_entry *ce = istate.cache[i];
+
+			if (S_ISGITLINK(ce->ce_mode)) {
+				found_submodules = 1;
+				break;
+			}
+		}
+	}
+	discard_index(&istate);
+
+	if (found_submodules)
+		die(_("This working tree contains submodules and cannot be moved yet"));
+}
+
 static int move_worktree(int ac, const char **av, const char *prefix)
 {
 	struct option options[] = {

@@ -562,6 +583,8 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	if (validate_worktree(wt, 0))
 		return -1;
 
+	validate_no_submodules(wt);
+
 	if (is_directory(dst.buf)) {
 		const char *sep = find_last_dir_sep(wt->path);

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-12 02:25 UTC #11 lore raw

PATCH 11/11 worktree remove: new command

In reply to Nguyễn Thái Ngọc Duy

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 Documentation/git-worktree.txt         | 21 +++++----
 builtin/worktree.c                     | 78 ++++++++++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |  5 ++-
 t/t2028-worktree-move.sh               | 26 ++++++++++++
 4 files changed, 120 insertions(+), 10 deletions(-)

Documentation/git-worktree.txt +12 -9

index 81f4fee..df0d551 100644
@@ -14,6 +14,7 @@ SYNOPSIS
 'git worktree lock' [--reason <string>] <worktree>
 'git worktree move' <worktree> <new-path>
 'git worktree prune' [-n] [-v] [--expire <expire>]
+'git worktree remove' [--force] <worktree>
 'git worktree unlock' <worktree>
 
 DESCRIPTION

@@ -81,6 +82,13 @@ prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
 
+remove::
+
+Remove a working tree. Only clean working trees (no untracked files
+and no modification in tracked files) can be removed. Unclean working
+trees can be removed with `--force`. The main working tree cannot be
+removed.
+
 unlock::
 
 Unlock a working tree, allowing it to be pruned, moved or deleted.

@@ -90,9 +98,10 @@ OPTIONS
 
 -f::
 --force::
-	By default, `add` refuses to create a new working tree when `<branch>`
-	is already checked out by another working tree. This option overrides
-	that safeguard.
+	By default, `add` refuses to create a new working tree when
+	`<branch>` is already checked out by another working tree and
+	`remove` refuses to remove an unclean working tree. This option
+	overrides that safeguard.
 
 -b <new-branch>::
 -B <new-branch>::

@@ -253,12 +262,6 @@ Multiple checkout in general is still experimental, and the support
 for submodules is incomplete. It is NOT recommended to make multiple
 checkouts of a superproject.
 
-git-worktree could provide more automation for tasks currently
-performed manually, such as:
-
-- `remove` to remove a linked working tree and its administrative files (and
-  warn if the working tree is dirty)
-
 GIT
 ---
 Part of the linkgit:git[1] suite

builtin/worktree.c +78 -0

95 lines not on this page · raw message

contrib/completion/git-completion.bash +4 -1

index 613e03b..f6855af 100644
@@ -2715,7 +2715,7 @@ _git_whatchanged ()
 
 _git_worktree ()
 {
-	local subcommands="add list lock move prune unlock"
+	local subcommands="add list lock move prune remove unlock"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"

@@ -2733,6 +2733,9 @@ _git_worktree ()
 		prune,--*)
 			__gitcomp "--dry-run --expire --verbose"
 			;;
+		remove,--*)
+			__gitcomp "--force"
+			;;
 		*)
 			;;
 		esac

t/t2028-worktree-move.sh +26 -0

index 74070bd..084acc6 100755
@@ -89,4 +89,30 @@ test_expect_success 'move main worktree' '
 	test_must_fail git worktree move . def
 '
 
+test_expect_success 'remove main worktree' '
+	test_must_fail git worktree remove .
+'
+
+test_expect_success 'remove locked worktree' '
+	git worktree lock destination &&
+	test_must_fail git worktree remove destination &&
+	git worktree unlock destination
+'
+
+test_expect_success 'remove worktree with dirty tracked file' '
+	echo dirty >>destination/init.t &&
+	test_must_fail git worktree remove destination
+'
+
+test_expect_success 'remove worktree with untracked file' '
+	git -C destination checkout init.t &&
+	: >destination/untracked &&
+	test_must_fail git worktree remove destination
+'
+
+test_expect_success 'force remove worktree with untracked file' '
+	git worktree remove --force destination &&
+	test_path_is_missing destination
+'
+
 test_done

Signature · 1 line

2.8.2.524.g6ff3d78

A message that is not in the archive (sent off-list or not yet received): [email protected]

Duy Nguyen

author

2016-11-16 13:05 UTC #14 lore raw

Quoted 6 lines · Junio C Hamano

not ok 12 - move worktree
#
# git worktree move source destination &&
# test_path_is_missing source &&
# git worktree list --porcelain | grep "^worktree" >actual &&
# cat <<-EOF >expected &&

Quoted 8 lines

# worktree $TRASH_DIRECTORY
# worktree $TRASH_DIRECTORY/destination
# worktree $TRASH_DIRECTORY/elsewhere
# EOF
# test_cmp expected actual &&
# git -C destination log --format=%s >actual2 &&
# echo init >expected2 &&
# test_cmp expected2 actual2

I think I've seen this (i.e. 'expected' and 'actual' differ only in the order of items) once after a rebase and ignored it, assuming something was changed during the rebase that caused this.

The following patch should fix it if that's the same thing you saw. I could pile it on worktree-move series, or you can make it a separate one-patch series. What's your preference?

-- 8< --
Subject: [PATCH] worktree list: keep the list sorted

It makes it easier to write tests for. But it should also be good for the user since locating a worktree by eye would be easier once they notice this.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 worktree.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

worktree.c +12 -0

index f7869f8..fe92d6f 100644
@@ -173,6 +173,13 @@ static void mark_current_worktree(struct worktree **worktrees)
 	free(git_dir);
 }
 
+static int compare_worktree(const void *a_, const void *b_)
+{
+	const struct worktree *const *a = a_;
+	const struct worktree *const *b = b_;
+	return fspathcmp((*a)->path, (*b)->path);
+}
+
 struct worktree **get_worktrees(void)
 {
 	struct worktree **list = NULL;

@@ -205,6 +212,11 @@ struct worktree **get_worktrees(void)
 	ALLOC_GROW(list, counter + 1, alloc);
 	list[counter] = NULL;
 
+	/*
+	 * don't sort the first item (main worktree), which will
+	 * always be the first
+	 */
+	qsort(list + 1, counter - 1, sizeof(*list), compare_worktree);
 	mark_current_worktree(list);
 	return list;
 }

Signature · 3 lines

2.8.2.524.g6ff3d78

-- 8< --

Duy Nguyen

author

2016-11-16 13:11 UTC #15 lore raw

Quoted 19 lines · Duy Nguyen

worktree.c +7 -0

index f7869f8..fe92d6f 100644
@@ -173,6 +173,13 @@ static void mark_current_worktree(struct worktree **worktrees)
        free(git_dir);
 }
 
+static int compare_worktree(const void *a_, const void *b_)
+{
+       const struct worktree *const *a = a_;
+       const struct worktree *const *b = b_;
+       return fspathcmp((*a)->path, (*b)->path);
+}
+
 struct worktree **get_worktrees(void)
 {
        struct worktree **list = NULL;

worktree.c struct worktree **get_worktrees(void) · line 218

Quoted 7 lines

worktree.c +4 -0

@@ -205,6 +212,11 @@ struct worktree **get_worktrees(void)
        ALLOC_GROW(list, counter + 1, alloc);
        list[counter] = NULL;
 
+       /*
+        * don't sort the first item (main worktree), which will
+        * always be the first
+        */

Urgh.. I should review my patches more carefully before sending out :( The main worktree could be missing (failing to parse HEAD) so I need a better trick than simply assuming the first item is the main worktree here. Tests did not catch this, naturally..

Quoted 4 lines

worktree.c +1 -0

+       qsort(list + 1, counter - 1, sizeof(*list), compare_worktree);
        mark_current_worktree(list);
        return list;
 }

Signature · 1 line

Duy

Junio C Hamano

2016-11-16 18:13 UTC #16 lore raw

In reply to Duy Nguyen

Quoted 3 lines · Duy Nguyen

The following patch should fix it if that's the same thing you saw. I could pile it on worktree-move series, or you can make it a separate one-patch series. What's your preference?

Giving a stable output to the users is probably a good preparatory fix to what is already in the released versions, so it would make the most sense to make it a separate patch to be applied to maint then build the remainder on top.

I do not think "always show the primary first" is necessarily a good idea (I would have expected an output more like "git branch --list").

Junio C Hamano

2016-11-16 18:40 UTC #17 lore raw

Quoted 6 lines · Junio C Hamano

Quoted 3 lines · Duy Nguyen
The following patch should fix it if that's the same thing you saw. I could pile it on worktree-move series, or you can make it a separate one-patch series. What's your preference?

Quoted 7 lines

Giving a stable output to the users is probably a good preparatory fix to what is already in the released versions, so it would make the most sense to make it a separate patch to be applied to maint then build the remainder on top.
I do not think "always show the primary first" is necessarily a good idea (I would have expected an output more like "git branch --list").

Yikes, "worktree list" is documented like so:

List details of each worktree. The main worktree is listed first, followed by each of the linked worktrees.

If the primary workree is somehow missing, it still should be listed first as missing---otherwise the readers of --porcelain readers will have hard time telling what is going on.

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:43 UTC #18 lore raw

v2 cover git worktree (re)move

In reply to Nguyễn Thái Ngọc Duy

v2 contains some style fix and adapts to the new get_worktrees() api from nd/worktree-list-fixup (which means it can't be built without that series).

 Documentation/git-worktree.txt         |  28 ++-
 builtin/worktree.c                     | 181 ++++++++++++++++
 cache.h                                |   1 +
 contrib/completion/git-completion.bash |   5 +-
 copy.c                                 | 369 +++++++++++++++++++++++++++++++++
 t/t2028-worktree-move.sh               |  56 +++++
 worktree.c                             |  84 ++++++++
 worktree.h                             |  11 +
 8 files changed, 724 insertions(+), 11 deletions(-)

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:43 UTC #19 lore raw

PATCH v2 1/11 copy.c: import copy_file() from busybox

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 copy.c | 331 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 331 insertions(+)

copy.c +331 -0

index 4de6a11..79623ac 100644
@@ -65,3 +65,334 @@ int copy_file_with_time(const char *dst, const char *src, int mode)
 		return copy_times(dst, src);
 	return status;
 }
+
+#if 0
+/* Return:
+ * -1 error, copy not made
+ *  0 copy is made or user answered "no" in interactive mode
+ *    (failures to preserve mode/owner/times are not reported in exit code)
+ */
+int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
+{
+	/* This is a recursive function, try to minimize stack usage */
+	/* NB: each struct stat is ~100 bytes */
+	struct stat source_stat;
+	struct stat dest_stat;
+	smallint retval = 0;
+	smallint dest_exists = 0;
+	smallint ovr;
+
+/* Inverse of cp -d ("cp without -d") */
+#define FLAGS_DEREF (flags & (FILEUTILS_DEREFERENCE + FILEUTILS_DEREFERENCE_L0))
+
+	if ((FLAGS_DEREF ? stat : lstat)(source, &source_stat) < 0) {
+		/* This may be a dangling symlink.
+		 * Making [sym]links to dangling symlinks works, so... */
+		if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK))
+			goto make_links;
+		bb_perror_msg("can't stat '%s'", source);
+		return -1;
+	}
+
+	if (lstat(dest, &dest_stat) < 0) {
+		if (errno != ENOENT) {
+			bb_perror_msg("can't stat '%s'", dest);
+			return -1;
+		}
+	} else {
+		if (source_stat.st_dev == dest_stat.st_dev
+		 && source_stat.st_ino == dest_stat.st_ino
+		) {
+			bb_error_msg("'%s' and '%s' are the same file", source, dest);
+			return -1;
+		}
+		dest_exists = 1;
+	}
+
+#if ENABLE_SELINUX
+	if ((flags & FILEUTILS_PRESERVE_SECURITY_CONTEXT) && is_selinux_enabled() > 0) {
+		security_context_t con;
+		if (lgetfilecon(source, &con) >= 0) {
+			if (setfscreatecon(con) < 0) {
+				bb_perror_msg("can't set setfscreatecon %s", con);
+				freecon(con);
+				return -1;
+			}
+		} else if (errno == ENOTSUP || errno == ENODATA) {
+			setfscreatecon_or_die(NULL);
+		} else {
+			bb_perror_msg("can't lgetfilecon %s", source);
+			return -1;
+		}
+	}
+#endif
+
+	if (S_ISDIR(source_stat.st_mode)) {
+		DIR *dp;
+		const char *tp;
+		struct dirent *d;
+		mode_t saved_umask = 0;
+
+		if (!(flags & FILEUTILS_RECUR)) {
+			bb_error_msg("omitting directory '%s'", source);
+			return -1;
+		}
+
+		/* Did we ever create source ourself before? */
+		tp = is_in_ino_dev_hashtable(&source_stat);
+		if (tp) {
+			/* We did! it's a recursion! man the lifeboats... */
+			bb_error_msg("recursion detected, omitting directory '%s'",
+					source);
+			return -1;
+		}
+
+		if (dest_exists) {
+			if (!S_ISDIR(dest_stat.st_mode)) {
+				bb_error_msg("target '%s' is not a directory", dest);
+				return -1;
+			}
+			/* race here: user can substitute a symlink between
+			 * this check and actual creation of files inside dest */
+		} else {
+			/* Create DEST */
+			mode_t mode;
+			saved_umask = umask(0);
+
+			mode = source_stat.st_mode;
+			if (!(flags & FILEUTILS_PRESERVE_STATUS))
+				mode = source_stat.st_mode & ~saved_umask;
+			/* Allow owner to access new dir (at least for now) */
+			mode |= S_IRWXU;
+			if (mkdir(dest, mode) < 0) {
+				umask(saved_umask);
+				bb_perror_msg("can't create directory '%s'", dest);
+				return -1;
+			}
+			umask(saved_umask);
+			/* need stat info for add_to_ino_dev_hashtable */
+			if (lstat(dest, &dest_stat) < 0) {
+				bb_perror_msg("can't stat '%s'", dest);
+				return -1;
+			}
+		}
+		/* remember (dev,inode) of each created dir.
+		 * NULL: name is not remembered */
+		add_to_ino_dev_hashtable(&dest_stat, NULL);
+
+		/* Recursively copy files in SOURCE */
+		dp = opendir(source);
+		if (dp == NULL) {
+			retval = -1;
+			goto preserve_mode_ugid_time;
+		}
+
+		while ((d = readdir(dp)) != NULL) {
+			char *new_source, *new_dest;
+
+			new_source = concat_subpath_file(source, d->d_name);
+			if (new_source == NULL)
+				continue;
+			new_dest = concat_path_file(dest, d->d_name);
+			if (copy_file(new_source, new_dest, flags & ~FILEUTILS_DEREFERENCE_L0) < 0)
+				retval = -1;
+			free(new_source);
+			free(new_dest);
+		}
+		closedir(dp);
+
+		if (!dest_exists
+		 && chmod(dest, source_stat.st_mode & ~saved_umask) < 0
+		) {
+			bb_perror_msg("can't preserve %s of '%s'", "permissions", dest);
+			/* retval = -1; - WRONG! copy *WAS* made */
+		}
+		goto preserve_mode_ugid_time;
+	}
+
+	if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK)) {
+		int (*lf)(const char *oldpath, const char *newpath);
+ make_links:
+		/* Hmm... maybe
+		 * if (DEREF && MAKE_SOFTLINK) source = realpath(source) ?
+		 * (but realpath returns NULL on dangling symlinks...) */
+		lf = (flags & FILEUTILS_MAKE_SOFTLINK) ? symlink : link;
+		if (lf(source, dest) < 0) {
+			ovr = ask_and_unlink(dest, flags);
+			if (ovr <= 0)
+				return ovr;
+			if (lf(source, dest) < 0) {
+				bb_perror_msg("can't create link '%s'", dest);
+				return -1;
+			}
+		}
+		/* _Not_ jumping to preserve_mode_ugid_time:
+		 * (sym)links don't have those */
+		return 0;
+	}
+
+	if (/* "cp thing1 thing2" without -R: just open and read() from thing1 */
+	    !(flags & FILEUTILS_RECUR)
+	    /* "cp [-opts] regular_file thing2" */
+	 || S_ISREG(source_stat.st_mode)
+	 /* DEREF uses stat, which never returns S_ISLNK() == true.
+	  * So the below is never true: */
+	 /* || (FLAGS_DEREF && S_ISLNK(source_stat.st_mode)) */
+	) {
+		int src_fd;
+		int dst_fd;
+		mode_t new_mode;
+
+		if (!FLAGS_DEREF && S_ISLNK(source_stat.st_mode)) {
+			/* "cp -d symlink dst": create a link */
+			goto dont_cat;
+		}
+
+		if (ENABLE_FEATURE_PRESERVE_HARDLINKS && !FLAGS_DEREF) {
+			const char *link_target;
+			link_target = is_in_ino_dev_hashtable(&source_stat);
+			if (link_target) {
+				if (link(link_target, dest) < 0) {
+					ovr = ask_and_unlink(dest, flags);
+					if (ovr <= 0)
+						return ovr;
+					if (link(link_target, dest) < 0) {
+						bb_perror_msg("can't create link '%s'", dest);
+						return -1;
+					}
+				}
+				return 0;
+			}
+			add_to_ino_dev_hashtable(&source_stat, dest);
+		}
+
+		src_fd = open_or_warn(source, O_RDONLY);
+		if (src_fd < 0)
+			return -1;
+
+		/* Do not try to open with weird mode fields */
+		new_mode = source_stat.st_mode;
+		if (!S_ISREG(source_stat.st_mode))
+			new_mode = 0666;
+
+		// POSIX way is a security problem versus (sym)link attacks
+		if (!ENABLE_FEATURE_NON_POSIX_CP) {
+			dst_fd = open(dest, O_WRONLY|O_CREAT|O_TRUNC, new_mode);
+		} else { /* safe way: */
+			dst_fd = open(dest, O_WRONLY|O_CREAT|O_EXCL, new_mode);
+		}
+		if (dst_fd == -1) {
+			ovr = ask_and_unlink(dest, flags);
+			if (ovr <= 0) {
+				close(src_fd);
+				return ovr;
+			}
+			/* It shouldn't exist. If it exists, do not open (symlink attack?) */
+			dst_fd = open3_or_warn(dest, O_WRONLY|O_CREAT|O_EXCL, new_mode);
+			if (dst_fd < 0) {
+				close(src_fd);
+				return -1;
+			}
+		}
+
+#if ENABLE_SELINUX
+		if ((flags & (FILEUTILS_PRESERVE_SECURITY_CONTEXT|FILEUTILS_SET_SECURITY_CONTEXT))
+		 && is_selinux_enabled() > 0
+		) {
+			security_context_t con;
+			if (getfscreatecon(&con) == -1) {
+				bb_perror_msg("getfscreatecon");
+				return -1;
+			}
+			if (con) {
+				if (setfilecon(dest, con) == -1) {
+					bb_perror_msg("setfilecon:%s,%s", dest, con);
+					freecon(con);
+					return -1;
+				}
+				freecon(con);
+			}
+		}
+#endif
+		if (bb_copyfd_eof(src_fd, dst_fd) == -1)
+			retval = -1;
+		/* Careful with writing... */
+		if (close(dst_fd) < 0) {
+			bb_perror_msg("error writing to '%s'", dest);
+			retval = -1;
+		}
+		/* ...but read size is already checked by bb_copyfd_eof */
+		close(src_fd);
+		/* "cp /dev/something new_file" should not
+		 * copy mode of /dev/something */
+		if (!S_ISREG(source_stat.st_mode))
+			return retval;
+		goto preserve_mode_ugid_time;
+	}
+ dont_cat:
+
+	/* Source is a symlink or a special file */
+	/* We are lazy here, a bit lax with races... */
+	if (dest_exists) {
+		errno = EEXIST;
+		ovr = ask_and_unlink(dest, flags);
+		if (ovr <= 0)
+			return ovr;
+	}
+	if (S_ISLNK(source_stat.st_mode)) {
+		char *lpath = xmalloc_readlink_or_warn(source);
+		if (lpath) {
+			int r = symlink(lpath, dest);
+			free(lpath);
+			if (r < 0) {
+				bb_perror_msg("can't create symlink '%s'", dest);
+				return -1;
+			}
+			if (flags & FILEUTILS_PRESERVE_STATUS)
+				if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
+					bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
+		}
+		/* _Not_ jumping to preserve_mode_ugid_time:
+		 * symlinks don't have those */
+		return 0;
+	}
+	if (S_ISBLK(source_stat.st_mode) || S_ISCHR(source_stat.st_mode)
+	 || S_ISSOCK(source_stat.st_mode) || S_ISFIFO(source_stat.st_mode)
+	) {
+		if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0) {
+			bb_perror_msg("can't create '%s'", dest);
+			return -1;
+		}
+	} else {
+		bb_error_msg("unrecognized file '%s' with mode %x", source, source_stat.st_mode);
+		return -1;
+	}
+
+ preserve_mode_ugid_time:
+
+	if (flags & FILEUTILS_PRESERVE_STATUS
+	/* Cannot happen: */
+	/* && !(flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK)) */
+	) {
+		struct timeval times[2];
+
+		times[1].tv_sec = times[0].tv_sec = source_stat.st_mtime;
+		times[1].tv_usec = times[0].tv_usec = 0;
+		/* BTW, utimes sets usec-precision time - just FYI */
+		if (utimes(dest, times) < 0)
+			bb_perror_msg("can't preserve %s of '%s'", "times", dest);
+		if (chown(dest, source_stat.st_uid, source_stat.st_gid) < 0) {
+			source_stat.st_mode &= ~(S_ISUID | S_ISGID);
+			bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
+		}
+		if (chmod(dest, source_stat.st_mode) < 0)
+			bb_perror_msg("can't preserve %s of '%s'", "permissions", dest);
+	}
+
+	if (flags & FILEUTILS_VERBOSE) {
+		printf("'%s' -> '%s'\n", source, dest);
+	}
+
+	return retval;
+}
+#endif

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:44 UTC #20 lore raw

PATCH v2 2/11 copy.c: delete unused code in copy_file()

In reply to Nguyễn Thái Ngọc Duy

- selinux preservation code
- make-link code
- delete link dereference code
- non-recursive copy code
- stat no preservation code
- verbose printing code

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 copy.c | 101 +++++------------------------------------------------------------
 1 file changed, 7 insertions(+), 94 deletions(-)

copy.c +7 ~~-94~~

index 79623ac..b7a87f1 100644
@@ -82,14 +82,7 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 	smallint dest_exists = 0;
 	smallint ovr;
 
-/* Inverse of cp -d ("cp without -d") */
-#define FLAGS_DEREF (flags & (FILEUTILS_DEREFERENCE + FILEUTILS_DEREFERENCE_L0))
-
-	if ((FLAGS_DEREF ? stat : lstat)(source, &source_stat) < 0) {
-		/* This may be a dangling symlink.
-		 * Making [sym]links to dangling symlinks works, so... */
-		if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK))
-			goto make_links;
+	if (lstat(source, &source_stat) < 0) {
 		bb_perror_msg("can't stat '%s'", source);
 		return -1;
 	}

@@ -109,35 +102,12 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		dest_exists = 1;
 	}
 
-#if ENABLE_SELINUX
-	if ((flags & FILEUTILS_PRESERVE_SECURITY_CONTEXT) && is_selinux_enabled() > 0) {
-		security_context_t con;
-		if (lgetfilecon(source, &con) >= 0) {
-			if (setfscreatecon(con) < 0) {
-				bb_perror_msg("can't set setfscreatecon %s", con);
-				freecon(con);
-				return -1;
-			}
-		} else if (errno == ENOTSUP || errno == ENODATA) {
-			setfscreatecon_or_die(NULL);
-		} else {
-			bb_perror_msg("can't lgetfilecon %s", source);
-			return -1;
-		}
-	}
-#endif
-
 	if (S_ISDIR(source_stat.st_mode)) {
 		DIR *dp;
 		const char *tp;
 		struct dirent *d;
 		mode_t saved_umask = 0;
 
-		if (!(flags & FILEUTILS_RECUR)) {
-			bb_error_msg("omitting directory '%s'", source);
-			return -1;
-		}
-
 		/* Did we ever create source ourself before? */
 		tp = is_in_ino_dev_hashtable(&source_stat);
 		if (tp) {

@@ -160,8 +130,6 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 			saved_umask = umask(0);
 
 			mode = source_stat.st_mode;
-			if (!(flags & FILEUTILS_PRESERVE_STATUS))
-				mode = source_stat.st_mode & ~saved_umask;
 			/* Allow owner to access new dir (at least for now) */
 			mode |= S_IRWXU;
 			if (mkdir(dest, mode) < 0) {

@@ -210,45 +178,17 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		goto preserve_mode_ugid_time;
 	}
 
-	if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK)) {
-		int (*lf)(const char *oldpath, const char *newpath);
- make_links:
-		/* Hmm... maybe
-		 * if (DEREF && MAKE_SOFTLINK) source = realpath(source) ?
-		 * (but realpath returns NULL on dangling symlinks...) */
-		lf = (flags & FILEUTILS_MAKE_SOFTLINK) ? symlink : link;
-		if (lf(source, dest) < 0) {
-			ovr = ask_and_unlink(dest, flags);
-			if (ovr <= 0)
-				return ovr;
-			if (lf(source, dest) < 0) {
-				bb_perror_msg("can't create link '%s'", dest);
-				return -1;
-			}
-		}
-		/* _Not_ jumping to preserve_mode_ugid_time:
-		 * (sym)links don't have those */
-		return 0;
-	}
-
-	if (/* "cp thing1 thing2" without -R: just open and read() from thing1 */
-	    !(flags & FILEUTILS_RECUR)
-	    /* "cp [-opts] regular_file thing2" */
-	 || S_ISREG(source_stat.st_mode)
-	 /* DEREF uses stat, which never returns S_ISLNK() == true.
-	  * So the below is never true: */
-	 /* || (FLAGS_DEREF && S_ISLNK(source_stat.st_mode)) */
-	) {
+	if (S_ISREG(source_stat.st_mode) ) { /* "cp [-opts] regular_file thing2" */
 		int src_fd;
 		int dst_fd;
 		mode_t new_mode;
 
-		if (!FLAGS_DEREF && S_ISLNK(source_stat.st_mode)) {
+		if (S_ISLNK(source_stat.st_mode)) {
 			/* "cp -d symlink dst": create a link */
 			goto dont_cat;
 		}
 
-		if (ENABLE_FEATURE_PRESERVE_HARDLINKS && !FLAGS_DEREF) {
+		if (ENABLE_FEATURE_PRESERVE_HARDLINKS) {
 			const char *link_target;
 			link_target = is_in_ino_dev_hashtable(&source_stat);
 			if (link_target) {

@@ -295,25 +235,6 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 			}
 		}
 
-#if ENABLE_SELINUX
-		if ((flags & (FILEUTILS_PRESERVE_SECURITY_CONTEXT|FILEUTILS_SET_SECURITY_CONTEXT))
-		 && is_selinux_enabled() > 0
-		) {
-			security_context_t con;
-			if (getfscreatecon(&con) == -1) {
-				bb_perror_msg("getfscreatecon");
-				return -1;
-			}
-			if (con) {
-				if (setfilecon(dest, con) == -1) {
-					bb_perror_msg("setfilecon:%s,%s", dest, con);
-					freecon(con);
-					return -1;
-				}
-				freecon(con);
-			}
-		}
-#endif
 		if (bb_copyfd_eof(src_fd, dst_fd) == -1)
 			retval = -1;
 		/* Careful with writing... */

@@ -348,9 +269,8 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 				bb_perror_msg("can't create symlink '%s'", dest);
 				return -1;
 			}
-			if (flags & FILEUTILS_PRESERVE_STATUS)
-				if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
-					bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
+			if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
+				bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
 		}
 		/* _Not_ jumping to preserve_mode_ugid_time:
 		 * symlinks don't have those */

@@ -370,10 +290,7 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 
  preserve_mode_ugid_time:
 
-	if (flags & FILEUTILS_PRESERVE_STATUS
-	/* Cannot happen: */
-	/* && !(flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK)) */
-	) {
+	if (1 /*FILEUTILS_PRESERVE_STATUS*/) {
 		struct timeval times[2];
 
 		times[1].tv_sec = times[0].tv_sec = source_stat.st_mtime;

@@ -389,10 +306,6 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 			bb_perror_msg("can't preserve %s of '%s'", "permissions", dest);
 	}
 
-	if (flags & FILEUTILS_VERBOSE) {
-		printf("'%s' -> '%s'\n", source, dest);
-	}
-
 	return retval;
 }
 #endif

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:44 UTC #21 lore raw

PATCH v2 3/11 copy.c: convert bb_(p)error_msg to error(_errno)

In reply to Nguyễn Thái Ngọc Duy

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 copy.c | 85 ++++++++++++++++++++++++------------------------------------------
 1 file changed, 31 insertions(+), 54 deletions(-)

copy.c +31 ~~-54~~

index b7a87f1..074b609 100644
@@ -82,23 +82,16 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 	smallint dest_exists = 0;
 	smallint ovr;
 
-	if (lstat(source, &source_stat) < 0) {
-		bb_perror_msg("can't stat '%s'", source);
-		return -1;
-	}
+	if (lstat(source, &source_stat) < 0)
+		return error_errno(_("can't stat '%s'"), source);
 
 	if (lstat(dest, &dest_stat) < 0) {
-		if (errno != ENOENT) {
-			bb_perror_msg("can't stat '%s'", dest);
-			return -1;
-		}
+		if (errno != ENOENT)
+			return error_errno(_("can't stat '%s'"), dest);
 	} else {
-		if (source_stat.st_dev == dest_stat.st_dev
-		 && source_stat.st_ino == dest_stat.st_ino
-		) {
-			bb_error_msg("'%s' and '%s' are the same file", source, dest);
-			return -1;
-		}
+		if (source_stat.st_dev == dest_stat.st_dev &&
+		    source_stat.st_ino == dest_stat.st_ino)
+			return error(_("'%s' and '%s' are the same file"), source, dest);
 		dest_exists = 1;
 	}

@@ -110,18 +103,14 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 
 		/* Did we ever create source ourself before? */
 		tp = is_in_ino_dev_hashtable(&source_stat);
-		if (tp) {
+		if (tp)
 			/* We did! it's a recursion! man the lifeboats... */
-			bb_error_msg("recursion detected, omitting directory '%s'",
-					source);
-			return -1;
-		}
+			return error(_("recursion detected, omitting directory '%s'"),
+				     source);
 
 		if (dest_exists) {
-			if (!S_ISDIR(dest_stat.st_mode)) {
-				bb_error_msg("target '%s' is not a directory", dest);
-				return -1;
-			}
+			if (!S_ISDIR(dest_stat.st_mode))
+				return error(_("target '%s' is not a directory"), dest);
 			/* race here: user can substitute a symlink between
 			 * this check and actual creation of files inside dest */
 		} else {

@@ -134,15 +123,12 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 			mode |= S_IRWXU;
 			if (mkdir(dest, mode) < 0) {
 				umask(saved_umask);
-				bb_perror_msg("can't create directory '%s'", dest);
-				return -1;
+				return error_errno(_("can't create directory '%s'"), dest);
 			}
 			umask(saved_umask);
 			/* need stat info for add_to_ino_dev_hashtable */
-			if (lstat(dest, &dest_stat) < 0) {
-				bb_perror_msg("can't stat '%s'", dest);
-				return -1;
-			}
+			if (lstat(dest, &dest_stat) < 0)
+				return error_errno(_("can't stat '%s'"), dest);
 		}
 		/* remember (dev,inode) of each created dir.
 		 * NULL: name is not remembered */

@@ -172,7 +158,7 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		if (!dest_exists
 		 && chmod(dest, source_stat.st_mode & ~saved_umask) < 0
 		) {
-			bb_perror_msg("can't preserve %s of '%s'", "permissions", dest);
+			error_errno(_("can't preserve permissions of '%s'"), dest);
 			/* retval = -1; - WRONG! copy *WAS* made */
 		}
 		goto preserve_mode_ugid_time;

@@ -196,10 +182,8 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 					ovr = ask_and_unlink(dest, flags);
 					if (ovr <= 0)
 						return ovr;
-					if (link(link_target, dest) < 0) {
-						bb_perror_msg("can't create link '%s'", dest);
-						return -1;
-					}
+					if (link(link_target, dest) < 0)
+						return error_errno(_("can't create link '%s'"), dest);
 				}
 				return 0;
 			}

@@ -238,10 +222,8 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		if (bb_copyfd_eof(src_fd, dst_fd) == -1)
 			retval = -1;
 		/* Careful with writing... */
-		if (close(dst_fd) < 0) {
-			bb_perror_msg("error writing to '%s'", dest);
-			retval = -1;
-		}
+		if (close(dst_fd) < 0)
+			retval = error_errno(_("error writing to '%s'"), dest);
 		/* ...but read size is already checked by bb_copyfd_eof */
 		close(src_fd);
 		/* "cp /dev/something new_file" should not

@@ -265,12 +247,10 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		if (lpath) {
 			int r = symlink(lpath, dest);
 			free(lpath);
-			if (r < 0) {
-				bb_perror_msg("can't create symlink '%s'", dest);
-				return -1;
-			}
+			if (r < 0)
+				return error_errno(_("can't create symlink '%s'"), dest);
 			if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
-				bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
+				error_errno(_("can't preserve %s of '%s'"), "ownership", dest);
 		}
 		/* _Not_ jumping to preserve_mode_ugid_time:
 		 * symlinks don't have those */

@@ -279,14 +259,11 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 	if (S_ISBLK(source_stat.st_mode) || S_ISCHR(source_stat.st_mode)
 	 || S_ISSOCK(source_stat.st_mode) || S_ISFIFO(source_stat.st_mode)
 	) {
-		if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0) {
-			bb_perror_msg("can't create '%s'", dest);
-			return -1;
-		}
-	} else {
-		bb_error_msg("unrecognized file '%s' with mode %x", source, source_stat.st_mode);
-		return -1;
-	}
+		if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0)
+			return error_errno(_("can't create '%s'"), dest);
+	} else
+		return error(_("unrecognized file '%s' with mode %x"),
+			     source, source_stat.st_mode);
 
  preserve_mode_ugid_time:

@@ -297,13 +274,13 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		times[1].tv_usec = times[0].tv_usec = 0;
 		/* BTW, utimes sets usec-precision time - just FYI */
 		if (utimes(dest, times) < 0)
-			bb_perror_msg("can't preserve %s of '%s'", "times", dest);
+			error_errno(_("can't preserve %s of '%s'"), "times", dest);
 		if (chown(dest, source_stat.st_uid, source_stat.st_gid) < 0) {
 			source_stat.st_mode &= ~(S_ISUID | S_ISGID);
-			bb_perror_msg("can't preserve %s of '%s'", "ownership", dest);
+			error_errno(_("can't preserve %s of '%s'"), "ownership", dest);
 		}
 		if (chmod(dest, source_stat.st_mode) < 0)
-			bb_perror_msg("can't preserve %s of '%s'", "permissions", dest);
+			error_errno(_("can't preserve %s of '%s'"), "permissions", dest);
 	}
 
 	return retval;

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:44 UTC #22 lore raw

PATCH v2 4/11 copy.c: style fix

In reply to Nguyễn Thái Ngọc Duy

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 copy.c | 50 +++++++++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 21 deletions(-)

copy.c +29 ~~-21~~

index 074b609..60c7d8a 100644
@@ -111,8 +111,10 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		if (dest_exists) {
 			if (!S_ISDIR(dest_stat.st_mode))
 				return error(_("target '%s' is not a directory"), dest);
-			/* race here: user can substitute a symlink between
-			 * this check and actual creation of files inside dest */
+			/*
+			 * race here: user can substitute a symlink between
+			 * this check and actual creation of files inside dest
+			 */
 		} else {
 			/* Create DEST */
 			mode_t mode;

@@ -130,22 +132,24 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 			if (lstat(dest, &dest_stat) < 0)
 				return error_errno(_("can't stat '%s'"), dest);
 		}
-		/* remember (dev,inode) of each created dir.
-		 * NULL: name is not remembered */
+		/*
+		 * remember (dev,inode) of each created dir. name is
+		 * not remembered
+		 */
 		add_to_ino_dev_hashtable(&dest_stat, NULL);
 
 		/* Recursively copy files in SOURCE */
 		dp = opendir(source);
-		if (dp == NULL) {
+		if (!dp) {
 			retval = -1;
 			goto preserve_mode_ugid_time;
 		}
 
-		while ((d = readdir(dp)) != NULL) {
+		while ((d = readdir(dp))) {
 			char *new_source, *new_dest;
 
 			new_source = concat_subpath_file(source, d->d_name);
-			if (new_source == NULL)
+			if (!new_source)
 				continue;
 			new_dest = concat_path_file(dest, d->d_name);
 			if (copy_file(new_source, new_dest, flags & ~FILEUTILS_DEREFERENCE_L0) < 0)

@@ -155,16 +159,15 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		}
 		closedir(dp);
 
-		if (!dest_exists
-		 && chmod(dest, source_stat.st_mode & ~saved_umask) < 0
-		) {
+		if (!dest_exists &&
+		    chmod(dest, source_stat.st_mode & ~saved_umask) < 0) {
 			error_errno(_("can't preserve permissions of '%s'"), dest);
 			/* retval = -1; - WRONG! copy *WAS* made */
 		}
 		goto preserve_mode_ugid_time;
 	}
 
-	if (S_ISREG(source_stat.st_mode) ) { /* "cp [-opts] regular_file thing2" */
+	if (S_ISREG(source_stat.st_mode)) { /* "cp [-opts] regular_file thing2" */
 		int src_fd;
 		int dst_fd;
 		mode_t new_mode;

@@ -199,7 +202,7 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 		if (!S_ISREG(source_stat.st_mode))
 			new_mode = 0666;
 
-		// POSIX way is a security problem versus (sym)link attacks
+		/* POSIX way is a security problem versus (sym)link attacks */
 		if (!ENABLE_FEATURE_NON_POSIX_CP) {
 			dst_fd = open(dest, O_WRONLY|O_CREAT|O_TRUNC, new_mode);
 		} else { /* safe way: */

@@ -226,13 +229,15 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 			retval = error_errno(_("error writing to '%s'"), dest);
 		/* ...but read size is already checked by bb_copyfd_eof */
 		close(src_fd);
-		/* "cp /dev/something new_file" should not
-		 * copy mode of /dev/something */
+		/*
+		 * "cp /dev/something new_file" should not
+		 * copy mode of /dev/something
+		 */
 		if (!S_ISREG(source_stat.st_mode))
 			return retval;
 		goto preserve_mode_ugid_time;
 	}
- dont_cat:
+dont_cat:
 
 	/* Source is a symlink or a special file */
 	/* We are lazy here, a bit lax with races... */

@@ -252,20 +257,23 @@ int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
 			if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
 				error_errno(_("can't preserve %s of '%s'"), "ownership", dest);
 		}
-		/* _Not_ jumping to preserve_mode_ugid_time:
-		 * symlinks don't have those */
+		/*
+		 * _Not_ jumping to preserve_mode_ugid_time: symlinks
+		 * don't have those
+		 */
 		return 0;
 	}
-	if (S_ISBLK(source_stat.st_mode) || S_ISCHR(source_stat.st_mode)
-	 || S_ISSOCK(source_stat.st_mode) || S_ISFIFO(source_stat.st_mode)
-	) {
+	if (S_ISBLK(source_stat.st_mode) ||
+	    S_ISCHR(source_stat.st_mode) ||
+	    S_ISSOCK(source_stat.st_mode) ||
+	    S_ISFIFO(source_stat.st_mode)) {
 		if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0)
 			return error_errno(_("can't create '%s'"), dest);
 	} else
 		return error(_("unrecognized file '%s' with mode %x"),
 			     source, source_stat.st_mode);
 
- preserve_mode_ugid_time:
+preserve_mode_ugid_time:
 
 	if (1 /*FILEUTILS_PRESERVE_STATUS*/) {
 		struct timeval times[2];

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:44 UTC #23 lore raw

PATCH v2 5/11 copy.c: convert copy_file() to copy_dir_recursively()

In reply to Nguyễn Thái Ngọc Duy

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 cache.h |   1 +
 copy.c  | 216 ++++++++++++++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 179 insertions(+), 38 deletions(-)

cache.h +1 -0

index a50a61a..a9a72f8 100644
@@ -1857,6 +1857,7 @@ extern void fprintf_or_die(FILE *, const char *fmt, ...);
 extern int copy_fd(int ifd, int ofd);
 extern int copy_file(const char *dst, const char *src, int mode);
 extern int copy_file_with_time(const char *dst, const char *src, int mode);
+extern int copy_dir_recursively(const char *source, const char *dest);
 
 extern void write_or_die(int fd, const void *buf, size_t count);
 extern void fsync_or_die(int fd, const char *);

copy.c +178 ~~-38~~

304 lines not on this page · raw message

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:44 UTC #24 lore raw

PATCH v2 6/11 worktree.c: add validate_worktree()

In reply to Nguyễn Thái Ngọc Duy

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 worktree.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 worktree.h |  5 +++++
 2 files changed, 68 insertions(+)

worktree.c +63 -0

index eb61212..929072a 100644
@@ -291,6 +291,69 @@ const char *is_worktree_locked(struct worktree *wt)
 	return wt->lock_reason;
 }
 
+static int report(int quiet, const char *fmt, ...)
+{
+	va_list params;
+
+	if (quiet)
+		return -1;
+
+	va_start(params, fmt);
+	vfprintf(stderr, fmt, params);
+	fputc('\n', stderr);
+	va_end(params);
+	return -1;
+}
+
+int validate_worktree(const struct worktree *wt, int quiet)
+{
+	struct strbuf sb = STRBUF_INIT;
+	const char *path;
+	int err;
+
+	if (is_main_worktree(wt)) {
+		/*
+		 * Main worktree using .git file to point to the
+		 * repository would make it impossible to know where
+		 * the actual worktree is if this function is executed
+		 * from another worktree. No .git file support for now.
+		 */
+		strbuf_addf(&sb, "%s/.git", wt->path);
+		if (!is_directory(sb.buf)) {
+			strbuf_release(&sb);
+			return report(quiet, _("'%s/.git' at main worktree is not the repository directory"),
+				      wt->path);
+		}
+		return 0;
+	}
+
+	/*
+	 * Make sure "gitdir" file points to a real .git file and that
+	 * file points back here.
+	 */
+	if (!is_absolute_path(wt->path))
+		return report(quiet, _("'%s' file does not contain absolute path to the worktree location"),
+			      git_common_path("worktrees/%s/gitdir", wt->id));
+
+	strbuf_addf(&sb, "%s/.git", wt->path);
+	if (!file_exists(sb.buf)) {
+		strbuf_release(&sb);
+		return report(quiet, _("'%s/.git' does not exist"), wt->path);
+	}
+
+	path = read_gitfile_gently(sb.buf, &err);
+	strbuf_release(&sb);
+	if (!path)
+		return report(quiet, _("'%s/.git' is not a .git file, error code %d"),
+			      wt->path, err);
+
+	if (fspathcmp(path, real_path(git_common_path("worktrees/%s", wt->id))))
+		return report(quiet, _("'%s' does not point back to"),
+			      wt->path, git_common_path("worktrees/%s", wt->id));
+
+	return 0;
+}
+
 int is_worktree_being_rebased(const struct worktree *wt,
 			      const char *target)
 {

worktree.h +5 -0

index d59ce1f..4433db2 100644
@@ -52,6 +52,11 @@ extern int is_main_worktree(const struct worktree *wt);
  */
 extern const char *is_worktree_locked(struct worktree *wt);
 
+/*
+ * Return zero if the worktree is in good condition.
+ */
+extern int validate_worktree(const struct worktree *wt, int quiet);
+
 /*
  * Free up the memory for worktree(s)
  */

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:44 UTC #25 lore raw

PATCH v2 7/11 worktree.c: add update_worktree_location()

In reply to Nguyễn Thái Ngọc Duy

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 worktree.c | 21 +++++++++++++++++++++
 worktree.h |  6 ++++++
 2 files changed, 27 insertions(+)

worktree.c +21 -0

index 929072a..7684951 100644
@@ -354,6 +354,27 @@ int validate_worktree(const struct worktree *wt, int quiet)
 	return 0;
 }
 
+int update_worktree_location(struct worktree *wt, const char *path_)
+{
+	struct strbuf path = STRBUF_INIT;
+	int ret = 0;
+
+	if (is_main_worktree(wt))
+		return 0;
+
+	strbuf_add_absolute_path(&path, path_);
+	if (fspathcmp(wt->path, path.buf)) {
+		write_file(git_common_path("worktrees/%s/gitdir",
+					   wt->id),
+			   "%s/.git", real_path(path.buf));
+		free(wt->path);
+		wt->path = strbuf_detach(&path, NULL);
+		ret = 0;
+	}
+	strbuf_release(&path);
+	return ret;
+}
+
 int is_worktree_being_rebased(const struct worktree *wt,
 			      const char *target)
 {

worktree.h +6 -0

index 4433db2..1ee03f4 100644
@@ -57,6 +57,12 @@ extern const char *is_worktree_locked(struct worktree *wt);
  */
 extern int validate_worktree(const struct worktree *wt, int quiet);
 
+/*
+ * Update worktrees/xxx/gitdir with the new path.
+ */
+extern int update_worktree_location(struct worktree *wt,
+				    const char *path_);
+
 /*
  * Free up the memory for worktree(s)
  */

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:44 UTC #26 lore raw

PATCH v2 8/11 worktree move: new command

In reply to Nguyễn Thái Ngọc Duy

There are two options to move the main worktree, but both have complications, so it's not implemented yet. Anyway the options are:

- convert the main worktree to a linked one and move it away, leave the git repository where it is. The repo essentially becomes bare after this move.

- move the repository with the main worktree. The tricky part is make sure all file descriptors to the repository are closed, or it may fail on Windows.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 Documentation/git-worktree.txt         |  7 +++-
 builtin/worktree.c                     | 62 ++++++++++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |  2 +-
 t/t2028-worktree-move.sh               | 30 ++++++++++++++++
 4 files changed, 99 insertions(+), 2 deletions(-)

Documentation/git-worktree.txt +6 -1

index e257c19..1310513 100644
@@ -12,6 +12,7 @@ SYNOPSIS
 'git worktree add' [-f] [--detach] [--checkout] [-b <new-branch>] <path> [<branch>]
 'git worktree list' [--porcelain]
 'git worktree lock' [--reason <string>] <worktree>
+'git worktree move' <worktree> <new-path>
 'git worktree prune' [-n] [-v] [--expire <expire>]
 'git worktree unlock' <worktree>

@@ -71,6 +72,11 @@ files from being pruned automatically. This also prevents it from
 being moved or deleted. Optionally, specify a reason for the lock
 with `--reason`.
 
+move::
+
+Move a working tree to a new location. Note that the main working tree
+cannot be moved yet.
+
 prune::
 
 Prune working tree information in $GIT_DIR/worktrees.

@@ -252,7 +258,6 @@ performed manually, such as:
 
 - `remove` to remove a linked working tree and its administrative files (and
   warn if the working tree is dirty)
-- `mv` to move or rename a working tree and update its administrative files
 
 GIT
 ---

builtin/worktree.c +62 -0

79 lines not on this page · raw message

contrib/completion/git-completion.bash +1 -1

index 21016bf..613e03b 100644
@@ -2715,7 +2715,7 @@ _git_whatchanged ()
 
 _git_worktree ()
 {
-	local subcommands="add list lock prune unlock"
+	local subcommands="add list lock move prune unlock"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"

t/t2028-worktree-move.sh +30 -0

index 8298aaf..74070bd 100755
@@ -59,4 +59,34 @@ test_expect_success 'unlock worktree twice' '
 	test_path_is_missing .git/worktrees/source/locked
 '
 
+test_expect_success 'move non-worktree' '
+	mkdir abc &&
+	test_must_fail git worktree move abc def
+'
+
+test_expect_success 'move locked worktree' '
+	git worktree lock source &&
+	test_must_fail git worktree move source destination &&
+	git worktree unlock source
+'
+
+test_expect_success 'move worktree' '
+	git worktree move source destination &&
+	test_path_is_missing source &&
+	git worktree list --porcelain | grep "^worktree" >actual &&
+	cat <<-EOF >expected &&
+	worktree $TRASH_DIRECTORY
+	worktree $TRASH_DIRECTORY/destination
+	worktree $TRASH_DIRECTORY/elsewhere
+	EOF
+	test_cmp expected actual &&
+	git -C destination log --format=%s >actual2 &&
+	echo init >expected2 &&
+	test_cmp expected2 actual2
+'
+
+test_expect_success 'move main worktree' '
+	test_must_fail git worktree move . def
+'
+
 test_done

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:44 UTC #27 lore raw

PATCH v2 9/11 worktree move: accept destination as directory

In reply to Nguyễn Thái Ngọc Duy

Similar to "mv a b/", which is actually "mv a b/a", we extract basename of source worktree and create a directory of the same name at destination if dst path is a directory.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 builtin/worktree.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

builtin/worktree.c +18 -1

index f114965..f732a74 100644
@@ -541,7 +541,13 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	strbuf_addstr(&dst, prefix_filename(prefix,
 					    strlen(prefix),
 					    av[1]));
-	if (file_exists(dst.buf))
+	if (is_directory(dst.buf))
+		/*
+		 * keep going, dst will be appended after we get the
+		 * source's absolute path
+		 */
+		;
+	else if (file_exists(dst.buf))
 		die(_("target '%s' already exists"), av[1]);
 
 	worktrees = get_worktrees(0);

@@ -559,6 +565,17 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	if (validate_worktree(wt, 0))
 		return -1;
 
+	if (is_directory(dst.buf)) {
+		const char *sep = find_last_dir_sep(wt->path);
+
+		if (!sep)
+			die(_("could not figure out destination name from '%s'"),
+			    wt->path);
+		strbuf_addstr(&dst, sep);
+		if (file_exists(dst.buf))
+			die(_("target '%s' already exists"), dst.buf);
+	}
+
 	/*
 	 * First try. Atomically move, and probably cheaper, if both
 	 * source and target are on the same file system.

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:44 UTC #28 lore raw

PATCH v2 10/11 worktree move: refuse to move worktrees with submodules

In reply to Nguyễn Thái Ngọc Duy

Submodules contains .git files with relative paths. After a worktree move, these files need to be updated or they may point to nowhere.

This is a bandage patch to make sure "worktree move" don't break people's worktrees by accident. When .git file update code is in place, this validate_no_submodules() could be removed.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 builtin/worktree.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

builtin/worktree.c +23 -0

index f732a74..e36e4dc 100644
@@ -525,6 +525,27 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
 	return ret;
 }
 
+static void validate_no_submodules(const struct worktree *wt)
+{
+	struct index_state istate = {0};
+	int i, found_submodules = 0;
+
+	if (read_index_from(&istate, worktree_git_path(wt, "index")) > 0) {
+		for (i = 0; i < istate.cache_nr; i++) {
+			struct cache_entry *ce = istate.cache[i];
+
+			if (S_ISGITLINK(ce->ce_mode)) {
+				found_submodules = 1;
+				break;
+			}
+		}
+	}
+	discard_index(&istate);
+
+	if (found_submodules)
+		die(_("This working tree contains submodules and cannot be moved yet"));
+}
+
 static int move_worktree(int ac, const char **av, const char *prefix)
 {
 	struct option options[] = {

@@ -565,6 +586,8 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	if (validate_worktree(wt, 0))
 		return -1;
 
+	validate_no_submodules(wt);
+
 	if (is_directory(dst.buf)) {
 		const char *sep = find_last_dir_sep(wt->path);

Signature · 1 line

2.8.2.524.g6ff3d78

Stefan Beller

2016-12-19 20:58 UTC #29 lore raw

Re: [PATCH v2 10/11] worktree move: refuse to move worktrees with submodules

builtin/worktree.c static int unlock_worktree(int ac, const char **av, const char *prefix) · line 540

Quoted 5 lines · Nguyễn Thái Ngọc Duy

builtin/worktree.c +5 -0

+
+                       if (S_ISGITLINK(ce->ce_mode)) {
+                               found_submodules = 1;
+                               break;
+                       }

While I applaud being careful with submodules, I think this may be a bit overeager, because empty (both not initialized as well as not populated) submodules are fine.

In origin/bw/grep-recurse-submodules^6 you find
int is_submodule_populated(const char *path)

that could be useful here, i.e. I'd imagine you'd change the condition to

    if (S_ISGITLINK(ce->ce_mode)
        && !is_submodule_populated(ce->name)) {
        ...

I guess that can come as a fixup later though.

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:44 UTC #30 lore raw

PATCH v2 11/11 worktree remove: new command

In reply to Nguyễn Thái Ngọc Duy

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 Documentation/git-worktree.txt         | 21 +++++----
 builtin/worktree.c                     | 79 ++++++++++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |  5 ++-
 t/t2028-worktree-move.sh               | 26 +++++++++++
 4 files changed, 121 insertions(+), 10 deletions(-)

Documentation/git-worktree.txt +12 -9

index 1310513..bbde6b8 100644
@@ -14,6 +14,7 @@ SYNOPSIS
 'git worktree lock' [--reason <string>] <worktree>
 'git worktree move' <worktree> <new-path>
 'git worktree prune' [-n] [-v] [--expire <expire>]
+'git worktree remove' [--force] <worktree>
 'git worktree unlock' <worktree>
 
 DESCRIPTION

@@ -81,6 +82,13 @@ prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
 
+remove::
+
+Remove a working tree. Only clean working trees (no untracked files
+and no modification in tracked files) can be removed. Unclean working
+trees can be removed with `--force`. The main working tree cannot be
+removed.
+
 unlock::
 
 Unlock a working tree, allowing it to be pruned, moved or deleted.

@@ -90,9 +98,10 @@ OPTIONS
 
 -f::
 --force::
-	By default, `add` refuses to create a new working tree when `<branch>`
-	is already checked out by another working tree. This option overrides
-	that safeguard.
+	By default, `add` refuses to create a new working tree when
+	`<branch>` is already checked out by another working tree and
+	`remove` refuses to remove an unclean working tree. This option
+	overrides that safeguard.
 
 -b <new-branch>::
 -B <new-branch>::

@@ -253,12 +262,6 @@ Multiple checkout in general is still experimental, and the support
 for submodules is incomplete. It is NOT recommended to make multiple
 checkouts of a superproject.
 
-git-worktree could provide more automation for tasks currently
-performed manually, such as:
-
-- `remove` to remove a linked working tree and its administrative files (and
-  warn if the working tree is dirty)
-
 GIT
 ---
 Part of the linkgit:git[1] suite

builtin/worktree.c +79 -0

96 lines not on this page · raw message

contrib/completion/git-completion.bash +4 -1

index 613e03b..f6855af 100644
@@ -2715,7 +2715,7 @@ _git_whatchanged ()
 
 _git_worktree ()
 {
-	local subcommands="add list lock move prune unlock"
+	local subcommands="add list lock move prune remove unlock"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"

@@ -2733,6 +2733,9 @@ _git_worktree ()
 		prune,--*)
 			__gitcomp "--dry-run --expire --verbose"
 			;;
+		remove,--*)
+			__gitcomp "--force"
+			;;
 		*)
 			;;
 		esac

t/t2028-worktree-move.sh +26 -0

index 74070bd..084acc6 100755
@@ -89,4 +89,30 @@ test_expect_success 'move main worktree' '
 	test_must_fail git worktree move . def
 '
 
+test_expect_success 'remove main worktree' '
+	test_must_fail git worktree remove .
+'
+
+test_expect_success 'remove locked worktree' '
+	git worktree lock destination &&
+	test_must_fail git worktree remove destination &&
+	git worktree unlock destination
+'
+
+test_expect_success 'remove worktree with dirty tracked file' '
+	echo dirty >>destination/init.t &&
+	test_must_fail git worktree remove destination
+'
+
+test_expect_success 'remove worktree with untracked file' '
+	git -C destination checkout init.t &&
+	: >destination/untracked &&
+	test_must_fail git worktree remove destination
+'
+
+test_expect_success 'force remove worktree with untracked file' '
+	git worktree remove --force destination &&
+	test_path_is_missing destination
+'
+
 test_done

Signature · 1 line

2.8.2.524.g6ff3d78

Junio C Hamano

2016-11-28 19:48 UTC #31 lore raw

In reply to Nguyễn Thái Ngọc Duy

Quoted 17 lines · Nguyễn Thái Ngọc Duy

v2 contains some style fix and adapts to the new get_worktrees() api from nd/worktree-list-fixup (which means it can't be built without that series).
Nguyễn Thái Ngọc Duy (11):
copy.c: import copy_file() from busybox copy.c: delete unused code in copy_file() copy.c: convert bb_(p)error_msg to error(_errno) copy.c: style fix
copy.c: convert copy_file() to copy_dir_recursively() worktree.c: add validate_worktree() worktree.c: add update_worktree_location() worktree move: new command
worktree move: accept destination as directory worktree move: refuse to move worktrees with submodules worktree remove: new command

Quoted 9 lines

 Documentation/git-worktree.txt         |  28 ++-
 builtin/worktree.c                     | 181 ++++++++++++++++
 cache.h                                |   1 +
 contrib/completion/git-completion.bash |   5 +-
 copy.c                                 | 369 +++++++++++++++++++++++++++++++++
 t/t2028-worktree-move.sh               |  56 +++++
 worktree.c                             |  84 ++++++++
 worktree.h                             |  11 +
 8 files changed, 724 insertions(+), 11 deletions(-)

Does this round address the issue raised in

http://public-inbox.org/git/alpine.DEB.2.20.1611161041040.3746@virtualbox

by Dscho?

Even if you are not tracking a fifo, for example, your working tree may have one created in t/trash* directory during testing, and letting platform "cp -r" taking care of it (if that is possible---I didn't look at the code that calls busybox copy to see if you are doing something exotic or just blindly copying everything in the directory) may turn out to be a more portable way to do this, and I suspect that the cost of copying one whole-tree would dominate the run_command() overhead.

Junio C Hamano

2016-11-28 20:20 UTC #32 lore raw

Quoted 6 lines · Junio C Hamano

Does this round address the issue raised in
http://public-inbox.org/git/alpine.DEB.2.20.1611161041040.3746@virtualbox
by Dscho?

Quoted 8 lines

Even if you are not tracking a fifo, for example, your working tree may have one created in t/trash* directory during testing, and letting platform "cp -r" taking care of it (if that is possible---I didn't look at the code that calls busybox copy to see if you are doing something exotic or just blindly copying everything in the directory) may turn out to be a more portable way to do this, and I suspect that the cost of copying one whole-tree would dominate the run_command() overhead.

Please do not take the above as me saying "you must spawn the platform cp -r".

A more traditional alternative solution seen on this list is to work together, leveraging expertise of each participant. From the build log Dscho gave us, it seems that his Windows port lack at least POSIX emulation for lchown, mknod, utimes and chown. It is hard to decide without involving Windows expert what the best way to deal with it in the code (e.g. To stub or #ifdef out these calls? Provide suitable emulation in compat/? Something else?), and what things other than these four are still missing.

Johannes Sixt

2016-11-28 21:26 UTC #33 lore raw

Am 28.11.2016 um 21:20 schrieb Junio C Hamano:

Quoted 17 lines

Quoted 6 lines · Junio C Hamano
Does this round address the issue raised in
http://public-inbox.org/git/alpine.DEB.2.20.1611161041040.3746@virtualbox
by Dscho?
Quoted 8 lines
Even if you are not tracking a fifo, for example, your working tree may have one created in t/trash* directory during testing, and letting platform "cp -r" taking care of it (if that is possible---I didn't look at the code that calls busybox copy to see if you are doing something exotic or just blindly copying everything in the directory) may turn out to be a more portable way to do this, and I suspect that the cost of copying one whole-tree would dominate the run_command() overhead.

Quoted 2 lines

Please do not take the above as me saying "you must spawn the platform cp -r".

copy_dir_recursively is used in 'worktree move' when the move is across file systems. My stance on it is to punt in this case. *I* would not trust Git, or any other program that is not *specifically* made to copy a whole directory structure, to get all cases right when a simple rename() is not sufficent. And, uh, oh, it does a remove_dir_recursively() after it has finshed copying. No, Git is not a tool to move directories, thank you very much!

-- Hannes

Duy Nguyen

author

2016-11-29 12:18 UTC #34 lore raw

Quoted 25 lines · Johannes Sixt

Am 28.11.2016 um 21:20 schrieb Junio C Hamano:
Quoted 20 lines
Quoted 7 lines · Junio C Hamano
Does this round address the issue raised in
http://public-inbox.org/git/alpine.DEB.2.20.1611161041040.3746@virtualbox
by Dscho?
Quoted 8 lines
Even if you are not tracking a fifo, for example, your working tree may have one created in t/trash* directory during testing, and letting platform "cp -r" taking care of it (if that is possible---I didn't look at the code that calls busybox copy to see if you are doing something exotic or just blindly copying everything in the directory) may turn out to be a more portable way to do this, and I suspect that the cost of copying one whole-tree would dominate the run_command() overhead.
Quoted 2 lines
Please do not take the above as me saying "you must spawn the platform cp -r".

Quoted 5 lines

copy_dir_recursively is used in 'worktree move' when the move is across file systems. My stance on it is to punt in this case. *I* would not trust Git, or any other program that is not *specifically* made to copy a whole directory structure, to get all cases right when a simple rename() is not sufficent.

This is why I did not write new copy code. The code was from busybox, probably battle tested for many years now. Of course bugs can slip in when I integrated it to git.

Quoted 3 lines

And, uh, oh, it does a remove_dir_recursively() after it has finshed copying. No, Git is not a tool to move directories, thank you very much!

I guess you won't like my (unsent) patch for moving .git dir either ;-) which does make me nervous. The thing is, these operations require some more modification in .git. We can't ask the user to "move this directory yourself, then come back to me and I will fix up the rest in .git". First step "only support moving within the same filesystem" works for me. But I don't know how rename() works on Windows...

Signature · 1 line

Duy

Duy Nguyen

author

2016-11-29 12:08 UTC #35 lore raw

In reply to Junio C Hamano

Quoted 7 lines · Junio C Hamano

Quoted 5 lines · Junio C Hamano
Does this round address the issue raised in
http://public-inbox.org/git/alpine.DEB.2.20.1611161041040.3746@virtualbox
by Dscho?

It does not (and is sort of expected), quoting from the commit message

copy.c: convert copy_file() to copy_dir_recursively()

I could "#ifdef WINDOWS return -ENOSYS" for now, which would make it build. "git worktree move" won't work on Windows of course...

Quoted 11 lines

Quoted 8 lines
Even if you are not tracking a fifo, for example, your working tree may have one created in t/trash* directory during testing, and letting platform "cp -r" taking care of it (if that is possible---I didn't look at the code that calls busybox copy to see if you are doing something exotic or just blindly copying everything in the directory) may turn out to be a more portable way to do this, and I suspect that the cost of copying one whole-tree would dominate the run_command() overhead.
Please do not take the above as me saying "you must spawn the platform cp -r".

For the the record this was my first move but it will make it much harder to handle errors, and there's no native "cp" on Windows.

Signature · 1 line

Duy

Duy Nguyen

author

2016-11-29 13:56 UTC #36 lore raw

Quoted 13 lines · Duy Nguyen

Quoted 7 lines · Junio C Hamano
Quoted 5 lines · Junio C Hamano
Does this round address the issue raised in
http://public-inbox.org/git/alpine.DEB.2.20.1611161041040.3746@virtualbox
by Dscho?
It does not (and is sort of expected), quoting from the commit message
copy.c: convert copy_file() to copy_dir_recursively()

Quoted 7 lines

This finally enables busybox's copy_file() code under a new name (because "copy_file" is already taken in Git code base). Because this comes from busybox, POSIXy (or even Linuxy) behavior is expected. More changes may be needed for Windows support.
I could "#ifdef WINDOWS return -ENOSYS" for now, which would make it build. "git worktree move" won't work on Windows of course...

Another way, as pointed out by j6t, is go with "move within filesystem only", at least at the first step. Which is probably a good idea anyway so we can concentrate on git-specific stuff before going to minor and complicated copy/move details.

If you drop all the "copy.c: " patches and squash this to "worktree
move: new command", and if Windows rename() can move directories, then git should build and new tests pass.

-- 8< --

builtin/worktree.c +3 -3

index f114965..d8d0127 100644
@@ -569,9 +569,9 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 				  wt->path, dst.buf);
 
 		/* second try.. */
-		if (copy_dir_recursively(wt->path, dst.buf))
-			die(_("failed to copy '%s' to '%s'"),
-			    wt->path, dst.buf);
+		if (copy_dir_recursively(dst.buf, wt->path))
+			die_errno(_("failed to copy '%s' to '%s'"),
+				  wt->path, dst.buf);
 		else {
 			struct strbuf sb = STRBUF_INIT;

cache.h +1 -0

index a50a61a..2d4edf6 100644
@@ -1857,6 +1857,7 @@ extern void fprintf_or_die(FILE *, const char *fmt, ...);
 extern int copy_fd(int ifd, int ofd);
 extern int copy_file(const char *dst, const char *src, int mode);
 extern int copy_file_with_time(const char *dst, const char *src, int mode);
+extern int copy_dir_recursively(const char *dst, const char *src);
 
 extern void write_or_die(int fd, const void *buf, size_t count);
 extern void fsync_or_die(int fd, const char *);

copy.c +6 -0

index 4de6a11..b232aec 100644
@@ -65,3 +65,9 @@ int copy_file_with_time(const char *dst, const char *src, int mode)
 		return copy_times(dst, src);
 	return status;
 }
+
+int copy_dir_recursively(const char *dst, const char *src)
+{
+	errno = ENOSYS;
+	return -1;
+}

-- 8< --

Junio C Hamano

2016-11-29 19:28 UTC #37 lore raw

Quoted 4 lines · Duy Nguyen

Another way, as pointed out by j6t, is go with "move within filesystem only", at least at the first step. Which is probably a good idea anyway so we can concentrate on git-specific stuff before going to minor and complicated copy/move details.

Yup, that is a very sensible approach.

Stefan Beller

2016-11-30 00:18 UTC #38 lore raw

Quoted 8 lines · Junio C Hamano

Quoted 4 lines · Duy Nguyen
Another way, as pointed out by j6t, is go with "move within filesystem only", at least at the first step. Which is probably a good idea anyway so we can concentrate on git-specific stuff before going to minor and complicated copy/move details.
Yup, that is a very sensible approach.

In case you decided to go another way than a plain rename, please make it easy to reuse (e.g. move_directory_safely exposed to the rest of the codebase). I'd find use for that in the series currently queued as:

sb/submodule-intern-gitdir

which moves git directories of submodules around (from inside the submodule/.git to the superprojects .git/modules/<name>)

Thanks,
Stefan

Johannes Sixt

2016-11-29 21:14 UTC #39 lore raw

In reply to Duy Nguyen

Am 29.11.2016 um 14:56 schrieb Duy Nguyen:

Quoted 3 lines

If you drop all the "copy.c: " patches and squash this to "worktree
move: new command", and if Windows rename() can move directories, then git should build and new tests pass.

Thanks! rename() can move directories on Windows, provided that *nothing* inside the directory is in any form of use by any process, particularly also not as the "current working directory" (as per getcwd()).

Quoted 5 lines

copy.c +0 -0

index 4de6a11..b232aec 100644

copy.c int copy_file_with_time(const char *dst, const char *src, int mode) · line 73

Quoted 9 lines

copy.c +6 -0

@@ -65,3 +65,9 @@ int copy_file_with_time(const char *dst, const char *src, int mode)
 		return copy_times(dst, src);
 	return status;
 }
+
+int copy_dir_recursively(const char *dst, const char *src)
+{
+	errno = ENOSYS;
+	return -1;
+}

An error message "cannot move directories across devices" or something would be preferable over "Function not implemented", of course. Or did you mean to set errno = EXDEV?

-- Hannes

Duy Nguyen

author

2016-11-30 00:09 UTC #40 lore raw

Quoted 12 lines · Johannes Sixt

Quoted 12 lines

copy.c +3 -0

index 4de6a11..b232aec 100644
@@ -65,3 +65,9 @@ int copy_file_with_time(const char *dst, const char
 *src, int mode)
                return copy_times(dst, src);
        return status;
 }
+
+int copy_dir_recursively(const char *dst, const char *src)
+{

Quoted 8 lines

copy.c int copy_file_with_time(const char *dst, const char *src, int mode) · line 73
Quoted 3 lines
copy.c +3 -0
+       errno = ENOSYS;
+       return -1;
+}
An error message "cannot move directories across devices" or something would be preferable over "Function not implemented", of course. Or did you mean to set errno = EXDEV?

The exact message is not super important right now. Though I'm thinking of adding move_directory() that is a wrapper of rename(). We can die("cannot move directories across devices") then and hopefully be able to move across devices at some point.

Signature · 1 line

Duy