warning: no common commits - slow pull

A couple of hours ago I pulled my reference copy of Linux tree, which brought the tip here:

commit 7cf712db6087342e5e7e259d3883a7b5ac3212d1
Merge: 58a14ee... 30ddb15...
Author: Linus Torvalds <[email protected]>
Date: Sun Feb 10 12:03:57 2008 -0800

Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-2.6

Then, 10 minutes ago I did a pull to bring the head here:

commit 19af35546de68c872dcb687613e0902a602cb20e

Author: Linus Torvalds <[email protected]>
Date:   Sun Feb 10 14:18:14 2008 -0800

    Linux 2.6.25-rc1

But this second pull seems to have re-downloaded 172MB, when it should have only needed the last few commits.

thanks,
-Len

[lenb@d975xbx2 linus (master)]$ git pull
remote: Counting objects: 447, done.
remote: Compressing objects: 100% (39/39), done.
remote: Total 328 (delta 291), reused 325 (delta 289) Receiving objects: 100% (328/328), 60.81 KiB, done. Resolving deltas: 100% (291/291), completed with 97 local objects.
warning: no common commits
remote: Counting objects: 708151, done.
remote: Compressing objects: 100% (124656/124656), done.
remote: Total 708151 (delta 587427), reused 702559 (delta 582537) Receiving objects: 100% (708151/708151), 172.17 MiB | 1251 KiB/s, done. Resolving deltas: 100% (587427/587427), done.
From git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6
* [new tag] v2.6.25-rc1 -> v2.6.25-rc1
Updating 7cf712d..19af355
Fast forward

 Makefile                                  |    4 +-
 arch/arm/configs/omap_h2_1610_defconfig   |  201 +++++---
 arch/arm/configs/omap_osk_5912_defconfig  |  110 ++--
 arch/arm/configs/orion_defconfig          |   93 ++--
 arch/arm/kernel/setup.c                   |    2 +-
 arch/arm/mach-at91/clock.c                |    2 +-
 arch/arm/mach-davinci/clock.c             |    4 +-
 arch/arm/mach-omap1/Makefile              |    6 +-
 arch/arm/mach-omap1/board-ams-delta.c     |    7 +-
 arch/arm/mach-omap1/board-fsample.c       |    6 +-
 arch/arm/mach-omap1/board-generic.c       |   24 +-
 arch/arm/mach-omap1/board-h2-mmc.c        |  110 ++++
 arch/arm/mach-omap1/board-h2.c            |  103 ++---
 arch/arm/mach-omap1/board-h3-mmc.c        |  114 ++++
 arch/arm/mach-omap1/board-h3.c            |   75 +--
 arch/arm/mach-omap1/board-innovator.c     |   19 +-
 arch/arm/mach-omap1/board-nokia770.c      |    3 +-
 arch/arm/mach-omap1/board-osk.c           |   25 +-
 arch/arm/mach-omap1/board-palmte.c        |   84 +--
 arch/arm/mach-omap1/board-palmtt.c        |   15 +-
 arch/arm/mach-omap1/board-palmz71.c       |    3 +-
 arch/arm/mach-omap1/board-perseus2.c      |    8 +-
 arch/arm/mach-omap1/board-sx1-mmc.c       |  124 +++++
 arch/arm/mach-omap1/board-sx1.c           |   64 +--
 arch/arm/mach-omap1/board-voiceblue.c     |    4 +-
 arch/arm/mach-omap1/clock.c               |    7 +-
 arch/arm/mach-omap1/leds-osk.c            |    4 +-
 arch/arm/mach-omap1/mailbox.c             |   14 +-
 arch/arm/mach-omap1/pm.c                  |   27 +-
 arch/arm/mach-omap1/sleep.S               |  161 ------
 arch/arm/mach-orion/addr-map.c            |   14 +-
 arch/arm/mach-orion/common.c              |   84 ++-
 arch/arm/mach-orion/common.h              |    8 +
 arch/arm/mach-orion/db88f5281-setup.c     |    4 +-
 arch/arm/mach-orion/dns323-setup.c        |    8 +-
 arch/arm/mach-orion/kurobox_pro-setup.c   |   17 +-
 arch/arm/mach-orion/pci.c                 |   10 +-
 arch/arm/mach-orion/rd88f5182-setup.c     |   17 +-
 arch/arm/mach-orion/ts209-setup.c         |   20 +-
 arch/arm/mach-pxa/pxa3xx.c                |   10 +-
 arch/arm/plat-omap/Makefile               |    1 +
 arch/arm/plat-omap/dma.c                  |  844 +++++++++++++++++++++++++++--
 arch/arm/plat-omap/dmtimer.c              |  118 +++-
 arch/arm/plat-omap/gpio.c                 |  256 +++++++---
 arch/arm/plat-omap/i2c.c                  |  148 +++++
 arch/arm/plat-omap/mcbsp.c                |   11 +
 arch/ia64/pci/pci.c                       |   25 +-
 arch/ia64/sn/pci/tioce_provider.c         |   16 +-
 arch/x86/kernel/quirks.c                  |    2 +-
 arch/x86/pci/common.c                     |   25 +-
 arch/x86/pci/direct.c                     |    4 +-
 arch/x86/pci/fixup.c                      |    6 +-
 arch/x86/pci/legacy.c                     |    2 +-
 arch/x86/pci/mmconfig-shared.c            |   41 +--
 arch/x86/pci/mmconfig_32.c                |   20 +-
 arch/x86/pci/mmconfig_64.c                |   18 +-
 arch/x86/pci/pci.h                        |   22 +-
 arch/x86/pci/visws.c                      |    3 -
 drivers/acpi/osl.c                        |   25 +-
 include/asm-arm/arch-omap/board-apollon.h |    2 +
 include/asm-arm/arch-omap/board-h2.h      |    3 +
 include/asm-arm/arch-omap/board-h3.h      |    2 +
 include/asm-arm/arch-omap/board-sx1.h     |    8 +-
 include/asm-arm/arch-omap/common.h        |   11 +
 include/asm-arm/arch-omap/cpu.h           |  127 ++++-
 include/asm-arm/arch-omap/dma.h           |  135 ++++--
 include/asm-arm/arch-omap/gpio.h          |    4 +
 include/asm-arm/arch-omap/irqs.h          |    2 +
 include/asm-arm/arch-omap/nand.h          |   24 +
 include/asm-arm/arch-orion/debug-macro.S  |    9 +-
 include/asm-arm/arch-orion/entry-macro.S  |    4 +-
 include/asm-arm/arch-orion/hardware.h     |   13 +-
 include/asm-arm/arch-orion/orion.h        |  102 +++--
 include/asm-arm/arch-orion/uncompress.h   |   14 +-
 include/asm-arm/arch-orion/vmalloc.h      |    2 +-
 include/linux/pci.h                       |   16 +-
 76 files changed, 2581 insertions(+), 1099 deletions(-)
 create mode 100644 arch/arm/mach-omap1/board-h2-mmc.c
 create mode 100644 arch/arm/mach-omap1/board-h3-mmc.c
 create mode 100644 arch/arm/mach-omap1/board-sx1-mmc.c
 create mode 100644 arch/arm/plat-omap/i2c.c
 create mode 100644 include/asm-arm/arch-omap/nand.h

[lenb@d975xbx2 linus (master)]$
[lenb@d975xbx2 acpi (release)]$ git --version
git version 1.5.4.1.34.g94bf

Yeah, I have this problem very often when I push to the ext4 tree on master.kernel.org. Apparently the push/pull logic isn't smart about objects are found via objects/info/alterntaes, so it will needlessly transfer objects that it doesn't need to.

What I do to deal with this problem is I'll manually log into master.kernel.org, and then use the command "git-update-ref refs/heads/origin 19af35546de68c872dcb687613e0902a602cb20e", and then go back and do the push/pull. Once there is a head which points to the latest from Linus, then the push/pull logic is smart and will only download the few commitments that aren't in the local git repository and aren't found in a shared repository.

Annoying, but as long as you have shell access on the machine with the destination repository, you can work around it.

- Ted

yeah, I think I have see this with pushes onto kernel.org also, but unlike Ted, I simply wait.

-Len

I am aware of that "push" side thing (basically it does not do the negotiation and unless you are always doing fast-forward pushes it tends to send needless stuff), but I had an impression that the issue Len is raising is different. Namely if you pull from Linus twice into the same tree you should never see that "No common commits".

Yeah, when I saw your response to him I realized that. I didn't notice the "No common commits" message in his transcript, and assumed he was referring to the problem I was describing.

I wouldn't mind waiting myself, but it does result in uneeded objects in the destination repository, which I gather results in slightly more disk load on the kernel.org servers since there's slightly less object sharing, and sometimes I'm pushing from behind a slow link (such as an EVDO wireless modem), and pushing the few megabytes worth of shared objects can take a while. So I've just always gotten in the habit of shelling into master.kernel.org and manually doing the git-update-ref; one of these days I'll get around to scripting it.

- Ted

it happened again.

this morning I pulled linus' tree up through
4ee29f6a52158cea526b16a44ae38643946103ec

then during the day, linus declared "rc2".

and now I pulled linus' tree again,
which has a HEAD now of

101142c37be8e5af9b847860219217e6b958c739

and the pull sucked down 172 MB even though the uncompressed diff between the two is 0.3 MB.

-Len

[lenb@d975xbx2 linus (master)]$ git pull
remote: Counting objects: 649, done.
remote: Compressing objects: 100% (106/106), done.
remote: Total 513 (delta 417), reused 503 (delta 407) Receiving objects: 100% (513/513), 116.67 KiB, done. Resolving deltas: 100% (417/417), completed with 103 local objects.
warning: no common commits
remote: Counting objects: 710725, done.
remote: Compressing objects: 100% (125738/125738), done.
remote: Total 710725 (delta 589584), reused 704450 (delta 584029) Receiving objects: 100% (710725/710725), 172.71 MiB | 1073 KiB/s, done. Resolving deltas: 100% (589584/589584), done.
From git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6
* [new tag] v2.6.25-rc2 -> v2.6.25-rc2
Updating 4ee29f6..101142c
Fast forward

 Makefile                               |    4 +-
 drivers/bluetooth/hci_ldisc.c          |    1 +
 drivers/net/8139too.c                  |    2 +-
 drivers/net/Kconfig                    |   18 +
 drivers/net/Makefile                   |    3 +-
 drivers/net/cxgb3/l2t.c                |    2 +-
 drivers/net/cxgb3/sge.c                |   35 +-
 drivers/net/dm9000.c                   |  654 +++++---
 drivers/net/e1000/e1000_main.c         |   18 +-
 drivers/net/forcedeth.c                |  132 +-
 drivers/net/netconsole.c               |    4 +-
 drivers/net/ni52.c                     | 1142 +++++++-------
 drivers/net/ni52.h                     |  158 +-
 drivers/net/pcnet32.c                  |   48 +-
 drivers/net/phy/fixed.c                |    4 +-
 drivers/net/ps3_gelic_net.c            | 1215 ++++++++------
 drivers/net/ps3_gelic_net.h            |  415 ++++--
 drivers/net/ps3_gelic_wireless.c       | 2753 ++++++++++++++++++++++++++++++++
 drivers/net/ps3_gelic_wireless.h       |  329 ++++
 drivers/net/r6040.c                    |  233 ++--
 drivers/net/sis190.c                   |    3 +-
 drivers/s390/net/claw.h                |   19 +-
 drivers/s390/net/lcs.c                 |    2 +-
 drivers/s390/net/lcs.h                 |   16 +-
 drivers/s390/net/netiucv.c             |   29 +-
 fs/compat.c                            |    3 -
 fs/nfs/callback.c                      |   18 +-
 fs/nfs/dir.c                           |    8 +-
 fs/nfs/nfs4state.c                     |    4 +-
 fs/nfs/super.c                         |    4 +
 include/linux/dm9000.h                 |    2 +
 include/linux/netdevice.h              |    8 +-
 include/net/ax25.h                     |    2 +
 include/net/ndisc.h                    |    1 -
 include/net/xfrm.h                     |    5 +-
 net/ax25/af_ax25.c                     |   12 +-
 net/ax25/ax25_dev.c                    |    2 +-
 net/ax25/ax25_ds_timer.c               |   12 +-
 net/ax25/ax25_route.c                  |   28 +-
 net/ax25/ax25_timer.c                  |   60 +-
 net/core/dev.c                         |    4 +-
 net/core/neighbour.c                   |   12 +-
 net/core/rtnetlink.c                   |   36 +-
 net/core/skbuff.c                      |    3 +-
 net/ipv4/ah4.c                         |    2 +-
 net/ipv4/arp.c                         |    3 -
 net/ipv4/esp4.c                        |    5 +-
 net/ipv4/fib_trie.c                    |   99 +-
 net/ipv4/inet_hashtables.c             |    3 -
 net/ipv4/ip_sockglue.c                 |    5 -
 net/ipv6/ah6.c                         |    2 +-
 net/ipv6/esp6.c                        |    5 +-
 net/ipv6/ip6_output.c                  |    6 +-
 net/ipv6/xfrm6_output.c                |    2 +-
 net/key/af_key.c                       |    1 +
 net/netfilter/nf_conntrack_proto_tcp.c |    2 +-
 net/netfilter/xt_SECMARK.c             |    2 +-
 net/netlabel/netlabel_domainhash.c     |    6 +-
 net/netlabel/netlabel_unlabeled.c      |   30 +-
 net/netlabel/netlabel_user.c           |    3 +-
 net/netlink/genetlink.c                |    6 +-
 net/socket.c                           |    3 +
 net/xfrm/Kconfig                       |    2 +-
 net/xfrm/xfrm_input.c                  |    4 +-
 net/xfrm/xfrm_output.c                 |    2 +-
 net/xfrm/xfrm_user.c                   |    1 +
 66 files changed, 5686 insertions(+), 1971 deletions(-)
 create mode 100644 drivers/net/ps3_gelic_wireless.c
 create mode 100644 drivers/net/ps3_gelic_wireless.h

[lenb@d975xbx2 linus (master)]$
[lenb@d975xbx2 linus (master)]$ git --version
git version 1.5.4.1.122.gaa8d

Hi,

Now, this is funny. Apparently, everything worked fine for the master branch (I suppose it is the master branch, anyway), but went wrong for fetching the _tag_.

I do not have that git version, but something is fishy there, so I guess it depends on your particular version.

Ciao,
Dscho

* Len Brown:

Same here, but on a slow DSL line, so I had to hit ^C:

fw@deneb:~/src/linux/linux-2.6$ git pull
remote: Counting objects: 94176, done.
remote: Compressing objects: 100% (17486/17486), done.
remote: Total 83287 (delta 70429), reused 78218 (delta 65712)
Receiving objects: 100% (83287/83287), 18.66 MiB | 478 KiB/s, done.
Resolving deltas: 100% (70429/70429), completed with 7457 local objects.
warning: no common commits
remote: Counting objects: 53267
^C

This is Debian's 1:1.5.4.2-2 GIT version.

Can you try making backups before pulling and see if you can get a reproducable case? I haven't been able to arrange to have it happen to me, and once it happens, it's changed the state of the repository such that it won't happen again immediately. I found something suspicious at some point, and suggested a possible fix, but it's impossible to tell if it's actually resolved without a test case.

-Daniel
*This .sig left intentionally blank*

...

It still happens with latest git. (linus has declared -rc3 this time) unfortunately for me, i'm not on broadband this time so it is extremely painful -- to the point that i simply can't update this tree until i get home.

I started at 4fa2b1cde0e3797549f711ce9e51c395b3d6d2a7 and Linus' tree is at 7704a8b6fc4a8f51599eb2af4dcf1e2ac9c7e576

The diff between these commits is about 100KB uncompressed, but it seems that I'm pulling down 175MB again...

remote: Counting objects: 661, done.
remote: Compressing objects: 100% (139/139), done.
remote: Total 501 (delta 411), reused 443 (delta 362) Receiving objects: 100% (501/501), 73.89 KiB | 11 KiB/s, done. Resolving deltas: 100% (411/411), completed with 101 local objects.
warning: no common commits
remote: Counting objects: 714841, done.
remote: Compressing objects: 100% (127590/127590), done.
Receiving objects: 1% (11116/714841), 3.96 MiB | 5 KiB/s

[lenb@t61 ~]$ git --version
git version 1.5.4.3.230.g2db511

That's because nobody has touched anything in this area since your last report.

But I now have a theory.

Next time this happens, could you run ls-remote for all four git servers and compare them?

    $ LINUS=kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6
    $ for i in 1 2 3 4; do git ls-remote git://git$i.$LINUS >git$i.txt; done

If my theory is correct, a workaround would be not to fetch from
git://git.kernel.org/ but from git://git$i.kernel.org/ for some value of $i.

Now to the theory part.

Suppose the ancestry is like this:

o (rc2) o (rc3)
/ /
---A---o---o---o---o---B---o---o---o---C

And the event sequence is:

(0) You are in sync with rc2 (have commits up to "A" and tag

     rc2);

(1) Linus builds up to "B", tags rc3, pushes out to the master;

(2) git1 and git2 mirror that history;

(3) Linus builds up to "C", pushes out to the master;

(4) git1 gets "C" but git2 mirror lags behind;

(5) You fetch, which happens to first go to git1; you get "C" and learn that rc3 points at "B" that is locally reachable, but you do not have rc3 tag itself yet. So git-fetch auto-follows (i.e. asks for "rc3" tag);

(6) However, that goes over a separate connection, and you happen to hit git2, which does have "B" and rc3, but it does not have "C" yet.

(7) You tell the other end "Hey, I have C", which git2 does not understand, because it hasn't seen the commit through the mirroring system yet.

Now, even if we are in the above situation, things _ought_ to work (and I strongly suspect that the scripted git-fetch did work correctly). git2 would say "You have "C"? I dunno that one, but keep talking", and you will keep sending "I have this, this, this,...", walking the ancestry chain down from C. When you say "I also have B", it would say "Aha, Ok, I heard enough to tell that rc3 tag is the only thing I need to send to you".

However, if the current version of git-fetch has a bug in the negotiation code when it auto-follows tags, that could throw this conversation to compute what's common way off, and that is what I suspect is happening.

For example, I notice that the list of old refs is reused (kept in the transport structure) when it reconnects to a different instance of git-upload-pack to auto-follow the tags, and never refreshed from the actual remote end you are talking with.

This stale list of refs is passed all the way down to fetch_pack(), representing _your_ idea of what they said they have, which does not match the reality if you are connecting to a different host (via DNS round robin). Even if there is no DNS round robin issue, if an update happened on the host between the time you fetched the refs from there and you started a different instance of git-upload-pack for auto follow the tags, the issue is the same (if you re-read the list of refs from the new instance of upload-pack, you should be Ok, as the upload-pack on the other end should be internally consistent).

Another possibile problem area I did not check is if the current git-fetch takes care of clearing various mark bits used and left by find_common() and get_rev() in builtin-fetch-pack.c during the main transfer, before it initiates another round to auto follow the tags. When we wrote fetch-pack the first time, we did not design it (most importantly, find_common()) to be callable more than once, as cleaning them was unnecessary overhead for the call-once-and-exit program.

The attached stops the stale set of refs from being used for common ancestry computation. It may or may not fix your issue, but at least it should be more correct than what we currently have, I think.

index ac335f2..c7cdd42 100644
@@ -544,9 +544,16 @@ static int do_fetch(struct transport *transport,
 
 	fetch_map = ref_map;
 
-	/* if neither --no-tags nor --tags was specified, do automated tag
-	 * following ... */
+	/*
+	 * If neither --no-tags nor --tags was specified, do automated tag
+	 * following.
+	 */
 	if (tags == TAGS_DEFAULT && autotags) {
+		if (transport->remote_refs) {
+			struct ref *stale = (struct ref *)transport->remote_refs;
+			free_refs(stale);
+			transport->remote_refs = NULL;
+		}
 		ref_map = find_non_local_tags(transport, fetch_map);
 		if (ref_map) {
 			transport_set_option(transport, TRANS_OPT_DEPTH, "0");

Updates.

I seem to have a reliable reproduction script, for this issue.

After rebuilding your git with the following patch, running the test script ./t000.sh would exhibit that "no common refs" while following the tag.

Then rebuild your git with "#if 0" part enabled, and run the test again, to see the problem fixed.

This adds --autofollow option which is _purely_ for debugging this issue. The command named by the option is executed after the command finishes the main transfer but before it starts the second transfer to auto follow the tags.

The test prepares two repositories (src0.git and src1.git) to simulate DNS round robin server side. src0.git grows, src1.git mirrors it with a bit of lag. src.git is a symlink that is used to simulate DNS round robin. Initially it points at src0.git (more up-to-date one) and a clone to Len (dst.git) is made. After src0.git grows history while src1.git mirrors it with a lag, Len fetches again, first from src0.git but autofollowing connection goes to src1.git.

While I was futzing with the test script, I also observed another error message "fatal: not our ref", but that was before I fixed the commit traversal order by giving them the test_tick timestamps. It won't reproduce with the attached test script, but the patch also seemed to fix it.

---

 builtin-fetch.c |   12 +++++++
 t000.sh         |   95 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+), 0 deletions(-)

index ac335f2..79359ac 100644
@@ -28,6 +28,7 @@ static const char *depth;
 static const char *upload_pack;
 static struct strbuf default_rla = STRBUF_INIT;
 static struct transport *transport;
+static char *autofollow;
 
 static struct option builtin_fetch_options[] = {
 	OPT__QUIET(&quiet),

@@ -45,6 +46,8 @@ static struct option builtin_fetch_options[] = {
 		    "allow updating of HEAD ref"),
 	OPT_STRING(0, "depth", &depth, "DEPTH",
 		   "deepen history of shallow clone"),
+	OPT_STRING(0, "autofollow", &autofollow, "COMMAND",
+		   "call the command just before autofollowing"),
 	OPT_END()
 };
 

@@ -547,6 +550,15 @@ static int do_fetch(struct transport *transport,
 	/* if neither --no-tags nor --tags was specified, do automated tag
 	 * following ... */
 	if (tags == TAGS_DEFAULT && autotags) {
+		if (autofollow)
+			system(autofollow);
+#if 0
+		if (transport->remote_refs) {
+			struct ref *stale = (struct ref *)transport->remote_refs;
+			free_refs(stale);
+			transport->remote_refs = NULL;
+		}
+#endif
 		ref_map = find_non_local_tags(transport, fetch_map);
 		if (ref_map) {
 			transport_set_option(transport, TRANS_OPT_DEPTH, "0");

Correcting the transport code is important (and should probably be done in transport.c, if possible), but I think we're being a bit silly in autofollowing tags anyway. If we decide to fetch T due to having T^{}, we should tell the remote up front that we have T^{}, before we mention anything else, because it's obviously true and it's also absolutely certain to make the remote immediately do the right thing. It's silly to decide to fetch T because we will only need that one object, and then not instantly tell the server we only need that one object. (And, as luck would have it, yesterday I wrote code to cause for_each_ref return some specific values in addition to and before the actual stored refs.)

Of course, we shouldn't do this until we make the transport code more correct, because it's certain to hide any possible bug there.

Am I missing something in my analysis?

-Daniel
*This .sig left intentionally blank*

That's correct, and the autofollowing code does so. You will not know if you have T^{} until your primary transfer finishes, so you cannot roll autofollow into it.

I think we can teach the upload-pack side to be more helpful and with a protocol extension to send tag objects that are pointing at commits that will be included in the result, or something like that, though. But that is outside the scope of 1.5.5; it would be a moderate to large protocol surgery, and I suspect it might even have to affect pack-objects.

You won't know if you need only one object, so seeing that you have T^{} and asking _only_ for T is _wrong_. Think of a tag that points at another tag that points at the commit. You need to tell the other end "I have T^{}, please give me T", and that is exactly what the autofollowing does.

Using a single connection, either by just telling the remote that you want to autofollow tags, and it should therefore include any tags that point to any objects it includes, or by allowing you to list more refs that you want after you've received the pack without disconnecting, would be quite nice, but I agree that it's a longer-term issue.

I don't see that. If the situation is:

T - tag master
/ /
O - A - O - O - B

the first fetch will see:

tag: T
tag^{}: A
master: B

Only heads are interesting, so we fetch B. When we've fetched B, we find that we now have tag^{}. So then we do a new fetch, and (bugs aside) list our now-current refs, including origin/master (=B) but not including A, because we don't (yet) have any refs for it. I'm suggesting that, while autofollowing tags, we should make a point of listing A, because we know it's relevant. Furthermore, if the remote doesn't have B (due to mirror skew, perhaps), listing B and not listing A (in particular) will lead to an inexact search for a common commit, when we know perfectly well that A is the closest common commit between what we have and tag.

The issue is that our starting set for our side of the negotiation is our current refs, which doesn't include A. I'm suggesting that, for the purposes of autofollow, A should be included.

-Daniel
*This .sig left intentionally blank*

I agree its outside of 1.5.5, as we'd all like to see 1.5.5 happen soon, but it could be 1.5.6 material, especially if someone starts working on it sooner rather than later.

Its actually probably not that difficult to implement. We'd just want to include a size threshold, to prevent the client from suddenly receiving a 1M tag (with say a build log embedded in it) on an otherwise 100K transfer. Autofollowing by having the remote include the tags in the pack and send to the client would be more efficient for both sides then autofollowing with a second set of ref requests, even if we are keeping the current connection open.

I'll try to work up a prototype of this soon, say in the next week. Obviously not for 1.5.5 but no reason to wait for the 1.5.6/1.6.0 window to open before developing it. I think its a better approach then supporting a second set of ref requests on the same connection.

_IF_ we are going to support a second set of ref requests on the same connection then we should also support being able to switch to another repository. I have 40 some odd repositories at day-job that a shell script loops over and does fetches in, over SSH. Setting up and tearing down 40+ SSH connections (especially with tag following!) sucks[*1*]. I think the X.org folks are in a similar position as me[*2*].

I agree. This is probably easier than coding the protocol extension above. :-)

*1* I know all about the SSH connection sharing feature, it is
unsupported on Cygwin. I'm on Cygwin at day-job. So that is a no-go.

*2* X.org users are more likely to be on a UNIX platform where the
OpenSSH connection share code works correctly.

Shawn.

OK, so I posted a fairly short series tonight (4 patches) that handles some of the common cases in a fairly small amount of code churn. It might just be 1.5.5-ish.

Doing anything better is going to require a new protocol extension, which is already 1.5.6 material. In the mean time maybe Junio's earlier patch to try and drop the ref_map when we do open the new connection is the way to deal with the round-robin DNS issues.

Shawn.

Hmmm... Might the any protocol extensions require a 1.6 release rather than a 1.5.x release? Or is this extension compatible enough that it can be transparent?

Thanks,
jdl

The client and server exchange a list of supported features at the beginning, and the difference in behavior would be at the end, so it should be no problem to have the client ask for the chance to make further requests and the server acknowledge that (or the server offer and the client accept, depending on the order they do it, which I don't remember) without affecting programs that don't report the feature.

-Daniel
*This .sig left intentionally blank*

I thought about discarding the cached refs upon disconnect, but didn't do that, because presumably a caller might want to:

transport_get_remote_refs() to find what they have decide what it wants transport_fetch_refs() to ask for them
do stuff about the refs obtained transport_disconnect() to finish the transfer still do stuff about the refs obtained

and such a change would forbid the last step. But the only reason I avoided to break such a potential caller was because I did not bother to check if such a caller exists, and not because I thought the above sequence is sane, so if you think it is saner to clean up the stale information upon disconnect, please do so.

Yes, that is what I was talking about.

By telling the other end that we have B, we are implicitly telling that we have A as well. Under normal situation, telling the other end we have A does not help nor hurt anything. Under abnormal situation (e.g. DNS round robin switching the other end in the middle), the other end may say "I dunno about B", but the protocol is designed to negotiate and find that both ends have A, by following the ancestry chain down, so I do not think telling the other end that we have A helps that much. I however think that such a change would help sweeping potential bugs under the rug by making them harder to trigger.

By the way, the situation I said your logic would break is this:

    ---o---A---o---o---B
            \
             T---S

Both T and S are annotated tags, pointing at A and T respectively, and they both peel to A. As long as you ask for both T and S you may be Ok, but it feels still wrong. Commit walkers may grab S, die before grabbing T (git-native protocol is atomic with respect to objects transfer, so it won't have such an issue).

Actually, I just realized something which should have been obvious: when we reconnect, we get a list of the remote's refs, which we currently discard immediately. We should actually pass this list to fetch_pack() if we just reconnected, so that the client side always does the interaction with the right idea of the server's refs, and discard it afterwards. The fact that the user of transport_*() doesn't find out that the server side's refs change in the middle of the life cycle and can't find out in any way doesn't matter too much, so long as each actual connection is internally consistant. (And the situation is no different from how it used to be with git-fetch.sh: if you get a different mirror later, you may discover that the server now doesn't have refs that it seemed to advertize, but nothing weird happens.)

I think it could be slightly less server load if it doesn't have to walk from B to A, and I could make up something about cache locality.

I remember your tests that didn't quite show the problem leading to the autofollow connection getting ~100 objects, which is better than 700000 but worse than the correct 1 for that case; I think it had found a commit commit not too far away, but not the perfect one.

They wouldn't write a ref for S, though, so the result would be consistant still. If you ask for T and S and say you have A, everything should work.

-Daniel
*This .sig left intentionally blank*

I think that would also be a valid way to solve this "stale idea of what the other side has" and can replace my weatherbaloon patch.

Another potential problem area is if find_common() does the right thing when it is called for the second time. I did not check if you clear COMMON, SEEN, COMPLETE etc. bits from the object database before initiating the second round, but if you didn't, I am afraid these bits left over from the primary transfer might interfere the common ancestor discovery during the second round.

Absolutely; that was actually my first guess at why it was failing, and I think it's a necessary aspect to the failure. Let me see if I can get your test case to exhibit the problem for me and look into it further.

-Daniel
*This .sig left intentionally blank*

We always report to the user the list of refs we got from the first connection, even if we do multiple connections. But we should always use each connection's own list of refs in the communication with the server, in case we got a different server out of DNS rotation or the timing was surprising or something.

---
This should fix the same part of the problem that discarding transport->remote_refs did, but without having transport.c try to do foolish stuff and builtin-fetch need to tell it not to.

 transport.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

index 397983d..fc92311 100644
@@ -622,6 +622,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	char *dest = xstrdup(transport->url);
 	struct fetch_pack_args args;
 	int i;
+	struct ref *refs_tmp = NULL;
 
 	memset(&args, 0, sizeof(args));
 	args.uploadpack = data->uploadpack;

@@ -634,15 +635,13 @@ static int fetch_refs_via_pack(struct transport *transport,
 	for (i = 0; i < nr_heads; i++)
 		origh[i] = heads[i] = xstrdup(to_fetch[i]->name);
 
-	refs = transport_get_remote_refs(transport);
 	if (!data->conn) {
-		struct ref *refs_tmp;
 		connect_setup(transport);
 		get_remote_heads(data->fd[0], &refs_tmp, 0, NULL, 0);
-		free_refs(refs_tmp);
 	}
 
-	refs = fetch_pack(&args, data->fd, data->conn, transport->remote_refs,
+	refs = fetch_pack(&args, data->fd, data->conn, 
+			  refs_tmp ? refs_tmp : transport->remote_refs,
 			  dest, nr_heads, heads, &transport->pack_lockfile);
 	close(data->fd[0]);
 	close(data->fd[1]);

@@ -650,6 +649,8 @@ static int fetch_refs_via_pack(struct transport *transport,
 		refs = NULL;
 	data->conn = NULL;
 
+	free_refs(refs_tmp);
+
 	for (i = 0; i < nr_heads; i++)
 		free(origh[i]);
 	free(origh);

1.5.4.3.328.gcaed

So it happens everytime a new tag is fetched.

What happens if you restart the pull after interrupting the first attempt?

Nicolas

The workaround, so far as I know, is to hit ^C and do it again; the second time it will fetch 1 object (the actual tag) instead of 700000. If that's not the case, I'm even more confused about what's going on. And if you notice Linux tagging something before you pull, it would be great if you could capture the contents of .git/refs/ before you pull and send it to me if it does this.

-Daniel
*This .sig left intentionally blank*

Thanks. This is very puzzling.

This part looks quite sane.

	$ git rev-list --objects ^7cf712d v2.6.25-rc1^0 | wc -l
	328

and the number of received objects exactly match.

This is however very unexpected. The sequence internally should be doing the equivalent of:

- fetch the objects to complete the branches we track (i.e. what the above "rev-list" that fetches to complete the commit pointed by the v2.6.25-rc1 tag based on your earlier tip 7cf712d);

- store the tip (19af355 = v2.6.25-rc1^0) to the tracking

    branch;

- run another "git fetch" to retrieve objects to complete the v2.6.25-rc1 tag itself, based on our available refs (which includes the commit 19af355).

which should result in transferring only one object, which would say something like:

From git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6
7cf712d..19af355 master -> linus
remote: Counting objects: 1, done.
remote: Total 1 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (1/1), done.
From git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6
* [new tag] v2.6.25-rc1 -> v2.6.25-rc1
Updating 7cf712d..19af355

We would need a bit more digging to reproduce it, as I do not seem to be able to.

I wonder if the problem is that something isn't getting reinitialized for the second connection. It's not a separate invocation of fetch-pack, and I can't say for sure that it's sending the right info to the server when the statics in builtin-fetch-pack.c are left over from the earlier call. This would particularly explain the information that hitting ctrl-c and trying again fixes it.

I don't really know the builtin-fetch-pack code all that well, but I'll see if I can reproduce the problem and if I can figure out anything obviously wrong.

-Daniel
*This .sig left intentionally blank*

Hi,

Oh, that should be it! After all, the code in get_rev() in builtin-fetch-pack.c marks commits as SEEN and COMMON and POPPED.

So I guess you'd need to set something like

	struct commit_list *rev_list_orig;
	...
	rev_list_orig = rev_list;

before

        while ((sha1 = get_rev())) {

in the function find_common(), and then, after the while() loop, do something like

	while (rev_list_orig) {
		clear_commit_marks(rev_list->item,
			COMPLETE | COMMON | COMMON_REF | SEEN | POPPED);
		rev_list_orig = rev_list_orig->next;
	}

possibly free()ing the rev_lists in the process.

Ciao,
Dscho

I seem to be slow today, but how does that explain that the problem is reported only by Len so far?

Hi,

Hmm. The code I was referencing is only in "next" so far, right? And AFAICT it only occurs when you are fetching something which autofetches tags, right?

But thinking about this again: do we reuse the connection also for automatic tag fetching? If not, my whole reasoning is wrong.

Ciao,
Dscho

I think the code you referenced is quite old; the new thing is having it called twice in the same process, and that's also in "master" along with builtin-fetch, I think.

No, the way the protocol works, you can't request more stuff after you've received stuff on a connection, so we have to start a second one for that case.

-Daniel
*This .sig left intentionally blank*

What's currently confusing me, which is probably why I haven't been able to reproduce the problem, is how we don't have the newly-received commits as still interesting. Clearly there's some way to end up with them either not being applicable or being already marked, but I'm not seeing it.

(There's a good change that we could fix the problem with your loop, but I'd like to have a test case to make sure it's fixed and stays fixed)

-Daniel
*This .sig left intentionally blank*

* Len Brown:

I've got a linux-2.6 tree which is reasonable up to date, but which has been created by some acient GIT version. I see the same behavior from time to time. The second pull, after I've canceled the first one, usually downloads just the expected data.

On 11 Feb 2008, Florian Weimer spake thusly:

I just saw it as well, doing a big update (most of the way from 2.6.23 to current tip):

loki 214 /usr/packages/linux/linux% git pull
remote: Counting objects: 118487, done.
remote: Compressing objects: 100% (22411/22411), done.
remote: Total 102959 (delta 85610), reused 97521 (delta 80449) Receiving objects: 100% (102959/102959), 26.41 MiB | 70 KiB/s, done. Resolving deltas: 100% (85610/85610), completed with 7493 local objects.
warning: no common commits
remote: Counting objects: 708160, done.
remote: Compressing objects: 100% (124705/124705), done.
Receiving objects: 9% (70213/708160), 25.33 MiB | 70 KiB/s

loki 215 /usr/packages/linux/linux% ls -l .git/objects/pack
total 240224
-r--r--r-- 1 compiler hackers 2651912 2008-02-11 20:44 pack-69c40f2970403946a75203cc393ecc2b1abf8aa3.idx
-r--r--r-- 1 compiler hackers 69189104 2008-02-11 20:44 pack-69c40f2970403946a75203cc393ecc2b1abf8aa3.pack -r--r--r-- 1 compiler hackers 14719304 2007-12-02 16:01 pack-7eb87d068cee2214e4b0c5b6b571014654cbaaa4.idx
-rw-r--r-- 1 compiler hackers 0 2007-12-06 14:45 pack-7eb87d068cee2214e4b0c5b6b571014654cbaaa4.keep
-r--r--r-- 1 compiler hackers 157916633 2007-12-02 16:01 pack-7eb87d068cee2214e4b0c5b6b571014654cbaaa4.pack
-r--r--r-- 1 compiler hackers 11744 2007-12-14 22:55 pack-993c8617968f3d44603663e4a2915ee260236f91.idx
-r--r--r-- 1 compiler hackers 1004082 2007-12-14 22:55 pack-993c8617968f3d44603663e4a2915ee260236f91.pack

Oddly enough I then halted it: I have no desire to blow an extra 160Mb of space on duplicates of objects I've already got.

Pullng again promptly grabbed 102791 objects (i.e. prety much the same set again) as if the pack up there at the top of the directory listing didn't even exist. (git-repack will happily clean up the duplicates for me, I'm sure.)

That time, it worked:

loki 216 /usr/packages/linux/linux% git pull
remote: Counting objects: 118319, done.
remote: Compressing objects: 100% (22389/22389), done.
remote: Total 102791 (delta 85463), reused 97353 (delta 80303) Receiving objects: 100% (102791/102791), 26.39 MiB | 91 KiB/s, done. Resolving deltas: 100% (85463/85463), completed with 7494 local objects. From git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6

 * [new tag]         v2.6.24    -> v2.6.24
 * [new tag]         v2.6.24-rc1 -> v2.6.24-rc1
 * [new tag]         v2.6.24-rc2 -> v2.6.24-rc2
 * [new tag]         v2.6.24-rc3 -> v2.6.24-rc3
 * [new tag]         v2.6.24-rc4 -> v2.6.24-rc4
 * [new tag]         v2.6.24-rc5 -> v2.6.24-rc5
 * [new tag]         v2.6.24-rc6 -> v2.6.24-rc6
 * [new tag]         v2.6.24-rc7 -> v2.6.24-rc7
 * [new tag]         v2.6.24-rc8 -> v2.6.24-rc8
 * [new tag]         v2.6.25-rc1 -> v2.6.25-rc1

[...]

This is with git version 1.5.4.25.g7f255-dirty. (only local changes are some makefile tweaks).

I've never seen this failure before with any earlier git version, but that might just be coincidence.

`The rest is a tale of post and counter-post.' --- Ian Rawlings
                                                   describes USENET