On 10/24/08, Deskin Miller <deskinm@xxxxxxxxx> wrote:
> On Thu, Oct 23, 2008 at 10:33:31PM +0700, Nguyen Thai Ngoc Duy wrote:
> > On 10/22/08, Deskin Miller <deskinm@xxxxxxxxx> wrote:
> > > On Wed, Oct 22, 2008 at 08:42:01AM +0000, kenneth johansson wrote:
> > > > I was going to make a tar of the latest stable linux kernel.
> > > > Done it before but now I got a strange problem.
> > > >
> > > > >git archive --format=tar v2.6.27.2
> > > > fatal: Not a valid object name
> > >
> > >
> > > I had the same thing happen to me, while trying to make an archive of Git.
> > > Were you perchance working in a bare repository, as I was? I spent some time
> > > looking at it and I think git archive sets up the environment in the wrong
> > > order, though of course I never finished a patch so I'm going from memory:
> > >
> > > After looking at the code again, I think the issue is that git_config is called
> > > in builtin-archive.c:cmd_archive before setup_git_directory is called in
> > > archive.c:write_archive. The former ends up setting GIT_DIR to be '.git' even
> > > if you're in a bare repository. My coding skills weren't up to fixing it
> > > easily; moving setup_git_directory before git_config in builtin-archive caused
> > > last test of t5000 to fail: GIT_DIR=some/nonexistent/path git archive --list
> > > should still display the archive formats.
> >
> > The problem affects some other commands as well. I tried the following
> > patch, ran "make test" and discovered "git mailinfo", "git
> > verify-pack", "git hash-object" and "git unpack-file". A bandage patch
> > is at the end of this mail. Solution is as Jeff suggested: call
> > setup_git_directory_gently() early.
>
>
> Nice work. The patches look like they're on the right track (to me at least).
> I'm not sure though what you want to ultimately submit as a patch; I'd suggest
> both, squashed into one, since the check seems like something we'd reasonably
> want no matter what.
No, the patches are not in good shape and have not been tested well. I
just wanted to point out the problem in other commands. Ideally the
check in setup_git_env() should go along with discover_git_directory()
as part of git setup rework.
>
> Few comments spread around below; also, can we see some testcases for
> regression? Or, does the first patch preclude the need for testcases?
>
> Deskin Miller
>
>
> > ---<---
> > diff --git a/environment.c b/environment.c
> > index 0693cd9..00ed640 100644
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -49,14 +49,18 @@ static char *work_tree;
> >
> > static const char *git_dir;
> > static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;
> > +int git_dir_discovered;
>
>
> Should this be 'int git_dir_discovered = 0;' ?
It is initialized by default IIRC.
> > Bandage patch:
> >
> > ---<---
> > diff --git a/builtin-archive.c b/builtin-archive.c
> > index 432ce2a..5ea0a12 100644
> > --- a/builtin-archive.c
> > +++ b/builtin-archive.c
> > @@ -110,7 +110,9 @@ static const char *extract_remote_arg(int *ac,
> > const char **av)
> > int cmd_archive(int argc, const char **argv, const char *prefix)
> > {
> > const char *remote = NULL;
> > + int nongit;
> >
> > + prefix = setup_git_directory_gently(&nongit);
>
>
> Here and elsewhere, the 'nongit' variable isn't used.
> setup_git_directory_gently can be passed a NULL pointer, why not do that?
Passing NULL to setup_git_directory_gently() tells it to die() if no
git repo can be found. If you pass a variable to it, it will set the
variable to 1 if no repo is found, 0 otherwise.
> > git_config(git_default_config, NULL);
> > while (1 < argc) {
> > if (!no_more_options && argv[1][0] == '-') {
> > diff --git a/hash-object.c b/hash-object.c
> > index 20937ff..a52b6be 100644
> > --- a/hash-object.c
> > +++ b/hash-object.c
> > @@ -78,19 +78,20 @@ int main(int argc, const char **argv)
> > const char *prefix = NULL;
> > int prefix_length = -1;
> > const char *errstr = NULL;
> > + int nongit;
> >
> > type = blob_type;
> >
> > - git_config(git_default_config, NULL);
> > -
> > argc = parse_options(argc, argv, hash_object_options, hash_object_usage, 0);
> >
> > - if (write_object) {
> > - prefix = setup_git_directory();
> > - prefix_length = prefix ? strlen(prefix) : 0;
> > - if (vpath && prefix)
> > - vpath = prefix_filename(prefix, prefix_length, vpath);
> > - }
> > + prefix = setup_git_directory_gently(&nongit);
> > + git_config(git_default_config, NULL);
> > + prefix_length = prefix ? strlen(prefix) : 0;
> > + if (vpath && prefix)
> > + vpath = prefix_filename(prefix, prefix_length, vpath);
> > +
> > + if (write_object && nongit)
> > + die("Git repository required");
>
>
> I'd move this check up to just after setup_git_directory_gently.
Yeah, sounds reasonable.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
