On Thu, Apr 23, 2015 at 10:56 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Stefan Beller <sbel...@google.com> writes:
>
>> diff --git a/refs.c b/refs.c
>> index 4f495bd..7ce7b97 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3041,6 +3041,13 @@ static int write_ref_sha1(struct ref_lock *lock,
>> errno = EINVAL;
>> return -1;
>> }
>> + if (lock->lk->fd == -1 && reopen_lock_file(lock->lk) == -1) {
>
> Granted, we explicitly assign -1 to lk->fd when we know it is
> closed, and the return value from reopen_lock_file() can come only
> from the return value from open(2), which is defined to return -1
> (i.e. not just any negative value) upon failure, but still, isn't it
> customary to check with "< 0" rather than "== -1" for errors?
< 0 would be better here indeed.
>
>> + int save_errno = errno;
>> + error("Couldn't reopen %s", lock->lk->filename.buf);
>
> No need to change this line, but I noticed that we might want to do
> something about the first one of the following two:
I personally like to have each error(...) to have a unique string, such
that when you run into trouble (most likely reported by a user),
you can easily pinpoint where the exact error is.
Maybe we could think about overriding error to actually report
"version, filename, linenumber, actual error message"
>
> $ git grep -e '[ ]error(_*"[A-Z]' | wc -l
> 146
> $ git grep -e '[ ]error(_*"[a-z]' | wc -l
> 390
>
>> + unlock_ref(lock);
>> + errno = save_errno;
>> + return -1;
>> + }
>
> Is this the only place in the entire codebase where a lockfile,
> which may have been closed to save number of open file descriptors,
> needs to be reopened? If I understand correctly, lockfile API is
> not for sole use of refs (don't the index and the pack codepaths use
> it, too?), so it may give us a better abstraction to have a helper
> function in lockfile.[ch] that takes a lock object, i.e.
>
> int make_lock_fd_valid(struct lock_file *lk)
> {
> if (lk->fd < 0 && reopen_lock_file(lk) < 0) {
> ... error ...
> return -1;
> }
> return 0;
> }
>
> and to call it at this point, i.e.
>
> if (make_lock_fd_valid(lock->lk) < 0)
> return -1;
>
> perhaps?
This is what I was originally looking for, thanks for the design guidance!
>
>> @@ -3733,6 +3741,20 @@ int ref_transaction_commit(struct ref_transaction
>> *transaction,
>> return 0;
>> }
>>
>> + /*
>> + * We need to open many files in a large transaction, so come up with
>> + * a reasonable maximum. We still keep some spares for stdin/out and
>> + * other open files. Experiments determined we need more fds when
>> + * running inside our test suite than directly in the shell. It's
>> + * unclear where these fds come from. 25 should be a reasonable large
>> + * number though.
>> + */
>
>> + remaining_fds = get_max_fd_limit();
>> + if (remaining_fds > 25)
>> + remaining_fds -= 25;
>> + else
>> + remaining_fds = 0;
>
> Is the value of pack_open_fds visible from here? I am wondering if
> we might want "scratch fds Git can use for its own use" accounting
> so that the two subsystems can collectively say "it is still safe
> to use one more fd and leave 25 for other people". With the code
> structure proposed here, the pack reader can grab all but 25 fds,
> which would leave the rest of Git including this code only 25,
> and the value in remaining_fds would become totally meaningless.
>
> It certainly can wait to worry about that and we do not have to do
> anything about it in this patch, but it may still be a good idea to
> leave "NEEDSWORK:" comment here (and point at open_packed_git_1() in
> sha1_file.c in the comment).
>
> I do not think the other side needs to know about the fd consumption
> in this function, as what is opened in here will be closed before
> this function returns.
>
>> @@ -3762,6 +3784,12 @@ int ref_transaction_commit(struct ref_transaction
>> *transaction,
>> update->refname);
>> goto cleanup;
>> }
>> + if (!(flags & REF_HAVE_NEW) ||
>> + is_null_sha1(update->new_sha1) ||
>> + remaining_fds == 0)
>> + close_lock_file(update->lock->lk);
>> + else
>> + remaining_fds--;
>> }
--
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
