On Tue, Apr 21, 2015 at 10:16 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Stefan Beller <sbel...@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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
