git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: "Michael Haggerty" <mhagger@alum.mit.edu>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Stefan Beller" <sbeller@google.com>, "Jeff King" <peff@peff.net>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"David Turner" <novalis@novalis.org>,
	"Brandon Williams" <bmwill@google.com>,
	git@vger.kernel.org
Subject: Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile
Date: Thu, 20 Jul 2017 23:01:45 -0700	[thread overview]
Message-ID: <xmqqpocuib06.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170720233051.GB159617@aiede.mtv.corp.google.com> (Jonathan Nieder's message of "Thu, 20 Jul 2017 16:30:51 -0700")

Jonathan Nieder <jrnieder@gmail.com> writes:

>> Do we care about the ancient layout that used symlinks to emulate
>> the more modern worktree one?
>
> I think we do care.  In the context of people's changing workflows,
> "git worktree" is a relatively new tool.  Breaking the older
> git-new-workdir (and tools that emulate it) would affect a large
> number of users that don't necessarily know how to clean up the
> result.

Fair enough.  Even though the git-new-workdir is on its way out, and
we really do want to do certain updates under lock, we cannot just
allow the topic to graduate to 'master' in its current form without
some transition plan or mitigation strategy.

The general strategy we take for these atomic update of a file
entity at $path is to:

 (1) try to create $path.lock exclusively; if we cannot, somebody
     else holds the lock so fail (or backoff and retry, which
     amounts to the same thing in the larger picture).

 (2) update $path.lock and close the fd.

 (3) rename $path.lock to $path atomically to unlock.

Would it be sufficent to tweak the above procedure with a new,
zeroth step?

 (0) see $path is a symlink; if so, readlink it and assign the
     result to $path.  Then go on to step (1) above.

I do not think such an enhancement would break atomicity guarantee
we want from the locking.  When updating .git/packed-refs in an
ancient new-workdir'ed directory, we would end up locking the
corresponding file in the real repository, which is exactly what we
want anyway.  As the basic assumption of having a symbolic link in
the new-workdir'ed directory is that a symlink can stay the same
forever while the linked target will be updated, I think this would
be a reasonable short-term "fix".

It would be ideal if we can do this at somewhat a low level, i.e. in
the lockfile subsystem.

Thoughts?  An even easier way out may be to just make pack-refs a
no-op when .git/refs/ is a symbolic link, but as a solution that is
far from satisfactory, while "locking the right file" smells right,
at least to me.


  reply	other threads:[~2017-07-21  6:01 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-15 14:47 [PATCH 00/28] Create a reference backend for packed refs Michael Haggerty
2017-06-15 14:47 ` [PATCH 01/28] add_packed_ref(): teach function to overwrite existing refs Michael Haggerty
2017-06-15 14:47 ` [PATCH 02/28] packed_ref_store: new struct Michael Haggerty
2017-06-15 14:47 ` [PATCH 03/28] packed_ref_store: move `packed_refs_path` here Michael Haggerty
2017-06-16  5:30   ` Stefan Beller
2017-06-15 14:47 ` [PATCH 04/28] packed_ref_store: move `packed_refs_lock` member here Michael Haggerty
2017-06-16  5:39   ` Stefan Beller
2017-06-16  6:43     ` Michael Haggerty
2017-06-16 15:44       ` Stefan Beller
2017-06-15 14:47 ` [PATCH 05/28] clear_packed_ref_cache(): take a `packed_ref_store *` parameter Michael Haggerty
2017-06-15 14:47 ` [PATCH 06/28] validate_packed_ref_cache(): " Michael Haggerty
2017-06-15 14:47 ` [PATCH 07/28] get_packed_ref_cache(): " Michael Haggerty
2017-06-15 14:47 ` [PATCH 08/28] get_packed_refs(): " Michael Haggerty
2017-06-15 14:47 ` [PATCH 09/28] add_packed_ref(): " Michael Haggerty
2017-06-15 14:47 ` [PATCH 10/28] lock_packed_refs(): " Michael Haggerty
2017-06-15 14:47 ` [PATCH 11/28] commit_packed_refs(): " Michael Haggerty
2017-06-15 14:47 ` [PATCH 12/28] rollback_packed_refs(): " Michael Haggerty
2017-06-15 14:47 ` [PATCH 13/28] get_packed_ref(): " Michael Haggerty
2017-06-15 14:47 ` [PATCH 14/28] repack_without_refs(): " Michael Haggerty
2017-06-15 14:47 ` [PATCH 15/28] packed_peel_ref(): new function, extracted from `files_peel_ref()` Michael Haggerty
2017-06-16  5:42   ` Stefan Beller
2017-06-16  6:46     ` Michael Haggerty
2017-06-16 15:47       ` Stefan Beller
2017-06-15 14:47 ` [PATCH 16/28] packed_ref_store: support iteration Michael Haggerty
2017-06-15 14:47 ` [PATCH 17/28] packed_read_raw_ref(): new function, replacing `resolve_packed_ref()` Michael Haggerty
2017-06-15 14:47 ` [PATCH 18/28] packed-backend: new module for handling packed references Michael Haggerty
2017-06-16  5:48   ` Stefan Beller
2017-06-15 14:47 ` [PATCH 19/28] packed_ref_store: make class into a subclass of `ref_store` Michael Haggerty
2017-06-15 14:47 ` [PATCH 20/28] commit_packed_refs(): report errors rather than dying Michael Haggerty
2017-06-15 14:47 ` [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile Michael Haggerty
2017-07-20 23:11   ` Junio C Hamano
2017-07-20 23:30     ` Jonathan Nieder
2017-07-21  6:01       ` Junio C Hamano [this message]
2017-07-21  7:14         ` Junio C Hamano
2017-07-24 16:59         ` Jeff King
2017-07-24 17:09           ` Jonathan Nieder
2017-07-24 17:10             ` Jeff King
2017-07-24 21:43               ` Junio C Hamano
2017-07-26 23:49                 ` Michael Haggerty
2017-06-15 14:47 ` [PATCH 22/28] packed_refs_lock(): function renamed from lock_packed_refs() Michael Haggerty
2017-06-15 14:47 ` [PATCH 23/28] packed_refs_lock(): report errors via a `struct strbuf *err` Michael Haggerty
2017-06-15 14:47 ` [PATCH 24/28] packed_refs_unlock(), packed_refs_is_locked(): new functions Michael Haggerty
2017-06-15 14:47 ` [PATCH 25/28] clear_packed_ref_cache(): don't protest if the lock is held Michael Haggerty
2017-06-15 14:47 ` [PATCH 26/28] commit_packed_refs(): remove call to `packed_refs_unlock()` Michael Haggerty
2017-06-15 14:47 ` [PATCH 27/28] repack_without_refs(): don't lock or unlock the packed refs Michael Haggerty
2017-06-15 14:47 ` [PATCH 28/28] read_packed_refs(): die if `packed-refs` contains bogus data Michael Haggerty
2017-06-19 18:51 ` [PATCH 00/28] Create a reference backend for packed refs Junio C Hamano
2017-06-19 19:25   ` Junio C Hamano
2017-06-19 19:43     ` Jeff King
2017-06-19 19:53       ` Jeff King
2017-06-22  7:36         ` Michael Haggerty
2017-06-22 15:33           ` 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=xmqqpocuib06.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=mhagger@alum.mit.edu \
    --cc=novalis@novalis.org \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    --cc=sbeller@google.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public 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).