[PATCH v4 00/32] Lockfile correctness and refactoring

[PATCH v4 00/32] Lockfile correctness and refactoring

[Michael Haggerty]

Sorry for the long delay since v3. This version mostly cleans up a
couple more places where the lockfile object was left in an
ill-defined state. Thanks to Johannes Sixt and Torsten Bögershausen
for their review of v3.

I believe that this series addresses all of the comments from v1 [1],
v2 [2], and v3 [3].

This series applies to the current "master". There is a trivial
conflict between these changes and "next", and a few not-too-serious
conflicts between these changes and Ronnie's reference-related series
in "pu". I've figured out how to resolve the conflicts locally. Is
there some form in which I can put the conflict resolution that would
help you?

Changes since v3:

* Rebase to the current master, including adjusting the patch series
  for 93dcaea2 (addition of reopen_lock_file()).

* Perform the internal consistency check right away in
  commit_lock_file(), rather than after possibly having closed the
  file.

* Improve the explanation of the rationale for marking lock_file
  fields volatile.

* Fix comments that still referred to lock_file::filename[0] even
  though it is now a strbuf.

* Change rollback_lock_file() to exit early if the lock is not active
  (rather than nesting the rest of the function in an "if" statement).

* Fix Johannes's email address in the trailers.

* Extract a function commit_lock_file_to(lk, filename) and delegate to
  it from commit_lock_file() and commit_locked_index() so that the
  latter gets the benefit of the improvements in this patch series.

[1] http://thread.gmane.org/gmane.comp.version-control.git/245609
[2] http://thread.gmane.org/gmane.comp.version-control.git/245801
[3] http://thread.gmane.org/gmane.comp.version-control.git/246222

Michael Haggerty (32):
  unable_to_lock_die(): rename function from unable_to_lock_index_die()
  api-lockfile: expand the documentation
  rollback_lock_file(): do not clear filename redundantly
  rollback_lock_file(): exit early if lock is not active
  rollback_lock_file(): set fd to -1
  lockfile: unlock file if lockfile permissions cannot be adjusted
  hold_lock_file_for_append(): release lock on errors
  lock_file(): always add lock_file object to lock_file_list
  lockfile.c: document the various states of lock_file objects
  cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN
  delete_ref_loose(): don't muck around in the lock_file's filename
  prepare_index(): declare return value to be (const char *)
  write_packed_entry_fn(): convert cb_data into a (const int *)
  lock_file(): exit early if lockfile cannot be opened
  remove_lock_file(): call rollback_lock_file()
  commit_lock_file(): inline temporary variable
  commit_lock_file(): die() if called for unlocked lockfile object
  commit_lock_file(): if close fails, roll back
  commit_lock_file(): rollback lock file on failure to rename
  api-lockfile: document edge cases
  dump_marks(): remove a redundant call to rollback_lock_file()
  git_config_set_multivar_in_file(): avoid call to rollback_lock_file()
  lockfile: avoid transitory invalid states
  struct lock_file: declare some fields volatile
  try_merge_strategy(): remove redundant lock_file allocation
  try_merge_strategy(): use a statically-allocated lock_file object
  commit_lock_file(): use a strbuf to manage temporary space
  Change lock_file::filename into a strbuf
  resolve_symlink(): use a strbuf for internal scratch space
  resolve_symlink(): take a strbuf parameter
  trim_last_path_elm(): replace last_path_elm()
  Extract a function commit_lock_file_to()

 Documentation/technical/api-lockfile.txt |  67 +++++--
 builtin/commit.c                         |  16 +-
 builtin/merge.c                          |  15 +-
 builtin/reflog.c                         |   2 +-
 builtin/update-index.c                   |   2 +-
 cache.h                                  |  16 +-
 config.c                                 |  28 +--
 fast-import.c                            |   4 +-
 lockfile.c                               | 299 ++++++++++++++++++-------------
 read-cache.c                             |  12 +-
 refs.c                                   |  29 +--
 shallow.c                                |   6 +-
 12 files changed, 296 insertions(+), 200 deletions(-)

-- 
2.1.0

[PATCH v4 01/32] unable_to_lock_die(): rename function from unable_to_lock_index_die()

[Michael Haggerty]

This function is used for other things besides the index, so rename it
accordingly.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/update-index.c | 2 +-
 cache.h                | 2 +-
 lockfile.c             | 6 +++---
 refs.c                 | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index e8c7fd4..6c95988 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -942,7 +942,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		if (newfd < 0) {
 			if (refresh_args.flags & REFRESH_QUIET)
 				exit(128);
-			unable_to_lock_index_die(get_index_file(), lock_error);
+			unable_to_lock_die(get_index_file(), lock_error);
 		}
 		if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
 			die("Unable to write new index file");
diff --git a/cache.h b/cache.h
index 4d5b76c..da77094 100644
--- a/cache.h
+++ b/cache.h
@@ -581,7 +581,7 @@ struct lock_file {
 extern int unable_to_lock_error(const char *path, int err);
 extern void unable_to_lock_message(const char *path, int err,
 				   struct strbuf *buf);
-extern NORETURN void unable_to_lock_index_die(const char *path, int err);
+extern NORETURN void unable_to_lock_die(const char *path, int err);
 extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
 extern int commit_lock_file(struct lock_file *);
diff --git a/lockfile.c b/lockfile.c
index 2a800ce..f1ce154 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -185,7 +185,7 @@ int unable_to_lock_error(const char *path, int err)
 	return -1;
 }
 
-NORETURN void unable_to_lock_index_die(const char *path, int err)
+NORETURN void unable_to_lock_die(const char *path, int err)
 {
 	struct strbuf buf = STRBUF_INIT;
 
@@ -198,7 +198,7 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
 {
 	int fd = lock_file(lk, path, flags);
 	if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
-		unable_to_lock_index_die(path, errno);
+		unable_to_lock_die(path, errno);
 	return fd;
 }
 
@@ -209,7 +209,7 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
 	fd = lock_file(lk, path, flags);
 	if (fd < 0) {
 		if (flags & LOCK_DIE_ON_ERROR)
-			unable_to_lock_index_die(path, errno);
+			unable_to_lock_die(path, errno);
 		return fd;
 	}
 
diff --git a/refs.c b/refs.c
index 27927f2..5ae8e69 100644
--- a/refs.c
+++ b/refs.c
@@ -2159,7 +2159,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			 */
 			goto retry;
 		else
-			unable_to_lock_index_die(ref_file, errno);
+			unable_to_lock_die(ref_file, errno);
 	}
 	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
 
-- 
2.1.0

[PATCH v4 02/32] api-lockfile: expand the documentation

[Michael Haggerty]

Document a couple more functions and the flags argument as used by
hold_lock_file_for_update() and hold_lock_file_for_append().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/technical/api-lockfile.txt | 36 +++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index dd89404..b53e300 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -28,9 +28,39 @@ hold_lock_file_for_update::
 	the final destination (e.g. `$GIT_DIR/index`) and a flag
 	`die_on_error`.  Attempt to create a lockfile for the
 	destination and return the file descriptor for writing
-	to the file.  If `die_on_error` flag is true, it dies if
-	a lock is already taken for the file; otherwise it
-	returns a negative integer to the caller on failure.
+	to the file.  The flags parameter is a combination of
++
+--
+LOCK_NODEREF::
+
+	Usually symbolic links in path are resolved in path and the
+	lockfile is created by adding ".lock" to the resolved path;
+	however, if `LOCK_NODEREF` is set, then the lockfile is
+	created by adding ".lock" to the path argument itself.
+
+LOCK_DIE_ON_ERROR::
+
+	If a lock is already taken for the file, `die()` with an error
+	message.  If this option is not specified, return a negative
+	integer to the caller on failure.
+--
+
+hold_lock_file_for_append::
+
+	Like `hold_lock_file_for_update()`, except that additionally
+	the existing contents of the file (if any) are copied to the
+	lockfile and its write pointer is positioned at the end of the
+	file before returning.
+
+unable_to_lock_error::
+
+	Emit an error describing that there was an error locking the
+	specified path.  The err parameter should be the errno of the
+	problem that caused the failure.
+
+unable_to_lock_die::
+
+	Like `unable_to_lock_error()`, but also `die()`.
 
 commit_lock_file::
 
-- 
2.1.0

Re: [PATCH v4 02/32] api-lockfile: expand the documentation

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> +LOCK_NODEREF::

I think I've seen this mentioned already but LOCK_NO_DEREF to avoid
"lock the node ref?" misreading?

[PATCH v4 03/32] rollback_lock_file(): do not clear filename redundantly

[Michael Haggerty]

It is only necessary to clear the lock_file's filename field if it was
not already clear.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lockfile.c b/lockfile.c
index f1ce154..a548e08 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -276,6 +276,6 @@ void rollback_lock_file(struct lock_file *lk)
 		if (lk->fd >= 0)
 			close(lk->fd);
 		unlink_or_warn(lk->filename);
+		lk->filename[0] = 0;
 	}
-	lk->filename[0] = 0;
 }
-- 
2.1.0

Re: [PATCH v4 03/32] rollback_lock_file(): do not clear filename redundantly

[Ronnie Sahlberg]

Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>

On Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> It is only necessary to clear the lock_file's filename field if it was
> not already clear.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  lockfile.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lockfile.c b/lockfile.c
> index f1ce154..a548e08 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -276,6 +276,6 @@ void rollback_lock_file(struct lock_file *lk)
>                 if (lk->fd >= 0)
>                         close(lk->fd);
>                 unlink_or_warn(lk->filename);
> +               lk->filename[0] = 0;
>         }
> -       lk->filename[0] = 0;
>  }
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 04/32] rollback_lock_file(): exit early if lock is not active

[Michael Haggerty]

Eliminate a layer of nesting.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index a548e08..49179d8 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -272,10 +272,11 @@ int hold_locked_index(struct lock_file *lk, int die_on_error)
 
 void rollback_lock_file(struct lock_file *lk)
 {
-	if (lk->filename[0]) {
-		if (lk->fd >= 0)
-			close(lk->fd);
-		unlink_or_warn(lk->filename);
-		lk->filename[0] = 0;
-	}
+	if (!lk->filename[0])
+		return;
+
+	if (lk->fd >= 0)
+		close(lk->fd);
+	unlink_or_warn(lk->filename);
+	lk->filename[0] = 0;
 }
-- 
2.1.0

Re: [PATCH v4 04/32] rollback_lock_file(): exit early if lock is not active

[Ronnie Sahlberg]

Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>

On Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Eliminate a layer of nesting.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  lockfile.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/lockfile.c b/lockfile.c
> index a548e08..49179d8 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -272,10 +272,11 @@ int hold_locked_index(struct lock_file *lk, int die_on_error)
>
>  void rollback_lock_file(struct lock_file *lk)
>  {
> -       if (lk->filename[0]) {
> -               if (lk->fd >= 0)
> -                       close(lk->fd);
> -               unlink_or_warn(lk->filename);
> -               lk->filename[0] = 0;
> -       }
> +       if (!lk->filename[0])
> +               return;
> +
> +       if (lk->fd >= 0)
> +               close(lk->fd);
> +       unlink_or_warn(lk->filename);
> +       lk->filename[0] = 0;
>  }
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 05/32] rollback_lock_file(): set fd to -1

[Michael Haggerty]

When rolling back the lockfile, call close_lock_file() so that the
lock_file's fd field gets set back to -1.  This keeps the lock_file
object in a valid state, which is important because these objects are
allowed to be reused.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lockfile.c b/lockfile.c
index 49179d8..b1c4ba0 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -276,7 +276,7 @@ void rollback_lock_file(struct lock_file *lk)
 		return;
 
 	if (lk->fd >= 0)
-		close(lk->fd);
+		close_lock_file(lk);
 	unlink_or_warn(lk->filename);
 	lk->filename[0] = 0;
 }
-- 
2.1.0

[PATCH v4 06/32] lockfile: unlock file if lockfile permissions cannot be adjusted

[Michael Haggerty]

If the call to adjust_shared_perm() fails, lock_file returns -1, which
to the caller looks like any other failure to lock the file.  So in
this case, roll back the lockfile before returning so that the lock
file is deleted immediately and the lockfile object is left in a
predictable state (namely, unlocked).  Previously, the lockfile was
retained until process cleanup in this situation.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lockfile.c b/lockfile.c
index b1c4ba0..d4f165d 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -153,6 +153,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 			int save_errno = errno;
 			error("cannot fix permission bits on %s",
 			      lk->filename);
+			rollback_lock_file(lk);
 			errno = save_errno;
 			return -1;
 		}
-- 
2.1.0

Re: [PATCH v4 06/32] lockfile: unlock file if lockfile permissions cannot be adjusted

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> If the call to adjust_shared_perm() fails, lock_file returns -1, which
> to the caller looks like any other failure to lock the file.  So in
> this case, roll back the lockfile before returning so that the lock
> file is deleted immediately and the lockfile object is left in a
> predictable state (namely, unlocked).  Previously, the lockfile was
> retained until process cleanup in this situation.

... which would mean that other processes can grab a lock on the
same file a bit earlier. Is there any negative implication caused by
that difference?  I do not think of any but I could be missing
something.

>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  lockfile.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lockfile.c b/lockfile.c
> index b1c4ba0..d4f165d 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -153,6 +153,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>  			int save_errno = errno;
>  			error("cannot fix permission bits on %s",
>  			      lk->filename);
> +			rollback_lock_file(lk);
>  			errno = save_errno;
>  			return -1;
>  		}

Re: [PATCH v4 06/32] lockfile: unlock file if lockfile permissions cannot be adjusted

[Michael Haggerty]

On 09/10/2014 12:39 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> If the call to adjust_shared_perm() fails, lock_file returns -1, which
>> to the caller looks like any other failure to lock the file.  So in
>> this case, roll back the lockfile before returning so that the lock
>> file is deleted immediately and the lockfile object is left in a
>> predictable state (namely, unlocked).  Previously, the lockfile was
>> retained until process cleanup in this situation.
> 
> ... which would mean that other processes can grab a lock on the
> same file a bit earlier. Is there any negative implication caused by
> that difference?  I do not think of any but I could be missing
> something.

I think the end effect would be the same as if another process had
grabbed the lock a nanosecond before this process tried to do so. So
assuming that callers handle that situation correctly, I don't think
this change can cause any problems.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH v4 07/32] hold_lock_file_for_append(): release lock on errors

[Michael Haggerty]

If there is an error copying the old contents to the lockfile, roll
back the lockfile before exiting so that the lockfile is not held
until process cleanup.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index d4f165d..983c3ec 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -219,13 +219,13 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
 		if (errno != ENOENT) {
 			if (flags & LOCK_DIE_ON_ERROR)
 				die("cannot open '%s' for copying", path);
-			close(fd);
+			rollback_lock_file(lk);
 			return error("cannot open '%s' for copying", path);
 		}
 	} else if (copy_fd(orig_fd, fd)) {
 		if (flags & LOCK_DIE_ON_ERROR)
 			exit(128);
-		close(fd);
+		rollback_lock_file(lk);
 		return -1;
 	}
 	return fd;
-- 
2.1.0

Re: [PATCH v4 07/32] hold_lock_file_for_append(): release lock on errors

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> If there is an error copying the old contents to the lockfile, roll
> back the lockfile before exiting so that the lockfile is not held
> until process cleanup.

Same comment as 06/32 applies here.

Re: [PATCH v4 07/32] hold_lock_file_for_append(): release lock on errors

[Michael Haggerty]

On 09/10/2014 12:41 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> If there is an error copying the old contents to the lockfile, roll
>> back the lockfile before exiting so that the lockfile is not held
>> until process cleanup.
> 
> Same comment as 06/32 applies here.

Same reply applies here, too.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH v4 08/32] lock_file(): always add lock_file object to lock_file_list

[Michael Haggerty]

It used to be that if locking failed, lock_file() usually did not
register the lock_file object in lock_file_list but sometimes it did.
This confusion was compounded if lock_file() was called via
hold_lock_file_for_append(), which has its own failure modes.

The ambiguity didn't have any ill effects, because lock_file objects
cannot be removed from the lock_file_list anyway.  But it is
unnecessary to leave this behavior inconsistent.

So change lock_file() to *always* ensure that the lock_file object is
registered in lock_file_list regardless of whether an error occurs.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 983c3ec..00c972c 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -129,6 +129,22 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	 */
 	static const size_t max_path_len = sizeof(lk->filename) - 5;
 
+	if (!lock_file_list) {
+		/* One-time initialization */
+		sigchain_push_common(remove_lock_file_on_signal);
+		atexit(remove_lock_file);
+	}
+
+	if (!lk->on_list) {
+		/* Initialize *lk and add it to lock_file_list: */
+		lk->fd = -1;
+		lk->owner = 0;
+		lk->on_list = 1;
+		lk->filename[0] = 0;
+		lk->next = lock_file_list;
+		lock_file_list = lk;
+	}
+
 	if (strlen(path) >= max_path_len) {
 		errno = ENAMETOOLONG;
 		return -1;
@@ -139,16 +155,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	strcat(lk->filename, ".lock");
 	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= lk->fd) {
-		if (!lock_file_list) {
-			sigchain_push_common(remove_lock_file_on_signal);
-			atexit(remove_lock_file);
-		}
 		lk->owner = getpid();
-		if (!lk->on_list) {
-			lk->next = lock_file_list;
-			lock_file_list = lk;
-			lk->on_list = 1;
-		}
 		if (adjust_shared_perm(lk->filename)) {
 			int save_errno = errno;
 			error("cannot fix permission bits on %s",
-- 
2.1.0

[PATCH v4 09/32] lockfile.c: document the various states of lock_file objects

[Michael Haggerty]

Document the valid states of lock_file objects, how they get into each
state, and how the state is encoded in the object's fields.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/lockfile.c b/lockfile.c
index 00c972c..964b3fc 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -4,6 +4,58 @@
 #include "cache.h"
 #include "sigchain.h"
 
+/*
+ * File write-locks as used by Git.
+ *
+ * When a file at $FILENAME needs to be written, it is done as
+ * follows:
+ *
+ * 1. Obtain a lock on the file by creating a lockfile at path
+ *    $FILENAME.lock.  The file is opened for read/write using O_CREAT
+ *    and O_EXCL mode to ensure that it doesn't already exist.  Such a
+ *    lock file is respected by writers *but not by readers*.
+ *
+ *    Usually, if $FILENAME is a symlink, then it is resolved, and the
+ *    file ultimately pointed to is the one that is locked and later
+ *    replaced.  However, if LOCK_NODEREF is used, then $FILENAME
+ *    itself is locked and later replaced, even if it is a symlink.
+ *
+ * 2. Write the new file contents to the lockfile.
+ *
+ * 3. Move the lockfile to its final destination using rename(2).
+ *
+ * Instead of (3), the change can be rolled back by deleting lockfile.
+ *
+ * This module keeps track of all locked files in lock_file_list.
+ * When the first file is locked, it registers an atexit(3) handler;
+ * when the program exits, the handler rolls back any files that have
+ * been locked but were never committed or rolled back.
+ *
+ * A lock_file object can be in several states:
+ *
+ * - Uninitialized.  In this state the object's on_list field must be
+ *   zero but the rest of its contents need not be initialized.  As
+ *   soon as the object is used in any way, it is irrevocably
+ *   registered in the lock_file_list, and on_list is set.
+ *
+ * - Locked, lockfile open (after hold_lock_file_for_update(),
+ *   hold_lock_file_for_append(), or reopen_lock_file()). In this
+ *   state, the lockfile exists, filename holds the filename of the
+ *   lockfile, fd holds a file descriptor open for writing to the
+ *   lockfile, and owner holds the PID of the process that locked the
+ *   file.
+ *
+ * - Locked, lockfile closed (after close_lock_file()).  Same as the
+ *   previous state, except that the lockfile is closed and fd is -1.
+ *
+ * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a
+ *   failed attempt to lock).  In this state, filename[0] == '\0' and
+ *   fd is -1.  The object is left registered in the lock_file_list,
+ *   and on_list is set.
+ *
+ * See Documentation/api-lockfile.txt for more information.
+ */
+
 static struct lock_file *lock_file_list;
 
 static void remove_lock_file(void)
-- 
2.1.0

Re: [PATCH v4 09/32] lockfile.c: document the various states of lock_file objects

[Ronnie Sahlberg]

Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>

On Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Document the valid states of lock_file objects, how they get into each
> state, and how the state is encoded in the object's fields.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  lockfile.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/lockfile.c b/lockfile.c
> index 00c972c..964b3fc 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -4,6 +4,58 @@
>  #include "cache.h"
>  #include "sigchain.h"
>
> +/*
> + * File write-locks as used by Git.
> + *
> + * When a file at $FILENAME needs to be written, it is done as
> + * follows:
> + *
> + * 1. Obtain a lock on the file by creating a lockfile at path
> + *    $FILENAME.lock.  The file is opened for read/write using O_CREAT
> + *    and O_EXCL mode to ensure that it doesn't already exist.  Such a
> + *    lock file is respected by writers *but not by readers*.
> + *
> + *    Usually, if $FILENAME is a symlink, then it is resolved, and the
> + *    file ultimately pointed to is the one that is locked and later
> + *    replaced.  However, if LOCK_NODEREF is used, then $FILENAME
> + *    itself is locked and later replaced, even if it is a symlink.
> + *
> + * 2. Write the new file contents to the lockfile.
> + *
> + * 3. Move the lockfile to its final destination using rename(2).
> + *
> + * Instead of (3), the change can be rolled back by deleting lockfile.
> + *
> + * This module keeps track of all locked files in lock_file_list.
> + * When the first file is locked, it registers an atexit(3) handler;
> + * when the program exits, the handler rolls back any files that have
> + * been locked but were never committed or rolled back.
> + *
> + * A lock_file object can be in several states:
> + *
> + * - Uninitialized.  In this state the object's on_list field must be
> + *   zero but the rest of its contents need not be initialized.  As
> + *   soon as the object is used in any way, it is irrevocably
> + *   registered in the lock_file_list, and on_list is set.
> + *
> + * - Locked, lockfile open (after hold_lock_file_for_update(),
> + *   hold_lock_file_for_append(), or reopen_lock_file()). In this
> + *   state, the lockfile exists, filename holds the filename of the
> + *   lockfile, fd holds a file descriptor open for writing to the
> + *   lockfile, and owner holds the PID of the process that locked the
> + *   file.
> + *
> + * - Locked, lockfile closed (after close_lock_file()).  Same as the
> + *   previous state, except that the lockfile is closed and fd is -1.
> + *
> + * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a
> + *   failed attempt to lock).  In this state, filename[0] == '\0' and
> + *   fd is -1.  The object is left registered in the lock_file_list,
> + *   and on_list is set.
> + *
> + * See Documentation/api-lockfile.txt for more information.
> + */
> +
>  static struct lock_file *lock_file_list;
>
>  static void remove_lock_file(void)
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN

[Michael Haggerty]

There are a few places that use these values, so define constants for
them.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 cache.h    |  4 ++++
 lockfile.c | 11 ++++++-----
 refs.c     |  7 ++++---
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/cache.h b/cache.h
index da77094..41d829b 100644
--- a/cache.h
+++ b/cache.h
@@ -569,6 +569,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);
 #define REFRESH_IN_PORCELAIN	0x0020	/* user friendly output, not "needs update" */
 extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);
 
+/* String appended to a filename to derive the lockfile name: */
+#define LOCK_SUFFIX ".lock"
+#define LOCK_SUFFIX_LEN 5
+
 struct lock_file {
 	struct lock_file *next;
 	int fd;
diff --git a/lockfile.c b/lockfile.c
index 964b3fc..bfea333 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -176,10 +176,11 @@ static char *resolve_symlink(char *p, size_t s)
 static int lock_file(struct lock_file *lk, const char *path, int flags)
 {
 	/*
-	 * subtract 5 from size to make sure there's room for adding
-	 * ".lock" for the lock file name
+	 * subtract LOCK_SUFFIX_LEN from size to make sure there's
+	 * room for adding ".lock" for the lock file name:
 	 */
-	static const size_t max_path_len = sizeof(lk->filename) - 5;
+	static const size_t max_path_len = sizeof(lk->filename) -
+					   LOCK_SUFFIX_LEN;
 
 	if (!lock_file_list) {
 		/* One-time initialization */
@@ -204,7 +205,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	strcpy(lk->filename, path);
 	if (!(flags & LOCK_NODEREF))
 		resolve_symlink(lk->filename, max_path_len);
-	strcat(lk->filename, ".lock");
+	strcat(lk->filename, LOCK_SUFFIX);
 	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= lk->fd) {
 		lk->owner = getpid();
@@ -314,7 +315,7 @@ int commit_lock_file(struct lock_file *lk)
 	if (lk->fd >= 0 && close_lock_file(lk))
 		return -1;
 	strcpy(result_file, lk->filename);
-	i = strlen(result_file) - 5; /* .lock */
+	i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */
 	result_file[i] = 0;
 	if (rename(lk->filename, result_file))
 		return -1;
diff --git a/refs.c b/refs.c
index 5ae8e69..828522d 100644
--- a/refs.c
+++ b/refs.c
@@ -74,7 +74,8 @@ out:
 		if (refname[1] == '\0')
 			return -1; /* Component equals ".". */
 	}
-	if (cp - refname >= 5 && !memcmp(cp - 5, ".lock", 5))
+	if (cp - refname >= LOCK_SUFFIX_LEN &&
+	    !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))
 		return -1; /* Refname ends with ".lock". */
 	return cp - refname;
 }
@@ -2545,11 +2546,11 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)
 {
 	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
 		/* loose */
-		int err, i = strlen(lock->lk->filename) - 5; /* .lock */
+		int err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN;
 
 		lock->lk->filename[i] = 0;
 		err = unlink_or_warn(lock->lk->filename);
-		lock->lk->filename[i] = '.';
+		lock->lk->filename[i] = LOCK_SUFFIX[0];
 		if (err && errno != ENOENT)
 			return 1;
 	}
-- 
2.1.0

Re: [PATCH v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN

[Ronnie Sahlberg]

On Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> There are a few places that use these values, so define constants for
> them.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  cache.h    |  4 ++++
>  lockfile.c | 11 ++++++-----
>  refs.c     |  7 ++++---
>  3 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index da77094..41d829b 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -569,6 +569,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);
>  #define REFRESH_IN_PORCELAIN   0x0020  /* user friendly output, not "needs update" */
>  extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);
>
> +/* String appended to a filename to derive the lockfile name: */
> +#define LOCK_SUFFIX ".lock"
> +#define LOCK_SUFFIX_LEN 5
> +
>  struct lock_file {
>         struct lock_file *next;
>         int fd;
> diff --git a/lockfile.c b/lockfile.c
> index 964b3fc..bfea333 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -176,10 +176,11 @@ static char *resolve_symlink(char *p, size_t s)
>  static int lock_file(struct lock_file *lk, const char *path, int flags)
>  {
>         /*
> -        * subtract 5 from size to make sure there's room for adding
> -        * ".lock" for the lock file name
> +        * subtract LOCK_SUFFIX_LEN from size to make sure there's
> +        * room for adding ".lock" for the lock file name:
>          */
> -       static const size_t max_path_len = sizeof(lk->filename) - 5;
> +       static const size_t max_path_len = sizeof(lk->filename) -
> +                                          LOCK_SUFFIX_LEN;
>
>         if (!lock_file_list) {
>                 /* One-time initialization */
> @@ -204,7 +205,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>         strcpy(lk->filename, path);
>         if (!(flags & LOCK_NODEREF))
>                 resolve_symlink(lk->filename, max_path_len);
> -       strcat(lk->filename, ".lock");
> +       strcat(lk->filename, LOCK_SUFFIX);
>         lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
>         if (0 <= lk->fd) {
>                 lk->owner = getpid();
> @@ -314,7 +315,7 @@ int commit_lock_file(struct lock_file *lk)
>         if (lk->fd >= 0 && close_lock_file(lk))
>                 return -1;
>         strcpy(result_file, lk->filename);
> -       i = strlen(result_file) - 5; /* .lock */
> +       i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */

Not a new bug since the previous code is broken too.
Should probably checkstrlen(result_file) >= 5 here before subtracting 5.

Otherwise, a caller that calls commit_lock_file() with an already
committed/closed  lock_file can cause writing outside the bounds of
the array on the line below.


>         result_file[i] = 0;
>         if (rename(lk->filename, result_file))
>                 return -1;
> diff --git a/refs.c b/refs.c
> index 5ae8e69..828522d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -74,7 +74,8 @@ out:
>                 if (refname[1] == '\0')
>                         return -1; /* Component equals ".". */
>         }
> -       if (cp - refname >= 5 && !memcmp(cp - 5, ".lock", 5))
> +       if (cp - refname >= LOCK_SUFFIX_LEN &&
> +           !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))
>                 return -1; /* Refname ends with ".lock". */
>         return cp - refname;
>  }
> @@ -2545,11 +2546,11 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)
>  {
>         if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
>                 /* loose */
> -               int err, i = strlen(lock->lk->filename) - 5; /* .lock */
> +               int err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN;
>
>                 lock->lk->filename[i] = 0;
>                 err = unlink_or_warn(lock->lk->filename);
> -               lock->lk->filename[i] = '.';
> +               lock->lk->filename[i] = LOCK_SUFFIX[0];
>                 if (err && errno != ENOENT)
>                         return 1;
>         }
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN

[Michael Haggerty]

On 09/12/2014 12:15 AM, Ronnie Sahlberg wrote:
> On Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> There are a few places that use these values, so define constants for
>> them.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  cache.h    |  4 ++++
>>  lockfile.c | 11 ++++++-----
>>  refs.c     |  7 ++++---
>>  3 files changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/cache.h b/cache.h
>> index da77094..41d829b 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -569,6 +569,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);
>>  #define REFRESH_IN_PORCELAIN   0x0020  /* user friendly output, not "needs update" */
>>  extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);
>>
>> +/* String appended to a filename to derive the lockfile name: */
>> +#define LOCK_SUFFIX ".lock"
>> +#define LOCK_SUFFIX_LEN 5
>> +
>>  struct lock_file {
>>         struct lock_file *next;
>>         int fd;
>> diff --git a/lockfile.c b/lockfile.c
>> index 964b3fc..bfea333 100644
>> --- a/lockfile.c
>> +++ b/lockfile.c
>> @@ -176,10 +176,11 @@ static char *resolve_symlink(char *p, size_t s)
>>  static int lock_file(struct lock_file *lk, const char *path, int flags)
>>  {
>>         /*
>> -        * subtract 5 from size to make sure there's room for adding
>> -        * ".lock" for the lock file name
>> +        * subtract LOCK_SUFFIX_LEN from size to make sure there's
>> +        * room for adding ".lock" for the lock file name:
>>          */
>> -       static const size_t max_path_len = sizeof(lk->filename) - 5;
>> +       static const size_t max_path_len = sizeof(lk->filename) -
>> +                                          LOCK_SUFFIX_LEN;
>>
>>         if (!lock_file_list) {
>>                 /* One-time initialization */
>> @@ -204,7 +205,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>>         strcpy(lk->filename, path);
>>         if (!(flags & LOCK_NODEREF))
>>                 resolve_symlink(lk->filename, max_path_len);
>> -       strcat(lk->filename, ".lock");
>> +       strcat(lk->filename, LOCK_SUFFIX);
>>         lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
>>         if (0 <= lk->fd) {
>>                 lk->owner = getpid();
>> @@ -314,7 +315,7 @@ int commit_lock_file(struct lock_file *lk)
>>         if (lk->fd >= 0 && close_lock_file(lk))
>>                 return -1;
>>         strcpy(result_file, lk->filename);
>> -       i = strlen(result_file) - 5; /* .lock */
>> +       i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */
> 
> Not a new bug since the previous code is broken too.
> Should probably checkstrlen(result_file) >= 5 here before subtracting 5.
> 
> Otherwise, a caller that calls commit_lock_file() with an already
> committed/closed  lock_file can cause writing outside the bounds of
> the array on the line below.

Good catch; thanks. I will fix this in the reroll (though probably in a
later patch).

>> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN

[Ronnie Sahlberg]

Maybe we should not have a public constant defined for the length :
+#define LOCK_SUFFIX_LEN 5

since it encourages unsafe code like :  (this was unsafe long before
your patch so not a regression)
+       i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */
        result_file[i] = 0;



What about removing LOCK_SUFFIX_LEN from the public API and introduce
a helper function something like :


/* pointer to the character where the lock suffix starts */
char *lock_suffix_ptr_safe(const char *filename)
{
    size_t len = strlen(filename);
    if (len < 5)
       die("BUG:...
    if (strcmp(filename + len - 5, LOCK_SUFFIX)
       die("BUG:...
    return filename + len - 5;
}

and use it instead?


On Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> There are a few places that use these values, so define constants for
> them.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  cache.h    |  4 ++++
>  lockfile.c | 11 ++++++-----
>  refs.c     |  7 ++++---
>  3 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index da77094..41d829b 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -569,6 +569,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);
>  #define REFRESH_IN_PORCELAIN   0x0020  /* user friendly output, not "needs update" */
>  extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);
>
> +/* String appended to a filename to derive the lockfile name: */
> +#define LOCK_SUFFIX ".lock"
> +#define LOCK_SUFFIX_LEN 5
> +
>  struct lock_file {
>         struct lock_file *next;
>         int fd;
> diff --git a/lockfile.c b/lockfile.c
> index 964b3fc..bfea333 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -176,10 +176,11 @@ static char *resolve_symlink(char *p, size_t s)
>  static int lock_file(struct lock_file *lk, const char *path, int flags)
>  {
>         /*
> -        * subtract 5 from size to make sure there's room for adding
> -        * ".lock" for the lock file name
> +        * subtract LOCK_SUFFIX_LEN from size to make sure there's
> +        * room for adding ".lock" for the lock file name:
>          */
> -       static const size_t max_path_len = sizeof(lk->filename) - 5;
> +       static const size_t max_path_len = sizeof(lk->filename) -
> +                                          LOCK_SUFFIX_LEN;
>
>         if (!lock_file_list) {
>                 /* One-time initialization */
> @@ -204,7 +205,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>         strcpy(lk->filename, path);
>         if (!(flags & LOCK_NODEREF))
>                 resolve_symlink(lk->filename, max_path_len);
> -       strcat(lk->filename, ".lock");
> +       strcat(lk->filename, LOCK_SUFFIX);
>         lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
>         if (0 <= lk->fd) {
>                 lk->owner = getpid();
> @@ -314,7 +315,7 @@ int commit_lock_file(struct lock_file *lk)
>         if (lk->fd >= 0 && close_lock_file(lk))
>                 return -1;
>         strcpy(result_file, lk->filename);
> -       i = strlen(result_file) - 5; /* .lock */
> +       i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */
>         result_file[i] = 0;
>         if (rename(lk->filename, result_file))
>                 return -1;
> diff --git a/refs.c b/refs.c
> index 5ae8e69..828522d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -74,7 +74,8 @@ out:
>                 if (refname[1] == '\0')
>                         return -1; /* Component equals ".". */
>         }
> -       if (cp - refname >= 5 && !memcmp(cp - 5, ".lock", 5))
> +       if (cp - refname >= LOCK_SUFFIX_LEN &&
> +           !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))
>                 return -1; /* Refname ends with ".lock". */
>         return cp - refname;
>  }
> @@ -2545,11 +2546,11 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)
>  {
>         if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
>                 /* loose */
> -               int err, i = strlen(lock->lk->filename) - 5; /* .lock */
> +               int err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN;
>
>                 lock->lk->filename[i] = 0;
>                 err = unlink_or_warn(lock->lk->filename);
> -               lock->lk->filename[i] = '.';
> +               lock->lk->filename[i] = LOCK_SUFFIX[0];
>                 if (err && errno != ENOENT)
>                         return 1;
>         }
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN

[Michael Haggerty]

On 09/12/2014 12:42 AM, Ronnie Sahlberg wrote:
> Maybe we should not have a public constant defined for the length :
> +#define LOCK_SUFFIX_LEN 5
> 
> since it encourages unsafe code like :  (this was unsafe long before
> your patch so not a regression)
> +       i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */
>         result_file[i] = 0;
> 
> 
> 
> What about removing LOCK_SUFFIX_LEN from the public API and introduce
> a helper function something like :
> 
> 
> /* pointer to the character where the lock suffix starts */
> char *lock_suffix_ptr_safe(const char *filename)
> {
>     size_t len = strlen(filename);
>     if (len < 5)
>        die("BUG:...
>     if (strcmp(filename + len - 5, LOCK_SUFFIX)
>        die("BUG:...
>     return filename + len - 5;
> }
> 
> and use it instead?

At the end of this patch series, LOCK_SUFFIX_LEN is only used in two
places outside of lockfile.c:

* In check_refname_component(), to ensure that no component of a
reference name ends with ".lock". This only indirectly has anything to
do with lockfiles.

* In delete_ref_loose(), to derive the name of the loose reference file
from the name of the lockfile. It immediately xmemdupz()s the part of
the filename that it needs, so it is kosher.

I will add a function get_locked_file_path() for the use of the second
caller.

I like being able to use the symbolic constant at the first caller, and
it is not dangerous. I don't think it is so important to make the
constant private, because I think somebody programming sloppily wouldn't
be deterred for long by not seeing a symbolic constant for the suffix
length. So if it's OK with you I'll leave the constant.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN

[Ronnie Sahlberg]

On Fri, Sep 12, 2014 at 10:13 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 09/12/2014 12:42 AM, Ronnie Sahlberg wrote:
>> Maybe we should not have a public constant defined for the length :
>> +#define LOCK_SUFFIX_LEN 5
>>
>> since it encourages unsafe code like :  (this was unsafe long before
>> your patch so not a regression)
>> +       i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */
>>         result_file[i] = 0;
>>
>>
>>
>> What about removing LOCK_SUFFIX_LEN from the public API and introduce
>> a helper function something like :
>>
>>
>> /* pointer to the character where the lock suffix starts */
>> char *lock_suffix_ptr_safe(const char *filename)
>> {
>>     size_t len = strlen(filename);
>>     if (len < 5)
>>        die("BUG:...
>>     if (strcmp(filename + len - 5, LOCK_SUFFIX)
>>        die("BUG:...
>>     return filename + len - 5;
>> }
>>
>> and use it instead?
>
> At the end of this patch series, LOCK_SUFFIX_LEN is only used in two
> places outside of lockfile.c:
>
> * In check_refname_component(), to ensure that no component of a
> reference name ends with ".lock". This only indirectly has anything to
> do with lockfiles.
>
> * In delete_ref_loose(), to derive the name of the loose reference file
> from the name of the lockfile. It immediately xmemdupz()s the part of
> the filename that it needs, so it is kosher.
>
> I will add a function get_locked_file_path() for the use of the second
> caller.
>
> I like being able to use the symbolic constant at the first caller, and
> it is not dangerous. I don't think it is so important to make the
> constant private, because I think somebody programming sloppily wouldn't
> be deterred for long by not seeing a symbolic constant for the suffix
> length. So if it's OK with you I'll leave the constant.

It is OK with me.

[PATCH v4 11/32] delete_ref_loose(): don't muck around in the lock_file's filename

[Michael Haggerty]

It's bad manners.  Especially since, if unlink_or_warn() failed, the
memory wasn't restored to its original contents.

So make our own copy to work with.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 828522d..8a63073 100644
--- a/refs.c
+++ b/refs.c
@@ -2545,12 +2545,15 @@ static int repack_without_ref(const char *refname)
 static int delete_ref_loose(struct ref_lock *lock, int flag)
 {
 	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
-		/* loose */
-		int err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN;
-
-		lock->lk->filename[i] = 0;
-		err = unlink_or_warn(lock->lk->filename);
-		lock->lk->filename[i] = LOCK_SUFFIX[0];
+		/*
+		 * loose.  The loose file name is the same as the
+		 * lockfile name, minus ".lock":
+		 */
+		char *loose_filename = xmemdupz(
+				lock->lk->filename,
+				strlen(lock->lk->filename) - LOCK_SUFFIX_LEN);
+		int err = unlink_or_warn(loose_filename);
+		free(loose_filename);
 		if (err && errno != ENOENT)
 			return 1;
 	}
-- 
2.1.0

Re: [PATCH v4 11/32] delete_ref_loose(): don't muck around in the lock_file's filename

[Johannes Sixt]

Am 06.09.2014 um 09:50 schrieb Michael Haggerty:
> It's bad manners.  Especially since, if unlink_or_warn() failed, the
> memory wasn't restored to its original contents.

I do not see how the old code did not restore the file name. Except for
this nit, the patch looks good.

> 
> So make our own copy to work with.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 828522d..8a63073 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2545,12 +2545,15 @@ static int repack_without_ref(const char *refname)
>  static int delete_ref_loose(struct ref_lock *lock, int flag)
>  {
>  	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
> -		/* loose */
> -		int err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN;
> -
> -		lock->lk->filename[i] = 0;
> -		err = unlink_or_warn(lock->lk->filename);
> -		lock->lk->filename[i] = LOCK_SUFFIX[0];
> +		/*
> +		 * loose.  The loose file name is the same as the
> +		 * lockfile name, minus ".lock":
> +		 */
> +		char *loose_filename = xmemdupz(
> +				lock->lk->filename,
> +				strlen(lock->lk->filename) - LOCK_SUFFIX_LEN);
> +		int err = unlink_or_warn(loose_filename);
> +		free(loose_filename);
>  		if (err && errno != ENOENT)
>  			return 1;
>  	}
> 

Re: [PATCH v4 11/32] delete_ref_loose(): don't muck around in the lock_file's filename

[Michael Haggerty]

On 09/13/2014 09:41 AM, Johannes Sixt wrote:
> Am 06.09.2014 um 09:50 schrieb Michael Haggerty:
>> It's bad manners.  Especially since, if unlink_or_warn() failed, the
>> memory wasn't restored to its original contents.
> 
> I do not see how the old code did not restore the file name. Except for
> this nit, the patch looks good.

Hmmmm, you're quite right. I thought I had found some circumstance in
which unlink_or_warn() could fail to allocate memory and die() or
something. But I can't find anything like that now.

I will remove that sentence from the commit message.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v4 11/32] delete_ref_loose(): don't muck around in the lock_file's filename

[Michael Haggerty]

On 09/14/2014 08:27 AM, Michael Haggerty wrote:
> On 09/13/2014 09:41 AM, Johannes Sixt wrote:
>> Am 06.09.2014 um 09:50 schrieb Michael Haggerty:
>>> It's bad manners.  Especially since, if unlink_or_warn() failed, the
>>> memory wasn't restored to its original contents.
>>
>> I do not see how the old code did not restore the file name. Except for
>> this nit, the patch looks good.
> 
> Hmmmm, you're quite right. I thought I had found some circumstance in
> which unlink_or_warn() could fail to allocate memory and die() or
> something. But I can't find anything like that now.
> 
> I will remove that sentence from the commit message.

I half withdraw my withdrawal. It's true that the failure of
unlink_or_warn() wouldn't cause a problem. But a signal could arrive
while unlink_or_warn() is executing, in which case the signal handler
would see the wrong filename and try to delete the loose reference file,
leaving the lockfile behind.

I will clarify the log message.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v4 11/32] delete_ref_loose(): don't muck around in the lock_file's filename

[Johannes Sixt]

Am 14.09.2014 um 08:38 schrieb Michael Haggerty:
> On 09/14/2014 08:27 AM, Michael Haggerty wrote:
>> On 09/13/2014 09:41 AM, Johannes Sixt wrote:
>>> Am 06.09.2014 um 09:50 schrieb Michael Haggerty:
>>>> It's bad manners.  Especially since, if unlink_or_warn() failed, the
>>>> memory wasn't restored to its original contents.
>>>
>>> I do not see how the old code did not restore the file name. Except for
>>> this nit, the patch looks good.
>>
>> Hmmmm, you're quite right. I thought I had found some circumstance in
>> which unlink_or_warn() could fail to allocate memory and die() or
>> something. But I can't find anything like that now.
>>
>> I will remove that sentence from the commit message.
> 
> I half withdraw my withdrawal. It's true that the failure of
> unlink_or_warn() wouldn't cause a problem. But a signal could arrive
> while unlink_or_warn() is executing, in which case the signal handler
> would see the wrong filename and try to delete the loose reference file,
> leaving the lockfile behind.

Good catch! This makes the patch much more important than just to
establish good manners.

-- Hannes

[PATCH v4 12/32] prepare_index(): declare return value to be (const char *)

[Michael Haggerty]

Declare the return value to be const to make it clear that we aren't
giving callers permission to write over the string that it points at.
(The return value is the filename field of a struct lock_file, which
can be used by a signal handler at any time and therefore shouldn't be
tampered with.)

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/commit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a3eaf4b..046be7e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -315,8 +315,8 @@ static void refresh_cache_or_die(int refresh_flags)
 		die_resolve_conflict("commit");
 }
 
-static char *prepare_index(int argc, const char **argv, const char *prefix,
-			   const struct commit *current_head, int is_status)
+static const char *prepare_index(int argc, const char **argv, const char *prefix,
+				 const struct commit *current_head, int is_status)
 {
 	struct string_list partial;
 	struct pathspec pathspec;
-- 
2.1.0

[PATCH v4 13/32] write_packed_entry_fn(): convert cb_data into a (const int *)

[Michael Haggerty]

This makes it obvious that we have no plans to change the integer
pointed to, which is actually the fd field from a struct lock_file.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 8a63073..21b0da3 100644
--- a/refs.c
+++ b/refs.c
@@ -2218,7 +2218,7 @@ static void write_packed_entry(int fd, char *refname, unsigned char *sha1,
  */
 static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
 {
-	int *fd = cb_data;
+	const int *fd = cb_data;
 	enum peel_status peel_status = peel_entry(entry, 0);
 
 	if (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)
-- 
2.1.0

Re: [PATCH v4 13/32] write_packed_entry_fn(): convert cb_data into a (const int *)

[Ronnie Sahlberg]

Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>

On Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> This makes it obvious that we have no plans to change the integer
> pointed to, which is actually the fd field from a struct lock_file.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 8a63073..21b0da3 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2218,7 +2218,7 @@ static void write_packed_entry(int fd, char *refname, unsigned char *sha1,
>   */
>  static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
>  {
> -       int *fd = cb_data;
> +       const int *fd = cb_data;
>         enum peel_status peel_status = peel_entry(entry, 0);
>
>         if (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 14/32] lock_file(): exit early if lockfile cannot be opened

[Michael Haggerty]

This is a bit easier to read than the old version, which nested part
of the non-error code in an "if" block.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index bfea333..71786c9 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -207,19 +207,18 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 		resolve_symlink(lk->filename, max_path_len);
 	strcat(lk->filename, LOCK_SUFFIX);
 	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
-	if (0 <= lk->fd) {
-		lk->owner = getpid();
-		if (adjust_shared_perm(lk->filename)) {
-			int save_errno = errno;
-			error("cannot fix permission bits on %s",
-			      lk->filename);
-			rollback_lock_file(lk);
-			errno = save_errno;
-			return -1;
-		}
-	}
-	else
+	if (lk->fd < 0) {
 		lk->filename[0] = 0;
+		return -1;
+	}
+	lk->owner = getpid();
+	if (adjust_shared_perm(lk->filename)) {
+		int save_errno = errno;
+		error("cannot fix permission bits on %s", lk->filename);
+		rollback_lock_file(lk);
+		errno = save_errno;
+		return -1;
+	}
 	return lk->fd;
 }
 
-- 
2.1.0

Re: [PATCH v4 14/32] lock_file(): exit early if lockfile cannot be opened

[Ronnie Sahlberg]

Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>


On Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> This is a bit easier to read than the old version, which nested part
> of the non-error code in an "if" block.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  lockfile.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/lockfile.c b/lockfile.c
> index bfea333..71786c9 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -207,19 +207,18 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>                 resolve_symlink(lk->filename, max_path_len);
>         strcat(lk->filename, LOCK_SUFFIX);
>         lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
> -       if (0 <= lk->fd) {
> -               lk->owner = getpid();
> -               if (adjust_shared_perm(lk->filename)) {
> -                       int save_errno = errno;
> -                       error("cannot fix permission bits on %s",
> -                             lk->filename);
> -                       rollback_lock_file(lk);
> -                       errno = save_errno;
> -                       return -1;
> -               }
> -       }
> -       else
> +       if (lk->fd < 0) {
>                 lk->filename[0] = 0;
> +               return -1;
> +       }
> +       lk->owner = getpid();
> +       if (adjust_shared_perm(lk->filename)) {
> +               int save_errno = errno;
> +               error("cannot fix permission bits on %s", lk->filename);
> +               rollback_lock_file(lk);
> +               errno = save_errno;
> +               return -1;
> +       }
>         return lk->fd;
>  }
>
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 15/32] remove_lock_file(): call rollback_lock_file()

[Michael Haggerty]

It does just what we need.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 71786c9..dacfc28 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -63,12 +63,8 @@ static void remove_lock_file(void)
 	pid_t me = getpid();
 
 	while (lock_file_list) {
-		if (lock_file_list->owner == me &&
-		    lock_file_list->filename[0]) {
-			if (lock_file_list->fd >= 0)
-				close(lock_file_list->fd);
-			unlink_or_warn(lock_file_list->filename);
-		}
+		if (lock_file_list->owner == me)
+			rollback_lock_file(lock_file_list);
 		lock_file_list = lock_file_list->next;
 	}
 }
-- 
2.1.0

[PATCH v4 16/32] commit_lock_file(): inline temporary variable

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index dacfc28..d64cf6b 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -306,12 +306,14 @@ int reopen_lock_file(struct lock_file *lk)
 int commit_lock_file(struct lock_file *lk)
 {
 	char result_file[PATH_MAX];
-	size_t i;
+
 	if (lk->fd >= 0 && close_lock_file(lk))
 		return -1;
+
 	strcpy(result_file, lk->filename);
-	i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */
-	result_file[i] = 0;
+	/* remove ".lock": */
+	result_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;
+
 	if (rename(lk->filename, result_file))
 		return -1;
 	lk->filename[0] = 0;
-- 
2.1.0

[PATCH v4 17/32] commit_lock_file(): die() if called for unlocked lockfile object

[Michael Haggerty]

It was previously a bug to call commit_lock_file() with a lock_file
object that was not active (an illegal access would happen within the
function).  It was presumably never done, but this would be an easy
programming error to overlook.  So before continuing, do a consistency
check that the lock_file object really is locked.

Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/technical/api-lockfile.txt | 4 +++-
 lockfile.c                               | 3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index b53e300..9a94ead 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -68,7 +68,9 @@ commit_lock_file::
 	with an earlier call to `hold_lock_file_for_update()`,
 	close the file descriptor and rename the lockfile to its
 	final destination.  Returns 0 upon success, a negative
-	value on failure to close(2) or rename(2).
+	value on failure to close(2) or rename(2).  It is a bug to
+	call `commit_lock_file()` for a `lock_file` object that is not
+	currently locked.
 
 rollback_lock_file::
 
diff --git a/lockfile.c b/lockfile.c
index d64cf6b..716b674 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -307,6 +307,9 @@ int commit_lock_file(struct lock_file *lk)
 {
 	char result_file[PATH_MAX];
 
+	if (!lk->filename[0])
+		die("BUG: attempt to commit unlocked object");
+
 	if (lk->fd >= 0 && close_lock_file(lk))
 		return -1;
 
-- 
2.1.0

[PATCH v4 18/32] commit_lock_file(): if close fails, roll back

[Michael Haggerty]

If closing an open lockfile fails, then we cannot be sure of the
contents of the lockfile, so there is nothing sensible to do but
delete it.  This change also leaves the lock_file object in a defined
state in this error path (namely, unlocked).

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lockfile.c b/lockfile.c
index 716b674..213783a 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -310,8 +310,12 @@ int commit_lock_file(struct lock_file *lk)
 	if (!lk->filename[0])
 		die("BUG: attempt to commit unlocked object");
 
-	if (lk->fd >= 0 && close_lock_file(lk))
+	if (lk->fd >= 0 && close_lock_file(lk)) {
+		int save_errno = errno;
+		rollback_lock_file(lk);
+		errno = save_errno;
 		return -1;
+	}
 
 	strcpy(result_file, lk->filename);
 	/* remove ".lock": */
-- 
2.1.0

[PATCH v4 19/32] commit_lock_file(): rollback lock file on failure to rename

[Michael Haggerty]

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.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 213783a..a7cdb64 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -306,25 +306,29 @@ int reopen_lock_file(struct lock_file *lk)
 int commit_lock_file(struct lock_file *lk)
 {
 	char result_file[PATH_MAX];
+	int save_errno;
 
 	if (!lk->filename[0])
 		die("BUG: attempt to commit unlocked object");
 
-	if (lk->fd >= 0 && close_lock_file(lk)) {
-		int save_errno = errno;
-		rollback_lock_file(lk);
-		errno = save_errno;
-		return -1;
-	}
+	if (lk->fd >= 0 && close_lock_file(lk))
+		goto rollback;
 
 	strcpy(result_file, lk->filename);
 	/* remove ".lock": */
 	result_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;
 
 	if (rename(lk->filename, result_file))
-		return -1;
+		goto rollback;
+
 	lk->filename[0] = 0;
 	return 0;
+
+rollback:
+	save_errno = errno;
+	rollback_lock_file(lk);
+	errno = save_errno;
+	return -1;
 }
 
 int hold_locked_index(struct lock_file *lk, int die_on_error)
-- 
2.1.0

Re: [PATCH v4 19/32] commit_lock_file(): rollback lock file on failure to rename

[Jeff King]

On Sat, Sep 06, 2014 at 09:50:33AM +0200, 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.

Unlike the previous patch, in this case the contents of the lockfile
_are_ defined. So in theory a caller could somehow retry.

I don't see any callers that want to do that, though (and besides, they
would not know if the error came from the close or the rename), so I
think we can consider that an uninteresting case until somebody
creates such a caller (at which point they can take responsibility for
extending the API).

BTW, while grepping for commit_lock_file calls, I notice we often commit
the shallow file without checking the return code. I'm not sure what we
should do in each case, but I imagine that calling die() is probably
better than continuing as if it succeeded. +cc Duy

-Peff

Re: [PATCH v4 19/32] commit_lock_file(): rollback lock file on failure to rename

[Duy Nguyen]

On Wed, Sep 10, 2014 at 2:55 PM, Jeff King <peff@peff.net> wrote:
> BTW, while grepping for commit_lock_file calls, I notice we often commit
> the shallow file without checking the return code. I'm not sure what we
> should do in each case, but I imagine that calling die() is probably
> better than continuing as if it succeeded. +cc Duy

Noted. To be fixed soon. It looks like we could make gcc warn about
ignoring return code like this with
__attribute__((warn_unused_result)) (but I haven't tested). If it does
help catch mishandling in future commit_lock_file calls and does not
upset other compilers, we may want to add it to commit_lock_file.
-- 
Duy

[PATCH v4 20/32] api-lockfile: document edge cases

[Michael Haggerty]

* Document the behavior of commit_lock_file() when it fails, namely
  that it rolls back the lock_file object and sets errno
  appropriately.

* Document the behavior of rollback_lock_file() when called for a
  lock_file object that has already been committed or rolled back,
  namely that it is a NOOP.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/technical/api-lockfile.txt | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index 9a94ead..2514559 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -64,19 +64,22 @@ unable_to_lock_die::
 
 commit_lock_file::
 
-	Take a pointer to the `struct lock_file` initialized
-	with an earlier call to `hold_lock_file_for_update()`,
-	close the file descriptor and rename the lockfile to its
-	final destination.  Returns 0 upon success, a negative
-	value on failure to close(2) or rename(2).  It is a bug to
-	call `commit_lock_file()` for a `lock_file` object that is not
+	Take a pointer to the `struct lock_file` initialized with an
+	earlier call to `hold_lock_file_for_update()`, close the file
+	descriptor and rename the lockfile to its final destination.
+	Return 0 upon success.  On failure, rollback the lock file and
+	return -1, with `errno` set to the value from the failing call
+	to `close(2)` or `rename(2)`.  It is a bug to call
+	`commit_lock_file()` for a `lock_file` object that is not
 	currently locked.
 
 rollback_lock_file::
 
 	Take a pointer to the `struct lock_file` initialized
 	with an earlier call to `hold_lock_file_for_update()`,
-	close the file descriptor and remove the lockfile.
+	close the file descriptor and remove the lockfile.  It is a
+	NOOP to call `rollback_lock_file()` for a `lock_file` object
+	that has already been committed or rolled back.
 
 close_lock_file::
 	Take a pointer to the `struct lock_file` initialized
-- 
2.1.0

[PATCH v4 21/32] dump_marks(): remove a redundant call to rollback_lock_file()

[Michael Haggerty]

When commit_lock_file() fails, it now always calls
rollback_lock_file() internally, so there is no need to call that
function here.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 fast-import.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index d73f58c..55cb47a 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1803,10 +1803,8 @@ static void dump_marks(void)
 	}
 
 	if (commit_lock_file(&mark_lock)) {
-		int saved_errno = errno;
-		rollback_lock_file(&mark_lock);
 		failure |= error("Unable to commit marks file %s: %s",
-			export_marks_file, strerror(saved_errno));
+			export_marks_file, strerror(errno));
 		return;
 	}
 }
-- 
2.1.0

[PATCH v4 22/32] git_config_set_multivar_in_file(): avoid call to rollback_lock_file()

[Michael Haggerty]

After commit_lock_file() is called, then the lock_file object is
necessarily either committed or rolled back.  So there is no need to
call rollback_lock_file() again in either of these cases.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 config.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/config.c b/config.c
index a191328..24f0abb 100644
--- a/config.c
+++ b/config.c
@@ -1964,17 +1964,17 @@ int git_config_set_multivar_in_file(const char *config_filename,
 	if (commit_lock_file(lock) < 0) {
 		error("could not commit config file %s", config_filename);
 		ret = CONFIG_NO_WRITE;
-		goto out_free;
-	}
+	} else
+		ret = 0;
 
 	/*
-	 * lock is committed, so don't try to roll it back below.
-	 * NOTE: Since lockfile.c keeps a linked list of all created
-	 * lock_file structures, it isn't safe to free(lock).  It's
-	 * better to just leave it hanging around.
+	 * lock is committed or rolled back now, so there is no need
+	 * to roll it back below.  NOTE: Since lockfile.c keeps a
+	 * linked list of all created lock_file structures, it isn't
+	 * safe to free(lock).  We have to just leave it hanging
+	 * around.
 	 */
 	lock = NULL;
-	ret = 0;
 
 	/* Invalidate the config cache */
 	git_config_clear();
-- 
2.1.0

[PATCH v4 23/32] lockfile: avoid transitory invalid states

[Michael Haggerty]

Because remove_lock_file() can be called any time by the signal
handler, it is important that any lock_file objects that are in the
lock_file_list are always in a valid state.  And since lock_file
objects are often reused (but are never removed from lock_file_list),
that means we have to be careful whenever mutating a lock_file object
to always keep it in a well-defined state.

This was formerly not the case, because part of the state was encoded
by setting lk->filename to the empty string vs. a valid filename.  It
is wrong to assume that this string can be updated atomically; for
example, even

    strcpy(lk->filename, value)

is unsafe.  But the old code was even more reckless; for example,

    strcpy(lk->filename, path);
    if (!(flags & LOCK_NODEREF))
            resolve_symlink(lk->filename, max_path_len);
    strcat(lk->filename, ".lock");

During the call to resolve_symlink(), lk->filename contained the name
of the file that was being locked, not the name of the lockfile.  If a
signal were raised during that interval, then the signal handler would
have deleted the valuable file!

We could probably continue to use the filename field to encode the
state by being careful to write characters 1..N-1 of the filename
first, and then overwrite the NUL at filename[0] with the first
character of the filename, but that would be awkward and error-prone.

So, instead of using the filename field to determine whether the
lock_file object is active, add a new field "lock_file::active" for
this purpose.  Be careful to set this field only when filename really
contains the name of a file that should be deleted on cleanup.

Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 cache.h      |  1 +
 lockfile.c   | 45 ++++++++++++++++++++++++++++++---------------
 read-cache.c |  1 +
 3 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/cache.h b/cache.h
index 41d829b..e739482 100644
--- a/cache.h
+++ b/cache.h
@@ -575,6 +575,7 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct
 
 struct lock_file {
 	struct lock_file *next;
+	volatile sig_atomic_t active;
 	int fd;
 	pid_t owner;
 	char on_list;
diff --git a/lockfile.c b/lockfile.c
index a7cdb64..920585d 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -27,11 +27,14 @@
  * Instead of (3), the change can be rolled back by deleting lockfile.
  *
  * This module keeps track of all locked files in lock_file_list.
- * When the first file is locked, it registers an atexit(3) handler;
- * when the program exits, the handler rolls back any files that have
- * been locked but were never committed or rolled back.
+ * When the first file is locked, it registers an atexit(3) handler
+ * and a signal handler; when the program exits, the handler rolls
+ * back any files that have been locked but were never committed or
+ * rolled back.
  *
- * A lock_file object can be in several states:
+ * Because the signal handler can be called at any time, a lock_file
+ * object must always be in a well-defined state.  The possible states
+ * are as follows:
  *
  * - Uninitialized.  In this state the object's on_list field must be
  *   zero but the rest of its contents need not be initialized.  As
@@ -39,19 +42,25 @@
  *   registered in the lock_file_list, and on_list is set.
  *
  * - Locked, lockfile open (after hold_lock_file_for_update(),
- *   hold_lock_file_for_append(), or reopen_lock_file()). In this
- *   state, the lockfile exists, filename holds the filename of the
- *   lockfile, fd holds a file descriptor open for writing to the
- *   lockfile, and owner holds the PID of the process that locked the
- *   file.
+ *   hold_lock_file_for_append(), or reopen_lock_file()).  In this
+ *   state:
+ *   - the lockfile exists
+ *   - active is set
+ *   - filename holds the filename of the lockfile
+ *   - fd holds a file descriptor open for writing to the lockfile
+ *   - owner holds the PID of the process that locked the file
  *
  * - Locked, lockfile closed (after close_lock_file()).  Same as the
  *   previous state, except that the lockfile is closed and fd is -1.
  *
  * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a
- *   failed attempt to lock).  In this state, filename[0] == '\0' and
- *   fd is -1.  The object is left registered in the lock_file_list,
- *   and on_list is set.
+ *   failed attempt to lock).  In this state:
+ *   - active is unset
+ *   - filename[0] == '\0' (usually, though there are transitory states
+ *     in which this condition doesn't hold)
+ *   - fd is -1
+ *   - the object is left registered in the lock_file_list, and
+ *     on_list is set.
  *
  * See Documentation/api-lockfile.txt for more information.
  */
@@ -184,9 +193,12 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 		atexit(remove_lock_file);
 	}
 
+	assert(!lk->active);
+
 	if (!lk->on_list) {
 		/* Initialize *lk and add it to lock_file_list: */
 		lk->fd = -1;
+		lk->active = 0;
 		lk->owner = 0;
 		lk->on_list = 1;
 		lk->filename[0] = 0;
@@ -208,6 +220,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 		return -1;
 	}
 	lk->owner = getpid();
+	lk->active = 1;
 	if (adjust_shared_perm(lk->filename)) {
 		int save_errno = errno;
 		error("cannot fix permission bits on %s", lk->filename);
@@ -297,7 +310,7 @@ int reopen_lock_file(struct lock_file *lk)
 {
 	if (0 <= lk->fd)
 		die(_("BUG: reopen a lockfile that is still open"));
-	if (!lk->filename[0])
+	if (!lk->active)
 		die(_("BUG: reopen a lockfile that has been committed"));
 	lk->fd = open(lk->filename, O_WRONLY);
 	return lk->fd;
@@ -308,7 +321,7 @@ int commit_lock_file(struct lock_file *lk)
 	char result_file[PATH_MAX];
 	int save_errno;
 
-	if (!lk->filename[0])
+	if (!lk->active)
 		die("BUG: attempt to commit unlocked object");
 
 	if (lk->fd >= 0 && close_lock_file(lk))
@@ -321,6 +334,7 @@ int commit_lock_file(struct lock_file *lk)
 	if (rename(lk->filename, result_file))
 		goto rollback;
 
+	lk->active = 0;
 	lk->filename[0] = 0;
 	return 0;
 
@@ -341,11 +355,12 @@ int hold_locked_index(struct lock_file *lk, int die_on_error)
 
 void rollback_lock_file(struct lock_file *lk)
 {
-	if (!lk->filename[0])
+	if (!lk->active)
 		return;
 
 	if (lk->fd >= 0)
 		close_lock_file(lk);
 	unlink_or_warn(lk->filename);
+	lk->active = 0;
 	lk->filename[0] = 0;
 }
diff --git a/read-cache.c b/read-cache.c
index 5d3c8bd..3d43dda 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2028,6 +2028,7 @@ static int commit_locked_index(struct lock_file *lk)
 			return -1;
 		if (rename(lk->filename, alternate_index_output))
 			return -1;
+		lk->active = 0;
 		lk->filename[0] = 0;
 		return 0;
 	} else {
-- 
2.1.0

[PATCH v4 24/32] struct lock_file: declare some fields volatile

[Michael Haggerty]

The function remove_lock_file_on_signal() is used as a signal handler.
It is not realistic to make the signal handler conform strictly to the
C standard, which is very restrictive about what a signal handler is
allowed to do.  But let's increase the likelihood that it will work:

The lock_file_list global variable and several fields from struct
lock_file are used by the signal handler.  Declare those values
"volatile" to (1) force the main process to write the values to RAM
promptly, and (2) prevent updates to these fields from being reordered
in a way that leaves an opportunity for a jump to the signal handler
while the object is in an inconsistent state.

We don't mark the filename field volatile because that would prevent
the use of strcpy(), and it is anyway unlikely that a compiler
re-orders a strcpy() call across other expressions.  So in practice it
should be possible to get away without "volatile" in the "filename"
case.

Suggested-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 cache.h    | 6 +++---
 lockfile.c | 2 +-
 refs.c     | 5 +++--
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/cache.h b/cache.h
index e739482..f7ad1b5 100644
--- a/cache.h
+++ b/cache.h
@@ -574,10 +574,10 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct
 #define LOCK_SUFFIX_LEN 5
 
 struct lock_file {
-	struct lock_file *next;
+	struct lock_file *volatile next;
 	volatile sig_atomic_t active;
-	int fd;
-	pid_t owner;
+	volatile int fd;
+	volatile pid_t owner;
 	char on_list;
 	char filename[PATH_MAX];
 };
diff --git a/lockfile.c b/lockfile.c
index 920585d..f51f73f 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -65,7 +65,7 @@
  * See Documentation/api-lockfile.txt for more information.
  */
 
-static struct lock_file *lock_file_list;
+static struct lock_file *volatile lock_file_list;
 
 static void remove_lock_file(void)
 {
diff --git a/refs.c b/refs.c
index 21b0da3..f076f2d 100644
--- a/refs.c
+++ b/refs.c
@@ -2260,15 +2260,16 @@ int commit_packed_refs(void)
 		get_packed_ref_cache(&ref_cache);
 	int error = 0;
 	int save_errno = 0;
+	int fd;
 
 	if (!packed_ref_cache->lock)
 		die("internal error: packed-refs not locked");
 	write_or_die(packed_ref_cache->lock->fd,
 		     PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));
 
+	fd = packed_ref_cache->lock->fd;
 	do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
-				 0, write_packed_entry_fn,
-				 &packed_ref_cache->lock->fd);
+				 0, write_packed_entry_fn, &fd);
 	if (commit_lock_file(packed_ref_cache->lock)) {
 		save_errno = errno;
 		error = -1;
-- 
2.1.0

[PATCH v4 25/32] try_merge_strategy(): remove redundant lock_file allocation

[Michael Haggerty]

By the time the "if" block is entered, the lock_file instance from the
main function block is no longer in use, so re-use that one instead of
allocating a second one.

Note that the "lock" variable in the "if" block shadowed the "lock"
variable at function scope, so the only change needed is to remove the
inner definition.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/merge.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index ce82eb2..0f5c185 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -669,7 +669,6 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 	if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
 		int clean, x;
 		struct commit *result;
-		struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
 		struct commit_list *reversed = NULL;
 		struct merge_options o;
 		struct commit_list *j;
-- 
2.1.0

[PATCH v4 26/32] try_merge_strategy(): use a statically-allocated lock_file object

[Michael Haggerty]

Even the one lockfile object needn't be allocated each time the
function is called.  Instead, define one statically-allocated
lock_file object and reuse it for every call.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/merge.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 0f5c185..d2415fe 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -657,14 +657,14 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 			      struct commit_list *remoteheads,
 			      struct commit *head, const char *head_arg)
 {
-	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
+	static struct lock_file lock;
 
-	hold_locked_index(lock, 1);
+	hold_locked_index(&lock, 1);
 	refresh_cache(REFRESH_QUIET);
 	if (active_cache_changed &&
-	    write_locked_index(&the_index, lock, COMMIT_LOCK))
+	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
 		return error(_("Unable to write index."));
-	rollback_lock_file(lock);
+	rollback_lock_file(&lock);
 
 	if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
 		int clean, x;
@@ -696,13 +696,13 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		for (j = common; j; j = j->next)
 			commit_list_insert(j->item, &reversed);
 
-		hold_locked_index(lock, 1);
+		hold_locked_index(&lock, 1);
 		clean = merge_recursive(&o, head,
 				remoteheads->item, reversed, &result);
 		if (active_cache_changed &&
-		    write_locked_index(&the_index, lock, COMMIT_LOCK))
+		    write_locked_index(&the_index, &lock, COMMIT_LOCK))
 			die (_("unable to write %s"), get_index_file());
-		rollback_lock_file(lock);
+		rollback_lock_file(&lock);
 		return clean ? 0 : 1;
 	} else {
 		return try_merge_command(strategy, xopts_nr, xopts,
-- 
2.1.0

[PATCH v4 27/32] commit_lock_file(): use a strbuf to manage temporary space

[Michael Haggerty]

Avoid relying on the filename length restrictions that are currently
checked by lock_file().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index f51f73f..f6b3866 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -318,8 +318,9 @@ int reopen_lock_file(struct lock_file *lk)
 
 int commit_lock_file(struct lock_file *lk)
 {
-	char result_file[PATH_MAX];
+	static struct strbuf result_file = STRBUF_INIT;
 	int save_errno;
+	int err;
 
 	if (!lk->active)
 		die("BUG: attempt to commit unlocked object");
@@ -327,13 +328,13 @@ int commit_lock_file(struct lock_file *lk)
 	if (lk->fd >= 0 && close_lock_file(lk))
 		goto rollback;
 
-	strcpy(result_file, lk->filename);
 	/* remove ".lock": */
-	result_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;
-
-	if (rename(lk->filename, result_file))
+	strbuf_add(&result_file, lk->filename,
+		   strlen(lk->filename) - LOCK_SUFFIX_LEN);
+	err = rename(lk->filename, result_file.buf);
+	strbuf_reset(&result_file);
+	if (err)
 		goto rollback;
-
 	lk->active = 0;
 	lk->filename[0] = 0;
 	return 0;
-- 
2.1.0

[PATCH v4 28/32] Change lock_file::filename into a strbuf

[Michael Haggerty]

For now, we still make sure to allocate at least PATH_MAX characters
for the strbuf because resolve_symlink() doesn't know how to expand
the space for its return value.  (That will be fixed in a moment.)

Another alternative would be to just use a strbuf as scratch space in
lock_file() but then store a pointer to the naked string in struct
lock_file.  But lock_file objects are often reused.  By reusing the
same strbuf, we can avoid having to reallocate the string most times
when a lock_file object is reused.

Helped-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/commit.c | 12 ++++++------
 builtin/reflog.c |  2 +-
 cache.h          |  2 +-
 config.c         | 14 +++++++-------
 lockfile.c       | 47 +++++++++++++++++++----------------------------
 read-cache.c     |  4 ++--
 refs.c           |  6 +++---
 shallow.c        |  6 +++---
 8 files changed, 42 insertions(+), 51 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 046be7e..b2fc9e8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -341,7 +341,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 			die(_("unable to create temporary index"));
 
 		old_index_env = getenv(INDEX_ENVIRONMENT);
-		setenv(INDEX_ENVIRONMENT, index_lock.filename, 1);
+		setenv(INDEX_ENVIRONMENT, index_lock.filename.buf, 1);
 
 		if (interactive_add(argc, argv, prefix, patch_interactive) != 0)
 			die(_("interactive add failed"));
@@ -352,10 +352,10 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 			unsetenv(INDEX_ENVIRONMENT);
 
 		discard_cache();
-		read_cache_from(index_lock.filename);
+		read_cache_from(index_lock.filename.buf);
 
 		commit_style = COMMIT_NORMAL;
-		return index_lock.filename;
+		return index_lock.filename.buf;
 	}
 
 	/*
@@ -378,7 +378,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 		if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))
 			die(_("unable to write new_index file"));
 		commit_style = COMMIT_NORMAL;
-		return index_lock.filename;
+		return index_lock.filename.buf;
 	}
 
 	/*
@@ -460,9 +460,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 		die(_("unable to write temporary index file"));
 
 	discard_cache();
-	read_cache_from(false_lock.filename);
+	read_cache_from(false_lock.filename.buf);
 
-	return false_lock.filename;
+	return false_lock.filename.buf;
 }
 
 static int run_status(FILE *fp, const char *index_file, const char *prefix, int nowarn,
diff --git a/builtin/reflog.c b/builtin/reflog.c
index e8a8fb1..7c78b15 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -431,7 +431,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,
 			 write_str_in_full(lock->lock_fd, "\n") != 1 ||
 			 close_ref(lock) < 0)) {
 			status |= error("Couldn't write %s",
-				lock->lk->filename);
+					lock->lk->filename.buf);
 			unlink(newlog_path);
 		} else if (rename(newlog_path, log_file)) {
 			status |= error("cannot rename %s to %s",
diff --git a/cache.h b/cache.h
index f7ad1b5..bea6296 100644
--- a/cache.h
+++ b/cache.h
@@ -579,7 +579,7 @@ struct lock_file {
 	volatile int fd;
 	volatile pid_t owner;
 	char on_list;
-	char filename[PATH_MAX];
+	struct strbuf filename;
 };
 #define LOCK_DIE_ON_ERROR 1
 #define LOCK_NODEREF 2
diff --git a/config.c b/config.c
index 24f0abb..d9247c6 100644
--- a/config.c
+++ b/config.c
@@ -1905,9 +1905,9 @@ int git_config_set_multivar_in_file(const char *config_filename,
 			MAP_PRIVATE, in_fd, 0);
 		close(in_fd);
 
-		if (chmod(lock->filename, st.st_mode & 07777) < 0) {
+		if (chmod(lock->filename.buf, st.st_mode & 07777) < 0) {
 			error("chmod on %s failed: %s",
-				lock->filename, strerror(errno));
+				lock->filename.buf, strerror(errno));
 			ret = CONFIG_NO_WRITE;
 			goto out_free;
 		}
@@ -1986,7 +1986,7 @@ out_free:
 	return ret;
 
 write_err_out:
-	ret = write_error(lock->filename);
+	ret = write_error(lock->filename.buf);
 	goto out_free;
 
 }
@@ -2087,9 +2087,9 @@ int git_config_rename_section_in_file(const char *config_filename,
 
 	fstat(fileno(config_file), &st);
 
-	if (chmod(lock->filename, st.st_mode & 07777) < 0) {
+	if (chmod(lock->filename.buf, st.st_mode & 07777) < 0) {
 		ret = error("chmod on %s failed: %s",
-				lock->filename, strerror(errno));
+				lock->filename.buf, strerror(errno));
 		goto out;
 	}
 
@@ -2110,7 +2110,7 @@ int git_config_rename_section_in_file(const char *config_filename,
 				}
 				store.baselen = strlen(new_name);
 				if (!store_write_section(out_fd, new_name)) {
-					ret = write_error(lock->filename);
+					ret = write_error(lock->filename.buf);
 					goto out;
 				}
 				/*
@@ -2136,7 +2136,7 @@ int git_config_rename_section_in_file(const char *config_filename,
 			continue;
 		length = strlen(output);
 		if (write_in_full(out_fd, output, length) != length) {
-			ret = write_error(lock->filename);
+			ret = write_error(lock->filename.buf);
 			goto out;
 		}
 	}
diff --git a/lockfile.c b/lockfile.c
index f6b3866..c34bf6c 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -56,8 +56,8 @@
  * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a
  *   failed attempt to lock).  In this state:
  *   - active is unset
- *   - filename[0] == '\0' (usually, though there are transitory states
- *     in which this condition doesn't hold)
+ *   - filename is the empty string (usually, though there are
+ *     transitory states in which this condition doesn't hold)
  *   - fd is -1
  *   - the object is left registered in the lock_file_list, and
  *     on_list is set.
@@ -180,13 +180,6 @@ static char *resolve_symlink(char *p, size_t s)
 /* Make sure errno contains a meaningful value on error */
 static int lock_file(struct lock_file *lk, const char *path, int flags)
 {
-	/*
-	 * subtract LOCK_SUFFIX_LEN from size to make sure there's
-	 * room for adding ".lock" for the lock file name:
-	 */
-	static const size_t max_path_len = sizeof(lk->filename) -
-					   LOCK_SUFFIX_LEN;
-
 	if (!lock_file_list) {
 		/* One-time initialization */
 		sigchain_push_common(remove_lock_file_on_signal);
@@ -201,29 +194,27 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 		lk->active = 0;
 		lk->owner = 0;
 		lk->on_list = 1;
-		lk->filename[0] = 0;
+		strbuf_init(&lk->filename, PATH_MAX);
 		lk->next = lock_file_list;
 		lock_file_list = lk;
 	}
 
-	if (strlen(path) >= max_path_len) {
-		errno = ENAMETOOLONG;
-		return -1;
+	strbuf_addstr(&lk->filename, path);
+	if (!(flags & LOCK_NODEREF)) {
+		resolve_symlink(lk->filename.buf, lk->filename.alloc);
+		strbuf_setlen(&lk->filename, strlen(lk->filename.buf));
 	}
-	strcpy(lk->filename, path);
-	if (!(flags & LOCK_NODEREF))
-		resolve_symlink(lk->filename, max_path_len);
-	strcat(lk->filename, LOCK_SUFFIX);
-	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
+	strbuf_addstr(&lk->filename, LOCK_SUFFIX);
+	lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (lk->fd < 0) {
-		lk->filename[0] = 0;
+		strbuf_reset(&lk->filename);
 		return -1;
 	}
 	lk->owner = getpid();
 	lk->active = 1;
-	if (adjust_shared_perm(lk->filename)) {
+	if (adjust_shared_perm(lk->filename.buf)) {
 		int save_errno = errno;
-		error("cannot fix permission bits on %s", lk->filename);
+		error("cannot fix permission bits on %s", lk->filename.buf);
 		rollback_lock_file(lk);
 		errno = save_errno;
 		return -1;
@@ -312,7 +303,7 @@ int reopen_lock_file(struct lock_file *lk)
 		die(_("BUG: reopen a lockfile that is still open"));
 	if (!lk->active)
 		die(_("BUG: reopen a lockfile that has been committed"));
-	lk->fd = open(lk->filename, O_WRONLY);
+	lk->fd = open(lk->filename.buf, O_WRONLY);
 	return lk->fd;
 }
 
@@ -329,14 +320,14 @@ int commit_lock_file(struct lock_file *lk)
 		goto rollback;
 
 	/* remove ".lock": */
-	strbuf_add(&result_file, lk->filename,
-		   strlen(lk->filename) - LOCK_SUFFIX_LEN);
-	err = rename(lk->filename, result_file.buf);
+	strbuf_add(&result_file, lk->filename.buf,
+		   lk->filename.len - LOCK_SUFFIX_LEN);
+	err = rename(lk->filename.buf, result_file.buf);
 	strbuf_reset(&result_file);
 	if (err)
 		goto rollback;
 	lk->active = 0;
-	lk->filename[0] = 0;
+	strbuf_reset(&lk->filename);
 	return 0;
 
 rollback:
@@ -361,7 +352,7 @@ void rollback_lock_file(struct lock_file *lk)
 
 	if (lk->fd >= 0)
 		close_lock_file(lk);
-	unlink_or_warn(lk->filename);
+	unlink_or_warn(lk->filename.buf);
 	lk->active = 0;
-	lk->filename[0] = 0;
+	strbuf_reset(&lk->filename);
 }
diff --git a/read-cache.c b/read-cache.c
index 3d43dda..158241d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2026,10 +2026,10 @@ static int commit_locked_index(struct lock_file *lk)
 	if (alternate_index_output) {
 		if (lk->fd >= 0 && close_lock_file(lk))
 			return -1;
-		if (rename(lk->filename, alternate_index_output))
+		if (rename(lk->filename.buf, alternate_index_output))
 			return -1;
 		lk->active = 0;
-		lk->filename[0] = 0;
+		strbuf_reset(&lk->filename);
 		return 0;
 	} else {
 		return commit_lock_file(lk);
diff --git a/refs.c b/refs.c
index f076f2d..717fc34 100644
--- a/refs.c
+++ b/refs.c
@@ -2551,8 +2551,8 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)
 		 * lockfile name, minus ".lock":
 		 */
 		char *loose_filename = xmemdupz(
-				lock->lk->filename,
-				strlen(lock->lk->filename) - LOCK_SUFFIX_LEN);
+				lock->lk->filename.buf,
+				lock->lk->filename.len - LOCK_SUFFIX_LEN);
 		int err = unlink_or_warn(loose_filename);
 		free(loose_filename);
 		if (err && errno != ENOENT)
@@ -2918,7 +2918,7 @@ int write_ref_sha1(struct ref_lock *lock,
 	    write_in_full(lock->lock_fd, &term, 1) != 1 ||
 	    close_ref(lock) < 0) {
 		int save_errno = errno;
-		error("Couldn't write %s", lock->lk->filename);
+		error("Couldn't write %s", lock->lk->filename.buf);
 		unlock_ref(lock);
 		errno = save_errno;
 		return -1;
diff --git a/shallow.c b/shallow.c
index de07709..33f3c7f 100644
--- a/shallow.c
+++ b/shallow.c
@@ -269,8 +269,8 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
 	if (write_shallow_commits(&sb, 0, extra)) {
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
 			die_errno("failed to write to %s",
-				  shallow_lock->filename);
-		*alternate_shallow_file = shallow_lock->filename;
+				  shallow_lock->filename.buf);
+		*alternate_shallow_file = shallow_lock->filename.buf;
 	} else
 		/*
 		 * is_repository_shallow() sees empty string as "no
@@ -316,7 +316,7 @@ void prune_shallow(int show_only)
 	if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
 			die_errno("failed to write to %s",
-				  shallow_lock.filename);
+				  shallow_lock.filename.buf);
 		commit_lock_file(&shallow_lock);
 	} else {
 		unlink(git_path("shallow"));
-- 
2.1.0

[PATCH v4 29/32] resolve_symlink(): use a strbuf for internal scratch space

[Michael Haggerty]

Aside from shortening and simplifying the code, this removes another
place where the path name length is arbitrarily limited.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index c34bf6c..9413f7a 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -136,44 +136,35 @@ static char *last_path_elm(char *p)
 static char *resolve_symlink(char *p, size_t s)
 {
 	int depth = MAXDEPTH;
+	static struct strbuf link = STRBUF_INIT;
 
 	while (depth--) {
-		char link[PATH_MAX];
-		int link_len = readlink(p, link, sizeof(link));
-		if (link_len < 0) {
-			/* not a symlink anymore */
-			return p;
-		}
-		else if (link_len < sizeof(link))
-			/* readlink() never null-terminates */
-			link[link_len] = '\0';
-		else {
-			warning("%s: symlink too long", p);
-			return p;
-		}
+		if (strbuf_readlink(&link, p, strlen(p)) < 0)
+			break;
 
-		if (is_absolute_path(link)) {
+		if (is_absolute_path(link.buf)) {
 			/* absolute path simply replaces p */
-			if (link_len < s)
-				strcpy(p, link);
+			if (link.len < s)
+				strcpy(p, link.buf);
 			else {
 				warning("%s: symlink too long", p);
-				return p;
+				break;
 			}
 		} else {
 			/*
-			 * link is a relative path, so I must replace the
+			 * link is a relative path, so replace the
 			 * last element of p with it.
 			 */
 			char *r = (char *)last_path_elm(p);
-			if (r - p + link_len < s)
-				strcpy(r, link);
+			if (r - p + link.len < s)
+				strcpy(r, link.buf);
 			else {
 				warning("%s: symlink too long", p);
-				return p;
+				break;
 			}
 		}
 	}
+	strbuf_reset(&link);
 	return p;
 }
 
-- 
2.1.0

[PATCH v4 30/32] resolve_symlink(): take a strbuf parameter

[Michael Haggerty]

Change resolve_symlink() to take a strbuf rather than a string as
parameter.  This simplifies the code and removes an arbitrary pathname
length restriction.  It also means that lock_file's filename field no
longer needs to be initialized to a large size.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 53 +++++++++++++++++++----------------------------------
 1 file changed, 19 insertions(+), 34 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 9413f7a..d78b6bf 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -119,53 +119,40 @@ static char *last_path_elm(char *p)
 #define MAXDEPTH 5
 
 /*
- * p = path that may be a symlink
- * s = full size of p
+ * path contains a path that might be a symlink.
  *
- * If p is a symlink, attempt to overwrite p with a path to the real
- * file or directory (which may or may not exist), following a chain of
- * symlinks if necessary.  Otherwise, leave p unmodified.
+ * If path is a symlink, attempt to overwrite it with a path to the
+ * real file or directory (which may or may not exist), following a
+ * chain of symlinks if necessary.  Otherwise, leave path unmodified.
  *
- * This is a best-effort routine.  If an error occurs, p will either be
- * left unmodified or will name a different symlink in a symlink chain
- * that started with p's initial contents.
- *
- * Always returns p.
+ * This is a best-effort routine.  If an error occurs, path will
+ * either be left unmodified or will name a different symlink in a
+ * symlink chain that started with the original path.
  */
-
-static char *resolve_symlink(char *p, size_t s)
+static void resolve_symlink(struct strbuf *path)
 {
 	int depth = MAXDEPTH;
 	static struct strbuf link = STRBUF_INIT;
 
 	while (depth--) {
-		if (strbuf_readlink(&link, p, strlen(p)) < 0)
+		if (strbuf_readlink(&link, path->buf, path->len) < 0)
 			break;
 
-		if (is_absolute_path(link.buf)) {
+		if (is_absolute_path(link.buf))
 			/* absolute path simply replaces p */
-			if (link.len < s)
-				strcpy(p, link.buf);
-			else {
-				warning("%s: symlink too long", p);
-				break;
-			}
-		} else {
+			strbuf_reset(path);
+		else {
 			/*
 			 * link is a relative path, so replace the
 			 * last element of p with it.
 			 */
-			char *r = (char *)last_path_elm(p);
-			if (r - p + link.len < s)
-				strcpy(r, link.buf);
-			else {
-				warning("%s: symlink too long", p);
-				break;
-			}
+			char *r = last_path_elm(path->buf);
+			strbuf_setlen(path, r - path->buf);
 		}
+
+		strbuf_addbuf(path, &link);
 	}
 	strbuf_reset(&link);
-	return p;
 }
 
 /* Make sure errno contains a meaningful value on error */
@@ -185,16 +172,14 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 		lk->active = 0;
 		lk->owner = 0;
 		lk->on_list = 1;
-		strbuf_init(&lk->filename, PATH_MAX);
+		strbuf_init(&lk->filename, 0);
 		lk->next = lock_file_list;
 		lock_file_list = lk;
 	}
 
 	strbuf_addstr(&lk->filename, path);
-	if (!(flags & LOCK_NODEREF)) {
-		resolve_symlink(lk->filename.buf, lk->filename.alloc);
-		strbuf_setlen(&lk->filename, strlen(lk->filename.buf));
-	}
+	if (!(flags & LOCK_NODEREF))
+		resolve_symlink(&lk->filename);
 	strbuf_addstr(&lk->filename, LOCK_SUFFIX);
 	lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (lk->fd < 0) {
-- 
2.1.0

[PATCH v4 31/32] trim_last_path_elm(): replace last_path_elm()

[Michael Haggerty]

Rewrite last_path_elm() to take a strbuf parameter and to trim off the
last path name element in place rather than returning a pointer to the
beginning of the last path name element.  This simplifies the function
a bit and makes it integrate better with its caller, which is now also
strbuf-based.  Rename the function accordingly.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 38 ++++++++++++++++----------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index d78b6bf..4c6e720 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -86,32 +86,28 @@ static void remove_lock_file_on_signal(int signo)
 }
 
 /*
- * p = absolute or relative path name
+ * path = absolute or relative path name
  *
- * Return a pointer into p showing the beginning of the last path name
- * element.  If p is empty or the root directory ("/"), just return p.
+ * Remove the last path name element from path (leaving the preceding
+ * "/", if any).  If path is empty or the root directory ("/"), set
+ * path to the empty string.
  */
-static char *last_path_elm(char *p)
+static void trim_last_path_elm(struct strbuf *path)
 {
-	/* r starts pointing to null at the end of the string */
-	char *r = strchr(p, '\0');
-
-	if (r == p)
-		return p; /* just return empty string */
-
-	r--; /* back up to last non-null character */
+	int i = path->len;
 
 	/* back up past trailing slashes, if any */
-	while (r > p && *r == '/')
-		r--;
+	while (i && path->buf[i - 1] == '/')
+		i--;
 
 	/*
-	 * then go backwards until I hit a slash, or the beginning of
-	 * the string
+	 * then go backwards until a slash, or the beginning of the
+	 * string
 	 */
-	while (r > p && *(r-1) != '/')
-		r--;
-	return r;
+	while (i && path->buf[i - 1] != '/')
+		i--;
+
+	strbuf_setlen(path, i);
 }
 
 
@@ -141,14 +137,12 @@ static void resolve_symlink(struct strbuf *path)
 		if (is_absolute_path(link.buf))
 			/* absolute path simply replaces p */
 			strbuf_reset(path);
-		else {
+		else
 			/*
 			 * link is a relative path, so replace the
 			 * last element of p with it.
 			 */
-			char *r = last_path_elm(path->buf);
-			strbuf_setlen(path, r - path->buf);
-		}
+			trim_last_path_elm(path);
 
 		strbuf_addbuf(path, &link);
 	}
-- 
2.1.0

[PATCH v4 32/32] Extract a function commit_lock_file_to()

[Michael Haggerty]

commit_locked_index(), when writing to an alternate index file,
duplicates (poorly) the code in commit_lock_file(). And anyway, it
shouldn't have to know so much about the internal workings of lockfile
objects. So extract a new function commit_lock_file_to() that does the
work common to the two functions, and call it from both
commit_lock_file() and commit_locked_index().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/technical/api-lockfile.txt | 14 ++++++++++----
 cache.h                                  |  1 +
 lockfile.c                               | 30 ++++++++++++++++++------------
 read-cache.c                             | 13 +++----------
 4 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index 2514559..3ee4299 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -73,6 +73,12 @@ commit_lock_file::
 	`commit_lock_file()` for a `lock_file` object that is not
 	currently locked.
 
+commit_lock_file_to::
+
+	Like `commit_lock_file()`, except that it takes an explicit
+	`path` argument to which the lockfile should be renamed. The
+	`path` must be on the same filesystem as the lock file.
+
 rollback_lock_file::
 
 	Take a pointer to the `struct lock_file` initialized
@@ -91,10 +97,10 @@ Because the structure is used in an `atexit(3)` handler, its
 storage has to stay throughout the life of the program.  It
 cannot be an auto variable allocated on the stack.
 
-Call `commit_lock_file()` or `rollback_lock_file()` when you are
-done writing to the file descriptor.  If you do not call either
-and simply `exit(3)` from the program, an `atexit(3)` handler
-will close and remove the lockfile.
+Call `commit_lock_file()`, `commit_lock_file_to()`, or
+`rollback_lock_file()` when you are done writing to the file
+descriptor. If you do not call either and simply `exit(3)` from the
+program, an `atexit(3)` handler will close and remove the lockfile.
 
 If you need to close the file descriptor you obtained from
 `hold_lock_file_for_update` function yourself, do so by calling
diff --git a/cache.h b/cache.h
index bea6296..cda91fc 100644
--- a/cache.h
+++ b/cache.h
@@ -589,6 +589,7 @@ extern void unable_to_lock_message(const char *path, int err,
 extern NORETURN void unable_to_lock_die(const char *path, int err);
 extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
+extern int commit_lock_file_to(struct lock_file *, const char *path);
 extern int commit_lock_file(struct lock_file *);
 extern int reopen_lock_file(struct lock_file *);
 extern void update_index_if_able(struct index_state *, struct lock_file *);
diff --git a/lockfile.c b/lockfile.c
index 4c6e720..e54d260 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -53,8 +53,9 @@
  * - Locked, lockfile closed (after close_lock_file()).  Same as the
  *   previous state, except that the lockfile is closed and fd is -1.
  *
- * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a
- *   failed attempt to lock).  In this state:
+ * - Unlocked (after commit_lock_file(), commit_lock_file_to(),
+ *   rollback_lock_file(), or a failed attempt to lock).  In this
+ *   state:
  *   - active is unset
  *   - filename is the empty string (usually, though there are
  *     transitory states in which this condition doesn't hold)
@@ -277,24 +278,16 @@ int reopen_lock_file(struct lock_file *lk)
 	return lk->fd;
 }
 
-int commit_lock_file(struct lock_file *lk)
+int commit_lock_file_to(struct lock_file *lk, const char *path)
 {
-	static struct strbuf result_file = STRBUF_INIT;
 	int save_errno;
-	int err;
 
 	if (!lk->active)
 		die("BUG: attempt to commit unlocked object");
 
 	if (lk->fd >= 0 && close_lock_file(lk))
 		goto rollback;
-
-	/* remove ".lock": */
-	strbuf_add(&result_file, lk->filename.buf,
-		   lk->filename.len - LOCK_SUFFIX_LEN);
-	err = rename(lk->filename.buf, result_file.buf);
-	strbuf_reset(&result_file);
-	if (err)
+	if (rename(lk->filename.buf, path))
 		goto rollback;
 	lk->active = 0;
 	strbuf_reset(&lk->filename);
@@ -307,6 +300,19 @@ rollback:
 	return -1;
 }
 
+int commit_lock_file(struct lock_file *lk)
+{
+	static struct strbuf result_file = STRBUF_INIT;
+	int err;
+
+	/* remove ".lock": */
+	strbuf_add(&result_file, lk->filename.buf,
+		   lk->filename.len - LOCK_SUFFIX_LEN);
+	err = commit_lock_file_to(lk, result_file.buf);
+	strbuf_reset(&result_file);
+	return err;
+}
+
 int hold_locked_index(struct lock_file *lk, int die_on_error)
 {
 	return hold_lock_file_for_update(lk, get_index_file(),
diff --git a/read-cache.c b/read-cache.c
index 158241d..11b80a0 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2023,17 +2023,10 @@ void set_alternate_index_output(const char *name)
 
 static int commit_locked_index(struct lock_file *lk)
 {
-	if (alternate_index_output) {
-		if (lk->fd >= 0 && close_lock_file(lk))
-			return -1;
-		if (rename(lk->filename.buf, alternate_index_output))
-			return -1;
-		lk->active = 0;
-		strbuf_reset(&lk->filename);
-		return 0;
-	} else {
+	if (alternate_index_output)
+		return commit_lock_file_to(lk, alternate_index_output);
+	else
 		return commit_lock_file(lk);
-	}
 }
 
 static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,
-- 
2.1.0

Re: [PATCH v4 00/32] Lockfile correctness and refactoring

[Torsten Bögershausen]

On 2014-09-06 09.50, Michael Haggerty wrote:
> Sorry for the long delay since v3. This version mostly cleans up a
> couple more places where the lockfile object was left in an
> ill-defined state. 
No problem with the delay.
The most important question is if we do the lk->active handling right.
Set it to false as seen as possible, and to true as late as possible,
then die() cleanly.
So the ->acive handling looks (more or less, please see below) and
deserves another critical review, may be.

Instead of commenting each patch, I collected a mixture of small questions
and possible suggestions into a diff file.

diff --git a/lockfile.c b/lockfile.c
index e54d260..7f27ea2 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -18,6 +18,10 @@
  *    Usually, if $FILENAME is a symlink, then it is resolved, and the
  *    file ultimately pointed to is the one that is locked and later
  *    replaced.  However, if LOCK_NODEREF is used, then $FILENAME
+LOCK_NODEREF can be read either as
+LOCK_NODE_REF or LOCK_NO_DEREF
+should we change it ?
+
  *    itself is locked and later replaced, even if it is a symlink.
  *
  * 2. Write the new file contents to the lockfile.
@@ -46,9 +50,18 @@
  *   state:
  *   - the lockfile exists
  *   - active is set
- *   - filename holds the filename of the lockfile
+ *   - filename holds the filename of the lockfile in a strbuf
  *   - fd holds a file descriptor open for writing to the lockfile
  *   - owner holds the PID of the process that locked the file
+question: Why do we need the PID here ?
+Do we open a lock file and do a fork() ?
+And if yes, the child gets a new PID, what happens when the
+child gets a signal ?
+Who "owns" the lockfile, the parent, the child, both ?
+The child has access to all data, the fd is open and can be used,
+why do we not allow a rollback, when the child dies ?
+
+
  *
  * - Locked, lockfile closed (after close_lock_file()).  Same as the
  *   previous state, except that the lockfile is closed and fd is -1.
@@ -57,7 +70,7 @@
  *   rollback_lock_file(), or a failed attempt to lock).  In this
  *   state:
  *   - active is unset
- *   - filename is the empty string (usually, though there are
+ *   - filename is an empty string buffer (usually, though there are
  *     transitory states in which this condition doesn't hold)
  *   - fd is -1
  *   - the object is left registered in the lock_file_list, and
@@ -68,7 +81,7 @@
 
 static struct lock_file *volatile lock_file_list;
 
-static void remove_lock_file(void)
+static void remove_lock_files(void)
 {
     pid_t me = getpid();
 
@@ -79,9 +92,9 @@ static void remove_lock_file(void)
     }
 }
 
-static void remove_lock_file_on_signal(int signo)
+static void remove_lock_files_on_signal(int signo)
 {
-    remove_lock_file();
+    remove_lock_files();
     sigchain_pop(signo);
     raise(signo);
 }
@@ -93,7 +106,7 @@ static void remove_lock_file_on_signal(int signo)
  * "/", if any).  If path is empty or the root directory ("/"), set
  * path to the empty string.
  */
-static void trim_last_path_elm(struct strbuf *path)
+static void trim_last_path_elem(struct strbuf *path)
 {
     int i = path->len;
 
@@ -143,7 +156,7 @@ static void resolve_symlink(struct strbuf *path)
              * link is a relative path, so replace the
              * last element of p with it.
              */
-            trim_last_path_elm(path);
+            trim_last_path_elem(path);
 
         strbuf_addbuf(path, &link);
     }
@@ -153,13 +166,16 @@ static void resolve_symlink(struct strbuf *path)
 /* Make sure errno contains a meaningful value on error */
 static int lock_file(struct lock_file *lk, const char *path, int flags)
 {
+    struct stat st;
+    int mode = 0666;
     if (!lock_file_list) {
         /* One-time initialization */
-        sigchain_push_common(remove_lock_file_on_signal);
-        atexit(remove_lock_file);
+        sigchain_push_common(remove_lock_files_on_signal);
+        atexit(remove_lock_files);
     }
 
-    assert(!lk->active);
+  if (lk->active)
+        die("lk->active %s", path);
 
     if (!lk->on_list) {
         /* Initialize *lk and add it to lock_file_list: */
@@ -167,16 +183,25 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
         lk->active = 0;
         lk->owner = 0;
         lk->on_list = 1;
-        strbuf_init(&lk->filename, 0);
+        strbuf_init(&lk->filename, strlen(path) + LOCK_SUFFIX_LEN);
         lk->next = lock_file_list;
         lock_file_list = lk;
     }
 
+    strbuf_reset(&lk->filename); /* Better to be save */
     strbuf_addstr(&lk->filename, path);
     if (!(flags & LOCK_NODEREF))
         resolve_symlink(&lk->filename);
     strbuf_addstr(&lk->filename, LOCK_SUFFIX);
-    lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
+    /*
+     * adjust_shared_perm() will widen permissions if needed,
+     * otherwise keep permissions restrictive
+     *
+     */
+    if (!stat(path, &st))
+        mode = st.st_mode & 07777;
+
+    lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, mode);
     if (lk->fd < 0) {
         strbuf_reset(&lk->filename);
         return -1;
@@ -268,7 +293,7 @@ int close_lock_file(struct lock_file *lk)
     return close(fd);
 }
 
-int reopen_lock_file(struct lock_file *lk)
+int reopen_lock_file_UNUSED_CAN_IT_BE_REMOVED(struct lock_file *lk)
 {
     if (0 <= lk->fd)
         die(_("BUG: reopen a lockfile that is still open"));
@@ -283,7 +308,7 @@ int commit_lock_file_to(struct lock_file *lk, const char *path)
     int save_errno;
 
     if (!lk->active)
-        die("BUG: attempt to commit unlocked object");
+        die("BUG: attempt to commit unlocked object %s", path);
 
     if (lk->fd >= 0 && close_lock_file(lk))
         goto rollback;
@@ -325,10 +350,12 @@ void rollback_lock_file(struct lock_file *lk)
 {
     if (!lk->active)
         return;
+    lk->active = 0; /* We are going to de-activate,
+                       so active is no longer valid already here ? */
 
     if (lk->fd >= 0)
         close_lock_file(lk);
     unlink_or_warn(lk->filename.buf);
-    lk->active = 0;
+    //lk->active = 0;
     strbuf_reset(&lk->filename);
 }

Re: [PATCH v4 00/32] Lockfile correctness and refactoring

[Michael Haggerty]

On 09/07/2014 04:21 PM, Torsten Bögershausen wrote:
> 
> On 2014-09-06 09.50, Michael Haggerty wrote:
>> Sorry for the long delay since v3. This version mostly cleans up a
>> couple more places where the lockfile object was left in an
>> ill-defined state. 
> No problem with the delay.
> The most important question is if we do the lk->active handling right.
> Set it to false as seen as possible, and to true as late as possible,
> then die() cleanly.
> So the ->acive handling looks (more or less, please see below) and
> deserves another critical review, may be.
> 
> Instead of commenting each patch, I collected a mixture of small questions
> and possible suggestions into a diff file.
> 
> diff --git a/lockfile.c b/lockfile.c
> index e54d260..7f27ea2 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -18,6 +18,10 @@
>   *    Usually, if $FILENAME is a symlink, then it is resolved, and the
>   *    file ultimately pointed to is the one that is locked and later
>   *    replaced.  However, if LOCK_NODEREF is used, then $FILENAME
> +LOCK_NODEREF can be read either as
> +LOCK_NODE_REF or LOCK_NO_DEREF
> +should we change it ?
> +

Good idea.

>   *    itself is locked and later replaced, even if it is a symlink.
>   *
>   * 2. Write the new file contents to the lockfile.
> @@ -46,9 +50,18 @@
>   *   state:
>   *   - the lockfile exists
>   *   - active is set
> - *   - filename holds the filename of the lockfile
> + *   - filename holds the filename of the lockfile in a strbuf

I don't think this is necessary. The point of this list is to describe
the state machine, not the contents of the lock_file structure, so this
detail is only a distraction. And even if a reader is confused, the
compiler will warn if he tries to use the strbuf as if it were a string.

>   *   - fd holds a file descriptor open for writing to the lockfile
>   *   - owner holds the PID of the process that locked the file
> +question: Why do we need the PID here ?
> +Do we open a lock file and do a fork() ?
> +And if yes, the child gets a new PID, what happens when the
> +child gets a signal ?
> +Who "owns" the lockfile, the parent, the child, both ?
> +The child has access to all data, the fd is open and can be used,
> +why do we not allow a rollback, when the child dies ?

Good questions. I will add an explanation of the purpose of the pid in
this docstring.

>   *
>   * - Locked, lockfile closed (after close_lock_file()).  Same as the
>   *   previous state, except that the lockfile is closed and fd is -1.
> @@ -57,7 +70,7 @@
>   *   rollback_lock_file(), or a failed attempt to lock).  In this
>   *   state:
>   *   - active is unset
> - *   - filename is the empty string (usually, though there are
> + *   - filename is an empty string buffer (usually, though there are
>   *     transitory states in which this condition doesn't hold)
>   *   - fd is -1
>   *   - the object is left registered in the lock_file_list, and
> @@ -68,7 +81,7 @@
>  
>  static struct lock_file *volatile lock_file_list;
>  
> -static void remove_lock_file(void)
> +static void remove_lock_files(void)

Good idea. I will rename these two functions.

>  {
>      pid_t me = getpid();
>  
> @@ -79,9 +92,9 @@ static void remove_lock_file(void)
>      }
>  }
>  
> -static void remove_lock_file_on_signal(int signo)
> +static void remove_lock_files_on_signal(int signo)
>  {
> -    remove_lock_file();
> +    remove_lock_files();
>      sigchain_pop(signo);
>      raise(signo);
>  }
> @@ -93,7 +106,7 @@ static void remove_lock_file_on_signal(int signo)
>   * "/", if any).  If path is empty or the root directory ("/"), set
>   * path to the empty string.
>   */
> -static void trim_last_path_elm(struct strbuf *path)
> +static void trim_last_path_elem(struct strbuf *path)

I agree that the old name was bad. I will make it even more explicit:
trim_last_path_component().

>  {
>      int i = path->len;
>  
> @@ -143,7 +156,7 @@ static void resolve_symlink(struct strbuf *path)
>               * link is a relative path, so replace the
>               * last element of p with it.
>               */
> -            trim_last_path_elm(path);
> +            trim_last_path_elem(path);
>  
>          strbuf_addbuf(path, &link);
>      }
> @@ -153,13 +166,16 @@ static void resolve_symlink(struct strbuf *path)
>  /* Make sure errno contains a meaningful value on error */
>  static int lock_file(struct lock_file *lk, const char *path, int flags)
>  {
> +    struct stat st;
> +    int mode = 0666;
>      if (!lock_file_list) {
>          /* One-time initialization */
> -        sigchain_push_common(remove_lock_file_on_signal);
> -        atexit(remove_lock_file);
> +        sigchain_push_common(remove_lock_files_on_signal);
> +        atexit(remove_lock_files);
>      }
>  
> -    assert(!lk->active);
> +  if (lk->active)
> +        die("lk->active %s", path);

OK, but I will use die("BUG:...") since this would be an indication of a
bug in git.

>      if (!lk->on_list) {
>          /* Initialize *lk and add it to lock_file_list: */
> @@ -167,16 +183,25 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>          lk->active = 0;
>          lk->owner = 0;
>          lk->on_list = 1;
> -        strbuf_init(&lk->filename, 0);
> +        strbuf_init(&lk->filename, strlen(path) + LOCK_SUFFIX_LEN);

OK.

>          lk->next = lock_file_list;
>          lock_file_list = lk;
>      }
>  
> +    strbuf_reset(&lk->filename); /* Better to be save */

I agree, but this would be a bug so I'd rather die("BUG:") in this case.

>      strbuf_addstr(&lk->filename, path);
>      if (!(flags & LOCK_NODEREF))
>          resolve_symlink(&lk->filename);
>      strbuf_addstr(&lk->filename, LOCK_SUFFIX);
> -    lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
> +    /*
> +     * adjust_shared_perm() will widen permissions if needed,
> +     * otherwise keep permissions restrictive
> +     *
> +     */
> +    if (!stat(path, &st))
> +        mode = st.st_mode & 07777;
> +
> +    lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, mode);

I think this change is separate from the changes made in this series,
and needs its own justification (i.e., it's not immediately obvious to
me whether the change is an improvement). Would you mind submitting it
as a separate patch?

>      if (lk->fd < 0) {
>          strbuf_reset(&lk->filename);
>          return -1;
> @@ -268,7 +293,7 @@ int close_lock_file(struct lock_file *lk)
>      return close(fd);
>  }
>  
> -int reopen_lock_file(struct lock_file *lk)
> +int reopen_lock_file_UNUSED_CAN_IT_BE_REMOVED(struct lock_file *lk)

Junio added this function recently, without any callers, in 93dcaea2. I
assume he has some diabolical plan for it. Junio?

>  {
>      if (0 <= lk->fd)
>          die(_("BUG: reopen a lockfile that is still open"));
> @@ -283,7 +308,7 @@ int commit_lock_file_to(struct lock_file *lk, const char *path)
>      int save_errno;
>  
>      if (!lk->active)
> -        die("BUG: attempt to commit unlocked object");
> +        die("BUG: attempt to commit unlocked object %s", path);

OK.

>  
>      if (lk->fd >= 0 && close_lock_file(lk))
>          goto rollback;
> @@ -325,10 +350,12 @@ void rollback_lock_file(struct lock_file *lk)
>  {
>      if (!lk->active)
>          return;
> +    lk->active = 0; /* We are going to de-activate,
> +                       so active is no longer valid already here ? */

I think the question is: what should happen if a signal arrives at this
moment? If we set active=0 here, then the signal handler wouldn't clean
up this lockfile at all. But we haven't removed it yet, either. So the
lockfile would get left behind.

So how much longer can we leave active set here?

>  
>      if (lk->fd >= 0)
>          close_lock_file(lk);

close_lock_file() is careful to clear lk->fd *before* closing it, so it
shouldn't be possible for the file to get closed twice [1]. Therefore I
think it is OK to leave lk->active set during the call to close_lock_file().

>      unlink_or_warn(lk->filename.buf);

This line is a bit dangerous. If we call unlink_or_warn() with
lk->active set, then there is a chance that unlink_and_warn() will be
called a second time by a signal handler that runs after this line but
before lk->active is cleared. The first call would delete the lockfile
that we created, but the second call (which might be delayed for a bit
while our signal handler works its way through the lockfile linked-list)
might end up deleting a lockfile that another process created between
our two calls. That would be a bad outcome, but it requires a double
coincidence: our process has to receive a signal at this pessimal moment
*and* another process has to create a lockfile after that event but
before our signal handler has run. And for *real* damage to occur, a
*third* process has to incorrectly acquire the *same* lockfile because
of its incorrect deletion by our process, and it has to make a change
that conflicts with the change that the second process is trying to make.

On the other hand, if we clear lk->active *before* calling
unlink_or_warn(), then the danger is that we receive a signal and never
clean up the lockfile. The result is perhaps not as bad as deleting
another process's lockfile, but it seems to me that it is far more
likely: it can happen regardless of whether another process is racing
with us, let alone a third process is trying to acquire the same lock.

Can anybody think of a reasonable way to make this 100% safe?

If not, I think the code as written is safer than your proposed change.

> -    lk->active = 0;
> +    //lk->active = 0;
>      strbuf_reset(&lk->filename);
>  }
> 
> 

Thanks very much for your feedback!

Michael

[1] Another question (and I think somebody else brought it up) is
whether close_lock_file() should actually close the file *before*
clearing lk->fd. I will address that question elsewhere.

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v4 00/32] Lockfile correctness and refactoring

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> This series applies to the current "master". There is a trivial
> conflict between these changes and "next", and a few not-too-serious
> conflicts between these changes and Ronnie's reference-related series
> in "pu".

The conflicts weren't very pretty as rs/transaction* series
progressed, but I think I got something readable queued at the tip
of 'jch' (which is what I usually run myself and is at somewhere
between 'pu^{/match next}' and 'pu').

I'd appreciate if both of you can double check the result.

> I've figured out how to resolve the conflicts locally. Is
> there some form in which I can put the conflict resolution that would
> help you?

A public tree that shows a suggested conflict resolution, so that I
can try it myself without looking at it first and then compare my
result with yours, would probably be the easiest for both of us.  In
the worst case, I could fetch from such a tree, use rerere-train in
contrib/ and figure out any necessary evil-merges that are not
covered by rerere.

Thanks.

Re: [PATCH v4 00/32] Lockfile correctness and refactoring

[Jeff King]

On Sat, Sep 06, 2014 at 09:50:14AM +0200, Michael Haggerty wrote:

> Sorry for the long delay since v3. This version mostly cleans up a
> couple more places where the lockfile object was left in an
> ill-defined state. Thanks to Johannes Sixt and Torsten Bögershausen
> for their review of v3.
> 
> I believe that this series addresses all of the comments from v1 [1],
> v2 [2], and v3 [3].

This looks pretty good to me overall.

I did coincidentally have an interesting experience with our lockfile
code earlier today, which I'd like to relate.

I was running pack-refs on a repository with a very large number of
loose refs (about 1.8 million). Needless to say, this ran very slowly
and thrashed the disk, as that's almost 7G using 4K inodes. But it did
eventually generate a packed-refs file, at which point it tried to prune
the loose refs.

To do so, we have to lock each ref before removing it (to protect
against a simultaneous update). Each call to lock_ref_sha1_basic
allocates a "struct lock_file", which then gets added to the global
lock_file list. Each one contains a fixed PATH_MAX buffer (4K on this
machine). After we're done updating the ref, we leak the lock_file
struct, since there's no way to remove it from the list.

As a result, git tried to allocate 7G of RAM and got OOM-killed (the
machine had only 8G). In addition to thrashing the disk even harder,
since there was no room left for disk cache while we touched millions of
loose refs. :)

Your change in this series to use a strbuf would make this a lot better.
But I still wonder how hard it would be to just remove lock_file structs
from the global list when they are committed or rolled back. That would
presumably also make the "avoid transitory valid states" patch from your
series a bit easier, too (you could prepare the lockfile in peace, and
then link it in fully formed, and do the opposite when removing it).

I think with your strbuf patch, this leak at least becomes reasonable.
So maybe it's not worth going further. But I'd be interested to hear
your thoughts since you've been touching the area recently.

-Peff

Re: [PATCH v4 00/32] Lockfile correctness and refactoring

[Duy Nguyen]

On Wed, Sep 10, 2014 at 3:13 PM, Jeff King <peff@peff.net> wrote:
> I was running pack-refs on a repository with a very large number of
> loose refs (about 1.8 million). Needless to say, this ran very slowly
> and thrashed the disk, as that's almost 7G using 4K inodes. But it did
> eventually generate a packed-refs file, at which point it tried to prune
> the loose refs.

Urghh.. ref advertisment for fetch/push would be unbelievably large.
Gotta look at the "git protocol v2" again soon..
-- 
Duy

Re: [PATCH v4 00/32] Lockfile correctness and refactoring

[Jeff King]

On Wed, Sep 10, 2014 at 05:25:36PM +0700, Duy Nguyen wrote:

> On Wed, Sep 10, 2014 at 3:13 PM, Jeff King <peff@peff.net> wrote:
> > I was running pack-refs on a repository with a very large number of
> > loose refs (about 1.8 million). Needless to say, this ran very slowly
> > and thrashed the disk, as that's almost 7G using 4K inodes. But it did
> > eventually generate a packed-refs file, at which point it tried to prune
> > the loose refs.
> 
> Urghh.. ref advertisment for fetch/push would be unbelievably large.
> Gotta look at the "git protocol v2" again soon..

Yes, we don't let normal fetchers see these repos. They're only for
holding shared objects and the ref tips to keep them reachable. So we
never fetch from them, even locally. We only fetch to them from normal
repos (and never push to or from them at all).

It's still rather painful just to do normal things, though. Every git
operation loads the whole packed-refs file into memory. I'm biding my
time on the ref-backend patches that are being worked on. :)

-Peff

Re: [PATCH v4 00/32] Lockfile correctness and refactoring

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yes, we don't let normal fetchers see these repos. They're only for
> holding shared objects and the ref tips to keep them reachable.

Are these individual refs have relations to the real world after
they are created?  To ask it another way, let's say that a branch in
a repository, which is using this as a shared object store, caused
one of these refs to be created; now the origin repository rewinds
or deletes that branch---do you do anything to the ref in the shared
object store at that point?

I am wondering if it makes sense to maintain a single ref that
reaches all the commits in this shared object store repository,
instead of keeping these millions of refs.  When you need to make
more objects kept and reachable, create an octopus with the current
tip and tips of all these refs that causes you to wish making these
"more objects kept and reachable".  Obviously that won't work well
if the reason why your current scheme uses refs is because you
adjust individual refs to prune some objects---hence the first
question in this message.

Re: [PATCH v4 00/32] Lockfile correctness and refactoring

[Jeff King]

On Wed, Sep 10, 2014 at 09:51:03AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Yes, we don't let normal fetchers see these repos. They're only for
> > holding shared objects and the ref tips to keep them reachable.
> 
> Are these individual refs have relations to the real world after
> they are created?  To ask it another way, let's say that a branch in
> a repository, which is using this as a shared object store, caused
> one of these refs to be created; now the origin repository rewinds
> or deletes that branch---do you do anything to the ref in the shared
> object store at that point?

Yes, we fetch from them before doing any maintenance in the shared
repository (like running repack). That's how objects migrate into the
shared repository, as well.

> I am wondering if it makes sense to maintain a single ref that
> reaches all the commits in this shared object store repository,
> instead of keeping these millions of refs.  When you need to make
> more objects kept and reachable, create an octopus with the current
> tip and tips of all these refs that causes you to wish making these
> "more objects kept and reachable".  Obviously that won't work well
> if the reason why your current scheme uses refs is because you
> adjust individual refs to prune some objects---hence the first
> question in this message.

Exactly. You could do this if you threw away and re-made the octopus
after each fetch (and then threw away the individual branches that went
into it). For that matter, if all you really want are the tips for
reachability, you can basically run "for-each-ref | sort -u"; most of
these refs are tags that are duplicated between each fork.

However, having the individual tips does make some things easier. If I
only keep unique tips and I drop a tip from fork A, I would then need to
check every other fork to see if any other fork has the same tip. OTOH,
that means visiting N packed-refs files, each with (let's say) 3000
refs. As opposed to dealing with a packed-refs file with N*3000 refs. So
it's really not that different.

We also use the individual ref tips for packing. They factor into the
bitmap selection, and we have some patches (which I've been meaning to
upstream for a while now) to make delta selections in the shared-object
repository that will have a high chance of reuse in clones of individual
forks. And it's useful to query them for various reasons (e.g., "who is
referencing this object?").

There a lot of different ways to do it, and the giant refs file is a
pain (not to mention writing objects to disk in the forks, and then
migrating them separately to the shared storage). But doing it this way
means that the forks and the shared-object repository are all real
first-class git repositories. We follow the usual object reachability
guarantees, and it's safe to run any stock git command on them.

I am leaning towards a system where the shared-object repository is a
pseudo-repository, forks actually write directly into the shared object
store (probably with a symlinked objects/ directory), and implementing a
ref backend that generates a virtual mapping on the fly (e.g., all refs in
"../foo.git" appear as "refs/remotes/foo/*" in the pseudo-repository.
I'm watching the refs-backend work anxiously. :)

-Peff

Re: [PATCH v4 00/32] Lockfile correctness and refactoring

[Michael Haggerty]

On 09/10/2014 09:11 PM, Jeff King wrote:
> On Wed, Sep 10, 2014 at 09:51:03AM -0700, Junio C Hamano wrote:
> 
>> Jeff King <peff@peff.net> writes:
>>
>>> Yes, we don't let normal fetchers see these repos. They're only for
>>> holding shared objects and the ref tips to keep them reachable.
>>
>> Are these individual refs have relations to the real world after
>> they are created?  To ask it another way, let's say that a branch in
>> a repository, which is using this as a shared object store, caused
>> one of these refs to be created; now the origin repository rewinds
>> or deletes that branch---do you do anything to the ref in the shared
>> object store at that point?
> 
> Yes, we fetch from them before doing any maintenance in the shared
> repository (like running repack). That's how objects migrate into the
> shared repository, as well.
> 
>> I am wondering if it makes sense to maintain a single ref that
>> reaches all the commits in this shared object store repository,
>> instead of keeping these millions of refs.  When you need to make
>> more objects kept and reachable, create an octopus with the current
>> tip and tips of all these refs that causes you to wish making these
>> "more objects kept and reachable".  Obviously that won't work well
>> if the reason why your current scheme uses refs is because you
>> adjust individual refs to prune some objects---hence the first
>> question in this message.
> 
> Exactly. You could do this if you threw away and re-made the octopus
> after each fetch (and then threw away the individual branches that went
> into it). For that matter, if all you really want are the tips for
> reachability, you can basically run "for-each-ref | sort -u"; most of
> these refs are tags that are duplicated between each fork.
> 
> However, having the individual tips does make some things easier. If I
> only keep unique tips and I drop a tip from fork A, I would then need to
> check every other fork to see if any other fork has the same tip. OTOH,
> that means visiting N packed-refs files, each with (let's say) 3000
> refs. As opposed to dealing with a packed-refs file with N*3000 refs. So
> it's really not that different.

I think it would make more sense to have one octopus per fork, rather
than one octopus for all of the forks. The octopus for a fork could be
discarded and re-created every time that fork changed, without having to
worry about which of the old tips is still reachable from another fork.
In fact, if you happen to know that a particular update to a fork is
pure fast-forward, you could update the fork octopus by merging the
changed tips into the old fork octopus without having to re-knit all of
the unchanged tips together again.

The shared repository would need one reference per fork--still much
better than a reference for every reference in every fork. If even that
is too many references, you could knit the fork octopuses into an
overall octopus for all forks. But then updating that octopus for a
change to a fork would become more difficult, because you would have to
read the octopuses for the N-1 unchanged forks from the old octopus and
knit those together with the new octopus for the modified fork.

But all this imagineering doesn't mitigate the other reasons you list
below for not wanting to guarantee reachability using this trick.

> We also use the individual ref tips for packing. They factor into the
> bitmap selection, and we have some patches (which I've been meaning to
> upstream for a while now) to make delta selections in the shared-object
> repository that will have a high chance of reuse in clones of individual
> forks. And it's useful to query them for various reasons (e.g., "who is
> referencing this object?").
> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v4 00/32] Lockfile correctness and refactoring

[Michael Haggerty]

On 09/10/2014 06:51 PM, Junio C Hamano wrote:
> [...]
> I am wondering if it makes sense to maintain a single ref that
> reaches all the commits in this shared object store repository,
> instead of keeping these millions of refs.  When you need to make
> more objects kept and reachable, create an octopus with the current
> tip and tips of all these refs that causes you to wish making these
> "more objects kept and reachable".  Obviously that won't work well
> if the reason why your current scheme uses refs is because you
> adjust individual refs to prune some objects---hence the first
> question in this message.

This is an interesting idea.

I wouldn't trust git to handle efficiently commits with thousands of
parents, but it would be easy to knit a huge number of tips together
using multiple layers of, say, 16-parent octopus merges.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v4 00/32] Lockfile correctness and refactoring

[Michael Haggerty]

On 09/10/2014 10:13 AM, Jeff King wrote:
> [...]
> I did coincidentally have an interesting experience with our lockfile
> code earlier today, which I'd like to relate.
> 
> I was running pack-refs on a repository with a very large number of
> loose refs (about 1.8 million). [...] Each call to lock_ref_sha1_basic
> allocates a "struct lock_file", which then gets added to the global
> lock_file list. Each one contains a fixed PATH_MAX buffer (4K on this
> machine). After we're done updating the ref, we leak the lock_file
> struct, since there's no way to remove it from the list.
> 
> As a result, git tried to allocate 7G of RAM and got OOM-killed (the
> machine had only 8G). In addition to thrashing the disk even harder,
> since there was no room left for disk cache while we touched millions of
> loose refs. :)
> 
> Your change in this series to use a strbuf would make this a lot better.
> But I still wonder how hard it would be to just remove lock_file structs
> from the global list when they are committed or rolled back. That would
> presumably also make the "avoid transitory valid states" patch from your
> series a bit easier, too (you could prepare the lockfile in peace, and
> then link it in fully formed, and do the opposite when removing it).
> 
> I think with your strbuf patch, this leak at least becomes reasonable.
> So maybe it's not worth going further. But I'd be interested to hear
> your thoughts since you've been touching the area recently.

I've thought about this too, but it didn't seem to be worth the effort.
(Though your use case certainly adds a bit of justification.)

To make that change, we would have to remove entries from the list of
lock_file objects in a way that the code can be interrupted at any time
by a signal while leaving it in a state that is traversable by the
signal handler.

I think that can be done pretty easily with a singly-linked list. But
with a singly-linked list, we would have to iterate through the list to
find the node that needs to be removed. This could get expensive if
there are a lot of nodes in the list (see below).

So we would probably want to switch to using a doubly-linked list. I
think this would also be fairly simple, given that the signal handler
only needs to iterate through the list in a single direction. You'd just
have to be careful about adjusting the pointers in the right order to
let (say) a forwards traversal always work.

Then the callers who use heap-allocated lock_file objects would have to
be changed to free them when they're done with them, probably using a
special function that releases the strbufs, too. Callers using
statically-allocated lock_file objects would probably not have to be
changed.

But...

The ref-transaction code is, I think, moving in the direction of
updating all references in a single transaction. This means that we
would need to hold locks for all of the references at once anyway. So it
might be all for naught.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v4 00/32] Lockfile correctness and refactoring

[Jeff King]

On Fri, Sep 12, 2014 at 04:21:09PM +0200, Michael Haggerty wrote:

> > But I still wonder how hard it would be to just remove lock_file structs
> > from the global list when they are committed or rolled back.
> [...]
>
> To make that change, we would have to remove entries from the list of
> lock_file objects in a way that the code can be interrupted at any time
> by a signal while leaving it in a state that is traversable by the
> signal handler.
> 
> I think that can be done pretty easily with a singly-linked list. But
> with a singly-linked list, we would have to iterate through the list to
> find the node that needs to be removed. This could get expensive if
> there are a lot of nodes in the list (see below).

Yes, I considered that, but noticed that if we actually cleaned up
closed files, the list would not grow to more than a handful of entries.
But...

> The ref-transaction code is, I think, moving in the direction of
> updating all references in a single transaction. This means that we
> would need to hold locks for all of the references at once anyway. So it
> might be all for naught.

That nullifies the whole discussion. Besides the list-traversal thing
above, it would mean that we literally _do_ have all of the lockfiles
open at once. So cleaning up after ourselves would be nice, but it would
not impact the peak memory usage, which would necessarily have one
allocated struct per ref.

The use of a strbuf is probably a big enough change to save us there.
This case was pathological for a few reasons:

  1. A ridiculous number of refs in the repository.

  2. Touching a large number of them in sequence (via pack-refs).

  3. Allocating a 4K buffer per object.

For (3), if the average allocation is dropped even to 400 bytes (which
would accommodate quite a long pathname), that would reduce the memory
usage to ~700MB. Not amazing, but enough not to tip over most modern
machines.

-Peff