* [PATCH v6 1/6] worktree: remove extra members from struct add_opts
2018-03-31 15:17 ` [PATCH v6 " Thomas Gummerer
@ 2018-03-31 15:17 ` Thomas Gummerer
2018-03-31 15:18 ` [PATCH v6 2/6] reset: introduce show-new-head-line option Thomas Gummerer
` (6 subsequent siblings)
7 siblings, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-03-31 15:17 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
There are two members of 'struct add_opts', which are only used inside
the 'add()' function, but being part of 'struct add_opts' they are
needlessly also passed to the 'add_worktree' function.
Make them local to the 'add()' function to make it clearer where they
are used.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/worktree.c | 32 +++++++++++++++-----------------
1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7cef5b120b..4d96a21a45 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 force_new_branch;
};
static int show_only;
@@ -363,10 +361,11 @@ 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;
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")),
@@ -384,7 +383,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);
@@ -395,33 +394,32 @@ static int add(int ac, const char **av, const char *prefix)
if (!strcmp(branch, "-"))
branch = "@{-1}";
- opts.force_new_branch = !!new_branch_force;
- if (opts.force_new_branch) {
+ 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) {
+ if (ac < 2 && !new_branch && !opts.detach) {
int n;
const char *s = worktree_basename(path, &n);
- opts.new_branch = xstrndup(s, n);
+ new_branch = xstrndup(s, n);
if (guess_remote) {
struct object_id oid;
const char *remote =
- unique_tracking_name(opts.new_branch, &oid);
+ unique_tracking_name(new_branch, &oid);
if (remote)
branch = remote;
}
}
- 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;
@@ -430,25 +428,25 @@ 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;
cp.git_cmd = 1;
argv_array_push(&cp.args, "branch");
- if (opts.force_new_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 (opt_track) {
die(_("--[no-]track can only be used if a new branch is created"));
}
--
2.16.1.78.ga2082135d8
^ permalink raw reply related [flat|nested] 112+ messages in thread
* [PATCH v6 2/6] reset: introduce show-new-head-line option
2018-03-31 15:17 ` [PATCH v6 " Thomas Gummerer
2018-03-31 15:17 ` [PATCH v6 1/6] worktree: remove extra members from struct add_opts Thomas Gummerer
@ 2018-03-31 15:18 ` Thomas Gummerer
2018-04-02 20:29 ` Junio C Hamano
2018-04-02 20:34 ` Junio C Hamano
2018-03-31 15:18 ` [PATCH v6 3/6] worktree: improve message when creating a new worktree Thomas Gummerer
` (5 subsequent siblings)
7 siblings, 2 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-03-31 15:18 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
Introduce a new --show-new-head-line command line option, that
determines whether the "HEAD is now at ..." message is printed or not.
It is enabled by default to preserve the current behaviour.
It will be used in a subsequent commit to disable printing the "HEAD is
now at ..." line in 'git worktree add', so it can be replaced with a
custom one, that explains the behaviour better.
We cannot just pass the --quite flag from 'git worktree add', as that
would also hide progress output when checking out large working trees,
which is undesirable.
As this option is only for internal use, which we probably don't want
to advertise to our users, at least until there's a need for it, make
it a hidden flag.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/reset.c | 5 ++++-
t/t7102-reset.sh | 5 +++++
2 files changed, 9 insertions(+), 1 deletion(-)
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/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
--
2.16.1.78.ga2082135d8
^ permalink raw reply related [flat|nested] 112+ messages in thread
* Re: [PATCH v6 2/6] reset: introduce show-new-head-line option
2018-03-31 15:18 ` [PATCH v6 2/6] reset: introduce show-new-head-line option Thomas Gummerer
@ 2018-04-02 20:29 ` Junio C Hamano
2018-04-02 22:07 ` Thomas Gummerer
2018-04-02 22:20 ` Thomas Gummerer
2018-04-02 20:34 ` Junio C Hamano
1 sibling, 2 replies; 112+ messages in thread
From: Junio C Hamano @ 2018-04-02 20:29 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: git, Eric Sunshine, Nguyễn Thái Ngọc Duy
Thomas Gummerer <t.gummerer@gmail.com> writes:
> Introduce a new --show-new-head-line command line option, that
> determines whether the "HEAD is now at ..." message is printed or not.
> It is enabled by default to preserve the current behaviour.
>
> It will be used in a subsequent commit to disable printing the "HEAD is
> now at ..." line in 'git worktree add', so it can be replaced with a
> custom one, that explains the behaviour better.
>
> We cannot just pass the --quite flag from 'git worktree add', as that
> would also hide progress output when checking out large working trees,
> which is undesirable.
>
> As this option is only for internal use, which we probably don't want
> to advertise to our users, at least until there's a need for it, make
> it a hidden flag.
As a temporary hack, adding something like this may be OK, but in
the longer run, I think we should (at least) consider refactoring
"reset --hard" codepath to a freestanding and silent helper function
so that both cmd_reset() and add_worktree() can call it and report
the outcome in their own phrasing.
And I support the decision not to advertise this as a new feature or
an option by implementing it as hidden-bool.
This is completely offtopic tangent, but I wonder how hidden-bool or
hidden options[] element in general interacts with the recent
addition of helping command line completion. Are we already doing
the right thing?
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 2/6] reset: introduce show-new-head-line option
2018-04-02 20:29 ` Junio C Hamano
@ 2018-04-02 22:07 ` Thomas Gummerer
2018-04-02 22:20 ` Thomas Gummerer
1 sibling, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-02 22:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Sunshine, Nguyễn Thái Ngọc Duy
On 04/02, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > Introduce a new --show-new-head-line command line option, that
> > determines whether the "HEAD is now at ..." message is printed or not.
> > It is enabled by default to preserve the current behaviour.
> >
> > It will be used in a subsequent commit to disable printing the "HEAD is
> > now at ..." line in 'git worktree add', so it can be replaced with a
> > custom one, that explains the behaviour better.
> >
> > We cannot just pass the --quite flag from 'git worktree add', as that
> > would also hide progress output when checking out large working trees,
> > which is undesirable.
> >
> > As this option is only for internal use, which we probably don't want
> > to advertise to our users, at least until there's a need for it, make
> > it a hidden flag.
>
> As a temporary hack, adding something like this may be OK, but in
> the longer run, I think we should (at least) consider refactoring
> "reset --hard" codepath to a freestanding and silent helper function
> so that both cmd_reset() and add_worktree() can call it and report
> the outcome in their own phrasing.
I agree that refactoring this would have been nicer. The biggest
obstacle there is that the 'git reset' needs to run with a different
"GIT_DIR" and "GIT_WORK_TREE", which is not easy to accomplish in the
current code.
It does seem like something that would be much easier once we have
'struct repository' being passed through everywhere. I'd rather wait
a bit more for the 'struct repository' series to land, which will
hopefully make the refactoring easier (although I didn't have time to
follow the series closely).
> And I support the decision not to advertise this as a new feature or
> an option by implementing it as hidden-bool.
>
> This is completely offtopic tangent, but I wonder how hidden-bool or
> hidden options[] element in general interacts with the recent
> addition of helping command line completion. Are we already doing
> the right thing?
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 2/6] reset: introduce show-new-head-line option
2018-04-02 20:29 ` Junio C Hamano
2018-04-02 22:07 ` Thomas Gummerer
@ 2018-04-02 22:20 ` Thomas Gummerer
1 sibling, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-02 22:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Sunshine, Nguyễn Thái Ngọc Duy
On 04/02, Junio C Hamano wrote:
> This is completely offtopic tangent, but I wonder how hidden-bool or
> hidden options[] element in general interacts with the recent
> addition of helping command line completion. Are we already doing
> the right thing?
I had a quick look at this, and it looks like we're doing the right
thing here. The following snipped from the 'show_gitcomp' function in
parse-options.c:
> for (; opts->type != OPTION_END; opts++) {
> const char *suffix = "";
>
> if (!opts->long_name)
> continue;
> if (opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE))
> continue;
So if the PARSE_OPT_HIDDEN flag is given, we skip printing the option,
which seems like the right thing to do.
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 2/6] reset: introduce show-new-head-line option
2018-03-31 15:18 ` [PATCH v6 2/6] reset: introduce show-new-head-line option Thomas Gummerer
2018-04-02 20:29 ` Junio C Hamano
@ 2018-04-02 20:34 ` Junio C Hamano
2018-04-02 22:09 ` Thomas Gummerer
1 sibling, 1 reply; 112+ messages in thread
From: Junio C Hamano @ 2018-04-02 20:34 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: git, Eric Sunshine, Nguyễn Thái Ngọc Duy
Thomas Gummerer <t.gummerer@gmail.com> writes:
> +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
> +'
As builtin/reset.c::print_new_head_line() does this:
printf(_("HEAD is now at %s"), hex);
this needs to use "test_i18ngrep !" instead, no?
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 2/6] reset: introduce show-new-head-line option
2018-04-02 20:34 ` Junio C Hamano
@ 2018-04-02 22:09 ` Thomas Gummerer
0 siblings, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-02 22:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Sunshine, Nguyễn Thái Ngọc Duy
On 04/02, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > +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
> > +'
>
> As builtin/reset.c::print_new_head_line() does this:
>
> printf(_("HEAD is now at %s"), hex);
>
> this needs to use "test_i18ngrep !" instead, no?
Ah yes you're right. Thanks for catching this.
^ permalink raw reply [flat|nested] 112+ messages in thread
* [PATCH v6 3/6] worktree: improve message when creating a new worktree
2018-03-31 15:17 ` [PATCH v6 " Thomas Gummerer
2018-03-31 15:17 ` [PATCH v6 1/6] worktree: remove extra members from struct add_opts Thomas Gummerer
2018-03-31 15:18 ` [PATCH v6 2/6] reset: introduce show-new-head-line option Thomas Gummerer
@ 2018-03-31 15:18 ` Thomas Gummerer
2018-04-08 9:27 ` Eric Sunshine
2018-03-31 15:18 ` [PATCH v6 4/6] worktree: be clearer when "add" dwim-ery kicks in Thomas Gummerer
` (4 subsequent siblings)
7 siblings, 1 reply; 112+ messages in thread
From: Thomas Gummerer @ 2018-03-31 15:18 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
Currently 'git worktree add' produces output like the following, when
'--no-checkout' is not given:
Preparing foo (identifier foo)
HEAD is now at 26da330922 <title>
where the first line is written to stderr, and the second line coming
from 'git reset --hard' is written to stdout, even though both lines are
supposed to tell the user what has happened. In addition to someone not
familiar with 'git worktree', this might seem as if the current HEAD was
modified, not the HEAD in the new working tree.
If the '--no-checkout' flag is given, the output of 'git worktree add'
is just:
Preparing foo (identifier foo)
even though the HEAD is set to a commit, which is just not checked out.
The identifier is also not particularly relevant for the user at the
moment, as it's only used internally to distinguish between different
worktrees that have the same $(basename <path>).
Fix these inconsistencies, and no longer show the identifier by making
the 'git reset --hard' call not print the "HEAD is now at ..." line
using the newly introduced flag from the previous commit, and printing
the message directly from the builtin command instead.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/worktree.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 4d96a21a45..cc94325886 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -301,8 +301,6 @@ static int add_worktree(const char *path, const char *refname,
strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
write_file(sb.buf, "../..");
- fprintf_ln(stderr, _("Preparing %s (identifier %s)"), path, name);
-
argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
cp.git_cmd = 1;
@@ -322,12 +320,22 @@ static int add_worktree(const char *path, const char *refname,
cp.argv = NULL;
argv_array_clear(&cp.args);
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);
--
2.16.1.78.ga2082135d8
^ permalink raw reply related [flat|nested] 112+ messages in thread
* Re: [PATCH v6 3/6] worktree: improve message when creating a new worktree
2018-03-31 15:18 ` [PATCH v6 3/6] worktree: improve message when creating a new worktree Thomas Gummerer
@ 2018-04-08 9:27 ` Eric Sunshine
0 siblings, 0 replies; 112+ messages in thread
From: Eric Sunshine @ 2018-04-08 9:27 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On Sat, Mar 31, 2018 at 11:18 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> [...]
> Fix these inconsistencies, and no longer show the identifier by making
> the 'git reset --hard' call not print the "HEAD is now at ..." line
> using the newly introduced flag from the previous commit, and printing
> the message directly from the builtin command instead.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -322,12 +320,22 @@ static int add_worktree(const char *path, const char *refname,
> 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);
Generally speaking, code such as this probably ought to be inserted
outside of the is_junk={1,0} context in order to keep that critical
section as small as possible. However, as mentioned in my response to
the v6 cover letter[1], I think this chunk of new code can just go
away entirely if git-reset is made to print the customized message on
git-worktree's behalf.
[1]: https://public-inbox.org/git/CAPig+cQ8VzDycUMo-QOexNDBgQGEGj2BPmPa-Y0vhGCt_brbhg@mail.gmail.com/
^ permalink raw reply [flat|nested] 112+ messages in thread
* [PATCH v6 4/6] worktree: be clearer when "add" dwim-ery kicks in
2018-03-31 15:17 ` [PATCH v6 " Thomas Gummerer
` (2 preceding siblings ...)
2018-03-31 15:18 ` [PATCH v6 3/6] worktree: improve message when creating a new worktree Thomas Gummerer
@ 2018-03-31 15:18 ` Thomas Gummerer
2018-03-31 15:18 ` [PATCH v6 5/6] worktree: factor out dwim_branch function Thomas Gummerer
` (3 subsequent siblings)
7 siblings, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-03-31 15:18 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
Currently there is no indication in the "git worktree add" output that
a new branch was created. This would be especially useful information
in the case where the dwim of "git worktree add <path>" kicks in, as the
user didn't explicitly ask for a new branch, but we create one for them.
Print some additional output showing that a branch was created and the
branch name to help the user.
This will also be useful in a subsequent commit, which introduces a new
kind of dwim-ery of checking out the branch if it exists instead of
refusing to create a new worktree in that case, and where it's nice to
tell the user which kind of dwim-ery kicked in.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/worktree.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index cc94325886..f686ee1440 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -444,6 +444,8 @@ static int add(int ac, const char **av, const char *prefix)
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)
--
2.16.1.78.ga2082135d8
^ permalink raw reply related [flat|nested] 112+ messages in thread
* [PATCH v6 5/6] worktree: factor out dwim_branch function
2018-03-31 15:17 ` [PATCH v6 " Thomas Gummerer
` (3 preceding siblings ...)
2018-03-31 15:18 ` [PATCH v6 4/6] worktree: be clearer when "add" dwim-ery kicks in Thomas Gummerer
@ 2018-03-31 15:18 ` Thomas Gummerer
2018-03-31 15:18 ` [PATCH v6 6/6] worktree: teach "add" to check out existing branches Thomas Gummerer
` (2 subsequent siblings)
7 siblings, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-03-31 15:18 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
Factor out a dwim_branch function, which takes care of the dwim'ery in
'git worktree add <path>'. It's not too much code currently, but we're
adding a new kind of dwim in a subsequent patch, at which point it makes
more sense to have it as a separate function.
Factor it out now to reduce the patch noise in the next patch.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/worktree.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index f686ee1440..47189d50dd 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -363,6 +363,20 @@ static int add_worktree(const char *path, const char *refname,
return ret;
}
+static const char *dwim_branch(const char *path, const char **new_branch)
+{
+ int n;
+ const char *s = worktree_basename(path, &n);
+ *new_branch = xstrndup(s, n);
+ if (guess_remote) {
+ struct object_id oid;
+ const char *remote =
+ unique_tracking_name(*new_branch, &oid);
+ return remote;
+ }
+ return NULL;
+}
+
static int add(int ac, const char **av, const char *prefix)
{
struct add_opts opts;
@@ -415,16 +429,9 @@ static int add(int ac, const char **av, const char *prefix)
}
if (ac < 2 && !new_branch && !opts.detach) {
- int n;
- const char *s = worktree_basename(path, &n);
- new_branch = xstrndup(s, n);
- if (guess_remote) {
- struct object_id oid;
- const char *remote =
- unique_tracking_name(new_branch, &oid);
- if (remote)
- branch = remote;
- }
+ const char *s = dwim_branch(path, &new_branch);
+ if (s)
+ branch = s;
}
if (ac == 2 && !new_branch && !opts.detach) {
--
2.16.1.78.ga2082135d8
^ permalink raw reply related [flat|nested] 112+ messages in thread
* [PATCH v6 6/6] worktree: teach "add" to check out existing branches
2018-03-31 15:17 ` [PATCH v6 " Thomas Gummerer
` (4 preceding siblings ...)
2018-03-31 15:18 ` [PATCH v6 5/6] worktree: factor out dwim_branch function Thomas Gummerer
@ 2018-03-31 15:18 ` Thomas Gummerer
2018-04-01 13:11 ` [PATCH v6 6.5/6] fixup! " Thomas Gummerer
2018-04-08 10:09 ` [PATCH v6 6/6] " Eric Sunshine
2018-04-08 9:08 ` [PATCH v6 0/6] " Eric Sunshine
2018-04-15 20:29 ` [PATCH v7 0/4] " Thomas Gummerer
7 siblings, 2 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-03-31 15:18 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
Currently 'git worktree add <path>' creates a new branch named after the
basename of the path by default. If a branch with that name already
exists, the command refuses to do anything, unless the '--force' option
is given.
However we can do a little better than that, and check the branch out if
it is not checked out anywhere else. This will help users who just want
to check an existing branch out into a new worktree, and save a few
keystrokes.
As the current behaviour is to simply 'die()' when a branch with the name
of the basename of the path already exists, there are no backwards
compatibility worries here.
We will still 'die()' if the branch is checked out in another worktree,
unless the --force flag is passed.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-worktree.txt | 9 +++++++--
builtin/worktree.c | 22 +++++++++++++++++++---
t/t2025-worktree-add.sh | 25 ++++++++++++++++++-------
3 files changed, 44 insertions(+), 12 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 41585f535d..eaa6bf713f 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -61,8 +61,13 @@ $ git worktree add --track -b <branch> <path> <remote>/<branch>
------------
+
If `<commit-ish>` is omitted and neither `-b` nor `-B` nor `--detach` used,
-then, as a convenience, a new branch based at HEAD is created automatically,
-as if `-b $(basename <path>)` was specified.
+then, as a convenience, a worktree with a branch named after
+`$(basename <path>)` (call it `<branch>`) is created. If `<branch>`
+doesn't exist, a new branch based on HEAD is automatically created as
+if `-b <branch>` was given. If `<branch>` exists in the repository,
+it will be checked out in the new worktree, if it's not checked out
+anywhere else, otherwise the command will refuse to create the
+worktree (unless `--force` is used).
list::
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 47189d50dd..511d0aa370 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -363,11 +363,23 @@ static int add_worktree(const char *path, const char *refname,
return ret;
}
-static const char *dwim_branch(const char *path, const char **new_branch)
+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);
- *new_branch = xstrndup(s, n);
+ const char *branchname = xstrndup(s, n);
+ struct strbuf ref = STRBUF_INIT;
+
+ if (!strbuf_check_branch_ref(&ref, branchname) &&
+ ref_exists(ref.buf)) {
+ *checkout_existing_branch = 1;
+ strbuf_release(&ref);
+ UNLEAK(branchname);
+ return branchname;
+ }
+
+ *new_branch = branchname;
if (guess_remote) {
struct object_id oid;
const char *remote =
@@ -385,6 +397,7 @@ static int add(int ac, const char **av, const char *prefix)
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, &new_branch, N_("branch"),
@@ -429,7 +442,8 @@ static int add(int ac, const char **av, const char *prefix)
}
if (ac < 2 && !new_branch && !opts.detach) {
- const char *s = dwim_branch(path, &new_branch);
+ const char *s = dwim_branch(path, &new_branch,
+ &checkout_existing_branch);
if (s)
branch = s;
}
@@ -464,6 +478,8 @@ static int add(int ac, const char **av, const char *prefix)
if (run_command(&cp))
return -1;
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 2b95944973..f72cb0eb0b 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -198,13 +198,24 @@ test_expect_success '"add" with <branch> omitted' '
test_cmp_rev HEAD bat
'
-test_expect_success '"add" auto-vivify does not clobber existing branch' '
- test_commit c1 &&
- test_commit c2 &&
- git branch precious HEAD~1 &&
- test_must_fail git worktree add precious &&
- test_cmp_rev HEAD~1 precious &&
- test_path_is_missing precious
+test_expect_success '"add" checks out existing branch of dwimd name' '
+ git branch dwim HEAD~1 &&
+ git worktree add dwim &&
+ test_cmp_rev HEAD~1 dwim &&
+ (
+ cd dwim &&
+ test_cmp_rev dwim HEAD
+ )
+'
+
+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
+'
+
+test_expect_success '"add --force" with existing dwimd name doesnt die' '
+ git worktree add --force test-branch
'
test_expect_success '"add" no auto-vivify with --detach and <branch> omitted' '
--
2.16.1.78.ga2082135d8
^ permalink raw reply related [flat|nested] 112+ messages in thread
* [PATCH v6 6.5/6] fixup! worktree: teach "add" to check out existing branches
2018-03-31 15:18 ` [PATCH v6 6/6] worktree: teach "add" to check out existing branches Thomas Gummerer
@ 2018-04-01 13:11 ` Thomas Gummerer
2018-04-09 0:23 ` Eric Sunshine
2018-04-08 10:09 ` [PATCH v6 6/6] " Eric Sunshine
1 sibling, 1 reply; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-01 13:11 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
So while playing with it a bit more I found one case where the new UI
is not ideal and a bit confusing. Namely when the new check out dwim
kicks in, but there is already a file/directory at the path we're
giving to 'git worktree add'.
In that case something like the following would be printed:
$ g worktree add ../next
Checking out branch 'next'
fatal: '../next' already exists
Instead I think we'd just want the error without the "Checking out
branch" message, which is what this fixup here does.
One thing that gets a bit strange is that the "Checking out branch"
message and the "Creating branch" messages are printed from different
places. But without doing quite some refactoring I don't think
there's a good way to do that, and I think having the UI do the right
thing is more important.
One thing I also noticed is that if a branch is created by 'git
worktree add', but we fail, we never clean up that branch again, which
I'm not sure is ideal. As a pre-existing problem I'd like to keep
fixing that out of the scope of this series though (at least after
this series the user would get some output showing that this happened,
even when the branch is not set up to track a remote), so I'd like to
keep fixing that out of the scope of this series.
Junio: could you please squash this in in 6/6 while queuing? I'd
prefer not to re-send the whole series just for fixing this up, but
obviously can if that makes your life easier :)
Thanks!
builtin/worktree.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 511d0aa370..ccc2e63e0f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -27,6 +27,7 @@ struct add_opts {
int detach;
int checkout;
int keep_locked;
+ int checkout_existing_branch;
};
static int show_only;
@@ -316,6 +317,8 @@ 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);
if (opts->checkout) {
cp.argv = NULL;
argv_array_clear(&cp.args);
@@ -397,7 +400,6 @@ static int add(int ac, const char **av, const char *prefix)
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, &new_branch, N_("branch"),
@@ -443,7 +445,7 @@ static int add(int ac, const char **av, const char *prefix)
if (ac < 2 && !new_branch && !opts.detach) {
const char *s = dwim_branch(path, &new_branch,
- &checkout_existing_branch);
+ &opts.checkout_existing_branch);
if (s)
branch = s;
}
@@ -478,8 +480,6 @@ static int add(int ac, const char **av, const char *prefix)
if (run_command(&cp))
return -1;
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"));
}
--
2.16.1.78.g71f731ae26.dirty
^ permalink raw reply related [flat|nested] 112+ messages in thread
* Re: [PATCH v6 6.5/6] fixup! worktree: teach "add" to check out existing branches
2018-04-01 13:11 ` [PATCH v6 6.5/6] fixup! " Thomas Gummerer
@ 2018-04-09 0:23 ` Eric Sunshine
2018-04-09 19:44 ` Thomas Gummerer
0 siblings, 1 reply; 112+ messages in thread
From: Eric Sunshine @ 2018-04-09 0:23 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On Sun, Apr 1, 2018 at 9:11 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> So while playing with it a bit more I found one case where the new UI
> is not ideal and a bit confusing. Namely when the new check out dwim
> kicks in, but there is already a file/directory at the path we're
> giving to 'git worktree add'.
>
> In that case something like the following would be printed:
>
> $ g worktree add ../next
> Checking out branch 'next'
> fatal: '../next' already exists
>
> Instead I think we'd just want the error without the "Checking out
> branch" message, which is what this fixup here does.
Doesn't the same UI "problem" exist when it creates a new branch?
$ git worktree add ../dwim
Creating branch 'dwim'
fatal: '../dwim' already exists
As you mention below, we don't (yet) clean up the newly-created branch
upon failure, so we can't suppress the "Creating branch" message as
you suppress the "Checking out branch" message above (since the user
needs to know that the new branch exists).
This is making me wonder if "Checking out branch" is perhaps the wrong
terminology. What if it said something like this instead:
$ git worktree add ../next
Preparing worktree (branch 'next' <= 'origin/next')
fatal: '../next' already exists
$ git worktree add ../gobble
Preparing worktree (new branch 'gobble')
fatal: '../gobble' already exists
This way, we don't need the special case added by this "fixup!" patch.
(I'm not wedded to the "Preparing" message but just used it as an
example; better suggestions welcome.)
> One thing that gets a bit strange is that the "Checking out branch"
> message and the "Creating branch" messages are printed from different
> places. But without doing quite some refactoring I don't think
> there's a good way to do that, and I think having the UI do the right
> thing is more important.
The implementation is getting rather ugly, though, especially with
these messages being printed by different bits of code like this.
worktree.c:add_worktree() was supposed to be the low-level worker; it
wasn't intended for it to take on UI duties like this "fixup!" makes
it do. UI should be handled by worktree.c:add().
Taking the above "Preparing..." idea into consideration, then it
should be possible to sidestep this implementation ugliness, I would
think.
> One thing I also noticed is that if a branch is created by 'git
> worktree add', but we fail, we never clean up that branch again, which
> I'm not sure is ideal. As a pre-existing problem I'd like to keep
> fixing that out of the scope of this series though (at least after
> this series the user would get some output showing that this happened,
> even when the branch is not set up to track a remote), so I'd like to
> keep fixing that out of the scope of this series.
Nice idea, but outside the scope of this series, as you mention.
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -27,6 +27,7 @@ struct add_opts {
> int keep_locked;
> + int checkout_existing_branch;
> };
> @@ -316,6 +317,8 @@ static int add_worktree(const char *path, const char *refname,
> + if (opts->checkout_existing_branch)
> + fprintf_ln(stderr, _("Checking out branch '%s'"), refname);
> if (opts->checkout) {
I'd have expected to see the "if (opts->checkout_existing_branch)
fprintf_ln(...)" inside the following "if (opts->checkout)"
conditional, though, as noted above, I'm not entirely happy about
worktree.c:add_worktree() taking on UI duties.
> cp.argv = NULL;
> argv_array_clear(&cp.args);
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 6.5/6] fixup! worktree: teach "add" to check out existing branches
2018-04-09 0:23 ` Eric Sunshine
@ 2018-04-09 19:44 ` Thomas Gummerer
2018-04-09 21:35 ` Eric Sunshine
0 siblings, 1 reply; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-09 19:44 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On 04/08, Eric Sunshine wrote:
> On Sun, Apr 1, 2018 at 9:11 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > So while playing with it a bit more I found one case where the new UI
> > is not ideal and a bit confusing. Namely when the new check out dwim
> > kicks in, but there is already a file/directory at the path we're
> > giving to 'git worktree add'.
> >
> > In that case something like the following would be printed:
> >
> > $ g worktree add ../next
> > Checking out branch 'next'
> > fatal: '../next' already exists
> >
> > Instead I think we'd just want the error without the "Checking out
> > branch" message, which is what this fixup here does.
>
> Doesn't the same UI "problem" exist when it creates a new branch?
>
> $ git worktree add ../dwim
> Creating branch 'dwim'
> fatal: '../dwim' already exists
>
> As you mention below, we don't (yet) clean up the newly-created branch
> upon failure, so we can't suppress the "Creating branch" message as
> you suppress the "Checking out branch" message above (since the user
> needs to know that the new branch exists).
>
> This is making me wonder if "Checking out branch" is perhaps the wrong
> terminology. What if it said something like this instead:
>
> $ git worktree add ../next
> Preparing worktree (branch 'next' <= 'origin/next')
> fatal: '../next' already exists
>
> $ git worktree add ../gobble
> Preparing worktree (new branch 'gobble')
> fatal: '../gobble' already exists
>
> This way, we don't need the special case added by this "fixup!" patch.
> (I'm not wedded to the "Preparing" message but just used it as an
> example; better suggestions welcome.)
Yeah, I think this looks like another improvement of what I currently
have. I'm not sure about the "(branch 'next' <= 'origin/next')"
message though, as it doesn't cover all the ways the branch could be
set up for tracking the remote branch, e.g. tracking by rebasing, when
'branch.autosetuprebase' is set up.
But how about just "Preparing worktree (new branch 'next')", and then
keeping the message from 'git branch' about setting up the remote
tracking branch?
> > One thing that gets a bit strange is that the "Checking out branch"
> > message and the "Creating branch" messages are printed from different
> > places. But without doing quite some refactoring I don't think
> > there's a good way to do that, and I think having the UI do the right
> > thing is more important.
>
> The implementation is getting rather ugly, though, especially with
> these messages being printed by different bits of code like this.
> worktree.c:add_worktree() was supposed to be the low-level worker; it
> wasn't intended for it to take on UI duties like this "fixup!" makes
> it do. UI should be handled by worktree.c:add().
>
> Taking the abonve "Preparing..." idea into consideration, then it
> should be possible to sidestep this implementation ugliness, I would
> think.
>
> > One thing I also noticed is that if a branch is created by 'git
> > worktree add', but we fail, we never clean up that branch again, which
> > I'm not sure is ideal. As a pre-existing problem I'd like to keep
> > fixing that out of the scope of this series though (at least after
> > this series the user would get some output showing that this happened,
> > even when the branch is not set up to track a remote), so I'd like to
> > keep fixing that out of the scope of this series.
>
> Nice idea, but outside the scope of this series, as you mention.
>
> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > @@ -27,6 +27,7 @@ struct add_opts {
> > int keep_locked;
> > + int checkout_existing_branch;
> > };
> > @@ -316,6 +317,8 @@ static int add_worktree(const char *path, const char *refname,
> > + if (opts->checkout_existing_branch)
> > + fprintf_ln(stderr, _("Checking out branch '%s'"), refname);
> > if (opts->checkout) {
>
> I'd have expected to see the "if (opts->checkout_existing_branch)
> fprintf_ln(...)" inside the following "if (opts->checkout)"
> conditional, though, as noted above, I'm not entirely happy about
> worktree.c:add_worktree() taking on UI duties.
Fair enough. I didn't quite like the code either. I think what you
have above would be much nicer, and I'll implement that in the
re-roll.
> > cp.argv = NULL;
> > argv_array_clear(&cp.args);
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 6.5/6] fixup! worktree: teach "add" to check out existing branches
2018-04-09 19:44 ` Thomas Gummerer
@ 2018-04-09 21:35 ` Eric Sunshine
0 siblings, 0 replies; 112+ messages in thread
From: Eric Sunshine @ 2018-04-09 21:35 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On Mon, Apr 9, 2018 at 3:44 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> On 04/08, Eric Sunshine wrote:
>> This is making me wonder if "Checking out branch" is perhaps the wrong
>> terminology. What if it said something like this instead:
>>
>> $ git worktree add ../next
>> Preparing worktree (branch 'next' <= 'origin/next')
>> fatal: '../next' already exists
>>
>> This way, we don't need the special case added by this "fixup!" patch.
>> (I'm not wedded to the "Preparing" message but just used it as an
>> example; better suggestions welcome.)
>
> Yeah, I think this looks like another improvement of what I currently
> have. I'm not sure about the "(branch 'next' <= 'origin/next')"
> message though, as it doesn't cover all the ways the branch could be
> set up for tracking the remote branch, e.g. tracking by rebasing, when
> 'branch.autosetuprebase' is set up.
>
> But how about just "Preparing worktree (new branch 'next')", and then
> keeping the message from 'git branch' about setting up the remote
> tracking branch?
Fair enough. Extra annotation (such as "'foo' <= 'bar'" or whatever)
can always be added later if someone wants it.
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 6/6] worktree: teach "add" to check out existing branches
2018-03-31 15:18 ` [PATCH v6 6/6] worktree: teach "add" to check out existing branches Thomas Gummerer
2018-04-01 13:11 ` [PATCH v6 6.5/6] fixup! " Thomas Gummerer
@ 2018-04-08 10:09 ` Eric Sunshine
2018-04-08 14:30 ` Thomas Gummerer
1 sibling, 1 reply; 112+ messages in thread
From: Eric Sunshine @ 2018-04-08 10:09 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On Sat, Mar 31, 2018 at 11:18 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> [...]
> However we can do a little better than that, and check the branch out if
> it is not checked out anywhere else. This will help users who just want
> to check an existing branch out into a new worktree, and save a few
> keystrokes.
> [...]
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -61,8 +61,13 @@ $ git worktree add --track -b <branch> <path> <remote>/<branch>
> If `<commit-ish>` is omitted and neither `-b` nor `-B` nor `--detach` used,
> +then, as a convenience, a worktree with a branch named after
> +`$(basename <path>)` (call it `<branch>`) is created.
I had a hard time digesting this. I _think_ it wants to say:
If `<commit-ish>` is omitted and neither `-b` nor `-B` nor
`--detach` is used, then, as a convenience, the new worktree is
associated with a branch (call it `<branch>`) named after
`$(basename <path>)`.
> If `<branch>`
> +doesn't exist, a new branch based on HEAD is automatically created as
> +if `-b <branch>` was given. If `<branch>` exists in the repository,
Maybe: s/exists in the repository/does exist/
Or: s/.../is a local branch/
Though, the latter may be getting too pedantic.
> +it will be checked out in the new worktree, if it's not checked out
> +anywhere else, otherwise the command will refuse to create the
> +worktree (unless `--force` is used).
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> @@ -198,13 +198,24 @@ test_expect_success '"add" with <branch> omitted' '
> +test_expect_success '"add" checks out existing branch of dwimd name' '
> + git branch dwim HEAD~1 &&
> + git worktree add dwim &&
> + test_cmp_rev HEAD~1 dwim &&
> + (
> + cd dwim &&
> + test_cmp_rev dwim HEAD
Nit: Argument order of the two test_cmp_rev() invocations differs.
> + )
> +'
> +
> +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
> +'
> +
> +test_expect_success '"add --force" with existing dwimd name doesnt die' '
> + git worktree add --force test-branch
> '
Maybe make this last test slightly more robust by having it "git
checkout test-branch" before "git worktree add ..." to protect against
someone inserting a new test (which checks out some other branch)
between these two. Probably not worth a re-roll.
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 6/6] worktree: teach "add" to check out existing branches
2018-04-08 10:09 ` [PATCH v6 6/6] " Eric Sunshine
@ 2018-04-08 14:30 ` Thomas Gummerer
0 siblings, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-08 14:30 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On 04/08, Eric Sunshine wrote:
> On Sat, Mar 31, 2018 at 11:18 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > [...]
> > However we can do a little better than that, and check the branch out if
> > it is not checked out anywhere else. This will help users who just want
> > to check an existing branch out into a new worktree, and save a few
> > keystrokes.
> > [...]
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> > @@ -61,8 +61,13 @@ $ git worktree add --track -b <branch> <path> <remote>/<branch>
> > If `<commit-ish>` is omitted and neither `-b` nor `-B` nor `--detach` used,
> > +then, as a convenience, a worktree with a branch named after
> > +`$(basename <path>)` (call it `<branch>`) is created.
>
> I had a hard time digesting this. I _think_ it wants to say:
>
> If `<commit-ish>` is omitted and neither `-b` nor `-B` nor
> `--detach` is used, then, as a convenience, the new worktree is
> associated with a branch (call it `<branch>`) named after
> `$(basename <path>)`.
Yeah, this is what it wants to say, and what you have sounds much
nicer, will change.
> > If `<branch>`
> > +doesn't exist, a new branch based on HEAD is automatically created as
> > +if `-b <branch>` was given. If `<branch>` exists in the repository,
>
> Maybe: s/exists in the repository/does exist/
> Or: s/.../is a local branch/
>
> Though, the latter may be getting too pedantic.
>
> > +it will be checked out in the new worktree, if it's not checked out
> > +anywhere else, otherwise the command will refuse to create the
> > +worktree (unless `--force` is used).
> > diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> > @@ -198,13 +198,24 @@ test_expect_success '"add" with <branch> omitted' '
> > +test_expect_success '"add" checks out existing branch of dwimd name' '
> > + git branch dwim HEAD~1 &&
> > + git worktree add dwim &&
> > + test_cmp_rev HEAD~1 dwim &&
> > + (
> > + cd dwim &&
> > + test_cmp_rev dwim HEAD
>
> Nit: Argument order of the two test_cmp_rev() invocations differs.
>
> > + )
> > +'
> > +
> > +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
> > +'
> > +
> > +test_expect_success '"add --force" with existing dwimd name doesnt die' '
> > + git worktree add --force test-branch
> > '
>
> Maybe make this last test slightly more robust by having it "git
> checkout test-branch" before "git worktree add ..." to protect against
> someone inserting a new test (which checks out some other branch)
> between these two. Probably not worth a re-roll.
As I'll have to re-roll anyway for the other suggestions, I'll change
this as well.
Thanks for your review!
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
2018-03-31 15:17 ` [PATCH v6 " Thomas Gummerer
` (5 preceding siblings ...)
2018-03-31 15:18 ` [PATCH v6 6/6] worktree: teach "add" to check out existing branches Thomas Gummerer
@ 2018-04-08 9:08 ` Eric Sunshine
2018-04-08 14:24 ` Thomas Gummerer
2018-04-09 19:30 ` Thomas Gummerer
2018-04-15 20:29 ` [PATCH v7 0/4] " Thomas Gummerer
7 siblings, 2 replies; 112+ messages in thread
From: Eric Sunshine @ 2018-04-08 9:08 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> This round should fix all the UI issues Eric found in the last round.
> The changes I made in a bit more detail:
>
> - 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.
As with Junio, I'm fine with this hidden option (for now), however, I
think you can take this a step further. Rather than having a (hidden)
git-reset option which suppresses "HEAD is now at...", instead have a
(hidden) option which augments the message. For example,
--new-head-desc="New worktree" would make it output "New worktree HEAD
is now at...". Changes to builtin/reset.c to support this would hardly
be larger than the changes you already made.
The major benefit is that patch 3/6 no longer has to duplicate the
code from builtin/reset.c:print_new_head_line() just to print its own
"New worktree HEAD is now at..." message. (As for the argument that
"git worktree add" must duplicate that code because it wants the
message on stderr, whereas git-reset prints it to stdout, I don't see
why git-worktree puts those messages to stderr in the first place. As
far as I can tell, it would be equally valid to print them to stdout.)
> 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'
I like the "creating" or "checking out" messages we now get for all
the DWIM cases. I wonder if it would make sense to print "Checkout out
blah..." for this case too. It's certainly not necessary since the
user specified <commit-ish> explicitly, but it would make the UI even
more consistent, and address your subsequent comment about missing
context above the "Checking out files: ...%" line for this case.
Thoughts?
> Compare this to the old UI (new dwim omitted, as there's no old
> version of that):
Thanks for contrasting the new with the old. The new output is nicer
and more helpful.
> The 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.
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
2018-04-08 9:08 ` [PATCH v6 0/6] " Eric Sunshine
@ 2018-04-08 14:24 ` Thomas Gummerer
2018-04-09 0:38 ` Eric Sunshine
2018-04-09 19:30 ` Thomas Gummerer
1 sibling, 1 reply; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-08 14:24 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On 04/08, Eric Sunshine wrote:
> On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > This round should fix all the UI issues Eric found in the last round.
> > The changes I made in a bit more detail:
> >
> > - 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.
>
> As with Junio, I'm fine with this hidden option (for now), however, I
> think you can take this a step further. Rather than having a (hidden)
> git-reset option which suppresses "HEAD is now at...", instead have a
> (hidden) option which augments the message. For example,
> --new-head-desc="New worktree" would make it output "New worktree HEAD
> is now at...". Changes to builtin/reset.c to support this would hardly
> be larger than the changes you already made.
>
> The major benefit is that patch 3/6 no longer has to duplicate the
> code from builtin/reset.c:print_new_head_line() just to print its own
> "New worktree HEAD is now at..." message. (As for the argument that
> "git worktree add" must duplicate that code because it wants the
> message on stderr, whereas git-reset prints it to stdout, I don't see
> why git-worktree puts those messages to stderr in the first place. As
> far as I can tell, it would be equally valid to print them to stdout.)
I didn't think of that, but I think that's nicer indeed. Will change.
This will also be nicer when we're in a position to remove the hidden
option and do all of this with internal functions. Then we can just
re-use the new function that also takes a prefix at that point (after
moving the function to 'libgit.a' of course.
> > 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'
>
> I like the "creating" or "checking out" messages we now get for all
> the DWIM cases. I wonder if it would make sense to print "Checkout out
> blah..." for this case too. It's certainly not necessary since the
> user specified <commit-ish> explicitly, but it would make the UI even
> more consistent, and address your subsequent comment about missing
> context above the "Checking out files: ...%" line for this case.
> Thoughts?
Let me think through some of the cases here, of 'git worktre add
<path> <commit-ish>' with various flags and what the UI would be with
that added:
- no flags:
$ git worktree add ../test origin/master
Checking out 'origin/master'
Checking out files: ...%
New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
- -b branch:
$ git worktree add -b test ../test origin/master
Creating branch 'test'
Checking out 'origin/master'
Checking out files: ...%
New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
Would we want to omit the "Checking out ..." here? I'm leaning
towards yes, but dunno?
- --no-checkout
$ git worktree add --no-checkout test ../test origin/master
New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
- Original dwim with --detach flag
$ git worktree add --detach ../test
Checking out 'c2a499e6c'
Checking out files: ...%
New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
Looking at this, I'm not sure what's best here. I'm not sure I'm a
fan of the duplicate "Checking out " message (I assume that's what you
meant above, or did you mean just "Checkout ..."?)
I als don't think it gives too much context compared to just "Checking
out files: ...%". I think it gives a bit more context when that
message is not displayed at all, as it shows whether files are checked
out or not, but if we do that, when we create a new branch, the amount
of output we'd display is getting a bit long, to the point where I
suspect users would just not read it anymore.
So I personally don't feel like this is worth it, even though it may
give some context in some cases. But I'm also far from an expert in
UI design, so if you (or others) feel this would be nicer I'm happy to
implement it :)
> > Compare this to the old UI (new dwim omitted, as there's no old
> > version of that):
>
> Thanks for contrasting the new with the old. The new output is nicer
> and more helpful.
>
> > The 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.
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
2018-04-08 14:24 ` Thomas Gummerer
@ 2018-04-09 0:38 ` Eric Sunshine
2018-04-09 19:47 ` Thomas Gummerer
0 siblings, 1 reply; 112+ messages in thread
From: Eric Sunshine @ 2018-04-09 0:38 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On Sun, Apr 8, 2018 at 10:24 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> On 04/08, Eric Sunshine wrote:
>> On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> Let me think through some of the cases here, of 'git worktre add
> <path> <commit-ish>' with various flags and what the UI would be with
> that added:
>
> - no flags:
>
> $ git worktree add ../test origin/master
> Checking out 'origin/master'
> Checking out files: ...%
> New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
>
> - -b branch:
>
> $ git worktree add -b test ../test origin/master
> Creating branch 'test'
> Checking out 'origin/master'
Did you mean "Checking out 'test'"?
> Checking out files: ...%
> New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
>
> Would we want to omit the "Checking out ..." here? I'm leaning
> towards yes, but dunno?
To which "Checking out" message do you refer, the one showing the
branch name or the one showing the checkout progress?
I'd probably agree that showing both "Creating" and "Checkout out" is
overkill. However, see my response[1] to your "fixup!" patch in which
I explore the idea that unifying "Checking out 'branch' and "Creating
branch" messages may be a good idea and get us out of some UI jams
which seem to be cropping up.
[1]: https://public-inbox.org/git/20180325134947.25828-1-t.gummerer@gmail.com/T/#m5d38b0c6427609e8c36aa6af83d518791c1e1581
> - Original dwim with --detach flag
>
> $ git worktree add --detach ../test
> Checking out 'c2a499e6c'
> Checking out files: ...%
> New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
>
> Looking at this, I'm not sure what's best here. I'm not sure I'm a
> fan of the duplicate "Checking out " message (I assume that's what you
> meant above, or did you mean just "Checkout ..."?)
Taking [1] into account, this might become something like:
$ git worktree add --detach ../test
Preparing worktree (detached HEAD c2a499e6c)
Checking out files: ...%
New worktree HEAD is now at c2a499e6c Gobbledygook
> I als don't think it gives too much context compared to just "Checking
> out files: ...%". I think it gives a bit more context when that
> message is not displayed at all, as it shows whether files are checked
> out or not, but if we do that, when we create a new branch, the amount
> of output we'd display is getting a bit long, to the point where I
> suspect users would just not read it anymore.
>
> So I personally don't feel like this is worth it, even though it may
> give some context in some cases.
Fair enough observation. The idea suggested in [1] may keep output to
a reasonable amount.
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
2018-04-09 0:38 ` Eric Sunshine
@ 2018-04-09 19:47 ` Thomas Gummerer
0 siblings, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-09 19:47 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On 04/08, Eric Sunshine wrote:
> On Sun, Apr 8, 2018 at 10:24 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > On 04/08, Eric Sunshine wrote:
> >> On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > Let me think through some of the cases here, of 'git worktre add
> > <path> <commit-ish>' with various flags and what the UI would be with
> > that added:
> >
> > - no flags:
> >
> > $ git worktree add ../test origin/master
> > Checking out 'origin/master'
> > Checking out files: ...%
> > New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
> >
> > - -b branch:
> >
> > $ git worktree add -b test ../test origin/master
> > Creating branch 'test'
> > Checking out 'origin/master'
>
> Did you mean "Checking out 'test'"?
>
> > Checking out files: ...%
> > New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
> >
> > Would we want to omit the "Checking out ..." here? I'm leaning
> > towards yes, but dunno?
>
> To which "Checking out" message do you refer, the one showing the
> branch name or the one showing the checkout progress?
>
> I'd probably agree that showing both "Creating" and "Checkout out" is
> overkill. However, see my response[1] to your "fixup!" patch in which
> I explore the idea that unifying "Checking out 'branch' and "Creating
> branch" messages may be a good idea and get us out of some UI jams
> which seem to be cropping up.
>
> [1]: https://public-inbox.org/git/20180325134947.25828-1-t.gummerer@gmail.com/T/#m5d38b0c6427609e8c36aa6af83d518791c1e1581
>
> > - Original dwim with --detach flag
> >
> > $ git worktree add --detach ../test
> > Checking out 'c2a499e6c'
> > Checking out files: ...%
> > New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
> >
> > Looking at this, I'm not sure what's best here. I'm not sure I'm a
> > fan of the duplicate "Checking out " message (I assume that's what you
> > meant above, or did you mean just "Checkout ..."?)
>
> Taking [1] into account, this might become something like:
>
> $ git worktree add --detach ../test
> Preparing worktree (detached HEAD c2a499e6c)
> Checking out files: ...%
> New worktree HEAD is now at c2a499e6c Gobbledygook
The more I look at this solution, the more I like it. I'll try to
implement this to see if there's anything I'm not thinking of right
now, but I think I'll take the suggestion and send a re-roll with it
implemented.
> > I als don't think it gives too much context compared to just "Checking
> > out files: ...%". I think it gives a bit more context when that
> > message is not displayed at all, as it shows whether files are checked
> > out or not, but if we do that, when we create a new branch, the amount
> > of output we'd display is getting a bit long, to the point where I
> > suspect users would just not read it anymore.
> >
> > So I personally don't feel like this is worth it, even though it may
> > give some context in some cases.
>
> Fair enough observation. The idea suggested in [1] may keep output to
> a reasonable amount.
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
2018-04-08 9:08 ` [PATCH v6 0/6] " Eric Sunshine
2018-04-08 14:24 ` Thomas Gummerer
@ 2018-04-09 19:30 ` Thomas Gummerer
2018-04-09 22:06 ` Eric Sunshine
1 sibling, 1 reply; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-09 19:30 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On 04/08, Eric Sunshine wrote:
> On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > This round should fix all the UI issues Eric found in the last round.
> > The changes I made in a bit more detail:
> >
> > - 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.
>
> As with Junio, I'm fine with this hidden option (for now), however, I
> think you can take this a step further. Rather than having a (hidden)
> git-reset option which suppresses "HEAD is now at...", instead have a
> (hidden) option which augments the message. For example,
> --new-head-desc="New worktree" would make it output "New worktree HEAD
> is now at...". Changes to builtin/reset.c to support this would hardly
> be larger than the changes you already made.
Something else I just noticed that may make this a worse solution is
that this breaks the sentence in two pieces for translators. I guess
we could somehow get the "New worktree" part of the option translated,
but that still means that if some language would require to move parts
of the sentence around that would be less than ideal for translation.
Duy pointed this out to me in an earlier patch series, and I think we
should probably not make life harder (or impossible) for translators
if we can avoid it.
Would factoring out what we have in 'print_new_head_line()' into some
common code, maybe in 'pretty.c', and still doing the printing from
here be a reasonable tradeoff?
I think this could potentially even be re-used in other places,
although again I'd like to keep that for a followup series to avoid
scope creep in this one.
> The major benefit is that patch 3/6 no longer has to duplicate the
> code from builtin/reset.c:print_new_head_line() just to print its own
> "New worktree HEAD is now at..." message. (As for the argument that
> "git worktree add" must duplicate that code because it wants the
> message on stderr, whereas git-reset prints it to stdout, I don't see
> why git-worktree puts those messages to stderr in the first place. As
> far as I can tell, it would be equally valid to print them to stdout.)
>
> > 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'
>
> I like the "creating" or "checking out" messages we now get for all
> the DWIM cases. I wonder if it would make sense to print "Checkout out
> blah..." for this case too. It's certainly not necessary since the
> user specified <commit-ish> explicitly, but it would make the UI even
> more consistent, and address your subsequent comment about missing
> context above the "Checking out files: ...%" line for this case.
> Thoughts?
>
> > Compare this to the old UI (new dwim omitted, as there's no old
> > version of that):
>
> Thanks for contrasting the new with the old. The new output is nicer
> and more helpful.
>
> > The 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.
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
2018-04-09 19:30 ` Thomas Gummerer
@ 2018-04-09 22:06 ` Eric Sunshine
2018-04-11 20:09 ` Thomas Gummerer
0 siblings, 1 reply; 112+ messages in thread
From: Eric Sunshine @ 2018-04-09 22:06 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On Mon, Apr 9, 2018 at 3:30 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> On 04/08, Eric Sunshine wrote:
>> As with Junio, I'm fine with this hidden option (for now), however, I
>> think you can take this a step further. Rather than having a (hidden)
>> git-reset option which suppresses "HEAD is now at...", instead have a
>> (hidden) option which augments the message. For example,
>> --new-head-desc="New worktree" would make it output "New worktree HEAD
>> is now at...". Changes to builtin/reset.c to support this would hardly
>> be larger than the changes you already made.
>
> Something else I just noticed that may make this a worse solution is
> that this breaks the sentence in two pieces for translators. I guess
> we could somehow get the "New worktree" part of the option translated,
> but that still means that if some language would require to move parts
> of the sentence around that would be less than ideal for translation.
Good point.
One solution would be to have the new hidden option replace the string
entirely: --new-head-msg="New worktree HEAD is now at %s", which would
allow translators to deal with the entire sentence. Even clearer would
be to drop "now", as in "New worktree HEAD is at %s". (Default in
reset.c would still be "HEAD is now at %s", of course.)
Another solution would be not to augment the "HEAD is now at..."
message at all. I realize that that augmentation was one of the
original motivations for this patch series, but with the upcoming
restoration of the "Preparing worktree" message:
Preparing worktree (_branch disposition_)
HEAD is now at ...
it seems clear enough (at least to me) from the context introduced by
the "Preparing..." message that "HEAD is now at..." refers to HEAD in
the worktree. (But that's just my opinion.)
> Would factoring out what we have in 'print_new_head_line()' into some
> common code, maybe in 'pretty.c', and still doing the printing from
> here be a reasonable tradeoff?
Isn't that getting uglier again? Not only would you have to publish
that function, but you'd still need the hidden git-reset
--show-new-head-line option.
Also, you'd end up calling that function from within low-level worker
worktree.c:add_worktree(), thus making it take on UI duties, which
would be nice to avoid if possible. (Unfortunately, the alternate idea
of having worktree.c:add() handle this UI task doesn't quite work
since add_worktree() doesn't return sufficient information for add()
to know whether or not it should print "HEAD is now at..."; however,
add_worktree() could be augmented to return more information.)
So, I don't presently have a good answer. I'm partial to the idea of
not augmenting "HEAD is now at...", partly because context makes it
relatively clear that it applies to the new worktree and partly
because it's simpler (less implementation work for you) to leave it as
is. If that choice isn't desirable, then next best would be for
--new-head-msg= to replace the entire "HEAD is now at..." string
rather than augmenting it.
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
2018-04-09 22:06 ` Eric Sunshine
@ 2018-04-11 20:09 ` Thomas Gummerer
2018-04-11 20:48 ` Eric Sunshine
2018-04-11 20:50 ` Thomas Gummerer
0 siblings, 2 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-11 20:09 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On 04/09, Eric Sunshine wrote:
> On Mon, Apr 9, 2018 at 3:30 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > On 04/08, Eric Sunshine wrote:
> >> As with Junio, I'm fine with this hidden option (for now), however, I
> >> think you can take this a step further. Rather than having a (hidden)
> >> git-reset option which suppresses "HEAD is now at...", instead have a
> >> (hidden) option which augments the message. For example,
> >> --new-head-desc="New worktree" would make it output "New worktree HEAD
> >> is now at...". Changes to builtin/reset.c to support this would hardly
> >> be larger than the changes you already made.
> >
> > Something else I just noticed that may make this a worse solution is
> > that this breaks the sentence in two pieces for translators. I guess
> > we could somehow get the "New worktree" part of the option translated,
> > but that still means that if some language would require to move parts
> > of the sentence around that would be less than ideal for translation.
>
> Good point.
>
> One solution would be to have the new hidden option replace the string
> entirely: --new-head-msg="New worktree HEAD is now at %s", which would
> allow translators to deal with the entire sentence. Even clearer would
> be to drop "now", as in "New worktree HEAD is at %s". (Default in
> reset.c would still be "HEAD is now at %s", of course.)
>
> Another solution would be not to augment the "HEAD is now at..."
> message at all. I realize that that augmentation was one of the
> original motivations for this patch series, but with the upcoming
> restoration of the "Preparing worktree" message:
My original motivation of the series was to just make the new dwim
work :) Because that's adding some magic, the secondary motivation
became improving the UI, to help users see which dwim was used. I
felt like this was going to be one of those improvements, especially
after we get rid of the "Preparing ..." line.
I do however like your suggestion of the "Preparing worktree (_branch
disposition_)", as that doesn't add more lines to the output, while
still giving a good indication of what exactly is happening. At that
point just showing "HEAD is now at ..." is fine by me, and doesn't
require adding the hidden flag to 'git reset'. So I'm happy just
dropping the change in the message here, which will simplify things.
Thanks for the suggestion!
> Preparing worktree (_branch disposition_)
> HEAD is now at ...
>
> it seems clear enough (at least to me) from the context introduced by
> the "Preparing..." message that "HEAD is now at..." refers to HEAD in
> the worktree. (But that's just my opinion.)
>
> > Would factoring out what we have in 'print_new_head_line()' into some
> > common code, maybe in 'pretty.c', and still doing the printing from
> > here be a reasonable tradeoff?
>
> Isn't that getting uglier again? Not only would you have to publish
> that function, but you'd still need the hidden git-reset
> --show-new-head-line option.
>
> Also, you'd end up calling that function from within low-level worker
> worktree.c:add_worktree(), thus making it take on UI duties, which
> would be nice to avoid if possible. (Unfortunately, the alternate idea
> of having worktree.c:add() handle this UI task doesn't quite work
> since add_worktree() doesn't return sufficient information for add()
> to know whether or not it should print "HEAD is now at..."; however,
> add_worktree() could be augmented to return more information.)
>
> So, I don't presently have a good answer. I'm partial to the idea of
> not augmenting "HEAD is now at...", partly because context makes it
> relatively clear that it applies to the new worktree and partly
> because it's simpler (less implementation work for you) to leave it as
> is. If that choice isn't desirable, then next best would be for
> --new-head-msg= to replace the entire "HEAD is now at..." string
> rather than augmenting it.
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
2018-04-11 20:09 ` Thomas Gummerer
@ 2018-04-11 20:48 ` Eric Sunshine
2018-04-11 20:50 ` Thomas Gummerer
1 sibling, 0 replies; 112+ messages in thread
From: Eric Sunshine @ 2018-04-11 20:48 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On Wed, Apr 11, 2018 at 4:09 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> On 04/09, Eric Sunshine wrote:
>> Another solution would be not to augment the "HEAD is now at..."
>> message at all. I realize that that augmentation was one of the
>> original motivations for this patch series, but with the upcoming
>> restoration of the "Preparing worktree" message:
>
> My original motivation of the series was to just make the new dwim
> work :) Because that's adding some magic, the secondary motivation
> became improving the UI, to help users see which dwim was used. I
> felt like this was going to be one of those improvements, especially
> after we get rid of the "Preparing ..." line.
>
> I do however like your suggestion of the "Preparing worktree (_branch
> disposition_)", as that doesn't add more lines to the output, while
> still giving a good indication of what exactly is happening. At that
> point just showing "HEAD is now at ..." is fine by me, and doesn't
> require adding the hidden flag to 'git reset'. So I'm happy just
> dropping the change in the message here, which will simplify things.
Nice. This sounds like a good plan for moving forward.
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
2018-04-11 20:09 ` Thomas Gummerer
2018-04-11 20:48 ` Eric Sunshine
@ 2018-04-11 20:50 ` Thomas Gummerer
2018-04-11 21:14 ` Eric Sunshine
1 sibling, 1 reply; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-11 20:50 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On 04/11, Thomas Gummerer wrote:
> On 04/09, Eric Sunshine wrote:
> > On Mon, Apr 9, 2018 at 3:30 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > > On 04/08, Eric Sunshine wrote:
> > >> As with Junio, I'm fine with this hidden option (for now), however, I
> > >> think you can take this a step further. Rather than having a (hidden)
> > >> git-reset option which suppresses "HEAD is now at...", instead have a
> > >> (hidden) option which augments the message. For example,
> > >> --new-head-desc="New worktree" would make it output "New worktree HEAD
> > >> is now at...". Changes to builtin/reset.c to support this would hardly
> > >> be larger than the changes you already made.
> > >
> > > Something else I just noticed that may make this a worse solution is
> > > that this breaks the sentence in two pieces for translators. I guess
> > > we could somehow get the "New worktree" part of the option translated,
> > > but that still means that if some language would require to move parts
> > > of the sentence around that would be less than ideal for translation.
> >
> > Good point.
> >
> > One solution would be to have the new hidden option replace the string
> > entirely: --new-head-msg="New worktree HEAD is now at %s", which would
> > allow translators to deal with the entire sentence. Even clearer would
> > be to drop "now", as in "New worktree HEAD is at %s". (Default in
> > reset.c would still be "HEAD is now at %s", of course.)
> >
> > Another solution would be not to augment the "HEAD is now at..."
> > message at all. I realize that that augmentation was one of the
> > original motivations for this patch series, but with the upcoming
> > restoration of the "Preparing worktree" message:
>
> My original motivation of the series was to just make the new dwim
> work :) Because that's adding some magic, the secondary motivation
> became improving the UI, to help users see which dwim was used. I
> felt like this was going to be one of those improvements, especially
> after we get rid of the "Preparing ..." line.
>
> I do however like your suggestion of the "Preparing worktree (_branch
> disposition_)", as that doesn't add more lines to the output, while
> still giving a good indication of what exactly is happening. At that
> point just showing "HEAD is now at ..." is fine by me, and doesn't
> require adding the hidden flag to 'git reset'. So I'm happy just
> dropping the change in the message here, which will simplify things.
And just as I'm re-reading my commit messages, I guess there was one
more motivation for printing the "HEAD is now at ..." line ourselves:
If the '--no-checkout' flag is given, the output of 'git worktree add'
is just:
Preparing foo (identifier foo)
even though the HEAD is set to a commit, which is just not checked out.
I think I can live with that for now, I personally don't use
'--no-checkout', and we could fix this at some point in the future if
we desire to do so. I think we can consider that out of scope for
this patch series, as its main goal is to introduce the new dwim.
> Thanks for the suggestion!
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
2018-04-11 20:50 ` Thomas Gummerer
@ 2018-04-11 21:14 ` Eric Sunshine
0 siblings, 0 replies; 112+ messages in thread
From: Eric Sunshine @ 2018-04-11 21:14 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On Wed, Apr 11, 2018 at 4:50 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> And just as I'm re-reading my commit messages, I guess there was one
> more motivation for printing the "HEAD is now at ..." line ourselves:
>
> If the '--no-checkout' flag is given, the output of 'git worktree add'
> is just:
>
> Preparing foo (identifier foo)
>
> even though the HEAD is set to a commit, which is just not checked out.
>
> I think I can live with that for now, I personally don't use
> '--no-checkout', and we could fix this at some point in the future if
> we desire to do so. I think we can consider that out of scope for
> this patch series, as its main goal is to introduce the new dwim.
Sounds reasonable, especially since the proposed "Preparing worktree
(_branch description_)" provides similarly useful feedback.
^ permalink raw reply [flat|nested] 112+ messages in thread
* [PATCH v7 0/4] worktree: teach "add" to check out existing branches
2018-03-31 15:17 ` [PATCH v6 " Thomas Gummerer
` (6 preceding siblings ...)
2018-04-08 9:08 ` [PATCH v6 0/6] " Eric Sunshine
@ 2018-04-15 20:29 ` Thomas Gummerer
2018-04-15 20:29 ` [PATCH v7 1/4] worktree: remove extra members from struct add_opts Thomas Gummerer
` (5 more replies)
7 siblings, 6 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-15 20:29 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
Thanks Eric for the review and all of the suggestions in 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>,
<20180325134947.25828-1-t.gummerer@gmail.com> and
<20180331151804.30380-1-t.gummerer@gmail.com>.
The main change once again in this series is the user interface. It
feels like we went in a complete circle here now, as this iteration is
bringing the "Preparing ..." line back (although in a slightly
different form than the original), and is moving away from printing
it's own "HEAD is now at..." line again. This also means we don't
need the new hidden option to 'git reset' anymore, which is nice.
I do like the new UI more than what we had in the last round (which I
already liked more than the original UI) :)
Other than those changes, it also includes Eric's suggestion for a
better wording in the documentation, fixes the argument order to
test_cmd_rev in a test, and makes a test more robust.
To demonstrate the UI updates, let's compare the new UI and the old UI
again. This is the same commands as used for the demonstration in the
last iteration, so please have a look at <20180331151804.30380-1-t.gummerer@gmail.com>
for an example of what it looked like after the last round.
New UI:
- guess-remote mode
$ git worktree add --guess-remote ../next
Preparing worktree (new branch 'next')
Branch 'next' set up to track remote branch 'next' from 'origin'.
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 worktree (new branch 'test')
HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
- new dwim (check out existing branch)
$ git worktree add ../test
Preparing worktree (checking out 'test')
HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
- detached HEAD
$ git worktree add ../test2 origin/master
Preparing worktree (detached HEAD c2a499e6c)
HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
- resetting existing branch
$ git worktree add -B next ../test3 origin/master
Preparing worktree (resetting branch 'next' (was at caa68db14))
Branch 'next' set up to track remote branch 'master' from 'origin'.
HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
This output is new in this round and wasn't previously discussed.
While working on the "Preparing ..." line I noticed this, and
thought it would be a good idea. I feel like this fits in the
scope of the series quite well, as it's improving the UI, but I'm
happy to split it out if that's preferred.
It may also be worth displaying the title of the commit here, but
at that point the line would get a bit long, so dunno.
- large repository (with original dwim)
$ git worktree add ../test
Preparing worktree (new branch '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-dm
- error when directory already exists
$ git worktree add ../test
Preparing worktree (checking out 'test')
fatal: '../test' already exists
Compare 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-dm
- error when directory already exists
$ git worktree add ../test
fatal: '../test' already exists
Interdiff below:
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index eaa6bf713f..5d54f36a71 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -61,13 +61,13 @@ $ git worktree add --track -b <branch> <path> <remote>/<branch>
------------
+
If `<commit-ish>` is omitted and neither `-b` nor `-B` nor `--detach` used,
-then, as a convenience, a worktree with a branch named after
-`$(basename <path>)` (call it `<branch>`) is created. If `<branch>`
+then, as a convenience, the new worktree is associated with a branch
+(call it `<branch>`) named after `$(basename <path>)`. If `<branch>`
doesn't exist, a new branch based on HEAD is automatically created as
-if `-b <branch>` was given. If `<branch>` exists in the repository,
-it will be checked out in the new worktree, if it's not checked out
-anywhere else, otherwise the command will refuse to create the
-worktree (unless `--force` is used).
+if `-b <branch>` was given. If `<branch>` does exist, it will be
+checked out in the new worktree, if it's not checked out anywhere
+else, otherwise the command will refuse to create the worktree (unless
+`--force` is used).
list::
diff --git a/builtin/reset.c b/builtin/reset.c
index 54b2576449..e15f595799 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -288,7 +288,6 @@ 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;
@@ -311,7 +310,6 @@ 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()
};
@@ -405,8 +403,7 @@ 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 &&
- show_new_head_line)
+ if (reset_type == HARD && !update_ref_status && !quiet)
print_new_head_line(lookup_commit_reference(&oid));
}
if (!pathspec.nr)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index ccc2e63e0f..f5a5283b39 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -27,7 +27,6 @@ struct add_opts {
int detach;
int checkout;
int keep_locked;
- int checkout_existing_branch;
};
static int show_only;
@@ -317,28 +316,16 @@ 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);
if (opts->checkout) {
cp.argv = NULL;
argv_array_clear(&cp.args);
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);
@@ -366,6 +353,34 @@ static int add_worktree(const char *path, const char *refname,
return ret;
}
+static void print_preparing_worktree_line(const char *branch,
+ const char *new_branch,
+ const char *new_branch_force,
+ int checkout_existing_branch)
+{
+ if (checkout_existing_branch) {
+ printf_ln(_("Preparing worktree (checking out '%s')"), branch);
+ } else if (new_branch_force) {
+ struct commit *commit = lookup_commit_reference_by_name(new_branch_force);
+ if (!commit)
+ printf_ln(_("Preparing worktree (new branch '%s')"), new_branch_force);
+ else
+ printf_ln(_("Preparing worktree (resetting branch '%s' (was at %s))"),
+ new_branch_force,
+ find_unique_abbrev(commit->object.oid.hash,
+ DEFAULT_ABBREV));
+ } else if (new_branch) {
+ printf_ln(_("Preparing worktree (new branch '%s')"), new_branch);
+ } else {
+ struct commit *commit = lookup_commit_reference_by_name(branch);
+ if (!commit)
+ die(_("invalid reference: %s"), branch);
+ printf_ln(_("Preparing worktree (detached HEAD %s)"),
+ find_unique_abbrev(commit->object.oid.hash,
+ DEFAULT_ABBREV));
+ }
+}
+
static const char *dwim_branch(const char *path, const char **new_branch,
int *checkout_existing_branch)
{
@@ -397,6 +412,7 @@ static int add(int ac, const char **av, const char *prefix)
struct add_opts opts;
const char *new_branch_force = NULL;
char *path;
+ int checkout_existing_branch = 0;
const char *branch;
const char *new_branch = NULL;
const char *opt_track = NULL;
@@ -445,7 +461,7 @@ static int add(int ac, const char **av, const char *prefix)
if (ac < 2 && !new_branch && !opts.detach) {
const char *s = dwim_branch(path, &new_branch,
- &opts.checkout_existing_branch);
+ &checkout_existing_branch);
if (s)
branch = s;
}
@@ -465,10 +481,11 @@ static int add(int ac, const char **av, const char *prefix)
}
}
+ print_preparing_worktree_line(branch, new_branch, new_branch_force,
+ checkout_existing_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)
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index f72cb0eb0b..ad38507d00 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -204,7 +204,7 @@ test_expect_success '"add" checks out existing branch of dwimd name' '
test_cmp_rev HEAD~1 dwim &&
(
cd dwim &&
- test_cmp_rev dwim HEAD
+ test_cmp_rev HEAD dwim
)
'
@@ -215,6 +215,7 @@ test_expect_success '"add <path>" dwim fails with checked out branch' '
'
test_expect_success '"add --force" with existing dwimd name doesnt die' '
+ git checkout test-branch &&
git worktree add --force test-branch
'
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index a160f78aba..95653a08ca 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -568,9 +568,4 @@ 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 (4):
worktree: remove extra members from struct add_opts
worktree: improve message when creating a new worktree
worktree: factor out dwim_branch function
worktree: teach "add" to check out existing branches
Documentation/git-worktree.txt | 9 +++-
builtin/worktree.c | 101 ++++++++++++++++++++++++++++++-----------
t/t2025-worktree-add.sh | 26 ++++++++---
3 files changed, 100 insertions(+), 36 deletions(-)
--
2.16.1.74.g6cd9b6cbe3
^ permalink raw reply related [flat|nested] 112+ messages in thread
* [PATCH v7 1/4] worktree: remove extra members from struct add_opts
2018-04-15 20:29 ` [PATCH v7 0/4] " Thomas Gummerer
@ 2018-04-15 20:29 ` Thomas Gummerer
2018-04-15 20:29 ` [PATCH v7 2/4] worktree: improve message when creating a new worktree Thomas Gummerer
` (4 subsequent siblings)
5 siblings, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-15 20:29 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
There are two members of 'struct add_opts', which are only used inside
the 'add()' function, but being part of 'struct add_opts' they are
needlessly also passed to the 'add_worktree' function.
Make them local to the 'add()' function to make it clearer where they
are used.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/worktree.c | 32 +++++++++++++++-----------------
1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7cef5b120b..4d96a21a45 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 force_new_branch;
};
static int show_only;
@@ -363,10 +361,11 @@ 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;
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")),
@@ -384,7 +383,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);
@@ -395,33 +394,32 @@ static int add(int ac, const char **av, const char *prefix)
if (!strcmp(branch, "-"))
branch = "@{-1}";
- opts.force_new_branch = !!new_branch_force;
- if (opts.force_new_branch) {
+ 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) {
+ if (ac < 2 && !new_branch && !opts.detach) {
int n;
const char *s = worktree_basename(path, &n);
- opts.new_branch = xstrndup(s, n);
+ new_branch = xstrndup(s, n);
if (guess_remote) {
struct object_id oid;
const char *remote =
- unique_tracking_name(opts.new_branch, &oid);
+ unique_tracking_name(new_branch, &oid);
if (remote)
branch = remote;
}
}
- 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;
@@ -430,25 +428,25 @@ 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;
cp.git_cmd = 1;
argv_array_push(&cp.args, "branch");
- if (opts.force_new_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 (opt_track) {
die(_("--[no-]track can only be used if a new branch is created"));
}
--
2.17.0.252.gfe0a9eaf31
^ permalink raw reply related [flat|nested] 112+ messages in thread
* [PATCH v7 2/4] worktree: improve message when creating a new worktree
2018-04-15 20:29 ` [PATCH v7 0/4] " Thomas Gummerer
2018-04-15 20:29 ` [PATCH v7 1/4] worktree: remove extra members from struct add_opts Thomas Gummerer
@ 2018-04-15 20:29 ` Thomas Gummerer
2018-04-16 2:09 ` Junio C Hamano
2018-04-23 4:27 ` Eric Sunshine
2018-04-15 20:29 ` [PATCH v7 3/4] worktree: factor out dwim_branch function Thomas Gummerer
` (3 subsequent siblings)
5 siblings, 2 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-15 20:29 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
Currently 'git worktree add' produces output like the following:
Preparing ../foo (identifier foo)
HEAD is now at 26da330922 <title>
The '../foo' is the path where the worktree is created, which the user
has just given on the command line. The identifier is an internal
implementation detail, which is not particularly relevant for the user
and indeed isn't mentioned explicitly anywhere in the man page.
Instead of this message, print a message that gives the user a bit more
detail of what exactly 'git worktree' is doing. There are various dwim
modes which are perform some magic under the hood, which should be
helpful to users. Just from the output of the command it is not always
visible to users what exactly has happened.
Help the users a bit more by modifying the "Preparing ..." message and
adding some additional information of what 'git worktree add' did under
the hood, while not displaying the identifier anymore.
Currently this ends up in three different cases:
- 'git worktree add -b ...' or 'git worktree add <path>', both of
which create a new branch, either through the user explicitly
requesting it, or through 'git worktree add' implicitly creating
it. This will end up with the following output:
Preparing worktree (new branch '<branch>')
HEAD is now at 26da330922 <title>
- 'git worktree add -B ...', which may either create a new branch if
the branch with the given name does not exist yet, or resets an
existing branch to the current HEAD, or the commit-ish given.
Depending on which action is taken, we'll end up with the following
output:
Preparing worktree (resetting branch 'next' (was at caa68db14))
HEAD is now at 26da330922 <title>
or:
Preparing worktree (new branch '<branch>')
HEAD is now at 26da330922 <title>
- 'git worktree add --detach' or 'git worktree add <path> <branch>',
both of which create a new worktree with a detached HEAD, for which
we will print the following output:
Preparing worktree (detached HEAD 26da330922)
HEAD is now at 26da330922 <title>
Additionally currently the "Preparing ..." line is printed to stderr,
while the "HEAD is now at ..." line is printed to stdout by 'git reset
--hard', which is used internally by 'git worktree add'. Fix this
inconsistency by printing the "Preparing ..." message to stdout as
well. As "Preparing ..." is not an error, stdout also seems like the
more appropriate output stream.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/worktree.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 4d96a21a45..a2667d74fb 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -301,8 +301,6 @@ static int add_worktree(const char *path, const char *refname,
strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
write_file(sb.buf, "../..");
- fprintf_ln(stderr, _("Preparing %s (identifier %s)"), path, name);
-
argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
cp.git_cmd = 1;
@@ -355,6 +353,31 @@ static int add_worktree(const char *path, const char *refname,
return ret;
}
+static void print_preparing_worktree_line(const char *branch,
+ const char *new_branch,
+ const char *new_branch_force)
+{
+ if (new_branch_force) {
+ struct commit *commit = lookup_commit_reference_by_name(new_branch_force);
+ if (!commit)
+ printf_ln(_("Preparing worktree (new branch '%s')"), new_branch_force);
+ else
+ printf_ln(_("Preparing worktree (resetting branch '%s' (was at %s))"),
+ new_branch_force,
+ find_unique_abbrev(commit->object.oid.hash,
+ DEFAULT_ABBREV));
+ } else if (new_branch) {
+ printf_ln(_("Preparing worktree (new branch '%s')"), new_branch);
+ } else {
+ struct commit *commit = lookup_commit_reference_by_name(branch);
+ if (!commit)
+ die(_("invalid reference: %s"), branch);
+ printf_ln(_("Preparing worktree (detached HEAD %s)"),
+ find_unique_abbrev(commit->object.oid.hash,
+ DEFAULT_ABBREV));
+ }
+}
+
static int add(int ac, const char **av, const char *prefix)
{
struct add_opts opts;
@@ -434,6 +457,8 @@ static int add(int ac, const char **av, const char *prefix)
}
}
+ print_preparing_worktree_line(branch, new_branch, new_branch_force);
+
if (new_branch) {
struct child_process cp = CHILD_PROCESS_INIT;
cp.git_cmd = 1;
--
2.17.0.252.gfe0a9eaf31
^ permalink raw reply related [flat|nested] 112+ messages in thread
* Re: [PATCH v7 2/4] worktree: improve message when creating a new worktree
2018-04-15 20:29 ` [PATCH v7 2/4] worktree: improve message when creating a new worktree Thomas Gummerer
@ 2018-04-16 2:09 ` Junio C Hamano
2018-04-23 18:55 ` Thomas Gummerer
2018-04-23 4:27 ` Eric Sunshine
1 sibling, 1 reply; 112+ messages in thread
From: Junio C Hamano @ 2018-04-16 2:09 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: git, Eric Sunshine, Nguyễn Thái Ngọc Duy
Thomas Gummerer <t.gummerer@gmail.com> writes:
> Currently 'git worktree add' produces output like the following:
>
> Preparing ../foo (identifier foo)
> HEAD is now at 26da330922 <title>
>
> The '../foo' is the path where the worktree is created, which the user
> has just given on the command line. The identifier is an internal
> implementation detail, which is not particularly relevant for the user
> and indeed isn't mentioned explicitly anywhere in the man page.
OK. Maybe there once was a place or two that took the identifier as
an input to name a specific worktree to work on (perhaps "prune"?),
but if we no longer do that (which makes sense, as we should be able
to uniquely identify a worktree by the path to it), then it makes
perfect sense to prevent it from appearing in the UI.
> Instead of this message, print a message that gives the user a bit more
> detail of what exactly 'git worktree' is doing. There are various dwim
> modes which are perform some magic under the hood, which should be
s/are perform/perform/, I think (no need to reroll, only to fix this).
> Additionally currently the "Preparing ..." line is printed to stderr,
> while the "HEAD is now at ..." line is printed to stdout by 'git reset
> --hard', which is used internally by 'git worktree add'. Fix this
> inconsistency by printing the "Preparing ..." message to stdout as
> well. As "Preparing ..." is not an error, stdout also seems like the
> more appropriate output stream.
I think it is a bug for reset to give this kind of informational
message to the standard output stream. A rule of thumb I use is "is
this something that a user who wants quiet operation would wish to
squelch with --quiet option?" and if the answer is yes, it should go
to the standard error output, so info and progress should go to the
standard output in general.
But I am OK to unify to the standard output with this patch. We may
want to come back and correct _both_ this new message and what reset
says, but that is outside the scope of this topic.
Thanks.
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v7 2/4] worktree: improve message when creating a new worktree
2018-04-16 2:09 ` Junio C Hamano
@ 2018-04-23 18:55 ` Thomas Gummerer
0 siblings, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-23 18:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Sunshine, Nguyễn Thái Ngọc Duy
On 04/16, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > Currently 'git worktree add' produces output like the following:
> >
> > Preparing ../foo (identifier foo)
> > HEAD is now at 26da330922 <title>
> >
> > The '../foo' is the path where the worktree is created, which the user
> > has just given on the command line. The identifier is an internal
> > implementation detail, which is not particularly relevant for the user
> > and indeed isn't mentioned explicitly anywhere in the man page.
>
> OK. Maybe there once was a place or two that took the identifier as
> an input to name a specific worktree to work on (perhaps "prune"?),
> but if we no longer do that (which makes sense, as we should be able
> to uniquely identify a worktree by the path to it), then it makes
> perfect sense to prevent it from appearing in the UI.
>
> > Instead of this message, print a message that gives the user a bit more
> > detail of what exactly 'git worktree' is doing. There are various dwim
> > modes which are perform some magic under the hood, which should be
>
> s/are perform/perform/, I think (no need to reroll, only to fix this).
Now that I'm re-rolling anyway, I'll fix this as well, thanks.
> > Additionally currently the "Preparing ..." line is printed to stderr,
> > while the "HEAD is now at ..." line is printed to stdout by 'git reset
> > --hard', which is used internally by 'git worktree add'. Fix this
> > inconsistency by printing the "Preparing ..." message to stdout as
> > well. As "Preparing ..." is not an error, stdout also seems like the
> > more appropriate output stream.
>
> I think it is a bug for reset to give this kind of informational
> message to the standard output stream. A rule of thumb I use is "is
> this something that a user who wants quiet operation would wish to
> squelch with --quiet option?" and if the answer is yes, it should go
> to the standard error output, so info and progress should go to the
> standard output in general.
I assume you meant "standard error output" just above?
> But I am OK to unify to the standard output with this patch. We may
> want to come back and correct _both_ this new message and what reset
> says, but that is outside the scope of this topic.
Cool, I'd rather not add more moving pieces to this series, as it's
already at v7 (v8 soon), but I'm happy to have a look at this and fix
it on top once the dust settled on this series.
> Thanks.
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v7 2/4] worktree: improve message when creating a new worktree
2018-04-15 20:29 ` [PATCH v7 2/4] worktree: improve message when creating a new worktree Thomas Gummerer
2018-04-16 2:09 ` Junio C Hamano
@ 2018-04-23 4:27 ` Eric Sunshine
2018-04-23 18:50 ` Thomas Gummerer
1 sibling, 1 reply; 112+ messages in thread
From: Eric Sunshine @ 2018-04-23 4:27 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On Sun, Apr 15, 2018 at 4:29 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> Currently 'git worktree add' produces output like the following:
>
> Preparing ../foo (identifier foo)
> HEAD is now at 26da330922 <title>
> [...]
> Instead of this message, print a message that gives the user a bit more
> detail of what exactly 'git worktree' is doing. There are various dwim
> modes which are perform some magic under the hood, which should be
> helpful to users. Just from the output of the command it is not always
> visible to users what exactly has happened.
>
> Help the users a bit more by modifying the "Preparing ..." message and
> adding some additional information of what 'git worktree add' did under
> the hood, while not displaying the identifier anymore.
>
> Currently this ends up in three different cases:
>
> - 'git worktree add -b ...' or 'git worktree add <path>' [...]
>
> - 'git worktree add -B ...', which may either create a new branch if
> the branch with the given name does not exist yet, or resets an
> existing branch to the current HEAD, or the commit-ish given.
> Depending on which action is taken, we'll end up with the following
> output:
>
> Preparing worktree (resetting branch 'next' (was at caa68db14))
> HEAD is now at 26da330922 <title>
The (...) embedded inside another (...) is ugly and hard to read.
Better perhaps:
Preparing worktree (resetting branch 'next'; was at caa68db14)
Not necessarily worth a re-roll. It would be nice to see this series
land; perhaps this can be tweaked later.
> or:
>
> Preparing worktree (new branch '<branch>')
> HEAD is now at 26da330922 <title>
>
> - 'git worktree add --detach' or 'git worktree add <path> <branch>',
> both of which create a new worktree with a detached HEAD, for which
> we will print the following output:
>
> Preparing worktree (detached HEAD 26da330922)
> HEAD is now at 26da330922 <title>
This is inaccurate, isn't it? Certainly, specifying something like
"origin/floop" for <branch> ends up detached:
% git worktree add w1 origin/floop
...
% git worktree list
/proj fe0a9eaf31 [master]
/proj/w1 b46fe60e1d (detached HEAD)
but specifying an existing local branch (say "wip") does not end up detached:
% git worktree add w2 wip
...
% git worktree list
/proj fe0a9eaf31 [master]
/proj/w1 b46fe60e1d (detached HEAD)
/proj/w2 820ed2a513 [wip]
> Additionally currently the "Preparing ..." line is printed to stderr,
> while the "HEAD is now at ..." line is printed to stdout by 'git reset
> --hard', which is used internally by 'git worktree add'. Fix this
> inconsistency by printing the "Preparing ..." message to stdout as
> well. As "Preparing ..." is not an error, stdout also seems like the
> more appropriate output stream.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v7 2/4] worktree: improve message when creating a new worktree
2018-04-23 4:27 ` Eric Sunshine
@ 2018-04-23 18:50 ` Thomas Gummerer
0 siblings, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-23 18:50 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On 04/23, Eric Sunshine wrote:
> On Sun, Apr 15, 2018 at 4:29 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > Currently 'git worktree add' produces output like the following:
> >
> > Preparing ../foo (identifier foo)
> > HEAD is now at 26da330922 <title>
> > [...]
> > Instead of this message, print a message that gives the user a bit more
> > detail of what exactly 'git worktree' is doing. There are various dwim
> > modes which are perform some magic under the hood, which should be
> > helpful to users. Just from the output of the command it is not always
> > visible to users what exactly has happened.
> >
> > Help the users a bit more by modifying the "Preparing ..." message and
> > adding some additional information of what 'git worktree add' did under
> > the hood, while not displaying the identifier anymore.
> >
> > Currently this ends up in three different cases:
> >
> > - 'git worktree add -b ...' or 'git worktree add <path>' [...]
> >
> > - 'git worktree add -B ...', which may either create a new branch if
> > the branch with the given name does not exist yet, or resets an
> > existing branch to the current HEAD, or the commit-ish given.
> > Depending on which action is taken, we'll end up with the following
> > output:
> >
> > Preparing worktree (resetting branch 'next' (was at caa68db14))
> > HEAD is now at 26da330922 <title>
>
> The (...) embedded inside another (...) is ugly and hard to read.
> Better perhaps:
>
> Preparing worktree (resetting branch 'next'; was at caa68db14)
>
> Not necessarily worth a re-roll. It would be nice to see this series
> land; perhaps this can be tweaked later.
I'll tweak it while fixing the other bit.
> > or:
> >
> > Preparing worktree (new branch '<branch>')
> > HEAD is now at 26da330922 <title>
> >
> > - 'git worktree add --detach' or 'git worktree add <path> <branch>',
> > both of which create a new worktree with a detached HEAD, for which
> > we will print the following output:
> >
> > Preparing worktree (detached HEAD 26da330922)
> > HEAD is now at 26da330922 <title>
>
> This is inaccurate, isn't it? Certainly, specifying something like
> "origin/floop" for <branch> ends up detached:
Ah indeed, this was the case I missed. I thought I managed to go
through all of them, but this one slipped through the cracks. Thanks
for catching this, will fix in a re-roll.
> % git worktree add w1 origin/floop
> ...
> % git worktree list
> /proj fe0a9eaf31 [master]
> /proj/w1 b46fe60e1d (detached HEAD)
>
> but specifying an existing local branch (say "wip") does not end up detached:
>
> % git worktree add w2 wip
> ...
> % git worktree list
> /proj fe0a9eaf31 [master]
> /proj/w1 b46fe60e1d (detached HEAD)
> /proj/w2 820ed2a513 [wip]
>
> > Additionally currently the "Preparing ..." line is printed to stderr,
> > while the "HEAD is now at ..." line is printed to stdout by 'git reset
> > --hard', which is used internally by 'git worktree add'. Fix this
> > inconsistency by printing the "Preparing ..." message to stdout as
> > well. As "Preparing ..." is not an error, stdout also seems like the
> > more appropriate output stream.
> >
> > Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
^ permalink raw reply [flat|nested] 112+ messages in thread
* [PATCH v7 3/4] worktree: factor out dwim_branch function
2018-04-15 20:29 ` [PATCH v7 0/4] " Thomas Gummerer
2018-04-15 20:29 ` [PATCH v7 1/4] worktree: remove extra members from struct add_opts Thomas Gummerer
2018-04-15 20:29 ` [PATCH v7 2/4] worktree: improve message when creating a new worktree Thomas Gummerer
@ 2018-04-15 20:29 ` Thomas Gummerer
2018-04-15 20:29 ` [PATCH v7 4/4] worktree: teach "add" to check out existing branches Thomas Gummerer
` (2 subsequent siblings)
5 siblings, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-15 20:29 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
Factor out a dwim_branch function, which takes care of the dwim'ery in
'git worktree add <path>'. It's not too much code currently, but we're
adding a new kind of dwim in a subsequent patch, at which point it makes
more sense to have it as a separate function.
Factor it out now to reduce the patch noise in the next patch.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/worktree.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index a2667d74fb..893139629a 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -378,6 +378,20 @@ static void print_preparing_worktree_line(const char *branch,
}
}
+static const char *dwim_branch(const char *path, const char **new_branch)
+{
+ int n;
+ const char *s = worktree_basename(path, &n);
+ *new_branch = xstrndup(s, n);
+ if (guess_remote) {
+ struct object_id oid;
+ const char *remote =
+ unique_tracking_name(*new_branch, &oid);
+ return remote;
+ }
+ return NULL;
+}
+
static int add(int ac, const char **av, const char *prefix)
{
struct add_opts opts;
@@ -430,16 +444,9 @@ static int add(int ac, const char **av, const char *prefix)
}
if (ac < 2 && !new_branch && !opts.detach) {
- int n;
- const char *s = worktree_basename(path, &n);
- new_branch = xstrndup(s, n);
- if (guess_remote) {
- struct object_id oid;
- const char *remote =
- unique_tracking_name(new_branch, &oid);
- if (remote)
- branch = remote;
- }
+ const char *s = dwim_branch(path, &new_branch);
+ if (s)
+ branch = s;
}
if (ac == 2 && !new_branch && !opts.detach) {
--
2.17.0.252.gfe0a9eaf31
^ permalink raw reply related [flat|nested] 112+ messages in thread
* [PATCH v7 4/4] worktree: teach "add" to check out existing branches
2018-04-15 20:29 ` [PATCH v7 0/4] " Thomas Gummerer
` (2 preceding siblings ...)
2018-04-15 20:29 ` [PATCH v7 3/4] worktree: factor out dwim_branch function Thomas Gummerer
@ 2018-04-15 20:29 ` Thomas Gummerer
2018-04-23 4:52 ` [PATCH v7 0/4] " Eric Sunshine
2018-04-23 19:38 ` [PATCH v8 " Thomas Gummerer
5 siblings, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-15 20:29 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
Currently 'git worktree add <path>' creates a new branch named after the
basename of the path by default. If a branch with that name already
exists, the command refuses to do anything, unless the '--force' option
is given.
However we can do a little better than that, and check the branch out if
it is not checked out anywhere else. This will help users who just want
to check an existing branch out into a new worktree, and save a few
keystrokes.
As the current behaviour is to simply 'die()' when a branch with the name
of the basename of the path already exists, there are no backwards
compatibility worries here.
We will still 'die()' if the branch is checked out in another worktree,
unless the --force flag is passed.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-worktree.txt | 9 +++++++--
builtin/worktree.c | 30 ++++++++++++++++++++++++------
t/t2025-worktree-add.sh | 26 +++++++++++++++++++-------
3 files changed, 50 insertions(+), 15 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 41585f535d..5d54f36a71 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -61,8 +61,13 @@ $ git worktree add --track -b <branch> <path> <remote>/<branch>
------------
+
If `<commit-ish>` is omitted and neither `-b` nor `-B` nor `--detach` used,
-then, as a convenience, a new branch based at HEAD is created automatically,
-as if `-b $(basename <path>)` was specified.
+then, as a convenience, the new worktree is associated with a branch
+(call it `<branch>`) named after `$(basename <path>)`. If `<branch>`
+doesn't exist, a new branch based on HEAD is automatically created as
+if `-b <branch>` was given. If `<branch>` does exist, it will be
+checked out in the new worktree, if it's not checked out anywhere
+else, otherwise the command will refuse to create the worktree (unless
+`--force` is used).
list::
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 893139629a..f5a5283b39 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -355,9 +355,12 @@ static int add_worktree(const char *path, const char *refname,
static void print_preparing_worktree_line(const char *branch,
const char *new_branch,
- const char *new_branch_force)
+ const char *new_branch_force,
+ int checkout_existing_branch)
{
- if (new_branch_force) {
+ if (checkout_existing_branch) {
+ printf_ln(_("Preparing worktree (checking out '%s')"), branch);
+ } else if (new_branch_force) {
struct commit *commit = lookup_commit_reference_by_name(new_branch_force);
if (!commit)
printf_ln(_("Preparing worktree (new branch '%s')"), new_branch_force);
@@ -378,11 +381,23 @@ static void print_preparing_worktree_line(const char *branch,
}
}
-static const char *dwim_branch(const char *path, const char **new_branch)
+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);
- *new_branch = xstrndup(s, n);
+ const char *branchname = xstrndup(s, n);
+ struct strbuf ref = STRBUF_INIT;
+
+ if (!strbuf_check_branch_ref(&ref, branchname) &&
+ ref_exists(ref.buf)) {
+ *checkout_existing_branch = 1;
+ strbuf_release(&ref);
+ UNLEAK(branchname);
+ return branchname;
+ }
+
+ *new_branch = branchname;
if (guess_remote) {
struct object_id oid;
const char *remote =
@@ -397,6 +412,7 @@ static int add(int ac, const char **av, const char *prefix)
struct add_opts opts;
const char *new_branch_force = NULL;
char *path;
+ int checkout_existing_branch = 0;
const char *branch;
const char *new_branch = NULL;
const char *opt_track = NULL;
@@ -444,7 +460,8 @@ static int add(int ac, const char **av, const char *prefix)
}
if (ac < 2 && !new_branch && !opts.detach) {
- const char *s = dwim_branch(path, &new_branch);
+ const char *s = dwim_branch(path, &new_branch,
+ &checkout_existing_branch);
if (s)
branch = s;
}
@@ -464,7 +481,8 @@ static int add(int ac, const char **av, const char *prefix)
}
}
- print_preparing_worktree_line(branch, new_branch, new_branch_force);
+ print_preparing_worktree_line(branch, new_branch, new_branch_force,
+ checkout_existing_branch);
if (new_branch) {
struct child_process cp = CHILD_PROCESS_INIT;
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 2b95944973..ad38507d00 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -198,13 +198,25 @@ test_expect_success '"add" with <branch> omitted' '
test_cmp_rev HEAD bat
'
-test_expect_success '"add" auto-vivify does not clobber existing branch' '
- test_commit c1 &&
- test_commit c2 &&
- git branch precious HEAD~1 &&
- test_must_fail git worktree add precious &&
- test_cmp_rev HEAD~1 precious &&
- test_path_is_missing precious
+test_expect_success '"add" checks out existing branch of dwimd name' '
+ git branch dwim HEAD~1 &&
+ git worktree add dwim &&
+ test_cmp_rev HEAD~1 dwim &&
+ (
+ cd dwim &&
+ test_cmp_rev HEAD dwim
+ )
+'
+
+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
+'
+
+test_expect_success '"add --force" with existing dwimd name doesnt die' '
+ git checkout test-branch &&
+ git worktree add --force test-branch
'
test_expect_success '"add" no auto-vivify with --detach and <branch> omitted' '
--
2.17.0.252.gfe0a9eaf31
^ permalink raw reply related [flat|nested] 112+ messages in thread
* Re: [PATCH v7 0/4] worktree: teach "add" to check out existing branches
2018-04-15 20:29 ` [PATCH v7 0/4] " Thomas Gummerer
` (3 preceding siblings ...)
2018-04-15 20:29 ` [PATCH v7 4/4] worktree: teach "add" to check out existing branches Thomas Gummerer
@ 2018-04-23 4:52 ` Eric Sunshine
2018-04-23 19:38 ` [PATCH v8 " Thomas Gummerer
5 siblings, 0 replies; 112+ messages in thread
From: Eric Sunshine @ 2018-04-23 4:52 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On Sun, Apr 15, 2018 at 4:29 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> The main change once again in this series is the user interface. It
> feels like we went in a complete circle here now, as this iteration is
> bringing the "Preparing ..." line back (although in a slightly
> different form than the original), and is moving away from printing
> it's own "HEAD is now at..." line again. This also means we don't
> need the new hidden option to 'git reset' anymore, which is nice.
I'm glad to see the proposed hidden git-reset option go away, and am
likewise happy to see that worktree no longer wants to print "HEAD is
now at" itself. I'm much more pleased with the direction this series
is now taking than in earlier rounds. It's also much simpler, which is
a nice plus.
> I do like the new UI more than what we had in the last round (which I
> already liked more than the original UI) :)
>
> To demonstrate the UI updates, let's compare the new UI and the old UI
> again. This is the same commands as used for the demonstration in the
> last iteration, so please have a look at <20180331151804.30380-1-t.gummerer@gmail.com>
> for an example of what it looked like after the last round.
Thanks for presenting examples of the new UI under various conditions.
Like you, I find the new "Preparing..." message superior to and much
more useful than the original.
Aside from the problem pointed out in my review of 2/4 in which it
incorrectly shows "detached HEAD" for "git worktree add wt
existing-local-branch", I think this series is just about ready, and
hope to see it land with the next re-roll.
Thanks for working on it.
^ permalink raw reply [flat|nested] 112+ messages in thread
* [PATCH v8 0/4] worktree: teach "add" to check out existing branches
2018-04-15 20:29 ` [PATCH v7 0/4] " Thomas Gummerer
` (4 preceding siblings ...)
2018-04-23 4:52 ` [PATCH v7 0/4] " Eric Sunshine
@ 2018-04-23 19:38 ` Thomas Gummerer
2018-04-23 19:38 ` [PATCH v8 1/4] worktree: remove extra members from struct add_opts Thomas Gummerer
` (4 more replies)
5 siblings, 5 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-23 19:38 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
Thanks Eric and Junio for the review the suggestions in 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>,
<20180325134947.25828-1-t.gummerer@gmail.com>,
<20180331151804.30380-1-t.gummerer@gmail.com> and
<20180415202917.4360-1-t.gummerer@gmail.com>.
This round updates the output for "resetting branch ..." to not have
braces embedded inside of another pair of braces, and is not correctly
printing "checking out '<branch>'" when 'git worktree add <path>
<local-branch>' is used. Both these changes are in patch 2/4, the
other patches are the same as in the previous round.
Interdiff below:
diff --git a/builtin/worktree.c b/builtin/worktree.c
index f5a5283b39..d52495f312 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -353,7 +353,8 @@ static int add_worktree(const char *path, const char *refname,
return ret;
}
-static void print_preparing_worktree_line(const char *branch,
+static void print_preparing_worktree_line(int detach,
+ const char *branch,
const char *new_branch,
const char *new_branch_force,
int checkout_existing_branch)
@@ -365,19 +366,27 @@ static void print_preparing_worktree_line(const char *branch,
if (!commit)
printf_ln(_("Preparing worktree (new branch '%s')"), new_branch_force);
else
- printf_ln(_("Preparing worktree (resetting branch '%s' (was at %s))"),
+ printf_ln(_("Preparing worktree (resetting branch '%s'; was at %s)"),
new_branch_force,
find_unique_abbrev(commit->object.oid.hash,
DEFAULT_ABBREV));
} else if (new_branch) {
printf_ln(_("Preparing worktree (new branch '%s')"), new_branch);
} else {
- struct commit *commit = lookup_commit_reference_by_name(branch);
- if (!commit)
- die(_("invalid reference: %s"), branch);
- printf_ln(_("Preparing worktree (detached HEAD %s)"),
- find_unique_abbrev(commit->object.oid.hash,
- DEFAULT_ABBREV));
+ struct strbuf s = STRBUF_INIT;
+ if (!detach && !strbuf_check_branch_ref(&s, branch) &&
+ ref_exists(s.buf))
+ printf_ln(_("Preparing worktree (checking out '%s')"),
+ branch);
+ else {
+ struct commit *commit = lookup_commit_reference_by_name(branch);
+ if (!commit)
+ die(_("invalid reference: %s"), branch);
+ printf_ln(_("Preparing worktree (detached HEAD %s)"),
+ find_unique_abbrev(commit->object.oid.hash,
+ DEFAULT_ABBREV));
+ }
+ strbuf_release(&s);
}
}
@@ -481,7 +490,7 @@ static int add(int ac, const char **av, const char *prefix)
}
}
- print_preparing_worktree_line(branch, new_branch, new_branch_force,
+ print_preparing_worktree_line(opts.detach, branch, new_branch, new_branch_force,
checkout_existing_branch);
if (new_branch) {
Thomas Gummerer (4):
worktree: remove extra members from struct add_opts
worktree: improve message when creating a new worktree
worktree: factor out dwim_branch function
worktree: teach "add" to check out existing branches
Documentation/git-worktree.txt | 9 +++-
builtin/worktree.c | 111 +++++++++++++++++++++++++++++++----------
t/t2025-worktree-add.sh | 26 +++++++---
3 files changed, 110 insertions(+), 36 deletions(-)
--
2.16.1.74.g7afd1c25cc.dirty
^ permalink raw reply related [flat|nested] 112+ messages in thread
* [PATCH v8 1/4] worktree: remove extra members from struct add_opts
2018-04-23 19:38 ` [PATCH v8 " Thomas Gummerer
@ 2018-04-23 19:38 ` Thomas Gummerer
2018-04-24 3:26 ` Eric Sunshine
2018-04-23 19:38 ` [PATCH v8 2/4] worktree: improve message when creating a new worktree Thomas Gummerer
` (3 subsequent siblings)
4 siblings, 1 reply; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-23 19:38 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
There are two members of 'struct add_opts', which are only used inside
the 'add()' function, but being part of 'struct add_opts' they are
needlessly also passed to the 'add_worktree' function.
Make them local to the 'add()' function to make it clearer where they
are used.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/worktree.c | 32 +++++++++++++++-----------------
1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7cef5b120b..4d96a21a45 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 force_new_branch;
};
static int show_only;
@@ -363,10 +361,11 @@ 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;
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")),
@@ -384,7 +383,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);
@@ -395,33 +394,32 @@ static int add(int ac, const char **av, const char *prefix)
if (!strcmp(branch, "-"))
branch = "@{-1}";
- opts.force_new_branch = !!new_branch_force;
- if (opts.force_new_branch) {
+ 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) {
+ if (ac < 2 && !new_branch && !opts.detach) {
int n;
const char *s = worktree_basename(path, &n);
- opts.new_branch = xstrndup(s, n);
+ new_branch = xstrndup(s, n);
if (guess_remote) {
struct object_id oid;
const char *remote =
- unique_tracking_name(opts.new_branch, &oid);
+ unique_tracking_name(new_branch, &oid);
if (remote)
branch = remote;
}
}
- 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;
@@ -430,25 +428,25 @@ 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;
cp.git_cmd = 1;
argv_array_push(&cp.args, "branch");
- if (opts.force_new_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 (opt_track) {
die(_("--[no-]track can only be used if a new branch is created"));
}
--
2.16.1.74.g7afd1c25cc.dirty
^ permalink raw reply related [flat|nested] 112+ messages in thread
* Re: [PATCH v8 1/4] worktree: remove extra members from struct add_opts
2018-04-23 19:38 ` [PATCH v8 1/4] worktree: remove extra members from struct add_opts Thomas Gummerer
@ 2018-04-24 3:26 ` Eric Sunshine
0 siblings, 0 replies; 112+ messages in thread
From: Eric Sunshine @ 2018-04-24 3:26 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On Mon, Apr 23, 2018 at 3:38 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> There are two members of 'struct add_opts', which are only used inside
> the 'add()' function, but being part of 'struct add_opts' they are
> needlessly also passed to the 'add_worktree' function.
>
> Make them local to the 'add()' function to make it clearer where they
> are used.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -27,8 +27,6 @@ struct add_opts {
> int keep_locked;
> - const char *new_branch;
> - int force_new_branch;
> };
> @@ -395,33 +394,32 @@ static int add(int ac, const char **av, const char *prefix)
> - if (ac < 2 && !opts.new_branch && !opts.detach) {
> + if (ac < 2 && !new_branch && !opts.detach) {
> int n;
> const char *s = worktree_basename(path, &n);
> - opts.new_branch = xstrndup(s, n);
> + new_branch = xstrndup(s, n);
Sorry for not noticing this earlier, but when 'new_branch' was part of
'opts', this leaked xstrndup'd string was covered by the UNLEAK(opts)
at the end of the function. However, now that 'new_branch' is a
standalone variable, it needs its own UNLEAK().
^ permalink raw reply [flat|nested] 112+ messages in thread
* [PATCH v8 2/4] worktree: improve message when creating a new worktree
2018-04-23 19:38 ` [PATCH v8 " Thomas Gummerer
2018-04-23 19:38 ` [PATCH v8 1/4] worktree: remove extra members from struct add_opts Thomas Gummerer
@ 2018-04-23 19:38 ` Thomas Gummerer
2018-04-24 3:58 ` Eric Sunshine
2018-04-23 19:38 ` [PATCH v8 3/4] worktree: factor out dwim_branch function Thomas Gummerer
` (2 subsequent siblings)
4 siblings, 1 reply; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-23 19:38 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
Currently 'git worktree add' produces output like the following:
Preparing ../foo (identifier foo)
HEAD is now at 26da330922 <title>
The '../foo' is the path where the worktree is created, which the user
has just given on the command line. The identifier is an internal
implementation detail, which is not particularly relevant for the user
and indeed isn't mentioned explicitly anywhere in the man page.
Instead of this message, print a message that gives the user a bit more
detail of what exactly 'git worktree' is doing. There are various dwim
modes which perform some magic under the hood, which should be
helpful to users. Just from the output of the command it is not always
visible to users what exactly has happened.
Help the users a bit more by modifying the "Preparing ..." message and
adding some additional information of what 'git worktree add' did under
the hood, while not displaying the identifier anymore.
Currently this ends up in three different cases:
- 'git worktree add -b ...' or 'git worktree add <path>', both of
which create a new branch, either through the user explicitly
requesting it, or through 'git worktree add' implicitly creating
it. This will end up with the following output:
Preparing worktree (new branch '<branch>')
HEAD is now at 26da330922 <title>
- 'git worktree add -B ...', which may either create a new branch if
the branch with the given name does not exist yet, or resets an
existing branch to the current HEAD, or the commit-ish given.
Depending on which action is taken, we'll end up with the following
output:
Preparing worktree (resetting branch 'next'; was at caa68db14)
HEAD is now at 26da330922 <title>
or:
Preparing worktree (new branch '<branch>')
HEAD is now at 26da330922 <title>
- 'git worktree add --detach' or 'git worktree add <path>
<commit-ish>', both of which create a new worktree with a detached
HEAD, for which we will print the following output:
Preparing worktree (detached HEAD 26da330922)
HEAD is now at 26da330922 <title>
- 'git worktree add <path> <local-branch>', which checks out the
branch prints the following output:
Preparing worktree (checking out '<branch>')
HEAD is now at 47007d5 <title>
Additionally currently the "Preparing ..." line is printed to stderr,
while the "HEAD is now at ..." line is printed to stdout by 'git reset
--hard', which is used internally by 'git worktree add'. Fix this
inconsistency by printing the "Preparing ..." message to stdout as
well. As "Preparing ..." is not an error, stdout also seems like the
more appropriate output stream.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/worktree.c | 38 ++++++++++++++++++++++++++++++++++++--
1 file changed, 36 insertions(+), 2 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 4d96a21a45..d348101216 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -301,8 +301,6 @@ static int add_worktree(const char *path, const char *refname,
strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
write_file(sb.buf, "../..");
- fprintf_ln(stderr, _("Preparing %s (identifier %s)"), path, name);
-
argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
cp.git_cmd = 1;
@@ -355,6 +353,40 @@ static int add_worktree(const char *path, const char *refname,
return ret;
}
+static void print_preparing_worktree_line(int detach,
+ const char *branch,
+ const char *new_branch,
+ const char *new_branch_force)
+{
+ if (new_branch_force) {
+ struct commit *commit = lookup_commit_reference_by_name(new_branch_force);
+ if (!commit)
+ printf_ln(_("Preparing worktree (new branch '%s')"), new_branch_force);
+ else
+ printf_ln(_("Preparing worktree (resetting branch '%s'; was at %s)"),
+ new_branch_force,
+ find_unique_abbrev(commit->object.oid.hash,
+ DEFAULT_ABBREV));
+ } else if (new_branch) {
+ printf_ln(_("Preparing worktree (new branch '%s')"), new_branch);
+ } else {
+ struct strbuf s = STRBUF_INIT;
+ if (!detach && !strbuf_check_branch_ref(&s, branch) &&
+ ref_exists(s.buf))
+ printf_ln(_("Preparing worktree (checking out '%s')"),
+ branch);
+ else {
+ struct commit *commit = lookup_commit_reference_by_name(branch);
+ if (!commit)
+ die(_("invalid reference: %s"), branch);
+ printf_ln(_("Preparing worktree (detached HEAD %s)"),
+ find_unique_abbrev(commit->object.oid.hash,
+ DEFAULT_ABBREV));
+ }
+ strbuf_release(&s);
+ }
+}
+
static int add(int ac, const char **av, const char *prefix)
{
struct add_opts opts;
@@ -434,6 +466,8 @@ static int add(int ac, const char **av, const char *prefix)
}
}
+ print_preparing_worktree_line(opts.detach, branch, new_branch, new_branch_force);
+
if (new_branch) {
struct child_process cp = CHILD_PROCESS_INIT;
cp.git_cmd = 1;
--
2.16.1.74.g7afd1c25cc.dirty
^ permalink raw reply related [flat|nested] 112+ messages in thread
* Re: [PATCH v8 2/4] worktree: improve message when creating a new worktree
2018-04-23 19:38 ` [PATCH v8 2/4] worktree: improve message when creating a new worktree Thomas Gummerer
@ 2018-04-24 3:58 ` Eric Sunshine
0 siblings, 0 replies; 112+ messages in thread
From: Eric Sunshine @ 2018-04-24 3:58 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On Mon, Apr 23, 2018 at 3:38 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> Currently 'git worktree add' produces output like the following:
>
> Preparing ../foo (identifier foo)
> HEAD is now at 26da330922 <title>
>
> The '../foo' is the path where the worktree is created, which the user
> has just given on the command line. The identifier is an internal
> implementation detail, which is not particularly relevant for the user
> and indeed isn't mentioned explicitly anywhere in the man page.
>
> Instead of this message, print a message that gives the user a bit more
> detail of what exactly 'git worktree' is doing. There are various dwim
> modes which perform some magic under the hood, which should be
> helpful to users. Just from the output of the command it is not always
> visible to users what exactly has happened.
>
> Help the users a bit more by modifying the "Preparing ..." message and
> adding some additional information of what 'git worktree add' did under
> the hood, while not displaying the identifier anymore.
>
> Currently this ends up in three different cases:
You now enumerate four cases, not three. Perhaps: "There are several
different cases:"
> - 'git worktree add -b ...' or 'git worktree add <path>', both of
> which create a new branch, either through the user explicitly
> requesting it, or through 'git worktree add' implicitly creating
> it. This will end up with the following output:
>
> Preparing worktree (new branch '<branch>')
> HEAD is now at 26da330922 <title>
>
> - 'git worktree add -B ...', which may either create a new branch if
> the branch with the given name does not exist yet, or resets an
> existing branch to the current HEAD, or the commit-ish given.
> Depending on which action is taken, we'll end up with the following
> output:
>
> Preparing worktree (resetting branch 'next'; was at caa68db14)
> HEAD is now at 26da330922 <title>
For consistency with the other examples: s/next/<branch>/
> or:
>
> Preparing worktree (new branch '<branch>')
> HEAD is now at 26da330922 <title>
>
> - 'git worktree add --detach' or 'git worktree add <path>
> <commit-ish>', both of which create a new worktree with a detached
> HEAD, for which we will print the following output:
>
> Preparing worktree (detached HEAD 26da330922)
> HEAD is now at 26da330922 <title>
>
> - 'git worktree add <path> <local-branch>', which checks out the
> branch prints the following output:
s/prints/and prints/
> Preparing worktree (checking out '<branch>')
> HEAD is now at 47007d5 <title>
s/<branch>/<local-branch>/ would probably be clearer since you use
<local-branch> in the example command.
> [...]
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -355,6 +353,40 @@ static int add_worktree(const char *path, const char *refname,
> +static void print_preparing_worktree_line(int detach,
> + const char *branch,
> + const char *new_branch,
> + const char *new_branch_force)
> +{
> + if (new_branch_force) {
> + struct commit *commit = lookup_commit_reference_by_name(new_branch_force);
Nit: By the time this function is called, 'new_branch' contains the
same value as 'new_branch_force' if "-B" was used. 'new_branch' _is_
the name of the new branch, forced or not, and it's easier to reason
about 'new_branch_force' as a mere boolean indicating whether "force"
is in effect. This suggests that the final argument to this function
should be a boolean (int) named either 'force' or 'force_new_branch',
and that 'new_branch' should be used everywhere the code needs to
reference the new branch name (forced or not).
Not worth a re-roll.
> + if (!commit)
> + printf_ln(_("Preparing worktree (new branch '%s')"), new_branch_force);
> + else
> + printf_ln(_("Preparing worktree (resetting branch '%s'; was at %s)"),
> + new_branch_force,
> + find_unique_abbrev(commit->object.oid.hash,
> + DEFAULT_ABBREV));
> + } else if (new_branch) {
> + printf_ln(_("Preparing worktree (new branch '%s')"), new_branch);
> + } else {
> + struct strbuf s = STRBUF_INIT;
> + if (!detach && !strbuf_check_branch_ref(&s, branch) &&
> + ref_exists(s.buf))
> + printf_ln(_("Preparing worktree (checking out '%s')"),
> + branch);
> + else {
> + struct commit *commit = lookup_commit_reference_by_name(branch);
> + if (!commit)
> + die(_("invalid reference: %s"), branch);
> + printf_ln(_("Preparing worktree (detached HEAD %s)"),
> + find_unique_abbrev(commit->object.oid.hash,
> + DEFAULT_ABBREV));
> + }
It's too bad this final case duplicates so much code from
add_worktree(). Perhaps some future refactoring patch (not part of
this series) can consolidate the code.
> + strbuf_release(&s);
> + }
> +}
> @@ -434,6 +466,8 @@ static int add(int ac, const char **av, const char *prefix)
> + print_preparing_worktree_line(opts.detach, branch, new_branch, new_branch_force);
> +
> if (new_branch) {
> struct child_process cp = CHILD_PROCESS_INIT;
^ permalink raw reply [flat|nested] 112+ messages in thread
* [PATCH v8 3/4] worktree: factor out dwim_branch function
2018-04-23 19:38 ` [PATCH v8 " Thomas Gummerer
2018-04-23 19:38 ` [PATCH v8 1/4] worktree: remove extra members from struct add_opts Thomas Gummerer
2018-04-23 19:38 ` [PATCH v8 2/4] worktree: improve message when creating a new worktree Thomas Gummerer
@ 2018-04-23 19:38 ` Thomas Gummerer
2018-04-23 19:38 ` [PATCH v8 4/4] worktree: teach "add" to check out existing branches Thomas Gummerer
2018-04-24 21:56 ` [PATCH v9 0/4] " Thomas Gummerer
4 siblings, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-23 19:38 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
Factor out a dwim_branch function, which takes care of the dwim'ery in
'git worktree add <path>'. It's not too much code currently, but we're
adding a new kind of dwim in a subsequent patch, at which point it makes
more sense to have it as a separate function.
Factor it out now to reduce the patch noise in the next patch.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/worktree.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index d348101216..bd4cf4583f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -387,6 +387,20 @@ static void print_preparing_worktree_line(int detach,
}
}
+static const char *dwim_branch(const char *path, const char **new_branch)
+{
+ int n;
+ const char *s = worktree_basename(path, &n);
+ *new_branch = xstrndup(s, n);
+ if (guess_remote) {
+ struct object_id oid;
+ const char *remote =
+ unique_tracking_name(*new_branch, &oid);
+ return remote;
+ }
+ return NULL;
+}
+
static int add(int ac, const char **av, const char *prefix)
{
struct add_opts opts;
@@ -439,16 +453,9 @@ static int add(int ac, const char **av, const char *prefix)
}
if (ac < 2 && !new_branch && !opts.detach) {
- int n;
- const char *s = worktree_basename(path, &n);
- new_branch = xstrndup(s, n);
- if (guess_remote) {
- struct object_id oid;
- const char *remote =
- unique_tracking_name(new_branch, &oid);
- if (remote)
- branch = remote;
- }
+ const char *s = dwim_branch(path, &new_branch);
+ if (s)
+ branch = s;
}
if (ac == 2 && !new_branch && !opts.detach) {
--
2.16.1.74.g7afd1c25cc.dirty
^ permalink raw reply related [flat|nested] 112+ messages in thread
* [PATCH v8 4/4] worktree: teach "add" to check out existing branches
2018-04-23 19:38 ` [PATCH v8 " Thomas Gummerer
` (2 preceding siblings ...)
2018-04-23 19:38 ` [PATCH v8 3/4] worktree: factor out dwim_branch function Thomas Gummerer
@ 2018-04-23 19:38 ` Thomas Gummerer
2018-04-24 4:25 ` Eric Sunshine
2018-04-24 21:56 ` [PATCH v9 0/4] " Thomas Gummerer
4 siblings, 1 reply; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-23 19:38 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
Currently 'git worktree add <path>' creates a new branch named after the
basename of the path by default. If a branch with that name already
exists, the command refuses to do anything, unless the '--force' option
is given.
However we can do a little better than that, and check the branch out if
it is not checked out anywhere else. This will help users who just want
to check an existing branch out into a new worktree, and save a few
keystrokes.
As the current behaviour is to simply 'die()' when a branch with the name
of the basename of the path already exists, there are no backwards
compatibility worries here.
We will still 'die()' if the branch is checked out in another worktree,
unless the --force flag is passed.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-worktree.txt | 9 +++++++--
builtin/worktree.c | 30 ++++++++++++++++++++++++------
t/t2025-worktree-add.sh | 26 +++++++++++++++++++-------
3 files changed, 50 insertions(+), 15 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 41585f535d..5d54f36a71 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -61,8 +61,13 @@ $ git worktree add --track -b <branch> <path> <remote>/<branch>
------------
+
If `<commit-ish>` is omitted and neither `-b` nor `-B` nor `--detach` used,
-then, as a convenience, a new branch based at HEAD is created automatically,
-as if `-b $(basename <path>)` was specified.
+then, as a convenience, the new worktree is associated with a branch
+(call it `<branch>`) named after `$(basename <path>)`. If `<branch>`
+doesn't exist, a new branch based on HEAD is automatically created as
+if `-b <branch>` was given. If `<branch>` does exist, it will be
+checked out in the new worktree, if it's not checked out anywhere
+else, otherwise the command will refuse to create the worktree (unless
+`--force` is used).
list::
diff --git a/builtin/worktree.c b/builtin/worktree.c
index bd4cf4583f..d52495f312 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -356,9 +356,12 @@ static int add_worktree(const char *path, const char *refname,
static void print_preparing_worktree_line(int detach,
const char *branch,
const char *new_branch,
- const char *new_branch_force)
+ const char *new_branch_force,
+ int checkout_existing_branch)
{
- if (new_branch_force) {
+ if (checkout_existing_branch) {
+ printf_ln(_("Preparing worktree (checking out '%s')"), branch);
+ } else if (new_branch_force) {
struct commit *commit = lookup_commit_reference_by_name(new_branch_force);
if (!commit)
printf_ln(_("Preparing worktree (new branch '%s')"), new_branch_force);
@@ -387,11 +390,23 @@ static void print_preparing_worktree_line(int detach,
}
}
-static const char *dwim_branch(const char *path, const char **new_branch)
+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);
- *new_branch = xstrndup(s, n);
+ const char *branchname = xstrndup(s, n);
+ struct strbuf ref = STRBUF_INIT;
+
+ if (!strbuf_check_branch_ref(&ref, branchname) &&
+ ref_exists(ref.buf)) {
+ *checkout_existing_branch = 1;
+ strbuf_release(&ref);
+ UNLEAK(branchname);
+ return branchname;
+ }
+
+ *new_branch = branchname;
if (guess_remote) {
struct object_id oid;
const char *remote =
@@ -406,6 +421,7 @@ static int add(int ac, const char **av, const char *prefix)
struct add_opts opts;
const char *new_branch_force = NULL;
char *path;
+ int checkout_existing_branch = 0;
const char *branch;
const char *new_branch = NULL;
const char *opt_track = NULL;
@@ -453,7 +469,8 @@ static int add(int ac, const char **av, const char *prefix)
}
if (ac < 2 && !new_branch && !opts.detach) {
- const char *s = dwim_branch(path, &new_branch);
+ const char *s = dwim_branch(path, &new_branch,
+ &checkout_existing_branch);
if (s)
branch = s;
}
@@ -473,7 +490,8 @@ static int add(int ac, const char **av, const char *prefix)
}
}
- print_preparing_worktree_line(opts.detach, branch, new_branch, new_branch_force);
+ print_preparing_worktree_line(opts.detach, branch, new_branch, new_branch_force,
+ checkout_existing_branch);
if (new_branch) {
struct child_process cp = CHILD_PROCESS_INIT;
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 2b95944973..ad38507d00 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -198,13 +198,25 @@ test_expect_success '"add" with <branch> omitted' '
test_cmp_rev HEAD bat
'
-test_expect_success '"add" auto-vivify does not clobber existing branch' '
- test_commit c1 &&
- test_commit c2 &&
- git branch precious HEAD~1 &&
- test_must_fail git worktree add precious &&
- test_cmp_rev HEAD~1 precious &&
- test_path_is_missing precious
+test_expect_success '"add" checks out existing branch of dwimd name' '
+ git branch dwim HEAD~1 &&
+ git worktree add dwim &&
+ test_cmp_rev HEAD~1 dwim &&
+ (
+ cd dwim &&
+ test_cmp_rev HEAD dwim
+ )
+'
+
+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
+'
+
+test_expect_success '"add --force" with existing dwimd name doesnt die' '
+ git checkout test-branch &&
+ git worktree add --force test-branch
'
test_expect_success '"add" no auto-vivify with --detach and <branch> omitted' '
--
2.16.1.74.g7afd1c25cc.dirty
^ permalink raw reply related [flat|nested] 112+ messages in thread
* Re: [PATCH v8 4/4] worktree: teach "add" to check out existing branches
2018-04-23 19:38 ` [PATCH v8 4/4] worktree: teach "add" to check out existing branches Thomas Gummerer
@ 2018-04-24 4:25 ` Eric Sunshine
0 siblings, 0 replies; 112+ messages in thread
From: Eric Sunshine @ 2018-04-24 4:25 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Nguyễn Thái Ngọc Duy, Junio C Hamano
On Mon, Apr 23, 2018 at 3:38 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> Currently 'git worktree add <path>' creates a new branch named after the
> basename of the path by default. If a branch with that name already
> exists, the command refuses to do anything, unless the '--force' option
> is given.
>
> However we can do a little better than that, and check the branch out if
> it is not checked out anywhere else. This will help users who just want
> to check an existing branch out into a new worktree, and save a few
> keystrokes.
> [...]
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -356,9 +356,12 @@ static int add_worktree(const char *path, const char *refname,
> static void print_preparing_worktree_line(int detach,
> const char *branch,
> const char *new_branch,
> - const char *new_branch_force)
> + const char *new_branch_force,
> + int checkout_existing_branch)
> {
> - if (new_branch_force) {
> + if (checkout_existing_branch) {
> + printf_ln(_("Preparing worktree (checking out '%s')"), branch);
Is the new 'checkout_existing_branch' argument and this new
conditional really needed? Both 'new_branch' and 'new_branch_force'
will be NULL in the case of an existing branch, so there should be no
need for 'checkout_existing_branch' to state explicitly what is
already indicated by their NULLness. With both of them NULL, the
existing "(checking out '...')" message later in the function will
kick in, so this new condition isn't needed.
> + } else if (new_branch_force) {
> @@ -387,11 +390,23 @@ static void print_preparing_worktree_line(int detach,
> +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);
> - *new_branch = xstrndup(s, n);
> + const char *branchname = xstrndup(s, n);
> + struct strbuf ref = STRBUF_INIT;
> +
> + if (!strbuf_check_branch_ref(&ref, branchname) &&
> + ref_exists(ref.buf)) {
> + *checkout_existing_branch = 1;
See above regarding apparent lack of need for this new variable.
> + strbuf_release(&ref);
> + UNLEAK(branchname);
> + return branchname;
> + }
Taking the above observation into account, I applied, atop this patch,
the following fixup! which removes the 'checkout_existing_branch'
gunk and still get the same output:
--- >8 ---
diff --git a/builtin/worktree.c b/builtin/worktree.c
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -356,12 +356,9 @@ static int add_worktree(const char *path, const char *refname,
static void print_preparing_worktree_line(int detach,
const char *branch,
const char *new_branch,
- const char *new_branch_force,
- int checkout_existing_branch)
+ const char *new_branch_force)
{
- if (checkout_existing_branch) {
- printf_ln(_("Preparing worktree (checking out '%s')"), branch);
- } else if (new_branch_force) {
+ if (new_branch_force) {
struct commit *commit = lookup_commit_reference_by_name(new_branch_force);
if (!commit)
printf_ln(_("Preparing worktree (new branch '%s')"), new_branch_force);
@@ -390,8 +387,7 @@ static void print_preparing_worktree_line(int detach,
}
}
-static const char *dwim_branch(const char *path, const char **new_branch,
- int *checkout_existing_branch)
+static const char *dwim_branch(const char *path, const char **new_branch)
{
int n;
const char *s = worktree_basename(path, &n);
@@ -400,7 +396,6 @@ static const char *dwim_branch(const char *path, const char **new_branch,
if (!strbuf_check_branch_ref(&ref, branchname) &&
ref_exists(ref.buf)) {
- *checkout_existing_branch = 1;
strbuf_release(&ref);
UNLEAK(branchname);
return branchname;
@@ -421,7 +416,6 @@ static int add(int ac, const char **av, const char *prefix)
struct add_opts opts;
const char *new_branch_force = NULL;
char *path;
- int checkout_existing_branch = 0;
const char *branch;
const char *new_branch = NULL;
const char *opt_track = NULL;
@@ -469,8 +463,7 @@ static int add(int ac, const char **av, const char *prefix)
}
if (ac < 2 && !new_branch && !opts.detach) {
- const char *s = dwim_branch(path, &new_branch,
- &checkout_existing_branch);
+ const char *s = dwim_branch(path, &new_branch);
if (s)
branch = s;
}
@@ -490,8 +483,7 @@ static int add(int ac, const char **av, const char *prefix)
}
}
- print_preparing_worktree_line(opts.detach, branch, new_branch, new_branch_force,
- checkout_existing_branch);
+ print_preparing_worktree_line(opts.detach, branch, new_branch, new_branch_force);
if (new_branch) {
struct child_process cp = CHILD_PROCESS_INIT;
--- >8 ---
^ permalink raw reply [flat|nested] 112+ messages in thread
* [PATCH v9 0/4] worktree: teach "add" to check out existing branches
2018-04-23 19:38 ` [PATCH v8 " Thomas Gummerer
` (3 preceding siblings ...)
2018-04-23 19:38 ` [PATCH v8 4/4] worktree: teach "add" to check out existing branches Thomas Gummerer
@ 2018-04-24 21:56 ` Thomas Gummerer
2018-04-24 21:56 ` [PATCH v9 1/4] worktree: remove extra members from struct add_opts Thomas Gummerer
` (4 more replies)
4 siblings, 5 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-24 21:56 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
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>,
<20180325134947.25828-1-t.gummerer@gmail.com>,
<20180331151804.30380-1-t.gummerer@gmail.com>,
<20180415202917.4360-1-t.gummerer@gmail.com> and
<20180423193848.5159-1-t.gummerer@gmail.com>.
Thanks Eric for the review and the suggestions on the previous round.
Changes since the previous round:
- UNLEAK new_branch after it was xstrndup'd
- update the commit message of 2/4 according to Eric's suggestions
- make force_new_branch a boolean flag in
print_preparing_worktree_line, instead of using it as the branch
name. Instead use new_branch as the new branch name everywhere in
that function.
- get rid of the ckeckout_existing_branch flag
Interdiff below:
diff --git a/builtin/worktree.c b/builtin/worktree.c
index d52495f312..d3aeb4877d 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -356,18 +356,15 @@ static int add_worktree(const char *path, const char *refname,
static void print_preparing_worktree_line(int detach,
const char *branch,
const char *new_branch,
- const char *new_branch_force,
- int checkout_existing_branch)
+ int force_new_branch)
{
- if (checkout_existing_branch) {
- printf_ln(_("Preparing worktree (checking out '%s')"), branch);
- } else if (new_branch_force) {
- struct commit *commit = lookup_commit_reference_by_name(new_branch_force);
+ if (force_new_branch) {
+ struct commit *commit = lookup_commit_reference_by_name(new_branch);
if (!commit)
- printf_ln(_("Preparing worktree (new branch '%s')"), new_branch_force);
+ printf_ln(_("Preparing worktree (new branch '%s')"), new_branch);
else
printf_ln(_("Preparing worktree (resetting branch '%s'; was at %s)"),
- new_branch_force,
+ new_branch,
find_unique_abbrev(commit->object.oid.hash,
DEFAULT_ABBREV));
} else if (new_branch) {
@@ -390,19 +387,17 @@ static void print_preparing_worktree_line(int detach,
}
}
-static const char *dwim_branch(const char *path, const char **new_branch,
- int *checkout_existing_branch)
+static const char *dwim_branch(const char *path, const char **new_branch)
{
int n;
const char *s = worktree_basename(path, &n);
const char *branchname = xstrndup(s, n);
struct strbuf ref = STRBUF_INIT;
+ UNLEAK(branchname);
if (!strbuf_check_branch_ref(&ref, branchname) &&
ref_exists(ref.buf)) {
- *checkout_existing_branch = 1;
strbuf_release(&ref);
- UNLEAK(branchname);
return branchname;
}
@@ -421,7 +416,6 @@ static int add(int ac, const char **av, const char *prefix)
struct add_opts opts;
const char *new_branch_force = NULL;
char *path;
- int checkout_existing_branch = 0;
const char *branch;
const char *new_branch = NULL;
const char *opt_track = NULL;
@@ -469,8 +463,7 @@ static int add(int ac, const char **av, const char *prefix)
}
if (ac < 2 && !new_branch && !opts.detach) {
- const char *s = dwim_branch(path, &new_branch,
- &checkout_existing_branch);
+ const char *s = dwim_branch(path, &new_branch);
if (s)
branch = s;
}
@@ -490,8 +483,7 @@ static int add(int ac, const char **av, const char *prefix)
}
}
- print_preparing_worktree_line(opts.detach, branch, new_branch, new_branch_force,
- checkout_existing_branch);
+ print_preparing_worktree_line(opts.detach, branch, new_branch, !!new_branch_force);
if (new_branch) {
struct child_process cp = CHILD_PROCESS_INIT;
Thomas Gummerer (4):
worktree: remove extra members from struct add_opts
worktree: improve message when creating a new worktree
worktree: factor out dwim_branch function
worktree: teach "add" to check out existing branches
Documentation/git-worktree.txt | 9 +++-
builtin/worktree.c | 103 ++++++++++++++++++++++++++++++-----------
t/t2025-worktree-add.sh | 26 ++++++++---
3 files changed, 102 insertions(+), 36 deletions(-)
--
2.16.1.74.g7afd1c25cc.dirty
^ permalink raw reply related [flat|nested] 112+ messages in thread
* [PATCH v9 1/4] worktree: remove extra members from struct add_opts
2018-04-24 21:56 ` [PATCH v9 0/4] " Thomas Gummerer
@ 2018-04-24 21:56 ` Thomas Gummerer
2018-04-24 21:56 ` [PATCH v9 2/4] worktree: improve message when creating a new worktree Thomas Gummerer
` (3 subsequent siblings)
4 siblings, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-24 21:56 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
There are two members of 'struct add_opts', which are only used inside
the 'add()' function, but being part of 'struct add_opts' they are
needlessly also passed to the 'add_worktree' function.
Make them local to the 'add()' function to make it clearer where they
are used.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/worktree.c | 33 ++++++++++++++++-----------------
1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7cef5b120b..bf305c8b7b 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 force_new_branch;
};
static int show_only;
@@ -363,10 +361,11 @@ 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;
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")),
@@ -384,7 +383,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);
@@ -395,33 +394,33 @@ static int add(int ac, const char **av, const char *prefix)
if (!strcmp(branch, "-"))
branch = "@{-1}";
- opts.force_new_branch = !!new_branch_force;
- if (opts.force_new_branch) {
+ 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) {
+ if (ac < 2 && !new_branch && !opts.detach) {
int n;
const char *s = worktree_basename(path, &n);
- opts.new_branch = xstrndup(s, n);
+ new_branch = xstrndup(s, n);
+ UNLEAK(new_branch);
if (guess_remote) {
struct object_id oid;
const char *remote =
- unique_tracking_name(opts.new_branch, &oid);
+ unique_tracking_name(new_branch, &oid);
if (remote)
branch = remote;
}
}
- 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;
@@ -430,25 +429,25 @@ 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;
cp.git_cmd = 1;
argv_array_push(&cp.args, "branch");
- if (opts.force_new_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 (opt_track) {
die(_("--[no-]track can only be used if a new branch is created"));
}
--
2.16.1.74.g7afd1c25cc.dirty
^ permalink raw reply related [flat|nested] 112+ messages in thread
* [PATCH v9 2/4] worktree: improve message when creating a new worktree
2018-04-24 21:56 ` [PATCH v9 0/4] " Thomas Gummerer
2018-04-24 21:56 ` [PATCH v9 1/4] worktree: remove extra members from struct add_opts Thomas Gummerer
@ 2018-04-24 21:56 ` Thomas Gummerer
2018-04-24 21:56 ` [PATCH v9 3/4] worktree: factor out dwim_branch function Thomas Gummerer
` (2 subsequent siblings)
4 siblings, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-24 21:56 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
Currently 'git worktree add' produces output like the following:
Preparing ../foo (identifier foo)
HEAD is now at 26da330922 <title>
The '../foo' is the path where the worktree is created, which the user
has just given on the command line. The identifier is an internal
implementation detail, which is not particularly relevant for the user
and indeed isn't mentioned explicitly anywhere in the man page.
Instead of this message, print a message that gives the user a bit more
detail of what exactly 'git worktree' is doing. There are various dwim
modes which perform some magic under the hood, which should be
helpful to users. Just from the output of the command it is not always
visible to users what exactly has happened.
Help the users a bit more by modifying the "Preparing ..." message and
adding some additional information of what 'git worktree add' did under
the hood, while not displaying the identifier anymore.
Currently there are several different cases:
- 'git worktree add -b ...' or 'git worktree add <path>', both of
which create a new branch, either through the user explicitly
requesting it, or through 'git worktree add' implicitly creating
it. This will end up with the following output:
Preparing worktree (new branch '<branch>')
HEAD is now at 26da330922 <title>
- 'git worktree add -B ...', which may either create a new branch if
the branch with the given name does not exist yet, or resets an
existing branch to the current HEAD, or the commit-ish given.
Depending on which action is taken, we'll end up with the following
output:
Preparing worktree (resetting branch '<branch>'; was at caa68db14)
HEAD is now at 26da330922 <title>
or:
Preparing worktree (new branch '<branch>')
HEAD is now at 26da330922 <title>
- 'git worktree add --detach' or 'git worktree add <path>
<commit-ish>', both of which create a new worktree with a detached
HEAD, for which we will print the following output:
Preparing worktree (detached HEAD 26da330922)
HEAD is now at 26da330922 <title>
- 'git worktree add <path> <local-branch>', which checks out the
branch and prints the following output:
Preparing worktree (checking out '<local-branch>')
HEAD is now at 47007d5 <title>
Additionally currently the "Preparing ..." line is printed to stderr,
while the "HEAD is now at ..." line is printed to stdout by 'git reset
--hard', which is used internally by 'git worktree add'. Fix this
inconsistency by printing the "Preparing ..." message to stdout as
well. As "Preparing ..." is not an error, stdout also seems like the
more appropriate output stream.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/worktree.c | 38 ++++++++++++++++++++++++++++++++++++--
1 file changed, 36 insertions(+), 2 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index bf305c8b7b..39bf1ea865 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -301,8 +301,6 @@ static int add_worktree(const char *path, const char *refname,
strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
write_file(sb.buf, "../..");
- fprintf_ln(stderr, _("Preparing %s (identifier %s)"), path, name);
-
argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
cp.git_cmd = 1;
@@ -355,6 +353,40 @@ static int add_worktree(const char *path, const char *refname,
return ret;
}
+static void print_preparing_worktree_line(int detach,
+ const char *branch,
+ const char *new_branch,
+ int force_new_branch)
+{
+ if (force_new_branch) {
+ struct commit *commit = lookup_commit_reference_by_name(new_branch);
+ if (!commit)
+ printf_ln(_("Preparing worktree (new branch '%s')"), new_branch);
+ else
+ printf_ln(_("Preparing worktree (resetting branch '%s'; was at %s)"),
+ new_branch,
+ find_unique_abbrev(commit->object.oid.hash,
+ DEFAULT_ABBREV));
+ } else if (new_branch) {
+ printf_ln(_("Preparing worktree (new branch '%s')"), new_branch);
+ } else {
+ struct strbuf s = STRBUF_INIT;
+ if (!detach && !strbuf_check_branch_ref(&s, branch) &&
+ ref_exists(s.buf))
+ printf_ln(_("Preparing worktree (checking out '%s')"),
+ branch);
+ else {
+ struct commit *commit = lookup_commit_reference_by_name(branch);
+ if (!commit)
+ die(_("invalid reference: %s"), branch);
+ printf_ln(_("Preparing worktree (detached HEAD %s)"),
+ find_unique_abbrev(commit->object.oid.hash,
+ DEFAULT_ABBREV));
+ }
+ strbuf_release(&s);
+ }
+}
+
static int add(int ac, const char **av, const char *prefix)
{
struct add_opts opts;
@@ -435,6 +467,8 @@ static int add(int ac, const char **av, const char *prefix)
}
}
+ print_preparing_worktree_line(opts.detach, branch, new_branch, !!new_branch_force);
+
if (new_branch) {
struct child_process cp = CHILD_PROCESS_INIT;
cp.git_cmd = 1;
--
2.16.1.74.g7afd1c25cc.dirty
^ permalink raw reply related [flat|nested] 112+ messages in thread
* [PATCH v9 3/4] worktree: factor out dwim_branch function
2018-04-24 21:56 ` [PATCH v9 0/4] " Thomas Gummerer
2018-04-24 21:56 ` [PATCH v9 1/4] worktree: remove extra members from struct add_opts Thomas Gummerer
2018-04-24 21:56 ` [PATCH v9 2/4] worktree: improve message when creating a new worktree Thomas Gummerer
@ 2018-04-24 21:56 ` Thomas Gummerer
2018-04-24 21:56 ` [PATCH v9 4/4] worktree: teach "add" to check out existing branches Thomas Gummerer
2018-04-27 7:36 ` [PATCH v9 0/4] " Eric Sunshine
4 siblings, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-24 21:56 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
Factor out a dwim_branch function, which takes care of the dwim'ery in
'git worktree add <path>'. It's not too much code currently, but we're
adding a new kind of dwim in a subsequent patch, at which point it makes
more sense to have it as a separate function.
Factor it out now to reduce the patch noise in the next patch.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
builtin/worktree.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 39bf1ea865..6bd32b6090 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -387,6 +387,21 @@ static void print_preparing_worktree_line(int detach,
}
}
+static const char *dwim_branch(const char *path, const char **new_branch)
+{
+ int n;
+ const char *s = worktree_basename(path, &n);
+ *new_branch = xstrndup(s, n);
+ UNLEAK(*new_branch);
+ if (guess_remote) {
+ struct object_id oid;
+ const char *remote =
+ unique_tracking_name(*new_branch, &oid);
+ return remote;
+ }
+ return NULL;
+}
+
static int add(int ac, const char **av, const char *prefix)
{
struct add_opts opts;
@@ -439,17 +454,9 @@ static int add(int ac, const char **av, const char *prefix)
}
if (ac < 2 && !new_branch && !opts.detach) {
- int n;
- const char *s = worktree_basename(path, &n);
- new_branch = xstrndup(s, n);
- UNLEAK(new_branch);
- if (guess_remote) {
- struct object_id oid;
- const char *remote =
- unique_tracking_name(new_branch, &oid);
- if (remote)
- branch = remote;
- }
+ const char *s = dwim_branch(path, &new_branch);
+ if (s)
+ branch = s;
}
if (ac == 2 && !new_branch && !opts.detach) {
--
2.16.1.74.g7afd1c25cc.dirty
^ permalink raw reply related [flat|nested] 112+ messages in thread
* [PATCH v9 4/4] worktree: teach "add" to check out existing branches
2018-04-24 21:56 ` [PATCH v9 0/4] " Thomas Gummerer
` (2 preceding siblings ...)
2018-04-24 21:56 ` [PATCH v9 3/4] worktree: factor out dwim_branch function Thomas Gummerer
@ 2018-04-24 21:56 ` Thomas Gummerer
2018-04-27 7:36 ` [PATCH v9 0/4] " Eric Sunshine
4 siblings, 0 replies; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-24 21:56 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Nguyễn Thái Ngọc Duy,
Junio C Hamano, Thomas Gummerer
Currently 'git worktree add <path>' creates a new branch named after the
basename of the path by default. If a branch with that name already
exists, the command refuses to do anything, unless the '--force' option
is given.
However we can do a little better than that, and check the branch out if
it is not checked out anywhere else. This will help users who just want
to check an existing branch out into a new worktree, and save a few
keystrokes.
As the current behaviour is to simply 'die()' when a branch with the name
of the basename of the path already exists, there are no backwards
compatibility worries here.
We will still 'die()' if the branch is checked out in another worktree,
unless the --force flag is passed.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-worktree.txt | 9 +++++++--
builtin/worktree.c | 13 +++++++++++--
t/t2025-worktree-add.sh | 26 +++++++++++++++++++-------
3 files changed, 37 insertions(+), 11 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 41585f535d..5d54f36a71 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -61,8 +61,13 @@ $ git worktree add --track -b <branch> <path> <remote>/<branch>
------------
+
If `<commit-ish>` is omitted and neither `-b` nor `-B` nor `--detach` used,
-then, as a convenience, a new branch based at HEAD is created automatically,
-as if `-b $(basename <path>)` was specified.
+then, as a convenience, the new worktree is associated with a branch
+(call it `<branch>`) named after `$(basename <path>)`. If `<branch>`
+doesn't exist, a new branch based on HEAD is automatically created as
+if `-b <branch>` was given. If `<branch>` does exist, it will be
+checked out in the new worktree, if it's not checked out anywhere
+else, otherwise the command will refuse to create the worktree (unless
+`--force` is used).
list::
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 6bd32b6090..d3aeb4877d 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -391,8 +391,17 @@ static const char *dwim_branch(const char *path, const char **new_branch)
{
int n;
const char *s = worktree_basename(path, &n);
- *new_branch = xstrndup(s, n);
- UNLEAK(*new_branch);
+ const char *branchname = xstrndup(s, n);
+ struct strbuf ref = STRBUF_INIT;
+
+ UNLEAK(branchname);
+ if (!strbuf_check_branch_ref(&ref, branchname) &&
+ ref_exists(ref.buf)) {
+ strbuf_release(&ref);
+ return branchname;
+ }
+
+ *new_branch = branchname;
if (guess_remote) {
struct object_id oid;
const char *remote =
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 2b95944973..ad38507d00 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -198,13 +198,25 @@ test_expect_success '"add" with <branch> omitted' '
test_cmp_rev HEAD bat
'
-test_expect_success '"add" auto-vivify does not clobber existing branch' '
- test_commit c1 &&
- test_commit c2 &&
- git branch precious HEAD~1 &&
- test_must_fail git worktree add precious &&
- test_cmp_rev HEAD~1 precious &&
- test_path_is_missing precious
+test_expect_success '"add" checks out existing branch of dwimd name' '
+ git branch dwim HEAD~1 &&
+ git worktree add dwim &&
+ test_cmp_rev HEAD~1 dwim &&
+ (
+ cd dwim &&
+ test_cmp_rev HEAD dwim
+ )
+'
+
+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
+'
+
+test_expect_success '"add --force" with existing dwimd name doesnt die' '
+ git checkout test-branch &&
+ git worktree add --force test-branch
'
test_expect_success '"add" no auto-vivify with --detach and <branch> omitted' '
--
2.16.1.74.g7afd1c25cc.dirty
^ permalink raw reply related [flat|nested] 112+ messages in thread
* Re: [PATCH v9 0/4] worktree: teach "add" to check out existing branches
2018-04-24 21:56 ` [PATCH v9 0/4] " Thomas Gummerer
` (3 preceding siblings ...)
2018-04-24 21:56 ` [PATCH v9 4/4] worktree: teach "add" to check out existing branches Thomas Gummerer
@ 2018-04-27 7:36 ` Eric Sunshine
2018-04-28 16:09 ` Thomas Gummerer
4 siblings, 1 reply; 112+ messages in thread
From: Eric Sunshine @ 2018-04-27 7:36 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On Tue, Apr 24, 2018 at 5:56 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> Thanks Eric for the review and the suggestions on the previous round.
>
> Changes since the previous round:
>
> - UNLEAK new_branch after it was xstrndup'd
> - update the commit message of 2/4 according to Eric's suggestions
> - make force_new_branch a boolean flag in
> print_preparing_worktree_line, instead of using it as the branch
> name. Instead use new_branch as the new branch name everywhere in
> that function.
> - get rid of the ckeckout_existing_branch flag
Thanks. I did another full review of all the patches and played around
with the new functionality again for good measure. Everything looked
good, and I think the patches are now ready to graduate.
For what it's worth, the entire series is:
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v9 0/4] worktree: teach "add" to check out existing branches
2018-04-27 7:36 ` [PATCH v9 0/4] " Eric Sunshine
@ 2018-04-28 16:09 ` Thomas Gummerer
2018-04-30 0:07 ` Junio C Hamano
0 siblings, 1 reply; 112+ messages in thread
From: Thomas Gummerer @ 2018-04-28 16:09 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano
On 04/27, Eric Sunshine wrote:
> On Tue, Apr 24, 2018 at 5:56 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > Thanks Eric for the review and the suggestions on the previous round.
> >
> > Changes since the previous round:
> >
> > - UNLEAK new_branch after it was xstrndup'd
> > - update the commit message of 2/4 according to Eric's suggestions
> > - make force_new_branch a boolean flag in
> > print_preparing_worktree_line, instead of using it as the branch
> > name. Instead use new_branch as the new branch name everywhere in
> > that function.
> > - get rid of the ckeckout_existing_branch flag
>
> Thanks. I did another full review of all the patches and played around
> with the new functionality again for good measure. Everything looked
> good, and I think the patches are now ready to graduate.
Thanks for all your review work on this series!
> For what it's worth, the entire series is:
>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
^ permalink raw reply [flat|nested] 112+ messages in thread
* Re: [PATCH v9 0/4] worktree: teach "add" to check out existing branches
2018-04-28 16:09 ` Thomas Gummerer
@ 2018-04-30 0:07 ` Junio C Hamano
0 siblings, 0 replies; 112+ messages in thread
From: Junio C Hamano @ 2018-04-30 0:07 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Eric Sunshine, Git List, Nguyễn Thái Ngọc Duy
Thomas Gummerer <t.gummerer@gmail.com> writes:
> On 04/27, Eric Sunshine wrote:
>> On Tue, Apr 24, 2018 at 5:56 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
>> > Thanks Eric for the review and the suggestions on the previous round.
>> >
>> > Changes since the previous round:
>> >
>> > - UNLEAK new_branch after it was xstrndup'd
>> > - update the commit message of 2/4 according to Eric's suggestions
>> > - make force_new_branch a boolean flag in
>> > print_preparing_worktree_line, instead of using it as the branch
>> > name. Instead use new_branch as the new branch name everywhere in
>> > that function.
>> > - get rid of the ckeckout_existing_branch flag
>>
>> Thanks. I did another full review of all the patches and played around
>> with the new functionality again for good measure. Everything looked
>> good, and I think the patches are now ready to graduate.
>
> Thanks for all your review work on this series!
Thanks, both.
^ permalink raw reply [flat|nested] 112+ messages in thread