* [PATCH v1 1/2] checkout: factor out functions to new lib file @ 2017-11-12 13:43 Thomas Gummerer 2017-11-12 13:43 ` [PATCH v1 2/2] worktree: make add dwim Thomas Gummerer ` (2 more replies) 0 siblings, 3 replies; 28+ messages in thread From: Thomas Gummerer @ 2017-11-12 13:43 UTC (permalink / raw) To: git; +Cc: Nguyễn Thái Ngọc Duy, Thomas Gummerer Factor the functions out, so they can be re-used from other places. In particular these functions will be re-used in builtin/worktree.c to make git worktree add dwim more. While there add a description to the function. Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- I'm not sure where these functions should go ideally, they don't seem to fit in any existing library file, so I created a new one for now. Any suggestions for a better home are welcome! Makefile | 1 + builtin/checkout.c | 41 +---------------------------------------- checkout.c | 43 +++++++++++++++++++++++++++++++++++++++++++ checkout.h | 14 ++++++++++++++ 4 files changed, 59 insertions(+), 40 deletions(-) create mode 100644 checkout.c create mode 100644 checkout.h diff --git a/Makefile b/Makefile index cd75985991..8d603c7443 100644 --- a/Makefile +++ b/Makefile @@ -757,6 +757,7 @@ LIB_OBJS += branch.o LIB_OBJS += bulk-checkin.o LIB_OBJS += bundle.o LIB_OBJS += cache-tree.o +LIB_OBJS += checkout.o LIB_OBJS += color.o LIB_OBJS += column.o LIB_OBJS += combine-diff.o diff --git a/builtin/checkout.c b/builtin/checkout.c index fc4f8fd2ea..9e1cfd10b3 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1,5 +1,6 @@ #include "builtin.h" #include "config.h" +#include "checkout.h" #include "lockfile.h" #include "parse-options.h" #include "refs.h" @@ -874,46 +875,6 @@ static int git_checkout_config(const char *var, const char *value, void *cb) return git_xmerge_config(var, value, NULL); } -struct tracking_name_data { - /* const */ char *src_ref; - char *dst_ref; - struct object_id *dst_oid; - int unique; -}; - -static int check_tracking_name(struct remote *remote, void *cb_data) -{ - struct tracking_name_data *cb = cb_data; - struct refspec query; - memset(&query, 0, sizeof(struct refspec)); - query.src = cb->src_ref; - if (remote_find_tracking(remote, &query) || - get_oid(query.dst, cb->dst_oid)) { - free(query.dst); - return 0; - } - if (cb->dst_ref) { - free(query.dst); - cb->unique = 0; - return 0; - } - cb->dst_ref = query.dst; - return 0; -} - -static const char *unique_tracking_name(const char *name, struct object_id *oid) -{ - struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 }; - cb_data.src_ref = xstrfmt("refs/heads/%s", name); - cb_data.dst_oid = oid; - for_each_remote(check_tracking_name, &cb_data); - free(cb_data.src_ref); - if (cb_data.unique) - return cb_data.dst_ref; - free(cb_data.dst_ref); - return NULL; -} - static int parse_branchname_arg(int argc, const char **argv, int dwim_new_local_branch_ok, struct branch_info *new, diff --git a/checkout.c b/checkout.c new file mode 100644 index 0000000000..a9cf378453 --- /dev/null +++ b/checkout.c @@ -0,0 +1,43 @@ +#include "cache.h" + +#include "remote.h" + +struct tracking_name_data { + /* const */ char *src_ref; + char *dst_ref; + struct object_id *dst_oid; + int unique; +}; + +static int check_tracking_name(struct remote *remote, void *cb_data) +{ + struct tracking_name_data *cb = cb_data; + struct refspec query; + memset(&query, 0, sizeof(struct refspec)); + query.src = cb->src_ref; + if (remote_find_tracking(remote, &query) || + get_oid(query.dst, cb->dst_oid)) { + free(query.dst); + return 0; + } + if (cb->dst_ref) { + free(query.dst); + cb->unique = 0; + return 0; + } + cb->dst_ref = query.dst; + return 0; +} + +const char *unique_tracking_name(const char *name, struct object_id *oid) +{ + struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 }; + cb_data.src_ref = xstrfmt("refs/heads/%s", name); + cb_data.dst_oid = oid; + for_each_remote(check_tracking_name, &cb_data); + free(cb_data.src_ref); + if (cb_data.unique) + return cb_data.dst_ref; + free(cb_data.dst_ref); + return NULL; +} diff --git a/checkout.h b/checkout.h new file mode 100644 index 0000000000..9272fe1449 --- /dev/null +++ b/checkout.h @@ -0,0 +1,14 @@ +#ifndef CHECKOUT_H +#define CHECKOUT_H + +#include "git-compat-util.h" +#include "cache.h" + +/* + * Check if the branch name uniquely matches a branch name on a remote + * tracking branch. Return the name of the remote if such a branch + * exists, NULL otherwise. + */ +extern const char *unique_tracking_name(const char *name, struct object_id *oid); + +#endif /* CHECKOUT_H */ -- 2.15.0.403.gc27cc4dac6 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v1 2/2] worktree: make add dwim 2017-11-12 13:43 [PATCH v1 1/2] checkout: factor out functions to new lib file Thomas Gummerer @ 2017-11-12 13:43 ` Thomas Gummerer 2017-11-13 3:04 ` Junio C Hamano 2017-11-13 2:41 ` [PATCH v1 1/2] checkout: factor out functions to new lib file Junio C Hamano 2017-11-18 18:11 ` [PATCH v2 1/3] " Thomas Gummerer 2 siblings, 1 reply; 28+ messages in thread From: Thomas Gummerer @ 2017-11-12 13:43 UTC (permalink / raw) To: git; +Cc: Nguyễn Thái Ngọc Duy, Thomas Gummerer Currently git worktree add creates a new branch, that matches the HEAD of whichever worktree we were on when calling "git worktree add". Add a --guess flag to git worktree add, that makes it behave like the dwim machinery in 'git checkout <new-branch>', i.e. check if the new branch name uniquely matches the branch name of a remote tracking branch, and if so check out that branch, and set the upstream to the remote tracking branch. Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- I'm a bit torn about hiding his behind an additional flag in git worktree add or not. I would like to have the feature without the additional flag, but it might break some peoples expectations, so dunno. builtin/worktree.c | 14 +++++++++- t/t2025-worktree-add.sh | 70 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/builtin/worktree.c b/builtin/worktree.c index 7b9307aa58..5740d1f8a3 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "checkout.h" #include "config.h" #include "builtin.h" #include "dir.h" @@ -341,6 +342,7 @@ static int add(int ac, const char **av, const char *prefix) const char *new_branch_force = NULL; char *path; const char *branch; + int dwim_new_branch = 0; struct option options[] = { OPT__FORCE(&opts.force, N_("checkout <branch> even if already checked out in other worktree")), OPT_STRING('b', NULL, &opts.new_branch, N_("branch"), @@ -350,6 +352,7 @@ static int add(int ac, const char **av, const char *prefix) OPT_BOOL(0, "detach", &opts.detach, N_("detach HEAD at named commit")), OPT_BOOL(0, "checkout", &opts.checkout, N_("populate the new working tree")), OPT_BOOL(0, "lock", &opts.keep_locked, N_("keep the new working tree locked")), + OPT_BOOL(0, "guess", &dwim_new_branch, N_("checkout upstream branch if there's a unique match")), OPT_END() }; @@ -363,6 +366,7 @@ static int add(int ac, const char **av, const char *prefix) path = prefix_filename(prefix, av[0]); branch = ac < 2 ? "HEAD" : av[1]; + dwim_new_branch = ac < 2 ? dwim_new_branch : 0; if (!strcmp(branch, "-")) branch = "@{-1}"; @@ -387,13 +391,21 @@ static int add(int ac, const char **av, const char *prefix) } if (opts.new_branch) { + struct object_id oid; + const char *remote; struct child_process cp = CHILD_PROCESS_INIT; + + remote = unique_tracking_name(opts.new_branch, &oid); + cp.git_cmd = 1; argv_array_push(&cp.args, "branch"); if (opts.force_new_branch) argv_array_push(&cp.args, "--force"); argv_array_push(&cp.args, opts.new_branch); - argv_array_push(&cp.args, branch); + if (dwim_new_branch && remote) + argv_array_push(&cp.args, remote); + else + argv_array_push(&cp.args, branch); if (run_command(&cp)) return -1; branch = opts.new_branch; diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh index b5c47ac602..b37c279787 100755 --- a/t/t2025-worktree-add.sh +++ b/t/t2025-worktree-add.sh @@ -6,6 +6,16 @@ test_description='test git worktree add' . "$TEST_DIRECTORY"/lib-rebase.sh +# Is branch "refs/heads/$1" set to pull from "$2/$3"? +test_branch_upstream () { + printf "%s\n" "$2" "refs/heads/$3" >expect.upstream && + { + git config "branch.$1.remote" && + git config "branch.$1.merge" + } >actual.upstream && + test_cmp expect.upstream actual.upstream +} + test_expect_success 'setup' ' test_commit init ' @@ -314,4 +324,64 @@ test_expect_success 'rename a branch under bisect not allowed' ' test_must_fail git branch -M under-bisect bisect-with-new-name ' +test_expect_success 'git worktree add does not dwim' ' + test_when_finished rm -rf repo_a && + test_when_finished rm -rf repo_b && + test_when_finished rm -rf foo && + git init repo_a && + ( + cd repo_a && + test_commit a_master && + git checkout -b foo && + test_commit a_foo + ) && + git init repo_b && + ( + cd repo_b && + test_commit b_master && + git remote add repo_a ../repo_a && + git config remote.repo_a.fetch \ + "+refs/heads/*:refs/remotes/other_a/*" && + git fetch --all && + git worktree add ../foo + ) && + ( + cd foo && + ! test_branch_upstream foo repo_a foo && + git rev-parse other_a/foo >expect && + git rev-parse foo >actual && + ! test_cmp expect actual + ) +' + +test_expect_success 'git worktree add --guess dwims' ' + test_when_finished rm -rf repo_a && + test_when_finished rm -rf repo_b && + test_when_finished rm -rf foo && + git init repo_a && + ( + cd repo_a && + test_commit a_master && + git checkout -b foo && + test_commit a_foo + ) && + git init repo_b && + ( + cd repo_b && + test_commit b_master && + git remote add repo_a ../repo_a && + git config remote.repo_a.fetch \ + "+refs/heads/*:refs/remotes/other_a/*" && + git fetch --all && + git worktree add --guess ../foo + ) && + ( + cd foo && + test_branch_upstream foo repo_a foo && + git rev-parse other_a/foo >expect && + git rev-parse foo >actual && + test_cmp expect actual + ) +' + test_done -- 2.15.0.403.gc27cc4dac6 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v1 2/2] worktree: make add dwim 2017-11-12 13:43 ` [PATCH v1 2/2] worktree: make add dwim Thomas Gummerer @ 2017-11-13 3:04 ` Junio C Hamano 2017-11-14 8:45 ` Thomas Gummerer 0 siblings, 1 reply; 28+ messages in thread From: Junio C Hamano @ 2017-11-13 3:04 UTC (permalink / raw) To: Thomas Gummerer; +Cc: git, Nguyễn Thái Ngọc Duy Thomas Gummerer <t.gummerer@gmail.com> writes: > I'm a bit torn about hiding his behind an additional flag in git > worktree add or not. I would like to have the feature without the > additional flag, but it might break some peoples expectations, so > dunno. Yeah, I agree with the sentiment. But what expectation would we be breaking, I wonder (see below)? At the conceptual level, it is even more unfortunate that "git worktree --help" says this for "git worktree add <path> [<branch>]": If `<branch>` 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. which means that it already does a DWIM, namely "since you didn't say what branch to create to track what other branch, we'll fork one for you from the HEAD, and give a name to it". That may be a useful DWIM for some existing users sometimes, and you may even find it useful some of the time but not always. Different people mean different things in different situations, and there is no single definition for DWIMming that would fit all of them. Which in turn means that the new variable name DWIM_NEW_BRANCH and the new option name GUESS are way underspecified. You'd need to name them after the "kind" of dwim; otherwise, not only the future additions for new (third) kind of dwim would become cumbersome, but readers of the code would be confused if they are about the existing dwim (fork a new branch from HEAD and give it a name guessed by the pathname) or your new dwim. This also needs a documentation update. Unlike the existing DWIM, it is totally unclear when you are dwimming in absence of which option. I am guessing that, when you do not have a branch "topic" but your upstream does, saying $ git worktree add ../a-new-worktree topic would create "refs/heads/topic" to build on top of "refs/remotes/origin/topic" just like "git checkout topic" would. IOW, when fully spelled out: $ git branch -t -b topic origin/topic $ git worktree add ../a-new-worktree topic is what your DWIM does? Am I following you correctly? If so, as long as the new DWIM kicks in ONLY when "topic" does not exist, I suspect that there is no backward compatibility worries. The command line $ git worktree add ../a-new-worktree topic would just have failed because three was no 'topic' branch yet, no? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v1 2/2] worktree: make add dwim 2017-11-13 3:04 ` Junio C Hamano @ 2017-11-14 8:45 ` Thomas Gummerer 2017-11-14 20:14 ` Eric Sunshine 0 siblings, 1 reply; 28+ messages in thread From: Thomas Gummerer @ 2017-11-14 8:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Nguyễn Thái Ngọc Duy On 11/13, Junio C Hamano wrote: > Thomas Gummerer <t.gummerer@gmail.com> writes: > > > I'm a bit torn about hiding his behind an additional flag in git > > worktree add or not. I would like to have the feature without the > > additional flag, but it might break some peoples expectations, so > > dunno. > > Yeah, I agree with the sentiment. But what expectation would we be > breaking, I wonder (see below)? > > At the conceptual level, it is even more unfortunate that "git > worktree --help" says this for "git worktree add <path> [<branch>]": > > If `<branch>` 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. > > which means that it already does a DWIM, namely "since you didn't > say what branch to create to track what other branch, we'll fork one > for you from the HEAD, and give a name to it". That may be a useful > DWIM for some existing users sometimes, and you may even find it > useful some of the time but not always. Different people mean > different things in different situations, and there is no single > definition for DWIMming that would fit all of them. > > Which in turn means that the new variable name DWIM_NEW_BRANCH and > the new option name GUESS are way underspecified. You'd need to > name them after the "kind" of dwim; otherwise, not only the future > additions for new (third) kind of dwim would become cumbersome, but > readers of the code would be confused if they are about the existing > dwim (fork a new branch from HEAD and give it a name guessed by the > pathname) or your new dwim. Yeah I definitely agree with that. I was mainly thinking about my personal use case with --guess, and didn't fully think through that it might mean different things to other people. > This also needs a documentation update. Unlike the existing DWIM, > it is totally unclear when you are dwimming in absence of which > option. Sorry about that, I totally forgot about the documentation for this. Will add in the next round. > I am guessing that, when you do not have a branch "topic" but your > upstream does, saying > > $ git worktree add ../a-new-worktree topic > > would create "refs/heads/topic" to build on top of > "refs/remotes/origin/topic" just like "git checkout topic" would. > > IOW, when fully spelled out: > > $ git branch -t -b topic origin/topic > $ git worktree add ../a-new-worktree topic > > is what your DWIM does? Am I following you correctly? It's not quite what my DWIM does/what I originally intended my DWIM to do. What it does is when $ git worktree add ../topic and omitting the branch argument, it would behave the way you spelled out, i.e. $ git branch -t -b topic origin/topic $ git worktree add ../topic topic instead of just checking it out from HEAD. I must confess I missed the branch argument, so that still behaves the old way with my code, while what you have above would make a lot more sense. > If so, as long as the new DWIM kicks in ONLY when "topic" does not > exist, I suspect that there is no backward compatibility worries. > The command line > > $ git worktree add ../a-new-worktree topic > > would just have failed because three was no 'topic' branch yet, no? Indeed, with this there would not be any backwards compatility worries. Ideally I'd still like to make $ git worktree add ../topic work as described above, to avoid having to type 'topic' twice (the directory name matches the branch name for the vast majority of my worktrees) but that should then come behind a flag/config option? Following your reasoning above it should probably be called something other than --guess. Maybe --guess-remote and worktree.guessRemote would do? I'm quite bad at naming though, so other suggestions are very welcome. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v1 2/2] worktree: make add dwim 2017-11-14 8:45 ` Thomas Gummerer @ 2017-11-14 20:14 ` Eric Sunshine 2017-11-14 20:29 ` Eric Sunshine 2017-11-15 8:50 ` Thomas Gummerer 0 siblings, 2 replies; 28+ messages in thread From: Eric Sunshine @ 2017-11-14 20:14 UTC (permalink / raw) To: Thomas Gummerer Cc: Junio C Hamano, Git List, Nguyễn Thái Ngọc Duy On Tue, Nov 14, 2017 at 3:45 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote: > On 11/13, Junio C Hamano wrote: >> If so, as long as the new DWIM kicks in ONLY when "topic" does not >> exist, I suspect that there is no backward compatibility worries. >> The command line >> >> $ git worktree add ../a-new-worktree topic >> >> would just have failed because three was no 'topic' branch yet, no? > > Indeed, with this there would not be any backwards compatility > worries. > > Ideally I'd still like to make > > $ git worktree add ../topic > > work as described above, to avoid having to type 'topic' twice (the > directory name matches the branch name for the vast majority of my > worktrees) but that should then come behind a flag/config option? > Following your reasoning above it should probably be called something > other than --guess. > > Maybe --guess-remote and worktree.guessRemote would do? I'm quite bad > at naming though, so other suggestions are very welcome. For my own edification... git worktree add ../dir branch * Checks out branch into ../dir if branch exists. * Errors out if branch does not exist. However, if we consider the history of the implementation of worktrees[*1*], then this really ought to try the "origin/branch -> branch" DWIM before erroring-out. Implementing this DWIM could be considered a regression fix according to [*1*] and, as Junio points out, should not pose backward compatibility worries. git worktree add ../topic * Correctly errors out, refusing to create a new branch named "topic", if "topic" is already a branch. * Creates a new branch named "topic" if no such local branch exists. The desired new DWIMing would change the second bullet point to: * If no local branch named "topic" exists, DWIM it from "origin/topic" if possible, else create a new local branch named "topic". But that's a behavior change since, with the existing implementation, a newly created local "topic" has no relation to, and does not track, any origin/topic branch. The proposed --guess option is to avoid users being hit by this backward incompatibility, however, one could also view the proposed DWIMing as some sort of bug fix since, at least some, users would expect the DWIMing since it already takes place elsewhere. So, at least two options exist: * Just make the new DWIMing the default behavior, accepting that it might bite a few people. Fallout can be mitigated somewhat by prominently announcing that the DWIMing took place, in which case the user can take corrective action (say, by choosing a different worktree name); nothing is lost and no damage done since this is happening only at worktree creation time. * Add a new option to enable DWIMing; perhaps name it -t/--track, which is familiar terminology and still gives you a relatively short and sweet "git worktree add -t ../topic". Given the mentioned mitigation factor and that some/many users likely would expect it to DWIM "origin/topic -> topic" anyhow, I'd lean in favor of the first option (but perhaps I'm too daring with other people's workflows). FOOTNOTES [*1*]: When Duy first implemented worktree support, he incorporated it directly into the git-checkout command ("git checkout --to worktree ..."), which means that he got all the git-checkout features for free, including the "origin/branch -> branch" DWIM. When worktree support was later moved to git-worktree, it lost most of the features inherited implicitly from git-checkout, such as -b, -B, --detach, so those were added back to git-worktree explicitly. However, at that early stage, git-worktree was still piggy-backing atop git-checkout, thus likely was still getting the "origin/branch -> branch" DWIM for free. A final iteration converted git-worktree away from heavyweight git-checkout to lightweight git-reset, at which point he DWIMing was lost. If you take this history into account, then loss of "origin/branch -> branch" DWIMing is a regression, so restoring it could be considered a bug fix. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v1 2/2] worktree: make add dwim 2017-11-14 20:14 ` Eric Sunshine @ 2017-11-14 20:29 ` Eric Sunshine 2017-11-15 8:52 ` Thomas Gummerer 2017-11-15 8:50 ` Thomas Gummerer 1 sibling, 1 reply; 28+ messages in thread From: Eric Sunshine @ 2017-11-14 20:29 UTC (permalink / raw) To: Thomas Gummerer Cc: Junio C Hamano, Git List, Nguyễn Thái Ngọc Duy On Tue, Nov 14, 2017 at 3:14 PM, Eric Sunshine <sunshine@sunshineco.com> wrote: > For my own edification... > [...] > git worktree add ../topic > > * Correctly errors out, refusing to create a new branch named "topic", > if "topic" is already a branch. By the way, there's an additional DWIM that could be done here instead of erroring out. Specifically, for "git worktree add ../topic": * If branch "topic" exists, check it out (rather than refusing to create a new branch named "topic"). * If origin/topic exists, DWIM local "topic" branch into existence. * Otherwise, create new local branch "topic". > * Creates a new branch named "topic" if no such local branch exists. > > The desired new DWIMing would change the second bullet point to: > > * If no local branch named "topic" exists, DWIM it from "origin/topic" > if possible, else create a new local branch named "topic". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v1 2/2] worktree: make add dwim 2017-11-14 20:29 ` Eric Sunshine @ 2017-11-15 8:52 ` Thomas Gummerer 2017-11-18 18:13 ` Thomas Gummerer 0 siblings, 1 reply; 28+ messages in thread From: Thomas Gummerer @ 2017-11-15 8:52 UTC (permalink / raw) To: Eric Sunshine Cc: Junio C Hamano, Git List, Nguyễn Thái Ngọc Duy On 11/14, Eric Sunshine wrote: > On Tue, Nov 14, 2017 at 3:14 PM, Eric Sunshine <sunshine@sunshineco.com> wrote: > > For my own edification... > > [...] > > git worktree add ../topic > > > > * Correctly errors out, refusing to create a new branch named "topic", > > if "topic" is already a branch. > > By the way, there's an additional DWIM that could be done here instead > of erroring out. Specifically, for "git worktree add ../topic": > > * If branch "topic" exists, check it out (rather than refusing to > create a new branch named "topic"). I think this would be a good improvement either way as I suspect this is what users would hope for, and as it currently just dies there are less backwards compatibility worries. > * If origin/topic exists, DWIM local "topic" branch into existence. > > * Otherwise, create new local branch "topic". > > > * Creates a new branch named "topic" if no such local branch exists. > > > > The desired new DWIMing would change the second bullet point to: > > > > * If no local branch named "topic" exists, DWIM it from "origin/topic" > > if possible, else create a new local branch named "topic". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v1 2/2] worktree: make add dwim 2017-11-15 8:52 ` Thomas Gummerer @ 2017-11-18 18:13 ` Thomas Gummerer 0 siblings, 0 replies; 28+ messages in thread From: Thomas Gummerer @ 2017-11-18 18:13 UTC (permalink / raw) To: Eric Sunshine Cc: Junio C Hamano, Git List, Nguyễn Thái Ngọc Duy On 11/15, Thomas Gummerer wrote: > On 11/14, Eric Sunshine wrote: > > On Tue, Nov 14, 2017 at 3:14 PM, Eric Sunshine <sunshine@sunshineco.com> wrote: > > > For my own edification... > > > [...] > > > git worktree add ../topic > > > > > > * Correctly errors out, refusing to create a new branch named "topic", > > > if "topic" is already a branch. > > > > By the way, there's an additional DWIM that could be done here instead > > of erroring out. Specifically, for "git worktree add ../topic": > > > > * If branch "topic" exists, check it out (rather than refusing to > > create a new branch named "topic"). > > I think this would be a good improvement either way as I suspect this > is what users would hope for, and as it currently just dies there are > less backwards compatibility worries. While I still think this would be an improvement, after thinking about it a bit more I think this is somewhat orthogonal to what I'm trying to achieve with this patch series. Therefore I didn't implement this yet, but I'm still thinking of implementing this in a separate topic. > > * If origin/topic exists, DWIM local "topic" branch into existence. > > > > * Otherwise, create new local branch "topic". > > > > > * Creates a new branch named "topic" if no such local branch exists. > > > > > > The desired new DWIMing would change the second bullet point to: > > > > > > * If no local branch named "topic" exists, DWIM it from "origin/topic" > > > if possible, else create a new local branch named "topic". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v1 2/2] worktree: make add dwim 2017-11-14 20:14 ` Eric Sunshine 2017-11-14 20:29 ` Eric Sunshine @ 2017-11-15 8:50 ` Thomas Gummerer 2017-11-15 9:12 ` Eric Sunshine 1 sibling, 1 reply; 28+ messages in thread From: Thomas Gummerer @ 2017-11-15 8:50 UTC (permalink / raw) To: Eric Sunshine Cc: Junio C Hamano, Git List, Nguyễn Thái Ngọc Duy On 11/14, Eric Sunshine wrote: > On Tue, Nov 14, 2017 at 3:45 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote: > > On 11/13, Junio C Hamano wrote: > >> If so, as long as the new DWIM kicks in ONLY when "topic" does not > >> exist, I suspect that there is no backward compatibility worries. > >> The command line > >> > >> $ git worktree add ../a-new-worktree topic > >> > >> would just have failed because three was no 'topic' branch yet, no? > > > > Indeed, with this there would not be any backwards compatility > > worries. > > > > Ideally I'd still like to make > > > > $ git worktree add ../topic > > > > work as described above, to avoid having to type 'topic' twice (the > > directory name matches the branch name for the vast majority of my > > worktrees) but that should then come behind a flag/config option? > > Following your reasoning above it should probably be called something > > other than --guess. > > > > Maybe --guess-remote and worktree.guessRemote would do? I'm quite bad > > at naming though, so other suggestions are very welcome. > > For my own edification... > > git worktree add ../dir branch > > * Checks out branch into ../dir if branch exists. > > * Errors out if branch does not exist. However, if we consider the > history of the implementation of worktrees[*1*], then this really > ought to try the "origin/branch -> branch" DWIM before erroring-out. > Implementing this DWIM could be considered a regression fix according > to [*1*] and, as Junio points out, should not pose backward > compatibility worries. Agreed, I think it is not very controversial that this would be an improvement. > git worktree add ../topic > > * Correctly errors out, refusing to create a new branch named "topic", > if "topic" is already a branch. > > * Creates a new branch named "topic" if no such local branch exists. > > The desired new DWIMing would change the second bullet point to: > > * If no local branch named "topic" exists, DWIM it from "origin/topic" > if possible, else create a new local branch named "topic". > > But that's a behavior change since, with the existing implementation, > a newly created local "topic" has no relation to, and does not track, > any origin/topic branch. The proposed --guess option is to avoid users > being hit by this backward incompatibility, however, one could also > view the proposed DWIMing as some sort of bug fix since, at least > some, users would expect the DWIMing since it already takes place > elsewhere. I'm not sure we can call it a bug fix anymore, since as Junio pointed out the current behaviour of creating a new branch at HEAD is documented in the man page. However git-worktree is also still marked as experimental in the man page, so we could allow ourselves to be a little bit more lax when it comes to backwards compatibility, especially because it is easy to take corrective action after the new DWIMing happened. > So, at least two options exist: > > * Just make the new DWIMing the default behavior, accepting that it > might bite a few people. Fallout can be mitigated somewhat by > prominently announcing that the DWIMing took place, in which case the > user can take corrective action (say, by choosing a different worktree > name); nothing is lost and no damage done since this is happening only > at worktree creation time. > > * Add a new option to enable DWIMing; perhaps name it -t/--track, > which is familiar terminology and still gives you a relatively short > and sweet "git worktree add -t ../topic". > > Given the mentioned mitigation factor and that some/many users likely > would expect it to DWIM "origin/topic -> topic" anyhow, I'd lean in > favor of the first option (but perhaps I'm too daring with other > people's workflows). Yeah, I'm leaning towards the first option as well, but I'm clearly biased as that's how I'd like it to behave, and others might want the other behaviour. Unfortunately I don't know many worktree users, so I can't tell what the general consensus on this would be. I guess the second option would be the safer one, and we can still switch that to be the default at some point if we wish to do so later. tl;dr I have no idea which of the options would be better :) > FOOTNOTES > > [*1*]: When Duy first implemented worktree support, he incorporated it > directly into the git-checkout command ("git checkout --to worktree > ..."), which means that he got all the git-checkout features for free, > including the "origin/branch -> branch" DWIM. When worktree support > was later moved to git-worktree, it lost most of the features > inherited implicitly from git-checkout, such as -b, -B, --detach, so > those were added back to git-worktree explicitly. However, at that > early stage, git-worktree was still piggy-backing atop git-checkout, > thus likely was still getting the "origin/branch -> branch" DWIM for > free. A final iteration converted git-worktree away from heavyweight > git-checkout to lightweight git-reset, at which point he DWIMing was > lost. If you take this history into account, then loss of > "origin/branch -> branch" DWIMing is a regression, so restoring it > could be considered a bug fix. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v1 2/2] worktree: make add dwim 2017-11-15 8:50 ` Thomas Gummerer @ 2017-11-15 9:12 ` Eric Sunshine 0 siblings, 0 replies; 28+ messages in thread From: Eric Sunshine @ 2017-11-15 9:12 UTC (permalink / raw) To: Thomas Gummerer Cc: Junio C Hamano, Git List, Nguyễn Thái Ngọc Duy On Wed, Nov 15, 2017 at 3:50 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote: > On 11/14, Eric Sunshine wrote: >> git worktree add ../topic >> [...] >> The desired new DWIMing would change the second bullet point to: >> >> * If no local branch named "topic" exists, DWIM it from "origin/topic" >> if possible, else create a new local branch named "topic". >> >> But that's a behavior change since, with the existing implementation, >> a newly created local "topic" has no relation to, and does not track, >> any origin/topic branch. The proposed --guess option is to avoid users >> being hit by this backward incompatibility, however, one could also >> view the proposed DWIMing as some sort of bug fix since, at least >> some, users would expect the DWIMing since it already takes place >> elsewhere. > > I'm not sure we can call it a bug fix anymore, since as Junio pointed > out the current behaviour of creating a new branch at HEAD is > documented in the man page. > > However git-worktree is also still marked as experimental in the man > page, so we could allow ourselves to be a little bit more lax when it > comes to backwards compatibility, especially because it is easy to > take corrective action after the new DWIMing happened. Yep, my leaning toward making this new DWIMing default (without a --guess or --track option) also is based in part on git-worktree still being marked "experimental". >> So, at least two options exist: >> >> * Just make the new DWIMing the default behavior, accepting that it >> might bite a few people. Fallout can be mitigated somewhat by >> prominently announcing that the DWIMing took place, in which case the >> user can take corrective action (say, by choosing a different worktree >> name); nothing is lost and no damage done since this is happening only >> at worktree creation time. >> >> * Add a new option to enable DWIMing; perhaps name it -t/--track, >> which is familiar terminology and still gives you a relatively short >> and sweet "git worktree add -t ../topic". >> >> Given the mentioned mitigation factor and that some/many users likely >> would expect it to DWIM "origin/topic -> topic" anyhow, I'd lean in >> favor of the first option (but perhaps I'm too daring with other >> people's workflows). > > Yeah, I'm leaning towards the first option as well, but I'm clearly > biased as that's how I'd like it to behave, and others might want the > other behaviour. Unfortunately I don't know many worktree users, so I > can't tell what the general consensus on this would be. Aside from the mentioned mitigation factor, which somewhat eases my worries about backward incompatibility, one genuine concern is breaking existing scripts. At the very least, if the new DWIM becomes default, there might need to be some escape hatch for scripts to opt out of it. > I guess the second option would be the safer one, and we can still > switch that to be the default at some point if we wish to do so > later. The longer you wait, the less likely you'll have the chance since git-worktree will (presumably) gain more users and be used in more scripts as time passes. So, if the new DWIMing is to become the default, better to do so earlier rather than later. > tl;dr I have no idea which of the options would be better :) I'm probably too cavalier and shortsighted (at least on this topic) to make a well-informed decision about it. Junio probably has a better feeling about whether such a change of behavior makes sense at this late date, and, of course, it's his decision whether to accept such a change into his tree. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v1 1/2] checkout: factor out functions to new lib file 2017-11-12 13:43 [PATCH v1 1/2] checkout: factor out functions to new lib file Thomas Gummerer 2017-11-12 13:43 ` [PATCH v1 2/2] worktree: make add dwim Thomas Gummerer @ 2017-11-13 2:41 ` Junio C Hamano 2017-11-14 8:46 ` Thomas Gummerer 2017-11-18 18:11 ` [PATCH v2 1/3] " Thomas Gummerer 2 siblings, 1 reply; 28+ messages in thread From: Junio C Hamano @ 2017-11-13 2:41 UTC (permalink / raw) To: Thomas Gummerer; +Cc: git, Nguyễn Thái Ngọc Duy Thomas Gummerer <t.gummerer@gmail.com> writes: > Factor the functions out, so they can be re-used from other places. In > particular these functions will be re-used in builtin/worktree.c to make > git worktree add dwim more. > > While there add a description to the function. > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > --- > > I'm not sure where these functions should go ideally, they don't seem > to fit in any existing library file, so I created a new one for now. > Any suggestions for a better home are welcome! > > Makefile | 1 + > builtin/checkout.c | 41 +---------------------------------------- > checkout.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > checkout.h | 14 ++++++++++++++ > 4 files changed, 59 insertions(+), 40 deletions(-) > create mode 100644 checkout.c > create mode 100644 checkout.h I am not sure either. I've always considered that entry.c is the libified thing codepath that want to do a "checkout" should call, but the functions that you are moving is at a "higher layer" that does not concern the core-git data structures (i.e. the 'tracking' is a mere "user" of the ref API, unlike things in entry.c such as checkout_entry() that is a more "maintainer" of the index integrity), so perhaps it makes sense to have new file for it. > diff --git a/checkout.c b/checkout.c > new file mode 100644 > index 0000000000..a9cf378453 > --- /dev/null > +++ b/checkout.c > @@ -0,0 +1,43 @@ > +#include "cache.h" > + A useless blank line. > +#include "remote.h" > + This one is useful ;-) > +struct tracking_name_data { > ... > diff --git a/checkout.h b/checkout.h > new file mode 100644 > index 0000000000..9272fe1449 > --- /dev/null > +++ b/checkout.h > @@ -0,0 +1,14 @@ > +#ifndef CHECKOUT_H > +#define CHECKOUT_H > + > +#include "git-compat-util.h" > +#include "cache.h" Try "git grep -e git-compat-util Documentation/CodingGuidelines" and just keep inclusion of "cache.h". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v1 1/2] checkout: factor out functions to new lib file 2017-11-13 2:41 ` [PATCH v1 1/2] checkout: factor out functions to new lib file Junio C Hamano @ 2017-11-14 8:46 ` Thomas Gummerer 0 siblings, 0 replies; 28+ messages in thread From: Thomas Gummerer @ 2017-11-14 8:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Nguyễn Thái Ngọc Duy On 11/13, Junio C Hamano wrote: > Thomas Gummerer <t.gummerer@gmail.com> writes: > > > Factor the functions out, so they can be re-used from other places. In > > particular these functions will be re-used in builtin/worktree.c to make > > git worktree add dwim more. > > > > While there add a description to the function. > > > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > > --- > > > > I'm not sure where these functions should go ideally, they don't seem > > to fit in any existing library file, so I created a new one for now. > > Any suggestions for a better home are welcome! > > > > Makefile | 1 + > > builtin/checkout.c | 41 +---------------------------------------- > > checkout.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > > checkout.h | 14 ++++++++++++++ > > 4 files changed, 59 insertions(+), 40 deletions(-) > > create mode 100644 checkout.c > > create mode 100644 checkout.h > > I am not sure either. I've always considered that entry.c is the > libified thing codepath that want to do a "checkout" should call, > but the functions that you are moving is at a "higher layer" that > does not concern the core-git data structures (i.e. the 'tracking' > is a mere "user" of the ref API, unlike things in entry.c such as > checkout_entry() that is a more "maintainer" of the index integrity), > so perhaps it makes sense to have new file for it. Makes sense, let me keep it as a new file then. > > diff --git a/checkout.c b/checkout.c > > new file mode 100644 > > index 0000000000..a9cf378453 > > --- /dev/null > > +++ b/checkout.c > > @@ -0,0 +1,43 @@ > > +#include "cache.h" > > + > > A useless blank line. > > > +#include "remote.h" > > + > > This one is useful ;-) > > > +struct tracking_name_data { > > ... > > diff --git a/checkout.h b/checkout.h > > new file mode 100644 > > index 0000000000..9272fe1449 > > --- /dev/null > > +++ b/checkout.h > > @@ -0,0 +1,14 @@ > > +#ifndef CHECKOUT_H > > +#define CHECKOUT_H > > + > > +#include "git-compat-util.h" > > +#include "cache.h" > > Try "git grep -e git-compat-util Documentation/CodingGuidelines" and > just keep inclusion of "cache.h". Ah I forgot about that. Will fix, thanks! ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 1/3] checkout: factor out functions to new lib file 2017-11-12 13:43 [PATCH v1 1/2] checkout: factor out functions to new lib file Thomas Gummerer 2017-11-12 13:43 ` [PATCH v1 2/2] worktree: make add dwim Thomas Gummerer 2017-11-13 2:41 ` [PATCH v1 1/2] checkout: factor out functions to new lib file Junio C Hamano @ 2017-11-18 18:11 ` Thomas Gummerer 2017-11-18 18:11 ` [PATCH v2 2/3] worktree: make add <path> <branch> dwim Thomas Gummerer ` (2 more replies) 2 siblings, 3 replies; 28+ messages in thread From: Thomas Gummerer @ 2017-11-18 18:11 UTC (permalink / raw) To: git Cc: Nguyễn Thái Ngọc Duy, Junio C Hamano, Eric Sunshine, Thomas Gummerer Factor the functions out, so they can be re-used from other places. In particular these functions will be re-used in builtin/worktree.c to make git worktree add dwim more. While there add some docs to the function. Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- The previous round of this series is at https://public-inbox.org/git/20171112134305.3949-1-t.gummerer@gmail.com/. Thanks Junio and Eric for the comments on the previous round! Makefile | 1 + builtin/checkout.c | 41 +---------------------------------------- checkout.c | 42 ++++++++++++++++++++++++++++++++++++++++++ checkout.h | 13 +++++++++++++ 4 files changed, 57 insertions(+), 40 deletions(-) create mode 100644 checkout.c create mode 100644 checkout.h diff --git a/Makefile b/Makefile index cd75985991..8d603c7443 100644 --- a/Makefile +++ b/Makefile @@ -757,6 +757,7 @@ LIB_OBJS += branch.o LIB_OBJS += bulk-checkin.o LIB_OBJS += bundle.o LIB_OBJS += cache-tree.o +LIB_OBJS += checkout.o LIB_OBJS += color.o LIB_OBJS += column.o LIB_OBJS += combine-diff.o diff --git a/builtin/checkout.c b/builtin/checkout.c index fc4f8fd2ea..9e1cfd10b3 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1,5 +1,6 @@ #include "builtin.h" #include "config.h" +#include "checkout.h" #include "lockfile.h" #include "parse-options.h" #include "refs.h" @@ -874,46 +875,6 @@ static int git_checkout_config(const char *var, const char *value, void *cb) return git_xmerge_config(var, value, NULL); } -struct tracking_name_data { - /* const */ char *src_ref; - char *dst_ref; - struct object_id *dst_oid; - int unique; -}; - -static int check_tracking_name(struct remote *remote, void *cb_data) -{ - struct tracking_name_data *cb = cb_data; - struct refspec query; - memset(&query, 0, sizeof(struct refspec)); - query.src = cb->src_ref; - if (remote_find_tracking(remote, &query) || - get_oid(query.dst, cb->dst_oid)) { - free(query.dst); - return 0; - } - if (cb->dst_ref) { - free(query.dst); - cb->unique = 0; - return 0; - } - cb->dst_ref = query.dst; - return 0; -} - -static const char *unique_tracking_name(const char *name, struct object_id *oid) -{ - struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 }; - cb_data.src_ref = xstrfmt("refs/heads/%s", name); - cb_data.dst_oid = oid; - for_each_remote(check_tracking_name, &cb_data); - free(cb_data.src_ref); - if (cb_data.unique) - return cb_data.dst_ref; - free(cb_data.dst_ref); - return NULL; -} - static int parse_branchname_arg(int argc, const char **argv, int dwim_new_local_branch_ok, struct branch_info *new, diff --git a/checkout.c b/checkout.c new file mode 100644 index 0000000000..b0c744d37a --- /dev/null +++ b/checkout.c @@ -0,0 +1,42 @@ +#include "cache.h" +#include "remote.h" + +struct tracking_name_data { + /* const */ char *src_ref; + char *dst_ref; + struct object_id *dst_oid; + int unique; +}; + +static int check_tracking_name(struct remote *remote, void *cb_data) +{ + struct tracking_name_data *cb = cb_data; + struct refspec query; + memset(&query, 0, sizeof(struct refspec)); + query.src = cb->src_ref; + if (remote_find_tracking(remote, &query) || + get_oid(query.dst, cb->dst_oid)) { + free(query.dst); + return 0; + } + if (cb->dst_ref) { + free(query.dst); + cb->unique = 0; + return 0; + } + cb->dst_ref = query.dst; + return 0; +} + +const char *unique_tracking_name(const char *name, struct object_id *oid) +{ + struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 }; + cb_data.src_ref = xstrfmt("refs/heads/%s", name); + cb_data.dst_oid = oid; + for_each_remote(check_tracking_name, &cb_data); + free(cb_data.src_ref); + if (cb_data.unique) + return cb_data.dst_ref; + free(cb_data.dst_ref); + return NULL; +} diff --git a/checkout.h b/checkout.h new file mode 100644 index 0000000000..9980711179 --- /dev/null +++ b/checkout.h @@ -0,0 +1,13 @@ +#ifndef CHECKOUT_H +#define CHECKOUT_H + +#include "cache.h" + +/* + * Check if the branch name uniquely matches a branch name on a remote + * tracking branch. Return the name of the remote if such a branch + * exists, NULL otherwise. + */ +extern const char *unique_tracking_name(const char *name, struct object_id *oid); + +#endif /* CHECKOUT_H */ -- 2.15.0.345.gf926f18f3d ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 2/3] worktree: make add <path> <branch> dwim 2017-11-18 18:11 ` [PATCH v2 1/3] " Thomas Gummerer @ 2017-11-18 18:11 ` Thomas Gummerer 2017-11-18 22:18 ` Thomas Gummerer 2017-11-18 18:11 ` [PATCH v2 3/3] worktree: make add <path> dwim Thomas Gummerer 2017-11-18 22:47 ` [PATCH v3 0/3] make git worktree add dwim more Thomas Gummerer 2 siblings, 1 reply; 28+ messages in thread From: Thomas Gummerer @ 2017-11-18 18:11 UTC (permalink / raw) To: git Cc: Nguyễn Thái Ngọc Duy, Junio C Hamano, Eric Sunshine, Thomas Gummerer Currently 'git worktree add <path> <branch>', errors out when 'branch' is not a local branch. It has no additional dwim'ing features that one might expect. Make it behave more like 'git checkout <branch>' when the branch doesn't exist locally, but a remote tracking branch uniquely matches the desired branch name, i.e. create a new branch from the remote tracking branch and set the upstream to the remote tracking branch. As 'git worktree add' currently just dies in this situation, there are no backwards compatibility worries when introducing this feature. Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- Documentation/git-worktree.txt | 7 +++++++ builtin/worktree.c | 15 ++++++++++++++ t/t2025-worktree-add.sh | 44 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+) diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index b472acc356..3c7c8c3cee 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -52,6 +52,13 @@ is linked to the current repository, sharing everything except working directory specific files such as HEAD, index, etc. `-` may also be specified as `<branch>`; it is synonymous with `@{-1}`. + +If <branch> is not found but there does exist a tracking branch in +exactly one remote (call it <remote>) with a matching name, treat as +equivalent to +------------ +$ git worktree add -b <branch> <path> <remote>/<branch> +------------ ++ If `<branch>` 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. diff --git a/builtin/worktree.c b/builtin/worktree.c index 7b9307aa58..92b583ae39 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "checkout.h" #include "config.h" #include "builtin.h" #include "dir.h" @@ -386,6 +387,20 @@ static int add(int ac, const char **av, const char *prefix) opts.new_branch = xstrndup(s, n); } + if (ac == 2) { + struct object_id oid; + struct commit *commit; + const char *remote; + + commit = lookup_commit_reference_by_name(branch); + if (!commit) + remote = unique_tracking_name(branch, &oid); + if (!commit && remote) { + opts.new_branch = branch; + branch = remote; + } + } + if (opts.new_branch) { struct child_process cp = CHILD_PROCESS_INIT; cp.git_cmd = 1; diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh index b5c47ac602..e5959800c0 100755 --- a/t/t2025-worktree-add.sh +++ b/t/t2025-worktree-add.sh @@ -6,6 +6,16 @@ test_description='test git worktree add' . "$TEST_DIRECTORY"/lib-rebase.sh +# Is branch "refs/heads/$1" set to pull from "$2/$3"? +test_branch_upstream () { + printf "%s\n" "$2" "refs/heads/$3" >expect.upstream && + { + git config "branch.$1.remote" && + git config "branch.$1.merge" + } >actual.upstream && + test_cmp expect.upstream actual.upstream +} + test_expect_success 'setup' ' test_commit init ' @@ -314,4 +324,38 @@ test_expect_success 'rename a branch under bisect not allowed' ' test_must_fail git branch -M under-bisect bisect-with-new-name ' +test_expect_success '"add" <path> <non-existent-branch> fails' ' + test_must_fail git worktree add foo non-existent +' + +test_expect_success '"add" <path> <branch> dwims' ' + test_when_finished rm -rf repo_upstream && + test_when_finished rm -rf repo_dwim && + test_when_finished rm -rf foo && + git init repo_upstream && + ( + cd repo_upstream && + test_commit upstream_master && + git checkout -b foo && + test_commit a_foo + ) && + git init repo_dwim && + ( + cd repo_dwim && + test_commit dwim_master && + git remote add repo_upstream ../repo_upstream && + git config remote.repo_upstream.fetch \ + "refs/heads/*:refs/remotes/repo_upstream/*" && + git fetch --all && + git worktree add ../foo foo + ) && + ( + cd foo && + test_branch_upstream foo repo_upstream foo && + git rev-parse repo_upstream/foo >expect && + git rev-parse foo >actual && + test_cmp expect actual + ) +' + test_done -- 2.15.0.345.gf926f18f3d ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/3] worktree: make add <path> <branch> dwim 2017-11-18 18:11 ` [PATCH v2 2/3] worktree: make add <path> <branch> dwim Thomas Gummerer @ 2017-11-18 22:18 ` Thomas Gummerer 0 siblings, 0 replies; 28+ messages in thread From: Thomas Gummerer @ 2017-11-18 22:18 UTC (permalink / raw) To: git; +Cc: Nguyễn Thái Ngọc Duy, Junio C Hamano, Eric Sunshine On 11/18, Thomas Gummerer wrote: > Currently 'git worktree add <path> <branch>', errors out when 'branch' > is not a local branch. It has no additional dwim'ing features that one > might expect. > > Make it behave more like 'git checkout <branch>' when the branch doesn't > exist locally, but a remote tracking branch uniquely matches the desired > branch name, i.e. create a new branch from the remote tracking branch > and set the upstream to the remote tracking branch. > > As 'git worktree add' currently just dies in this situation, there are > no backwards compatibility worries when introducing this feature. > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > --- > Documentation/git-worktree.txt | 7 +++++++ > builtin/worktree.c | 15 ++++++++++++++ > t/t2025-worktree-add.sh | 44 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 66 insertions(+) > > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > index b472acc356..3c7c8c3cee 100644 > --- a/Documentation/git-worktree.txt > +++ b/Documentation/git-worktree.txt > @@ -52,6 +52,13 @@ is linked to the current repository, sharing everything except working > directory specific files such as HEAD, index, etc. `-` may also be > specified as `<branch>`; it is synonymous with `@{-1}`. > + > +If <branch> is not found but there does exist a tracking branch in > +exactly one remote (call it <remote>) with a matching name, treat as > +equivalent to > +------------ > +$ git worktree add -b <branch> <path> <remote>/<branch> > +------------ > ++ > If `<branch>` 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. > diff --git a/builtin/worktree.c b/builtin/worktree.c > index 7b9307aa58..92b583ae39 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -1,4 +1,5 @@ > #include "cache.h" > +#include "checkout.h" > #include "config.h" > #include "builtin.h" > #include "dir.h" > @@ -386,6 +387,20 @@ static int add(int ac, const char **av, const char *prefix) > opts.new_branch = xstrndup(s, n); > } > > + if (ac == 2) { Err I just realized this doesn't quite make sense. Similar to the dwim for 'git worktree add <path>', this condition should include '!opts.new_branch && !opts.detach'. In these cases it's still better to error out, as the user explicitly asked for a new branch with a different name/asked not to be put onto a branch. I'll send a v3 with this fixed in a bit. > + struct object_id oid; > + struct commit *commit; > + const char *remote; > + > + commit = lookup_commit_reference_by_name(branch); > + if (!commit) > + remote = unique_tracking_name(branch, &oid); > + if (!commit && remote) { > + opts.new_branch = branch; > + branch = remote; > + } > + } > + > if (opts.new_branch) { > struct child_process cp = CHILD_PROCESS_INIT; > cp.git_cmd = 1; > diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh > index b5c47ac602..e5959800c0 100755 > --- a/t/t2025-worktree-add.sh > +++ b/t/t2025-worktree-add.sh > @@ -6,6 +6,16 @@ test_description='test git worktree add' > > . "$TEST_DIRECTORY"/lib-rebase.sh > > +# Is branch "refs/heads/$1" set to pull from "$2/$3"? > +test_branch_upstream () { > + printf "%s\n" "$2" "refs/heads/$3" >expect.upstream && > + { > + git config "branch.$1.remote" && > + git config "branch.$1.merge" > + } >actual.upstream && > + test_cmp expect.upstream actual.upstream > +} > + > test_expect_success 'setup' ' > test_commit init > ' > @@ -314,4 +324,38 @@ test_expect_success 'rename a branch under bisect not allowed' ' > test_must_fail git branch -M under-bisect bisect-with-new-name > ' > > +test_expect_success '"add" <path> <non-existent-branch> fails' ' > + test_must_fail git worktree add foo non-existent > +' > + > +test_expect_success '"add" <path> <branch> dwims' ' > + test_when_finished rm -rf repo_upstream && > + test_when_finished rm -rf repo_dwim && > + test_when_finished rm -rf foo && > + git init repo_upstream && > + ( > + cd repo_upstream && > + test_commit upstream_master && > + git checkout -b foo && > + test_commit a_foo > + ) && > + git init repo_dwim && > + ( > + cd repo_dwim && > + test_commit dwim_master && > + git remote add repo_upstream ../repo_upstream && > + git config remote.repo_upstream.fetch \ > + "refs/heads/*:refs/remotes/repo_upstream/*" && > + git fetch --all && > + git worktree add ../foo foo > + ) && > + ( > + cd foo && > + test_branch_upstream foo repo_upstream foo && > + git rev-parse repo_upstream/foo >expect && > + git rev-parse foo >actual && > + test_cmp expect actual > + ) > +' > + > test_done > -- > 2.15.0.345.gf926f18f3d > ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 3/3] worktree: make add <path> dwim 2017-11-18 18:11 ` [PATCH v2 1/3] " Thomas Gummerer 2017-11-18 18:11 ` [PATCH v2 2/3] worktree: make add <path> <branch> dwim Thomas Gummerer @ 2017-11-18 18:11 ` Thomas Gummerer 2017-11-18 22:47 ` [PATCH v3 0/3] make git worktree add dwim more Thomas Gummerer 2 siblings, 0 replies; 28+ messages in thread From: Thomas Gummerer @ 2017-11-18 18:11 UTC (permalink / raw) To: git Cc: Nguyễn Thái Ngọc Duy, Junio C Hamano, Eric Sunshine, Thomas Gummerer Currently 'git worktree add <path>' creates a new branch named after the basename of the <path>, that matches the HEAD of whichever worktree we were on when calling "git worktree add <path>". Make 'git worktree add <path> behave more like the dwim machinery in 'git checkout <new-branch>', i.e. check if the new branch name uniquely matches the branch name of a remote tracking branch, and if so check out that branch and set the upstream to the remote tracking branch. This is a change of behaviour compared to the current behaviour, where we create a new branch matching HEAD. However as 'git worktree' is still an experimental feature, and it's easy to notice/correct the behaviour in case it's not what the user desired it's probably okay to break existing behaviour here. In order to also satisfy users who want the current behaviour of creating a new branch from HEAD, add a '--no-track' flag, which disables the new behaviour, and keeps the old behaviour of creating a new branch from the head of the current worktree. Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- I went back and forth between hiding this behing a flag, and making it default and having a flag for getting the old behaviour back. In the end I went for making the new behaviour the default, because the 'worktree' feature is still experimental, and it's easy to correct the behaviour if it's not what was desired. I also think this is the more intuitve behaviour, but clearly I'm biased because *I* want to it to behave this way. I'm happy to keep the old behaviour the default and hide the new behaviour behind a flag if we feel this is too much of a change in behaviour at this point. Documentation/git-worktree.txt | 15 ++++++++--- builtin/worktree.c | 9 +++++++ t/t2025-worktree-add.sh | 60 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 3 deletions(-) diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index 3c7c8c3cee..11cac104d9 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -60,9 +60,18 @@ $ git worktree add -b <branch> <path> <remote>/<branch> ------------ + If `<branch>` 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, if there exists a tracking branch in exactly +one remote (call it <remote>) matching the basename of the path +(call it <branch>), treat it as equivalent to +------------ +$ git worktree add -b <branch> <path> <remote>/<branch> +------------ +If no tracking branch exists in exactly one remote, <branch> is +created based on HEAD, as if `-b $(basename <path>)` was specified. ++ +To disable the behaviour of trying to match the basename of <path> to +a remote, and always create a new branch from HEAD, the `--no-track` +flag can be passed to `git worktree add`. list:: List details of each worktree. The main worktree is listed first, followed by diff --git a/builtin/worktree.c b/builtin/worktree.c index 92b583ae39..82088415b8 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -342,6 +342,7 @@ static int add(int ac, const char **av, const char *prefix) const char *new_branch_force = NULL; char *path; const char *branch; + int track_dwim = 1; 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"), @@ -351,6 +352,7 @@ static int add(int ac, const char **av, const char *prefix) OPT_BOOL(0, "detach", &opts.detach, N_("detach HEAD at named commit")), OPT_BOOL(0, "checkout", &opts.checkout, N_("populate the new working tree")), OPT_BOOL(0, "lock", &opts.keep_locked, N_("keep the new working tree locked")), + OPT_BOOL(0, "track", &track_dwim, N_("checkout upstream branch if there's a unique match")), OPT_END() }; @@ -385,6 +387,13 @@ static int add(int ac, const char **av, const char *prefix) int n; const char *s = worktree_basename(path, &n); opts.new_branch = xstrndup(s, n); + if (track_dwim) { + struct object_id oid; + const char *remote = + unique_tracking_name(opts.new_branch, &oid); + if (remote) + branch = remote; + } } if (ac == 2) { diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh index e5959800c0..a566d867fe 100755 --- a/t/t2025-worktree-add.sh +++ b/t/t2025-worktree-add.sh @@ -358,4 +358,64 @@ test_expect_success '"add" <path> <branch> dwims' ' ) ' +test_expect_success 'git worktree add --no-track does not set up tracking' ' + test_when_finished rm -rf repo_a && + test_when_finished rm -rf repo_b && + test_when_finished rm -rf foo && + git init repo_a && + ( + cd repo_a && + test_commit a_master && + git checkout -b foo && + test_commit a_foo + ) && + git init repo_b && + ( + cd repo_b && + test_commit b_master && + git remote add repo_a ../repo_a && + git config remote.repo_a.fetch \ + "+refs/heads/*:refs/remotes/other_a/*" && + git fetch --all && + git worktree add --no-track ../foo + ) && + ( + cd foo && + ! test_branch_upstream foo repo_a foo && + git rev-parse other_a/foo >expect && + git rev-parse foo >actual && + ! test_cmp expect actual + ) +' + +test_expect_success 'git worktree add sets up tracking' ' + test_when_finished rm -rf repo_a && + test_when_finished rm -rf repo_b && + test_when_finished rm -rf foo && + git init repo_a && + ( + cd repo_a && + test_commit a_master && + git checkout -b foo && + test_commit a_foo + ) && + git init repo_b && + ( + cd repo_b && + test_commit b_master && + git remote add repo_a ../repo_a && + git config remote.repo_a.fetch \ + "+refs/heads/*:refs/remotes/other_a/*" && + git fetch --all && + git worktree add ../foo + ) && + ( + cd foo && + test_branch_upstream foo repo_a foo && + git rev-parse other_a/foo >expect && + git rev-parse foo >actual && + test_cmp expect actual + ) +' + test_done -- 2.15.0.345.gf926f18f3d ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 0/3] make git worktree add dwim more 2017-11-18 18:11 ` [PATCH v2 1/3] " Thomas Gummerer 2017-11-18 18:11 ` [PATCH v2 2/3] worktree: make add <path> <branch> dwim Thomas Gummerer 2017-11-18 18:11 ` [PATCH v2 3/3] worktree: make add <path> dwim Thomas Gummerer @ 2017-11-18 22:47 ` Thomas Gummerer 2017-11-18 22:47 ` [PATCH v3 1/3] checkout: factor out functions to new lib file Thomas Gummerer ` (2 more replies) 2 siblings, 3 replies; 28+ messages in thread From: Thomas Gummerer @ 2017-11-18 22:47 UTC (permalink / raw) To: git Cc: Nguyễn Thái Ngọc Duy, Junio C Hamano, Eric Sunshine, Thomas Gummerer Sorry about the noise with v2 and v3 so quickly one after another. I only too late realized that the new dwim for 'add <path> <branch>' doesn't make much sense if -b or --detach are given, and it's better to keep on erroring out in these cases. The previous rounds were at https://public-inbox.org/git/20171112134305.3949-1-t.gummerer@gmail.com/ and https://public-inbox.org/git/20171118181103.28354-1-t.gummerer@gmail.com/. In case anybody already started reading v2, an interdiff between the two versions is below: diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index 11cac104d9..eedead2c4c 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -52,9 +52,9 @@ is linked to the current repository, sharing everything except working directory specific files such as HEAD, index, etc. `-` may also be specified as `<branch>`; it is synonymous with `@{-1}`. + -If <branch> is not found but there does exist a tracking branch in -exactly one remote (call it <remote>) with a matching name, treat as -equivalent to +If <branch> is not found, and neither `-b` nor `-B` nor `--detach` are +used, but there does exist a tracking branch in exactly one remote +(call it <remote>) with a matching name, treat as equivalent to ------------ $ git worktree add -b <branch> <path> <remote>/<branch> ------------ diff --git a/builtin/worktree.c b/builtin/worktree.c index 82088415b8..b2a6dd020c 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -396,7 +396,7 @@ static int add(int ac, const char **av, const char *prefix) } } - if (ac == 2) { + if (ac == 2 && !opts.new_branch && !opts.detach) { struct object_id oid; struct commit *commit; const char *remote; diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh index a566d867fe..87e233f812 100755 --- a/t/t2025-worktree-add.sh +++ b/t/t2025-worktree-add.sh @@ -347,6 +347,8 @@ test_expect_success '"add" <path> <branch> dwims' ' git config remote.repo_upstream.fetch \ "refs/heads/*:refs/remotes/repo_upstream/*" && git fetch --all && + test_must_fail git worktree add -b foo ../foo foo && + test_must_fail git worktree add --detach ../foo foo && git worktree add ../foo foo ) && ( Thomas Gummerer (3): checkout: factor out functions to new lib file worktree: make add <path> <branch> dwim worktree: make add <path> dwim Documentation/git-worktree.txt | 22 +++++++-- Makefile | 1 + builtin/checkout.c | 41 +--------------- builtin/worktree.c | 24 ++++++++++ checkout.c | 42 ++++++++++++++++ checkout.h | 13 +++++ t/t2025-worktree-add.sh | 106 +++++++++++++++++++++++++++++++++++++++++ 7 files changed, 206 insertions(+), 43 deletions(-) create mode 100644 checkout.c create mode 100644 checkout.h -- 2.15.0.345.gf926f18f3 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 1/3] checkout: factor out functions to new lib file 2017-11-18 22:47 ` [PATCH v3 0/3] make git worktree add dwim more Thomas Gummerer @ 2017-11-18 22:47 ` Thomas Gummerer 2017-11-18 22:47 ` [PATCH v3 2/3] worktree: make add <path> <branch> dwim Thomas Gummerer 2017-11-18 22:47 ` [PATCH v3 3/3] worktree: make add <path> dwim Thomas Gummerer 2 siblings, 0 replies; 28+ messages in thread From: Thomas Gummerer @ 2017-11-18 22:47 UTC (permalink / raw) To: git Cc: Nguyễn Thái Ngọc Duy, Junio C Hamano, Eric Sunshine, Thomas Gummerer Factor the functions out, so they can be re-used from other places. In particular these functions will be re-used in builtin/worktree.c to make git worktree add dwim more. While there add some docs to the function. Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- Makefile | 1 + builtin/checkout.c | 41 +---------------------------------------- checkout.c | 42 ++++++++++++++++++++++++++++++++++++++++++ checkout.h | 13 +++++++++++++ 4 files changed, 57 insertions(+), 40 deletions(-) create mode 100644 checkout.c create mode 100644 checkout.h diff --git a/Makefile b/Makefile index cd75985991..8d603c7443 100644 --- a/Makefile +++ b/Makefile @@ -757,6 +757,7 @@ LIB_OBJS += branch.o LIB_OBJS += bulk-checkin.o LIB_OBJS += bundle.o LIB_OBJS += cache-tree.o +LIB_OBJS += checkout.o LIB_OBJS += color.o LIB_OBJS += column.o LIB_OBJS += combine-diff.o diff --git a/builtin/checkout.c b/builtin/checkout.c index fc4f8fd2ea..9e1cfd10b3 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1,5 +1,6 @@ #include "builtin.h" #include "config.h" +#include "checkout.h" #include "lockfile.h" #include "parse-options.h" #include "refs.h" @@ -874,46 +875,6 @@ static int git_checkout_config(const char *var, const char *value, void *cb) return git_xmerge_config(var, value, NULL); } -struct tracking_name_data { - /* const */ char *src_ref; - char *dst_ref; - struct object_id *dst_oid; - int unique; -}; - -static int check_tracking_name(struct remote *remote, void *cb_data) -{ - struct tracking_name_data *cb = cb_data; - struct refspec query; - memset(&query, 0, sizeof(struct refspec)); - query.src = cb->src_ref; - if (remote_find_tracking(remote, &query) || - get_oid(query.dst, cb->dst_oid)) { - free(query.dst); - return 0; - } - if (cb->dst_ref) { - free(query.dst); - cb->unique = 0; - return 0; - } - cb->dst_ref = query.dst; - return 0; -} - -static const char *unique_tracking_name(const char *name, struct object_id *oid) -{ - struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 }; - cb_data.src_ref = xstrfmt("refs/heads/%s", name); - cb_data.dst_oid = oid; - for_each_remote(check_tracking_name, &cb_data); - free(cb_data.src_ref); - if (cb_data.unique) - return cb_data.dst_ref; - free(cb_data.dst_ref); - return NULL; -} - static int parse_branchname_arg(int argc, const char **argv, int dwim_new_local_branch_ok, struct branch_info *new, diff --git a/checkout.c b/checkout.c new file mode 100644 index 0000000000..b0c744d37a --- /dev/null +++ b/checkout.c @@ -0,0 +1,42 @@ +#include "cache.h" +#include "remote.h" + +struct tracking_name_data { + /* const */ char *src_ref; + char *dst_ref; + struct object_id *dst_oid; + int unique; +}; + +static int check_tracking_name(struct remote *remote, void *cb_data) +{ + struct tracking_name_data *cb = cb_data; + struct refspec query; + memset(&query, 0, sizeof(struct refspec)); + query.src = cb->src_ref; + if (remote_find_tracking(remote, &query) || + get_oid(query.dst, cb->dst_oid)) { + free(query.dst); + return 0; + } + if (cb->dst_ref) { + free(query.dst); + cb->unique = 0; + return 0; + } + cb->dst_ref = query.dst; + return 0; +} + +const char *unique_tracking_name(const char *name, struct object_id *oid) +{ + struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 }; + cb_data.src_ref = xstrfmt("refs/heads/%s", name); + cb_data.dst_oid = oid; + for_each_remote(check_tracking_name, &cb_data); + free(cb_data.src_ref); + if (cb_data.unique) + return cb_data.dst_ref; + free(cb_data.dst_ref); + return NULL; +} diff --git a/checkout.h b/checkout.h new file mode 100644 index 0000000000..9980711179 --- /dev/null +++ b/checkout.h @@ -0,0 +1,13 @@ +#ifndef CHECKOUT_H +#define CHECKOUT_H + +#include "cache.h" + +/* + * Check if the branch name uniquely matches a branch name on a remote + * tracking branch. Return the name of the remote if such a branch + * exists, NULL otherwise. + */ +extern const char *unique_tracking_name(const char *name, struct object_id *oid); + +#endif /* CHECKOUT_H */ -- 2.15.0.345.gf926f18f3 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 2/3] worktree: make add <path> <branch> dwim 2017-11-18 22:47 ` [PATCH v3 0/3] make git worktree add dwim more Thomas Gummerer 2017-11-18 22:47 ` [PATCH v3 1/3] checkout: factor out functions to new lib file Thomas Gummerer @ 2017-11-18 22:47 ` Thomas Gummerer 2017-11-19 8:31 ` Eric Sunshine 2017-11-18 22:47 ` [PATCH v3 3/3] worktree: make add <path> dwim Thomas Gummerer 2 siblings, 1 reply; 28+ messages in thread From: Thomas Gummerer @ 2017-11-18 22:47 UTC (permalink / raw) To: git Cc: Nguyễn Thái Ngọc Duy, Junio C Hamano, Eric Sunshine, Thomas Gummerer Currently 'git worktree add <path> <branch>', errors out when 'branch' is not a local branch. It has no additional dwim'ing features that one might expect. Make it behave more like 'git checkout <branch>' when the branch doesn't exist locally, but a remote tracking branch uniquely matches the desired branch name, i.e. create a new branch from the remote tracking branch and set the upstream to the remote tracking branch. As 'git worktree add' currently just dies in this situation, there are no backwards compatibility worries when introducing this feature. Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- Documentation/git-worktree.txt | 7 +++++++ builtin/worktree.c | 15 ++++++++++++++ t/t2025-worktree-add.sh | 46 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+) diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index b472acc356..abf48fecb8 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -52,6 +52,13 @@ is linked to the current repository, sharing everything except working directory specific files such as HEAD, index, etc. `-` may also be specified as `<branch>`; it is synonymous with `@{-1}`. + +If <branch> is not found, and neither `-b` nor `-B` nor `--detach` are +used, but there does exist a tracking branch in exactly one remote +(call it <remote>) with a matching name, treat as equivalent to +------------ +$ git worktree add -b <branch> <path> <remote>/<branch> +------------ ++ If `<branch>` 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. diff --git a/builtin/worktree.c b/builtin/worktree.c index 7b9307aa58..05fc902bcc 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "checkout.h" #include "config.h" #include "builtin.h" #include "dir.h" @@ -386,6 +387,20 @@ static int add(int ac, const char **av, const char *prefix) opts.new_branch = xstrndup(s, n); } + if (ac == 2 && !opts.new_branch && !opts.detach) { + struct object_id oid; + struct commit *commit; + const char *remote; + + commit = lookup_commit_reference_by_name(branch); + if (!commit) + remote = unique_tracking_name(branch, &oid); + if (!commit && remote) { + opts.new_branch = branch; + branch = remote; + } + } + if (opts.new_branch) { struct child_process cp = CHILD_PROCESS_INIT; cp.git_cmd = 1; diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh index b5c47ac602..e3fc60dd1c 100755 --- a/t/t2025-worktree-add.sh +++ b/t/t2025-worktree-add.sh @@ -6,6 +6,16 @@ test_description='test git worktree add' . "$TEST_DIRECTORY"/lib-rebase.sh +# Is branch "refs/heads/$1" set to pull from "$2/$3"? +test_branch_upstream () { + printf "%s\n" "$2" "refs/heads/$3" >expect.upstream && + { + git config "branch.$1.remote" && + git config "branch.$1.merge" + } >actual.upstream && + test_cmp expect.upstream actual.upstream +} + test_expect_success 'setup' ' test_commit init ' @@ -314,4 +324,40 @@ test_expect_success 'rename a branch under bisect not allowed' ' test_must_fail git branch -M under-bisect bisect-with-new-name ' +test_expect_success '"add" <path> <non-existent-branch> fails' ' + test_must_fail git worktree add foo non-existent +' + +test_expect_success '"add" <path> <branch> dwims' ' + test_when_finished rm -rf repo_upstream && + test_when_finished rm -rf repo_dwim && + test_when_finished rm -rf foo && + git init repo_upstream && + ( + cd repo_upstream && + test_commit upstream_master && + git checkout -b foo && + test_commit a_foo + ) && + git init repo_dwim && + ( + cd repo_dwim && + test_commit dwim_master && + git remote add repo_upstream ../repo_upstream && + git config remote.repo_upstream.fetch \ + "refs/heads/*:refs/remotes/repo_upstream/*" && + git fetch --all && + test_must_fail git worktree add -b foo ../foo foo && + test_must_fail git worktree add --detach ../foo foo && + git worktree add ../foo foo + ) && + ( + cd foo && + test_branch_upstream foo repo_upstream foo && + git rev-parse repo_upstream/foo >expect && + git rev-parse foo >actual && + test_cmp expect actual + ) +' + test_done -- 2.15.0.345.gf926f18f3 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/3] worktree: make add <path> <branch> dwim 2017-11-18 22:47 ` [PATCH v3 2/3] worktree: make add <path> <branch> dwim Thomas Gummerer @ 2017-11-19 8:31 ` Eric Sunshine 2017-11-19 17:43 ` Thomas Gummerer 0 siblings, 1 reply; 28+ messages in thread From: Eric Sunshine @ 2017-11-19 8:31 UTC (permalink / raw) To: Thomas Gummerer Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano On Sat, Nov 18, 2017 at 5:47 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote: > Currently 'git worktree add <path> <branch>', errors out when 'branch' > is not a local branch. It has no additional dwim'ing features that one > might expect. > > Make it behave more like 'git checkout <branch>' when the branch doesn't > exist locally, but a remote tracking branch uniquely matches the desired > branch name, i.e. create a new branch from the remote tracking branch > and set the upstream to the remote tracking branch. > > As 'git worktree add' currently just dies in this situation, there are > no backwards compatibility worries when introducing this feature. > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > --- > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > @@ -52,6 +52,13 @@ is linked to the current repository, sharing everything except working > +If <branch> is not found, and neither `-b` nor `-B` nor `--detach` are > +used, but there does exist a tracking branch in exactly one remote > +(call it <remote>) with a matching name, treat as equivalent to > +------------ > +$ git worktree add -b <branch> <path> <remote>/<branch> > +------------ The example from which this was copied in git-checkout.txt shows the --track option being used, which makes it clear that the new local branch will track the remote-tracking branch. git-worktree, of course, does not have a --track option, but would it make sense to mention explicitly in the prose that the newly-created local branch tracks the remote one? (Or are readers sufficiently savvy to intuit it?) > diff --git a/builtin/worktree.c b/builtin/worktree.c > index 7b9307aa58..05fc902bcc 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -1,4 +1,5 @@ > #include "cache.h" > +#include "checkout.h" > #include "config.h" > #include "builtin.h" > #include "dir.h" > @@ -386,6 +387,20 @@ static int add(int ac, const char **av, const char *prefix) > opts.new_branch = xstrndup(s, n); > } > > + if (ac == 2 && !opts.new_branch && !opts.detach) { > + struct object_id oid; > + struct commit *commit; > + const char *remote; > + > + commit = lookup_commit_reference_by_name(branch); > + if (!commit) > + remote = unique_tracking_name(branch, &oid); > + if (!commit && remote) { > + opts.new_branch = branch; > + branch = remote; > + } > + } You can simplify the above conditionals to: commit = ... if (!commit) { remote = ... if (remote) { ... } } So, although you're not passing --track explicitly to the "git branch" invocation just below, you get --track for free since it's the default behavior when creating a new local branch from a remote one, correct? (Just checking my understanding.) > if (opts.new_branch) { > struct child_process cp = CHILD_PROCESS_INIT; > cp.git_cmd = 1; > diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh > @@ -6,6 +6,16 @@ test_description='test git worktree add' > +# Is branch "refs/heads/$1" set to pull from "$2/$3"? > +test_branch_upstream () { > + printf "%s\n" "$2" "refs/heads/$3" >expect.upstream && > + { > + git config "branch.$1.remote" && > + git config "branch.$1.merge" > + } >actual.upstream && > + test_cmp expect.upstream actual.upstream > +} Not a big deal, but it wouldn't hurt to move this function down to the point where it is first needed... > test_expect_success 'setup' ' > test_commit init > ' > @@ -314,4 +324,40 @@ test_expect_success 'rename a branch under bisect not allowed' ' > +test_expect_success '"add" <path> <non-existent-branch> fails' ' > + test_must_fail git worktree add foo non-existent > +' > + > +test_expect_success '"add" <path> <branch> dwims' ' > + test_when_finished rm -rf repo_upstream && > + test_when_finished rm -rf repo_dwim && > + test_when_finished rm -rf foo && Also not a big deal, but could all be combined into a single invocation: test_when_finished rm -rf repo_upstream repo_dwim foo && > + git init repo_upstream && > + ( > + cd repo_upstream && > + test_commit upstream_master && > + git checkout -b foo && > + test_commit a_foo > + ) && > + git init repo_dwim && > + ( > + cd repo_dwim && > + test_commit dwim_master && > + git remote add repo_upstream ../repo_upstream && > + git config remote.repo_upstream.fetch \ > + "refs/heads/*:refs/remotes/repo_upstream/*" && > + git fetch --all && > + test_must_fail git worktree add -b foo ../foo foo && > + test_must_fail git worktree add --detach ../foo foo && > + git worktree add ../foo foo > + ) && > + ( > + cd foo && > + test_branch_upstream foo repo_upstream foo && > + git rev-parse repo_upstream/foo >expect && > + git rev-parse foo >actual && > + test_cmp expect actual > + ) > +' ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/3] worktree: make add <path> <branch> dwim 2017-11-19 8:31 ` Eric Sunshine @ 2017-11-19 17:43 ` Thomas Gummerer 0 siblings, 0 replies; 28+ messages in thread From: Thomas Gummerer @ 2017-11-19 17:43 UTC (permalink / raw) To: Eric Sunshine Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano On 11/19, Eric Sunshine wrote: > On Sat, Nov 18, 2017 at 5:47 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote: > > Currently 'git worktree add <path> <branch>', errors out when 'branch' > > is not a local branch. It has no additional dwim'ing features that one > > might expect. > > > > Make it behave more like 'git checkout <branch>' when the branch doesn't > > exist locally, but a remote tracking branch uniquely matches the desired > > branch name, i.e. create a new branch from the remote tracking branch > > and set the upstream to the remote tracking branch. > > > > As 'git worktree add' currently just dies in this situation, there are > > no backwards compatibility worries when introducing this feature. > > > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > > --- > > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > > @@ -52,6 +52,13 @@ is linked to the current repository, sharing everything except working > > +If <branch> is not found, and neither `-b` nor `-B` nor `--detach` are > > +used, but there does exist a tracking branch in exactly one remote > > +(call it <remote>) with a matching name, treat as equivalent to > > +------------ > > +$ git worktree add -b <branch> <path> <remote>/<branch> > > +------------ > > The example from which this was copied in git-checkout.txt shows the > --track option being used, which makes it clear that the new local > branch will track the remote-tracking branch. git-worktree, of course, > does not have a --track option, but would it make sense to mention > explicitly in the prose that the newly-created local branch tracks the > remote one? (Or are readers sufficiently savvy to intuit it?) It is how 'git worktree add -b <branch> <path> <remote>/<branch>' currently works, which is also not documented anywhere. However I'm not sure it's super intuitive, from just reading the command, so I'll add an explicit mention about it. > > diff --git a/builtin/worktree.c b/builtin/worktree.c > > index 7b9307aa58..05fc902bcc 100644 > > --- a/builtin/worktree.c > > +++ b/builtin/worktree.c > > @@ -1,4 +1,5 @@ > > #include "cache.h" > > +#include "checkout.h" > > #include "config.h" > > #include "builtin.h" > > #include "dir.h" > > @@ -386,6 +387,20 @@ static int add(int ac, const char **av, const char *prefix) > > opts.new_branch = xstrndup(s, n); > > } > > > > + if (ac == 2 && !opts.new_branch && !opts.detach) { > > + struct object_id oid; > > + struct commit *commit; > > + const char *remote; > > + > > + commit = lookup_commit_reference_by_name(branch); > > + if (!commit) > > + remote = unique_tracking_name(branch, &oid); > > + if (!commit && remote) { > > + opts.new_branch = branch; > > + branch = remote; > > + } > > + } > > You can simplify the above conditionals to: > > commit = ... > if (!commit) { > remote = ... > if (remote) { > ... > } > } Will change, thanks! > So, although you're not passing --track explicitly to the "git branch" > invocation just below, you get --track for free since it's the default > behavior when creating a new local branch from a remote one, correct? > (Just checking my understanding.) Yes that's correct. The default behaviour of git branch does exactly what we want here, so we're relying on it, instead of gouing through the trouble of explicitly specifying --track. We have a test checking the expected behaviour of setting up the upstream, so in the unlikely event that the behaviour in 'git branch' changes, we'd still guard against it there. Therefore I don't think there's a need to be extra defensive here. > > if (opts.new_branch) { > > struct child_process cp = CHILD_PROCESS_INIT; > > cp.git_cmd = 1; > > diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh > > @@ -6,6 +6,16 @@ test_description='test git worktree add' > > +# Is branch "refs/heads/$1" set to pull from "$2/$3"? > > +test_branch_upstream () { > > + printf "%s\n" "$2" "refs/heads/$3" >expect.upstream && > > + { > > + git config "branch.$1.remote" && > > + git config "branch.$1.merge" > > + } >actual.upstream && > > + test_cmp expect.upstream actual.upstream > > +} > > Not a big deal, but it wouldn't hurt to move this function down to the > point where it is first needed... Will do. > > test_expect_success 'setup' ' > > test_commit init > > ' > > @@ -314,4 +324,40 @@ test_expect_success 'rename a branch under bisect not allowed' ' > > +test_expect_success '"add" <path> <non-existent-branch> fails' ' > > + test_must_fail git worktree add foo non-existent > > +' > > + > > +test_expect_success '"add" <path> <branch> dwims' ' > > + test_when_finished rm -rf repo_upstream && > > + test_when_finished rm -rf repo_dwim && > > + test_when_finished rm -rf foo && > > Also not a big deal, but could all be combined into a single invocation: > > test_when_finished rm -rf repo_upstream repo_dwim foo && Thanks, will change! Thanks a lot for the review! > > + git init repo_upstream && > > + ( > > + cd repo_upstream && > > + test_commit upstream_master && > > + git checkout -b foo && > > + test_commit a_foo > > + ) && > > + git init repo_dwim && > > + ( > > + cd repo_dwim && > > + test_commit dwim_master && > > + git remote add repo_upstream ../repo_upstream && > > + git config remote.repo_upstream.fetch \ > > + "refs/heads/*:refs/remotes/repo_upstream/*" && > > + git fetch --all && > > + test_must_fail git worktree add -b foo ../foo foo && > > + test_must_fail git worktree add --detach ../foo foo && > > + git worktree add ../foo foo > > + ) && > > + ( > > + cd foo && > > + test_branch_upstream foo repo_upstream foo && > > + git rev-parse repo_upstream/foo >expect && > > + git rev-parse foo >actual && > > + test_cmp expect actual > > + ) > > +' ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 3/3] worktree: make add <path> dwim 2017-11-18 22:47 ` [PATCH v3 0/3] make git worktree add dwim more Thomas Gummerer 2017-11-18 22:47 ` [PATCH v3 1/3] checkout: factor out functions to new lib file Thomas Gummerer 2017-11-18 22:47 ` [PATCH v3 2/3] worktree: make add <path> <branch> dwim Thomas Gummerer @ 2017-11-18 22:47 ` Thomas Gummerer 2017-11-19 19:04 ` Eric Sunshine 2 siblings, 1 reply; 28+ messages in thread From: Thomas Gummerer @ 2017-11-18 22:47 UTC (permalink / raw) To: git Cc: Nguyễn Thái Ngọc Duy, Junio C Hamano, Eric Sunshine, Thomas Gummerer Currently 'git worktree add <path>' creates a new branch named after the basename of the <path>, that matches the HEAD of whichever worktree we were on when calling "git worktree add <path>". Make 'git worktree add <path> behave more like the dwim machinery in 'git checkout <new-branch>', i.e. check if the new branch name uniquely matches the branch name of a remote tracking branch, and if so check out that branch and set the upstream to the remote tracking branch. This is a change of behaviour compared to the current behaviour, where we create a new branch matching HEAD. However as 'git worktree' is still an experimental feature, and it's easy to notice/correct the behaviour in case it's not what the user desired it's probably okay to break existing behaviour here. In order to also satisfy users who want the current behaviour of creating a new branch from HEAD, add a '--no-track' flag, which disables the new behaviour, and keeps the old behaviour of creating a new branch from the head of the current worktree. Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- Documentation/git-worktree.txt | 15 ++++++++--- builtin/worktree.c | 9 +++++++ t/t2025-worktree-add.sh | 60 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 3 deletions(-) diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index abf48fecb8..eedead2c4c 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -60,9 +60,18 @@ $ git worktree add -b <branch> <path> <remote>/<branch> ------------ + If `<branch>` 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, if there exists a tracking branch in exactly +one remote (call it <remote>) matching the basename of the path +(call it <branch>), treat it as equivalent to +------------ +$ git worktree add -b <branch> <path> <remote>/<branch> +------------ +If no tracking branch exists in exactly one remote, <branch> is +created based on HEAD, as if `-b $(basename <path>)` was specified. ++ +To disable the behaviour of trying to match the basename of <path> to +a remote, and always create a new branch from HEAD, the `--no-track` +flag can be passed to `git worktree add`. list:: List details of each worktree. The main worktree is listed first, followed by diff --git a/builtin/worktree.c b/builtin/worktree.c index 05fc902bcc..b2a6dd020c 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -342,6 +342,7 @@ static int add(int ac, const char **av, const char *prefix) const char *new_branch_force = NULL; char *path; const char *branch; + int track_dwim = 1; 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"), @@ -351,6 +352,7 @@ static int add(int ac, const char **av, const char *prefix) OPT_BOOL(0, "detach", &opts.detach, N_("detach HEAD at named commit")), OPT_BOOL(0, "checkout", &opts.checkout, N_("populate the new working tree")), OPT_BOOL(0, "lock", &opts.keep_locked, N_("keep the new working tree locked")), + OPT_BOOL(0, "track", &track_dwim, N_("checkout upstream branch if there's a unique match")), OPT_END() }; @@ -385,6 +387,13 @@ static int add(int ac, const char **av, const char *prefix) int n; const char *s = worktree_basename(path, &n); opts.new_branch = xstrndup(s, n); + if (track_dwim) { + struct object_id oid; + const char *remote = + unique_tracking_name(opts.new_branch, &oid); + if (remote) + branch = remote; + } } if (ac == 2 && !opts.new_branch && !opts.detach) { diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh index e3fc60dd1c..87e233f812 100755 --- a/t/t2025-worktree-add.sh +++ b/t/t2025-worktree-add.sh @@ -360,4 +360,64 @@ test_expect_success '"add" <path> <branch> dwims' ' ) ' +test_expect_success 'git worktree add --no-track does not set up tracking' ' + test_when_finished rm -rf repo_a && + test_when_finished rm -rf repo_b && + test_when_finished rm -rf foo && + git init repo_a && + ( + cd repo_a && + test_commit a_master && + git checkout -b foo && + test_commit a_foo + ) && + git init repo_b && + ( + cd repo_b && + test_commit b_master && + git remote add repo_a ../repo_a && + git config remote.repo_a.fetch \ + "+refs/heads/*:refs/remotes/other_a/*" && + git fetch --all && + git worktree add --no-track ../foo + ) && + ( + cd foo && + ! test_branch_upstream foo repo_a foo && + git rev-parse other_a/foo >expect && + git rev-parse foo >actual && + ! test_cmp expect actual + ) +' + +test_expect_success 'git worktree add sets up tracking' ' + test_when_finished rm -rf repo_a && + test_when_finished rm -rf repo_b && + test_when_finished rm -rf foo && + git init repo_a && + ( + cd repo_a && + test_commit a_master && + git checkout -b foo && + test_commit a_foo + ) && + git init repo_b && + ( + cd repo_b && + test_commit b_master && + git remote add repo_a ../repo_a && + git config remote.repo_a.fetch \ + "+refs/heads/*:refs/remotes/other_a/*" && + git fetch --all && + git worktree add ../foo + ) && + ( + cd foo && + test_branch_upstream foo repo_a foo && + git rev-parse other_a/foo >expect && + git rev-parse foo >actual && + test_cmp expect actual + ) +' + test_done -- 2.15.0.345.gf926f18f3 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v3 3/3] worktree: make add <path> dwim 2017-11-18 22:47 ` [PATCH v3 3/3] worktree: make add <path> dwim Thomas Gummerer @ 2017-11-19 19:04 ` Eric Sunshine 2017-11-19 20:20 ` Eric Sunshine 0 siblings, 1 reply; 28+ messages in thread From: Eric Sunshine @ 2017-11-19 19:04 UTC (permalink / raw) To: Thomas Gummerer Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano On Sat, Nov 18, 2017 at 5:47 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote: > Currently 'git worktree add <path>' creates a new branch named after the > basename of the <path>, that matches the HEAD of whichever worktree we > were on when calling "git worktree add <path>". > > Make 'git worktree add <path> behave more like the dwim machinery in > 'git checkout <new-branch>', i.e. check if the new branch name uniquely > matches the branch name of a remote tracking branch, and if so check out > that branch and set the upstream to the remote tracking branch. > > This is a change of behaviour compared to the current behaviour, where > we create a new branch matching HEAD. However as 'git worktree' is > still an experimental feature, and it's easy to notice/correct the > behaviour in case it's not what the user desired it's probably okay to > break existing behaviour here. > > In order to also satisfy users who want the current behaviour of > creating a new branch from HEAD, add a '--no-track' flag, which disables > the new behaviour, and keeps the old behaviour of creating a new branch > from the head of the current worktree. > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > --- > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > @@ -60,9 +60,18 @@ $ git worktree add -b <branch> <path> <remote>/<branch> > ------------ > If `<branch>` 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, if there exists a tracking branch in exactly > +one remote (call it <remote>) matching the basename of the path > +(call it <branch>), treat it as equivalent to Inconsistent typesetting. In the context, typesetting is `<foo>`, whereas in the new content, you've used <foo> for these two. > +------------ > +$ git worktree add -b <branch> <path> <remote>/<branch> > +------------ > +If no tracking branch exists in exactly one remote, <branch> is Typesetting: `<foo>` > +created based on HEAD, as if `-b $(basename <path>)` was specified. > ++ > +To disable the behaviour of trying to match the basename of <path> to > +a remote, and always create a new branch from HEAD, the `--no-track` Does --[no-]track deserve to be documented in the OPTIONS section like the other options are? > +flag can be passed to `git worktree add`. > list:: You lost a blank line before "list::". > List details of each worktree. The main worktree is listed first, followed by > diff --git a/builtin/worktree.c b/builtin/worktree.c > @@ -351,6 +352,7 @@ static int add(int ac, const char **av, const char *prefix) > + OPT_BOOL(0, "track", &track_dwim, N_("checkout upstream branch if there's a unique match")), git-checkout and git-branch help describe this as "setting upstream" and "setting up tracking", respectively. Using "checkout" in this help message could be confusing, especially since git-worktree has a --no-checkout option; this seems to imply that --track would override it. > OPT_END() > }; > > diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh > @@ -360,4 +360,64 @@ test_expect_success '"add" <path> <branch> dwims' ' > +test_expect_success 'git worktree add --no-track does not set up tracking' ' > + test_when_finished rm -rf repo_a && > + test_when_finished rm -rf repo_b && > + test_when_finished rm -rf foo && > + git init repo_a && > + ( > + cd repo_a && > + test_commit a_master && > + git checkout -b foo && > + test_commit a_foo > + ) && > + git init repo_b && > + ( > + cd repo_b && > + test_commit b_master && > + git remote add repo_a ../repo_a && > + git config remote.repo_a.fetch \ > + "+refs/heads/*:refs/remotes/other_a/*" && > + git fetch --all && > + git worktree add --no-track ../foo > + ) && > + ( > + cd foo && > + ! test_branch_upstream foo repo_a foo && > + git rev-parse other_a/foo >expect && > + git rev-parse foo >actual && > + ! test_cmp expect actual > + ) > +' Most of the boilerplate in the three new tests (added in this and the previous patch) is identical (and very verbose). Perhaps the bulk of it can be factored out into a function? > + > +test_expect_success 'git worktree add sets up tracking' ' > + test_when_finished rm -rf repo_a && > + test_when_finished rm -rf repo_b && > + test_when_finished rm -rf foo && > + git init repo_a && > + ( > + cd repo_a && > + test_commit a_master && > + git checkout -b foo && > + test_commit a_foo > + ) && > + git init repo_b && > + ( > + cd repo_b && > + test_commit b_master && > + git remote add repo_a ../repo_a && > + git config remote.repo_a.fetch \ > + "+refs/heads/*:refs/remotes/other_a/*" && > + git fetch --all && > + git worktree add ../foo > + ) && > + ( > + cd foo && > + test_branch_upstream foo repo_a foo && > + git rev-parse other_a/foo >expect && > + git rev-parse foo >actual && > + test_cmp expect actual > + ) > +' > + > test_done ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 3/3] worktree: make add <path> dwim 2017-11-19 19:04 ` Eric Sunshine @ 2017-11-19 20:20 ` Eric Sunshine 2017-11-20 0:39 ` Junio C Hamano 2017-11-21 22:13 ` Thomas Gummerer 0 siblings, 2 replies; 28+ messages in thread From: Eric Sunshine @ 2017-11-19 20:20 UTC (permalink / raw) To: Thomas Gummerer Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano On Sun, Nov 19, 2017 at 2:04 PM, Eric Sunshine <sunshine@sunshineco.com> wrote: > On Sat, Nov 18, 2017 at 5:47 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote: >> +To disable the behaviour of trying to match the basename of <path> to >> +a remote, and always create a new branch from HEAD, the `--no-track` > > Does --[no-]track deserve to be documented in the OPTIONS section like > the other options are? One other question: Since this is re-using the well-known option name --no-track, should it also get applied to the "git worktree add -b foo dir origin/foo" case, as well, which you pointed out (in the patch 2/3 thread) already DWIMs tracking automatically? (I can easily see someone reporting it as a bug if "git worktree add --no-track -b foo dir origin/foo" does not suppress tracking.) ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 3/3] worktree: make add <path> dwim 2017-11-19 20:20 ` Eric Sunshine @ 2017-11-20 0:39 ` Junio C Hamano 2017-11-21 22:13 ` Thomas Gummerer 1 sibling, 0 replies; 28+ messages in thread From: Junio C Hamano @ 2017-11-20 0:39 UTC (permalink / raw) To: Eric Sunshine Cc: Thomas Gummerer, Git List, Nguyễn Thái Ngọc Duy Eric Sunshine <sunshine@sunshineco.com> writes: > On Sun, Nov 19, 2017 at 2:04 PM, Eric Sunshine <sunshine@sunshineco.com> wrote: >> On Sat, Nov 18, 2017 at 5:47 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote: >>> +To disable the behaviour of trying to match the basename of <path> to >>> +a remote, and always create a new branch from HEAD, the `--no-track` >> >> Does --[no-]track deserve to be documented in the OPTIONS section like >> the other options are? > > One other question: Since this is re-using the well-known option name > --no-track, should it also get applied to the "git worktree add -b foo > dir origin/foo" case, as well, which you pointed out (in the patch 2/3 > thread) already DWIMs tracking automatically? (I can easily see > someone reporting it as a bug if "git worktree add --no-track -b foo > dir origin/foo" does not suppress tracking.) Sensible suggestion. Thanks. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 3/3] worktree: make add <path> dwim 2017-11-19 20:20 ` Eric Sunshine 2017-11-20 0:39 ` Junio C Hamano @ 2017-11-21 22:13 ` Thomas Gummerer 2017-11-22 1:20 ` Junio C Hamano 1 sibling, 1 reply; 28+ messages in thread From: Thomas Gummerer @ 2017-11-21 22:13 UTC (permalink / raw) To: Eric Sunshine Cc: Git List, Nguyễn Thái Ngọc Duy, Junio C Hamano On 11/19, Eric Sunshine wrote: > On Sun, Nov 19, 2017 at 2:04 PM, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Sat, Nov 18, 2017 at 5:47 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote: > >> +To disable the behaviour of trying to match the basename of <path> to > >> +a remote, and always create a new branch from HEAD, the `--no-track` > > > > Does --[no-]track deserve to be documented in the OPTIONS section like > > the other options are? > > One other question: Since this is re-using the well-known option name > --no-track, should it also get applied to the "git worktree add -b foo > dir origin/foo" case, as well, which you pointed out (in the patch 2/3 > thread) already DWIMs tracking automatically? (I can easily see > someone reporting it as a bug if "git worktree add --no-track -b foo > dir origin/foo" does not suppress tracking.) I didn't consider that, I think you are right, and the flag should apply in that case as well. I think at that point we may as well pass this flag through to the 'git branch' call, and let users set up tracking if they want to, the same way it works in 'git branch'. Thanks! ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 3/3] worktree: make add <path> dwim 2017-11-21 22:13 ` Thomas Gummerer @ 2017-11-22 1:20 ` Junio C Hamano 2017-11-22 19:49 ` Thomas Gummerer 0 siblings, 1 reply; 28+ messages in thread From: Junio C Hamano @ 2017-11-22 1:20 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: > I didn't consider that, I think you are right, and the flag should > apply in that case as well. I think at that point we may as well pass > this flag through to the 'git branch' call, and let users set up > tracking if they want to, the same way it works in 'git branch'. OK, so tracking is set up by default in the current implementation of "git worktree" (even without your proposed update), but we will stop doing so, and instead take an explicit "--track" option (or "--no-track" to countermand an earlier "--track" on the command line and/or a default configured with branch.autosetupmerge) just like "git branch" does? I think that it is very sensible thing to make sure that "branch", "checkout -b" and "worktree", i.e. the three ways to create a branch to work on (the latter two being short-hands), behave consistently. Thanks. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 3/3] worktree: make add <path> dwim 2017-11-22 1:20 ` Junio C Hamano @ 2017-11-22 19:49 ` Thomas Gummerer 0 siblings, 0 replies; 28+ messages in thread From: Thomas Gummerer @ 2017-11-22 19:49 UTC (permalink / raw) To: Junio C Hamano Cc: Eric Sunshine, Git List, Nguyễn Thái Ngọc Duy On 11/22, Junio C Hamano wrote: > Thomas Gummerer <t.gummerer@gmail.com> writes: > > > I didn't consider that, I think you are right, and the flag should > > apply in that case as well. I think at that point we may as well pass > > this flag through to the 'git branch' call, and let users set up > > tracking if they want to, the same way it works in 'git branch'. > > OK, so tracking is set up by default in the current implementation > of "git worktree" (even without your proposed update), but we will > stop doing so, and instead take an explicit "--track" option (or > "--no-track" to countermand an earlier "--track" on the command line > and/or a default configured with branch.autosetupmerge) just like > "git branch" does? I was a bit brief in the above. The full story is that tracking is set up by default if the '<branch>' given is a remote tracking branch, and isn't set up otherwise, the same way as 'git branch' behaves. What I'm planning to do is introduce a --[no-]track flag to override this behaviour. As 'git worktree' really just calls 'git branch' internally, the branch.autoSetupMerge configuration is also respected. > I think that it is very sensible thing to make sure that "branch", > "checkout -b" and "worktree", i.e. the three ways to create a branch > to work on (the latter two being short-hands), behave consistently. So in summary "branch" and "worktree" already behave consistently, the plan is just to introduce the same "--[no-]track" flag as branch to allow users to override the behaviour the same way as they are allowed in "branch". > Thanks. ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2017-11-22 19:48 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-12 13:43 [PATCH v1 1/2] checkout: factor out functions to new lib file Thomas Gummerer 2017-11-12 13:43 ` [PATCH v1 2/2] worktree: make add dwim Thomas Gummerer 2017-11-13 3:04 ` Junio C Hamano 2017-11-14 8:45 ` Thomas Gummerer 2017-11-14 20:14 ` Eric Sunshine 2017-11-14 20:29 ` Eric Sunshine 2017-11-15 8:52 ` Thomas Gummerer 2017-11-18 18:13 ` Thomas Gummerer 2017-11-15 8:50 ` Thomas Gummerer 2017-11-15 9:12 ` Eric Sunshine 2017-11-13 2:41 ` [PATCH v1 1/2] checkout: factor out functions to new lib file Junio C Hamano 2017-11-14 8:46 ` Thomas Gummerer 2017-11-18 18:11 ` [PATCH v2 1/3] " Thomas Gummerer 2017-11-18 18:11 ` [PATCH v2 2/3] worktree: make add <path> <branch> dwim Thomas Gummerer 2017-11-18 22:18 ` Thomas Gummerer 2017-11-18 18:11 ` [PATCH v2 3/3] worktree: make add <path> dwim Thomas Gummerer 2017-11-18 22:47 ` [PATCH v3 0/3] make git worktree add dwim more Thomas Gummerer 2017-11-18 22:47 ` [PATCH v3 1/3] checkout: factor out functions to new lib file Thomas Gummerer 2017-11-18 22:47 ` [PATCH v3 2/3] worktree: make add <path> <branch> dwim Thomas Gummerer 2017-11-19 8:31 ` Eric Sunshine 2017-11-19 17:43 ` Thomas Gummerer 2017-11-18 22:47 ` [PATCH v3 3/3] worktree: make add <path> dwim Thomas Gummerer 2017-11-19 19:04 ` Eric Sunshine 2017-11-19 20:20 ` Eric Sunshine 2017-11-20 0:39 ` Junio C Hamano 2017-11-21 22:13 ` Thomas Gummerer 2017-11-22 1:20 ` Junio C Hamano 2017-11-22 19:49 ` Thomas Gummerer
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.