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

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

[Nguyễn Thái Ngọc Duy]

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

As a consequence, command line arguments are supposed to be relative
to worktree topdir, not current working directory. Not very intuitive.
Moreover, output from such situation is (again) relative to worktree
topdir. Users are expected to understand that.

This patch allows builtin commands access to original cwd even if it's
outside worktree, via cwd_to_worktree and worktree_to_cwd fields. As
the name implies, if you stay at original cwd, "cd $(cwd_to_worktree)"
would take you to worktree topdir and vice versa.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 startup_info->cwd_to_worktree would be as same as opt.submodule_prefix
 in your 2/3 patch.

 builtin/rev-parse.c        |   10 ++++
 cache.h                    |    2 +
 setup.c                    |  108 ++++++++++++++++++++++++++++++++++++++++++--
 t/t1510-worktree-prefix.sh |   52 +++++++++++++++++++++
 4 files changed, 168 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 2ef2fa3..8256e1c 100644
--- a/cache.h
+++ b/cache.h
@@ -1110,6 +1110,8 @@ const char *split_cmdline_strerror(int cmdline_errno);
 /* git.c */
 struct startup_info {
 	int have_repository;
+	char *cwd_to_worktree; /* chdir("this"); from cwd would return to worktree */
+	char *worktree_to_cwd; /* chdir("this"); from worktree would return to cwd */
 };
 extern struct startup_info *startup_info;
 
diff --git a/setup.c b/setup.c
index a3b76de..413703b 100644
--- a/setup.c
+++ b/setup.c
@@ -313,10 +313,109 @@ 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
+ */
+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 (*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;
+		*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 && worktree[len] != '/')
+			len--;
+		len++;
+	}
+	else {
+		if (worktree[len]) {
+			if (worktree[len] != '/') {
+				while (len && worktree[len] != '/')
+					len--;
+			}
+		}
+		else {
+			if (cwd[len] != '/') {
+				while (len && 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))
@@ -337,9 +436,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.0.2.445.gcbdb3

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

[Chris Packham]

On 06/10/10 06:59, Nguyễn Thái Ngọc Duy wrote:
> 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".
> 
> As a consequence, command line arguments are supposed to be relative
> to worktree topdir, not current working directory. Not very intuitive.
> Moreover, output from such situation is (again) relative to worktree
> topdir. Users are expected to understand that.
> 
> This patch allows builtin commands access to original cwd even if it's
> outside worktree, via cwd_to_worktree and worktree_to_cwd fields. As
> the name implies, if you stay at original cwd, "cd $(cwd_to_worktree)"
> would take you to worktree topdir and vice versa.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  startup_info->cwd_to_worktree would be as same as opt.submodule_prefix
>  in your 2/3 patch.

Thanks. I must admit I was struggling a little with implementing this
part. I'll be sending out a re-roll of my series shortly and I'll
include your patch if Junio doesn't pick it up on its own. I was also
planning on re-basing my patches on top of next or pu so more people can
actually compile it.

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

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

> This patch allows builtin commands access to original cwd even if it's
> outside worktree, via cwd_to_worktree and worktree_to_cwd fields.
> --- 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;
> +			}

Nice.

I wonder if this should use something like

	else
		puts(".");

or

	else
		putchar('\n');

.  What would be most convenient for scripted callers?

What do these commands do when run from a bare repository?  Is the
worktree the .git dir in that case, do they fail, or does something
else happen?

Are there any examples to illustrate whether teaching --show-prefix to
do what your --worktree-to-cwd does would be a good or bad idea?
(Just curious.)

> --- a/cache.h
> +++ b/cache.h
> @@ -1110,6 +1110,8 @@ const char *split_cmdline_strerror(int cmdline_errno);
>  /* git.c */
>  struct startup_info {
>  	int have_repository;
> +	char *cwd_to_worktree; /* chdir("this"); from cwd would return to worktree */
> +	char *worktree_to_cwd; /* chdir("this"); from worktree would return to cwd */

Comment nit: would

				/* path from original cwd to worktree */
				/* path from worktree to original cwd */

be clearer?  But presumably any confused people should be able to find
your log message.

> --- a/setup.c
> +++ b/setup.c
> @@ -313,10 +313,109 @@ 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
> + */
> +static char *make_path_to_path(const char *path1, const char *path2)

Nice.  Do we need to worry about:

 - alternate directory separators? (hey\hello\world)
 - DOS drive prefix? (c:\foo\bar, d:\hey\hello\world)
 - relative paths with DOS drive? (c:\foo\bar, d:hello)
 - doubled-up directory separators? (hey//hello/world, //foo/bar)
 - non-canonical paths? (hey/./hello/../hello/world)

I'm guessing some of the answers are "no", depending on where these
paths come from.  Compare make_relative_path().
[...]

>  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];

Why?

It might make sense to error out a little before PATH_MAX (though
later than 1024), to account for subdirs (e.g., objects/).  Not sure.

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Are there any examples to illustrate whether teaching --show-prefix to
> do what your --worktree-to-cwd does would be a good or bad idea?
> (Just curious.)

Do these additions interact well with the notion of "we are in the working
tree" vs "we are outside the working tree"?  Even when we happen to know
via GIT_WORK_TREE that the root of the working tree is /var/tmp/junk, we
should correctly diagnse that we are outside the working tree when we are
in /var/tmp/, and require_work_tree should say "no you are not allowed to
do this", no?

>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1110,6 +1110,8 @@ const char *split_cmdline_strerror(int cmdline_errno);
>>  /* git.c */
>>  struct startup_info {
>>  	int have_repository;
>> +	char *cwd_to_worktree; /* chdir("this"); from cwd would return to worktree */
>> +	char *worktree_to_cwd; /* chdir("this"); from worktree would return to cwd */
>
> Comment nit: would
>
> 				/* path from original cwd to worktree */
> 				/* path from worktree to original cwd */
>
> be clearer?

Much better.

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

[Nguyen Thai Ngoc Duy]

2010/10/7 Jonathan Nieder <jrnieder@gmail.com>:
> Nguyễn Thái Ngọc Duy wrote:
>
>> This patch allows builtin commands access to original cwd even if it's
>> outside worktree, via cwd_to_worktree and worktree_to_cwd fields.
>> --- 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;
>> +                     }
>
> Nice.
>
> I wonder if this should use something like
>
>        else
>                puts(".");
>
> or
>
>        else
>                putchar('\n');
>
> .  What would be most convenient for scripted callers?

I followed the convention in rev-parse.c, specifically --show-prefix
code. Don't know if it's good or bad for scripts though.

> What do these commands do when run from a bare repository?  Is the
> worktree the .git dir in that case, do they fail, or does something
> else happen?

These two new fields are only set when both GIT_DIR and GIT_WORK_TREE
are set, which means non-bare repository. I'll need to handle other
setup cases as well, but for subproject-aware grep, this should be
enough.

> Are there any examples to illustrate whether teaching --show-prefix to
> do what your --worktree-to-cwd does would be a good or bad idea?
> (Just curious.)

I think it's a bad idea. --show-prefix traditionally never returns
"../(something)" and scripts (even builtin commands) depend on that.
It's better to slowly migrate over the new prefix(es) when it makes
sense.

>> --- a/setup.c
>> +++ b/setup.c
>> @@ -313,10 +313,109 @@ 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
>> + */
>> +static char *make_path_to_path(const char *path1, const char *path2)
>
> Nice.  Do we need to worry about:
>
>  - alternate directory separators? (hey\hello\world)
>  - DOS drive prefix? (c:\foo\bar, d:\hey\hello\world)
>  - relative paths with DOS drive? (c:\foo\bar, d:hello)
>  - doubled-up directory separators? (hey//hello/world, //foo/bar)
>  - non-canonical paths? (hey/./hello/../hello/world)
>
> I'm guessing some of the answers are "no", depending on where these
> paths come from.  Compare make_relative_path().

The two last ones are OK. Paths passed to this function come from
either getcwd or make_absolute_path(). I'm sure they are
canonicalized. But I'll add a comment to note that.

I'll look into backslashes and DOS drives (hmm and UNC path too).

> [...]
>
>>  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];
>
> Why?
>
> It might make sense to error out a little before PATH_MAX (though
> later than 1024), to account for subdirs (e.g., objects/).  Not sure.

I'm not really POSIX-fluent to tell. But I'm sure in this function
there won't be any subdirs.
-- 
Duy

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

[Nguyen Thai Ngoc Duy]

On Thu, Oct 7, 2010 at 1:32 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Are there any examples to illustrate whether teaching --show-prefix to
>> do what your --worktree-to-cwd does would be a good or bad idea?
>> (Just curious.)
>
> Do these additions interact well with the notion of "we are in the working
> tree" vs "we are outside the working tree"?  Even when we happen to know
> via GIT_WORK_TREE that the root of the working tree is /var/tmp/junk, we
> should correctly diagnse that we are outside the working tree when we are
> in /var/tmp/, and require_work_tree should say "no you are not allowed to
> do this", no?

It should work as it does now. I don't change prefix calculation and
chdir() behavior. So when you're at /var/tmp, require_work_tree should
say no.
-- 
Duy