git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Martin Ågren" <martin.agren@gmail.com>
To: git@vger.kernel.org
Cc: Jeff King <peff@peff.net>,
	Christian Couder <christian.couder@gmail.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: [PATCH v2 07/12] apply: move lockfile into `apply_state`
Date: Thu,  5 Oct 2017 22:32:09 +0200	[thread overview]
Message-ID: <c8426ab17ccdd07d75282b612c19a88d5d292c5f.1507228170.git.martin.agren@gmail.com> (raw)
In-Reply-To: <cover.1507228170.git.martin.agren@gmail.com>

We have two users of `struct apply_state` and the related functionality
in apply.c. Each user sets up its `apply_state` by handing over a
pointer to its static `lock_file`. (Before 076aa2cbd (tempfile:
auto-allocate tempfiles on heap, 2017-09-05), we could never free
lockfiles, so making them static was a reasonable approach.)

Other than that, they never directly access their `lock_file`s, which
are instead handled by the functionality in apply.c.

To make life easier for the caller and to make it less tempting for a
future caller to mess with the lock, make apply.c fully responsible for
setting up the `lock_file`. As mentioned above, it is now safe to free a
`lock_file`, so we can make the `struct apply_state` contain an actual
`struct lock_file` instead of a pointer to one.

The user in builtin/apply.c is rather simple. For builtin/am.c, we might
worry that the lock state is actually meant to be inherited across
calls. But the lock is only taken as `apply_all_patches()` executes, and
code inspection shows that it will always be released.

Alternatively, we can observe that the lock itself is never queried
directly. When we decide whether we should lock, we check a related
variable `newfd`. That variable is not inherited, so from the point of
view of apply.c, the state machine really is reset with each call to
`init_apply_state()`. (It would be a bug if `newfd` and the lock status
were not in sync. The duplication of information in `newfd` and the lock
will be addressed in the next patch.)

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
v2: Identical.

 apply.c         | 14 +++++---------
 apply.h         |  5 ++---
 builtin/am.c    |  3 +--
 builtin/apply.c |  4 +---
 4 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/apply.c b/apply.c
index c022af53a..5a6ca10a7 100644
--- a/apply.c
+++ b/apply.c
@@ -75,12 +75,10 @@ static int parse_ignorewhitespace_option(struct apply_state *state,
 }
 
 int init_apply_state(struct apply_state *state,
-		     const char *prefix,
-		     struct lock_file *lock_file)
+		     const char *prefix)
 {
 	memset(state, 0, sizeof(*state));
 	state->prefix = prefix;
-	state->lock_file = lock_file;
 	state->newfd = -1;
 	state->apply = 1;
 	state->line_termination = '\n';
@@ -146,8 +144,6 @@ int check_apply_state(struct apply_state *state, int force_apply)
 	}
 	if (state->check_index)
 		state->unsafe_paths = 0;
-	if (!state->lock_file)
-		return error("BUG: state->lock_file should not be NULL");
 
 	if (state->apply_verbosity <= verbosity_silent) {
 		state->saved_error_routine = get_error_routine();
@@ -4711,11 +4707,11 @@ static int apply_patch(struct apply_state *state,
 	state->update_index = state->check_index && state->apply;
 	if (state->update_index && state->newfd < 0) {
 		if (state->index_file)
-			state->newfd = hold_lock_file_for_update(state->lock_file,
+			state->newfd = hold_lock_file_for_update(&state->lock_file,
 								 state->index_file,
 								 LOCK_DIE_ON_ERROR);
 		else
-			state->newfd = hold_locked_index(state->lock_file, LOCK_DIE_ON_ERROR);
+			state->newfd = hold_locked_index(&state->lock_file, LOCK_DIE_ON_ERROR);
 	}
 
 	if (state->check_index && read_apply_cache(state) < 0) {
@@ -4911,7 +4907,7 @@ int apply_all_patches(struct apply_state *state,
 	}
 
 	if (state->update_index) {
-		res = write_locked_index(&the_index, state->lock_file, COMMIT_LOCK);
+		res = write_locked_index(&the_index, &state->lock_file, COMMIT_LOCK);
 		if (res) {
 			error(_("Unable to write new index file"));
 			res = -128;
@@ -4924,7 +4920,7 @@ int apply_all_patches(struct apply_state *state,
 
 end:
 	if (state->newfd >= 0) {
-		rollback_lock_file(state->lock_file);
+		rollback_lock_file(&state->lock_file);
 		state->newfd = -1;
 	}
 
diff --git a/apply.h b/apply.h
index d9b395770..cf00cda17 100644
--- a/apply.h
+++ b/apply.h
@@ -37,7 +37,7 @@ struct apply_state {
 	const char *prefix;
 
 	/* These are lock_file related */
-	struct lock_file *lock_file;
+	struct lock_file lock_file;
 	int newfd;
 
 	/* These control what gets looked at and modified */
@@ -116,8 +116,7 @@ extern int apply_parse_options(int argc, const char **argv,
 			       int *force_apply, int *options,
 			       const char * const *apply_usage);
 extern int init_apply_state(struct apply_state *state,
-			    const char *prefix,
-			    struct lock_file *lock_file);
+			    const char *prefix);
 extern void clear_apply_state(struct apply_state *state);
 extern int check_apply_state(struct apply_state *state, int force_apply);
 
diff --git a/builtin/am.c b/builtin/am.c
index 4e16fd428..40968428d 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1488,11 +1488,10 @@ static int run_apply(const struct am_state *state, const char *index_file)
 	struct argv_array apply_opts = ARGV_ARRAY_INIT;
 	struct apply_state apply_state;
 	int res, opts_left;
-	static struct lock_file lock_file;
 	int force_apply = 0;
 	int options = 0;
 
-	if (init_apply_state(&apply_state, NULL, &lock_file))
+	if (init_apply_state(&apply_state, NULL))
 		die("BUG: init_apply_state() failed");
 
 	argv_array_push(&apply_opts, "apply");
diff --git a/builtin/apply.c b/builtin/apply.c
index 81b9a61c3..48d398933 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -9,8 +9,6 @@ static const char * const apply_usage[] = {
 	NULL
 };
 
-static struct lock_file lock_file;
-
 int cmd_apply(int argc, const char **argv, const char *prefix)
 {
 	int force_apply = 0;
@@ -18,7 +16,7 @@ int cmd_apply(int argc, const char **argv, const char *prefix)
 	int ret;
 	struct apply_state state;
 
-	if (init_apply_state(&state, prefix, &lock_file))
+	if (init_apply_state(&state, prefix))
 		exit(128);
 
 	argc = apply_parse_options(argc, argv,
-- 
2.14.2.666.gea220ee40


  parent reply	other threads:[~2017-10-05 20:32 UTC|newest]

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-01 14:56 [PATCH 00/11] various lockfile-leaks and -fixes Martin Ågren
2017-10-01 14:56 ` [PATCH 01/11] sha1_file: do not leak `lock_file` Martin Ågren
2017-10-02  5:26   ` Jeff King
2017-10-02 10:15     ` Martin Ågren
2017-10-01 14:56 ` [PATCH 02/11] treewide: prefer lockfiles on the stack Martin Ågren
2017-10-02  3:37   ` Junio C Hamano
2017-10-02  4:12     ` Martin Ågren
2017-10-02  5:34   ` Jeff King
2017-10-01 14:56 ` [PATCH 03/11] lockfile: fix documentation on `close_lock_file_gently()` Martin Ågren
2017-10-02  5:35   ` Jeff King
2017-10-01 14:56 ` [PATCH 04/11] tempfile: fix documentation on `delete_tempfile()` Martin Ågren
2017-10-02  5:38   ` Jeff King
2017-10-01 14:56 ` [PATCH 05/11] cache-tree: simplify locking logic Martin Ågren
2017-10-02  3:40   ` Junio C Hamano
2017-10-02  5:41   ` Jeff King
2017-10-01 14:56 ` [PATCH 06/11] apply: move lockfile into `apply_state` Martin Ågren
2017-10-02  5:48   ` Jeff King
2017-10-01 14:56 ` [PATCH 07/11] apply: remove `newfd` from `struct apply_state` Martin Ågren
2017-10-02  5:50   ` Jeff King
2017-10-01 14:56 ` [PATCH 08/11] cache.h: document `write_locked_index()` Martin Ågren
2017-10-01 14:56 ` [PATCH 09/11] read-cache: require flags for `write_locked_index()` Martin Ågren
2017-10-02  3:49   ` Junio C Hamano
2017-10-02  4:14     ` Martin Ågren
2017-10-02 10:16       ` Martin Ågren
2017-10-02  6:00   ` Jeff King
2017-10-01 14:56 ` [PATCH 10/11] read-cache: don't leave dangling pointer in `do_write_index()` Martin Ågren
2017-10-02  6:15   ` Jeff King
2017-10-02  6:20     ` Jeff King
2017-10-01 14:56 ` [PATCH 11/11] read-cache: roll back lock on error with `COMMIT_LOCK` Martin Ågren
2017-10-02  4:01   ` Junio C Hamano
2017-10-02  2:37 ` [PATCH 00/11] various lockfile-leaks and -fixes Junio C Hamano
2017-10-02  6:22 ` Jeff King
2017-10-02  6:30   ` Junio C Hamano
2017-10-02 10:19     ` Martin Ågren
2017-10-03  6:21       ` Junio C Hamano
2017-10-05 20:32         ` [PATCH v2 00/12] " Martin Ågren
2017-10-05 20:32           ` [PATCH v2 01/12] sha1_file: do not leak `lock_file` Martin Ågren
2017-10-06  1:17             ` Junio C Hamano
2017-10-05 20:32           ` [PATCH v2 02/12] treewide: prefer lockfiles on the stack Martin Ågren
2017-10-05 20:32           ` [PATCH v2 03/12] lockfile: fix documentation on `close_lock_file_gently()` Martin Ågren
2017-10-05 20:32           ` [PATCH v2 04/12] tempfile: fix documentation on `delete_tempfile()` Martin Ågren
2017-10-05 20:32           ` [PATCH v2 05/12] checkout-index: simplify locking logic Martin Ågren
2017-10-06  1:21             ` Junio C Hamano
2017-10-05 20:32           ` [PATCH v2 06/12] cache-tree: " Martin Ågren
2017-10-05 20:32           ` Martin Ågren [this message]
2017-10-05 20:32           ` [PATCH v2 08/12] apply: remove `newfd` from `struct apply_state` Martin Ågren
2017-10-05 20:32           ` [PATCH v2 09/12] cache.h: document `write_locked_index()` Martin Ågren
2017-10-05 20:32           ` [PATCH v2 10/12] read-cache: drop explicit `CLOSE_LOCK`-flag Martin Ågren
2017-10-06  1:39             ` Junio C Hamano
2017-10-06 11:02               ` Martin Ågren
2017-10-05 20:32           ` [PATCH v2 11/12] read-cache: leave lock in right state in `write_locked_index()` Martin Ågren
2017-10-06  2:01             ` Junio C Hamano
2017-10-06 11:04               ` Martin Ågren
2017-10-06 12:02                 ` Junio C Hamano
2017-10-06 19:44                   ` Martin Ågren
2017-10-06 20:12                     ` [PATCH v3 00/12] Re: various lockfile-leaks and -fixes Martin Ågren
2017-10-06 20:12                       ` [PATCH v3 01/12] sha1_file: do not leak `lock_file` Martin Ågren
2017-10-06 20:12                       ` [PATCH v3 02/12] treewide: prefer lockfiles on the stack Martin Ågren
2017-10-06 20:12                       ` [PATCH v3 03/12] lockfile: fix documentation on `close_lock_file_gently()` Martin Ågren
2017-10-06 20:12                       ` [PATCH v3 04/12] tempfile: fix documentation on `delete_tempfile()` Martin Ågren
2017-10-06 20:12                       ` [PATCH v3 05/12] checkout-index: simplify locking logic Martin Ågren
2017-10-06 20:12                       ` [PATCH v3 06/12] cache-tree: " Martin Ågren
2017-10-06 20:12                       ` [PATCH v3 07/12] apply: move lockfile into `apply_state` Martin Ågren
2017-10-06 20:12                       ` [PATCH v3 08/12] apply: remove `newfd` from `struct apply_state` Martin Ågren
2017-10-06 20:12                       ` [PATCH v3 09/12] cache.h: document `write_locked_index()` Martin Ågren
2017-10-06 20:12                       ` [PATCH v3 10/12] read-cache: drop explicit `CLOSE_LOCK`-flag Martin Ågren
2017-10-06 20:12                       ` [PATCH v3 11/12] read-cache: leave lock in right state in `write_locked_index()` Martin Ågren
2017-10-06 20:12                       ` [PATCH v3 12/12] read_cache: roll back lock in `update_index_if_able()` Martin Ågren
2017-10-05 20:32           ` [PATCH v2 " Martin Ågren

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=c8426ab17ccdd07d75282b612c19a88d5d292c5f.1507228170.git.martin.agren@gmail.com \
    --to=martin.agren@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.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
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).