Re: [PATCH v2 00/11] git worktree (re)move

Junio C Hamano [off-list ref] writes:

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.

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.

Am 28.11.2016 um 21:20 schrieb Junio C Hamano:

Junio C Hamano [off-list ref] writes:

quoted

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.

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

On Tue, Nov 29, 2016 at 3:20 AM, Junio C Hamano [off-list ref] wrote:

Junio C Hamano [off-list ref] writes:

quoted

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

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

quoted

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

On Tue, Nov 29, 2016 at 4:25 AM, Johannes Sixt [off-list ref] wrote:

Am 28.11.2016 um 21:20 schrieb Junio C Hamano:

quoted

Junio C Hamano [off-list ref] writes:

quoted

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.

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.

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.

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

On Tue, Nov 29, 2016 at 07:08:16PM +0700, Duy Nguyen wrote:

On Tue, Nov 29, 2016 at 3:20 AM, Junio C Hamano [off-list ref] wrote:

quoted

Junio C Hamano [off-list ref] writes:

quoted

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

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

diff --git a/builtin/worktree.c b/builtin/worktree.c
index f114965..d8d0127 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c

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

diff --git a/cache.h b/cache.h
index a50a61a..2d4edf6 100644
--- a/cache.h
+++ b/cache.h

@@ -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 *);

diff --git a/copy.c b/copy.c
index 4de6a11..b232aec 100644
--- a/copy.c
+++ b/copy.c

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

Am 29.11.2016 um 14:56 schrieb Duy Nguyen:

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()).

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

On Wed, Nov 30, 2016 at 4:14 AM, Johannes Sixt [off-list ref] wrote:

quoted

diff --git a/copy.c b/copy.c
index 4de6a11..b232aec 100644
--- a/copy.c
+++ b/copy.c

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

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