git archive

git archive

[kenneth johansson]

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

Re: git archive

[Deskin Miller]

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.

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

Deskin Miller

Re: git archive

[kenneth johansson]

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.

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

[Deskin Miller]

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

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

[Jeff King]

On Wed, Oct 22, 2008 at 04:37:22PM -0400, Deskin Miller wrote:

> 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

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

[Charles Bailey]

On Wed, Oct 22, 2008 at 04:37:22PM -0400, Deskin Miller wrote:
> 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/

[PATCH] Fixed git archive for bare repos

[Charles Bailey]

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

Re: [PATCH] Fixed git archive for bare repos

[Deskin Miller]

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>

Re: [PATCH] Fixed git archive for bare repos

[René Scharfe]

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"

[PATCH v2] Fixed git archive for bare repos

[Deskin Miller]

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

Re: [PATCH v2] Fixed git archive for bare repos

[Junio C Hamano]

Thanks all.  Will apply to 'maint' for 1.6.0.4.

Re: git archive

[Nguyen Thai Ngoc Duy]

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.

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

Re: git archive

[Deskin Miller]

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.

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

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

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

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

Re: git archive

[Nguyen Thai Ngoc Duy]

On 10/24/08, Deskin Miller <deskinm@umich.edu> wrote:
> 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.

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