[PATCH] grep: handle corrupt index files early

[PATCH] grep: handle corrupt index files early

[Stefan Beller]

Any other caller of 'repo_read_index' dies upon a negative return of
it, so grep should, too.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Found while reviewing the series
https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/

 builtin/grep.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 6e7bc76785a..69f0743619f 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,
 		strbuf_addstr(&name, repo->submodule_prefix);
 	}
 
-	repo_read_index(repo);
+	if (repo_read_index(repo) < 0)
+		die("index file corrupt");
 
 	for (nr = 0; nr < repo->index->cache_nr; nr++) {
 		const struct cache_entry *ce = repo->index->cache[nr];
-- 
2.17.0.582.gccdcbd54c44.dirty

Re: [PATCH] grep: handle corrupt index files early

[Johannes Schindelin]

Hi Stefan,

On Mon, 14 May 2018, Stefan Beller wrote:

> Any other caller of 'repo_read_index' dies upon a negative return of
> it, so grep should, too.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> Found while reviewing the series
> https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/
> 
>  builtin/grep.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 6e7bc76785a..69f0743619f 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,
>  		strbuf_addstr(&name, repo->submodule_prefix);
>  	}
>  
> -	repo_read_index(repo);
> +	if (repo_read_index(repo) < 0)
> +		die("index file corrupt");

Looks obviously correct, thanks!

Ciao,
Dscho

Re: [PATCH] grep: handle corrupt index files early

[Duy Nguyen]

On Tue, May 15, 2018 at 3:04 AM, Stefan Beller <sbeller@google.com> wrote:
> Any other caller of 'repo_read_index' dies upon a negative return of
> it, so grep should, too.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Found while reviewing the series
> https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/
>
>  builtin/grep.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 6e7bc76785a..69f0743619f 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,
>                 strbuf_addstr(&name, repo->submodule_prefix);
>         }
>
> -       repo_read_index(repo);
> +       if (repo_read_index(repo) < 0)
> +               die("index file corrupt");

_() the string (and maybe reuse an existing phrase if found to reduce
workload on translators)

>
>         for (nr = 0; nr < repo->index->cache_nr; nr++) {
>                 const struct cache_entry *ce = repo->index->cache[nr];
> --
> 2.17.0.582.gccdcbd54c44.dirty
>



-- 
Duy

Re: [PATCH] grep: handle corrupt index files early

[Stefan Beller]

On Tue, May 15, 2018 at 6:13 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, May 15, 2018 at 3:04 AM, Stefan Beller <sbeller@google.com> wrote:
>> Any other caller of 'repo_read_index' dies upon a negative return of
>> it, so grep should, too.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> Found while reviewing the series
>> https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/
>>
>>  builtin/grep.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/grep.c b/builtin/grep.c
>> index 6e7bc76785a..69f0743619f 100644
>> --- a/builtin/grep.c
>> +++ b/builtin/grep.c
>> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,
>>                 strbuf_addstr(&name, repo->submodule_prefix);
>>         }
>>
>> -       repo_read_index(repo);
>> +       if (repo_read_index(repo) < 0)
>> +               die("index file corrupt");
>
> _() the string (and maybe reuse an existing phrase if found to reduce
> workload on translators)

sbeller@sbeller:/u/git$ git grep -A 1 repo_read_index
builtin/grep.c:491:     if (repo_read_index(repo) < 0)
builtin/grep.c-492-             die("index file corrupt");
--
builtin/ls-files.c:213: if (repo_read_index(&submodule) < 0)
builtin/ls-files.c-214-         die("index file corrupt");
--
builtin/ls-files.c:582: if (repo_read_index(the_repository) < 0)
builtin/ls-files.c-583-         die("index file corrupt");
--
dir.c:3028:     if (repo_read_index(&subrepo) < 0)
dir.c-3029-             die("index file corrupt in repo %s", subrepo.gitdir);
--
repository.c:245:int repo_read_index(struct repository *repo)
repository.c-246-{
--
repository.h:70:         * 'repo_read_index()' can be used to populate 'index'.
repository.h-71-         */
--
repository.h:119:extern int repo_read_index(struct repository *repo);
repository.h-120-
--
submodule-config.c:583:         if (repo_read_index(repo) < 0)
submodule-config.c-584-                 return;
--
submodule.c:1336:       if (repo_read_index(r) < 0)
submodule.c-1337-               die("index file corrupt");

I think this is as good as it gets for using an existing phrase.
None of them are translated, which I would defer to a follow up patch
that translates all(?) of them or just the porcelains.

Thanks,
Stefan

Re: [PATCH] grep: handle corrupt index files early

[Duy Nguyen]

On Tue, May 15, 2018 at 6:44 PM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, May 15, 2018 at 6:13 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Tue, May 15, 2018 at 3:04 AM, Stefan Beller <sbeller@google.com> wrote:
>>> Any other caller of 'repo_read_index' dies upon a negative return of
>>> it, so grep should, too.
>>>
>>> Signed-off-by: Stefan Beller <sbeller@google.com>
>>> ---
>>>
>>> Found while reviewing the series
>>> https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/
>>>
>>>  builtin/grep.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/builtin/grep.c b/builtin/grep.c
>>> index 6e7bc76785a..69f0743619f 100644
>>> --- a/builtin/grep.c
>>> +++ b/builtin/grep.c
>>> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,
>>>                 strbuf_addstr(&name, repo->submodule_prefix);
>>>         }
>>>
>>> -       repo_read_index(repo);
>>> +       if (repo_read_index(repo) < 0)
>>> +               die("index file corrupt");
>>
>> _() the string (and maybe reuse an existing phrase if found to reduce
>> workload on translators)
>
> sbeller@sbeller:/u/git$ git grep -A 1 repo_read_index
> builtin/grep.c:491:     if (repo_read_index(repo) < 0)
> builtin/grep.c-492-             die("index file corrupt");
> --
> builtin/ls-files.c:213: if (repo_read_index(&submodule) < 0)
> builtin/ls-files.c-214-         die("index file corrupt");
> --
> builtin/ls-files.c:582: if (repo_read_index(the_repository) < 0)
> builtin/ls-files.c-583-         die("index file corrupt");
> --
> dir.c:3028:     if (repo_read_index(&subrepo) < 0)
> dir.c-3029-             die("index file corrupt in repo %s", subrepo.gitdir);
> --
> repository.c:245:int repo_read_index(struct repository *repo)
> repository.c-246-{
> --
> repository.h:70:         * 'repo_read_index()' can be used to populate 'index'.
> repository.h-71-         */
> --
> repository.h:119:extern int repo_read_index(struct repository *repo);
> repository.h-120-
> --
> submodule-config.c:583:         if (repo_read_index(repo) < 0)
> submodule-config.c-584-                 return;
> --
> submodule.c:1336:       if (repo_read_index(r) < 0)
> submodule.c-1337-               die("index file corrupt");
>
> I think this is as good as it gets for using an existing phrase.
> None of them are translated, which I would defer to a follow up patch
> that translates all(?) of them or just the porcelains.

If you have time, yes translate them all. I don't see how any of these
strings are meant for script. If not, just _() the new string you
added is fine.

With a majority of call sites dying like this though, I wonder if we
should just add repo_read_index_or_die() with die() inside. Then the
next person won't likely accidentally forget _()

>
> Thanks,
> Stefan



-- 
Duy

[PATCH 00/11]

[Stefan Beller]

> If you have time, yes translate them all. I don't see how any of these
> strings are meant for script. If not, just _() the new string you
> added is fine.

> With a majority of call sites dying like this though, I wonder if we
> should just add repo_read_index_or_die() with die() inside. Then the
> next person won't likely accidentally forget _()

So this comment tricked me into coming up with a patch series. :)

Each patch is themed, I tried to make each commit special w.r.t. reviewers
attention.

We'd start out with a resend of the origin patch, which is boring.

Then we'll move all similar cases into one function (no-op for
introducing the repo_read_index_or_die function as all callers will have the
same error message and the same localisation).

Any following patch will be more controversial then the previous patches,
I would expect, as we introduce more and more change.

The last patch is just an attempt to finish the series gracefully,
and may contain errors (sometimes we do not want to die() in case of
corrupt index).

Is this series roughly what you had in mind?

Thanks,
Stefan


Stefan Beller (11):
  grep: handle corrupt index files early
  repository: introduce repo_read_index_or_die
  builtin/grep: use repo_read_index_or_die
  submodule: use repo_read_index_or_die
  builtin/ls-files: use repo_read_index_or_die
  read_cache: use repo_read_index_or_die with different error messages
  rerere: use repo_read_index_or_die
  check-attr: switch to repo_read_index_or_die
  checkout-index: switch to repo_read_index
  test helpers: switch to repo_read_index_or_die
  read_cache: convert most calls to repo_read_index_or_die

 blame.c                              |  5 +++--
 builtin/add.c                        |  7 +++----
 builtin/am.c                         |  3 ++-
 builtin/check-attr.c                 |  5 ++---
 builtin/check-ignore.c               |  7 ++++---
 builtin/checkout-index.c             |  5 ++---
 builtin/clean.c                      |  4 ++--
 builtin/commit.c                     |  9 +++++----
 builtin/diff.c                       |  3 ++-
 builtin/fsck.c                       |  3 ++-
 builtin/grep.c                       |  2 +-
 builtin/ls-files.c                   |  7 +++----
 builtin/merge-index.c                |  3 ++-
 builtin/mv.c                         |  3 +--
 builtin/reset.c                      |  3 +--
 builtin/rev-parse.c                  |  4 ++--
 builtin/rm.c                         |  3 +--
 builtin/submodule--helper.c          |  3 +--
 check-racy.c                         |  2 +-
 diff.c                               |  5 +++--
 merge-recursive.c                    |  3 ++-
 merge.c                              |  4 ++--
 repository.c                         |  6 ++++++
 repository.h                         |  8 ++++++++
 rerere.c                             | 10 ++++------
 revision.c                           |  5 +++--
 sequencer.c                          |  5 +++--
 sha1-name.c                          |  2 +-
 submodule.c                          |  3 +--
 t/helper/test-dump-cache-tree.c      |  5 ++---
 t/helper/test-dump-untracked-cache.c |  4 ++--
 t/helper/test-lazy-init-name-hash.c  | 11 ++++++-----
 t/helper/test-read-cache.c           |  3 ++-
 t/helper/test-scrap-cache-tree.c     |  4 ++--
 t/helper/test-write-cache.c          |  3 ++-
 35 files changed, 89 insertions(+), 73 deletions(-)

-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 01/11] grep: handle corrupt index files early

[Stefan Beller]

Any other caller of 'repo_read_index' dies upon a negative return of
it, so grep should, too.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/grep.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 6e7bc76785a..69f0743619f 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,
 		strbuf_addstr(&name, repo->submodule_prefix);
 	}
 
-	repo_read_index(repo);
+	if (repo_read_index(repo) < 0)
+		die("index file corrupt");
 
 	for (nr = 0; nr < repo->index->cache_nr; nr++) {
 		const struct cache_entry *ce = repo->index->cache[nr];
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 02/11] repository: introduce repo_read_index_or_die

[Stefan Beller]

A common pattern with the repo_read_index function is to die if the return
of repo_read_index is negative.  Move this pattern into a function.

This patch replaces the calls of repo_read_index with its _or_die version,
if the error message is exactly the same.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/add.c               | 3 +--
 builtin/check-ignore.c      | 7 ++++---
 builtin/clean.c             | 4 ++--
 builtin/mv.c                | 3 +--
 builtin/reset.c             | 3 +--
 builtin/rm.c                | 3 +--
 builtin/submodule--helper.c | 3 +--
 repository.c                | 6 ++++++
 repository.h                | 8 ++++++++
 9 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index c9e2619a9ad..e4751c198c1 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -445,8 +445,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		return 0;
 	}
 
-	if (read_cache() < 0)
-		die(_("index file corrupt"));
+	repo_read_index_or_die(the_repository);
 
 	die_in_unpopulated_submodule(&the_index, prefix);
 
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index ec9a959e08d..2a46bf9af7f 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -6,6 +6,7 @@
 #include "pathspec.h"
 #include "parse-options.h"
 #include "submodule.h"
+#include "repository.h"
 
 static int quiet, verbose, stdin_paths, show_non_matching, no_index;
 static const char * const check_ignore_usage[] = {
@@ -172,9 +173,9 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix)
 	if (show_non_matching && !verbose)
 		die(_("--non-matching is only valid with --verbose"));
 
-	/* read_cache() is only necessary so we can watch out for submodules. */
-	if (!no_index && read_cache() < 0)
-		die(_("index file corrupt"));
+	/* repo_read_index() is only necessary so we can watch out for submodules. */
+	if (!no_index)
+		repo_read_index_or_die(the_repository);
 
 	memset(&dir, 0, sizeof(dir));
 	setup_standard_excludes(&dir);
diff --git a/builtin/clean.c b/builtin/clean.c
index fad533a0a73..6c1c6fdd7f9 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -16,6 +16,7 @@
 #include "column.h"
 #include "color.h"
 #include "pathspec.h"
+#include "repository.h"
 
 static int force = -1; /* unset */
 static int interactive;
@@ -954,8 +955,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	if (remove_directories)
 		dir.flags |= DIR_SHOW_IGNORED_TOO | DIR_KEEP_UNTRACKED_CONTENTS;
 
-	if (read_cache() < 0)
-		die(_("index file corrupt"));
+	repo_read_index_or_die(the_repository);
 
 	if (!ignored)
 		setup_standard_excludes(&dir);
diff --git a/builtin/mv.c b/builtin/mv.c
index 7a63667d648..7f62e626dda 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -140,8 +140,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_mv_usage, builtin_mv_options);
 
 	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
-	if (read_cache() < 0)
-		die(_("index file corrupt"));
+	repo_read_index_or_die(the_repository);
 
 	source = internal_prefix_pathspec(prefix, argv, argc, 0);
 	modes = xcalloc(argc, sizeof(enum update_mode));
diff --git a/builtin/reset.c b/builtin/reset.c
index 7f1c3f02a30..fd514eec822 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -237,8 +237,7 @@ static void parse_args(struct pathspec *pathspec,
 	}
 	*rev_ret = rev;
 
-	if (read_cache() < 0)
-		die(_("index file corrupt"));
+	repo_read_index_or_die(the_repository);
 
 	parse_pathspec(pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
diff --git a/builtin/rm.c b/builtin/rm.c
index 5b6fc7ee818..3b90191aa53 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -267,8 +267,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 
 	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
 
-	if (read_cache() < 0)
-		die(_("index file corrupt"));
+	repo_read_index_or_die(the_repository);
 
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD,
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c2403a915ff..7aebed9bd66 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -323,8 +323,7 @@ static int module_list_compute(int argc, const char **argv,
 	if (pathspec->nr)
 		ps_matched = xcalloc(pathspec->nr, 1);
 
-	if (read_cache() < 0)
-		die(_("index file corrupt"));
+	repo_read_index_or_die(the_repository);
 
 	for (i = 0; i < active_nr; i++) {
 		const struct cache_entry *ce = active_cache[i];
diff --git a/repository.c b/repository.c
index beff3caa9e2..ceca14ba718 100644
--- a/repository.c
+++ b/repository.c
@@ -249,3 +249,9 @@ int repo_read_index(struct repository *repo)
 
 	return read_index_from(repo->index, repo->index_file, repo->gitdir);
 }
+
+void repo_read_index_or_die(struct repository *repo)
+{
+	if (repo_read_index(repo) < 0)
+		die(_("index file corrupt"));
+}
diff --git a/repository.h b/repository.h
index f2646f0c52a..8efd8979ad3 100644
--- a/repository.h
+++ b/repository.h
@@ -118,4 +118,12 @@ extern void repo_clear(struct repository *repo);
  */
 extern int repo_read_index(struct repository *repo);
 
+/*
+ * Populates the index in the repository from its index file,
+ * allocating the struct index if needed.
+ *
+ * If the index file cannot be read, die.
+ */
+void repo_read_index_or_die(struct repository *r);
+
 #endif /* REPOSITORY_H */
-- 
2.17.0.582.gccdcbd54c44.dirty

Re: [PATCH 02/11] repository: introduce repo_read_index_or_die

[Duy Nguyen]

On Wed, May 16, 2018 at 03:21:09PM -0700, Stefan Beller wrote:
> A common pattern with the repo_read_index function is to die if the return
> of repo_read_index is negative.  Move this pattern into a function.
> 
> This patch replaces the calls of repo_read_index with its _or_die version,
> if the error message is exactly the same.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/add.c               | 3 +--
>  builtin/check-ignore.c      | 7 ++++---
>  builtin/clean.c             | 4 ++--
>  builtin/mv.c                | 3 +--
>  builtin/reset.c             | 3 +--
>  builtin/rm.c                | 3 +--
>  builtin/submodule--helper.c | 3 +--

'git grep -w -A3 read_cache' tells me you're missing (*)

builtin/diff-tree.c:    if (read_cache() < 0)
builtin/diff-tree.c-            die(_("index file corrupt"));

builtin/merge-ours.c:   if (read_cache() < 0)
builtin/merge-ours.c:           die_errno("read_cache failed");

builtin/update-index.c: entries = read_cache();
builtin/update-index.c- if (entries < 0)
builtin/update-index.c-         die("cache corrupted");

merge-ours.c is interesting because it uses die_errno() version
instead. I think that might give inaccurate diagnostics because we do
not only fail when a libc/system call fails in read_cache(), so it
should be safe to just use die() here.

update-index.c is also interesting because it actually saves the
return value. I don't think it's used anywhere, so you can just drop
that 'entries' variable. But perhaps it's good to make
repo_read_index_or_die() return the number of entries too.

(*) yes yes you did mention "if the error message is exactly the
same". But these look like good candicates to convert anyway

--
Duy

Re: [PATCH 02/11] repository: introduce repo_read_index_or_die

[Stefan Beller]

On Fri, May 18, 2018 at 11:37 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, May 16, 2018 at 03:21:09PM -0700, Stefan Beller wrote:
>> A common pattern with the repo_read_index function is to die if the return
>> of repo_read_index is negative.  Move this pattern into a function.
>>
>> This patch replaces the calls of repo_read_index with its _or_die version,
>> if the error message is exactly the same.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  builtin/add.c               | 3 +--
>>  builtin/check-ignore.c      | 7 ++++---
>>  builtin/clean.c             | 4 ++--
>>  builtin/mv.c                | 3 +--
>>  builtin/reset.c             | 3 +--
>>  builtin/rm.c                | 3 +--
>>  builtin/submodule--helper.c | 3 +--
>
> 'git grep -w -A3 read_cache' tells me you're missing (*)

> (*) yes yes you did mention "if the error message is exactly the
> same". But these look like good candicates to convert anyway
>
> builtin/diff-tree.c:    if (read_cache() < 0)
> builtin/diff-tree.c-            die(_("index file corrupt"));
>

I would expect this to be covered in a follow up patch as I did look
for (read_cache() < 0) specifically.

> builtin/merge-ours.c:   if (read_cache() < 0)
> builtin/merge-ours.c:           die_errno("read_cache failed");
>
> builtin/update-index.c: entries = read_cache();
> builtin/update-index.c- if (entries < 0)
> builtin/update-index.c-         die("cache corrupted");
>
> merge-ours.c is interesting because it uses die_errno() version
> instead. I think that might give inaccurate diagnostics because we do
> not only fail when a libc/system call fails in read_cache(), so it
> should be safe to just use die() here.
>
> update-index.c is also interesting because it actually saves the
> return value. I don't think it's used anywhere, so you can just drop
> that 'entries' variable. But perhaps it's good to make
> repo_read_index_or_die() return the number of entries too.

Yeah this series left out all the interesting cases, as I just sent it out
without much thought.

Returning the number of entries makes sense.

One of the reviewers of the series questioned the overall goal of the
series as we want to move away from _die() in top level code but this
series moves more towards it.

I don't know.

Stefan

Re: [PATCH 02/11] repository: introduce repo_read_index_or_die

[Brandon Williams]

On 05/21, Stefan Beller wrote:
> On Fri, May 18, 2018 at 11:37 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> > On Wed, May 16, 2018 at 03:21:09PM -0700, Stefan Beller wrote:
> >> A common pattern with the repo_read_index function is to die if the return
> >> of repo_read_index is negative.  Move this pattern into a function.
> >>
> >> This patch replaces the calls of repo_read_index with its _or_die version,
> >> if the error message is exactly the same.
> >>
> >> Signed-off-by: Stefan Beller <sbeller@google.com>
> >> ---
> >>  builtin/add.c               | 3 +--
> >>  builtin/check-ignore.c      | 7 ++++---
> >>  builtin/clean.c             | 4 ++--
> >>  builtin/mv.c                | 3 +--
> >>  builtin/reset.c             | 3 +--
> >>  builtin/rm.c                | 3 +--
> >>  builtin/submodule--helper.c | 3 +--
> >
> > 'git grep -w -A3 read_cache' tells me you're missing (*)
> 
> > (*) yes yes you did mention "if the error message is exactly the
> > same". But these look like good candicates to convert anyway
> >
> > builtin/diff-tree.c:    if (read_cache() < 0)
> > builtin/diff-tree.c-            die(_("index file corrupt"));
> >
> 
> I would expect this to be covered in a follow up patch as I did look
> for (read_cache() < 0) specifically.
> 
> > builtin/merge-ours.c:   if (read_cache() < 0)
> > builtin/merge-ours.c:           die_errno("read_cache failed");
> >
> > builtin/update-index.c: entries = read_cache();
> > builtin/update-index.c- if (entries < 0)
> > builtin/update-index.c-         die("cache corrupted");
> >
> > merge-ours.c is interesting because it uses die_errno() version
> > instead. I think that might give inaccurate diagnostics because we do
> > not only fail when a libc/system call fails in read_cache(), so it
> > should be safe to just use die() here.
> >
> > update-index.c is also interesting because it actually saves the
> > return value. I don't think it's used anywhere, so you can just drop
> > that 'entries' variable. But perhaps it's good to make
> > repo_read_index_or_die() return the number of entries too.
> 
> Yeah this series left out all the interesting cases, as I just sent it out
> without much thought.
> 
> Returning the number of entries makes sense.
> 
> One of the reviewers of the series questioned the overall goal of the
> series as we want to move away from _die() in top level code but this
> series moves more towards it.

I've heard every once in a while that we want to move toward this but I
don't believe there is an actual effort along those lines just yet.  For
that to be the case we would need a clearly defined error handling
methodology (aside from the existing "die"ing behavior), which we don't
currently have.

> 
> I don't know.
> 
> Stefan

-- 
Brandon Williams

Re: [PATCH 02/11] repository: introduce repo_read_index_or_die

[Stefan Beller]

Hi Brandon,

>> One of the reviewers of the series questioned the overall goal of the
>> series as we want to move away from _die() in top level code but this
>> series moves more towards it.
>
> I've heard every once in a while that we want to move toward this but I
> don't believe there is an actual effort along those lines just yet.  For
> that to be the case we would need a clearly defined error handling
> methodology (aside from the existing "die"ing behavior), which we don't
> currently have.

We have the example in the refs code, which I would want to
imitate. :)

/*
 * Return 0 if a reference named refname could be created without
 * conflicting with the name of an existing reference. Otherwise,
 * return a negative value and write an explanation to err. [...]
 */

int refs_verify_refname_available(struct ref_store *refs, ...
    struct strbuf *err);

extern int refs_init_db(struct strbuf *err);

But it is true that there is no active effort currently being pushed.

Thanks,
Stefan

Re: [PATCH 02/11] repository: introduce repo_read_index_or_die

[Duy Nguyen]

On Mon, May 21, 2018 at 9:27 PM, Stefan Beller <sbeller@google.com> wrote:
> Hi Brandon,
>
>>> One of the reviewers of the series questioned the overall goal of the
>>> series as we want to move away from _die() in top level code but this
>>> series moves more towards it.
>>
>> I've heard every once in a while that we want to move toward this but I
>> don't believe there is an actual effort along those lines just yet.  For
>> that to be the case we would need a clearly defined error handling
>> methodology (aside from the existing "die"ing behavior), which we don't
>> currently have.
>
> We have the example in the refs code, which I would want to
> imitate. :)
>
> /*
>  * Return 0 if a reference named refname could be created without
>  * conflicting with the name of an existing reference. Otherwise,
>  * return a negative value and write an explanation to err. [...]
>  */
>
> int refs_verify_refname_available(struct ref_store *refs, ...
>     struct strbuf *err);
>
> extern int refs_init_db(struct strbuf *err);

apply.c and sequencer.c too are libified and try hard to avoid die()
if I remember correctly.

> But it is true that there is no active effort currently being pushed.

Yeah. It took a lot of work to put refs code in the current shape
today. I think we just be careful about adding die(). If a function
has no way to return an error, then die() is unavoidable. If it's a
really fatal error, then die() might still be the right choice. But if
not we should try to return an error instead.

Speaking of avoiding die() (and going off-topic again), maybe we
should introduce NO_DIE_PLEASE macro in the same spirit as
NO_THE_INDEX_COMPATIBILITY_MACROS. If the macro is defined, die() and
friends are not declared (which will result in compile errors at least
on DEVELOPER=1 builds). This helps draw boundary between die-able code
and un-die-able code (not perfect, but good enough)
-- 
Duy

Why do we have both x*() and *_or_die() for "do or die"?

[Ævar Arnfjörð Bjarmason]

On Wed, May 16 2018, Stefan Beller wrote:

> A common pattern with the repo_read_index function is to die if the return
> of repo_read_index is negative.  Move this pattern into a function.

Just a side-question unrelated to this patch per-se, why do we have both
x*() and *_or_die() functions in the codebase? I can't find any pattern
for one or the other, e.g. we have both xopen() and then write_or_die(),
so it's not a matter of x*() just being for syscalls and *_or_die()
being for our own functions (also as e.g. strbuf uses x*(), not
*_or_die()).

I'm not trying to litigate the difference and understand it could have
just emerged organically. I'm just wondering if that's the full story or
if one is preferred, or we prefer one or the other in some
circumstances.

Re: Why do we have both x*() and *_or_die() for "do or die"?

[Duy Nguyen]

On Tue, May 22, 2018 at 7:49 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Wed, May 16 2018, Stefan Beller wrote:
>
>> A common pattern with the repo_read_index function is to die if the return
>> of repo_read_index is negative.  Move this pattern into a function.
>
> Just a side-question unrelated to this patch per-se, why do we have both
> x*() and *_or_die() functions in the codebase?

I wondered about that myself shortly after suggesting
repo_read_index_or_die(). My only guess is xfoo is better version of
foo, which sometimes involves dying inside but that's not the only
possible improvement. Later I guess people go with _or_die() more
because it describes what the function does much better.

> I can't find any pattern
> for one or the other, e.g. we have both xopen() and then write_or_die(),
> so it's not a matter of x*() just being for syscalls and *_or_die()
> being for our own functions (also as e.g. strbuf uses x*(), not
> *_or_die()).
>
> I'm not trying to litigate the difference and understand it could have
> just emerged organically. I'm just wondering if that's the full story or
> if one is preferred, or we prefer one or the other in some
> circumstances.



-- 
Duy

Re: Why do we have both x*() and *_or_die() for "do or die"?

[Jonathan Nieder]

Duy Nguyen wrote:
> On Tue, May 22, 2018 at 7:49 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:

>> Just a side-question unrelated to this patch per-se, why do we have both
>> x*() and *_or_die() functions in the codebase?
>
> I wondered about that myself shortly after suggesting
> repo_read_index_or_die(). My only guess is xfoo is better version of
> foo, which sometimes involves dying inside but that's not the only
> possible improvement. Later I guess people go with _or_die() more
> because it describes what the function does much better.

In particular, there are functions like xwrite that don't die on error
and write_or_die that do.

I'd probably be in favor of a series using cocinelle to rename the
functions that do die on error to _or_die.  The main case where I
pause for a moment is xmalloc, since I'm worried about the verboseness
of malloc_or_die, but I suspect I would get used to it.

Thanks,
Jonathan

Re: Why do we have both x*() and *_or_die() for "do or die"?

[Stefan Beller]

On Tue, May 22, 2018 at 10:49 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Wed, May 16 2018, Stefan Beller wrote:
>
>> A common pattern with the repo_read_index function is to die if the return
>> of repo_read_index is negative.  Move this pattern into a function.

This was done organically, i.e. taking the smallest available step that yields
a better code base. There was definitely no stepping back or looking at the
big picture involved.

> Just a side-question unrelated to this patch per-se, why do we have both
> x*() and *_or_die() functions in the codebase?

Digging into the history, the x*() emerges from 112db553b0d (Shrink the
git binary a bit by avoiding unnecessary inline functions, 2008-06-22) and
is wrapping common patterns to reduce the size of the binary.

The or_die pattern comes from 7230e6d042a (Add write_or_die(),
a helper function, 2006-08-21). This was motivated as better code
("making additional error handling unnecessary", "replace two similar ones")

None of them really hint at the _die() as a problem in the
libification efforts which came later IIUC.

> I can't find any pattern
> for one or the other, e.g. we have both xopen() and then write_or_die(),

hah! Up to now I assumed the x* is system calls with write_or_die as an
exception. There is also a xwrite and write_in_full, so they are slightly
different in the way that xwrite covers only the system call and copes
with some common error patterns, whereas write_or_die uses write_in_full
which itself uses xwrite.

> so it's not a matter of x*() just being for syscalls and *_or_die()
> being for our own functions (also as e.g. strbuf uses x*(), not
> *_or_die()).

I would tend to argue that way: x* are strictly to system calls (no die()
in there), and the _die() functions are "hands off *no* error handling
needed"

> I'm not trying to litigate the difference and understand it could have
> just emerged organically. I'm just wondering if that's the full story or
> if one is preferred, or we prefer one or the other in some
> circumstances.

I think we'd prefer the x() for lib code and the _or_die code in builtin/
due to the nature of effort needed to get it right.

Thanks,
Stefan

Re: Why do we have both x*() and *_or_die() for "do or die"?

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Just a side-question unrelated to this patch per-se, why do we have both
> x*() and *_or_die() functions in the codebase? I can't find any pattern
> for one or the other.

My understanding is that x*() were meant for system library
functions.  read-index-or-die should never be x-read-index.

Quite honestly, read-index should probably have designed to die from
the beginning, with read-index-gently as a variant to report an
error instead of dying.

[PATCH 03/11] builtin/grep: use repo_read_index_or_die

[Stefan Beller]

grep is a porcelain command, so translating its error message is a good
idea.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/grep.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 69f0743619f..2c2d6cc6bca 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -488,8 +488,7 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,
 		strbuf_addstr(&name, repo->submodule_prefix);
 	}
 
-	if (repo_read_index(repo) < 0)
-		die("index file corrupt");
+	repo_read_index_or_die(repo);
 
 	for (nr = 0; nr < repo->index->cache_nr; nr++) {
 		const struct cache_entry *ce = repo->index->cache[nr];
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 04/11] submodule: use repo_read_index_or_die

[Stefan Beller]

The code is used by the fetch command, which is a main porcelain command,
so we should localize its error messages.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 74d35b25779..71c042e1371 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1333,8 +1333,7 @@ int fetch_populated_submodules(struct repository *r,
 	if (!r->worktree)
 		goto out;
 
-	if (repo_read_index(r) < 0)
-		die("index file corrupt");
+	repo_read_index_or_die(r);
 
 	argv_array_push(&spf.args, "fetch");
 	for (i = 0; i < options->argc; i++)
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 05/11] builtin/ls-files: use repo_read_index_or_die

[Stefan Beller]

Despite ls-files being a plumbing command, which promises to not change its
output ever, and to be easy on machines (e.g. non-localized output),
it may make sense to localize the error message for a corrupt index
nevertheless:

1. that is more consistent with the rest of Git.
2. Searching for "ls-tree corrupt index file" on the web doesn't yield
   any hits, that suggest the exact string is parsed for.
   Probably the script authors rely on the exit code of ls-tree anyways.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/ls-files.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index a71f6bd088a..502f2f6db04 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -20,6 +20,7 @@
 #include "run-command.h"
 #include "submodule.h"
 #include "submodule-config.h"
+#include "repository.h"
 
 static int abbrev;
 static int show_deleted;
@@ -210,8 +211,7 @@ static void show_submodule(struct repository *superproject,
 	if (repo_submodule_init(&submodule, superproject, path))
 		return;
 
-	if (repo_read_index(&submodule) < 0)
-		die("index file corrupt");
+	repo_read_index_or_die(&submodule);
 
 	show_files(&submodule, dir);
 
@@ -579,8 +579,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		prefix_len = strlen(prefix);
 	git_config(git_default_config, NULL);
 
-	if (repo_read_index(the_repository) < 0)
-		die("index file corrupt");
+	repo_read_index_or_die(the_repository);
 
 	argc = parse_options(argc, argv, prefix, builtin_ls_files_options,
 			ls_files_usage, 0);
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 06/11] read_cache: use repo_read_index_or_die with different error messages

[Stefan Beller]

This replaces all patterns of "if (read_cached() < 0) die(some-msg);"
with repo_read_index_or_die; this changes the output of the error message.

However as all error messages before were translated, these are for human
consumption, so a change in error message is not bad; in fact it makes Git
more consistent.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/add.c       | 4 ++--
 builtin/commit.c    | 9 +++++----
 builtin/rev-parse.c | 4 ++--
 merge.c             | 4 ++--
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index e4751c198c1..910f619b7d5 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -19,6 +19,7 @@
 #include "bulk-checkin.h"
 #include "argv-array.h"
 #include "submodule.h"
+#include "repository.h"
 
 static const char * const builtin_add_usage[] = {
 	N_("git add [<options>] [--] <pathspec>..."),
@@ -229,8 +230,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 
-	if (read_cache() < 0)
-		die(_("Could not read the index"));
+	repo_read_index_or_die(the_repository);
 
 	init_revisions(&rev, prefix);
 	rev.diffopt.context = 7;
diff --git a/builtin/commit.c b/builtin/commit.c
index 5571d4a3e2b..9ebfb4db415 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -32,6 +32,8 @@
 #include "column.h"
 #include "sequencer.h"
 #include "mailmap.h"
+#include "sigchain.h"
+#include "repository.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [<options>] [--] <pathspec>..."),
@@ -428,8 +430,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 		exit(1);
 
 	discard_cache();
-	if (read_cache() < 0)
-		die(_("cannot read the index"));
+	repo_read_index_or_die(the_repository);
 
 	hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
 	add_remove_files(&partial);
@@ -853,8 +854,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		struct object_id oid;
 		const char *parent = "HEAD";
 
-		if (!active_nr && read_cache() < 0)
-			die(_("Cannot read index"));
+		if (!active_nr)
+			repo_read_index_or_die(the_repository);
 
 		if (amend)
 			parent = "HEAD^1";
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 36b20877828..37f29fd850d 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -14,6 +14,7 @@
 #include "revision.h"
 #include "split-index.h"
 #include "submodule.h"
+#include "repository.h"
 
 #define DO_REVS		1
 #define DO_NOREV	2
@@ -884,8 +885,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (!strcmp(arg, "--shared-index-path")) {
-				if (read_cache() < 0)
-					die(_("Could not read the index"));
+				repo_read_index_or_die(the_repository);
 				if (the_index.split_index) {
 					const unsigned char *sha1 = the_index.split_index->base_sha1;
 					const char *path = git_path("sharedindex.%s", sha1_to_hex(sha1));
diff --git a/merge.c b/merge.c
index f06a4773d4f..654d049e80d 100644
--- a/merge.c
+++ b/merge.c
@@ -8,6 +8,7 @@
 #include "tree-walk.h"
 #include "unpack-trees.h"
 #include "dir.h"
+#include "repository.h"
 
 static const char *merge_argument(struct commit *commit)
 {
@@ -70,8 +71,7 @@ int try_merge_command(const char *strategy, size_t xopts_nr,
 	argv_array_clear(&args);
 
 	discard_cache();
-	if (read_cache() < 0)
-		die(_("failed to read the cache"));
+	repo_read_index_or_die(the_repository);
 	resolve_undo_clear();
 
 	return ret;
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 07/11] rerere: use repo_read_index_or_die

[Stefan Beller]

By switching to repo_read_index_or_die, we'll get a slightly different
error message ("index file corrupt") as well as localization of it.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 rerere.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/rerere.c b/rerere.c
index 18cae2d11c9..5f35dd66f90 100644
--- a/rerere.c
+++ b/rerere.c
@@ -10,6 +10,7 @@
 #include "attr.h"
 #include "pathspec.h"
 #include "sha1-lookup.h"
+#include "repository.h"
 
 #define RESOLVED 0
 #define PUNTED 1
@@ -567,8 +568,7 @@ static int check_one_conflict(int i, int *type)
 static int find_conflict(struct string_list *conflict)
 {
 	int i;
-	if (read_cache() < 0)
-		return error("Could not read index");
+	repo_read_index_or_die(the_repository);
 
 	for (i = 0; i < active_nr;) {
 		int conflict_type;
@@ -600,8 +600,7 @@ int rerere_remaining(struct string_list *merge_rr)
 	int i;
 	if (setup_rerere(merge_rr, RERERE_READONLY))
 		return 0;
-	if (read_cache() < 0)
-		return error("Could not read index");
+	repo_read_index_or_die(the_repository);
 
 	for (i = 0; i < active_nr;) {
 		int conflict_type;
@@ -1103,8 +1102,7 @@ int rerere_forget(struct pathspec *pathspec)
 	struct string_list conflict = STRING_LIST_INIT_DUP;
 	struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
-	if (read_cache() < 0)
-		return error("Could not read index");
+	repo_read_index_or_die(the_repository);
 
 	fd = setup_rerere(&merge_rr, RERERE_NOAUTOUPDATE);
 	if (fd < 0)
-- 
2.17.0.582.gccdcbd54c44.dirty

Re: [PATCH 07/11] rerere: use repo_read_index_or_die

[Thomas Gummerer]

On 05/16, Stefan Beller wrote:
> By switching to repo_read_index_or_die, we'll get a slightly different
> error message ("index file corrupt") as well as localization of it.

Apart from the slightly different error message, and the localization
(both of which I think are a good thing), I notice that this also
turns a "return error(...)" into a "die(...)".  I thought we were
going more towards not 'die'ing in libgit.a code, and letting the
caller handling the errors?  Either way I think this change should be
described in the commit message.

Also all other messages in 'rerere.c' are currently not translated.
I'm currently working on a series that includes a patch to do just
that (amongst some other changes to 'rerere'), which I'm hoping to
send soon-ish, but the textual conflicts should we still want this
patch should be easy to solve.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  rerere.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/rerere.c b/rerere.c
> index 18cae2d11c9..5f35dd66f90 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -10,6 +10,7 @@
>  #include "attr.h"
>  #include "pathspec.h"
>  #include "sha1-lookup.h"
> +#include "repository.h"
>  
>  #define RESOLVED 0
>  #define PUNTED 1
> @@ -567,8 +568,7 @@ static int check_one_conflict(int i, int *type)
>  static int find_conflict(struct string_list *conflict)
>  {
>  	int i;
> -	if (read_cache() < 0)
> -		return error("Could not read index");
> +	repo_read_index_or_die(the_repository);
>  
>  	for (i = 0; i < active_nr;) {
>  		int conflict_type;
> @@ -600,8 +600,7 @@ int rerere_remaining(struct string_list *merge_rr)
>  	int i;
>  	if (setup_rerere(merge_rr, RERERE_READONLY))
>  		return 0;
> -	if (read_cache() < 0)
> -		return error("Could not read index");
> +	repo_read_index_or_die(the_repository);
>  
>  	for (i = 0; i < active_nr;) {
>  		int conflict_type;
> @@ -1103,8 +1102,7 @@ int rerere_forget(struct pathspec *pathspec)
>  	struct string_list conflict = STRING_LIST_INIT_DUP;
>  	struct string_list merge_rr = STRING_LIST_INIT_DUP;
>  
> -	if (read_cache() < 0)
> -		return error("Could not read index");
> +	repo_read_index_or_die(the_repository);
>  
>  	fd = setup_rerere(&merge_rr, RERERE_NOAUTOUPDATE);
>  	if (fd < 0)
> -- 
> 2.17.0.582.gccdcbd54c44.dirty
> 

Re: [PATCH 07/11] rerere: use repo_read_index_or_die

[Stefan Beller]

Hi Thomas,

On Sun, May 20, 2018 at 10:45 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> On 05/16, Stefan Beller wrote:
>> By switching to repo_read_index_or_die, we'll get a slightly different
>> error message ("index file corrupt") as well as localization of it.
>
> Apart from the slightly different error message, and the localization
> (both of which I think are a good thing), I notice that this also
> turns a "return error(...)" into a "die(...)".  I thought we were
> going more towards not 'die'ing in libgit.a code, and letting the
> caller handling the errors?  Either way I think this change should be
> described in the commit message.

oops, I will to drop or fix this patch from the series as I think the
not die()ing is a good idea for libgit.

> Also all other messages in 'rerere.c' are currently not translated.
> I'm currently working on a series that includes a patch to do just
> that (amongst some other changes to 'rerere'), which I'm hoping to
> send soon-ish, but the textual conflicts should we still want this
> patch should be easy to solve.

I did not mark this series well enough, it was a mere attempt to
"see how it goes", but was retracted as a whole[1] after Junio
dreaded some merge conflicts.

[1] https://public-inbox.org/git/CAGZ79kbvjoTq5079Ks+h2HNb+D99RELYPcJk2=pvZf9-Y8dToQ@mail.gmail.com/

So do not feel bad about any merge conflicts in rerere, as this
series will not land any time soon.

Stefan

[PATCH 08/11] check-attr: switch to repo_read_index_or_die

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/check-attr.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 91444dc0448..bf05e7e93ca 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -4,6 +4,7 @@
 #include "attr.h"
 #include "quote.h"
 #include "parse-options.h"
+#include "repository.h"
 
 static int all_attrs;
 static int cached_attrs;
@@ -115,9 +116,7 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, check_attr_options,
 			     check_attr_usage, PARSE_OPT_KEEP_DASHDASH);
 
-	if (read_cache() < 0) {
-		die("invalid cache");
-	}
+	repo_read_index_or_die(the_repository);
 
 	if (cached_attrs)
 		git_attr_set_direction(GIT_ATTR_INDEX, NULL);
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 09/11] checkout-index: switch to repo_read_index

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/checkout-index.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index a730f6a1aa4..aaba99d36c0 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -10,6 +10,7 @@
 #include "quote.h"
 #include "cache-tree.h"
 #include "parse-options.h"
+#include "repository.h"
 
 #define CHECKOUT_ALL 4
 static int nul_term_line;
@@ -184,9 +185,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	prefix_length = prefix ? strlen(prefix) : 0;
 
-	if (read_cache() < 0) {
-		die("invalid cache");
-	}
+	repo_read_index_or_die(the_repository);
 
 	argc = parse_options(argc, argv, prefix, builtin_checkout_index_options,
 			builtin_checkout_index_usage, 0);
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 10/11] test helpers: switch to repo_read_index_or_die

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/helper/test-dump-cache-tree.c      |  5 ++---
 t/helper/test-dump-untracked-cache.c |  4 ++--
 t/helper/test-lazy-init-name-hash.c  | 11 ++++++-----
 t/helper/test-read-cache.c           |  3 ++-
 t/helper/test-scrap-cache-tree.c     |  4 ++--
 t/helper/test-write-cache.c          |  3 ++-
 6 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/t/helper/test-dump-cache-tree.c b/t/helper/test-dump-cache-tree.c
index 98a4891f1dc..a58c26084dd 100644
--- a/t/helper/test-dump-cache-tree.c
+++ b/t/helper/test-dump-cache-tree.c
@@ -2,7 +2,7 @@
 #include "cache.h"
 #include "tree.h"
 #include "cache-tree.h"
-
+#include "repository.h"
 
 static void dump_one(struct cache_tree *it, const char *pfx, const char *x)
 {
@@ -60,8 +60,7 @@ int cmd__dump_cache_tree(int ac, const char **av)
 	struct index_state istate;
 	struct cache_tree *another = cache_tree();
 	setup_git_directory();
-	if (read_cache() < 0)
-		die("unable to read index file");
+	repo_read_index_or_die(the_repository);
 	istate = the_index;
 	istate.cache_tree = another;
 	cache_tree_update(&istate, WRITE_TREE_DRY_RUN);
diff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c
index d7c55c2355e..171ba143c9a 100644
--- a/t/helper/test-dump-untracked-cache.c
+++ b/t/helper/test-dump-untracked-cache.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "dir.h"
+#include "repository.h"
 
 static int compare_untracked(const void *a_, const void *b_)
 {
@@ -47,8 +48,7 @@ int cmd_main(int ac, const char **av)
 	ignore_untracked_cache_config = 1;
 
 	setup_git_directory();
-	if (read_cache() < 0)
-		die("unable to read index file");
+	repo_read_index_or_die(the_repository);
 	uc = the_index.untracked;
 	if (!uc) {
 		printf("no untracked cache\n");
diff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c
index b99a37080d9..e25bfe27ab9 100644
--- a/t/helper/test-lazy-init-name-hash.c
+++ b/t/helper/test-lazy-init-name-hash.c
@@ -1,6 +1,7 @@
 #include "test-tool.h"
 #include "cache.h"
 #include "parse-options.h"
+#include "repository.h"
 
 static int single;
 static int multi;
@@ -32,7 +33,7 @@ static void dump_run(void)
 	struct dir_entry *dir;
 	struct cache_entry *ce;
 
-	read_cache();
+	repo_read_index_or_die(the_repository);
 	if (single) {
 		test_lazy_init_name_hash(&the_index, 0);
 	} else {
@@ -70,7 +71,7 @@ static uint64_t time_runs(int try_threaded)
 
 	for (i = 0; i < count; i++) {
 		t0 = getnanotime();
-		read_cache();
+		repo_read_index_or_die(the_repository);
 		t1 = getnanotime();
 		nr_threads_used = test_lazy_init_name_hash(&the_index, try_threaded);
 		t2 = getnanotime();
@@ -117,7 +118,7 @@ static void analyze_run(void)
 	int i;
 	int nr;
 
-	read_cache();
+	repo_read_index_or_die(the_repository);
 	cache_nr_limit = the_index.cache_nr;
 	discard_cache();
 
@@ -132,7 +133,7 @@ static void analyze_run(void)
 			nr = cache_nr_limit;
 
 		for (i = 0; i < count; i++) {
-			read_cache();
+			repo_read_index_or_die(the_repository);
 			the_index.cache_nr = nr; /* cheap truncate of index */
 			t1s = getnanotime();
 			test_lazy_init_name_hash(&the_index, 0);
@@ -141,7 +142,7 @@ static void analyze_run(void)
 			the_index.cache_nr = cache_nr_limit;
 			discard_cache();
 
-			read_cache();
+			repo_read_index_or_die(the_repository);
 			the_index.cache_nr = nr; /* cheap truncate of index */
 			t1m = getnanotime();
 			nr_threads_used = test_lazy_init_name_hash(&the_index, 1);
diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index d674c88ba09..7a4a91bf328 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -1,5 +1,6 @@
 #include "test-tool.h"
 #include "cache.h"
+#include "repository.h"
 
 int cmd__read_cache(int argc, const char **argv)
 {
@@ -8,7 +9,7 @@ int cmd__read_cache(int argc, const char **argv)
 		cnt = strtol(argv[1], NULL, 0);
 	setup_git_directory();
 	for (i = 0; i < cnt; i++) {
-		read_cache();
+		repo_read_index_or_die(the_repository);
 		discard_cache();
 	}
 	return 0;
diff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c
index d26d3e7c8b1..401917abb49 100644
--- a/t/helper/test-scrap-cache-tree.c
+++ b/t/helper/test-scrap-cache-tree.c
@@ -3,6 +3,7 @@
 #include "lockfile.h"
 #include "tree.h"
 #include "cache-tree.h"
+#include "repository.h"
 
 static struct lock_file index_lock;
 
@@ -10,8 +11,7 @@ int cmd__scrap_cache_tree(int ac, const char **av)
 {
 	setup_git_directory();
 	hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
-	if (read_cache() < 0)
-		die("unable to read index file");
+	repo_read_index_or_die(the_repository);
 	active_cache_tree = NULL;
 	if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
 		die("unable to write index file");
diff --git a/t/helper/test-write-cache.c b/t/helper/test-write-cache.c
index 017dc303800..6fd750da254 100644
--- a/t/helper/test-write-cache.c
+++ b/t/helper/test-write-cache.c
@@ -1,6 +1,7 @@
 #include "test-tool.h"
 #include "cache.h"
 #include "lockfile.h"
+#include "repository.h"
 
 static struct lock_file index_lock;
 
@@ -10,7 +11,7 @@ int cmd__write_cache(int argc, const char **argv)
 	if (argc == 2)
 		cnt = strtol(argv[1], NULL, 0);
 	setup_git_directory();
-	read_cache();
+	repo_read_index_or_die(the_repository);
 	for (i = 0; i < cnt; i++) {
 		lockfd = hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
 		if (0 <= lockfd) {
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 11/11] read_cache: convert most calls to repo_read_index_or_die

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 blame.c               | 5 +++--
 builtin/am.c          | 3 ++-
 builtin/diff.c        | 3 ++-
 builtin/fsck.c        | 3 ++-
 builtin/merge-index.c | 3 ++-
 check-racy.c          | 2 +-
 diff.c                | 5 +++--
 merge-recursive.c     | 3 ++-
 revision.c            | 5 +++--
 sequencer.c           | 5 +++--
 sha1-name.c           | 2 +-
 11 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/blame.c b/blame.c
index 78c9808bd1a..ebfa1c8efcd 100644
--- a/blame.c
+++ b/blame.c
@@ -5,6 +5,7 @@
 #include "diff.h"
 #include "diffcore.h"
 #include "tag.h"
+#include "repository.h"
 #include "blame.h"
 
 void blame_origin_decref(struct blame_origin *o)
@@ -159,7 +160,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	unsigned mode;
 	struct strbuf msg = STRBUF_INIT;
 
-	read_cache();
+	repo_read_index_or_die(the_repository);
 	time(&now);
 	commit = alloc_commit_node();
 	commit->object.parsed = 1;
@@ -241,7 +242,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	 * want to run "diff-index --cached".
 	 */
 	discard_cache();
-	read_cache();
+	repo_read_index_or_die(the_repository);
 
 	len = strlen(path);
 	if (!mode) {
diff --git a/builtin/am.c b/builtin/am.c
index d834f9e62b6..3c6e77a5369 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -32,6 +32,7 @@
 #include "apply.h"
 #include "string-list.h"
 #include "packfile.h"
+#include "repository.h"
 
 /**
  * Returns 1 if the file is empty or does not exist, 0 otherwise.
@@ -1581,7 +1582,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa
 	say(state, stdout, _("Falling back to patching base and 3-way merge..."));
 
 	discard_cache();
-	read_cache();
+	repo_read_index_or_die(the_repository);
 
 	/*
 	 * This is not so wrong. Depending on which base we picked, orig_tree
diff --git a/builtin/diff.c b/builtin/diff.c
index 16bfb22f738..4bba211f1c7 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -17,6 +17,7 @@
 #include "builtin.h"
 #include "submodule.h"
 #include "sha1-array.h"
+#include "repository.h"
 
 #define DIFF_NO_INDEX_EXPLICIT 1
 #define DIFF_NO_INDEX_IMPLICIT 2
@@ -210,7 +211,7 @@ static void refresh_index_quietly(void)
 	if (fd < 0)
 		return;
 	discard_cache();
-	read_cache();
+	repo_read_index(the_repository); /* do not die on error */
 	refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);
 	update_index_if_able(&the_index, &lock_file);
 }
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 087360a6757..a42e98235da 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -18,6 +18,7 @@
 #include "decorate.h"
 #include "packfile.h"
 #include "object-store.h"
+#include "repository.h"
 
 #define REACHABLE 0x0001
 #define SEEN      0x0002
@@ -795,7 +796,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 	if (keep_cache_objects) {
 		verify_index_checksum = 1;
 		verify_ce_order = 1;
-		read_cache();
+		repo_read_index_or_die(the_repository);
 		for (i = 0; i < active_nr; i++) {
 			unsigned int mode;
 			struct blob *blob;
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index c99443b095b..2d91c7c3b5e 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "run-command.h"
+#include "repository.h"
 
 static const char *pgm;
 static int one_shot, quiet;
@@ -77,7 +78,7 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix)
 	if (argc < 3)
 		usage("git merge-index [-o] [-q] <merge-program> (-a | [--] [<filename>...])");
 
-	read_cache();
+	repo_read_index_or_die(the_repository);
 
 	i = 1;
 	if (!strcmp(argv[i], "-o")) {
diff --git a/check-racy.c b/check-racy.c
index 24b6542352a..9b884639cf4 100644
--- a/check-racy.c
+++ b/check-racy.c
@@ -6,7 +6,7 @@ int main(int ac, char **av)
 	int dirty, clean, racy;
 
 	dirty = clean = racy = 0;
-	read_cache();
+	repo_read_index_or_die(the_repository);
 	for (i = 0; i < active_nr; i++) {
 		struct cache_entry *ce = active_cache[i];
 		struct stat st;
diff --git a/diff.c b/diff.c
index 1289df4b1f9..383f52fa118 100644
--- a/diff.c
+++ b/diff.c
@@ -22,6 +22,7 @@
 #include "argv-array.h"
 #include "graph.h"
 #include "packfile.h"
+#include "repository.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -4210,13 +4211,13 @@ void diff_setup_done(struct diff_options *options)
 		options->rename_limit = diff_rename_limit_default;
 	if (options->setup & DIFF_SETUP_USE_CACHE) {
 		if (!active_cache)
-			/* read-cache does not die even when it fails
+			/* repo_read_indexe does not die even when it fails
 			 * so it is safe for us to do this here.  Also
 			 * it does not smudge active_cache or active_nr
 			 * when it fails, so we do not have to worry about
 			 * cleaning it up ourselves either.
 			 */
-			read_cache();
+			repo_read_index(the_repository);
 	}
 	if (40 < options->abbrev)
 		options->abbrev = 40; /* full */
diff --git a/merge-recursive.c b/merge-recursive.c
index 0c0d48624da..76911c935c3 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2151,7 +2151,8 @@ int merge_recursive(struct merge_options *o,
 
 	discard_cache();
 	if (!o->call_depth)
-		read_cache();
+		if (read_cache() < 0)
+			return err(o, _("index file corrupt"));
 
 	o->ancestor = "merged common ancestors";
 	clean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree,
diff --git a/revision.c b/revision.c
index 1cff11833e7..8ad9824143d 100644
--- a/revision.c
+++ b/revision.c
@@ -23,6 +23,7 @@
 #include "packfile.h"
 #include "worktree.h"
 #include "argv-array.h"
+#include "repository.h"
 
 volatile show_early_output_fn_t show_early_output;
 
@@ -1344,7 +1345,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 {
 	struct worktree **worktrees, **p;
 
-	read_cache();
+	repo_read_index_or_die(the_repository);
 	do_add_index_objects_to_pending(revs, &the_index);
 
 	if (revs->single_worktree)
@@ -1486,7 +1487,7 @@ static void prepare_show_merge(struct rev_info *revs)
 	head->object.flags |= SYMMETRIC_LEFT;
 
 	if (!active_nr)
-		read_cache();
+		repo_read_index_or_die(the_repository);
 	for (i = 0; i < active_nr; i++) {
 		const struct cache_entry *ce = active_cache[i];
 		if (!ce_stage(ce))
diff --git a/sequencer.c b/sequencer.c
index 4ce5120e777..773165c8cde 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -23,6 +23,7 @@
 #include "hashmap.h"
 #include "notes-utils.h"
 #include "sigchain.h"
+#include "repository.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -432,7 +433,7 @@ static int fast_forward_to(const struct object_id *to, const struct object_id *f
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf err = STRBUF_INIT;
 
-	read_cache();
+	repo_read_index_or_die(the_repository);
 	if (checkout_fast_forward(from, to, 1))
 		return -1; /* the callee should have complained already */
 
@@ -489,7 +490,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	if (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR) < 0)
 		return -1;
 
-	read_cache();
+	repo_read_index_or_die(the_repository);
 
 	init_merge_options(&o);
 	o.ancestor = base ? base_label : "(empty tree)";
diff --git a/sha1-name.c b/sha1-name.c
index 5b93bf8da36..83d5f945cf1 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -1639,7 +1639,7 @@ static int get_oid_with_context_1(const char *name,
 			oc->path = xstrdup(cp);
 
 		if (!active_cache)
-			read_cache();
+			repo_read_index_or_die(the_repository);
 		pos = cache_name_pos(cp, namelen);
 		if (pos < 0)
 			pos = -pos - 1;
-- 
2.17.0.582.gccdcbd54c44.dirty

Re: [PATCH 11/11] read_cache: convert most calls to repo_read_index_or_die

[Brandon Williams]

On 05/16, Stefan Beller wrote:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  blame.c               | 5 +++--
>  builtin/am.c          | 3 ++-
>  builtin/diff.c        | 3 ++-
>  builtin/fsck.c        | 3 ++-
>  builtin/merge-index.c | 3 ++-
>  check-racy.c          | 2 +-
>  diff.c                | 5 +++--
>  merge-recursive.c     | 3 ++-
>  revision.c            | 5 +++--
>  sequencer.c           | 5 +++--
>  sha1-name.c           | 2 +-
>  11 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/blame.c b/blame.c
> index 78c9808bd1a..ebfa1c8efcd 100644
> --- a/blame.c
> +++ b/blame.c
> @@ -5,6 +5,7 @@
>  #include "diff.h"
>  #include "diffcore.h"
>  #include "tag.h"
> +#include "repository.h"
>  #include "blame.h"
>  
>  void blame_origin_decref(struct blame_origin *o)
> @@ -159,7 +160,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
>  	unsigned mode;
>  	struct strbuf msg = STRBUF_INIT;
>  
> -	read_cache();
> +	repo_read_index_or_die(the_repository);
>  	time(&now);
>  	commit = alloc_commit_node();
>  	commit->object.parsed = 1;
> @@ -241,7 +242,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
>  	 * want to run "diff-index --cached".
>  	 */
>  	discard_cache();
> -	read_cache();
> +	repo_read_index_or_die(the_repository);
>  
>  	len = strlen(path);
>  	if (!mode) {
> diff --git a/builtin/am.c b/builtin/am.c
> index d834f9e62b6..3c6e77a5369 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -32,6 +32,7 @@
>  #include "apply.h"
>  #include "string-list.h"
>  #include "packfile.h"
> +#include "repository.h"
>  
>  /**
>   * Returns 1 if the file is empty or does not exist, 0 otherwise.
> @@ -1581,7 +1582,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa
>  	say(state, stdout, _("Falling back to patching base and 3-way merge..."));
>  
>  	discard_cache();
> -	read_cache();
> +	repo_read_index_or_die(the_repository);
>  
>  	/*
>  	 * This is not so wrong. Depending on which base we picked, orig_tree
> diff --git a/builtin/diff.c b/builtin/diff.c
> index 16bfb22f738..4bba211f1c7 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -17,6 +17,7 @@
>  #include "builtin.h"
>  #include "submodule.h"
>  #include "sha1-array.h"
> +#include "repository.h"
>  
>  #define DIFF_NO_INDEX_EXPLICIT 1
>  #define DIFF_NO_INDEX_IMPLICIT 2
> @@ -210,7 +211,7 @@ static void refresh_index_quietly(void)
>  	if (fd < 0)
>  		return;
>  	discard_cache();
> -	read_cache();
> +	repo_read_index(the_repository); /* do not die on error */
>  	refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);
>  	update_index_if_able(&the_index, &lock_file);
>  }
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 087360a6757..a42e98235da 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -18,6 +18,7 @@
>  #include "decorate.h"
>  #include "packfile.h"
>  #include "object-store.h"
> +#include "repository.h"
>  
>  #define REACHABLE 0x0001
>  #define SEEN      0x0002
> @@ -795,7 +796,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
>  	if (keep_cache_objects) {
>  		verify_index_checksum = 1;
>  		verify_ce_order = 1;
> -		read_cache();
> +		repo_read_index_or_die(the_repository);
>  		for (i = 0; i < active_nr; i++) {
>  			unsigned int mode;
>  			struct blob *blob;
> diff --git a/builtin/merge-index.c b/builtin/merge-index.c
> index c99443b095b..2d91c7c3b5e 100644
> --- a/builtin/merge-index.c
> +++ b/builtin/merge-index.c
> @@ -1,5 +1,6 @@
>  #include "builtin.h"
>  #include "run-command.h"
> +#include "repository.h"
>  
>  static const char *pgm;
>  static int one_shot, quiet;
> @@ -77,7 +78,7 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix)
>  	if (argc < 3)
>  		usage("git merge-index [-o] [-q] <merge-program> (-a | [--] [<filename>...])");
>  
> -	read_cache();
> +	repo_read_index_or_die(the_repository);
>  
>  	i = 1;
>  	if (!strcmp(argv[i], "-o")) {
> diff --git a/check-racy.c b/check-racy.c
> index 24b6542352a..9b884639cf4 100644
> --- a/check-racy.c
> +++ b/check-racy.c
> @@ -6,7 +6,7 @@ int main(int ac, char **av)
>  	int dirty, clean, racy;
>  
>  	dirty = clean = racy = 0;
> -	read_cache();
> +	repo_read_index_or_die(the_repository);
>  	for (i = 0; i < active_nr; i++) {
>  		struct cache_entry *ce = active_cache[i];
>  		struct stat st;
> diff --git a/diff.c b/diff.c
> index 1289df4b1f9..383f52fa118 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -22,6 +22,7 @@
>  #include "argv-array.h"
>  #include "graph.h"
>  #include "packfile.h"
> +#include "repository.h"
>  
>  #ifdef NO_FAST_WORKING_DIRECTORY
>  #define FAST_WORKING_DIRECTORY 0
> @@ -4210,13 +4211,13 @@ void diff_setup_done(struct diff_options *options)
>  		options->rename_limit = diff_rename_limit_default;
>  	if (options->setup & DIFF_SETUP_USE_CACHE) {
>  		if (!active_cache)
> -			/* read-cache does not die even when it fails
> +			/* repo_read_indexe does not die even when it fails

s/repo_read_indexe/repo_read_index

>  			 * so it is safe for us to do this here.  Also
>  			 * it does not smudge active_cache or active_nr
>  			 * when it fails, so we do not have to worry about
>  			 * cleaning it up ourselves either.
>  			 */
> -			read_cache();
> +			repo_read_index(the_repository);
>  	}
>  	if (40 < options->abbrev)
>  		options->abbrev = 40; /* full */
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 0c0d48624da..76911c935c3 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2151,7 +2151,8 @@ int merge_recursive(struct merge_options *o,
>  
>  	discard_cache();
>  	if (!o->call_depth)
> -		read_cache();
> +		if (read_cache() < 0)
> +			return err(o, _("index file corrupt"));

if we're already moving to change read_cache() calls then we could
substitute it for repo_read_index here as well.

Maybe we can kill read_cache() while at it?

>  
>  	o->ancestor = "merged common ancestors";
>  	clean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree,
> diff --git a/revision.c b/revision.c
> index 1cff11833e7..8ad9824143d 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -23,6 +23,7 @@
>  #include "packfile.h"
>  #include "worktree.h"
>  #include "argv-array.h"
> +#include "repository.h"
>  
>  volatile show_early_output_fn_t show_early_output;
>  
> @@ -1344,7 +1345,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
>  {
>  	struct worktree **worktrees, **p;
>  
> -	read_cache();
> +	repo_read_index_or_die(the_repository);
>  	do_add_index_objects_to_pending(revs, &the_index);
>  
>  	if (revs->single_worktree)
> @@ -1486,7 +1487,7 @@ static void prepare_show_merge(struct rev_info *revs)
>  	head->object.flags |= SYMMETRIC_LEFT;
>  
>  	if (!active_nr)
> -		read_cache();
> +		repo_read_index_or_die(the_repository);
>  	for (i = 0; i < active_nr; i++) {
>  		const struct cache_entry *ce = active_cache[i];
>  		if (!ce_stage(ce))
> diff --git a/sequencer.c b/sequencer.c
> index 4ce5120e777..773165c8cde 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -23,6 +23,7 @@
>  #include "hashmap.h"
>  #include "notes-utils.h"
>  #include "sigchain.h"
> +#include "repository.h"
>  
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>  
> @@ -432,7 +433,7 @@ static int fast_forward_to(const struct object_id *to, const struct object_id *f
>  	struct strbuf sb = STRBUF_INIT;
>  	struct strbuf err = STRBUF_INIT;
>  
> -	read_cache();
> +	repo_read_index_or_die(the_repository);
>  	if (checkout_fast_forward(from, to, 1))
>  		return -1; /* the callee should have complained already */
>  
> @@ -489,7 +490,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>  	if (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR) < 0)
>  		return -1;
>  
> -	read_cache();
> +	repo_read_index_or_die(the_repository);
>  
>  	init_merge_options(&o);
>  	o.ancestor = base ? base_label : "(empty tree)";
> diff --git a/sha1-name.c b/sha1-name.c
> index 5b93bf8da36..83d5f945cf1 100644
> --- a/sha1-name.c
> +++ b/sha1-name.c
> @@ -1639,7 +1639,7 @@ static int get_oid_with_context_1(const char *name,
>  			oc->path = xstrdup(cp);
>  
>  		if (!active_cache)
> -			read_cache();
> +			repo_read_index_or_die(the_repository);
>  		pos = cache_name_pos(cp, namelen);
>  		if (pos < 0)
>  			pos = -pos - 1;
> -- 
> 2.17.0.582.gccdcbd54c44.dirty
> 

-- 
Brandon Williams

Re: [PATCH 11/11] read_cache: convert most calls to repo_read_index_or_die

[Duy Nguyen]

On Wed, May 16, 2018 at 03:27:33PM -0700, Brandon Williams wrote:
> Maybe we can kill read_cache() while at it?

I took this out of context. But with the move to repo_* stuff,
eventually these macros around the_index should die. Not today though.
--
Duy

Re: [PATCH 11/11] read_cache: convert most calls to repo_read_index_or_die

[Duy Nguyen]

On Wed, May 16, 2018 at 03:21:18PM -0700, Stefan Beller wrote:

This commit may need some more explanation. It's not always safe to
replace "read_cache();" with "repo_read_index_or_die();" and you do
sometimes avoid that variant.

While I agree _or_die() is the right thing to do most of the time. You
need to consider that we (or some of us) have tried to avoid die() in
some library code. So..

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  blame.c               | 5 +++--
>  builtin/am.c          | 3 ++-
>  builtin/diff.c        | 3 ++-
>  builtin/fsck.c        | 3 ++-
>  builtin/merge-index.c | 3 ++-

These should be good.

>  check-racy.c          | 2 +-

Meh.. this one is test code.

> diff --git a/diff.c b/diff.c
> index 1289df4b1f9..383f52fa118 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -22,6 +22,7 @@
>  #include "argv-array.h"
>  #include "graph.h"
>  #include "packfile.h"
> +#include "repository.h"
>  
>  #ifdef NO_FAST_WORKING_DIRECTORY
>  #define FAST_WORKING_DIRECTORY 0
> @@ -4210,13 +4211,13 @@ void diff_setup_done(struct diff_options *options)
>  		options->rename_limit = diff_rename_limit_default;
>  	if (options->setup & DIFF_SETUP_USE_CACHE) {
>  		if (!active_cache)
> -			/* read-cache does not die even when it fails
> +			/* repo_read_indexe does not die even when it fails

s/indexe/index/

>  			 * so it is safe for us to do this here.  Also
>  			 * it does not smudge active_cache or active_nr
>  			 * when it fails, so we do not have to worry about
>  			 * cleaning it up ourselves either.
>  			 */
> -			read_cache();
> +			repo_read_index(the_repository);

Should we write a warning message or something?

> diff --git a/revision.c b/revision.c
> index 1cff11833e7..8ad9824143d 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -23,6 +23,7 @@
>  #include "packfile.h"
>  #include "worktree.h"
>  #include "argv-array.h"
> +#include "repository.h"
>  
>  volatile show_early_output_fn_t show_early_output;
>  
> @@ -1344,7 +1345,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
>  {
>  	struct worktree **worktrees, **p;
>  
> -	read_cache();
> +	repo_read_index_or_die(the_repository);

I think here we should be able to tolerate a bad index file. If you
have bad index file, we ignore object references from it and continue
on the rev walk without it. So repo_read_index() may be better,
optionally with a warning.

> diff --git a/sha1-name.c b/sha1-name.c
> index 5b93bf8da36..83d5f945cf1 100644
> --- a/sha1-name.c
> +++ b/sha1-name.c
> @@ -1639,7 +1639,7 @@ static int get_oid_with_context_1(const char *name,
>  			oc->path = xstrdup(cp);
>  
>  		if (!active_cache)
> -			read_cache();
> +			repo_read_index_or_die(the_repository);

This should return an error code instead. I notice there's a
"only_to_die" flag somewhere in here. Something to think about.

BTW you probably want to move away from "active_cache" too. It makes
sense to read "if active cache is null then read the cache". But with
the move to the repository it now seems disconnected.

This looks clearer but a bit verbose

		if (!the_repository->index_file.cache)
			repo_read_index_or_die(the_repository);

This might be the best

		if (!repo_index_loaded(the_repository))
			repo_read_index_or_die(the_repository);

but obviously more work for you, so your choice :)

>  		pos = cache_name_pos(cp, namelen);
>  		if (pos < 0)
>  			pos = -pos - 1;
> -- 
> 2.17.0.582.gccdcbd54c44.dirty
> 

Re: [PATCH 00/11]

[Duy Nguyen]

On Wed, May 16, 2018 at 03:21:07PM -0700, Stefan Beller wrote:
> > If you have time, yes translate them all. I don't see how any of these
> > strings are meant for script. If not, just _() the new string you
> > added is fine.
> 
> > With a majority of call sites dying like this though, I wonder if we
> > should just add repo_read_index_or_die() with die() inside. Then the
> > next person won't likely accidentally forget _()
> 
> So this comment tricked me into coming up with a patch series. :)

Yay! (And sorry, Junio)

> 
> Each patch is themed, I tried to make each commit special w.r.t. reviewers
> attention.
> 
> We'd start out with a resend of the origin patch, which is boring.
> 
> Then we'll move all similar cases into one function (no-op for
> introducing the repo_read_index_or_die function as all callers will have the
> same error message and the same localisation).
> 
> Any following patch will be more controversial then the previous patches,
> I would expect, as we introduce more and more change.
> 
> The last patch is just an attempt to finish the series gracefully,
> and may contain errors (sometimes we do not want to die() in case of
> corrupt index).
> 
> Is this series roughly what you had in mind?

This is better than what I had in mind, in fact.

--
Duy

Re: [PATCH] grep: handle corrupt index files early

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> With a majority of call sites dying like this though, I wonder if we
> should just add repo_read_index_or_die() with die() inside. Then the
> next person won't likely accidentally forget _()

Yuck.

That sounds like inviting a major code churn.  I tend to agree that
it would be a good clean-up for longer term maintenance, but I am
not sure if I can honestly say I'd look forward to such a clean-up
at this point in the cycle when there are tons of large-ish topics
in flight X-<.

Re: [PATCH] grep: handle corrupt index files early

[Stefan Beller]

On Wed, May 16, 2018 at 6:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> With a majority of call sites dying like this though, I wonder if we
>> should just add repo_read_index_or_die() with die() inside. Then the
>> next person won't likely accidentally forget _()
>
> Yuck.
>
> That sounds like inviting a major code churn.  I tend to agree that
> it would be a good clean-up for longer term maintenance, but I am
> not sure if I can honestly say I'd look forward to such a clean-up
> at this point in the cycle when there are tons of large-ish topics
> in flight X-<.

ok, consider the series
https://public-inbox.org/git/20180516222118.233868-1-sbeller@google.com/
retracted for this cycle; I will keep it around and resend it at some future
date, hopefully.

Feel free to comment on it, though.

Stefan

Re: [PATCH] grep: handle corrupt index files early

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Wed, May 16, 2018 at 6:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>>
>>> With a majority of call sites dying like this though, I wonder if we
>>> should just add repo_read_index_or_die() with die() inside. Then the
>>> next person won't likely accidentally forget _()
>>
>> Yuck.
>>
>> That sounds like inviting a major code churn.  I tend to agree that
>> it would be a good clean-up for longer term maintenance, but I am
>> not sure if I can honestly say I'd look forward to such a clean-up
>> at this point in the cycle when there are tons of large-ish topics
>> in flight X-<.
>
> ok, consider the series
> https://public-inbox.org/git/20180516222118.233868-1-sbeller@google.com/
> retracted for this cycle; I will keep it around and resend it at some future
> date, hopefully.

Thanks.  I didn't realize you've _already_ done that.

Re: [PATCH] grep: handle corrupt index files early

[Brandon Williams]

On 05/14, Stefan Beller wrote:
> Any other caller of 'repo_read_index' dies upon a negative return of
> it, so grep should, too.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> Found while reviewing the series
> https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/
> 
>  builtin/grep.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 6e7bc76785a..69f0743619f 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,
>  		strbuf_addstr(&name, repo->submodule_prefix);
>  	}
>  
> -	repo_read_index(repo);
> +	if (repo_read_index(repo) < 0)
> +		die("index file corrupt");

Looks good to me!

>  
>  	for (nr = 0; nr < repo->index->cache_nr; nr++) {
>  		const struct cache_entry *ce = repo->index->cache[nr];
> -- 
> 2.17.0.582.gccdcbd54c44.dirty
> 

-- 
Brandon Williams

Re: [PATCH] grep: handle corrupt index files early

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Any other caller of 'repo_read_index' dies upon a negative return of
> it, so grep should, too.

Makes sense.  When the function returns a failure, there is no
sensible fallback action anyway, so dying here is alright.

Will queue; thanks.

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Found while reviewing the series
> https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/
>
>  builtin/grep.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 6e7bc76785a..69f0743619f 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,
>  		strbuf_addstr(&name, repo->submodule_prefix);
>  	}
>  
> -	repo_read_index(repo);
> +	if (repo_read_index(repo) < 0)
> +		die("index file corrupt");
>  
>  	for (nr = 0; nr < repo->index->cache_nr; nr++) {
>  		const struct cache_entry *ce = repo->index->cache[nr];