threads / discuss / 16008

git archive

Subject: git archive

## tl;dr

14 messages between Oct 22, 2008 and Oct 25, 2008.

replies: 13people: 7as markdown or json

kenneth johansson· Oct 22, 2008, 08:42 UTC · lore

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
this is the output from some other command on the same bare repository.
>git tag | grep 2.6.27.2
v2.6.27.2
>git cat-file -p v2.6.27.2

object 6bcd6d778419101dd96cbbdf03eeab8d779b1d66 type commit tag v2.6.27.2 tagger Greg Kroah-Hartman <gregkh@suse.de> Sat Oct 18 10:58:00 2008 -0700

This is the v2.6.27.2 stable release
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)

iEYEABECAAYFAkj6I6sACgkQMUfUDdst+ylcigCg0/e3kQZwAqLp6wPuPqdWl7nL X0wAnjuNPJG1OqZfhMiAGccLx0QGvMQz =INvo -----END PGP SIGNATURE-----

>git cat-file -p 6bcd6d778419101dd96cbbdf03eeab8d779b1d66

tree a717af81e5a2e8a7ee36f3b80aa077965f570197 parent 6505670551fa3deeb6e5d7cab6983514384c7220 author Greg Kroah-Hartman <gregkh@suse.de> 1224352642 -0700 committer Greg Kroah-Hartman <gregkh@suse.de> 1224352642 -0700

Linux 2.6.27.2
>git archive --format=tar a717af81e5a2e8a7ee36f3b80aa077965f570197
fatal: not a tree object
>git cat-file -t a717af81e5a2e8a7ee36f3b80aa077965f570197
tree
Deskin Miller· Oct 22, 2008, 13:08 UTC · re: kenneth johansson · lore

Re: git archive

On Wed, Oct 22, 2008 at 08:42:01AM +0000, kenneth johansson wrote:
Show 5 quoted lines
> 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.

Another vote for from me for the discussion carried on here: http://article.gmane.org/gmane.comp.version-control.git/98800

Deskin Miller
kenneth johansson· Oct 22, 2008, 18:45 UTC · re: Deskin Miller · lore

Re: git archive

On Wed, 22 Oct 2008 09:08:29 -0400, Deskin Miller wrote:
Show 12 quoted lines
> 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:
Yes it was a bare repository.
Show 9 quoted lines
> 
> 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.

if I do GIT_DIR=. git archive --format=tar v2.6.27.2

it does work so it looks like you are on the right track.
Deskin Miller· Oct 22, 2008, 20:37 UTC · re: kenneth johansson · lore

[RFC PATCH] archive: fix setup to work in bare repositories

cmd_archive was calling git_config -> setup_git_env prior to write_archive calling setup_git_directory. In a bare repository, the former will set git_dir to be '.git' since the latter has not determined that it's operating in a bare repository yet.

Things are complicated, however, by the fact that git archive --list should work from anywhere, not just in git repositories, so that argument needs to be checked for before setup_git_directory is called. --- On Wed, Oct 22, 2008 at 06:45:30PM +0000, kenneth johansson wrote:

Show 31 quoted lines
> On Wed, 22 Oct 2008 09:08:29 -0400, Deskin Miller 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:
> 
> Yes it was a bare repository.
> 
> > 
> > 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.
> 
> if I do
> GIT_DIR=. git  archive --format=tar v2.6.27.2
> 
> it does work so it looks like you are on the right track.

Looks like this works, but I think it's really ugly; let me know if you have any suggestions for improvement.

 archive.c           |   13 +++++++++++++
 archive.h           |    1 +
 builtin-archive.c   |    4 +++-
 t/t5000-tar-tree.sh |    9 +++++++++
 4 files changed, 26 insertions(+), 1 deletions(-)
diff --git a/archive.c b/archive.c
index e2280df..d8e4373 100644
--- a/archive.c
+++ b/archive.c
@@ -325,6 +325,19 @@ static int parse_archive_args(int argc, const char **argv,
 	return argc;
 }
 
+int archive_parse_options_early(int argc, const char **argv)
+{
+	int i;
+	for (i = 1; i < argc; ++i) {
+		if (!strcmp(argv[i], "--list")) {
+			for (i = 0; i < ARRAY_SIZE(archivers); i++)
+				printf("%s\n", archivers[i].name);
+			exit(0);
+		}
+	}
+	return 0;
+}
+
 int write_archive(int argc, const char **argv, const char *prefix,
 		int setup_prefix)
 {
diff --git a/archive.h b/archive.h
index 0b15b35..ff5b6cf 100644
--- a/archive.h
+++ b/archive.h
@@ -24,6 +24,7 @@ extern int write_tar_archive(struct archiver_args *);
 extern int write_zip_archive(struct archiver_args *);
 
 extern int write_archive_entries(struct archiver_args *args, write_archive_entry_fn_t write_entry);
+extern int archive_parse_options_early(int argc, const char **argv);
 extern int write_archive(int argc, const char **argv, const char *prefix, int setup_prefix);
 
 #endif	/* ARCHIVE_H */
diff --git a/builtin-archive.c b/builtin-archive.c
index 432ce2a..e518113 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -111,6 +111,8 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 {
 	const char *remote = NULL;
 
+	archive_parse_options_early(argc, argv);
+	prefix = setup_git_directory();
 	git_config(git_default_config, NULL);
 
 	remote = extract_remote_arg(&argc, argv);
@@ -119,5 +121,5 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 
 	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
 
-	return write_archive(argc, argv, prefix, 1);
+	return write_archive(argc, argv, prefix, 0);
 }
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index e395ff4..53fe25c 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -192,4 +192,13 @@ test_expect_success \
     'git archive --list outside of a git repo' \
     'GIT_DIR=some/non-existing/directory git archive --list'
 
+test_expect_success \
+    'git archive inside bare repository' \
+    'git clone --bare "$(pwd)"/.git trash-bare &&
+    cd trash-bare &&
+    git archive --format=tar HEAD >/dev/null &&
+    cd .. &&
+    rm -rf trash-bare
+    '
+
 test_done
-- 
1.6.0.2.554.g3041b
Jeff King· Oct 22, 2008, 20:46 UTC · re: Deskin Miller · lore

Re: [RFC PATCH] archive: fix setup to work in bare repositories

On Wed, Oct 22, 2008 at 04:37:22PM -0400, Deskin Miller wrote:
Show 8 quoted lines
> cmd_archive was calling git_config -> setup_git_env prior to
> write_archive calling setup_git_directory.  In a bare repository, the
> former will set git_dir to be '.git' since the latter has not
> determined that it's operating in a bare repository yet.
> 
> Things are complicated, however, by the fact that git archive --list
> should work from anywhere, not just in git repositories, so that
> argument needs to be checked for before setup_git_directory is called.

Should you perhaps be able to call setup_git_directory_gently(), and then once you decide that you really do need the setup, call setup_git_directory()?

You would have to add a "did we already do run" flag to setup_git_directory_gently(), but I think it is already an error to call it twice, so you wouldn't be hurting anything by that.

Note also that by moving the setup, you are moving the chdir() that happens; you may need to prefix paths to any arguments to accomodate this (I don't think it should matter, since git-archive shouldn't look at any paths until after it would have done the setup_git_directory() before, but I didn't check carefully).

-Peff
Charles Bailey· Oct 22, 2008, 21:09 UTC · re: Deskin Miller · lore

Re: [RFC PATCH] archive: fix setup to work in bare repositories

On Wed, Oct 22, 2008 at 04:37:22PM -0400, Deskin Miller wrote:
Show 8 quoted lines
> cmd_archive was calling git_config -> setup_git_env prior to
> write_archive calling setup_git_directory.  In a bare repository, the
> former will set git_dir to be '.git' since the latter has not
> determined that it's operating in a bare repository yet.
> 
> Things are complicated, however, by the fact that git archive --list
> should work from anywhere, not just in git repositories, so that
> argument needs to be checked for before setup_git_directory is called.

Just for some background, I think that this issue might be an unintended consequence of this commit:

b99b5b40cffb5269e4aa38b6b60391b55039e27d

The only reason that the git_config call has been added is to ensure the correct setting of core.autocrlf before convert_to_working_tree is called on any blobs.

I haven't looked in detail, but would moving this call later make for an cleaner change?

-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/
Charles Bailey· Oct 22, 2008, 21:47 UTC · re: Charles Bailey · lore

[PATCH] Fixed git archive for bare repos

This moves the call to git config to a place where it doesn't break the logic for using git archive in a bare repository but retains the fix to make git archive respect core.autocrlf.

Signed-off-by: Charles Bailey <charles@hashpling.org>
---

OK, I've had a chance to have a quick look at this issue and I have an altenative patch.

This is an alternative fix to the current git archive in a bare repository issue.

It's been lightly tested and doesn't re-break the zip / autocrlf issue that the previous introduction of git_config was designed to fix.

 archive.c         |    2 ++
 builtin-archive.c |    2 --
 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/archive.c b/archive.c
index 849eed5..9ac455d 100644
--- a/archive.c
+++ b/archive.c
@@ -336,5 +336,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
 	parse_treeish_arg(argv, &args, prefix);
 	parse_pathspec_arg(argv + 1, &args);
 
+	git_config(git_default_config, NULL);
+
 	return ar->write_archive(&args);
 }
diff --git a/builtin-archive.c b/builtin-archive.c
index 432ce2a..5ceec43 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -111,8 +111,6 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 {
 	const char *remote = NULL;
 
-	git_config(git_default_config, NULL);
-
 	remote = extract_remote_arg(&argc, argv);
 	if (remote)
 		return run_remote_archiver(remote, argc, argv);
-- 
1.6.0.2.534.g5ab59
Deskin Miller· Oct 23, 2008, 01:37 UTC · re: Charles Bailey · lore

Re: [PATCH] Fixed git archive for bare repos

On Wed, Oct 22, 2008 at 10:47:03PM +0100, Charles Bailey wrote:
> OK, I've had a chance to have a quick look at this issue and I have an
> altenative patch.

Thanks for making this much simpler than my version was; it passes the testcase I wrote, as well as the rest of the testsuite, so if you want to include my testcase (or an equivalent one) and re-send, consider it

Tested-by: Deskin Miller <deskinm@umich.edu>
René Scharfe· Oct 24, 2008, 22:19 UTC · re: Charles Bailey · lore

Re: [PATCH] Fixed git archive for bare repos

Charles Bailey schrieb:
> This moves the call to git config to a place where it doesn't break
> the logic for using git archive in a bare repository but retains the
> fix to make git archive respect core.autocrlf.

If one combines your patch, Deskin's commit message and test and extends on the latter a bit then I think we have a winner. :)

Here are a few more tests which create a ZIP file in addition to a tar archive and compare them to their non-bare counterparts.

Care to resend?

Thanks, René

 t/t5000-tar-tree.sh |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index e395ff4..bf5fa25 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -58,6 +58,11 @@ test_expect_success \
      git commit-tree $treeid </dev/null)'
 
 test_expect_success \
+    'create bare clone' \
+    'git clone --bare . bare.git &&
+     cp .gitattributes bare.git/info/attributes'
+
+test_expect_success \
     'remove ignored file' \
     'rm a/ignored'
 
@@ -74,6 +79,14 @@ test_expect_success \
     'diff b.tar b2.tar'
 
 test_expect_success \
+    'git archive in a bare repo' \
+    '(cd bare.git && git archive HEAD) >b3.tar'
+
+test_expect_success \
+    'git archive vs. the same in a bare repo' \
+    'test_cmp b.tar b3.tar'
+
+test_expect_success \
     'validate file modification time' \
     'mkdir extract &&
      "$TAR" xf b.tar -C extract a/a &&
@@ -151,6 +164,14 @@ test_expect_success \
     'git archive --format=zip' \
     'git archive --format=zip HEAD >d.zip'
 
+test_expect_success \
+    'git archive --format=zip in a bare repo' \
+    '(cd bare.git && git archive --format=zip HEAD) >d1.zip'
+
+test_expect_success \
+    'git archive --format=zip vs. the same in a bare repo' \
+    'test_cmp d.zip d1.zip'
+
 $UNZIP -v >/dev/null 2>&1
 if [ $? -eq 127 ]; then
 	echo "Skipping ZIP tests, because unzip was not found"
Deskin Miller· Oct 25, 2008, 15:38 UTC · re: René Scharfe · lore

[PATCH v2] Fixed git archive for bare repos

From 8f0dce75427283e0333cce1f1e66f4eac9978ad4 Mon Sep 17 00:00:00 2001
From: Charles Bailey <charles@hashpling.org>

This moves the call to git_config to a place where it doesn't break the logic for using git archive in a bare repository but retains the fix to make git archive respect core.autocrlf.

Signed-off-by: Charles Bailey <charles@hashpling.org>
Tested-by: Deskin Miller <deskinm@umich.edu>
---
On Sat, Oct 25, 2008 at 12:19:49AM +0200, René Scharfe wrote:
Show 15 quoted lines
> Charles Bailey schrieb:
> > This moves the call to git config to a place where it doesn't break
> > the logic for using git archive in a bare repository but retains the
> > fix to make git archive respect core.autocrlf.
> 
> If one combines your patch, Deskin's commit message and test and extends
> on the latter a bit then I think we have a winner. :)
> 
> Here are a few more tests which create a ZIP file in addition to a tar
> archive and compare them to their non-bare counterparts.
> 
> Care to resend?
> 
> Thanks,
> René

Here's a resend. I've kept Charles's change and commit message, but added René's tests and removed my test, since it was presupposed by the new tests.

Still needs a signoff from René.
 archive.c           |    2 ++
 builtin-archive.c   |    2 --
 t/t5000-tar-tree.sh |   21 +++++++++++++++++++++
 3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/archive.c b/archive.c
index e2280df..45d242b 100644
--- a/archive.c
+++ b/archive.c
@@ -338,5 +338,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
 	parse_treeish_arg(argv, &args, prefix);
 	parse_pathspec_arg(argv + 1, &args);
 
+	git_config(git_default_config, NULL);
+
 	return ar->write_archive(&args);
 }
diff --git a/builtin-archive.c b/builtin-archive.c
index 432ce2a..5ceec43 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -111,8 +111,6 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 {
 	const char *remote = NULL;
 
-	git_config(git_default_config, NULL);
-
 	remote = extract_remote_arg(&argc, argv);
 	if (remote)
 		return run_remote_archiver(remote, argc, argv);
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index e395ff4..0f27d73 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -58,6 +58,11 @@ test_expect_success \
      git commit-tree $treeid </dev/null)'
 
 test_expect_success \
+    'create bare clone' \
+    'git clone --bare . bare.git &&
+     cp .gitattributes bare.git/info/attributes'
+
+test_expect_success \
     'remove ignored file' \
     'rm a/ignored'
 
@@ -74,6 +79,14 @@ test_expect_success \
     'diff b.tar b2.tar'
 
 test_expect_success \
+    'git archive in a bare repo' \
+    '(cd bare.git && git archive HEAD) >b3.tar'
+
+test_expect_success \
+    'git archive vs. the same in a bare repo' \
+    'test_cmp b.tar b3.tar'
+
+test_expect_success \
     'validate file modification time' \
     'mkdir extract &&
      "$TAR" xf b.tar -C extract a/a &&
@@ -151,6 +164,14 @@ test_expect_success \
     'git archive --format=zip' \
     'git archive --format=zip HEAD >d.zip'
 
+test_expect_success \
+    'git archive --format=zip in a bare repo' \
+    '(cd bare.git && git archive --format=zip HEAD) >d1.zip'
+
+test_expect_success \
+    'git archive --format=zip vs. the same in a bare repo' \
+    'test_cmp d.zip d1.zip'
+
 $UNZIP -v >/dev/null 2>&1
 if [ $? -eq 127 ]; then
 	echo "Skipping ZIP tests, because unzip was not found"
-- 
1.6.0.3.515.g304f
Nguyen Thai Ngoc Duy· Oct 23, 2008, 15:33 UTC · re: Deskin Miller · lore

Re: git archive

On 10/22/08, Deskin Miller <deskinm@umich.edu> wrote:
Show 20 quoted lines
> 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.

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

 static void setup_git_env(void)
 {
 	git_dir = getenv(GIT_DIR_ENVIRONMENT);
 	if (!git_dir)
 		git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
-	if (!git_dir)
+	if (!git_dir) {
+		if (!git_dir_discovered)
+			die("Internal error: .git must be relocated at cwd by setup_git_*");
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
+	}
 	git_object_dir = getenv(DB_ENVIRONMENT);
 	if (!git_object_dir) {
 		git_object_dir = xmalloc(strlen(git_dir) + 9);
diff --git a/setup.c b/setup.c
index 78a8041..d404c21 100644
--- a/setup.c
+++ b/setup.c
@@ -368,6 +368,7 @@ const char *read_gitfile_gently(const char *path)
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.
  */
+extern int git_dir_discovered;
 const char *setup_git_directory_gently(int *nongit_ok)
 {
 	const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
@@ -472,6 +473,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		}
 		chdir("..");
 	}
+	/* It is safe to call setup_git_env() now */
+	git_dir_discovered = 1;

 	inside_git_dir = 0;
 	if (!work_tree_env)
---<---


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);
 	git_config(git_default_config, NULL);

 	remote = extract_remote_arg(&argc, argv);
diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index e890f7a..5d401fb 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -916,10 +916,9 @@ static const char mailinfo_usage[] =
 int cmd_mailinfo(int argc, const char **argv, const char *prefix)
 {
 	const char *def_charset;
+	int nongit;

-	/* NEEDSWORK: might want to do the optional .git/ directory
-	 * discovery
-	 */
+	prefix = setup_git_directory_gently(&nongit);
 	git_config(git_default_config, NULL);

 	def_charset = (git_commit_encoding ? git_commit_encoding : "utf-8");
diff --git a/builtin-verify-pack.c b/builtin-verify-pack.c
index 25a29f1..35a4eb2 100644
--- a/builtin-verify-pack.c
+++ b/builtin-verify-pack.c
@@ -115,7 +115,9 @@ int cmd_verify_pack(int argc, const char **argv,
const char *prefix)
 	int verbose = 0;
 	int no_more_options = 0;
 	int nothing_done = 1;
+	int nongit;

+	prefix = setup_git_directory_gently(&nongit);
 	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");

 	if (stdin_paths) {
 		if (hashstdin)
diff --git a/unpack-file.c b/unpack-file.c
index bcdc8bb..1a58d72 100644
--- a/unpack-file.c
+++ b/unpack-file.c
@@ -27,10 +27,10 @@ int main(int argc, char **argv)

 	if (argc != 2)
 		usage("git-unpack-file <sha1>");
+	setup_git_directory();
 	if (get_sha1(argv[1], sha1))
 		die("Not a valid object name %s", argv[1]);

-	setup_git_directory();
 	git_config(git_default_config, NULL);

 	puts(create_temp_file(sha1));
---<---
-- 
Duy
Deskin Miller· Oct 23, 2008, 18:21 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: git archive

On Thu, Oct 23, 2008 at 10:33:31PM +0700, Nguyen Thai Ngoc Duy wrote:
Show 27 quoted lines
> On 10/22/08, Deskin Miller <deskinm@umich.edu> 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.

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
 
Show 10 quoted lines
> ---<---
> 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;' ?
 
Show 53 quoted lines
>  static void setup_git_env(void)
>  {
>  	git_dir = getenv(GIT_DIR_ENVIRONMENT);
>  	if (!git_dir)
>  		git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
> -	if (!git_dir)
> +	if (!git_dir) {
> +		if (!git_dir_discovered)
> +			die("Internal error: .git must be relocated at cwd by setup_git_*");
>  		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
> +	}
>  	git_object_dir = getenv(DB_ENVIRONMENT);
>  	if (!git_object_dir) {
>  		git_object_dir = xmalloc(strlen(git_dir) + 9);
> diff --git a/setup.c b/setup.c
> index 78a8041..d404c21 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -368,6 +368,7 @@ const char *read_gitfile_gently(const char *path)
>   * We cannot decide in this function whether we are in the work tree or
>   * not, since the config can only be read _after_ this function was called.
>   */
> +extern int git_dir_discovered;
>  const char *setup_git_directory_gently(int *nongit_ok)
>  {
>  	const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
> @@ -472,6 +473,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  		}
>  		chdir("..");
>  	}
> +	/* It is safe to call setup_git_env() now */
> +	git_dir_discovered = 1;
> 
>  	inside_git_dir = 0;
>  	if (!work_tree_env)
> ---<---
> 
> 
> 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?

Show 17 quoted lines
>  	git_config(git_default_config, NULL);
> 
>  	remote = extract_remote_arg(&argc, argv);
> diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
> index e890f7a..5d401fb 100644
> --- a/builtin-mailinfo.c
> +++ b/builtin-mailinfo.c
> @@ -916,10 +916,9 @@ static const char mailinfo_usage[] =
>  int cmd_mailinfo(int argc, const char **argv, const char *prefix)
>  {
>  	const char *def_charset;
> +	int nongit;
> 
> -	/* NEEDSWORK: might want to do the optional .git/ directory
> -	 * discovery
> -	 */
> +	prefix = setup_git_directory_gently(&nongit);
Same 'nongit' issue.
Show 15 quoted lines
>  	git_config(git_default_config, NULL);
> 
>  	def_charset = (git_commit_encoding ? git_commit_encoding : "utf-8");
> diff --git a/builtin-verify-pack.c b/builtin-verify-pack.c
> index 25a29f1..35a4eb2 100644
> --- a/builtin-verify-pack.c
> +++ b/builtin-verify-pack.c
> @@ -115,7 +115,9 @@ int cmd_verify_pack(int argc, const char **argv,
> const char *prefix)
>  	int verbose = 0;
>  	int no_more_options = 0;
>  	int nothing_done = 1;
> +	int nongit;
> 
> +	prefix = setup_git_directory_gently(&nongit);
Same 'nongit' issue.
Show 33 quoted lines
>  	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.
 
Show 21 quoted lines
>  	if (stdin_paths) {
>  		if (hashstdin)
> diff --git a/unpack-file.c b/unpack-file.c
> index bcdc8bb..1a58d72 100644
> --- a/unpack-file.c
> +++ b/unpack-file.c
> @@ -27,10 +27,10 @@ int main(int argc, char **argv)
> 
>  	if (argc != 2)
>  		usage("git-unpack-file <sha1>");
> +	setup_git_directory();
>  	if (get_sha1(argv[1], sha1))
>  		die("Not a valid object name %s", argv[1]);
> 
> -	setup_git_directory();
>  	git_config(git_default_config, NULL);
> 
>  	puts(create_temp_file(sha1));
> ---<---
> -- 
> Duy
Nguyen Thai Ngoc Duy· Oct 24, 2008, 03:58 UTC · re: Deskin Miller · lore

Re: git archive

On 10/24/08, Deskin Miller <deskinm@umich.edu> wrote:
Show 34 quoted lines
> On Thu, Oct 23, 2008 at 10:33:31PM +0700, Nguyen Thai Ngoc Duy wrote:
>  > On 10/22/08, Deskin Miller <deskinm@umich.edu> 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.

Show 20 quoted lines
>
>  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.
Show 19 quoted lines
>  > 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.

Show 36 quoted lines
>  >       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

← back to recent threads