Re: [PATCH 2/7] worktree.c: add update_worktree_location()

From: Duy Nguyen <pclouds@gmail.com>
Date: Feb 2, 2018, 09:35 UTC
Message-ID: <CACsJy8ALCro_4n0u2TBp8Lon_2kX49w9UcAnD8awHmsoriQ7-Q@mail.gmail.com>
In-Reply-To: <CAPig+cQzdFhUzwDykO8su=ne0qZr6=51vTxXbMYYnceuH_N=iw@mail.gmail.com>

On Fri, Feb 2, 2018 at 3:23 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 22 quoted lines

> On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> diff --git a/worktree.c b/worktree.c
>> @@ -326,6 +326,24 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)
>> +void update_worktree_location(struct worktree *wt, const char *path_)
>> +{
>> +       struct strbuf path = STRBUF_INIT;
>> +
>> +       if (is_main_worktree(wt))
>> +               die("BUG: can't relocate main worktree");
>> +
>> +       strbuf_add_absolute_path(&path, path_);
>> +       if (fspathcmp(wt->path, path.buf)) {
>> +               write_file(git_common_path("worktrees/%s/gitdir",
>> +                                          wt->id),
>> +                          "%s/.git", real_path(path.buf));
>
> For the path stored in 'worktrees/<id>/gitdir' (and in wt->path), this
> and other worktree-related code sometimes treats it only as "absolute
> path" and sometimes as "real path". As a reviewer, I'm having trouble
> understanding the logic of why, how, and when this distinction is
> made. Can you explain a bit to help clarify when "absolute path" is
> good enough and when "real path" is needed?

Of the top of my head, I think it's "anything but a relative path". real_path() is normally used to normalize paths before I compare them. Writing a normalized path down is nice to avoid "../" but I don't think it's strictly necessary. Well, it's also prettier when you have to print the path out. Printing "/foo/bar/abc/../../xyz" is not nice.

Hmm.. looking back in add_worktree(), we also write "gitdir" with real_path(). I think the problem here is that I should have real_path()'d "path.buf" _before_ doing fspathcmp(). Maybe that's where the confusion's from.

-- 
Duy