Re: [PATCH v1 2/2] worktree: make add dwim
On 11/15, Thomas Gummerer wrote: > On 11/14, Eric Sunshine wrote: > > On Tue, Nov 14, 2017 at 3:14 PM, Eric Sunshine > > 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".
Re: [PATCH v1 2/2] worktree: make add dwim
On Wed, Nov 15, 2017 at 3:50 AM, Thomas Gummerer 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.
Re: [PATCH v1 2/2] worktree: make add dwim
On 11/14, Eric Sunshine wrote: > On Tue, Nov 14, 2017 at 3:14 PM, Eric Sunshine > 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".
Re: [PATCH v1 2/2] worktree: make add dwim
On 11/14, Eric Sunshine wrote:
> On Tue, Nov 14, 2017 at 3:45 AM, Thomas Gummerer 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
> l
Re: [PATCH v1 2/2] worktree: make add dwim
On Tue, Nov 14, 2017 at 3:14 PM, Eric Sunshine 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".
Re: [PATCH v1 2/2] worktree: make add dwim
On Tue, Nov 14, 2017 at 3:45 AM, Thomas Gummerer 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.
Re: [PATCH v1 2/2] worktree: make add dwim
On 11/13, Junio C Hamano wrote: > Thomas Gummerer 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 []": > > If `` 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 )` 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.
Re: [PATCH v1 2/2] worktree: make add dwim
Thomas Gummerer 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 []": If `` 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 )` 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?
[PATCH v1 2/2] worktree: make add dwim
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 ', 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
---
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 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 >actu
