[PATCHv3 3/5] Add a common is_gitfile function

Phil Hord· Oct 5, 2011, 13:33 UTC · lore

There are at least two locations in the code that check for gitfiles. Each one is slightly different for pretty good reasons. Work towards a common API by abstracting the "is this a gitfile" question into a single function.

Signed-off-by: Phil Hord <hordp@cisco.com>

diff --git a/builtin/clone.c b/builtin/clone.c
index 488f48e..5110399 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -120,13 +120,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 			return xstrdup(absolute_path(path));
 		} else if (S_ISREG(st.st_mode) && st.st_size > 8) {
 			/* Is it a "gitfile"? */
-			char signature[8];
-			int len, fd = open(path, O_RDONLY);
-			if (fd < 0)
-				continue;
-			len = read_in_full(fd, signature, 8);
-			close(fd);
-			if (len != 8 || strncmp(signature, "gitdir: ", 8))
+			if (!is_gitfile(path))
 				continue;
 			path = read_gitfile(path);
 			if (path) {
diff --git a/cache.h b/cache.h
index 7eeb8cf..61e1b0f 100644
--- a/cache.h
+++ b/cache.h
@@ -441,6 +441,7 @@ extern const char *get_git_work_tree(void);
 extern const char *read_gitfile(const char *path);
 extern const char *resolve_gitdir(const char *suspect);
 extern void set_git_work_tree(const char *tree);
+extern int is_gitfile(const char *path);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
diff --git a/setup.c b/setup.c
index 61c22e6..a3d5a41 100644
--- a/setup.c
+++ b/setup.c
@@ -358,7 +358,7 @@ const char *read_gitfile(const char *path)
 
 	if (stat(path, &st))
 		return NULL;
-	if (!S_ISREG(st.st_mode))
+	if (!is_gitfile(path))
 		return NULL;
 	fd = open(path, O_RDONLY);
 	if (fd < 0)
@@ -368,9 +368,6 @@ const char *read_gitfile(const char *path)
 	close(fd);
 	if (len != st.st_size)
 		die("Error reading %s", path);
-	buf[len] = '\0';
-	if (prefixcmp(buf, "gitdir: "))
-		die("Invalid gitfile format: %s", path);
 	while (buf[len - 1] == '\n' || buf[len - 1] == '\r')
 		len--;
 	if (len < 9)
@@ -397,6 +394,32 @@ const char *read_gitfile(const char *path)
 	return path;
 }
 
+/*
+ * See if the referenced file looks like a 'gitfile'.
+ * Does not try to determine if the referenced gitdir is actually valid.
+ */
+int is_gitfile(const char *path)
+{
+	struct stat st;
+	char buf[9];
+	int fd, len;
+	if (stat(path, &st))
+		return 0;
+	if (!S_ISREG(st.st_mode))
+		return 0;
+	if (st.st_size < 10 || st.st_size > PATH_MAX)
+		return 0;
+
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		return 0;
+	len = read_in_full(fd, buf, sizeof(buf));
+	close(fd);
+	if (len != sizeof(buf))
+		return 0;
+	return !prefixcmp(buf, "gitdir: ");
+}
+
 static const char *setup_explicit_git_dir(const char *gitdirenv,
 					  char *cwd, int len,
 					  int *nongit_ok)

-- 
1.7.7.505.ga09f6

Nguyen Thai Ngoc Duy· Oct 6, 2011, 02:59 UTC · re: Phil Hord · lore

Re: [PATCHv3 3/5] Add a common is_gitfile function

On Thu, Oct 6, 2011 at 12:33 AM, Phil Hord <hordp@cisco.com> wrote:

Show 23 quoted lines

> diff --git a/setup.c b/setup.c
> index 61c22e6..a3d5a41 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -358,7 +358,7 @@ const char *read_gitfile(const char *path)
>
>        if (stat(path, &st))
>                return NULL;
> -       if (!S_ISREG(st.st_mode))
> +       if (!is_gitfile(path))
>                return NULL;
>        fd = open(path, O_RDONLY);
>        if (fd < 0)
> @@ -368,9 +368,6 @@ const char *read_gitfile(const char *path)
>        close(fd);
>        if (len != st.st_size)
>                die("Error reading %s", path);
> -       buf[len] = '\0';
> -       if (prefixcmp(buf, "gitdir: "))
> -               die("Invalid gitfile format: %s", path);
>        while (buf[len - 1] == '\n' || buf[len - 1] == '\r')
>                len--;
>        if (len < 9)

I think you are changing the behavior here. Currently if .git is a file and not a valid gitfile, die() will be called and repo discovery process stops. With your patch, it skips invalid .git files and moves on.

I'm not saying it's good or bad, just pointing out.

-- 
Duy