[PATCHv2 0/5] Fix bug in large transactions

[PATCHv2 0/5] Fix bug in large transactions

[Stefan Beller]

version2:

* This applies on top of origin/sb/atomic-push though it will result in a one
  line merge conflict with origin/jk/lock-ref-sha1-basic-return-errors when
  merging to origin/next.

* It now uses the FILE* pointer instead of file descriptors. This
  results in a combination of the 2 former patches "refs.c: have
  a write_in_full_to_lock_file wrapper" and "refs.c: write to a
  lock file only once" as the wrapper function is more adapted to
  its consumers

* no need to dance around with char *pointers which may leak.

* another new patch sneaked into the series: Renaming ULIMIT in t7004
  to ULIMIT_STACK_SIZE

That said, only the first and third patch are updated from the first version
of the patches. The others are new in the sense that rewriting them was cheaper
than keeping notes in between.

version1:

(reported as: git update-ref --stdin : too many open files, 2014-12-20)

First a test case is introduced to demonstrate the failure,
the patches 2-6 are little refactoring and the last patch
fixes the bug and also marks the bugs as resolved in the
test suite.

Unfortunately this applies on top of origin/next.

Any feedback would be welcome!

Thanks,
Stefan

Stefan Beller (5):
  update-ref: test handling large transactions properly
  t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE
  refs.c: remove lock_fd from struct ref_lock
  refs.c: have a write_sha1_to_lock_file wrapper
  refs.c: enable large transactions

 refs.c                | 34 ++++++++++++++++++++++------------
 t/t1400-update-ref.sh | 28 ++++++++++++++++++++++++++++
 t/t7004-tag.sh        |  4 ++--
 3 files changed, 52 insertions(+), 14 deletions(-)

-- 
2.2.1.62.g3f15098

[PATCHv2 1/5] update-ref: test handling large transactions properly

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t1400-update-ref.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 7b4707b..47d2fe9 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -973,4 +973,32 @@ test_expect_success 'stdin -z delete refs works with packed and loose refs' '
 	test_must_fail git rev-parse --verify -q $c
 '
 
+run_with_limited_open_files () {
+	(ulimit -n 32 && "$@")
+}
+
+test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'
+
+test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
+(
+	for i in $(test_seq 33)
+	do
+		echo "create refs/heads/$i HEAD"
+	done >large_input &&
+	run_with_limited_open_files git update-ref --stdin <large_input &&
+	git rev-parse --verify -q refs/heads/33
+)
+'
+
+test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
+(
+	for i in $(test_seq 33)
+	do
+		echo "delete refs/heads/$i HEAD"
+	done >large_input &&
+	run_with_limited_open_files git update-ref --stdin <large_input &&
+	test_must_fail git rev-parse --verify -q refs/heads/33
+)
+'
+
 test_done
-- 
2.2.1.62.g3f15098

[PATCHv2 2/5] t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE

[Stefan Beller]

During creation of the patch series our discussion we could have a
more descriptive name for the prerequisite for the test so it stays
unique when other limits of ulimit are introduced.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7004-tag.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 796e9f7..06b8e0d 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1463,10 +1463,10 @@ run_with_limited_stack () {
 	(ulimit -s 128 && "$@")
 }
 
-test_lazy_prereq ULIMIT 'run_with_limited_stack true'
+test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
 
 # we require ulimit, this excludes Windows
-test_expect_success ULIMIT '--contains works in a deep repo' '
+test_expect_success ULIMIT_STACK_SIZE '--contains works in a deep repo' '
 	>expect &&
 	i=1 &&
 	while test $i -lt 8000
-- 
2.2.1.62.g3f15098

[PATCHv2 3/5] refs.c: remove lock_fd from struct ref_lock

[Stefan Beller]

The 'lock_fd' is the same as 'lk->fd'. No need to store it twice so remove
it. You may argue this introduces more coupling as we need to know more
about the internals of the lock file mechanism, but this will be solved in
a later patch.

No functional changes intended.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index 14e52ca..2472e61 100644
--- a/refs.c
+++ b/refs.c
@@ -11,7 +11,6 @@ struct ref_lock {
 	char *orig_ref_name;
 	struct lock_file *lk;
 	unsigned char old_sha1[20];
-	int lock_fd;
 	int force_write;
 };
 
@@ -2259,7 +2258,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	int attempts_remaining = 3;
 
 	lock = xcalloc(1, sizeof(struct ref_lock));
-	lock->lock_fd = -1;
 
 	if (mustexist)
 		resolve_flags |= RESOLVE_REF_READING;
@@ -2335,8 +2333,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		goto error_return;
 	}
 
-	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
-	if (lock->lock_fd < 0) {
+	if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {
 		if (errno == ENOENT && --attempts_remaining > 0)
 			/*
 			 * Maybe somebody just deleted one of the
@@ -2904,7 +2901,6 @@ static int close_ref(struct ref_lock *lock)
 {
 	if (close_lock_file(lock->lk))
 		return -1;
-	lock->lock_fd = -1;
 	return 0;
 }
 
@@ -2912,7 +2908,6 @@ static int commit_ref(struct ref_lock *lock)
 {
 	if (commit_lock_file(lock->lk))
 		return -1;
-	lock->lock_fd = -1;
 	return 0;
 }
 
@@ -3090,8 +3085,8 @@ static int write_ref_sha1(struct ref_lock *lock,
 		errno = EINVAL;
 		return -1;
 	}
-	if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||
-	    write_in_full(lock->lock_fd, &term, 1) != 1 ||
+	if (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 ||
+	    write_in_full(lock->lk->fd, &term, 1) != 1 ||
 	    close_ref(lock) < 0) {
 		int save_errno = errno;
 		error("Couldn't write %s", lock->lk->filename.buf);
@@ -4047,9 +4042,9 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 			status |= error("couldn't write %s: %s", log_file,
 					strerror(errno));
 		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
-			(write_in_full(lock->lock_fd,
+			(write_in_full(lock->lk->fd,
 				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
-			 write_str_in_full(lock->lock_fd, "\n") != 1 ||
+			 write_str_in_full(lock->lk->fd, "\n") != 1 ||
 			 close_ref(lock) < 0)) {
 			status |= error("couldn't write %s",
 					lock->lk->filename.buf);
-- 
2.2.1.62.g3f15098

[PATCHv2 4/5] refs.c: have a write_sha1_to_lock_file wrapper

[Stefan Beller]

Now we only have one place where we need to touch the internals of
the lock_file struct.

No functional changes intended.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 2472e61..2013d37 100644
--- a/refs.c
+++ b/refs.c
@@ -3052,6 +3052,16 @@ int is_branch(const char *refname)
 	return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/");
 }
 
+static int write_sha1_to_lock_file(struct ref_lock *lock,
+				   const unsigned char *sha1)
+{
+	if (fdopen_lock_file(lock->lk, "w") < 0
+	    || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41)
+		return -1;
+	else
+		return 0;
+}
+
 /*
  * Write sha1 into the ref specified by the lock. Make sure that errno
  * is sane on error.
@@ -3059,7 +3069,6 @@ int is_branch(const char *refname)
 static int write_ref_sha1(struct ref_lock *lock,
 	const unsigned char *sha1, const char *logmsg)
 {
-	static char term = '\n';
 	struct object *o;
 
 	if (!lock) {
@@ -3085,8 +3094,7 @@ static int write_ref_sha1(struct ref_lock *lock,
 		errno = EINVAL;
 		return -1;
 	}
-	if (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 ||
-	    write_in_full(lock->lk->fd, &term, 1) != 1 ||
+	if (write_sha1_to_lock_file(lock, sha1) ||
 	    close_ref(lock) < 0) {
 		int save_errno = errno;
 		error("Couldn't write %s", lock->lk->filename.buf);
@@ -4042,9 +4050,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 			status |= error("couldn't write %s: %s", log_file,
 					strerror(errno));
 		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
-			(write_in_full(lock->lk->fd,
-				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
-			 write_str_in_full(lock->lk->fd, "\n") != 1 ||
+			(write_sha1_to_lock_file(lock, cb.last_kept_sha1) ||
 			 close_ref(lock) < 0)) {
 			status |= error("couldn't write %s",
 					lock->lk->filename.buf);
-- 
2.2.1.62.g3f15098

[PATCHv2 5/5] refs.c: enable large transactions

[Stefan Beller]

By closing the file descriptors after creating the lock file we are not
limiting the size of the transaction by the number of available file
descriptors.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c                | 17 +++++++++++++----
 t/t1400-update-ref.sh |  4 ++--
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 2013d37..9d01102 100644
--- a/refs.c
+++ b/refs.c
@@ -3055,11 +3055,18 @@ int is_branch(const char *refname)
 static int write_sha1_to_lock_file(struct ref_lock *lock,
 				   const unsigned char *sha1)
 {
-	if (fdopen_lock_file(lock->lk, "w") < 0
-	    || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41)
+	if (lock->lk->fd == -1) {
+		if (reopen_lock_file(lock->lk) < 0
+		    || fdopen_lock_file(lock->lk, "w") < 0
+		    || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41
+		    || close_lock_file(lock->lk) < 0)
+		    return -1;
+	} else {
+		if (fdopen_lock_file(lock->lk, "w") < 0
+		    || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41)
 		return -1;
-	else
-		return 0;
+	}
+	return 0;
 }
 
 /*
@@ -3761,6 +3768,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				    update->refname);
 			goto cleanup;
 		}
+		/* Do not keep all lock files open at the same time. */
+		close_lock_file(update->lock->lk);
 	}
 
 	/* Perform updates first so live commits remain referenced */
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 47d2fe9..c593a1d 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -979,7 +979,7 @@ run_with_limited_open_files () {
 
 test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'
 
-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
 (
 	for i in $(test_seq 33)
 	do
@@ -990,7 +990,7 @@ test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches
 )
 '
 
-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
 (
 	for i in $(test_seq 33)
 	do
-- 
2.2.1.62.g3f15098

Re: [PATCHv2 1/5] update-ref: test handling large transactions properly

[Michael Haggerty]

On 01/22/2015 03:32 AM, Stefan Beller wrote:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/t1400-update-ref.sh | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 7b4707b..47d2fe9 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -973,4 +973,32 @@ test_expect_success 'stdin -z delete refs works with packed and loose refs' '
>  	test_must_fail git rev-parse --verify -q $c
>  '
>  
> +run_with_limited_open_files () {
> +	(ulimit -n 32 && "$@")
> +}

Regarding the choice of "32", I wonder what is the worst-case number of
open file descriptors that are needed *before* counting the ones that
are currently wasted on open loose-reference locks. On Linux it seems to
be only 4 with my setup:

    $ (ulimit -n 3 && git update-ref --stdin </dev/null)
    bash: /dev/null: Too many open files
    $ (ulimit -n 4 && git update-ref --stdin </dev/null)
    $

This number might depend a little bit on details of the repository, like
whether config files import other config files. But as long as the
"background" number of fds required is at least a few less than 32, then
your number should be safe.

Does anybody know of a platform where file descriptors are eaten up
gluttonously by, for example, each shared library that is in use or
something? That's the only think I can think of that could potentially
make your choice of 32 problematic.

> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCHv2 5/5] refs.c: enable large transactions

[Michael Haggerty]

On 01/22/2015 03:32 AM, Stefan Beller wrote:
> By closing the file descriptors after creating the lock file we are not
> limiting the size of the transaction by the number of available file
> descriptors.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  refs.c                | 17 +++++++++++++----
>  t/t1400-update-ref.sh |  4 ++--
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 2013d37..9d01102 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3055,11 +3055,18 @@ int is_branch(const char *refname)
>  static int write_sha1_to_lock_file(struct ref_lock *lock,
>  				   const unsigned char *sha1)
>  {
> -	if (fdopen_lock_file(lock->lk, "w") < 0
> -	    || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41)
> +	if (lock->lk->fd == -1) {
> +		if (reopen_lock_file(lock->lk) < 0
> +		    || fdopen_lock_file(lock->lk, "w") < 0
> +		    || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41
> +		    || close_lock_file(lock->lk) < 0)
> +		    return -1;
> +	} else {
> +		if (fdopen_lock_file(lock->lk, "w") < 0
> +		    || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41)
>  		return -1;
> -	else
> -		return 0;
> +	}
> +	return 0;
>  }

I can't figure out where to apply this series or where to fetch it from,
so I can't see these changes in context, so maybe I'm misunderstanding
something. It looks like this code is doing

    open(), close(), open(), fdopen(), write(), fclose(), rename()

on each lockfile. But don't we have enough information to write the
SHA-1 into the lockfile the first time we touch it? I.e., couldn't we
reduce this to

    open(), fdopen(), write(), fclose(), rename()

, where the first four calls all happen in the initial loop? If a
problem is discovered when writing a later reference, we would roll back
the transaction anyway.

I understand that this would require a bigger rewrite, so maybe it is
not worth it.

>  /*
> @@ -3761,6 +3768,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  				    update->refname);
>  			goto cleanup;
>  		}
> +		/* Do not keep all lock files open at the same time. */
> +		close_lock_file(update->lock->lk);
>  	}
>  
>  	/* Perform updates first so live commits remain referenced */
> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCHv2 0/5] Fix bug in large transactions

[Michael Haggerty]

On 01/22/2015 03:32 AM, Stefan Beller wrote:
> version2:

Summary: patches 1-4 look good to me. I sent a separate comment about
patch 5, which seems to do more system calls than necessary.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCHv2 5/5] refs.c: enable large transactions

[Ramsay Jones]

On 22/01/15 02:32, Stefan Beller wrote:
> By closing the file descriptors after creating the lock file we are not
> limiting the size of the transaction by the number of available file
> descriptors.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  refs.c                | 17 +++++++++++++----
>  t/t1400-update-ref.sh |  4 ++--
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 2013d37..9d01102 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3055,11 +3055,18 @@ int is_branch(const char *refname)
>  static int write_sha1_to_lock_file(struct ref_lock *lock,
>  				   const unsigned char *sha1)
>  {
> -	if (fdopen_lock_file(lock->lk, "w") < 0
> -	    || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41)
> +	if (lock->lk->fd == -1) {
> +		if (reopen_lock_file(lock->lk) < 0
> +		    || fdopen_lock_file(lock->lk, "w") < 0

fdopen_lock_file() returns a 'FILE *', so this causes sparse to bark:

    refs.c:3105:56: error: incompatible types for operation (<)
    refs.c:3105:56:    left side has type struct _IO_FILE [usertype] *
    refs.c:3105:56:    right side has type int

> +		    || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41
> +		    || close_lock_file(lock->lk) < 0)
> +		    return -1;
> +	} else {
> +		if (fdopen_lock_file(lock->lk, "w") < 0

Similarly, sparse barks:

    refs.c:3110:53: error: incompatible types for operation (<)
    refs.c:3110:53:    left side has type struct _IO_FILE [usertype] *
    refs.c:3110:53:    right side has type int

> +		    || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41)
>  		return -1;
> -	else
> -		return 0;
> +	}
> +	return 0;
>  }
>  
>  /*
> @@ -3761,6 +3768,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  				    update->refname);
>  			goto cleanup;
>  		}
> +		/* Do not keep all lock files open at the same time. */
> +		close_lock_file(update->lock->lk);
>  	}
>  
>  	/* Perform updates first so live commits remain referenced */
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 47d2fe9..c593a1d 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -979,7 +979,7 @@ run_with_limited_open_files () {
>  
>  test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'
>  
> -test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
> +test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
>  (
>  	for i in $(test_seq 33)
>  	do
> @@ -990,7 +990,7 @@ test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches
>  )
>  '
>  
> -test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
> +test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
>  (
>  	for i in $(test_seq 33)
>  	do
> 

ATB,
Ramsay Jones

Re: [PATCHv2 1/5] update-ref: test handling large transactions properly

[Jeff King]

On Thu, Jan 22, 2015 at 11:54:49AM +0100, Michael Haggerty wrote:

> > +run_with_limited_open_files () {
> > +	(ulimit -n 32 && "$@")
> > +}
> 
> Regarding the choice of "32", I wonder what is the worst-case number of
> open file descriptors that are needed *before* counting the ones that
> are currently wasted on open loose-reference locks. On Linux it seems to
> be only 4 with my setup:
> 
>     $ (ulimit -n 3 && git update-ref --stdin </dev/null)
>     bash: /dev/null: Too many open files
>     $ (ulimit -n 4 && git update-ref --stdin </dev/null)
>     $
> 
> This number might depend a little bit on details of the repository, like
> whether config files import other config files. But as long as the
> "background" number of fds required is at least a few less than 32, then
> your number should be safe.
> 
> Does anybody know of a platform where file descriptors are eaten up
> gluttonously by, for example, each shared library that is in use or
> something? That's the only think I can think of that could potentially
> make your choice of 32 problematic.

It's not just choice of platform. There could be inherited descriptors
in the environment (e.g., the test suite is being run by a shell that
keeps a pipe to CI server open, or something). And the test suite itself
uses several extra descriptors for hiding and showing output.

I think this is the sort of thing that we have to determine with a mix
of guessing and empiricism.  4 is almost certainly too low. 32 looks
"pretty big" in practice but not so big that it will make the test slow.
I think our best bet is probably to ship it and see if anybody reports
problems while the patch cooks.  Then we can bump the number (or find a
new approach) as necessary.

-Peff

Re: [PATCHv2 5/5] refs.c: enable large transactions

[Jeff King]

On Thu, Jan 22, 2015 at 12:24:23PM +0100, Michael Haggerty wrote:

> I can't figure out where to apply this series or where to fetch it from,
> so I can't see these changes in context, so maybe I'm misunderstanding
> something. It looks like this code is doing
> 
>     open(), close(), open(), fdopen(), write(), fclose(), rename()
> 
> on each lockfile. But don't we have enough information to write the
> SHA-1 into the lockfile the first time we touch it? I.e., couldn't we
> reduce this to
> 
>     open(), fdopen(), write(), fclose(), rename()
> 
> , where the first four calls all happen in the initial loop? If a
> problem is discovered when writing a later reference, we would roll back
> the transaction anyway.
> 
> I understand that this would require a bigger rewrite, so maybe it is
> not worth it.

I had a nagging feeling on the multiple-open thing, too, and would much
prefer to just write out the contents early (since we know what they
are). It looks like we would just need to split write_ref_sha1() into
its two halves:

  1. Write out the lockfile

  2. Commit the change

And then call them at the appropriate spots from ref_transaction_commit().

I guess that is maybe a step backwards for abstracted ref backends,
though.

-Peff

Re: [PATCHv2 5/5] refs.c: enable large transactions

[Michael Haggerty]

On 01/22/2015 02:10 PM, Jeff King wrote:
> On Thu, Jan 22, 2015 at 12:24:23PM +0100, Michael Haggerty wrote:
> 
>> I can't figure out where to apply this series or where to fetch it from,
>> so I can't see these changes in context, so maybe I'm misunderstanding
>> something. It looks like this code is doing
>>
>>     open(), close(), open(), fdopen(), write(), fclose(), rename()
>>
>> on each lockfile. But don't we have enough information to write the
>> SHA-1 into the lockfile the first time we touch it? I.e., couldn't we
>> reduce this to
>>
>>     open(), fdopen(), write(), fclose(), rename()
>>
>> , where the first four calls all happen in the initial loop? If a
>> problem is discovered when writing a later reference, we would roll back
>> the transaction anyway.
>>
>> I understand that this would require a bigger rewrite, so maybe it is
>> not worth it.
> 
> I had a nagging feeling on the multiple-open thing, too, and would much
> prefer to just write out the contents early (since we know what they
> are). It looks like we would just need to split write_ref_sha1() into
> its two halves:
> 
>   1. Write out the lockfile
> 
>   2. Commit the change
> 
> And then call them at the appropriate spots from ref_transaction_commit().
> 
> I guess that is maybe a step backwards for abstracted ref backends,
> though.

Nah, the implementation of ref_transaction_commit() will have to differ
between backends anyway. I don't think this would be a step backwards.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCHv2 5/5] refs.c: enable large transactions

[Stefan Beller]

How do you run sparse on git?

I noticed there is 'make sparse' though I cannot get it working
here in the corporate world as I have problems with openssl
headers not being found.

Also the line numbers seem to bit off compared to what I have
here, did you need to modify/preprocess files to get sparse running?

As for the fix, would it be sufficient to check != NULL instead of < 0?

Thanks,
Stefan


On Thu, Jan 22, 2015 at 4:59 AM, Ramsay Jones
<ramsay@ramsay1.demon.co.uk> wrote:
> On 22/01/15 02:32, Stefan Beller wrote:
>> By closing the file descriptors after creating the lock file we are not
>> limiting the size of the transaction by the number of available file
>> descriptors.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  refs.c                | 17 +++++++++++++----
>>  t/t1400-update-ref.sh |  4 ++--
>>  2 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 2013d37..9d01102 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3055,11 +3055,18 @@ int is_branch(const char *refname)
>>  static int write_sha1_to_lock_file(struct ref_lock *lock,
>>                                  const unsigned char *sha1)
>>  {
>> -     if (fdopen_lock_file(lock->lk, "w") < 0
>> -         || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41)
>> +     if (lock->lk->fd == -1) {
>> +             if (reopen_lock_file(lock->lk) < 0
>> +                 || fdopen_lock_file(lock->lk, "w") < 0
>
> fdopen_lock_file() returns a 'FILE *', so this causes sparse to bark:
>
>     refs.c:3105:56: error: incompatible types for operation (<)
>     refs.c:3105:56:    left side has type struct _IO_FILE [usertype] *
>     refs.c:3105:56:    right side has type int
>
>> +                 || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41
>> +                 || close_lock_file(lock->lk) < 0)
>> +                 return -1;
>> +     } else {
>> +             if (fdopen_lock_file(lock->lk, "w") < 0
>
> Similarly, sparse barks:
>
>     refs.c:3110:53: error: incompatible types for operation (<)
>     refs.c:3110:53:    left side has type struct _IO_FILE [usertype] *
>     refs.c:3110:53:    right side has type int
>
>> +                 || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41)
>>               return -1;
>> -     else
>> -             return 0;
>> +     }
>> +     return 0;
>>  }
>>
>>  /*
>> @@ -3761,6 +3768,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>>                                   update->refname);
>>                       goto cleanup;
>>               }
>> +             /* Do not keep all lock files open at the same time. */
>> +             close_lock_file(update->lock->lk);
>>       }
>>
>>       /* Perform updates first so live commits remain referenced */
>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
>> index 47d2fe9..c593a1d 100755
>> --- a/t/t1400-update-ref.sh
>> +++ b/t/t1400-update-ref.sh
>> @@ -979,7 +979,7 @@ run_with_limited_open_files () {
>>
>>  test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'
>>
>> -test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
>> +test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
>>  (
>>       for i in $(test_seq 33)
>>       do
>> @@ -990,7 +990,7 @@ test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches
>>  )
>>  '
>>
>> -test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
>> +test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
>>  (
>>       for i in $(test_seq 33)
>>       do
>>
>
> ATB,
> Ramsay Jones
>
>
>

Re: [PATCHv2 5/5] refs.c: enable large transactions

[Stefan Beller]

On Thu, Jan 22, 2015 at 8:33 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 01/22/2015 02:10 PM, Jeff King wrote:
>> On Thu, Jan 22, 2015 at 12:24:23PM +0100, Michael Haggerty wrote:
>>
>>> I can't figure out where to apply this series or where to fetch it from,
>>> so I can't see these changes in context, so maybe I'm misunderstanding
>>> something. It looks like this code is doing
>>>
>>>     open(), close(), open(), fdopen(), write(), fclose(), rename()
>>>
>>> on each lockfile. But don't we have enough information to write the
>>> SHA-1 into the lockfile the first time we touch it? I.e., couldn't we
>>> reduce this to
>>>
>>>     open(), fdopen(), write(), fclose(), rename()
>>>
>>> , where the first four calls all happen in the initial loop? If a
>>> problem is discovered when writing a later reference, we would roll back
>>> the transaction anyway.
>>>
>>> I understand that this would require a bigger rewrite, so maybe it is
>>> not worth it.
>>
>> I had a nagging feeling on the multiple-open thing, too, and would much
>> prefer to just write out the contents early (since we know what they
>> are). It looks like we would just need to split write_ref_sha1() into
>> its two halves:
>>
>>   1. Write out the lockfile
>>
>>   2. Commit the change
>>
>> And then call them at the appropriate spots from ref_transaction_commit().
>>
>> I guess that is maybe a step backwards for abstracted ref backends,
>> though.
>
> Nah, the implementation of ref_transaction_commit() will have to differ
> between backends anyway. I don't think this would be a step backwards.
>
> Michael
>

I also dislike the double open/close thing, but I just wanted to come up with
a quick and unobtrusive fix which doesn't rewrite the whole refs backend as
we have some code churn in the refs lately.

Michael, I forgot your short term intentions on the refs backend, so I tried to
be shy with that bug fix. What huge changes are you planning in the next few
weeks w.r.t. the refs handling? I would look more into that if there are no code
conflicts likely to arise.

Thanks,
Stefan

Re: [PATCHv2 5/5] refs.c: enable large transactions

[Ramsay Jones]

On 22/01/15 19:16, Stefan Beller wrote:
> How do you run sparse on git?

  $ make sparse >sp-out 2>&1

> 
> I noticed there is 'make sparse' though I cannot get it working
> here in the corporate world as I have problems with openssl
> headers not being found.

If you can build git with gcc, you should be able to run 'make sparse'
(modulo bugs, of course!). Having said that, I build sparse from source
and (on Linux) I'm running:

  $ sparse --version
  v0.5.0-30-gca3309e
  $

The most up-to-date (from git://git.kernel.org/pub/scm/devel/sparse/sparse.git)
is actually:

  $ sparse --version
  v0.5.0-41-g6c2d743
  $ 

which should work just fine. (I also run sparse on cygwin and MinGW).

> 
> Also the line numbers seem to bit off compared to what I have
> here, did you need to modify/preprocess files to get sparse running?

No, I am simply building the 'pu' branch (currently @ 028c360).

> 
> As for the fix, would it be sufficient to check != NULL instead of < 0?

Hmm, I didn't give it any thought, but don't you want that to be '== NULL'?
(you don't want to use a NULL lock->lk->fp in the following fprintf()).
Or simply '!fdopen_lock_file(lock->lk, "w")' I suppose.

ATB,
Ramsay Jones

Re: [PATCHv2 5/5] refs.c: enable large transactions

[Ramsay Jones]

On 22/01/15 19:51, Ramsay Jones wrote:
> On 22/01/15 19:16, Stefan Beller wrote:
>> How do you run sparse on git?
> 
>   $ make sparse >sp-out 2>&1
> 

BTW, you can get gcc to warn about this also:

 $ rm refs.o
 $ make CFLAGS='-Wall -Wextra' refs.o
    * new build flags
    CC refs.o
In file included from cache.h:4:0,
                 from refs.c:1:
git-compat-util.h: In function ‘git_has_dos_drive_prefix’:
git-compat-util.h:277:56: warning: unused parameter ‘path’ [-Wunused-parameter]
 static inline int git_has_dos_drive_prefix(const char *path)
                                                        ^
In file included from cache.h:4:0,
                 from refs.c:1:
git-compat-util.h: In function ‘xsize_t’:
git-compat-util.h:689:10: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  if (len > (size_t) len)
          ^
refs.c: In function ‘check_refname_component’:
refs.c:54:61: warning: unused parameter ‘flags’ [-Wunused-parameter]
 static int check_refname_component(const char *refname, int flags)
                                                             ^
refs.c: In function ‘warn_if_dangling_symref’:
refs.c:1814:78: warning: unused parameter ‘sha1’ [-Wunused-parameter]
 static int warn_if_dangling_symref(const char *refname, const unsigned char *sha1,
                                                                              ^
refs.c: In function ‘log_ref_write_fd’:
refs.c:3042:14: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  if (written != len)
              ^
refs.c: In function ‘write_sha1_to_lock_file’:
refs.c:3105:42: warning: ordered comparison of pointer with integer zero [-Wextra]
       || fdopen_lock_file(lock->lk, "w") < 0
                                          ^
refs.c:3110:39: warning: ordered comparison of pointer with integer zero [-Wextra]
   if (fdopen_lock_file(lock->lk, "w") < 0
                                       ^
refs.c: In function ‘create_symref’:
refs.c:3220:18: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  if (sizeof(ref) <= len) {
                  ^
refs.c: In function ‘read_ref_at_ent’:
refs.c:3277:15: warning: unused parameter ‘email’ [-Wunused-parameter]
   const char *email, unsigned long timestamp, int tz,
               ^
refs.c: In function ‘read_ref_at_ent_oldest’:
refs.c:3324:19: warning: unused parameter ‘email’ [-Wunused-parameter]
       const char *email, unsigned long timestamp,
                   ^
refs.c: In function ‘for_each_reflog_ent_reverse’:
refs.c:3451:22: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   cnt = (sizeof(buf) < pos) ? sizeof(buf) : pos;
                      ^
refs.c:3451:43: warning: signed and unsigned type in conditional expression [-Wsign-compare]
   cnt = (sizeof(buf) < pos) ? sizeof(buf) : pos;
                                           ^
refs.c: In function ‘ref_transaction_free’:
refs.c:3659:16: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  for (i = 0; i < transaction->nr; i++) {
                ^
 $ 

Notice the [-Wextra] warnings above. ;-)

ATB,
Ramsay Jones

Re: [PATCHv2 5/5] refs.c: enable large transactions

[Stefan Beller]

On Thu, Jan 22, 2015 at 12:13 PM, Ramsay Jones
<ramsay@ramsay1.demon.co.uk> wrote:
>
> Notice the [-Wextra] warnings above. ;-)
>
> ATB,
> Ramsay Jones
>

Thanks, I put that into my config.mak
Though recompiling the whole project yields

      4 [-Wempty-body]
    477 [-Wmissing-field-initializers]
    966 [-Wsign-compare]
    899 [-Wunused-parameter]

so maybe I'll disable it again when I think it's too much output.

Re: [PATCHv2 5/5] refs.c: enable large transactions

[Ramsay Jones]

On 22/01/15 20:20, Stefan Beller wrote:
> On Thu, Jan 22, 2015 at 12:13 PM, Ramsay Jones
> <ramsay@ramsay1.demon.co.uk> wrote:
>>
>> Notice the [-Wextra] warnings above. ;-)
>>
>> ATB,
>> Ramsay Jones
>>
> 
> Thanks, I put that into my config.mak
> Though recompiling the whole project yields
> 
>       4 [-Wempty-body]
>     477 [-Wmissing-field-initializers]
>     966 [-Wsign-compare]
>     899 [-Wunused-parameter]
> 
> so maybe I'll disable it again when I think it's too much output.
> 

Yes, you don't want to use -Wextra for everyday usage! :-D

[BTW, I don't know if sparse works on OS X, so ...]

ATB,
Ramsay Jones

[RFC PATCH 0/5] So you dislike the sequence of system calls?

[Stefan Beller]

This series goes on top of origin/sb/atomic-push-fix for now.

I am inclined to squash the first patch into the last commit of 
origin/sb/atomic-push-fix when rerolling that series as it just 
fixes the warning Ramsay pointed out. I'd also like to combine
this series with the origin/sb/atomic-push-fix in a reroll of
either series such that it becomes one larger series.

The patches 2,3,4 are just preparations (no change intended)
for the patch 5 which then corrects the sequence of system calls
such that we don't close and reopen the lock file.

(Background: Why am I spending time to fix that bug the way I do?)
I think writing out the sha1 early to the .lock file helps in
further patch series for cross repository atomic pushes, because
if we can split the transaction_commit into two parts, where the
latter part only has lock file pathes in memory, dealing with
cross repository ref updates becomes easy in such a way:
(compare discussion [RFC] Introducing different handling 
for small/large transactions)

        cat << EOF > stdin_pipe
        delete topic2 2345
        update master 4567 2378
        repository ../API-consumer # this switches to another repository
        delete topic2 3459
        update master 6787 9878
        EOF
        git update-ref --stdin <stdin_pipe

Internally update-ref would be easy to implement when all you 
have left are lock files you'd need to commit.

Any feedback welcome!

Thanks,
Stefan

Stefan Beller (5):
  fixup for "refs.c: enable large transactions"
  refs.c: remove unlock_ref from write_ref_sha1
  refs.c: move static functions to close and commit refs
  refs.c: remove committing the ref from write_ref_sha1
  refs.c: write values to lock files early for committing

 refs.c | 81 ++++++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 44 insertions(+), 37 deletions(-)

-- 
2.2.1.62.g3f15098

[PATCH 1/5] fixup for "refs.c: enable large transactions"

[Stefan Beller]

This will fix the warnings Ramsay Jones pointed out.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 9fc0e60..aae3b66 100644
--- a/refs.c
+++ b/refs.c
@@ -3058,12 +3058,12 @@ static int write_sha1_to_lock_file(struct ref_lock *lock,
 {
 	if (lock->lk->fd == -1) {
 		if (reopen_lock_file(lock->lk) < 0
-		    || fdopen_lock_file(lock->lk, "w") < 0
+		    || fdopen_lock_file(lock->lk, "w") == NULL
 		    || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41
 		    || close_lock_file(lock->lk) < 0)
 		    return -1;
 	} else {
-		if (fdopen_lock_file(lock->lk, "w") < 0
+		if (fdopen_lock_file(lock->lk, "w") == NULL
 		    || fprintf(lock->lk->fp, "%s\n", sha1_to_hex(sha1)) != 41)
 		return -1;
 	}
-- 
2.2.1.62.g3f15098

[PATCH 2/5] refs.c: remove unlock_ref from write_ref_sha1

[Stefan Beller]

Instead of calling unlock_ref before each return in write_ref_sha1
we can call this after the call. This is a first step to split up
write_ref_sha1 into the write and commit phase which is done in a
later patch.

There is a call in each code path after write_ref_sha1 now. Even in
the last hunk in the error case, the 'goto cleanup;' will make sure
there is a call to unlock_ref.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/refs.c b/refs.c
index aae3b66..4580919 100644
--- a/refs.c
+++ b/refs.c
@@ -2866,9 +2866,11 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	lock->force_write = 1;
 	hashcpy(lock->old_sha1, orig_sha1);
 	if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+		unlock_ref(lock);
 		error("unable to write current sha1 into %s", newrefname);
 		goto rollback;
 	}
+	unlock_ref(lock);
 
 	return 0;
 
@@ -2884,6 +2886,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	log_all_ref_updates = 0;
 	if (write_ref_sha1(lock, orig_sha1, NULL))
 		error("unable to write current sha1 into %s", oldrefname);
+	unlock_ref(lock);
 	log_all_ref_updates = flag;
 
  rollbacklog:
@@ -3083,22 +3086,19 @@ static int write_ref_sha1(struct ref_lock *lock,
 		errno = EINVAL;
 		return -1;
 	}
-	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
-		unlock_ref(lock);
+	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
 		return 0;
-	}
+
 	o = parse_object(sha1);
 	if (!o) {
 		error("Trying to write ref %s with nonexistent object %s",
 			lock->ref_name, sha1_to_hex(sha1));
-		unlock_ref(lock);
 		errno = EINVAL;
 		return -1;
 	}
 	if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {
 		error("Trying to write non-commit object %s to branch %s",
 			sha1_to_hex(sha1), lock->ref_name);
-		unlock_ref(lock);
 		errno = EINVAL;
 		return -1;
 	}
@@ -3106,7 +3106,6 @@ static int write_ref_sha1(struct ref_lock *lock,
 	    close_ref(lock) < 0) {
 		int save_errno = errno;
 		error("Couldn't write %s", lock->lk->filename.buf);
-		unlock_ref(lock);
 		errno = save_errno;
 		return -1;
 	}
@@ -3114,7 +3113,6 @@ static int write_ref_sha1(struct ref_lock *lock,
 	if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||
 	    (strcmp(lock->ref_name, lock->orig_ref_name) &&
 	     log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {
-		unlock_ref(lock);
 		return -1;
 	}
 	if (strcmp(lock->orig_ref_name, "HEAD") != 0) {
@@ -3141,10 +3139,8 @@ static int write_ref_sha1(struct ref_lock *lock,
 	}
 	if (commit_ref(lock)) {
 		error("Couldn't set %s", lock->ref_name);
-		unlock_ref(lock);
 		return -1;
 	}
-	unlock_ref(lock);
 	return 0;
 }
 
@@ -3770,7 +3766,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			goto cleanup;
 		}
 		/* Do not keep all lock files open at the same time. */
-		close_lock_file(update->lock->lk);
+		close_ref(update->lock);
 	}
 
 	/* Perform updates first so live commits remain referenced */
@@ -3780,13 +3776,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		if (!is_null_sha1(update->new_sha1)) {
 			if (write_ref_sha1(update->lock, update->new_sha1,
 					   update->msg)) {
-				update->lock = NULL; /* freed by write_ref_sha1 */
 				strbuf_addf(err, "Cannot update the ref '%s'.",
 					    update->refname);
 				ret = TRANSACTION_GENERIC_ERROR;
 				goto cleanup;
 			}
-			update->lock = NULL; /* freed by write_ref_sha1 */
+			unlock_ref(update->lock);
+			update->lock = NULL;
 		}
 	}
 
-- 
2.2.1.62.g3f15098

[PATCH 3/5] refs.c: move static functions to close and commit refs

[Stefan Beller]

By moving the functions up we don't need to have to declare them first
when using them in a later patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/refs.c b/refs.c
index 4580919..6f3cd7b 100644
--- a/refs.c
+++ b/refs.c
@@ -2808,6 +2808,20 @@ static int rename_ref_available(const char *oldname, const char *newname)
 static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
 			  const char *logmsg);
 
+static int close_ref(struct ref_lock *lock)
+{
+	if (close_lock_file(lock->lk))
+		return -1;
+	return 0;
+}
+
+static int commit_ref(struct ref_lock *lock)
+{
+	if (commit_lock_file(lock->lk))
+		return -1;
+	return 0;
+}
+
 int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)
 {
 	unsigned char sha1[20], orig_sha1[20];
@@ -2901,20 +2915,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	return 1;
 }
 
-static int close_ref(struct ref_lock *lock)
-{
-	if (close_lock_file(lock->lk))
-		return -1;
-	return 0;
-}
-
-static int commit_ref(struct ref_lock *lock)
-{
-	if (commit_lock_file(lock->lk))
-		return -1;
-	return 0;
-}
-
 /*
  * copy the reflog message msg to buf, which has been allocated sufficiently
  * large, while cleaning up the whitespaces.  Especially, convert LF to space,
-- 
2.2.1.62.g3f15098

[PATCH 4/5] refs.c: remove committing the ref from write_ref_sha1

[Stefan Beller]

This makes write_ref_sha1 only write the the lock file, committing
needs to be done outside of that function. This will help us change
the ref_transaction_commit in a later patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index 6f3cd7b..c108c95 100644
--- a/refs.c
+++ b/refs.c
@@ -2815,8 +2815,11 @@ static int close_ref(struct ref_lock *lock)
 	return 0;
 }
 
-static int commit_ref(struct ref_lock *lock)
+static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
 {
+	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
+		return 0;
+
 	if (commit_lock_file(lock->lk))
 		return -1;
 	return 0;
@@ -2879,7 +2882,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	}
 	lock->force_write = 1;
 	hashcpy(lock->old_sha1, orig_sha1);
-	if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+	if (write_ref_sha1(lock, orig_sha1, logmsg)
+	    || commit_ref(lock, orig_sha1)) {
 		unlock_ref(lock);
 		error("unable to write current sha1 into %s", newrefname);
 		goto rollback;
@@ -2898,8 +2902,10 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	lock->force_write = 1;
 	flag = log_all_ref_updates;
 	log_all_ref_updates = 0;
-	if (write_ref_sha1(lock, orig_sha1, NULL))
+	if (write_ref_sha1(lock, orig_sha1, NULL)
+	    || commit_ref(lock, orig_sha1))
 		error("unable to write current sha1 into %s", oldrefname);
+
 	unlock_ref(lock);
 	log_all_ref_updates = flag;
 
@@ -3137,10 +3143,6 @@ static int write_ref_sha1(struct ref_lock *lock,
 		    !strcmp(head_ref, lock->ref_name))
 			log_ref_write("HEAD", lock->old_sha1, sha1, logmsg);
 	}
-	if (commit_ref(lock)) {
-		error("Couldn't set %s", lock->ref_name);
-		return -1;
-	}
 	return 0;
 }
 
@@ -3775,7 +3777,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 		if (!is_null_sha1(update->new_sha1)) {
 			if (write_ref_sha1(update->lock, update->new_sha1,
-					   update->msg)) {
+					   update->msg)
+			    || commit_ref(update->lock, update->new_sha1)) {
 				strbuf_addf(err, "Cannot update the ref '%s'.",
 					    update->refname);
 				ret = TRANSACTION_GENERIC_ERROR;
@@ -4064,7 +4067,8 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 		} else if (commit_lock_file(&reflog_lock)) {
 			status |= error("unable to commit reflog '%s' (%s)",
 					log_file, strerror(errno));
-		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {
+		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF)
+			    && commit_ref(lock, cb.last_kept_sha1)) {
 			status |= error("couldn't set %s", lock->ref_name);
 		}
 	}
-- 
2.2.1.62.g3f15098

[PATCH 5/5] refs.c: write values to lock files early for committing

[Stefan Beller]

By writing the values to the lock file early we have a better sequence
of system calls. Before this commit we had

    open(), close(), open(), fdopen(), write(), fclose(), rename()

Now there is:

    open(), fdopen(), write(), fclose(), rename()

The first four operations are performed in the first loop of
ref_transaction_commit ("/* Acquire all locks while verifying
old values */"), while the renameing is left to the next loop.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index c108c95..3d1890f 100644
--- a/refs.c
+++ b/refs.c
@@ -3767,6 +3767,15 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				    update->refname);
 			goto cleanup;
 		}
+		if (!is_null_sha1(update->new_sha1)) {
+			if (write_ref_sha1(update->lock, update->new_sha1,
+					   update->msg)) {
+				strbuf_addf(err, "Cannot update the ref '%s'.",
+					    update->refname);
+				ret = TRANSACTION_GENERIC_ERROR;
+				goto cleanup;
+			}
+		}
 		/* Do not keep all lock files open at the same time. */
 		close_ref(update->lock);
 	}
@@ -3776,9 +3785,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		struct ref_update *update = updates[i];
 
 		if (!is_null_sha1(update->new_sha1)) {
-			if (write_ref_sha1(update->lock, update->new_sha1,
-					   update->msg)
-			    || commit_ref(update->lock, update->new_sha1)) {
+			if (commit_ref(update->lock, update->new_sha1)) {
 				strbuf_addf(err, "Cannot update the ref '%s'.",
 					    update->refname);
 				ret = TRANSACTION_GENERIC_ERROR;
-- 
2.2.1.62.g3f15098

[PATCHv3 0/6] Fix bug in large transactions

[Stefan Beller]

version3:
  patches 1,2,3 stayed completely as is, while patches 4,5 are new, patch 6 is 
  rewritten to first write the contents of the lock files before closing them.
  
  This combines the series "Enable large transactions v2" as sent out yesterday
  with the follow up series "[RFC PATCH 0/5] So you dislike the sequence of 
  system calls?"
  
  There is no write_in_full_to_lock_file wrapper any more, but write_ref_sha1
  was reduced in functionality in patch 5.
  
  This applies on top of origin/sb/atomic-push and results in a merge conflict
  when merging it to origin/jk/lock-ref-sha1-basic-return-errors which looks like
        
        $git checkout origin/jk/lock-ref-sha1-basic-return-errors
        $git merge enable_large_transactions
        CONFLICT (content): Merge conflict in refs.c
        $git diff
++<<<<<<< HEAD
 +      lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
 +      if (lock->lock_fd < 0) {
++=======
+       if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {
++>>>>>>> enable_large_transactions

which is best resolved as:
@@@ -2316,8 -2333,7 +2333,12 @@@ static struct ref_lock *lock_ref_sha1_b
                goto error_return;
        }
  
+        if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {
                last_errno = errno;
                if (errno == ENOENT && --attempts_remaining > 0)
                        /*


version2:

* This applies on top of origin/sb/atomic-push though it will result in a one
  line merge conflict with origin/jk/lock-ref-sha1-basic-return-errors when
  merging to origin/next.

* It now uses the FILE* pointer instead of file descriptors. This
  results in a combination of the 2 former patches "refs.c: have
  a write_in_full_to_lock_file wrapper" and "refs.c: write to a
  lock file only once" as the wrapper function is more adapted to
  its consumers

* no need to dance around with char *pointers which may leak.

* another new patch sneaked into the series: Renaming ULIMIT in t7004
  to ULIMIT_STACK_SIZE

That said, only the first and third patch are updated from the first version
of the patches. The others are new in the sense that rewriting them was cheaper
than keeping notes in between.

version1:

(reported as: git update-ref --stdin : too many open files, 2014-12-20)

First a test case is introduced to demonstrate the failure,
the patches 2-6 are little refactoring and the last patch
fixes the bug and also marks the bugs as resolved in the
test suite.

Unfortunately this applies on top of origin/next.

Any feedback would be welcome!

Thanks,
Stefan

Stefan Beller (6):
  update-ref: test handling large transactions properly
  t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE
  refs.c: remove lock_fd from struct ref_lock
  refs.c: move static functions to close and commit refs
  refs.c: remove unlock_ref and commit_ref from write_ref_sha1
  refs.c: enable large transactions

 refs.c                | 93 +++++++++++++++++++++++++++------------------------
 t/t1400-update-ref.sh | 28 ++++++++++++++++
 t/t7004-tag.sh        |  4 +--
 3 files changed, 79 insertions(+), 46 deletions(-)

-- 
2.2.1.62.g3f15098

[PATCHv3 1/6] update-ref: test handling large transactions properly

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

Notes:
    v2->v3:
    no changes

 t/t1400-update-ref.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 7b4707b..47d2fe9 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -973,4 +973,32 @@ test_expect_success 'stdin -z delete refs works with packed and loose refs' '
 	test_must_fail git rev-parse --verify -q $c
 '
 
+run_with_limited_open_files () {
+	(ulimit -n 32 && "$@")
+}
+
+test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'
+
+test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
+(
+	for i in $(test_seq 33)
+	do
+		echo "create refs/heads/$i HEAD"
+	done >large_input &&
+	run_with_limited_open_files git update-ref --stdin <large_input &&
+	git rev-parse --verify -q refs/heads/33
+)
+'
+
+test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
+(
+	for i in $(test_seq 33)
+	do
+		echo "delete refs/heads/$i HEAD"
+	done >large_input &&
+	run_with_limited_open_files git update-ref --stdin <large_input &&
+	test_must_fail git rev-parse --verify -q refs/heads/33
+)
+'
+
 test_done
-- 
2.2.1.62.g3f15098

[PATCHv3 2/6] t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE

[Stefan Beller]

During creation of the patch series our discussion we could have a
more descriptive name for the prerequisite for the test so it stays
unique when other limits of ulimit are introduced.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

Notes:
    v2->v3:
    no changes

 t/t7004-tag.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 796e9f7..06b8e0d 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1463,10 +1463,10 @@ run_with_limited_stack () {
 	(ulimit -s 128 && "$@")
 }
 
-test_lazy_prereq ULIMIT 'run_with_limited_stack true'
+test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
 
 # we require ulimit, this excludes Windows
-test_expect_success ULIMIT '--contains works in a deep repo' '
+test_expect_success ULIMIT_STACK_SIZE '--contains works in a deep repo' '
 	>expect &&
 	i=1 &&
 	while test $i -lt 8000
-- 
2.2.1.62.g3f15098

[PATCHv3 3/6] refs.c: remove lock_fd from struct ref_lock

[Stefan Beller]

The 'lock_fd' is the same as 'lk->fd'. No need to store it twice so remove
it. You may argue this introduces more coupling as we need to know more
about the internals of the lock file mechanism, but this will be solved in
a later patch.

No functional changes intended.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

Notes:
    v2->v3:
    no changes

 refs.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index 14e52ca..4066752 100644
--- a/refs.c
+++ b/refs.c
@@ -11,7 +11,6 @@ struct ref_lock {
 	char *orig_ref_name;
 	struct lock_file *lk;
 	unsigned char old_sha1[20];
-	int lock_fd;
 	int force_write;
 };
 
@@ -2259,7 +2258,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	int attempts_remaining = 3;
 
 	lock = xcalloc(1, sizeof(struct ref_lock));
-	lock->lock_fd = -1;
 
 	if (mustexist)
 		resolve_flags |= RESOLVE_REF_READING;
@@ -2335,8 +2333,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		goto error_return;
 	}
 
-	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
-	if (lock->lock_fd < 0) {
+	if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {
+		last_errno = errno;
 		if (errno == ENOENT && --attempts_remaining > 0)
 			/*
 			 * Maybe somebody just deleted one of the
@@ -2904,7 +2902,6 @@ static int close_ref(struct ref_lock *lock)
 {
 	if (close_lock_file(lock->lk))
 		return -1;
-	lock->lock_fd = -1;
 	return 0;
 }
 
@@ -2912,7 +2909,6 @@ static int commit_ref(struct ref_lock *lock)
 {
 	if (commit_lock_file(lock->lk))
 		return -1;
-	lock->lock_fd = -1;
 	return 0;
 }
 
@@ -3090,8 +3086,8 @@ static int write_ref_sha1(struct ref_lock *lock,
 		errno = EINVAL;
 		return -1;
 	}
-	if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||
-	    write_in_full(lock->lock_fd, &term, 1) != 1 ||
+	if (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 ||
+	    write_in_full(lock->lk->fd, &term, 1) != 1 ||
 	    close_ref(lock) < 0) {
 		int save_errno = errno;
 		error("Couldn't write %s", lock->lk->filename.buf);
@@ -4047,9 +4043,9 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 			status |= error("couldn't write %s: %s", log_file,
 					strerror(errno));
 		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
-			(write_in_full(lock->lock_fd,
+			(write_in_full(lock->lk->fd,
 				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
-			 write_str_in_full(lock->lock_fd, "\n") != 1 ||
+			 write_str_in_full(lock->lk->fd, "\n") != 1 ||
 			 close_ref(lock) < 0)) {
 			status |= error("couldn't write %s",
 					lock->lk->filename.buf);
-- 
2.2.1.62.g3f15098

[PATCHv3 4/6] refs.c: move static functions to close and commit refs

[Stefan Beller]

By moving the functions up we don't need to have to declare them first
when using them in a later patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    new in v3

 refs.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/refs.c b/refs.c
index 4066752..f1eefc7 100644
--- a/refs.c
+++ b/refs.c
@@ -2808,6 +2808,20 @@ static int rename_ref_available(const char *oldname, const char *newname)
 static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
 			  const char *logmsg);
 
+static int close_ref(struct ref_lock *lock)
+{
+	if (close_lock_file(lock->lk))
+		return -1;
+	return 0;
+}
+
+static int commit_ref(struct ref_lock *lock)
+{
+	if (commit_lock_file(lock->lk))
+		return -1;
+	return 0;
+}
+
 int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)
 {
 	unsigned char sha1[20], orig_sha1[20];
@@ -2898,20 +2912,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	return 1;
 }
 
-static int close_ref(struct ref_lock *lock)
-{
-	if (close_lock_file(lock->lk))
-		return -1;
-	return 0;
-}
-
-static int commit_ref(struct ref_lock *lock)
-{
-	if (commit_lock_file(lock->lk))
-		return -1;
-	return 0;
-}
-
 /*
  * copy the reflog message msg to buf, which has been allocated sufficiently
  * large, while cleaning up the whitespaces.  Especially, convert LF to space,
-- 
2.2.1.62.g3f15098

[PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1

[Stefan Beller]

This makes write_ref_sha1 only write the the lock file, committing
needs to be done outside of that function. This will help us change
the ref_transaction_commit in a later patch.

Also instead of calling unlock_ref before each return in write_ref_sha1
we can call this after the call. This is a first step to split up
write_ref_sha1 into the write and commit phase which is done in a
later patch.

There is a call in each code path after write_ref_sha1 now. Even in
the last hunk in the error case, the 'goto cleanup;' will make sure
there is a call to unlock_ref.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    new in v3

 refs.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/refs.c b/refs.c
index f1eefc7..1bfc84b 100644
--- a/refs.c
+++ b/refs.c
@@ -2815,8 +2815,11 @@ static int close_ref(struct ref_lock *lock)
 	return 0;
 }
 
-static int commit_ref(struct ref_lock *lock)
+static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
 {
+	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
+		return 0;
+
 	if (commit_lock_file(lock->lk))
 		return -1;
 	return 0;
@@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	}
 	lock->force_write = 1;
 	hashcpy(lock->old_sha1, orig_sha1);
-	if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+	if (write_ref_sha1(lock, orig_sha1, logmsg)
+	    || commit_ref(lock, orig_sha1)) {
+		unlock_ref(lock);
 		error("unable to write current sha1 into %s", newrefname);
 		goto rollback;
 	}
+	unlock_ref(lock);
 
 	return 0;
 
@@ -2896,8 +2902,11 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	lock->force_write = 1;
 	flag = log_all_ref_updates;
 	log_all_ref_updates = 0;
-	if (write_ref_sha1(lock, orig_sha1, NULL))
+	if (write_ref_sha1(lock, orig_sha1, NULL)
+	    || commit_ref(lock, orig_sha1))
 		error("unable to write current sha1 into %s", oldrefname);
+
+	unlock_ref(lock);
 	log_all_ref_updates = flag;
 
  rollbacklog:
@@ -3067,22 +3076,19 @@ static int write_ref_sha1(struct ref_lock *lock,
 		errno = EINVAL;
 		return -1;
 	}
-	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
-		unlock_ref(lock);
+	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
 		return 0;
-	}
+
 	o = parse_object(sha1);
 	if (!o) {
 		error("Trying to write ref %s with nonexistent object %s",
 			lock->ref_name, sha1_to_hex(sha1));
-		unlock_ref(lock);
 		errno = EINVAL;
 		return -1;
 	}
 	if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {
 		error("Trying to write non-commit object %s to branch %s",
 			sha1_to_hex(sha1), lock->ref_name);
-		unlock_ref(lock);
 		errno = EINVAL;
 		return -1;
 	}
@@ -3091,7 +3097,6 @@ static int write_ref_sha1(struct ref_lock *lock,
 	    close_ref(lock) < 0) {
 		int save_errno = errno;
 		error("Couldn't write %s", lock->lk->filename.buf);
-		unlock_ref(lock);
 		errno = save_errno;
 		return -1;
 	}
@@ -3099,7 +3104,6 @@ static int write_ref_sha1(struct ref_lock *lock,
 	if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||
 	    (strcmp(lock->ref_name, lock->orig_ref_name) &&
 	     log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {
-		unlock_ref(lock);
 		return -1;
 	}
 	if (strcmp(lock->orig_ref_name, "HEAD") != 0) {
@@ -3124,12 +3128,6 @@ static int write_ref_sha1(struct ref_lock *lock,
 		    !strcmp(head_ref, lock->ref_name))
 			log_ref_write("HEAD", lock->old_sha1, sha1, logmsg);
 	}
-	if (commit_ref(lock)) {
-		error("Couldn't set %s", lock->ref_name);
-		unlock_ref(lock);
-		return -1;
-	}
-	unlock_ref(lock);
 	return 0;
 }
 
@@ -3762,14 +3760,15 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 		if (!is_null_sha1(update->new_sha1)) {
 			if (write_ref_sha1(update->lock, update->new_sha1,
-					   update->msg)) {
-				update->lock = NULL; /* freed by write_ref_sha1 */
+					   update->msg)
+			    || commit_ref(update->lock, update->new_sha1)) {
 				strbuf_addf(err, "Cannot update the ref '%s'.",
 					    update->refname);
 				ret = TRANSACTION_GENERIC_ERROR;
 				goto cleanup;
 			}
-			update->lock = NULL; /* freed by write_ref_sha1 */
+			unlock_ref(update->lock);
+			update->lock = NULL;
 		}
 	}
 
@@ -4053,7 +4052,8 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 		} else if (commit_lock_file(&reflog_lock)) {
 			status |= error("unable to commit reflog '%s' (%s)",
 					log_file, strerror(errno));
-		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {
+		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF)
+			    && commit_ref(lock, cb.last_kept_sha1)) {
 			status |= error("couldn't set %s", lock->ref_name);
 		}
 	}
-- 
2.2.1.62.g3f15098

[PATCHv3 6/6] refs.c: enable large transactions

[Stefan Beller]

By closing the file descriptors after creating the lock file we are not
limiting the size of the transaction by the number of available file
descriptors.

When closing the file descriptors early, we also need to write the values
in early, if we don't want to reopen the files.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    version3:
    * Do not reopen the files after closing them. Make sure we have
      written all necessary information before closing the file.
      Doing it that way enabled us to drop the patch "[PATCH 4/6]
      refs.c: Have a write_in_full_to_lock_file wrapping write_in_full"

 refs.c                | 15 ++++++++++++---
 t/t1400-update-ref.sh |  4 ++--
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index 1bfc84b..2594b23 100644
--- a/refs.c
+++ b/refs.c
@@ -3752,6 +3752,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				    update->refname);
 			goto cleanup;
 		}
+		if (!is_null_sha1(update->new_sha1)) {
+			if (write_ref_sha1(update->lock, update->new_sha1,
+					   update->msg)) {
+				strbuf_addf(err, "Cannot write to the ref lock '%s'.",
+					    update->refname);
+				ret = TRANSACTION_GENERIC_ERROR;
+				goto cleanup;
+			}
+		}
+		/* Do not keep all lock files open at the same time. */
+		close_ref(update->lock);
 	}
 
 	/* Perform updates first so live commits remain referenced */
@@ -3759,9 +3770,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		struct ref_update *update = updates[i];
 
 		if (!is_null_sha1(update->new_sha1)) {
-			if (write_ref_sha1(update->lock, update->new_sha1,
-					   update->msg)
-			    || commit_ref(update->lock, update->new_sha1)) {
+			if (commit_ref(update->lock, update->new_sha1)) {
 				strbuf_addf(err, "Cannot update the ref '%s'.",
 					    update->refname);
 				ret = TRANSACTION_GENERIC_ERROR;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 47d2fe9..c593a1d 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -979,7 +979,7 @@ run_with_limited_open_files () {
 
 test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'
 
-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
 (
 	for i in $(test_seq 33)
 	do
@@ -990,7 +990,7 @@ test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches
 )
 '
 
-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
 (
 	for i in $(test_seq 33)
 	do
-- 
2.2.1.62.g3f15098

Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> -static int commit_ref(struct ref_lock *lock)
> +static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
>  {
> +	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
> +		return 0;
>  	if (commit_lock_file(lock->lk))
>  		return -1;
>  	return 0;
> @@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>  	}
>  	lock->force_write = 1;
>  	hashcpy(lock->old_sha1, orig_sha1);
> -	if (write_ref_sha1(lock, orig_sha1, logmsg)) {
> +	if (write_ref_sha1(lock, orig_sha1, logmsg)
> +	    || commit_ref(lock, orig_sha1)) {
> +		unlock_ref(lock);

This is not a new problem, but the two lines in pre-context of this
patch look strange.  When the code is renaming into some ref, the
ref either would have no original SHA-1 (i.e. we are renaming to a
non-existing name) or have unrelated SHA-1 (i.e. we are overwriting
an existing one).  For some (unknown to me) reason, however, the
code pretends that lock->old_sha1 has the new SHA-1 already before
we start to do the write or commit.

And because both write and commit tries to pretend to be no-op when
the caller tries to update a ref with the same SHA-1, but in this
codepath it does want the write to happen, it needs to set the
force_write bit set, which look like an unnecessary workaround.

Regardless of what this particular caller does, I am not sure if the
early-return codepath in commit_ref() is correct.  From the callers'
point of view, it sometimes unlocks the ref (i.e. when a different
SHA-1 is written or force_write is set) and sometimes keeps the ref
locked (i.e. when early-return is taken).  Shouldn't these two cases
behave identically?  Or am I wrong to assume that the early return
using "hashcmp(lock->old_sha1, sha1)" is a mere optimization?

Re: [PATCHv3 6/6] refs.c: enable large transactions

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> By closing the file descriptors after creating the lock file we are not
> limiting the size of the transaction by the number of available file
> descriptors.
>
> When closing the file descriptors early, we also need to write the values
> in early, if we don't want to reopen the files.


I am not sure if an early return in write_ref_sha1() is entirely
correct.  The unlock step was split out of write and commit
functions in the previous step because you eventually want to be
able to close the file descriptor that is open to $ref.lock early,
while still keeping the $ref.lock file around to avoid others
competing with you, so that you can limit the number of open file
descriptors, no?

If so, shouldn't the write function at least close the file
descriptor even when it knows that the $ref.lock already has the
correct object name?  The call to close_ref() is never made when the
early-return codepath is taken as far as I can see.

Puzzled...

Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1

[Stefan Beller]

On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> -static int commit_ref(struct ref_lock *lock)
>> +static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
>>  {
>> +     if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
>> +             return 0;
>>       if (commit_lock_file(lock->lk))
>>               return -1;
>>       return 0;
>> @@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>>       }
>>       lock->force_write = 1;
>>       hashcpy(lock->old_sha1, orig_sha1);
>> -     if (write_ref_sha1(lock, orig_sha1, logmsg)) {
>> +     if (write_ref_sha1(lock, orig_sha1, logmsg)
>> +         || commit_ref(lock, orig_sha1)) {
>> +             unlock_ref(lock);
>
> This is not a new problem, but the two lines in pre-context of this
> patch look strange.

Which (not new) problem are you talking about here? Do you have
a reference?

> Regardless of what this particular caller does, I am not sure if the
> early-return codepath in commit_ref() is correct.  From the callers'
> point of view, it sometimes unlocks the ref (i.e. when a different
> SHA-1 is written or force_write is set) and sometimes keeps the ref
> locked (i.e. when early-return is taken).  Shouldn't these two cases
> behave identically?  Or am I wrong to assume that the early return
> using "hashcmp(lock->old_sha1, sha1)" is a mere optimization?
>

I assumed it was not just an optimization as the test suite fails if I
touch that line. I'll look into cleaning it up in a more obvious fashion.

Re: [PATCHv3 6/6] refs.c: enable large transactions

[Stefan Beller]

On Fri, Jan 23, 2015 at 4:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> By closing the file descriptors after creating the lock file we are not
>> limiting the size of the transaction by the number of available file
>> descriptors.
>>
>> When closing the file descriptors early, we also need to write the values
>> in early, if we don't want to reopen the files.
>
>
> I am not sure if an early return in write_ref_sha1() is entirely
> correct.  The unlock step was split out of write and commit
> functions in the previous step because you eventually want to be
> able to close the file descriptor that is open to $ref.lock early,
> while still keeping the $ref.lock file around to avoid others
> competing with you, so that you can limit the number of open file
> descriptors, no?

yeah that's the goal. Though as we're in one transaction, as soon
as we have an early exit, the transaction will abort.

>
> If so, shouldn't the write function at least close the file
> descriptor even when it knows that the $ref.lock already has the
> correct object name?  The call to close_ref() is never made when the
> early-return codepath is taken as far as I can see.
>

The  goto cleanup; will take care of unlocking (and closing fds of) all refs

Re: [PATCHv3 6/6] refs.c: enable large transactions

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Fri, Jan 23, 2015 at 4:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> yeah that's the goal. Though as we're in one transaction, as soon
> as we have an early exit, the transaction will abort.

An early exit I am talking about is this:

static int write_ref_sha1(struct ref_lock *lock,
	const unsigned char *sha1, const char *logmsg)
{
	static char term = '\n';
	struct object *o;

	if (!lock) {
		errno = EINVAL;
		return -1;
	}
	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
		return 0;

It returns 0 and then the transaction side has this call in a loop:

		if (!is_null_sha1(update->new_sha1)) {
			if (write_ref_sha1(update->lock, update->new_sha1,
					   update->msg)) {
				strbuf_addf(err, "Cannot write to the ref lock '%s'.",
					    update->refname);
				ret = TRANSACTION_GENERIC_ERROR;
				goto cleanup;
			}
		}

>> If so, shouldn't the write function at least close the file
>> descriptor even when it knows that the $ref.lock already has the
>> correct object name?  The call to close_ref() is never made when the
>> early-return codepath is taken as far as I can see.
>
> The  goto cleanup; will take care of unlocking (and closing fds of) all refs

Yes, if write_ref_sha1() returns with non-zero signaling an error,
then the goto will trigger.

But if it short-cuts and returns zero, that goto will not be
reached.

Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> -static int commit_ref(struct ref_lock *lock)
>>> +static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
>>>  {
>>> +     if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
>>> +             return 0;
>>>       if (commit_lock_file(lock->lk))
>>>               return -1;
>>>       return 0;
>>> @@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>>>       }
>>>       lock->force_write = 1;
>>>       hashcpy(lock->old_sha1, orig_sha1);
>>> -     if (write_ref_sha1(lock, orig_sha1, logmsg)) {
>>> +     if (write_ref_sha1(lock, orig_sha1, logmsg)
>>> +         || commit_ref(lock, orig_sha1)) {
>>> +             unlock_ref(lock);
>>
>> This is not a new problem, but the two lines in pre-context of this
>> patch look strange.
>
> Which (not new) problem are you talking about here? Do you have
> a reference?

These two lines in pre-context of the hunk:

>>>       lock->force_write = 1;
>>>       hashcpy(lock->old_sha1, orig_sha1);

Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1

[Stefan Beller]

On Fri, Jan 23, 2015 at 4:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Stefan Beller <sbeller@google.com> writes:
>>>
>>>> -static int commit_ref(struct ref_lock *lock)
>>>> +static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
>>>>  {
>>>> +     if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
>>>> +             return 0;
>>>>       if (commit_lock_file(lock->lk))
>>>>               return -1;
>>>>       return 0;
>>>> @@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>>>>       }
>>>>       lock->force_write = 1;
>>>>       hashcpy(lock->old_sha1, orig_sha1);
>>>> -     if (write_ref_sha1(lock, orig_sha1, logmsg)) {
>>>> +     if (write_ref_sha1(lock, orig_sha1, logmsg)
>>>> +         || commit_ref(lock, orig_sha1)) {
>>>> +             unlock_ref(lock);
>>>
>>> This is not a new problem, but the two lines in pre-context of this
>>> patch look strange.
>>
>> Which (not new) problem are you talking about here? Do you have
>> a reference?
>
> These two lines in pre-context of the hunk:
>
>>>>       lock->force_write = 1;
>>>>       hashcpy(lock->old_sha1, orig_sha1);
>

So these 2 lines (specially the force_write=1 line) is just there to trigger
the valid early exit path as you sent in the other mail :

> if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
>             return 0;

when we have the same sha1?
and you're saying that's a problem because hard to understand?

I am confused as well by now.

Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Fri, Jan 23, 2015 at 4:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Stefan Beller <sbeller@google.com> writes:
>>>>
>>>>> -static int commit_ref(struct ref_lock *lock)
>>>>> +static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
>>>>>  {
>>>>> +     if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
>>>>> +             return 0;
>>>>>       if (commit_lock_file(lock->lk))
>>>>>               return -1;
>>>>>       return 0;
>>>>> @@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>>>>>       }
>>>>>       lock->force_write = 1;
>>>>>       hashcpy(lock->old_sha1, orig_sha1);
>>>>> -     if (write_ref_sha1(lock, orig_sha1, logmsg)) {
>>>>> +     if (write_ref_sha1(lock, orig_sha1, logmsg)
>>>>> +         || commit_ref(lock, orig_sha1)) {
>>>>> +             unlock_ref(lock);
>>>>
>>>> This is not a new problem, but the two lines in pre-context of this
>>>> patch look strange.
>>>
>>> Which (not new) problem are you talking about here? Do you have
>>> a reference?
>>
>> These two lines in pre-context of the hunk:
>>
>>>>>       lock->force_write = 1;
>>>>>       hashcpy(lock->old_sha1, orig_sha1);
>>
>
> So these 2 lines (specially the force_write=1 line) is just there to trigger
> the valid early exit path as you sent in the other mail :
>
>> if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
>>             return 0;
>
> when we have the same sha1?
> and you're saying that's a problem because hard to understand?

What is the point of that hashcmp() in the first place?  My
understanding is that 

 (1) lock->old_sha1 is to hold the original SHA-1 in the ref we took
     the lock on.

 (2) when not forcing, and when the two SHA-1 are the same, we
     bypass and return early because overwriting the ref with the
     same value is no-op.

Now, in that codepath, when the code is renaming into some ref, the
ref either would have no original SHA-1 (i.e. we are renaming to a
non-existing name) or have unrelated SHA-1 (i.e. we are overwriting
an existing one).  For some (unknown to me) reason, however, the
code pretends that lock->old_sha1 has the new SHA-1 already before
we start to do the write or commit.

And because both write and commit tries to pretend to be no-op when
the caller tries to update a ref with the same SHA-1, but in this
codepath it does want the write to happen, it needs to set the
force_write bit set, which look like an unnecessary workaround.

Let me rephrase.

A natural way to write that caller, I think, would be more like
this:

	... lock is given by the caller, probably with ->old_sha1
	... filled in; it is not likely to be orig_sha1, as it is
        ... either null (if locking to create a new ref) or some
        ... unrelated value read from the ref being overwritten by
        ... this rename_ref() operation.

	write_ref_sha1(lock, orig_sha1, logmsg);
	# which may do the "don't write the same value" optmization
        # if we are renaming another ref that happens to have the
        # same value, in which case it is OK. Otherwise this will
        # overwrite without being forced.

	... *IF* and only if there is some reason lock->old_sha1
        ... needs to match what is in the filesystem ref right now,
        ... then do
	hashcpy(lock->old_sha1, orig_sha1);
	... but probably there is no reason to do so.

The two lines hashcpy() and ->force_write = 1 that appear before the
write_ref_sha1() do not conceptually make sense.  The former does
not make sense because ->old_sha1 is supposed to be the original
value that is already stored in the ref, to allow us optimize "write
the same value" case, and you are breaking that invariant by doing
hashcpy() before write_ref_sha1().  The lock->old_sha1 value does
not have anything to do with the (original) value of the ref we took
the lock on.

And ->force_write = 1 becomes necessary only because the effect of
this nonsensical hashcpy() is to make the !hashcmp() used by the
short-cut logic trigger.  Since the code needs to override that
logic, you need to say "force" before calling write_ref_sha1().  If
you didn't do the hashcpy(), you wouldn't have to say "force", no?

Re: [PATCHv3 6/6] refs.c: enable large transactions

[Stefan Beller]

On Fri, Jan 23, 2015 at 4:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Fri, Jan 23, 2015 at 4:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> yeah that's the goal. Though as we're in one transaction, as soon
>> as we have an early exit, the transaction will abort.
>
> An early exit I am talking about is this:
>
> static int write_ref_sha1(struct ref_lock *lock,
>         const unsigned char *sha1, const char *logmsg)
> {
>         static char term = '\n';
>         struct object *o;
>
>         if (!lock) {
>                 errno = EINVAL;
>                 return -1;
>         }
>         if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
>                 return 0;
>
> It returns 0 and then the transaction side has this call in a loop:
>
>                 if (!is_null_sha1(update->new_sha1)) {
>                         if (write_ref_sha1(update->lock, update->new_sha1,
>                                            update->msg)) {
>                                 strbuf_addf(err, "Cannot write to the ref lock '%s'.",
>                                             update->refname);
>                                 ret = TRANSACTION_GENERIC_ERROR;
>                                 goto cleanup;
>                         }
>                 }

And just after this code path there is the new
+               /* Do not keep all lock files open at the same time. */
+               close_ref(update->lock);

So in case we go the zero return path of write_ref_sha1 the loop looks like
    /* Acquire all locks while verifying old values */
    for (all updates) {
        write_ref_sha1(...)
        close_ref(...)
    }

In case we do go the non zero return path, it is
  if (write_ref_sha1(update->lock, update->new_sha1, ..)
    goto cleanup;
...
cleanup:
    for (i = 0; i < n; i++)
        if (updates[i]->lock)
            unlock_ref(updates[i]->lock);

I do not see the problem in the code itself, but rather in understanding
the code. I will send a follow up patch which makes it easier to follow
by removing the early exit with no problem away.

>
>>> If so, shouldn't the write function at least close the file
>>> descriptor even when it knows that the $ref.lock already has the
>>> correct object name?  The call to close_ref() is never made when the
>>> early-return codepath is taken as far as I can see.
>>
>> The  goto cleanup; will take care of unlocking (and closing fds of) all refs
>
> Yes, if write_ref_sha1() returns with non-zero signaling an error,
> then the goto will trigger.
>
> But if it short-cuts and returns zero, that goto will not be
> reached.

Yes, if the goto is not reached we just drop down to
close_ref(update->lock); which should take care of
the open fd.

Thanks,
Stefan

[PATCH] refs.c: clean up write_ref_sha1 returns

[Stefan Beller]

write_ref_sha1 now either returns 0 for a successful write or !=0 if an
error occurred. This cleanup results in cleaning the code at other places
as well where we had to set force_write to make the write_ref_sha1(...)
|| commit_ref(...) combination work. Also the checks for the optimisation
of old and new sha1 values being the same has been moved to a helper
function so that check is not part of write_ref_sha1 or commit_ref any
more.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    v1:
    
    applies on top of origin/sb/atomic-push-fix (79370dd9656)
    
    This undoes some of the changes of previous patches
    such as removing the check if old and new sha1 are equal
    and not being forced to rewrite the value.
    
    Junio, I think this patch adresses the concerns you raised.
    I can redo the atomic-push-fix series with this cleanup merged
    into the appropriate patches or you could just queue it on top 
    of said series.

 refs.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/refs.c b/refs.c
index 2594b23..c8fd4a4 100644
--- a/refs.c
+++ b/refs.c
@@ -2817,9 +2817,6 @@ static int close_ref(struct ref_lock *lock)
 
 static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
 {
-	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
-		return 0;
-
 	if (commit_lock_file(lock->lk))
 		return -1;
 	return 0;
@@ -2880,7 +2877,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 		error("unable to lock %s for update", newrefname);
 		goto rollback;
 	}
-	lock->force_write = 1;
 	hashcpy(lock->old_sha1, orig_sha1);
 	if (write_ref_sha1(lock, orig_sha1, logmsg)
 	    || commit_ref(lock, orig_sha1)) {
@@ -2899,7 +2895,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 		goto rollbacklog;
 	}
 
-	lock->force_write = 1;
 	flag = log_all_ref_updates;
 	log_all_ref_updates = 0;
 	if (write_ref_sha1(lock, orig_sha1, NULL)
@@ -3062,12 +3057,22 @@ int is_branch(const char *refname)
 	return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/");
 }
 
+static int should_write_ref_sha1(struct ref_lock *lock,
+				 const unsigned char *new_sha1)
+{
+	/* If the old and new sha1 are equal only write if forced to. */
+	if (!lock->force_write && !hashcmp(lock->old_sha1, new_sha1))
+		return 0;
+	/* null sha indicates deletion, so don't write it */
+	return !is_null_sha1(new_sha1);
+}
+
 /*
  * Write sha1 into the ref specified by the lock. Make sure that errno
  * is sane on error.
  */
-static int write_ref_sha1(struct ref_lock *lock,
-	const unsigned char *sha1, const char *logmsg)
+static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
+			  const char *logmsg)
 {
 	static char term = '\n';
 	struct object *o;
@@ -3076,8 +3081,6 @@ static int write_ref_sha1(struct ref_lock *lock,
 		errno = EINVAL;
 		return -1;
 	}
-	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
-		return 0;
 
 	o = parse_object(sha1);
 	if (!o) {
@@ -3752,7 +3755,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				    update->refname);
 			goto cleanup;
 		}
-		if (!is_null_sha1(update->new_sha1)) {
+		if (should_write_ref_sha1(update->lock, update->new_sha1)) {
 			if (write_ref_sha1(update->lock, update->new_sha1,
 					   update->msg)) {
 				strbuf_addf(err, "Cannot write to the ref lock '%s'.",
@@ -3769,7 +3772,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
-		if (!is_null_sha1(update->new_sha1)) {
+		if (should_write_ref_sha1(update->lock, update->new_sha1)) {
 			if (commit_ref(update->lock, update->new_sha1)) {
 				strbuf_addf(err, "Cannot update the ref '%s'.",
 					    update->refname);
@@ -3785,7 +3788,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
-		if (update->lock) {
+		if (is_null_sha1(update->new_sha1)) {
 			if (delete_ref_loose(update->lock, update->type, err)) {
 				ret = TRANSACTION_GENERIC_ERROR;
 				goto cleanup;
-- 
2.2.1.62.g3f15098

Re: [PATCHv3 6/6] refs.c: enable large transactions

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> I do not see the problem in the code itself, but rather in understanding
> the code. I will send a follow up patch which makes it easier to follow
> by removing the early exit with no problem away.


Taken as a whole the code may function correctly but the division of
roles of individual functions seems screwed up.  write_ref_sha1()
sometimes unlocks, and sometimes leaves the unlocking to the caller,
and the caller cannot even tell if it is expected to do the unlocking
for it from the return value because both cases return 0 (success).

I am not sure if it is sensible to call that "correct but hard to
understand".  I'd rather see us admit that its behaviour is screwey
and needs fixing for better code health longer term.

Re: [PATCH] refs.c: clean up write_ref_sha1 returns

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>     I can redo the atomic-push-fix series with this cleanup merged
>     into the appropriate patches or you could just queue it on top 
>     of said series.

Yeah, I do not think we are expecting to fast track these two series
through 'next' to 'master' before 2.3 final, so I think it would be
better to use this patch _only_ to see if the final shape of the
code this patch represents makes sense, so that we can expedite the
final submission in the next development cycle, at which time we
will have a chance to refresh 'next', hence a chance to clean-up
atomic-push series in place.

Thanks.

>  refs.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 2594b23..c8fd4a4 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2817,9 +2817,6 @@ static int close_ref(struct ref_lock *lock)
>  
>  static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
>  {
> -	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
> -		return 0;
> -
>  	if (commit_lock_file(lock->lk))
>  		return -1;
>  	return 0;
> @@ -2880,7 +2877,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>  		error("unable to lock %s for update", newrefname);
>  		goto rollback;
>  	}
> -	lock->force_write = 1;
>  	hashcpy(lock->old_sha1, orig_sha1);

Is this hashcpy() still necessary?

>  	if (write_ref_sha1(lock, orig_sha1, logmsg)
>  	    || commit_ref(lock, orig_sha1)) {

Re: [PATCH] refs.c: clean up write_ref_sha1 returns

[Stefan Beller]

On Mon, Jan 26, 2015 at 7:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>     I can redo the atomic-push-fix series with this cleanup merged
>>     into the appropriate patches or you could just queue it on top
>>     of said series.
>
> Yeah, I do not think we are expecting to fast track these two series
> through 'next' to 'master' before 2.3 final, so I think it would be
> better to use this patch _only_ to see if the final shape of the
> code this patch represents makes sense, so that we can expedite the
> final submission in the next development cycle, at which time we
> will have a chance to refresh 'next', hence a chance to clean-up
> atomic-push series in place.

I tried to rip this patch and its 3 previous patches apart to see if it could be
done another way. The outcome was to actually sqquash this patch
completely into b1c6da0a13 (refs.c: remove unlock_ref and commit_ref
from write_ref_sha1).

Looking at the end result the write_ref_sha1 function it has a good
design contract. Either you get 0 returned and all is good, or -1 is returned
and errno is set to a meaningful value which seems to adress your concerns
on that patch:

> I am not sure if it is sensible to call that "correct but hard to
> understand".  I'd rather see us admit that its behaviour is screwey
> and needs fixing for better code health longer term.


>> @@ -2880,7 +2877,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>>               error("unable to lock %s for update", newrefname);
>>               goto rollback;
>>       }
>> -     lock->force_write = 1;
>>       hashcpy(lock->old_sha1, orig_sha1);
>
> Is this hashcpy() still necessary?

Thanks for catching that! It is not necessary any more and will be
removed in a reroll.
I think I'll wait for rerolling the atomic-push-fix series until 2.3
is out then?