-
-
Notifications
You must be signed in to change notification settings - Fork 402
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Porcelain Clone 0.22.3 not Cloning GitHub Branches Correctly in at least one case #1389
Comments
It's hard for me to reproduce this, since it's a private repository. It is probably a regression from the new v2 support. Can you try passing protocol_version=0 to clone and seeing if that improves things? |
That's odd. It is listed as a public repository. |
Ah, I've managed now - not sure what went wrong last time, unrelated issue with my SSH key. |
Cool. FYI - I was able to replicate it from another GitHub account using Ubuntu as well as from my MacBook. |
That said, I have trouble reproducing the bug:
|
Huh. That's weird. I get the same behavior I described from two different systems using two different GitHub users. Setting protocol_version=0 does "fix" the behavior, though. |
What is funny about the output you pasted is that I see:
when it is going to fail and
when it is going to work, but you saw it work with the former output. |
I have seen it with |
Is there anything else you would like me to look at, since I can get it to happen? |
I tried it from a cloned version of dulwich too (checking out the dulwich-0.22.3 tag and doing a |
I doubt this is significant, but on both systems I am working in a venv, in case that sheds any light. |
It's probably not relevant, but do you have the rust extensions compiled? The other thing you could try is see if things still fail at commit 436ce9f, which was just before the v2 improvements landed. |
I don't believe I have the rust extensions. If I do, it would be because of something I don't know about. I hate to say "no" absolutely without checking, any idea how I would check that? I will check 436ce9f for you. |
Not seeing the problem at 436ce9f. |
If there's files starting with _ in your dulwich package directory, you've got the Rust extensions. |
A big help would be if you could bisect to find the problematic commit, since I don't have a way to reproduce this. There shouldn't be too many in between 0.22.1 and 436ce9f |
Except for the normal python files (init.py, pychache and the like) and some C files and their related .so files, I don't see anything interesting starting with _, so I think, no rust extensions. I will try to track down the specific commit later this morning. |
I am curious about why you can't reproduce it. I have managed to get it reliably with two very different GitHub users on two very different systems. I am going to see if someone else who is not me (physically) can reproduce it. I will report back shortly. |
So, just to see whether my GitHub repo is somehow corrupt or weird, I turned on packet tracing and version 2 protocol with my local
and the command retrieved the repo the way I was expecting it to (i.e. with a There are two things I am seeing here that seem interesting. You seem to be able to reproduce one of them but not the other:
It might be useful to focus on the disparity in objects and pack entries, since you are able to reproduce that (check whether you see 248/247 running with I am getting a Raspberry Pi I have lying around set up to try this too so I can see if the problem exists there. That is taking some time because I have to update Python on it. I will update when I have results there. |
I freshly installed Python 3.12 on my Pi, cloned |
I think I know what's happening; with v2 there are two separate fetches of capabilities, and the "symref" capability is only extracted from the first one, while it appears in the second. (The symref capability provides information as to where HEAD will point, with dulwich defaulting to "master" if none is provided by the server) |
@erl-hpe Would you be able to double check that this fixes the issue for you as well? I'm a bit hesitant to declare it fixed since I had trouble reproducing it earlier. |
I just tried it on my Pi, and now I am seeing no branches at al (i.e. no
|
Alright, I will be taking a look at this today. |
There are two different issues to be considered here. The first is indeed due to a lack of tests of symref handling in our compat tests. I have implemented tests for this, will send a PR shortly. The added tests uncovered a bug in fetch over HTTP (AbstractHttpGitClient) where the symref discovered in self._discover_references() gets clobbered by an empty symrefs dict returned from self._negotiate_upload_pack_capabilities(server_capabilities(). This can be fixed by using a separate variable and now the tests are passing. The second problem seems to be specific to github's server because this problem does not show when I clone the repository from git-daemon. In the Git protocol trace above we see that Git is requesting specific reference prefixes. The dulwich implementation does not ask for ref-prefixes by default, assuming this would trigger the server to return all refs, as it does with git-daemon. However, github only returns the HEAD ref unless we specifically ask for "refs/". So this code works with github:
And this code works with git-daemon serving the cloned repository:
Not sure what to do here. Should we always add a default ref_prefix to please github? Should we document the issue and require users to pass a ref_prefix to avoid it? |
…t case related to issue jelmer#1389
The behaviour of clone should be the same whether v0 or v2 is used, otherwise existing code breaks. In that case, i think that means if ref_prefix is not specified then it should probably default to [b'refs/'] |
…t case related to issue jelmer#1389
Alright, I like the idea of using a default argument. I will include HEAD in the ref-prefix list because Git does that, too. Just in case the HEAD ref would otherwise get filtered out. Doesn't seem to be the case with github, but who knows what other implementations might do. |
Github's SSH git protocol v2 server returns a useless reference list which only contains "HEAD" unless we explicitly ask for the "refs/" prefix. Fixes issue jelmer#1389
Github's SSH git protocol v2 server returns a useless reference list which only contains "HEAD" unless we explicitly ask for the "refs/" prefix. Fixes issue #1389
Thanks folks! It sounds like you have been busy. Let me know when you are happy with the state of things and I will try it out with my application. |
@erl-hpe please give it another try |
I am seeing another failure from my which could be a coding error in my application, but it is a difference in behavior between 0.22.1 and 0.22.4. You will see (below) that I went back to my simple example and am still seeing a problem (different but still there) with the behavior there too. My code has in it the following (where
That is getting a Playing with this a bit interactively, I can see a difference in behavior for protocol 2 and protocol 1:
Notice that we only see a local head, and that head is names Let me know if I am making an invalid assumption here about there being a On the other hand, this might be related to the original problem? |
As I noted in the previous comment, when I switched from not specifying a version (i.e. wanting the current head of the default branch) to specifying
|
If I drop back to 0.22.1, this is what I see:
|
I tried the same thing in 0.22.4 with protocol version set to 0, and got the 0.22.1 behavior:
|
If I were to add a fetch after the clone call, I think I would get closer to the behavior I am looking for. Is this how the porcelain operations are meant to be done? If so, is there a way other than looking for the remote HEAD branch (which may not exist) to determine what the remote repo thinks its default branch is? I am thinking that there must be, because |
This is a genuine bug, no extra calls should be necessary. |
Okay. Cool. For the moment, I am working around it in my application by forcing the protocol_version to 0 in my call to clone. Everything else I am doing seems to work fine when I do that. It is easy enough to change that out either to test fixes or to return to the regularly scheduled behavior when it is fixed. For now it lets me follow the release stream without needing to pin an older version of Dulwich. Thanks for your work on this. If there is anything I can reasonably do to help, let me know and I will. |
The porcelain layer fell back on just "HEAD" as the default ref-prefix, exluding all other refs from the cloned repository. Fixes issue jelmer#1389
The above behaviour is due to a bad default setting in the porcelain layer. The PR I submitted should fix it. |
@carlosmn Hey, any idea if something could be done about the above problem at Github's end? It looks like a server-side bug to me. |
@stspdotname will explicitly setting On reflection, I could imagine cases where I am also curious why Not saying it isn't a GitHub bug, or at least inconsistency, just wondering why it impacts Dulwich differently from the Git commands. |
Also, I can confirm that the UPDATE: actually, I am finding that what is at the tip of |
@erl-hpe Several more fixes have gone into main - can you try again? The original issue now reliable results in a "main" branch being created for me. |
The latest main ( Thanks for the quick and diligent response to this issue! |
Thanks for your patience in helping us debug this, and to @stspdotname for debugging and fixes. |
Did you figure out what was causing the bare |
Using dulwich.porcelain clone() at the 0.22.3 release level to clone
I wind up with refs that contain a single branch named 'master'. Unfortunately, the remote repository no 'master' branch, but it does have a 'main' branch. In other words, 'master' in the clone should be 'main' but is not. When I do the same thing with 0.22.1 I get what I expect.
Here is a transcript of the (correct) 0.22.1 behavior:
and the list of branches in the resulting repo:
Here is what I see with 0.22.3:
and the branches:
The text was updated successfully, but these errors were encountered: