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

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

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.

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.

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