{"thread":{"id":"44695","subject":"[PATCH] fix pushing to //server/share/dir paths on Windows","startedAt":"2016-12-13T21:33:01Z","lastAt":"2016-12-19T17:19:40Z","messageCount":8,"participants":["Johannes Sixt","Junio C Hamano","Jeff King","Torsten Bögershausen","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"307688","messageId":"2ff2613c-47da-a780-5d38-93e16cb16328@kdbg.org","threadId":"44695","inReplyTo":null,"subject":"[PATCH] fix pushing to //server/share/dir paths on Windows","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-12-13T21:32:01Z","receivedAt":"2016-12-13T21:33:01Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"normalize_path_copy() is not prepared to keep the double-slash of a\n//server/share/dir kind of path, but treats it like a regular POSIX\nstyle path and transforms it to /server/share/dir.\n\nThe bug manifests when 'git push //server/share/dir master' is run,\nbecause tmp_objdir_add_as_alternate() uses the path in normalized\nform when it registers the quarantine object database via\nlink_alt_odb_entries(). Needless to say that the directory cannot be\naccessed using the wrongly normalized path.\n\nFix it by skipping all of the root part, not just a potential drive\nprefix. offset_1st_component takes care of this, see the\nimplementation in compat/mingw.c::mingw_offset_1st_component().\n\nThere is a change in behavior: \\\\server\\share is not transformed\ninto //server/share anymore, but all subsequent directory separators\nare rewritten to '/'. This should not make a difference; Windows can\nhandle the mix. In the context of 'git push' this cannot be verified,\nthough, as there seems to be an independent bug that transforms the\ndouble '\\\\' to a single '\\' on the way.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n Another long-standing bug uncovered by the quarantine series.\n\n Dscho, it looks like this could fix the original report at\n https://github.com/git-for-windows/git/issues/979\n\n This patch should cook well because of the change in behavior.\n I would not be surprised if there is some fall-out.\n\n The other bug I'm alluding to, I still have to investigate. I do\n not think that it can be counted as fall-out.\n\n path.c | 24 +++++++++++++++---------\n 1 file changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 52d889c88e..02dc70fb92 100644\n--- a/path.c\n+++ b/path.c\n@@ -991,7 +991,7 @@ const char *remove_leading_path(const char *in, const char *prefix)\n  *\n  * Performs the following normalizations on src, storing the result in dst:\n  * - Ensures that components are separated by '/' (Windows only)\n- * - Squashes sequences of '/'.\n+ * - Squashes sequences of '/' except \"//server/share\" on Windows\n  * - Removes \".\" components.\n  * - Removes \"..\" components, and the components the precede them.\n  * Returns failure (non-zero) if a \"..\" component appears as first path\n@@ -1014,17 +1014,23 @@ const char *remove_leading_path(const char *in, const char *prefix)\n int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)\n {\n \tchar *dst0;\n-\tint i;\n-\n-\tfor (i = has_dos_drive_prefix(src); i > 0; i--)\n-\t\t*dst++ = *src++;\n-\tdst0 = dst;\n+\tint offset;\n \n-\tif (is_dir_sep(*src)) {\n+\t/*\n+\t * Handle initial part of absolute path: \"/\", \"C:/\", \"\\\\server\\share/\".\n+\t */\n+\toffset = offset_1st_component(src);\n+\tif (offset) {\n+\t\t/* Convert the trailing separator to '/' on Windows. */\n+\t\tmemcpy(dst, src, offset - 1);\n+\t\tdst += offset - 1;\n \t\t*dst++ = '/';\n-\t\twhile (is_dir_sep(*src))\n-\t\t\tsrc++;\n+\t\tsrc += offset;\n \t}\n+\tdst0 = dst;\n+\n+\twhile (is_dir_sep(*src))\n+\t\tsrc++;\n \n \tfor (;;) {\n \t\tchar c = *src;\n-- \n2.11.0.79.g263f27a\n\n"},{"id":"307695","messageId":"xmqqpokvzbqo.fsf@gitster.mtv.corp.google.com","threadId":"44695","inReplyTo":"2ff2613c-47da-a780-5d38-93e16cb16328@kdbg.org","subject":"Re: [PATCH] fix pushing to //server/share/dir paths on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-13T22:48:47Z","receivedAt":"2016-12-13T22:48:55Z","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> There is a change in behavior: \\\\server\\share is not transformed\n> into //server/share anymore, but all subsequent directory separators\n> are rewritten to '/'. This should not make a difference; Windows can\n> handle the mix.\n\nI saw Dscho had a similar \"windows can handle the mix\" change in an\nearlier development cycle, I think, and this is being consistent.\n\n>  Another long-standing bug uncovered by the quarantine series.\n>\n>  Dscho, it looks like this could fix the original report at\n>  https://github.com/git-for-windows/git/issues/979\n>\n>  This patch should cook well because of the change in behavior.\n>  I would not be surprised if there is some fall-out.\n>\n>  The other bug I'm alluding to, I still have to investigate. I do\n>  not think that it can be counted as fall-out.\n>\n>  path.c | 24 +++++++++++++++---------\n>  1 file changed, 15 insertions(+), 9 deletions(-)\n\nThanks.\n\n> diff --git a/path.c b/path.c\n> index 52d889c88e..02dc70fb92 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -991,7 +991,7 @@ const char *remove_leading_path(const char *in, const char *prefix)\n>   *\n>   * Performs the following normalizations on src, storing the result in dst:\n>   * - Ensures that components are separated by '/' (Windows only)\n> - * - Squashes sequences of '/'.\n> + * - Squashes sequences of '/' except \"//server/share\" on Windows\n\n\"on windows\" because offset_1st_component() does the magic only\nthere?  Makes sense.\n\n>   * - Removes \".\" components.\n>   * - Removes \"..\" components, and the components the precede them.\n>   * Returns failure (non-zero) if a \"..\" component appears as first path\n> @@ -1014,17 +1014,23 @@ const char *remove_leading_path(const char *in, const char *prefix)\n>  int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)\n>  {\n>  \tchar *dst0;\n> -\tint i;\n> -\n> -\tfor (i = has_dos_drive_prefix(src); i > 0; i--)\n> -\t\t*dst++ = *src++;\n> -\tdst0 = dst;\n> +\tint offset;\n>  \n> -\tif (is_dir_sep(*src)) {\n> +\t/*\n> +\t * Handle initial part of absolute path: \"/\", \"C:/\", \"\\\\server\\share/\".\n> +\t */\n> +\toffset = offset_1st_component(src);\n> +\tif (offset) {\n> +\t\t/* Convert the trailing separator to '/' on Windows. */\n> +\t\tmemcpy(dst, src, offset - 1);\n> +\t\tdst += offset - 1;\n>  \t\t*dst++ = '/';\n> -\t\twhile (is_dir_sep(*src))\n> -\t\t\tsrc++;\n> +\t\tsrc += offset;\n>  \t}\n> +\tdst0 = dst;\n\nBy resetting dst0 here, we ensure that up_one that is triggered by\nseeing \"../\" will not escape the \\\\server\\share\\ part, which makes\nsense to me.\n\n> +\twhile (is_dir_sep(*src))\n> +\t\tsrc++;\n>  \n>  \tfor (;;) {\n>  \t\tchar c = *src;\n"},{"id":"307788","messageId":"20161214173034.inbyakdykjv5j7ua@sigill.intra.peff.net","threadId":"44695","inReplyTo":"2ff2613c-47da-a780-5d38-93e16cb16328@kdbg.org","subject":"Re: [PATCH] fix pushing to //server/share/dir paths on Windows","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-14T17:30:34Z","receivedAt":"2016-12-14T17:30:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2016 at 10:32:01PM +0100, Johannes Sixt wrote:\n\n> normalize_path_copy() is not prepared to keep the double-slash of a\n> //server/share/dir kind of path, but treats it like a regular POSIX\n> style path and transforms it to /server/share/dir.\n> \n> The bug manifests when 'git push //server/share/dir master' is run,\n> because tmp_objdir_add_as_alternate() uses the path in normalized\n> form when it registers the quarantine object database via\n> link_alt_odb_entries(). Needless to say that the directory cannot be\n> accessed using the wrongly normalized path.\n\nThanks for digging this up! I had a feeling that the problem was going\nto be in the underlying path code, but I didn't want to just pass the\nbuck without evidence. :)\n\n> -\tif (is_dir_sep(*src)) {\n> +\t/*\n> +\t * Handle initial part of absolute path: \"/\", \"C:/\", \"\\\\server\\share/\".\n> +\t */\n> +\toffset = offset_1st_component(src);\n> +\tif (offset) {\n> +\t\t/* Convert the trailing separator to '/' on Windows. */\n> +\t\tmemcpy(dst, src, offset - 1);\n> +\t\tdst += offset - 1;\n>  \t\t*dst++ = '/';\n\nHmm. So this is the \"change-of-behavior\" bit. Would it be reasonable to\nwrite:\n\n  /* Copy initial part of absolute path, converting separators on Windows */\n  const char *end = src + offset_1st_component(src);\n  while (src < end) {\n\t  char c = *src++;\n\t  if (c == '\\\\')\n\t\t  c = '/';\n\t  *dst++ = c;\n  }\n\n? I'm not sure if it's wrong to convert backslashes in that first\ncomponent or not (but certainly we were before). I don't think we'd need\nis_dir_sep() in that \"if()\", because we can leave slashes as-is. But\nmaybe it would make the code easier to read.\n\n-Peff\n"},{"id":"307803","messageId":"787a421b-8b7a-14c5-768f-06c3dc183cf4@kdbg.org","threadId":"44695","inReplyTo":"20161214173034.inbyakdykjv5j7ua@sigill.intra.peff.net","subject":"[PATCH v2] fix pushing to //server/share/dir on Windows","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-12-14T19:37:38Z","receivedAt":"2016-12-14T19:37:45Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"normalize_path_copy() is not prepared to keep the double-slash of a\n//server/share/dir kind of path, but treats it like a regular POSIX\nstyle path and transforms it to /server/share/dir.\n\nThe bug manifests when 'git push //server/share/dir master' is run,\nbecause tmp_objdir_add_as_alternate() uses the path in normalized\nform when it registers the quarantine object database via\nlink_alt_odb_entries(). Needless to say that the directory cannot be\naccessed using the wrongly normalized path.\n\nFix it by skipping all of the root part, not just a potential drive\nprefix. offset_1st_component takes care of this, see the\nimplementation in compat/mingw.c::mingw_offset_1st_component().\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\nAm 14.12.2016 um 18:30 schrieb Jeff King:\n> Would it be reasonable to\n> write:\n> \n>   /* Copy initial part of absolute path, converting separators on Windows */\n>   const char *end = src + offset_1st_component(src);\n>   while (src < end) {\n> \t  char c = *src++;\n> \t  if (c == '\\\\')\n> \t\t  c = '/';\n> \t  *dst++ = c;\n>   }\n\nMakes a lot of sense! I haven't had an opportunity, though, to test\non Windows.\n\n> ? I'm not sure if it's wrong to convert backslashes in that first\n> component or not (but certainly we were before). I don't think we'd need\n> is_dir_sep() in that \"if()\", because we can leave slashes as-is. But\n> maybe it would make the code easier to read.\n\nis_dir_sep() is preferable, IMO.\n\nI also changed the commit message and subject line slightly.\n\n path.c | 23 ++++++++++++++---------\n 1 file changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 52d889c88e..efcedafba6 100644\n--- a/path.c\n+++ b/path.c\n@@ -991,7 +991,7 @@ const char *remove_leading_path(const char *in, const char *prefix)\n  *\n  * Performs the following normalizations on src, storing the result in dst:\n  * - Ensures that components are separated by '/' (Windows only)\n- * - Squashes sequences of '/'.\n+ * - Squashes sequences of '/' except \"//server/share\" on Windows\n  * - Removes \".\" components.\n  * - Removes \"..\" components, and the components the precede them.\n  * Returns failure (non-zero) if a \"..\" component appears as first path\n@@ -1014,17 +1014,22 @@ const char *remove_leading_path(const char *in, const char *prefix)\n int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)\n {\n \tchar *dst0;\n-\tint i;\n+\tconst char *end;\n \n-\tfor (i = has_dos_drive_prefix(src); i > 0; i--)\n-\t\t*dst++ = *src++;\n+\t/*\n+\t * Copy initial part of absolute path: \"/\", \"C:/\", \"//server/share/\".\n+\t */\n+\tend = src + offset_1st_component(src);\n+\twhile (src < end) {\n+\t\tchar c = *src++;\n+\t\tif (is_dir_sep(c))\n+\t\t\tc = '/';\n+\t\t*dst++ = c;\n+\t}\n \tdst0 = dst;\n \n-\tif (is_dir_sep(*src)) {\n-\t\t*dst++ = '/';\n-\t\twhile (is_dir_sep(*src))\n-\t\t\tsrc++;\n-\t}\n+\twhile (is_dir_sep(*src))\n+\t\tsrc++;\n \n \tfor (;;) {\n \t\tchar c = *src;\n-- \n2.11.0.79.g263f27a\n\n"},{"id":"307832","messageId":"c8501e28-db8a-5b6e-717c-5bda1e63c2e7@web.de","threadId":"44695","inReplyTo":"787a421b-8b7a-14c5-768f-06c3dc183cf4@kdbg.org","subject":"Re: [PATCH v2] fix pushing to //server/share/dir on Windows","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-12-15T07:30:52Z","receivedAt":"2016-12-15T07:31:55Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\n\nOn 14/12/16 20:37, Johannes Sixt wrote:\n> normalize_path_copy() is not prepared to keep the double-slash of a\n> //server/share/dir kind of path, but treats it like a regular POSIX\n> style path and transforms it to /server/share/dir.\n>\n> The bug manifests when 'git push //server/share/dir master' is run,\n> because tmp_objdir_add_as_alternate() uses the path in normalized\n> form when it registers the quarantine object database via\n> link_alt_odb_entries(). Needless to say that the directory cannot be\n> accessed using the wrongly normalized path.\n>\n> Fix it by skipping all of the root part, not just a potential drive\n> prefix. offset_1st_component takes care of this, see the\n> implementation in compat/mingw.c::mingw_offset_1st_component().\n>\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n> Am 14.12.2016 um 18:30 schrieb Jeff King:\n>> Would it be reasonable to\n>> write:\n>>\n>>    /* Copy initial part of absolute path, converting separators on Windows */\n>>    const char *end = src + offset_1st_component(src);\n>>    while (src < end) {\n>> \t  char c = *src++;\n>> \t  if (c == '\\\\')\n>> \t\t  c = '/';\n>> \t  *dst++ = c;\n>>    }\n> Makes a lot of sense! I haven't had an opportunity, though, to test\n> on Windows.\nI'm not sure, if a conversion should be done here, in this part of code.\nTo my knowledge,\n\nC:\\dir1\\file\nis the same\nas\nC:/dir1/file\nand that is handled by windows.\n\nThe \\\\server\\share\\dir1\\file is native to windows,\nand I can't see good reasons to change '\\' into '/' somewhere in Git,\nwhen UNC is used.\n\nCygwin does a translation from\n//server/share/dir1/file\ninto\n\\\\server\\share\\dir1\\file\n\nIn other words:\nThe patch looks good as is, and once I get a Windows machine,\nmay be able to do some testing and come up with test cases\n\n\n<https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx>\n[]\n\n"},{"id":"307836","messageId":"20161215110155.it7ptkbju5etmnpn@sigill.intra.peff.net","threadId":"44695","inReplyTo":"c8501e28-db8a-5b6e-717c-5bda1e63c2e7@web.de","subject":"Re: [PATCH v2] fix pushing to //server/share/dir on Windows","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-15T11:01:55Z","receivedAt":"2016-12-15T11:03:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 15, 2016 at 08:30:52AM +0100, Torsten Bögershausen wrote:\n\n> > > Would it be reasonable to\n> > > write:\n> > > \n> > >    /* Copy initial part of absolute path, converting separators on Windows */\n> > >    const char *end = src + offset_1st_component(src);\n> > >    while (src < end) {\n> > > \t  char c = *src++;\n> > > \t  if (c == '\\\\')\n> > > \t\t  c = '/';\n> > > \t  *dst++ = c;\n> > >    }\n> > Makes a lot of sense! I haven't had an opportunity, though, to test\n> > on Windows.\n> I'm not sure, if a conversion should be done here, in this part of code.\n> To my knowledge,\n> \n> C:\\dir1\\file\n> is the same\n> as\n> C:/dir1/file\n> and that is handled by windows.\n\nI don't have an opinion either way on what Windows would want, but note\nthat the function already _does_ convert separators to slashes. With\nJohannes's original patch, you'd end up with a mix, like:\n\n  \\\\server\\share/dir1/file\n\nSo this conversion is really just retaining the original behavior, and\nmaking it consistent throughout the path.\n\nWhich isn't to say that the function as it currently exists isn't a\nlittle bit buggy. :)\n\nOne of the points of normalizing, though, is that Git can then do\ntextual comparisons between the output. So I think there's value in\nhaving a canonical internal representation, even if the OS could handle\nmore exotic ones.\n\n-Peff\n"},{"id":"307850","messageId":"xmqq8trhw091.fsf@gitster.mtv.corp.google.com","threadId":"44695","inReplyTo":"20161215110155.it7ptkbju5etmnpn@sigill.intra.peff.net","subject":"Re: [PATCH v2] fix pushing to //server/share/dir on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-15T17:49:46Z","receivedAt":"2016-12-15T17:49:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I don't have an opinion either way on what Windows would want, but note\n> that the function already _does_ convert separators to slashes. With\n> Johannes's original patch, you'd end up with a mix, like:\n>\n>   \\\\server\\share/dir1/file\n>\n> So this conversion is really just retaining the original behavior, and\n> making it consistent throughout the path.\n\n>\n> Which isn't to say that the function as it currently exists isn't a\n> little bit buggy. :)\n>\n> One of the points of normalizing, though, is that Git can then do\n> textual comparisons between the output. So I think there's value in\n> having a canonical internal representation, even if the OS could handle\n> more exotic ones.\n\nE.g. the log message of d53c2c6738 (\"mingw: fix t9700's assumption\nabout directory separators\", 2016-01-27) says the two kinds of\nslashes are equivalent over there, but the patch text ends up doing\nexactly that normalization.\n\n5ca6b7bb47 (\"config --show-origin: report paths with forward\nslashes\", 2016-03-23) is an example of us trying to normalize in\norder to give consistent output to the users.\n\nHaving said all that, I do not have an opinion either way on what\nWindows would want, either ;-)\n\n"},{"id":"308060","messageId":"alpine.DEB.2.20.1612191817530.54750@virtualbox","threadId":"44695","inReplyTo":"2ff2613c-47da-a780-5d38-93e16cb16328@kdbg.org","subject":"Re: [PATCH] fix pushing to //server/share/dir paths on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-19T17:18:28Z","receivedAt":"2016-12-19T17:19:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Tue, 13 Dec 2016, Johannes Sixt wrote:\n\n>  Dscho, it looks like this could fix the original report at\n>  https://github.com/git-for-windows/git/issues/979\n\nYes, thank you so much for fixing it!\n\nI think your v2 is safe enough to cherry-pick directly into Git for\nWindows' `master`.\n\nThanks,\nDscho\n"}]}