From: Junio C Hamano Date: Fri, 22 May 2015 19:41:53 +0000 (-0700) Subject: Merge branch 'mh/ref-directory-file' X-Git-Tag: v2.5.0-rc0~89 X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/faa4b2ecbb6ea5fdc8d685343c4985dfb97f4dbf?hp=-c Merge branch 'mh/ref-directory-file' The ref API did not handle cases where 'refs/heads/xyzzy/frotz' is removed at the same time as 'refs/heads/xyzzy' is added (or vice versa) very well. * mh/ref-directory-file: reflog_expire(): integrate lock_ref_sha1_basic() errors into ours ref_transaction_commit(): delete extra "the" from error message ref_transaction_commit(): provide better error messages rename_ref(): integrate lock_ref_sha1_basic() errors into ours lock_ref_sha1_basic(): improve diagnostics for ref D/F conflicts lock_ref_sha1_basic(): report errors via a "struct strbuf *err" verify_refname_available(): report errors via a "struct strbuf *err" verify_refname_available(): rename function refs: check for D/F conflicts among refs created in a transaction ref_transaction_commit(): use a string_list for detecting duplicates is_refname_available(): use dirname in first loop struct nonmatching_ref_data: store a refname instead of a ref_entry report_refname_conflict(): inline function entry_matches(): inline function is_refname_available(): convert local variable "dirname" to strbuf is_refname_available(): avoid shadowing "dir" variable is_refname_available(): revamp the comments t1404: new tests of ref D/F conflicts within transactions --- faa4b2ecbb6ea5fdc8d685343c4985dfb97f4dbf diff --combined refs.c index 825a1f6847,97043fd2ef..f704ee285c --- a/refs.c +++ b/refs.c @@@ -11,6 -11,7 +11,6 @@@ struct ref_lock char *orig_ref_name; struct lock_file *lk; unsigned char old_sha1[20]; - int lock_fd; }; /* @@@ -56,12 -57,6 +56,12 @@@ static unsigned char refname_dispositio */ #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 @@@ -268,7 -263,7 +268,7 @@@ struct ref_dir * presence of an empty subdirectory does not block the creation of a * similarly-named reference. (The fact that reference names with the * same leading components can conflict *with each other* is a - * separate issue that is regulated by is_refname_available().) + * separate issue that is regulated by verify_refname_available().) * * Please note that the name field contains the fully-qualified * reference (or subdirectory) name. Space could be saved by only @@@ -349,6 -344,8 +349,6 @@@ static struct ref_entry *create_ref_ent if (check_name && check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) die("Reference has invalid format: '%s'", refname); - if (!check_name && !refname_is_safe(refname)) - die("Reference has invalid name: '%s'", refname); len = strlen(refname) + 1; ref = xmalloc(sizeof(struct ref_entry) + len); hashcpy(ref->u.value.sha1, sha1); @@@ -844,121 -841,181 +844,181 @@@ static void prime_ref_dir(struct ref_di } } - static int entry_matches(struct ref_entry *entry, const struct string_list *list) - { - return list && string_list_has_string(list, entry->name); - } - struct nonmatching_ref_data { const struct string_list *skip; - struct ref_entry *found; + const char *conflicting_refname; }; static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata) { struct nonmatching_ref_data *data = vdata; - if (entry_matches(entry, data->skip)) + if (data->skip && string_list_has_string(data->skip, entry->name)) return 0; - data->found = entry; + data->conflicting_refname = entry->name; return 1; } - static void report_refname_conflict(struct ref_entry *entry, - const char *refname) - { - error("'%s' exists; cannot create '%s'", entry->name, refname); - } - /* - * Return true iff a reference named refname could be created without - * conflicting with the name of an existing reference in dir. If - * skip is non-NULL, ignore potential conflicts with refs in skip - * (e.g., because they are scheduled for deletion in the same - * operation). + * Return 0 if a reference named refname could be created without + * conflicting with the name of an existing reference in dir. + * Otherwise, return a negative value and write an explanation to err. + * If extras is non-NULL, it is a list of additional refnames with + * which refname is not allowed to conflict. If skip is non-NULL, + * ignore potential conflicts with refs in skip (e.g., because they + * are scheduled for deletion in the same operation). Behavior is + * undefined if the same name is listed in both extras and skip. * * Two reference names conflict if one of them exactly matches the - * leading components of the other; e.g., "foo/bar" conflicts with - * both "foo" and with "foo/bar/baz" but not with "foo/bar" or - * "foo/barbados". + * leading components of the other; e.g., "refs/foo/bar" conflicts + * with both "refs/foo" and with "refs/foo/bar/baz" but not with + * "refs/foo/bar" or "refs/foo/barbados". * - * skip must be sorted. + * extras and skip must be sorted. */ - static int is_refname_available(const char *refname, - const struct string_list *skip, - struct ref_dir *dir) + static int verify_refname_available(const char *refname, + const struct string_list *extras, + const struct string_list *skip, + struct ref_dir *dir, + struct strbuf *err) { const char *slash; - size_t len; int pos; - char *dirname; + struct strbuf dirname = STRBUF_INIT; + int ret = -1; + + /* + * For the sake of comments in this function, suppose that + * refname is "refs/foo/bar". + */ + assert(err); + + strbuf_grow(&dirname, strlen(refname) + 1); for (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) { + /* Expand dirname to the new prefix, not including the trailing slash: */ + strbuf_add(&dirname, refname + dirname.len, slash - refname - dirname.len); + /* - * We are still at a leading dir of the refname; we are - * looking for a conflict with a leaf entry. - * - * If we find one, we still must make sure it is - * not in "skip". + * We are still at a leading dir of the refname (e.g., + * "refs/foo"; if there is a reference with that name, + * it is a conflict, *unless* it is in skip. */ - pos = search_ref_dir(dir, refname, slash - refname); - if (pos >= 0) { - struct ref_entry *entry = dir->entries[pos]; - if (entry_matches(entry, skip)) - return 1; - report_refname_conflict(entry, refname); - return 0; + if (dir) { + pos = search_ref_dir(dir, dirname.buf, dirname.len); + if (pos >= 0 && + (!skip || !string_list_has_string(skip, dirname.buf))) { + /* + * We found a reference whose name is + * a proper prefix of refname; e.g., + * "refs/foo", and is not in skip. + */ + strbuf_addf(err, "'%s' exists; cannot create '%s'", + dirname.buf, refname); + goto cleanup; + } } + if (extras && string_list_has_string(extras, dirname.buf) && + (!skip || !string_list_has_string(skip, dirname.buf))) { + strbuf_addf(err, "cannot process '%s' and '%s' at the same time", + refname, dirname.buf); + goto cleanup; + } /* * Otherwise, we can try to continue our search with - * the next component; if we come up empty, we know - * there is nothing under this whole prefix. + * the next component. So try to look up the + * directory, e.g., "refs/foo/". If we come up empty, + * we know there is nothing under this whole prefix, + * but even in that case we still have to continue the + * search for conflicts with extras. */ - pos = search_ref_dir(dir, refname, slash + 1 - refname); - if (pos < 0) - return 1; - - dir = get_ref_dir(dir->entries[pos]); + strbuf_addch(&dirname, '/'); + if (dir) { + pos = search_ref_dir(dir, dirname.buf, dirname.len); + if (pos < 0) { + /* + * There was no directory "refs/foo/", + * so there is nothing under this + * whole prefix. So there is no need + * to continue looking for conflicting + * references. But we need to continue + * looking for conflicting extras. + */ + dir = NULL; + } else { + dir = get_ref_dir(dir->entries[pos]); + } + } } /* - * We are at the leaf of our refname; we want to - * make sure there are no directories which match it. + * We are at the leaf of our refname (e.g., "refs/foo/bar"). + * There is no point in searching for a reference with that + * name, because a refname isn't considered to conflict with + * itself. But we still need to check for references whose + * names are in the "refs/foo/bar/" namespace, because they + * *do* conflict. */ - len = strlen(refname); - dirname = xmallocz(len + 1); - sprintf(dirname, "%s/", refname); - pos = search_ref_dir(dir, dirname, len + 1); - free(dirname); + strbuf_addstr(&dirname, refname + dirname.len); + strbuf_addch(&dirname, '/'); + + if (dir) { + pos = search_ref_dir(dir, dirname.buf, dirname.len); - if (pos >= 0) { + if (pos >= 0) { + /* + * We found a directory named "$refname/" + * (e.g., "refs/foo/bar/"). It is a problem + * iff it contains any ref that is not in + * "skip". + */ + struct nonmatching_ref_data data; + + data.skip = skip; + data.conflicting_refname = NULL; + dir = get_ref_dir(dir->entries[pos]); + sort_ref_dir(dir); + if (do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data)) { + strbuf_addf(err, "'%s' exists; cannot create '%s'", + data.conflicting_refname, refname); + goto cleanup; + } + } + } + + if (extras) { /* - * We found a directory named "refname". It is a - * problem iff it contains any ref that is not - * in "skip". + * Check for entries in extras that start with + * "$refname/". We do that by looking for the place + * where "$refname/" would be inserted in extras. If + * there is an entry at that position that starts with + * "$refname/" and is not in skip, then we have a + * conflict. */ - struct ref_entry *entry = dir->entries[pos]; - struct ref_dir *dir = get_ref_dir(entry); - struct nonmatching_ref_data data; + for (pos = string_list_find_insert_index(extras, dirname.buf, 0); + pos < extras->nr; pos++) { + const char *extra_refname = extras->items[pos].string; - data.skip = skip; - sort_ref_dir(dir); - if (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data)) - return 1; + if (!starts_with(extra_refname, dirname.buf)) + break; - report_refname_conflict(data.found, refname); - return 0; + if (!skip || !string_list_has_string(skip, extra_refname)) { + strbuf_addf(err, "cannot process '%s' and '%s' at the same time", + refname, extra_refname); + goto cleanup; + } + } } - /* - * There is no point in searching for another leaf - * node which matches it; such an entry would be the - * ref we are looking for, not a conflict. - */ - return 1; + /* No conflicts were found */ + ret = 0; + + cleanup: + strbuf_release(&dirname); + return ret; } struct packed_ref_cache { @@@ -1181,8 -1238,6 +1241,8 @@@ static void read_packed_refs(FILE *f, s int flag = REF_ISPACKED; if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { + if (!refname_is_safe(refname)) + die("packed refname is dangerous: %s", refname); hashclr(sha1); flag |= REF_BAD_NAME | REF_ISBROKEN; } @@@ -1328,8 -1383,6 +1388,8 @@@ static void read_loose_refs(const char } if (check_refname_format(refname.buf, REFNAME_ALLOW_ONELEVEL)) { + if (!refname_is_safe(refname.buf)) + die("loose refname is dangerous: %s", refname.buf); hashclr(sha1); flag |= REF_BAD_NAME | REF_ISBROKEN; } @@@ -1389,7 -1442,7 +1449,7 @@@ static int resolve_gitlink_ref_recursiv { int fd, len; char buffer[128], *p; - char *path; + const char *path; if (recursion > MAXDEPTH || strlen(refname) > MAXREFLEN) return -1; @@@ -1482,11 -1535,7 +1542,11 @@@ static int resolve_missing_loose_ref(co } /* This function needs to return a meaningful errno on failure */ -const char *resolve_ref_unsafe(const char *refname, int resolve_flags, unsigned char *sha1, int *flags) +static const char *resolve_ref_unsafe_1(const char *refname, + int resolve_flags, + unsigned char *sha1, + int *flags, + struct strbuf *sb_path) { int depth = MAXDEPTH; ssize_t len; @@@ -1517,7 -1566,7 +1577,7 @@@ bad_name = 1; } for (;;) { - char path[PATH_MAX]; + const char *path; struct stat st; char *buf; int fd; @@@ -1527,9 -1576,7 +1587,9 @@@ return NULL; } - git_snpath(path, sizeof(path), "%s", refname); + strbuf_reset(sb_path); + strbuf_git_path(sb_path, "%s", refname); + path = sb_path->buf; /* * We might have to loop back here to avoid a race @@@ -1656,16 -1703,6 +1716,16 @@@ } } +const char *resolve_ref_unsafe(const char *refname, int resolve_flags, + unsigned char *sha1, int *flags) +{ + struct strbuf sb_path = STRBUF_INIT; + const char *ret = resolve_ref_unsafe_1(refname, resolve_flags, + sha1, flags, &sb_path); + strbuf_release(&sb_path); + return ret; +} + char *resolve_refdup(const char *ref, int resolve_flags, unsigned char *sha1, int *flags) { return xstrdup_or_null(resolve_ref_unsafe(ref, resolve_flags, sha1, flags)); @@@ -2294,10 -2331,12 +2354,12 @@@ int dwim_log(const char *str, int len, */ static struct ref_lock *lock_ref_sha1_basic(const char *refname, const unsigned char *old_sha1, + const struct string_list *extras, const struct string_list *skip, - unsigned int flags, int *type_p) + unsigned int flags, int *type_p, + struct strbuf *err) { - char *ref_file; + const char *ref_file; const char *orig_refname = refname; struct ref_lock *lock; int last_errno = 0; @@@ -2306,7 -2345,10 +2368,9 @@@ int resolve_flags = 0; int attempts_remaining = 3; + assert(err); + lock = xcalloc(1, sizeof(struct ref_lock)); - lock->lock_fd = -1; if (mustexist) resolve_flags |= RESOLVE_REF_READING; @@@ -2327,7 -2369,12 +2391,12 @@@ ref_file = git_path("%s", orig_refname); if (remove_empty_directories(ref_file)) { last_errno = errno; - error("there are still refs under '%s'", orig_refname); + + if (!verify_refname_available(orig_refname, extras, skip, + get_loose_refs(&ref_cache), err)) + strbuf_addf(err, "there are still refs under '%s'", + orig_refname); + goto error_return; } refname = resolve_ref_unsafe(orig_refname, resolve_flags, @@@ -2337,8 -2384,12 +2406,12 @@@ *type_p = type; if (!refname) { last_errno = errno; - error("unable to resolve reference %s: %s", - orig_refname, strerror(errno)); + if (last_errno != ENOTDIR || + !verify_refname_available(orig_refname, extras, skip, + get_loose_refs(&ref_cache), err)) + strbuf_addf(err, "unable to resolve reference %s: %s", + orig_refname, strerror(last_errno)); + goto error_return; } /* @@@ -2348,7 -2399,8 +2421,8 @@@ * our refname. */ if (is_null_sha1(lock->old_sha1) && - !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) { + verify_refname_available(refname, extras, skip, + get_packed_refs(&ref_cache), err)) { last_errno = ENOTDIR; goto error_return; } @@@ -2365,7 -2417,7 +2439,7 @@@ ref_file = git_path("%s", refname); retry: - switch (safe_create_leading_directories(ref_file)) { + switch (safe_create_leading_directories_const(ref_file)) { case SCLD_OK: break; /* success */ case SCLD_VANISHED: @@@ -2374,11 -2426,12 +2448,11 @@@ /* fall through */ default: last_errno = errno; - error("unable to create directory for %s", ref_file); + strbuf_addf(err, "unable to create directory for %s", ref_file); goto error_return; } - lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags); - if (lock->lock_fd < 0) { + if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) { last_errno = errno; if (errno == ENOENT && --attempts_remaining > 0) /* @@@ -2388,10 -2441,7 +2462,7 @@@ */ goto retry; else { - struct strbuf err = STRBUF_INIT; - unable_to_lock_message(ref_file, errno, &err); - error("%s", err.buf); - strbuf_release(&err); + unable_to_lock_message(ref_file, errno, err); goto error_return; } } @@@ -2742,7 -2792,7 +2813,7 @@@ static int rename_tmp_log(const char *n int attempts_remaining = 4; retry: - switch (safe_create_leading_directories(git_path("logs/%s", newrefname))) { + switch (safe_create_leading_directories_const(git_path("logs/%s", newrefname))) { case SCLD_OK: break; /* success */ case SCLD_VANISHED: @@@ -2785,18 -2835,24 +2856,25 @@@ static int rename_ref_available(const char *oldname, const char *newname) { struct string_list skip = STRING_LIST_INIT_NODUP; + struct strbuf err = STRBUF_INIT; int ret; string_list_insert(&skip, oldname); - ret = is_refname_available(newname, &skip, get_packed_refs(&ref_cache)) - && is_refname_available(newname, &skip, get_loose_refs(&ref_cache)); + ret = !verify_refname_available(newname, NULL, &skip, + get_packed_refs(&ref_cache), &err) + && !verify_refname_available(newname, NULL, &skip, + get_loose_refs(&ref_cache), &err); + if (!ret) + error("%s", err.buf); + string_list_clear(&skip, 0); + strbuf_release(&err); 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) { @@@ -2806,6 -2862,7 +2884,7 @@@ struct stat loginfo; int log = !lstat(git_path("logs/%s", oldrefname), &loginfo); const char *symref = NULL; + struct strbuf err = STRBUF_INIT; if (log && S_ISLNK(loginfo.st_mode)) return error("reflog for %s is a symlink", oldrefname); @@@ -2848,15 -2905,14 +2927,16 @@@ logmoved = log; - lock = lock_ref_sha1_basic(newrefname, NULL, NULL, 0, NULL); + lock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, 0, NULL, &err); if (!lock) { - error("unable to lock %s for update", newrefname); + error("unable to rename '%s' to '%s': %s", oldrefname, newrefname, err.buf); + strbuf_release(&err); 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; } @@@ -2864,16 -2920,16 +2944,17 @@@ return 0; rollback: - lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, 0, NULL); + lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, 0, NULL, &err); if (!lock) { - error("unable to lock %s for rollback", oldrefname); + error("unable to lock %s for rollback: %s", oldrefname, err.buf); + strbuf_release(&err); goto rollbacklog; } 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; @@@ -2893,6 -2949,7 +2974,6 @@@ static int close_ref(struct ref_lock *l { if (close_lock_file(lock->lk)) return -1; - lock->lock_fd = -1; return 0; } @@@ -2900,6 -2957,7 +2981,6 @@@ static int commit_ref(struct ref_lock * { if (commit_lock_file(lock->lk)) return -1; - lock->lock_fd = -1; return 0; } @@@ -2930,15 -2988,11 +3011,15 @@@ static int copy_msg(char *buf, const ch } /* This function must set a meaningful errno on failure */ -int log_ref_setup(const char *refname, char *logfile, int bufsize) +int log_ref_setup(const char *refname, struct strbuf *sb_logfile) { int logfd, oflags = O_APPEND | O_WRONLY; + char *logfile; - git_snpath(logfile, bufsize, "logs/%s", refname); + strbuf_git_path(sb_logfile, "logs/%s", refname); + logfile = sb_logfile->buf; + /* make sure the rest of the function can't change "logfile" */ + sb_logfile = NULL; if (log_all_ref_updates && (starts_with(refname, "refs/heads/") || starts_with(refname, "refs/remotes/") || @@@ -3009,22 -3063,18 +3090,22 @@@ static int log_ref_write_fd(int fd, con return 0; } -static int log_ref_write(const char *refname, const unsigned char *old_sha1, - const unsigned char *new_sha1, const char *msg) +static int log_ref_write_1(const char *refname, const unsigned char *old_sha1, + const unsigned char *new_sha1, const char *msg, + struct strbuf *sb_log_file) { int logfd, result, oflags = O_APPEND | O_WRONLY; - char log_file[PATH_MAX]; + char *log_file; if (log_all_ref_updates < 0) log_all_ref_updates = !is_bare_repository(); - result = log_ref_setup(refname, log_file, sizeof(log_file)); + result = log_ref_setup(refname, sb_log_file); if (result) return result; + log_file = sb_log_file->buf; + /* make sure the rest of the function can't change "log_file" */ + sb_log_file = NULL; logfd = open(log_file, oflags); if (logfd < 0) @@@ -3047,26 -3097,17 +3128,26 @@@ return 0; } +static int log_ref_write(const char *refname, const unsigned char *old_sha1, + const unsigned char *new_sha1, const char *msg) +{ + struct strbuf sb = STRBUF_INIT; + int ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb); + strbuf_release(&sb); + return ret; +} + int is_branch(const char *refname) { return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/"); } /* - * 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; @@@ -3086,8 -3127,8 +3167,8 @@@ errno = EINVAL; return -1; } - if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 || - write_in_full(lock->lock_fd, &term, 1) != 1 || + if (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 || + write_in_full(lock->lk->fd, &term, 1) != 1 || close_ref(lock) < 0) { int save_errno = errno; error("Couldn't write %s", lock->lk->filename.buf); @@@ -3095,17 -3136,6 +3176,17 @@@ 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) && @@@ -3746,25 -3776,18 +3827,18 @@@ int update_ref(const char *msg, const c return 0; } - static int ref_update_compare(const void *r1, const void *r2) - { - const struct ref_update * const *u1 = r1; - const struct ref_update * const *u2 = r2; - return strcmp((*u1)->refname, (*u2)->refname); - } - - static int ref_update_reject_duplicates(struct ref_update **updates, int n, + static int ref_update_reject_duplicates(struct string_list *refnames, struct strbuf *err) { - int i; + int i, n = refnames->nr; assert(err); for (i = 1; i < n; i++) - if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) { + if (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) { strbuf_addf(err, "Multiple updates for ref '%s' not allowed.", - updates[i]->refname); + refnames->items[i].string); return 1; } return 0; @@@ -3778,6 -3801,7 +3852,7 @@@ int ref_transaction_commit(struct ref_t struct ref_update **updates = transaction->updates; struct string_list refs_to_delete = STRING_LIST_INIT_NODUP; struct string_list_item *ref_to_delete; + struct string_list affected_refnames = STRING_LIST_INIT_NODUP; assert(err); @@@ -3789,94 -3813,70 +3864,101 @@@ return 0; } - /* Copy, sort, and reject duplicate refs */ - qsort(updates, n, sizeof(*updates), ref_update_compare); - if (ref_update_reject_duplicates(updates, n, err)) { + /* Fail if a refname appears more than once in the transaction: */ + for (i = 0; i < n; i++) + string_list_append(&affected_refnames, updates[i]->refname); + string_list_sort(&affected_refnames); + if (ref_update_reject_duplicates(&affected_refnames, err)) { ret = TRANSACTION_GENERIC_ERROR; 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), - NULL, + &affected_refnames, NULL, - flags, + update->flags, - &update->type); + &update->type, + err); if (!update->lock) { + char *reason; + ret = (errno == ENOTDIR) ? TRANSACTION_NAME_CONFLICT : TRANSACTION_GENERIC_ERROR; - strbuf_addf(err, "Cannot lock the ref '%s'.", - update->refname); + reason = strbuf_detach(err, NULL); + strbuf_addf(err, "Cannot lock ref '%s': %s", + update->refname, reason); + 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; } } @@@ -3885,14 -3885,15 +3967,14 @@@ /* 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); } @@@ -3913,6 -3914,7 +3995,7 @@@ cleanup if (updates[i]->lock) unlock_ref(updates[i]->lock); string_list_clear(&refs_to_delete, 0); + string_list_clear(&affected_refnames, 0); return ret; } @@@ -4102,6 -4104,7 +4185,7 @@@ int reflog_expire(const char *refname, char *log_file; int status = 0; int type; + struct strbuf err = STRBUF_INIT; memset(&cb, 0, sizeof(cb)); cb.flags = flags; @@@ -4113,9 -4116,12 +4197,12 @@@ * reference itself, plus we might need to update the * reference if --updateref was specified: */ - lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, &type); - if (!lock) - return error("cannot lock ref '%s'", refname); + lock = lock_ref_sha1_basic(refname, sha1, NULL, NULL, 0, &type, &err); + if (!lock) { + error("cannot lock ref '%s': %s", refname, err.buf); + strbuf_release(&err); + return -1; + } if (!reflog_exists(refname)) { unlock_ref(lock); return 0; @@@ -4165,9 -4171,9 +4252,9 @@@ status |= error("couldn't write %s: %s", log_file, strerror(errno)); } else if (update && - (write_in_full(lock->lock_fd, + (write_in_full(lock->lk->fd, sha1_to_hex(cb.last_kept_sha1), 40) != 40 || - write_str_in_full(lock->lock_fd, "\n") != 1 || + write_str_in_full(lock->lk->fd, "\n") != 1 || close_ref(lock) < 0)) { status |= error("couldn't write %s", lock->lk->filename.buf); diff --combined t/t1400-update-ref.sh index 636d3a167c,86fa46856c..ba89f4c009 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@@ -519,7 -519,7 +519,7 @@@ test_expect_success 'stdin create ref w test_expect_success 'stdin update ref fails with wrong old value' ' echo "update $c $m $m~1" >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err && + grep "fatal: Cannot lock ref '"'"'$c'"'"'" err && test_must_fail git rev-parse --verify -q $c ' @@@ -555,7 -555,7 +555,7 @@@ test_expect_success 'stdin update ref w test_expect_success 'stdin delete ref fails with wrong old value' ' echo "delete $a $m~1" >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: Cannot lock the ref '"'"'$a'"'"'" err && + grep "fatal: Cannot lock ref '"'"'$a'"'"'" err && git rev-parse $m >expect && git rev-parse $a >actual && test_cmp expect actual @@@ -688,7 -688,7 +688,7 @@@ test_expect_success 'stdin update refs update $c '' EOF test_must_fail git update-ref --stdin err && - grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err && + grep "fatal: Cannot lock ref '"'"'$c'"'"'" err && git rev-parse $m >expect && git rev-parse $a >actual && test_cmp expect actual && @@@ -883,7 -883,7 +883,7 @@@ test_expect_success 'stdin -z create re test_expect_success 'stdin -z update ref fails with wrong old value' ' printf $F "update $c" "$m" "$m~1" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err && + grep "fatal: Cannot lock ref '"'"'$c'"'"'" err && test_must_fail git rev-parse --verify -q $c ' @@@ -899,7 -899,7 +899,7 @@@ test_expect_success 'stdin -z create re git rev-parse "$c" >expect && printf $F "create $c" "$m~1" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err && + grep "fatal: Cannot lock ref '"'"'$c'"'"'" err && git rev-parse "$c" >actual && test_cmp expect actual ' @@@ -930,7 -930,7 +930,7 @@@ test_expect_success 'stdin -z update re test_expect_success 'stdin -z delete ref fails with wrong old value' ' printf $F "delete $a" "$m~1" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: Cannot lock the ref '"'"'$a'"'"'" err && + grep "fatal: Cannot lock ref '"'"'$a'"'"'" err && git rev-parse $m >expect && git rev-parse $a >actual && test_cmp expect actual @@@ -1045,7 -1045,7 +1045,7 @@@ test_expect_success 'stdin -z update re git update-ref $c $m && printf $F "update $a" "$m" "$m" "update $b" "$m" "$m" "update $c" "$m" "$Z" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err && + grep "fatal: Cannot lock ref '"'"'$c'"'"'" err && git rev-parse $m >expect && git rev-parse $a >actual && test_cmp expect actual && @@@ -1065,32 -1065,4 +1065,32 @@@ test_expect_success 'stdin -z delete re 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 && + run_with_limited_open_files git update-ref --stdin