git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Duy Nguyen <pclouds@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jeff King <peff@peff.net>, Eric Wong <e@80x24.org>,
	Antonio Ospite <ao2@ao2.it>,
	Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH] get_oid: handle NULL repo->index
Date: Wed, 15 May 2019 16:29:12 +0700	[thread overview]
Message-ID: <CACsJy8BS8NR6aZR29VLYUrRjBE_oyzH=L6X7CSpCO9G+sPjcbA@mail.gmail.com> (raw)
In-Reply-To: <xmqqh89w70w8.fsf@gitster-ct.c.googlers.com>

On Wed, May 15, 2019 at 12:16 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
> > Also from my earlier message, if you missed it:
> >
> >   I also wondered if we should simply allocate an empty index whenever
> >   we have a non-toplevel "struct repository", which might be less
> >   surprising to other callers. I don't have a strong opinion either way.
> >   I did grep around for other callers which might have similar problems,
> >   but couldn't find any.
>
> That is an approach to make it harder to make mistakes by accepting
> possibly a small wasted resource; but at that point, I think calling
> repo_read_index() unconditionally from here and similar places would
> be a simpler fix in the same spirit.

For repo_read_index() case, maybe. But we have a lot of
"r(epo)->index->something". All or most of these references
traditionally are the_index.something, which is always safe to
dereference. Submodule repos with the optionally NULL repo->index
break this assumption.

So either we audit all the code and make sure "repo->index" cannot be
NULL because repo_read_index() has been called before (and may have to
repeat auditing), or we just stick to the old assumption and make sure
repo->index is not NULL from the beginning. This makes me think the
small extra resource is worth it. Much less time will be spent on
similar issues now and in the future.

PS. Sorry Jeff for the noise. I should have waited until I come home
and can read your mail more carefully.
-- 
Duy

  reply	other threads:[~2019-05-15  9:29 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-11 20:57 new segfault in master (6a6c0f10a70a6eb1) Eric Wong
2019-05-11 22:31 ` Jeff King
2019-05-11 23:02   ` Jeff King
2019-05-12  4:26     ` Duy Nguyen
2019-05-14 13:54     ` [PATCH] get_oid: handle NULL repo->index Jeff King
2019-05-14 23:38       ` Eric Wong
2019-05-15  1:24       ` Duy Nguyen
2019-05-15  1:46         ` Jeff King
2019-05-15  5:16           ` Junio C Hamano
2019-05-15  9:29             ` Duy Nguyen [this message]
2019-05-16  1:43               ` Junio C Hamano
2019-05-19  2:56                 ` [PATCH] repository.c: always allocate 'index' at repo init time Nguyễn Thái Ngọc Duy
2019-05-20 13:17                   ` Jeff King
2019-05-21 10:34                     ` Duy Nguyen
2019-05-21 20:58                       ` Jeff King
2019-05-28 16:07                       ` Junio C Hamano

Reply instructions:

You may reply publicly 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='CACsJy8BS8NR6aZR29VLYUrRjBE_oyzH=L6X7CSpCO9G+sPjcbA@mail.gmail.com' \
    --to=pclouds@gmail.com \
    --cc=ao2@ao2.it \
    --cc=e@80x24.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --subject='Re: [PATCH] get_oid: handle NULL repo->index' \
    /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

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).