{"thread":{"id":"49551","subject":"issue: strange `git diff --numstat` behavior.","startedAt":"2018-10-12T18:23:43Z","lastAt":"2018-10-19T17:04:32Z","messageCount":8,"participants":["Sergey Andreenko","Junio C Hamano","Johannes Sixt","Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"360331","messageId":"CAD1gVxMg0ZiKqFABrnwt0v_=wr2X_3ErkR92RmCAtFQnuM+L5w@mail.gmail.com","threadId":"49551","inReplyTo":null,"subject":"issue: strange `git diff --numstat` behavior.","fromName":"Sergey Andreenko","fromEmail":"andreenkosa@gmail.com","sentAt":"2018-10-12T18:23:28Z","receivedAt":"2018-10-12T18:23:43Z","isPatch":false,"sender":{"key":"andreenkosa@gmail.com","avatar":null},"body":"git diff –numstat FOLDER1 FOLDER2 works strange when run from a git\ncontrolled folder.\n\nThe output shrinks some symbols in the diff file paths.\n\n\n\nFor example:\n\nCreate a folder and call git init, for example: `C:\\test`.\n\n mkdir C:\\test\n\n cd C:\\test\n\n git init\n\nCreate two folders with to diff. For example: ` C:\\diff`, `C:\\base`\nand put a file in them with a diff (for example `1.txt` with `1` in\nbase and `1.txt` with 2 in diff).\n\n mkdir C:\\base\n\n mkdir C:\\diff\n\n echo. 12>C:\\base\\1.txt\n\n echo 13>C:\\diff\\1.txt\n\nRun git diff:\n\npushd C:\\\n\ngit.exe diff --numstat \"C:\\diff\" \"C:\\base\"\n\nOutput will be:\n\n1 1 \"C:\\\\diff/1.txt\" => \"C:\\\\base/1.txt\"\n\nNow move into C:\\test and run it again:\n\npushd C:\\test\n\ngit.exe diff --numstat \"C:\\diff\" \"C:\\base\"\n\n1 1 \"C:\\\\diff/1.txt\" => \"C:\\\\base/1.txt\"\n\nNow create a folder in `C:\\test`, for example `one`:\n\nmkdir one\n\ncd one\n\ngit.exe diff --numstat \"C:\\diff\" \"C:\\base\"\n\n\n\noutput will be:\n\n 0 1 {iff => ase}/1.txt\n\nSo (folder_name_length) symbols were cut from the path (“C:\\\\d” and “C:\\\\b”).\n\n\n\nIs any way to avoid that? I have checked several git versions and they\nall do the same.\n\n\n\nCommands to repro:\n\nmkdir C:\\test\n\ncd C:\\test\n\ngit init\n\nmkdir C:\\base\n\nmkdir C:\\diff\n\necho. 12>C:\\base\\1.txt\n\necho 13>C:\\diff\\1.txt\n\npushd C:\\\n\ngit.exe diff --numstat \"C:\\diff\" \"C:\\base\"\n\npushd C:\\test\n\ngit.exe diff --numstat \"C:\\diff\" \"C:\\base\"\n\nmkdir one\n\ncd one\n\ngit.exe diff --numstat \"C:\\diff\" \"C:\\base\"\n\n\n\nBest Regards,\n\nSergey Andrenko\n"},{"id":"360386","messageId":"xmqqr2gu8dsx.fsf@gitster-ct.c.googlers.com","threadId":"49551","inReplyTo":"CAD1gVxMg0ZiKqFABrnwt0v_=wr2X_3ErkR92RmCAtFQnuM+L5w@mail.gmail.com","subject":"Re: issue: strange `git diff --numstat` behavior.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-13T08:13:18Z","receivedAt":"2018-10-13T08:13:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Andreenko writes:\n\n> git.exe diff --numstat \"C:\\diff\" \"C:\\base\"\n> ...\n> output will be:\n>\n> 0 1 {iff => ase}/1.txt\n>\n> So (folder_name_length) symbols were cut from the path (“C:\\\\d” and “C:\\\\b”).\n>\n> Is any way to avoid that? I have checked several git versions and they\n> all do the same.\n\nNot an attempt to offer a solution (I don't do windows), but just\ntrying to see what random things we can try, I wonder what you'd get\nif you did something like\n\n\t$ git diff --numstat //c/diff //c/base\n\n"},{"id":"360859","messageId":"ae6fc699-6e09-2979-40dc-9cc49f4f8365@kdbg.org","threadId":"49551","inReplyTo":"xmqqr2gu8dsx.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] diff: don't attempt to strip prefix from absolute Windows paths","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-10-18T18:38:25Z","receivedAt":"2018-10-18T18:38:30Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"git diff can be invoked with absolute paths. Typically, this triggers\nthe --no-index case. Then the absolute paths remain in the file names\nthat are printed in the output.\n\nThere is one peculiarity, though: When the command is invoked from a\na sub-directory in a repository, then it is attempted to strip the\nsub-directory from the beginning of relative paths. Yet, to detect a\nrelative path the code just checks for an initial forward slash.\nThis mistakes a Windows style path like D:/base as a relative path\nand the output looks like this, for example:\n\n D:\\test\\one>git -P diff --numstat D:\\base D:\\diff\n 1 1 {ase => iff}/1.txt\n\nwhere the correct output should be\n\n D:\\test\\one>git -P diff --numstat D:\\base D:\\diff\n 1 1 D:/{base => diff}/1.txt\n\nIf the sub-directory where 'git diff' is invoked is sufficiently deep\nthat the prefix becomes longer than the path to be printed, then the\nsubsequent code even accesses the paths out of bounds!\n\nUse is_absolute_path() to detect Windows style absolute paths.\n\nOne might wonder whether the check for a directory separator that\nis visible in the patch context should be changed from == '/' to\nis_dir_sep() or not. It turns out not to be necessary. That code\nonly ever investigates paths that have undergone pathspec\nnormalization, after which there are only forward slashes even on\nWindows.\n\nSigned-off-by: Johannes Sixt \n---\n diff.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex f0c7557b40..d18eb198f2 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4267,12 +4267,12 @@ static void diff_fill_oid_info(struct diff_filespec *one)\n static void strip_prefix(int prefix_length, const char **namep, const char **otherp)\n {\n \t/* Strip the prefix but do not molest /dev/null and absolute paths */\n-\tif (*namep && **namep != '/') {\n+\tif (*namep && !is_absolute_path(*namep)) {\n \t\t*namep += prefix_length;\n \t\tif (**namep == '/')\n \t\t\t++*namep;\n \t}\n-\tif (*otherp && **otherp != '/') {\n+\tif (*otherp && !is_absolute_path(*otherp)) {\n \t\t*otherp += prefix_length;\n \t\tif (**otherp == '/')\n \t\t\t++*otherp;\n-- \n2.19.1.406.g1aa3f475f3\n"},{"id":"360864","messageId":"CAGZ79kYChLNDB_f1KR2k7G9FvHkX7x_yXdhxMRpLFWpRR0SoPg@mail.gmail.com","threadId":"49551","inReplyTo":"ae6fc699-6e09-2979-40dc-9cc49f4f8365@kdbg.org","subject":"Re: [PATCH] diff: don't attempt to strip prefix from absolute Windows paths","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-18T18:49:58Z","receivedAt":"2018-10-18T18:50:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 18, 2018 at 11:38 AM Johannes Sixt wrote:\n\n> There is one peculiarity, though: [...]\n\nThe explanation makes sense, and the code looks good.\nDo we want to have a test for this niche case?\n"},{"id":"360869","messageId":"a9a51e28-208d-d8dd-a868-bc2f8cc79598@kdbg.org","threadId":"49551","inReplyTo":"CAGZ79kYChLNDB_f1KR2k7G9FvHkX7x_yXdhxMRpLFWpRR0SoPg@mail.gmail.com","subject":"Re: [PATCH] diff: don't attempt to strip prefix from absolute Windows paths","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-10-18T19:11:48Z","receivedAt":"2018-10-18T19:11:52Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 18.10.18 um 20:49 schrieb Stefan Beller:\n> On Thu, Oct 18, 2018 at 11:38 AM Johannes Sixt wrote:\n> \n>> There is one peculiarity, though: [...]\n> \n> The explanation makes sense, and the code looks good.\n> Do we want to have a test for this niche case?\n> \n\nGood point. That would be the following. But give me a day or two to\ncross-check on Windows and whether it really catches the breakage.\n\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 453e6c35eb..6e0dd6f9e5 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -127,4 +127,14 @@ test_expect_success 'diff --no-index from repo subdir respects config (implicit)\n \ttest_cmp expect actual.head\n '\n \n+test_expect_success 'diff --no-index from repo subdir with absolute paths' '\n+\tcat <<-EOF >expect &&\n+\t1\t1\t$(pwd)/non/git/{a => b}\n+\tEOF\n+\ttest_expect_code 1 \\\n+\t\tgit -C repo/sub diff --numstat \\\n+\t\t\"$(pwd)/non/git/a\" \"$(pwd)/non/git/b\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n"},{"id":"360890","messageId":"xmqqa7na4t40.fsf@gitster-ct.c.googlers.com","threadId":"49551","inReplyTo":"ae6fc699-6e09-2979-40dc-9cc49f4f8365@kdbg.org","subject":"Re: [PATCH] diff: don't attempt to strip prefix from absolute Windows paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-19T01:34:23Z","receivedAt":"2018-10-19T01:34:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Use is_absolute_path() to detect Windows style absolute paths.\n\nWhen cd676a51 (\"diff --relative: output paths as relative to the\ncurrent subdirectory\", 2008-02-12) was done, neither \"is_dir_sep()\"\nnor \"has_dos_drive_prefix()\" existed---the latter had to wait until\n25fe217b (\"Windows: Treat Windows style path names.\", 2008-03-05),\nbut we didn't notice that the above code needs to use the Windows\naware is_absolute_path() when we applied the change.\n\n> One might wonder whether the check for a directory separator that\n> is visible in the patch context should be changed from == '/' to\n> is_dir_sep() or not. It turns out not to be necessary. That code\n> only ever investigates paths that have undergone pathspec\n> normalization, after which there are only forward slashes even on\n> Windows.\n\nThanks for carefully explaining.\n\n> static void strip_prefix(int prefix_length, const char **namep, const char **otherp)\n> {\n> \t/* Strip the prefix but do not molest /dev/null and absolute paths */\n> -\tif (*namep && **namep != '/') {\n> +\tif (*namep && !is_absolute_path(*namep)) {\n> \t\t*namep += prefix_length;\n> \t\tif (**namep == '/')\n> \t\t\t++*namep;\n> \t}\n> -\tif (*otherp && **otherp != '/') {\n> +\tif (*otherp && !is_absolute_path(*otherp)) {\n> \t\t*otherp += prefix_length;\n> \t\tif (**otherp == '/')\n> \t\t\t++*otherp;\n\nWhen I read the initial report and guessed the root cause without\nlooking at the code, I didn't expect the problematic area to be this\nisolated and the solution to be this simple.\n\nNicely done.\n"},{"id":"360953","messageId":"2ca9d427-38fb-ff6e-4e2f-1947d86e6273@kdbg.org","threadId":"49551","inReplyTo":"a9a51e28-208d-d8dd-a868-bc2f8cc79598@kdbg.org","subject":"[PATCH v2] diff: don't attempt to strip prefix from absolute Windows paths","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-10-19T16:58:07Z","receivedAt":"2018-10-19T16:58:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"git diff can be invoked with absolute paths. Typically, this triggers\nthe --no-index case. Then the absolute paths remain in the file names\nthat are printed in the output.\n\nThere is one peculiarity, though: When the command is invoked from a\na sub-directory in a repository, then it is attempted to strip the\nsub-directory from the beginning of relative paths. Yet, to detect a\nrelative path the code just checks for an initial forward slash.\nThis mistakes a Windows style path like \"D:/base\" as a relative path\nand the output looks like this, for example:\n\n D:\\dir\\test\\one>git -P diff --numstat D:\\dir\\base D:\\dir\\diff\n 1 1 ir/{base => diff}/1.txt\n\nwhere the correct output should be\n\n D:\\dir\\test\\one>git -P diff --numstat D:\\dir\\base D:\\dir\\diff\n 1 1 D:/dir/{base => diff}/1.txt\n\nIf the sub-directory where 'git diff' is invoked is sufficiently deep\nthat the prefix becomes longer than the path to be printed, then the\nsubsequent code accesses the path out of bounds.\n\nUse is_absolute_path() to detect Windows style absolute paths.\n\nOne might wonder whether the check for a directory separator that\nis visible in the patch context should be changed from == '/' to\nis_dir_sep() or not. It turns out not to be necessary. That code\nonly ever investigates paths that have undergone pathspec\nnormalization, after which there are only forward slashes even on\nWindows.\n\nSigned-off-by: Johannes Sixt \n---\nv2:\n- added a test that demonstrates the problem on Windows\n- changed the example in the commit message to clarify that this is\n about truncated paths, not about failure to detect common prefix\n\n diff.c | 4 ++--\n t/t4053-diff-no-index.sh | 10 ++++++++++\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex f0c7557b40..d18eb198f2 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4267,12 +4267,12 @@ static void diff_fill_oid_info(struct diff_filespec *one)\n static void strip_prefix(int prefix_length, const char **namep, const char **otherp)\n {\n \t/* Strip the prefix but do not molest /dev/null and absolute paths */\n-\tif (*namep && **namep != '/') {\n+\tif (*namep && !is_absolute_path(*namep)) {\n \t\t*namep += prefix_length;\n \t\tif (**namep == '/')\n \t\t\t++*namep;\n \t}\n-\tif (*otherp && **otherp != '/') {\n+\tif (*otherp && !is_absolute_path(*otherp)) {\n \t\t*otherp += prefix_length;\n \t\tif (**otherp == '/')\n \t\t\t++*otherp;\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 453e6c35eb..6e0dd6f9e5 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -127,4 +127,14 @@ test_expect_success 'diff --no-index from repo subdir respects config (implicit)\n \ttest_cmp expect actual.head\n '\n \n+test_expect_success 'diff --no-index from repo subdir with absolute paths' '\n+\tcat <<-EOF >expect &&\n+\t1\t1\t$(pwd)/non/git/{a => b}\n+\tEOF\n+\ttest_expect_code 1 \\\n+\t\tgit -C repo/sub diff --numstat \\\n+\t\t\"$(pwd)/non/git/a\" \"$(pwd)/non/git/b\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.19.1.406.g1aa3f475f3\n"},{"id":"360954","messageId":"CAGZ79kZ=u8UrLbbrAu1PnOg57bqx0wuzfx4P3Ki60LmfUTo4Jw@mail.gmail.com","threadId":"49551","inReplyTo":"2ca9d427-38fb-ff6e-4e2f-1947d86e6273@kdbg.org","subject":"Re: [PATCH v2] diff: don't attempt to strip prefix from absolute Windows paths","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-19T17:04:17Z","receivedAt":"2018-10-19T17:04:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> +test_expect_success 'diff --no-index from repo subdir with absolute paths' '\n\nI was late looking at the test, and was about to propose to guard it to run only\non Windows (as this test seems of little use in other OS), but after thinking\nabout it I think we should keep it as-is, as there may be other OS that have\ninteresting absolute path which I may be unaware of.\n\nReviewed-by: Stefan Beller \n"}]}