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

From: Johannes Sixt <j6t@kdbg.org>
Date: May 11, 2016, 18:32 UTC
Message-ID: <57337ACF.3020402@kdbg.org>
In-Reply-To: <CAPig+cTB8tdPo=wd5UdB84owKJ6c5hj6H9d4_YGDRecBdUE0vA@mail.gmail.com>

Am 11.05.2016 um 19:32 schrieb Eric Sunshine:

Show 23 quoted lines

> On Wed, May 11, 2016 at 9:34 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> 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.

Please allow me to disagree. There should not be any assumption that a path uses forward slashes as directory separator, except when the path is

- a pathspec
- a ref
- a path found in the object database including the index

In particular, everything concerning paths in the file system (including paths pointing to Git's own files) must not assume forward slashes.

We do convert backslashes to forward slashes in a number of places, but this is only for cosmetic reasons, not to maintain an invariant.

> 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).

That sounds reasonable.

-- Hannes