Re: [PATCH v7 01/33] setup: call setup_git_directory_gently before accessing refs

From: Jeff King <peff@peff.net>
Date: Mar 2, 2016, 02:45 UTC
Message-ID: <20160302024509.GA20625@sigill.intra.peff.net>
In-Reply-To: <1456876072.5981.5.camel@twopensource.com>

On Tue, Mar 01, 2016 at 06:47:52PM -0500, David Turner wrote:

Show 9 quoted lines

> > My fix for this was to teach read_mailmap to avoid looking for
> > HEAD:.mailmap if we are not in a repository, but to continue with the
> > others (.mailmap in the cwd, and the mailmap.file config variable).
> > ...
> > But I do think your patch is a potential regression there, if anybody
> > does do that.
> 
> Your version sounds better.  But I don't see it in the patch set you
> sent earlier?

It's not. Sorry to be unclear. There were _two_ cleanups I was talking about (cases where we don't check whether we're in a repo, and fact that the repo startup code is unreliable), and I got sucked into the second one. I'll try to work up and share my startup_info one today.

Show 9 quoted lines

> When writing my patch, I had assumed that the issue was the resolve_ref
> on the HEAD that's an argument -- but it's not.  The actual traceback
> is:
> [...]
> #2  resolve_ref_unsafe (refname=refname@entry=0x550b3b "HEAD", 
>     resolve_flags=resolve_flags@entry=0, 
>     sha1=sha1@entry=0x7fffffffd100 "\b\326\377\377\377\177", 
>     flags=flags@entry=0x7fffffffd0fc) at refs/files-backend.c:1600
> #3  0x00000000004ffe69 in read_config () at remote.c:471

Oh, right. I did see problems here but missed them when comparing my patch to yours. I ended up in remote.c:read_config, having it check whether startup_info->have_repository is set; if it isn't, there is no point in looking at HEAD.

That covers this case, and several others I happened across. Thanks for clarifying.

Show 17 quoted lines

> I'm not sure what the right way to fix this is -- in read_config, we're
> about to access some stuff in a repo (config, HEAD).  It's OK to skip
> that stuff if we're not in a repo, but we don't want to run
> setup_git_directory twice (that breaks some stuff), and some of the
> other callers have already called it.  On top of your earlier
> repo_initialized patch, we could add the following to read_config:
> 
> +       if (!repo_initialized) {
> +               int nongit = 0;
> +               setup_git_directory_gently(&nongit);
> +               if (nongit)
> +                       return;
> +       }
> 
> But that patch I think was not intended to be permanent.  Still, it
> does seem odd that there's no straightforward way to know if the repo
> is initialized. Am I missing something?

No, there isn't a straightforward way; I think we'll have to add one. I'll polish up my series which does this.

-Peff