diff options
author | Martin Ågren <martin.agren@gmail.com> | 2017-09-22 23:34:53 (GMT) |
---|---|---|
committer | Junio C Hamano <gitster@pobox.com> | 2017-09-24 01:06:04 (GMT) |
commit | 719920393737b3934a168f35ab45e09104edeed8 (patch) | |
tree | 5e68a3dfd103a03036d371b1eb3f0858f4f51d8d | |
parent | dcb572ab94f83a1a857d276fcebff5700077f2b7 (diff) | |
download | git-719920393737b3934a168f35ab45e09104edeed8.zip git-719920393737b3934a168f35ab45e09104edeed8.tar.gz git-719920393737b3934a168f35ab45e09104edeed8.tar.bz2 |
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>
-rw-r--r-- | builtin/fast-export.c | 3 | ||||
-rw-r--r-- | builtin/fsck.c | 7 | ||||
-rw-r--r-- | builtin/reflog.c | 2 | ||||
-rw-r--r-- | object.c | 13 | ||||
-rw-r--r-- | object.h | 8 | ||||
-rw-r--r-- | shallow.c | 2 |
6 files changed, 25 insertions, 10 deletions
diff --git a/builtin/fast-export.c b/builtin/fast-export.c index d412c0a..cff8d0f 100644 --- 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 d18244a..7d4ad02 100644 --- 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 6b34f23..2067cca 100644 --- 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; @@ -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) { @@ -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 *); /* @@ -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; } } |