From: Junio C Hamano Date: Tue, 17 Jan 2017 22:49:26 +0000 (-0800) Subject: Merge branch 'hv/submodule-not-yet-pushed-fix' into maint X-Git-Tag: v2.11.1~61 X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/9da9965ba6672dc0016a5ac694271bbdd4589e15?hp=-c Merge branch 'hv/submodule-not-yet-pushed-fix' into maint The code in "git push" to compute if any commit being pushed in the superproject binds a commit in a submodule that hasn't been pushed out was overly inefficient, making it unusable even for a small project that does not have any submodule but have a reasonable number of refs. * hv/submodule-not-yet-pushed-fix: submodule_needs_pushing(): explain the behaviour when we cannot answer batch check whether submodule needs pushing into one call serialize collection of refs that contain submodule changes serialize collection of changed submodules --- 9da9965ba6672dc0016a5ac694271bbdd4589e15 diff --combined submodule.c index 6f7d883de9,b509488182..00dd655a53 --- a/submodule.c +++ b/submodule.c @@@ -123,7 -123,9 +123,7 @@@ void stage_updated_gitmodules(void static int add_submodule_odb(const char *path) { struct strbuf objects_directory = STRBUF_INIT; - struct alternate_object_database *alt_odb; int ret = 0; - size_t alloc; ret = strbuf_git_path_submodule(&objects_directory, path, "objects/"); if (ret) @@@ -132,7 -134,26 +132,7 @@@ ret = -1; goto done; } - /* avoid adding it twice */ - prepare_alt_odb(); - for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next) - if (alt_odb->name - alt_odb->base == objects_directory.len && - !strncmp(alt_odb->base, objects_directory.buf, - objects_directory.len)) - goto done; - - alloc = st_add(objects_directory.len, 42); /* for "12/345..." sha1 */ - alt_odb = xmalloc(st_add(sizeof(*alt_odb), alloc)); - alt_odb->next = alt_odb_list; - xsnprintf(alt_odb->base, alloc, "%s", objects_directory.buf); - alt_odb->name = alt_odb->base + objects_directory.len; - alt_odb->name[2] = '/'; - alt_odb->name[40] = '\0'; - alt_odb->name[41] = '\0'; - alt_odb_list = alt_odb; - - /* add possible alternates from the submodule */ - read_info_alternates(objects_directory.buf, 0); + add_to_alternates_memory(objects_directory.buf); done: strbuf_release(&objects_directory); return ret; @@@ -371,9 -392,10 +371,9 @@@ static void show_submodule_header(FILE } output_header: - strbuf_addf(&sb, "%s%sSubmodule %s %s..", line_prefix, meta, path, - find_unique_abbrev(one->hash, DEFAULT_ABBREV)); - if (!fast_backward && !fast_forward) - strbuf_addch(&sb, '.'); + strbuf_addf(&sb, "%s%sSubmodule %s ", line_prefix, meta, path); + strbuf_add_unique_abbrev(&sb, one->hash, DEFAULT_ABBREV); + strbuf_addstr(&sb, (fast_backward || fast_forward) ? ".." : "..."); strbuf_add_unique_abbrev(&sb, two->hash, DEFAULT_ABBREV); if (message) strbuf_addf(&sb, " %s%s\n", message, reset); @@@ -500,27 -522,67 +500,67 @@@ static int has_remote(const char *refna return 1; } - static int submodule_needs_pushing(const char *path, const unsigned char sha1[20]) + static int append_sha1_to_argv(const unsigned char sha1[20], void *data) { - if (add_submodule_odb(path) || !lookup_commit_reference(sha1)) + struct argv_array *argv = data; + argv_array_push(argv, sha1_to_hex(sha1)); + return 0; + } + + static int check_has_commit(const unsigned char sha1[20], void *data) + { + int *has_commit = data; + + if (!lookup_commit_reference(sha1)) + *has_commit = 0; + + return 0; + } + + static int submodule_has_commits(const char *path, struct sha1_array *commits) + { + int has_commit = 1; + + if (add_submodule_odb(path)) + return 0; + + sha1_array_for_each_unique(commits, check_has_commit, &has_commit); + return has_commit; + } + + static int submodule_needs_pushing(const char *path, struct sha1_array *commits) + { + if (!submodule_has_commits(path, commits)) + /* + * NOTE: We do consider it safe to return "no" here. The + * correct answer would be "We do not know" instead of + * "No push needed", but it is quite hard to change + * the submodule pointer without having the submodule + * around. If a user did however change the submodules + * without having the submodule around, this indicates + * an expert who knows what they are doing or a + * maintainer integrating work from other people. In + * both cases it should be safe to skip this check. + */ return 0; if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) { struct child_process cp = CHILD_PROCESS_INIT; - const char *argv[] = {"rev-list", NULL, "--not", "--remotes", "-n", "1" , NULL}; struct strbuf buf = STRBUF_INIT; int needs_pushing = 0; - argv[1] = sha1_to_hex(sha1); - cp.argv = argv; + argv_array_push(&cp.args, "rev-list"); + sha1_array_for_each_unique(commits, append_sha1_to_argv, &cp.args); + argv_array_pushl(&cp.args, "--not", "--remotes", "-n", "1" , NULL); + prepare_submodule_repo_env(&cp.env_array); cp.git_cmd = 1; cp.no_stdin = 1; cp.out = -1; cp.dir = path; if (start_command(&cp)) - die("Could not run 'git rev-list %s --not --remotes -n 1' command in submodule %s", - sha1_to_hex(sha1), path); + die("Could not run 'git rev-list --not --remotes -n 1' command in submodule %s", + path); if (strbuf_read(&buf, cp.out, 41)) needs_pushing = 1; finish_command(&cp); @@@ -532,19 -594,34 +572,34 @@@ return 0; } + static struct sha1_array *submodule_commits(struct string_list *submodules, + const char *path) + { + struct string_list_item *item; + + item = string_list_insert(submodules, path); + if (item->util) + return (struct sha1_array *) item->util; + + /* NEEDSWORK: should we have sha1_array_init()? */ + item->util = xcalloc(1, sizeof(struct sha1_array)); + return (struct sha1_array *) item->util; + } + static void collect_submodules_from_diff(struct diff_queue_struct *q, struct diff_options *options, void *data) { int i; - struct string_list *needs_pushing = data; + struct string_list *submodules = data; for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; + struct sha1_array *commits; if (!S_ISGITLINK(p->two->mode)) continue; - if (submodule_needs_pushing(p->two->path, p->two->oid.hash)) - string_list_insert(needs_pushing, p->two->path); + commits = submodule_commits(submodules, p->two->path); + sha1_array_append(commits, p->two->oid.hash); } } @@@ -560,32 -637,48 +615,48 @@@ static void find_unpushed_submodule_com diff_tree_combined_merge(commit, 1, &rev); } - int find_unpushed_submodules(unsigned char new_sha1[20], + static void free_submodules_sha1s(struct string_list *submodules) + { + struct string_list_item *item; + for_each_string_list_item(item, submodules) + sha1_array_clear((struct sha1_array *) item->util); + string_list_clear(submodules, 1); + } + + int find_unpushed_submodules(struct sha1_array *commits, const char *remotes_name, struct string_list *needs_pushing) { struct rev_info rev; struct commit *commit; - const char *argv[] = {NULL, NULL, "--not", "NULL", NULL}; - int argc = ARRAY_SIZE(argv) - 1; - char *sha1_copy; + struct string_list submodules = STRING_LIST_INIT_DUP; + struct string_list_item *submodule; + struct argv_array argv = ARGV_ARRAY_INIT; - struct strbuf remotes_arg = STRBUF_INIT; - - strbuf_addf(&remotes_arg, "--remotes=%s", remotes_name); init_revisions(&rev, NULL); - sha1_copy = xstrdup(sha1_to_hex(new_sha1)); - argv[1] = sha1_copy; - argv[3] = remotes_arg.buf; - setup_revisions(argc, argv, &rev, NULL); + + /* argv.argv[0] will be ignored by setup_revisions */ + argv_array_push(&argv, "find_unpushed_submodules"); + sha1_array_for_each_unique(commits, append_sha1_to_argv, &argv); + argv_array_push(&argv, "--not"); + argv_array_pushf(&argv, "--remotes=%s", remotes_name); + + setup_revisions(argv.argc, argv.argv, &rev, NULL); if (prepare_revision_walk(&rev)) die("revision walk setup failed"); while ((commit = get_revision(&rev)) != NULL) - find_unpushed_submodule_commits(commit, needs_pushing); + find_unpushed_submodule_commits(commit, &submodules); reset_revision_walk(); - free(sha1_copy); - strbuf_release(&remotes_arg); + argv_array_clear(&argv); + + for_each_string_list_item(submodule, &submodules) { + struct sha1_array *commits = (struct sha1_array *) submodule->util; + + if (submodule_needs_pushing(submodule->string, commits)) + string_list_insert(needs_pushing, submodule->string); + } + free_submodules_sha1s(&submodules); return needs_pushing->nr; } @@@ -612,12 -705,12 +683,12 @@@ static int push_submodule(const char *p return 1; } - int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name) + int push_unpushed_submodules(struct sha1_array *commits, const char *remotes_name) { int i, ret = 1; struct string_list needs_pushing = STRING_LIST_INIT_DUP; - if (!find_unpushed_submodules(new_sha1, remotes_name, &needs_pushing)) + if (!find_unpushed_submodules(commits, remotes_name, &needs_pushing)) return 1; for (i = 0; i < needs_pushing.nr; i++) { diff --combined transport.c index d57e8dec28,c3fdd5d251..f482869057 --- a/transport.c +++ b/transport.c @@@ -307,9 -307,7 +307,9 @@@ void transport_update_tracking_ref(stru } } -static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg, int porcelain) +static void print_ref_status(char flag, const char *summary, + struct ref *to, struct ref *from, const char *msg, + int porcelain, int summary_width) { if (porcelain) { if (from) @@@ -321,7 -319,7 +321,7 @@@ else fprintf(stdout, "%s\n", summary); } else { - fprintf(stderr, " %c %-*s ", flag, TRANSPORT_SUMMARY_WIDTH, summary); + fprintf(stderr, " %c %-*s ", flag, summary_width, summary); if (from) fprintf(stderr, "%s -> %s", prettify_refname(from->name), prettify_refname(to->name)); else @@@ -335,16 -333,15 +335,16 @@@ } } -static void print_ok_ref_status(struct ref *ref, int porcelain) +static void print_ok_ref_status(struct ref *ref, int porcelain, int summary_width) { if (ref->deletion) - print_ref_status('-', "[deleted]", ref, NULL, NULL, porcelain); + print_ref_status('-', "[deleted]", ref, NULL, NULL, + porcelain, summary_width); else if (is_null_oid(&ref->old_oid)) print_ref_status('*', (starts_with(ref->name, "refs/tags/") ? "[new tag]" : "[new branch]"), - ref, ref->peer_ref, NULL, porcelain); + ref, ref->peer_ref, NULL, porcelain, summary_width); else { struct strbuf quickref = STRBUF_INIT; char type; @@@ -364,14 -361,12 +364,14 @@@ strbuf_add_unique_abbrev(&quickref, ref->new_oid.hash, DEFAULT_ABBREV); - print_ref_status(type, quickref.buf, ref, ref->peer_ref, msg, porcelain); + print_ref_status(type, quickref.buf, ref, ref->peer_ref, msg, + porcelain, summary_width); strbuf_release(&quickref); } } -static int print_one_push_status(struct ref *ref, const char *dest, int count, int porcelain) +static int print_one_push_status(struct ref *ref, const char *dest, int count, + int porcelain, int summary_width) { if (!count) { char *url = transport_anonymize_url(dest); @@@ -381,87 -376,62 +381,87 @@@ switch(ref->status) { case REF_STATUS_NONE: - print_ref_status('X', "[no match]", ref, NULL, NULL, porcelain); + print_ref_status('X', "[no match]", ref, NULL, NULL, + porcelain, summary_width); break; case REF_STATUS_REJECT_NODELETE: print_ref_status('!', "[rejected]", ref, NULL, - "remote does not support deleting refs", porcelain); + "remote does not support deleting refs", + porcelain, summary_width); break; case REF_STATUS_UPTODATE: print_ref_status('=', "[up to date]", ref, - ref->peer_ref, NULL, porcelain); + ref->peer_ref, NULL, porcelain, summary_width); break; case REF_STATUS_REJECT_NONFASTFORWARD: print_ref_status('!', "[rejected]", ref, ref->peer_ref, - "non-fast-forward", porcelain); + "non-fast-forward", porcelain, summary_width); break; case REF_STATUS_REJECT_ALREADY_EXISTS: print_ref_status('!', "[rejected]", ref, ref->peer_ref, - "already exists", porcelain); + "already exists", porcelain, summary_width); break; case REF_STATUS_REJECT_FETCH_FIRST: print_ref_status('!', "[rejected]", ref, ref->peer_ref, - "fetch first", porcelain); + "fetch first", porcelain, summary_width); break; case REF_STATUS_REJECT_NEEDS_FORCE: print_ref_status('!', "[rejected]", ref, ref->peer_ref, - "needs force", porcelain); + "needs force", porcelain, summary_width); break; case REF_STATUS_REJECT_STALE: print_ref_status('!', "[rejected]", ref, ref->peer_ref, - "stale info", porcelain); + "stale info", porcelain, summary_width); break; case REF_STATUS_REJECT_SHALLOW: print_ref_status('!', "[rejected]", ref, ref->peer_ref, - "new shallow roots not allowed", porcelain); + "new shallow roots not allowed", + porcelain, summary_width); break; case REF_STATUS_REMOTE_REJECT: print_ref_status('!', "[remote rejected]", ref, - ref->deletion ? NULL : ref->peer_ref, - ref->remote_status, porcelain); + ref->deletion ? NULL : ref->peer_ref, + ref->remote_status, porcelain, summary_width); break; case REF_STATUS_EXPECTING_REPORT: print_ref_status('!', "[remote failure]", ref, - ref->deletion ? NULL : ref->peer_ref, - "remote failed to report status", porcelain); + ref->deletion ? NULL : ref->peer_ref, + "remote failed to report status", + porcelain, summary_width); break; case REF_STATUS_ATOMIC_PUSH_FAILED: print_ref_status('!', "[rejected]", ref, ref->peer_ref, - "atomic push failed", porcelain); + "atomic push failed", porcelain, summary_width); break; case REF_STATUS_OK: - print_ok_ref_status(ref, porcelain); + print_ok_ref_status(ref, porcelain, summary_width); break; } return 1; } +static int measure_abbrev(const struct object_id *oid, int sofar) +{ + char hex[GIT_SHA1_HEXSZ + 1]; + int w = find_unique_abbrev_r(hex, oid->hash, DEFAULT_ABBREV); + + return (w < sofar) ? sofar : w; +} + +int transport_summary_width(const struct ref *refs) +{ + int maxw = -1; + + for (; refs; refs = refs->next) { + maxw = measure_abbrev(&refs->old_oid, maxw); + maxw = measure_abbrev(&refs->new_oid, maxw); + } + if (maxw < 0) + maxw = FALLBACK_DEFAULT_ABBREV; + return (2 * maxw + 3); +} + void transport_print_push_status(const char *dest, struct ref *refs, int verbose, int porcelain, unsigned int *reject_reasons) { @@@ -469,29 -439,25 +469,29 @@@ int n = 0; unsigned char head_sha1[20]; char *head; + int summary_width = transport_summary_width(refs); head = resolve_refdup("HEAD", RESOLVE_REF_READING, head_sha1, NULL); if (verbose) { for (ref = refs; ref; ref = ref->next) if (ref->status == REF_STATUS_UPTODATE) - n += print_one_push_status(ref, dest, n, porcelain); + n += print_one_push_status(ref, dest, n, + porcelain, summary_width); } for (ref = refs; ref; ref = ref->next) if (ref->status == REF_STATUS_OK) - n += print_one_push_status(ref, dest, n, porcelain); + n += print_one_push_status(ref, dest, n, + porcelain, summary_width); *reject_reasons = 0; for (ref = refs; ref; ref = ref->next) { if (ref->status != REF_STATUS_NONE && ref->status != REF_STATUS_UPTODATE && ref->status != REF_STATUS_OK) - n += print_one_push_status(ref, dest, n, porcelain); + n += print_one_push_status(ref, dest, n, + porcelain, summary_width); if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) { if (head != NULL && !strcmp(head, ref->name)) *reject_reasons |= REJECT_NON_FF_HEAD; @@@ -949,23 -915,36 +949,36 @@@ int transport_push(struct transport *tr if ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) { struct ref *ref = remote_refs; + struct sha1_array commits = SHA1_ARRAY_INIT; + for (; ref; ref = ref->next) - if (!is_null_oid(&ref->new_oid) && - !push_unpushed_submodules(ref->new_oid.hash, - transport->remote->name)) - die ("Failed to push all needed submodules!"); + if (!is_null_oid(&ref->new_oid)) + sha1_array_append(&commits, ref->new_oid.hash); + + if (!push_unpushed_submodules(&commits, transport->remote->name)) { + sha1_array_clear(&commits); + die("Failed to push all needed submodules!"); + } + sha1_array_clear(&commits); } if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND | TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) { struct ref *ref = remote_refs; struct string_list needs_pushing = STRING_LIST_INIT_DUP; + struct sha1_array commits = SHA1_ARRAY_INIT; for (; ref; ref = ref->next) - if (!is_null_oid(&ref->new_oid) && - find_unpushed_submodules(ref->new_oid.hash, - transport->remote->name, &needs_pushing)) - die_with_unpushed_submodules(&needs_pushing); + if (!is_null_oid(&ref->new_oid)) + sha1_array_append(&commits, ref->new_oid.hash); + + if (find_unpushed_submodules(&commits, transport->remote->name, + &needs_pushing)) { + sha1_array_clear(&commits); + die_with_unpushed_submodules(&needs_pushing); + } + string_list_clear(&needs_pushing, 0); + sha1_array_clear(&commits); } push_ret = transport->push_refs(transport, remote_refs, flags); @@@ -1130,7 -1109,9 +1143,7 @@@ static int refs_from_alternate_cb(struc const struct ref *extra; struct alternate_refs_data *cb = data; - e->name[-1] = '\0'; - other = xstrdup(real_path(e->base)); - e->name[-1] = '/'; + other = xstrdup(real_path(e->path)); len = strlen(other); while (other[len-1] == '/')