[PATCH 0/5] getting rid of most "static struct lock_file"s

[PATCH 0/5] getting rid of most "static struct lock_file"s

[Martin Ågren]

This series addresses two classes of "static struct lock_file", removing
the staticness: Those locks that already live inside a function, and
those that can simply be moved into the function they are used from.

The first three patches are some cleanups I noticed along the way, where
we first take a lock using LOCK_DIE_ON_ERROR, then check whether we got
it.

After this series, we have a small number of "static struct lock_file"
left, namely those locks that are used from within more than one
function. Thus, after this series, when one stumbles upon a static
lock_file, it should be a clear warning that the lock is being
used by more than one function.

Martin

Martin Ågren (5):
  t/helper/test-write-cache: clean up lock-handling
  refs.c: do not die if locking fails in `write_pseudoref()`
  refs.c: drop dead code checking lock status in `delete_pseudoref()`
  lock_file: make function-local locks non-static
  lock_file: move static locks into functions

 t/helper/test-scrap-cache-tree.c |  4 ++--
 t/helper/test-write-cache.c      | 14 +++++---------
 apply.c                          |  2 +-
 builtin/add.c                    |  3 +--
 builtin/describe.c               |  2 +-
 builtin/difftool.c               |  2 +-
 builtin/gc.c                     |  2 +-
 builtin/merge.c                  |  4 ++--
 builtin/mv.c                     |  2 +-
 builtin/read-tree.c              |  3 +--
 builtin/receive-pack.c           |  2 +-
 builtin/rm.c                     |  3 +--
 bundle.c                         |  2 +-
 fast-import.c                    |  2 +-
 refs.c                           | 12 ++++--------
 refs/files-backend.c             |  2 +-
 rerere.c                         |  3 +--
 shallow.c                        |  2 +-
 18 files changed, 27 insertions(+), 39 deletions(-)

-- 
2.17.0.411.g9fd64c8e46

Re: [PATCH 0/5] getting rid of most "static struct lock_file"s

[Jeff King]

On Sun, May 06, 2018 at 04:10:26PM +0200, Martin Ågren wrote:

> This series addresses two classes of "static struct lock_file", removing
> the staticness: Those locks that already live inside a function, and
> those that can simply be moved into the function they are used from.
> 
> The first three patches are some cleanups I noticed along the way, where
> we first take a lock using LOCK_DIE_ON_ERROR, then check whether we got
> it.
> 
> After this series, we have a small number of "static struct lock_file"
> left, namely those locks that are used from within more than one
> function. Thus, after this series, when one stumbles upon a static
> lock_file, it should be a clear warning that the lock is being
> used by more than one function.

These all look fine to me. The commit messages all made perfect sense to
me, but it sounds like some people weren't aware of the new
post-076aa2cbda rules. So maybe it makes sense to reference that even in
the earlier commits, and to explicitly say that it's safe to convert
even in the case where the lock_file goes out of scope while still
active.

The only dangerous thing left to check for is anybody holding onto a
pointer-to-lockfile. The only such pointer declared outside of a
parameter list is in create_reflock(), and there it's just to
temporarily coerce a void pointer. So unless somebody is doing something
really tricky (putting a pointer-to-lock in a "void *"), I think these
conversions all have to be trivially correct (famous last words...).

-Peff

[PATCH v2 0/5] getting rid of most "static struct lock_file"s

[Martin Ågren]

This is take two of my attempt at making almost all `struct lock_file`s
non-static. All patches have been equipped with more detailed commit
messages. The only diff that has changed is patch 3/5, where I now take
a small step towards gentle error-handling, rather than going in the
opposite direction.

Thanks all for the valuable feedback on v1. I could have saved everyone
some trouble by writing better commit messages from the start, and
probably also by using `--thread` when formatting the patches...

Martin

Martin Ågren (5):
  t/helper/test-write-cache: clean up lock-handling
  refs.c: do not die if locking fails in `write_pseudoref()`
  refs.c: do not die if locking fails in `delete_pseudoref()`
  lock_file: make function-local locks non-static
  lock_file: move static locks into functions

 t/helper/test-scrap-cache-tree.c |  4 ++--
 t/helper/test-write-cache.c      | 14 +++++---------
 apply.c                          |  2 +-
 builtin/add.c                    |  3 +--
 builtin/describe.c               |  2 +-
 builtin/difftool.c               |  2 +-
 builtin/gc.c                     |  2 +-
 builtin/merge.c                  |  4 ++--
 builtin/mv.c                     |  2 +-
 builtin/read-tree.c              |  3 +--
 builtin/receive-pack.c           |  2 +-
 builtin/rm.c                     |  3 +--
 bundle.c                         |  2 +-
 fast-import.c                    |  2 +-
 refs.c                           | 16 +++++++++-------
 refs/files-backend.c             |  2 +-
 rerere.c                         |  3 +--
 shallow.c                        |  2 +-
 18 files changed, 32 insertions(+), 38 deletions(-)

-- 
2.17.0.411.g9fd64c8e46

[PATCH v2 1/5] t/helper/test-write-cache: clean up lock-handling

[Martin Ågren]

Die in case writing the index fails, so that the caller can notice
(instead of, say, being impressed by how performant the writing is).

While at it, note that after opening a lock with `LOCK_DIE_ON_ERROR`, we
do not need to worry about whether we succeeded. Also, we can move the
`struct lock_file` into the function and drop the staticness. (Placing
`struct lock_file`s on the stack used to be a bad idea, because the
temp- and lockfile-machinery would keep a pointer into the struct. But
after 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),
we can safely have lockfiles on the stack.)

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/helper/test-write-cache.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/t/helper/test-write-cache.c b/t/helper/test-write-cache.c
index 017dc30380..8837717d36 100644
--- a/t/helper/test-write-cache.c
+++ b/t/helper/test-write-cache.c
@@ -2,22 +2,18 @@
 #include "cache.h"
 #include "lockfile.h"
 
-static struct lock_file index_lock;
-
 int cmd__write_cache(int argc, const char **argv)
 {
-	int i, cnt = 1, lockfd;
+	struct lock_file index_lock = LOCK_INIT;
+	int i, cnt = 1;
 	if (argc == 2)
 		cnt = strtol(argv[1], NULL, 0);
 	setup_git_directory();
 	read_cache();
 	for (i = 0; i < cnt; i++) {
-		lockfd = hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
-		if (0 <= lockfd) {
-			write_locked_index(&the_index, &index_lock, COMMIT_LOCK);
-		} else {
-			rollback_lock_file(&index_lock);
-		}
+		hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
+		if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
+			die("unable to write index file");
 	}
 
 	return 0;
-- 
2.17.0.411.g9fd64c8e46

[PATCH v2 2/5] refs.c: do not die if locking fails in `write_pseudoref()`

[Martin Ågren]

If we could not take the lock, we add an error to the `strbuf err` and
return. However, this code is dead. The reason is that we take the lock
using `LOCK_DIE_ON_ERROR`. Drop the flag to allow our more gentle
error-handling to actually kick in.

We could instead just drop the dead code and die here. But everything is
prepared for gently propagating the error, so let's do that instead.

There is similar dead code in `delete_pseudoref()`, but let's save that
for the next patch.

While at it, make the lock non-static. (Placing `struct lock_file`s on
the stack used to be a bad idea, because the temp- and
lockfile-machinery would keep a pointer into the struct. But after
076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we
can safely have lockfiles on the stack.)

Reviewed-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 refs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 8b7a77fe5e..8c50b8b139 100644
--- a/refs.c
+++ b/refs.c
@@ -644,7 +644,7 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,
 {
 	const char *filename;
 	int fd;
-	static struct lock_file lock;
+	struct lock_file lock = LOCK_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	int ret = -1;
 
@@ -654,8 +654,7 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,
 	strbuf_addf(&buf, "%s\n", oid_to_hex(oid));
 
 	filename = git_path("%s", pseudoref);
-	fd = hold_lock_file_for_update_timeout(&lock, filename,
-					       LOCK_DIE_ON_ERROR,
+	fd = hold_lock_file_for_update_timeout(&lock, filename, 0,
 					       get_files_ref_lock_timeout_ms());
 	if (fd < 0) {
 		strbuf_addf(err, "could not open '%s' for writing: %s",
-- 
2.17.0.411.g9fd64c8e46

[PATCH v2 3/5] refs.c: do not die if locking fails in `delete_pseudoref()`

[Martin Ågren]

After taking the lock we check whether we got it and die otherwise. But
since we take the lock using `LOCK_DIE_ON_ERROR`, we would already have
died.

Considering the choice between dropping the dead code and dropping the
flag, let's go for option number three: Drop the flag, write an error
instead of dying, then return -1. This function already returns -1 for
another error, so the caller (or rather, its callers) should be able to
handle this. There is some inconsistency around how we handle errors in
this function and elsewhere in this file, but let's take this small step
towards gentle error-reporting now and leave the rest for another time.

While at it, make the lock non-static and reduce its scope. (Placing
`struct lock_file`s on the stack used to be a bad idea, because the
temp- and lockfile-machinery would keep a pointer into the struct. But
after 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),
we can safely have lockfiles on the stack.)

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 refs.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 8c50b8b139..c006004bcd 100644
--- a/refs.c
+++ b/refs.c
@@ -689,20 +689,23 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,
 
 static int delete_pseudoref(const char *pseudoref, const struct object_id *old_oid)
 {
-	static struct lock_file lock;
 	const char *filename;
 
 	filename = git_path("%s", pseudoref);
 
 	if (old_oid && !is_null_oid(old_oid)) {
+		struct lock_file lock = LOCK_INIT;
 		int fd;
 		struct object_id actual_old_oid;
 
 		fd = hold_lock_file_for_update_timeout(
-				&lock, filename, LOCK_DIE_ON_ERROR,
+				&lock, filename, 0,
 				get_files_ref_lock_timeout_ms());
-		if (fd < 0)
-			die_errno(_("Could not open '%s' for writing"), filename);
+		if (fd < 0) {
+			error_errno(_("could not open '%s' for writing"),
+				    filename);
+			return -1;
+		}
 		if (read_ref(pseudoref, &actual_old_oid))
 			die("could not read ref '%s'", pseudoref);
 		if (oidcmp(&actual_old_oid, old_oid)) {
-- 
2.17.0.411.g9fd64c8e46

[PATCH v2 4/5] lock_file: make function-local locks non-static

[Martin Ågren]

Placing `struct lock_file`s on the stack used to be a bad idea, because
the temp- and lockfile-machinery would keep a pointer into the struct.
But after 076aa2cbd (tempfile: auto-allocate tempfiles on heap,
2017-09-05), we can safely have lockfiles on the stack. (This applies
even if a user returns early, leaving a locked lock behind.)

These `struct lock_file`s are local to their respective functions and we
can drop their staticness.

For good measure, I have inspected these sites and come to believe that
they always release the lock, with the possible exception of bailing out
using `die()` or `exit()` or by returning from a `cmd_foo()`.

As pointed out by Jeff King, it would be bad if someone held on to a
`struct lock_file *` for some reason. After some grepping, I agree with
his findings: no-one appears to be doing that.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 apply.c                | 2 +-
 builtin/describe.c     | 2 +-
 builtin/difftool.c     | 2 +-
 builtin/gc.c           | 2 +-
 builtin/merge.c        | 4 ++--
 builtin/receive-pack.c | 2 +-
 bundle.c               | 2 +-
 fast-import.c          | 2 +-
 refs/files-backend.c   | 2 +-
 shallow.c              | 2 +-
 10 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/apply.c b/apply.c
index 7e5792c996..07b14d1127 100644
--- a/apply.c
+++ b/apply.c
@@ -4058,7 +4058,7 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list)
 {
 	struct patch *patch;
 	struct index_state result = { NULL };
-	static struct lock_file lock;
+	struct lock_file lock = LOCK_INIT;
 	int res;
 
 	/* Once we start supporting the reverse patch, it may be
diff --git a/builtin/describe.c b/builtin/describe.c
index b5afc45846..8bd95cf371 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -612,7 +612,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 				suffix = broken;
 			}
 		} else if (dirty) {
-			static struct lock_file index_lock;
+			struct lock_file index_lock = LOCK_INIT;
 			struct rev_info revs;
 			struct argv_array args = ARGV_ARRAY_INIT;
 			int fd, result;
diff --git a/builtin/difftool.c b/builtin/difftool.c
index aad0e073ee..162806f238 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -610,7 +610,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 			continue;
 
 		if (!indices_loaded) {
-			static struct lock_file lock;
+			struct lock_file lock = LOCK_INIT;
 			strbuf_reset(&buf);
 			strbuf_addf(&buf, "%s/wtindex", tmpdir);
 			if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||
diff --git a/builtin/gc.c b/builtin/gc.c
index 3e67124eaa..274660d9dc 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -234,7 +234,7 @@ static int need_to_gc(void)
 /* return NULL on success, else hostname running the gc */
 static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 {
-	static struct lock_file lock;
+	struct lock_file lock = LOCK_INIT;
 	char my_host[HOST_NAME_MAX + 1];
 	struct strbuf sb = STRBUF_INIT;
 	struct stat st;
diff --git a/builtin/merge.c b/builtin/merge.c
index 9db5a2cf16..de62b2c5c6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -647,7 +647,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 			      struct commit_list *remoteheads,
 			      struct commit *head)
 {
-	static struct lock_file lock;
+	struct lock_file lock = LOCK_INIT;
 	const char *head_arg = "HEAD";
 
 	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
@@ -805,7 +805,7 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 {
 	struct object_id result_tree, result_commit;
 	struct commit_list *parents, **pptr = &parents;
-	static struct lock_file lock;
+	struct lock_file lock = LOCK_INIT;
 
 	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
 	refresh_cache(REFRESH_QUIET);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 4b68a28e92..d3cf36cef5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -876,7 +876,7 @@ static void refuse_unconfigured_deny_delete_current(void)
 static int command_singleton_iterator(void *cb_data, struct object_id *oid);
 static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 {
-	static struct lock_file shallow_lock;
+	struct lock_file shallow_lock = LOCK_INIT;
 	struct oid_array extra = OID_ARRAY_INIT;
 	struct check_connected_options opt = CHECK_CONNECTED_INIT;
 	uint32_t mask = 1 << (cmd->index % 32);
diff --git a/bundle.c b/bundle.c
index 902c9b5448..160bbfdc64 100644
--- a/bundle.c
+++ b/bundle.c
@@ -409,7 +409,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
 int create_bundle(struct bundle_header *header, const char *path,
 		  int argc, const char **argv)
 {
-	static struct lock_file lock;
+	struct lock_file lock = LOCK_INIT;
 	int bundle_fd = -1;
 	int bundle_to_stdout;
 	int ref_count = 0;
diff --git a/fast-import.c b/fast-import.c
index 05d1079d23..09443c1a9d 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1817,7 +1817,7 @@ static void dump_marks_helper(FILE *f,
 
 static void dump_marks(void)
 {
-	static struct lock_file mark_lock;
+	struct lock_file mark_lock = LOCK_INIT;
 	FILE *f;
 
 	if (!export_marks_file || (import_marks_file && !import_marks_file_done))
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a92a2aa821..7b2da7b8c9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2995,7 +2995,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 {
 	struct files_ref_store *refs =
 		files_downcast(ref_store, REF_STORE_WRITE, "reflog_expire");
-	static struct lock_file reflog_lock;
+	struct lock_file reflog_lock = LOCK_INIT;
 	struct expire_reflog_cb cb;
 	struct ref_lock *lock;
 	struct strbuf log_file_sb = STRBUF_INIT;
diff --git a/shallow.c b/shallow.c
index df4d44ea7a..85313619ea 100644
--- a/shallow.c
+++ b/shallow.c
@@ -353,7 +353,7 @@ void advertise_shallow_grafts(int fd)
  */
 void prune_shallow(int show_only)
 {
-	static struct lock_file shallow_lock;
+	struct lock_file shallow_lock = LOCK_INIT;
 	struct strbuf sb = STRBUF_INIT;
 	int fd;
 
-- 
2.17.0.411.g9fd64c8e46

[PATCH v2 5/5] lock_file: move static locks into functions

[Martin Ågren]

Placing `struct lock_file`s on the stack used to be a bad idea, because
the temp- and lockfile-machinery would keep a pointer into the struct.
But after 076aa2cbd (tempfile: auto-allocate tempfiles on heap,
2017-09-05), we can safely have lockfiles on the stack. (This applies
even if a user returns early, leaving a locked lock behind.)

Each of these `struct lock_file`s is used from within a single function.
Move them into the respective functions to make the scope clearer and
drop the staticness.

For good measure, I have inspected these sites and come to believe that
they always release the lock, with the possible exception of bailing out
using `die()` or `exit()` or by returning from a `cmd_foo()`.

As pointed out by Jeff King, it would be bad if someone held on to a
`struct lock_file *` for some reason. After some grepping, I agree with
his findings: no-one appears to be doing that.

After this commit, the remaining occurrences of "static struct
lock_file" are locks that are used from within different functions. That
is, they need to remain static. (Short of more intrusive changes like
passing around pointers to non-static locks.)

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/helper/test-scrap-cache-tree.c | 4 ++--
 builtin/add.c                    | 3 +--
 builtin/mv.c                     | 2 +-
 builtin/read-tree.c              | 3 +--
 builtin/rm.c                     | 3 +--
 rerere.c                         | 3 +--
 6 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c
index d26d3e7c8b..393f1604ff 100644
--- a/t/helper/test-scrap-cache-tree.c
+++ b/t/helper/test-scrap-cache-tree.c
@@ -4,10 +4,10 @@
 #include "tree.h"
 #include "cache-tree.h"
 
-static struct lock_file index_lock;
-
 int cmd__scrap_cache_tree(int ac, const char **av)
 {
+	struct lock_file index_lock = LOCK_INIT;
+
 	setup_git_directory();
 	hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
 	if (read_cache() < 0)
diff --git a/builtin/add.c b/builtin/add.c
index c9e2619a9a..8a155dd41e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -265,8 +265,6 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
-static struct lock_file lock_file;
-
 static const char ignore_error[] =
 N_("The following paths are ignored by one of your .gitignore files:\n");
 
@@ -393,6 +391,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	int add_new_files;
 	int require_pathspec;
 	char *seen = NULL;
+	struct lock_file lock_file = LOCK_INIT;
 
 	git_config(add_config, NULL);
 
diff --git a/builtin/mv.c b/builtin/mv.c
index 6d141f7a53..b4692409e3 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -72,7 +72,6 @@ static const char *add_slash(const char *path)
 	return path;
 }
 
-static struct lock_file lock_file;
 #define SUBMODULE_WITH_GITDIR ((const char *)1)
 
 static void prepare_move_submodule(const char *src, int first,
@@ -131,6 +130,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
+	struct lock_file lock_file = LOCK_INIT;
 
 	git_config(git_default_config, NULL);
 
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index bf87a2710b..ebc43eb805 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -107,8 +107,6 @@ static int git_read_tree_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
-static struct lock_file lock_file;
-
 int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 {
 	int i, stage = 0;
@@ -116,6 +114,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 	struct tree_desc t[MAX_UNPACK_TREES];
 	struct unpack_trees_options opts;
 	int prefix_set = 0;
+	struct lock_file lock_file = LOCK_INIT;
 	const struct option read_tree_options[] = {
 		{ OPTION_CALLBACK, 0, "index-output", NULL, N_("file"),
 		  N_("write resulting index to <file>"),
diff --git a/builtin/rm.c b/builtin/rm.c
index 5b6fc7ee81..65b448ef8e 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -233,8 +233,6 @@ static int check_local_mod(struct object_id *head, int index_only)
 	return errs;
 }
 
-static struct lock_file lock_file;
-
 static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;
 static int ignore_unmatch = 0;
 
@@ -251,6 +249,7 @@ static struct option builtin_rm_options[] = {
 
 int cmd_rm(int argc, const char **argv, const char *prefix)
 {
+	struct lock_file lock_file = LOCK_INIT;
 	int i;
 	struct pathspec pathspec;
 	char *seen;
diff --git a/rerere.c b/rerere.c
index 18cae2d11c..e0862e2778 100644
--- a/rerere.c
+++ b/rerere.c
@@ -703,10 +703,9 @@ static int merge(const struct rerere_id *id, const char *path)
 	return ret;
 }
 
-static struct lock_file index_lock;
-
 static void update_paths(struct string_list *update)
 {
+	struct lock_file index_lock = LOCK_INIT;
 	int i;
 
 	hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
-- 
2.17.0.411.g9fd64c8e46

Re: [PATCH v2 0/5] getting rid of most "static struct lock_file"s

[Jeff King]

On Wed, May 09, 2018 at 10:55:34PM +0200, Martin Ågren wrote:

> This is take two of my attempt at making almost all `struct lock_file`s
> non-static. All patches have been equipped with more detailed commit
> messages. The only diff that has changed is patch 3/5, where I now take
> a small step towards gentle error-handling, rather than going in the
> opposite direction.
> 
> Thanks all for the valuable feedback on v1. I could have saved everyone
> some trouble by writing better commit messages from the start, and
> probably also by using `--thread` when formatting the patches...

Indeed, the world would be a more efficient place if we all did
everything perfectly all the time. Sadly, that's not how it works. :)

This version looks good to me. Thanks for modernizing things.

I don't think it's worth re-rolling, but one thing to think about for
future cleanups: you split the patches by touched area, not by
functionality. So the first three patches have a "while we're here..."
that has to explain why dropping the "static" is the right thing over
and over. If you instead did the error-handling fixes independently
first, then you could lump the "static" cleanups together with one
explanation (possibly even just as part of the 4th patch).

-Peff

Re: [PATCH v2 0/5] getting rid of most "static struct lock_file"s

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't think it's worth re-rolling, but one thing to think about for
> future cleanups: you split the patches by touched area, not by
> functionality. So the first three patches have a "while we're here..."
> that has to explain why dropping the "static" is the right thing over
> and over. If you instead did the error-handling fixes independently
> first, then you could lump the "static" cleanups together with one
> explanation (possibly even just as part of the 4th patch).

Thanks Peff for a good pice of advice.  I agree with the assessment
after reading the series through (includng "not worth rerolling"
part).

Thanks, Martin.

Re: [PATCH v2 0/5] getting rid of most "static struct lock_file"s

[Martin Ågren]

On 10 May 2018 at 08:01, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> I don't think it's worth re-rolling, but one thing to think about for
>> future cleanups: you split the patches by touched area, not by
>> functionality. So the first three patches have a "while we're here..."
>> that has to explain why dropping the "static" is the right thing over
>> and over. If you instead did the error-handling fixes independently
>> first, then you could lump the "static" cleanups together with one
>> explanation (possibly even just as part of the 4th patch).
>
> Thanks Peff for a good pice of advice.  I agree with the assessment
> after reading the series through (includng "not worth rerolling"
> part).

Right. In the first version, the while-at-its were really while-at-its
-- and it turned out it needed some motivation. So, in the reroll, I
focused on expanding the commit messages. Any benefit from making
patches four and five somewhat smaller definitely got lost in the blown
up first three commit messages. Thanks for pointing it out.

Martin