Re: [PATCH] refs.c: get_ref_cache: use a bucket hash

[Jeff King <peff@peff.net>]

On Sat, Nov 14, 2015 at 02:35:01PM +0100, Andreas Krey wrote:

> On Fri, 13 Nov 2015 19:01:18 +0000, Jeff King wrote:
> ...
> >   2. But for a little more work, pushing the is_git_directory() check
> >      out to the call-sites gives us probably saner semantics overall.
> 
> Oops, now I get it[1]: You mean replacing resolve_gitlink_ref usages
> with is_git_directory, like:

Yes. I mistakenly said is_git_directory, when I really meant
is_git_repository, the new function added in 0179ca7a62. You seem to
have figured out what I meant, but the critical thing is that we check
"$dir/.git", not just "$dir" (and check it both as a git dir and as a
gitfile, as is_git_repository() does).

I'm not sure if we can simply make that function public or not. It's
mostly straightforward, but it does err on the side of "yes, this is a
git repo" if we see a ".git" file we can't read. I think that's probably
reasonable in most sites, but I didn't look closely.

> diff --git a/dir.c b/dir.c
> index d2a8f06..7765dc6 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1375,8 +1375,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
>  		if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
>  			break;
>  		if (!(dir->flags & DIR_NO_GITLINKS)) {
> -			unsigned char sha1[20];
> -			if (resolve_gitlink_ref(dirname, "HEAD", sha1) == 0)
> +			if (is_git_directory(dirname))
>  				return path_untracked;
>  		}
>  		return path_recurse;
> 
> That, I like. If it is correct.

Yes, that's what I had in mind, modulo the directory/repository thing
above (the is_git_repository function also takes a strbuf, so we'd need
to handle that extra allocation somewhere).

-Peff