Re: [RFC/PATCHv2 1/5] worktree: provide better prefix to go back to original cwd
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Oct 16, 2010, 18:42 UTC
- Message-ID
- <20101016184259.GB30457@burratino>
- In-Reply-To
- <1287185204-843-2-git-send-email-judge.packham@gmail.com>
Hi,
Chris Packham wrote:
Show 8 quoted lines
> 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?
Show 8 quoted lines
> --- 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.
Show 21 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)
> +{
> + 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...)
Show 21 quoted lines
> +/*
> + * 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.
*/Show 5 quoted lines
> + while (len && worktree[len] != '/')
> + len--;
> + len++;
> + }
> + else {Likewise.
Show 7 quoted lines
> + 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?
[...]
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];Unexplained.
[...]
> --- /dev/null > +++ b/t/t1510-worktree-prefix.sh > @@ -0,0 +1,52 @@
[...]
Show 8 quoted lines
> +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