{"thread":{"id":"46007","subject":"[PATCH 0/2] Fix warnings on access of a remote with Windows paths","startedAt":"2017-05-20T06:29:21Z","lastAt":"2017-05-25T23:16:18Z","messageCount":19,"participants":["Johannes Sixt","Johannes Schindelin","stefan.naewe@atlas-elektronik.com","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"320297","messageId":"cover.1495261020.git.j6t@kdbg.org","threadId":"46007","inReplyTo":null,"subject":"[PATCH 0/2] Fix warnings on access of a remote with Windows paths","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-05-20T06:28:37Z","receivedAt":"2017-05-20T06:29:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"This small series fixes these warnings on Windows:\n\nC:\\Temp\\gittest>git fetch C:\\Temp\\gittest\nwarning: unable to access '.git/remotes/C:\\Temp\\gittest': Invalid argument\nwarning: unable to access '.git/branches/C:\\Temp\\gittest': Invalid argument\nFrom C:\\Temp\\gittest\n * branch            HEAD       -> FETCH_HEAD\n\nThe fix is in the second patch; the first patch is a\npreparation that allows to write the fix in my preferred style.\n\nJohannes Sixt (2):\n  mingw.h: permit arguments with side effects for is_dir_sep\n  Windows: do not treat a path with backslashes as a remote's nick name\n\n compat/mingw.h | 6 +++++-\n remote.c       | 5 ++++-\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\n-- \n2.13.0.55.g17b7d13330\n\n"},{"id":"320298","messageId":"69bafb7c9e08ba837a9b92573bef165a62c5bf92.1495261020.git.j6t@kdbg.org","threadId":"46007","inReplyTo":"cover.1495261020.git.j6t@kdbg.org","subject":"[PATCH 1/2] mingw.h: permit arguments with side effects for is_dir_sep","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-05-20T06:28:38Z","receivedAt":"2017-05-20T06:29:28Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"The implementation of is_dir_sep in git-compat-util.h uses an inline\nfunction. Use one also for the implementation in compat/mingw.h to support\nnon-trivial argument expressions.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n compat/mingw.h | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex cdc112526a..5e8447019b 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -398,7 +398,11 @@ HANDLE winansi_get_osfhandle(int fd);\n \t(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)\n int mingw_skip_dos_drive_prefix(char **path);\n #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix\n-#define is_dir_sep(c) ((c) == '/' || (c) == '\\\\')\n+static inline int mingw_is_dir_sep(int c)\n+{\n+\treturn c == '/' || c == '\\\\';\n+}\n+#define is_dir_sep mingw_is_dir_sep\n static inline char *mingw_find_last_dir_sep(const char *path)\n {\n \tchar *ret = NULL;\n-- \n2.13.0.55.g17b7d13330\n\n"},{"id":"320299","messageId":"6347731f26396b1256070e02376bf3623bfb7102.1495261020.git.j6t@kdbg.org","threadId":"46007","inReplyTo":"cover.1495261020.git.j6t@kdbg.org","subject":"[PATCH 2/2] Windows: do not treat a path with backslashes as a remote's nick name","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-05-20T06:28:39Z","receivedAt":"2017-05-20T06:29:34Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Fetching from a remote using a native Windows path produces these warnings:\n\nC:\\Temp\\gittest>git fetch C:\\Temp\\gittest\nwarning: unable to access '.git/remotes/C:\\Temp\\gittest': Invalid argument\nwarning: unable to access '.git/branches/C:\\Temp\\gittest': Invalid argument\nFrom C:\\Temp\\gittest\n * branch            HEAD       -> FETCH_HEAD\n\nThe functions that read the branches and remotes files are protected by\na valid_remote_nick() check. Its implementation does not take Windows\npaths into account, so that the caller simply concatenates the paths,\nleading to the error seen above.\n\nUse is_dir_sep() to check for both slashes and backslashes on Windows.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n remote.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex 9584af366d..9501357c06 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -649,7 +649,10 @@ static int valid_remote_nick(const char *name)\n {\n \tif (!name[0] || is_dot_or_dotdot(name))\n \t\treturn 0;\n-\treturn !strchr(name, '/'); /* no slash */\n+\twhile (*name)\n+\t\tif (is_dir_sep(*name++))\t/* no slash */\n+\t\t\treturn 0;\n+\treturn 1;\n }\n \n const char *remote_for_branch(struct branch *branch, int *explicit)\n-- \n2.13.0.55.g17b7d13330\n\n"},{"id":"320405","messageId":"alpine.DEB.2.21.1.1705221349200.3610@virtualbox","threadId":"46007","inReplyTo":"69bafb7c9e08ba837a9b92573bef165a62c5bf92.1495261020.git.j6t@kdbg.org","subject":"Re: [PATCH 1/2] mingw.h: permit arguments with side effects for is_dir_sep","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-22T11:52:58Z","receivedAt":"2017-05-22T11:53:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\n\nOn Sat, 20 May 2017, Johannes Sixt wrote:\n\n> The implementation of is_dir_sep in git-compat-util.h uses an inline\n> function. Use one also for the implementation in compat/mingw.h to support\n> non-trivial argument expressions.\n> \n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n\nI was a bit confused by the oneline. Maybe something like\n\n\tmingw.h: avoid side effects with is_dir_sep()'s argument\n\n\tTaking git-compat-util.h's cue (which uses an inline function\n\tto back is_dir_sep()), let's use an inline function to back\n\talso the Windows version of is_dir_sep(); This avoids problems\n\twhen calling the function with arguments that do more than just\n\tprovide a single character, e.g. incrementing a pointer. Example:\n\n\t\tis_dir_sep(*(p++))\n\n> diff --git a/compat/mingw.h b/compat/mingw.h\n> index cdc112526a..5e8447019b 100644\n> --- a/compat/mingw.h\n> +++ b/compat/mingw.h\n> @@ -398,7 +398,11 @@ HANDLE winansi_get_osfhandle(int fd);\n>  \t(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)\n>  int mingw_skip_dos_drive_prefix(char **path);\n>  #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix\n> -#define is_dir_sep(c) ((c) == '/' || (c) == '\\\\')\n> +static inline int mingw_is_dir_sep(int c)\n> +{\n> +\treturn c == '/' || c == '\\\\';\n> +}\n> +#define is_dir_sep mingw_is_dir_sep\n>  static inline char *mingw_find_last_dir_sep(const char *path)\n>  {\n>  \tchar *ret = NULL;\n\nThe patch is very good, of course.\n\nACK,\nDscho\n"},{"id":"320406","messageId":"alpine.DEB.2.21.1.1705221353570.3610@virtualbox","threadId":"46007","inReplyTo":"6347731f26396b1256070e02376bf3623bfb7102.1495261020.git.j6t@kdbg.org","subject":"Re: [PATCH 2/2] Windows: do not treat a path with backslashes as a remote's nick name","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-22T11:56:55Z","receivedAt":"2017-05-22T11:57:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Sat, 20 May 2017, Johannes Sixt wrote:\n\n> Fetching from a remote using a native Windows path produces these warnings:\n> \n> C:\\Temp\\gittest>git fetch C:\\Temp\\gittest\n> warning: unable to access '.git/remotes/C:\\Temp\\gittest': Invalid argument\n> warning: unable to access '.git/branches/C:\\Temp\\gittest': Invalid argument\n> From C:\\Temp\\gittest\n>  * branch            HEAD       -> FETCH_HEAD\n> \n> The functions that read the branches and remotes files are protected by\n> a valid_remote_nick() check. Its implementation does not take Windows\n> paths into account, so that the caller simply concatenates the paths,\n> leading to the error seen above.\n> \n> Use is_dir_sep() to check for both slashes and backslashes on Windows.\n> \n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n\nI like this explanation.\n\n> diff --git a/remote.c b/remote.c\n> index 9584af366d..9501357c06 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -649,7 +649,10 @@ static int valid_remote_nick(const char *name)\n>  {\n>  \tif (!name[0] || is_dot_or_dotdot(name))\n>  \t\treturn 0;\n> -\treturn !strchr(name, '/'); /* no slash */\n> +\twhile (*name)\n> +\t\tif (is_dir_sep(*name++))\t/* no slash */\n\nI understand that you simply kept the comment, yet I have to admit that I\nfind it confusing in the post image. Something like\n\n\t/* remote nicknames cannot contain slashes */\n\n(and putting it *above* the `if` line instead of appending it separated by\na <tab>) would have made it easier on me.\n\n> +\t\t\treturn 0;\n> +\treturn 1;\n>  }\n>  \n\nWith my suggested change or without it, ACK.\n\nCiao,\nDscho\n"},{"id":"320407","messageId":"alpine.DEB.2.21.1.1705221357360.3610@virtualbox","threadId":"46007","inReplyTo":"cover.1495261020.git.j6t@kdbg.org","subject":"Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-22T11:59:29Z","receivedAt":"2017-05-22T12:00:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\n\nOn Sat, 20 May 2017, Johannes Sixt wrote:\n\n> This small series fixes these warnings on Windows:\n> \n> C:\\Temp\\gittest>git fetch C:\\Temp\\gittest\n> warning: unable to access '.git/remotes/C:\\Temp\\gittest': Invalid argument\n> warning: unable to access '.git/branches/C:\\Temp\\gittest': Invalid argument\n> From C:\\Temp\\gittest\n>  * branch            HEAD       -> FETCH_HEAD\n> \n> The fix is in the second patch; the first patch is a\n> preparation that allows to write the fix in my preferred style.\n\nThank you!\n\nMaybe you want to accompany these patches with a simple test case that\nuses e.g. ls-remote on $(pwd | tr / \\\\\\\\)?\n\nCiao,\nDscho\n"},{"id":"320408","messageId":"122838f4-5f6b-6921-9242-2724bb6d8c95@atlas-elektronik.com","threadId":"46007","inReplyTo":"cover.1495261020.git.j6t@kdbg.org","subject":"Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths","fromName":"","fromEmail":"stefan.naewe@atlas-elektronik.com","sentAt":"2017-05-22T12:38:35Z","receivedAt":"2017-05-22T12:49:19Z","isPatch":true,"sender":{"key":"stefan.naewe@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4468?v=4"},"body":"Am 20.05.2017 um 08:28 schrieb Johannes Sixt:\n> This small series fixes these warnings on Windows:\n> \n> C:\\Temp\\gittest>git fetch C:\\Temp\\gittest\n> warning: unable to access '.git/remotes/C:\\Temp\\gittest': Invalid argument\n> warning: unable to access '.git/branches/C:\\Temp\\gittest': Invalid argument\n> From C:\\Temp\\gittest\n>  * branch            HEAD       -> FETCH_HEAD\n\nWhat is the exact precondition to get such a warning ?\n\nJust wondering, because I'm not able to reproduce that warning\n(with Git4win version 2.13.0.windows.1).\n\nStefan\n-- \n----------------------------------------------------------------\n/dev/random says: Spock, you are such a putz!\npython -c \"print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')\" \nGPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9  9666 829B 49C5 9221 27AF"},{"id":"320418","messageId":"alpine.DEB.2.21.1.1705221600100.3610@virtualbox","threadId":"46007","inReplyTo":"122838f4-5f6b-6921-9242-2724bb6d8c95@atlas-elektronik.com","subject":"Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-22T14:01:56Z","receivedAt":"2017-05-22T14:02:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Mon, 22 May 2017, stefan.naewe@atlas-elektronik.com wrote:\n\n> Am 20.05.2017 um 08:28 schrieb Johannes Sixt:\n> > This small series fixes these warnings on Windows:\n> > \n> > C:\\Temp\\gittest>git fetch C:\\Temp\\gittest\n> > warning: unable to access '.git/remotes/C:\\Temp\\gittest': Invalid argument\n> > warning: unable to access '.git/branches/C:\\Temp\\gittest': Invalid argument\n> > From C:\\Temp\\gittest\n> >  * branch            HEAD       -> FETCH_HEAD\n> \n> What is the exact precondition to get such a warning ?\n> \n> Just wondering, because I'm not able to reproduce that warning\n> (with Git4win version 2.13.0.windows.1).\n\nI had tested this also, and came to the conclusion that only Hannes'\nMSys-based custom version is affected that is much closer to git.git's\n`master` than to Git for Windows' fork.\n\nStill, the patches are good in my opinion.\n\nCiao,\nDscho\n"},{"id":"320449","messageId":"1542d54f-d530-b60d-3270-d9b4e94c814d@kdbg.org","threadId":"46007","inReplyTo":"alpine.DEB.2.21.1.1705221600100.3610@virtualbox","subject":"Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-05-22T16:28:20Z","receivedAt":"2017-05-22T16:28:30Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.05.2017 um 16:01 schrieb Johannes Schindelin:\n> On Mon, 22 May 2017, stefan.naewe@atlas-elektronik.com wrote:\n>> Am 20.05.2017 um 08:28 schrieb Johannes Sixt:\n>>> This small series fixes these warnings on Windows:\n>>>\n>>> C:\\Temp\\gittest>git fetch C:\\Temp\\gittest\n>>> warning: unable to access '.git/remotes/C:\\Temp\\gittest': Invalid argument\n>>> warning: unable to access '.git/branches/C:\\Temp\\gittest': Invalid argument\n>>>  From C:\\Temp\\gittest\n>>>   * branch            HEAD       -> FETCH_HEAD\n>>\n>> What is the exact precondition to get such a warning ?\n>>\n>> Just wondering, because I'm not able to reproduce that warning\n>> (with Git4win version 2.13.0.windows.1).\n> \n> I had tested this also, and came to the conclusion that only Hannes'\n> MSys-based custom version is affected that is much closer to git.git's\n> `master` than to Git for Windows' fork.\n\nIn this case, the warning occurs because I build with nd/fopen-errors.\n\nWhich explains why I observed it only recently even though I fetch from \nWindows paths regularly.\n\n-- Hannes\n"},{"id":"320450","messageId":"947b2453-52b6-4940-fd20-ddf350c5df4d@kdbg.org","threadId":"46007","inReplyTo":"alpine.DEB.2.21.1.1705221357360.3610@virtualbox","subject":"Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-05-22T16:36:35Z","receivedAt":"2017-05-22T16:36:50Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.05.2017 um 13:59 schrieb Johannes Schindelin:\n> On Sat, 20 May 2017, Johannes Sixt wrote:\n>> This small series fixes these warnings on Windows:\n>>\n>> C:\\Temp\\gittest>git fetch C:\\Temp\\gittest\n>> warning: unable to access '.git/remotes/C:\\Temp\\gittest': Invalid argument\n>> warning: unable to access '.git/branches/C:\\Temp\\gittest': Invalid argument\n>>  From C:\\Temp\\gittest\n>>   * branch            HEAD       -> FETCH_HEAD\n>>\n>> The fix is in the second patch; the first patch is a\n>> preparation that allows to write the fix in my preferred style.\n> \n> Thank you!\n> \n> Maybe you want to accompany these patches with a simple test case that\n> uses e.g. ls-remote on $(pwd | tr / \\\\\\\\)?\n\nActually, no, I don't want to. It would have to be protected by MINGW, \nand I don't want to burden us (and here I mean Windows folks) with a \ncheck for a cosmetic deficiency. (Shell scripts, slow forks, yadda, \nyadda...)\n\n-- Hannes\n"},{"id":"320459","messageId":"7b1f97ff-52b7-72c3-211f-e73dce562911@kdbg.org","threadId":"46007","inReplyTo":"cover.1495261020.git.j6t@kdbg.org","subject":"[PATCH v2 1/2] mingw.h: permit arguments with side effects for is_dir_sep","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-05-22T18:58:32Z","receivedAt":"2017-05-22T18:58:53Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Taking git-compat-util.h's cue (which uses an inline function to back\nis_dir_sep()), let's use an inline function to back also the Windows\nversion of is_dir_sep(). This avoids problems when calling the function\nwith arguments that do more than just provide a single character, e.g.\nincrementing a pointer. Example:\n\n    is_dir_sep(*p++)\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n This v2 takes your commit message body because I like it better\n than mine. I did not change the subject because your suggestion\n sounded like the exact opposite of what this patch wants to achieve\n on first reading. Patch text is unchanged.\n\n compat/mingw.h | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 034fff9479..d2168c1e5e 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -395,7 +395,11 @@ HANDLE winansi_get_osfhandle(int fd);\n \t(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)\n int mingw_skip_dos_drive_prefix(char **path);\n #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix\n-#define is_dir_sep(c) ((c) == '/' || (c) == '\\\\')\n+static inline int mingw_is_dir_sep(int c)\n+{\n+\treturn c == '/' || c == '\\\\';\n+}\n+#define is_dir_sep mingw_is_dir_sep\n static inline char *mingw_find_last_dir_sep(const char *path)\n {\n \tchar *ret = NULL;\n-- \n2.13.0.55.g17b7d13330\n\n"},{"id":"320460","messageId":"b2fa1cff-9902-5bef-0392-1371bf1f4268@kdbg.org","threadId":"46007","inReplyTo":"7b1f97ff-52b7-72c3-211f-e73dce562911@kdbg.org","subject":"[PATCH v2 2/2] Windows: do not treat a path with backslashes as a remote's nick name","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-05-22T19:01:08Z","receivedAt":"2017-05-22T19:01:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Fetching from a remote using a native Windows path produces these warnings:\n\nC:\\Temp\\gittest>git fetch C:\\Temp\\gittest\nwarning: unable to access '.git/remotes/C:\\Temp\\gittest': Invalid argument\nwarning: unable to access '.git/branches/C:\\Temp\\gittest': Invalid argument\nFrom C:\\Temp\\gittest\n * branch            HEAD       -> FETCH_HEAD\n\nThe functions that read the branches and remotes files are protected by\na valid_remote_nick() check. Its implementation does not take Windows\npaths into account, so that the caller simply concatenates the paths,\nleading to the error seen above.\n\nUse is_dir_sep() to check for both slashes and backslashes on Windows.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n This v2 writes your suggested comment in front of the loop (not just\n the 'if' because then I feel I would have to add braces).\n\n remote.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex ad6c5424ed..1949882c10 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -645,7 +645,12 @@ static int valid_remote_nick(const char *name)\n {\n \tif (!name[0] || is_dot_or_dotdot(name))\n \t\treturn 0;\n-\treturn !strchr(name, '/'); /* no slash */\n+\n+\t/* remote nicknames cannot contain slashes */\n+\twhile (*name)\n+\t\tif (is_dir_sep(*name++))\n+\t\t\treturn 0;\n+\treturn 1;\n }\n \n const char *remote_for_branch(struct branch *branch, int *explicit)\n-- \n2.13.0.55.g17b7d13330\n"},{"id":"320547","messageId":"alpine.DEB.2.21.1.1705231245300.3610@virtualbox","threadId":"46007","inReplyTo":"1542d54f-d530-b60d-3270-d9b4e94c814d@kdbg.org","subject":"Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-23T10:46:24Z","receivedAt":"2017-05-23T10:46:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Mon, 22 May 2017, Johannes Sixt wrote:\n\n> Am 22.05.2017 um 16:01 schrieb Johannes Schindelin:\n> > On Mon, 22 May 2017, stefan.naewe@atlas-elektronik.com wrote:\n> > > Am 20.05.2017 um 08:28 schrieb Johannes Sixt:\n> > > > This small series fixes these warnings on Windows:\n> > > >\n> > > > C:\\Temp\\gittest>git fetch C:\\Temp\\gittest\n> > > > warning: unable to access '.git/remotes/C:\\Temp\\gittest': Invalid\n> > > > warning: argument\n> > > > warning: unable to access '.git/branches/C:\\Temp\\gittest': Invalid\n> > > > warning: argument\n> > > >  From C:\\Temp\\gittest\n> > > >   * branch            HEAD       -> FETCH_HEAD\n> > >\n> > > What is the exact precondition to get such a warning ?\n> > >\n> > > Just wondering, because I'm not able to reproduce that warning\n> > > (with Git4win version 2.13.0.windows.1).\n> > \n> > I had tested this also, and came to the conclusion that only Hannes'\n> > MSys-based custom version is affected that is much closer to git.git's\n> > `master` than to Git for Windows' fork.\n> \n> In this case, the warning occurs because I build with nd/fopen-errors.\n\nAh. So the base commit Junio chose for your v1 is completely\ninappropriate. It should be nd/fopen-errors instead.\n\nThanks for the clarification!\nDscho\n"},{"id":"320548","messageId":"alpine.DEB.2.21.1.1705231248150.3610@virtualbox","threadId":"46007","inReplyTo":"947b2453-52b6-4940-fd20-ddf350c5df4d@kdbg.org","subject":"Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-23T10:53:18Z","receivedAt":"2017-05-23T10:53:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes (& Junio, see below),\n\nOn Mon, 22 May 2017, Johannes Sixt wrote:\n\n> Am 22.05.2017 um 13:59 schrieb Johannes Schindelin:\n> > On Sat, 20 May 2017, Johannes Sixt wrote:\n> > > This small series fixes these warnings on Windows:\n> > >\n> > > C:\\Temp\\gittest>git fetch C:\\Temp\\gittest\n> > > warning: unable to access '.git/remotes/C:\\Temp\\gittest': Invalid argument\n> > > warning: unable to access '.git/branches/C:\\Temp\\gittest': Invalid\n> > > warning: argument\n> > >  From C:\\Temp\\gittest\n> > >   * branch            HEAD       -> FETCH_HEAD\n> > >\n> > > The fix is in the second patch; the first patch is a\n> > > preparation that allows to write the fix in my preferred style.\n> > \n> > Thank you!\n> > \n> > Maybe you want to accompany these patches with a simple test case that\n> > uses e.g. ls-remote on $(pwd | tr / \\\\\\\\)?\n> \n> Actually, no, I don't want to. It would have to be protected by MINGW, and I\n> don't want to burden us (and here I mean Windows folks) with a check for a\n> cosmetic deficiency. (Shell scripts, slow forks, yadda, yadda...)\n\nActually, yes, I want to.\n\nYes, it would have to be protected by MINGW, and yes, it would put a\nburden on us, but also yes: it would put an even higher burden on me to\ncheck this manually before releasing Git for Windows, or even worse: this\nregression would be the kind of bug that triggers many bug reports,\naddressing which would take a lot more time than writing this test case\nand executing it as part of our test suite.\n\nSo here goes:\n\n-- snipsnap --\nFrom: Johannes Schindelin <johannes.schindelin@gmx.de>\nDate: Tue, 23 May 2017 12:42:13 +0200\nSubject: [PATCH] mingw: verify that paths are not mistaken for remote nicknames\n\nThis added test case simply verifies that users will not be bothered\nwith bogus complaints à la\n\n\twarning: unable to access '.git/remotes/D:\\repo': Invalid argument\n\nwhen fetching from a Windows path (in this case, D:\\repo).\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n t/t5580-clone-push-unc.sh | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh\nindex b195f71ea98..93ce99ba3c6 100755\n--- a/t/t5580-clone-push-unc.sh\n+++ b/t/t5580-clone-push-unc.sh\n@@ -1,13 +1,19 @@\n #!/bin/sh\n \n-test_description='various UNC path tests (Windows-only)'\n+test_description='various Windows-only path tests'\n . ./test-lib.sh\n \n if ! test_have_prereq MINGW; then\n-\tskip_all='skipping UNC path tests, requires Windows'\n+\tskip_all='skipping Windows-only path tests'\n \ttest_done\n fi\n \n+test_expect_success 'remote nick cannot contain backslashes' '\n+\tBACKSLASHED=\"$(pwd | tr / \\\\\\\\)\" &&\n+\tgit ls-remote \"$BACKSLASHED\" >out 2>err &&\n+\t! grep \"unable to access\" err\n+'\n+\n UNCPATH=\"$(pwd)\"\n case \"$UNCPATH\" in\n [A-Z]:*)\n-- \n2.13.0.windows.1\n"},{"id":"320568","messageId":"574f9050-e711-e83c-ba93-6be2dc2a44bc@kdbg.org","threadId":"46007","inReplyTo":"alpine.DEB.2.21.1.1705231248150.3610@virtualbox","subject":"Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-05-23T18:39:06Z","receivedAt":"2017-05-23T18:39:15Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 23.05.2017 um 12:53 schrieb Johannes Schindelin:\n> Hi Hannes (& Junio, see below),\n> \n> On Mon, 22 May 2017, Johannes Sixt wrote:\n> \n>> Am 22.05.2017 um 13:59 schrieb Johannes Schindelin:\n>>> On Sat, 20 May 2017, Johannes Sixt wrote:\n>>>> This small series fixes these warnings on Windows:\n>>>>\n>>>> C:\\Temp\\gittest>git fetch C:\\Temp\\gittest\n>>>> warning: unable to access '.git/remotes/C:\\Temp\\gittest': Invalid argument\n>>>> warning: unable to access '.git/branches/C:\\Temp\\gittest': Invalid\n>>>> warning: argument\n>>>>   From C:\\Temp\\gittest\n>>>>    * branch            HEAD       -> FETCH_HEAD\n>>>>\n>>>> The fix is in the second patch; the first patch is a\n>>>> preparation that allows to write the fix in my preferred style.\n>>>\n>>> Thank you!\n>>>\n>>> Maybe you want to accompany these patches with a simple test case that\n>>> uses e.g. ls-remote on $(pwd | tr / \\\\\\\\)?\n>>\n>> Actually, no, I don't want to. It would have to be protected by MINGW, and I\n>> don't want to burden us (and here I mean Windows folks) with a check for a\n>> cosmetic deficiency. (Shell scripts, slow forks, yadda, yadda...)\n> \n> Actually, yes, I want to.\n> \n> Yes, it would have to be protected by MINGW, and yes, it would put a\n> burden on us, but also yes: it would put an even higher burden on me to\n> check this manually before releasing Git for Windows, or even worse: this\n> regression would be the kind of bug that triggers many bug reports,\n> addressing which would take a lot more time than writing this test case\n> and executing it as part of our test suite.\n\nFair enough. The patch looks good. I'll be able to test no earlier than \nMonday, though.\n\n> \n> So here goes:\n> \n> -- snipsnap --\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Date: Tue, 23 May 2017 12:42:13 +0200\n> Subject: [PATCH] mingw: verify that paths are not mistaken for remote nicknames\n> \n> This added test case simply verifies that users will not be bothered\n> with bogus complaints à la\n> \n> \twarning: unable to access '.git/remotes/D:\\repo': Invalid argument\n> \n> when fetching from a Windows path (in this case, D:\\repo).\n> \n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>   t/t5580-clone-push-unc.sh | 10 ++++++++--\n>   1 file changed, 8 insertions(+), 2 deletions(-)\n> \n> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh\n> index b195f71ea98..93ce99ba3c6 100755\n> --- a/t/t5580-clone-push-unc.sh\n> +++ b/t/t5580-clone-push-unc.sh\n> @@ -1,13 +1,19 @@\n>   #!/bin/sh\n>   \n> -test_description='various UNC path tests (Windows-only)'\n> +test_description='various Windows-only path tests'\n>   . ./test-lib.sh\n>   \n>   if ! test_have_prereq MINGW; then\n> -\tskip_all='skipping UNC path tests, requires Windows'\n> +\tskip_all='skipping Windows-only path tests'\n>   \ttest_done\n>   fi\n>   \n> +test_expect_success 'remote nick cannot contain backslashes' '\n> +\tBACKSLASHED=\"$(pwd | tr / \\\\\\\\)\" &&\n> +\tgit ls-remote \"$BACKSLASHED\" >out 2>err &&\n> +\t! grep \"unable to access\" err\n> +'\n> +\n>   UNCPATH=\"$(pwd)\"\n>   case \"$UNCPATH\" in\n>   [A-Z]:*)\n> \n\n"},{"id":"320600","messageId":"xmqqinkrjkfb.fsf@gitster.mtv.corp.google.com","threadId":"46007","inReplyTo":"alpine.DEB.2.21.1.1705231245300.3610@virtualbox","subject":"Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-23T22:08:24Z","receivedAt":"2017-05-23T22:08:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> In this case, the warning occurs because I build with nd/fopen-errors.\n>\n> Ah. So the base commit Junio chose for your v1 is completely\n> inappropriate. It should be nd/fopen-errors instead.\n\nActuallly, Hannes's patch text and problem description are\nconfusingly inconsistent, and I have to say that the above statement\nis wrong---do not react to this statement just yet, because I'll\nalso say \"but you are right\" real soon.\n\nBecause \"git fetch a/b\" your UNIXy friends run will not consider\n\"a/b\" as a remote nickname, \"a\\b\" in \"git fetch a\\b\" Windows folks\nrun must not be taken as a remote nickname either.  That is a\njustification enough for Hannes's patch text, and that does not\nchange whether we take nd/fopen-errors or discard it.  So in that\nsense, the patch text does not have anything to do with the other\ntopic.\n\nBut you are right (as promised ;-) to say that nd/fopen-errors needs\na bit more polishing to work correctly on Windows.  I unfortunately\ndo not think Hannes's patch addresses the real issue.  \n\nIsn't the root cause of \"Invalid argument\" a colon in a (mistakenly\nidentified) remote nickname, not a backslash?  I doubt that it would\nhelp to loosen valid_remote_nick() to pay attention to backslashes.\nSurely, it may hide the issue when the path also has a backslash,\nbut wouldn't Windows folks see the same warning for \"git fetch c:c\"\nfor example even with Hannes's patch?\n\nWe use the pattern \"try to open a path, and treat a failure to open\nas indicating that the path is missing (and take information we\nwould have obtained from the contents of the path, if existed, from\nelsewhere, as necessary)\" pretty much everywhere, not just when\naccessing a remote.  And nd/fopen-errors tries to tweak the \"treat a\nfailure to open as a sign of missing path\" to be a bit more careful\nby warning unless the error we get is ENOENT or ENOTDIR (otherwise\nwe may treat a file that exists but cannot be read as if it is\nmissing without any indication).\n\nThe solution for the problem Hannes's proposed log message describes\nlies in warn_on_fopen_errors() function that was introduced in the\nnd/fopen-errors topic.  It appears that in Windows port, open() and\nfopen() return EINVAL for a file that does not exist and whose name\nWindows does not like, so we probably should do something like the\nattached to work around the EINVAL (I do not know about the symbol\nchecked for #ifdef---there may be a more appropriate one).\n\nI am not entirely happy with the workaround, because I do not know\nif EINVAL can come _ONLY_ due to a colon in the pathname on Windows,\nor if there are some other cases that deserve to be warned that also\nyield EINVAL, and the attached will sweep the problem under the rug\nfor the \"other cases\", partially undoing what nd/fopen-errors topic\nwanted to do.  But it does not make it worse than before the topic\nhappened [*1*].\n\nAnd that kind of solution does belong to nd/fopen-errors.\n\nSo in short:\n\n (1) Hannes's patches are good, but they solve a problem that is\n     different from what their log messages say; the log message\n     needs to be updated;\n\n (2) In nd/fopen-errors topic, we need a new patch that deals with\n     EINVAL on Windows.\n\nThanks.\n\n\n[Footnote]\n\n*1* An alternative that may allow us to avoid sweeping the issue\n    under the rug may be to have a more precise logic to see if the\n    open failure is due to missing file in the implementation of\n    open() and fopen() emulation---at that level, the logic can be\n    built with more platform knowledge (e.g. EINVAL?  Let's see what\n    path we got---ah, there is a colon)---and turn EINVAL into\n    ENOENT, or something like that.  I do not have a strong opinion\n    if that is a better solution or the attached workaround is good\n    enough.\n\n wrapper.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 6e513c904a..7b6d3e3f94 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -418,9 +418,18 @@ FILE *fopen_for_writing(const char *path)\n \treturn ret;\n }\n \n+static int is_an_error_for_missing_path(int no)\n+{\n+#if WINDOWS\n+\tif (errno == EINVAL)\n+\t\treturn 1;\n+#endif\n+\treturn (errno == ENOENT || errno == ENOTDIR);\n+}\n+\n int warn_on_fopen_errors(const char *path)\n {\n-\tif (errno != ENOENT && errno != ENOTDIR) {\n+\tif (!is_error_for_missing_path(errno)) {\n \t\twarn_on_inaccessible(path);\n \t\treturn -1;\n \t}\n\n"},{"id":"320648","messageId":"c36ae9af-b566-5bb3-ad70-ee7f0051ca9a@kdbg.org","threadId":"46007","inReplyTo":"xmqqinkrjkfb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-05-24T05:45:42Z","receivedAt":"2017-05-24T05:45:50Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.05.2017 um 00:08 schrieb Junio C Hamano:\n> So in short:\n> \n>   (1) Hannes's patches are good, but they solve a problem that is\n>       different from what their log messages say; the log message\n>       needs to be updated;\n> \n>   (2) In nd/fopen-errors topic, we need a new patch that deals with\n>       EINVAL on Windows.\n\nWill reroll the patches. Good analysis, BTW. (It was a late discovery of \nmine that nd/fopen-errors is actually the source of the warnings.)\n\n-- Hannes\n"},{"id":"320707","messageId":"10ee6b91-cd54-d43a-4cfb-d3baa2af7e7a@kdbg.org","threadId":"46007","inReplyTo":"c36ae9af-b566-5bb3-ad70-ee7f0051ca9a@kdbg.org","subject":"[PATCH v3 2/2] Windows: do not treat a path with backslashes as a remote's nick name","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-05-25T12:00:13Z","receivedAt":"2017-05-25T12:00:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Windows, the remote repository name in, e.g., `git fetch foo\\bar`\nis clearly not a nickname for a configured remote repository. However,\nthe function valid_remote_nick() does not account for backslashes.\nUse is_dir_sep() to check for both slashes and backslashes on Windows.\n\nThis was discovered while playing with Duy's patches that warn after\nfopen() failures. The functions that read the branches and remotes\nfiles are protected by a valid_remote_nick() check. Without this\nchange, a Windows style absolute path is incorrectly regarded as\nnickname and is concatenated to a prefix and used with fopen(). This\ntriggers warnings because a colon in a path name is not allowed:\n\nC:\\Temp\\gittest>git fetch C:\\Temp\\gittest\nwarning: unable to access '.git/remotes/C:\\Temp\\gittest': Invalid argument\nwarning: unable to access '.git/branches/C:\\Temp\\gittest': Invalid argument\nFrom C:\\Temp\\gittest\n * branch            HEAD       -> FETCH_HEAD\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\nAm 24.05.2017 um 07:45 schrieb Johannes Sixt:\n> Am 24.05.2017 um 00:08 schrieb Junio C Hamano:\n>> So in short:\n>>\n>>   (1) Hannes's patches are good, but they solve a problem that is\n>>       different from what their log messages say; the log message\n>>       needs to be updated;\n\nI do not resend patch 1/2 as it is unchanged in all regards. This 2/2\nchanges the justification; patch text is unchanged.\n\n remote.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex ad6c5424ed..1949882c10 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -645,7 +645,12 @@ static int valid_remote_nick(const char *name)\n {\n \tif (!name[0] || is_dot_or_dotdot(name))\n \t\treturn 0;\n-\treturn !strchr(name, '/'); /* no slash */\n+\n+\t/* remote nicknames cannot contain slashes */\n+\twhile (*name)\n+\t\tif (is_dir_sep(*name++))\n+\t\t\treturn 0;\n+\treturn 1;\n }\n \n const char *remote_for_branch(struct branch *branch, int *explicit)\n-- \n2.13.0.55.g17b7d13330\n"},{"id":"320772","messageId":"xmqq37bseddy.fsf@gitster.mtv.corp.google.com","threadId":"46007","inReplyTo":"10ee6b91-cd54-d43a-4cfb-d3baa2af7e7a@kdbg.org","subject":"Re: [PATCH v3 2/2] Windows: do not treat a path with backslashes as a remote's nick name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-25T23:16:09Z","receivedAt":"2017-05-25T23:16:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n>>> So in short:\n>>>\n>>>   (1) Hannes's patches are good, but they solve a problem that is\n>>>       different from what their log messages say; the log message\n>>>       needs to be updated;\n>\n> I do not resend patch 1/2 as it is unchanged in all regards. This 2/2\n> changes the justification; patch text is unchanged.\n\nThanks.  I think this is explained better.  Complaints against\nfopen() warnings sounded as if we should avoid attempting to open a\nfile that may result in _any_ failure, which I felt was misleading,\nbut it is not a huge issue.\n\nSo how do we want to proceed on the point (2), i.e. updating the\n\"warn on _unexpected_ errors from fopen\" series to make it aware of\nthe EINVAL we can expect on Windows?  My primary question is if all\nEINVAL we could ever see on Windows after open/fopen returns an\nerror is because the pathname the caller gave is not liked by the\nfilesystem (hence we also know that the path does not exist).\n\nIf that is the case, then the \"workaround\" patch I sent would be an\nOK approach (even though I do not know what to write after #ifdef\nand I suspect that is not \"WINDOWS\". We would want to cover the one\nyou use, the one Dscho releases and possibly the cygwin build).\n\nIf we can see EINVAL after open/fopen error that is _not_ expected\nand indicates a failure that is worth reporting to the user (just\nlike we want to report e.g. I/O or permission errors), I think\nWindows folks are in a better position than I am to decide between\nthat approach and a patch at lower level (e.g. teach open/fopen not\nto give EINVAL and instead give ENOENT when appropriate).\n\n\n>  remote.c | 7 ++++++-\n>  1 file changed, 6 insertions(+), 1 deletion(-)\n>\n> diff --git a/remote.c b/remote.c\n> index ad6c5424ed..1949882c10 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -645,7 +645,12 @@ static int valid_remote_nick(const char *name)\n>  {\n>  \tif (!name[0] || is_dot_or_dotdot(name))\n>  \t\treturn 0;\n> -\treturn !strchr(name, '/'); /* no slash */\n> +\n> +\t/* remote nicknames cannot contain slashes */\n> +\twhile (*name)\n> +\t\tif (is_dir_sep(*name++))\n> +\t\t\treturn 0;\n> +\treturn 1;\n>  }\n>  \n>  const char *remote_for_branch(struct branch *branch, int *explicit)\n"}]}