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

From: Jeff King <peff@peff.net>
Date: Nov 16, 2015, 16:31 UTC
Message-ID: <20151116163130.GA15046@sigill.intra.peff.net>
In-Reply-To: <20151114133501.GI16219@inner.h.apk.li>

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

Show 7 quoted lines

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

Show 16 quoted lines

> 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