From 219a0f33cae0f164f4353b2171ecd4c08d5b7ced Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:00 +0200 Subject: describe: make own copy of refname Do not retain a reference to the refname passed to the each_ref_fn callback get_name(), because there is no guarantee of the lifetimes of these names. Instead, make a local copy when needed. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/builtin/describe.c b/builtin/describe.c index 6636a68..3dc09eb 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -42,7 +42,7 @@ struct commit_name { unsigned prio:2; /* annotated tag = 2, tag = 1, head = 0 */ unsigned name_checked:1; unsigned char sha1[20]; - const char *path; + char *path; }; static const char *prio_names[] = { "head", "lightweight", "annotated", @@ -126,12 +126,14 @@ static void add_to_known_names(const char *path, } else { e->next = NULL; } + e->path = NULL; } e->tag = tag; e->prio = prio; e->name_checked = 0; hashcpy(e->sha1, sha1); - e->path = path; + free(e->path); + e->path = xstrdup(path); } } -- cgit v0.10.2-6-g49f6 From b87dbcc89940edbf92cbab381001542a7cac3627 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:01 +0200 Subject: fetch: make own copies of refnames Do not retain references to refnames passed to the each_ref_fn callback add_existing(), because their lifetime is not guaranteed. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/builtin/fetch.c b/builtin/fetch.c index 4b6b1df..f949115 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -590,7 +590,7 @@ static void find_non_local_tags(struct transport *transport, struct ref **head, struct ref ***tail) { - struct string_list existing_refs = STRING_LIST_INIT_NODUP; + struct string_list existing_refs = STRING_LIST_INIT_DUP; struct string_list remote_refs = STRING_LIST_INIT_NODUP; const struct ref *ref; struct string_list_item *item = NULL; @@ -693,7 +693,7 @@ static int truncate_fetch_head(void) static int do_fetch(struct transport *transport, struct refspec *refs, int ref_count) { - struct string_list existing_refs = STRING_LIST_INIT_NODUP; + struct string_list existing_refs = STRING_LIST_INIT_DUP; struct string_list_item *peer_item = NULL; struct ref *ref_map; struct ref *rm; -- cgit v0.10.2-6-g49f6 From df835d3a0c013b9ef912172663c7c58dad87164d Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:02 +0200 Subject: add_rev_cmdline(): make a copy of the name argument Instead of assuming that the memory pointed to by the name argument will live forever, make a local copy of it before storing it in the ref_cmdline_info. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/revision.c b/revision.c index a67b615..25e424c 100644 --- a/revision.c +++ b/revision.c @@ -898,6 +898,10 @@ static int limit_list(struct rev_info *revs) return 0; } +/* + * Add an entry to refs->cmdline with the specified information. + * *name is copied. + */ static void add_rev_cmdline(struct rev_info *revs, struct object *item, const char *name, @@ -909,7 +913,7 @@ static void add_rev_cmdline(struct rev_info *revs, ALLOC_GROW(info->rev, nr + 1, info->alloc); info->rev[nr].item = item; - info->rev[nr].name = name; + info->rev[nr].name = xstrdup(name); info->rev[nr].whence = whence; info->rev[nr].flags = flags; info->nr++; -- cgit v0.10.2-6-g49f6 From 91de344d7696ba8d7b92b7c503fdc0d3961e8cd2 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:03 +0200 Subject: builtin_diff_tree(): make it obvious that function wants two entries Instead of accepting an array and using exactly two elements from the array, take two single (struct object_array_entry *) arguments. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/builtin/diff.c b/builtin/diff.c index 8c2af6c..abdd613 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -153,7 +153,8 @@ static int builtin_diff_index(struct rev_info *revs, static int builtin_diff_tree(struct rev_info *revs, int argc, const char **argv, - struct object_array_entry *ent) + struct object_array_entry *ent0, + struct object_array_entry *ent1) { const unsigned char *(sha1[2]); int swap = 0; @@ -161,13 +162,14 @@ static int builtin_diff_tree(struct rev_info *revs, if (argc > 1) usage(builtin_diff_usage); - /* We saw two trees, ent[0] and ent[1]. - * if ent[1] is uninteresting, they are swapped + /* + * We saw two trees, ent0 and ent1. If ent1 is uninteresting, + * swap them. */ - if (ent[1].item->flags & UNINTERESTING) + if (ent1->item->flags & UNINTERESTING) swap = 1; - sha1[swap] = ent[0].item->sha1; - sha1[1-swap] = ent[1].item->sha1; + sha1[swap] = ent0->item->sha1; + sha1[1-swap] = ent1->item->sha1; diff_tree_sha1(sha1[0], sha1[1], "", &revs->diffopt); log_tree_diff_flush(revs); return 0; @@ -403,7 +405,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) else if (ents == 1) result = builtin_diff_index(&rev, argc, argv); else if (ents == 2) - result = builtin_diff_tree(&rev, argc, argv, ent); + result = builtin_diff_tree(&rev, argc, argv, &ent[0], &ent[1]); else if (ent[0].item->flags & UNINTERESTING) { /* * diff A...B where there is at least one merge base @@ -412,8 +414,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) * between the base and B. Note that we pick one * merge base at random if there are more than one. */ - ent[1] = ent[ents-1]; - result = builtin_diff_tree(&rev, argc, argv, ent); + result = builtin_diff_tree(&rev, argc, argv, &ent[0], &ent[ents-1]); } else result = builtin_diff_combined(&rev, argc, argv, ent, ents); -- cgit v0.10.2-6-g49f6 From 33055fa823eff9f4fddb858856f9b9a8d85316fc Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:04 +0200 Subject: cmd_diff(): use an object_array for holding trees Change cmd_diff() to use a (struct object_array) for holding the trees that it accumulates, rather than rolling its own equivalent. Incidentally, this change removes a hard-coded limit of 100 trees in combined diff, not that it matters in practice. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/builtin/diff.c b/builtin/diff.c index abdd613..661fdde 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -253,8 +253,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix) { int i; struct rev_info rev; - struct object_array_entry ent[100]; - int ents = 0, blobs = 0, paths = 0; + struct object_array ent = OBJECT_ARRAY_INIT; + int blobs = 0, paths = 0; const char *path = NULL; struct blobinfo blob[2]; int nongit; @@ -351,13 +351,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix) if (obj->type == OBJ_COMMIT) obj = &((struct commit *)obj)->tree->object; if (obj->type == OBJ_TREE) { - if (ARRAY_SIZE(ent) <= ents) - die(_("more than %d trees given: '%s'"), - (int) ARRAY_SIZE(ent), name); obj->flags |= flags; - ent[ents].item = obj; - ent[ents].name = name; - ents++; + add_object_array(obj, name, &ent); continue; } if (obj->type == OBJ_BLOB) { @@ -381,7 +376,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) /* * Now, do the arguments look reasonable? */ - if (!ents) { + if (!ent.nr) { switch (blobs) { case 0: result = builtin_diff_files(&rev, argc, argv); @@ -402,22 +397,26 @@ int cmd_diff(int argc, const char **argv, const char *prefix) } else if (blobs) usage(builtin_diff_usage); - else if (ents == 1) + else if (ent.nr == 1) result = builtin_diff_index(&rev, argc, argv); - else if (ents == 2) - result = builtin_diff_tree(&rev, argc, argv, &ent[0], &ent[1]); - else if (ent[0].item->flags & UNINTERESTING) { + else if (ent.nr == 2) + result = builtin_diff_tree(&rev, argc, argv, + &ent.objects[0], &ent.objects[1]); + else if (ent.objects[0].item->flags & UNINTERESTING) { /* * diff A...B where there is at least one merge base - * between A and B. We have ent[0] == merge-base, - * ent[ents-2] == A, and ent[ents-1] == B. Show diff - * between the base and B. Note that we pick one - * merge base at random if there are more than one. + * between A and B. We have ent.objects[0] == + * merge-base, ent.objects[ents-2] == A, and + * ent.objects[ents-1] == B. Show diff between the + * base and B. Note that we pick one merge base at + * random if there are more than one. */ - result = builtin_diff_tree(&rev, argc, argv, &ent[0], &ent[ents-1]); + result = builtin_diff_tree(&rev, argc, argv, + &ent.objects[0], + &ent.objects[ent.nr-1]); } else result = builtin_diff_combined(&rev, argc, argv, - ent, ents); + ent.objects, ent.nr); result = diff_result_code(&rev.diffopt, result); if (1 < rev.diffopt.skip_stat_unmatch) refresh_index_quietly(); -- cgit v0.10.2-6-g49f6 From 026f09e79689f99f573b8399443e8c4ffa84a794 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:05 +0200 Subject: cmd_diff(): rename local variable "list" -> "entry" It's not a list, it's an array entry. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/builtin/diff.c b/builtin/diff.c index 661fdde..84243d9 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -339,9 +339,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix) } for (i = 0; i < rev.pending.nr; i++) { - struct object_array_entry *list = rev.pending.objects+i; - struct object *obj = list->item; - const char *name = list->name; + struct object_array_entry *entry = &rev.pending.objects[i]; + struct object *obj = entry->item; + const char *name = entry->name; int flags = (obj->flags & UNINTERESTING); if (!obj->parsed) obj = parse_object(obj->sha1); @@ -360,7 +360,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) die(_("more than two blobs given: '%s'"), name); hashcpy(blob[blobs].sha1, obj->sha1); blob[blobs].name = name; - blob[blobs].mode = list->mode; + blob[blobs].mode = entry->mode; blobs++; continue; -- cgit v0.10.2-6-g49f6 From 5b1e14eab3109c1d33e27b00ee18f8f9f60e779c Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:06 +0200 Subject: cmd_diff(): make it obvious which cases are exclusive of each other At first glance the OBJ_COMMIT, OBJ_TREE, and OBJ_BLOB cases look like they might be mutually exclusive. But the OBJ_COMMIT case doesn't end the loop iteration with "continue" like the other two cases, but rather falls through. So use if...else if...else construct to make it more obvious that only the last two cases are mutually exclusive. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/builtin/diff.c b/builtin/diff.c index 84243d9..9fc273d 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -350,22 +350,21 @@ int cmd_diff(int argc, const char **argv, const char *prefix) die(_("invalid object '%s' given."), name); if (obj->type == OBJ_COMMIT) obj = &((struct commit *)obj)->tree->object; + if (obj->type == OBJ_TREE) { obj->flags |= flags; add_object_array(obj, name, &ent); - continue; - } - if (obj->type == OBJ_BLOB) { + } else if (obj->type == OBJ_BLOB) { if (2 <= blobs) die(_("more than two blobs given: '%s'"), name); hashcpy(blob[blobs].sha1, obj->sha1); blob[blobs].name = name; blob[blobs].mode = entry->mode; blobs++; - continue; + } else { + die(_("unhandled object '%s' given."), name); } - die(_("unhandled object '%s' given."), name); } if (rev.prune_data.nr) { if (!path) -- cgit v0.10.2-6-g49f6 From ff5f5f268fd36b6db059c06d124305866ecaa6ce Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:07 +0200 Subject: revision: split some overly-long lines Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/revision.c b/revision.c index 25e424c..8ac88d6 100644 --- a/revision.c +++ b/revision.c @@ -70,7 +70,8 @@ static int show_path_truncated(FILE *out, const struct name_path *path) return ours || emitted; } -void show_object_with_name(FILE *out, struct object *obj, const struct name_path *path, const char *component) +void show_object_with_name(FILE *out, struct object *obj, + const struct name_path *path, const char *component) { struct name_path leaf; leaf.up = (struct name_path *)path; @@ -186,7 +187,9 @@ void mark_parents_uninteresting(struct commit *commit) } } -static void add_pending_object_with_mode(struct rev_info *revs, struct object *obj, const char *name, unsigned mode) +static void add_pending_object_with_mode(struct rev_info *revs, + struct object *obj, + const char *name, unsigned mode) { if (!obj) return; @@ -209,7 +212,8 @@ static void add_pending_object_with_mode(struct rev_info *revs, struct object *o add_object_array_with_mode(obj, name, &revs->pending, mode); } -void add_pending_object(struct rev_info *revs, struct object *obj, const char *name) +void add_pending_object(struct rev_info *revs, + struct object *obj, const char *name) { add_pending_object_with_mode(revs, obj, name, S_IFINVALID); } @@ -226,7 +230,9 @@ void add_head_to_pending(struct rev_info *revs) add_pending_object(revs, obj, "HEAD"); } -static struct object *get_reference(struct rev_info *revs, const char *name, const unsigned char *sha1, unsigned int flags) +static struct object *get_reference(struct rev_info *revs, const char *name, + const unsigned char *sha1, + unsigned int flags) { struct object *object; @@ -247,7 +253,8 @@ void add_pending_sha1(struct rev_info *revs, const char *name, add_pending_object(revs, object, name); } -static struct commit *handle_commit(struct rev_info *revs, struct object *object, const char *name) +static struct commit *handle_commit(struct rev_info *revs, + struct object *object, const char *name) { unsigned long flags = object->flags; @@ -368,7 +375,8 @@ static void file_change(struct diff_options *options, DIFF_OPT_SET(options, HAS_CHANGES); } -static int rev_compare_tree(struct rev_info *revs, struct commit *parent, struct commit *commit) +static int rev_compare_tree(struct rev_info *revs, + struct commit *parent, struct commit *commit) { struct tree *t1 = parent->tree; struct tree *t2 = commit->tree; diff --git a/revision.h b/revision.h index 01bd2b7..9628465 100644 --- a/revision.h +++ b/revision.h @@ -195,19 +195,23 @@ struct setup_revision_opt { }; extern void init_revisions(struct rev_info *revs, const char *prefix); -extern int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *); +extern int setup_revisions(int argc, const char **argv, struct rev_info *revs, + struct setup_revision_opt *); extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx, - const struct option *options, - const char * const usagestr[]); + const struct option *options, + const char * const usagestr[]); #define REVARG_CANNOT_BE_FILENAME 01 #define REVARG_COMMITTISH 02 -extern int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt); +extern int handle_revision_arg(const char *arg, struct rev_info *revs, + int flags, unsigned revarg_opt); extern void reset_revision_walk(void); extern int prepare_revision_walk(struct rev_info *revs); extern struct commit *get_revision(struct rev_info *revs); -extern char *get_revision_mark(const struct rev_info *revs, const struct commit *commit); -extern void put_revision_mark(const struct rev_info *revs, const struct commit *commit); +extern char *get_revision_mark(const struct rev_info *revs, + const struct commit *commit); +extern void put_revision_mark(const struct rev_info *revs, + const struct commit *commit); extern void mark_parents_uninteresting(struct commit *commit); extern void mark_tree_uninteresting(struct tree *tree); @@ -220,15 +224,19 @@ struct name_path { char *path_name(const struct name_path *path, const char *name); -extern void show_object_with_name(FILE *, struct object *, const struct name_path *, const char *); +extern void show_object_with_name(FILE *, struct object *, + const struct name_path *, const char *); extern void add_object(struct object *obj, struct object_array *p, struct name_path *path, const char *name); -extern void add_pending_object(struct rev_info *revs, struct object *obj, const char *name); -extern void add_pending_sha1(struct rev_info *revs, const char *name, const unsigned char *sha1, unsigned int flags); +extern void add_pending_object(struct rev_info *revs, + struct object *obj, const char *name); +extern void add_pending_sha1(struct rev_info *revs, + const char *name, const unsigned char *sha1, + unsigned int flags); extern void add_head_to_pending(struct rev_info *); @@ -238,7 +246,9 @@ enum commit_action { commit_error }; -extern enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit); -extern enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit); +extern enum commit_action get_commit_action(struct rev_info *revs, + struct commit *commit); +extern enum commit_action simplify_commit(struct rev_info *revs, + struct commit *commit); #endif -- cgit v0.10.2-6-g49f6 From aeb4a51ef82c71c9a65d11f77aeedb53bea0e983 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:08 +0200 Subject: object_array: add function object_array_filter() Add a function that allows unwanted entries in an object_array to be removed. This encapsulation is a step towards giving object_array ownership of its entries' name memory. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/object.c b/object.c index 20703f5..fcd4a82 100644 --- a/object.c +++ b/object.c @@ -278,6 +278,22 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj array->nr = ++nr; } +void object_array_filter(struct object_array *array, + object_array_each_func_t want, void *cb_data) +{ + unsigned nr = array->nr, src, dst; + struct object_array_entry *objects = array->objects; + + for (src = dst = 0; src < nr; src++) { + if (want(&objects[src], cb_data)) { + if (src != dst) + objects[dst] = objects[src]; + dst++; + } + } + array->nr = dst; +} + void object_array_remove_duplicates(struct object_array *array) { unsigned int ref, src, dst; diff --git a/object.h b/object.h index 97d384b..0d39ff4 100644 --- a/object.h +++ b/object.h @@ -85,6 +85,17 @@ int object_list_contains(struct object_list *list, struct object *obj); /* Object array handling .. */ void add_object_array(struct object *obj, const char *name, struct object_array *array); void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode); + +typedef int (*object_array_each_func_t)(struct object_array_entry *, void *); + +/* + * Apply want to each entry in array, retaining only the entries for + * which the function returns true. Preserve the order of the entries + * that are retained. + */ +void object_array_filter(struct object_array *array, + object_array_each_func_t want, void *cb_data); + void object_array_remove_duplicates(struct object_array *); void clear_object_flags(unsigned flags); -- cgit v0.10.2-6-g49f6 From be6754c67f5aff02e9528116d06890391524f48e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:09 +0200 Subject: revision: use object_array_filter() in implementation of gc_boundary() Use object_array_filter(), which will soon be made smarter about cleaning up discarded entries properly. Also add a function comment. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/revision.c b/revision.c index 8ac88d6..be73cb4 100644 --- a/revision.c +++ b/revision.c @@ -2435,25 +2435,23 @@ static struct commit *get_revision_1(struct rev_info *revs) return NULL; } -static void gc_boundary(struct object_array *array) +/* + * Return true for entries that have not yet been shown. (This is an + * object_array_each_func_t.) + */ +static int entry_unshown(struct object_array_entry *entry, void *cb_data_unused) { - unsigned nr = array->nr; - unsigned alloc = array->alloc; - struct object_array_entry *objects = array->objects; + return !(entry->item->flags & SHOWN); +} - if (alloc <= nr) { - unsigned i, j; - for (i = j = 0; i < nr; i++) { - if (objects[i].item->flags & SHOWN) - continue; - if (i != j) - objects[j] = objects[i]; - j++; - } - for (i = j; i < nr; i++) - objects[i].item = NULL; - array->nr = j; - } +/* + * If array is on the verge of a realloc, garbage-collect any entries + * that have already been shown to try to free up some space. + */ +static void gc_boundary(struct object_array *array) +{ + if (array->nr == array->alloc) + object_array_filter(array, entry_unshown, NULL); } static void create_boundary_commit_list(struct rev_info *revs) -- cgit v0.10.2-6-g49f6 From 1506510c170d23b8f769757dc81904f334c40281 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:10 +0200 Subject: object_array_remove_duplicates(): rewrite to reduce copying The old version copied one entry to its destination position, then deleted any matching entries from the tail of the array. This required the tail of the array to be copied multiple times. It didn't affect the complexity of the algorithm because the whole tail has to be searched through anyway. But all the copying was unnecessary. Instead, check for the existence of an entry with the same name in the *head* of the list before copying an entry to its final position. This way each entry has to be copied at most one time. Extract a helper function contains_name() to do a bit of the work. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/object.c b/object.c index fcd4a82..10b5349 100644 --- a/object.c +++ b/object.c @@ -294,22 +294,32 @@ void object_array_filter(struct object_array *array, array->nr = dst; } +/* + * Return true iff array already contains an entry with name. + */ +static int contains_name(struct object_array *array, const char *name) +{ + unsigned nr = array->nr, i; + struct object_array_entry *object = array->objects; + + for (i = 0; i < nr; i++, object++) + if (!strcmp(object->name, name)) + return 1; + return 0; +} + void object_array_remove_duplicates(struct object_array *array) { - unsigned int ref, src, dst; + unsigned nr = array->nr, src; struct object_array_entry *objects = array->objects; - for (ref = 0; ref + 1 < array->nr; ref++) { - for (src = ref + 1, dst = src; - src < array->nr; - src++) { - if (!strcmp(objects[ref].name, objects[src].name)) - continue; - if (src != dst) - objects[dst] = objects[src]; - dst++; + array->nr = 0; + for (src = 0; src < nr; src++) { + if (!contains_name(array, objects[src].name)) { + if (src != array->nr) + objects[array->nr] = objects[src]; + array->nr++; } - array->nr = dst; } } diff --git a/object.h b/object.h index 0d39ff4..6c1c27f 100644 --- a/object.h +++ b/object.h @@ -96,7 +96,11 @@ typedef int (*object_array_each_func_t)(struct object_array_entry *, void *); void object_array_filter(struct object_array *array, object_array_each_func_t want, void *cb_data); -void object_array_remove_duplicates(struct object_array *); +/* + * Remove from array all but the first entry with a given name. + * Warning: this function uses an O(N^2) algorithm. + */ +void object_array_remove_duplicates(struct object_array *array); void clear_object_flags(unsigned flags); -- cgit v0.10.2-6-g49f6 From 16aa3bfc9b9dc587d02f29d8e54810480cf955e4 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:11 +0200 Subject: fsck: don't put a void*-shaped peg in a char*-shaped hole The source of this nonsense was 04d3975937 fsck: reduce stack footprint , which wedged a pointer to parent into the object_array_entry's name field. The parent pointer was passed to traverse_one_object(), even though that function *didn't use it*. The useless code has been deleted over time. Commit a1cdc25172 fsck: drop unused parameter from traverse_one_object() removed the parent pointer from traverse_one_object()'s signature. Commit c0aa335c95 Remove unused variables removed the code that read the parent pointer back out of the name field. This commit takes the last step: don't write the parent pointer into the name field in the first place. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/builtin/fsck.c b/builtin/fsck.c index bb9a2cd..9909b6d 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -112,7 +112,7 @@ static int mark_object(struct object *obj, int type, void *data) return 1; } - add_object_array(obj, (void *) parent, &pending); + add_object_array(obj, NULL, &pending); return 0; } -- cgit v0.10.2-6-g49f6 From 3826902d25febf61274925d6f12cc7471ba3c38f Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:12 +0200 Subject: find_first_merges(): initialize merges variable using initializer Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/submodule.c b/submodule.c index e728025..b837c04 100644 --- a/submodule.c +++ b/submodule.c @@ -846,7 +846,7 @@ static int find_first_merges(struct object_array *result, const char *path, struct commit *a, struct commit *b) { int i, j; - struct object_array merges; + struct object_array merges = OBJECT_ARRAY_INIT; struct commit *commit; int contains_another; @@ -856,7 +856,6 @@ static int find_first_merges(struct object_array *result, const char *path, struct rev_info revs; struct setup_revision_opt rev_opts; - memset(&merges, 0, sizeof(merges)); memset(result, 0, sizeof(struct object_array)); memset(&rev_opts, 0, sizeof(rev_opts)); -- cgit v0.10.2-6-g49f6 From 5de0c0155c824803a7ee39326b09eeb93e9ec899 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:13 +0200 Subject: find_first_merges(): remove unnecessary code No names are ever set for the object_array_entries in merges, so there is no need to pretend to copy them to the result array. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/submodule.c b/submodule.c index b837c04..ad476ce 100644 --- a/submodule.c +++ b/submodule.c @@ -893,8 +893,7 @@ static int find_first_merges(struct object_array *result, const char *path, } if (!contains_another) - add_object_array(merges.objects[i].item, - merges.objects[i].name, result); + add_object_array(merges.objects[i].item, NULL, result); } free(merges.objects); -- cgit v0.10.2-6-g49f6 From 31faeb2088ef35d0108ad81df3550513d6cec798 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:14 +0200 Subject: object_array_entry: fix memory handling of the name field Previously, the memory management of the object_array_entry::name field was inconsistent and undocumented. object_array_entries are ultimately created by a single function, add_object_array_with_mode(), which has an argument "const char *name". This function used to simply set the name field to reference the string pointed to by the name parameter, and nobody on the object_array side ever freed the memory. Thus, it assumed that the memory for the name field would be managed by the caller, and that the lifetime of that string would be at least as long as the lifetime of the object_array_entry. But callers were inconsistent: * Some passed pointers to constant strings or argv entries, which was OK. * Some passed pointers to newly-allocated memory, but didn't arrange for the memory ever to be freed. * Some passed the return value of sha1_to_hex(), which is a pointer to a statically-allocated buffer that can be overwritten at any time. * Some passed pointers to refnames that they received from a for_each_ref()-type iteration, but the lifetimes of such refnames is not guaranteed by the refs API. Bring consistency to this mess by changing object_array to make its own copy for the object_array_entry::name field and free this memory when an object_array_entry is deleted from the array. Many callers were passing the empty string as the name parameter, so as a performance optimization, treat the empty string specially. Instead of making a copy, store a pointer to a statically-allocated empty string to object_array_entry::name. When deleting such an entry, skip the free(). Change the callers that were already passing copies to add_object_array_with_mode() to either skip the copy, or (if the memory needed to be allocated anyway) freeing the memory itself. A part of this commit effectively reverts 70d26c6e76 read_revisions_from_stdin: make copies for handle_revision_arg because the copying introduced by that commit (which is still necessary) is now done at a deeper level. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/bundle.c b/bundle.c index 4b0e5cd..3d64311 100644 --- a/bundle.c +++ b/bundle.c @@ -281,7 +281,7 @@ int create_bundle(struct bundle_header *header, const char *path, if (!get_sha1_hex(buf.buf + 1, sha1)) { struct object *object = parse_object_or_die(sha1, buf.buf); object->flags |= UNINTERESTING; - add_pending_object(&revs, object, xstrdup(buf.buf)); + add_pending_object(&revs, object, buf.buf); } } else if (!get_sha1_hex(buf.buf, sha1)) { struct object *object = parse_object_or_die(sha1, buf.buf); diff --git a/object.c b/object.c index 10b5349..243d694 100644 --- a/object.c +++ b/object.c @@ -260,11 +260,18 @@ void add_object_array(struct object *obj, const char *name, struct object_array add_object_array_with_mode(obj, name, array, S_IFINVALID); } +/* + * A zero-length string to which object_array_entry::name can be + * initialized without requiring a malloc/free. + */ +static char object_array_slopbuf[1]; + void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode) { unsigned nr = array->nr; unsigned alloc = array->alloc; struct object_array_entry *objects = array->objects; + struct object_array_entry *entry; if (nr >= alloc) { alloc = (alloc + 32) * 2; @@ -272,9 +279,16 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj array->alloc = alloc; array->objects = objects; } - objects[nr].item = obj; - objects[nr].name = name; - objects[nr].mode = mode; + entry = &objects[nr]; + entry->item = obj; + if (!name) + entry->name = NULL; + else if (!*name) + /* Use our own empty string instead of allocating one: */ + entry->name = object_array_slopbuf; + else + entry->name = xstrdup(name); + entry->mode = mode; array->nr = ++nr; } @@ -289,6 +303,9 @@ void object_array_filter(struct object_array *array, if (src != dst) objects[dst] = objects[src]; dst++; + } else { + if (objects[src].name != object_array_slopbuf) + free(objects[src].name); } } array->nr = dst; @@ -319,6 +336,9 @@ void object_array_remove_duplicates(struct object_array *array) if (src != array->nr) objects[array->nr] = objects[src]; array->nr++; + } else { + if (objects[src].name != object_array_slopbuf) + free(objects[src].name); } } } diff --git a/object.h b/object.h index 6c1c27f..2ff68c5 100644 --- a/object.h +++ b/object.h @@ -11,7 +11,13 @@ struct object_array { unsigned int alloc; struct object_array_entry { struct object *item; - const char *name; + /* + * name or NULL. If non-NULL, the memory pointed to + * is owned by this object *except* if it points at + * object_array_slopbuf, which is a static copy of the + * empty string. + */ + char *name; unsigned mode; } *objects; }; diff --git a/revision.c b/revision.c index be73cb4..4aeda33 100644 --- a/revision.c +++ b/revision.c @@ -88,7 +88,9 @@ void add_object(struct object *obj, struct name_path *path, const char *name) { - add_object_array(obj, path_name(path, name), p); + char *pn = path_name(path, name); + add_object_array(obj, pn, p); + free(pn); } static void mark_blob_uninteresting(struct blob *blob) @@ -1288,7 +1290,7 @@ static void read_revisions_from_stdin(struct rev_info *revs, } die("options not supported in --stdin mode"); } - if (handle_revision_arg(xstrdup(sb.buf), revs, 0, + if (handle_revision_arg(sb.buf, revs, 0, REVARG_CANNOT_BE_FILENAME)) die("bad revision '%s'", sb.buf); } -- cgit v0.10.2-6-g49f6 From 6f64a16faff0189d54cf30854483493574476c6e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:15 +0200 Subject: do_fetch(): reduce scope of peer_item Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/builtin/fetch.c b/builtin/fetch.c index f949115..80c6e37 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -694,7 +694,6 @@ static int do_fetch(struct transport *transport, struct refspec *refs, int ref_count) { struct string_list existing_refs = STRING_LIST_INIT_DUP; - struct string_list_item *peer_item = NULL; struct ref *ref_map; struct ref *rm; int autotags = (transport->remote->fetch_tags == 1); @@ -724,8 +723,9 @@ static int do_fetch(struct transport *transport, for (rm = ref_map; rm; rm = rm->next) { if (rm->peer_ref) { - peer_item = string_list_lookup(&existing_refs, - rm->peer_ref->name); + struct string_list_item *peer_item = + string_list_lookup(&existing_refs, + rm->peer_ref->name); if (peer_item) hashcpy(rm->peer_ref->old_sha1, peer_item->util); -- cgit v0.10.2-6-g49f6 From 5b87d8d3f53c1dfb54027123af612488abc85006 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:16 +0200 Subject: do_fetch(): clean up existing_refs before exiting Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/builtin/fetch.c b/builtin/fetch.c index 80c6e37..48df5fa 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -697,6 +697,7 @@ static int do_fetch(struct transport *transport, struct ref *ref_map; struct ref *rm; int autotags = (transport->remote->fetch_tags == 1); + int retcode = 0; for_each_ref(add_existing, &existing_refs); @@ -712,9 +713,9 @@ static int do_fetch(struct transport *transport, /* if not appending, truncate FETCH_HEAD */ if (!append && !dry_run) { - int errcode = truncate_fetch_head(); - if (errcode) - return errcode; + retcode = truncate_fetch_head(); + if (retcode) + goto cleanup; } ref_map = get_ref_map(transport, refs, ref_count, tags, &autotags); @@ -736,7 +737,8 @@ static int do_fetch(struct transport *transport, transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1"); if (fetch_refs(transport, ref_map)) { free_refs(ref_map); - return 1; + retcode = 1; + goto cleanup; } if (prune) { /* If --tags was specified, pretend the user gave us the canonical tags refspec */ @@ -779,7 +781,9 @@ static int do_fetch(struct transport *transport, free_refs(ref_map); } - return 0; + cleanup: + string_list_clear(&existing_refs, 0); + return retcode; } static void set_option(const char *name, const char *value) -- cgit v0.10.2-6-g49f6 From f83918edcb6fbcd1c5d8378c11e57edcc47bd232 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:17 +0200 Subject: add_existing(): do not retain a reference to sha1 Its lifetime is not guaranteed, so make a copy. Free the memory when the string_list is cleared. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/builtin/fetch.c b/builtin/fetch.c index 48df5fa..fa6fe44 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -571,7 +571,8 @@ static int add_existing(const char *refname, const unsigned char *sha1, { struct string_list *list = (struct string_list *)cbdata; struct string_list_item *item = string_list_insert(list, refname); - item->util = (void *)sha1; + item->util = xmalloc(20); + hashcpy(item->util, sha1); return 0; } @@ -636,7 +637,7 @@ static void find_non_local_tags(struct transport *transport, item = string_list_insert(&remote_refs, ref->name); item->util = (void *)ref->old_sha1; } - string_list_clear(&existing_refs, 0); + string_list_clear(&existing_refs, 1); /* * We may have a final lightweight tag that needs to be @@ -782,7 +783,7 @@ static int do_fetch(struct transport *transport, } cleanup: - string_list_clear(&existing_refs, 0); + string_list_clear(&existing_refs, 1); return retcode; } -- cgit v0.10.2-6-g49f6 From 1d811dbd04afc9fd4e59d1a1a2c090abb835d55f Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:18 +0200 Subject: show_head_ref(): do not shadow name of argument Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/http-backend.c b/http-backend.c index 6b85ffa..3135835 100644 --- a/http-backend.c +++ b/http-backend.c @@ -416,8 +416,8 @@ static int show_head_ref(const char *name, const unsigned char *sha1, struct strbuf *buf = cb_data; if (flag & REF_ISSYMREF) { - unsigned char sha1[20]; - const char *target = resolve_ref_unsafe(name, sha1, 1, NULL); + unsigned char unused[20]; + const char *target = resolve_ref_unsafe(name, unused, 1, NULL); const char *target_nons = strip_namespace(target); strbuf_addf(buf, "ref: %s\n", target_nons); -- cgit v0.10.2-6-g49f6 From 3e4ca43fd0ea0f6a150de3be096b52954d64f523 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:19 +0200 Subject: show_head_ref(): rename first parameter to "refname" This is the usual convention. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/http-backend.c b/http-backend.c index 3135835..0324417 100644 --- a/http-backend.c +++ b/http-backend.c @@ -410,14 +410,14 @@ static void get_info_refs(char *arg) strbuf_release(&buf); } -static int show_head_ref(const char *name, const unsigned char *sha1, +static int show_head_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data) { struct strbuf *buf = cb_data; if (flag & REF_ISSYMREF) { unsigned char unused[20]; - const char *target = resolve_ref_unsafe(name, unused, 1, NULL); + const char *target = resolve_ref_unsafe(refname, unused, 1, NULL); const char *target_nons = strip_namespace(target); strbuf_addf(buf, "ref: %s\n", target_nons); -- cgit v0.10.2-6-g49f6 From d235e994f893fddd7808a4f5af879fd718563649 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:20 +0200 Subject: string_list_add_one_ref(): rename first parameter to "refname" This is the usual convention. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/notes.c b/notes.c index f63fd57..fa7cdf7 100644 --- a/notes.c +++ b/notes.c @@ -918,12 +918,12 @@ out: return ret; } -static int string_list_add_one_ref(const char *path, const unsigned char *sha1, +static int string_list_add_one_ref(const char *refname, const unsigned char *sha1, int flag, void *cb) { struct string_list *refs = cb; - if (!unsorted_string_list_has_string(refs, path)) - string_list_append(refs, path); + if (!unsorted_string_list_has_string(refs, refname)) + string_list_append(refs, refname); return 0; } -- cgit v0.10.2-6-g49f6 From 8c46bf904fe95105faaa1823332c91d2802182c2 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:21 +0200 Subject: string_list_add_refs_by_glob(): add a comment about memory management Since string_list_add_one_ref() adds refname to the string list, but the lifetime of refname is limited, it is important that the string_list passed to string_list_add_one_ref() has strdup_strings set. Document this fact. All current callers do the right thing. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/notes.c b/notes.c index fa7cdf7..b69c0b8 100644 --- a/notes.c +++ b/notes.c @@ -927,8 +927,12 @@ static int string_list_add_one_ref(const char *refname, const unsigned char *sha return 0; } +/* + * The list argument must have strdup_strings set on it. + */ void string_list_add_refs_by_glob(struct string_list *list, const char *glob) { + assert(list->strdup_strings); if (has_glob_specials(glob)) { for_each_glob_ref(string_list_add_one_ref, glob, list); } else { -- cgit v0.10.2-6-g49f6 From 66ce0366285f73c340f04a21b3d8be898d7bf5a7 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:22 +0200 Subject: exclude_existing(): set existing_refs.strdup_strings The each_ref_fn add_existing() adds refnames to the existing_refs list. But the lifetimes of these refnames is not guaranteed by the refs API, so configure the string_list to make copies as it adds them. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/builtin/show-ref.c b/builtin/show-ref.c index 8d9b76a..4a0310d 100644 --- a/builtin/show-ref.c +++ b/builtin/show-ref.c @@ -103,7 +103,7 @@ static int add_existing(const char *refname, const unsigned char *sha1, int flag */ static int exclude_existing(const char *match) { - static struct string_list existing_refs = STRING_LIST_INIT_NODUP; + static struct string_list existing_refs = STRING_LIST_INIT_DUP; char buf[1024]; int matchlen = match ? strlen(match) : 0; -- cgit v0.10.2-6-g49f6 From bf42772e38db8e758aa28a045e8cba88096a9fcc Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:23 +0200 Subject: register_ref(): make a copy of the bad reference SHA-1 The lifetime of the sha1 parameter passed to an each_ref_fn callback is not guaranteed, so make a copy for later use. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/bisect.c b/bisect.c index 374d9e2..71c1958 100644 --- a/bisect.c +++ b/bisect.c @@ -15,7 +15,7 @@ static struct sha1_array good_revs; static struct sha1_array skipped_revs; -static const unsigned char *current_bad_sha1; +static unsigned char *current_bad_sha1; static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL}; static const char *argv_show_branch[] = {"show-branch", NULL, NULL}; @@ -404,7 +404,8 @@ static int register_ref(const char *refname, const unsigned char *sha1, int flags, void *cb_data) { if (!strcmp(refname, "bad")) { - current_bad_sha1 = sha1; + current_bad_sha1 = xmalloc(20); + hashcpy(current_bad_sha1, sha1); } else if (!prefixcmp(refname, "good-")) { sha1_array_append(&good_revs, sha1); } else if (!prefixcmp(refname, "skip-")) { -- cgit v0.10.2-6-g49f6 From 4f78c24c63bf0b035afc02372727a3b5897d9835 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 25 May 2013 11:08:24 +0200 Subject: refs: document the lifetime of the args passed to each_ref_fn The lifetime of the memory pointed to by the refname and sha1 arguments to each_ref_fn was never documented, but some callers used to assume that it was essentially permanent. In fact the API does *not* guarantee that these objects live beyond a single callback invocation. In the current code, the lifetimes are bound together with the lifetimes of the ref_caches. Since these are usually long, the callers usually got away with their sloppiness. But even today, if a ref_cache is invalidated the memory can be freed. And planned changes to reference caching, needed to eliminate race conditions, will probably need to shorten the lifetimes of these objects. The commits leading up to this have (hopefully) fixed all of the callers of the for_each_ref()-like functions. This commit does the last step: documents what each_ref_fn callbacks can assume about object lifetimes. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/refs.h b/refs.h index a35eafc..122ec03 100644 --- a/refs.h +++ b/refs.h @@ -15,13 +15,23 @@ struct ref_lock { #define REF_ISBROKEN 0x04 /* - * Calls the specified function for each ref file until it returns - * nonzero, and returns the value. Please note that it is not safe to - * modify references while an iteration is in progress, unless the - * same callback function invocation that modifies the reference also - * returns a nonzero value to immediately stop the iteration. + * The signature for the callback function for the for_each_*() + * functions below. The memory pointed to by the refname and sha1 + * arguments is only guaranteed to be valid for the duration of a + * single callback invocation. + */ +typedef int each_ref_fn(const char *refname, + const unsigned char *sha1, int flags, void *cb_data); + +/* + * The following functions invoke the specified callback function for + * each reference indicated. If the function ever returns a nonzero + * value, stop the iteration and return that value. Please note that + * it is not safe to modify references while an iteration is in + * progress, unless the same callback function invocation that + * modifies the reference also returns a nonzero value to immediately + * stop the iteration. */ -typedef int each_ref_fn(const char *refname, const unsigned char *sha1, int flags, void *cb_data); extern int head_ref(each_ref_fn, void *); extern int for_each_ref(each_ref_fn, void *); extern int for_each_ref_in(const char *, each_ref_fn, void *); -- cgit v0.10.2-6-g49f6