threads / patch / 25367

patchworktree: provide better prefix to go back to original cwd

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

## tl;dr

6 messages between Oct 6, 2010 and Oct 7, 2010. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Nguyễn Thái Ngọc Duy· Oct 6, 2010, 13:59 UTC · lore

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
Show changes to 4 files +168 −4

builtin/rev-parse.c, cache.h, setup.c, 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
Chris Packham· Oct 6, 2010, 15:47 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On 06/10/10 06:59, Nguyễn Thái Ngọc Duy wrote:
Show 18 quoted lines
> 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.

Jonathan Nieder· Oct 6, 2010, 18:07 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy wrote:
Show 18 quoted lines
> 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.)

Show 8 quoted lines
> --- 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.

Show 11 quoted lines
> --- 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(). [...]

Show 5 quoted lines
>  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.

Junio C Hamano· Oct 6, 2010, 18:32 UTC · re: Jonathan Nieder · lore

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

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?

Show 15 quoted lines
>> --- 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.
Nguyen Thai Ngoc Duy· Oct 7, 2010, 03:14 UTC · re: Junio C Hamano · lore

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

On Thu, Oct 7, 2010 at 1:32 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> 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
Nguyen Thai Ngoc Duy· Oct 7, 2010, 03:08 UTC · re: Jonathan Nieder · lore

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

2010/10/7 Jonathan Nieder <jrnieder@gmail.com>:
Show 34 quoted lines
> 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.

Show 22 quoted lines
>> --- 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).
Show 12 quoted lines
> [...]
>
>>  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

← back to recent threads