Merge branch 'hv/submodule-not-yet-pushed-fix' into maint
authorJunio C Hamano <gitster@pobox.com>
Tue, 17 Jan 2017 22:49:26 +0000 (14:49 -0800)
committerJunio C Hamano <gitster@pobox.com>
Tue, 17 Jan 2017 22:49:26 +0000 (14:49 -0800)
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

1  2 
submodule.c
transport.c
diff --combined submodule.c
index 6f7d883de950af8ba6427537d09e021a4bed36bd,b5094881823c9abf33625ff0b396cbcb6114e7da..00dd655a5339b8e16c5b631d11c8072359430cb7
@@@ -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)
                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 <commits> --not --remotes -n 1' command in submodule %s",
+                                       path);
                if (strbuf_read(&buf, cp.out, 41))
                        needs_pushing = 1;
                finish_command(&cp);
        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 d57e8dec28d6890ce981a8422e9b8b60b697ecd6,c3fdd5d251319cd8eaa6d3680c6c626df590e543..f48286905786a9492edd82adc3b633411dbce152
@@@ -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)
                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
        }
  }
  
 -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;
                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);
  
        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)
  {
        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] == '/')