object_array: add and use `object_array_pop()`

In a couple of places, we pop objects off an object array `foo` by
decreasing `foo.nr`. We access `foo.nr` in many places, but most if not
all other times we do so read-only, e.g., as we iterate over the array.
But when we change `foo.nr` behind the array's back, it feels a bit
nasty and looks like it might leak memory.

Leaks happen if the popped element has an allocated `name` or `path`.
At the moment, that is not the case. Still, 1) the object array might
gain more fields that want to be freed, 2) a code path where we pop
might start using names or paths, 3) one of these code paths might be
copied to somewhere where we do, and 4) using a dedicated function for
popping is conceptually cleaner.

Introduce and use `object_array_pop()` instead. Release memory in the
new function. Document that popping an object leaves the associated
elements in limbo.

The converted places were identified by grepping for "\.nr\>" and
looking for "--".

Make the new function return NULL on an empty array. This is consistent
with `pop_commit()` and allows the following:

	while ((o = object_array_pop(&foo)) != NULL) {
		// do something
	}

But as noted above, we don't need to go out of our way to avoid reading
`foo.nr`. This is probably more readable:

	while (foo.nr) {
		... o = object_array_pop(&foo);
		// do something
	}

The name of `object_array_pop()` does not quite align with
`add_object_array()`. That is unfortunate. On the other hand, it matches
`object_array_clear()`. Arguably it's `add_...` that is the odd one out,
since it reads like it's used to "add" an "object array". For that
reason, side with `object_array_clear()`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d412c0a8f354659acade8ea6271f5c60826a0b3f..cff8d0fc5bbd8bd7acdd561b6c10ddc28e93b5bb 100644 (file)
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -634,11 +634,10 @@ static void handle_tail(struct object_array *commits, struct rev_info *revs)
 {
        struct commit *commit;
        while (commits->nr) {
-               commit = (struct commit *)commits->objects[commits->nr - 1].item;
+               commit = (struct commit *)object_array_pop(commits);
                if (has_unshown_parent(commit))
                        return;
                handle_commit(commit, revs);
-               commits->nr--;
        }
 }
 

diff --git a/builtin/fsck.c b/builtin/fsck.c
index d18244ab546bae863e6ef81d58663dc5c27c49c0..7d4ad027338ee59218378edf97884ea648b3c39e 100644 (file)
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -181,12 +181,7 @@ static int traverse_reachable(void)
        if (show_progress)
                progress = start_progress_delay(_("Checking connectivity"), 0, 0, 2);
        while (pending.nr) {
-               struct object_array_entry *entry;
-               struct object *obj;
-
-               entry = pending.objects + --pending.nr;
-               obj = entry->item;
-               result |= traverse_one_object(obj);
+               result |= traverse_one_object(object_array_pop(&pending));
                display_progress(progress, ++nr);
        }
        stop_progress(&progress);

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 6b34f23e782805159f3de5ff0dcfaac9db910adc..2067cca5b1ec4ed141a3e404cb7738c9b0ebc5d9 100644 (file)
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -126,7 +126,7 @@ static int commit_is_complete(struct commit *commit)
                struct commit *c;
                struct commit_list *parent;
 
-               c = (struct commit *)study.objects[--study.nr].item;
+               c = (struct commit *)object_array_pop(&study);
                if (!c->object.parsed && !parse_object(&c->object.oid))
                        c->object.flags |= INCOMPLETE;
 

diff --git a/object.c b/object.c
index 321d7e9201b2ba6c8b39592a591786409a87edb1..b9a4a0e50172fb169226786b17134c6ab0627333 100644 (file)
--- a/object.c
+++ b/object.c
@@ -353,6 +353,19 @@ static void object_array_release_entry(struct object_array_entry *ent)
        free(ent->path);
 }
 
+struct object *object_array_pop(struct object_array *array)
+{
+       struct object *ret;
+
+       if (!array->nr)
+               return NULL;
+
+       ret = array->objects[array->nr - 1].item;
+       object_array_release_entry(&array->objects[array->nr - 1]);
+       array->nr--;
+       return ret;
+}
+
 void object_array_filter(struct object_array *array,
                         object_array_each_func_t want, void *cb_data)
 {

diff --git a/object.h b/object.h
index 0a419ba8da52453ebbd202f50d5c2aa1c9aea2f0..df8abe96f7da2585f7f07c68f53695a2d450e2ae 100644 (file)
--- a/object.h
+++ b/object.h
@@ -116,6 +116,14 @@ int object_list_contains(struct object_list *list, struct object *obj);
 void add_object_array(struct object *obj, const char *name, struct object_array *array);
 void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path);
 
+/*
+ * Returns NULL if the array is empty. Otherwise, returns the last object
+ * after removing its entry from the array. Other resources associated
+ * with that object are left in an unspecified state and should not be
+ * examined.
+ */
+struct object *object_array_pop(struct object_array *array);
+
 typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);
 
 /*

diff --git a/shallow.c b/shallow.c
index 54359d549075b5cfe1670ca979ff3cf5eeedc9d1..901ac979171e4563d09724c48478c96a212a8bb3 100644 (file)
--- a/shallow.c
+++ b/shallow.c
@@ -99,7 +99,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
                                cur_depth = 0;
                        } else {
                                commit = (struct commit *)
-                                       stack.objects[--stack.nr].item;
+                                       object_array_pop(&stack);
                                cur_depth = *(int *)commit->util;
                        }
                }