[PATCH 00/22] Lockfile refactoring and pre-activation

[PATCH 00/22] Lockfile refactoring and pre-activation

[Michael Haggerty]

I've had this patch series kicking around for a long time, along with
some followup patches to fix a race in reference deletion.  I haven't
had the time to get everything done and tested, but let me at least
push this first series out there.  I especially want to submit it in
case we accept a GSoC student for the project "Refactor tempfile
handling", because (1) I don't want me and the student to be stepping
on each others' toes, and (2) there are some cleanups and
documentation improvements here that will hopefully be useful to the
student.

The first patch actually demonstrates the race condition that I hope
to fix someday.  The last patch introduces the lockfile feature that I
think is needed to fix it: the ability to activate a packed-refs file
while still holding the lock to prevent another process from
overwriting it before the accompanying loose reference updates are all
finished.  But the fix itself is not included here, so you might want
to keep the last patch on hold until there is a concrete user of the
feature.

The rest of the patches hopefully make the lockfile API more
consistent and better documented.  There were a lot of situations when
a failure in one step of the lock/commit procedure left a lock in an
ill-defined state.  That's probably not a problem very often in
practice, because we tend to die() soon after any locking problems.
But it is still helpful for lockfile objects to have a well-defined
state diagram (especially if your goal is to add a new feature to
them!)

Several patches are also devoted to making lock filename handling use
strbufs (for the usual reasons), and reducing the need for callers to
reach into the "filename" field and especially for them to use that
field as temporary scratch space.

Michael

Michael Haggerty (22):
  t3204: test deleting references when lock files already exist
  try_merge_strategy(): remove redundant lock_file allocation
  rollback_lock_file(): do not clear filename redundantly
  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
  struct lock_file: replace on_list field with flags field
  api-lockfile: expand the documentation
  lockfile.c: document the various states of lock_file objects
  lockfile: define a constant LOCK_SUFFIX_LEN
  delete_ref_loose(): don't muck around in the lock_file's filename
  config: change write_error() to take a (struct lock_file *) argument
  lockfile: use strbufs when handling (most) paths
  resolve_symlink(): use a strbuf internally
  commit_lock_file(): don't work with a fixed-length buffer
  lock_file(): exit early if lockfile cannot be opened
  lockfile: also keep track of the filename of the file being locked
  struct lock_file: rename lock_filename field to staging_filename
  remove_lock_file(): call rollback_lock_file()
  lockfile: extract a function reset_lock_file()
  lockfile: allow new file contents to be written while retaining lock

 Documentation/technical/api-lockfile.txt |  90 +++++--
 builtin/commit.c                         |  12 +-
 builtin/merge.c                          |   1 -
 builtin/reflog.c                         |   2 +-
 cache.h                                  |   9 +-
 config.c                                 |  11 +-
 lockfile.c                               | 389 +++++++++++++++++++++----------
 refs.c                                   |   8 +-
 shallow.c                                |   6 +-
 t/t3204-branch-locks.sh                  |  40 ++++
 10 files changed, 403 insertions(+), 165 deletions(-)
 create mode 100755 t/t3204-branch-locks.sh

-- 
1.9.0

[PATCH 01/22] t3204: test deleting references when lock files already exist

[Michael Haggerty]

When deleting a reference, it might be that another process already
holds the lock on the loose reference file and/or the packed-refs
file.  In those cases, there is no alternative but for the delete to
fail.  Verify that in such cases the reference values are left
unchanged.

But in fact, when the packed-refs file lock cannot be acquired, the
loose reference file is deleted anyway, potentially leaving the
reference with a changed value (its packed value, which might even
point at an object that has been garbage collected).  Thus one of the
new tests is marked test_expect_failure.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t3204-branch-locks.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100755 t/t3204-branch-locks.sh

diff --git a/t/t3204-branch-locks.sh b/t/t3204-branch-locks.sh
new file mode 100755
index 0000000..cc4e2c1
--- /dev/null
+++ b/t/t3204-branch-locks.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+test_description='git branch lock tests'
+
+. ./test-lib.sh
+
+test_expect_success 'prepare a trivial repository' '
+	git commit --allow-empty -m "Initial" &&
+	git commit --allow-empty -m "First"
+'
+
+test_expect_success 'delete a ref while loose ref file is locked' '
+	git branch b1 master &&
+	git for-each-ref >expected1 &&
+	# Simulate another process holding the loose file lock:
+	: >.git/refs/heads/b1.lock &&
+	test_must_fail git branch -D b1 &&
+	rm .git/refs/heads/b1.lock &&
+	# Delete failed; reference values should be unchanged:
+	git for-each-ref >actual1 &&
+	test_cmp expected1 actual1
+'
+
+test_expect_failure 'delete a ref while packed-refs file is locked' '
+	git branch b2 master &&
+	# Pack current value of b2:
+	git pack-refs --all &&
+	# Overwrite packed value with a loose value:
+	git branch -f b2 master^ &&
+	git for-each-ref >expected2 &&
+	# Simulate another process holding the packed-refs file lock:
+	: >.git/packed-refs.lock &&
+	test_must_fail git branch -D b2 &&
+	rm .git/packed-refs.lock &&
+	# Delete failed; reference values should be unchanged:
+	git for-each-ref >actual2 &&
+	test_cmp expected2 actual2
+'
+
+test_done
-- 
1.9.0

Re: [PATCH 01/22] t3204: test deleting references when lock files already exist

[Jeff King]

On Tue, Apr 01, 2014 at 05:58:09PM +0200, Michael Haggerty wrote:

> When deleting a reference, it might be that another process already
> holds the lock on the loose reference file and/or the packed-refs
> file.  In those cases, there is no alternative but for the delete to
> fail.  Verify that in such cases the reference values are left
> unchanged.
> 
> But in fact, when the packed-refs file lock cannot be acquired, the
> loose reference file is deleted anyway, potentially leaving the
> reference with a changed value (its packed value, which might even
> point at an object that has been garbage collected).  Thus one of the
> new tests is marked test_expect_failure.

Nice find. If I understand correctly, the problem is at the plumbing
level, and this could also be demonstrated with update-ref?

I don't think it is a big deal, but I was thrown for a minute by the use
of "git branch" (as in, "is this something special with branches, or is
this about all refs?").

-Peff

Re: [PATCH 01/22] t3204: test deleting references when lock files already exist

[Michael Haggerty]

On 04/01/2014 09:53 PM, Jeff King wrote:
> On Tue, Apr 01, 2014 at 05:58:09PM +0200, Michael Haggerty wrote:
> 
>> When deleting a reference, it might be that another process already
>> holds the lock on the loose reference file and/or the packed-refs
>> file.  In those cases, there is no alternative but for the delete to
>> fail.  Verify that in such cases the reference values are left
>> unchanged.
>>
>> But in fact, when the packed-refs file lock cannot be acquired, the
>> loose reference file is deleted anyway, potentially leaving the
>> reference with a changed value (its packed value, which might even
>> point at an object that has been garbage collected).  Thus one of the
>> new tests is marked test_expect_failure.
> 
> Nice find. If I understand correctly, the problem is at the plumbing
> level, and this could also be demonstrated with update-ref?
> 
> I don't think it is a big deal, but I was thrown for a minute by the use
> of "git branch" (as in, "is this something special with branches, or is
> this about all refs?").

Good point.  Will change.

Michael

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

[PATCH 02/22] 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 used to shadow 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 e15d0e1..f714961 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -671,7 +671,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));
 		int index_fd;
 		struct commit_list *reversed = NULL;
 		struct merge_options o;
-- 
1.9.0

Re: [PATCH 02/22] try_merge_strategy(): remove redundant lock_file allocation

[Jeff King]

On Tue, Apr 01, 2014 at 05:58:10PM +0200, Michael Haggerty wrote:

> 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 used to shadow the
> "lock" variable at function scope, so the only change needed is to
> remove the inner definition.

I wonder if this would also be simpler if "lock" were simply declared as
a static variable, and we drop the allocation entirely. I suppose that
does create more cognitive load, though, in that it is only correct if
the function is not recursive. On the other hand, the current code makes
a reader unfamiliar with "struct lock" wonder if there is a free(lock)
missing.

-Peff

Re: [PATCH 02/22] try_merge_strategy(): remove redundant lock_file allocation

[Michael Haggerty]

On 04/01/2014 09:56 PM, Jeff King wrote:
> On Tue, Apr 01, 2014 at 05:58:10PM +0200, Michael Haggerty wrote:
> 
>> 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 used to shadow the
>> "lock" variable at function scope, so the only change needed is to
>> remove the inner definition.
> 
> I wonder if this would also be simpler if "lock" were simply declared as
> a static variable, and we drop the allocation entirely. I suppose that
> does create more cognitive load, though, in that it is only correct if
> the function is not recursive. On the other hand, the current code makes
> a reader unfamiliar with "struct lock" wonder if there is a free(lock)
> missing.

Yes, a single lock_file object should suffice.  I will make this change
in the next version of the patch series.

Michael

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

Re: [PATCH 02/22] try_merge_strategy(): remove redundant lock_file allocation

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Apr 01, 2014 at 05:58:10PM +0200, Michael Haggerty wrote:
>
>> 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 used to shadow the
>> "lock" variable at function scope, so the only change needed is to
>> remove the inner definition.
>
> I wonder if this would also be simpler if "lock" were simply declared as
> a static variable, and we drop the allocation entirely. I suppose that
> does create more cognitive load, though, in that it is only correct if
> the function is not recursive. On the other hand, the current code makes
> a reader unfamiliar with "struct lock" wonder if there is a free(lock)
> missing.

Another thing that makes a reader wonder if this is a valid rewrite
is if it is safe to reuse a lock_file structure, especially because
the original gives a piece of memory _cleared_ with xcalloc().  The
second invocation of hold_locked_index() is now done on a dirty
piece of memory, and the reader needs to drill down the callchain to
see if that is safe (and if not, hold_locked_index() and probably
the underlying lock_file() needs to memset() it to NULs).

Re: [PATCH 02/22] try_merge_strategy(): remove redundant lock_file allocation

[Michael Haggerty]

On 04/02/2014 06:53 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> On Tue, Apr 01, 2014 at 05:58:10PM +0200, Michael Haggerty wrote:
>>
>>> 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 used to shadow the
>>> "lock" variable at function scope, so the only change needed is to
>>> remove the inner definition.
>>
>> I wonder if this would also be simpler if "lock" were simply declared as
>> a static variable, and we drop the allocation entirely. I suppose that
>> does create more cognitive load, though, in that it is only correct if
>> the function is not recursive. On the other hand, the current code makes
>> a reader unfamiliar with "struct lock" wonder if there is a free(lock)
>> missing.
> 
> Another thing that makes a reader wonder if this is a valid rewrite
> is if it is safe to reuse a lock_file structure, especially because
> the original gives a piece of memory _cleared_ with xcalloc().  The
> second invocation of hold_locked_index() is now done on a dirty
> piece of memory, and the reader needs to drill down the callchain to
> see if that is safe (and if not, hold_locked_index() and probably
> the underlying lock_file() needs to memset() it to NULs).

It's good that you and Peff asked questions about this sort of thing.

We reuse lock_file structures *all over the place*; for example, just
search for "static struct lock_file".  It has to be safe...

...and yet it isn't.  Look in the definition of lock_file() (before my
changes):

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

Remember that a reused lock_file structure is already in lock_file_list,
and there is already a signal handler registered that will call
remove_lock_file(), which looks like:

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);
		}
		lock_file_list = lock_file_list->next;
	}
}

So, if the process gets a signal during the call to resolve_symlink(),
the atexit() cleanup routine will delete the valuable file (the one
being locked)!

It definitely looks like this area needs more work.

Michael

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

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

[Michael Haggerty]

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

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

diff --git a/lockfile.c b/lockfile.c
index 8fbcb6a..d26711f 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -277,6 +277,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;
 }
-- 
1.9.0

[PATCH 04/22] 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 could help prevent
confusion in the face of hypothetical future programming errors.

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

diff --git a/lockfile.c b/lockfile.c
index d26711f..c1af65b 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -275,7 +275,7 @@ void rollback_lock_file(struct lock_file *lk)
 {
 	if (lk->filename[0]) {
 		if (lk->fd >= 0)
-			close(lk->fd);
+			close_lock_file(lk);
 		unlink_or_warn(lk->filename);
 		lk->filename[0] = 0;
 	}
-- 
1.9.0

Re: [PATCH 04/22] rollback_lock_file(): set fd to -1

[Jeff King]

On Tue, Apr 01, 2014 at 05:58:12PM +0200, Michael Haggerty wrote:

> When rolling back the lockfile, call close_lock_file() so that the
> lock_file's fd field gets set back to -1.  This could help prevent
> confusion in the face of hypothetical future programming errors.

This also solves a race. We could be in the middle of rollback_lock_file
when we get a signal, and double-close. It's probably not a big deal,
though, since nobody could have opened a new descriptor in the interim
that got the same number (so the second close will just fail silently).

Still, this seems like a definite improvement.

-Peff

Re: [PATCH 04/22] rollback_lock_file(): set fd to -1

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Apr 01, 2014 at 05:58:12PM +0200, Michael Haggerty wrote:
>
>> When rolling back the lockfile, call close_lock_file() so that the
>> lock_file's fd field gets set back to -1.  This could help prevent
>> confusion in the face of hypothetical future programming errors.
>
> This also solves a race. We could be in the middle of rollback_lock_file
> when we get a signal, and double-close. It's probably not a big deal,
> though, since nobody could have opened a new descriptor in the interim
> that got the same number (so the second close will just fail silently).
>
> Still, this seems like a definite improvement.

This is probably related to my comments on 2/22, but is "fd" the
only thing that has a non-zero safe value?  Perhaps lock_file_init()
that clears the structure fields to 0/NULL and fd to -1, and a
convenience function lock_file_alloc() that does xmalloc() and then
calls lock_file_init() may help us a bit when the lockfile structure
is reused?

Re: [PATCH 04/22] rollback_lock_file(): set fd to -1

[Michael Haggerty]

On 04/02/2014 06:58 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> On Tue, Apr 01, 2014 at 05:58:12PM +0200, Michael Haggerty wrote:
>>
>>> When rolling back the lockfile, call close_lock_file() so that the
>>> lock_file's fd field gets set back to -1.  This could help prevent
>>> confusion in the face of hypothetical future programming errors.
>>
>> This also solves a race. We could be in the middle of rollback_lock_file
>> when we get a signal, and double-close. It's probably not a big deal,
>> though, since nobody could have opened a new descriptor in the interim
>> that got the same number (so the second close will just fail silently).
>>
>> Still, this seems like a definite improvement.
> 
> This is probably related to my comments on 2/22, but is "fd" the
> only thing that has a non-zero safe value?  Perhaps lock_file_init()
> that clears the structure fields to 0/NULL and fd to -1, and a
> convenience function lock_file_alloc() that does xmalloc() and then
> calls lock_file_init() may help us a bit when the lockfile structure
> is reused?

The first use of a lock_file object necessarily passes through
lock_file().  The only precondition for that function is that the
on_list field is zero, which is satisfied by a xcalloc()ed object.

Subsequent uses of a lock_file object must *not* zero the object.
lock_file objects are added to the lock_file_list and never removed.  So
zeroing a lock_file object would discard the rest of the linked list.
But subsequent uses must also pass through lock_file(), which sees that
on_list is set, and assumes that the object is in a self-consistent
state as left by commit_lock_file() or rollback_lock_file().

At least that's how it is supposed to work.  But lock_file objects are
in fact not cleaned up correctly in all circumstances.  The next version
of this patch series will work to fix that.

Michael

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

Re: [PATCH 04/22] rollback_lock_file(): set fd to -1

[Junio C Hamano]

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

> The first use of a lock_file object necessarily passes through
> lock_file().  The only precondition for that function is that the
> on_list field is zero, which is satisfied by a xcalloc()ed object.
>
> Subsequent uses of a lock_file object must *not* zero the object.
> lock_file objects are added to the lock_file_list and never removed.  So
> zeroing a lock_file object would discard the rest of the linked list.
> But subsequent uses must also pass through lock_file(), which sees that
> on_list is set, and assumes that the object is in a self-consistent
> state as left by commit_lock_file() or rollback_lock_file().
>
> At least that's how it is supposed to work.  But lock_file objects are
> in fact not cleaned up correctly in all circumstances.  The next version
> of this patch series will work to fix that.

Thanks; I see the next round already posted to the list.

[PATCH 05/22] 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 that (namely, unlocked).  Previously, the lockfile
was retained until process cleanup in this situation.

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 c1af65b..1928e5e 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -148,9 +148,11 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 			lock_file_list = lk;
 			lk->on_list = 1;
 		}
-		if (adjust_shared_perm(lk->filename))
-			return error("cannot fix permission bits on %s",
-				     lk->filename);
+		if (adjust_shared_perm(lk->filename)) {
+			error("cannot fix permission bits on %s", lk->filename);
+			rollback_lock_file(lk);
+			return -1;
+		}
 	}
 	else
 		lk->filename[0] = 0;
-- 
1.9.0

Re: [PATCH 05/22] lockfile: unlock file if lockfile permissions cannot be adjusted

[Jeff King]

On Tue, Apr 01, 2014 at 05:58:13PM +0200, Michael Haggerty wrote:

> 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 that (namely, unlocked).  Previously, the lockfile
> was retained until process cleanup in this situation.

Another nice find. I wondered if we could test this, but I think it
would be hard to trigger. The obvious reason for adjust_shared_perm to
fail is that you do not have permissions on the file, but by definition
you just created it. So I doubt this ever comes up in practice short of
weird races (somebody dropping the "x" bit from an intermediate
directory between the open() and chmod() or something).

-Peff

Re: [PATCH 05/22] lockfile: unlock file if lockfile permissions cannot be adjusted

[Jeff King]

On Tue, Apr 01, 2014 at 04:02:42PM -0400, Jeff King wrote:

> On Tue, Apr 01, 2014 at 05:58:13PM +0200, Michael Haggerty wrote:
> 
> > 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 that (namely, unlocked).  Previously, the lockfile
> > was retained until process cleanup in this situation.
> 
> Another nice find. I wondered if we could test this, but I think it
> would be hard to trigger. The obvious reason for adjust_shared_perm to
> fail is that you do not have permissions on the file, but by definition
> you just created it. So I doubt this ever comes up in practice short of
> weird races (somebody dropping the "x" bit from an intermediate
> directory between the open() and chmod() or something).

...and I should have read the final sentence in your message more
carefully. Even if we did trigger it, the problem would only last until
the program exits anyway.

I agree that this is a nice cleanup, though; a caller that wants to
retry the lock before exiting would be much less surprised. And the same
logic applies to 06/22.

-Peff

Re: [PATCH 05/22] lockfile: unlock file if lockfile permissions cannot be adjusted

[Torsten Bögershausen]

[]

diff --git a/lockfile.c b/lockfile.c
index c1af65b..1928e5e 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -148,9 +148,11 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
  			lock_file_list = lk;
  			lk->on_list = 1;
  		}
-		if (adjust_shared_perm(lk->filename))
-			return error("cannot fix permission bits on %s",
-				     lk->filename);
+		if (adjust_shared_perm(lk->filename)) {
+			error("cannot fix permission bits on %s", lk->filename);
+			rollback_lock_file(lk);
+			return -1;

Would it make sense to change the order of rollback() and error()?
Make the rollback first (and as early as possible) and whine then?

Re: [PATCH 05/22] lockfile: unlock file if lockfile permissions cannot be adjusted

[Michael Haggerty]

On 04/02/2014 08:47 AM, Torsten Bögershausen wrote:
> []
> 
> diff --git a/lockfile.c b/lockfile.c
> index c1af65b..1928e5e 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -148,9 +148,11 @@ static int lock_file(struct lock_file *lk, const
> char *path, int flags)
>              lock_file_list = lk;
>              lk->on_list = 1;
>          }
> -        if (adjust_shared_perm(lk->filename))
> -            return error("cannot fix permission bits on %s",
> -                     lk->filename);
> +        if (adjust_shared_perm(lk->filename)) {
> +            error("cannot fix permission bits on %s", lk->filename);
> +            rollback_lock_file(lk);
> +            return -1;
> 
> Would it make sense to change the order of rollback() and error()?
> Make the rollback first (and as early as possible) and whine then?

Thanks for the feedback.  It is a nice idea.  But given that
rollback_lock_file() erases the filename field, making the change you
suggest would require us to make a copy before calling
rollback_lock_file().  The only advantage would be that we hold the lock
file for a moment less, right?  Since this code path is only reached
when the repository has screwy permissions anyway, the next caller will
probably fail too.  So the extra code complication doesn't seem worth it
to me.

Michael

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

[PATCH 06/22] 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 1928e5e..e679e4c 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -212,13 +212,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;
-- 
1.9.0

[PATCH 07/22] 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 | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index e679e4c..c989f6c 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -130,6 +130,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);
+	}
+
+	lk->owner = getpid();
+	if (!lk->on_list) {
+		/* Initialize *lk and add it to lock_file_list: */
+		lk->fd = -1;
+		lk->on_list = 1;
+		lk->filename[0] = 0;
+		lk->next = lock_file_list;
+		lock_file_list = lk;
+	}
+
 	if (strlen(path) >= max_path_len)
 		return -1;
 	strcpy(lk->filename, path);
@@ -138,16 +154,6 @@ 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)) {
 			error("cannot fix permission bits on %s", lk->filename);
 			rollback_lock_file(lk);
-- 
1.9.0

Re: [PATCH 07/22] lock_file(): always add lock_file object to lock_file_list

[Jeff King]

On Tue, Apr 01, 2014 at 05:58:15PM +0200, Michael Haggerty wrote:

> diff --git a/lockfile.c b/lockfile.c
> index e679e4c..c989f6c 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -130,6 +130,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);
> +	}
> +
> +	lk->owner = getpid();
> +	if (!lk->on_list) {
> +		/* Initialize *lk and add it to lock_file_list: */
> +		lk->fd = -1;
> +		lk->on_list = 1;
> +		lk->filename[0] = 0;
> +		lk->next = lock_file_list;
> +		lock_file_list = lk;
> +	}

Initializing here is good, since we might be interrupted by a signal at
any time. But what about during the locking procedure? We do:

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

So for a moment, lk->filename contains the name of the valuable file we
are locking.  If we get a signal at that moment, do we accidentally
delete it in remove_lock_file?

I think the answer is "no", because we check lk->owner before deleting,
which will not match our pid (it should generally be zero due to xcalloc
or static initialization, though perhaps we should clear it here).

But that makes me wonder about the case of a reused lock. It will have
lk->owner set from a previous invocation, and would potentially suffer
from this problem. In other words, I think the change you are
introducing does not have the problem, but the existing code does. :-/

I didn't reproduce it experimentally, though.  We should be able to just

    lk->owner = 0;

before the initial strcpy to fix it, I would think.

-Peff

Re: [PATCH 07/22] lock_file(): always add lock_file object to lock_file_list

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So for a moment, lk->filename contains the name of the valuable file we
> are locking.  If we get a signal at that moment, do we accidentally
> delete it in remove_lock_file?
>
> I think the answer is "no", because we check lk->owner before deleting,
> which will not match our pid (it should generally be zero due to xcalloc
> or static initialization, though perhaps we should clear it here).

That "generally be zero" no longer holds since 2/22, no?

> But that makes me wonder about the case of a reused lock. It will have
> lk->owner set from a previous invocation, and would potentially suffer
> from this problem. In other words, I think the change you are
> introducing does not have the problem, but the existing code does. :-/

Yes, I tend to agree.

> I didn't reproduce it experimentally, though.  We should be able to just
>
>     lk->owner = 0;
>
> before the initial strcpy to fix it, I would think.
>
> -Peff

Re: [PATCH 07/22] lock_file(): always add lock_file object to lock_file_list

[Michael Haggerty]

On 04/01/2014 10:16 PM, Jeff King wrote:
> On Tue, Apr 01, 2014 at 05:58:15PM +0200, Michael Haggerty wrote:
> 
>> diff --git a/lockfile.c b/lockfile.c
>> index e679e4c..c989f6c 100644
>> --- a/lockfile.c
>> +++ b/lockfile.c
>> @@ -130,6 +130,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);
>> +	}
>> +
>> +	lk->owner = getpid();
>> +	if (!lk->on_list) {
>> +		/* Initialize *lk and add it to lock_file_list: */
>> +		lk->fd = -1;
>> +		lk->on_list = 1;
>> +		lk->filename[0] = 0;
>> +		lk->next = lock_file_list;
>> +		lock_file_list = lk;
>> +	}
> 
> Initializing here is good, since we might be interrupted by a signal at
> any time. But what about during the locking procedure? We do:
> 
>     strcpy(lk->filename, path);
>     if (!(flags & LOCK_NODEREF))
>             resolve_symlink(lk->filename, max_path_len);
>     strcat(lk->filename, ".lock");
> 
> So for a moment, lk->filename contains the name of the valuable file we
> are locking.  If we get a signal at that moment, do we accidentally
> delete it in remove_lock_file?
> 
> I think the answer is "no", because we check lk->owner before deleting,
> which will not match our pid (it should generally be zero due to xcalloc
> or static initialization, though perhaps we should clear it here).
> 
> But that makes me wonder about the case of a reused lock. It will have
> lk->owner set from a previous invocation, and would potentially suffer
> from this problem. In other words, I think the change you are
> introducing does not have the problem, but the existing code does. :-/

Good point.  Yes, I agree that this is a problem in the existing code
and that it wasn't improved by my work.

> I didn't reproduce it experimentally, though.  We should be able to just
> 
>     lk->owner = 0;
> 
> before the initial strcpy to fix it, I would think.

I think that using the owner field to avoid this problem is a bit
indirect, so I will soon submit a fix that involves adding a flag to
lock_file objects indicating whether the filename field currently
contains the name of a file that needs to be deleted.

Michael

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

Re: [PATCH 07/22] lock_file(): always add lock_file object to lock_file_list

[Jeff King]

On Sun, Apr 06, 2014 at 11:54:59PM +0200, Michael Haggerty wrote:

> > I didn't reproduce it experimentally, though.  We should be able to just
> > 
> >     lk->owner = 0;
> > 
> > before the initial strcpy to fix it, I would think.
> 
> I think that using the owner field to avoid this problem is a bit
> indirect, so I will soon submit a fix that involves adding a flag to
> lock_file objects indicating whether the filename field currently
> contains the name of a file that needs to be deleted.

Yeah, I agree that the current code is a bit subtle. I was planning to
address this during the tempfile cleanup project (either in GSoC, if it
gets a slot, or just doing it myself). But I don't mind if you want to
do something in the interim.

-Peff

[PATCH 08/22] struct lock_file: replace on_list field with flags field

[Michael Haggerty]

This makes space for other bits.

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

diff --git a/cache.h b/cache.h
index 107ac61..08d0e48 100644
--- a/cache.h
+++ b/cache.h
@@ -537,7 +537,7 @@ struct lock_file {
 	struct lock_file *next;
 	int fd;
 	pid_t owner;
-	char on_list;
+	unsigned char flags;
 	char filename[PATH_MAX];
 };
 #define LOCK_DIE_ON_ERROR 1
diff --git a/lockfile.c b/lockfile.c
index c989f6c..33325a4 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -4,6 +4,11 @@
 #include "cache.h"
 #include "sigchain.h"
 
+/* Values for lock_file::flags: */
+
+/* This lock_file instance is in the lock_file_list */
+#define LOCK_FLAGS_ON_LIST 0x01
+
 static struct lock_file *lock_file_list;
 static const char *alternate_index_output;
 
@@ -137,10 +142,10 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	}
 
 	lk->owner = getpid();
-	if (!lk->on_list) {
+	if (!(lk->flags & LOCK_FLAGS_ON_LIST)) {
 		/* Initialize *lk and add it to lock_file_list: */
 		lk->fd = -1;
-		lk->on_list = 1;
+		lk->flags |= LOCK_FLAGS_ON_LIST;
 		lk->filename[0] = 0;
 		lk->next = lock_file_list;
 		lock_file_list = lk;
-- 
1.9.0

[PATCH 09/22] 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..95ed03b 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_index_die::
+
+	Like `unable_to_lock_error()`, but also `die()`.
 
 commit_lock_file::
 
-- 
1.9.0

Re: [PATCH 09/22] api-lockfile: expand the documentation

[Jeff King]

On Tue, Apr 01, 2014 at 05:58:17PM +0200, Michael Haggerty wrote:

> +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_index_die::
> +
> +	Like `unable_to_lock_error()`, but also `die()`.

Should this last one lost the "index" in its name? I think it is
vestigial at this point.

-Peff

Re: [PATCH 09/22] api-lockfile: expand the documentation

[Michael Haggerty]

On 04/01/2014 10:19 PM, Jeff King wrote:
> On Tue, Apr 01, 2014 at 05:58:17PM +0200, Michael Haggerty wrote:
> 
>> +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_index_die::
>> +
>> +	Like `unable_to_lock_error()`, but also `die()`.
> 
> Should this last one lost the "index" in its name? I think it is
> vestigial at this point.

Yes.  Will be done in re-roll.

Michael

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

[PATCH 10/22] lockfile.c: document the various states of lock_file objects

[Michael Haggerty]

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

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

diff --git a/lockfile.c b/lockfile.c
index 33325a4..4a9ceda 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -4,6 +4,58 @@
 #include "cache.h"
 #include "sigchain.h"
 
+/*
+ * File write-locks as used by Git.
+ *
+ * When a file at $FILENAME needs to be written, it is done as
+ * follows:
+ *
+ * 1. Obtain a lock on the file by creating a lockfile at path
+ *    $FILENAME.lock.  The file is opened for read/write using O_CREAT
+ *    and O_EXCL mode to ensure that it doesn't already exist.  Such a
+ *    lock file is respected by writers *but not by readers*.
+ *
+ *    Usually, if $FILENAME is a symlink, then it is resolved, and the
+ *    file ultimately pointed to is the one that is locked and later
+ *    replaced.  However, if LOCK_NODEREF is used, then $FILENAME
+ *    itself is locked and later replaced, even if it is a symlink.
+ *
+ * 2. Write the new file contents to the lockfile.
+ *
+ * 3. Move the lockfile to its final destination using rename(2).
+ *
+ * Instead of (3), the change can be rolled back by deleting lockfile.
+ *
+ * This module keeps track of all locked files in lock_file_list.
+ * When the first file is locked, it registers an atexit(3) handler;
+ * when the program exits, the handler rolls back any files that have
+ * been locked but were never committed or rolled back.
+ *
+ * A lock_file object can be in several states:
+ *
+ * - Uninitialized.  In this state the object's flags field must be
+ *   zero but the rest of the 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 flags & LOCK_FLAGS_ON_LIST
+ *   is set.
+ *
+ * - Locked, lockfile open (after hold_lock_file_for_update() or
+ *   hold_lock_file_for_append()).  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 flags & LOCK_FLAGS_ON_LIST is set.
+ *
+ * See Documentation/api-lockfile.txt for more information.
+ */
+
 /* Values for lock_file::flags: */
 
 /* This lock_file instance is in the lock_file_list */
-- 
1.9.0

[PATCH 11/22] lockfile: define a constant LOCK_SUFFIX_LEN

[Michael Haggerty]

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

diff --git a/lockfile.c b/lockfile.c
index 4a9ceda..4e3ada8 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -178,14 +178,17 @@ static char *resolve_symlink(char *p, size_t s)
 	return p;
 }
 
+/* We append ".lock" to the filename to derive the lockfile name: */
+#define LOCK_SUFFIX_LEN 5
 
 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 */
@@ -301,7 +304,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;
-- 
1.9.0

Re: [PATCH 11/22] lockfile: define a constant LOCK_SUFFIX_LEN

[Junio C Hamano]

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

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  lockfile.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/lockfile.c b/lockfile.c
> index 4a9ceda..4e3ada8 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -178,14 +178,17 @@ static char *resolve_symlink(char *p, size_t s)
>  	return p;
>  }
>  
> +/* We append ".lock" to the filename to derive the lockfile name: */
> +#define LOCK_SUFFIX_LEN 5
>  
>  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;

SP{8,} in the indent; I've cleaned it up while queueing.

Thanks.

[PATCH 12/22] delete_ref_loose(): don't muck around in the lock_file's filename

[Michael Haggerty]

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

So make our own copy to work with.

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

diff --git a/refs.c b/refs.c
index 28d5eca..11ad23e 100644
--- a/refs.c
+++ b/refs.c
@@ -2485,12 +2485,14 @@ static int repack_without_ref(const char *refname)
 static int delete_ref_loose(struct ref_lock *lock, int flag)
 {
 	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
-		/* loose */
-		int err, i = strlen(lock->lk->filename) - 5; /* .lock */
-
-		lock->lk->filename[i] = 0;
-		err = unlink_or_warn(lock->lk->filename);
-		lock->lk->filename[i] = '.';
+		/*
+		 * 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) - 5);
+		int err = unlink_or_warn(loose_filename);
+		free(loose_filename);
 		if (err && errno != ENOENT)
 			return 1;
 	}
-- 
1.9.0

Re: [PATCH 12/22] delete_ref_loose(): don't muck around in the lock_file's filename

[Jeff King]

On Tue, Apr 01, 2014 at 05:58:20PM +0200, Michael Haggerty wrote:

> It's bad manners.  Especially since, if unlink_or_warn() failed, the
> memory wasn't restored to its original contents.
> 
> So make our own copy to work with.

Sounds good...

>  	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
> -		/* loose */
> -		int err, i = strlen(lock->lk->filename) - 5; /* .lock */
> -
> -		lock->lk->filename[i] = 0;
> -		err = unlink_or_warn(lock->lk->filename);
> -		lock->lk->filename[i] = '.';
> +		/*
> +		 * 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) - 5);
> +		int err = unlink_or_warn(loose_filename);
> +		free(loose_filename);

Should we be using LOCK_SUFFIX_LEN from the previous commit here?

-Peff

Re: [PATCH 12/22] delete_ref_loose(): don't muck around in the lock_file's filename

[Michael Haggerty]

On 04/01/2014 10:21 PM, Jeff King wrote:
> On Tue, Apr 01, 2014 at 05:58:20PM +0200, Michael Haggerty wrote:
> 
>> It's bad manners.  Especially since, if unlink_or_warn() failed, the
>> memory wasn't restored to its original contents.
>>
>> So make our own copy to work with.
> 
> Sounds good...
> 
>>  	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
>> -		/* loose */
>> -		int err, i = strlen(lock->lk->filename) - 5; /* .lock */
>> -
>> -		lock->lk->filename[i] = 0;
>> -		err = unlink_or_warn(lock->lk->filename);
>> -		lock->lk->filename[i] = '.';
>> +		/*
>> +		 * 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) - 5);
>> +		int err = unlink_or_warn(loose_filename);
>> +		free(loose_filename);
> 
> Should we be using LOCK_SUFFIX_LEN from the previous commit here?

LOCK_SUFFIX_LEN is not in scope to this file, and I think it should stay
that way.  But never fear; this figuring-out-filename-from-lockfile-name
nonsense is gone by the end of the patch series.

Michael

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

Re: [PATCH 12/22] delete_ref_loose(): don't muck around in the lock_file's filename

[Torsten Bögershausen]

[]
diff --git a/refs.c b/refs.c
index 28d5eca..11ad23e 100644
--- a/refs.c
+++ b/refs.c
@@ -2485,12 +2485,14 @@ static int repack_without_ref(const char *refname)
  static int delete_ref_loose(struct ref_lock *lock, int flag)
  {
  	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
-		/* loose */
-		int err, i = strlen(lock->lk->filename) - 5; /* .lock */
-
-		lock->lk->filename[i] = 0;
-		err = unlink_or_warn(lock->lk->filename);
-		lock->lk->filename[i] = '.';
+		/*
+		 * 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) - 5);

At other places (lockfile.c) we use this

+#define LOCK_SUFFIX_LEN 5

I think it makes sense to move this definition to an include file (lockfile.h ??)
and use it here.

Re: [PATCH 12/22] delete_ref_loose(): don't muck around in the lock_file's filename

[Jeff King]

On Wed, Apr 02, 2014 at 08:52:17AM +0200, Torsten Bögershausen wrote:

> +		/*
> +		 * 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) - 5);
> 
> At other places (lockfile.c) we use this
> 
> +#define LOCK_SUFFIX_LEN 5
> 
> I think it makes sense to move this definition to an include file (lockfile.h ??)
> and use it here.

I had the same comment, but if you read through to patch 18, this manual
munging goes away entirely, and the end result is much cleaner.

-Peff

[PATCH 13/22] config: change write_error() to take a (struct lock_file *) argument

[Michael Haggerty]

Reduce the amount of code that has to know about the lock_file's
filename field.

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

diff --git a/config.c b/config.c
index 6821cef..1ea3f39 100644
--- a/config.c
+++ b/config.c
@@ -1303,9 +1303,9 @@ static int store_aux(const char *key, const char *value, void *cb)
 	return 0;
 }
 
-static int write_error(const char *filename)
+static int write_error(struct lock_file *lk)
 {
-	error("failed to write new configuration file %s", filename);
+	error("failed to write new configuration file %s", lk->filename);
 
 	/* Same error code as "failed to rename". */
 	return 4;
@@ -1706,7 +1706,7 @@ out_free:
 	return ret;
 
 write_err_out:
-	ret = write_error(lock->filename);
+	ret = write_error(lock);
 	goto out_free;
 
 }
@@ -1821,7 +1821,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);
 					goto out;
 				}
 				/*
@@ -1847,7 +1847,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);
 			goto out;
 		}
 	}
-- 
1.9.0

Re: [PATCH 13/22] config: change write_error() to take a (struct lock_file *) argument

[Torsten Bögershausen]

On 04/01/2014 05:58 PM, Michael Haggerty wrote:
> Reduce the amount of code that has to know about the lock_file's
> filename field.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>   config.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/config.c b/config.c
> index 6821cef..1ea3f39 100644
> --- a/config.c
> +++ b/config.c
> @@ -1303,9 +1303,9 @@ static int store_aux(const char *key, const char *value, void *cb)
>   	return 0;
>   }
>   
> -static int write_error(const char *filename)
> +static int write_error(struct lock_file *lk)
Does the write_error() really need to know about  struct lock_file ?
(The name of the function does not indicate that it is doing something 
with lk)
And if, would it make sense to rename it into 
write_error_and_do_something() ?
>   {
> -	error("failed to write new configuration file %s", filename);
> +	error("failed to write new configuration file %s", lk->filename);
>   
>   	/* Same error code as "failed to rename". */
>   	return 4;
> @@ -1706,7 +1706,7 @@ out_free:
>   	return ret;
>   
>   write_err_out:
> -	ret = write_error(lock->filename);
> +	ret = write_error(lock);
>   	goto out_free;
>   
>   }
> @@ -1821,7 +1821,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);
>   					goto out;
>   				}
>   				/*
> @@ -1847,7 +1847,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);
>   			goto out;
>   		}
>   	}

Re: [PATCH 13/22] config: change write_error() to take a (struct lock_file *) argument

[Michael Haggerty]

On 04/02/2014 08:58 AM, Torsten Bögershausen wrote:
> On 04/01/2014 05:58 PM, Michael Haggerty wrote:
>> Reduce the amount of code that has to know about the lock_file's
>> filename field.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>   config.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/config.c b/config.c
>> index 6821cef..1ea3f39 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -1303,9 +1303,9 @@ static int store_aux(const char *key, const char
>> *value, void *cb)
>>       return 0;
>>   }
>>   -static int write_error(const char *filename)
>> +static int write_error(struct lock_file *lk)
> Does the write_error() really need to know about  struct lock_file ?
> (The name of the function does not indicate that it is doing something
> with lk)
> And if, would it make sense to rename it into
> write_error_and_do_something() ?

I'm going to leave this part out of the next re-roll, because you are
right: this change is mostly a distraction and probably not an improvement.

Michael

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

Re: [PATCH 13/22] config: change write_error() to take a (struct lock_file *) argument

[Junio C Hamano]

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

> Reduce the amount of code that has to know about the lock_file's
> filename field.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  config.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/config.c b/config.c
> index 6821cef..1ea3f39 100644
> --- a/config.c
> +++ b/config.c
> @@ -1303,9 +1303,9 @@ static int store_aux(const char *key, const char *value, void *cb)
>  	return 0;
>  }
>  
> -static int write_error(const char *filename)
> +static int write_error(struct lock_file *lk)
>  {

The earlier one would have been usable for reporting an error while
writing any file, but the caller must hold a lock file on it with a
new one.  Would this change warrant a renaming of the function, I
wonder.

It is a file-scope static, so all callers know about how they are
supposed to call it, hence the keeping the original name would be
OK, I would think.

This hunk triggered my smello-meter, primarily because "write-error"
would not be the name I would pick for this function if I were
writing everything in this file from scratch (before or after this
particular patch).

> -	error("failed to write new configuration file %s", filename);
> +	error("failed to write new configuration file %s", lk->filename);
>  
>  	/* Same error code as "failed to rename". */
>  	return 4;
> @@ -1706,7 +1706,7 @@ out_free:
>  	return ret;
>  
>  write_err_out:
> -	ret = write_error(lock->filename);
> +	ret = write_error(lock);
>  	goto out_free;
>  
>  }
> @@ -1821,7 +1821,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);
>  					goto out;
>  				}
>  				/*
> @@ -1847,7 +1847,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);
>  			goto out;
>  		}
>  	}

[PATCH 14/22] lockfile: use strbufs when handling (most) paths

[Michael Haggerty]

Change struct lock_file's filename field from a fixed-length buffer
into a strbuf.  This:

* Make buffer overflows a bit less likely

* Removes arbitrary limitations on the length of the path in some
  places, though the restrictions still have to be removed from other
  places to make a real difference

* Shortens the code

Name the new strbuf field lock_filename to make it clear that it
contains the name of the lockfile as opposed to the name of the file
being locked.  Initialize the strbuf the first time the lock_file
object is used.

Change resolve_symlink() to work on a strbuf.

Replace last_path_elm() with a new function, trim_last_path_elm(),
which operates on a strbuf.

Adjust other places in the code that refer directly to the filename.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/commit.c |  12 ++--
 builtin/reflog.c |   2 +-
 cache.h          |   2 +-
 config.c         |   3 +-
 lockfile.c       | 183 ++++++++++++++++++++++++-------------------------------
 refs.c           |   6 +-
 shallow.c        |   6 +-
 7 files changed, 97 insertions(+), 117 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d9550c5..28944de 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -330,7 +330,7 @@ static 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.lock_filename.buf, 1);
 
 		if (interactive_add(argc, argv, prefix, patch_interactive) != 0)
 			die(_("interactive add failed"));
@@ -341,10 +341,10 @@ static 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.lock_filename.buf);
 
 		commit_style = COMMIT_NORMAL;
-		return index_lock.filename;
+		return index_lock.lock_filename.buf;
 	}
 
 	/*
@@ -368,7 +368,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 		    close_lock_file(&index_lock))
 			die(_("unable to write new_index file"));
 		commit_style = COMMIT_NORMAL;
-		return index_lock.filename;
+		return index_lock.lock_filename.buf;
 	}
 
 	/*
@@ -453,9 +453,9 @@ static 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.lock_filename.buf);
 
-	return false_lock.filename;
+	return false_lock.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 c12a9784..29bc07b 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->lock_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 08d0e48..3d0a835 100644
--- a/cache.h
+++ b/cache.h
@@ -538,7 +538,7 @@ struct lock_file {
 	int fd;
 	pid_t owner;
 	unsigned char flags;
-	char filename[PATH_MAX];
+	struct strbuf lock_filename;
 };
 #define LOCK_DIE_ON_ERROR 1
 #define LOCK_NODEREF 2
diff --git a/config.c b/config.c
index 1ea3f39..cea0af0 100644
--- a/config.c
+++ b/config.c
@@ -1305,7 +1305,8 @@ static int store_aux(const char *key, const char *value, void *cb)
 
 static int write_error(struct lock_file *lk)
 {
-	error("failed to write new configuration file %s", lk->filename);
+	error("failed to write new configuration file %s",
+	      lk->lock_filename.buf);
 
 	/* Same error code as "failed to rename". */
 	return 4;
diff --git a/lockfile.c b/lockfile.c
index 4e3ada8..931ebbd 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -34,24 +34,25 @@
  * A lock_file object can be in several states:
  *
  * - Uninitialized.  In this state the object's flags field must be
- *   zero but the rest of the 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 flags & LOCK_FLAGS_ON_LIST
- *   is set.
+ *   zero but the rest of the contents need not be initialized.  In
+ *   particular, the lock_filename strbuf should *not* be initialized
+ *   externally.  The first time the object is used in any way, it is
+ *   initialized, permanently registered in the lock_file_list, and
+ *   flags & LOCK_FLAGS_ON_LIST is set.
  *
  * - Locked, lockfile open (after hold_lock_file_for_update() or
  *   hold_lock_file_for_append()).  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.
+ *   exists, lock_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 flags & LOCK_FLAGS_ON_LIST is set.
+ *   failed attempt to lock).  In this state, lock_filename is the
+ *   empty string and fd is -1.  The object is left registered in the
+ *   lock_file_list, and flags & LOCK_FLAGS_ON_LIST is set.
  *
  * See Documentation/api-lockfile.txt for more information.
  */
@@ -70,10 +71,10 @@ static void remove_lock_file(void)
 
 	while (lock_file_list) {
 		if (lock_file_list->owner == me &&
-		    lock_file_list->filename[0]) {
+		    lock_file_list->lock_filename.len) {
 			if (lock_file_list->fd >= 0)
 				close(lock_file_list->fd);
-			unlink_or_warn(lock_file_list->filename);
+			unlink_or_warn(lock_file_list->lock_filename.buf);
 		}
 		lock_file_list = lock_file_list->next;
 	}
@@ -87,95 +88,70 @@ static void remove_lock_file_on_signal(int signo)
 }
 
 /*
- * p = absolute or relative path name
+ * path = absolute or relative path name
  *
- * Return a pointer into p showing the beginning of the last path name
- * element.  If p is empty or the root directory ("/"), just return p.
+ * Remove the last path name element from path (leaving the preceding
+ * "/", if any).  If path is empty or the root directory ("/"), set
+ * path to the empty string.
  */
-static char *last_path_elm(char *p)
+static void trim_last_path_elm(struct strbuf *path)
 {
-	/* r starts pointing to null at the end of the string */
-	char *r = strchr(p, '\0');
-
-	if (r == p)
-		return p; /* just return empty string */
-
-	r--; /* back up to last non-null character */
+	int i = path->len;
 
 	/* back up past trailing slashes, if any */
-	while (r > p && *r == '/')
-		r--;
-
+	while (i && path->buf[i - 1] == '/')
+		i--;
 	/*
-	 * then go backwards until I hit a slash, or the beginning of
-	 * the string
+	 * then go backwards until a slash, or the beginning of the
+	 * string
 	 */
-	while (r > p && *(r-1) != '/')
-		r--;
-	return r;
-}
+	while (i && path->buf[i - 1] != '/')
+		i--;
 
+	strbuf_setlen(path, i);
+}
 
 /* We allow "recursive" symbolic links. Only within reason, though */
 #define MAXDEPTH 5
 
 /*
- * p = path that may be a symlink
- * s = full size of p
- *
- * 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.
+ * path contains a path that may be a symlink
  *
- * 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.
+ * 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.
  *
- * 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 path's initial contents.
  */
-
-static char *resolve_symlink(char *p, size_t s)
+static void resolve_symlink(struct strbuf *path)
 {
 	int depth = MAXDEPTH;
 
 	while (depth--) {
 		char link[PATH_MAX];
-		int link_len = readlink(p, link, sizeof(link));
+		int link_len = readlink(path->buf, link, sizeof(link));
 		if (link_len < 0) {
 			/* not a symlink anymore */
-			return p;
+			return;
 		}
-		else if (link_len < sizeof(link))
-			/* readlink() never null-terminates */
-			link[link_len] = '\0';
-		else {
-			warning("%s: symlink too long", p);
-			return p;
+		if (link_len >= sizeof(link)) {
+			warning("%s: symlink too long", path->buf);
+			return;
 		}
+		/* readlink() never null-terminates */
+		link[link_len] = '\0';
 
-		if (is_absolute_path(link)) {
-			/* absolute path simply replaces p */
-			if (link_len < s)
-				strcpy(p, link);
-			else {
-				warning("%s: symlink too long", p);
-				return p;
-			}
-		} else {
-			/*
-			 * link is a relative path, so I must replace the
-			 * last element of p with it.
-			 */
-			char *r = (char *)last_path_elm(p);
-			if (r - p + link_len < s)
-				strcpy(r, link);
-			else {
-				warning("%s: symlink too long", p);
-				return p;
-			}
-		}
+		if (is_absolute_path(link))
+			/* an absolute path replaces the whole path: */
+			strbuf_setlen(path, 0);
+		else
+			/* a relative path replaces the last element of path: */
+			trim_last_path_elm(path);
+
+		strbuf_add(path, link, link_len);
 	}
-	return p;
 }
 
 /* We append ".lock" to the filename to derive the lockfile name: */
@@ -183,12 +159,7 @@ static char *resolve_symlink(char *p, size_t s)
 
 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;
+	size_t path_len = strlen(path);
 
 	if (!lock_file_list) {
 		/* One-time initialization */
@@ -197,31 +168,39 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	}
 
 	lk->owner = getpid();
-	if (!(lk->flags & LOCK_FLAGS_ON_LIST)) {
+	if (lk->flags & LOCK_FLAGS_ON_LIST) {
+		assert(!lk->lock_filename.len); /* object not already in use */
+		if (strbuf_avail(&lk->lock_filename) < path_len + LOCK_SUFFIX_LEN)
+			strbuf_grow(&lk->lock_filename, path_len + LOCK_SUFFIX_LEN);
+	} else {
 		/* Initialize *lk and add it to lock_file_list: */
 		lk->fd = -1;
 		lk->flags |= LOCK_FLAGS_ON_LIST;
-		lk->filename[0] = 0;
+		strbuf_init(&lk->lock_filename, path_len + LOCK_SUFFIX_LEN);
 		lk->next = lock_file_list;
 		lock_file_list = lk;
 	}
 
-	if (strlen(path) >= max_path_len)
-		return -1;
-	strcpy(lk->filename, path);
+	strbuf_add(&lk->lock_filename, path, path_len);
 	if (!(flags & LOCK_NODEREF))
-		resolve_symlink(lk->filename, max_path_len);
-	strcat(lk->filename, ".lock");
-	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
+		resolve_symlink(&lk->lock_filename);
+	strbuf_addstr(&lk->lock_filename, ".lock");
+
+	if (lk->lock_filename.len >= PATH_MAX) {
+		error("%s: path too long", lk->lock_filename.buf);
+		strbuf_setlen(&lk->lock_filename, 0);
+		return -1;
+	}
+	lk->fd = open(lk->lock_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= lk->fd) {
-		if (adjust_shared_perm(lk->filename)) {
-			error("cannot fix permission bits on %s", lk->filename);
+		if (adjust_shared_perm(lk->lock_filename.buf)) {
+			error("cannot fix permission bits on %s", lk->lock_filename.buf);
 			rollback_lock_file(lk);
 			return -1;
 		}
 	}
 	else
-		lk->filename[0] = 0;
+		strbuf_setlen(&lk->lock_filename, 0);
 	return lk->fd;
 }
 
@@ -300,15 +279,15 @@ int close_lock_file(struct lock_file *lk)
 int commit_lock_file(struct lock_file *lk)
 {
 	char result_file[PATH_MAX];
-	size_t i;
+	size_t path_len = lk->lock_filename.len - LOCK_SUFFIX_LEN;
+
 	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;
-	if (rename(lk->filename, result_file))
+	memcpy(result_file, lk->lock_filename.buf, path_len);
+	result_file[path_len] = '\0';
+	if (rename(lk->lock_filename.buf, result_file))
 		return -1;
-	lk->filename[0] = 0;
+	strbuf_setlen(&lk->lock_filename, 0);
 	return 0;
 }
 
@@ -330,9 +309,9 @@ 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->lock_filename.buf, alternate_index_output))
 			return -1;
-		lk->filename[0] = 0;
+		strbuf_setlen(&lk->lock_filename, 0);
 		return 0;
 	}
 	else
@@ -341,10 +320,10 @@ int commit_locked_index(struct lock_file *lk)
 
 void rollback_lock_file(struct lock_file *lk)
 {
-	if (lk->filename[0]) {
+	if (lk->lock_filename.len) {
 		if (lk->fd >= 0)
 			close_lock_file(lk);
-		unlink_or_warn(lk->filename);
-		lk->filename[0] = 0;
+		unlink_or_warn(lk->lock_filename.buf);
+		strbuf_setlen(&lk->lock_filename, 0);
 	}
 }
diff --git a/refs.c b/refs.c
index 11ad23e..93abc94 100644
--- a/refs.c
+++ b/refs.c
@@ -2489,8 +2489,8 @@ 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) - 5);
+		char *loose_filename = xmemdupz(lock->lk->lock_filename.buf,
+						lock->lk->lock_filename.len - 5);
 		int err = unlink_or_warn(loose_filename);
 		free(loose_filename);
 		if (err && errno != ENOENT)
@@ -2828,7 +2828,7 @@ int write_ref_sha1(struct ref_lock *lock,
 	if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||
 	    write_in_full(lock->lock_fd, &term, 1) != 1
 		|| close_ref(lock) < 0) {
-		error("Couldn't write %s", lock->lk->filename);
+		error("Couldn't write %s", lock->lk->lock_filename.buf);
 		unlock_ref(lock);
 		return -1;
 	}
diff --git a/shallow.c b/shallow.c
index 0b267b6..a27d587 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->lock_filename.buf);
+		*alternate_shallow_file = shallow_lock->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.lock_filename.buf);
 		commit_lock_file(&shallow_lock);
 	} else {
 		unlink(git_path("shallow"));
-- 
1.9.0

Re: [PATCH 14/22] lockfile: use strbufs when handling (most) paths

[Jeff King]

On Tue, Apr 01, 2014 at 05:58:22PM +0200, Michael Haggerty wrote:

>  /*
> - * p = path that may be a symlink
> - * s = full size of p
> - *
> - * 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.
> + * path contains a path that may be a symlink
>   *
> - * 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.
> + * 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.
>   *
> - * 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 path's initial contents.
>   */
> -
> -static char *resolve_symlink(char *p, size_t s)
> +static void resolve_symlink(struct strbuf *path)
> [...]

This is not a problem you are introducing, but do we really want
resolve_symlink to be best-effort here?

What happens if "a" is a symlink to "b", and two processes try to lock
"a" simultaneously? If one succeeds, it will take "b.lock". But the
other will take "a.lock", and both will think they have the target
locked.

I suspect we need to recognize ENOENT for cases where we are creating
the file for the first time, but it seems like we should bail on locking
from any other transient errors.

-Peff

Re: [PATCH 14/22] lockfile: use strbufs when handling (most) paths

[Junio C Hamano]

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

> Change struct lock_file's filename field from a fixed-length buffer
> into a strbuf.

Good.

As I allued to in a review on an unrelated patch, I do not think it
is a good idea to name the lock filename field "lock_filename" in a
structure that is about a lockfile, though.

[PATCH 15/22] resolve_symlink(): use a strbuf internally

[Michael Haggerty]

This removes another place where the path name is arbitrarily limited.

This gets rid of a warning

    %s: symlink too long

even though strbuf_readlink() in fact still limits the length of what
it can handle to something like 2*PATH_MAX.  But (1) the limit is now
much longer, and (2) strbuf_readlink() doesn't return enough
information to distinguish this problem from other symlink-reading
problems, so just let it go.

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

diff --git a/lockfile.c b/lockfile.c
index 931ebbd..0ade314 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -128,30 +128,25 @@ static void trim_last_path_elm(struct strbuf *path)
 static void resolve_symlink(struct strbuf *path)
 {
 	int depth = MAXDEPTH;
+	struct strbuf link;
+
+	strbuf_init(&link, path->len);
 
 	while (depth--) {
-		char link[PATH_MAX];
-		int link_len = readlink(path->buf, link, sizeof(link));
-		if (link_len < 0) {
-			/* not a symlink anymore */
-			return;
-		}
-		if (link_len >= sizeof(link)) {
-			warning("%s: symlink too long", path->buf);
-			return;
-		}
-		/* readlink() never null-terminates */
-		link[link_len] = '\0';
+		if (strbuf_readlink(&link, path->buf, path->len) < 0)
+			break;
 
-		if (is_absolute_path(link))
+		if (is_absolute_path(link.buf))
 			/* an absolute path replaces the whole path: */
 			strbuf_setlen(path, 0);
 		else
 			/* a relative path replaces the last element of path: */
 			trim_last_path_elm(path);
 
-		strbuf_add(path, link, link_len);
+		strbuf_addbuf(path, &link);
 	}
+
+	strbuf_release(&link);
 }
 
 /* We append ".lock" to the filename to derive the lockfile name: */
-- 
1.9.0

[PATCH 16/22] commit_lock_file(): don't work with a fixed-length buffer

[Michael Haggerty]

Allocate temporary space to hold result_file instead of storing it in
a fixed-length buffer.  This removes the last arbitrary path-length
limitation, so remove the path length check in lock_file().

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

diff --git a/lockfile.c b/lockfile.c
index 0ade314..c1ca5b1 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -181,11 +181,6 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 		resolve_symlink(&lk->lock_filename);
 	strbuf_addstr(&lk->lock_filename, ".lock");
 
-	if (lk->lock_filename.len >= PATH_MAX) {
-		error("%s: path too long", lk->lock_filename.buf);
-		strbuf_setlen(&lk->lock_filename, 0);
-		return -1;
-	}
 	lk->fd = open(lk->lock_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= lk->fd) {
 		if (adjust_shared_perm(lk->lock_filename.buf)) {
@@ -273,17 +268,19 @@ int close_lock_file(struct lock_file *lk)
 
 int commit_lock_file(struct lock_file *lk)
 {
-	char result_file[PATH_MAX];
+	char *result_file;
 	size_t path_len = lk->lock_filename.len - LOCK_SUFFIX_LEN;
+	int err = 0;
 
 	if (lk->fd >= 0 && close_lock_file(lk))
 		return -1;
-	memcpy(result_file, lk->lock_filename.buf, path_len);
-	result_file[path_len] = '\0';
+	result_file = xmemdupz(lk->lock_filename.buf, path_len);
 	if (rename(lk->lock_filename.buf, result_file))
-		return -1;
-	strbuf_setlen(&lk->lock_filename, 0);
-	return 0;
+		err = -1;
+	else
+		strbuf_setlen(&lk->lock_filename, 0);
+	free(result_file);
+	return err;
 }
 
 int hold_locked_index(struct lock_file *lk, int die_on_error)
-- 
1.9.0

[PATCH 17/22] lock_file(): exit early if lockfile cannot be opened

[Michael Haggerty]

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

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

diff --git a/lockfile.c b/lockfile.c
index c1ca5b1..87b40c4 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -182,15 +182,16 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	strbuf_addstr(&lk->lock_filename, ".lock");
 
 	lk->fd = open(lk->lock_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
-	if (0 <= lk->fd) {
-		if (adjust_shared_perm(lk->lock_filename.buf)) {
-			error("cannot fix permission bits on %s", lk->lock_filename.buf);
-			rollback_lock_file(lk);
-			return -1;
-		}
-	}
-	else
+	if (lk->fd < 0) {
 		strbuf_setlen(&lk->lock_filename, 0);
+		return -1;
+	}
+	if (adjust_shared_perm(lk->lock_filename.buf)) {
+		error("cannot fix permission bits on %s", lk->lock_filename.buf);
+		rollback_lock_file(lk);
+		return -1;
+	}
+
 	return lk->fd;
 }
 
-- 
1.9.0

[PATCH 18/22] lockfile: also keep track of the filename of the file being locked

[Michael Haggerty]

This reduces the amount of string editing gyrations and makes it
unnecessary for callers to know how to derive the filename from the
lock_filename.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 cache.h    |  1 +
 lockfile.c | 57 ++++++++++++++++++++++++++++++++-------------------------
 refs.c     | 10 ++--------
 3 files changed, 35 insertions(+), 33 deletions(-)

diff --git a/cache.h b/cache.h
index 3d0a835..0fefc66 100644
--- a/cache.h
+++ b/cache.h
@@ -538,6 +538,7 @@ struct lock_file {
 	int fd;
 	pid_t owner;
 	unsigned char flags;
+	struct strbuf filename;
 	struct strbuf lock_filename;
 };
 #define LOCK_DIE_ON_ERROR 1
diff --git a/lockfile.c b/lockfile.c
index 87b40c4..07b5c21 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -35,24 +35,26 @@
  *
  * - Uninitialized.  In this state the object's flags field must be
  *   zero but the rest of the contents need not be initialized.  In
- *   particular, the lock_filename strbuf should *not* be initialized
- *   externally.  The first time the object is used in any way, it is
- *   initialized, permanently registered in the lock_file_list, and
- *   flags & LOCK_FLAGS_ON_LIST is set.
+ *   particular, the filename and lock_filename strbufs should *not*
+ *   be initialized externally.  The first time the object is used in
+ *   any way, it is initialized, permanently registered in the
+ *   lock_file_list, and flags & LOCK_FLAGS_ON_LIST is set.
  *
  * - Locked, lockfile open (after hold_lock_file_for_update() or
  *   hold_lock_file_for_append()).  In this state, the lockfile
- *   exists, lock_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.
+ *   exists, filename holds the filename of the locked file,
+ *   lock_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, lock_filename is the
- *   empty string and fd is -1.  The object is left registered in the
- *   lock_file_list, and flags & LOCK_FLAGS_ON_LIST is set.
+ *   failed attempt to lock).  In this state, filename and
+ *   lock_filename are the empty string and fd is -1.  The object is
+ *   left registered in the lock_file_list, and flags &
+ *   LOCK_FLAGS_ON_LIST is set.
  *
  * See Documentation/api-lockfile.txt for more information.
  */
@@ -164,25 +166,30 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 
 	lk->owner = getpid();
 	if (lk->flags & LOCK_FLAGS_ON_LIST) {
-		assert(!lk->lock_filename.len); /* object not already in use */
-		if (strbuf_avail(&lk->lock_filename) < path_len + LOCK_SUFFIX_LEN)
-			strbuf_grow(&lk->lock_filename, path_len + LOCK_SUFFIX_LEN);
+		/* Sanity check that object is not already in use: */
+		assert(!lk->filename.len);
+		assert(!lk->lock_filename.len);
 	} else {
 		/* Initialize *lk and add it to lock_file_list: */
 		lk->fd = -1;
 		lk->flags |= LOCK_FLAGS_ON_LIST;
-		strbuf_init(&lk->lock_filename, path_len + LOCK_SUFFIX_LEN);
+		strbuf_init(&lk->filename, path_len);
+		strbuf_init(&lk->lock_filename, 0);
 		lk->next = lock_file_list;
 		lock_file_list = lk;
 	}
 
-	strbuf_add(&lk->lock_filename, path, path_len);
+	strbuf_add(&lk->filename, path, path_len);
 	if (!(flags & LOCK_NODEREF))
-		resolve_symlink(&lk->lock_filename);
+		resolve_symlink(&lk->filename);
+
+	strbuf_grow(&lk->lock_filename, lk->filename.len + LOCK_SUFFIX_LEN);
+	strbuf_addbuf(&lk->lock_filename, &lk->filename);
 	strbuf_addstr(&lk->lock_filename, ".lock");
 
 	lk->fd = open(lk->lock_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (lk->fd < 0) {
+		strbuf_setlen(&lk->filename, 0);
 		strbuf_setlen(&lk->lock_filename, 0);
 		return -1;
 	}
@@ -269,18 +276,16 @@ int close_lock_file(struct lock_file *lk)
 
 int commit_lock_file(struct lock_file *lk)
 {
-	char *result_file;
-	size_t path_len = lk->lock_filename.len - LOCK_SUFFIX_LEN;
 	int err = 0;
 
 	if (lk->fd >= 0 && close_lock_file(lk))
 		return -1;
-	result_file = xmemdupz(lk->lock_filename.buf, path_len);
-	if (rename(lk->lock_filename.buf, result_file))
+	if (rename(lk->lock_filename.buf, lk->filename.buf)) {
 		err = -1;
-	else
+	} else {
+		strbuf_setlen(&lk->filename, 0);
 		strbuf_setlen(&lk->lock_filename, 0);
-	free(result_file);
+	}
 	return err;
 }
 
@@ -304,19 +309,21 @@ int commit_locked_index(struct lock_file *lk)
 			return -1;
 		if (rename(lk->lock_filename.buf, alternate_index_output))
 			return -1;
+		strbuf_setlen(&lk->filename, 0);
 		strbuf_setlen(&lk->lock_filename, 0);
 		return 0;
-	}
-	else
+	} else {
 		return commit_lock_file(lk);
+	}
 }
 
 void rollback_lock_file(struct lock_file *lk)
 {
-	if (lk->lock_filename.len) {
+	if (lk->filename.len) {
 		if (lk->fd >= 0)
 			close_lock_file(lk);
 		unlink_or_warn(lk->lock_filename.buf);
+		strbuf_setlen(&lk->filename, 0);
 		strbuf_setlen(&lk->lock_filename, 0);
 	}
 }
diff --git a/refs.c b/refs.c
index 93abc94..85967e7 100644
--- a/refs.c
+++ b/refs.c
@@ -2485,14 +2485,8 @@ static int repack_without_ref(const char *refname)
 static int delete_ref_loose(struct ref_lock *lock, int flag)
 {
 	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
-		/*
-		 * loose.  The loose file name is the same as the
-		 * lockfile name, minus ".lock":
-		 */
-		char *loose_filename = xmemdupz(lock->lk->lock_filename.buf,
-						lock->lk->lock_filename.len - 5);
-		int err = unlink_or_warn(loose_filename);
-		free(loose_filename);
+		/* loose */
+		int err = unlink_or_warn(lock->lk->filename.buf);
 		if (err && errno != ENOENT)
 			return 1;
 	}
-- 
1.9.0

Re: [PATCH 18/22] lockfile: also keep track of the filename of the file being locked

[Junio C Hamano]

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

> This reduces the amount of string editing gyrations and makes it
> unnecessary for callers to know how to derive the filename from the
> lock_filename.

Hmph.

Is it worth duplicating the whole thing?  If you are planning to
break the invariant lock_filename === filename + LOCK_SUFFIX in
future changes in the series, it is understandable, though.

Re: [PATCH 18/22] lockfile: also keep track of the filename of the file being locked

[Michael Haggerty]

On 04/02/2014 07:19 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> This reduces the amount of string editing gyrations and makes it
>> unnecessary for callers to know how to derive the filename from the
>> lock_filename.
> 
> Hmph.
> 
> Is it worth duplicating the whole thing?  If you are planning to
> break the invariant lock_filename === filename + LOCK_SUFFIX in
> future changes in the series, it is understandable, though.

That is indeed the plan, but I'm splitting that change into a future
patch series because this one is getting big enough already dealing with
correctness issues.

Michael

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

[PATCH 19/22] struct lock_file: rename lock_filename field to staging_filename

[Michael Haggerty]

Now that the filename is stored separately, the external users of this
field only use it as the filename to which they are trying to write.
Soon it will not necessarily be the name of the lock file, so rename
the field more generically.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/commit.c | 12 ++++++------
 builtin/reflog.c |  2 +-
 cache.h          |  2 +-
 config.c         |  2 +-
 lockfile.c       | 53 +++++++++++++++++++++++++++--------------------------
 refs.c           |  2 +-
 shallow.c        |  6 +++---
 7 files changed, 40 insertions(+), 39 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 28944de..6557bde 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -330,7 +330,7 @@ static 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.lock_filename.buf, 1);
+		setenv(INDEX_ENVIRONMENT, index_lock.staging_filename.buf, 1);
 
 		if (interactive_add(argc, argv, prefix, patch_interactive) != 0)
 			die(_("interactive add failed"));
@@ -341,10 +341,10 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 			unsetenv(INDEX_ENVIRONMENT);
 
 		discard_cache();
-		read_cache_from(index_lock.lock_filename.buf);
+		read_cache_from(index_lock.staging_filename.buf);
 
 		commit_style = COMMIT_NORMAL;
-		return index_lock.lock_filename.buf;
+		return index_lock.staging_filename.buf;
 	}
 
 	/*
@@ -368,7 +368,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 		    close_lock_file(&index_lock))
 			die(_("unable to write new_index file"));
 		commit_style = COMMIT_NORMAL;
-		return index_lock.lock_filename.buf;
+		return index_lock.staging_filename.buf;
 	}
 
 	/*
@@ -453,9 +453,9 @@ static 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.lock_filename.buf);
+	read_cache_from(false_lock.staging_filename.buf);
 
-	return false_lock.lock_filename.buf;
+	return false_lock.staging_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 29bc07b..08fd6b3 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->lock_filename.buf);
+				lock->lk->staging_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 0fefc66..3b9dbd4 100644
--- a/cache.h
+++ b/cache.h
@@ -539,7 +539,7 @@ struct lock_file {
 	pid_t owner;
 	unsigned char flags;
 	struct strbuf filename;
-	struct strbuf lock_filename;
+	struct strbuf staging_filename;
 };
 #define LOCK_DIE_ON_ERROR 1
 #define LOCK_NODEREF 2
diff --git a/config.c b/config.c
index cea0af0..c39bd31 100644
--- a/config.c
+++ b/config.c
@@ -1306,7 +1306,7 @@ static int store_aux(const char *key, const char *value, void *cb)
 static int write_error(struct lock_file *lk)
 {
 	error("failed to write new configuration file %s",
-	      lk->lock_filename.buf);
+	      lk->staging_filename.buf);
 
 	/* Same error code as "failed to rename". */
 	return 4;
diff --git a/lockfile.c b/lockfile.c
index 07b5c21..3974137 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -35,26 +35,26 @@
  *
  * - Uninitialized.  In this state the object's flags field must be
  *   zero but the rest of the contents need not be initialized.  In
- *   particular, the filename and lock_filename strbufs should *not*
- *   be initialized externally.  The first time the object is used in
- *   any way, it is initialized, permanently registered in the
+ *   particular, the filename and staging_filename strbufs should
+ *   *not* be initialized externally.  The first time the object is
+ *   used in any way, it is initialized, permanently registered in the
  *   lock_file_list, and flags & LOCK_FLAGS_ON_LIST is set.
  *
  * - Locked, lockfile open (after hold_lock_file_for_update() or
  *   hold_lock_file_for_append()).  In this state, the lockfile
  *   exists, filename holds the filename of the locked file,
- *   lock_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.
+ *   staging_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 and
- *   lock_filename are the empty string and fd is -1.  The object is
- *   left registered in the lock_file_list, and flags &
- *   LOCK_FLAGS_ON_LIST is set.
+ *   staging_filename are the empty string and fd is -1.  The object is left
+ *   registered in the lock_file_list, and flags & LOCK_FLAGS_ON_LIST
+ *   is set.
  *
  * See Documentation/api-lockfile.txt for more information.
  */
@@ -73,10 +73,10 @@ static void remove_lock_file(void)
 
 	while (lock_file_list) {
 		if (lock_file_list->owner == me &&
-		    lock_file_list->lock_filename.len) {
+		    lock_file_list->staging_filename.len) {
 			if (lock_file_list->fd >= 0)
 				close(lock_file_list->fd);
-			unlink_or_warn(lock_file_list->lock_filename.buf);
+			unlink_or_warn(lock_file_list->staging_filename.buf);
 		}
 		lock_file_list = lock_file_list->next;
 	}
@@ -168,13 +168,13 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	if (lk->flags & LOCK_FLAGS_ON_LIST) {
 		/* Sanity check that object is not already in use: */
 		assert(!lk->filename.len);
-		assert(!lk->lock_filename.len);
+		assert(!lk->staging_filename.len);
 	} else {
 		/* Initialize *lk and add it to lock_file_list: */
 		lk->fd = -1;
 		lk->flags |= LOCK_FLAGS_ON_LIST;
 		strbuf_init(&lk->filename, path_len);
-		strbuf_init(&lk->lock_filename, 0);
+		strbuf_init(&lk->staging_filename, 0);
 		lk->next = lock_file_list;
 		lock_file_list = lk;
 	}
@@ -183,18 +183,19 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	if (!(flags & LOCK_NODEREF))
 		resolve_symlink(&lk->filename);
 
-	strbuf_grow(&lk->lock_filename, lk->filename.len + LOCK_SUFFIX_LEN);
-	strbuf_addbuf(&lk->lock_filename, &lk->filename);
-	strbuf_addstr(&lk->lock_filename, ".lock");
+	strbuf_grow(&lk->staging_filename, lk->filename.len + LOCK_SUFFIX_LEN);
+	strbuf_addbuf(&lk->staging_filename, &lk->filename);
+	strbuf_addstr(&lk->staging_filename, ".lock");
 
-	lk->fd = open(lk->lock_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
+	lk->fd = open(lk->staging_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (lk->fd < 0) {
 		strbuf_setlen(&lk->filename, 0);
-		strbuf_setlen(&lk->lock_filename, 0);
+		strbuf_setlen(&lk->staging_filename, 0);
 		return -1;
 	}
-	if (adjust_shared_perm(lk->lock_filename.buf)) {
-		error("cannot fix permission bits on %s", lk->lock_filename.buf);
+	if (adjust_shared_perm(lk->staging_filename.buf)) {
+		error("cannot fix permission bits on %s",
+		      lk->staging_filename.buf);
 		rollback_lock_file(lk);
 		return -1;
 	}
@@ -280,11 +281,11 @@ int commit_lock_file(struct lock_file *lk)
 
 	if (lk->fd >= 0 && close_lock_file(lk))
 		return -1;
-	if (rename(lk->lock_filename.buf, lk->filename.buf)) {
+	if (rename(lk->staging_filename.buf, lk->filename.buf)) {
 		err = -1;
 	} else {
 		strbuf_setlen(&lk->filename, 0);
-		strbuf_setlen(&lk->lock_filename, 0);
+		strbuf_setlen(&lk->staging_filename, 0);
 	}
 	return err;
 }
@@ -307,10 +308,10 @@ 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->lock_filename.buf, alternate_index_output))
+		if (rename(lk->staging_filename.buf, alternate_index_output))
 			return -1;
 		strbuf_setlen(&lk->filename, 0);
-		strbuf_setlen(&lk->lock_filename, 0);
+		strbuf_setlen(&lk->staging_filename, 0);
 		return 0;
 	} else {
 		return commit_lock_file(lk);
@@ -322,8 +323,8 @@ void rollback_lock_file(struct lock_file *lk)
 	if (lk->filename.len) {
 		if (lk->fd >= 0)
 			close_lock_file(lk);
-		unlink_or_warn(lk->lock_filename.buf);
+		unlink_or_warn(lk->staging_filename.buf);
 		strbuf_setlen(&lk->filename, 0);
-		strbuf_setlen(&lk->lock_filename, 0);
+		strbuf_setlen(&lk->staging_filename, 0);
 	}
 }
diff --git a/refs.c b/refs.c
index 85967e7..47a49a0 100644
--- a/refs.c
+++ b/refs.c
@@ -2822,7 +2822,7 @@ int write_ref_sha1(struct ref_lock *lock,
 	if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||
 	    write_in_full(lock->lock_fd, &term, 1) != 1
 		|| close_ref(lock) < 0) {
-		error("Couldn't write %s", lock->lk->lock_filename.buf);
+		error("Couldn't write %s", lock->lk->staging_filename.buf);
 		unlock_ref(lock);
 		return -1;
 	}
diff --git a/shallow.c b/shallow.c
index a27d587..76dc6df 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->lock_filename.buf);
-		*alternate_shallow_file = shallow_lock->lock_filename.buf;
+				  shallow_lock->staging_filename.buf);
+		*alternate_shallow_file = shallow_lock->staging_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.lock_filename.buf);
+				  shallow_lock.staging_filename.buf);
 		commit_lock_file(&shallow_lock);
 	} else {
 		unlink(git_path("shallow"));
-- 
1.9.0

[PATCH 20/22] remove_lock_file(): call rollback_lock_file()

[Michael Haggerty]

It does exactly 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 3974137..852d717 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -72,12 +72,8 @@ static void remove_lock_file(void)
 	pid_t me = getpid();
 
 	while (lock_file_list) {
-		if (lock_file_list->owner == me &&
-		    lock_file_list->staging_filename.len) {
-			if (lock_file_list->fd >= 0)
-				close(lock_file_list->fd);
-			unlink_or_warn(lock_file_list->staging_filename.buf);
-		}
+		if (lock_file_list->owner == me)
+			rollback_lock_file(lock_file_list);
 		lock_file_list = lock_file_list->next;
 	}
 }
-- 
1.9.0

[PATCH 21/22] lockfile: extract a function reset_lock_file()

[Michael Haggerty]

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

diff --git a/lockfile.c b/lockfile.c
index 852d717..c06e134 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -85,6 +85,14 @@ static void remove_lock_file_on_signal(int signo)
 	raise(signo);
 }
 
+static void reset_lock_file(struct lock_file *lk)
+{
+	lk->fd = -1;
+	strbuf_setlen(&lk->filename, 0);
+	strbuf_setlen(&lk->staging_filename, 0);
+	lk->flags = LOCK_FLAGS_ON_LIST;
+}
+
 /*
  * path = absolute or relative path name
  *
@@ -185,8 +193,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 
 	lk->fd = open(lk->staging_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (lk->fd < 0) {
-		strbuf_setlen(&lk->filename, 0);
-		strbuf_setlen(&lk->staging_filename, 0);
+		reset_lock_file(lk);
 		return -1;
 	}
 	if (adjust_shared_perm(lk->staging_filename.buf)) {
@@ -273,17 +280,12 @@ int close_lock_file(struct lock_file *lk)
 
 int commit_lock_file(struct lock_file *lk)
 {
-	int err = 0;
-
 	if (lk->fd >= 0 && close_lock_file(lk))
 		return -1;
-	if (rename(lk->staging_filename.buf, lk->filename.buf)) {
-		err = -1;
-	} else {
-		strbuf_setlen(&lk->filename, 0);
-		strbuf_setlen(&lk->staging_filename, 0);
-	}
-	return err;
+	if (rename(lk->staging_filename.buf, lk->filename.buf))
+		return -1;
+	reset_lock_file(lk);
+	return 0;
 }
 
 int hold_locked_index(struct lock_file *lk, int die_on_error)
@@ -306,8 +308,8 @@ int commit_locked_index(struct lock_file *lk)
 			return -1;
 		if (rename(lk->staging_filename.buf, alternate_index_output))
 			return -1;
-		strbuf_setlen(&lk->filename, 0);
-		strbuf_setlen(&lk->staging_filename, 0);
+
+		reset_lock_file(lk);
 		return 0;
 	} else {
 		return commit_lock_file(lk);
@@ -320,7 +322,6 @@ void rollback_lock_file(struct lock_file *lk)
 		if (lk->fd >= 0)
 			close_lock_file(lk);
 		unlink_or_warn(lk->staging_filename.buf);
-		strbuf_setlen(&lk->filename, 0);
-		strbuf_setlen(&lk->staging_filename, 0);
+		reset_lock_file(lk);
 	}
 }
-- 
1.9.0

Re: [PATCH 21/22] lockfile: extract a function reset_lock_file()

[Eric Sunshine]

On Tue, Apr 1, 2014 at 11:58 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  lockfile.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/lockfile.c b/lockfile.c
> index 852d717..c06e134 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -85,6 +85,14 @@ static void remove_lock_file_on_signal(int signo)
>         raise(signo);
>  }
>
> +static void reset_lock_file(struct lock_file *lk)
> +{
> +       lk->fd = -1;
> +       strbuf_setlen(&lk->filename, 0);
> +       strbuf_setlen(&lk->staging_filename, 0);

strbuf_reset() perhaps?

> +       lk->flags = LOCK_FLAGS_ON_LIST;
> +}
> +
>  /*
>   * path = absolute or relative path name
>   *
> @@ -185,8 +193,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>
>         lk->fd = open(lk->staging_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
>         if (lk->fd < 0) {
> -               strbuf_setlen(&lk->filename, 0);
> -               strbuf_setlen(&lk->staging_filename, 0);
> +               reset_lock_file(lk);
>                 return -1;
>         }
>         if (adjust_shared_perm(lk->staging_filename.buf)) {
> @@ -273,17 +280,12 @@ int close_lock_file(struct lock_file *lk)
>
>  int commit_lock_file(struct lock_file *lk)
>  {
> -       int err = 0;
> -
>         if (lk->fd >= 0 && close_lock_file(lk))
>                 return -1;
> -       if (rename(lk->staging_filename.buf, lk->filename.buf)) {
> -               err = -1;
> -       } else {
> -               strbuf_setlen(&lk->filename, 0);
> -               strbuf_setlen(&lk->staging_filename, 0);
> -       }
> -       return err;
> +       if (rename(lk->staging_filename.buf, lk->filename.buf))
> +               return -1;
> +       reset_lock_file(lk);
> +       return 0;
>  }
>
>  int hold_locked_index(struct lock_file *lk, int die_on_error)
> @@ -306,8 +308,8 @@ int commit_locked_index(struct lock_file *lk)
>                         return -1;
>                 if (rename(lk->staging_filename.buf, alternate_index_output))
>                         return -1;
> -               strbuf_setlen(&lk->filename, 0);
> -               strbuf_setlen(&lk->staging_filename, 0);
> +
> +               reset_lock_file(lk);
>                 return 0;
>         } else {
>                 return commit_lock_file(lk);
> @@ -320,7 +322,6 @@ void rollback_lock_file(struct lock_file *lk)
>                 if (lk->fd >= 0)
>                         close_lock_file(lk);
>                 unlink_or_warn(lk->staging_filename.buf);
> -               strbuf_setlen(&lk->filename, 0);
> -               strbuf_setlen(&lk->staging_filename, 0);
> +               reset_lock_file(lk);
>         }
>  }
> --
> 1.9.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 21/22] lockfile: extract a function reset_lock_file()

[Michael Haggerty]

On 04/02/2014 09:06 AM, Eric Sunshine wrote:
> On Tue, Apr 1, 2014 at 11:58 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  lockfile.c | 31 ++++++++++++++++---------------
>>  1 file changed, 16 insertions(+), 15 deletions(-)
>>
>> diff --git a/lockfile.c b/lockfile.c
>> index 852d717..c06e134 100644
>> --- a/lockfile.c
>> +++ b/lockfile.c
>> @@ -85,6 +85,14 @@ static void remove_lock_file_on_signal(int signo)
>>         raise(signo);
>>  }
>>
>> +static void reset_lock_file(struct lock_file *lk)
>> +{
>> +       lk->fd = -1;
>> +       strbuf_setlen(&lk->filename, 0);
>> +       strbuf_setlen(&lk->staging_filename, 0);
> 
> strbuf_reset() perhaps?

Thanks, Eric.  For some reason I always have it in the back of my head
that strbuf_reset() frees the memory associated with the strbuf, but of
course that is strbuf_release() that I'm thinking of.

I just fixed all strbuf_setlen(..., 0) -> strbuf_reset(...) throughout
the patch series.  The fix will be in the re-roll.

Michael

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

[PATCH 22/22] lockfile: allow new file contents to be written while retaining lock

[Michael Haggerty]

Add a new option flag, LOCK_SEPARATE_STAGING_FILE, that can be passed
to hold_lock_file_for_update() or hold_lock_file_for_append() to use a
staging file that is independent of the lock file.

Add a new function activate_staging_file() that activates the contents
that have been written to the staging file without releasing the lock.

This functionality can be used to ensure that changes to two files are
seen by other processes in one order even if correctness requires the
locks to be released in another order.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/technical/api-lockfile.txt |  54 ++++++++----
 cache.h                                  |   4 +
 lockfile.c                               | 140 ++++++++++++++++++++++++++-----
 3 files changed, 162 insertions(+), 36 deletions(-)

diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index 95ed03b..06b49f9 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -43,6 +43,13 @@ 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.
+
+LOCK_SEPARATE_STAGING_FILE::
+
+	Use a separate file (not the lockfile) for staging the new
+	file contents.  This option makes it possible to call
+	activate_staging_file() to overwrite the old file while
+	retaining the file lock.
 --
 
 hold_lock_file_for_append::
@@ -64,23 +71,39 @@ unable_to_lock_index_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).
+	Take a pointer to the `struct lock_file` initialized with an
+	earlier call to `hold_lock_file_for_update()` or
+	`hold_lock_file_for_append()`.  Close the file descriptor.
+	Then, if the lock was created with the
+	LOCK_SEPARATE_STAGING_FILE flag, rename the staging file on
+	top of the locked file (if `activate_staging_file()` hasn't
+	already been called) and delete the lockfile; otherwise,
+	rename the lockfile to its final destination.  Returns 0 upon
+	success, a negative value on failure to close(2) or rename(2).
+
+activate_staging_file::
+
+	Write the contents of the separate staging file over those of
+	the locked file while retaining the lock.  This function may
+	only be called if the lock was created with the
+	LOCK_SEPARATE_STAGING_FILE flag.
 
 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.
+	Take a pointer to the `struct lock_file` initialized with an
+	earlier call to `hold_lock_file_for_update()`, close the file
+	descriptor, remove the separate staging file (if any), and
+	remove the lockfile.  Please note that if
+	`activate_staging_file()` has already been called, then the
+	contents committed at that time will remain active.
 
 close_lock_file::
 	Take a pointer to the `struct lock_file` initialized
 	with an earlier call to `hold_lock_file_for_update()`,
 	and close the file descriptor.  Returns 0 upon success,
-	a negative value on failure to close(2).
+	a negative value on failure to close(2).  This function must
+	only be called if the lock was created without the
+	LOCK_SEPARATE_STAGING_FILE flag.
 
 Because the structure is used in an `atexit(3)` handler, its
 storage has to stay throughout the life of the program.  It
@@ -94,11 +117,10 @@ 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
 `close_lock_file()`.  You should never call `close(2)` yourself!
-Otherwise the `struct
-lock_file` structure still remembers that the file descriptor
-needs to be closed, and a later call to `commit_lock_file()` or
-`rollback_lock_file()` will result in duplicate calls to
-`close(2)`.  Worse yet, if you `close(2)`, open another file
-descriptor for completely different purpose, and then call
-`commit_lock_file()` or `rollback_lock_file()`, they may close
+Otherwise the `struct lock_file` structure still remembers that the
+file descriptor needs to be closed, and a later call to
+`commit_lock_file()` or `rollback_lock_file()` will result in
+duplicate calls to `close(2)`.  Worse yet, if you `close(2)`, open
+another file descriptor for completely different purpose, and then
+call `commit_lock_file()` or `rollback_lock_file()`, they may close
 that unrelated file descriptor.
diff --git a/cache.h b/cache.h
index 3b9dbd4..50fdc68 100644
--- a/cache.h
+++ b/cache.h
@@ -541,12 +541,16 @@ struct lock_file {
 	struct strbuf filename;
 	struct strbuf staging_filename;
 };
+
 #define LOCK_DIE_ON_ERROR 1
 #define LOCK_NODEREF 2
+#define LOCK_SEPARATE_STAGING_FILE 4
+
 extern int unable_to_lock_error(const char *path, int err);
 extern NORETURN void unable_to_lock_index_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 activate_staging_file(struct lock_file *);
 extern int commit_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 c06e134..336b914 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -40,8 +40,16 @@
  *   used in any way, it is initialized, permanently registered in the
  *   lock_file_list, and flags & LOCK_FLAGS_ON_LIST is set.
  *
- * - Locked, lockfile open (after hold_lock_file_for_update() or
- *   hold_lock_file_for_append()).  In this state, the lockfile
+ * - Initialized but unlocked (after commit_lock_file(),
+ *   rollback_lock_file(), or a failed attempt to lock).  In this
+ *   state, filename and staging_filename are the empty string.  The
+ *   object is left registered in the lock_file_list, and flags ==
+ *   LOCK_FLAGS_ON_LIST.
+ *
+ * - Locked, lockfile open for writing (after
+ *   hold_lock_file_for_update() or hold_lock_file_for_append()
+ *   without the LOCK_SEPARATE_STAGING_FILE flag).  In this state,
+ *   flags & LOCK_FLAGS_SEPARATE_STAGING_FILE is not set, the lockfile
  *   exists, filename holds the filename of the locked file,
  *   staging_filename holds the filename of the lockfile, fd holds a
  *   file descriptor open for writing to the lockfile, and owner holds
@@ -50,11 +58,21 @@
  * - 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 and
- *   staging_filename are the empty string and fd is -1.  The object is left
- *   registered in the lock_file_list, and flags & LOCK_FLAGS_ON_LIST
- *   is set.
+ * - Locked, separate staging file open for writing (after
+ *   hold_lock_file_for_update() or hold_lock_file_for_append() with
+ *   the LOCK_SEPARATE_STAGING_FILE flag).  In this state, flags &
+ *   LOCK_FLAGS_SEPARATE_STAGING_FILE is set, the lockfile exists but
+ *   is closed, filename holds the filename of the locked file,
+ *   staging_filename holds the filename of the separate staging file,
+ *   fd holds a file descriptor open for writing to the separate
+ *   staging file, and owner holds the PID of the process that locked
+ *   the file.
+ *
+ * - Locked, staging file closed and activated (after
+ *   activate_staging_file()).  Same as the previous state, except
+ *   that the separate staging file has already been closed and
+ *   renamed on top of the file, staging_filename is the empty string,
+ *   and fd is -1.
  *
  * See Documentation/api-lockfile.txt for more information.
  */
@@ -64,6 +82,9 @@
 /* This lock_file instance is in the lock_file_list */
 #define LOCK_FLAGS_ON_LIST 0x01
 
+/* A separate staging file is being used */
+#define LOCK_FLAGS_SEPARATE_STAGING_FILE 0x02
+
 static struct lock_file *lock_file_list;
 static const char *alternate_index_output;
 
@@ -155,9 +176,31 @@ static void resolve_symlink(struct strbuf *path)
 	strbuf_release(&link);
 }
 
-/* We append ".lock" to the filename to derive the lockfile name: */
+/*
+ * We append ".lock" to the filename to derive the lockfile name, and
+ * ".new" to derive the staging file name.  The longer of the two:
+ */
 #define LOCK_SUFFIX_LEN 5
 
+static int open_staging_file(struct lock_file *lk)
+{
+	strbuf_setlen(&lk->staging_filename, lk->filename.len);
+	strbuf_addstr(&lk->staging_filename, ".new");
+	lk->fd = open(lk->staging_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
+	if (lk->fd < 0) {
+		return -1;
+	}
+	if (adjust_shared_perm(lk->staging_filename.buf)) {
+		error("cannot fix permission bits on %s", lk->staging_filename.buf);
+		if (close(lk->fd))
+			error("cannot close staging file %s", lk->staging_filename.buf);
+		lk->fd = -1;
+		unlink_or_warn(lk->staging_filename.buf);
+		return -1;
+	}
+	return 0;
+}
+
 static int lock_file(struct lock_file *lk, const char *path, int flags)
 {
 	size_t path_len = strlen(path);
@@ -199,11 +242,22 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	if (adjust_shared_perm(lk->staging_filename.buf)) {
 		error("cannot fix permission bits on %s",
 		      lk->staging_filename.buf);
-		rollback_lock_file(lk);
-		return -1;
+		goto rollback_and_fail;
+	}
+	if (flags & LOCK_SEPARATE_STAGING_FILE) {
+		if (close_lock_file(lk))
+			goto rollback_and_fail;
+
+		lk->flags |= LOCK_FLAGS_SEPARATE_STAGING_FILE;
+		if (open_staging_file(lk))
+			goto rollback_and_fail;
 	}
 
 	return lk->fd;
+
+rollback_and_fail:
+	rollback_lock_file(lk);
+	return -1;
 }
 
 static char *unable_to_lock_message(const char *path, int err)
@@ -271,19 +325,54 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
 	return fd;
 }
 
-int close_lock_file(struct lock_file *lk)
+static int close_staging_file(struct lock_file *lk)
 {
 	int fd = lk->fd;
+
 	lk->fd = -1;
 	return close(fd);
 }
 
-int commit_lock_file(struct lock_file *lk)
+int close_lock_file(struct lock_file *lk)
 {
-	if (lk->fd >= 0 && close_lock_file(lk))
-		return -1;
-	if (rename(lk->staging_filename.buf, lk->filename.buf))
+	assert(!(lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE));
+	return close_staging_file(lk);
+}
+
+int activate_staging_file(struct lock_file *lk)
+{
+	int err;
+
+	assert(lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE);
+	assert(lk->fd >= 0);
+	assert(lk->staging_filename.len);
+
+	if (close_staging_file(lk))
 		return -1;
+
+	err = rename(lk->staging_filename.buf, lk->filename.buf);
+	strbuf_setlen(&lk->staging_filename, 0);
+
+	return err;
+}
+
+int commit_lock_file(struct lock_file *lk)
+{
+	if (lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE) {
+		if (lk->staging_filename.len) {
+			assert(lk->fd >= 0);
+			if (activate_staging_file(lk))
+				return -1;
+		}
+		strbuf_addbuf(&lk->staging_filename, &lk->filename);
+		strbuf_addstr(&lk->staging_filename, ".lock");
+		unlink_or_warn(lk->staging_filename.buf);
+	} else {
+		if (lk->fd >= 0 && close_lock_file(lk))
+			return -1;
+		if (rename(lk->staging_filename.buf, lk->filename.buf))
+			return -1;
+	}
 	reset_lock_file(lk);
 	return 0;
 }
@@ -318,10 +407,21 @@ int commit_locked_index(struct lock_file *lk)
 
 void rollback_lock_file(struct lock_file *lk)
 {
-	if (lk->filename.len) {
-		if (lk->fd >= 0)
-			close_lock_file(lk);
-		unlink_or_warn(lk->staging_filename.buf);
-		reset_lock_file(lk);
+	if (!lk->filename.len)
+		return;
+
+	if (lk->fd >= 0)
+		close_staging_file(lk);
+
+	if (lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE) {
+		if (lk->staging_filename.len) {
+			unlink_or_warn(lk->staging_filename.buf);
+			strbuf_setlen(&lk->staging_filename, 0);
+		}
+		strbuf_addbuf(&lk->staging_filename, &lk->filename);
+		strbuf_addstr(&lk->staging_filename, ".lock");
 	}
+
+	unlink_or_warn(lk->staging_filename.buf);
+	reset_lock_file(lk);
 }
-- 
1.9.0

Re: [PATCH 22/22] lockfile: allow new file contents to be written while retaining lock

[Jeff King]

On Tue, Apr 01, 2014 at 05:58:30PM +0200, Michael Haggerty wrote:

> Add a new option flag, LOCK_SEPARATE_STAGING_FILE, that can be passed
> to hold_lock_file_for_update() or hold_lock_file_for_append() to use a
> staging file that is independent of the lock file.
> 
> Add a new function activate_staging_file() that activates the contents
> that have been written to the staging file without releasing the lock.
> 
> This functionality can be used to ensure that changes to two files are
> seen by other processes in one order even if correctness requires the
> locks to be released in another order.

Can you give an example of when this is useful? I'm guessing the
application is for writing out packed-refs before pruning loose refs in
git-pack-refs?

It seems like this makes the API much more confusing.  If I understand
correctly, this is basically allowing us to take a lock, write to
_another_ tmpfile that is not the lock, then rename the tmpfile into
place without releasing the lock (and then we can drop the lock at our
convenience).

I wonder if it would be simpler to build an API for that around the
lock_file API, rather than as part of it. Or am I misunderstanding
what's going on?

-Peff

Re: [PATCH 22/22] lockfile: allow new file contents to be written while retaining lock

[Eric Sunshine]

On Tue, Apr 1, 2014 at 11:58 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Add a new option flag, LOCK_SEPARATE_STAGING_FILE, that can be passed
> to hold_lock_file_for_update() or hold_lock_file_for_append() to use a
> staging file that is independent of the lock file.
>
> Add a new function activate_staging_file() that activates the contents
> that have been written to the staging file without releasing the lock.
>
> This functionality can be used to ensure that changes to two files are
> seen by other processes in one order even if correctness requires the
> locks to be released in another order.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> diff --git a/lockfile.c b/lockfile.c
> index c06e134..336b914 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -271,19 +325,54 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
>         return fd;
>  }
>
> -int close_lock_file(struct lock_file *lk)
> +static int close_staging_file(struct lock_file *lk)
>  {
>         int fd = lk->fd;
> +
>         lk->fd = -1;
>         return close(fd);
>  }
>
> -int commit_lock_file(struct lock_file *lk)
> +int close_lock_file(struct lock_file *lk)
>  {
> -       if (lk->fd >= 0 && close_lock_file(lk))
> -               return -1;
> -       if (rename(lk->staging_filename.buf, lk->filename.buf))
> +       assert(!(lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE));
> +       return close_staging_file(lk);
> +}
> +
> +int activate_staging_file(struct lock_file *lk)
> +{
> +       int err;
> +
> +       assert(lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE);
> +       assert(lk->fd >= 0);
> +       assert(lk->staging_filename.len);
> +
> +       if (close_staging_file(lk))
>                 return -1;
> +
> +       err = rename(lk->staging_filename.buf, lk->filename.buf);
> +       strbuf_setlen(&lk->staging_filename, 0);

strbuf_reset()?

> +
> +       return err;
> +}
> +
> +int commit_lock_file(struct lock_file *lk)
> +{
> +       if (lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE) {
> +               if (lk->staging_filename.len) {
> +                       assert(lk->fd >= 0);
> +                       if (activate_staging_file(lk))
> +                               return -1;
> +               }
> +               strbuf_addbuf(&lk->staging_filename, &lk->filename);
> +               strbuf_addstr(&lk->staging_filename, ".lock");
> +               unlink_or_warn(lk->staging_filename.buf);
> +       } else {
> +               if (lk->fd >= 0 && close_lock_file(lk))
> +                       return -1;
> +               if (rename(lk->staging_filename.buf, lk->filename.buf))
> +                       return -1;
> +       }
>         reset_lock_file(lk);
>         return 0;
>  }
> @@ -318,10 +407,21 @@ int commit_locked_index(struct lock_file *lk)
>
>  void rollback_lock_file(struct lock_file *lk)
>  {
> -       if (lk->filename.len) {
> -               if (lk->fd >= 0)
> -                       close_lock_file(lk);
> -               unlink_or_warn(lk->staging_filename.buf);
> -               reset_lock_file(lk);
> +       if (!lk->filename.len)
> +               return;
> +
> +       if (lk->fd >= 0)
> +               close_staging_file(lk);
> +
> +       if (lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE) {
> +               if (lk->staging_filename.len) {
> +                       unlink_or_warn(lk->staging_filename.buf);
> +                       strbuf_setlen(&lk->staging_filename, 0);

strbuf_reset()?

> +               }
> +               strbuf_addbuf(&lk->staging_filename, &lk->filename);
> +               strbuf_addstr(&lk->staging_filename, ".lock");
>         }
> +
> +       unlink_or_warn(lk->staging_filename.buf);
> +       reset_lock_file(lk);
>  }
> --
> 1.9.0

Re: [PATCH 22/22] lockfile: allow new file contents to be written while retaining lock

[Junio C Hamano]

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

> +static int open_staging_file(struct lock_file *lk)
> +{
> +	strbuf_setlen(&lk->staging_filename, lk->filename.len);
> +	strbuf_addstr(&lk->staging_filename, ".new");
> +	lk->fd = open(lk->staging_filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
> +	if (lk->fd < 0) {
> +		return -1;
> +	}

All the other "if (lk->fd < 0)" calls reset_lock_file(lk).  Is it an
intentional omission that this one does not?

If so, please drop the extraneous {} around the single "return -1"
statement.

I also share the same puzzlement in Peff's review.

Re: [PATCH 00/22] Lockfile refactoring and pre-activation

[Jeff King]

On Tue, Apr 01, 2014 at 05:58:08PM +0200, Michael Haggerty wrote:

> I've had this patch series kicking around for a long time, along with
> some followup patches to fix a race in reference deletion.  I haven't
> had the time to get everything done and tested, but let me at least
> push this first series out there.  I especially want to submit it in
> case we accept a GSoC student for the project "Refactor tempfile
> handling", because (1) I don't want me and the student to be stepping
> on each others' toes, and (2) there are some cleanups and
> documentation improvements here that will hopefully be useful to the
> student.

Thanks, this sort of preparation for GSoC students is very much welcome.

> The first patch actually demonstrates the race condition that I hope
> to fix someday.  The last patch introduces the lockfile feature that I
> think is needed to fix it: the ability to activate a packed-refs file
> while still holding the lock to prevent another process from
> overwriting it before the accompanying loose reference updates are all
> finished.  But the fix itself is not included here, so you might want
> to keep the last patch on hold until there is a concrete user of the
> feature.

I should have read this more carefully when I responded to the final
patch. I surmised your intent based on our previous work on packed-refs,
but here you spell out my guesses explicitly. :)

I think all of the patches look good. I'd prefer to hold back the final
one (and probably 19/22, which has no purpose except to prepare for
22/22) until seeing its application in practice.

Thanks for a very pleasant read.

-Peff

Re: [PATCH 00/22] Lockfile refactoring and pre-activation

[Michael Haggerty]

On 04/01/2014 10:44 PM, Jeff King wrote:
> [...]
> I think all of the patches look good. I'd prefer to hold back the final
> one (and probably 19/22, which has no purpose except to prepare for
> 22/22) until seeing its application in practice.

OK, I shuffled 19/22 to the end of the patch series, just before 22/22.
 That way both can be taken or left (probably left for now).

> Thanks for a very pleasant read.

Thanks for being a very pleasant (and thorough) reader :-)

I'm still working on the backlog of comments and then I'll get back to
the list with v2.

Michael

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