git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Duy Nguyen <pclouds@gmail.com>
Cc: Antonio Ospite <ao2@ao2.it>, Brandon Williams <bmwill@google.com>,
	git <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 02/11] repository: introduce repo_read_index_or_die
Date: Mon, 21 May 2018 11:38:53 -0700
Message-ID: <CAGZ79kbxptYvDoTqsVRe3KOA_--ja8UZir=MkMXw8_LxVXG_-Q@mail.gmail.com> (raw)
In-Reply-To: <20180519063729.GA14755@duynguyen.home>

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

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

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

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

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

Returning the number of entries makes sense.

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

I don't know.

Stefan

  reply index

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-15  1:04 [PATCH] grep: handle corrupt index files early Stefan Beller
2018-05-15 12:18 ` Johannes Schindelin
2018-05-15 13:13 ` Duy Nguyen
2018-05-15 16:44   ` Stefan Beller
2018-05-16 15:24     ` Duy Nguyen
2018-05-16 22:21       ` [PATCH 00/11] Stefan Beller
2018-05-16 22:21         ` [PATCH 01/11] grep: handle corrupt index files early Stefan Beller
2018-05-16 22:21         ` [PATCH 02/11] repository: introduce repo_read_index_or_die Stefan Beller
2018-05-19  6:37           ` Duy Nguyen
2018-05-21 18:38             ` Stefan Beller [this message]
2018-05-21 18:50               ` Brandon Williams
2018-05-21 19:27                 ` Stefan Beller
2018-05-22 15:13                   ` Duy Nguyen
2018-05-22 17:49           ` Why do we have both x*() and *_or_die() for "do or die"? Ævar Arnfjörð Bjarmason
2018-05-22 17:55             ` Duy Nguyen
2018-05-22 18:38               ` Jonathan Nieder
2018-05-22 18:04             ` Stefan Beller
2018-05-23  3:19             ` Junio C Hamano
2018-05-16 22:21         ` [PATCH 03/11] builtin/grep: use repo_read_index_or_die Stefan Beller
2018-05-16 22:21         ` [PATCH 04/11] submodule: " Stefan Beller
2018-05-16 22:21         ` [PATCH 05/11] builtin/ls-files: " Stefan Beller
2018-05-16 22:21         ` [PATCH 06/11] read_cache: use repo_read_index_or_die with different error messages Stefan Beller
2018-05-16 22:21         ` [PATCH 07/11] rerere: use repo_read_index_or_die Stefan Beller
2018-05-20 17:45           ` Thomas Gummerer
2018-05-21 18:46             ` Stefan Beller
2018-05-16 22:21         ` [PATCH 08/11] check-attr: switch to repo_read_index_or_die Stefan Beller
2018-05-16 22:21         ` [PATCH 09/11] checkout-index: switch to repo_read_index Stefan Beller
2018-05-16 22:21         ` [PATCH 10/11] test helpers: switch to repo_read_index_or_die Stefan Beller
2018-05-16 22:21         ` [PATCH 11/11] read_cache: convert most calls " Stefan Beller
2018-05-16 22:27           ` Brandon Williams
2018-05-19  6:55             ` Duy Nguyen
2018-05-19  6:54           ` Duy Nguyen
2018-05-19  6:57         ` [PATCH 00/11] Duy Nguyen
2018-05-17  1:36       ` [PATCH] grep: handle corrupt index files early Junio C Hamano
2018-05-17 17:21         ` Stefan Beller
2018-05-17 22:57           ` Junio C Hamano
2018-05-15 17:01 ` Brandon Williams
2018-05-15 23:58 ` Junio C Hamano

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGZ79kbxptYvDoTqsVRe3KOA_--ja8UZir=MkMXw8_LxVXG_-Q@mail.gmail.com' \
    --to=sbeller@google.com \
    --cc=ao2@ao2.it \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox