Re: [PATCH v3] checkout & worktree: introduce checkout.implicitRemote

[Ævar Arnfjörð Bjarmason]

On Thu, May 24 2018, Ævar Arnfjörð Bjarmason wrote:

>  struct tracking_name_data {
>   /* const */ char *src_ref;
>   char *dst_ref;
>   struct object_id *dst_oid;
>   int unique;
> + const char *implicit_remote;
> + char *implicit_dst_ref;
>  };

There's a bug here that I'll fix in a v3. We need to have a implicit_*
variant for dst_oid as well. Currently this will be buggy and check out
origin/, but then check the index out to the tree of whatever
the last / we iterated over was.

Easiy fix and I already have it locally, I just want to improve some of
the testing. I missed it because in my tests I'd just re-add the same
remote again, so the trees were the same.

>  static int check_tracking_name(struct remote *remote, void *cb_data)
> @@ -20,6 +23,8 @@ static int check_tracking_name(struct remote *remote, void 
> *cb_data)
>   free(query.dst);
>   return 0;
>   }
> + if (cb->implicit_remote && !strcmp(remote->name, cb->implicit_remote))
> + cb->implicit_dst_ref = xstrdup(query.dst);
>   if (cb->dst_ref) {
>   free(query.dst);
>   cb->unique = 0;
> @@ -31,13 +36,21 @@ static int check_tracking_name(struct remote *remote, 
> void *cb_data)
>
>  const char *unique_tracking_name(const char *name, struct object_id *oid)
>  {
> - struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 };
> + const char *implicit_remote = NULL;
> + struct tracking_name_data cb_data = { NULL, NULL, NULL, 1, NULL, NULL };
> + if (!git_config_get_string_const("checkout.implicitremote", 
> _remote))
> + cb_data.implicit_remote = implicit_remote;
>   cb_data.src_ref = xstrfmt("refs/heads/%s", name);
>   cb_data.dst_oid = oid;
>   for_each_remote(check_tracking_name, _data);
>   free(cb_data.src_ref);
> - if (cb_data.unique)
> + free((char *)implicit_remote);
> + if (cb_data.unique) {
> + free(cb_data.implicit_dst_ref);
>   return cb_data.dst_ref;
> + }
>   free(cb_data.dst_ref);
> + if (cb_data.implicit_dst_ref)
> + return cb_data.implicit_dst_ref;
>   return NULL;
>  }
> diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
> index 3e5ac81bd2..da6bd74bbc 100755
> --- a/t/t2024-checkout-dwim.sh
> +++ b/t/t2024-checkout-dwim.sh
> @@ -68,6 +68,16 @@ test_expect_success 'checkout of branch from multiple 
> remotes fails #1' '
>   test_branch master
>  '
>
> +test_expect_success 'checkout of branch from multiple remotes succeeds with 
> checkout.implicitRemote #1' '
> + git checkout -B master &&
> + test_might_fail git branch -D foo &&
> +
> + git -c checkout.implicitRemote=repo_a checkout foo &&
> + test_branch foo &&
> + test_cmp_rev remotes/repo_a/foo HEAD &&
> + test_branch_upstream foo repo_a foo
> +'
> +
>  test_expect_success 'checkout of branch from a single remote succeeds #1' '
>   git checkout -B master &&
>   test_might_fail git branch -D bar &&
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 2240498924..271a6413f0 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -402,6 +402,24 @@ test_expect_success '"add"   dwims' '
>   )
>  '
>
> +test_expect_success '"add"   dwims with 
> checkout.implicitRemote' '
> + test_when_finished rm -rf repo_upstream repo_dwim foo &&
> + setup_remote_repo repo_upstream repo_dwim &&
> + git init repo_dwim &&
> + (
> + cd repo_dwim &&
> + git remote add repo_upstream2 ../repo_upstream &&
> + git fetch repo_upstream2 &&
> + test_must_fail git worktree add ../foo foo &&
> + git -c checkout.implicitRemote=repo_upstream worktree add 
> ../foo foo
> + ) &&
> + (
> + cd foo &&
> + test_branch_upstream foo repo_upstream foo &&
> + test_cmp_rev refs/remotes/repo_upstream/foo refs/heads/foo
> + )
> +'
> +
>  test_expect_success 'git worktree add does not match remote' '
>   test_when_finished rm -rf repo_a repo_b foo &&
>   setup_remote_repo repo_a repo_b &&

Re: [PATCH v3] checkout & worktree: introduce checkout.implicitRemote

[Duy Nguyen]

On Fri, May 25, 2018 at 8:38 PM, Ævar Arnfjörð Bjarmason
 wrote:
>
> On Fri, May 25 2018, Duy Nguyen wrote:
>
>> On Fri, May 25, 2018 at 10:12 AM, Junio C Hamano  wrote:
 +Currently this is used by linkgit:git-checkout[1] when 'git checkout
 +' will checkout the '' branch on another remote,
 +and by linkgit:git-worktree[1] when 'git worktree add' refers to a
 +remote branch. This setting might be used for other checkout-like
 +commands or functionality in the future.
>>>
>>> Hmph, that is an interesting direction.  You foresee that you'll
>>> have a single repository with multiple remotes to grab and share
>>> objects from different people working on the same project, and use
>>> multiple worktrees to work on different branches, yet you are happy
>>> to declare that each worktree is to work with one particular remote?
>>>
>>> We'd need a per-worktree config file to make it work, I guess, or
>>> a three-level checkout.$worktree_id.defaultRemote configuration
>>> variable, perhaps?
>>
>> I still plan to revisit per-worktree config support [1] at some point
>> and finish it off. Or is it decided that we don't need a generic
>> mechanism and will add a new level like this for each config var that
>> is per-worktree? If defaultRemote sets a precedence, then it'll be the
>> way to go from now on or we'll have another mess when some config does
>> "foo.$worktree.bar" while others use per-worktree config files.
>
> What do you think about including worktree in this at this time? I'm not
> very familiar with it and just included it because I worked my way
> backwards from the DWIM function, but I could exclude it if you think
> it's too much trouble, i.e. if you'd like to hold out for some facility
> like the per-worktree config Junio mentioned.

I think Junio was talking about the future. What is currently in the
patch, both code and document, is fine. But if you're going to
implement core.$worktree.defaultRemote at this time, maybe wait a bit.
I could try to resurrect the per-worktree config topic in a month or
so and if it does not work out, then everybody will add new config
vars if they want per-worktree support.
-- 
Duy

Re: [PATCH v3] checkout & worktree: introduce checkout.implicitRemote

[Ævar Arnfjörð Bjarmason]

On Fri, May 25 2018, Duy Nguyen wrote:

> On Fri, May 25, 2018 at 10:12 AM, Junio C Hamano  wrote:
>>> +Currently this is used by linkgit:git-checkout[1] when 'git checkout
>>> +' will checkout the '' branch on another remote,
>>> +and by linkgit:git-worktree[1] when 'git worktree add' refers to a
>>> +remote branch. This setting might be used for other checkout-like
>>> +commands or functionality in the future.
>>
>> Hmph, that is an interesting direction.  You foresee that you'll
>> have a single repository with multiple remotes to grab and share
>> objects from different people working on the same project, and use
>> multiple worktrees to work on different branches, yet you are happy
>> to declare that each worktree is to work with one particular remote?
>>
>> We'd need a per-worktree config file to make it work, I guess, or
>> a three-level checkout.$worktree_id.defaultRemote configuration
>> variable, perhaps?
>
> I still plan to revisit per-worktree config support [1] at some point
> and finish it off. Or is it decided that we don't need a generic
> mechanism and will add a new level like this for each config var that
> is per-worktree? If defaultRemote sets a precedence, then it'll be the
> way to go from now on or we'll have another mess when some config does
> "foo.$worktree.bar" while others use per-worktree config files.

What do you think about including worktree in this at this time? I'm not
very familiar with it and just included it because I worked my way
backwards from the DWIM function, but I could exclude it if you think
it's too much trouble, i.e. if you'd like to hold out for some facility
like the per-worktree config Junio mentioned.

Re: [PATCH v3] checkout & worktree: introduce checkout.implicitRemote

[Duy Nguyen]

On Fri, May 25, 2018 at 10:12 AM, Junio C Hamano  wrote:
>> +Currently this is used by linkgit:git-checkout[1] when 'git checkout
>> +' will checkout the '' branch on another remote,
>> +and by linkgit:git-worktree[1] when 'git worktree add' refers to a
>> +remote branch. This setting might be used for other checkout-like
>> +commands or functionality in the future.
>
> Hmph, that is an interesting direction.  You foresee that you'll
> have a single repository with multiple remotes to grab and share
> objects from different people working on the same project, and use
> multiple worktrees to work on different branches, yet you are happy
> to declare that each worktree is to work with one particular remote?
>
> We'd need a per-worktree config file to make it work, I guess, or
> a three-level checkout.$worktree_id.defaultRemote configuration
> variable, perhaps?

I still plan to revisit per-worktree config support [1] at some point
and finish it off. Or is it decided that we don't need a generic
mechanism and will add a new level like this for each config var that
is per-worktree? If defaultRemote sets a precedence, then it'll be the
way to go from now on or we'll have another mess when some config does
"foo.$worktree.bar" while others use per-worktree config files.

[1] https://public-inbox.org/git/20170110112524.12870-1-pclo...@gmail.com/
-- 
Duy

Re: [PATCH v3] checkout & worktree: introduce checkout.implicitRemote

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> +checkout.implicitRemote::
> + When you run 'git checkout ' and only have one
> + remote, it may implicitly fall back on checking out and
> + tracking e.g. 'origin/'.

Yup, that is quite implicit.  It works without configuring anything,
only as long as you have a single remote.  That is quite implicit.


> This stops working as soon
> + as you have more than one remote with a ''
> + reference. This setting allows for setting the name of a
> + preferred remote that should always win when it comes to
> + disambiguation. The typical use-case is to set this to
> + `origin`.

So this feature and configuration feels more like an explicit one,
to be used to affect how Git works when the implicit one does not
work well.  I would have called it checkout.defaultRemote, as it
would be a nonsense name to call it checkout.explicitRemote ;-).

> +Currently this is used by linkgit:git-checkout[1] when 'git checkout
> +' will checkout the '' branch on another remote,
> +and by linkgit:git-worktree[1] when 'git worktree add' refers to a
> +remote branch. This setting might be used for other checkout-like
> +commands or functionality in the future.

Hmph, that is an interesting direction.  You foresee that you'll
have a single repository with multiple remotes to grab and share
objects from different people working on the same project, and use
multiple worktrees to work on different branches, yet you are happy
to declare that each worktree is to work with one particular remote?

We'd need a per-worktree config file to make it work, I guess, or
a three-level checkout.$worktree_id.defaultRemote configuration
variable, perhaps?

In any case I can see how this will help those with multiple remotes
(including me ;-).  Thanks for moving this topic forward.

[PATCH v3] checkout & worktree: introduce checkout.implicitRemote

[Ævar Arnfjörð Bjarmason]

Introduce a checkout.implicitRemote setting which can be used to
designate a remote to prefer (via checkout.implicitRemote=origin) when
running e.g. "git checkout master" to mean origin/master, even though
there's other remotes that have the "master" branch.

I want this because it's very handy to use this workflow to checkout a
repository and create a topic branch, then get back to a "master" as
retrieved from upstream:

(
rm -rf /tmp/tbdiff &&
git clone g...@github.com:trast/tbdiff.git &&
cd tbdiff &&
git branch -m topic &&
git checkout master
)

That will output:

Branch 'master' set up to track remote branch 'master' from 'origin'.
Switched to a new branch 'master'

But as soon as a new remote is added (e.g. just to inspect something
from someone else) the DWIMery goes away:

(
rm -rf /tmp/tbdiff &&
git clone g...@github.com:trast/tbdiff.git &&
cd tbdiff &&
git branch -m topic &&
git remote add avar g...@github.com:avar/tbdiff.git &&
git fetch avar &&
git checkout master
)

Will output:

error: pathspec 'master' did not match any file(s) known to git.

The new checkout.implicitRemote config allows me to say that whenever
that ambiguity comes up I'd like to prefer "origin", and it'll still
work as though the only remote I had was "origin".

I considered splitting this into checkout.implicitRemote and
worktree.implicitRemote, but it's probably less confusing to break our
own rules that anything shared between config should live in core.*
than have two config settings, and I couldn't come up with a short
name under core.* that made sense (core.implicitRemoteForCheckout?).

See also 70c9ac2f19 ("DWIM "git checkout frotz" to "git checkout -b
frotz origin/frotz"", 2009-10-18) which introduced this DWIM feature
to begin with, and 4e85333197 ("worktree: make add  
dwim", 2017-11-26) which added it to git-worktree.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---

Took me a while to get around to this, but this v3 addresses feedback
by Eric about the docs. Thanks! interdiff:

1: 905a17f35f ! 1: b23d1b71e9 checkout & worktree: introduce 
checkout.implicitRemote
@@ -71,16 +71,15 @@
 +  tracking e.g. 'origin/'. This stops working as soon
 +  as you have more than one remote with a ''
 +  reference. This setting allows for setting the name of a
-+  special remote that should always win when it comes to
++  preferred remote that should always win when it comes to
 +  disambiguation. The typical use-case is to set this to
 +  `origin`.
 ++
 +Currently this is used by linkgit:git-checkout[1] when 'git checkout
 +' will checkout the '' branch on another remote,
-+and by linkgit:git-worktree[1] when 'git worktree add' when referring
-+to a remote branch.  This setting might be used for other
-+checkout-like commands or functionality in the future when
-+appropriate.
++and by linkgit:git-worktree[1] when 'git worktree add' refers to a
++remote branch. This setting might be used for other checkout-like
++commands or functionality in the future.
 +
  clean.requireForce::
A boolean to make git-clean do nothing unless given -f,
@@ -110,14 +109,14 @@
  $ git worktree add --track -b   /
  
  +
-+It's also possible to use the `checkout.implicitRemote` setting to
-+designate a special remote this rule should be applied to, even if the
-+branch isn't unique across all remotes. See `checkout.implicitRemote`
-+in linkgit:git-config[1].
++The `checkout.implicitRemote` setting can be used to to designate a
++preferred `` this rule should be applied to, even if the
++`` isn't unique across all remotes. See
++`checkout.implicitRemote` in linkgit:git-config[1].
 ++
  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.
+ then, as a convenience, the new worktree is associated with a branch
+ (call it ``) named after `$(basename )`.  If ``
 
 diff --git a/builtin/checkout.c b/builtin/checkout.c
 --- a/builtin/checkout.c
@@ -235,6 +234,6 @@
 +  )
 +'
 +
- post_checkout_hook () {
-   gitdir=${1:-.git}
-   test_when_finished "rm -f $gitdir/hooks/post-checkout" &&
+ test_expect_success 'git worktree add does not match remote' '
+   test_when_finished rm -rf repo_a repo_b foo &&
+   setup_remote_repo repo_a repo_b &&

 Documentation/config.txt   | 16