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

[Stefan Beller <sbeller@google.com>]

On Tue, Apr 21, 2015 at 10:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> +     /*
>> +      * 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

Yes it can go negative. It doesn't matter as we'd then run into the
"close_lock_file(update->lock->lk);" case below.

I thought about putting a cap on it to not let it go negative in the first
place, but I did not find an easily accessible max() function, as I'd like
to write it as

    int remaining_fds = max(get_max_fd_limit() - 32, 0);

to have it in one line. The alternative of

    int remaining_fds = get_max_fd_limit() - 32;
    ...
    if (remaining_fds < 0)
        remaining_fds = 0

seemed to cuttered to me. Yet another alternative

     int remaining_fds = get_max_fd_limit() - 32 < 0 ? 0 :
get_max_fd_limit() - 32;

is also not appealing as we'd have to call get_max_fd_limit 2 times.
That's why I thought going negative is not as bad, but have readable
code instead.

As you think about the possibility of remaining_fds being negative,
this might not
be the best idea though


> code doesn't barf, but starting from a negative "remaining" feels
> cryptic, compared to starting from a zero "remaining".
>
>>       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?

I'll remove the braces. :(

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