Re: [PATCH 1/3] externalize is_git_repository

[Jeff King <peff@peff.net>]

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

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

>  /*
> + * 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).

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