threads / patch / 48497

patchgrep: handle corrupt index files early

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

## tl;dr

38 messages between May 15, 2018 and May 23, 2018. Diffs are folded; open one to read it.

replies: 37people: 8as markdown or json

Stefan Beller· May 15, 2018, 01:04 UTC · lore

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(-)
Show changes to builtin/grep.c +2 −1
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
Johannes Schindelin· May 15, 2018, 12:18 UTC · re: Stefan Beller · lore

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

Hi Stefan,
On Mon, 14 May 2018, Stefan Beller wrote:
Show 23 quoted lines
> 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

Duy Nguyen· May 15, 2018, 13:13 UTC · re: Stefan Beller · lore

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

On Tue, May 15, 2018 at 3:04 AM, Stefan Beller <sbeller@google.com> wrote:
Show 23 quoted lines
> 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)

Show 6 quoted lines
>
>         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
Stefan Beller· May 15, 2018, 16:44 UTC · re: Duy Nguyen · lore

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

On Tue, May 15, 2018 at 6:13 AM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 27 quoted lines
> 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

Duy Nguyen· May 16, 2018, 15:24 UTC · re: Stefan Beller · lore

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

On Tue, May 15, 2018 at 6:44 PM, Stefan Beller <sbeller@google.com> wrote:
Show 60 quoted lines
> 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
Stefan Beller· May 16, 2018, 22:21 UTC · re: Duy Nguyen · lore

[PATCH 00/11]

> 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
Stefan Beller· May 16, 2018, 22:21 UTC · re: Stefan Beller · lore

[PATCH 01/11] 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 <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/grep.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to builtin/grep.c +2 −1
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
Stefan Beller· May 16, 2018, 22:21 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to 4 files +11 −10

builtin/add.c, builtin/commit.c, builtin/rev-parse.c, merge.c

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
Stefan Beller· May 16, 2018, 22:21 UTC · re: Stefan Beller · lore

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/checkout-index.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
Show changes to builtin/checkout-index.c +2 −3
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
Stefan Beller· May 16, 2018, 22:21 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to builtin/ls-files.c +3 −4
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
Stefan Beller· May 16, 2018, 22:21 UTC · re: Stefan Beller · lore

[PATCH 07/11] rerere: use repo_read_index_or_die

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(-)
Show changes to rerere.c +4 −6
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
Thomas Gummerer· May 20, 2018, 17:45 UTC · re: Stefan Beller · lore

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

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.

Show 50 quoted lines
> 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
>
Stefan Beller· May 21, 2018, 18:46 UTC · re: Thomas Gummerer · lore

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

Hi Thomas,
On Sun, May 20, 2018 at 10:45 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
Show 10 quoted lines
> 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.

Show 5 quoted lines
> 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
Stefan Beller· May 16, 2018, 22:21 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to 11 files +24 −15

blame.c, builtin/am.c, builtin/diff.c, builtin/fsck.c, builtin/merge-index.c, check-racy.c, diff.c, merge-recursive.c, revision.c, sequencer.c, sha1-name.c

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
Brandon Williams· May 16, 2018, 22:27 UTC · re: Stefan Beller · lore

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

On 05/16, Stefan Beller wrote:
Show 159 quoted lines
> 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
Show 21 quoted lines
>  			 * 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?
Show 79 quoted lines
>  
>  	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
Duy Nguyen· May 19, 2018, 06:55 UTC · re: Brandon Williams · lore

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

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

Duy Nguyen· May 19, 2018, 06:54 UTC · re: Stefan Beller · lore

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

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

Show 7 quoted lines
> 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.
Show 18 quoted lines
> 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/
Show 7 quoted lines
>  			 * 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?
Show 18 quoted lines
> 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.

Show 10 quoted lines
> 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 :)
Show 6 quoted lines
>  		pos = cache_name_pos(cp, namelen);
>  		if (pos < 0)
>  			pos = -pos - 1;
> -- 
> 2.17.0.582.gccdcbd54c44.dirty
>
Stefan Beller· May 16, 2018, 22:21 UTC · re: Stefan Beller · lore

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/check-attr.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
Show changes to builtin/check-attr.c +2 −3
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
Stefan Beller· May 16, 2018, 22:21 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to 6 files +16 −14

t/helper/test-dump-cache-tree.c, t/helper/test-dump-untracked-cache.c, t/helper/test-lazy-init-name-hash.c, t/helper/test-read-cache.c, t/helper/test-scrap-cache-tree.c, t/helper/test-write-cache.c

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
Stefan Beller· May 16, 2018, 22:21 UTC · re: Stefan Beller · lore

[PATCH 02/11] repository: introduce repo_read_index_or_die

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(-)
Show changes to 9 files +25 −15

builtin/add.c, builtin/check-ignore.c, builtin/clean.c, builtin/mv.c, builtin/reset.c, builtin/rm.c, builtin/submodule--helper.c, repository.c, repository.h

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
Duy Nguyen· May 19, 2018, 06:37 UTC · re: Stefan Beller · lore

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

On Wed, May 16, 2018 at 03:21:09PM -0700, Stefan Beller wrote:
Show 15 quoted lines
> 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

Stefan Beller· May 21, 2018, 18:38 UTC · re: Duy Nguyen · lore

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

On Fri, May 18, 2018 at 11:37 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 18 quoted lines
> 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 (*)
Show 6 quoted lines
> (*) 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.

Show 16 quoted 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.
>
> 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
Brandon Williams· May 21, 2018, 18:50 UTC · re: Stefan Beller · lore

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

On 05/21, Stefan Beller wrote:
Show 55 quoted lines
> 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
Stefan Beller· May 21, 2018, 19:27 UTC · re: Brandon Williams · lore

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

Hi Brandon,
Show 9 quoted 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.

Thanks, Stefan

Duy Nguyen· May 22, 2018, 15:13 UTC · re: Stefan Beller · lore

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

On Mon, May 21, 2018 at 9:27 PM, Stefan Beller <sbeller@google.com> wrote:
Show 25 quoted lines
> 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
Ævar Arnfjörð Bjarmason· May 22, 2018, 17:49 UTC · re: Stefan Beller · lore

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

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.

Duy Nguyen· May 22, 2018, 17:55 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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

Show 8 quoted lines
>
> 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.

Show 10 quoted lines
> 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
Jonathan Nieder· May 22, 2018, 18:38 UTC · re: Duy Nguyen · lore

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

Duy Nguyen wrote:
> On Tue, May 22, 2018 at 7:49 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
Show 8 quoted lines
>> 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

Stefan Beller· May 22, 2018, 18:04 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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

Show 5 quoted lines
>
> 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

Junio C Hamano· May 23, 2018, 03:19 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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

Stefan Beller· May 16, 2018, 22:21 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to builtin/grep.c +1 −2
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
Stefan Beller· May 16, 2018, 22:21 UTC · re: Stefan Beller · lore

[PATCH 04/11] submodule: use repo_read_index_or_die

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(-)
Show changes to submodule.c +1 −2
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
Duy Nguyen· May 19, 2018, 06:57 UTC · re: Stefan Beller · lore

Re: [PATCH 00/11]

On Wed, May 16, 2018 at 03:21:07PM -0700, Stefan Beller wrote:
Show 9 quoted 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. :)
Yay! (And sorry, Junio)
Show 18 quoted lines
> 
> 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

Junio C Hamano· May 17, 2018, 01:36 UTC · re: Duy Nguyen · lore

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

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

Stefan Beller· May 17, 2018, 17:21 UTC · re: Junio C Hamano · lore

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

On Wed, May 16, 2018 at 6:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
> 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
Junio C Hamano· May 17, 2018, 22:57 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 19 quoted lines
> 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.
Brandon Williams· May 15, 2018, 17:01 UTC · re: Stefan Beller · lore

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

On 05/14, Stefan Beller wrote:
Show 23 quoted lines
> 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!
Show 6 quoted lines
>  
>  	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
Junio C Hamano· May 15, 2018, 23:58 UTC · re: Stefan Beller · lore

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

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.
Show 24 quoted lines
>
> 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];

← back to recent threads