Merge branch 'bp/checkout-new-branch-optim'
authorJunio C Hamano <gitster@pobox.com>
Tue, 5 Feb 2019 22:26:17 +0000 (14:26 -0800)
committerJunio C Hamano <gitster@pobox.com>
Tue, 5 Feb 2019 22:26:17 +0000 (14:26 -0800)
"git checkout -b <new> [HEAD]" to create a new branch from the
current commit and check it out ought to be a no-op in the index
and the working tree in normal cases, but there are corner cases
that do require updates to the index and the working tree. Running
it immediately after "git clone --no-checkout" is one of these
cases that an earlier optimization kicked in incorrectly, which has
been fixed.

* bp/checkout-new-branch-optim:
checkout: fix regression in checkout -b on intitial checkout
checkout: add test demonstrating regression with checkout -b on initial commit

1  2 
builtin/checkout.c
diff --combined builtin/checkout.c
index 6fadf412e830ba4aeaa8d2940fd9a7aab9bd8fa5,99b873d3cd4075d59cd4133ca1de021812bc2990..9f8f3466f66d8845a60719cc608375b71679d4b4
@@@ -23,7 -23,6 +23,7 @@@
  #include "resolve-undo.h"
  #include "submodule-config.h"
  #include "submodule.h"
 +#include "advice.h"
  
  static int checkout_optimize_new_branch;
  
@@@ -44,7 -43,6 +44,7 @@@ struct checkout_opts 
        int ignore_skipworktree;
        int ignore_other_worktrees;
        int show_progress;
 +      int count_checkout_paths;
        /*
         * If new checkout options are added, skip_merge_working_tree
         * should be updated accordingly.
@@@ -86,7 -84,7 +86,7 @@@ static int update_some(const struct obj
                return READ_TREE_RECURSIVE;
  
        len = base->len + strlen(pathname);
 -      ce = xcalloc(1, cache_entry_size(len));
 +      ce = make_empty_cache_entry(&the_index, len);
        oidcpy(&ce->oid, oid);
        memcpy(ce->name, base->buf, base->len);
        memcpy(ce->name + base->len, pathname, len - base->len);
        if (pos >= 0) {
                struct cache_entry *old = active_cache[pos];
                if (ce->ce_mode == old->ce_mode &&
 -                  !oidcmp(&ce->oid, &old->oid)) {
 +                  oideq(&ce->oid, &old->oid)) {
                        old->ce_flags |= CE_UPDATE;
 -                      free(ce);
 +                      discard_cache_entry(ce);
                        return 0;
                }
        }
  
  static int read_tree_some(struct tree *tree, const struct pathspec *pathspec)
  {
 -      read_tree_recursive(tree, "", 0, 0, pathspec, update_some, NULL);
 +      read_tree_recursive(the_repository, tree, "", 0, 0,
 +                          pathspec, update_some, NULL);
  
        /* update the index with the given tree's info
         * for all args, expanding wildcards, and exit
@@@ -167,13 -164,12 +167,13 @@@ static int check_stages(unsigned stages
  }
  
  static int checkout_stage(int stage, const struct cache_entry *ce, int pos,
 -                        const struct checkout *state)
 +                        const struct checkout *state, int *nr_checkouts)
  {
        while (pos < active_nr &&
               !strcmp(active_cache[pos]->name, ce->name)) {
                if (ce_stage(active_cache[pos]) == stage)
 -                      return checkout_entry(active_cache[pos], state, NULL);
 +                      return checkout_entry(active_cache[pos], state,
 +                                            NULL, nr_checkouts);
                pos++;
        }
        if (stage == 2)
                return error(_("path '%s' does not have their version"), ce->name);
  }
  
 -static int checkout_merged(int pos, const struct checkout *state)
 +static int checkout_merged(int pos, const struct checkout *state, int *nr_checkouts)
  {
        struct cache_entry *ce = active_cache[pos];
        const char *path = ce->name;
         * merge.renormalize set, too
         */
        status = ll_merge(&result_buf, path, &ancestor, "base",
 -                        &ours, "ours", &theirs, "theirs", NULL);
 +                        &ours, "ours", &theirs, "theirs",
 +                        state->istate, NULL);
        free(ancestor.ptr);
        free(ours.ptr);
        free(theirs.ptr);
        if (write_object_file(result_buf.ptr, result_buf.size, blob_type, &oid))
                die(_("Unable to add merge result for '%s'"), path);
        free(result_buf.ptr);
 -      ce = make_cache_entry(mode, oid.hash, path, 2, 0);
 +      ce = make_transient_cache_entry(mode, &oid, path, 2);
        if (!ce)
                die(_("make_cache_entry failed for path '%s'"), path);
 -      status = checkout_entry(ce, state, NULL);
 -      free(ce);
 +      status = checkout_entry(ce, state, NULL, nr_checkouts);
 +      discard_cache_entry(ce);
        return status;
  }
  
@@@ -260,7 -255,6 +260,7 @@@ static int checkout_paths(const struct 
        struct commit *head;
        int errs = 0;
        struct lock_file lock_file = LOCK_INIT;
 +      int nr_checkouts = 0;
  
        if (opts->track != BRANCH_TRACK_UNSPECIFIED)
                die(_("'%s' cannot be used with updating paths"), "--track");
                 * match_pathspec() for _all_ entries when
                 * opts->source_tree != NULL.
                 */
 -              if (ce_path_match(ce, &opts->pathspec, ps_matched))
 +              if (ce_path_match(&the_index, ce, &opts->pathspec, ps_matched))
                        ce->ce_flags |= CE_MATCHED;
        }
  
                struct cache_entry *ce = active_cache[pos];
                if (ce->ce_flags & CE_MATCHED) {
                        if (!ce_stage(ce)) {
 -                              errs |= checkout_entry(ce, &state, NULL);
 +                              errs |= checkout_entry(ce, &state,
 +                                                     NULL, &nr_checkouts);
                                continue;
                        }
                        if (opts->writeout_stage)
 -                              errs |= checkout_stage(opts->writeout_stage, ce, pos, &state);
 +                              errs |= checkout_stage(opts->writeout_stage,
 +                                                     ce, pos,
 +                                                     &state, &nr_checkouts);
                        else if (opts->merge)
 -                              errs |= checkout_merged(pos, &state);
 +                              errs |= checkout_merged(pos, &state,
 +                                                      &nr_checkouts);
                        pos = skip_same_name(ce, pos) - 1;
                }
        }
 -      errs |= finish_delayed_checkout(&state);
 +      errs |= finish_delayed_checkout(&state, &nr_checkouts);
 +
 +      if (opts->count_checkout_paths) {
 +              if (opts->source_tree)
 +                      fprintf_ln(stderr, Q_("Checked out %d path out of %s",
 +                                            "Checked out %d paths out of %s",
 +                                            nr_checkouts),
 +                                 nr_checkouts,
 +                                 find_unique_abbrev(&opts->source_tree->object.oid,
 +                                                    DEFAULT_ABBREV));
 +              else
 +                      fprintf_ln(stderr, Q_("Checked out %d path out of the index",
 +                                            "Checked out %d paths out of the index",
 +                                            nr_checkouts),
 +                                 nr_checkouts);
 +      }
  
        if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
                die(_("unable to write new index file"));
  
        read_ref_full("HEAD", 0, &rev, NULL);
 -      head = lookup_commit_reference_gently(&rev, 1);
 +      head = lookup_commit_reference_gently(the_repository, &rev, 1);
  
        errs |= post_checkout_hook(head, head, 0);
        return errs;
@@@ -421,7 -396,7 +421,7 @@@ static void show_local_changes(struct o
  {
        struct rev_info rev;
        /* I think we want full paths, even if we're in a subdirectory. */
 -      init_revisions(&rev, NULL);
 +      repo_init_revisions(the_repository, &rev, NULL);
        rev.diffopt.flags = opts->flags;
        rev.diffopt.output_format |= DIFF_FORMAT_NAME_STATUS;
        diff_setup_done(&rev.diffopt);
@@@ -521,8 -496,7 +521,8 @@@ static int skip_merge_working_tree(cons
         * We must do the merge if we are actually moving to a new commit.
         */
        if (!old_branch_info->commit || !new_branch_info->commit ||
 -              oidcmp(&old_branch_info->commit->object.oid, &new_branch_info->commit->object.oid))
 +              !oideq(&old_branch_info->commit->object.oid,
 +                     &new_branch_info->commit->object.oid))
                return 0;
  
        /*
         * Remaining variables are not checkout options but used to track state
         */
  
+        /*
+         * Do the merge if this is the initial checkout. We cannot use
+         * is_cache_unborn() here because the index hasn't been loaded yet
+         * so cache_nr and timestamp.sec are always zero.
+         */
+       if (!file_exists(get_index_file()))
+               return 0;
        return 1;
  }
  
@@@ -776,8 -758,7 +784,8 @@@ static void update_refs_for_switch(cons
                        free(refname);
                }
                else
 -                      create_branch(opts->new_branch, new_branch_info->name,
 +                      create_branch(the_repository,
 +                                    opts->new_branch, new_branch_info->name,
                                      opts->new_branch_force ? 1 : 0,
                                      opts->new_branch_force ? 1 : 0,
                                      opts->new_branch_log,
                                delete_reflog(old_branch_info->path);
                }
        }
 -      remove_branch_state();
 +      remove_branch_state(the_repository);
        strbuf_release(&msg);
        if (!opts->quiet &&
            (new_branch_info->path || (!opts->force_detach && !strcmp(new_branch_info->name, "HEAD"))))
@@@ -924,7 -905,7 +932,7 @@@ static void orphaned_commit_warning(str
        struct rev_info revs;
        struct object *object = &old_commit->object;
  
 -      init_revisions(&revs, NULL);
 +      repo_init_revisions(the_repository, &revs, NULL);
        setup_revisions(0, NULL, &revs, NULL);
  
        object->flags &= ~UNINTERESTING;
@@@ -955,7 -936,7 +963,7 @@@ static int switch_branches(const struc
        memset(&old_branch_info, 0, sizeof(old_branch_info));
        old_branch_info.path = path_to_free = resolve_refdup("HEAD", 0, &rev, &flag);
        if (old_branch_info.path)
 -              old_branch_info.commit = lookup_commit_reference_gently(&rev, 1);
 +              old_branch_info.commit = lookup_commit_reference_gently(the_repository, &rev, 1);
        if (!(flag & REF_ISSYMREF))
                old_branch_info.path = NULL;
  
@@@ -1018,8 -999,7 +1026,8 @@@ static int parse_branchname_arg(int arg
                                int dwim_new_local_branch_ok,
                                struct branch_info *new_branch_info,
                                struct checkout_opts *opts,
 -                              struct object_id *rev)
 +                              struct object_id *rev,
 +                              int *dwim_remotes_matched)
  {
        struct tree **source_tree = &opts->source_tree;
        const char **new_branch = &opts->new_branch;
         *   (b) If <something> is _not_ a commit, either "--" is present
         *       or <something> is not a path, no -t or -b was given, and
         *       and there is a tracking branch whose name is <something>
 -       *       in one and only one remote, then this is a short-hand to
 -       *       fork local <something> from that remote-tracking branch.
 +       *       in one and only one remote (or if the branch exists on the
 +       *       remote named in checkout.defaultRemote), then this is a
 +       *       short-hand to fork local <something> from that
 +       *       remote-tracking branch.
         *
         *   (c) Otherwise, if "--" is present, treat it like case (1).
         *
                has_dash_dash = 1; /* case (3) or (1) */
        else if (dash_dash_pos >= 2)
                die(_("only one reference expected, %d given."), dash_dash_pos);
 +      opts->count_checkout_paths = !opts->quiet && !has_dash_dash;
  
        if (!strcmp(arg, "-"))
                arg = "@{-1}";
                 */
                int recover_with_dwim = dwim_new_local_branch_ok;
  
 -              if (!has_dash_dash &&
 -                  (check_filename(opts->prefix, arg) || !no_wildcard(arg)))
 +              int could_be_checkout_paths = !has_dash_dash &&
 +                      check_filename(opts->prefix, arg);
 +
 +              if (!has_dash_dash && !no_wildcard(arg))
                        recover_with_dwim = 0;
 +
                /*
                 * Accept "git checkout foo" and "git checkout foo --"
                 * as candidates for dwim.
                        recover_with_dwim = 0;
  
                if (recover_with_dwim) {
 -                      const char *remote = unique_tracking_name(arg, rev);
 +                      const char *remote = unique_tracking_name(arg, rev,
 +                                                                dwim_remotes_matched);
                        if (remote) {
 +                              if (could_be_checkout_paths)
 +                                      die(_("'%s' could be both a local file and a tracking branch.\n"
 +                                            "Please use -- (and optionally --no-guess) to disambiguate"),
 +                                          arg);
                                *new_branch = arg;
                                arg = remote;
                                /* DWIMmed to create local branch, case (3).(b) */
        else
                new_branch_info->path = NULL; /* not an existing branch */
  
 -      new_branch_info->commit = lookup_commit_reference_gently(rev, 1);
 +      new_branch_info->commit = lookup_commit_reference_gently(the_repository, rev, 1);
        if (!new_branch_info->commit) {
                /* not a commit */
                *source_tree = parse_tree_indirect(rev);
@@@ -1260,8 -1229,7 +1268,8 @@@ int cmd_checkout(int argc, const char *
        struct checkout_opts opts;
        struct branch_info new_branch_info;
        char *conflict_style = NULL;
 -      int dwim_new_local_branch = 1;
 +      int dwim_new_local_branch, no_dwim_new_local_branch = 0;
 +      int dwim_remotes_matched = 0;
        struct option options[] = {
                OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
                OPT_STRING('b', NULL, &opts.new_branch, N_("branch"),
                OPT_BOOL('p', "patch", &opts.patch_mode, N_("select hunks interactively")),
                OPT_BOOL(0, "ignore-skip-worktree-bits", &opts.ignore_skipworktree,
                         N_("do not limit pathspecs to sparse entries only")),
 -              OPT_HIDDEN_BOOL(0, "guess", &dwim_new_local_branch,
 -                              N_("second guess 'git checkout <no-such-branch>'")),
 +              OPT_BOOL(0, "no-guess", &no_dwim_new_local_branch,
 +                       N_("do not second guess 'git checkout <no-such-branch>'")),
                OPT_BOOL(0, "ignore-other-worktrees", &opts.ignore_other_worktrees,
                         N_("do not check if another worktree is holding the given ref")),
                { OPTION_CALLBACK, 0, "recurse-submodules", NULL,
        argc = parse_options(argc, argv, prefix, options, checkout_usage,
                             PARSE_OPT_KEEP_DASHDASH);
  
 +      dwim_new_local_branch = !no_dwim_new_local_branch;
        if (opts.show_progress < 0) {
                if (opts.quiet)
                        opts.show_progress = 0;
        if (opts.track != BRANCH_TRACK_UNSPECIFIED && !opts.new_branch) {
                const char *argv0 = argv[0];
                if (!argc || !strcmp(argv0, "--"))
 -                      die (_("--track needs a branch name"));
 +                      die(_("--track needs a branch name"));
                skip_prefix(argv0, "refs/", &argv0);
                skip_prefix(argv0, "remotes/", &argv0);
                argv0 = strchr(argv0, '/');
                if (!argv0 || !argv0[1])
 -                      die (_("Missing branch name; try -b"));
 +                      die(_("missing branch name; try -b"));
                opts.new_branch = argv0 + 1;
        }
  
                        opts.track == BRANCH_TRACK_UNSPECIFIED &&
                        !opts.new_branch;
                int n = parse_branchname_arg(argc, argv, dwim_ok,
 -                                           &new_branch_info, &opts, &rev);
 +                                           &new_branch_info, &opts, &rev,
 +                                           &dwim_remotes_matched);
                argv += n;
                argc -= n;
        }
        }
  
        UNLEAK(opts);
 -      if (opts.patch_mode || opts.pathspec.nr)
 -              return checkout_paths(&opts, new_branch_info.name);
 -      else
 +      if (opts.patch_mode || opts.pathspec.nr) {
 +              int ret = checkout_paths(&opts, new_branch_info.name);
 +              if (ret && dwim_remotes_matched > 1 &&
 +                  advice_checkout_ambiguous_remote_branch_name)
 +                      advise(_("'%s' matched more than one remote tracking branch.\n"
 +                               "We found %d remotes with a reference that matched. So we fell back\n"
 +                               "on trying to resolve the argument as a path, but failed there too!\n"
 +                               "\n"
 +                               "If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
 +                               "you can do so by fully qualifying the name with the --track option:\n"
 +                               "\n"
 +                               "    git checkout --track origin/<name>\n"
 +                               "\n"
 +                               "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
 +                               "one remote, e.g. the 'origin' remote, consider setting\n"
 +                               "checkout.defaultRemote=origin in your config."),
 +                             argv[0],
 +                             dwim_remotes_matched);
 +              return ret;
 +      } else {
                return checkout_branch(&opts, &new_branch_info);
 +      }
  }