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

On Tue, 2016-03-01 at 03:35 -0500, Jeff King wrote:

On Mon, Feb 29, 2016 at 07:52:34PM -0500, David Turner wrote:

quoted

Usually, git calls some form of setup_git_directory at startup. 
 But
sometimes, it doesn't.  Usually, that's OK because it's not really
using the repository.  But in some cases, it is using the repo.  In
those cases, either setup_git_directory_gently must be called, or
the
repository (e.g. the refs) must not be accessed.

It's actually not just setup_git_directory(). We can also use
check_repository_format(), which is used by enter_repo() (and hence
by
things like upload-pack). I think the rule really ought to be: if we
didn't have check_repository_format_gently() tell us we have a valid
repo, we should not access any repo elements (refs, objects, etc).

I'll change that commit message to say
"check_repository_format_gently".

quoted

diff --git a/builtin/grep.c b/builtin/grep.

[snip: this is a probably-good behavior change]

Agreed.

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? 

quoted

diff --git a/git.c b/git.c
index 6cc0c07..51e0508 100644
--- a/git.c
+++ b/git.c

@@ -376,7 +376,7 @@ static struct cmd_struct commands[] = {
 	{ "am", cmd_am, RUN_SETUP | NEED_WORK_TREE },
 	{ "annotate", cmd_annotate, RUN_SETUP },
 	{ "apply", cmd_apply, RUN_SETUP_GENTLY },
-	{ "archive", cmd_archive },
+	{ "archive", cmd_archive, RUN_SETUP_GENTLY },
 	{ "bisect--helper", cmd_bisect__helper, RUN_SETUP },
 	{ "blame", cmd_blame, RUN_SETUP },
 	{ "branch", cmd_branch, RUN_SETUP },

I didn't have to touch this case in my experimenting. I wonder if
it's
because I resolved the "grep" case a little differently.

I taught get_ref_cache() to only assert() that we have a repository
when
we are looking at the main ref-cache, not a submodule. In theory, we
can
look at a submodule from inside an outer non-repo (it's not really a
submodule then, but just a plain git dir). I don't think there's
anything in git right now that says you can't do so, though I think
your
refs-backend work does introduce that restriction (because it
actually
requires the submodules to use the same backend).

So with that requirement, I think we do need to require a repo even
to
access submodule refs. Is that what triggered this change?

No.  What triggered this change was a test failure with your earlier
patch on master -- none of my stuff at all.  The failing command was:

git archive --remote=. HEAD

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:

#0  die (
    err=err@entry=0x57ddb0 "BUG: resolve_ref called without
initializing repo") at usage.c:99
#1  0x00000000004f7ed9 in resolve_ref_1 (sb_refname=0x7c4a50
<sb_refname>, 
    sb_contents=0x7fffffffcfc0, sb_path=0x7fffffffcfe0,
flags=0x7fffffffdaaa, 
    sha1=0x7fffffffd100 "\b\326\377\377\377\177",
resolve_flags=5572384, 
    refname=0x2 <error: Cannot access memory at address 0x2>)
    at refs/files-backend.c:1429
#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
#4  0x0000000000500235 in read_config () at remote.c:705
#5  remote_get_1 (name=0x7fffffffdaaa ".", 
    get_default=get_default@entry=0x4fe230 <remote_for_branch>)
    at remote.c:688
#6  0x00000000005004ca in remote_get (name=<optimized out>) at
remote.c:713
#7  0x00000000004159d8 in run_remote_archiver (name_hint=0x0, 
    exec=0x550720 "git-upload-archive", remote=<optimized out>, 
    argv=0x7fffffffd608, argc=2) at builtin/archive.c:35
#8  cmd_archive (argc=2, argv=0x7fffffffd608, prefix=0x0)
    at builtin/archive.c:104
#9  0x0000000000406051 in run_builtin (argv=0x7fffffffd608, argc=3, 
    p=0x7bd7a0 <commands+96>) at git.c:357
#10 handle_builtin (argc=3, argv=0x7fffffffd608) at git.c:540
#11 0x000000000040519a in main (argc=3, av=<optimized out>) at
git.c:671

I'd think you would need a matching line inside cmd_archive, too. It
should allow "--remote" without a repo, but generating a local
archive
does need one.  And indeed, I see in write_archive() that we run
setup_git_repository ourselves, and die if we're not in a git repo.
So
I'm puzzled about which code path accesses the refs.

I agree that  --remote should work without a repo,  It seems that we do
n't test this and we probably should.  

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? 

quoted

diff --git a/test-match-trees.c b/test-match-trees.c

But your patch here is certainly the right thing to do if we are 
keeping it.

Let's keep it for now; we could always remove it later.