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

From: Stefan Beller <sbeller@google.com>
Date: May 21, 2018, 18:38 UTC
Message-ID: <CAGZ79kbxptYvDoTqsVRe3KOA_--ja8UZir=MkMXw8_LxVXG_-Q@mail.gmail.com>
In-Reply-To: <20180519063729.GA14755@duynguyen.home>

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