[PATCH 0/2] Allow cloning to an existing empty directory

[PATCH 0/2] Allow cloning to an existing empty directory

[Alexander Potashev]

The problem I experienced today was that I couldn't clone a repo to a
separate filesystem. I've created a new LVM volume, a FS on it (XFS)
and mounted it to a directory.

But wasn't able to clone the repo to that directory. It's impossible to
mount a FS to a non-existent directory, right? But Git refuses to clone
to an existing directory.

The solution in my first patch allows cloning to an existing empty
directory. However, there could be problems doing the same as I did
with XFS using ext2-like filesystems, because they have lost+found
directories, i.e. the root directory of those FSs is never empty.



The first patch adds a function (is_pseudo_dir_name) to compare a
string with "." and "..", the second patch reuses that function in
the rest of the code.


Alexander Potashev (2):
  Allow cloning to an existing empty directory
  Use is_pseudo_dir_name everywhere

 builtin-clone.c         |    8 +++++---
 builtin-count-objects.c |    5 ++---
 builtin-fsck.c          |   14 ++++----------
 builtin-prune.c         |   14 ++++----------
 builtin-rerere.c        |   11 +++++------
 dir.c                   |   31 +++++++++++++++++++++++--------
 dir.h                   |    8 ++++++++
 entry.c                 |    5 ++---
 remote.c                |    6 ++----
 transport.c             |    4 +---
 10 files changed, 56 insertions(+), 50 deletions(-)

[PATCH 1/2] Allow cloning to an existing empty directory

[Alexander Potashev]

The die() message changed accordingly.

The previous behaviour was to only allow cloning when the destination
directory doesn't exist.

A new inline function is_pseudo_dir_name is used to check if the
directory name is either "." or "..". It returns a non-zero value if
the given string is "." or "..". It's applicable to a lot of other Git
source code.

Signed-off-by: Alexander Potashev <aspotashev@gmail.com>
---
 builtin-clone.c |    8 +++++---
 dir.c           |   19 +++++++++++++++++++
 dir.h           |    8 ++++++++
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index f1a1a0c..e732f15 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -357,6 +357,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	struct stat buf;
 	const char *repo_name, *repo, *work_tree, *git_dir;
 	char *path, *dir;
+	int dest_exists;
 	const struct ref *refs, *head_points_at, *remote_head, *mapped_refs;
 	struct strbuf key = STRBUF_INIT, value = STRBUF_INIT;
 	struct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;
@@ -406,8 +407,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		dir = guess_dir_name(repo_name, is_bundle, option_bare);
 	strip_trailing_slashes(dir);
 
-	if (!stat(dir, &buf))
-		die("destination directory '%s' already exists.", dir);
+	if ((dest_exists = !stat(dir, &buf)) && !is_empty_dir(dir))
+		die("destination path '%s' already exists and is not "
+			"an empty directory.", dir);
 
 	strbuf_addf(&reflog_msg, "clone: from %s", repo);
 
@@ -431,7 +433,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		if (safe_create_leading_directories_const(work_tree) < 0)
 			die("could not create leading directories of '%s': %s",
 					work_tree, strerror(errno));
-		if (mkdir(work_tree, 0755))
+		if (!dest_exists && mkdir(work_tree, 0755))
 			die("could not create work tree dir '%s': %s.",
 					work_tree, strerror(errno));
 		set_git_work_tree(work_tree);
diff --git a/dir.c b/dir.c
index 0131983..bd97e50 100644
--- a/dir.c
+++ b/dir.c
@@ -779,6 +779,25 @@ int is_inside_dir(const char *dir)
 	return get_relative_cwd(buffer, sizeof(buffer), dir) != NULL;
 }
 
+int is_empty_dir(const char *path)
+{
+	DIR *dir = opendir(path);
+	struct dirent *e;
+	int ret = 1;
+
+	if (!dir)
+		return 0;
+
+	while ((e = readdir(dir)) != NULL)
+		if (!is_pseudo_dir_name(e->d_name)) {
+			ret = 0;
+			break;
+		}
+
+	closedir(dir);
+	return ret;
+}
+
 int remove_dir_recursively(struct strbuf *path, int only_empty)
 {
 	DIR *dir = opendir(path->buf);
diff --git a/dir.h b/dir.h
index 768425a..940e057 100644
--- a/dir.h
+++ b/dir.h
@@ -77,6 +77,14 @@ extern int file_exists(const char *);
 extern char *get_relative_cwd(char *buffer, int size, const char *dir);
 extern int is_inside_dir(const char *dir);
 
+static inline int is_pseudo_dir_name(const char *name)
+{
+	return name[0] == '.' && (name[1] == '\0' ||
+		(name[1] == '.' && name[2] == '\0')); /* "." and ".." */
+}
+
+extern int is_empty_dir(const char *dir);
+
 extern void setup_standard_excludes(struct dir_struct *dir);
 extern int remove_dir_recursively(struct strbuf *path, int only_empty);
 
-- 
1.6.1.77.g84c9

[PATCH 2/2] Use is_pseudo_dir_name everywhere

[Alexander Potashev]

Signed-off-by: Alexander Potashev <aspotashev@gmail.com>
---
 builtin-count-objects.c |    5 ++---
 builtin-fsck.c          |   14 ++++----------
 builtin-prune.c         |   14 ++++----------
 builtin-rerere.c        |   11 +++++------
 dir.c                   |   12 ++++--------
 entry.c                 |    5 ++---
 remote.c                |    6 ++----
 transport.c             |    4 +---
 8 files changed, 24 insertions(+), 47 deletions(-)

diff --git a/builtin-count-objects.c b/builtin-count-objects.c
index ab35b65..492a173 100644
--- a/builtin-count-objects.c
+++ b/builtin-count-objects.c
@@ -5,6 +5,7 @@
  */
 
 #include "cache.h"
+#include "dir.h"
 #include "builtin.h"
 #include "parse-options.h"
 
@@ -21,9 +22,7 @@ static void count_objects(DIR *d, char *path, int len, int verbose,
 		const char *cp;
 		int bad = 0;
 
-		if ((ent->d_name[0] == '.') &&
-		    (ent->d_name[1] == 0 ||
-		     ((ent->d_name[1] == '.') && (ent->d_name[2] == 0))))
+		if (is_pseudo_dir_name(ent->d_name))
 			continue;
 		for (cp = ent->d_name; *cp; cp++) {
 			int ch = *cp;
diff --git a/builtin-fsck.c b/builtin-fsck.c
index 297b2c4..291ca8e 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -10,6 +10,7 @@
 #include "tree-walk.h"
 #include "fsck.h"
 #include "parse-options.h"
+#include "dir.h"
 
 #define REACHABLE 0x0001
 #define SEEN      0x0002
@@ -395,19 +396,12 @@ static void fsck_dir(int i, char *path)
 	while ((de = readdir(dir)) != NULL) {
 		char name[100];
 		unsigned char sha1[20];
-		int len = strlen(de->d_name);
 
-		switch (len) {
-		case 2:
-			if (de->d_name[1] != '.')
-				break;
-		case 1:
-			if (de->d_name[0] != '.')
-				break;
+		if (is_pseudo_dir_name(de->d_name))
 			continue;
-		case 38:
+		if (strlen(de->d_name) == 38) {
 			sprintf(name, "%02x", i);
-			memcpy(name+2, de->d_name, len+1);
+			memcpy(name+2, de->d_name, 39);
 			if (get_sha1_hex(name, sha1) < 0)
 				break;
 			add_sha1_list(sha1, DIRENT_SORT_HINT(de));
diff --git a/builtin-prune.c b/builtin-prune.c
index 7b4ec80..06b61ea 100644
--- a/builtin-prune.c
+++ b/builtin-prune.c
@@ -5,6 +5,7 @@
 #include "builtin.h"
 #include "reachable.h"
 #include "parse-options.h"
+#include "dir.h"
 
 static const char * const prune_usage[] = {
 	"git prune [-n] [-v] [--expire <time>] [--] [<head>...]",
@@ -61,19 +62,12 @@ static int prune_dir(int i, char *path)
 	while ((de = readdir(dir)) != NULL) {
 		char name[100];
 		unsigned char sha1[20];
-		int len = strlen(de->d_name);
 
-		switch (len) {
-		case 2:
-			if (de->d_name[1] != '.')
-				break;
-		case 1:
-			if (de->d_name[0] != '.')
-				break;
+		if (is_pseudo_dir_name(de->d_name))
 			continue;
-		case 38:
+		if (strlen(de->d_name) == 38) {
 			sprintf(name, "%02x", i);
-			memcpy(name+2, de->d_name, len+1);
+			memcpy(name+2, de->d_name, 39);
 			if (get_sha1_hex(name, sha1) < 0)
 				break;
 
diff --git a/builtin-rerere.c b/builtin-rerere.c
index d4dec6b..1ac5225 100644
--- a/builtin-rerere.c
+++ b/builtin-rerere.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
+#include "dir.h"
 #include "string-list.h"
 #include "rerere.h"
 #include "xdiff/xdiff.h"
@@ -59,17 +60,15 @@ static void garbage_collect(struct string_list *rr)
 	git_config(git_rerere_gc_config, NULL);
 	dir = opendir(git_path("rr-cache"));
 	while ((e = readdir(dir))) {
-		const char *name = e->d_name;
-		if (name[0] == '.' &&
-		    (name[1] == '\0' || (name[1] == '.' && name[2] == '\0')))
+		if (is_pseudo_dir_name (e->d_name))
 			continue;
-		then = rerere_created_at(name);
+		then = rerere_created_at(e->d_name);
 		if (!then)
 			continue;
-		cutoff = (has_resolution(name)
+		cutoff = (has_resolution(e->d_name)
 			  ? cutoff_resolve : cutoff_noresolve);
 		if (then < now - cutoff * 86400)
-			string_list_append(name, &to_remove);
+			string_list_append(e->d_name, &to_remove);
 	}
 	for (i = 0; i < to_remove.nr; i++)
 		unlink_rr_item(to_remove.items[i].string);
diff --git a/dir.c b/dir.c
index bd97e50..cdd3beb 100644
--- a/dir.c
+++ b/dir.c
@@ -585,10 +585,8 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
 			int len, dtype;
 			int exclude;
 
-			if ((de->d_name[0] == '.') &&
-			    (de->d_name[1] == 0 ||
-			     !strcmp(de->d_name + 1, ".") ||
-			     !strcmp(de->d_name + 1, "git")))
+			if (is_pseudo_dir_name(de->d_name) ||
+			     !strcmp(de->d_name, ".git"))
 				continue;
 			len = strlen(de->d_name);
 			/* Ignore overly long pathnames! */
@@ -812,10 +810,8 @@ int remove_dir_recursively(struct strbuf *path, int only_empty)
 	len = path->len;
 	while ((e = readdir(dir)) != NULL) {
 		struct stat st;
-		if ((e->d_name[0] == '.') &&
-		    ((e->d_name[1] == 0) ||
-		     ((e->d_name[1] == '.') && e->d_name[2] == 0)))
-			continue; /* "." and ".." */
+		if (is_pseudo_dir_name(e->d_name))
+			continue;
 
 		strbuf_setlen(path, len);
 		strbuf_addstr(path, e->d_name);
diff --git a/entry.c b/entry.c
index aa2ee46..9c6a9cf 100644
--- a/entry.c
+++ b/entry.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "blob.h"
+#include "dir.h"
 
 static void create_directories(const char *path, const struct checkout *state)
 {
@@ -62,9 +63,7 @@ static void remove_subtree(const char *path)
 	*name++ = '/';
 	while ((de = readdir(dir)) != NULL) {
 		struct stat st;
-		if ((de->d_name[0] == '.') &&
-		    ((de->d_name[1] == 0) ||
-		     ((de->d_name[1] == '.') && de->d_name[2] == 0)))
+		if (is_pseudo_dir_name(de->d_name))
 			continue;
 		strcpy(name, de->d_name);
 		if (lstat(pathbuf, &st))
diff --git a/remote.c b/remote.c
index 570e112..2fb5143 100644
--- a/remote.c
+++ b/remote.c
@@ -4,6 +4,7 @@
 #include "commit.h"
 #include "diff.h"
 #include "revision.h"
+#include "dir.h"
 
 static struct refspec s_tag_refspec = {
 	0,
@@ -634,10 +635,7 @@ static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)
 
 static int valid_remote_nick(const char *name)
 {
-	if (!name[0] || /* not empty */
-	    (name[0] == '.' && /* not "." */
-	     (!name[1] || /* not ".." */
-	      (name[1] == '.' && !name[2]))))
+	if (!name[0] || is_pseudo_dir_name(name))
 		return 0;
 	return !strchr(name, '/'); /* no slash */
 }
diff --git a/transport.c b/transport.c
index 56831c5..d4e3c25 100644
--- a/transport.c
+++ b/transport.c
@@ -50,9 +50,7 @@ static int read_loose_refs(struct strbuf *path, int name_offset,
 	memset (&list, 0, sizeof(list));
 
 	while ((de = readdir(dir))) {
-		if (de->d_name[0] == '.' && (de->d_name[1] == '\0' ||
-				(de->d_name[1] == '.' &&
-				 de->d_name[2] == '\0')))
+		if (is_pseudo_dir_name(de->d_name))
 			continue;
 		ALLOC_GROW(list.entries, list.nr + 1, list.alloc);
 		list.entries[list.nr++] = xstrdup(de->d_name);
-- 
1.6.1.77.g84c9

Re: [PATCH 2/2] Use is_pseudo_dir_name everywhere

[Johannes Sixt]

Alexander Potashev schrieb:
> -		if ((ent->d_name[0] == '.') &&
> -		    (ent->d_name[1] == 0 ||
> -		     ((ent->d_name[1] == '.') && (ent->d_name[2] == 0))))
> +		if (is_pseudo_dir_name(ent->d_name))

Nit-pick: When I read the resulting code, then I will have to look up that
  is_pseudo_dir_name() indeed only checks for "." and "..". But if it were
named is_dot_or_dotdot(), then I would have to do that.

-- Hannes

Re: [PATCH 2/2] Use is_pseudo_dir_name everywhere

[Johannes Sixt]

Johannes Sixt schrieb:
> Alexander Potashev schrieb:
>> -		if ((ent->d_name[0] == '.') &&
>> -		    (ent->d_name[1] == 0 ||
>> -		     ((ent->d_name[1] == '.') && (ent->d_name[2] == 0))))
>> +		if (is_pseudo_dir_name(ent->d_name))
> 
> Nit-pick: When I read the resulting code, then I will have to look up that
>   is_pseudo_dir_name() indeed only checks for "." and "..". But if it were
> named is_dot_or_dotdot(), then I would have to do that.

... then I would *not* have to do that, of course.

-- Hannes

Re: [PATCH 2/2] Use is_pseudo_dir_name everywhere

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Johannes Sixt schrieb:
>> Alexander Potashev schrieb:
>>> -		if ((ent->d_name[0] == '.') &&
>>> -		    (ent->d_name[1] == 0 ||
>>> -		     ((ent->d_name[1] == '.') && (ent->d_name[2] == 0))))
>>> +		if (is_pseudo_dir_name(ent->d_name))
>> 
>> Nit-pick: When I read the resulting code, then I will have to look up that
>>   is_pseudo_dir_name() indeed only checks for "." and "..". But if it were
>> named is_dot_or_dotdot(), then I would have to do that.
>
> ... then I would *not* have to do that, of course.

I think the unstated motivation of this choice of the name is to keep the
door open to include lost+found and friends to the repertoire, and perhaps
to have an isolated place for customization for non-POSIX platforms and
for local conventions.  It is more like is_uninteresting_dirent_name().

As long as this function is used only to detect and skip "uninteresting"
dirent, I think that is not a bad direction.

On the other hand, I am a bit worried about is_empty_dir() abused outside
its intended purpose to say "this directory does not have anything
interesting".  E.g. "Oh, it's empty so we can nuke it":

	if (is_empty_dir(dir))
        	rmdir(dir);

even though the current callers do not do something crazy like this (the
usual order we do things is rmdir() and then check for errors).

Re: [PATCH 2/2] Use is_pseudo_dir_name everywhere

[Alexander Potashev]

On 00:33 Fri 09 Jan     , Junio C Hamano wrote:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
> > Johannes Sixt schrieb:
> >> Alexander Potashev schrieb:
> >>> -		if ((ent->d_name[0] == '.') &&
> >>> -		    (ent->d_name[1] == 0 ||
> >>> -		     ((ent->d_name[1] == '.') && (ent->d_name[2] == 0))))
> >>> +		if (is_pseudo_dir_name(ent->d_name))
> >> 
> >> Nit-pick: When I read the resulting code, then I will have to look up that
> >>   is_pseudo_dir_name() indeed only checks for "." and "..". But if it were
> >> named is_dot_or_dotdot(), then I would have to do that.
> >
> > ... then I would *not* have to do that, of course.
> 
> I think the unstated motivation of this choice of the name is to keep the
> door open to include lost+found and friends to the repertoire, and perhaps
> to have an isolated place for customization for non-POSIX platforms and
> for local conventions.  It is more like is_uninteresting_dirent_name().

I didn't think over the support of 'lost+found'. But the name like
is_uninteresting_dirent_name is more flexible, indeed. I prefer a bit
shorter name, 'is_dummy_dirent_name'.

But if you're going to support 'lost+found's, remember that a Git
repository might have its own 'lost+found' directory. It's a bit crazy,
but it's possible:
	---
	 lost+found/file |    1 +
	 1 files changed, 1 insertions(+), 0 deletions(-)
	 create mode 100644 lost+found/file

	diff --git a/lost+found/file b/lost+found/file
	new file mode 100644
	index 0000000..190a180
	--- /dev/null
	+++ b/lost+found/file
	@@ -0,0 +1 @@
	+123
	-- 

Git shouldn't allow to clone at least repositories that have lost+found
directory into a directory with already existing lost+found (neither
it's a ordinary directory created using 'mkdir' nor it's an ext2's
property)

We should probably forbid cloning to a directory with lost+found,
because a 'lost+found' may appear after pulling from somebody and the
user won't be able to resolve this anyhow.

> 
> As long as this function is used only to detect and skip "uninteresting"
> dirent, I think that is not a bad direction.
> 
> On the other hand, I am a bit worried about is_empty_dir() abused outside
> its intended purpose to say "this directory does not have anything
> interesting".  E.g. "Oh, it's empty so we can nuke it":

I propose to rename it (if it's really necessary) to is_clean_dir, which
means "There's no old crap here, we can safely clone".

> 
> 	if (is_empty_dir(dir))
>         	rmdir(dir);
> 
> even though the current callers do not do something crazy like this (the
> usual order we do things is rmdir() and then check for errors).

I think, it's rather early to send [PATCHES v2] (with updated function
names), will wait for your comments.

Re: [PATCH 2/2] Use is_pseudo_dir_name everywhere

[Junio C Hamano]

Alexander Potashev <aspotashev@gmail.com> writes:

> I didn't think over the support of 'lost+found'.

Yeah, I do not think it is particularly a good idea, but that is what (I
thought) you implied in your original message.  

In any case, you can always do

    $ git clone -n $there it.git
    $ mv it.git/.git . && rmdir it.git && git checkout -f

or something like that (adjust what you move out of it.git when you are
doing a bare clone), so in that sense I do not deeply care about the
motivation of your patch myself.

But I liked the helper function to abstract away the many identical checks
we do for "is it a dot or a dot dot?", and that was the primary reason why
I commented on your patches.