Re: [PATCH] init: don't set core.worktree when initializing /.git

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mar 31, 2015, 19:14 UTC
Message-ID: <20150331191420.GE22844@google.com>
In-Reply-To: <20150331183423.GD19206@peff.net>

Jeff King wrote:

> No tests, as we would need to be able to write to "/" to do
> so.

t1509-root-worktree.sh is supposed to test the repository-at-/ case. But I wouldn't be surprised if it's bitrotted, since people don't set up a throwaway chroot or VM for tests too often.

[...]

Show 5 quoted lines

> The current behavior isn't _wrong_, in the sense that it's OK to set
> core.worktree when we don't need to. But I think it is unnecessarily
> confusing to users who expect to be able to move .git directories
> around, because it usually just works. So I'd call this an extremely
> minor bug.

This belongs in the commit message.

[...]

Show 13 quoted lines

> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -182,6 +182,21 @@ static int git_init_db_config(const char *k, const char *v, void *cb)
>  	return 0;
>  }
>  
> +/*
> + * If the git_dir is not directly inside the working tree, then git will not
> + * find it by default, and we need to set the worktree explicitly.
> + */
> +static int needs_work_tree_config(const char *git_dir, const char *work_tree)
> +{
> +	/* special case that is missed by the general rule below */

(optional) I'd leave out this comment --- it seems obvious enough in context and the purpose of the comment is unobvious without looking at the history.

Show 5 quoted lines

> +	if (!strcmp(work_tree, "/") && !strcmp(git_dir, "/.git"))
> +		return 0;
> +	if (skip_prefix(git_dir, work_tree, &git_dir) &&
> +	    !strcmp(git_dir, "/.git"))
> +		return 0;

work_tree has been cleaned up with real_path, so in the normal case it contains getcwd() output which will not end with a / unless it has to. The only exception I can see is when git hits the MAXDEPTH limit for symlink resolution (5 nested symlinks), in which case we take what we find instead of erroring out, which looks like a bug.

We have called set_git_dir_init so git_dir has been cleaned up by real_path in the same way. Good.

With or without the commit message, comment, and test improvements
mentioned above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.