git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Jeff King <peff@peff.net>, Brandon Williams <bmwill@google.com>
Cc: Michael Haggerty <mhagger@alum.mit.edu>, Junio C Hamano <gitster@pobox.com>, Nguyễn Thái Ngọc Duy  <pclouds@gmail.com>, Ævar Arnfjörð Bjarmason <avarab@gmail.com>, David Turner <novalis@novalis.org>, "git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct
Date: Wed, 17 May 2017 10:18:17 -0700
Message-ID: <CAGZ79kbdQ-MqYKH-L9unqEAPUtgQxNv6ANJERKSEQCADjgmm1w@mail.gmail.com> (raw)
In-Reply-To: <20170517131753.rditx62clmkrdmeq@sigill.intra.peff.net>

On Wed, May 17, 2017 at 6:17 AM, Jeff King <peff@peff.net> wrote:
> On Wed, May 17, 2017 at 02:05:33PM +0200, Michael Haggerty wrote:
>
>> Instead of using a global `lock_file` instance for the main
>> "packed-refs" file and using a pointer in `files_ref_store` to keep
>> track of whether it is locked, embed the `lock_file` instance directly
>> in the `files_ref_store` struct and use the new
>> `is_lock_file_locked()` function to keep track of whether it is
>> locked. This keeps related data together and makes the main reference
>> store less of a special case.
>
> This made me wonder how we handle the locking for ref_stores besides the
> main one (e.g., for submodules). The lockfile structs have to remain
> valid for the length of the program. Previously those stores could have
> xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()
> and leak their whole structs.

+cc Brandon, who is eager to go down that road.

> I suspect the answer is "we don't ever lock anything except the main ref
> store because that is the only one we write to", so it doesn't matter
> anyway.
>
> -Peff


> @@ -102,7 +98,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)
>        if (refs->packed) {
>                struct packed_ref_cache *packed_refs = refs->packed;
>
> -               if (refs->packlock)
> +               if (is_lock_file_locked(&refs->packlock))
>                        die("internal error: packed-ref cache cleared while locked");

I think the error message needs adjustment here as well? Maybe

     die("internal error: packed refs locked in cleanup");

Thanks,
Stefan

  parent reply index

Thread overview: 73+ messages in thread (expand / mbox.gz / Atom feed / [top])
2017-05-17 12:05 [PATCH 00/23] Prepare to separate out a packed_ref_store Michael Haggerty
2017-05-17 12:05 ` [PATCH 01/23] t3600: clean up permissions test properly Michael Haggerty
2017-05-17 12:42   ` Jeff King
2017-05-17 14:01     ` Michael Haggerty
2017-05-18  4:10   ` Junio C Hamano
2017-05-19  3:37     ` Michael Haggerty
2017-05-17 12:05 ` [PATCH 02/23] refs.h: clarify docstring for the ref_transaction_update()-related fns Michael Haggerty
2017-05-17 16:46   ` Stefan Beller
2017-05-18  4:13     ` Junio C Hamano
2017-05-17 12:05 ` [PATCH 03/23] ref_iterator_begin_fn(): fix docstring Michael Haggerty
2017-05-17 12:05 ` [PATCH 04/23] prefix_ref_iterator: don't trim too much Michael Haggerty
2017-05-17 12:55   ` Jeff King
2017-05-17 14:11     ` Michael Haggerty
2017-05-17 14:22       ` Jeff King
2017-05-18  4:19   ` Junio C Hamano
2017-05-18  4:50     ` Michael Haggerty
2017-05-17 12:05 ` [PATCH 05/23] refs_ref_iterator_begin(): don't check prefixes redundantly Michael Haggerty
2017-05-17 12:59   ` Jeff King
2017-05-17 14:21     ` Michael Haggerty
2017-05-17 12:05 ` [PATCH 06/23] refs: use `size_t` indexes when iterating over ref transaction updates Michael Haggerty
2017-05-17 16:59   ` Stefan Beller
2017-05-18  4:55     ` Michael Haggerty
2017-05-17 12:05 ` [PATCH 07/23] ref_store: take `logmsg` parameter when deleting references Michael Haggerty
2017-05-17 13:12   ` Jeff King
2017-05-17 15:01     ` Michael Haggerty
2017-05-17 15:03       ` Jeff King
2017-05-17 12:05 ` [PATCH 08/23] lockfile: add a new method, is_lock_file_locked() Michael Haggerty
2017-05-17 13:12   ` Jeff King
2017-05-17 12:05 ` [PATCH 09/23] files-backend: move `lock` member to `files_ref_store` Michael Haggerty
2017-05-17 13:15   ` Jeff King
2017-05-17 15:49     ` Michael Haggerty
2017-05-17 12:05 ` Michael Haggerty
2017-05-17 13:17   ` Jeff King
2017-05-17 15:05     ` Michael Haggerty
2017-05-17 17:18     ` Stefan Beller [this message]
2017-05-18  0:18       ` Brandon Williams
2017-05-19  4:00       ` Michael Haggerty
2017-05-18  0:17     ` Brandon Williams
2017-05-18  1:11       ` Jeff King
2017-05-18 15:42         ` Brandon Williams
2017-05-17 12:05 ` [PATCH 11/23] files_transaction_cleanup(): new helper function Michael Haggerty
2017-05-17 13:19   ` Jeff King
2017-05-19  4:49     ` Michael Haggerty
2017-05-17 17:26   ` Stefan Beller
2017-05-19  4:42     ` Michael Haggerty
2017-05-17 12:05 ` [PATCH 12/23] ref_transaction_commit(): break into multiple functions Michael Haggerty
2017-05-17 17:44   ` Stefan Beller
2017-05-19  7:58     ` Michael Haggerty
2017-05-17 12:05 ` [PATCH 13/23] ref_update_reject_duplicates(): expose function to whole refs module Michael Haggerty
2017-05-17 12:05 ` [PATCH 14/23] ref_update_reject_duplicates(): use `size_t` rather than `int` Michael Haggerty
2017-05-17 12:05 ` [PATCH 15/23] ref_update_reject_duplicates(): add a sanity check Michael Haggerty
2017-05-17 12:05 ` [PATCH 16/23] should_pack_ref(): new function, extracted from `files_pack_refs()` Michael Haggerty
2017-05-17 12:05 ` [PATCH 17/23] get_packed_ref_cache(): assume "packed-refs" won't change while locked Michael Haggerty
2017-05-17 17:57   ` Stefan Beller
2017-05-18  1:15     ` Jeff King
2017-05-18 16:58       ` Stefan Beller
2017-05-17 12:05 ` [PATCH 18/23] read_packed_refs(): do more of the work of reading packed refs Michael Haggerty
2017-05-17 12:05 ` [PATCH 19/23] read_packed_refs(): report unexpected fopen() failures Michael Haggerty
2017-05-17 13:28   ` Jeff King
2017-05-17 15:27     ` Michael Haggerty
2017-05-18  4:57     ` Junio C Hamano
2017-05-18  5:08       ` Jeff King
2017-05-17 12:05 ` [PATCH 20/23] refs_ref_iterator_begin(): handle `GIT_REF_PARANOIA` Michael Haggerty
2017-05-17 13:29   ` Jeff King
2017-05-17 12:05 ` [PATCH 21/23] create_ref_entry(): remove `check_name` option Michael Haggerty
2017-05-17 12:05 ` [PATCH 22/23] ref-filter: limit traversal to prefix Michael Haggerty
2017-05-17 13:38   ` Jeff King
2017-05-19 10:02     ` Michael Haggerty
2017-05-17 12:05 ` [PATCH 23/23] cache_ref_iterator_begin(): avoid priming unneeded directories Michael Haggerty
2017-05-17 13:42 ` Jeff King
2017-05-17 18:14   ` Stefan Beller
2017-05-18 17:14 ` Johannes Sixt
2017-05-18 17:22   ` Jeff King

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 to all the recipients using the --to, --cc,
  and --in-reply-to switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGZ79kbdQ-MqYKH-L9unqEAPUtgQxNv6ANJERKSEQCADjgmm1w@mail.gmail.com \
    --to=sbeller@google.com \
    --cc=avarab@gmail.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=mhagger@alum.mit.edu \
    --cc=novalis@novalis.org \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    /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