Re: [PATCH 4/9] setup: split off a function to checks working dir for .git file

[Nguyen Thai Ngoc Duy <pclouds@gmail.com>]

2010/7/24 Jonathan Nieder <jrnieder@gmail.com>:
> The repository discovery procedure looks something like this:
>
>        while (same filesystem) {
>                check .git in working dir
>                check .
>                chdir(..)
>        }
>
> Add a function for the first step to make the actual code look a bit
> closer to that pseudocode.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  setup.c |   20 +++++++++++++-------
>  1 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 16bee6d..3d25d0f 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -348,6 +348,18 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
>        return retval;
>  }
>
> +static int cwd_contains_git_dir(const char **gitfile_dirp)
> +{
> +       const char *gitfile_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
> +       *gitfile_dirp = gitfile_dir;
> +       if (gitfile_dir) {
> +               if (set_git_dir(gitfile_dir))
> +                       die("Repository setup failed");
> +               return 1;
> +       }
> +       return is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT);
> +}
> +
>  /*
>  * We cannot decide in this function whether we are in the work tree or
>  * not, since the config can only be read _after_ this function was called.
> @@ -407,13 +419,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
>                current_device = buf.st_dev;
>        }
>        for (;;) {
> -               gitfile_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
> -               if (gitfile_dir) {
> -                       if (set_git_dir(gitfile_dir))
> -                               die("Repository setup failed");
> -                       break;
> -               }
> -               if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
> +               if (cwd_contains_git_dir(&gitfile_dir))
>                        break;

Notice the two "break;" here, which is the only way to reach out the
code below the loop. Perhaps you should move that code into
cwd_contains_git_dir() too and change its name appropriately.
setup_git_directory_gently() would become "discovery only" as real
setup would be done by three new functions.

Something like this on top (won't even compile):

diff --git a/setup.c b/setup.c
index 8901149..45b9860 100644
--- a/setup.c
+++ b/setup.c
@@ -346,16 +346,36 @@ static const char *setup_explicit_git_dir(const
char *gitdirenv,
 	return retval;
 }

-static int cwd_contains_git_dir(const char **gitfile_dirp)
+static int cwd_contains_git_dir(const char **gitfile_dirp, char *cwd)
 {
 	const char *gitfile_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
 	*gitfile_dirp = gitfile_dir;
 	if (gitfile_dir) {
 		if (set_git_dir(gitfile_dir))
 			die("Repository setup failed");
-		return 1;
+		goto found;
 	}
-	return is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT);
+	if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
+		goto found;
+
+	return NULL;
+
+found:
+	inside_git_dir = 0;
+	if (!work_tree_env)
+		inside_work_tree = 1;
+	root_len = offset_1st_component(cwd);
+	git_work_tree_cfg = xstrndup(cwd, offset > root_len ? offset : root_len);
+	if (check_repository_format_gently(nongit_ok))
+		return NULL;
+	if (offset == len)
+		return NULL;
+
+	/* Make "offset" point to past the '/', and add a '/' at the end */
+	offset++;
+	cwd[len++] = '/';
+	cwd[len] = 0;
+	return cwd + offset;
 }

 /*
@@ -369,6 +389,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	static char cwd[PATH_MAX+1];
 	const char *gitdirenv;
 	const char *gitfile_dir;
+	const char *prefix;
 	int len, offset, ceil_offset, root_len;
 	dev_t current_device = 0;
 	int one_filesystem = 1;
@@ -417,8 +438,9 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		current_device = buf.st_dev;
 	}
 	for (;;) {
-		if (cwd_contains_git_dir(&gitfile_dir))
-			break;
+		if ((prefix = cwd_contains_git_dir(&gitfile_dir, cwd)))
+			return prefix;
+
 		if (is_git_directory(".")) {
 			inside_git_dir = 1;
 			if (!work_tree_env)
@@ -464,22 +486,6 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			die_errno("Cannot change to '%s/..'", cwd);
 		}
 	}
-
-	inside_git_dir = 0;
-	if (!work_tree_env)
-		inside_work_tree = 1;
-	root_len = offset_1st_component(cwd);
-	git_work_tree_cfg = xstrndup(cwd, offset > root_len ? offset : root_len);
-	if (check_repository_format_gently(nongit_ok))
-		return NULL;
-	if (offset == len)
-		return NULL;
-
-	/* Make "offset" point to past the '/', and add a '/' at the end */
-	offset++;
-	cwd[len++] = '/';
-	cwd[len] = 0;
-	return cwd + offset;
 }

 int git_config_perm(const char *var, const char *value)

-- 
Duy