Hi,
On Mon, 18 Feb 2008, Lars Hjemli wrote:
> diff --git a/environment.c b/environment.c
> index 3527f16..8058e7b 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -49,6 +49,8 @@ static void setup_git_env(void)
> {
> git_dir = getenv(GIT_DIR_ENVIRONMENT);
> if (!git_dir)
> + git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
> + if (!git_dir)
> git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
I still maintain that the code (maybe not the diff) is easier to read like
this:
if (!git_dir) {
git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
if (!git_dir)
git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
}
> diff --git a/setup.c b/setup.c
> index 4509598..20502be 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -239,6 +239,44 @@ static int check_repository_format_gently(int *nongit_ok)
> }
>
> /*
> + * Try to read the location of the git directory from the .git file,
> + * return path to git directory if found.
> + */
> +const char *read_gitfile_gently(const char *path)
> +{
> + char *buf;
> + struct stat st;
> + int fd;
> + size_t len;
> +
> + if (stat(path, &st))
> + return NULL;
> + if (!S_ISREG(st.st_mode))
> + return NULL;
> + fd = open(path, O_RDONLY);
> + if (fd < 0)
> + die("Error opening %s: %s", path, strerror(errno));
Hmm. Like I said, in the "gently" case, we might want to just print a
warning and return NULL.
However, since you have 5 die()s in this function that would clutter the
code tremendously. I briefly considered (shut your eyes now if you do
not like ugly code):
int (*show_error)(const char *format, ...) = nongit_ok ?
error : (int (*)(const char *format, ...))die;
but now I think a better method would be
static int show_error(int die_on_error, const char *format, ...)
{
va_list params;
va_start(params, err);
if (die_on_error)
die_routine(err, params);
else
error_routine(err, params);
va_end(params);
return -1;
}
This would even be a candidate for a global function die_or_error().
Then you could use it like this:
if (fd < 0 && die("Error opening %s: %s", path, strerror(errno))
return NULL;
Hmm. Seeing what I wrote, it does not really feel elegant.
So maybe we can just scratch all that, and I agree that an invalid .git
file means "no repository" (as opposed to "no valid repository").
In that case, you might want to test for that, too...
Speaking about tests:
> +test_expect_success 'setup' '
> + REAL="$(pwd)/.real" &&
> + mv .git "$REAL" &&
> + echo "gitdir: $REAL" >.git
> +'
Let's not do this. It would clutter the t/ directory unnecessarily.
Instead, do something like this:
test_expect_success setup '
REAL="$(pwd)/.real" &&
mkdir test &&
cd test &&
echo "gitdir: $REAL" > .git
'
Hmm?
Ciao,
Dscho "who likes to write 'Hmm' three times in a mail"
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
