Re: [RFC/PATCHv2 1/5] worktree: provide better prefix to go back to original cwd

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Chris Packham wrote:

> 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".
> 
> As a consequence, command line arguments are supposed to be relative
> to worktree topdir, not current working directory. Not very intuitive.

Thanks.  More detailed history for this patch:

 - v0: http://thread.gmane.org/gmane.comp.version-control.git/157599/focus=157601
 - v1: http://thread.gmane.org/gmane.comp.version-control.git/158287
 - v2: http://thread.gmane.org/gmane.comp.version-control.git/158369

Any thoughts about the previous questions?

> --- a/cache.h
> +++ b/cache.h
> @@ -1117,6 +1117,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 */

e.g. I still find these comments hard to understand.

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

This still assumes Unix-style path separators.  Is that okay?  (The
answer could be yes; it just seems useful to document the assumptions...)

> +/*
> + * 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]) {

Style: use cuddled braces.

	} else if (worktree[len] && cwd[len]) {
		/*
		 * The result should be /foo/, not /foo/baa, when
		 * worktree is /foo/baa1 and cwd is /foo/baa2.
		 */

> +		while (len && worktree[len] != '/')
> +			len--;
> +		len++;
> +	}
> +	else {

Likewise.

> +		if (worktree[len]) {
> +			if (worktree[len] != '/') {
> +				while (len && worktree[len] != '/')
> +					len--;
> +			}
> +		}
> +		else {

Likewise.

> +			if (cwd[len] != '/') {
> +				while (len && cwd[len] != '/')
> +					len--;
> +			}

This is repetitive.  Why is the if necessary?

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

Unexplained.

[...]
> --- /dev/null
> +++ b/t/t1510-worktree-prefix.sh
> @@ -0,0 +1,52 @@
[...]
> +test_expect_success 'at root' '
> +	(
> +	cd foo &&
> +	git rev-parse --cwd-to-worktree --worktree-to-cwd >result &&
> +	: >expected &&
> +	test_cmp expected result
> +	)
> +'

It is clearer where a subshell begins and ends if it is indented.  The
usual style in git shell scripts is to omit the ":" in

	>empty

commands.  So maybe:

	(
		cd foo &&
		git rev-parse --cwd-to-worktree --worktree-to-cwd >result
	) &&
	>expected &&
	test_cmp expected foo/result

Sorry, a bit grumpy today.  Still, hope that helps,
Jonathan