All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Torsten Bögershausen" <tboegi@web.de>
To: Johannes Sixt <j6t@kdbg.org>
Cc: "Jiang Xin" <worldhello.net@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Torsten Bögershausen" <tboegi@web.de>,
	"Git List" <git@vger.kernel.org>, Tvangeste <i.4m.l33t@yandex.ru>,
	"Karsten Blees" <karsten.blees@gmail.com>
Subject: Re: [PATCH 1/2] relative_path should honor dos_drive_prefix
Date: Wed, 18 Sep 2013 16:29:05 +0200	[thread overview]
Message-ID: <5239B8B1.5050605@web.de> (raw)
In-Reply-To: <5238AE60.1010700@kdbg.org>

On 2013-09-17 21.32, Johannes Sixt wrote:
> Am 17.09.2013 10:24, schrieb Jiang Xin:
>> I have checked the behavior of UNC path on Windows (msysGit):
>>
>> * I can cd to a UNC path:
>>
>>     cd //server1/share1/path
>>
>> * can cd to other share:
>>
>>     cd ../../share2/path
>>
>> * and can cd to other server's share:
>>
>>     cd ../../../server2/share/path
>>
>> That means relative_path(path1, path2) support UNC paths out of the box.
>> We only need to check both path1 and path2 are UNC paths, or both not.
> 
> Your tests are flawed. You issued the commands in bash, which (or rather
> MSYS) does everything for you that you need to make it work. But in
> reality it does not, because the system cannot apply .. to //server/share:
> 
> $ git ls-remote //srv/public/../repos/his/setups.git
> fatal: '//srv/public/../repos/his/setups.git' does not appear to be a
> git repository
> fatal: Could not read from remote repository.
> 
> Please make sure you have the correct access rights
> and the repository exists.
> 
> even though the repository (and //srv/public, let me assure) exists:
> 
> $ git ls-remote //srv/repos/his/setups.git
> bea489b0611a72c41f133343fdccbd3e2b9f80b5        HEAD
> ...
> 
> The situation does not change with your latest round (v3).
> 
> Please let me suggest not to scratch where there is no itch. ;) Your
> round v2 was good enough.
> 
> If you really want to check UNC paths, then you must compare two path
> components after the the double-slash, not just one.
> 
> Furthermore, you should audit all code that references
> is_absolute_path(), relative_path(), normalize_path_copy(), and possibly
> a few others whether the functions or call sites need improvement.
> That's worth a separate patch.
> 
> -- Hannes

I tend to agree here.
The V2 patch fixed a regression.
This should be one commit on its own:
Documentation/SubmittingPatches:
(1) Make separate commits for logically separate changes.

Fixing a bug is a good thing, thanks for working on this,

The support for UNC paths is a new feature, and this deserves a seperate commit.
/Torsten

  reply	other threads:[~2013-09-18 14:29 UTC|newest]

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-09-10 13:14 Regression in e02ca72: git svn rebase is broken on Windows Tvangeste
2013-09-10 16:13 ` Johannes Schindelin
2013-09-10 17:43   ` Tvangeste
2013-09-10 18:02     ` Johannes Schindelin
2013-09-10 20:53       ` Tvangeste
2013-09-10 16:52 ` Johannes Sixt
2013-09-10 17:44   ` Tvangeste
2013-09-10 17:06 ` Junio C Hamano
2013-09-10 22:17   ` Karsten Blees
2013-09-11  4:41     ` Jiang Xin
2013-09-11  3:19   ` Jiang Xin
2013-09-11  5:43     ` Johannes Sixt
2013-09-12  9:12   ` [PATCH 1/2] relative_path should honor dos_drive_prefix Jiang Xin
2013-09-12  9:32     ` Tvangeste
2013-09-12 14:13     ` Torsten Bögershausen
2013-09-12 15:48       ` Junio C Hamano
2013-09-12 17:22       ` Johannes Sixt
2013-09-12 19:11         ` Junio C Hamano
2013-09-13  4:55           ` Jiang Xin
2013-09-13  5:53             ` Junio C Hamano
2013-09-17  8:24               ` Jiang Xin
2013-09-17  8:30                 ` [PATCH v3 1/3] test: use unambigous leading path (/foo) for mingw Jiang Xin
2013-09-17  8:30                 ` [PATCH v3 2/3] relative_path should honor DOS and UNC paths Jiang Xin
2013-09-17 16:12                   ` Junio C Hamano
2013-09-18  9:02                     ` Jiang Xin
2013-09-18 16:00                       ` Junio C Hamano
     [not found]                     ` <5239BA98.9000205@gmail.com>
2013-09-18 15:09                       ` Torsten Bögershausen
2013-09-17  8:30                 ` [PATCH v3 3/3] Use simpler relative_path when set_git_dir Jiang Xin
2013-09-17 19:32                 ` [PATCH 1/2] relative_path should honor dos_drive_prefix Johannes Sixt
2013-09-18 14:29                   ` Torsten Bögershausen [this message]
2013-09-20  1:26                   ` Jiang Xin
2013-09-20  2:38                     ` [PATCH v4 0/3] relative path regression fix Jiang Xin
2013-09-20  2:38                     ` [PATCH v4 1/3] test: use unambigous leading path (/foo) for mingw Jiang Xin
2013-10-10 20:32                       ` Sebastian Schuberth
2013-10-14  1:33                         ` Jiang Xin
2013-10-14  2:29                         ` [PATCH v5 0/3] relative path regression fix Jiang Xin
2013-10-14  2:29                         ` [PATCH v5 1/3] test: use unambigous leading path (/foo) for MSYS Jiang Xin
2013-10-14  6:50                           ` Sebastian Schuberth
2013-10-14 19:40                           ` Eric Sunshine
2013-10-14  2:29                         ` [PATCH v5 2/3] relative_path should honor dos-drive-prefix Jiang Xin
2013-10-14  2:29                         ` [PATCH v5 3/3] Use simpler relative_path when set_git_dir Jiang Xin
2013-09-20  2:38                     ` [PATCH v4 2/3] relative_path should honor dos-driver-prefix Jiang Xin
2013-10-10 20:34                       ` Sebastian Schuberth
2013-09-20  2:38                     ` [PATCH v4 3/3] Use simpler relative_path when set_git_dir Jiang Xin
2013-09-13 13:59             ` [PATCH 1/2] relative_path should honor dos_drive_prefix Torsten Bögershausen
2013-09-12 15:45     ` Karsten Blees
2013-09-12  9:12   ` [PATCH 2/2] Use simpler relative_path when set_git_dir Jiang Xin
2013-09-12 17:36     ` Johannes Sixt
2013-09-13  5:08   ` [PATCH v2 0/3] fixes for relative_path Jiang Xin
2013-09-13  5:08   ` [PATCH v2 1/3] test: use unambigous leading path (/foo) for mingw Jiang Xin
2013-09-13 19:51     ` Junio C Hamano
2013-09-14  6:52       ` Torsten Bögershausen
2013-09-17 16:19         ` Junio C Hamano
2013-09-13  5:08   ` [PATCH v2 2/3] relative_path should honor dos_drive_prefix Jiang Xin
2013-09-14  6:11     ` Torsten Bögershausen
2013-09-13  5:08   ` [PATCH v2 3/3] Use simpler relative_path when set_git_dir Jiang Xin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5239B8B1.5050605@web.de \
    --to=tboegi@web.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=i.4m.l33t@yandex.ru \
    --cc=j6t@kdbg.org \
    --cc=karsten.blees@gmail.com \
    --cc=worldhello.net@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.