[PATCH 2/2] worktree: provide better prefix to go back to original cwd

[PATCH 2/2] worktree: provide better prefix to go back to original cwd

[pclouds]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

When both GIT_DIR and GIT_WORK_TREE are set, if cwd is outside
worktree, prefix (the one passed to every builtin commands) will be
set to NULL, which means "user stays at worktree topdir", but cwd is
not moved to worktree topdir.

As a consequence, command line arguments will be interpreted
differently. Assume there is a file named "bar" in foo/.git. A command
can expect command line arguments to be "path in repository":

$ GIT_DIR=foo/.git GIT_WORK_TREE=foo git blah bar

or "path in file system":

$ GIT_DIR=foo/.git GIT_WORK_TREE=foo git blah foo/bar

Some commands may want "path in repository" and "path in file system"
to be identical. Moreover, output from commands in such situations are
relative to worktree topdir (because prefix is NULL), not what users
expect. It's just confusing.

This patch does not address that. Instead it prepares some variables
to ease access to original cwd in this case. Commands that work well
outside worktree should:

 - detect this case, via is_inside_work_tree()

 - enter_work_tree() (not strictly needed, but I believe it would
   less work this way)

 - prepend startup_info->cwd_to_worktree to output so it becomes
   relative to original cwd

 - prepend startup_info->worktree_to_cwd to command line arguments
   if they are meant to access file system

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I don't care Windows UNC path. And I did not test the case
 where cwd is on C:\ while worktree is on another drive.

 As stated elsewhere, I neglect other cases where cwd may be
 outside worktree. The goal of these patches is to help
 Chris' grep series. If it works out, I'll fix the rest.

 Sorry Chris, I think I have dragged you into worktree setup
 mess.

 builtin/rev-parse.c        |   10 ++++
 cache.h                    |    2 +
 setup.c                    |  119 ++++++++++++++++++++++++++++++++++++++++++--
 t/t1510-worktree-prefix.sh |   52 +++++++++++++++++++
 4 files changed, 179 insertions(+), 4 deletions(-)
 create mode 100755 t/t1510-worktree-prefix.sh

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index a5a1c86..525610e 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -623,6 +623,16 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 					puts(prefix);
 				continue;
 			}
+			if (!strcmp(arg, "--cwd-to-worktree")) {
+				if (startup_info->cwd_to_worktree)
+					puts(startup_info->cwd_to_worktree);
+				continue;
+			}
+			if (!strcmp(arg, "--worktree-to-cwd")) {
+				if (startup_info->worktree_to_cwd)
+					puts(startup_info->worktree_to_cwd);
+				continue;
+			}
 			if (!strcmp(arg, "--show-cdup")) {
 				const char *pfx = prefix;
 				if (!is_inside_work_tree()) {
diff --git a/cache.h b/cache.h
index e71db23..be35b8f 100644
--- a/cache.h
+++ b/cache.h
@@ -1111,6 +1111,8 @@ const char *split_cmdline_strerror(int cmdline_errno);
 /* git.c */
 struct startup_info {
 	int have_repository;
+	char *cwd_to_worktree; /* path from original cwd to worktree */
+	char *worktree_to_cwd; /* path from worktree to original cwd */
 };
 extern struct startup_info *startup_info;
 
diff --git a/setup.c b/setup.c
index 3fbe928..1f5117a 100644
--- a/setup.c
+++ b/setup.c
@@ -320,10 +320,120 @@ const char *read_gitfile_gently(const char *path)
 	return path;
 }
 
+/*
+ * Given "foo/bar" and "hey/hello/world", return "../../hey/hello/world/"
+ * Either path1 or path2 can be NULL. All must be canonical.
+ */
+static char *make_path_to_path(const char *path1, const char *path2)
+{
+	int nr_back = 0;
+	int i, pathlen = path2 ? strlen(path2) : 0;
+	char *buf, *p;
+
+	if (path1 && *path1) {
+		nr_back = 1;
+		while (*path1) {
+			if (is_dir_sep(*path1))
+				nr_back++;
+			path1++;
+		}
+	}
+
+	if (!nr_back && !pathlen)
+		return NULL;
+
+	p = buf = xmalloc(3*nr_back + pathlen + 2); /* "../"^nr_back + path2 + '/' + NULL */
+	for (i = 0; i < nr_back; i++) {
+		memcpy(p, "../", 3);
+		p += 3;
+	}
+	if (pathlen) {
+		memcpy(p, path2, pathlen);
+		p += pathlen;
+		if (p[-1] != '/') /* path2 may be c:/, don't put another slash  */
+			*p++ = '/';
+	}
+	*p = '\0';
+	return buf;
+}
+
+/*
+ * Return a prefix if cwd inside worktree, or NULL otherwise.
+ * Also fill startup_info struct.
+ */
+static const char *setup_prefix(const char *cwd)
+{
+	const char *worktree = get_git_work_tree();
+	int len = 0, cwd_len = strlen(cwd), worktree_len = strlen(worktree);
+
+	while (worktree[len] && worktree[len] == cwd[len])
+		len++;
+
+	if (!worktree[len] && !cwd[len]) {
+		if (startup_info) {
+			startup_info->cwd_to_worktree = NULL;
+			startup_info->worktree_to_cwd = NULL;
+		}
+		return NULL;
+	}
+	/* get /foo/, not /foo/baa if /foo/baa1 and /foo/baa2 are given */
+	else if (worktree[len] && cwd[len]) {
+		while (len && !is_dir_sep(worktree[len]))
+			len--;
+		len++;
+
+		/* Worktree and cwd are on different drives? */
+		if (len == 3 && has_dos_drive_prefix(cwd)) {
+			if (startup_info) {
+				/* make_path_to_path will add the trailing slash */
+				startup_info->cwd_to_worktree = make_path_to_path(NULL, worktree);
+				startup_info->worktree_to_cwd = make_path_to_path(NULL, cwd);
+			}
+			return NULL;
+		}
+	}
+	else {
+		if (worktree[len]) {
+			if (!is_dir_sep(worktree[len])) {
+				while (len && !is_dir_sep(worktree[len]))
+					len--;
+			}
+		}
+		else {
+			if (!is_dir_sep(cwd[len])) {
+				while (len && !is_dir_sep(cwd[len]))
+					len--;
+			}
+		}
+		len++;		/* must be a slash here, skip it */
+	}
+
+	if (len < cwd_len && len < worktree_len) {
+		if (startup_info) {
+			startup_info->cwd_to_worktree = make_path_to_path(cwd+len, worktree+len);
+			startup_info->worktree_to_cwd = make_path_to_path(worktree+len, cwd+len);
+		}
+		return NULL;
+	}
+
+	if (startup_info) {
+		if (len < cwd_len) { /* cwd inside worktree */
+			startup_info->cwd_to_worktree = make_path_to_path(cwd+len, NULL);
+			startup_info->worktree_to_cwd = make_path_to_path(NULL, cwd+len);
+		}
+		else {
+			startup_info->cwd_to_worktree = make_path_to_path(NULL, worktree+len);
+			startup_info->worktree_to_cwd = make_path_to_path(worktree+len, NULL);
+		}
+	}
+
+	return len < cwd_len ? cwd+len : NULL;
+}
+
 static const char *setup_explicit_git_dir(const char *gitdirenv,
 				const char *work_tree_env, int *nongit_ok)
 {
-	static char buffer[1024 + 1];
+	static char buffer[PATH_MAX];
 	const char *retval;
 
 	if (PATH_MAX - 40 < strlen(gitdirenv))
@@ -344,9 +454,10 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 	}
 	if (check_repository_format_gently(nongit_ok))
 		return NULL;
-	retval = get_relative_cwd(buffer, sizeof(buffer) - 1,
-			get_git_work_tree());
-	if (!retval || !*retval)
+	if (!getcwd(buffer, sizeof(buffer)))
+		die_errno("can't find the current directory");
+	retval = setup_prefix(buffer);
+	if (!retval)
 		return NULL;
 	set_git_dir(make_absolute_path(gitdirenv));
 	if (chdir(work_tree_env) < 0)
diff --git a/t/t1510-worktree-prefix.sh b/t/t1510-worktree-prefix.sh
new file mode 100755
index 0000000..3839493
--- /dev/null
+++ b/t/t1510-worktree-prefix.sh
@@ -0,0 +1,52 @@
+#!/bin/sh
+
+test_description='test rev-parse --cwd-to-worktree and --worktree-to-cwd'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	mkdir foo bar &&
+	mv .git foo &&
+	mkdir foo/bar &&
+	GIT_DIR=`pwd`/foo/.git &&
+	GIT_WORK_TREE=`pwd`/foo &&
+	export GIT_DIR GIT_WORK_TREE
+'
+
+test_expect_success 'at root' '
+	(
+	cd foo &&
+	git rev-parse --cwd-to-worktree --worktree-to-cwd >result &&
+	: >expected &&
+	test_cmp expected result
+	)
+'
+
+test_expect_success 'cwd inside worktree' '
+	(
+	cd foo/bar &&
+	git rev-parse --cwd-to-worktree --worktree-to-cwd >result &&
+	echo ../ >expected &&
+	echo bar/ >>expected &&
+	test_cmp expected result
+	)
+'
+
+test_expect_success 'cwd outside worktree' '
+	git rev-parse --cwd-to-worktree --worktree-to-cwd >result &&
+	echo foo/ >expected &&
+	echo ../ >>expected &&
+	test_cmp expected result
+'
+
+test_expect_success 'cwd outside worktree (2)' '
+	(
+	cd bar &&
+	git rev-parse --cwd-to-worktree --worktree-to-cwd >result &&
+	echo ../foo/ >expected &&
+	echo ../bar/ >>expected &&
+	test_cmp expected result
+	)
+'
+
+test_done
-- 
1.7.1.rc1.70.g788ca

Re: [PATCH 2/2] worktree: provide better prefix to go back to original cwd

[Nguyen Thai Ngoc Duy]

On Thu, Oct 7, 2010 at 11:46 AM,  <pclouds@gmail.com> wrote:
> +               /* Worktree and cwd are on different drives? */
> +               if (len == 3 && has_dos_drive_prefix(cwd)) {
> +                       if (startup_info) {
> +                               /* make_path_to_path will add the trailing slash */
> +                               startup_info->cwd_to_worktree = make_path_to_path(NULL, worktree);
> +                               startup_info->worktree_to_cwd = make_path_to_path(NULL, cwd);
> +                       }
> +                       return NULL;
> +               }

Went for lunch and realized this code is just wrong. Gaahh..
-- 
Duy

Re: [PATCH 2/2] worktree: provide better prefix to go back to original cwd

[Junio C Hamano]

pclouds@gmail.com writes:

> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> When both GIT_DIR and GIT_WORK_TREE are set, if cwd is outside
> worktree, prefix (the one passed to every builtin commands) will be
> set to NULL, which means "user stays at worktree topdir", but cwd is
> not moved to worktree topdir.

Well, NULL traditionally meant "_if_ the cwd is inside the working tree,
it is at the top", not "stays at worktree topdir" if you started from
elsewhere.

And when cwd is inside the working tree, we do want to feed paths given
from the command line after adding the prefix, so that we will see the
paths relative to the cwd after we do cd-to-topdir.

Obviously when you run git command that requires you to be in a working
tree from outside the working tree, relative or full paths would not make
any difference.  The command should simply fail.

I suspect that you are introducing a new semantics on top of that
traditional semantics; if so, you may want to state it more clearly.

For example:

    When you run git command that requires you to be in a working tree
    from outside the working tree, the command should simply fail.

    When GIT_WORK_TREE is in use, however, it could be argued that we do
    not necessarily have to be in anywhere in the working tree to perform
    a whole-tree operation.  Instead, we could just say the operation
    always runs as if the command was started at the root level of the
    working tree.

    To support this new mode of operation, however, prefix needs to be
    adjusted to allow the program, after running chdir(2) to the root of
    the working tree, to refer to non-absolute paths originally given from
    the command line as relative to the original cwd.  This patch adds a
    mechanism to support that.

I have a queasy feeling about the idea of the second paragraph above,
though.

If the original cwd is inside GIT_WORK_TREE, limiting ourselves inside
prefix naturally limits the operation to the subdirectory we started from
(if the original cwd is at GIT_WORK_TREE, that would make it a whole-tree
operation).  A natural extension of this idea to limit the operation to
the part of the subtree of the working tree we started from is to refuse
to work in the case where the original cwd is outside GIT_WORK_TREE (the
current implementation of GIT_WORK_TREE may or may not correctly implement
it, though---I never use it myself).

Futzing with the prefix that is not a prefix to reach into the working
tree from sideways may make the relative paths given from the command line
mean something to the current implementation, but it doesn't change the
fundamental fact that you are introducing a funny special case where your
cwd does _not_ mean anything with respect to which part of the working
tree should be affected.

> Some commands may want "path in repository" and "path in file system"
> to be identical. Moreover, output from commands in such situations are
> relative to worktree topdir (because prefix is NULL), not what users
> expect. It's just confusing.

My gut feeling is that this is probably made more confusing, not less,
with the change.  Perhaps we should instead make sure this fails?

    $ cd /srv/git/git.git
    $ export GIT_DIR=$(pwd)/.git GIT_WORK_TREE=$(pwd)
    $ cd /var/tmp ;# no git stuff there
    $ git status

Re: [PATCH 2/2] worktree: provide better prefix to go back to original cwd

[Nguyen Thai Ngoc Duy]

On Thu, Oct 7, 2010 at 12:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> pclouds@gmail.com writes:
>
>> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>
>> When both GIT_DIR and GIT_WORK_TREE are set, if cwd is outside
>> worktree, prefix (the one passed to every builtin commands) will be
>> set to NULL, which means "user stays at worktree topdir", but cwd is
>> not moved to worktree topdir.
>
> Well, NULL traditionally meant "_if_ the cwd is inside the working tree,
> it is at the top", not "stays at worktree topdir" if you started from
> elsewhere.
>
> And when cwd is inside the working tree, we do want to feed paths given
> from the command line after adding the prefix, so that we will see the
> paths relative to the cwd after we do cd-to-topdir.
>
> Obviously when you run git command that requires you to be in a working
> tree from outside the working tree, relative or full paths would not make
> any difference.  The command should simply fail.
>
> I suspect that you are introducing a new semantics on top of that
> traditional semantics; if so, you may want to state it more clearly.

Yes. The better semantics _when_ cwd is outside worktree.

> For example:
>
>    When you run git command that requires you to be in a working tree
>    from outside the working tree, the command should simply fail.
>
>    When GIT_WORK_TREE is in use, however, it could be argued that we do
>    not necessarily have to be in anywhere in the working tree to perform
>    a whole-tree operation.  Instead, we could just say the operation
>    always runs as if the command was started at the root level of the
>    working tree.
>
>    To support this new mode of operation, however, prefix needs to be
>    adjusted to allow the program, after running chdir(2) to the root of
>    the working tree, to refer to non-absolute paths originally given from
>    the command line as relative to the original cwd.  This patch adds a
>    mechanism to support that.
>
> I have a queasy feeling about the idea of the second paragraph above,
> though.
>
> If the original cwd is inside GIT_WORK_TREE, limiting ourselves inside
> prefix naturally limits the operation to the subdirectory we started from
> (if the original cwd is at GIT_WORK_TREE, that would make it a whole-tree
> operation).  A natural extension of this idea to limit the operation to
> the part of the subtree of the working tree we started from is to refuse
> to work in the case where the original cwd is outside GIT_WORK_TREE (the
> current implementation of GIT_WORK_TREE may or may not correctly implement
> it, though---I never use it myself).

I tend to think that as we go up to worktree's root, prefix is shorten
and the operation area is widen. When cwd is at worktree's, we operate
on full worktree. If it goes up one level higher, the operation area
remains full worktree (but not everything under cwd because cwd now
can have non-worktree directories).

> Futzing with the prefix that is not a prefix to reach into the working
> tree from sideways may make the relative paths given from the command line
> mean something to the current implementation, but it doesn't change the
> fundamental fact that you are introducing a funny special case where your
> cwd does _not_ mean anything with respect to which part of the working
> tree should be affected.

It would mean full worktree is affected. But not full cwd. In other
words, in set theory, it's a union of cwd and worktree.

>> Some commands may want "path in repository" and "path in file system"
>> to be identical. Moreover, output from commands in such situations are
>> relative to worktree topdir (because prefix is NULL), not what users
>> expect. It's just confusing.
>
> My gut feeling is that this is probably made more confusing, not less,
> with the change.  Perhaps we should instead make sure this fails?
>
>    $ cd /srv/git/git.git

Err.. you did not mean bare repo, did you?

>    $ export GIT_DIR=$(pwd)/.git GIT_WORK_TREE=$(pwd)
>    $ cd /var/tmp ;# no git stuff there
>    $ git status

I think it used to fail some time ago. Though I see nothing wrong if
that command works. And it'd be better if it outputs relative to
/var/tmp rather than /srv/git/git.git. I don't use this a lot but
sometimes I export GIT_DIR/GIT_WORK_TREE, cd around then do "git add
/srv/git/git.git/blah". To me, it should work. I may stand outside
worktree, but I give input inside worktree.
-- 
Duy

Re: [PATCH 2/2] worktree: provide better prefix to go back to original cwd

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Thu, Oct 7, 2010 at 12:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> pclouds@gmail.com writes:
>> ...
>> If the original cwd is inside GIT_WORK_TREE, limiting ourselves inside
>> prefix naturally limits the operation to the subdirectory we started from
>> (if the original cwd is at GIT_WORK_TREE, that would make it a whole-tree
>> operation).  A natural extension of this idea to limit the operation to
>> the part of the subtree of the working tree we started from is to refuse
>> to work in the case where the original cwd is outside GIT_WORK_TREE (the
>> current implementation of GIT_WORK_TREE may or may not correctly implement
>> it, though---I never use it myself).
>
> I tend to think that as we go up to worktree's root, prefix is shorten
> and the operation area is widen. When cwd is at worktree's, we operate
> on full worktree. If it goes up one level higher, the operation area
> remains full worktree (but not everything under cwd because cwd now
> can have non-worktree directories).

I have a feeling that you did not understand my /srv/git/git.git (no, it
is not a bare repository) vs /var/tmp example.

I think it makes the new semantics much less yucky if the special case is
limited to "the working tree is a subdirectory somewhere under cwd".  But
does your patch check and notice that /var/tmp is not above the working
tree and they are completely unrelated?

Re: [PATCH 2/2] worktree: provide better prefix to go back to original cwd

[Nguyen Thai Ngoc Duy]

On Thu, Oct 7, 2010 at 9:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> On Thu, Oct 7, 2010 at 12:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> pclouds@gmail.com writes:
>>> ...
>>> If the original cwd is inside GIT_WORK_TREE, limiting ourselves inside
>>> prefix naturally limits the operation to the subdirectory we started from
>>> (if the original cwd is at GIT_WORK_TREE, that would make it a whole-tree
>>> operation).  A natural extension of this idea to limit the operation to
>>> the part of the subtree of the working tree we started from is to refuse
>>> to work in the case where the original cwd is outside GIT_WORK_TREE (the
>>> current implementation of GIT_WORK_TREE may or may not correctly implement
>>> it, though---I never use it myself).
>>
>> I tend to think that as we go up to worktree's root, prefix is shorten
>> and the operation area is widen. When cwd is at worktree's, we operate
>> on full worktree. If it goes up one level higher, the operation area
>> remains full worktree (but not everything under cwd because cwd now
>> can have non-worktree directories).
>
> I have a feeling that you did not understand my /srv/git/git.git (no, it
> is not a bare repository) vs /var/tmp example.
>
> I think it makes the new semantics much less yucky if the special case is
> limited to "the working tree is a subdirectory somewhere under cwd".  But
> does your patch check and notice that /var/tmp is not above the working
> tree and they are completely unrelated?
>

OK I see it now. I think my patch deals with completely unrelated
worktree/cwd just fine (both cwd-to-worktree and worktree-to-cwd
contain a few "../").

There is however a case where "git status" on separate cwd/worktree
might make sense. Suppose a master repo a/.git has two submodules
a/b/.git and a/c/.git. If user stands in a/c and want to do git-status
on all repos (let's skip how it finds out a/.git is the master repo),
it would make sense to display paths, including ones from a/b/.git,
relative to a/c (which is cwd and completely unrelated to a/b/.git).
-- 
Duy