From: Michael Haggerty <mhagger@alum.mit.edu>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: "Junio C Hamano" <gitster@pobox.com>,
"Johannes Sixt" <j6t@kdbg.org>,
"Torsten Bögershausen" <tboegi@web.de>,
"Jeff King" <peff@peff.net>,
"Ronnie Sahlberg" <sahlberg@google.com>,
git@vger.kernel.org
Subject: Re: [PATCH v5 19/35] commit_lock_file(): rollback lock file on failure to rename
Date: Tue, 23 Sep 2014 13:57:27 +0200 [thread overview]
Message-ID: <54216027.90406@alum.mit.edu> (raw)
In-Reply-To: <20140916222207.GS29050@google.com>
On 09/17/2014 12:22 AM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
>
>> If rename() fails, call rollback_lock_file() to delete the lock file
>> (in case it is still present) and reset the filename field to the
>> empty string so that the lockfile object is left in a valid state.
>
> Can you spell out more what the goal is? Is the idea to keep the
> .lock file for a shorter period of time, so other processes can lock
> that file before the current process exits?
I doubt that there are situations where releasing the .lock file
immediately vs. holding it until the end of the process makes any
practical difference. Most callers die() right away if the commit fails.
This change is more about cleaning up the API by making the state
diagram for lock_file objects well-defined. commit_lock_file() can fail
in multiple ways that callers cannot easily distinguish. So all of the
failure paths have to leave the lock_file object in the same final
state, otherwise the callers cannot know what state it is in. Since
lock_file objects are reusable, this is an API design bug.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
next prev parent reply other threads:[~2014-09-23 11:57 UTC|newest]
Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-16 19:33 [PATCH v5 00/35] Lockfile correctness and refactoring Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 01/35] unable_to_lock_die(): rename function from unable_to_lock_index_die() Michael Haggerty
2014-09-16 19:52 ` Jonathan Nieder
2014-09-16 19:33 ` [PATCH v5 02/35] api-lockfile: expand the documentation Michael Haggerty
2014-09-16 20:25 ` Jonathan Nieder
2014-09-22 14:13 ` Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 03/35] rollback_lock_file(): do not clear filename redundantly Michael Haggerty
2014-09-16 20:37 ` Jonathan Nieder
2014-09-16 19:33 ` [PATCH v5 04/35] rollback_lock_file(): exit early if lock is not active Michael Haggerty
2014-09-16 20:38 ` Jonathan Nieder
2014-09-16 19:33 ` [PATCH v5 05/35] rollback_lock_file(): set fd to -1 Michael Haggerty
2014-09-16 20:38 ` Jonathan Nieder
2014-09-16 20:39 ` Jonathan Nieder
2014-09-17 15:02 ` Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 06/35] lockfile: unlock file if lockfile permissions cannot be adjusted Michael Haggerty
2014-09-16 20:42 ` Jonathan Nieder
2014-09-16 19:33 ` [PATCH v5 07/35] hold_lock_file_for_append(): release lock on errors Michael Haggerty
2014-09-16 20:48 ` Jonathan Nieder
2014-09-17 15:39 ` Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 08/35] lock_file(): always add lock_file object to lock_file_list Michael Haggerty
2014-09-16 20:57 ` Jonathan Nieder
2014-09-17 16:10 ` Michael Haggerty
2014-09-18 4:32 ` Torsten Bögershausen
2014-09-18 7:47 ` Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 09/35] lockfile.c: document the various states of lock_file objects Michael Haggerty
2014-09-16 21:03 ` Jonathan Nieder
2014-09-22 15:20 ` Michael Haggerty
2014-09-22 22:34 ` Jonathan Nieder
2014-09-16 19:33 ` [PATCH v5 10/35] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN Michael Haggerty
2014-09-16 21:05 ` Jonathan Nieder
2014-09-22 15:25 ` Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 11/35] delete_ref_loose(): don't muck around in the lock_file's filename Michael Haggerty
2014-09-16 21:11 ` Jonathan Nieder
2014-09-16 19:33 ` [PATCH v5 12/35] prepare_index(): declare return value to be (const char *) Michael Haggerty
2014-09-16 21:17 ` Jonathan Nieder
2014-09-16 19:33 ` [PATCH v5 13/35] write_packed_entry_fn(): convert cb_data into a (const int *) Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 14/35] lock_file(): exit early if lockfile cannot be opened Michael Haggerty
2014-09-16 22:12 ` Jonathan Nieder
2014-09-16 19:33 ` [PATCH v5 15/35] remove_lock_file(): call rollback_lock_file() Michael Haggerty
2014-09-16 22:13 ` Jonathan Nieder
2014-09-16 19:33 ` [PATCH v5 16/35] commit_lock_file(): inline temporary variable Michael Haggerty
2014-09-16 22:16 ` Jonathan Nieder
2014-09-16 19:33 ` [PATCH v5 17/35] commit_lock_file(): die() if called for unlocked lockfile object Michael Haggerty
2014-09-16 22:17 ` Jonathan Nieder
2014-09-16 19:33 ` [PATCH v5 18/35] commit_lock_file(): if close fails, roll back Michael Haggerty
2014-09-16 22:19 ` Jonathan Nieder
2014-09-23 11:30 ` Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 19/35] commit_lock_file(): rollback lock file on failure to rename Michael Haggerty
2014-09-16 22:22 ` Jonathan Nieder
2014-09-23 11:57 ` Michael Haggerty [this message]
2014-09-16 19:33 ` [PATCH v5 20/35] api-lockfile: document edge cases Michael Haggerty
2014-09-16 22:23 ` Jonathan Nieder
2014-09-23 12:56 ` Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 21/35] dump_marks(): remove a redundant call to rollback_lock_file() Michael Haggerty
2014-09-16 22:24 ` Jonathan Nieder
2014-09-16 19:33 ` [PATCH v5 22/35] git_config_set_multivar_in_file(): avoid " Michael Haggerty
2014-09-16 22:28 ` Jonathan Nieder
2014-09-23 13:08 ` Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 23/35] lockfile: avoid transitory invalid states Michael Haggerty
2014-09-16 22:45 ` Jonathan Nieder
2014-09-23 13:40 ` Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 24/35] struct lock_file: declare some fields volatile Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 25/35] try_merge_strategy(): remove redundant lock_file allocation Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 26/35] try_merge_strategy(): use a statically-allocated lock_file object Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 27/35] commit_lock_file(): use a strbuf to manage temporary space Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 28/35] Change lock_file::filename into a strbuf Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 29/35] resolve_symlink(): use a strbuf for internal scratch space Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 30/35] resolve_symlink(): take a strbuf parameter Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 31/35] trim_last_path_component(): replace last_path_elm() Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 32/35] Extract a function commit_lock_file_to() Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 33/35] Rename LOCK_NODEREF to LOCK_NO_DEREF Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 34/35] lockfile.c: rename static functions Michael Haggerty
2014-09-16 19:33 ` [PATCH v5 35/35] get_locked_file_path(): new function Michael Haggerty
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54216027.90406@alum.mit.edu \
--to=mhagger@alum.mit.edu \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=j6t@kdbg.org \
--cc=jrnieder@gmail.com \
--cc=peff@peff.net \
--cc=sahlberg@google.com \
--cc=tboegi@web.de \
/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.
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 NNTP newsgroup(s).