patchRe: [PATCH] Make git-clone --use-separate-remote the default

and --use-immingled-remote can be used to get the original behaviour; it is also implied by --bare.

We get confused, frustrated and data-losing users *daily* on #git now because git-clone still produces the crippled repositories having the remote and local heads freely mixed together.

Signed-off-by: Petr Baudis <pasky@suse.cz>
---

 Documentation/git-clone.txt |   20 ++++++++++++++------
 git-clone.sh                |   14 +++++++-------
 2 files changed, 21 insertions(+), 13 deletions(-)

Petr Baudis <pasky@suse.cz> writes:

What's immingled?

Being strongly opinionated, not giving enough credit for the evolutionary process behind the history and venting frustration in the proposed commit log message is never a good strategy to get the patch applied.

Even though I fully agree that use-separate-remotes should be the default, to the point that I do not think we do not even need a backward compatibility option. People who want to use traditional layout for simple one-remote-branch-only project would not suffer anyway because 'origin' still means origin in the new layout (refs/remotes/origin/HEAD).

We would need to update the tutorials to match this,though. I think it talks about the traditional layout and say 'See, now you can run "ls .git/refs/heads/{master,origin}"' or something like that.

Junio C Hamano wrote:

Had me reaching for the dictionary too. Seems to mean the same as intermingled which would be my choice ...

On Fri, Nov 24, 2006 at 12:12:10AM CET, Junio C Hamano wrote:

One dictionary says

   Immingle \Im*min"gle\, v. t.
      To mingle; to mix; to unite; to blend. [R.] --Thomson.

but perhaps it's too much an obscure word... better suggestions welcomed.

Yes, sorry, the last days were a bit tiring to me.

I'm not sure what evolutionary process should I describe, though...

I don't know, we still at least need to keep the functionality for --bare.

Oops, yes. I can try to go through the tutorials during tomorrow or the next week...

On Fri, Nov 24, 2006 at 12:42:03AM +0100, Petr Baudis wrote:

commingled? legacy? flat?

Petr Baudis <pasky@suse.cz> writes:

I agree --bare should continue to be a "snapshot mirror"; I am not advocating for the removal of the internal implementation detail such as $use_separate_remote variable.

However, I think having one sane behaviour is the right thing to do for a clone that prepares a repository with a working tree (including the one made with -n option, which only means "do not do the check-out immediately after cloning" for such a repository).

The traditional layout is slightly simpler for a project with the simplest needs (that is, a single upstream repository that has a single 'master' branch), but I do think even that is not an advantage anymore.

With the separate-remote layout, git-fetch would still fetch and update the "origin" (although that is now remotes/origin/master which is pointed at by remotes/origin/HEAD) and the user can still refer to it with "origin". Commands "git-pull origin", "git-pull . origin", and "git-merge origin" all will continue to work the same way as before for such a project as in the traditional layout, and that is why I think we do not need backward compatibility flag in this case.

Junio C Hamano <junkio@cox.net> writes:

Just to let you know, I'll take the patch almost as is (even with the --use-immingled-remote), except with a slight rewording in the documentation to warn people that the backward compatibility option will be removed before the next major release.

However, this simple command fails:

	$ git push $URL master

if the target repository $URL is made with use-separate-remote.

This is because 'master' matches more than one on the remote side (heads/master and remotes/origin/master) which triggers "Hey, that's ambiguous, make yourself clear which one you mean!" check. This breaks t5400 test. We could "fix" the test to make it more explicit, but that is just a workaround.

I think the send-pack/receive-pack pair needs to be taught that an unadorned branch name 'master' never matches anything under refs/remotes. This means that it would require an explicit refspec heads/master:remotes/origin/master in order to pudate refs under refs/remotes on the remote side with a push. I do not think that is a big problem, because the normal patch-flow for shared repository workflow is:

	remote			local

		      (fecth)
	heads/master	--->	remotes/origin/master ---.
							 | (merge)
	heads/master	<---	heads/master	      <--'

and pushing into remotes/origin/* is not a norm.

The function to fix is connect.c::match_explicit_refs() and I _think_ making connect.c::count_refspec_match() not to consider 'foo' to match 'refs/remotes/origin/foo' (but still keeping it to match 'refs/heads/foo' or 'refs/tags/foo') is enough to make this happen.

This brings up two related issues. Currently we automatically prepare "Pull: refs/heads/$branch:refs/remotes/origin/$branch" for all branches that exists at the remote site when a clone happens. Andy Parkins has a patch to allow a glob pattern to be there, like this [*1*]:

	Pull: refs/heads/*:refs/remotes/origin/*

which makes sense, and we might want to have this as the default after the clone [*2*].

Another is if we might want to add "Push: " entry in the default after the clone. I am a bit reluctant to make the default setup too specific to CVS style "central shared repo" workflow, but any stupid default would not suit people with truly distributed workflow anyway, so it might be fine.

[Footnotes]

*1* I rewrote the patch because I wanted to deal with the
    fallout from recent packed-refs work at the same time.  So bugs
    in the counter-proposal patch is mine while the credit for the
    initiative and the idea goes to Andy.

*2* I think the fetch wildcarding has an issue with what remote
    head to merge when used with "git pull".  I think it should
    use the one that is pointed at by refs/remotes/origin/HEAD,
    but there is no code for that yet.  Hints, hints...

Junio C Hamano <junkio@cox.net> writes:

That is,...

-- >8 -- [PATCH] refs outside refs/{heads,tags} match less strongly.

This changes the refname matching logic used to decide which ref is updated with git-send-pack. We used to error out when pushing 'master' when the other end has both 'master' branch and a tracking branch 'remotes/$name/master' but with this, 'master' matches only 'refs/heads/master' when both and no other 'master' exist.

Pushing 'foo' when both heads/foo and tags/foo exist at the remote end is still considered an error and you would need to disambiguate between them by being more explicit.

When neither heads/foo nor tags/foo exists at the remote, pushing 'foo' when there is only remotes/origin/foo is not ambiguous, while it still is ambiguous when there are more than one such weaker match (remotes/origin/foo and remotes/alt/foo, for example).

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

Junio C Hamano wrote:

git-push.1 has following description:

    Some short-cut notations are also supported.

              o   tag <tag> means the same as refs/tags/<tag>:refs/tags/<tag>.

              o  A parameter <ref> without a colon is equivalent to
                 <ref>:<ref>, hence updates <ref>  in
                 the destination from <ref> in the source.

Maybe this is only my reading of manual page, but I understood it like it does not leave the room for ambiguity, because it is using _the same_ refspec as the local one.

That's why, when I do

   git-push repo x

and it results in

   git-push repo refs/heads/x:refs/remotes/origin/x

instead of expected

   git-push repo refs/heads/x:refs/heads/x

just because the remote repo did not have refs/heads/x, but happened to have refs/remotes/origin/x, would be highly surprising to me.

The expected behaviour on 'git-push repo x' in my understanding is
1) git finds the exact reference for 'x' (i.e. either refs/heads/x or
refs/tags/x) according to local lookup rules
2) git uses the found reference _unambiguously_ to create or update exactly the
same reference in the remote repo.

Am I the only one to have this understanding?

Salikh Zakirov <Salikh.Zakirov@Intel.com> writes:

If you write

	git push $remote tag $string

it is handled exactly as if you wrote:

	git push $remote refs/tags/${string}:refs/tags/${string}

and if you write

	git push $remote master

it is handled exactly as if you wrote:

	git push $remote master:master

The manual correctly describes the above, but the issue the fix addresses is about what happens to that 'master' string that follows the colon, and the 'master' string becomes ambiguous if the remote end uses separate-remote layout.

The way this command:

	git push $remote $src:$dst

is handled is:

 (0) send-pack gets ls-remote equivalent from the remote.  This
     tells us the set of refs the remote has and the value of
     each of them.

 (1) $src can be a ref that is resolved locally the usual way.
     You could have any valid SHA-1 expression (e.g. HEAD~6).

 (2) $dst is compared with the list of refs that the remote
     has, and unique match is found.  So if the set of refs the
     remote side has:

	refs/heads/origin
        refs/heads/master
        refs/tags/v1.0.0

     and if $dst is 'master', refs/heads/master is what will be
     updated.

 (3) Then send-pack generates and sends the necessary pack to
     update the remote side with objects needed for $src, using
     the knowledge of what the remote has.  Also, send-pack
     instructs the remote to update which ref with what value.
     Continuing with the example, it tells the remote to update
     its refs/heads/master with the value of our 'master'.

That *matching* in step (2) is what the fix is about.

The matching code of send-pack from the beginning has been the unique tail-match. When the other end had a branch 'bugfix' and a tag 'bugfix', then both of them would match because both refs/heads/bugfix and refs/tags/bugfix ends with 'bugfix' ('gfix' does not match 'refs/heads/bugfix' -- we are not that stupid ;-).

So you had to disambiguate this case by saying heads/bugfix if you want to push the branch. That was fine between branches and tags, since having a branch and a tag with the same name is usually not done in order to keep user's sanity.

However, separate-remote layout poses a more serious problem, because most of the time you would expect to see similar names under refs/heads/ and refs/remotes/origin/ directories. If we kept the original ref matching code, a cloned remote would have both refs/heads/master and refs/remotes/origin/master almost always, so somebody who is pushing 'master' to such a remote would have had to disambiguate it by saying:

	git push heads/master

which is (as described in the part of the manual you quoted) a shorthand for

	git push heads/master:heads/master

'heads/master' before the colon is used to find out which commit in your local repository we are pushing, and 'heads/master' after the colon is used to match against the list of refs from the remote (which contains both 'refs/heads/master' and 'refs/remotes/origin/master'), and only because the user said 'heads/master' (not just 'master') this avoids ambiguity.

Even under separate-remote layout, we would want to be able to say:

	git push master

to mean we want to push to remote's heads/master when the remote has remotes/{origin,blech}/master.

And that is what the fix is about.

Junio C Hamano wrote:

Indeed, the manual describes it correctly. My point is that this semantics fairly complex and easy to understand incorrectly.

I agree with your main point that 'git push master' should "just work" for all existing and new repositories, however, it is very confusing that 'git push master' can update something other than refs/heads/master, depending on the refs existing in the remote repo.

Junio C Hamano wrote:

I think that remote matching semantics is confusing, and the following change would make understanding easier.

I was understanding the manual incorrectly for a long time until you've explained its true meaning today (thanks!).

As a side effect, making 'git push repo master' unambiguously expanded to 'git push repo refs/heads/master:refs/heads/master' will make the syntax 'git push repo tag v1' unneeded at all, because it would be exactly the same as 'git push repo v1' (expanded to 'git push repo refs/tags/v1:refs/tags/v1').

--- connect.c +++ connect.c

Salikh Zakirov <salikh@gmail.com> writes:

Hmm.  I think this is somewhat wrong.

Have you tested the patch with repositories with existing refs?

You do not seem to check if that fabricated matched_dst exists on the other side, so matched_dst lacks "where was this ref initially" information (aka old_sha1), if I am reading your patch correctly. Wouldn't that mean that you would confuse the fast-forward check logic?

One setup I have that would be broken with this change is that the remote end has refs/heads/up/obsd and no refs/heads/obsd, and local end has refs/heads/obsd. This is to work on portability fix for OpenBSD. With the current git-push, I think

	git push $remote_openbsd_box obsd

would correctly update the remote refs/heads/up/obsd with the local tip of the obsd branch, so that then I can ssh into the remote and say "git merge up/obsd" to continue on that OpenBSD machine from where I left off on the local, non-OpenBSD machine.

I am not sure if people would mind breaking existing setups like this.

By the way, there are other glitches with the current git-push (rather, git-send-pack) that we need to tighten. For example:

	git push $remote HEAD~6

does not error out as it should. 'HEAD~6' is expanded to 'HEAD~6:HEAD~6'; its left hand side is valid (6 revs before the tip is used to update the remote side) but its right hand side is not checked for validity ("HEAD~6" is not a valid refname) and creates .git/HEAD~6 at the remote end which is completely bogus.

On Fri, 24 Nov 2006 13:14:00 +0300 Salikh Zakirov wrote:

BTW, this is broken (and was broken even in 1.4.3.x):

$ mkdir ~/tmp/test_repo $ ( cd ~/tmp/test_repo; git-init-db ) defaulting to local storage area $ git push ~/tmp/test_repo tag v1.4.4.1 error: src refspec tag does not match any. error: dst refspec tag does not match any existing ref on the remote and does not start with refs/. fatal: unexpected EOF

Omitting the "tag" word works:

$ git push ~/tmp/test_repo v1.4.4.1
updating 'refs/tags/v1.4.4.1'
  from 0000000000000000000000000000000000000000
  to   21dff5f4982333d694d105595a701540d4d0d1db
Generating pack...
Done counting 28130 objects.
Deltifying 28130 objects.
 100% (28130/28130) done
Writing 28130 objects.
 100% (28130/28130) done
Total 28130, written 28130 (delta 19344), reused 27628 (delta 18891)
refs/tags/v1.4.4.1: 0000000000000000000000000000000000000000 -> 21dff5f4982333d694d105595a701540d4d0d1db

Seems that nobody really uses the "tag NAME" syntax...

Such interpretation would indeed be horrible, but I'm afraid this is exactly the case now:

$ mkdir ~/tmp/test_repo
$ ( cd ~/tmp/test_repo; git-init-db )
defaulting to local storage area
$ git push ~/tmp/test_repo v1.4.0^0:refs/remotes/origin/master
updating 'refs/remotes/origin/master' using 'v1.4.0^0'
  from 0000000000000000000000000000000000000000
  to   41292ddd37202ff6dce34986c87a6000c5d3fbfa
Generating pack...
Done counting 19857 objects.
Deltifying 19857 objects.
 100% (19857/19857) done
Writing 19857 objects.
 100% (19857/19857) done
Total 19857, written 19857 (delta 13472), reused 19038 (delta 12884)
refs/remotes/origin/master: 0000000000000000000000000000000000000000 -> 41292ddd37202ff6dce34986c87a6000c5d3fbfa

$ git push ~/tmp/test_repo master
updating 'refs/remotes/origin/master' using 'refs/heads/master'
  from 41292ddd37202ff6dce34986c87a6000c5d3fbfa
  to   e945f95157c2c515e763ade874931fc1eb671a0b
Generating pack...
Done counting 8667 objects.
Result has 8278 objects.
Deltifying 8278 objects.
 100% (8278/8278) done
Writing 8278 objects.
 100% (8278/8278) done
Total 8278, written 8278 (delta 5924), reused 7396 (delta 5065)
refs/remotes/origin/master: 41292ddd37202ff6dce34986c87a6000c5d3fbfa -> e945f95157c2c515e763ade874931fc1eb671a0b

BTW, I cannot find the description of the matching algorithm used by connect.c:count_refspec_match() anywhere in the git-push or git-fetch man page, and I cannot understand why this algorithm is different from the default search order ($name, refs/$name, refs/tags/$name, refs/heads/$name, refs/remotes/$name, refs/remotes/$name/HEAD).

The problem is that "$x" and "$x:$x" would be not equivalent anymore, unless we add a special case for "$x:$y" where $x == $y - hmm, but the current code seems to have that special case:

			else if (!strcmp(rs[i].src, rs[i].dst) &&
				 matched_src) {
				/* pushing "master:master" when
				 * remote does not have master yet.
				 */

(but that code triggers only in case we did not find any matching ref in the destination repo).

Sergey Vlasov <vsu@altlinux.ru> writes:

I think this was broken when git-push was made a built-in, and the documentation was not updated.

I use only tags in vN.M.L.. format and Linus does so too, so probably that was one of the reasons why this was not noticed for quite some time.

Fixes welcome, preferably to the builtin-push.c not to the documentation.

Junio C Hamano wrote:

By the way, I think the backward compatibility option should be simply named --dont-use-separate-remote, or --without-separate-remote, or --no-separate-remote (the last is probably the best choice).

 
The exception being that with --use-separate-remote you cannot checkout
tracking branches to see what it is there (at least for now, but IIRC we
want to relax this constraint; i.e. to forbid commiting to non-heads,
instead of forbidding checking out), you cannot use it as alternate
source (as alternate repo to check from) while still allowing to work
on it, and that gitweb doesn't show anything except heads and tags;
it doesn't show remotes.

By the way, does new "git peek-remote -a ." show anything except refs/heads/, refs/tags/ and refs/remotes (e.g. StGit refs/bases/ and refs/patches/)?

Petr Baudis wrote:

This description does not apply to repositories which do not have 'master' branch. Maybe "only the HEAD branch of remote repository, where HEAD is the branch designated as main branch in repository".