Re: [PATCH 2/5] Add platform-independent .git "symlink"

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 18, 2008, 12:34 UTC
Message-ID: <alpine.LSU.1.00.0802181217580.30505@racer.site>
In-Reply-To: <1203331463-13857-3-git-send-email-hjemli@gmail.com>

Hi,

On Mon, 18 Feb 2008, Lars Hjemli wrote:

Show 11 quoted lines

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

Show 25 quoted lines

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

Show 5 quoted lines

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