Re: [PATCH] grep: handle corrupt index files early
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- May 16, 2018, 15:24 UTC
- Message-ID
- <CACsJy8A8WZ-Gqe2Y-whJmbADrt+gZjLZ7MTwCtdnK7JDnEdtog@mail.gmail.com>
- In-Reply-To
- <CAGZ79kYYy-r+Zs_6yg1ZVRNsBcTUdPWE=FeHrp_=0cV0Uf_GfQ@mail.gmail.com>
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