grep: handle corrupt index files early

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

Signed-off-by Stefan Beller <[email protected]>

---

Found while reviewing the series
https://public-inbox.org/git/[email protected]/

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

builtin/grep.c +0 -0

index 6e7bc76785a..69f0743619f 100644

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

Signed-off-by Stefan Beller <[email protected]>

---

Found while reviewing the series
https://public-inbox.org/git/[email protected]/

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

builtin/grep.c +0 -0

index 6e7bc76785a..69f0743619f 100644

builtin/grep.c +0 -1

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

2.17.0.582.gccdcbd54c44.dirty

Quoted 20 lines · 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 <[email protected]>

---

Found while reviewing the series
https://public-inbox.org/git/[email protected]/

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

builtin/grep.c +0 -0

index 6e7bc76785a..69f0743619f 100644
@@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,
                strbuf_addstr(&name, repo->submodule_prefix);
        }
 

builtin/grep.c static int grep_cache(struct grep_opt *opt, struct repository *repo, · line 492

Quoted 3 lines

builtin/grep.c +2 -1

-       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)

Quoted 21 lines · Duy Nguyen

Quoted 20 lines · 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 <[email protected]>

---

Found while reviewing the series
https://public-inbox.org/git/[email protected]/

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

builtin/grep.c +0 -0

index 6e7bc76785a..69f0743619f 100644
@@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,
                strbuf_addstr(&name, repo->submodule_prefix);
        }
 

Quoted 6 lines

builtin/grep.c static int grep_cache(struct grep_opt *opt, struct repository *repo, · line 492

Quoted 3 lines

builtin/grep.c +2 -1

-       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

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 _()

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 <[email protected]>

---
 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 +--

Quoted 7 lines · 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 <[email protected]>

Quoted 8 lines

---
 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"));

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.

Quoted 17 lines · Duy Nguyen

Quoted 7 lines · 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 <[email protected]>

Quoted 8 lines

---
 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 +--

Quoted 8 lines

'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.

Quoted 12 lines

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.

Quoted 4 lines

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

Quoted 3 lines

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.

Hi Brandon,

Quoted 9 lines

Quoted 3 lines

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.

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.

Quoted 2 lines · 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.

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.

Quoted 2 lines · Ævar Arnfjörð Bjarmason

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.

Quoted 2 lines · 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.

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.

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.

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 <[email protected]>

---
 rerere.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

rerere.c +4 -6

index 18cae2d11c9..5f35dd66f90 100644
@@ -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)

Signature · 1 line

2.17.0.582.gccdcbd54c44.dirty

Quoted 2 lines · 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.

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 <[email protected]>

---
 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(-)

blame.c +3 -2

index 78c9808bd1a..ebfa1c8efcd 100644
@@ -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) {

builtin/am.c +2 -1

index d834f9e62b6..3c6e77a5369 100644
@@ -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

builtin/diff.c +2 -1

index 16bfb22f738..4bba211f1c7 100644
@@ -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);
 }

builtin/fsck.c +2 -1

index 087360a6757..a42e98235da 100644
@@ -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;

builtin/merge-index.c +2 -1

index c99443b095b..2d91c7c3b5e 100644
@@ -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")) {

check-racy.c +1 -1

index 24b6542352a..9b884639cf4 100644
@@ -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.c +1 -0

index 1289df4b1f9..383f52fa118 100644
@@ -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

diff.c +1 -1

 			 * 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 */

merge-recursive.c +0 -0

index 0c0d48624da..76911c935c3 100644

merge-recursive.c +0 -0

 	o->ancestor = "merged common ancestors";
 	clean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree,

revision.c +3 -2

index 1cff11833e7..8ad9824143d 100644
@@ -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))

sequencer.c +3 -2

index 4ce5120e777..773165c8cde 100644
@@ -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)";

sha1-name.c +1 -1

index 5b93bf8da36..83d5f945cf1 100644
@@ -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;

Signature · 1 line

2.17.0.582.gccdcbd54c44.dirty

Maybe we can kill read_cache() while at it?

Signed-off-by Stefan Beller <[email protected]>

---
 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 +1 -0

index 1289df4b1f9..383f52fa118 100644
@@ -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

revision.c +1 -0

index 1cff11833e7..8ad9824143d 100644
@@ -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;
 

sha1-name.c +0 -0

 		pos = cache_name_pos(cp, namelen);
 		if (pos < 0)
 			pos = -pos - 1;

Signature · 1 line

2.17.0.582.gccdcbd54c44.dirty

Quoted 7 lines

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?

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 _()

Quoted 3 lines · Duy Nguyen

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-<.

Quoted 6 lines · Junio C Hamano

Quoted 3 lines · Duy Nguyen

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 _()

Quoted 7 lines

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/[email protected]/
retracted for this cycle; I will keep it around and resend it at some future date, hopefully.

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

Signed-off-by Stefan Beller <[email protected]>

---

Found while reviewing the series
https://public-inbox.org/git/[email protected]/

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

builtin/grep.c +0 -0

index 6e7bc76785a..69f0743619f 100644

builtin/grep.c +0 -0

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

Signature · 1 line

2.17.0.582.gccdcbd54c44.dirty

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

Signed-off-by Stefan Beller <[email protected]>

---

Found while reviewing the series
https://public-inbox.org/git/[email protected]/

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

builtin/grep.c +2 -1

index 6e7bc76785a..69f0743619f 100644
@@ -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];