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

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Oct 7, 2010, 03:08 UTC
Message-ID: <AANLkTin8zFb9mP7A4=hWV-9SkFqqYLbMQxUw=diW6Vbf@mail.gmail.com>
In-Reply-To: <20101006180727.GA2118@burratino>

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