[PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[marcnarc]

From: Marc Branchaud <marcnarc@xiplink.com>

get_default_remote() tries to use the checked-out branch's 'remote' config
value to figure out the remote's name.  This fails if there is no currently
checked-out branch (i.e. HEAD is detached) or if the checked-out branch
doesn't track a remote.  In these cases and the function would just fall
back to "origin".

Instead, let's use the first remote listed in the configuration, and fall
back to "origin" only if we don't find any configured remotes.

Prior to this change, git would fail to initialize a relative-path
submodule if the super-repo was on a detached HEAD and it had no remote
named "origin".

Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---

Our build system likes to use detached HEADs, so we got tripped up when we
started using relative submodule URLs.

(I'm not sure about the portability of my change, or if I should wrap it
to 80 columns...)

 git-parse-remote.sh        |  1 +
 t/t7400-submodule-basic.sh | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index 484b2e6..225ad94 100644
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -8,6 +8,7 @@ get_default_remote () {
 	curr_branch=$(git symbolic-ref -q HEAD)
 	curr_branch="${curr_branch#refs/heads/}"
 	origin=$(git config --get "branch.$curr_branch.remote")
+	test -z "$origin" && origin=$(git config --list | grep '^remote\.' | head -1 | awk -F . '{print $2}')
 	echo ${origin:-origin}
 }
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 81827e6..8f1ff4f 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -507,6 +507,28 @@ test_expect_success 'relative path works with user@host:path' '
 	)
 '
 
+test_expect_success 'relative path works on detached HEAD and remote is not named "origin"' '
+	mkdir detachtest &&
+	(
+		cd detachtest &&
+		git init &&
+		mkdir sub &&
+		(
+			cd sub &&
+			git init &&
+			test_commit foo
+		) &&
+		git add sub &&
+		git commit -m "added sub" &&
+		git checkout HEAD@{0} &&
+		git config -f .gitmodules submodule.sub.path sub &&
+		git config -f .gitmodules submodule.sub.url ../subrepo &&
+		git remote add awkward ssh://awkward/repo &&
+		git submodule init sub &&
+		test "$(git config submodule.sub.url)" = ssh://awkward/subrepo
+	)
+'
+
 test_expect_success 'moving the superproject does not break submodules' '
 	(
 		cd addtest &&
-- 
1.7.11.dirty

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Junio C Hamano]

marcnarc@xiplink.com writes:

> From: Marc Branchaud <marcnarc@xiplink.com>
>
> get_default_remote() tries to use the checked-out branch's 'remote' config
> value to figure out the remote's name.  This fails if there is no currently
> checked-out branch (i.e. HEAD is detached) or if the checked-out branch
> doesn't track a remote.  In these cases and the function would just fall
> back to "origin".
>
> Instead, let's use the first remote listed in the configuration, and fall
> back to "origin" only if we don't find any configured remotes.

I admit that I wouldn't do anything that relies on any remote to be
used while on detached head myself, so in that sense I am a biased
audience, but guessing (or not guessing and blindly assuming
'origin') feels wrong, and trying even harder to come up with an
even wilder guess feels even more wrong.

Shouldn't we be erroring out instead?

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Marc Branchaud]

On 12-06-18 01:33 PM, Junio C Hamano wrote:
> marcnarc@xiplink.com writes:
> 
>> From: Marc Branchaud <marcnarc@xiplink.com>
>>
>> get_default_remote() tries to use the checked-out branch's 'remote' config
>> value to figure out the remote's name.  This fails if there is no currently
>> checked-out branch (i.e. HEAD is detached) or if the checked-out branch
>> doesn't track a remote.  In these cases and the function would just fall
>> back to "origin".
>>
>> Instead, let's use the first remote listed in the configuration, and fall
>> back to "origin" only if we don't find any configured remotes.
> 
> I admit that I wouldn't do anything that relies on any remote to be
> used while on detached head myself, so in that sense I am a biased
> audience, but guessing (or not guessing and blindly assuming
> 'origin') feels wrong, and trying even harder to come up with an
> even wilder guess feels even more wrong.

OK, but what would be right?  AFAIK git doesn't have any real way of
designating an official default remote.  It seems to me that the first one in
the config is actually better than just using "origin".  At least, that
remote seems very likely to be the one used when the repo was cloned.

> Shouldn't we be erroring out instead?

That would be bad for our situation.  As I said, our automated build system
uses detached HEADs a lot.  Erroring-out in this case would break us.  It's
really only the near-ubiquity of the name "origin" that has kept things
working so far.

But we finally ran into a situation where someone used "git clone -o", and
that seems to have broken relative-path submodules in some situations.  (I
would not be at all surprised if "git clone -o" breaks an assortment of other
features, too.  I think there are a few places in the code where "origin" is
presumed to be a valid remote name, and also the one the user wants to use.)

		M.

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Junio C Hamano]

Marc Branchaud <marcnarc@xiplink.com> writes:

> On 12-06-18 01:33 PM, Junio C Hamano wrote:
>> marcnarc@xiplink.com writes:
>> 
>>> From: Marc Branchaud <marcnarc@xiplink.com>
>>>
>>> get_default_remote() tries to use the checked-out branch's 'remote' config
>>> value to figure out the remote's name.  This fails if there is no currently
>>> checked-out branch (i.e. HEAD is detached) or if the checked-out branch
>>> doesn't track a remote.  In these cases and the function would just fall
>>> back to "origin".
>>>
>>> Instead, let's use the first remote listed in the configuration, and fall
>>> back to "origin" only if we don't find any configured remotes.
>> 
>> I admit that I wouldn't do anything that relies on any remote to be
>> used while on detached head myself, so in that sense I am a biased
>> audience, but guessing (or not guessing and blindly assuming
>> 'origin') feels wrong, and trying even harder to come up with an
>> even wilder guess feels even more wrong.
>
> OK, but what would be right?  AFAIK git doesn't have any real way of
> designating an official default remote.

Correct, and that is why I tend to think "right" is to error out.

> That would be bad for our situation.  As I said, our automated build system
> uses detached HEADs a lot.  Erroring-out in this case would break us.  It's
> really only the near-ubiquity of the name "origin" that has kept things
> working so far.

That reliance of "origin" is what made me think that "not guessing
and blindly assuming" a wrong thing to do.

It is OK that your build usesdetached HEAD, but if that is the case
shoudln't it be the one deciding which specific remote it wants to
take the updated sources from, and telling Git to do so?

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Marc Branchaud]

On 12-06-18 06:12 PM, Junio C Hamano wrote:
> Marc Branchaud <marcnarc@xiplink.com> writes:
> 
>> On 12-06-18 01:33 PM, Junio C Hamano wrote:
>>> marcnarc@xiplink.com writes:
>>>
>>>> From: Marc Branchaud <marcnarc@xiplink.com>
>>>>
>>>> get_default_remote() tries to use the checked-out branch's 'remote' config
>>>> value to figure out the remote's name.  This fails if there is no currently
>>>> checked-out branch (i.e. HEAD is detached) or if the checked-out branch
>>>> doesn't track a remote.  In these cases and the function would just fall
>>>> back to "origin".
>>>>
>>>> Instead, let's use the first remote listed in the configuration, and fall
>>>> back to "origin" only if we don't find any configured remotes.
>>>
>>> I admit that I wouldn't do anything that relies on any remote to be
>>> used while on detached head myself, so in that sense I am a biased
>>> audience, but guessing (or not guessing and blindly assuming
>>> 'origin') feels wrong, and trying even harder to come up with an
>>> even wilder guess feels even more wrong.
>>
>> OK, but what would be right?  AFAIK git doesn't have any real way of
>> designating an official default remote.
> 
> Correct, and that is why I tend to think "right" is to error out.

Erroring out seems like a step backwards to me.  Things already work just
fine when the remote the user wants is called "origin".

I suppose you could say that I'm arguing for a better default remote than
"origin".

>> That would be bad for our situation.  As I said, our automated build system
>> uses detached HEADs a lot.  Erroring-out in this case would break us.  It's
>> really only the near-ubiquity of the name "origin" that has kept things
>> working so far.
> 
> That reliance of "origin" is what made me think that "not guessing
> and blindly assuming" a wrong thing to do.

I think git can do better than erroring out, though.

> It is OK that your build usesdetached HEAD, but if that is the case
> shoudln't it be the one deciding which specific remote it wants to
> take the updated sources from, and telling Git to do so?

Sure, but I feel it did that already when it cloned.  It seems reasonable for
the submodules to default to using the remote specified when the super-repo
was cloned.

		M.

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Junio C Hamano]

Marc Branchaud <marcnarc@xiplink.com> writes:

> On 12-06-18 06:12 PM, Junio C Hamano wrote:
> ...
>> That reliance of "origin" is what made me think that "not guessing
>> and blindly assuming" a wrong thing to do.
>
> I think git can do better than erroring out, though.
>
>> It is OK that your build usesdetached HEAD, but if that is the case
>> shoudln't it be the one deciding which specific remote it wants to
>> take the updated sources from, and telling Git to do so?
>
> Sure, but I feel it did that already when it cloned.  It seems reasonable for
> the submodules to default to using the remote specified when the super-repo
> was cloned.

I do not have a strong opinion either way, other than that I would
prefer predictable behaviour over "works most of the time provided
if the user does X, otherwise does this random thing".  And coming
from that standpoint, erroring out when there needs a guess involved
is certainly more predictable---it is a cop-out option for me in
areas of the system I do not have strong preferences.

If you can work with submodule people (it is good that you Cc'ed
Jens) to come up with a solution to make everything always use
'origin' when unconfigured in a consistent way, without breaking
existing users who rely on the current behaviour, that would be
good.  Or a solution with a predictable behaviour you come up with
may end up being something more involved than "just use 'origin'
when you do not know". As long as the end result can be easily
explained to end users, I wouldn't have any objection.

Thanks.

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Heiko Voigt]

Hi,

On Tue, Jun 19, 2012 at 10:55:13AM -0700, Junio C Hamano wrote:
> Marc Branchaud <marcnarc@xiplink.com> writes:
> 
> > On 12-06-18 06:12 PM, Junio C Hamano wrote:
> > ...
> >> That reliance of "origin" is what made me think that "not guessing
> >> and blindly assuming" a wrong thing to do.
> >
> > I think git can do better than erroring out, though.
> >
> >> It is OK that your build usesdetached HEAD, but if that is the case
> >> shoudln't it be the one deciding which specific remote it wants to
> >> take the updated sources from, and telling Git to do so?
> >
> > Sure, but I feel it did that already when it cloned.  It seems reasonable for
> > the submodules to default to using the remote specified when the super-repo
> > was cloned.
> 
> I do not have a strong opinion either way, other than that I would
> prefer predictable behaviour over "works most of the time provided
> if the user does X, otherwise does this random thing".  And coming
> from that standpoint, erroring out when there needs a guess involved
> is certainly more predictable---it is a cop-out option for me in
> areas of the system I do not have strong preferences.
> 
> If you can work with submodule people (it is good that you Cc'ed
> Jens) to come up with a solution to make everything always use
> 'origin' when unconfigured in a consistent way, without breaking
> existing users who rely on the current behaviour, that would be
> good.  Or a solution with a predictable behaviour you come up with
> may end up being something more involved than "just use 'origin'
> when you do not know". As long as the end result can be easily
> explained to end users, I wouldn't have any objection.

Using the first configured remote as a fallback also sounds quite
arbitrary to me. Your current HEAD might have nothing to do with that
remote.

Before falling back to origin how about guessing the remote from the
first branch thats contained in HEAD?

To me that sounds quite natural. The first branch could be ambiguous so
we would have to come up with some ordering. Maybe a depth search and
first parents first? Or a breadth first search with younger generations
first and then first parents first?

Would that work for your use case Marc?

Cheers Heiko

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Marc Branchaud]

On 12-06-19 03:31 PM, Heiko Voigt wrote:
> Hi,
> 
> On Tue, Jun 19, 2012 at 10:55:13AM -0700, Junio C Hamano wrote:
>> Marc Branchaud <marcnarc@xiplink.com> writes:
>>
>>> On 12-06-18 06:12 PM, Junio C Hamano wrote:
>>> ...
>>>> That reliance of "origin" is what made me think that "not guessing
>>>> and blindly assuming" a wrong thing to do.
>>>
>>> I think git can do better than erroring out, though.
>>>
>>>> It is OK that your build usesdetached HEAD, but if that is the case
>>>> shoudln't it be the one deciding which specific remote it wants to
>>>> take the updated sources from, and telling Git to do so?
>>>
>>> Sure, but I feel it did that already when it cloned.  It seems reasonable for
>>> the submodules to default to using the remote specified when the super-repo
>>> was cloned.
>>
>> I do not have a strong opinion either way, other than that I would
>> prefer predictable behaviour over "works most of the time provided
>> if the user does X, otherwise does this random thing".  And coming
>> from that standpoint, erroring out when there needs a guess involved
>> is certainly more predictable---it is a cop-out option for me in
>> areas of the system I do not have strong preferences.
>>
>> If you can work with submodule people (it is good that you Cc'ed
>> Jens) to come up with a solution to make everything always use
>> 'origin' when unconfigured in a consistent way, without breaking
>> existing users who rely on the current behaviour, that would be
>> good.  Or a solution with a predictable behaviour you come up with
>> may end up being something more involved than "just use 'origin'
>> when you do not know". As long as the end result can be easily
>> explained to end users, I wouldn't have any objection.
> 
> Using the first configured remote as a fallback also sounds quite
> arbitrary to me. Your current HEAD might have nothing to do with that
> remote.

Sure, but if it doesn't specify a remote then why not?  (Instead of "first
configured remote" think "remote used in the initial clone command".)

> Before falling back to origin how about guessing the remote from the
> first branch thats contained in HEAD?
>
> To me that sounds quite natural. The first branch could be ambiguous so
> we would have to come up with some ordering. Maybe a depth search and
> first parents first? Or a breadth first search with younger generations
> first and then first parents first?

This sounds much harder to explain to a user than "the remote you used when
you cloned the repo".

> Would that work for your use case Marc?

Maybe, but it seems much more complicated than necessary.

		M.

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Heiko Voigt]

Hi,

On Tue, Jun 19, 2012 at 05:42:58PM -0400, Marc Branchaud wrote:
> On 12-06-19 03:31 PM, Heiko Voigt wrote:
> > Using the first configured remote as a fallback also sounds quite
> > arbitrary to me. Your current HEAD might have nothing to do with that
> > remote.
> 
> Sure, but if it doesn't specify a remote then why not?  (Instead of "first
> configured remote" think "remote used in the initial clone command".)

Returning the "remote used in the initial clone" sounds ok to me as
well but that does not have to be the same as "first remote git config
--list returns".

> > Before falling back to origin how about guessing the remote from the
> > first branch thats contained in HEAD?
> >
> > To me that sounds quite natural. The first branch could be ambiguous so
> > we would have to come up with some ordering. Maybe a depth search and
> > first parents first? Or a breadth first search with younger generations
> > first and then first parents first?
> 
> This sounds much harder to explain to a user than "the remote you used when
> you cloned the repo".

Well I think "it uses the remote of the branch that you based your work
on" is not hard to explain. What's harder is the implementation.

> > Would that work for your use case Marc?
> 
> Maybe, but it seems much more complicated than necessary.

Git is a tool used in very different workflows so a change needs to be
quite generic. Make it as simple as possible but *no simpler*. I think
your patch is currently not respecting the "no simpler".

Cheers Heiko

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Jeff King]

On Tue, Jun 19, 2012 at 10:55:13AM -0700, Junio C Hamano wrote:

> Marc Branchaud <marcnarc@xiplink.com> writes:
> 
> > On 12-06-18 06:12 PM, Junio C Hamano wrote:
> > ...
> >> That reliance of "origin" is what made me think that "not guessing
> >> and blindly assuming" a wrong thing to do.
> >
> > I think git can do better than erroring out, though.
> >
> >> It is OK that your build usesdetached HEAD, but if that is the case
> >> shoudln't it be the one deciding which specific remote it wants to
> >> take the updated sources from, and telling Git to do so?
> >
> > Sure, but I feel it did that already when it cloned.  It seems reasonable for
> > the submodules to default to using the remote specified when the super-repo
> > was cloned.
> 
> I do not have a strong opinion either way, other than that I would
> prefer predictable behaviour over "works most of the time provided
> if the user does X, otherwise does this random thing".  And coming
> from that standpoint, erroring out when there needs a guess involved
> is certainly more predictable---it is a cop-out option for me in
> areas of the system I do not have strong preferences.

One thing that makes me nervous about this patch is that it is not just a
change to git-submodule, but rather to git-parse-remote.  So it could
affect other parts of the system, too, where a guess might not be as
desirable.

The number of affected code paths is fortunately quite small, since this
is updating the shell library, and most of the remote-handling code is
written in C these days. But it raises a few questions:

  1. git-pull can call into get_default_remote via get_remote_merge_branch.
     Is it impacted by this change?

  2. We install git-parse-remote as part of the plumbing API. Do we know
     of any other 3rd-party scripts that use this interface and might be
     affected?

  3. The C code sets up remote.c:default_remote_name, which defaults to
     "origin". Should this be consistent with what git-parse-remote
     does?

Should this be a submodule-only thing?

-Peff

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jun 19, 2012 at 10:55:13AM -0700, Junio C Hamano wrote:
>
>> I do not have a strong opinion either way, other than that I would
>> prefer predictable behaviour over "works most of the time provided
>> if the user does X, otherwise does this random thing".  And coming
>> from that standpoint, erroring out when there needs a guess involved
>> is certainly more predictable---it is a cop-out option for me in
>> areas of the system I do not have strong preferences.
>
> One thing that makes me nervous about this patch is that it is not just a
> change to git-submodule, but rather to git-parse-remote.  So it could
> affect other parts of the system, too, where a guess might not be as
> desirable.

That is exactly what I meant when I said "to make everything ... in
a consistent way, without breaking existing users who rely on the
current behaviour" (emphasis on *everything*).  Also, I was (and am)
reasonably sure that no such acceptable change exists in the "guess
harder and pick a remote randomly" direction.  Rather, I suspect
that a consistent solution would be to tighten things to error out
when in doubt, and correct submodule codepath that blindly uses
'origin' without erroring out by mistake, if that is the case (if
what Marc alluded to was true; I didn't check).

> The number of affected code paths is fortunately quite small, since this
> is updating the shell library, and most of the remote-handling code is
> written in C these days.

Which would mean that users of git-parse-remote will end up
deviating further from the norm if we allow patches that head in
this direction to continue.  That is one more reason to reject it.

> ...
> Should this be a submodule-only thing?

I'd rather not have any "submodule-only" thing; that would give us
one less inconsistency to worry about.  As Jens and Heiko both seem
to think "pick a remote randomly" is a bad approach, I am not so
worried about this discussion breaking areas outside submodules.

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Marc Branchaud]

On 12-06-19 04:12 PM, Jeff King wrote:
> On Tue, Jun 19, 2012 at 10:55:13AM -0700, Junio C Hamano wrote:
> 
>> Marc Branchaud <marcnarc@xiplink.com> writes:
>>
>>> On 12-06-18 06:12 PM, Junio C Hamano wrote:
>>> ...
>>>> That reliance of "origin" is what made me think that "not guessing
>>>> and blindly assuming" a wrong thing to do.
>>>
>>> I think git can do better than erroring out, though.
>>>
>>>> It is OK that your build usesdetached HEAD, but if that is the case
>>>> shoudln't it be the one deciding which specific remote it wants to
>>>> take the updated sources from, and telling Git to do so?
>>>
>>> Sure, but I feel it did that already when it cloned.  It seems reasonable for
>>> the submodules to default to using the remote specified when the super-repo
>>> was cloned.
>>
>> I do not have a strong opinion either way, other than that I would
>> prefer predictable behaviour over "works most of the time provided
>> if the user does X, otherwise does this random thing".  And coming
>> from that standpoint, erroring out when there needs a guess involved
>> is certainly more predictable---it is a cop-out option for me in
>> areas of the system I do not have strong preferences.
> 
> One thing that makes me nervous about this patch is that it is not just a
> change to git-submodule, but rather to git-parse-remote.  So it could
> affect other parts of the system, too, where a guess might not be as
> desirable.
> 
> The number of affected code paths is fortunately quite small, since this
> is updating the shell library, and most of the remote-handling code is
> written in C these days. But it raises a few questions:
> 
>   1. git-pull can call into get_default_remote via get_remote_merge_branch.
>      Is it impacted by this change?

I imagine it is.

>   2. We install git-parse-remote as part of the plumbing API. Do we know
>      of any other 3rd-party scripts that use this interface and might be
>      affected?

I certainly don't know of any.

>   3. The C code sets up remote.c:default_remote_name, which defaults to
>      "origin". Should this be consistent with what git-parse-remote
>      does?

Yes, I think it should.

I think you're raising some good points, but I'm not in a good position to
evaluate the impacts on git-pull or 3rd-party apps.

I suggest git would be better off changing the way it finds the default
remote to:

	Use the currently checked-out branch's remote;
	or Use the remote specified in the original clone command[*];
	or use "origin".

[*] With some strong mechanism for identifying this remote.

I think this would make the default-remote concept work properly in more
cases, while still maintaining compatibility with current assumptions
(because folks who are happy with the arbitrary choice of "origin" probably
never used -o in their clone commands anyway).

I'd be fine with erroring-out (or just having no default remote) instead of
using "origin", but I suspect that would cause more headaches than it solves.

I'm happy to cobble together a patch, if we agree to move in this direction.

		M.

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Jeff King]

On Tue, Jun 19, 2012 at 05:43:03PM -0400, Marc Branchaud wrote:

> I suggest git would be better off changing the way it finds the default
> remote to:
> 
> 	Use the currently checked-out branch's remote;
> 	or Use the remote specified in the original clone command[*];
> 	or use "origin".
> 
> [*] With some strong mechanism for identifying this remote.

Yes, that sounds like a much saner path. I think your [*] is just
"record the different name in remote.default during the clone".

Then we continue to use "origin" when that is not set (so existing repos
without "-o" see no change at all). New repos cloned with "-o" would be
fixed. Old repos cloned with "-o" are still broken, but there is at
least a simple one-time workaround ("git config remote.default foo").

-Peff

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jun 19, 2012 at 05:43:03PM -0400, Marc Branchaud wrote:
>
>> I suggest git would be better off changing the way it finds the default
>> remote to:
>> 
>> 	Use the currently checked-out branch's remote;
>> 	or Use the remote specified in the original clone command[*];
>> 	or use "origin".
>> 
>> [*] With some strong mechanism for identifying this remote.
>
> Yes, that sounds like a much saner path. I think your [*] is just
> "record the different name in remote.default during the clone".
>
> Then we continue to use "origin" when that is not set (so existing repos
> without "-o" see no change at all). New repos cloned with "-o" would be
> fixed. Old repos cloned with "-o" are still broken, but there is at
> least a simple one-time workaround ("git config remote.default foo").

Yeah, I can certainly buy that.

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Jeff King]

On Tue, Jun 19, 2012 at 02:58:38PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Tue, Jun 19, 2012 at 05:43:03PM -0400, Marc Branchaud wrote:
> >
> >> I suggest git would be better off changing the way it finds the default
> >> remote to:
> >> 
> >> 	Use the currently checked-out branch's remote;
> >> 	or Use the remote specified in the original clone command[*];
> >> 	or use "origin".
> >> 
> >> [*] With some strong mechanism for identifying this remote.
> >
> > Yes, that sounds like a much saner path. I think your [*] is just
> > "record the different name in remote.default during the clone".
> >
> > Then we continue to use "origin" when that is not set (so existing repos
> > without "-o" see no change at all). New repos cloned with "-o" would be
> > fixed. Old repos cloned with "-o" are still broken, but there is at
> > least a simple one-time workaround ("git config remote.default foo").
> 
> Yeah, I can certainly buy that.

It is also a step towards defining remote.defaultFetch and
remote.defaultPush if you wanted them to be different, something that
has come up in conversation a few times (e.g., when you treat a
read-only upstream as your origin, but publish elsewhere).

-Peff

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jun 19, 2012 at 02:58:38PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > On Tue, Jun 19, 2012 at 05:43:03PM -0400, Marc Branchaud wrote:
>> >
>> >> I suggest git would be better off changing the way it finds the default
>> >> remote to:
>> >> 
>> >> 	Use the currently checked-out branch's remote;
>> >> 	or Use the remote specified in the original clone command[*];
>> >> 	or use "origin".
>> >> 
>> >> [*] With some strong mechanism for identifying this remote.
>> >
>> > Yes, that sounds like a much saner path. I think your [*] is just
>> > "record the different name in remote.default during the clone".
>> >
>> > Then we continue to use "origin" when that is not set (so existing repos
>> > without "-o" see no change at all). New repos cloned with "-o" would be
>> > fixed. Old repos cloned with "-o" are still broken, but there is at
>> > least a simple one-time workaround ("git config remote.default foo").
>> 
>> Yeah, I can certainly buy that.
>
> It is also a step towards defining remote.defaultFetch and
> remote.defaultPush if you wanted them to be different, something that
> has come up in conversation a few times (e.g., when you treat a
> read-only upstream as your origin, but publish elsewhere).

Yeah, we are on the same page on that one.

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Jens Lehmann]

Am 19.06.2012 16:07, schrieb Marc Branchaud:
> On 12-06-18 06:12 PM, Junio C Hamano wrote:
>> Marc Branchaud <marcnarc@xiplink.com> writes:
>>> That would be bad for our situation.  As I said, our automated build system
>>> uses detached HEADs a lot.  Erroring-out in this case would break us.  It's
>>> really only the near-ubiquity of the name "origin" that has kept things
>>> working so far.

And the "submodule add" documentation clearly talks about relative
submodule URLs being relative to the superproject's origin.

Your buildbot could also check if an origin is configured and use the
magic in your patch to configure one to the URL of the first remote it
finds if it isn't before initializing the submodules.

>> That reliance of "origin" is what made me think that "not guessing
>> and blindly assuming" a wrong thing to do.
> 
> I think git can do better than erroring out, though.

Hmm, but guessing and using the first remote it finds (which might or
might not be the one used in the initial clone) doesn't sound like a
terribly good idea.

> Sure, but I feel it did that already when it cloned.  It seems reasonable for
> the submodules to default to using the remote specified when the super-repo
> was cloned.

Is there a way to reliably tell that remote without relying e.g. on
the implementation details of git config (e.g. it could sort remotes
alphabetically some day)? What happens if someone changes the config
later? A lot of ambiguity here ...

And I think origin should always be the second choice if it exists,
the first being the remote configured for the checked out branch.
This gives the user the opportunity to say "Oh, I screwed up using
'git clone -o', let's set origin to the upstream repo". But should we
try to guess the remote the superproject was cloned from as third
option? I am not convinced.

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Marc Branchaud]

On 12-06-19 03:41 PM, Jens Lehmann wrote:
> Am 19.06.2012 16:07, schrieb Marc Branchaud:
>> On 12-06-18 06:12 PM, Junio C Hamano wrote:
>>> Marc Branchaud <marcnarc@xiplink.com> writes:
>>>> That would be bad for our situation.  As I said, our automated build system
>>>> uses detached HEADs a lot.  Erroring-out in this case would break us.  It's
>>>> really only the near-ubiquity of the name "origin" that has kept things
>>>> working so far.
> 
> And the "submodule add" documentation clearly talks about relative
> submodule URLs being relative to the superproject's origin.

This whole thing seems a bit weird...

So user A adds a submodule with <repository> "../others/thing.git".  Clearly
user A has some remote in mind when they added this submodule.

But consider user B, who cloned the super-repo from the same remote that user
A had in mind when creating the submodule.  If user B then checks out a
non-tracking branch (or a branch that tracks a different remote) and then
tries to initialize/update the submodule, user B will get an error.

To me this is clearly wrong.  It's also wrong to error-out and expect the
user to fix it.  Should the user temporarily set their branch's remote to the
right place, initialize the submodule, then undo the branch setting?  Should
the user check our a branch that tracks the correct remote, initialize the
submodule, then check out their branch?  Both of those "solutions" look
pretty weak to me.

I'm starting to think that maybe "git submodule init" (and "update") should
learn a --remote option.  That way at least user B could tell git what to do.

> Your buildbot could also check if an origin is configured and use the
> magic in your patch to configure one to the URL of the first remote it
> finds if it isn't before initializing the submodules.

Yes, it seems my assumptions about how to determine the default remote
shouldn't be coded into git.  But dang it, the current fallback to "origin"
is really lame.

>>> That reliance of "origin" is what made me think that "not guessing
>>> and blindly assuming" a wrong thing to do.
>>
>> I think git can do better than erroring out, though.
> 
> Hmm, but guessing and using the first remote it finds (which might or
> might not be the one used in the initial clone) doesn't sound like a
> terribly good idea.

Fair enough, but I still think it's better than guessing that the right
remote is "origin".  :)

>> Sure, but I feel it did that already when it cloned.  It seems reasonable for
>> the submodules to default to using the remote specified when the super-repo
>> was cloned.
> 
> Is there a way to reliably tell that remote without relying e.g. on
> the implementation details of git config (e.g. it could sort remotes
> alphabetically some day)? What happens if someone changes the config
> later? A lot of ambiguity here ...

Yes, I agree.

Should there perhaps be some kind of "cloned from this remote" setting in the
config?

> And I think origin should always be the second choice if it exists,
> the first being the remote configured for the checked out branch.

Do you mean "the origin(al) remote repository" or just "the remote named
'origin'"?

> This gives the user the opportunity to say "Oh, I screwed up using
> 'git clone -o', let's set origin to the upstream repo". But should we
> try to guess the remote the superproject was cloned from as third
> option? I am not convinced.

Maybe I'm misinterpreting you.  Are you attaching a special meaning to a
remote named "origin"?

		M.

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Phil Hord]

I mostly want to say "Amen, brother", so if you are not interested in
a me-too, then feel free to skip this reply.

On Tue, Jun 19, 2012 at 5:43 PM, Marc Branchaud <marcnarc@xiplink.com> wrote:
>
> On 12-06-19 03:41 PM, Jens Lehmann wrote:
> > Am 19.06.2012 16:07, schrieb Marc Branchaud:
> >> On 12-06-18 06:12 PM, Junio C Hamano wrote:
> >>> Marc Branchaud <marcnarc@xiplink.com> writes:
> >>>> That would be bad for our situation.  As I said, our automated build
> >>>> system
> >>>> uses detached HEADs a lot.  Erroring-out in this case would break us.
> >>>>  It's
> >>>> really only the near-ubiquity of the name "origin" that has kept
> >>>> things
> >>>> working so far.
> >
> > And the "submodule add" documentation clearly talks about relative
> > submodule URLs being relative to the superproject's origin.
>
> This whole thing seems a bit weird...
>
> So user A adds a submodule with <repository> "../others/thing.git".
>  Clearly
> user A has some remote in mind when they added this submodule.
>
> But consider user B, who cloned the super-repo from the same remote that
> user
> A had in mind when creating the submodule.  If user B then checks out a
> non-tracking branch (or a branch that tracks a different remote) and then
> tries to initialize/update the submodule, user B will get an error.

This specific situation is what makes me think that picking any
arbitrary remote should work in most cases.  I think this is why
picking 'origin' works just fine now, except in the special case of
"no remote named origin and no tracking branch".  If I'm using
relative urls, I've got to have my associated submodules available at
each remote I might push to or clone from.  Otherwise I'm just
shooting my feet for fun.

> To me this is clearly wrong.  It's also wrong to error-out and expect the
> user to fix it.  Should the user temporarily set their branch's remote to
> the
> right place, initialize the submodule, then undo the branch setting?
>  Should
> the user check our a branch that tracks the correct remote, initialize the
> submodule, then check out their branch?  Both of those "solutions" look
> pretty weak to me.
>
> I'm starting to think that maybe "git submodule init" (and "update")
> should
> learn a --remote option.  That way at least user B could tell git what to
> do.

I like this one.

> > Your buildbot could also check if an origin is configured and use the
> > magic in your patch to configure one to the URL of the first remote it
> > finds if it isn't before initializing the submodules.
>
> Yes, it seems my assumptions about how to determine the default remote
> shouldn't be coded into git.  But dang it, the current fallback to
> "origin"
> is really lame.

Amen!

I also think the assumption of "same as the current tracking branch"
is lame, too.  (But I have to admit, it works better than 95% of the
time, and it's easier than erroring out.  If it works for Apple, it
should work here.)

>
> >>> That reliance of "origin" is what made me think that "not guessing
> >>> and blindly assuming" a wrong thing to do.
> >>
> >> I think git can do better than erroring out, though.
> >
> > Hmm, but guessing and using the first remote it finds (which might or
> > might not be the one used in the initial clone) doesn't sound like a
> > terribly good idea.
>
> Fair enough, but I still think it's better than guessing that the right
> remote is "origin".  :)
>
> >> Sure, but I feel it did that already when it cloned.  It seems
> >> reasonable for
> >> the submodules to default to using the remote specified when the
> >> super-repo
> >> was cloned.
> >
> > Is there a way to reliably tell that remote without relying e.g. on
> > the implementation details of git config (e.g. it could sort remotes
> > alphabetically some day)? What happens if someone changes the config
> > later? A lot of ambiguity here ...
>
> Yes, I agree.
>
> Should there perhaps be some kind of "cloned from this remote" setting in
> the
> config?

I'm not sure I like the "give preference to my original clone source"
idea.  Who said it could be king? What if it was temporary and I've
since moved on to a new server? Well, yeah, I can reconfigure my
default.  So I guess I do like it a little.

>
> > And I think origin should always be the second choice if it exists,
> > the first being the remote configured for the checked out branch.
>
> Do you mean "the origin(al) remote repository" or just "the remote named
> 'origin'"?
>
> > This gives the user the opportunity to say "Oh, I screwed up using
> > 'git clone -o', let's set origin to the upstream repo". But should we
> > try to guess the remote the superproject was cloned from as third
> > option? I am not convinced.
>
> Maybe I'm misinterpreting you.  Are you attaching a special meaning to a
> remote named "origin"?
>
>                M.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Try harder to find a remote when on a detached HEAD or non-tracking branch.

[Arnaud Lacombe]

Hi,

On Mon, Jun 18, 2012 at 1:01 PM,  <marcnarc@xiplink.com> wrote:
> From: Marc Branchaud <marcnarc@xiplink.com>
>
> get_default_remote() tries to use the checked-out branch's 'remote' config
> value to figure out the remote's name.  This fails if there is no currently
> checked-out branch (i.e. HEAD is detached) or if the checked-out branch
> doesn't track a remote.  In these cases and the function would just fall
> back to "origin".
>
> Instead, let's use the first remote listed in the configuration, and fall
> back to "origin" only if we don't find any configured remotes.
>
> Prior to this change, git would fail to initialize a relative-path
> submodule if the super-repo was on a detached HEAD and it had no remote
> named "origin".
>
Marc, could you explain the problem you were trying to solve ? You are
only giving hints about the change made, not the reason behind it.

Thanks,
 - Arnaud

> Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
> ---
>
> Our build system likes to use detached HEADs, so we got tripped up when we
> started using relative submodule URLs.
>
> (I'm not sure about the portability of my change, or if I should wrap it
> to 80 columns...)
>
>  git-parse-remote.sh        |  1 +
>  t/t7400-submodule-basic.sh | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+)
>
> diff --git a/git-parse-remote.sh b/git-parse-remote.sh
> index 484b2e6..225ad94 100644
> --- a/git-parse-remote.sh
> +++ b/git-parse-remote.sh
> @@ -8,6 +8,7 @@ get_default_remote () {
>        curr_branch=$(git symbolic-ref -q HEAD)
>        curr_branch="${curr_branch#refs/heads/}"
>        origin=$(git config --get "branch.$curr_branch.remote")
> +       test -z "$origin" && origin=$(git config --list | grep '^remote\.' | head -1 | awk -F . '{print $2}')
>        echo ${origin:-origin}
>  }
>
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 81827e6..8f1ff4f 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -507,6 +507,28 @@ test_expect_success 'relative path works with user@host:path' '
>        )
>  '
>
> +test_expect_success 'relative path works on detached HEAD and remote is not named "origin"' '
> +       mkdir detachtest &&
> +       (
> +               cd detachtest &&
> +               git init &&
> +               mkdir sub &&
> +               (
> +                       cd sub &&
> +                       git init &&
> +                       test_commit foo
> +               ) &&
> +               git add sub &&
> +               git commit -m "added sub" &&
> +               git checkout HEAD@{0} &&
> +               git config -f .gitmodules submodule.sub.path sub &&
> +               git config -f .gitmodules submodule.sub.url ../subrepo &&
> +               git remote add awkward ssh://awkward/repo &&
> +               git submodule init sub &&
> +               test "$(git config submodule.sub.url)" = ssh://awkward/subrepo
> +       )
> +'
> +
>  test_expect_success 'moving the superproject does not break submodules' '
>        (
>                cd addtest &&
> --
> 1.7.11.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html