[PATCH v6 0/6] worktree: teach "add" to check out existing branches
- From
Thomas Gummerer <t.gummerer@gmail.com>
- Date
- Mar 31, 2018, 15:17 UTC
- Message-ID
- <20180331151804.30380-1-t.gummerer@gmail.com>
- In-Reply-To
- <20180325134947.25828-1-t.gummerer@gmail.com>
Thanks Eric for the review of the last round.
Previous rounds are at <20180121120208.12760-1-t.gummerer@gmail.com>, <20180204221305.28300-1-t.gummerer@gmail.com>, <20180317220830.30963-1-t.gummerer@gmail.com>, <20180317222219.4940-1-t.gummerer@gmail.com> and 20180325134947.25828-1-t.gummerer@gmail.com.
This round should fix all the UI issues Eric found in the last round.
The changes I made in a bit more detail:
- in addition to removing the 'force_new_branch' flag from 'struct add_opts', also remove the 'new_branch' member, which is local to the 'add()' function. The other four members are needed in the 'worktree_add()' function, so I left them there. This patch is now the first patch in the series.
- added a new commit introducing a new hidden --show-new-head-line flag in 'git reset'. This is used to suppress the "HEAD is now at ..." line that 'git reset --hard' usually prints, so we can replace it with our own "New worktree HEAD is now at ..." line instead, while keeping the progress indicator for larger repositories.
I guess this may be the most controversial bit at this point, as we'd be adding an option for internal use only. Not sure how we feel about that. But short of going back to the old output format, I don't see a good option to make this work otherwise. I tried re-using internal functions for this, but until we have 'struct repository' everywhere, that's going to be quite hard.
- Print the "Creating branch ..." and "Checking out branch ..." messages earlier in the process. This is mainly to avoid the out of order "Branch '...' now set up to track ..." message.
- Various commit message and style cleanups
Note that these fixes are quite differently executed than I had imagined them in the reply to the review comments, as things didn't work as I had imagined. The UI problems should be fixed now nonetheless :)
Some examples of the new UI behaviour here for reference:
- guess-remote mode
$ git worktree add --guess-remote ../next
Creating branch 'next'
Branch 'next' set up to track remote branch 'next' from 'origin'.
New worktree HEAD is now at caa68db14 Merge branch 'sb/packfiles-in-repository' into next- original dwim (create a branch based on the current HEAD)
$ git worktree add ../test
Creating branch 'test'
New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'- new dwim (check out existing branch)
$ git worktree add ../test
Checking out branch 'test'
New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone' - no new branch created
$ git worktree add ../test2 origin/master
New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'- large repository (with original dwim)
$ g worktree add ../test
Creating branch 'test'
Checking out files: 100% (62915/62915), done.
New worktree HEAD is now at c2a9838452a4 Merge tag 'for-4.16/dm-fixes-4' of git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dmCompare this to the old UI (new dwim omitted, as there's no old version of that):
- guess-remote mode
$ git worktree add --guess-remote ../next
Branch 'next' set up to track remote branch 'next' from 'origin'.
Preparing ../next (identifier next)
HEAD is now at caa68db14 Merge branch 'sb/packfiles-in-repository' into next- original dwim (create a branch based on the current HEAD)
$ git worktree add ../test
Preparing ../test (identifier test)
HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'- no new branch created
$ git worktree add ../test2 origin/master
Preparing ../test2 (identifier test2)
HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'- large repository
$ git worktree add ../test
Preparing ../test (identifier test)
Checking out files: 100% (62915/62915), done.
HEAD is now at c2a9838452a4 Merge tag 'for-4.16/dm-fixes-4' of git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dmThe one thing we are loosing is a context line before "Checking out files:", if no new branch is created. Personally I feel like that's acceptable, as the user just used the 'git worktree add' command, so it should be intuitive where those files are being checked out.
We could also print "Preparing worktree <path>" as a line in the beginning (without mentioning the identifier, so we can print it in the 'add()' function), but I don't feel like that's worth spending the extra screen estate. I don't feel strongly about that though, so if someone has a moderately strong preference for that line being there, I'm happy to add it.
Interdiff below:
diff --git a/builtin/reset.c b/builtin/reset.c index e15f595799..54b2576449 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -288,6 +288,7 @@ static int git_reset_config(const char *var, const char *value, void *cb) int cmd_reset(int argc, const char **argv, const char *prefix) { int reset_type = NONE, update_ref_status = 0, quiet = 0; + int show_new_head_line = 1; int patch_mode = 0, unborn; const char *rev; struct object_id oid; @@ -310,6 +311,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) OPT_BOOL('p', "patch", &patch_mode, N_("select hunks interactively")), OPT_BOOL('N', "intent-to-add", &intent_to_add, N_("record only the fact that removed paths will be added later")), + OPT_HIDDEN_BOOL(0, "show-new-head-line", &show_new_head_line, N_("show information on the new HEAD")), OPT_END() }; @@ -403,7 +405,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix) * switched to, saving the previous head in ORIG_HEAD before. */ update_ref_status = reset_refs(rev, &oid); - if (reset_type == HARD && !update_ref_status && !quiet) + if (reset_type == HARD && !update_ref_status && !quiet && + show_new_head_line) print_new_head_line(lookup_commit_reference(&oid)); } if (!pathspec.nr) diff --git a/builtin/worktree.c b/builtin/worktree.c index 895838b943..511d0aa370 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -27,8 +27,6 @@ struct add_opts { int detach; int checkout; int keep_locked; - const char *new_branch; - int checkout_existing_branch; }; static int show_only; @@ -318,31 +316,26 @@ static int add_worktree(const char *path, const char *refname, if (ret) goto done; - if (opts->checkout_existing_branch) - fprintf_ln(stderr, _("checking out branch '%s'"), - refname); - else if (opts->new_branch) - fprintf_ln(stderr, _("creating branch '%s'"), opts->new_branch); - - fprintf(stderr, _("new worktree HEAD is now at %s"), - find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV)); - - strbuf_reset(&sb); - pp_commit_easy(CMIT_FMT_ONELINE, commit, &sb); - if (sb.len > 0) - fprintf(stderr, " %s", sb.buf); - fputc('\n', stderr); - if (opts->checkout) { cp.argv = NULL; argv_array_clear(&cp.args); - argv_array_pushl(&cp.args, "reset", "--hard", "--quiet", NULL); + argv_array_pushl(&cp.args, "reset", "--hard", NULL); + argv_array_push(&cp.args, "--no-show-new-head-line"); cp.env = child_env.argv; ret = run_command(&cp); if (ret) goto done; } + fprintf(stderr, _("New worktree HEAD is now at %s"), + find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV)); + + strbuf_reset(&sb); + pp_commit_easy(CMIT_FMT_ONELINE, commit, &sb); + if (sb.len > 0) + fprintf(stderr, " %s", sb.buf); + fputc('\n', stderr); + is_junk = 0; FREE_AND_NULL(junk_work_tree); FREE_AND_NULL(junk_git_dir); @@ -370,7 +363,8 @@ static int add_worktree(const char *path, const char *refname, return ret; } -static const char *dwim_branch(const char *path, struct add_opts *opts) +static const char *dwim_branch(const char *path, const char **new_branch, + int *checkout_existing_branch) { int n; const char *s = worktree_basename(path, &n); @@ -379,17 +373,17 @@ static const char *dwim_branch(const char *path, struct add_opts *opts) if (!strbuf_check_branch_ref(&ref, branchname) && ref_exists(ref.buf)) { - opts->checkout_existing_branch = 1; + *checkout_existing_branch = 1; strbuf_release(&ref); UNLEAK(branchname); return branchname; } - opts->new_branch = branchname; + *new_branch = branchname; if (guess_remote) { struct object_id oid; const char *remote = - unique_tracking_name(opts->new_branch, &oid); + unique_tracking_name(*new_branch, &oid); return remote; } return NULL; @@ -401,10 +395,12 @@ static int add(int ac, const char **av, const char *prefix) const char *new_branch_force = NULL; char *path; const char *branch; + const char *new_branch = NULL; const char *opt_track = NULL; + int checkout_existing_branch = 0; struct option options[] = { OPT__FORCE(&opts.force, N_("checkout <branch> even if already checked out in other worktree")), - OPT_STRING('b', NULL, &opts.new_branch, N_("branch"), + OPT_STRING('b', NULL, &new_branch, N_("branch"), N_("create a new branch")), OPT_STRING('B', NULL, &new_branch_force, N_("branch"), N_("create or reset a branch")), @@ -422,7 +418,7 @@ static int add(int ac, const char **av, const char *prefix) memset(&opts, 0, sizeof(opts)); opts.checkout = 1; ac = parse_options(ac, av, prefix, options, worktree_usage, 0); - if (!!opts.detach + !!opts.new_branch + !!new_branch_force > 1) + if (!!opts.detach + !!new_branch + !!new_branch_force > 1) die(_("-b, -B, and --detach are mutually exclusive")); if (ac < 1 || ac > 2) usage_with_options(worktree_usage, options); @@ -436,22 +432,23 @@ static int add(int ac, const char **av, const char *prefix) if (new_branch_force) { struct strbuf symref = STRBUF_INIT; - opts.new_branch = new_branch_force; + new_branch = new_branch_force; if (!opts.force && - !strbuf_check_branch_ref(&symref, opts.new_branch) && + !strbuf_check_branch_ref(&symref, new_branch) && ref_exists(symref.buf)) die_if_checked_out(symref.buf, 0); strbuf_release(&symref); } - if (ac < 2 && !opts.new_branch && !opts.detach) { - const char *dwim_branchname = dwim_branch(path, &opts); - if (dwim_branchname) - branch = dwim_branchname; + if (ac < 2 && !new_branch && !opts.detach) { + const char *s = dwim_branch(path, &new_branch, + &checkout_existing_branch); + if (s) + branch = s; } - if (ac == 2 && !opts.new_branch && !opts.detach) { + if (ac == 2 && !new_branch && !opts.detach) { struct object_id oid; struct commit *commit; const char *remote; @@ -460,25 +457,29 @@ static int add(int ac, const char **av, const char *prefix) if (!commit) { remote = unique_tracking_name(branch, &oid); if (remote) { - opts.new_branch = branch; + new_branch = branch; branch = remote; } } } - if (opts.new_branch) { + if (new_branch) { struct child_process cp = CHILD_PROCESS_INIT; + + fprintf_ln(stderr, _("Creating branch '%s'"), new_branch); cp.git_cmd = 1; argv_array_push(&cp.args, "branch"); if (new_branch_force) argv_array_push(&cp.args, "--force"); - argv_array_push(&cp.args, opts.new_branch); + argv_array_push(&cp.args, new_branch); argv_array_push(&cp.args, branch); if (opt_track) argv_array_push(&cp.args, opt_track); if (run_command(&cp)) return -1; - branch = opts.new_branch; + branch = new_branch; + } else if (checkout_existing_branch) { + fprintf_ln(stderr, _("Checking out branch '%s'"), branch); } else if (opt_track) { die(_("--[no-]track can only be used if a new branch is created")); } diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh index fb99f4c46f..f72cb0eb0b 100755 --- a/t/t2025-worktree-add.sh +++ b/t/t2025-worktree-add.sh @@ -199,8 +199,6 @@ test_expect_success '"add" with <branch> omitted' ' ' test_expect_success '"add" checks out existing branch of dwimd name' ' - test_commit c1 && - test_commit c2 && git branch dwim HEAD~1 && git worktree add dwim && test_cmp_rev HEAD~1 dwim && @@ -210,7 +208,7 @@ test_expect_success '"add" checks out existing branch of dwimd name' ' ) ' -test_expect_success '"add" auto-vivify fails with checked out branch' ' +test_expect_success '"add <path>" dwim fails with checked out branch' ' git checkout -b test-branch && test_must_fail git worktree add test-branch && test_path_is_missing test-branch diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh index 95653a08ca..a160f78aba 100755 --- a/t/t7102-reset.sh +++ b/t/t7102-reset.sh @@ -568,4 +568,9 @@ test_expect_success 'reset --mixed sets up work tree' ' test_cmp expect actual ' +test_expect_success 'reset --no-show-new-head-line suppresses "HEAD is now at" output' ' + git reset --hard --no-show-new-head-line HEAD >actual && + ! grep "HEAD is now at" <actual +' + test_done Thomas Gummerer (6): worktree: remove extra members from struct add_opts reset: introduce show-new-head-line option worktree: improve message when creating a new worktree worktree: be clearer when "add" dwim-ery kicks in worktree: factor out dwim_branch function worktree: teach "add" to check out existing branches Documentation/git-worktree.txt | 9 ++++- builtin/reset.c | 5 ++- builtin/worktree.c | 85 ++++++++++++++++++++++++++++-------------- t/t2025-worktree-add.sh | 25 +++++++++---- t/t7102-reset.sh | 5 +++ 5 files changed, 92 insertions(+), 37 deletions(-)
-- 2.16.1.78.g383dce0c66