[PATCH v5 00/35] Lockfile correctness and refactoring

[PATCH v5 00/35] Lockfile correctness and refactoring

[Michael Haggerty]

Next iteration of my lockfile fixes and refactoring. Thanks to
Torsten Bögershausen, Junio, Peff, Ronnie Sahlberg, and Johannes Sixt
for their comments about v4.

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

Changes since v4:

* Rebase to current master.

* Explain lock_file ownership and the point of its pid field.

* Correct the log message for

    "delete_ref_loose(): don't muck around in the lock_file's filename"

* Replace an assert() with a die("BUG:").

* Add a sanity check that lk->filename is empty before reusing a
  lock_file object.

* Initialize the length of lk->filename more intelligently.

* Rename trim_last_path_elm() to trim_last_path_component().

* Make some die() messages more informative.

* Add some sanity checks to commit_lock_file().

* Rename REF_NODEREF to REF_NO_DEREF.

* Rename some static functions:
  * remove_lock_file() -> remove_lock_files()
  * remove_lock_file_on_signal() -> remove_lock_files_on_signal()

* Add a function get_locked_file_path(), to isolate callers a bit more
  from the innards of lock_file.

There are some conflicts with branch rs/ref-transaction; I pushed my
proposed merge of these two branches to

    https://github.com/mhagger/git.git lock-correctness-v5-rs-ref-transaction

[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
[4] http://thread.gmane.org/gmane.comp.version-control.git/256564

Michael Haggerty (35):
  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_component(): replace last_path_elm()
  Extract a function commit_lock_file_to()
  Rename LOCK_NODEREF to LOCK_NO_DEREF
  lockfile.c: rename static functions
  get_locked_file_path(): new function

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

-- 
2.1.0

[PATCH v5 01/35] 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>
Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>
---
 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 dfa1a56..73ba5d0 100644
--- a/cache.h
+++ b/cache.h
@@ -582,7 +582,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 2ce5d69..8cb3539 100644
--- a/refs.c
+++ b/refs.c
@@ -2167,7 +2167,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

Re: [PATCH v5 01/35] unable_to_lock_die(): rename function from unable_to_lock_index_die()

[Jonathan Nieder]

Michael Haggerty wrote:

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

Makes sense.

[...]
>  builtin/update-index.c | 2 +-
>  cache.h                | 2 +-
>  lockfile.c             | 6 +++---
>  refs.c                 | 2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[PATCH v5 02/35] 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 v5 02/35] api-lockfile: expand the documentation

[Jonathan Nieder]

Michael Haggerty wrote:

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

Thanks.

[...]
> --- 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
> ++
> +--

Context: this document has structure

	lockfile API
	============

	Explanation of purpose (nice!).

	The functions
	-------------

	Quick descriptions of each of the four functions
	`hold_lock_file_for_update`, `commit_lock_file`,
	`rollback_lock_file`, `close_lock_file`.

	Reminder about lifetime of the lock_file structure.

	Description of cleanup convention (thou shalt either
	commit or roll back; if you forget to, the atexit
	handler will roll back for you).

	Long warning about the harder use cases.  The above
	"thou shalt" was a lie --- you can also
	close_lock_file if you know what you're doing
	[jn: why is that function part of the public API?].

What's nice about the existing structure is that you can get
a sense of how to use the API at a glance.  Would there be a
way to add this extra information while preserving that property?

E.g.:

	lockfile API
	============

	Nice brief explanation of purpose ("is this the API
	I want to use?"), as before.

	Calling sequence
	----------------

	The caller:

	* Allocates a variable `struct lock_file lock` in the bss
	section or heap.  Because the `lock_file` 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.

	* Attempts to create a lockfile by passing that variable and
	the filename of the final destination (e.g. `$GIT_DIR/index`)
	to `hold_lock_file_for_update` or `hold_lock_file_for_append`.
	+
	If the `die_on_error` flag is true, git dies if a lock is
	already taken for the file.

	* Writes new content for the destination file by writing to
	`lock->fd`.

	When finished writing, the caller can:

	* Close the file descriptor and rename the lockfile to
	its final destination by calling `commit_lock_file`.

	* Close the file descriptor and remove the lockfile by
	calling `rollback_lock_file`.

	* Close the file descriptor without removing or renaming
	the lockfile by calling `close_lock_file`.

	If you do not call one of `commit_lock_file`,
	`rollback_lock_file`, and `close_lock_file` and instead
	simply `exit(3)` from the program, an `atexit(3)` handler will
	close and remove the lockfile.

	You should never call `close(2)` on `lock->fd` yourself~
	Otherwise the ...

	Error handling
	--------------

	Functions return 0 on success, -1 on failure.  errno is?
	isn't? meaningful on error.

	... description of unable_to_lock_error and unable_to_lock_die
	here ...

	Flags
	-----

	LOCK_NODEREF::

		Usually symbolic links in the destination path are
		resolved and the lockfile is created by adding ".lock"
		to the resolved path.  If `LOCK_NODEREF` is set, then
		the lockfile is created by adding ".lock" to the path
		argument itself.

What is the user-visible effect of that flag?  When would I want
to pass that flag, and when wouldn't I?

	LOCK_DIE_ON_ERROR::

		If a lock is already taken for the file, `die()` with
		an error message.  If this option is not specified,
		trying to hold a lock file that is already taken will
		return -1 to the caller.

Sensible?
Jonathan

Re: [PATCH v5 02/35] api-lockfile: expand the documentation

[Michael Haggerty]

On 09/16/2014 10:25 PM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> Document a couple more functions and the flags argument as used by
>> hold_lock_file_for_update() and hold_lock_file_for_append().
> 
> Thanks.
> 
> [...]
>> --- 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
>> ++
>> +--
> 
> Context: this document has structure
> 
> 	lockfile API
> 	============
> 
> 	Explanation of purpose (nice!).
> 
> 	The functions
> 	-------------
> 
> 	Quick descriptions of each of the four functions
> 	`hold_lock_file_for_update`, `commit_lock_file`,
> 	`rollback_lock_file`, `close_lock_file`.
> 
> 	Reminder about lifetime of the lock_file structure.
> 
> 	Description of cleanup convention (thou shalt either
> 	commit or roll back; if you forget to, the atexit
> 	handler will roll back for you).
> 
> 	Long warning about the harder use cases.  The above
> 	"thou shalt" was a lie --- you can also
> 	close_lock_file if you know what you're doing
> 	[jn: why is that function part of the public API?].
> 
> What's nice about the existing structure is that you can get
> a sense of how to use the API at a glance.  Would there be a
> way to add this extra information while preserving that property?
> 
> E.g.:
> 
> 	lockfile API
> 	============
> 
> 	Nice brief explanation of purpose ("is this the API
> 	I want to use?"), as before.
> 
> 	Calling sequence
> 	----------------
> 
> 	The caller:
> 
> 	* Allocates a variable `struct lock_file lock` in the bss
> 	section or heap.  Because the `lock_file` 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.
> 
> 	* Attempts to create a lockfile by passing that variable and
> 	the filename of the final destination (e.g. `$GIT_DIR/index`)
> 	to `hold_lock_file_for_update` or `hold_lock_file_for_append`.
> 	+
> 	If the `die_on_error` flag is true, git dies if a lock is
> 	already taken for the file.
> 
> 	* Writes new content for the destination file by writing to
> 	`lock->fd`.
> 
> 	When finished writing, the caller can:
> 
> 	* Close the file descriptor and rename the lockfile to
> 	its final destination by calling `commit_lock_file`.
> 
> 	* Close the file descriptor and remove the lockfile by
> 	calling `rollback_lock_file`.
> 
> 	* Close the file descriptor without removing or renaming
> 	the lockfile by calling `close_lock_file`.
> 
> 	If you do not call one of `commit_lock_file`,
> 	`rollback_lock_file`, and `close_lock_file` and instead
> 	simply `exit(3)` from the program, an `atexit(3)` handler will
> 	close and remove the lockfile.
> 
> 	You should never call `close(2)` on `lock->fd` yourself~
> 	Otherwise the ...
> 
> 	Error handling
> 	--------------
> 
> 	Functions return 0 on success, -1 on failure.  errno is?
> 	isn't? meaningful on error.
> 
> 	... description of unable_to_lock_error and unable_to_lock_die
> 	here ...
> 
> 	Flags
> 	-----
> 
> 	LOCK_NODEREF::
> 
> 		Usually symbolic links in the destination path are
> 		resolved and the lockfile is created by adding ".lock"
> 		to the resolved path.  If `LOCK_NODEREF` is set, then
> 		the lockfile is created by adding ".lock" to the path
> 		argument itself.
> 
> What is the user-visible effect of that flag?  When would I want
> to pass that flag, and when wouldn't I?
> 
> 	LOCK_DIE_ON_ERROR::
> 
> 		If a lock is already taken for the file, `die()` with
> 		an error message.  If this option is not specified,
> 		trying to hold a lock file that is already taken will
> 		return -1 to the caller.
> 
> Sensible?
> Jonathan

OK, in the next reroll I will revise the documentation pretty
thoroughly. Please let me know what you think.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH v5 03/35] 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>
Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>
---
 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 v5 03/35] rollback_lock_file(): do not clear filename redundantly

[Jonathan Nieder]

Michael Haggerty wrote:

> It is only necessary to clear the lock_file's filename field if it was
> not already clear.
[...]
> --- 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;

Now that it does nothing when lk->filename[0] == '\0', this could be
de-indented a little:

	if (!lk->filename[0])
		return;
	if (lk->fd >= 0)
		close(lk->fd);
	unlink_or_warn(lk->filename);
	lk->filename[0] = 0;

This could technically double-close if interrupted by a signal that
tries to remove the file again, right?  Should this use
close_lock_file which sets fd to -1 before closing?

	if (!lk->filename[0])
		return;
	close_lock_file(lk);
	unlink_or_warn(lk->filename);
	lk->filename[0] = 0;

With or without such changes,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[PATCH v5 04/35] 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>
Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>
---
 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 v5 04/35] rollback_lock_file(): exit early if lock is not active

[Jonathan Nieder]

Michael Haggerty wrote:

> Eliminate a layer of nesting.

Oh --- guess I should have been more patient. :)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[PATCH v5 05/35] 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>
Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>
---
 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

Re: [PATCH v5 05/35] rollback_lock_file(): set fd to -1

[Jonathan Nieder]

Michael Haggerty wrote:

> --- 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);

Doesn't need to be guarded by the 'if'.

Re: [PATCH v5 05/35] rollback_lock_file(): set fd to -1

[Jonathan Nieder]

Jonathan Nieder wrote:
> Michael Haggerty wrote:

>> --- 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);
>
> Doesn't need to be guarded by the 'if'.

Err, yes it does.

Why doesn't close_lock_file bail out early when fd < 0?

In any case,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH v5 05/35] rollback_lock_file(): set fd to -1

[Michael Haggerty]

On 09/16/2014 10:39 PM, Jonathan Nieder wrote:
> Jonathan Nieder wrote:
>> Michael Haggerty wrote:
> 
>>> --- 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);
>>
>> Doesn't need to be guarded by the 'if'.
> 
> Err, yes it does.
> 
> Why doesn't close_lock_file bail out early when fd < 0?

OK, I will change close_lock_file() to exit (with no error) when fd < 0.
Then this "if" can also go away.

Thanks,
Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH v5 06/35] 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 v5 06/35] lockfile: unlock file if lockfile permissions cannot be adjusted

[Jonathan Nieder]

Michael Haggerty wrote:

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

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[PATCH v5 07/35] 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 v5 07/35] hold_lock_file_for_append(): release lock on errors

[Jonathan Nieder]

Michael Haggerty wrote:

> --- 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);

Makes sense.

Now that I'm here, I wonder a little at the error convention.  If the
caller doesn't pass LOCK_DIE_ON_ERROR, are they supposed to be able to
use unable_to_lock_message?  What errno would they pass in the err
parameter?  Would callers want handle failure to acquire a lock
differently from other errors (e.g., by sleeping and trying again),
and if not, what is the optionally-die behavior in hold_lock_file
about?

In any case,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH v5 07/35] hold_lock_file_for_append(): release lock on errors

[Michael Haggerty]

On 09/16/2014 10:48 PM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> --- 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);
> 
> Makes sense.
> 
> Now that I'm here, I wonder a little at the error convention.  If the
> caller doesn't pass LOCK_DIE_ON_ERROR, are they supposed to be able to
> use unable_to_lock_message?  What errno would they pass in the err
> parameter?  Would callers want handle failure to acquire a lock
> differently from other errors (e.g., by sleeping and trying again),
> and if not, what is the optionally-die behavior in hold_lock_file
> about?

The same applies to hold_lock_file_for_update(), so I'll discuss both at
once:

Most callers do pass LOCK_DIE_ON_ERROR. Of the ones that don't, a couple
appear to want to emit more meaningful error messages. A couple don't
die at all but return an error code to their caller. At least one
(add_to_alternates_file()) calls die_errno().

As it happens, hold_lock_file_for_append() sometimes overwrites errno
before it returns. I will add a patch on top of this series that fixes that.

I don't see any callers that retry, though I've thought about
implementing that in some places. But it's outside of the scope of this
patch series.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH v5 08/35] 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

Re: [PATCH v5 08/35] lock_file(): always add lock_file object to lock_file_list

[Jonathan Nieder]

Michael Haggerty wrote:

> 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.

Nit: commit messages usually use present tense for current behavior
(and imperative for the behavior after the patch).

More importantly, the above + the diffstat don't leave me very happy
about the change.

Can you spell out more about the intended effect?  E.g., is this about
making sure other code is appropriately exercised to tolerate the
signal handler even when there are a lot of errors (which should make
debugging easier) or something?

Re: [PATCH v5 08/35] lock_file(): always add lock_file object to lock_file_list

[Michael Haggerty]

On 09/16/2014 10:57 PM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> 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.
> 
> Nit: commit messages usually use present tense for current behavior
> (and imperative for the behavior after the patch).

OK, will change.

> More importantly, the above + the diffstat don't leave me very happy
> about the change.
> 
> Can you spell out more about the intended effect?  E.g., is this about
> making sure other code is appropriately exercised to tolerate the
> signal handler even when there are a lot of errors (which should make
> debugging easier) or something?

This patch is mostly about two things:

* Making the state diagram for lock_file objects easier to document and
reason about.

* Defining a clear moment when the lock_file object is initialized,
early in the function. This will be useful later when its filename field
is turned into a strbuf and requires nontrivial initialization before it
can even be used to construct the filename of the lockfile.

By "diffstat" I assume you mean that some lines were added to the
initialization. This is also to make the state diagram of lock_objects
very clear.

I will rewrite the commit message to try to explain the commit's purpose
better. Let me know if you still think that the commit is unjustified.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v5 08/35] lock_file(): always add lock_file object to lock_file_list

[Torsten Bögershausen]

On 09/16/2014 09:33 PM, Michael Haggerty wrote:
[]
>
> 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;
Does it makes sense to change the order here:

Do the full initialization, and once that is completed, set on_list = 1
+		lk->filename[0] = 0;
+		lk->on_list = 1;

Re: [PATCH v5 08/35] lock_file(): always add lock_file object to lock_file_list

[Michael Haggerty]

On 09/18/2014 06:32 AM, Torsten Bögershausen wrote:
> On 09/16/2014 09:33 PM, Michael Haggerty wrote:
> []
>>
>> 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;
> Does it makes sense to change the order here:
> 
> Do the full initialization, and once that is completed, set on_list = 1
> +		lk->filename[0] = 0;
> +		lk->on_list = 1;

From a functional standpoint, it doesn't matter. This function is the
only place that uses on_list, and it is basically only used to make sure
that each lock_file structure is initialized and registered in
lock_file_list exactly once. In particular, the signal handling code
doesn't care about the on_list field.

So the only important timing requirement WRT on_list is that it be set
before this function is called again with the same lock_file object. But
any code that would call this function twice, simultaneously, with the
same lock_file argument would be broken far more seriously than could be
fixed by changing the order that the fields are initialized.

But I guess you are right that it looks more natural to set this field
only after all of the initialization is done. I will make the change.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH v5 09/35] 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>
Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>
---
 lockfile.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/lockfile.c b/lockfile.c
index 00c972c..b6fe848 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -4,6 +4,63 @@
 #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 is owned by the process that created it. The lock_file
+ * object has an "owner" field that records its owner. This field is
+ * used to prevent a forked process from closing a lock_file of its
+ * parent.
+ *
+ * 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 v5 09/35] lockfile.c: document the various states of lock_file objects

[Jonathan Nieder]

Michael Haggerty wrote:

> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -4,6 +4,63 @@
>  #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:

This overlaps a lot with the API doc, which makes me worry a little
about it going out of date.  Would improving the API doc help, or if
aspects are especially relevant here, is there a way to summarize them
more quickly to avoid some of the redundancy?

[...]
> + * A lock_file object can be in several states:

Would it make sense for this information to go near the definition of
'struct lock_file'?  That's where I'd start if I were looking for
information on what states a lock_file can be in.

My two cents,
Jonathan

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

[Michael Haggerty]

On 09/16/2014 11:03 PM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> --- a/lockfile.c
>> +++ b/lockfile.c
>> @@ -4,6 +4,63 @@
>>  #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:
> 
> This overlaps a lot with the API doc, which makes me worry a little
> about it going out of date.  Would improving the API doc help, or if
> aspects are especially relevant here, is there a way to summarize them
> more quickly to avoid some of the redundancy?
> 
> [...]
>> + * A lock_file object can be in several states:
> 
> Would it make sense for this information to go near the definition of
> 'struct lock_file'?  That's where I'd start if I were looking for
> information on what states a lock_file can be in.

I agree with your point about overlap. I will split the documentation
into two parts with less redundancy:

* Documentation/technical/api-lockfile.txt: How to use the API.

* lockfile.{c,h}: Internal implementation details.

I think the implementation details would get lost among the thousand
things in cache.h. So instead, I will add a commit on top of the patch
series that splits out a lockfile.h header file (which I think is a good
idea anyway), and moves the internal documentation there. Sound good?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

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

[Jonathan Nieder]

Michael Haggerty wrote:

> I agree with your point about overlap. I will split the documentation
> into two parts with less redundancy:
>
> * Documentation/technical/api-lockfile.txt: How to use the API.
>
> * lockfile.{c,h}: Internal implementation details.
>
> I think the implementation details would get lost among the thousand
> things in cache.h. So instead, I will add a commit on top of the patch
> series that splits out a lockfile.h header file (which I think is a good
> idea anyway), and moves the internal documentation there. Sound good?

Yep, a separate lockfile.h header file sounds sensible to me.

Thanks,
Jonathan

[PATCH v5 10/35] 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 73ba5d0..e4e7c56 100644
--- a/cache.h
+++ b/cache.h
@@ -570,6 +570,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 b6fe848..99a774b 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -181,10 +181,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 */
@@ -209,7 +210,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();
@@ -319,7 +320,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 8cb3539..53cd4fb 100644
--- a/refs.c
+++ b/refs.c
@@ -79,7 +79,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;
 }
@@ -2551,11 +2552,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 v5 10/35] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN

[Jonathan Nieder]

Michael Haggerty wrote:

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

Seems like a symptom of the API leaving out a useful helper (e.g.,
something that strips off the lock suffix and returns a memdupz'd
filename).

[...]
> --- a/cache.h
> +++ b/cache.h
> @@ -570,6 +570,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

My suspicion is that error handling would be better if fewer callers
outside of lockfile.c did the '- LOCK_SUFFIX_LEN' dance, so this seems
like a step in the wrong direction.

Adding constants in lockfile.c would make sense, though.

Thanks,
Jonathan

Re: [PATCH v5 10/35] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN

[Michael Haggerty]

On 09/16/2014 11:05 PM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> There are a few places that use these values, so define constants for
>> them.
> 
> Seems like a symptom of the API leaving out a useful helper (e.g.,
> something that strips off the lock suffix and returns a memdupz'd
> filename).

I will add a function get_locked_file_path().

> [...]
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -570,6 +570,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
> 
> My suspicion is that error handling would be better if fewer callers
> outside of lockfile.c did the '- LOCK_SUFFIX_LEN' dance, so this seems
> like a step in the wrong direction.
> 
> Adding constants in lockfile.c would make sense, though.

I agree that other call sites shouldn't be grubbing around in the
lock_file data structure. But with the addition of
get_locked_file_path(), the only remaining user of these constants is in
refs.c, in check_refname_component():

	if (cp - refname >= LOCK_SUFFIX_LEN &&
	    !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))
		return -1; /* Refname ends with ".lock". */

I think it is forgivable there.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

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

[Michael Haggerty]

It's bad manners. Especially since there could be a signal during the
call to unlink_or_warn(), in which case the signal handler will see
the wrong filename and delete the reference file, leaving the lockfile
behind.

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 53cd4fb..11c76ec 100644
--- a/refs.c
+++ b/refs.c
@@ -2551,12 +2551,15 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 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 v5 11/35] delete_ref_loose(): don't muck around in the lock_file's filename

[Jonathan Nieder]

Michael Haggerty wrote:

> It's bad manners. Especially since there could be a signal during the
> call to unlink_or_warn(), in which case the signal handler will see
> the wrong filename and delete the reference file, leaving the lockfile
> behind.
>
> So make our own copy to work with.

Nice.  Could this be a helper, to help others avoid a PATH_MAX
sized buffer too?

[...]
> --- a/refs.c
> +++ b/refs.c
> @@ -2551,12 +2551,15 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
>  static int delete_ref_loose(struct ref_lock *lock, int flag)
[...]
> +		/*
> +		 * 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);

Ronnie mentioned that this would misbehave in the
!lock->lk->filename[0] case, which shouldn't come up because
lock_ref_sha1_basic dies on error but seems worth futureproofing
against.  Maybe something like

	assert(lock->lk->filename[0]);

would be enough to make the assumption obvious for people reading.

Otherwise looks good.

Thanks,
Jonathan

[PATCH v5 12/35] 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 41f481b..d4228c9 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

Re: [PATCH v5 12/35] prepare_index(): declare return value to be (const char *)

[Jonathan Nieder]

Michael Haggerty wrote:

> 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(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

I wonder if we should just bite the bullet and make lock_file an
opaque struct with accessors for int fd and const char *filename.

[PATCH v5 13/35] 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>
Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 11c76ec..4f313bc 100644
--- a/refs.c
+++ b/refs.c
@@ -2217,7 +2217,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

[PATCH v5 14/35] 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>
Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>
---
 lockfile.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 99a774b..911f123 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -212,19 +212,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 v5 14/35] lock_file(): exit early if lockfile cannot be opened

[Jonathan Nieder]

Michael Haggerty wrote:

>  lockfile.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[PATCH v5 15/35] 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 911f123..1c85b18 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -68,12 +68,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

Re: [PATCH v5 15/35] remove_lock_file(): call rollback_lock_file()

[Jonathan Nieder]

Michael Haggerty wrote:

>  lockfile.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)

Nice.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[PATCH v5 16/35] 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 1c85b18..9e1cdd2 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -311,12 +311,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

Re: [PATCH v5 16/35] commit_lock_file(): inline temporary variable

[Jonathan Nieder]

Michael Haggerty wrote:

> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -311,12 +311,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;
> +

Same comment as the delete_ref_loose patch: the reader can gain some
piece of mind from an assertion at the top that makes sure filename is
valid (which it always should be, according to the API):

	assert(lk->filename[0]);

It would also be nice to use the same xmemdupz-based code as in the
delete_ref_loose case (ideally via a helper), to avoid having to work
with a PATH_MAX-sized buffer.

Otherwise looks good.

Thanks,
Jonathan

[PATCH v5 17/35] 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 9e1cdd2..6436111 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -312,6 +312,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

Re: [PATCH v5 17/35] commit_lock_file(): die() if called for unlocked lockfile object

[Jonathan Nieder]

Michael Haggerty wrote:

> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -312,6 +312,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");

Sure, this is fine instead of an assert() too. :)

Thanks,
Jonathan

[PATCH v5 18/35] 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 6436111..becb3da 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -315,8 +315,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

Re: [PATCH v5 18/35] commit_lock_file(): if close fails, roll back

[Jonathan Nieder]

Michael Haggerty wrote:

> If closing an open lockfile fails, then we cannot be sure of the
> contents of the lockfile

Is that true?  It seems more like a bug in close_lock_file: if it
fails, perhaps it should either set lk->fd back to fd or unlink the
lockfile itself.

What do other callers do on close_lock_file failure?

Jonathan

Re: [PATCH v5 18/35] commit_lock_file(): if close fails, roll back

[Michael Haggerty]

On 09/17/2014 12:19 AM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> If closing an open lockfile fails, then we cannot be sure of the
>> contents of the lockfile
> 
> Is that true?  It seems more like a bug in close_lock_file: if it
> fails, perhaps it should either set lk->fd back to fd or unlink the
> lockfile itself.

>From close(2) (note especially the last sentence):

> Not checking the return value of close() is a common but nevertheless serious  programming
> error.   It  is  quite  possible  that  errors  on a previous write(2) operation are first
> reported at the final close().  Not checking the return value when closing  the  file  may
> lead  to  silent  loss  of  data.   This can especially be observed with NFS and with disk
> quota.  Note that the return value should only be used  for  diagnostics.   In  particular
> close() should not be retried after an EINTR since this may cause a reused descriptor from
> another thread to be closed.

>From that I conclude that if close() fails, pretty much all bets are off
about the contents of the lockfile. So we wouldn't want to set lk->fd
back to fd.

But finishing the rollback itself might be an alternative...

> What do other callers do on close_lock_file failure?

>From what I can see, the only callers that don't die() immediately are
the following (which call close_lock_file() directly or indirectly via
write_locked_index()):

try_merge_strategy(): returns an error. It looks like this could end up
reusing the same lock_file object before it had been rolled back ->
would be improved if close_lock_file() would rollback on failure.

write_cache_as_tree(): does a rollback -> wouldn't mind an automatic
rollback.

merge_recursive_generic(): returns an error, and caller exits
immediately -> wouldn't mind an automatic rollback.

So, I will change close_lock_file() to roll back on errors.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH v5 19/35] 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 becb3da..6ae5c84 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -311,25 +311,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 v5 19/35] commit_lock_file(): rollback lock file on failure to rename

[Jonathan Nieder]

Michael Haggerty wrote:

> If rename() fails, call rollback_lock_file() to delete the lock file
> (in case it is still present) and reset the filename field to the
> empty string so that the lockfile object is left in a valid state.

Can you spell out more what the goal is?  Is the idea to keep the
.lock file for a shorter period of time, so other processes can lock
that file before the current process exits?

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

[Michael Haggerty]

On 09/17/2014 12:22 AM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> If rename() fails, call rollback_lock_file() to delete the lock file
>> (in case it is still present) and reset the filename field to the
>> empty string so that the lockfile object is left in a valid state.
> 
> Can you spell out more what the goal is?  Is the idea to keep the
> .lock file for a shorter period of time, so other processes can lock
> that file before the current process exits?

I doubt that there are situations where releasing the .lock file
immediately vs. holding it until the end of the process makes any
practical difference. Most callers die() right away if the commit fails.

This change is more about cleaning up the API by making the state
diagram for lock_file objects well-defined. commit_lock_file() can fail
in multiple ways that callers cannot easily distinguish. So all of the
failure paths have to leave the lock_file object in the same final
state, otherwise the callers cannot know what state it is in. Since
lock_file objects are reusable, this is an API design bug.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH v5 20/35] 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

Re: [PATCH v5 20/35] api-lockfile: document edge cases

[Jonathan Nieder]

Michael Haggerty wrote:

> * 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.

I think this would be easier to read in a separate error handling
section.  That way, when writing new code using the lockfile API,
I can quickly find what functions to use and quickly find out what
the error handling conventions are --- each use of the documentation
wouldn't interfere with the other.

Jonathan

Re: [PATCH v5 20/35] api-lockfile: document edge cases

[Michael Haggerty]

On 09/17/2014 12:23 AM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> * 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.
> 
> I think this would be easier to read in a separate error handling
> section.  That way, when writing new code using the lockfile API,
> I can quickly find what functions to use and quickly find out what
> the error handling conventions are --- each use of the documentation
> wouldn't interfere with the other.

I added a little blurb in the error handling section explaining how
commit_lock_file() and close_lock_file() behave on failure, but I left
the detailed information in the sections on those functions because I
couldn't find a graceful way to put all of the information in the error
handling sections.

If you have a concrete suggestion, as they say, "patches are welcome" :-)

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH v5 21/35] 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 c071253..742d4d7 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1828,10 +1828,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

Re: [PATCH v5 21/35] dump_marks(): remove a redundant call to rollback_lock_file()

[Jonathan Nieder]

Michael Haggerty wrote:

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

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[PATCH v5 22/35] 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 83c913a..a8ab809 100644
--- a/config.c
+++ b/config.c
@@ -2076,17 +2076,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

Re: [PATCH v5 22/35] git_config_set_multivar_in_file(): avoid call to rollback_lock_file()

[Jonathan Nieder]

Michael Haggerty wrote:

> 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>

This seems to involve an unadvertised behavior change: before
it wouldn't call git_config_clear() on failure, and after the
patch it would.  Intended?

I think it would be clearer with the goto for exception handling
maintained (and just a second 'lock = NULL' assignment).

Re: [PATCH v5 22/35] git_config_set_multivar_in_file(): avoid call to rollback_lock_file()

[Michael Haggerty]

On 09/17/2014 12:28 AM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> 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>
> 
> This seems to involve an unadvertised behavior change: before
> it wouldn't call git_config_clear() on failure, and after the
> patch it would.  Intended?
> 
> I think it would be clearer with the goto for exception handling
> maintained (and just a second 'lock = NULL' assignment).

Good catch; will fix.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH v5 23/35] 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   | 47 ++++++++++++++++++++++++++++++++---------------
 read-cache.c |  1 +
 3 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/cache.h b/cache.h
index e4e7c56..a367220 100644
--- a/cache.h
+++ b/cache.h
@@ -576,6 +576,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 6ae5c84..55fbb41 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -27,16 +27,19 @@
  * 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 is owned by the process that created it. The lock_file
  * object has an "owner" field that records its owner. This field is
  * used to prevent a forked process from closing a lock_file of its
  * parent.
  *
- * 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
@@ -44,19 +47,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.
  */
@@ -189,9 +198,14 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 		atexit(remove_lock_file);
 	}
 
+	if (lk->active)
+		die("BUG: lock_file(\"%s\") called with an active lock_file object",
+		    path);
+
 	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;
@@ -213,6 +227,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);
@@ -302,7 +317,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;
@@ -313,7 +328,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))
@@ -326,6 +341,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;
 
@@ -346,11 +362,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 b5917e0..e64bc87 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

Re: [PATCH v5 23/35] lockfile: avoid transitory invalid states

[Jonathan Nieder]

Michael Haggerty wrote:

> 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.

Nice.

[...]
> --- a/cache.h
> +++ b/cache.h
> @@ -576,6 +576,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;
[...]
> +++ b/lockfile.c
> @@ -27,16 +27,19 @@
[...]
> @@ -189,9 +198,14 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>  		atexit(remove_lock_file);
>  	}
>  
> +	if (lk->active)
> +		die("BUG: lock_file(\"%s\") called with an active lock_file object",
> +		    path);

The error message doesn't make it entirely obvious to me what went
wrong.

Maybe something like

		die("BUG: cannot lock_file(\"%s\") on active struct lock_file",
		    path);

lock_file already assumed on_list was initialized to zero but it
wasn't particularly obvious since everything else is blindly
scribbled over.  Probably worth mentioning in the API docs that
the lock_file should be zeroed before calling hold_lock_file_...

[...]
> @@ -326,6 +341,7 @@ int commit_lock_file(struct lock_file *lk)
>  	if (rename(lk->filename, result_file))
>  		goto rollback;
>  
> +	lk->active = 0;
>  	lk->filename[0] = 0;

Is it useful to set filename[0] any more?

It seems potentially fragile to set both, since new code can appear
that only sets or checks one or the other.  Would it make sense to
rename the filename field to make sure no new callers relying on the
filename[0] convention sneak in (with the new convention being that
the filename doesn't get cleared, to avoid problems)?

Jonathan

Re: [PATCH v5 23/35] lockfile: avoid transitory invalid states

[Michael Haggerty]

On 09/17/2014 12:45 AM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> 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.
> 
> Nice.
> 
> [...]
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -576,6 +576,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;
> [...]
>> +++ b/lockfile.c
>> @@ -27,16 +27,19 @@
> [...]
>> @@ -189,9 +198,14 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>>  		atexit(remove_lock_file);
>>  	}
>>  
>> +	if (lk->active)
>> +		die("BUG: lock_file(\"%s\") called with an active lock_file object",
>> +		    path);
> 
> The error message doesn't make it entirely obvious to me what went
> wrong.
> 
> Maybe something like
> 
> 		die("BUG: cannot lock_file(\"%s\") on active struct lock_file",
> 		    path);

This is an internal sanity check that users should never see, and if
they do the first thing a developer will do is grep the source code for
the error message in the bug report and then he/she will see exactly
what went wrong. So I don't think it is very important that this error
message be super self-explanatory.

But it doesn't hurt, so I'll make the change you suggest.

> lock_file already assumed on_list was initialized to zero but it
> wasn't particularly obvious since everything else is blindly
> scribbled over.  Probably worth mentioning in the API docs that
> the lock_file should be zeroed before calling hold_lock_file_...

Good point. I will document this better.

> [...]
>> @@ -326,6 +341,7 @@ int commit_lock_file(struct lock_file *lk)
>>  	if (rename(lk->filename, result_file))
>>  		goto rollback;
>>  
>> +	lk->active = 0;
>>  	lk->filename[0] = 0;
> 
> Is it useful to set filename[0] any more?
> 
> It seems potentially fragile to set both, since new code can appear
> that only sets or checks one or the other.  Would it make sense to
> rename the filename field to make sure no new callers relying on the
> filename[0] convention sneak in (with the new convention being that
> the filename doesn't get cleared, to avoid problems)?

I admit that nobody should be relying on filename being cleared anymore.
And I can see your point that somebody might come to rely on this
implementation detail. But I don't like leaving valid filenames around
where a bug might cause them to be accessed accidentally. I would rather
set the filename to the empty string so that any attempt to use it
causes an immediate error message from the OS rather than accessing the
wrong file.

I will note in the lock_file docstring that client code should not rely
on the filename being empty when in the 'unlocked' state.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH v5 24/35] 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 a367220..f251acd 100644
--- a/cache.h
+++ b/cache.h
@@ -575,10 +575,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 55fbb41..a09b4a3 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -70,7 +70,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 4f313bc..9971ac5 100644
--- a/refs.c
+++ b/refs.c
@@ -2259,15 +2259,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 v5 25/35] 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 9da9e30..6b430f0 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -668,7 +668,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 v5 26/35] 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 6b430f0..1a5a5f2 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -656,14 +656,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;
@@ -695,13 +695,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 v5 27/35] 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 a09b4a3..59c822d 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -325,8 +325,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");
@@ -334,13 +335,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 v5 28/35] 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       | 51 +++++++++++++++++++++++----------------------------
 read-cache.c     |  4 ++--
 refs.c           |  6 +++---
 shallow.c        |  6 +++---
 8 files changed, 46 insertions(+), 51 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d4228c9..4049d06 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,7 +352,7 @@ 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);
 		if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {
 			if (reopen_lock_file(&index_lock) < 0)
 				die(_("unable to write index file"));
@@ -362,7 +362,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 			warning(_("Failed to update main cache tree"));
 
 		commit_style = COMMIT_NORMAL;
-		return index_lock.filename;
+		return index_lock.filename.buf;
 	}
 
 	/*
@@ -385,7 +385,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;
 	}
 
 	/*
@@ -472,9 +472,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 f251acd..307cc8e 100644
--- a/cache.h
+++ b/cache.h
@@ -580,7 +580,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 a8ab809..34c93c7 100644
--- a/config.c
+++ b/config.c
@@ -2017,9 +2017,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;
 		}
@@ -2098,7 +2098,7 @@ out_free:
 	return ret;
 
 write_err_out:
-	ret = write_error(lock->filename);
+	ret = write_error(lock->filename.buf);
 	goto out_free;
 
 }
@@ -2199,9 +2199,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;
 	}
 
@@ -2222,7 +2222,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;
 				}
 				/*
@@ -2248,7 +2248,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 59c822d..eecb526 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -61,8 +61,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 empty (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.
@@ -185,13 +185,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);
@@ -208,29 +201,31 @@ 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;
+	} else if (lk->filename.len) {
+		/* This shouldn't happen, but better safe than sorry. */
+		die("BUG: lock_file(\"%s\") called with improperly-reset lock_file object",
+		    path);
 	}
 
-	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;
@@ -319,7 +314,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;
 }
 
@@ -336,14 +331,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:
@@ -368,7 +363,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 e64bc87..c126916 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 9971ac5..c6e15f9a 100644
--- a/refs.c
+++ b/refs.c
@@ -2557,8 +2557,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)
@@ -2922,7 +2922,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 v5 29/35] 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 eecb526..ae55f33 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -141,44 +141,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 v5 30/35] 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.

Helped-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 57 ++++++++++++++++++++++-----------------------------------
 1 file changed, 22 insertions(+), 35 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index ae55f33..e689a84 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -124,58 +124,47 @@ 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 */
 static int lock_file(struct lock_file *lk, const char *path, int flags)
 {
+	size_t pathlen = strlen(path);
+
 	if (!lock_file_list) {
 		/* One-time initialization */
 		sigchain_push_common(remove_lock_file_on_signal);
@@ -192,7 +181,7 @@ 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, pathlen + LOCK_SUFFIX_LEN);
 		lk->next = lock_file_list;
 		lock_file_list = lk;
 	} else if (lk->filename.len) {
@@ -201,11 +190,9 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 		    path);
 	}
 
-	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));
-	}
+	strbuf_add(&lk->filename, path, pathlen);
+	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 v5 31/35] trim_last_path_component(): 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 and a bit less tersely.

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 e689a84..3d5ab4f 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -91,32 +91,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_component(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);
 }
 
 
@@ -146,14 +142,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_component(path);
 
 		strbuf_addbuf(path, &link);
 	}
-- 
2.1.0

[PATCH v5 32/35] 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                               | 39 +++++++++++++++++++++-----------
 read-cache.c                             | 13 +++--------
 4 files changed, 40 insertions(+), 27 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 307cc8e..45688d5 100644
--- a/cache.h
+++ b/cache.h
@@ -590,6 +590,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 3d5ab4f..480c2ba 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -58,8 +58,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 empty (usually, though there are transitory
  *     states in which this condition doesn't hold)
@@ -290,24 +291,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");
+		die("BUG: attempt to commit unlocked object to \"%s\"", path);
 
 	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);
@@ -320,6 +313,26 @@ rollback:
 	return -1;
 }
 
+int commit_lock_file(struct lock_file *lk)
+{
+	static struct strbuf result_file = STRBUF_INIT;
+	int err;
+
+	if (!lk->active)
+		die("BUG: attempt to commit unlocked object");
+
+	if (lk->filename.len <= LOCK_SUFFIX_LEN ||
+	    strcmp(lk->filename.buf + lk->filename.len - LOCK_SUFFIX_LEN, LOCK_SUFFIX))
+		die("BUG: lockfile filename corrupt");
+
+	/* 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 c126916..f3eb5e9 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

[PATCH v5 33/35] Rename LOCK_NODEREF to LOCK_NO_DEREF

[Michael Haggerty]

This makes it harder to misread the name as LOCK_NODE_REF.

Suggested-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/technical/api-lockfile.txt | 4 ++--
 cache.h                                  | 2 +-
 lockfile.c                               | 4 ++--
 refs.c                                   | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index 3ee4299..40cd524 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -31,11 +31,11 @@ hold_lock_file_for_update::
 	to the file.  The flags parameter is a combination of
 +
 --
-LOCK_NODEREF::
+LOCK_NO_DEREF::
 
 	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
+	however, if `LOCK_NO_DEREF` is set, then the lockfile is
 	created by adding ".lock" to the path argument itself.
 
 LOCK_DIE_ON_ERROR::
diff --git a/cache.h b/cache.h
index 45688d5..d610fab 100644
--- a/cache.h
+++ b/cache.h
@@ -583,7 +583,7 @@ struct lock_file {
 	struct strbuf filename;
 };
 #define LOCK_DIE_ON_ERROR 1
-#define LOCK_NODEREF 2
+#define LOCK_NO_DEREF 2
 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);
diff --git a/lockfile.c b/lockfile.c
index 480c2ba..432d624 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -17,7 +17,7 @@
  *
  *    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
+ *    replaced.  However, if LOCK_NO_DEREF 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.
@@ -186,7 +186,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	}
 
 	strbuf_add(&lk->filename, path, pathlen);
-	if (!(flags & LOCK_NODEREF))
+	if (!(flags & LOCK_NO_DEREF))
 		resolve_symlink(&lk->filename);
 	strbuf_addstr(&lk->filename, LOCK_SUFFIX);
 	lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
diff --git a/refs.c b/refs.c
index c6e15f9a..525ce4b 100644
--- a/refs.c
+++ b/refs.c
@@ -2134,7 +2134,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	lflags = 0;
 	if (flags & REF_NODEREF) {
 		refname = orig_refname;
-		lflags |= LOCK_NODEREF;
+		lflags |= LOCK_NO_DEREF;
 	}
 	lock->ref_name = xstrdup(refname);
 	lock->orig_ref_name = xstrdup(orig_refname);
-- 
2.1.0

[PATCH v5 34/35] lockfile.c: rename static functions

[Michael Haggerty]

* remove_lock_file() -> remove_lock_files()
* remove_lock_file_on_signal() -> remove_lock_files_on_signal()

Suggested-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 432d624..0b63554 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -73,7 +73,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();
 
@@ -84,9 +84,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);
 }
@@ -162,8 +162,8 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 
 	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);
 	}
 
 	if (lk->active)
-- 
2.1.0

[PATCH v5 35/35] get_locked_file_path(): new function

[Michael Haggerty]

Add a function to return the path of the file that is locked by a
lock_file object. This reduces the knowledge that callers have to have
about the lock_file layout.

Suggested-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/technical/api-lockfile.txt | 5 +++++
 cache.h                                  | 1 +
 lockfile.c                               | 9 +++++++++
 refs.c                                   | 4 +---
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index 40cd524..22f1b5c 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -52,6 +52,11 @@ hold_lock_file_for_append::
 	lockfile and its write pointer is positioned at the end of the
 	file before returning.
 
+get_locked_file_path::
+
+	Return the path of the file that is locked by the specified
+	lock_file object. The caller must free the memory.
+
 unable_to_lock_error::
 
 	Emit an error describing that there was an error locking the
diff --git a/cache.h b/cache.h
index d610fab..4bbaffa 100644
--- a/cache.h
+++ b/cache.h
@@ -590,6 +590,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 char *get_locked_file_path(struct lock_file *);
 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 *);
diff --git a/lockfile.c b/lockfile.c
index 0b63554..42cea0a 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -274,6 +274,15 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
 	return fd;
 }
 
+char *get_locked_file_path(struct lock_file *lk)
+{
+	if (!lk->active)
+		die("BUG: get_locked_file_path() called for unlocked object");
+	if (lk->filename.len <= LOCK_SUFFIX_LEN)
+		die("BUG: get_locked_file_path() called for malformed lock object");
+	return xmemdupz(lk->filename.buf, lk->filename.len - LOCK_SUFFIX_LEN);
+}
+
 int close_lock_file(struct lock_file *lk)
 {
 	int fd = lk->fd;
diff --git a/refs.c b/refs.c
index 525ce4b..5842dd0 100644
--- a/refs.c
+++ b/refs.c
@@ -2556,9 +2556,7 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)
 		 * loose.  The loose file name is the same as the
 		 * lockfile name, minus ".lock":
 		 */
-		char *loose_filename = xmemdupz(
-				lock->lk->filename.buf,
-				lock->lk->filename.len - LOCK_SUFFIX_LEN);
+		char *loose_filename = get_locked_file_path(lock->lk);
 		int err = unlink_or_warn(loose_filename);
 		free(loose_filename);
 		if (err && errno != ENOENT)
-- 
2.1.0