Re: [PATCH v3] worktree repair: detect relative path in .git file correctly

[Junio C Hamano <gitster@pobox.com>]

"Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
>
> Given a state in which the cross-references between the worktree and
> the repository (specifically worktree/id/gitdir in the main repository
> and the .git file in the worktree) are recorded using absolute paths,
> setting 'worktree.useRelativePaths=true' and running 'git worktree
> repair' within the main worktree converts them to relative paths.
>
> Conversely, given a state in which the cross-references are recorded
> using relative paths, one would expect that setting
> 'worktree.useRelativePaths=false' and running 'git worktree repair'
> would convert them to absolute paths. However, they remain as relative
> paths.
>
> This is because we incorrectly use read_gitfile_gently(), which always
> returns an absolute path. To fix this, introduce read_gitfile_raw(),
> which is almost identical to read_gitfile_gently(), but skips checking
> the existence of the referenced repository and returns the path as-is
> from the .git file.

Excellent observation of the problem addressed by the patch.  I wish
everybody wrote his or her proposed log message this clearly.

> diff --git a/setup.c b/setup.c
> index 95909e9603..9041827336 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -962,16 +962,48 @@ void read_gitfile_error_die(int error_code, const char *path)
>   * cases).
>   */
>  const char *read_gitfile_gently(const char *path, int *return_error_code)
> +{
> +	int error_code = 0;
> +	const char *slash;
> +	struct strbuf contents = STRBUF_INIT;
> +	static struct strbuf realpath = STRBUF_INIT;
> +
> +	error_code = read_gitfile_raw(&contents, path);
> +	if (error_code)
> +		goto cleanup_return;
> +
> +	if (!is_absolute_path(contents.buf) && (slash = strrchr(path, '/'))) {
> +		size_t pathlen = slash+1 - path;
> +		char *dir = xstrfmt("%.*s%s", (int)pathlen, path, contents.buf);
> +		strbuf_reset(&contents);
> +		strbuf_addstr(&contents, dir);
> +		free(dir);
> +	}

This massages path = "worktrees/foo/.git" into "worktrees/foo". And
the non-absolute contents.buf "../main/.git/worktrees/foo" that is
relative to gitfile is turned into relative to cwd of our process by
prepending "worktrees/foo" to it.

> +	if (!is_git_directory(contents.buf)) {
> +		error_code = READ_GITFILE_ERR_NOT_A_REPO;
> +		goto cleanup_return;
> +	}

This ensures that the thing referenced by .git file (i.e., what
comes after "gitdir:") is a sanely formatted git directory.

> +	strbuf_realpath(&realpath, contents.buf, 1);

This turns the thing into an absolute path.

Among these three, the last one obviously belongs here.  Leaving the
relative path relative was the reason why we wanted to add
read_gitfile_raw() in the first place.

But moving the other two to here is a bit iffy.  The worktree repair
job used to call read_gitfile_gently(), which means it used to
depend on what the first two did for it, namely, to make the
relative path after "gitdir:" from the .git file relative to the
current process to make it usable, and to ensure that the directory
pointed at by .git is indeed a git directory.  Is it correct to drop
these from the caller, which now calls read_gitfile_raw() instead?

IOW, I am not sure if the two functions are split correctly.  I
expected that the only two things read_gitfile_gently() would do
after read_gitfile_raw() are (1) upon error, jump to cleanup_return,
and (2) otherwise call strbuf_realpath().

Thanks.