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

[Ramsay Jones <ramsay@ramsay1.demon.co.uk>]

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