Re: [PATCH 1/3] externalize is_git_repository

From: Jeff King <peff@peff.net>
Date: Dec 5, 2015, 06:58 UTC
Message-ID: <20151205065839.GA21639@sigill.intra.peff.net>
In-Reply-To: <1449252917-3877-1-git-send-email-a.krey@gmx.de>

On Fri, Dec 04, 2015 at 07:15:15PM +0100, Andreas Krey wrote:

Show 9 quoted lines

> diff --git a/cache.h b/cache.h
> index 736abc0..6626e97 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -439,6 +439,7 @@ extern int is_inside_work_tree(void);
>  extern const char *get_git_dir(void);
>  extern const char *get_git_common_dir(void);
>  extern int is_git_directory(const char *path);
> +extern int is_git_repository(struct strbuf *sb);

I wonder if we need to give this a better name if it is going to be globally available. Seeing these two functions defined next to each other, I have to wonder: what is the difference?

The first one ("is_git_directory") is about testing whether a particular path is a ".git" directory. The second is about looking for a ".git" inside the path, and seeing if _that_ points to a valid repository. That's one way for it to be a repository, but not all (a repository could itself simply be a bare repo that passes is_git_directory(), after all).

So maybe a better name for the new function would be "directory_contains_dotgit_repo" or something?

Show 7 quoted lines

>  /*
> + * Return 1 if the given path is the root of a git repository or
> + * submodule else 0. Will not return 1 for bare repositories with the
> + * exception of creating a bare repository in "foo/.git" and calling
> + * is_git_repository("foo").
> + */
> +int is_git_repository(struct strbuf *path)

I think it's more useful to put a descriptive comment like this in the header, where the interface is defined (I know you are just cutting and pasting this, but in the prior version the declaration and the definition were in the same place).

Show 13 quoted lines

> +{
> +	int ret = 0;
> +	int gitfile_error;
> +	size_t orig_path_len = path->len;
> +	assert(orig_path_len != 0);
> +	strbuf_complete(path, '/');
> +	strbuf_addstr(path, ".git");
> +	if (read_gitfile_gently(path->buf, &gitfile_error) || is_git_directory(path->buf))
> +		ret = 1;
> +	if (gitfile_error == READ_GITFILE_ERR_OPEN_FAILED ||
> +	    gitfile_error == READ_GITFILE_ERR_READ_FAILED)
> +		ret = 1;  /* This could be a real .git file, take the
> +			   * safe option and avoid cleaning */

This comment is somewhat stale; we don't know we're cleaning.

Is it always going to be appropriate to err on the side of "yes, this could be a repository?". My hunch is "yes", because we generally consider sub-repos to be precious, so that's the safer thing to do.

-Peff