Re: git archive

[Deskin Miller <deskinm@umich.edu>]

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