{"thread":{"id":"50249","subject":"git add —ignore-errors causes --renormalize","startedAt":"2019-01-17T15:22:14Z","lastAt":"2019-01-29T22:51:27Z","messageCount":6,"participants":["Dmitriy Smirnov","Jeff King","Torsten Bögershausen","SZEDER Gábor"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"366996","messageId":"401130AF-056B-40BF-8001-8492223B8A6B@jetbrains.com","threadId":"50249","inReplyTo":null,"subject":"git add —ignore-errors causes --renormalize","fromName":"Dmitriy Smirnov","fromEmail":"dmitriy.smirnov@jetbrains.com","sentAt":"2019-01-17T15:22:05Z","receivedAt":"2019-01-17T15:22:14Z","isPatch":false,"sender":{"key":"dmitriy.smirnov@jetbrains.com","avatar":null},"body":"Hello,\n\nCalling `git add —ignore-errors` appears to be equal to calling `git add —renormalize`:\n\nMain.java is saved with CRLF in repo\ngit config core.autocrlf = input\n\n$ src  git:(master) echo line >> Main.java  \n$ src  git:(master) git add --ignore-errors Main.java  \n$ src  git:(master) git commit -m \"Ignore errors\"                          \n[master cf24b3b] Ignore errors\n 1 file changed, 13 insertions(+), 12 deletions(-)\n\n\nThe reason appears to be wrong bit mask usage\n\n#define ADD_CACHE_IGNORE_ERRORS    4\nand\n#define HASH_RENORMALIZE  4\n\n\nLooks like a regression since 2.16.0 - 9472935d81eaf9faed771878c9df0216ae0d9045"},{"id":"367000","messageId":"20190117162711.GA7935@sigill.intra.peff.net","threadId":"50249","inReplyTo":"401130AF-056B-40BF-8001-8492223B8A6B@jetbrains.com","subject":"Re: git add —ignore-errors causes --renormalize","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-17T16:27:11Z","receivedAt":"2019-01-17T16:27:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 17, 2019 at 06:22:05PM +0300, Dmitriy Smirnov wrote:\n\n> Calling `git add —ignore-errors` appears to be equal to calling `git add —renormalize`:\n> \n> Main.java is saved with CRLF in repo\n> git config core.autocrlf = input\n> \n> $ src  git:(master) echo line >> Main.java  \n> $ src  git:(master) git add --ignore-errors Main.java  \n> $ src  git:(master) git commit -m \"Ignore errors\"                          \n> [master cf24b3b] Ignore errors\n>  1 file changed, 13 insertions(+), 12 deletions(-)\n> \n> \n> The reason appears to be wrong bit mask usage\n> \n> #define ADD_CACHE_IGNORE_ERRORS    4\n> and\n> #define HASH_RENORMALIZE  4\n> \n> Looks like a regression since 2.16.0 - 9472935d81eaf9faed771878c9df0216ae0d9045\n\nThanks for a very clear report! The patch below should fix it.\n\n-- >8 --\nSubject: [PATCH] add: use separate ADD_CACHE_RENORMALIZE flag\n\nCommit 9472935d81 (add: introduce \"--renormalize\", 2017-11-16) taught\ngit-add to pass HASH_RENORMALIZE to add_to_index(), which then passes\nthe flag along to index_path(). However, the flags taken by\nadd_to_index() and the ones taken by index_path() are distinct\nnamespaces. We cannot take HASH_* flags in add_to_index(), because they\noverlap with the ADD_CACHE_* flags we already take (in this case,\nHASH_RENORMALIZE conflicts with ADD_CACHE_IGNORE_ERRORS).\n\nWe can solve this by adding a new ADD_CACHE_RENORMALIZE flag, and using\nit to set HASH_RENORMALIZE within add_to_index(). In order to make it\nclear that these two flags come from distinct sets, let's also change\nthe name \"newflags\" in the function to \"hash_flags\".\n\nReported-by: Dmitriy Smirnov <dmitriy.smirnov@jetbrains.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/add.c               | 2 +-\n cache.h                     | 1 +\n read-cache.c                | 8 ++++----\n t/t0025-crlf-renormalize.sh | 9 +++++++++\n 4 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 12247b48fd..f481ae548e 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -137,7 +137,7 @@ static int renormalize_tracked_files(const struct pathspec *pathspec, int flags)\n \t\t\tcontinue; /* do not touch non blobs */\n \t\tif (pathspec && !ce_path_match(&the_index, ce, pathspec, NULL))\n \t\t\tcontinue;\n-\t\tretval |= add_file_to_cache(ce->name, flags | HASH_RENORMALIZE);\n+\t\tretval |= add_file_to_cache(ce->name, flags | ADD_CACHE_RENORMALIZE);\n \t}\n \n \treturn retval;\ndiff --git a/cache.h b/cache.h\nindex 49713cc5a5..11b19505fc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -745,6 +745,7 @@ extern int index_name_pos(const struct index_state *, const char *name, int name\n #define ADD_CACHE_JUST_APPEND 8\t\t/* Append only; tree.c::read_tree() */\n #define ADD_CACHE_NEW_ONLY 16\t\t/* Do not replace existing ones */\n #define ADD_CACHE_KEEP_CACHE_TREE 32\t/* Do not invalidate cache-tree */\n+#define ADD_CACHE_RENORMALIZE 64        /* Pass along HASH_RENORMALIZE */\n extern int add_index_entry(struct index_state *, struct cache_entry *ce, int option);\n extern void rename_index_entry_at(struct index_state *, int pos, const char *new_name);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex bfff271a3d..9783c493a3 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -703,10 +703,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \tint intent_only = flags & ADD_CACHE_INTENT;\n \tint add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|\n \t\t\t  (intent_only ? ADD_CACHE_NEW_ONLY : 0));\n-\tint newflags = HASH_WRITE_OBJECT;\n+\tint hash_flags = HASH_WRITE_OBJECT;\n \n-\tif (flags & HASH_RENORMALIZE)\n-\t\tnewflags |= HASH_RENORMALIZE;\n+\tif (flags & ADD_CACHE_RENORMALIZE)\n+\t\thash_flags |= HASH_RENORMALIZE;\n \n \tif (!S_ISREG(st_mode) && !S_ISLNK(st_mode) && !S_ISDIR(st_mode))\n \t\treturn error(_(\"%s: can only add regular files, symbolic links or git-directories\"), path);\n@@ -762,7 +762,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \t\t}\n \t}\n \tif (!intent_only) {\n-\t\tif (index_path(istate, &ce->oid, path, st, newflags)) {\n+\t\tif (index_path(istate, &ce->oid, path, st, hash_flags)) {\n \t\t\tdiscard_cache_entry(ce);\n \t\t\treturn error(_(\"unable to index file '%s'\"), path);\n \t\t}\ndiff --git a/t/t0025-crlf-renormalize.sh b/t/t0025-crlf-renormalize.sh\nindex 9d9e02a211..e13363ade5 100755\n--- a/t/t0025-crlf-renormalize.sh\n+++ b/t/t0025-crlf-renormalize.sh\n@@ -27,4 +27,13 @@ test_expect_success 'renormalize CRLF in repo' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ignore-errors not mistaken for renormalize' '\n+\tgit reset --hard &&\n+\techo \"*.txt text=auto\" >.gitattributes &&\n+\tgit ls-files --eol >expect &&\n+\tgit add --ignore-errors \"*.txt\" &&\n+\tgit ls-files --eol >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.20.1.689.g635a1dda8a\n\n"},{"id":"367002","messageId":"20190117164507.GA27819@sigill.intra.peff.net","threadId":"50249","inReplyTo":"20190117162711.GA7935@sigill.intra.peff.net","subject":"Re: git add —ignore-errors causes --renormalize","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-17T16:45:07Z","receivedAt":"2019-01-17T16:45:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 17, 2019 at 11:27:11AM -0500, Jeff King wrote:\n\n> -- >8 --\n> Subject: [PATCH] add: use separate ADD_CACHE_RENORMALIZE flag\n> \n> Commit 9472935d81 (add: introduce \"--renormalize\", 2017-11-16) taught\n> git-add to pass HASH_RENORMALIZE to add_to_index(), which then passes\n> the flag along to index_path(). However, the flags taken by\n> add_to_index() and the ones taken by index_path() are distinct\n> namespaces. We cannot take HASH_* flags in add_to_index(), because they\n> overlap with the ADD_CACHE_* flags we already take (in this case,\n> HASH_RENORMALIZE conflicts with ADD_CACHE_IGNORE_ERRORS).\n> \n> We can solve this by adding a new ADD_CACHE_RENORMALIZE flag, and using\n> it to set HASH_RENORMALIZE within add_to_index(). In order to make it\n> clear that these two flags come from distinct sets, let's also change\n> the name \"newflags\" in the function to \"hash_flags\".\n\nBy the way, I wondered if there was a good way for the compiler to help\nus find an error like this. There's no type-checking here, since all of\nthe flags are \"int\", with the values #define macros. Could enums give us\nbetter safety?\n\nMy experiments suggest no, since the compiler is pretty loose about what\nit will allow, even with -Wenum-compare. In particular, I think it's\nhappy to allow bitwise-AND even against tags from other enums. But if\nsomebody can figure out a way to make it work, I'm all ears. :)\n\nA more drastic option is to replace the flag int with a struct\ncontaining a bitfield. I.e., something like:\n\n  struct add_cache_flags {\n\tunsigned ignore_errors : 1;\n\tunsigned renormalize : 1;\n\t... etc ...\n  };\n\nThat gives us real type safety and eliminates the need to manually\nassign numbers to each flag. But there are downsides:\n\n  - it's syntactically more awkward to modify the flags. I.e.:\n\n      foo(flags | BAR);\n\n    becomes:\n\n      struct flags new_flags = old_flags;\n      new_flags.bar = 1;\n      foo(&new_flags);\n\n    There might be some C99-isms that can help us out (though I suspect\n    not completely -- even if we can use anonymous structs, I don't\n    think there's such a thing as \"initialize this anonymous struct and\n    then modify these fields\").\n\n  - there's no grouping, so you can't mask off certain values\n\nSo I dunno. Maybe that road is too painful, and we're stuck with what C\ngives us.\n\n-Peff\n"},{"id":"367177","messageId":"7fb24fe7-4d14-a2ba-f8b6-94c88cb0ecde@web.de","threadId":"50249","inReplyTo":"20190117162711.GA7935@sigill.intra.peff.net","subject":"Re: git add —ignore-errors causes --renormalize","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2019-01-19T17:08:11Z","receivedAt":"2019-01-19T17:08:18Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 17.01.19 17:27, Jeff King wrote:\n> On Thu, Jan 17, 2019 at 06:22:05PM +0300, Dmitriy Smirnov wrote:\n>\n>> Calling `git add —ignore-errors` appears to be equal to calling `git add —renormalize`:\n>>\n>> Main.java is saved with CRLF in repo\n>> git config core.autocrlf = input\n>>\n>> $ src  git:(master) echo line >> Main.java\n>> $ src  git:(master) git add --ignore-errors Main.java\n>> $ src  git:(master) git commit -m \"Ignore errors\"\n>> [master cf24b3b] Ignore errors\n>>  1 file changed, 13 insertions(+), 12 deletions(-)\n>>\n>>\n>> The reason appears to be wrong bit mask usage\n>>\n>> #define ADD_CACHE_IGNORE_ERRORS    4\n>> and\n>> #define HASH_RENORMALIZE  4\n>>\n>> Looks like a regression since 2.16.0 - 9472935d81eaf9faed771878c9df0216ae0d9045\n>\n> Thanks for a very clear report! The patch below should fix it.\n\n...\n\nThanks for cleaning up my mess\n\n\n"},{"id":"368050","messageId":"20190129213533.GE13764@szeder.dev","threadId":"50249","inReplyTo":"20190117162711.GA7935@sigill.intra.peff.net","subject":"Re: git add —ignore-errors causes --renormalize","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-29T21:35:33Z","receivedAt":"2019-01-29T21:35:40Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Jan 17, 2019 at 11:27:11AM -0500, Jeff King wrote:\n> On Thu, Jan 17, 2019 at 06:22:05PM +0300, Dmitriy Smirnov wrote:\n> \n> > Calling `git add —ignore-errors` appears to be equal to calling `git add —renormalize`:\n> > \n> > Main.java is saved with CRLF in repo\n> > git config core.autocrlf = input\n> > \n> > $ src  git:(master) echo line >> Main.java  \n> > $ src  git:(master) git add --ignore-errors Main.java  \n> > $ src  git:(master) git commit -m \"Ignore errors\"                          \n> > [master cf24b3b] Ignore errors\n> >  1 file changed, 13 insertions(+), 12 deletions(-)\n> > \n> > \n> > The reason appears to be wrong bit mask usage\n> > \n> > #define ADD_CACHE_IGNORE_ERRORS    4\n> > and\n> > #define HASH_RENORMALIZE  4\n> > \n> > Looks like a regression since 2.16.0 - 9472935d81eaf9faed771878c9df0216ae0d9045\n> \n> Thanks for a very clear report! The patch below should fix it.\n> \n> -- >8 --\n> Subject: [PATCH] add: use separate ADD_CACHE_RENORMALIZE flag\n> \n> Commit 9472935d81 (add: introduce \"--renormalize\", 2017-11-16) taught\n> git-add to pass HASH_RENORMALIZE to add_to_index(), which then passes\n> the flag along to index_path(). However, the flags taken by\n> add_to_index() and the ones taken by index_path() are distinct\n> namespaces. We cannot take HASH_* flags in add_to_index(), because they\n> overlap with the ADD_CACHE_* flags we already take (in this case,\n> HASH_RENORMALIZE conflicts with ADD_CACHE_IGNORE_ERRORS).\n> \n> We can solve this by adding a new ADD_CACHE_RENORMALIZE flag, and using\n> it to set HASH_RENORMALIZE within add_to_index(). In order to make it\n> clear that these two flags come from distinct sets, let's also change\n> the name \"newflags\" in the function to \"hash_flags\".\n> \n> Reported-by: Dmitriy Smirnov <dmitriy.smirnov@jetbrains.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nt0025 with '--stress' usually fails within seconds when run on the\nmerge of 'jk/add-ignore-errors-bit-assignment-fix' and 'master' [1]:\n\n  + echo *.txt text=auto\n  + git add --renormalize *.txt\n  + cat\n  + sed -e s/     / /g -e s/  */ /g\n  + sort\n  + git ls-files --eol\n  + test_cmp expect actual\n  + diff -u expect actual\n  --- expect      2019-01-29 21:27:34.043344898 +0000\n  +++ actual      2019-01-29 21:27:34.055345252 +0000\n  @@ -1,3 +1,3 @@\n  -i/lf w/crlf attr/text=auto CRLF.txt\n  +i/crlf w/crlf attr/text=auto CRLF.txt\n   i/lf w/lf attr/text=auto LF.txt\n  -i/lf w/mixed attr/text=auto CRLF_mix_LF.txt\n  +i/mixed w/mixed attr/text=auto CRLF_mix_LF.txt\n  error: last command exited with $?=1\n  not ok 2 - renormalize CRLF in repo\n  #       \n  #               echo \"*.txt text=auto\" >.gitattributes &&\n  #               git add --renormalize \"*.txt\" &&\n  #               cat >expect <<-\\EOF &&\n  #               i/lf w/crlf attr/text=auto CRLF.txt\n  #               i/lf w/lf attr/text=auto LF.txt\n  #               i/lf w/mixed attr/text=auto CRLF_mix_LF.txt\n  #               EOF\n  #               git ls-files --eol |\n  #               sed -e \"s/      / /g\" -e \"s/  */ /g\" |\n  #               sort >actual &&\n  #               test_cmp expect actual\n  #       \n\n[1] 'jk/add-ignore-errors-bit-assignment-fix' doesn't contains\n    '--stress' yet, hence the merge to 'master'.\n\n> ---\n>  builtin/add.c               | 2 +-\n>  cache.h                     | 1 +\n>  read-cache.c                | 8 ++++----\n>  t/t0025-crlf-renormalize.sh | 9 +++++++++\n>  4 files changed, 15 insertions(+), 5 deletions(-)\n> \n> diff --git a/builtin/add.c b/builtin/add.c\n> index 12247b48fd..f481ae548e 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -137,7 +137,7 @@ static int renormalize_tracked_files(const struct pathspec *pathspec, int flags)\n>  \t\t\tcontinue; /* do not touch non blobs */\n>  \t\tif (pathspec && !ce_path_match(&the_index, ce, pathspec, NULL))\n>  \t\t\tcontinue;\n> -\t\tretval |= add_file_to_cache(ce->name, flags | HASH_RENORMALIZE);\n> +\t\tretval |= add_file_to_cache(ce->name, flags | ADD_CACHE_RENORMALIZE);\n>  \t}\n>  \n>  \treturn retval;\n> diff --git a/cache.h b/cache.h\n> index 49713cc5a5..11b19505fc 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -745,6 +745,7 @@ extern int index_name_pos(const struct index_state *, const char *name, int name\n>  #define ADD_CACHE_JUST_APPEND 8\t\t/* Append only; tree.c::read_tree() */\n>  #define ADD_CACHE_NEW_ONLY 16\t\t/* Do not replace existing ones */\n>  #define ADD_CACHE_KEEP_CACHE_TREE 32\t/* Do not invalidate cache-tree */\n> +#define ADD_CACHE_RENORMALIZE 64        /* Pass along HASH_RENORMALIZE */\n>  extern int add_index_entry(struct index_state *, struct cache_entry *ce, int option);\n>  extern void rename_index_entry_at(struct index_state *, int pos, const char *new_name);\n>  \n> diff --git a/read-cache.c b/read-cache.c\n> index bfff271a3d..9783c493a3 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -703,10 +703,10 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n>  \tint intent_only = flags & ADD_CACHE_INTENT;\n>  \tint add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|\n>  \t\t\t  (intent_only ? ADD_CACHE_NEW_ONLY : 0));\n> -\tint newflags = HASH_WRITE_OBJECT;\n> +\tint hash_flags = HASH_WRITE_OBJECT;\n>  \n> -\tif (flags & HASH_RENORMALIZE)\n> -\t\tnewflags |= HASH_RENORMALIZE;\n> +\tif (flags & ADD_CACHE_RENORMALIZE)\n> +\t\thash_flags |= HASH_RENORMALIZE;\n>  \n>  \tif (!S_ISREG(st_mode) && !S_ISLNK(st_mode) && !S_ISDIR(st_mode))\n>  \t\treturn error(_(\"%s: can only add regular files, symbolic links or git-directories\"), path);\n> @@ -762,7 +762,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n>  \t\t}\n>  \t}\n>  \tif (!intent_only) {\n> -\t\tif (index_path(istate, &ce->oid, path, st, newflags)) {\n> +\t\tif (index_path(istate, &ce->oid, path, st, hash_flags)) {\n>  \t\t\tdiscard_cache_entry(ce);\n>  \t\t\treturn error(_(\"unable to index file '%s'\"), path);\n>  \t\t}\n> diff --git a/t/t0025-crlf-renormalize.sh b/t/t0025-crlf-renormalize.sh\n> index 9d9e02a211..e13363ade5 100755\n> --- a/t/t0025-crlf-renormalize.sh\n> +++ b/t/t0025-crlf-renormalize.sh\n> @@ -27,4 +27,13 @@ test_expect_success 'renormalize CRLF in repo' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'ignore-errors not mistaken for renormalize' '\n> +\tgit reset --hard &&\n> +\techo \"*.txt text=auto\" >.gitattributes &&\n> +\tgit ls-files --eol >expect &&\n> +\tgit add --ignore-errors \"*.txt\" &&\n> +\tgit ls-files --eol >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n> -- \n> 2.20.1.689.g635a1dda8a\n> \n"},{"id":"368063","messageId":"20190129225121.GD1895@sigill.intra.peff.net","threadId":"50249","inReplyTo":"20190129213533.GE13764@szeder.dev","subject":"Re: git add —ignore-errors causes --renormalize","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-29T22:51:22Z","receivedAt":"2019-01-29T22:51:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 29, 2019 at 10:35:33PM +0100, SZEDER Gábor wrote:\n\n> t0025 with '--stress' usually fails within seconds when run on the\n> merge of 'jk/add-ignore-errors-bit-assignment-fix' and 'master' [1]:\n\nThanks for the pointer. I mostly cargo-culted the test from the rest of\nthat script, so I didn't think too hard about race conditions. I think\nthe key difference is that the other tests are _actually_ using\n--renormalize, which is going to re-hash the files and notice the line\nending change. And my test is not, so it will sometimes notice and\nsometimes not.\n\nIt seems like we could probably solve it through some clever tweaking of\nmtimes to make sure we have a consistent timestamp setup, but it's\nresisting my quick efforts. I'm at an airport traveling right now, so I\nmay not get to it for a day or three.\n\n-Peff\n"}]}