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

[Jonathan Nieder <jrnieder@gmail.com>]

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.

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

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

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