{"thread":{"id":"52129","subject":"[PATCH 0/1] add: respect --ignore-errors when lstat() reports errors","startedAt":"2019-10-26T21:59:49Z","lastAt":"2019-11-03T20:27:14Z","messageCount":6,"participants":["Qusielle via GitGitGadget","qusielle via GitGitGadget","Junio C Hamano","qusielle@gmail.com"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"384922","messageId":"pull.432.git.1572127149.gitgitgadget@gmail.com","threadId":"52129","inReplyTo":null,"subject":"[PATCH 0/1] add: respect --ignore-errors when lstat() reports errors","fromName":"Qusielle via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-26T21:59:08Z","receivedAt":"2019-10-26T21:59:49Z","isPatch":true,"sender":{"key":"name:Qusielle","avatar":null},"body":"\"git add --ignore-errors\" command fails immediately when lstat returns an\nerror, despite the ignore errors' flag is enabled.\n\nThere could be files that triggers an error on stat(), when other files\nproceed correctly. Issue can be reproduced when running git under Cygwin and\nsome target files have utf-8 long names (200+ utf chars). Windows can handle\nthem, but all operations on them failed under Cygwin. Issue can not be\nreproduced with usual latin/numeric only names. For example, create a file\nwith 220 'й' letters by Windows Explorer, then in Cygwin:\n\nHere and below \"ййй...\" means the line of й copied 220 times.\n=============================================================\n\n$ echo -n 'ййй...' | wc -c # check the real size 440\n\n$ ls -la ls: cannot access 'ййй...'$'\\320': No such file or directory\n-????????? ? ? ? ? ? 'ййй...'$'\\320'\n\n$ ls й* ls: cannot access 'ййй...'$'\\320': No such file or directory\n\n$ stat й* stat: cannot stat 'ййй...'$'\\320': No such file or directory\n\nIn my perspective, it's okay to skip these problematic files when ignore\nerror flag is specified, but official Git terminates entire git add command\nwhen such files come up. But with proposed patch it is the desired behavior:\n\n$ git add --ignore-errors . error: ййй... can only add regular files,\nsymbolic links or git-directories\n\nAll other files have been added correctly.\n==========================================\n\nSigned-off-by: Qusielle <qusielle@gmail.com>\n\nqusielle (1):\n  add: respect `--ignore-errors` when `lstat()` reports errors\n\n read-cache.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\n\nbase-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-432%2Fqusielle%2Fmaster-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-432/qusielle/master-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/432\n-- \ngitgitgadget\n"},{"id":"384923","messageId":"fd022f88f54f6cf0feb61965b2dc47bca64c0937.1572127149.git.gitgitgadget@gmail.com","threadId":"52129","inReplyTo":"pull.432.git.1572127149.gitgitgadget@gmail.com","subject":"[PATCH 1/1] add: respect `--ignore-errors` when `lstat()` reports errors","fromName":"qusielle via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-26T21:59:09Z","receivedAt":"2019-10-26T21:59:49Z","isPatch":true,"sender":{"key":"name:qusielle","avatar":null},"body":"From: qusielle <31454380+qusielle@users.noreply.github.com>\n\n\"git add --ignore-errors\" command fails immediately when lstat returns\nan error, despite the ignore errors' flag is enabled.\n\nThere could be files that triggers an error on stat(), when other files\nproceed correctly.\nIssue can be reproduced when running git under Cygwin and some target files\nhave utf-8 long names (200+ utf chars). Windows can handle them, but all\noperations on them failed under Cygwin.\nIssue can not be reproduced with usual latin/numeric only names.\nFor example, create a file with 220 'й' letters by Windows Explorer,\nthen in Cygwin:\n\n # Here and below \"ййй...\" means the line of й copied 220 times.\n$ echo -n 'ййй...' | wc -c  # check the real size\n440\n\n$ ls -la\nls: cannot access 'ййй...'$'\\320': No such file or directory\n-????????? ? ?              ?        ?            ? 'ййй...'$'\\320'\n\n$ ls й*\nls: cannot access 'ййй...'$'\\320': No such file or directory\n\n$ stat й*\nstat: cannot stat 'ййй...'$'\\320': No such file or directory\n\nIn my perspective, it's okay to skip these problematic files when ignore\nerror flag is specified, but official Git terminates entire git add command\nwhen such files come up. But with proposed patch it is the desired behavior:\n\n$ git add --ignore-errors .\nerror: ййй... can only add regular files, symbolic links or git-directories\n # All other files have been added correctly.\n\nSigned-off-by: Qusielle <qusielle@gmail.com>\n---\n read-cache.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 133f790fa4..67237ecd29 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -801,7 +801,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n int add_file_to_index(struct index_state *istate, const char *path, int flags)\n {\n \tstruct stat st;\n-\tif (lstat(path, &st))\n+\tif (lstat(path, &st) && !(flags & ADD_CACHE_IGNORE_ERRORS))\n \t\tdie_errno(_(\"unable to stat '%s'\"), path);\n \treturn add_to_index(istate, path, &st, flags);\n }\n-- \ngitgitgadget\n"},{"id":"384959","messageId":"xmqq36fdbp8v.fsf@gitster-ct.c.googlers.com","threadId":"52129","inReplyTo":"fd022f88f54f6cf0feb61965b2dc47bca64c0937.1572127149.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] add: respect `--ignore-errors` when `lstat()` reports errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-28T02:03:28Z","receivedAt":"2019-10-28T02:03:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"qusielle via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: qusielle <31454380+qusielle@users.noreply.github.com>\n>\n> \"git add --ignore-errors\" command fails immediately when lstat returns\n> an error, despite the ignore errors' flag is enabled.\n> ...\n> diff --git a/read-cache.c b/read-cache.c\n> index 133f790fa4..67237ecd29 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -801,7 +801,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n>  int add_file_to_index(struct index_state *istate, const char *path, int flags)\n>  {\n>  \tstruct stat st;\n> -\tif (lstat(path, &st))\n> +\tif (lstat(path, &st) && !(flags & ADD_CACHE_IGNORE_ERRORS))\n>  \t\tdie_errno(_(\"unable to stat '%s'\"), path);\n>  \treturn add_to_index(istate, path, &st, flags);\n>  }\n\nThe only callers of this function that matter calls it and then\nresponds to an error return like so:\n\n(in builtin/add.c::update_callback())\n\n\tif (add_file_to_index(&the_index, path,\tdata->flags)) {\n\t\tif (!(data->flags & ADD_CACHE_IGNORE_ERRORS))\n\t\t\tdie(_(\"updating files failed\"));\n\n\n(in builtin/add.c::add_files(), where ignore_add_errors was used to\nset the ADD_CACHE_IGNORE_ERRORS to flags in its caller)\n\n\tif (add_file_to_index(&the_index, dir->entries[i]->name, flags)) {\n\t\tif (!ignore_add_errors)\n\t\t\tdie(_(\"adding files failed\"));\n\nSo you correctly identified what is the right place to fix.  We\nshould not \"die_errno()\"; we should give the control back to the\ncaller instead.\n\nBut after a failed stat, the code with your patch still calls\nadd_to_index() using the now undefined stat data, which would\ncontaminate the in-core index with wrong data.  \n\nI think we should instead return without touching the index for the\npath we had trouble lstat()ing.\n\nIOW\n\n\tif (lstat(path, &st)) {\n\t\tif (flags & ADD_CACHE_IGNORE_ERRORS)\n\t\t\treturn -1;\n\t\telse\n\t\t\tdie_errno(_(\"unable to ...\"));\n\t}\n\treturn add_to_index(...);\n\n"},{"id":"385352","messageId":"eac0456e-0f61-9424-975d-0567bb92327d@gmail.com","threadId":"52129","inReplyTo":"xmqq36fdbp8v.fsf@gitster-ct.c.googlers.com","subject":"Re: Re: [PATCH 1/1] add: respect `--ignore-errors` when `lstat()` reports errors","fromName":"qusielle@gmail.com","fromEmail":"qusielle@gmail.com","sentAt":"2019-11-03T20:17:10Z","receivedAt":"2019-11-03T20:17:16Z","isPatch":true,"sender":{"key":"qusielle@gmail.com","avatar":null},"body":"Dear Junio,\n\nThank you for reviewing my patch. I completely agree with you, that \nadd_to_index() should not be called with undefined data.\n\nI will amend patch now with proposed changes.\n\nThank you!\n\nBest regards,\nQusielle\n\n\nOn 28.10.2019 03:03, Junio C Hamano wrote:\n> \"qusielle via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n>> From: qusielle <31454380+qusielle@users.noreply.github.com>\n>>\n>> \"git add --ignore-errors\" command fails immediately when lstat returns\n>> an error, despite the ignore errors' flag is enabled.\n>> ...\n>> diff --git a/read-cache.c b/read-cache.c\n>> index 133f790fa4..67237ecd29 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -801,7 +801,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n>>   int add_file_to_index(struct index_state *istate, const char *path, int flags)\n>>   {\n>>   \tstruct stat st;\n>> -\tif (lstat(path, &st))\n>> +\tif (lstat(path, &st) && !(flags & ADD_CACHE_IGNORE_ERRORS))\n>>   \t\tdie_errno(_(\"unable to stat '%s'\"), path);\n>>   \treturn add_to_index(istate, path, &st, flags);\n>>   }\n> The only callers of this function that matter calls it and then\n> responds to an error return like so:\n>\n> (in builtin/add.c::update_callback())\n>\n> \tif (add_file_to_index(&the_index, path,\tdata->flags)) {\n> \t\tif (!(data->flags & ADD_CACHE_IGNORE_ERRORS))\n> \t\t\tdie(_(\"updating files failed\"));\n>\n>\n> (in builtin/add.c::add_files(), where ignore_add_errors was used to\n> set the ADD_CACHE_IGNORE_ERRORS to flags in its caller)\n>\n> \tif (add_file_to_index(&the_index, dir->entries[i]->name, flags)) {\n> \t\tif (!ignore_add_errors)\n> \t\t\tdie(_(\"adding files failed\"));\n>\n> So you correctly identified what is the right place to fix.  We\n> should not \"die_errno()\"; we should give the control back to the\n> caller instead.\n>\n> But after a failed stat, the code with your patch still calls\n> add_to_index() using the now undefined stat data, which would\n> contaminate the in-core index with wrong data.\n>\n> I think we should instead return without touching the index for the\n> path we had trouble lstat()ing.\n>\n> IOW\n>\n> \tif (lstat(path, &st)) {\n> \t\tif (flags & ADD_CACHE_IGNORE_ERRORS)\n> \t\t\treturn -1;\n> \t\telse\n> \t\t\tdie_errno(_(\"unable to ...\"));\n> \t}\n> \treturn add_to_index(...);\n>\n>\n"},{"id":"385353","messageId":"pull.432.v2.git.1572812819.gitgitgadget@gmail.com","threadId":"52129","inReplyTo":"pull.432.git.1572127149.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] add: respect --ignore-errors when lstat() reports errors","fromName":"Qusielle via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-03T20:26:57Z","receivedAt":"2019-11-03T20:27:03Z","isPatch":true,"sender":{"key":"name:Qusielle","avatar":null},"body":"\"git add --ignore-errors\" command fails immediately when lstat returns an\nerror, despite the ignore errors' flag is enabled.\n\nThere could be files that triggers an error on stat(), when other files\nproceed correctly. Issue can be reproduced when running git under Cygwin and\nsome target files have utf-8 long names (200+ utf chars). Windows can handle\nthem, but all operations on them failed under Cygwin. Issue can not be\nreproduced with usual latin/numeric only names. For example, create a file\nwith 220 'й' letters by Windows Explorer, then in Cygwin:\n\nHere and below \"ййй...\" means the line of й copied 220 times.\n=============================================================\n\n$ echo -n 'ййй...' | wc -c # check the real size 440\n\n$ ls -la ls: cannot access 'ййй...'$'\\320': No such file or directory\n-????????? ? ? ? ? ? 'ййй...'$'\\320'\n\n$ ls й* ls: cannot access 'ййй...'$'\\320': No such file or directory\n\n$ stat й* stat: cannot stat 'ййй...'$'\\320': No such file or directory\n\nIn my perspective, it's okay to skip these problematic files when ignore\nerror flag is specified, but official Git terminates entire git add command\nwhen such files come up. But with proposed patch it is the desired behavior:\n\n$ git add --ignore-errors . error: ййй... can only add regular files,\nsymbolic links or git-directories\n\nAll other files have been added correctly.\n==========================================\n\nSigned-off-by: Qusielle <qusielle@gmail.com>\n\nqusielle (1):\n  add: respect `--ignore-errors` when `lstat()` reports errors\n\n read-cache.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\n\nbase-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-432%2Fqusielle%2Fmaster-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-432/qusielle/master-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/432\n\nRange-diff vs v1:\n\n 1:  fd022f88f5 ! 1:  d88ea544d9 add: respect `--ignore-errors` when `lstat()` reports errors\n     @@ -46,7 +46,13 @@\n       {\n       \tstruct stat st;\n      -\tif (lstat(path, &st))\n     -+\tif (lstat(path, &st) && !(flags & ADD_CACHE_IGNORE_ERRORS))\n     - \t\tdie_errno(_(\"unable to stat '%s'\"), path);\n     +-\t\tdie_errno(_(\"unable to stat '%s'\"), path);\n     ++\tif (lstat(path, &st)) {\n     ++\t\tif (flags & ADD_CACHE_IGNORE_ERRORS)\n     ++\t\t\treturn -1;\n     ++\t\telse\n     ++\t\t\tdie_errno(_(\"unable to stat '%s'\"), path);\n     ++\t}\n       \treturn add_to_index(istate, path, &st, flags);\n       }\n     + \n\n-- \ngitgitgadget\n"},{"id":"385354","messageId":"d88ea544d93ffc9b222ac3a4974df4c499e76430.1572812819.git.gitgitgadget@gmail.com","threadId":"52129","inReplyTo":"pull.432.v2.git.1572812819.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] add: respect `--ignore-errors` when `lstat()` reports errors","fromName":"qusielle via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-03T20:26:58Z","receivedAt":"2019-11-03T20:27:14Z","isPatch":true,"sender":{"key":"name:qusielle","avatar":null},"body":"From: qusielle <31454380+qusielle@users.noreply.github.com>\n\n\"git add --ignore-errors\" command fails immediately when lstat returns\nan error, despite the ignore errors' flag is enabled.\n\nThere could be files that triggers an error on stat(), when other files\nproceed correctly.\nIssue can be reproduced when running git under Cygwin and some target files\nhave utf-8 long names (200+ utf chars). Windows can handle them, but all\noperations on them failed under Cygwin.\nIssue can not be reproduced with usual latin/numeric only names.\nFor example, create a file with 220 'й' letters by Windows Explorer,\nthen in Cygwin:\n\n # Here and below \"ййй...\" means the line of й copied 220 times.\n$ echo -n 'ййй...' | wc -c  # check the real size\n440\n\n$ ls -la\nls: cannot access 'ййй...'$'\\320': No such file or directory\n-????????? ? ?              ?        ?            ? 'ййй...'$'\\320'\n\n$ ls й*\nls: cannot access 'ййй...'$'\\320': No such file or directory\n\n$ stat й*\nstat: cannot stat 'ййй...'$'\\320': No such file or directory\n\nIn my perspective, it's okay to skip these problematic files when ignore\nerror flag is specified, but official Git terminates entire git add command\nwhen such files come up. But with proposed patch it is the desired behavior:\n\n$ git add --ignore-errors .\nerror: ййй... can only add regular files, symbolic links or git-directories\n # All other files have been added correctly.\n\nSigned-off-by: Qusielle <qusielle@gmail.com>\n---\n read-cache.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 133f790fa4..791ef65bac 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -801,8 +801,12 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n int add_file_to_index(struct index_state *istate, const char *path, int flags)\n {\n \tstruct stat st;\n-\tif (lstat(path, &st))\n-\t\tdie_errno(_(\"unable to stat '%s'\"), path);\n+\tif (lstat(path, &st)) {\n+\t\tif (flags & ADD_CACHE_IGNORE_ERRORS)\n+\t\t\treturn -1;\n+\t\telse\n+\t\t\tdie_errno(_(\"unable to stat '%s'\"), path);\n+\t}\n \treturn add_to_index(istate, path, &st, flags);\n }\n \n-- \ngitgitgadget\n"}]}