refs: check for D/F conflicts among refs created in a transaction

If two references that D/F conflict (e.g., "refs/foo" and "refs/foo/bar") are created in a single transaction, the old code discovered the problem only after the "commit" phase of ref_transaction_commit() had already begun. This could leave some references updated and others not, which violates the promise of atomicity. Instead, check for such conflicts during the "locking" phase: * Teach is_refname_available() to take an "extras" parameter that can contain extra reference names with which the specified refname must not conflict. * Change lock_ref_sha1_basic() to take an "extras" parameter, which it passes through to is_refname_available(). * Change ref_transaction_commit() to pass "affected_refnames" to lock_ref_sha1_basic() as its "extras" argument. This change fixes a test case in t1404. This code is a bit stricter than it needs to be. We could conceivably allow reference "refs/foo/bar" to be created in the same transaction as "refs/foo" is deleted (or vice versa). But that would be complicated to implement, because it is not possible to lock "refs/foo/bar" while "refs/foo" exists as a loose reference, but on the other hand we don't want to delete some references before adding others (because that could leave a gap during which required objects are unreachable). There is also a complication that reflog files' paths can conflict. Any less-strict implementation would probably require tricks like the packing of all references before the start of the real transaction, or the use of temporary intermediate reference names. So for now let's accept too-strict checks. Some reference update transactions will be rejected unnecessarily, but they will be rejected in their entirety rather than leaving the repository in an intermediate state, as would happen now. Please note that there is still one kind of D/F conflict that is *not* handled correctly. If two processes are running at the same time, and one tries to create "refs/foo" at the same time that the other tries to create "refs/foo/bar", then they can race with each other. Both processes can obtain their respective locks ("refs/foo.lock" and "refs/foo/bar.lock"), proceed to the "commit" phase of ref_transaction_commit(), and then the slower process will discover that it cannot rename its lockfile into place (after possibly having committed changes to other references). There appears to be no way to fix this race without changing the locking policy, which in turn would require a change to *all* Git clients. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

Michael Haggerty committed May 11, 2015 at 17:25 UTC e911104c84f2efd64aded6831bd7e6625c8a0fab
2 files changed +95 -63
refs.c
+94 -62
@@ -859,19 +859,22 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
859
860 /*
861 * Return true iff a reference named refname could be created without
862 - * conflicting with the name of an existing reference in dir. If
863 - * skip is non-NULL, ignore potential conflicts with refs in skip
864 - * (e.g., because they are scheduled for deletion in the same
865 - * operation).
862 + * conflicting with the name of an existing reference in dir. If
863 + * extras is non-NULL, it is a list of additional refnames with which
864 + * refname is not allowed to conflict. If skip is non-NULL, ignore
865 + * potential conflicts with refs in skip (e.g., because they are
866 + * scheduled for deletion in the same operation). Behavior is
867 + * undefined if the same name is listed in both extras and skip.
868 *
869 * Two reference names conflict if one of them exactly matches the
870 * leading components of the other; e.g., "refs/foo/bar" conflicts
871 * with both "refs/foo" and with "refs/foo/bar/baz" but not with
872 * "refs/foo/bar" or "refs/foo/barbados".
873 *
872 - * skip must be sorted.
874 + * extras and skip must be sorted.
875 */
876 static int is_refname_available(const char *refname,
877 + const struct string_list *extras,
878 const struct string_list *skip,
879 struct ref_dir *dir)
880 {
@@ -895,51 +898,53 @@ static int is_refname_available(const char *refname,
898 * "refs/foo"; if there is a reference with that name,
899 * it is a conflict, *unless* it is in skip.
900 */
898 - pos = search_ref_dir(dir, dirname.buf, dirname.len);
899 - if (pos >= 0) {
900 - /*
901 - * We found a reference whose name is a proper
902 - * prefix of refname; e.g., "refs/foo".
903 - */
904 - if (skip && string_list_has_string(skip, dirname.buf)) {
901 + if (dir) {
902 + pos = search_ref_dir(dir, dirname.buf, dirname.len);
903 + if (pos >= 0 &&
904 + (!skip || !string_list_has_string(skip, dirname.buf))) {
905 /*
906 - * The reference we just found, e.g.,
907 - * "refs/foo", is also in skip, so it
908 - * is not considered a conflict.
909 - * Moreover, the fact that "refs/foo"
910 - * exists means that there cannot be
911 - * any references anywhere under the
912 - * "refs/foo/" namespace (because they
913 - * would have conflicted with
914 - * "refs/foo"). So we can stop looking
915 - * now and return true.
906 + * We found a reference whose name is
907 + * a proper prefix of refname; e.g.,
908 + * "refs/foo", and is not in skip.
909 */
917 - ret = 1;
910 + error("'%s' exists; cannot create '%s'",
911 + dirname.buf, refname);
912 goto cleanup;
913 }
920 - error("'%s' exists; cannot create '%s'", dirname.buf, refname);
921 - goto cleanup;
914 }
915
916 + if (extras && string_list_has_string(extras, dirname.buf) &&
917 + (!skip || !string_list_has_string(skip, dirname.buf))) {
918 + error("cannot process '%s' and '%s' at the same time",
919 + refname, dirname.buf);
920 + goto cleanup;
921 + }
922
923 /*
924 * Otherwise, we can try to continue our search with
925 * the next component. So try to look up the
928 - * directory, e.g., "refs/foo/".
926 + * directory, e.g., "refs/foo/". If we come up empty,
927 + * we know there is nothing under this whole prefix,
928 + * but even in that case we still have to continue the
929 + * search for conflicts with extras.
930 */
931 strbuf_addch(&dirname, '/');
931 - pos = search_ref_dir(dir, dirname.buf, dirname.len);
932 - if (pos < 0) {
933 - /*
934 - * There was no directory "refs/foo/", so
935 - * there is nothing under this whole prefix,
936 - * and we are OK.
937 - */
938 - ret = 1;
939 - goto cleanup;
932 + if (dir) {
933 + pos = search_ref_dir(dir, dirname.buf, dirname.len);
934 + if (pos < 0) {
935 + /*
936 + * There was no directory "refs/foo/",
937 + * so there is nothing under this
938 + * whole prefix. So there is no need
939 + * to continue looking for conflicting
940 + * references. But we need to continue
941 + * looking for conflicting extras.
942 + */
943 + dir = NULL;
944 + } else {
945 + dir = get_ref_dir(dir->entries[pos]);
946 + }
947 }
941 -
942 - dir = get_ref_dir(dir->entries[pos]);
948 }
949
950 /*
@@ -952,30 +957,56 @@ static int is_refname_available(const char *refname,
957 */
958 strbuf_addstr(&dirname, refname + dirname.len);
959 strbuf_addch(&dirname, '/');
955 - pos = search_ref_dir(dir, dirname.buf, dirname.len);
960
957 - if (pos >= 0) {
961 + if (dir) {
962 + pos = search_ref_dir(dir, dirname.buf, dirname.len);
963 +
964 + if (pos >= 0) {
965 + /*
966 + * We found a directory named "$refname/"
967 + * (e.g., "refs/foo/bar/"). It is a problem
968 + * iff it contains any ref that is not in
969 + * "skip".
970 + */
971 + struct nonmatching_ref_data data;
972 +
973 + data.skip = skip;
974 + data.conflicting_refname = NULL;
975 + dir = get_ref_dir(dir->entries[pos]);
976 + sort_ref_dir(dir);
977 + if (do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data)) {
978 + error("'%s' exists; cannot create '%s'",
979 + data.conflicting_refname, refname);
980 + goto cleanup;
981 + }
982 + }
983 + }
984 +
985 + if (extras) {
986 /*
959 - * We found a directory named "$refname/" (e.g.,
960 - * "refs/foo/bar/"). It is a problem iff it contains
961 - * any ref that is not in "skip".
987 + * Check for entries in extras that start with
988 + * "$refname/". We do that by looking for the place
989 + * where "$refname/" would be inserted in extras. If
990 + * there is an entry at that position that starts with
991 + * "$refname/" and is not in skip, then we have a
992 + * conflict.
993 */
963 - struct nonmatching_ref_data data;
964 - struct ref_entry *entry = dir->entries[pos];
965 -
966 - dir = get_ref_dir(entry);
967 - data.skip = skip;
968 - sort_ref_dir(dir);
969 - if (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data)) {
970 - ret = 1;
971 - goto cleanup;
972 - }
994 + for (pos = string_list_find_insert_index(extras, dirname.buf, 0);
995 + pos < extras->nr; pos++) {
996 + const char *extra_refname = extras->items[pos].string;
997
974 - error("'%s' exists; cannot create '%s'",
975 - data.conflicting_refname, refname);
976 - goto cleanup;
998 + if (!starts_with(extra_refname, dirname.buf))
999 + break;
1000 +
1001 + if (!skip || !string_list_has_string(skip, extra_refname)) {
1002 + error("cannot process '%s' and '%s' at the same time",
1003 + refname, extra_refname);
1004 + goto cleanup;
1005 + }
1006 + }
1007 }
1008
1009 + /* No conflicts were found */
1010 ret = 1;
1011
1012 cleanup:
@@ -2296,6 +2327,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
2327 */
2328 static struct ref_lock *lock_ref_sha1_basic(const char *refname,
2329 const unsigned char *old_sha1,
2330 + const struct string_list *extras,
2331 const struct string_list *skip,
2332 unsigned int flags, int *type_p)
2333 {
@@ -2351,7 +2383,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
2383 * our refname.
2384 */
2385 if (is_null_sha1(lock->old_sha1) &&
2354 - !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) {
2386 + !is_refname_available(refname, extras, skip, get_packed_refs(&ref_cache))) {
2387 last_errno = ENOTDIR;
2388 goto error_return;
2389 }
@@ -2792,8 +2824,8 @@ static int rename_ref_available(const char *oldname, const char *newname)
2824 int ret;
2825
2826 string_list_insert(&skip, oldname);
2795 - ret = is_refname_available(newname, &skip, get_packed_refs(&ref_cache))
2796 - && is_refname_available(newname, &skip, get_loose_refs(&ref_cache));
2827 + ret = is_refname_available(newname, NULL, &skip, get_packed_refs(&ref_cache))
2828 + && is_refname_available(newname, NULL, &skip, get_loose_refs(&ref_cache));
2829 string_list_clear(&skip, 0);
2830 return ret;
2831 }
@@ -2851,7 +2883,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
2883
2884 logmoved = log;
2885
2854 - lock = lock_ref_sha1_basic(newrefname, NULL, NULL, 0, NULL);
2886 + lock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, 0, NULL);
2887 if (!lock) {
2888 error("unable to lock %s for update", newrefname);
2889 goto rollback;
@@ -2865,7 +2897,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
2897 return 0;
2898
2899 rollback:
2868 - lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, 0, NULL);
2900 + lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, 0, NULL);
2901 if (!lock) {
2902 error("unable to lock %s for rollback", oldrefname);
2903 goto rollbacklog;
@@ -3777,7 +3809,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
3809 update->refname,
3810 ((update->flags & REF_HAVE_OLD) ?
3811 update->old_sha1 : NULL),
3780 - NULL,
3812 + &affected_refnames, NULL,
3813 flags,
3814 &update->type);
3815 if (!update->lock) {
@@ -4054,7 +4086,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
4086 * reference itself, plus we might need to update the
4087 * reference if --updateref was specified:
4088 */
4057 - lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, &type);
4089 + lock = lock_ref_sha1_basic(refname, sha1, NULL, NULL, 0, &type);
4090 if (!lock)
4091 return error("cannot lock ref '%s'", refname);
4092 if (!reflog_exists(refname)) {
t/t1404-update-ref-df-conflicts.sh
+1 -1
@@ -96,7 +96,7 @@ test_expect_success 'new ref is a deeper prefix of existing packed' '
96
97 '
98
99 -test_expect_failure 'one new ref is a simple prefix of another' '
99 +test_expect_success 'one new ref is a simple prefix of another' '
100
101 prefix=refs/5 &&
102 test_update_rejected $prefix "a e" false "b c c/x d" \