Skip to content

Commit

Permalink
Merge branch 'mh/write-refs-sooner-2.4' into maint
Browse files Browse the repository at this point in the history 
Multi-ref transaction support we merged a few releases ago
unnecessarily kept many file descriptors open, risking to fail with
resource exhaustion.  This is for 2.4.x track.

* mh/write-refs-sooner-2.4:
  ref_transaction_commit(): fix atomicity and avoid fd exhaustion
  ref_transaction_commit(): remove the local flags variable
  ref_transaction_commit(): inline call to write_ref_sha1()
  rename_ref(): inline calls to write_ref_sha1() from this function
  commit_ref_update(): new function, extracted from write_ref_sha1()
  write_ref_to_lockfile(): new function, extracted from write_ref_sha1()
  t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE
  update-ref: test handling large transactions properly
  ref_transaction_commit(): fix atomicity and avoid fd exhaustion
  ref_transaction_commit(): remove the local flags variable
  ref_transaction_commit(): inline call to write_ref_sha1()
  rename_ref(): inline calls to write_ref_sha1() from this function
  commit_ref_update(): new function, extracted from write_ref_sha1()
  write_ref_to_lockfile(): new function, extracted from write_ref_sha1()
  t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE
  update-ref: test handling large transactions properly
  • Loading branch information
Junio C Hamano committed Jun 5, 2015
2 parents 4ba8846 + 185ce3a commit 7c997bc
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 33 deletions.
113 changes: 82 additions & 31 deletions refs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ static unsigned char refname_disposition[256] = {
*/
#define REF_HAVE_OLD 0x10

/*
* Used as a flag in ref_update::flags when the lockfile needs to be
* committed.
*/
#define REF_NEEDS_COMMIT 0x20

/*
* Try to read one refname component from the front of refname.
* Return the length of the component found, or -1 if the component is
Expand Down Expand Up @@ -2851,8 +2857,9 @@ static int rename_ref_available(const char *oldname, const char *newname)
return ret;
}

static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
const char *logmsg);
static int write_ref_to_lockfile(struct ref_lock *lock, const unsigned char *sha1);
static int commit_ref_update(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg);

int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)
{
Expand Down Expand Up @@ -2912,7 +2919,9 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
goto rollback;
}
hashcpy(lock->old_sha1, orig_sha1);
if (write_ref_sha1(lock, orig_sha1, logmsg)) {

if (write_ref_to_lockfile(lock, orig_sha1) ||
commit_ref_update(lock, orig_sha1, logmsg)) {
error("unable to write current sha1 into %s", newrefname);
goto rollback;
}
Expand All @@ -2929,7 +2938,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms

flag = log_all_ref_updates;
log_all_ref_updates = 0;
if (write_ref_sha1(lock, orig_sha1, NULL))
if (write_ref_to_lockfile(lock, orig_sha1) ||
commit_ref_update(lock, orig_sha1, NULL))
error("unable to write current sha1 into %s", oldrefname);
log_all_ref_updates = flag;

Expand Down Expand Up @@ -3103,11 +3113,11 @@ int is_branch(const char *refname)
}

/*
* Write sha1 into the ref specified by the lock. Make sure that errno
* is sane on error.
* Write sha1 into the open lockfile, then close the lockfile. On
* errors, rollback the lockfile and set errno to reflect the problem.
*/
static int write_ref_sha1(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg)
static int write_ref_to_lockfile(struct ref_lock *lock,
const unsigned char *sha1)
{
static char term = '\n';
struct object *o;
Expand Down Expand Up @@ -3136,6 +3146,17 @@ static int write_ref_sha1(struct ref_lock *lock,
errno = save_errno;
return -1;
}
return 0;
}

/*
* Commit a change to a loose reference that has already been written
* to the loose reference lockfile. Also update the reflogs if
* necessary, using the specified lockmsg (which can be NULL).
*/
static int commit_ref_update(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg)
{
clear_loose_ref_cache(&ref_cache);
if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||
(strcmp(lock->ref_name, lock->orig_ref_name) &&
Expand Down Expand Up @@ -3822,19 +3843,24 @@ int ref_transaction_commit(struct ref_transaction *transaction,
goto cleanup;
}

/* Acquire all locks while verifying old values */
/*
* Acquire all locks, verify old values if provided, check
* that new values are valid, and write new values to the
* lockfiles, ready to be activated. Only keep one lockfile
* open at a time to avoid running out of file descriptors.
*/
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
unsigned int flags = update->flags;

if ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1))
flags |= REF_DELETING;
if ((update->flags & REF_HAVE_NEW) &&
is_null_sha1(update->new_sha1))
update->flags |= REF_DELETING;
update->lock = lock_ref_sha1_basic(
update->refname,
((update->flags & REF_HAVE_OLD) ?
update->old_sha1 : NULL),
&affected_refnames, NULL,
flags,
update->flags,
&update->type,
err);
if (!update->lock) {
Expand All @@ -3849,34 +3875,60 @@ int ref_transaction_commit(struct ref_transaction *transaction,
free(reason);
goto cleanup;
}
}

/* Perform updates first so live commits remain referenced */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
int flags = update->flags;

if ((flags & REF_HAVE_NEW) && !is_null_sha1(update->new_sha1)) {
if ((update->flags & REF_HAVE_NEW) &&
!(update->flags & REF_DELETING)) {
int overwriting_symref = ((update->type & REF_ISSYMREF) &&
(update->flags & REF_NODEREF));

if (!overwriting_symref
&& !hashcmp(update->lock->old_sha1, update->new_sha1)) {
if (!overwriting_symref &&
!hashcmp(update->lock->old_sha1, update->new_sha1)) {
/*
* The reference already has the desired
* value, so we don't need to write it.
*/
unlock_ref(update->lock);
} else if (write_ref_to_lockfile(update->lock,
update->new_sha1)) {
/*
* The lock was freed upon failure of
* write_ref_to_lockfile():
*/
update->lock = NULL;
strbuf_addf(err, "Cannot update the ref '%s'.",
update->refname);
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
} else {
update->flags |= REF_NEEDS_COMMIT;
}
}
if (!(update->flags & REF_NEEDS_COMMIT)) {
/*
* We didn't have to write anything to the lockfile.
* Close it to free up the file descriptor:
*/
if (close_ref(update->lock)) {
strbuf_addf(err, "Couldn't close %s.lock",
update->refname);
goto cleanup;
}
}
}

/* Perform updates first so live commits remain referenced */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];

if (update->flags & REF_NEEDS_COMMIT) {
if (commit_ref_update(update->lock,
update->new_sha1, update->msg)) {
/* freed by commit_ref_update(): */
update->lock = NULL;
} else if (write_ref_sha1(update->lock, update->new_sha1,
update->msg)) {
update->lock = NULL; /* freed by write_ref_sha1 */
strbuf_addf(err, "Cannot update the ref '%s'.",
update->refname);
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
} else {
/* freed by write_ref_sha1(): */
/* freed by commit_ref_update(): */
update->lock = NULL;
}
}
Expand All @@ -3885,15 +3937,14 @@ int ref_transaction_commit(struct ref_transaction *transaction,
/* Perform deletes now that updates are safely completed */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
int flags = update->flags;

if ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1)) {
if (update->flags & REF_DELETING) {
if (delete_ref_loose(update->lock, update->type, err)) {
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}

if (!(flags & REF_ISPRUNING))
if (!(update->flags & REF_ISPRUNING))
string_list_append(&refs_to_delete,
update->lock->ref_name);
}
Expand Down
28 changes: 28 additions & 0 deletions t/t1400-update-ref.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1065,4 +1065,32 @@ test_expect_success 'stdin -z delete refs works with packed and loose refs' '
test_must_fail git rev-parse --verify -q $c
'

run_with_limited_open_files () {
(ulimit -n 32 && "$@")
}

test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'

test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
(
for i in $(test_seq 33)
do
echo "create refs/heads/$i HEAD"
done >large_input &&
run_with_limited_open_files git update-ref --stdin <large_input &&
git rev-parse --verify -q refs/heads/33
)
'

test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
(
for i in $(test_seq 33)
do
echo "delete refs/heads/$i HEAD"
done >large_input &&
run_with_limited_open_files git update-ref --stdin <large_input &&
test_must_fail git rev-parse --verify -q refs/heads/33
)
'

test_done
4 changes: 2 additions & 2 deletions t/t7004-tag.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1491,10 +1491,10 @@ run_with_limited_stack () {
(ulimit -s 128 && "$@")
}

test_lazy_prereq ULIMIT 'run_with_limited_stack true'
test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'

# we require ulimit, this excludes Windows
test_expect_success ULIMIT '--contains works in a deep repo' '
test_expect_success ULIMIT_STACK_SIZE '--contains works in a deep repo' '
>expect &&
i=1 &&
while test $i -lt 8000
Expand Down

0 comments on commit 7c997bc

Please sign in to comment.