refs/iterator: separate lifecycle from iteration

The ref and reflog iterators have their lifecycle attached to iteration:
once the iterator reaches its end, it is automatically released and the
caller doesn't have to care about that anymore. When the iterator should
be released before it has been exhausted, callers must explicitly abort
the iterator via `ref_iterator_abort()`.

This lifecycle is somewhat unusual in the Git codebase and creates two
problems:

  - Callsites need to be very careful about when exactly they call
    `ref_iterator_abort()`, as calling the function is only valid when
    the iterator itself still is. This leads to somewhat awkward calling
    patterns in some situations.

  - It is impossible to reuse iterators and re-seek them to a different
    prefix. This feature isn't supported by any iterator implementation
    except for the reftable iterators anyway, but if it was implemented
    it would allow us to optimize cases where we need to search for
    specific references repeatedly by reusing internal state.

Detangle the lifecycle from iteration so that we don't deallocate the
iterator anymore once it is exhausted. Instead, callers are now expected
to always call a newly introduce `ref_iterator_free()` function that
deallocates the iterator and its internal state.

Note that the `dir_iterator` is somewhat special because it does not
implement the `ref_iterator` interface, but is only used to implement
other iterators. Consequently, we have to provide `dir_iterator_free()`
instead of `dir_iterator_release()` as the allocated structure itself is
managed by the `dir_iterator` interfaces, as well, and not freed by
`ref_iterator_free()` like in all the other cases.

While at it, drop the return value of `ref_iterator_abort()`, which
wasn't really required by any of the iterator implementations anyway.
Furthermore, stop calling `base_ref_iterator_free()` in any of the
backends, but instead call it in `ref_iterator_free()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/clone.c b/builtin/clone.c
index f9a2ecbe9cc944793203d722b42573e779575c5a..add9d8600c707728f4db1378a420544c0b49be64 100644 (file)
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -342,6 +342,8 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
                strbuf_setlen(src, src_len);
                die(_("failed to iterate over '%s'"), src->buf);
        }
+
+       dir_iterator_free(iter);
 }
 
 static void clone_local(const char *src_repo, const char *dest_repo)

diff --git a/dir-iterator.c b/dir-iterator.c
index de619846f29ad9c51d42b538d12c1b62e3958b5d..857e1d9bdaf3019b20b2857d5ba3e93cd0c6847c 100644 (file)
--- a/dir-iterator.c
+++ b/dir-iterator.c
@@ -193,9 +193,9 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
 
        if (S_ISDIR(iter->base.st.st_mode) && push_level(iter)) {
                if (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)
-                       goto error_out;
+                       return ITER_ERROR;
                if (iter->levels_nr == 0)
-                       goto error_out;
+                       return ITER_ERROR;
        }
 
        /* Loop until we find an entry that we can give back to the caller. */
@@ -211,11 +211,11 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
                        int ret = next_directory_entry(level->dir, iter->base.path.buf, &de);
                        if (ret < 0) {
                                if (iter->flags & DIR_ITERATOR_PEDANTIC)
-                                       goto error_out;
+                                       return ITER_ERROR;
                                continue;
                        } else if (ret > 0) {
                                if (pop_level(iter) == 0)
-                                       return dir_iterator_abort(dir_iterator);
+                                       return ITER_DONE;
                                continue;
                        }
 
@@ -223,7 +223,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
                } else {
                        if (level->entries_idx >= level->entries.nr) {
                                if (pop_level(iter) == 0)
-                                       return dir_iterator_abort(dir_iterator);
+                                       return ITER_DONE;
                                continue;
                        }
 
@@ -232,22 +232,21 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
 
                if (prepare_next_entry_data(iter, name)) {
                        if (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)
-                               goto error_out;
+                               return ITER_ERROR;
                        continue;
                }
 
                return ITER_OK;
        }
-
-error_out:
-       dir_iterator_abort(dir_iterator);
-       return ITER_ERROR;
 }
 
-int dir_iterator_abort(struct dir_iterator *dir_iterator)
+void dir_iterator_free(struct dir_iterator *dir_iterator)
 {
        struct dir_iterator_int *iter = (struct dir_iterator_int *)dir_iterator;
 
+       if (!iter)
+               return;
+
        for (; iter->levels_nr; iter->levels_nr--) {
                struct dir_iterator_level *level =
                        &iter->levels[iter->levels_nr - 1];
@@ -266,7 +265,6 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)
        free(iter->levels);
        strbuf_release(&iter->base.path);
        free(iter);
-       return ITER_DONE;
 }
 
 struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags)
@@ -301,7 +299,7 @@ struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags)
        return dir_iterator;
 
 error_out:
-       dir_iterator_abort(dir_iterator);
+       dir_iterator_free(dir_iterator);
        errno = saved_errno;
        return NULL;
 }

diff --git a/dir-iterator.h b/dir-iterator.h
index 6d438809b6ed51b5735080f878c08aa2302b7552..ccd6a1973436a9173283776feace017e69f6c3f0 100644 (file)
--- a/dir-iterator.h
+++ b/dir-iterator.h
@@ -28,7 +28,7 @@
  *
  *     while ((ok = dir_iterator_advance(iter)) == ITER_OK) {
  *             if (want_to_stop_iteration()) {
- *                     ok = dir_iterator_abort(iter);
+ *                     ok = ITER_DONE;
  *                     break;
  *             }
  *
@@ -39,6 +39,7 @@
  *
  *     if (ok != ITER_DONE)
  *             handle_error();
+ *     dir_iterator_free(iter);
  *
  * Callers are allowed to modify iter->path while they are working,
  * but they must restore it to its original contents before calling
@@ -107,11 +108,7 @@ struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags);
  */
 int dir_iterator_advance(struct dir_iterator *iterator);
 
-/*
- * End the iteration before it has been exhausted. Free the
- * dir_iterator and any associated resources and return ITER_DONE. On
- * error, free the dir_iterator and return ITER_ERROR.
- */
-int dir_iterator_abort(struct dir_iterator *iterator);
+/* Free the dir_iterator and any associated resources. */
+void dir_iterator_free(struct dir_iterator *iterator);
 
 #endif

diff --git a/iterator.h b/iterator.h
index 0f6900e43ad68b6c06d46ea50f86d94487b94a1a..6b77dcc26262d944a1e1d270a253f02e79241192 100644 (file)
--- a/iterator.h
+++ b/iterator.h
@@ -12,7 +12,7 @@
 #define ITER_OK 0
 
 /*
- * The iterator is exhausted and has been freed.
+ * The iterator is exhausted.
  */
 #define ITER_DONE -1
 

diff --git a/refs.c b/refs.c
index 957446da9e53b6259627acac769ff119e385c22b..eeb8fb102169f2cbc29231af21c9e4294e2b9301 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -2485,6 +2485,7 @@ int refs_verify_refnames_available(struct ref_store *refs,
        struct strbuf dirname = STRBUF_INIT;
        struct strbuf referent = STRBUF_INIT;
        struct string_list_item *item;
+       struct ref_iterator *iter = NULL;
        struct strset dirnames;
        int ret = -1;
 
@@ -2561,7 +2562,6 @@ int refs_verify_refnames_available(struct ref_store *refs,
                strbuf_addch(&dirname, '/');
 
                if (!initial_transaction) {
-                       struct ref_iterator *iter;
                        int ok;
 
                        iter = refs_ref_iterator_begin(refs, dirname.buf, NULL, 0,
@@ -2573,12 +2573,14 @@ int refs_verify_refnames_available(struct ref_store *refs,
 
                                strbuf_addf(err, _("'%s' exists; cannot create '%s'"),
                                            iter->refname, refname);
-                               ref_iterator_abort(iter);
                                goto cleanup;
                        }
 
                        if (ok != ITER_DONE)
                                BUG("error while iterating over references");
+
+                       ref_iterator_free(iter);
+                       iter = NULL;
                }
 
                extra_refname = find_descendant_ref(dirname.buf, extras, skip);
@@ -2595,6 +2597,7 @@ cleanup:
        strbuf_release(&referent);
        strbuf_release(&dirname);
        strset_clear(&dirnames);
+       ref_iterator_free(iter);
        return ret;
 }
 

diff --git a/refs/debug.c b/refs/debug.c
index fbc4df08b43ca73a866a0494079f26c1ad161293..a9786da4ba175ac3d9030324805dbdf0b3527d14 100644 (file)
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -179,19 +179,18 @@ static int debug_ref_iterator_peel(struct ref_iterator *ref_iterator,
        return res;
 }
 
-static int debug_ref_iterator_abort(struct ref_iterator *ref_iterator)
+static void debug_ref_iterator_release(struct ref_iterator *ref_iterator)
 {
        struct debug_ref_iterator *diter =
                (struct debug_ref_iterator *)ref_iterator;
-       int res = diter->iter->vtable->abort(diter->iter);
-       trace_printf_key(&trace_refs, "iterator_abort: %d\n", res);
-       return res;
+       diter->iter->vtable->release(diter->iter);
+       trace_printf_key(&trace_refs, "iterator_abort\n");
 }
 
 static struct ref_iterator_vtable debug_ref_iterator_vtable = {
        .advance = debug_ref_iterator_advance,
        .peel = debug_ref_iterator_peel,
-       .abort = debug_ref_iterator_abort,
+       .release = debug_ref_iterator_release,
 };
 
 static struct ref_iterator *

diff --git a/refs/files-backend.c b/refs/files-backend.c
index ab6f0af55029748943a4020d77490f588960c83f..e97a267ad6520573fe04fce394383c46e123370a 100644 (file)
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -915,10 +915,6 @@ static int files_ref_iterator_advance(struct ref_iterator *ref_iterator)
                return ITER_OK;
        }
 
-       iter->iter0 = NULL;
-       if (ref_iterator_abort(ref_iterator) != ITER_DONE)
-               ok = ITER_ERROR;
-
        return ok;
 }
 
@@ -931,23 +927,17 @@ static int files_ref_iterator_peel(struct ref_iterator *ref_iterator,
        return ref_iterator_peel(iter->iter0, peeled);
 }
 
-static int files_ref_iterator_abort(struct ref_iterator *ref_iterator)
+static void files_ref_iterator_release(struct ref_iterator *ref_iterator)
 {
        struct files_ref_iterator *iter =
                (struct files_ref_iterator *)ref_iterator;
-       int ok = ITER_DONE;
-
-       if (iter->iter0)
-               ok = ref_iterator_abort(iter->iter0);
-
-       base_ref_iterator_free(ref_iterator);
-       return ok;
+       ref_iterator_free(iter->iter0);
 }
 
 static struct ref_iterator_vtable files_ref_iterator_vtable = {
        .advance = files_ref_iterator_advance,
        .peel = files_ref_iterator_peel,
-       .abort = files_ref_iterator_abort,
+       .release = files_ref_iterator_release,
 };
 
 static struct ref_iterator *files_ref_iterator_begin(
@@ -1378,7 +1368,7 @@ static int should_pack_refs(struct files_ref_store *refs,
                                    iter->flags, opts))
                        refcount++;
                if (refcount >= limit) {
-                       ref_iterator_abort(iter);
+                       ref_iterator_free(iter);
                        return 1;
                }
        }
@@ -1386,6 +1376,7 @@ static int should_pack_refs(struct files_ref_store *refs,
        if (ret != ITER_DONE)
                die("error while iterating over references");
 
+       ref_iterator_free(iter);
        return 0;
 }
 
@@ -1452,6 +1443,7 @@ static int files_pack_refs(struct ref_store *ref_store,
        packed_refs_unlock(refs->packed_ref_store);
 
        prune_refs(refs, &refs_to_prune);
+       ref_iterator_free(iter);
        strbuf_release(&err);
        return 0;
 }
@@ -2299,9 +2291,6 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
                return ITER_OK;
        }
 
-       iter->dir_iterator = NULL;
-       if (ref_iterator_abort(ref_iterator) == ITER_ERROR)
-               ok = ITER_ERROR;
        return ok;
 }
 
@@ -2311,23 +2300,17 @@ static int files_reflog_iterator_peel(struct ref_iterator *ref_iterator UNUSED,
        BUG("ref_iterator_peel() called for reflog_iterator");
 }
 
-static int files_reflog_iterator_abort(struct ref_iterator *ref_iterator)
+static void files_reflog_iterator_release(struct ref_iterator *ref_iterator)
 {
        struct files_reflog_iterator *iter =
                (struct files_reflog_iterator *)ref_iterator;
-       int ok = ITER_DONE;
-
-       if (iter->dir_iterator)
-               ok = dir_iterator_abort(iter->dir_iterator);
-
-       base_ref_iterator_free(ref_iterator);
-       return ok;
+       dir_iterator_free(iter->dir_iterator);
 }
 
 static struct ref_iterator_vtable files_reflog_iterator_vtable = {
        .advance = files_reflog_iterator_advance,
        .peel = files_reflog_iterator_peel,
-       .abort = files_reflog_iterator_abort,
+       .release = files_reflog_iterator_release,
 };
 
 static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
@@ -3837,6 +3820,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
                ret = error(_("failed to iterate over '%s'"), sb.buf);
 
 out:
+       dir_iterator_free(iter);
        strbuf_release(&sb);
        strbuf_release(&refname);
        return ret;

diff --git a/refs/iterator.c b/refs/iterator.c
index d25e568bf0b7684611ea74b1bb658ad4b816cc2d..d61474cba758c5c183461e943c5cada1a7aa13ca 100644 (file)
--- a/refs/iterator.c
+++ b/refs/iterator.c
@@ -21,9 +21,14 @@ int ref_iterator_peel(struct ref_iterator *ref_iterator,
        return ref_iterator->vtable->peel(ref_iterator, peeled);
 }
 
-int ref_iterator_abort(struct ref_iterator *ref_iterator)
+void ref_iterator_free(struct ref_iterator *ref_iterator)
 {
-       return ref_iterator->vtable->abort(ref_iterator);
+       if (ref_iterator) {
+               ref_iterator->vtable->release(ref_iterator);
+               /* Help make use-after-free bugs fail quickly: */
+               ref_iterator->vtable = NULL;
+               free(ref_iterator);
+       }
 }
 
 void base_ref_iterator_init(struct ref_iterator *iter,
@@ -36,20 +41,13 @@ void base_ref_iterator_init(struct ref_iterator *iter,
        iter->flags = 0;
 }
 
-void base_ref_iterator_free(struct ref_iterator *iter)
-{
-       /* Help make use-after-free bugs fail quickly: */
-       iter->vtable = NULL;
-       free(iter);
-}
-
 struct empty_ref_iterator {
        struct ref_iterator base;
 };
 
-static int empty_ref_iterator_advance(struct ref_iterator *ref_iterator)
+static int empty_ref_iterator_advance(struct ref_iterator *ref_iterator UNUSED)
 {
-       return ref_iterator_abort(ref_iterator);
+       return ITER_DONE;
 }
 
 static int empty_ref_iterator_peel(struct ref_iterator *ref_iterator UNUSED,
@@ -58,16 +56,14 @@ static int empty_ref_iterator_peel(struct ref_iterator *ref_iterator UNUSED,
        BUG("peel called for empty iterator");
 }
 
-static int empty_ref_iterator_abort(struct ref_iterator *ref_iterator)
+static void empty_ref_iterator_release(struct ref_iterator *ref_iterator UNUSED)
 {
-       base_ref_iterator_free(ref_iterator);
-       return ITER_DONE;
 }
 
 static struct ref_iterator_vtable empty_ref_iterator_vtable = {
        .advance = empty_ref_iterator_advance,
        .peel = empty_ref_iterator_peel,
-       .abort = empty_ref_iterator_abort,
+       .release = empty_ref_iterator_release,
 };
 
 struct ref_iterator *empty_ref_iterator_begin(void)
@@ -151,11 +147,13 @@ static int merge_ref_iterator_advance(struct ref_iterator *ref_iterator)
        if (!iter->current) {
                /* Initialize: advance both iterators to their first entries */
                if ((ok = ref_iterator_advance(iter->iter0)) != ITER_OK) {
+                       ref_iterator_free(iter->iter0);
                        iter->iter0 = NULL;
                        if (ok == ITER_ERROR)
                                goto error;
                }
                if ((ok = ref_iterator_advance(iter->iter1)) != ITER_OK) {
+                       ref_iterator_free(iter->iter1);
                        iter->iter1 = NULL;
                        if (ok == ITER_ERROR)
                                goto error;
@@ -166,6 +164,7 @@ static int merge_ref_iterator_advance(struct ref_iterator *ref_iterator)
                 * entry:
                 */
                if ((ok = ref_iterator_advance(*iter->current)) != ITER_OK) {
+                       ref_iterator_free(*iter->current);
                        *iter->current = NULL;
                        if (ok == ITER_ERROR)
                                goto error;
@@ -179,9 +178,8 @@ static int merge_ref_iterator_advance(struct ref_iterator *ref_iterator)
                        iter->select(iter->iter0, iter->iter1, iter->cb_data);
 
                if (selection == ITER_SELECT_DONE) {
-                       return ref_iterator_abort(ref_iterator);
+                       return ITER_DONE;
                } else if (selection == ITER_SELECT_ERROR) {
-                       ref_iterator_abort(ref_iterator);
                        return ITER_ERROR;
                }
 
@@ -195,6 +193,7 @@ static int merge_ref_iterator_advance(struct ref_iterator *ref_iterator)
 
                if (selection & ITER_SKIP_SECONDARY) {
                        if ((ok = ref_iterator_advance(*secondary)) != ITER_OK) {
+                               ref_iterator_free(*secondary);
                                *secondary = NULL;
                                if (ok == ITER_ERROR)
                                        goto error;
@@ -211,7 +210,6 @@ static int merge_ref_iterator_advance(struct ref_iterator *ref_iterator)
        }
 
 error:
-       ref_iterator_abort(ref_iterator);
        return ITER_ERROR;
 }
 
@@ -227,28 +225,18 @@ static int merge_ref_iterator_peel(struct ref_iterator *ref_iterator,
        return ref_iterator_peel(*iter->current, peeled);
 }
 
-static int merge_ref_iterator_abort(struct ref_iterator *ref_iterator)
+static void merge_ref_iterator_release(struct ref_iterator *ref_iterator)
 {
        struct merge_ref_iterator *iter =
                (struct merge_ref_iterator *)ref_iterator;
-       int ok = ITER_DONE;
-
-       if (iter->iter0) {
-               if (ref_iterator_abort(iter->iter0) != ITER_DONE)
-                       ok = ITER_ERROR;
-       }
-       if (iter->iter1) {
-               if (ref_iterator_abort(iter->iter1) != ITER_DONE)
-                       ok = ITER_ERROR;
-       }
-       base_ref_iterator_free(ref_iterator);
-       return ok;
+       ref_iterator_free(iter->iter0);
+       ref_iterator_free(iter->iter1);
 }
 
 static struct ref_iterator_vtable merge_ref_iterator_vtable = {
        .advance = merge_ref_iterator_advance,
        .peel = merge_ref_iterator_peel,
-       .abort = merge_ref_iterator_abort,
+       .release = merge_ref_iterator_release,
 };
 
 struct ref_iterator *merge_ref_iterator_begin(
@@ -310,10 +298,10 @@ struct ref_iterator *overlay_ref_iterator_begin(
         * them.
         */
        if (is_empty_ref_iterator(front)) {
-               ref_iterator_abort(front);
+               ref_iterator_free(front);
                return back;
        } else if (is_empty_ref_iterator(back)) {
-               ref_iterator_abort(back);
+               ref_iterator_free(back);
                return front;
        }
 
@@ -350,19 +338,15 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator)
 
        while ((ok = ref_iterator_advance(iter->iter0)) == ITER_OK) {
                int cmp = compare_prefix(iter->iter0->refname, iter->prefix);
-
                if (cmp < 0)
                        continue;
-
-               if (cmp > 0) {
-                       /*
-                        * As the source iterator is ordered, we
-                        * can stop the iteration as soon as we see a
-                        * refname that comes after the prefix:
-                        */
-                       ok = ref_iterator_abort(iter->iter0);
-                       break;
-               }
+               /*
+                * As the source iterator is ordered, we
+                * can stop the iteration as soon as we see a
+                * refname that comes after the prefix:
+                */
+               if (cmp > 0)
+                       return ITER_DONE;
 
                if (iter->trim) {
                        /*
@@ -386,9 +370,6 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator)
                return ITER_OK;
        }
 
-       iter->iter0 = NULL;
-       if (ref_iterator_abort(ref_iterator) != ITER_DONE)
-               return ITER_ERROR;
        return ok;
 }
 
@@ -401,23 +382,18 @@ static int prefix_ref_iterator_peel(struct ref_iterator *ref_iterator,
        return ref_iterator_peel(iter->iter0, peeled);
 }
 
-static int prefix_ref_iterator_abort(struct ref_iterator *ref_iterator)
+static void prefix_ref_iterator_release(struct ref_iterator *ref_iterator)
 {
        struct prefix_ref_iterator *iter =
                (struct prefix_ref_iterator *)ref_iterator;
-       int ok = ITER_DONE;
-
-       if (iter->iter0)
-               ok = ref_iterator_abort(iter->iter0);
+       ref_iterator_free(iter->iter0);
        free(iter->prefix);
-       base_ref_iterator_free(ref_iterator);
-       return ok;
 }
 
 static struct ref_iterator_vtable prefix_ref_iterator_vtable = {
        .advance = prefix_ref_iterator_advance,
        .peel = prefix_ref_iterator_peel,
-       .abort = prefix_ref_iterator_abort,
+       .release = prefix_ref_iterator_release,
 };
 
 struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,
@@ -453,20 +429,14 @@ int do_for_each_ref_iterator(struct ref_iterator *iter,
        current_ref_iter = iter;
        while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
                retval = fn(iter->refname, iter->referent, iter->oid, iter->flags, cb_data);
-               if (retval) {
-                       /*
-                        * If ref_iterator_abort() returns ITER_ERROR,
-                        * we ignore that error in deference to the
-                        * callback function's return value.
-                        */
-                       ref_iterator_abort(iter);
+               if (retval)
                        goto out;
-               }
        }
 
 out:
        current_ref_iter = old_ref_iter;
        if (ok == ITER_ERROR)
-               return -1;
+               retval = -1;
+       ref_iterator_free(iter);
        return retval;
 }

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index a7b6f74b6e35f897f619c540cbc600bbd888bc67..38a1956d1a8d943f3b43287bb57f9597727dc1de 100644 (file)
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -954,9 +954,6 @@ static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)
                return ITER_OK;
        }
 
-       if (ref_iterator_abort(ref_iterator) != ITER_DONE)
-               ok = ITER_ERROR;
-
        return ok;
 }
 
@@ -976,23 +973,19 @@ static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator,
        }
 }
 
-static int packed_ref_iterator_abort(struct ref_iterator *ref_iterator)
+static void packed_ref_iterator_release(struct ref_iterator *ref_iterator)
 {
        struct packed_ref_iterator *iter =
                (struct packed_ref_iterator *)ref_iterator;
-       int ok = ITER_DONE;
-
        strbuf_release(&iter->refname_buf);
        free(iter->jump);
        release_snapshot(iter->snapshot);
-       base_ref_iterator_free(ref_iterator);
-       return ok;
 }
 
 static struct ref_iterator_vtable packed_ref_iterator_vtable = {
        .advance = packed_ref_iterator_advance,
        .peel = packed_ref_iterator_peel,
-       .abort = packed_ref_iterator_abort
+       .release = packed_ref_iterator_release,
 };
 
 static int jump_list_entry_cmp(const void *va, const void *vb)
@@ -1362,8 +1355,10 @@ static int write_with_updates(struct packed_ref_store *refs,
         */
        iter = packed_ref_iterator_begin(&refs->base, "", NULL,
                                         DO_FOR_EACH_INCLUDE_BROKEN);
-       if ((ok = ref_iterator_advance(iter)) != ITER_OK)
+       if ((ok = ref_iterator_advance(iter)) != ITER_OK) {
+               ref_iterator_free(iter);
                iter = NULL;
+       }
 
        i = 0;
 
@@ -1411,8 +1406,10 @@ static int write_with_updates(struct packed_ref_store *refs,
                                 * the iterator over the unneeded
                                 * value.
                                 */
-                               if ((ok = ref_iterator_advance(iter)) != ITER_OK)
+                               if ((ok = ref_iterator_advance(iter)) != ITER_OK) {
+                                       ref_iterator_free(iter);
                                        iter = NULL;
+                               }
                                cmp = +1;
                        } else {
                                /*
@@ -1449,8 +1446,10 @@ static int write_with_updates(struct packed_ref_store *refs,
                                               peel_error ? NULL : &peeled))
                                goto write_error;
 
-                       if ((ok = ref_iterator_advance(iter)) != ITER_OK)
+                       if ((ok = ref_iterator_advance(iter)) != ITER_OK) {
+                               ref_iterator_free(iter);
                                iter = NULL;
+                       }
                } else if (is_null_oid(&update->new_oid)) {
                        /*
                         * The update wants to delete the reference,
@@ -1499,9 +1498,7 @@ write_error:
                    get_tempfile_path(refs->tempfile), strerror(errno));
 
 error:
-       if (iter)
-               ref_iterator_abort(iter);
-
+       ref_iterator_free(iter);
        delete_tempfile(&refs->tempfile);
        return -1;
 }

diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index 02f09e4df88f23955678fa5cee4ef1ac4832cf2d..6457e02c1eaeea77ed3eadd39c0acbbd18813b94 100644 (file)
--- a/refs/ref-cache.c
+++ b/refs/ref-cache.c
@@ -409,7 +409,7 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)
                if (++level->index == level->dir->nr) {
                        /* This level is exhausted; pop up a level */
                        if (--iter->levels_nr == 0)
-                               return ref_iterator_abort(ref_iterator);
+                               return ITER_DONE;
 
                        continue;
                }
@@ -452,21 +452,18 @@ static int cache_ref_iterator_peel(struct ref_iterator *ref_iterator,
        return peel_object(iter->repo, ref_iterator->oid, peeled) ? -1 : 0;
 }
 
-static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)
+static void cache_ref_iterator_release(struct ref_iterator *ref_iterator)
 {
        struct cache_ref_iterator *iter =
                (struct cache_ref_iterator *)ref_iterator;
-
        free((char *)iter->prefix);
        free(iter->levels);
-       base_ref_iterator_free(ref_iterator);
-       return ITER_DONE;
 }
 
 static struct ref_iterator_vtable cache_ref_iterator_vtable = {
        .advance = cache_ref_iterator_advance,
        .peel = cache_ref_iterator_peel,
-       .abort = cache_ref_iterator_abort
+       .release = cache_ref_iterator_release,
 };
 
 struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 8894b43d1d1a327d404d3923c507d2d39649de19..7d3bab654b0788d0c113568610128a4f805e1af6 100644 (file)
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -273,11 +273,11 @@ enum do_for_each_ref_flags {
  * the next reference and returns ITER_OK. The data pointed at by
  * refname and oid belong to the iterator; if you want to retain them
  * after calling ref_iterator_advance() again or calling
- * ref_iterator_abort(), you must make a copy. When the iteration has
+ * ref_iterator_free(), you must make a copy. When the iteration has
  * been exhausted, ref_iterator_advance() releases any resources
  * associated with the iteration, frees the ref_iterator object, and
  * returns ITER_DONE. If you want to abort the iteration early, call
- * ref_iterator_abort(), which also frees the ref_iterator object and
+ * ref_iterator_free(), which also frees the ref_iterator object and
  * any associated resources. If there was an internal error advancing
  * to the next entry, ref_iterator_advance() aborts the iteration,
  * frees the ref_iterator, and returns ITER_ERROR.
@@ -293,7 +293,7 @@ enum do_for_each_ref_flags {
  *
  *     while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
  *             if (want_to_stop_iteration()) {
- *                     ok = ref_iterator_abort(iter);
+ *                     ok = ITER_DONE;
  *                     break;
  *             }
  *
@@ -307,6 +307,7 @@ enum do_for_each_ref_flags {
  *
  *     if (ok != ITER_DONE)
  *             handle_error();
+ *     ref_iterator_free(iter);
  */
 struct ref_iterator {
        struct ref_iterator_vtable *vtable;
@@ -333,12 +334,8 @@ int ref_iterator_advance(struct ref_iterator *ref_iterator);
 int ref_iterator_peel(struct ref_iterator *ref_iterator,
                      struct object_id *peeled);
 
-/*
- * End the iteration before it has been exhausted, freeing the
- * reference iterator and any associated resources and returning
- * ITER_DONE. If the abort itself failed, return ITER_ERROR.
- */
-int ref_iterator_abort(struct ref_iterator *ref_iterator);
+/* Free the reference iterator and any associated resources. */
+void ref_iterator_free(struct ref_iterator *ref_iterator);
 
 /*
  * An iterator over nothing (its first ref_iterator_advance() call
@@ -438,13 +435,6 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,
 void base_ref_iterator_init(struct ref_iterator *iter,
                            struct ref_iterator_vtable *vtable);
 
-/*
- * Base class destructor for ref_iterators. Destroy the ref_iterator
- * part of iter and shallow-free the object. This is meant to be
- * called only by the destructors of derived classes.
- */
-void base_ref_iterator_free(struct ref_iterator *iter);
-
 /* Virtual function declarations for ref_iterators: */
 
 /*
@@ -463,15 +453,14 @@ typedef int ref_iterator_peel_fn(struct ref_iterator *ref_iterator,
 
 /*
  * Implementations of this function should free any resources specific
- * to the derived class, then call base_ref_iterator_free() to clean
- * up and free the ref_iterator object.
+ * to the derived class.
  */
-typedef int ref_iterator_abort_fn(struct ref_iterator *ref_iterator);
+typedef void ref_iterator_release_fn(struct ref_iterator *ref_iterator);
 
 struct ref_iterator_vtable {
        ref_iterator_advance_fn *advance;
        ref_iterator_peel_fn *peel;
-       ref_iterator_abort_fn *abort;
+       ref_iterator_release_fn *release;
 };
 
 /*

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 546861d64c21ff50d5b7778c7e7b6193b91c9942..2d5f4afe6b52f262fb1fec2ba1cc4ec629f805b4 100644 (file)
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -711,17 +711,10 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
                break;
        }
 
-       if (iter->err > 0) {
-               if (ref_iterator_abort(ref_iterator) != ITER_DONE)
-                       return ITER_ERROR;
+       if (iter->err > 0)
                return ITER_DONE;
-       }
-
-       if (iter->err < 0) {
-               ref_iterator_abort(ref_iterator);
+       if (iter->err < 0)
                return ITER_ERROR;
-       }
-
        return ITER_OK;
 }
 
@@ -740,7 +733,7 @@ static int reftable_ref_iterator_peel(struct ref_iterator *ref_iterator,
        return -1;
 }
 
-static int reftable_ref_iterator_abort(struct ref_iterator *ref_iterator)
+static void reftable_ref_iterator_release(struct ref_iterator *ref_iterator)
 {
        struct reftable_ref_iterator *iter =
                (struct reftable_ref_iterator *)ref_iterator;
@@ -751,14 +744,12 @@ static int reftable_ref_iterator_abort(struct ref_iterator *ref_iterator)
                        free(iter->exclude_patterns[i]);
                free(iter->exclude_patterns);
        }
-       free(iter);
-       return ITER_DONE;
 }
 
 static struct ref_iterator_vtable reftable_ref_iterator_vtable = {
        .advance = reftable_ref_iterator_advance,
        .peel = reftable_ref_iterator_peel,
-       .abort = reftable_ref_iterator_abort
+       .release = reftable_ref_iterator_release,
 };
 
 static int qsort_strcmp(const void *va, const void *vb)
@@ -2020,17 +2011,10 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)
                break;
        }
 
-       if (iter->err > 0) {
-               if (ref_iterator_abort(ref_iterator) != ITER_DONE)
-                       return ITER_ERROR;
+       if (iter->err > 0)
                return ITER_DONE;
-       }
-
-       if (iter->err < 0) {
-               ref_iterator_abort(ref_iterator);
+       if (iter->err < 0)
                return ITER_ERROR;
-       }
-
        return ITER_OK;
 }
 
@@ -2041,21 +2025,19 @@ static int reftable_reflog_iterator_peel(struct ref_iterator *ref_iterator UNUSE
        return -1;
 }
 
-static int reftable_reflog_iterator_abort(struct ref_iterator *ref_iterator)
+static void reftable_reflog_iterator_release(struct ref_iterator *ref_iterator)
 {
        struct reftable_reflog_iterator *iter =
                (struct reftable_reflog_iterator *)ref_iterator;
        reftable_log_record_release(&iter->log);
        reftable_iterator_destroy(&iter->iter);
        strbuf_release(&iter->last_name);
-       free(iter);
-       return ITER_DONE;
 }
 
 static struct ref_iterator_vtable reftable_reflog_iterator_vtable = {
        .advance = reftable_reflog_iterator_advance,
        .peel = reftable_reflog_iterator_peel,
-       .abort = reftable_reflog_iterator_abort
+       .release = reftable_reflog_iterator_release,
 };
 
 static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftable_ref_store *refs,

diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c
index 6b297bd75361407fbba7cb6dcab0190d5ef5a1b2..8d46e8ba40953b2966447290f3942318a43d5128 100644 (file)
--- a/t/helper/test-dir-iterator.c
+++ b/t/helper/test-dir-iterator.c
@@ -53,6 +53,7 @@ int cmd__dir_iterator(int argc, const char **argv)
                printf("(%s) [%s] %s\n", diter->relative_path, diter->basename,
                       diter->path.buf);
        }
+       dir_iterator_free(diter);
 
        if (iter_status != ITER_DONE) {
                printf("dir_iterator_advance failure\n");