From 8828f2985f1967201c256fb01f92a91acfdb5001 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Tue, 28 Oct 2014 21:52:34 +0100 Subject: [PATCH 1/4] use child_process_init() to initialize struct child_process variables Call child_process_init() instead of zeroing the memory of variables of type struct child_process by hand before use because the former is both clearer and shorter. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- bundle.c | 2 +- column.c | 2 +- trailer.c | 2 +- transport-helper.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/bundle.c b/bundle.c index fa67057e6..c846092e9 100644 --- a/bundle.c +++ b/bundle.c @@ -381,7 +381,7 @@ int create_bundle(struct bundle_header *header, const char *path, write_or_die(bundle_fd, "\n", 1); /* write pack */ - memset(&rls, 0, sizeof(rls)); + child_process_init(&rls); argv_array_pushl(&rls.args, "pack-objects", "--all-progress-implied", "--stdout", "--thin", "--delta-base-offset", diff --git a/column.c b/column.c index 8082a944f..786abe62b 100644 --- a/column.c +++ b/column.c @@ -374,7 +374,7 @@ int run_column_filter(int colopts, const struct column_options *opts) if (fd_out != -1) return -1; - memset(&column_process, 0, sizeof(column_process)); + child_process_init(&column_process); argv = &column_process.args; argv_array_push(argv, "column"); diff --git a/trailer.c b/trailer.c index 851456656..7ff036cb8 100644 --- a/trailer.c +++ b/trailer.c @@ -237,7 +237,7 @@ static const char *apply_command(const char *command, const char *arg) strbuf_replace(&cmd, TRAILER_ARG_STRING, arg); argv[0] = cmd.buf; - memset(&cp, 0, sizeof(cp)); + child_process_init(&cp); cp.argv = argv; cp.env = local_repo_env; cp.no_stdin = 1; diff --git a/transport-helper.c b/transport-helper.c index 6cd9dd1f9..0224687a2 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -414,7 +414,7 @@ static int get_exporter(struct transport *transport, struct child_process *helper = get_helper(transport); int i; - memset(fastexport, 0, sizeof(*fastexport)); + child_process_init(fastexport); /* we need to duplicate helper->in because we want to use it after * fastexport is done with it. */ From 5e626b91d4a5d2cfee8747facd53d7661f1f9112 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 30 Oct 2014 10:45:41 -0700 Subject: [PATCH 2/4] bundle: split out a helper function to create pack data The create_bundle() function, while it does one single logical thing, takes a rather large implementation to do so. Let's start separating what it does into smaller steps to make it easier to see what is going on. This is a first step to separate out the actual pack-data generation, after the earlier part of the function figures out which part of the history to place in the bundle. Signed-off-by: Junio C Hamano --- bundle.c | 64 ++++++++++++++++++++++++++++++++------------------------ 1 file changed, 37 insertions(+), 27 deletions(-) diff --git a/bundle.c b/bundle.c index c846092e9..9c87532ff 100644 --- a/bundle.c +++ b/bundle.c @@ -235,6 +235,41 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs) return result; } +static int write_pack_data(int bundle_fd, struct lock_file *lock, struct rev_info *revs) +{ + struct child_process pack_objects = CHILD_PROCESS_INIT; + int i; + + argv_array_pushl(&pack_objects.args, + "pack-objects", "--all-progress-implied", + "--stdout", "--thin", "--delta-base-offset", + NULL); + pack_objects.in = -1; + pack_objects.out = bundle_fd; + pack_objects.git_cmd = 1; + if (start_command(&pack_objects)) + return error(_("Could not spawn pack-objects")); + + /* + * start_command closed bundle_fd if it was > 1 + * so set the lock fd to -1 so commit_lock_file() + * won't fail trying to close it. + */ + lock->fd = -1; + + for (i = 0; i < revs->pending.nr; i++) { + struct object *object = revs->pending.objects[i].item; + if (object->flags & UNINTERESTING) + write_or_die(pack_objects.in, "^", 1); + write_or_die(pack_objects.in, sha1_to_hex(object->sha1), 40); + write_or_die(pack_objects.in, "\n", 1); + } + close(pack_objects.in); + if (finish_command(&pack_objects)) + return error(_("pack-objects died")); + return 0; +} + int create_bundle(struct bundle_header *header, const char *path, int argc, const char **argv) { @@ -381,34 +416,9 @@ int create_bundle(struct bundle_header *header, const char *path, write_or_die(bundle_fd, "\n", 1); /* write pack */ - child_process_init(&rls); - argv_array_pushl(&rls.args, - "pack-objects", "--all-progress-implied", - "--stdout", "--thin", "--delta-base-offset", - NULL); - rls.in = -1; - rls.out = bundle_fd; - rls.git_cmd = 1; - if (start_command(&rls)) - return error(_("Could not spawn pack-objects")); - - /* - * start_command closed bundle_fd if it was > 1 - * so set the lock fd to -1 so commit_lock_file() - * won't fail trying to close it. - */ - lock.fd = -1; + if (write_pack_data(bundle_fd, &lock, &revs)) + return -1; - for (i = 0; i < revs.pending.nr; i++) { - struct object *object = revs.pending.objects[i].item; - if (object->flags & UNINTERESTING) - write_or_die(rls.in, "^", 1); - write_or_die(rls.in, sha1_to_hex(object->sha1), 40); - write_or_die(rls.in, "\n", 1); - } - close(rls.in); - if (finish_command(&rls)) - return error(_("pack-objects died")); if (!bundle_to_stdout) { if (commit_lock_file(&lock)) die_errno(_("cannot create '%s'"), path); From e8eb25122e2cf966bf774fd0e3596d1b3dc93be6 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 30 Oct 2014 11:01:37 -0700 Subject: [PATCH 3/4] bundle: split out a helper function to compute and write prerequisites The new helper compute_and_write_prerequistes() is ugly, but it cannot be avoided. Ideally we should avoid a function that computes and does I/O at the same time, but the prerequisites lines in the output needs the human readable title only to help the recipient of the bundle. The code copies them straight from the rev-list output and immediately discards as no other internal computation needs that information. Signed-off-by: Junio C Hamano --- bundle.c | 59 +++++++++++++++++++++++++++++++++----------------------- 1 file changed, 35 insertions(+), 24 deletions(-) diff --git a/bundle.c b/bundle.c index 9c87532ff..558503550 100644 --- a/bundle.c +++ b/bundle.c @@ -270,33 +270,15 @@ static int write_pack_data(int bundle_fd, struct lock_file *lock, struct rev_inf return 0; } -int create_bundle(struct bundle_header *header, const char *path, - int argc, const char **argv) +static int compute_and_write_prerequisites(int bundle_fd, + struct rev_info *revs, + int argc, const char **argv) { - static struct lock_file lock; - int bundle_fd = -1; - int bundle_to_stdout; - int i, ref_count = 0; - struct strbuf buf = STRBUF_INIT; - struct rev_info revs; struct child_process rls = CHILD_PROCESS_INIT; + struct strbuf buf = STRBUF_INIT; FILE *rls_fout; + int i; - bundle_to_stdout = !strcmp(path, "-"); - if (bundle_to_stdout) - bundle_fd = 1; - else - bundle_fd = hold_lock_file_for_update(&lock, path, - LOCK_DIE_ON_ERROR); - - /* write signature */ - write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature)); - - /* init revs to list objects for pack-objects later */ - save_commit_buffer = 0; - init_revisions(&revs, NULL); - - /* write prerequisites */ argv_array_pushl(&rls.args, "rev-list", "--boundary", "--pretty=oneline", NULL); @@ -314,7 +296,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, 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); @@ -325,6 +307,35 @@ int create_bundle(struct bundle_header *header, const char *path, fclose(rls_fout); if (finish_command(&rls)) return error(_("rev-list died")); + return 0; +} + +int create_bundle(struct bundle_header *header, const char *path, + int argc, const char **argv) +{ + static struct lock_file lock; + int bundle_fd = -1; + int bundle_to_stdout; + int i, ref_count = 0; + struct rev_info revs; + + bundle_to_stdout = !strcmp(path, "-"); + if (bundle_to_stdout) + bundle_fd = 1; + else + bundle_fd = hold_lock_file_for_update(&lock, path, + LOCK_DIE_ON_ERROR); + + /* write signature */ + write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature)); + + /* init revs to list objects for pack-objects later */ + save_commit_buffer = 0; + init_revisions(&revs, NULL); + + /* write prerequisites */ + if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv)) + return -1; /* write references */ argc = setup_revisions(argc, argv, &revs, NULL); From d9362ef9b91ea2dccc80553dbcb37dc6c5d78548 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 30 Oct 2014 17:35:24 -0400 Subject: [PATCH 4/4] bundle: split out ref writing from bundle_create The bundle_create() function has a number of logical steps: process the input, write the refs, and write the packfile. Recent commits split the first and third into separate sub-functions. It's worth splitting the middle step out, too, if only because it makes the progression of the steps more obvious. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- bundle.c | 97 +++++++++++++++++++++++++++++++++----------------------- 1 file changed, 58 insertions(+), 39 deletions(-) diff --git a/bundle.c b/bundle.c index 558503550..2e2dbd50e 100644 --- a/bundle.c +++ b/bundle.c @@ -310,43 +310,22 @@ static int compute_and_write_prerequisites(int bundle_fd, return 0; } -int create_bundle(struct bundle_header *header, const char *path, - int argc, const char **argv) +/* + * Write out bundle refs based on the tips already + * parsed into revs.pending. As a side effect, may + * manipulate revs.pending to include additional + * necessary objects (like tags). + * + * Returns the number of refs written, or negative + * on error. + */ +static int write_bundle_refs(int bundle_fd, struct rev_info *revs) { - static struct lock_file lock; - int bundle_fd = -1; - int bundle_to_stdout; - int i, ref_count = 0; - struct rev_info revs; - - bundle_to_stdout = !strcmp(path, "-"); - if (bundle_to_stdout) - bundle_fd = 1; - else - bundle_fd = hold_lock_file_for_update(&lock, path, - LOCK_DIE_ON_ERROR); - - /* write signature */ - write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature)); - - /* init revs to list objects for pack-objects later */ - save_commit_buffer = 0; - init_revisions(&revs, NULL); - - /* write prerequisites */ - if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv)) - return -1; - - /* write references */ - argc = setup_revisions(argc, argv, &revs, NULL); - - if (argc > 1) - return error(_("unrecognized argument: %s"), argv[1]); - - object_array_remove_duplicates(&revs.pending); + int i; + int ref_count = 0; - for (i = 0; i < revs.pending.nr; i++) { - struct object_array_entry *e = revs.pending.objects + i; + for (i = 0; i < revs->pending.nr; i++) { + struct object_array_entry *e = revs->pending.objects + i; unsigned char sha1[20]; char *ref; const char *display_ref; @@ -361,7 +340,7 @@ int create_bundle(struct bundle_header *header, const char *path, display_ref = (flag & REF_ISSYMREF) ? e->name : ref; if (e->item->type == OBJ_TAG && - !is_tag_in_date_range(e->item, &revs)) { + !is_tag_in_date_range(e->item, revs)) { e->item->flags |= UNINTERESTING; continue; } @@ -407,7 +386,7 @@ int create_bundle(struct bundle_header *header, const char *path, */ obj = parse_object_or_die(sha1, e->name); obj->flags |= SHOWN; - add_pending_object(&revs, obj, e->name); + add_pending_object(revs, obj, e->name); } free(ref); continue; @@ -420,11 +399,51 @@ int create_bundle(struct bundle_header *header, const char *path, write_or_die(bundle_fd, "\n", 1); free(ref); } - if (!ref_count) - die(_("Refusing to create empty bundle.")); /* end header */ write_or_die(bundle_fd, "\n", 1); + return ref_count; +} + +int create_bundle(struct bundle_header *header, const char *path, + int argc, const char **argv) +{ + static struct lock_file lock; + int bundle_fd = -1; + int bundle_to_stdout; + int ref_count = 0; + struct rev_info revs; + + bundle_to_stdout = !strcmp(path, "-"); + if (bundle_to_stdout) + bundle_fd = 1; + else + bundle_fd = hold_lock_file_for_update(&lock, path, + LOCK_DIE_ON_ERROR); + + /* write signature */ + write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature)); + + /* init revs to list objects for pack-objects later */ + save_commit_buffer = 0; + init_revisions(&revs, NULL); + + /* write prerequisites */ + if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv)) + return -1; + + argc = setup_revisions(argc, argv, &revs, NULL); + + if (argc > 1) + return error(_("unrecognized argument: %s"), argv[1]); + + object_array_remove_duplicates(&revs.pending); + + ref_count = write_bundle_refs(bundle_fd, &revs); + if (!ref_count) + die(_("Refusing to create empty bundle.")); + else if (ref_count < 0) + return -1; /* write pack */ if (write_pack_data(bundle_fd, &lock, &revs))