{"thread":{"id":"48497","subject":"[PATCH] grep: handle corrupt index files early","startedAt":"2018-05-15T01:04:31Z","lastAt":"2018-05-23T03:19:09Z","messageCount":38,"participants":["Stefan Beller","Johannes Schindelin","Duy Nguyen","Brandon Williams","Junio C Hamano","Thomas Gummerer","Ævar Arnfjörð Bjarmason","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"347667","messageId":"20180515010425.149200-1-sbeller@google.com","threadId":"48497","inReplyTo":null,"subject":"[PATCH] grep: handle corrupt index files early","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-15T01:04:25Z","receivedAt":"2018-05-15T01:04:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Any other caller of 'repo_read_index' dies upon a negative return of\nit, so grep should, too.\n\nSigned-off-by: Stefan Beller \n---\n\nFound while reviewing the series\nhttps://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/\n\n builtin/grep.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 6e7bc76785a..69f0743619f 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,\n \t\tstrbuf_addstr(&name, repo->submodule_prefix);\n \t}\n \n-\trepo_read_index(repo);\n+\tif (repo_read_index(repo) < 0)\n+\t\tdie(\"index file corrupt\");\n \n \tfor (nr = 0; nr < repo->index->cache_nr; nr++) {\n \t\tconst struct cache_entry *ce = repo->index->cache[nr];\n-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"347695","messageId":"nycvar.QRO.7.76.6.1805151418170.77@tvgsbejvaqbjf.bet","threadId":"48497","inReplyTo":"20180515010425.149200-1-sbeller@google.com","subject":"Re: [PATCH] grep: handle corrupt index files early","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-05-15T12:18:39Z","receivedAt":"2018-05-15T12:18:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Mon, 14 May 2018, Stefan Beller wrote:\n\n> Any other caller of 'repo_read_index' dies upon a negative return of\n> it, so grep should, too.\n> \n> Signed-off-by: Stefan Beller \n> ---\n> \n> Found while reviewing the series\n> https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/\n> \n> builtin/grep.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 6e7bc76785a..69f0743619f 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,\n> \t\tstrbuf_addstr(&name, repo->submodule_prefix);\n> \t}\n> \n> -\trepo_read_index(repo);\n> +\tif (repo_read_index(repo) < 0)\n> +\t\tdie(\"index file corrupt\");\n\nLooks obviously correct, thanks!\n\nCiao,\nDscho\n"},{"id":"347696","messageId":"CACsJy8AcG6zGa9vLCwm2B4ishyJVWdFQ2YV0FOZTor_0x8Q64g@mail.gmail.com","threadId":"48497","inReplyTo":"20180515010425.149200-1-sbeller@google.com","subject":"Re: [PATCH] grep: handle corrupt index files early","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-15T13:13:35Z","receivedAt":"2018-05-15T13:14:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 15, 2018 at 3:04 AM, Stefan Beller wrote:\n> Any other caller of 'repo_read_index' dies upon a negative return of\n> it, so grep should, too.\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> Found while reviewing the series\n> https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/\n>\n> builtin/grep.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 6e7bc76785a..69f0743619f 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,\n> strbuf_addstr(&name, repo->submodule_prefix);\n> }\n>\n> - repo_read_index(repo);\n> + if (repo_read_index(repo) < 0)\n> + die(\"index file corrupt\");\n\n_() the string (and maybe reuse an existing phrase if found to reduce\nworkload on translators)\n\n>\n> for (nr = 0; nr < repo->index->cache_nr; nr++) {\n> const struct cache_entry *ce = repo->index->cache[nr];\n> --\n> 2.17.0.582.gccdcbd54c44.dirty\n>\n\n\n\n-- \nDuy\n"},{"id":"347697","messageId":"CAGZ79kYYy-r+Zs_6yg1ZVRNsBcTUdPWE=FeHrp_=0cV0Uf_GfQ@mail.gmail.com","threadId":"48497","inReplyTo":"CACsJy8AcG6zGa9vLCwm2B4ishyJVWdFQ2YV0FOZTor_0x8Q64g@mail.gmail.com","subject":"Re: [PATCH] grep: handle corrupt index files early","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-15T16:44:42Z","receivedAt":"2018-05-15T16:44:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 15, 2018 at 6:13 AM, Duy Nguyen wrote:\n> On Tue, May 15, 2018 at 3:04 AM, Stefan Beller wrote:\n>> Any other caller of 'repo_read_index' dies upon a negative return of\n>> it, so grep should, too.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>>\n>> Found while reviewing the series\n>> https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/\n>>\n>> builtin/grep.c | 3 ++-\n>> 1 file changed, 2 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/grep.c b/builtin/grep.c\n>> index 6e7bc76785a..69f0743619f 100644\n>> --- a/builtin/grep.c\n>> +++ b/builtin/grep.c\n>> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,\n>> strbuf_addstr(&name, repo->submodule_prefix);\n>> }\n>>\n>> - repo_read_index(repo);\n>> + if (repo_read_index(repo) < 0)\n>> + die(\"index file corrupt\");\n>\n> _() the string (and maybe reuse an existing phrase if found to reduce\n> workload on translators)\n\nsbeller@sbeller:/u/git$ git grep -A 1 repo_read_index\nbuiltin/grep.c:491: if (repo_read_index(repo) < 0)\nbuiltin/grep.c-492- die(\"index file corrupt\");\n--\nbuiltin/ls-files.c:213: if (repo_read_index(&submodule) < 0)\nbuiltin/ls-files.c-214- die(\"index file corrupt\");\n--\nbuiltin/ls-files.c:582: if (repo_read_index(the_repository) < 0)\nbuiltin/ls-files.c-583- die(\"index file corrupt\");\n--\ndir.c:3028: if (repo_read_index(&subrepo) < 0)\ndir.c-3029- die(\"index file corrupt in repo %s\", subrepo.gitdir);\n--\nrepository.c:245:int repo_read_index(struct repository *repo)\nrepository.c-246-{\n--\nrepository.h:70: * 'repo_read_index()' can be used to populate 'index'.\nrepository.h-71- */\n--\nrepository.h:119:extern int repo_read_index(struct repository *repo);\nrepository.h-120-\n--\nsubmodule-config.c:583: if (repo_read_index(repo) < 0)\nsubmodule-config.c-584- return;\n--\nsubmodule.c:1336: if (repo_read_index(r) < 0)\nsubmodule.c-1337- die(\"index file corrupt\");\n\nI think this is as good as it gets for using an existing phrase.\nNone of them are translated, which I would defer to a follow up patch\nthat translates all(?) of them or just the porcelains.\n\nThanks,\nStefan\n"},{"id":"347701","messageId":"20180515170137.GC72551@google.com","threadId":"48497","inReplyTo":"20180515010425.149200-1-sbeller@google.com","subject":"Re: [PATCH] grep: handle corrupt index files early","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-05-15T17:01:37Z","receivedAt":"2018-05-15T17:01:45Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/14, Stefan Beller wrote:\n> Any other caller of 'repo_read_index' dies upon a negative return of\n> it, so grep should, too.\n> \n> Signed-off-by: Stefan Beller \n> ---\n> \n> Found while reviewing the series\n> https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/\n> \n> builtin/grep.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 6e7bc76785a..69f0743619f 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,\n> \t\tstrbuf_addstr(&name, repo->submodule_prefix);\n> \t}\n> \n> -\trepo_read_index(repo);\n> +\tif (repo_read_index(repo) < 0)\n> +\t\tdie(\"index file corrupt\");\n\nLooks good to me!\n\n> \n> \tfor (nr = 0; nr < repo->index->cache_nr; nr++) {\n> \t\tconst struct cache_entry *ce = repo->index->cache[nr];\n> -- \n> 2.17.0.582.gccdcbd54c44.dirty\n> \n\n-- \nBrandon Williams\n"},{"id":"347743","messageId":"xmqq603o8o9w.fsf@gitster-ct.c.googlers.com","threadId":"48497","inReplyTo":"20180515010425.149200-1-sbeller@google.com","subject":"Re: [PATCH] grep: handle corrupt index files early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-15T23:58:19Z","receivedAt":"2018-05-15T23:58:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Any other caller of 'repo_read_index' dies upon a negative return of\n> it, so grep should, too.\n\nMakes sense. When the function returns a failure, there is no\nsensible fallback action anyway, so dying here is alright.\n\nWill queue; thanks.\n\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> Found while reviewing the series\n> https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/\n>\n> builtin/grep.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 6e7bc76785a..69f0743619f 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,\n> \t\tstrbuf_addstr(&name, repo->submodule_prefix);\n> \t}\n> \n> -\trepo_read_index(repo);\n> +\tif (repo_read_index(repo) < 0)\n> +\t\tdie(\"index file corrupt\");\n> \n> \tfor (nr = 0; nr < repo->index->cache_nr; nr++) {\n> \t\tconst struct cache_entry *ce = repo->index->cache[nr];\n"},{"id":"347791","messageId":"CACsJy8A8WZ-Gqe2Y-whJmbADrt+gZjLZ7MTwCtdnK7JDnEdtog@mail.gmail.com","threadId":"48497","inReplyTo":"CAGZ79kYYy-r+Zs_6yg1ZVRNsBcTUdPWE=FeHrp_=0cV0Uf_GfQ@mail.gmail.com","subject":"Re: [PATCH] grep: handle corrupt index files early","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-16T15:24:11Z","receivedAt":"2018-05-16T15:24:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 15, 2018 at 6:44 PM, Stefan Beller wrote:\n> On Tue, May 15, 2018 at 6:13 AM, Duy Nguyen wrote:\n>> On Tue, May 15, 2018 at 3:04 AM, Stefan Beller wrote:\n>>> Any other caller of 'repo_read_index' dies upon a negative return of\n>>> it, so grep should, too.\n>>>\n>>> Signed-off-by: Stefan Beller \n>>> ---\n>>>\n>>> Found while reviewing the series\n>>> https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/\n>>>\n>>> builtin/grep.c | 3 ++-\n>>> 1 file changed, 2 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/builtin/grep.c b/builtin/grep.c\n>>> index 6e7bc76785a..69f0743619f 100644\n>>> --- a/builtin/grep.c\n>>> +++ b/builtin/grep.c\n>>> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,\n>>> strbuf_addstr(&name, repo->submodule_prefix);\n>>> }\n>>>\n>>> - repo_read_index(repo);\n>>> + if (repo_read_index(repo) < 0)\n>>> + die(\"index file corrupt\");\n>>\n>> _() the string (and maybe reuse an existing phrase if found to reduce\n>> workload on translators)\n>\n> sbeller@sbeller:/u/git$ git grep -A 1 repo_read_index\n> builtin/grep.c:491: if (repo_read_index(repo) < 0)\n> builtin/grep.c-492- die(\"index file corrupt\");\n> --\n> builtin/ls-files.c:213: if (repo_read_index(&submodule) < 0)\n> builtin/ls-files.c-214- die(\"index file corrupt\");\n> --\n> builtin/ls-files.c:582: if (repo_read_index(the_repository) < 0)\n> builtin/ls-files.c-583- die(\"index file corrupt\");\n> --\n> dir.c:3028: if (repo_read_index(&subrepo) < 0)\n> dir.c-3029- die(\"index file corrupt in repo %s\", subrepo.gitdir);\n> --\n> repository.c:245:int repo_read_index(struct repository *repo)\n> repository.c-246-{\n> --\n> repository.h:70: * 'repo_read_index()' can be used to populate 'index'.\n> repository.h-71- */\n> --\n> repository.h:119:extern int repo_read_index(struct repository *repo);\n> repository.h-120-\n> --\n> submodule-config.c:583: if (repo_read_index(repo) < 0)\n> submodule-config.c-584- return;\n> --\n> submodule.c:1336: if (repo_read_index(r) < 0)\n> submodule.c-1337- die(\"index file corrupt\");\n>\n> I think this is as good as it gets for using an existing phrase.\n> None of them are translated, which I would defer to a follow up patch\n> that translates all(?) of them or just the porcelains.\n\nIf you have time, yes translate them all. I don't see how any of these\nstrings are meant for script. If not, just _() the new string you\nadded is fine.\n\nWith a majority of call sites dying like this though, I wonder if we\nshould just add repo_read_index_or_die() with die() inside. Then the\nnext person won't likely accidentally forget _()\n\n>\n> Thanks,\n> Stefan\n\n\n\n-- \nDuy\n"},{"id":"347835","messageId":"20180516222118.233868-2-sbeller@google.com","threadId":"48497","inReplyTo":"20180516222118.233868-1-sbeller@google.com","subject":"[PATCH 01/11] grep: handle corrupt index files early","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-16T22:21:08Z","receivedAt":"2018-05-16T22:21:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Any other caller of 'repo_read_index' dies upon a negative return of\nit, so grep should, too.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n builtin/grep.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 6e7bc76785a..69f0743619f 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,\n \t\tstrbuf_addstr(&name, repo->submodule_prefix);\n \t}\n \n-\trepo_read_index(repo);\n+\tif (repo_read_index(repo) < 0)\n+\t\tdie(\"index file corrupt\");\n \n \tfor (nr = 0; nr < repo->index->cache_nr; nr++) {\n \t\tconst struct cache_entry *ce = repo->index->cache[nr];\n-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"347836","messageId":"20180516222118.233868-7-sbeller@google.com","threadId":"48497","inReplyTo":"20180516222118.233868-1-sbeller@google.com","subject":"[PATCH 06/11] read_cache: use repo_read_index_or_die with different error messages","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-16T22:21:13Z","receivedAt":"2018-05-16T22:21:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This replaces all patterns of \"if (read_cached() < 0) die(some-msg);\"\nwith repo_read_index_or_die; this changes the output of the error message.\n\nHowever as all error messages before were translated, these are for human\nconsumption, so a change in error message is not bad; in fact it makes Git\nmore consistent.\n\nSigned-off-by: Stefan Beller \n---\n builtin/add.c | 4 ++--\n builtin/commit.c | 9 +++++----\n builtin/rev-parse.c | 4 ++--\n merge.c | 4 ++--\n 4 files changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex e4751c198c1..910f619b7d5 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -19,6 +19,7 @@\n #include \"bulk-checkin.h\"\n #include \"argv-array.h\"\n #include \"submodule.h\"\n+#include \"repository.h\"\n \n static const char * const builtin_add_usage[] = {\n \tN_(\"git add [] [--] ...\"),\n@@ -229,8 +230,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \n-\tif (read_cache() < 0)\n-\t\tdie(_(\"Could not read the index\"));\n+\trepo_read_index_or_die(the_repository);\n \n \tinit_revisions(&rev, prefix);\n \trev.diffopt.context = 7;\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 5571d4a3e2b..9ebfb4db415 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -32,6 +32,8 @@\n #include \"column.h\"\n #include \"sequencer.h\"\n #include \"mailmap.h\"\n+#include \"sigchain.h\"\n+#include \"repository.h\"\n \n static const char * const builtin_commit_usage[] = {\n \tN_(\"git commit [] [--] ...\"),\n@@ -428,8 +430,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\texit(1);\n \n \tdiscard_cache();\n-\tif (read_cache() < 0)\n-\t\tdie(_(\"cannot read the index\"));\n+\trepo_read_index_or_die(the_repository);\n \n \thold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n \tadd_remove_files(&partial);\n@@ -853,8 +854,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tstruct object_id oid;\n \t\tconst char *parent = \"HEAD\";\n \n-\t\tif (!active_nr && read_cache() < 0)\n-\t\t\tdie(_(\"Cannot read index\"));\n+\t\tif (!active_nr)\n+\t\t\trepo_read_index_or_die(the_repository);\n \n \t\tif (amend)\n \t\t\tparent = \"HEAD^1\";\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 36b20877828..37f29fd850d 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -14,6 +14,7 @@\n #include \"revision.h\"\n #include \"split-index.h\"\n #include \"submodule.h\"\n+#include \"repository.h\"\n \n #define DO_REVS\t\t1\n #define DO_NOREV\t2\n@@ -884,8 +885,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--shared-index-path\")) {\n-\t\t\t\tif (read_cache() < 0)\n-\t\t\t\t\tdie(_(\"Could not read the index\"));\n+\t\t\t\trepo_read_index_or_die(the_repository);\n \t\t\t\tif (the_index.split_index) {\n \t\t\t\t\tconst unsigned char *sha1 = the_index.split_index->base_sha1;\n \t\t\t\t\tconst char *path = git_path(\"sharedindex.%s\", sha1_to_hex(sha1));\ndiff --git a/merge.c b/merge.c\nindex f06a4773d4f..654d049e80d 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -8,6 +8,7 @@\n #include \"tree-walk.h\"\n #include \"unpack-trees.h\"\n #include \"dir.h\"\n+#include \"repository.h\"\n \n static const char *merge_argument(struct commit *commit)\n {\n@@ -70,8 +71,7 @@ int try_merge_command(const char *strategy, size_t xopts_nr,\n \targv_array_clear(&args);\n \n \tdiscard_cache();\n-\tif (read_cache() < 0)\n-\t\tdie(_(\"failed to read the cache\"));\n+\trepo_read_index_or_die(the_repository);\n \tresolve_undo_clear();\n \n \treturn ret;\n-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"347837","messageId":"20180516222118.233868-10-sbeller@google.com","threadId":"48497","inReplyTo":"20180516222118.233868-1-sbeller@google.com","subject":"[PATCH 09/11] checkout-index: switch to repo_read_index","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-16T22:21:16Z","receivedAt":"2018-05-16T22:21:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n builtin/checkout-index.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex a730f6a1aa4..aaba99d36c0 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -10,6 +10,7 @@\n #include \"quote.h\"\n #include \"cache-tree.h\"\n #include \"parse-options.h\"\n+#include \"repository.h\"\n \n #define CHECKOUT_ALL 4\n static int nul_term_line;\n@@ -184,9 +185,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \tprefix_length = prefix ? strlen(prefix) : 0;\n \n-\tif (read_cache() < 0) {\n-\t\tdie(\"invalid cache\");\n-\t}\n+\trepo_read_index_or_die(the_repository);\n \n \targc = parse_options(argc, argv, prefix, builtin_checkout_index_options,\n \t\t\tbuiltin_checkout_index_usage, 0);\n-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"347838","messageId":"20180516222118.233868-6-sbeller@google.com","threadId":"48497","inReplyTo":"20180516222118.233868-1-sbeller@google.com","subject":"[PATCH 05/11] builtin/ls-files: use repo_read_index_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-16T22:21:12Z","receivedAt":"2018-05-16T22:21:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Despite ls-files being a plumbing command, which promises to not change its\noutput ever, and to be easy on machines (e.g. non-localized output),\nit may make sense to localize the error message for a corrupt index\nnevertheless:\n\n1. that is more consistent with the rest of Git.\n2. Searching for \"ls-tree corrupt index file\" on the web doesn't yield\n any hits, that suggest the exact string is parsed for.\n Probably the script authors rely on the exit code of ls-tree anyways.\n\nSigned-off-by: Stefan Beller \n---\n builtin/ls-files.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex a71f6bd088a..502f2f6db04 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -20,6 +20,7 @@\n #include \"run-command.h\"\n #include \"submodule.h\"\n #include \"submodule-config.h\"\n+#include \"repository.h\"\n \n static int abbrev;\n static int show_deleted;\n@@ -210,8 +211,7 @@ static void show_submodule(struct repository *superproject,\n \tif (repo_submodule_init(&submodule, superproject, path))\n \t\treturn;\n \n-\tif (repo_read_index(&submodule) < 0)\n-\t\tdie(\"index file corrupt\");\n+\trepo_read_index_or_die(&submodule);\n \n \tshow_files(&submodule, dir);\n \n@@ -579,8 +579,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\tprefix_len = strlen(prefix);\n \tgit_config(git_default_config, NULL);\n \n-\tif (repo_read_index(the_repository) < 0)\n-\t\tdie(\"index file corrupt\");\n+\trepo_read_index_or_die(the_repository);\n \n \targc = parse_options(argc, argv, prefix, builtin_ls_files_options,\n \t\t\tls_files_usage, 0);\n-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"347839","messageId":"20180516222118.233868-8-sbeller@google.com","threadId":"48497","inReplyTo":"20180516222118.233868-1-sbeller@google.com","subject":"[PATCH 07/11] rerere: use repo_read_index_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-16T22:21:14Z","receivedAt":"2018-05-16T22:21:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"By switching to repo_read_index_or_die, we'll get a slightly different\nerror message (\"index file corrupt\") as well as localization of it.\n\nSigned-off-by: Stefan Beller \n---\n rerere.c | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex 18cae2d11c9..5f35dd66f90 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -10,6 +10,7 @@\n #include \"attr.h\"\n #include \"pathspec.h\"\n #include \"sha1-lookup.h\"\n+#include \"repository.h\"\n \n #define RESOLVED 0\n #define PUNTED 1\n@@ -567,8 +568,7 @@ static int check_one_conflict(int i, int *type)\n static int find_conflict(struct string_list *conflict)\n {\n \tint i;\n-\tif (read_cache() < 0)\n-\t\treturn error(\"Could not read index\");\n+\trepo_read_index_or_die(the_repository);\n \n \tfor (i = 0; i < active_nr;) {\n \t\tint conflict_type;\n@@ -600,8 +600,7 @@ int rerere_remaining(struct string_list *merge_rr)\n \tint i;\n \tif (setup_rerere(merge_rr, RERERE_READONLY))\n \t\treturn 0;\n-\tif (read_cache() < 0)\n-\t\treturn error(\"Could not read index\");\n+\trepo_read_index_or_die(the_repository);\n \n \tfor (i = 0; i < active_nr;) {\n \t\tint conflict_type;\n@@ -1103,8 +1102,7 @@ int rerere_forget(struct pathspec *pathspec)\n \tstruct string_list conflict = STRING_LIST_INIT_DUP;\n \tstruct string_list merge_rr = STRING_LIST_INIT_DUP;\n \n-\tif (read_cache() < 0)\n-\t\treturn error(\"Could not read index\");\n+\trepo_read_index_or_die(the_repository);\n \n \tfd = setup_rerere(&merge_rr, RERERE_NOAUTOUPDATE);\n \tif (fd < 0)\n-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"347840","messageId":"20180516222118.233868-12-sbeller@google.com","threadId":"48497","inReplyTo":"20180516222118.233868-1-sbeller@google.com","subject":"[PATCH 11/11] read_cache: convert most calls to repo_read_index_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-16T22:21:18Z","receivedAt":"2018-05-16T22:21:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n blame.c | 5 +++--\n builtin/am.c | 3 ++-\n builtin/diff.c | 3 ++-\n builtin/fsck.c | 3 ++-\n builtin/merge-index.c | 3 ++-\n check-racy.c | 2 +-\n diff.c | 5 +++--\n merge-recursive.c | 3 ++-\n revision.c | 5 +++--\n sequencer.c | 5 +++--\n sha1-name.c | 2 +-\n 11 files changed, 24 insertions(+), 15 deletions(-)\n\ndiff --git a/blame.c b/blame.c\nindex 78c9808bd1a..ebfa1c8efcd 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -5,6 +5,7 @@\n #include \"diff.h\"\n #include \"diffcore.h\"\n #include \"tag.h\"\n+#include \"repository.h\"\n #include \"blame.h\"\n \n void blame_origin_decref(struct blame_origin *o)\n@@ -159,7 +160,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \tunsigned mode;\n \tstruct strbuf msg = STRBUF_INIT;\n \n-\tread_cache();\n+\trepo_read_index_or_die(the_repository);\n \ttime(&now);\n \tcommit = alloc_commit_node();\n \tcommit->object.parsed = 1;\n@@ -241,7 +242,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \t * want to run \"diff-index --cached\".\n \t */\n \tdiscard_cache();\n-\tread_cache();\n+\trepo_read_index_or_die(the_repository);\n \n \tlen = strlen(path);\n \tif (!mode) {\ndiff --git a/builtin/am.c b/builtin/am.c\nindex d834f9e62b6..3c6e77a5369 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -32,6 +32,7 @@\n #include \"apply.h\"\n #include \"string-list.h\"\n #include \"packfile.h\"\n+#include \"repository.h\"\n \n /**\n * Returns 1 if the file is empty or does not exist, 0 otherwise.\n@@ -1581,7 +1582,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n \tsay(state, stdout, _(\"Falling back to patching base and 3-way merge...\"));\n \n \tdiscard_cache();\n-\tread_cache();\n+\trepo_read_index_or_die(the_repository);\n \n \t/*\n \t * This is not so wrong. Depending on which base we picked, orig_tree\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 16bfb22f738..4bba211f1c7 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -17,6 +17,7 @@\n #include \"builtin.h\"\n #include \"submodule.h\"\n #include \"sha1-array.h\"\n+#include \"repository.h\"\n \n #define DIFF_NO_INDEX_EXPLICIT 1\n #define DIFF_NO_INDEX_IMPLICIT 2\n@@ -210,7 +211,7 @@ static void refresh_index_quietly(void)\n \tif (fd < 0)\n \t\treturn;\n \tdiscard_cache();\n-\tread_cache();\n+\trepo_read_index(the_repository); /* do not die on error */\n \trefresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);\n \tupdate_index_if_able(&the_index, &lock_file);\n }\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 087360a6757..a42e98235da 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -18,6 +18,7 @@\n #include \"decorate.h\"\n #include \"packfile.h\"\n #include \"object-store.h\"\n+#include \"repository.h\"\n \n #define REACHABLE 0x0001\n #define SEEN 0x0002\n@@ -795,7 +796,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \tif (keep_cache_objects) {\n \t\tverify_index_checksum = 1;\n \t\tverify_ce_order = 1;\n-\t\tread_cache();\n+\t\trepo_read_index_or_die(the_repository);\n \t\tfor (i = 0; i < active_nr; i++) {\n \t\t\tunsigned int mode;\n \t\t\tstruct blob *blob;\ndiff --git a/builtin/merge-index.c b/builtin/merge-index.c\nindex c99443b095b..2d91c7c3b5e 100644\n--- a/builtin/merge-index.c\n+++ b/builtin/merge-index.c\n@@ -1,5 +1,6 @@\n #include \"builtin.h\"\n #include \"run-command.h\"\n+#include \"repository.h\"\n \n static const char *pgm;\n static int one_shot, quiet;\n@@ -77,7 +78,7 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix)\n \tif (argc < 3)\n \t\tusage(\"git merge-index [-o] [-q] (-a | [--] [...])\");\n \n-\tread_cache();\n+\trepo_read_index_or_die(the_repository);\n \n \ti = 1;\n \tif (!strcmp(argv[i], \"-o\")) {\ndiff --git a/check-racy.c b/check-racy.c\nindex 24b6542352a..9b884639cf4 100644\n--- a/check-racy.c\n+++ b/check-racy.c\n@@ -6,7 +6,7 @@ int main(int ac, char **av)\n \tint dirty, clean, racy;\n \n \tdirty = clean = racy = 0;\n-\tread_cache();\n+\trepo_read_index_or_die(the_repository);\n \tfor (i = 0; i < active_nr; i++) {\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tstruct stat st;\ndiff --git a/diff.c b/diff.c\nindex 1289df4b1f9..383f52fa118 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -22,6 +22,7 @@\n #include \"argv-array.h\"\n #include \"graph.h\"\n #include \"packfile.h\"\n+#include \"repository.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -4210,13 +4211,13 @@ void diff_setup_done(struct diff_options *options)\n \t\toptions->rename_limit = diff_rename_limit_default;\n \tif (options->setup & DIFF_SETUP_USE_CACHE) {\n \t\tif (!active_cache)\n-\t\t\t/* read-cache does not die even when it fails\n+\t\t\t/* repo_read_indexe does not die even when it fails\n \t\t\t * so it is safe for us to do this here. Also\n \t\t\t * it does not smudge active_cache or active_nr\n \t\t\t * when it fails, so we do not have to worry about\n \t\t\t * cleaning it up ourselves either.\n \t\t\t */\n-\t\t\tread_cache();\n+\t\t\trepo_read_index(the_repository);\n \t}\n \tif (40 < options->abbrev)\n \t\toptions->abbrev = 40; /* full */\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 0c0d48624da..76911c935c3 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2151,7 +2151,8 @@ int merge_recursive(struct merge_options *o,\n \n \tdiscard_cache();\n \tif (!o->call_depth)\n-\t\tread_cache();\n+\t\tif (read_cache() < 0)\n+\t\t\treturn err(o, _(\"index file corrupt\"));\n \n \to->ancestor = \"merged common ancestors\";\n \tclean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree,\ndiff --git a/revision.c b/revision.c\nindex 1cff11833e7..8ad9824143d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -23,6 +23,7 @@\n #include \"packfile.h\"\n #include \"worktree.h\"\n #include \"argv-array.h\"\n+#include \"repository.h\"\n \n volatile show_early_output_fn_t show_early_output;\n \n@@ -1344,7 +1345,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)\n {\n \tstruct worktree **worktrees, **p;\n \n-\tread_cache();\n+\trepo_read_index_or_die(the_repository);\n \tdo_add_index_objects_to_pending(revs, &the_index);\n \n \tif (revs->single_worktree)\n@@ -1486,7 +1487,7 @@ static void prepare_show_merge(struct rev_info *revs)\n \thead->object.flags |= SYMMETRIC_LEFT;\n \n \tif (!active_nr)\n-\t\tread_cache();\n+\t\trepo_read_index_or_die(the_repository);\n \tfor (i = 0; i < active_nr; i++) {\n \t\tconst struct cache_entry *ce = active_cache[i];\n \t\tif (!ce_stage(ce))\ndiff --git a/sequencer.c b/sequencer.c\nindex 4ce5120e777..773165c8cde 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -23,6 +23,7 @@\n #include \"hashmap.h\"\n #include \"notes-utils.h\"\n #include \"sigchain.h\"\n+#include \"repository.h\"\n \n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n \n@@ -432,7 +433,7 @@ static int fast_forward_to(const struct object_id *to, const struct object_id *f\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct strbuf err = STRBUF_INIT;\n \n-\tread_cache();\n+\trepo_read_index_or_die(the_repository);\n \tif (checkout_fast_forward(from, to, 1))\n \t\treturn -1; /* the callee should have complained already */\n \n@@ -489,7 +490,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \tif (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR) < 0)\n \t\treturn -1;\n \n-\tread_cache();\n+\trepo_read_index_or_die(the_repository);\n \n \tinit_merge_options(&o);\n \to.ancestor = base ? base_label : \"(empty tree)\";\ndiff --git a/sha1-name.c b/sha1-name.c\nindex 5b93bf8da36..83d5f945cf1 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -1639,7 +1639,7 @@ static int get_oid_with_context_1(const char *name,\n \t\t\toc->path = xstrdup(cp);\n \n \t\tif (!active_cache)\n-\t\t\tread_cache();\n+\t\t\trepo_read_index_or_die(the_repository);\n \t\tpos = cache_name_pos(cp, namelen);\n \t\tif (pos < 0)\n \t\t\tpos = -pos - 1;\n-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"347841","messageId":"20180516222118.233868-9-sbeller@google.com","threadId":"48497","inReplyTo":"20180516222118.233868-1-sbeller@google.com","subject":"[PATCH 08/11] check-attr: switch to repo_read_index_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-16T22:21:15Z","receivedAt":"2018-05-16T22:22:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n builtin/check-attr.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex 91444dc0448..bf05e7e93ca 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -4,6 +4,7 @@\n #include \"attr.h\"\n #include \"quote.h\"\n #include \"parse-options.h\"\n+#include \"repository.h\"\n \n static int all_attrs;\n static int cached_attrs;\n@@ -115,9 +116,7 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, check_attr_options,\n \t\t\t check_attr_usage, PARSE_OPT_KEEP_DASHDASH);\n \n-\tif (read_cache() < 0) {\n-\t\tdie(\"invalid cache\");\n-\t}\n+\trepo_read_index_or_die(the_repository);\n \n \tif (cached_attrs)\n \t\tgit_attr_set_direction(GIT_ATTR_INDEX, NULL);\n-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"347842","messageId":"20180516222118.233868-11-sbeller@google.com","threadId":"48497","inReplyTo":"20180516222118.233868-1-sbeller@google.com","subject":"[PATCH 10/11] test helpers: switch to repo_read_index_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-16T22:21:17Z","receivedAt":"2018-05-16T22:22:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n t/helper/test-dump-cache-tree.c | 5 ++---\n t/helper/test-dump-untracked-cache.c | 4 ++--\n t/helper/test-lazy-init-name-hash.c | 11 ++++++-----\n t/helper/test-read-cache.c | 3 ++-\n t/helper/test-scrap-cache-tree.c | 4 ++--\n t/helper/test-write-cache.c | 3 ++-\n 6 files changed, 16 insertions(+), 14 deletions(-)\n\ndiff --git a/t/helper/test-dump-cache-tree.c b/t/helper/test-dump-cache-tree.c\nindex 98a4891f1dc..a58c26084dd 100644\n--- a/t/helper/test-dump-cache-tree.c\n+++ b/t/helper/test-dump-cache-tree.c\n@@ -2,7 +2,7 @@\n #include \"cache.h\"\n #include \"tree.h\"\n #include \"cache-tree.h\"\n-\n+#include \"repository.h\"\n \n static void dump_one(struct cache_tree *it, const char *pfx, const char *x)\n {\n@@ -60,8 +60,7 @@ int cmd__dump_cache_tree(int ac, const char **av)\n \tstruct index_state istate;\n \tstruct cache_tree *another = cache_tree();\n \tsetup_git_directory();\n-\tif (read_cache() < 0)\n-\t\tdie(\"unable to read index file\");\n+\trepo_read_index_or_die(the_repository);\n \tistate = the_index;\n \tistate.cache_tree = another;\n \tcache_tree_update(&istate, WRITE_TREE_DRY_RUN);\ndiff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c\nindex d7c55c2355e..171ba143c9a 100644\n--- a/t/helper/test-dump-untracked-cache.c\n+++ b/t/helper/test-dump-untracked-cache.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"dir.h\"\n+#include \"repository.h\"\n \n static int compare_untracked(const void *a_, const void *b_)\n {\n@@ -47,8 +48,7 @@ int cmd_main(int ac, const char **av)\n \tignore_untracked_cache_config = 1;\n \n \tsetup_git_directory();\n-\tif (read_cache() < 0)\n-\t\tdie(\"unable to read index file\");\n+\trepo_read_index_or_die(the_repository);\n \tuc = the_index.untracked;\n \tif (!uc) {\n \t\tprintf(\"no untracked cache\\n\");\ndiff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c\nindex b99a37080d9..e25bfe27ab9 100644\n--- a/t/helper/test-lazy-init-name-hash.c\n+++ b/t/helper/test-lazy-init-name-hash.c\n@@ -1,6 +1,7 @@\n #include \"test-tool.h\"\n #include \"cache.h\"\n #include \"parse-options.h\"\n+#include \"repository.h\"\n \n static int single;\n static int multi;\n@@ -32,7 +33,7 @@ static void dump_run(void)\n \tstruct dir_entry *dir;\n \tstruct cache_entry *ce;\n \n-\tread_cache();\n+\trepo_read_index_or_die(the_repository);\n \tif (single) {\n \t\ttest_lazy_init_name_hash(&the_index, 0);\n \t} else {\n@@ -70,7 +71,7 @@ static uint64_t time_runs(int try_threaded)\n \n \tfor (i = 0; i < count; i++) {\n \t\tt0 = getnanotime();\n-\t\tread_cache();\n+\t\trepo_read_index_or_die(the_repository);\n \t\tt1 = getnanotime();\n \t\tnr_threads_used = test_lazy_init_name_hash(&the_index, try_threaded);\n \t\tt2 = getnanotime();\n@@ -117,7 +118,7 @@ static void analyze_run(void)\n \tint i;\n \tint nr;\n \n-\tread_cache();\n+\trepo_read_index_or_die(the_repository);\n \tcache_nr_limit = the_index.cache_nr;\n \tdiscard_cache();\n \n@@ -132,7 +133,7 @@ static void analyze_run(void)\n \t\t\tnr = cache_nr_limit;\n \n \t\tfor (i = 0; i < count; i++) {\n-\t\t\tread_cache();\n+\t\t\trepo_read_index_or_die(the_repository);\n \t\t\tthe_index.cache_nr = nr; /* cheap truncate of index */\n \t\t\tt1s = getnanotime();\n \t\t\ttest_lazy_init_name_hash(&the_index, 0);\n@@ -141,7 +142,7 @@ static void analyze_run(void)\n \t\t\tthe_index.cache_nr = cache_nr_limit;\n \t\t\tdiscard_cache();\n \n-\t\t\tread_cache();\n+\t\t\trepo_read_index_or_die(the_repository);\n \t\t\tthe_index.cache_nr = nr; /* cheap truncate of index */\n \t\t\tt1m = getnanotime();\n \t\t\tnr_threads_used = test_lazy_init_name_hash(&the_index, 1);\ndiff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c\nindex d674c88ba09..7a4a91bf328 100644\n--- a/t/helper/test-read-cache.c\n+++ b/t/helper/test-read-cache.c\n@@ -1,5 +1,6 @@\n #include \"test-tool.h\"\n #include \"cache.h\"\n+#include \"repository.h\"\n \n int cmd__read_cache(int argc, const char **argv)\n {\n@@ -8,7 +9,7 @@ int cmd__read_cache(int argc, const char **argv)\n \t\tcnt = strtol(argv[1], NULL, 0);\n \tsetup_git_directory();\n \tfor (i = 0; i < cnt; i++) {\n-\t\tread_cache();\n+\t\trepo_read_index_or_die(the_repository);\n \t\tdiscard_cache();\n \t}\n \treturn 0;\ndiff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c\nindex d26d3e7c8b1..401917abb49 100644\n--- a/t/helper/test-scrap-cache-tree.c\n+++ b/t/helper/test-scrap-cache-tree.c\n@@ -3,6 +3,7 @@\n #include \"lockfile.h\"\n #include \"tree.h\"\n #include \"cache-tree.h\"\n+#include \"repository.h\"\n \n static struct lock_file index_lock;\n \n@@ -10,8 +11,7 @@ int cmd__scrap_cache_tree(int ac, const char **av)\n {\n \tsetup_git_directory();\n \thold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n-\tif (read_cache() < 0)\n-\t\tdie(\"unable to read index file\");\n+\trepo_read_index_or_die(the_repository);\n \tactive_cache_tree = NULL;\n \tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n \t\tdie(\"unable to write index file\");\ndiff --git a/t/helper/test-write-cache.c b/t/helper/test-write-cache.c\nindex 017dc303800..6fd750da254 100644\n--- a/t/helper/test-write-cache.c\n+++ b/t/helper/test-write-cache.c\n@@ -1,6 +1,7 @@\n #include \"test-tool.h\"\n #include \"cache.h\"\n #include \"lockfile.h\"\n+#include \"repository.h\"\n \n static struct lock_file index_lock;\n \n@@ -10,7 +11,7 @@ int cmd__write_cache(int argc, const char **argv)\n \tif (argc == 2)\n \t\tcnt = strtol(argv[1], NULL, 0);\n \tsetup_git_directory();\n-\tread_cache();\n+\trepo_read_index_or_die(the_repository);\n \tfor (i = 0; i < cnt; i++) {\n \t\tlockfd = hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n \t\tif (0 <= lockfd) {\n-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"347843","messageId":"20180516222118.233868-1-sbeller@google.com","threadId":"48497","inReplyTo":"CACsJy8A8WZ-Gqe2Y-whJmbADrt+gZjLZ7MTwCtdnK7JDnEdtog@mail.gmail.com","subject":"[PATCH 00/11]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-16T22:21:07Z","receivedAt":"2018-05-16T22:22:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> If you have time, yes translate them all. I don't see how any of these\n> strings are meant for script. If not, just _() the new string you\n> added is fine.\n\n> With a majority of call sites dying like this though, I wonder if we\n> should just add repo_read_index_or_die() with die() inside. Then the\n> next person won't likely accidentally forget _()\n\nSo this comment tricked me into coming up with a patch series. :)\n\nEach patch is themed, I tried to make each commit special w.r.t. reviewers\nattention.\n\nWe'd start out with a resend of the origin patch, which is boring.\n\nThen we'll move all similar cases into one function (no-op for\nintroducing the repo_read_index_or_die function as all callers will have the\nsame error message and the same localisation).\n\nAny following patch will be more controversial then the previous patches,\nI would expect, as we introduce more and more change.\n\nThe last patch is just an attempt to finish the series gracefully,\nand may contain errors (sometimes we do not want to die() in case of\ncorrupt index).\n\nIs this series roughly what you had in mind?\n\nThanks,\nStefan\n\n\nStefan Beller (11):\n grep: handle corrupt index files early\n repository: introduce repo_read_index_or_die\n builtin/grep: use repo_read_index_or_die\n submodule: use repo_read_index_or_die\n builtin/ls-files: use repo_read_index_or_die\n read_cache: use repo_read_index_or_die with different error messages\n rerere: use repo_read_index_or_die\n check-attr: switch to repo_read_index_or_die\n checkout-index: switch to repo_read_index\n test helpers: switch to repo_read_index_or_die\n read_cache: convert most calls to repo_read_index_or_die\n\n blame.c | 5 +++--\n builtin/add.c | 7 +++----\n builtin/am.c | 3 ++-\n builtin/check-attr.c | 5 ++---\n builtin/check-ignore.c | 7 ++++---\n builtin/checkout-index.c | 5 ++---\n builtin/clean.c | 4 ++--\n builtin/commit.c | 9 +++++----\n builtin/diff.c | 3 ++-\n builtin/fsck.c | 3 ++-\n builtin/grep.c | 2 +-\n builtin/ls-files.c | 7 +++----\n builtin/merge-index.c | 3 ++-\n builtin/mv.c | 3 +--\n builtin/reset.c | 3 +--\n builtin/rev-parse.c | 4 ++--\n builtin/rm.c | 3 +--\n builtin/submodule--helper.c | 3 +--\n check-racy.c | 2 +-\n diff.c | 5 +++--\n merge-recursive.c | 3 ++-\n merge.c | 4 ++--\n repository.c | 6 ++++++\n repository.h | 8 ++++++++\n rerere.c | 10 ++++------\n revision.c | 5 +++--\n sequencer.c | 5 +++--\n sha1-name.c | 2 +-\n submodule.c | 3 +--\n t/helper/test-dump-cache-tree.c | 5 ++---\n t/helper/test-dump-untracked-cache.c | 4 ++--\n t/helper/test-lazy-init-name-hash.c | 11 ++++++-----\n t/helper/test-read-cache.c | 3 ++-\n t/helper/test-scrap-cache-tree.c | 4 ++--\n t/helper/test-write-cache.c | 3 ++-\n 35 files changed, 89 insertions(+), 73 deletions(-)\n\n-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"347844","messageId":"20180516222118.233868-3-sbeller@google.com","threadId":"48497","inReplyTo":"20180516222118.233868-1-sbeller@google.com","subject":"[PATCH 02/11] repository: introduce repo_read_index_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-16T22:21:09Z","receivedAt":"2018-05-16T22:22:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"A common pattern with the repo_read_index function is to die if the return\nof repo_read_index is negative. Move this pattern into a function.\n\nThis patch replaces the calls of repo_read_index with its _or_die version,\nif the error message is exactly the same.\n\nSigned-off-by: Stefan Beller \n---\n builtin/add.c | 3 +--\n builtin/check-ignore.c | 7 ++++---\n builtin/clean.c | 4 ++--\n builtin/mv.c | 3 +--\n builtin/reset.c | 3 +--\n builtin/rm.c | 3 +--\n builtin/submodule--helper.c | 3 +--\n repository.c | 6 ++++++\n repository.h | 8 ++++++++\n 9 files changed, 25 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex c9e2619a9ad..e4751c198c1 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -445,8 +445,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n \n-\tif (read_cache() < 0)\n-\t\tdie(_(\"index file corrupt\"));\n+\trepo_read_index_or_die(the_repository);\n \n \tdie_in_unpopulated_submodule(&the_index, prefix);\n \ndiff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\nindex ec9a959e08d..2a46bf9af7f 100644\n--- a/builtin/check-ignore.c\n+++ b/builtin/check-ignore.c\n@@ -6,6 +6,7 @@\n #include \"pathspec.h\"\n #include \"parse-options.h\"\n #include \"submodule.h\"\n+#include \"repository.h\"\n \n static int quiet, verbose, stdin_paths, show_non_matching, no_index;\n static const char * const check_ignore_usage[] = {\n@@ -172,9 +173,9 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix)\n \tif (show_non_matching && !verbose)\n \t\tdie(_(\"--non-matching is only valid with --verbose\"));\n \n-\t/* read_cache() is only necessary so we can watch out for submodules. */\n-\tif (!no_index && read_cache() < 0)\n-\t\tdie(_(\"index file corrupt\"));\n+\t/* repo_read_index() is only necessary so we can watch out for submodules. */\n+\tif (!no_index)\n+\t\trepo_read_index_or_die(the_repository);\n \n \tmemset(&dir, 0, sizeof(dir));\n \tsetup_standard_excludes(&dir);\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex fad533a0a73..6c1c6fdd7f9 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -16,6 +16,7 @@\n #include \"column.h\"\n #include \"color.h\"\n #include \"pathspec.h\"\n+#include \"repository.h\"\n \n static int force = -1; /* unset */\n static int interactive;\n@@ -954,8 +955,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \tif (remove_directories)\n \t\tdir.flags |= DIR_SHOW_IGNORED_TOO | DIR_KEEP_UNTRACKED_CONTENTS;\n \n-\tif (read_cache() < 0)\n-\t\tdie(_(\"index file corrupt\"));\n+\trepo_read_index_or_die(the_repository);\n \n \tif (!ignored)\n \t\tsetup_standard_excludes(&dir);\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 7a63667d648..7f62e626dda 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -140,8 +140,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(builtin_mv_usage, builtin_mv_options);\n \n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n-\tif (read_cache() < 0)\n-\t\tdie(_(\"index file corrupt\"));\n+\trepo_read_index_or_die(the_repository);\n \n \tsource = internal_prefix_pathspec(prefix, argv, argc, 0);\n \tmodes = xcalloc(argc, sizeof(enum update_mode));\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 7f1c3f02a30..fd514eec822 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -237,8 +237,7 @@ static void parse_args(struct pathspec *pathspec,\n \t}\n \t*rev_ret = rev;\n \n-\tif (read_cache() < 0)\n-\t\tdie(_(\"index file corrupt\"));\n+\trepo_read_index_or_die(the_repository);\n \n \tparse_pathspec(pathspec, 0,\n \t\t PATHSPEC_PREFER_FULL |\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 5b6fc7ee818..3b90191aa53 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -267,8 +267,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n-\tif (read_cache() < 0)\n-\t\tdie(_(\"index file corrupt\"));\n+\trepo_read_index_or_die(the_repository);\n \n \tparse_pathspec(&pathspec, 0,\n \t\t PATHSPEC_PREFER_CWD,\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c2403a915ff..7aebed9bd66 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -323,8 +323,7 @@ static int module_list_compute(int argc, const char **argv,\n \tif (pathspec->nr)\n \t\tps_matched = xcalloc(pathspec->nr, 1);\n \n-\tif (read_cache() < 0)\n-\t\tdie(_(\"index file corrupt\"));\n+\trepo_read_index_or_die(the_repository);\n \n \tfor (i = 0; i < active_nr; i++) {\n \t\tconst struct cache_entry *ce = active_cache[i];\ndiff --git a/repository.c b/repository.c\nindex beff3caa9e2..ceca14ba718 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -249,3 +249,9 @@ int repo_read_index(struct repository *repo)\n \n \treturn read_index_from(repo->index, repo->index_file, repo->gitdir);\n }\n+\n+void repo_read_index_or_die(struct repository *repo)\n+{\n+\tif (repo_read_index(repo) < 0)\n+\t\tdie(_(\"index file corrupt\"));\n+}\ndiff --git a/repository.h b/repository.h\nindex f2646f0c52a..8efd8979ad3 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -118,4 +118,12 @@ extern void repo_clear(struct repository *repo);\n */\n extern int repo_read_index(struct repository *repo);\n \n+/*\n+ * Populates the index in the repository from its index file,\n+ * allocating the struct index if needed.\n+ *\n+ * If the index file cannot be read, die.\n+ */\n+void repo_read_index_or_die(struct repository *r);\n+\n #endif /* REPOSITORY_H */\n-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"347845","messageId":"20180516222118.233868-4-sbeller@google.com","threadId":"48497","inReplyTo":"20180516222118.233868-1-sbeller@google.com","subject":"[PATCH 03/11] builtin/grep: use repo_read_index_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-16T22:21:10Z","receivedAt":"2018-05-16T22:22:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"grep is a porcelain command, so translating its error message is a good\nidea.\n\nSigned-off-by: Stefan Beller \n---\n builtin/grep.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 69f0743619f..2c2d6cc6bca 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -488,8 +488,7 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,\n \t\tstrbuf_addstr(&name, repo->submodule_prefix);\n \t}\n \n-\tif (repo_read_index(repo) < 0)\n-\t\tdie(\"index file corrupt\");\n+\trepo_read_index_or_die(repo);\n \n \tfor (nr = 0; nr < repo->index->cache_nr; nr++) {\n \t\tconst struct cache_entry *ce = repo->index->cache[nr];\n-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"347846","messageId":"20180516222118.233868-5-sbeller@google.com","threadId":"48497","inReplyTo":"20180516222118.233868-1-sbeller@google.com","subject":"[PATCH 04/11] submodule: use repo_read_index_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-16T22:21:11Z","receivedAt":"2018-05-16T22:22:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The code is used by the fetch command, which is a main porcelain command,\nso we should localize its error messages.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 74d35b25779..71c042e1371 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1333,8 +1333,7 @@ int fetch_populated_submodules(struct repository *r,\n \tif (!r->worktree)\n \t\tgoto out;\n \n-\tif (repo_read_index(r) < 0)\n-\t\tdie(\"index file corrupt\");\n+\trepo_read_index_or_die(r);\n \n \targv_array_push(&spf.args, \"fetch\");\n \tfor (i = 0; i < options->argc; i++)\n-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"347847","messageId":"20180516222733.GB60301@google.com","threadId":"48497","inReplyTo":"20180516222118.233868-12-sbeller@google.com","subject":"Re: [PATCH 11/11] read_cache: convert most calls to repo_read_index_or_die","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-05-16T22:27:33Z","receivedAt":"2018-05-16T22:27:40Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/16, Stefan Beller wrote:\n> Signed-off-by: Stefan Beller \n> ---\n> blame.c | 5 +++--\n> builtin/am.c | 3 ++-\n> builtin/diff.c | 3 ++-\n> builtin/fsck.c | 3 ++-\n> builtin/merge-index.c | 3 ++-\n> check-racy.c | 2 +-\n> diff.c | 5 +++--\n> merge-recursive.c | 3 ++-\n> revision.c | 5 +++--\n> sequencer.c | 5 +++--\n> sha1-name.c | 2 +-\n> 11 files changed, 24 insertions(+), 15 deletions(-)\n> \n> diff --git a/blame.c b/blame.c\n> index 78c9808bd1a..ebfa1c8efcd 100644\n> --- a/blame.c\n> +++ b/blame.c\n> @@ -5,6 +5,7 @@\n> #include \"diff.h\"\n> #include \"diffcore.h\"\n> #include \"tag.h\"\n> +#include \"repository.h\"\n> #include \"blame.h\"\n> \n> void blame_origin_decref(struct blame_origin *o)\n> @@ -159,7 +160,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n> \tunsigned mode;\n> \tstruct strbuf msg = STRBUF_INIT;\n> \n> -\tread_cache();\n> +\trepo_read_index_or_die(the_repository);\n> \ttime(&now);\n> \tcommit = alloc_commit_node();\n> \tcommit->object.parsed = 1;\n> @@ -241,7 +242,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n> \t * want to run \"diff-index --cached\".\n> \t */\n> \tdiscard_cache();\n> -\tread_cache();\n> +\trepo_read_index_or_die(the_repository);\n> \n> \tlen = strlen(path);\n> \tif (!mode) {\n> diff --git a/builtin/am.c b/builtin/am.c\n> index d834f9e62b6..3c6e77a5369 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -32,6 +32,7 @@\n> #include \"apply.h\"\n> #include \"string-list.h\"\n> #include \"packfile.h\"\n> +#include \"repository.h\"\n> \n> /**\n> * Returns 1 if the file is empty or does not exist, 0 otherwise.\n> @@ -1581,7 +1582,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n> \tsay(state, stdout, _(\"Falling back to patching base and 3-way merge...\"));\n> \n> \tdiscard_cache();\n> -\tread_cache();\n> +\trepo_read_index_or_die(the_repository);\n> \n> \t/*\n> \t * This is not so wrong. Depending on which base we picked, orig_tree\n> diff --git a/builtin/diff.c b/builtin/diff.c\n> index 16bfb22f738..4bba211f1c7 100644\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -17,6 +17,7 @@\n> #include \"builtin.h\"\n> #include \"submodule.h\"\n> #include \"sha1-array.h\"\n> +#include \"repository.h\"\n> \n> #define DIFF_NO_INDEX_EXPLICIT 1\n> #define DIFF_NO_INDEX_IMPLICIT 2\n> @@ -210,7 +211,7 @@ static void refresh_index_quietly(void)\n> \tif (fd < 0)\n> \t\treturn;\n> \tdiscard_cache();\n> -\tread_cache();\n> +\trepo_read_index(the_repository); /* do not die on error */\n> \trefresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);\n> \tupdate_index_if_able(&the_index, &lock_file);\n> }\n> diff --git a/builtin/fsck.c b/builtin/fsck.c\n> index 087360a6757..a42e98235da 100644\n> --- a/builtin/fsck.c\n> +++ b/builtin/fsck.c\n> @@ -18,6 +18,7 @@\n> #include \"decorate.h\"\n> #include \"packfile.h\"\n> #include \"object-store.h\"\n> +#include \"repository.h\"\n> \n> #define REACHABLE 0x0001\n> #define SEEN 0x0002\n> @@ -795,7 +796,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n> \tif (keep_cache_objects) {\n> \t\tverify_index_checksum = 1;\n> \t\tverify_ce_order = 1;\n> -\t\tread_cache();\n> +\t\trepo_read_index_or_die(the_repository);\n> \t\tfor (i = 0; i < active_nr; i++) {\n> \t\t\tunsigned int mode;\n> \t\t\tstruct blob *blob;\n> diff --git a/builtin/merge-index.c b/builtin/merge-index.c\n> index c99443b095b..2d91c7c3b5e 100644\n> --- a/builtin/merge-index.c\n> +++ b/builtin/merge-index.c\n> @@ -1,5 +1,6 @@\n> #include \"builtin.h\"\n> #include \"run-command.h\"\n> +#include \"repository.h\"\n> \n> static const char *pgm;\n> static int one_shot, quiet;\n> @@ -77,7 +78,7 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix)\n> \tif (argc < 3)\n> \t\tusage(\"git merge-index [-o] [-q] (-a | [--] [...])\");\n> \n> -\tread_cache();\n> +\trepo_read_index_or_die(the_repository);\n> \n> \ti = 1;\n> \tif (!strcmp(argv[i], \"-o\")) {\n> diff --git a/check-racy.c b/check-racy.c\n> index 24b6542352a..9b884639cf4 100644\n> --- a/check-racy.c\n> +++ b/check-racy.c\n> @@ -6,7 +6,7 @@ int main(int ac, char **av)\n> \tint dirty, clean, racy;\n> \n> \tdirty = clean = racy = 0;\n> -\tread_cache();\n> +\trepo_read_index_or_die(the_repository);\n> \tfor (i = 0; i < active_nr; i++) {\n> \t\tstruct cache_entry *ce = active_cache[i];\n> \t\tstruct stat st;\n> diff --git a/diff.c b/diff.c\n> index 1289df4b1f9..383f52fa118 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -22,6 +22,7 @@\n> #include \"argv-array.h\"\n> #include \"graph.h\"\n> #include \"packfile.h\"\n> +#include \"repository.h\"\n> \n> #ifdef NO_FAST_WORKING_DIRECTORY\n> #define FAST_WORKING_DIRECTORY 0\n> @@ -4210,13 +4211,13 @@ void diff_setup_done(struct diff_options *options)\n> \t\toptions->rename_limit = diff_rename_limit_default;\n> \tif (options->setup & DIFF_SETUP_USE_CACHE) {\n> \t\tif (!active_cache)\n> -\t\t\t/* read-cache does not die even when it fails\n> +\t\t\t/* repo_read_indexe does not die even when it fails\n\ns/repo_read_indexe/repo_read_index\n\n> \t\t\t * so it is safe for us to do this here. Also\n> \t\t\t * it does not smudge active_cache or active_nr\n> \t\t\t * when it fails, so we do not have to worry about\n> \t\t\t * cleaning it up ourselves either.\n> \t\t\t */\n> -\t\t\tread_cache();\n> +\t\t\trepo_read_index(the_repository);\n> \t}\n> \tif (40 < options->abbrev)\n> \t\toptions->abbrev = 40; /* full */\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 0c0d48624da..76911c935c3 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -2151,7 +2151,8 @@ int merge_recursive(struct merge_options *o,\n> \n> \tdiscard_cache();\n> \tif (!o->call_depth)\n> -\t\tread_cache();\n> +\t\tif (read_cache() < 0)\n> +\t\t\treturn err(o, _(\"index file corrupt\"));\n\nif we're already moving to change read_cache() calls then we could\nsubstitute it for repo_read_index here as well.\n\nMaybe we can kill read_cache() while at it?\n\n> \n> \to->ancestor = \"merged common ancestors\";\n> \tclean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree,\n> diff --git a/revision.c b/revision.c\n> index 1cff11833e7..8ad9824143d 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -23,6 +23,7 @@\n> #include \"packfile.h\"\n> #include \"worktree.h\"\n> #include \"argv-array.h\"\n> +#include \"repository.h\"\n> \n> volatile show_early_output_fn_t show_early_output;\n> \n> @@ -1344,7 +1345,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)\n> {\n> \tstruct worktree **worktrees, **p;\n> \n> -\tread_cache();\n> +\trepo_read_index_or_die(the_repository);\n> \tdo_add_index_objects_to_pending(revs, &the_index);\n> \n> \tif (revs->single_worktree)\n> @@ -1486,7 +1487,7 @@ static void prepare_show_merge(struct rev_info *revs)\n> \thead->object.flags |= SYMMETRIC_LEFT;\n> \n> \tif (!active_nr)\n> -\t\tread_cache();\n> +\t\trepo_read_index_or_die(the_repository);\n> \tfor (i = 0; i < active_nr; i++) {\n> \t\tconst struct cache_entry *ce = active_cache[i];\n> \t\tif (!ce_stage(ce))\n> diff --git a/sequencer.c b/sequencer.c\n> index 4ce5120e777..773165c8cde 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -23,6 +23,7 @@\n> #include \"hashmap.h\"\n> #include \"notes-utils.h\"\n> #include \"sigchain.h\"\n> +#include \"repository.h\"\n> \n> #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n> \n> @@ -432,7 +433,7 @@ static int fast_forward_to(const struct object_id *to, const struct object_id *f\n> \tstruct strbuf sb = STRBUF_INIT;\n> \tstruct strbuf err = STRBUF_INIT;\n> \n> -\tread_cache();\n> +\trepo_read_index_or_die(the_repository);\n> \tif (checkout_fast_forward(from, to, 1))\n> \t\treturn -1; /* the callee should have complained already */\n> \n> @@ -489,7 +490,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n> \tif (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR) < 0)\n> \t\treturn -1;\n> \n> -\tread_cache();\n> +\trepo_read_index_or_die(the_repository);\n> \n> \tinit_merge_options(&o);\n> \to.ancestor = base ? base_label : \"(empty tree)\";\n> diff --git a/sha1-name.c b/sha1-name.c\n> index 5b93bf8da36..83d5f945cf1 100644\n> --- a/sha1-name.c\n> +++ b/sha1-name.c\n> @@ -1639,7 +1639,7 @@ static int get_oid_with_context_1(const char *name,\n> \t\t\toc->path = xstrdup(cp);\n> \n> \t\tif (!active_cache)\n> -\t\t\tread_cache();\n> +\t\t\trepo_read_index_or_die(the_repository);\n> \t\tpos = cache_name_pos(cp, namelen);\n> \t\tif (pos < 0)\n> \t\t\tpos = -pos - 1;\n> -- \n> 2.17.0.582.gccdcbd54c44.dirty\n> \n\n-- \nBrandon Williams\n"},{"id":"347893","messageId":"xmqqbmdf6p1t.fsf@gitster-ct.c.googlers.com","threadId":"48497","inReplyTo":"CACsJy8A8WZ-Gqe2Y-whJmbADrt+gZjLZ7MTwCtdnK7JDnEdtog@mail.gmail.com","subject":"Re: [PATCH] grep: handle corrupt index files early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-17T01:36:46Z","receivedAt":"2018-05-17T01:36:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> With a majority of call sites dying like this though, I wonder if we\n> should just add repo_read_index_or_die() with die() inside. Then the\n> next person won't likely accidentally forget _()\n\nYuck.\n\nThat sounds like inviting a major code churn. I tend to agree that\nit would be a good clean-up for longer term maintenance, but I am\nnot sure if I can honestly say I'd look forward to such a clean-up\nat this point in the cycle when there are tons of large-ish topics\nin flight X-<.\n\n"},{"id":"347913","messageId":"CAGZ79kbvjoTq5079Ks+h2HNb+D99RELYPcJk2=pvZf9-Y8dToQ@mail.gmail.com","threadId":"48497","inReplyTo":"xmqqbmdf6p1t.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] grep: handle corrupt index files early","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-17T17:21:23Z","receivedAt":"2018-05-17T17:21:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 16, 2018 at 6:36 PM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>> With a majority of call sites dying like this though, I wonder if we\n>> should just add repo_read_index_or_die() with die() inside. Then the\n>> next person won't likely accidentally forget _()\n>\n> Yuck.\n>\n> That sounds like inviting a major code churn. I tend to agree that\n> it would be a good clean-up for longer term maintenance, but I am\n> not sure if I can honestly say I'd look forward to such a clean-up\n> at this point in the cycle when there are tons of large-ish topics\n> in flight X-<.\n\nok, consider the series\nhttps://public-inbox.org/git/20180516222118.233868-1-sbeller@google.com/\nretracted for this cycle; I will keep it around and resend it at some future\ndate, hopefully.\n\nFeel free to comment on it, though.\n\nStefan\n"},{"id":"347962","messageId":"xmqq36yp6gaw.fsf@gitster-ct.c.googlers.com","threadId":"48497","inReplyTo":"CAGZ79kbvjoTq5079Ks+h2HNb+D99RELYPcJk2=pvZf9-Y8dToQ@mail.gmail.com","subject":"Re: [PATCH] grep: handle corrupt index files early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-17T22:57:59Z","receivedAt":"2018-05-17T22:58:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Wed, May 16, 2018 at 6:36 PM, Junio C Hamano wrote:\n>> Duy Nguyen writes:\n>>\n>>> With a majority of call sites dying like this though, I wonder if we\n>>> should just add repo_read_index_or_die() with die() inside. Then the\n>>> next person won't likely accidentally forget _()\n>>\n>> Yuck.\n>>\n>> That sounds like inviting a major code churn. I tend to agree that\n>> it would be a good clean-up for longer term maintenance, but I am\n>> not sure if I can honestly say I'd look forward to such a clean-up\n>> at this point in the cycle when there are tons of large-ish topics\n>> in flight X-<.\n>\n> ok, consider the series\n> https://public-inbox.org/git/20180516222118.233868-1-sbeller@google.com/\n> retracted for this cycle; I will keep it around and resend it at some future\n> date, hopefully.\n\nThanks. I didn't realize you've _already_ done that.\n"},{"id":"348068","messageId":"20180519063729.GA14755@duynguyen.home","threadId":"48497","inReplyTo":"20180516222118.233868-3-sbeller@google.com","subject":"Re: [PATCH 02/11] repository: introduce repo_read_index_or_die","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-19T06:37:29Z","receivedAt":"2018-05-19T06:37:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 16, 2018 at 03:21:09PM -0700, Stefan Beller wrote:\n> A common pattern with the repo_read_index function is to die if the return\n> of repo_read_index is negative. Move this pattern into a function.\n> \n> This patch replaces the calls of repo_read_index with its _or_die version,\n> if the error message is exactly the same.\n> \n> Signed-off-by: Stefan Beller \n> ---\n> builtin/add.c | 3 +--\n> builtin/check-ignore.c | 7 ++++---\n> builtin/clean.c | 4 ++--\n> builtin/mv.c | 3 +--\n> builtin/reset.c | 3 +--\n> builtin/rm.c | 3 +--\n> builtin/submodule--helper.c | 3 +--\n\n'git grep -w -A3 read_cache' tells me you're missing (*)\n\nbuiltin/diff-tree.c: if (read_cache() < 0)\nbuiltin/diff-tree.c- die(_(\"index file corrupt\"));\n\nbuiltin/merge-ours.c: if (read_cache() < 0)\nbuiltin/merge-ours.c: die_errno(\"read_cache failed\");\n\nbuiltin/update-index.c: entries = read_cache();\nbuiltin/update-index.c- if (entries < 0)\nbuiltin/update-index.c- die(\"cache corrupted\");\n\nmerge-ours.c is interesting because it uses die_errno() version\ninstead. I think that might give inaccurate diagnostics because we do\nnot only fail when a libc/system call fails in read_cache(), so it\nshould be safe to just use die() here.\n\nupdate-index.c is also interesting because it actually saves the\nreturn value. I don't think it's used anywhere, so you can just drop\nthat 'entries' variable. But perhaps it's good to make\nrepo_read_index_or_die() return the number of entries too.\n\n(*) yes yes you did mention \"if the error message is exactly the\nsame\". But these look like good candicates to convert anyway\n\n--\nDuy\n"},{"id":"348069","messageId":"20180519065411.GB14755@duynguyen.home","threadId":"48497","inReplyTo":"20180516222118.233868-12-sbeller@google.com","subject":"Re: [PATCH 11/11] read_cache: convert most calls to repo_read_index_or_die","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-19T06:54:12Z","receivedAt":"2018-05-19T06:54:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 16, 2018 at 03:21:18PM -0700, Stefan Beller wrote:\n\nThis commit may need some more explanation. It's not always safe to\nreplace \"read_cache();\" with \"repo_read_index_or_die();\" and you do\nsometimes avoid that variant.\n\nWhile I agree _or_die() is the right thing to do most of the time. You\nneed to consider that we (or some of us) have tried to avoid die() in\nsome library code. So..\n\n> Signed-off-by: Stefan Beller \n> ---\n> blame.c | 5 +++--\n> builtin/am.c | 3 ++-\n> builtin/diff.c | 3 ++-\n> builtin/fsck.c | 3 ++-\n> builtin/merge-index.c | 3 ++-\n\nThese should be good.\n\n> check-racy.c | 2 +-\n\nMeh.. this one is test code.\n\n> diff --git a/diff.c b/diff.c\n> index 1289df4b1f9..383f52fa118 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -22,6 +22,7 @@\n> #include \"argv-array.h\"\n> #include \"graph.h\"\n> #include \"packfile.h\"\n> +#include \"repository.h\"\n> \n> #ifdef NO_FAST_WORKING_DIRECTORY\n> #define FAST_WORKING_DIRECTORY 0\n> @@ -4210,13 +4211,13 @@ void diff_setup_done(struct diff_options *options)\n> \t\toptions->rename_limit = diff_rename_limit_default;\n> \tif (options->setup & DIFF_SETUP_USE_CACHE) {\n> \t\tif (!active_cache)\n> -\t\t\t/* read-cache does not die even when it fails\n> +\t\t\t/* repo_read_indexe does not die even when it fails\n\ns/indexe/index/\n\n> \t\t\t * so it is safe for us to do this here. Also\n> \t\t\t * it does not smudge active_cache or active_nr\n> \t\t\t * when it fails, so we do not have to worry about\n> \t\t\t * cleaning it up ourselves either.\n> \t\t\t */\n> -\t\t\tread_cache();\n> +\t\t\trepo_read_index(the_repository);\n\nShould we write a warning message or something?\n\n> diff --git a/revision.c b/revision.c\n> index 1cff11833e7..8ad9824143d 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -23,6 +23,7 @@\n> #include \"packfile.h\"\n> #include \"worktree.h\"\n> #include \"argv-array.h\"\n> +#include \"repository.h\"\n> \n> volatile show_early_output_fn_t show_early_output;\n> \n> @@ -1344,7 +1345,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)\n> {\n> \tstruct worktree **worktrees, **p;\n> \n> -\tread_cache();\n> +\trepo_read_index_or_die(the_repository);\n\nI think here we should be able to tolerate a bad index file. If you\nhave bad index file, we ignore object references from it and continue\non the rev walk without it. So repo_read_index() may be better,\noptionally with a warning.\n\n> diff --git a/sha1-name.c b/sha1-name.c\n> index 5b93bf8da36..83d5f945cf1 100644\n> --- a/sha1-name.c\n> +++ b/sha1-name.c\n> @@ -1639,7 +1639,7 @@ static int get_oid_with_context_1(const char *name,\n> \t\t\toc->path = xstrdup(cp);\n> \n> \t\tif (!active_cache)\n> -\t\t\tread_cache();\n> +\t\t\trepo_read_index_or_die(the_repository);\n\nThis should return an error code instead. I notice there's a\n\"only_to_die\" flag somewhere in here. Something to think about.\n\nBTW you probably want to move away from \"active_cache\" too. It makes\nsense to read \"if active cache is null then read the cache\". But with\nthe move to the repository it now seems disconnected.\n\nThis looks clearer but a bit verbose\n\n\t\tif (!the_repository->index_file.cache)\n\t\t\trepo_read_index_or_die(the_repository);\n\nThis might be the best\n\n\t\tif (!repo_index_loaded(the_repository))\n\t\t\trepo_read_index_or_die(the_repository);\n\nbut obviously more work for you, so your choice :)\n\n> \t\tpos = cache_name_pos(cp, namelen);\n> \t\tif (pos < 0)\n> \t\t\tpos = -pos - 1;\n> -- \n> 2.17.0.582.gccdcbd54c44.dirty\n> \n"},{"id":"348070","messageId":"20180519065545.GC14755@duynguyen.home","threadId":"48497","inReplyTo":"20180516222733.GB60301@google.com","subject":"Re: [PATCH 11/11] read_cache: convert most calls to repo_read_index_or_die","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-19T06:55:45Z","receivedAt":"2018-05-19T06:55:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 16, 2018 at 03:27:33PM -0700, Brandon Williams wrote:\n> Maybe we can kill read_cache() while at it?\n\nI took this out of context. But with the move to repo_* stuff,\neventually these macros around the_index should die. Not today though.\n--\nDuy\n"},{"id":"348071","messageId":"20180519065718.GD14755@duynguyen.home","threadId":"48497","inReplyTo":"20180516222118.233868-1-sbeller@google.com","subject":"Re: [PATCH 00/11]","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-19T06:57:18Z","receivedAt":"2018-05-19T06:57:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 16, 2018 at 03:21:07PM -0700, Stefan Beller wrote:\n> > If you have time, yes translate them all. I don't see how any of these\n> > strings are meant for script. If not, just _() the new string you\n> > added is fine.\n> \n> > With a majority of call sites dying like this though, I wonder if we\n> > should just add repo_read_index_or_die() with die() inside. Then the\n> > next person won't likely accidentally forget _()\n> \n> So this comment tricked me into coming up with a patch series. :)\n\nYay! (And sorry, Junio)\n\n> \n> Each patch is themed, I tried to make each commit special w.r.t. reviewers\n> attention.\n> \n> We'd start out with a resend of the origin patch, which is boring.\n> \n> Then we'll move all similar cases into one function (no-op for\n> introducing the repo_read_index_or_die function as all callers will have the\n> same error message and the same localisation).\n> \n> Any following patch will be more controversial then the previous patches,\n> I would expect, as we introduce more and more change.\n> \n> The last patch is just an attempt to finish the series gracefully,\n> and may contain errors (sometimes we do not want to die() in case of\n> corrupt index).\n> \n> Is this series roughly what you had in mind?\n\nThis is better than what I had in mind, in fact.\n\n--\nDuy\n"},{"id":"348108","messageId":"20180520174537.GA18193@hank.intra.tgummerer.com","threadId":"48497","inReplyTo":"20180516222118.233868-8-sbeller@google.com","subject":"Re: [PATCH 07/11] rerere: use repo_read_index_or_die","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-05-20T17:45:37Z","receivedAt":"2018-05-20T17:55:03Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 05/16, Stefan Beller wrote:\n> By switching to repo_read_index_or_die, we'll get a slightly different\n> error message (\"index file corrupt\") as well as localization of it.\n\nApart from the slightly different error message, and the localization\n(both of which I think are a good thing), I notice that this also\nturns a \"return error(...)\" into a \"die(...)\". I thought we were\ngoing more towards not 'die'ing in libgit.a code, and letting the\ncaller handling the errors? Either way I think this change should be\ndescribed in the commit message.\n\nAlso all other messages in 'rerere.c' are currently not translated.\nI'm currently working on a series that includes a patch to do just\nthat (amongst some other changes to 'rerere'), which I'm hoping to\nsend soon-ish, but the textual conflicts should we still want this\npatch should be easy to solve.\n\n> Signed-off-by: Stefan Beller \n> ---\n> rerere.c | 10 ++++------\n> 1 file changed, 4 insertions(+), 6 deletions(-)\n> \n> diff --git a/rerere.c b/rerere.c\n> index 18cae2d11c9..5f35dd66f90 100644\n> --- a/rerere.c\n> +++ b/rerere.c\n> @@ -10,6 +10,7 @@\n> #include \"attr.h\"\n> #include \"pathspec.h\"\n> #include \"sha1-lookup.h\"\n> +#include \"repository.h\"\n> \n> #define RESOLVED 0\n> #define PUNTED 1\n> @@ -567,8 +568,7 @@ static int check_one_conflict(int i, int *type)\n> static int find_conflict(struct string_list *conflict)\n> {\n> \tint i;\n> -\tif (read_cache() < 0)\n> -\t\treturn error(\"Could not read index\");\n> +\trepo_read_index_or_die(the_repository);\n> \n> \tfor (i = 0; i < active_nr;) {\n> \t\tint conflict_type;\n> @@ -600,8 +600,7 @@ int rerere_remaining(struct string_list *merge_rr)\n> \tint i;\n> \tif (setup_rerere(merge_rr, RERERE_READONLY))\n> \t\treturn 0;\n> -\tif (read_cache() < 0)\n> -\t\treturn error(\"Could not read index\");\n> +\trepo_read_index_or_die(the_repository);\n> \n> \tfor (i = 0; i < active_nr;) {\n> \t\tint conflict_type;\n> @@ -1103,8 +1102,7 @@ int rerere_forget(struct pathspec *pathspec)\n> \tstruct string_list conflict = STRING_LIST_INIT_DUP;\n> \tstruct string_list merge_rr = STRING_LIST_INIT_DUP;\n> \n> -\tif (read_cache() < 0)\n> -\t\treturn error(\"Could not read index\");\n> +\trepo_read_index_or_die(the_repository);\n> \n> \tfd = setup_rerere(&merge_rr, RERERE_NOAUTOUPDATE);\n> \tif (fd < 0)\n> -- \n> 2.17.0.582.gccdcbd54c44.dirty\n> \n"},{"id":"348236","messageId":"CAGZ79kbxptYvDoTqsVRe3KOA_--ja8UZir=MkMXw8_LxVXG_-Q@mail.gmail.com","threadId":"48497","inReplyTo":"20180519063729.GA14755@duynguyen.home","subject":"Re: [PATCH 02/11] repository: introduce repo_read_index_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-21T18:38:53Z","receivedAt":"2018-05-21T18:38:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 18, 2018 at 11:37 PM, Duy Nguyen wrote:\n> On Wed, May 16, 2018 at 03:21:09PM -0700, Stefan Beller wrote:\n>> A common pattern with the repo_read_index function is to die if the return\n>> of repo_read_index is negative. Move this pattern into a function.\n>>\n>> This patch replaces the calls of repo_read_index with its _or_die version,\n>> if the error message is exactly the same.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> builtin/add.c | 3 +--\n>> builtin/check-ignore.c | 7 ++++---\n>> builtin/clean.c | 4 ++--\n>> builtin/mv.c | 3 +--\n>> builtin/reset.c | 3 +--\n>> builtin/rm.c | 3 +--\n>> builtin/submodule--helper.c | 3 +--\n>\n> 'git grep -w -A3 read_cache' tells me you're missing (*)\n\n> (*) yes yes you did mention \"if the error message is exactly the\n> same\". But these look like good candicates to convert anyway\n>\n> builtin/diff-tree.c: if (read_cache() < 0)\n> builtin/diff-tree.c- die(_(\"index file corrupt\"));\n>\n\nI would expect this to be covered in a follow up patch as I did look\nfor (read_cache() < 0) specifically.\n\n> builtin/merge-ours.c: if (read_cache() < 0)\n> builtin/merge-ours.c: die_errno(\"read_cache failed\");\n>\n> builtin/update-index.c: entries = read_cache();\n> builtin/update-index.c- if (entries < 0)\n> builtin/update-index.c- die(\"cache corrupted\");\n>\n> merge-ours.c is interesting because it uses die_errno() version\n> instead. I think that might give inaccurate diagnostics because we do\n> not only fail when a libc/system call fails in read_cache(), so it\n> should be safe to just use die() here.\n>\n> update-index.c is also interesting because it actually saves the\n> return value. I don't think it's used anywhere, so you can just drop\n> that 'entries' variable. But perhaps it's good to make\n> repo_read_index_or_die() return the number of entries too.\n\nYeah this series left out all the interesting cases, as I just sent it out\nwithout much thought.\n\nReturning the number of entries makes sense.\n\nOne of the reviewers of the series questioned the overall goal of the\nseries as we want to move away from _die() in top level code but this\nseries moves more towards it.\n\nI don't know.\n\nStefan\n"},{"id":"348238","messageId":"CAGZ79kYkon8n9adR8AxUK9DuHGCEzJ136wgj+_q7hYn9wy-Kmg@mail.gmail.com","threadId":"48497","inReplyTo":"20180520174537.GA18193@hank.intra.tgummerer.com","subject":"Re: [PATCH 07/11] rerere: use repo_read_index_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-21T18:46:55Z","receivedAt":"2018-05-21T18:48:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi Thomas,\n\nOn Sun, May 20, 2018 at 10:45 AM, Thomas Gummerer wrote:\n> On 05/16, Stefan Beller wrote:\n>> By switching to repo_read_index_or_die, we'll get a slightly different\n>> error message (\"index file corrupt\") as well as localization of it.\n>\n> Apart from the slightly different error message, and the localization\n> (both of which I think are a good thing), I notice that this also\n> turns a \"return error(...)\" into a \"die(...)\". I thought we were\n> going more towards not 'die'ing in libgit.a code, and letting the\n> caller handling the errors? Either way I think this change should be\n> described in the commit message.\n\noops, I will to drop or fix this patch from the series as I think the\nnot die()ing is a good idea for libgit.\n\n> Also all other messages in 'rerere.c' are currently not translated.\n> I'm currently working on a series that includes a patch to do just\n> that (amongst some other changes to 'rerere'), which I'm hoping to\n> send soon-ish, but the textual conflicts should we still want this\n> patch should be easy to solve.\n\nI did not mark this series well enough, it was a mere attempt to\n\"see how it goes\", but was retracted as a whole[1] after Junio\ndreaded some merge conflicts.\n\n[1] https://public-inbox.org/git/CAGZ79kbvjoTq5079Ks+h2HNb+D99RELYPcJk2=pvZf9-Y8dToQ@mail.gmail.com/\n\nSo do not feel bad about any merge conflicts in rerere, as this\nseries will not land any time soon.\n\nStefan\n"},{"id":"348239","messageId":"20180521185040.GA125693@google.com","threadId":"48497","inReplyTo":"CAGZ79kbxptYvDoTqsVRe3KOA_--ja8UZir=MkMXw8_LxVXG_-Q@mail.gmail.com","subject":"Re: [PATCH 02/11] repository: introduce repo_read_index_or_die","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-05-21T18:50:40Z","receivedAt":"2018-05-21T18:50:47Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/21, Stefan Beller wrote:\n> On Fri, May 18, 2018 at 11:37 PM, Duy Nguyen wrote:\n> > On Wed, May 16, 2018 at 03:21:09PM -0700, Stefan Beller wrote:\n> >> A common pattern with the repo_read_index function is to die if the return\n> >> of repo_read_index is negative. Move this pattern into a function.\n> >>\n> >> This patch replaces the calls of repo_read_index with its _or_die version,\n> >> if the error message is exactly the same.\n> >>\n> >> Signed-off-by: Stefan Beller \n> >> ---\n> >> builtin/add.c | 3 +--\n> >> builtin/check-ignore.c | 7 ++++---\n> >> builtin/clean.c | 4 ++--\n> >> builtin/mv.c | 3 +--\n> >> builtin/reset.c | 3 +--\n> >> builtin/rm.c | 3 +--\n> >> builtin/submodule--helper.c | 3 +--\n> >\n> > 'git grep -w -A3 read_cache' tells me you're missing (*)\n> \n> > (*) yes yes you did mention \"if the error message is exactly the\n> > same\". But these look like good candicates to convert anyway\n> >\n> > builtin/diff-tree.c: if (read_cache() < 0)\n> > builtin/diff-tree.c- die(_(\"index file corrupt\"));\n> >\n> \n> I would expect this to be covered in a follow up patch as I did look\n> for (read_cache() < 0) specifically.\n> \n> > builtin/merge-ours.c: if (read_cache() < 0)\n> > builtin/merge-ours.c: die_errno(\"read_cache failed\");\n> >\n> > builtin/update-index.c: entries = read_cache();\n> > builtin/update-index.c- if (entries < 0)\n> > builtin/update-index.c- die(\"cache corrupted\");\n> >\n> > merge-ours.c is interesting because it uses die_errno() version\n> > instead. I think that might give inaccurate diagnostics because we do\n> > not only fail when a libc/system call fails in read_cache(), so it\n> > should be safe to just use die() here.\n> >\n> > update-index.c is also interesting because it actually saves the\n> > return value. I don't think it's used anywhere, so you can just drop\n> > that 'entries' variable. But perhaps it's good to make\n> > repo_read_index_or_die() return the number of entries too.\n> \n> Yeah this series left out all the interesting cases, as I just sent it out\n> without much thought.\n> \n> Returning the number of entries makes sense.\n> \n> One of the reviewers of the series questioned the overall goal of the\n> series as we want to move away from _die() in top level code but this\n> series moves more towards it.\n\nI've heard every once in a while that we want to move toward this but I\ndon't believe there is an actual effort along those lines just yet. For\nthat to be the case we would need a clearly defined error handling\nmethodology (aside from the existing \"die\"ing behavior), which we don't\ncurrently have.\n\n> \n> I don't know.\n> \n> Stefan\n\n-- \nBrandon Williams\n"},{"id":"348242","messageId":"CAGZ79kZ0TBzDMqpim1Wtr9QtF2OtskE=nRPx2Nj+rwyT8HMqTw@mail.gmail.com","threadId":"48497","inReplyTo":"20180521185040.GA125693@google.com","subject":"Re: [PATCH 02/11] repository: introduce repo_read_index_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-21T19:27:04Z","receivedAt":"2018-05-21T19:27:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi Brandon,\n\n>> One of the reviewers of the series questioned the overall goal of the\n>> series as we want to move away from _die() in top level code but this\n>> series moves more towards it.\n>\n> I've heard every once in a while that we want to move toward this but I\n> don't believe there is an actual effort along those lines just yet. For\n> that to be the case we would need a clearly defined error handling\n> methodology (aside from the existing \"die\"ing behavior), which we don't\n> currently have.\n\nWe have the example in the refs code, which I would want to\nimitate. :)\n\n/*\n * Return 0 if a reference named refname could be created without\n * conflicting with the name of an existing reference. Otherwise,\n * return a negative value and write an explanation to err. [...]\n */\n\nint refs_verify_refname_available(struct ref_store *refs, ...\n struct strbuf *err);\n\nextern int refs_init_db(struct strbuf *err);\n\nBut it is true that there is no active effort currently being pushed.\n\nThanks,\nStefan\n"},{"id":"348310","messageId":"CACsJy8DfF+Bn94dQH2cSFfHpXqHa07vstAKrJeRhd-5EP8avWA@mail.gmail.com","threadId":"48497","inReplyTo":"CAGZ79kZ0TBzDMqpim1Wtr9QtF2OtskE=nRPx2Nj+rwyT8HMqTw@mail.gmail.com","subject":"Re: [PATCH 02/11] repository: introduce repo_read_index_or_die","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-22T15:13:06Z","receivedAt":"2018-05-22T15:13:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, May 21, 2018 at 9:27 PM, Stefan Beller wrote:\n> Hi Brandon,\n>\n>>> One of the reviewers of the series questioned the overall goal of the\n>>> series as we want to move away from _die() in top level code but this\n>>> series moves more towards it.\n>>\n>> I've heard every once in a while that we want to move toward this but I\n>> don't believe there is an actual effort along those lines just yet. For\n>> that to be the case we would need a clearly defined error handling\n>> methodology (aside from the existing \"die\"ing behavior), which we don't\n>> currently have.\n>\n> We have the example in the refs code, which I would want to\n> imitate. :)\n>\n> /*\n> * Return 0 if a reference named refname could be created without\n> * conflicting with the name of an existing reference. Otherwise,\n> * return a negative value and write an explanation to err. [...]\n> */\n>\n> int refs_verify_refname_available(struct ref_store *refs, ...\n> struct strbuf *err);\n>\n> extern int refs_init_db(struct strbuf *err);\n\napply.c and sequencer.c too are libified and try hard to avoid die()\nif I remember correctly.\n\n> But it is true that there is no active effort currently being pushed.\n\nYeah. It took a lot of work to put refs code in the current shape\ntoday. I think we just be careful about adding die(). If a function\nhas no way to return an error, then die() is unavoidable. If it's a\nreally fatal error, then die() might still be the right choice. But if\nnot we should try to return an error instead.\n\nSpeaking of avoiding die() (and going off-topic again), maybe we\nshould introduce NO_DIE_PLEASE macro in the same spirit as\nNO_THE_INDEX_COMPATIBILITY_MACROS. If the macro is defined, die() and\nfriends are not declared (which will result in compile errors at least\non DEVELOPER=1 builds). This helps draw boundary between die-able code\nand un-die-able code (not perfect, but good enough)\n-- \nDuy\n"},{"id":"348313","messageId":"874liz8tsi.fsf@evledraar.gmail.com","threadId":"48497","inReplyTo":"20180516222118.233868-3-sbeller@google.com","subject":"Why do we have both x*() and *_or_die() for \"do or die\"?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-22T17:49:33Z","receivedAt":"2018-05-22T17:49:40Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, May 16 2018, Stefan Beller wrote:\n\n> A common pattern with the repo_read_index function is to die if the return\n> of repo_read_index is negative. Move this pattern into a function.\n\nJust a side-question unrelated to this patch per-se, why do we have both\nx*() and *_or_die() functions in the codebase? I can't find any pattern\nfor one or the other, e.g. we have both xopen() and then write_or_die(),\nso it's not a matter of x*() just being for syscalls and *_or_die()\nbeing for our own functions (also as e.g. strbuf uses x*(), not\n*_or_die()).\n\nI'm not trying to litigate the difference and understand it could have\njust emerged organically. I'm just wondering if that's the full story or\nif one is preferred, or we prefer one or the other in some\ncircumstances.\n"},{"id":"348314","messageId":"CACsJy8BnsbOvz+iSz=9ug99Cs5Dm17Fgk65K2nXusBg0dML4xg@mail.gmail.com","threadId":"48497","inReplyTo":"874liz8tsi.fsf@evledraar.gmail.com","subject":"Re: Why do we have both x*() and *_or_die() for \"do or die\"?","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-22T17:55:29Z","receivedAt":"2018-05-22T17:56:02Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 22, 2018 at 7:49 PM, Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Wed, May 16 2018, Stefan Beller wrote:\n>\n>> A common pattern with the repo_read_index function is to die if the return\n>> of repo_read_index is negative. Move this pattern into a function.\n>\n> Just a side-question unrelated to this patch per-se, why do we have both\n> x*() and *_or_die() functions in the codebase?\n\nI wondered about that myself shortly after suggesting\nrepo_read_index_or_die(). My only guess is xfoo is better version of\nfoo, which sometimes involves dying inside but that's not the only\npossible improvement. Later I guess people go with _or_die() more\nbecause it describes what the function does much better.\n\n> I can't find any pattern\n> for one or the other, e.g. we have both xopen() and then write_or_die(),\n> so it's not a matter of x*() just being for syscalls and *_or_die()\n> being for our own functions (also as e.g. strbuf uses x*(), not\n> *_or_die()).\n>\n> I'm not trying to litigate the difference and understand it could have\n> just emerged organically. I'm just wondering if that's the full story or\n> if one is preferred, or we prefer one or the other in some\n> circumstances.\n\n\n\n-- \nDuy\n"},{"id":"348315","messageId":"CAGZ79kZs7aNXB2cn8p+m5gUOAv0OqhPW6b23y4cbm9d3Njwn=A@mail.gmail.com","threadId":"48497","inReplyTo":"874liz8tsi.fsf@evledraar.gmail.com","subject":"Re: Why do we have both x*() and *_or_die() for \"do or die\"?","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-22T18:04:20Z","receivedAt":"2018-05-22T18:04:25Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 22, 2018 at 10:49 AM, Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Wed, May 16 2018, Stefan Beller wrote:\n>\n>> A common pattern with the repo_read_index function is to die if the return\n>> of repo_read_index is negative. Move this pattern into a function.\n\nThis was done organically, i.e. taking the smallest available step that yields\na better code base. There was definitely no stepping back or looking at the\nbig picture involved.\n\n> Just a side-question unrelated to this patch per-se, why do we have both\n> x*() and *_or_die() functions in the codebase?\n\nDigging into the history, the x*() emerges from 112db553b0d (Shrink the\ngit binary a bit by avoiding unnecessary inline functions, 2008-06-22) and\nis wrapping common patterns to reduce the size of the binary.\n\nThe or_die pattern comes from 7230e6d042a (Add write_or_die(),\na helper function, 2006-08-21). This was motivated as better code\n(\"making additional error handling unnecessary\", \"replace two similar ones\")\n\nNone of them really hint at the _die() as a problem in the\nlibification efforts which came later IIUC.\n\n> I can't find any pattern\n> for one or the other, e.g. we have both xopen() and then write_or_die(),\n\nhah! Up to now I assumed the x* is system calls with write_or_die as an\nexception. There is also a xwrite and write_in_full, so they are slightly\ndifferent in the way that xwrite covers only the system call and copes\nwith some common error patterns, whereas write_or_die uses write_in_full\nwhich itself uses xwrite.\n\n> so it's not a matter of x*() just being for syscalls and *_or_die()\n> being for our own functions (also as e.g. strbuf uses x*(), not\n> *_or_die()).\n\nI would tend to argue that way: x* are strictly to system calls (no die()\nin there), and the _die() functions are \"hands off *no* error handling\nneeded\"\n\n> I'm not trying to litigate the difference and understand it could have\n> just emerged organically. I'm just wondering if that's the full story or\n> if one is preferred, or we prefer one or the other in some\n> circumstances.\n\nI think we'd prefer the x() for lib code and the _or_die code in builtin/\ndue to the nature of effort needed to get it right.\n\nThanks,\nStefan\n"},{"id":"348318","messageId":"20180522183807.GK10623@aiede.svl.corp.google.com","threadId":"48497","inReplyTo":"CACsJy8BnsbOvz+iSz=9ug99Cs5Dm17Fgk65K2nXusBg0dML4xg@mail.gmail.com","subject":"Re: Why do we have both x*() and *_or_die() for \"do or die\"?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-05-22T18:38:07Z","receivedAt":"2018-05-22T18:38:13Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Duy Nguyen wrote:\n> On Tue, May 22, 2018 at 7:49 PM, Ævar Arnfjörð Bjarmason\n> wrote:\n\n>> Just a side-question unrelated to this patch per-se, why do we have both\n>> x*() and *_or_die() functions in the codebase?\n>\n> I wondered about that myself shortly after suggesting\n> repo_read_index_or_die(). My only guess is xfoo is better version of\n> foo, which sometimes involves dying inside but that's not the only\n> possible improvement. Later I guess people go with _or_die() more\n> because it describes what the function does much better.\n\nIn particular, there are functions like xwrite that don't die on error\nand write_or_die that do.\n\nI'd probably be in favor of a series using cocinelle to rename the\nfunctions that do die on error to _or_die. The main case where I\npause for a moment is xmalloc, since I'm worried about the verboseness\nof malloc_or_die, but I suspect I would get used to it.\n\nThanks,\nJonathan\n"},{"id":"348348","messageId":"xmqq4lizvz2v.fsf@gitster-ct.c.googlers.com","threadId":"48497","inReplyTo":"874liz8tsi.fsf@evledraar.gmail.com","subject":"Re: Why do we have both x*() and *_or_die() for \"do or die\"?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-23T03:19:04Z","receivedAt":"2018-05-23T03:19:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Just a side-question unrelated to this patch per-se, why do we have both\n> x*() and *_or_die() functions in the codebase? I can't find any pattern\n> for one or the other.\n\nMy understanding is that x*() were meant for system library\nfunctions. read-index-or-die should never be x-read-index.\n\nQuite honestly, read-index should probably have designed to die from\nthe beginning, with read-index-gently as a variant to report an\nerror instead of dying.\n"}]}