Re: [PATCH 24/25] worktree move: accept destination as directory

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 11, 2016, 17:32 UTC
Message-ID: <CAPig+cTB8tdPo=wd5UdB84owKJ6c5hj6H9d4_YGDRecBdUE0vA@mail.gmail.com>
In-Reply-To: <CACsJy8CCAan9ALxULPFeGSU7wsfwbrywRWFr4Hsjx3=PGwosLA@mail.gmail.com>

On Wed, May 11, 2016 at 9:34 AM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 17 quoted lines

> On Wed, May 11, 2016 at 11:43 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Apr 13, 2016 at 9:15 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>>> +       if (is_directory(dst.buf)) {
>>> +               const char *sep = strrchr(wt->path, '/');
>>
>> Does this need to take Windows into account?
>
> wt->path comes from $GIT_DIR/worktrees/xxx/gitdir, which normally uses
> forward slashes, so we should be safe. We already rely on forward
> slashes in get_linked_worktree()
>
>> Perhaps git_find_last_dir_sep()?
>
> But this is probably a good thing to do anyway, to be more robust in
> future. But it could confuse the reader later on why it's necessary
> when backward slashes can't exist in wt->path. I don't know. Maybe
> just have a comment that backward slashes can't never appear here?

As this path is read from a file git itself creates, and if we know that it will always contain forward slashes, then I agree that it could be potentially confusing to later readers to see git_find_last_dir_sep(). So, keeping it as-is seems correct.

Not sure if it needs a comment. I reviewed this rather quickly since (I think) you plan on re-rolling it and I'm far behind on my reviews. Consequently, I didn't check the existing code, and reviewed only within the context of the patch itself. If the end result is that it's clear from reading the code that it will always contain forward slashes, then a comment would be redundant. You could perhaps mention in the commit message that the slash will always be forward, which should satisfy future reviewers and readers of the code once its in the tree.

Show 7 quoted lines

> There is also a potential problem with find_worktree_by_path(). I was
> counting on real_path() to normalize paths and could simply do
> strcmp_icase (or its new name, fspathcmp). But real_path() does not
> seem to convert unify slashes. I will need to have a closer look at
> this. Hopefully prefix_filename() already makes sure everything uses
> forward slashes. Or maybe we could improve fspathcmp to see '/' and
> '\' the same thing on Windows.

If we look at fspathcmp() as a function which performs whatever magic is needed to make comparisons work on a platform/filesystem, then it might indeed be reasonable to enhance it to recognize '/' and '\' as equivalent (with possible caveats for Windows corner cases).