Re: [PATCH] refs.c: enable large transactions

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 21, 2015, 17:16 UTC
Message-ID: <xmqqegndieqd.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1429576506-10790-1-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 6 quoted lines

> +	/*
> +	 * We may want to open many files in a large transaction, so come up with
> +	 * a reasonable maximum, keep some spares for stdin/out and other open
> +	 * files.
> +	 */
> +	int remaining_fds = get_max_fd_limit() - 32;

Can this go negative? If it does so, does it matter? I think the code doesn't barf, but starting from a negative "remaining" feels cryptic, compared to starting from a zero "remaining".

Show 12 quoted lines

>  	struct ref_update **updates = transaction->updates;
>  	struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
>  	struct string_list_item *ref_to_delete;
> @@ -3762,6 +3770,11 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  				    update->refname);
>  			goto cleanup;
>  		}
> +		if (remaining_fds > 0) {
> +			remaining_fds--;
> +		} else {
> +			close_lock_file(update->lock->lk);
> +		}

Any plan to add more code to these blocks in future updates?

Thanks.

Show 22 quoted lines

> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 7a69f1a..636d3a1 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -1071,7 +1071,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
> @@ -1082,7 +1082,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