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

Am 11.05.2016 um 19:32 schrieb Eric Sunshine:

On Wed, May 11, 2016 at 9:34 AM, Duy Nguyen [off-list ref] wrote:

quoted

On Wed, May 11, 2016 at 11:43 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Wed, Apr 13, 2016 at 9:15 AM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:

quoted

+       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()

quoted

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