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

On Tue, Apr 21, 2015 at 10:16 AM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

+     /*
+      * 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".

quoted

      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.

quoted

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