Unbreak real_path on Windows for already absolute paths (with Visual Studio)
diff mbox series

Message ID 6c7d4155-e554-dc9a-053e-f3a8c7cd4075@cs-ware.de
State New
Headers show
Series
  • Unbreak real_path on Windows for already absolute paths (with Visual Studio)
Related show

Commit Message

Sven Strickroth April 8, 2019, 11:16 a.m. UTC
A path such as 'c:/somepath/submodule/../.git/modules/submodule' wasn't
resolved correctly any more, because the *nix variant of
offset_1st_component is used instead of the Win32 specific version.

Regression was introduced in commit
25d90d1cb72ce51407324259516843406142fe89.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 git-compat-util.h | 1 +
 1 file changed, 1 insertion(+)

Comments

Taylor Blau April 9, 2019, 2:36 a.m. UTC | #1
Hi Sven,

On Mon, Apr 08, 2019 at 01:16:33PM +0200, Sven Strickroth wrote:
> A path such as 'c:/somepath/submodule/../.git/modules/submodule' wasn't
> resolved correctly any more, because the *nix variant of
> offset_1st_component is used instead of the Win32 specific version.

I'm not a win32 expert by any sense, but I am do have a meta-question
about your patch...

> Regression was introduced in commit
> 25d90d1cb72ce51407324259516843406142fe89.

I can't seem to find this commit anywhere upstream. Is this SHA-1 pasted
correctly?

>
> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---
>  git-compat-util.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index e0275da7e0..9be177e588 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -210,6 +210,7 @@
>  #include "compat/mingw.h"
>  #include "compat/win32/fscache.h"
>  #elif defined(_MSC_VER)
> +#include "compat/win32/path-utils.h"
>  #include "compat/msvc.h"
>  #include "compat/win32/fscache.h"
>  #else
> --
> 2.21.0.windows.1

Thanks,
Taylor
Torsten Bögershausen April 9, 2019, 5:53 a.m. UTC | #2
On 2019-04-08 13:16, Sven Strickroth wrote:
> A path such as 'c:/somepath/submodule/../.git/modules/submodule' wasn't
> resolved correctly any more, because the *nix variant of
> offset_1st_component is used instead of the Win32 specific version.
>
> Regression was introduced in commit
> 25d90d1cb72ce51407324259516843406142fe89.

Was it ?
25d90d1cb merged this commit:
1cadad6f6 (junio/tb/use-common-win32-pathfuncs-on-cygwin)

And, if I read that correctly,  1cadad6f6 does not change anything for MSVC.
And the problem with the missing/wrong path resolution was there before
1cadad6f6 and after 1cadad6f6.

From that point of view, the patch looks correct, but:

The other question:

In config.mak.uname  we need to add a line
compat/win32/path-utils.o
for the Windows build.
In the git-for windows codebase I see
  COMPAT_OBJS +=compat/win32/path-utils

3 times:
For Cygwin, MINGW and Windows.

In git.git only for Cygwin and MINGW.

(I don't have MSVC, so I can't test)

>
> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---
>  git-compat-util.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index e0275da7e0..9be177e588 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -210,6 +210,7 @@
>  #include "compat/mingw.h"
>  #include "compat/win32/fscache.h"
>  #elif defined(_MSC_VER)
> +#include "compat/win32/path-utils.h"
>  #include "compat/msvc.h"
>  #include "compat/win32/fscache.h"
>  #else
>
Sven Strickroth April 9, 2019, 7:34 a.m. UTC | #3
Am 09.04.2019 um 07:53 schrieb Torsten Bögershausen:
>> Regression was introduced in commit
>> 25d90d1cb72ce51407324259516843406142fe89.
> 
> Was it ?
> 25d90d1cb merged this commit:
> 1cadad6f6 (junio/tb/use-common-win32-pathfuncs-on-cygwin)

Yes, I copied the revision of the merge commit.

> And, if I read that correctly,  1cadad6f6 does not change anything for MSVC.
> And the problem with the missing/wrong path resolution was there before
> 1cadad6f6 and after 1cadad6f6.

That's not correct, it was correct before:
1cadad6f6 removes mingw_offset_1st_component from mingw.c which is
included by msvc.c. Then the in git-compat.h the new file
"compat/win32/path-utils.h" is only included for __CYGWIN__ and
__MINGW32__, here _MSC_VER is missing -> that's the regression.

> In config.mak.uname  we need to add a line
> compat/win32/path-utils.o
> for the Windows build.
> In the git-for windows codebase I see
>   COMPAT_OBJS +=compat/win32/path-utils

I don't use config.mak.uname and never did, so I can't tell you about that.
Torsten Bögershausen April 9, 2019, 4:19 p.m. UTC | #4
On 2019-04-09 09:34, Sven Strickroth wrote:
> Am 09.04.2019 um 07:53 schrieb Torsten Bögershausen:
>>> Regression was introduced in commit
>>> 25d90d1cb72ce51407324259516843406142fe89.
>>
>> Was it ?
>> 25d90d1cb merged this commit:
>> 1cadad6f6 (junio/tb/use-common-win32-pathfuncs-on-cygwin)
>
> Yes, I copied the revision of the merge commit.
>
>> And, if I read that correctly,  1cadad6f6 does not change anything for MSVC.
>> And the problem with the missing/wrong path resolution was there before
>> 1cadad6f6 and after 1cadad6f6.
>
> That's not correct, it was correct before:

No, I wasn't aware that msvc.c include mingw.c - for whatever reason.


> 1cadad6f6 removes mingw_offset_1st_component from mingw.c which is
> included by msvc.c. Then the in git-compat.h the new file
> "compat/win32/path-utils.h" is only included for __CYGWIN__ and
> __MINGW32__, here _MSC_VER is missing -> that's the regression.
>

OK, good.
If possible, I would like to see this kind of information
in the commit message.
Thanks for cleaning up my mess.
Junio C Hamano April 9, 2019, 4:46 p.m. UTC | #5
Torsten Bögershausen <tboegi@web.de> writes:

>> 1cadad6f6 removes mingw_offset_1st_component from mingw.c which is
>> included by msvc.c. Then the in git-compat.h the new file
>> "compat/win32/path-utils.h" is only included for __CYGWIN__ and
>> __MINGW32__, here _MSC_VER is missing -> that's the regression.
>>
>
> OK, good.
> If possible, I would like to see this kind of information
> in the commit message.
> Thanks for cleaning up my mess.

Thanks, both.  Should I wait for an update that fixes the proposed
log message?
Torsten Bögershausen April 10, 2019, 5:32 a.m. UTC | #6
On 2019-04-09 18:46, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>>> 1cadad6f6 removes mingw_offset_1st_component from mingw.c which is
>>> included by msvc.c. Then the in git-compat.h the new file
>>> "compat/win32/path-utils.h" is only included for __CYGWIN__ and
>>> __MINGW32__, here _MSC_VER is missing -> that's the regression.
>>>
>>
>> OK, good.
>> If possible, I would like to see this kind of information
>> in the commit message.
>> Thanks for cleaning up my mess.
>
> Thanks, both.  Should I wait for an update that fixes the proposed
> log message?
>

It seems that I haven't read all messages in my mailbox (or messages crossed).

The V2 patch describes the problem well and looks OK for me.
Junio C Hamano April 12, 2019, 1:15 a.m. UTC | #7
Torsten Bögershausen <tboegi@web.de> writes:

> It seems that I haven't read all messages in my mailbox (or messages crossed).
>
> The V2 patch describes the problem well and looks OK for me.

Yeah, I just re-read the log message with a fresh pair of eyes, and
I think it is clear enough.

Thanks, both.

Patch
diff mbox series

diff --git a/git-compat-util.h b/git-compat-util.h
index e0275da7e0..9be177e588 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -210,6 +210,7 @@ 
 #include "compat/mingw.h"
 #include "compat/win32/fscache.h"
 #elif defined(_MSC_VER)
+#include "compat/win32/path-utils.h"
 #include "compat/msvc.h"
 #include "compat/win32/fscache.h"
 #else