checkout: factor out functions to new lib file
Subsystems (2)
PATCH 1/2 checkout: factor out functions to new lib file
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.
---
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
Makefile +1 -0
@@ -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
builtin/checkout.c +1 -40
52 lines not on this page · raw message
checkout.c new file +43 -0
43 lines not on this page · raw message
checkout.h new file +14 -0
@@ -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 */
Signature · 1 line
2.15.0.403.gc27cc4dac6
PATCH 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 <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.
---
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(-)
builtin/worktree.c +13 -1
@@ -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;
t/t2025-worktree-add.sh +70 -0
80 lines not on this page · raw message
Signature · 1 line
2.15.0.403.gc27cc4dac6
Re: [PATCH v1 2/2] worktree: make add dwim
Quoted 4 lines · 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.
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 topicis 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?
Re: [PATCH v1 2/2] worktree: make add dwim
Quoted 26 lines · Junio C Hamano
Quoted 4 lines · 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.
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-bnor-Bnor--detachused, 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.
Quoted 7 lines
Which in turn means that the new variable name
DWIM_NEW_BRANCHand 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.
Quoted 3 lines
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.
Quoted 8 lines
I am guessing that, when you do not have a branch "topic" but your upstream does, saying
$ git worktree add ../a-new-worktree topicwould create "refs/heads/topic" to build on top of "refs/remotes/origin/topic" just like "git checkout topic" would.
Quoted 6 lines
IOW, when fully spelled out:
$ git branch -t -b topic origin/topic $ git worktree add ../a-new-worktree topicis 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 topicinstead 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.
Quoted 7 lines
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 topicwould 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
Quoted 16 lines · Thomas Gummerer
Quoted 7 lines · Junio C Hamano
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 topicwould 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
Quoted 8 lines
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
Quoted 6 lines · Eric Sunshine
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".
Quoted 6 lines
* 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
Quoted 5 lines · Eric Sunshine
Quoted 4 lines · Eric Sunshine
For my own edification...
[...]
git worktree add ../topic
Quoted 8 lines
Quoted 2 lines
* 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.
Quoted 10 lines
* If origin/topic exists, DWIM local "topic" branch into existence.
* Otherwise, create new local branch "topic".
Quoted 6 lines
* 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
Quoted 12 lines · Thomas Gummerer
Quoted 11 lines · Eric Sunshine
Quoted 6 lines · Eric Sunshine
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":
Quoted 6 lines
Quoted 2 lines
* 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.
Quoted 10 lines
Quoted 10 lines
* If origin/topic exists, DWIM local "topic" branch into existence.
* Otherwise, create new local branch "topic".
Quoted 6 lines
* 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
In reply to Eric SunshineQuoted 30 lines · Eric Sunshine
Quoted 16 lines · Thomas Gummerer
Quoted 7 lines · Junio C Hamano
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 topicwould 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 ../topicQuoted 8 lines
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
Quoted 8 lines
* 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.
Quoted 12 lines
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".
Quoted 7 lines
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.
Quoted 9 lines
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.
Quoted 8 lines
* 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 :)
Quoted 16 lines
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
Quoted 16 lines · Thomas Gummerer
Quoted 7 lines · Eric Sunshine
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".
Quoted 7 lines
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.
Quoted 8 lines
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".
Quoted 18 lines
Quoted 9 lines
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.
Quoted 8 lines
* 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).
Quoted 4 lines
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.
Quoted 3 lines
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.
Quoted 1 line
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.
Quoted 13 lines · 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.cto make git worktree add dwim more.While there add a description to the function.
Signed-off-by Thomas Gummerer <[email protected]>---
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!
Quoted 7 lines
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.
checkout.c line 2
Quoted 8 lines
checkout.c new file +2
-0@@ -0,0 +1,43 @@ +#include "cache.h" +
A useless blank line.
checkout.c line 4
Quoted 2 lines
checkout.c new file +2
-0+#include "remote.h" +
This one is useful ;-)
checkout.h line 5
Quoted 13 lines
checkout.c new file +1
-0+struct tracking_name_data {checkout.h new file +5
-0@@ -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".
Quoted 23 lines · Junio C Hamano
Quoted 13 lines · 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.cto make git worktree add dwim more.While there add a description to the function.
Signed-off-by Thomas Gummerer <[email protected]>---
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!
Quoted 7 lines
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
Quoted 7 lines
I am not sure either. I've always considered that
entry.cis 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 inentry.csuch ascheckout_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.
Quoted 16 lines
checkout.c line 2
Quoted 8 lines
checkout.c new file +2
-0@@ -0,0 +1,43 @@ +#include "cache.h" +A useless blank line.
checkout.c line 4
Quoted 2 lines
checkout.c new file +2
-0+#include "remote.h" +This one is useful ;-)
Quoted 16 lines
checkout.h line 5
Quoted 13 lines
checkout.c new file +1
-0+struct tracking_name_data {checkout.h new file +5
-0@@ -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!
PATCH v2 1/3 checkout: factor out functions to new lib file
In reply to Thomas GummererFactor 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.
---
The previous round of this series is at
https://public-inbox.org/git/[email protected]/.
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
Makefile +1 -0
@@ -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
builtin/checkout.c +1 -40
52 lines not on this page · raw message
checkout.c new file +42 -0
42 lines not on this page · raw message
checkout.h new file +13 -0
@@ -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 */
Signature · 1 line
2.15.0.345.gf926f18f3d
PATCH v2 2/3 worktree: make add <path> <branch> dwim
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.
--- Documentation/git-worktree.txt | 7 +++++++ builtin/worktree.c | 15 ++++++++++++++ t/t2025-worktree-add.sh | 44 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+)
Documentation/git-worktree.txt +7 -0
@@ -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.
builtin/worktree.c +15 -0
@@ -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 ;
t/t2025-worktree-add.sh +44 -0
54 lines not on this page · raw message
Signature · 1 line
2.15.0.345.gf926f18f3d
Re: [PATCH v2 2/3] worktree: make add <path> <branch> dwim
Quoted 48 lines · 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 <[email protected]>--- Documentation/git-worktree.txt | 7 +++++++ builtin/worktree.c | 15 ++++++++++++++ t/t2025-worktree-add.sh | 44 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+)Documentation/git-worktree.txt +7
-0@@ -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.builtin/worktree.c +1
-0@@ -1,4 +1,5 @@#include "cache.h" +#include "checkout.h" #include "config.h" #include "builtin.h" #include "dir.h"
builtin/worktree.c static int add(int ac, const char **av, const char *prefix) · line 390
Quoted 4 lines
builtin/worktree.c +1
-0@@ -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.
Quoted 79 lines
builtin/worktree.c +13
-0+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 ;t/t2025-worktree-add.sh +44
-0@@ -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_doneSignature · 1 line
2.15.0.345.gf926f18f3d
PATCH v2 3/3 worktree: make add <path> dwim
In reply to Thomas GummererCurrently '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.
---
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(-)
Documentation/git-worktree.txt +12 -3
@@ -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
builtin/worktree.c +9 -0
@@ -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 ) {
t/t2025-worktree-add.sh +60 -0
64 lines not on this page · raw message
Signature · 1 line
2.15.0.345.gf926f18f3d
v3 cover make git worktree add dwim more
In reply to Thomas GummererSorry 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/[email protected]/
and https://public-inbox.org/git/[email protected]/.
In case anybody already started reading v2, an interdiff between the two versions is below:
Documentation/git-worktree.txt +3 -3
@@ -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> ------------
builtin/worktree.c +1 -1
@@ -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;
t/t2025-worktree-add.sh +2 -0
@@ -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
Signature · 1 line
2.15.0.345.gf926f18f3
PATCH v3 1/3 checkout: factor out functions to new lib file
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.
--- 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
Makefile +1 -0
@@ -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
builtin/checkout.c +1 -40
52 lines not on this page · raw message
checkout.c new file +42 -0
42 lines not on this page · raw message
checkout.h new file +13 -0
@@ -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 */
Signature · 1 line
2.15.0.345.gf926f18f3
PATCH v3 2/3 worktree: make add <path> <branch> dwim
In reply to Thomas GummererCurrently '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.
--- Documentation/git-worktree.txt | 7 +++++++ builtin/worktree.c | 15 ++++++++++++++ t/t2025-worktree-add.sh | 46 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+)
Documentation/git-worktree.txt +7 -0
@@ -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.
builtin/worktree.c +15 -0
@@ -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 ;
t/t2025-worktree-add.sh +46 -0
56 lines not on this page · raw message
Signature · 1 line
2.15.0.345.gf926f18f3
Re: [PATCH v3 2/3] worktree: make add <path> <branch> dwim
Quoted 16 lines · 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 <[email protected]>---
Documentation/git-worktree.txt is linked to the current repository, sharing everything except working · line 57
Quoted 6 lines
Documentation/git-worktree.txt +6
-0@@ -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?)
Quoted 17 lines
builtin/worktree.c +4
-0@@ -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;
builtin/worktree.c static int add(int ac, const char **av, const char *prefix) · line 402
Quoted 10 lines
builtin/worktree.c +10
-0+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.)
Quoted 5 lines
builtin/worktree.c +0
-0if (opts.new_branch) {struct child_process cp =CHILD_PROCESS_INIT ; cp.git_cmd =1 ;
t/t2025-worktree-add.sh test_description='test git worktree add' · line 14
Quoted 9 lines
t/t2025-worktree-add.sh +9
-0@@ -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...
t/t2025-worktree-add.sh test_expect_success 'rename a branch under bisect not allowed' ' · line 331
Quoted 12 lines
t/t2025-worktree-add.sh +8
-0test_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 &&
Quoted 27 lines
t/t2025-worktree-add.sh +27
-0+ 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 + ) +'
Re: [PATCH v3 2/3] worktree: make add <path> <branch> dwim
Quoted 22 lines · Eric Sunshine
Quoted 21 lines · 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 <[email protected]>---
Documentation/git-worktree.txt +5
-0@@ -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>
Quoted 8 lines
Documentation/git-worktree.txt is linked to the current repository, sharing everything except working · line 57
Quoted 1 line
Documentation/git-worktree.txt +1
-0+------------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.
Quoted 30 lines
Quoted 17 lines
builtin/worktree.c +4
-0@@ -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;builtin/worktree.c static int add(int ac, const char **av, const char *prefix) · line 402
Quoted 10 lines
builtin/worktree.c +10
-0+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:
Quoted 7 lines
commit = ...
if (!commit) {
remote = ...
if (remote) {
...
}
}Will change, thanks!
Quoted 4 lines
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.
Quoted 9 lines
Quoted 9 lines
builtin/worktree.c +0
-0if (opts.new_branch) {struct child_process cp =CHILD_PROCESS_INIT ; cp.git_cmd =1 ;t/t2025-worktree-add.sh +4
-0@@ -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 && + {
Quoted 8 lines
t/t2025-worktree-add.sh test_description='test git worktree add' · line 17
Quoted 5 lines
t/t2025-worktree-add.sh +5
-0+ 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.
Quoted 15 lines
t/t2025-worktree-add.sh test_expect_success 'rename a branch under bisect not allowed' ' · line 331
Quoted 12 lines
t/t2025-worktree-add.sh +8
-0test_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:
Quoted 1 line
test_when_finished rm -rf repo_upstream repo_dwim foo &&
Thanks, will change!
Thanks a lot for the review!
Quoted 27 lines
Quoted 27 lines
t/t2025-worktree-add.sh +27
-0+ 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 + ) +'
PATCH v3 3/3 worktree: make add <path> dwim
In reply to Thomas GummererCurrently '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.
--- Documentation/git-worktree.txt | 15 ++++++++--- builtin/worktree.c | 9 +++++++ t/t2025-worktree-add.sh | 60 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 3 deletions(-)
Documentation/git-worktree.txt +12 -3
@@ -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
builtin/worktree.c +9 -0
@@ -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) {
t/t2025-worktree-add.sh +60 -0
64 lines not on this page · raw message
Signature · 1 line
2.15.0.345.gf926f18f3
Re: [PATCH v3 3/3] worktree: make add <path> dwim
Quoted 24 lines · 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 <[email protected]>---
Documentation/git-worktree.txt $ git worktree add -b <branch> <path> <remote>/<branch> · line 64
Quoted 8 lines
Documentation/git-worktree.txt +3
-3@@ -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.
Documentation/git-worktree.txt $ git worktree add -b <branch> <path> <remote>/<branch> · line 69
Quoted 4 lines
Documentation/git-worktree.txt +4
-0+------------ +$ git worktree add -b <branch> <path> <remote>/<branch> +------------ +If no tracking branch exists in exactly one remote, <branch> is
Typesetting: <foo>
Documentation/git-worktree.txt $ git worktree add -b <branch> <path> <remote>/<branch> · line 73
Quoted 4 lines
Documentation/git-worktree.txt +4
-0+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?
Documentation/git-worktree.txt $ git worktree add -b <branch> <path> <remote>/<branch> · line 75
Quoted 2 lines
Documentation/git-worktree.txt +1
-0+flag can be passed to `git worktree add`. list::
You lost a blank line before "list::".
builtin/worktree.c static int add(int ac, const char **av, const char *prefix) · line 352
Quoted 4 lines
Documentation/git-worktree.txt +0
-0List details of each worktree. The main worktree is listed first, followed bybuiltin/worktree.c +1
-0@@ -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.
Quoted 24 lines
builtin/worktree.c +0
-0OPT_END() };t/t2025-worktree-add.sh +19
-0@@ -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 &&
t/t2025-worktree-add.sh test_expect_success '"add" <path> <branch> dwims' ' · line 388
Quoted 10 lines
t/t2025-worktree-add.sh +10
-0+ 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?
Quoted 32 lines
t/t2025-worktree-add.sh +31
-0+ +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
Re: [PATCH v3 3/3] worktree: make add <path> dwim
Quoted 6 lines · Eric Sunshine
Documentation/git-worktree.txt $ git worktree add -b <branch> <path> <remote>/<branch> · line 73
Quoted 2 lines · Thomas Gummerer
Documentation/git-worktree.txt +2
-0+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.)
Re: [PATCH v3 3/3] worktree: make add <path> dwim
Quoted 8 lines · Eric Sunshine
Quoted 6 lines · Eric Sunshine
Documentation/git-worktree.txt $ git worktree add -b <branch> <path> <remote>/<branch> · line 73
Quoted 2 lines · Thomas Gummerer
Documentation/git-worktree.txt +2
-0+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?
Quoted 6 lines
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.
Re: [PATCH v3 3/3] worktree: make add <path> dwim
In reply to Eric SunshineQuoted 8 lines · Eric Sunshine
Quoted 6 lines · Eric Sunshine
Documentation/git-worktree.txt $ git worktree add -b <branch> <path> <remote>/<branch> · line 73
Quoted 2 lines · Thomas Gummerer
Documentation/git-worktree.txt +2
-0+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?
Quoted 6 lines
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!
Re: [PATCH v3 3/3] worktree: make add <path> dwim
Quoted 4 lines · Thomas Gummerer
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.
Re: [PATCH v3 3/3] worktree: make add <path> dwim
Quoted 7 lines · Junio C Hamano
Quoted 4 lines · Thomas Gummerer
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'.
Quoted 6 lines
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.
Quoted 3 lines
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".
Quoted 1 line
Thanks.