Skip to content
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

Trying to fetch only non-existant refs results in "Could not decode server reply" #1405

Closed
udoprog opened this issue Jun 16, 2024 · 5 comments · Fixed by #1368
Closed

Trying to fetch only non-existant refs results in "Could not decode server reply" #1405

udoprog opened this issue Jun 16, 2024 · 5 comments · Fixed by #1368
Assignees
Labels
acknowledged an issue is accepted as shortcoming to be fixed

Comments

@udoprog
Copy link

udoprog commented Jun 16, 2024

Current behavior 😯

If you try to fetch a non-existant ref over https (github specifically), you get a vague error indicating that:

Error: Could not decode server reply

Caused by:
    0: Failed to read from line reader
    1: failed to fill whole buffer

Expected behavior 🤔

I'm not sure what to expect. But I hoped that the protocols involved would indicate that the references do not exist, and a better error message propagated to the user. Alternatively that the outcome would simply not be populated, and any refs specified which does not exist are simply ignored which is what happens if you do specify refs that exist alongside ones which do not.

Maybe this isn't an error at all, and I'm simply expected to enumerate the remote refs before attempting to fetch them, all though this would be racy since the refs can change between enumeration and fetching.

Git behavior

git fetch https://github.com/dtolnay/rust-toolchain refs/tags/stable
fatal: couldn't find remote ref refs/tags/stable

Steps to reproduce 🕹

Note that I can only reproduce this on Linux , on Windows it seems to work as expected for some reason.

I've set up a minimal repro here where I try to fetch refs/tags/stable from dtolnay/rust-toolchain. Note that refs/heads/stable is actually what exists.

See: https://github.com/udoprog/playground/tree/gix-issue2

Rust version:

rustc 1.79.0 (129f3b996 2024-06-10)
binary: rustc
commit-hash: 129f3b9964af4d4a709d1383930ade12dfe7c081
commit-date: 2024-06-10
host: x86_64-unknown-linux-gnu
release: 1.79.0
LLVM version: 18.1.7
@Byron
Copy link
Member

Byron commented Jun 17, 2024

Thanks for reporting!

I think this needs more code in order to reproduce it as I don't get this particular error when trying it with gix v0.36:

❯ gix fetch -r https://github.com/dtolnay/rust-toolchain refs/tags/stable
server sent 1 tips, 1 were filtered due to 1 refspec(s).
no negotiation was necessary

The message to the user isn't quite as descriptive as the one Git produces, but it's far-fetched from running into an obscure error as well.

Maybe take a look at the gix fetch code to see what the difference is? My guess is that the program using gix has control over running into this error, but I'd really like to know how this can be improved so ideally that can't happen, or is unlikely to happen.

Thanks for your help with this.

@udoprog
Copy link
Author

udoprog commented Jun 17, 2024

I think this needs more code in order to reproduce it as I don't get this particular error when trying it with gix v0.36:

Did you try the included reproduction?

I tried it now, and I'm also getting it on Windows:

$ git clone https://github.com/udoprog/playground -b gix-issue2 gix-issue2
$ cd gix-issue2
$ cargo run
   Compiling playground v0.0.0 (D:\repo\gix-issue2)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 14.82s
     Running `target\debug\playground.exe`
Error: Could not decode server reply

Caused by:
    0: Failed to read from line reader
    1: failed to fill whole buffer
error: process didn't exit successfully: `target\debug\playground.exe` (exit code: 1)

@Byron Byron added acknowledged an issue is accepted as shortcoming to be fixed and removed feedback requested labels Jun 17, 2024
@Byron
Copy link
Member

Byron commented Jun 17, 2024

Incredible, I missed it.

Research

gix sends

0014command=ls-refs
001bagent=git/oxide-0.63.0
0001000csymrefs
0009peel
000bunborn
001aref-prefix refs/tags/
0000

playground

0014command=ls-refs
001bagent=git/oxide-0.63.0
0001000csymrefs
0009peel
000bunborn
001aref-prefix refs/tags/
001aref-prefix refs/tags/
0000

Git does:

❯ git fetch https://github.com/dtolnay/rust-toolchain refs/tags/stable
0014command=ls-refs
0024agent=git/2.39.3.(Apple.Git-146)0016object-format=sha100010009peel
000csymrefs
000bunborn
0020ref-prefix refs/tags/stable
0025ref-prefix refs/refs/tags/stable
002aref-prefix refs/tags/refs/tags/stable
002bref-prefix refs/heads/refs/tags/stable
002dref-prefix refs/remotes/refs/tags/stable
0032ref-prefix refs/remotes/refs/tags/stable/HEAD
001aref-prefix refs/tags/
0000

All receive:

003a1482605bfc5719782e1267fd0c0cc350fe7646b8 refs/tags/v1
0000

gix and git stop there, but the playground tries to receive a pack:

0012command=fetch
001bagent=git/oxide-0.63.0
0001000ethin-pack
000eofs-delta
0010include-tag
000ddeepen 1
0032have 1a09566f8d0c4865627c46707ebf81db5577d9d3
0032have 49dece33e758a532e37526e6b03f07883895cd9e
0032have 47312a0feccaafb26affe3e8489f90bef029e9dd
0032have 9cf99d71a84ba6e5c6756e249ed20762f78db50a
0009done
0000

Which is met with a completely empty response, something that it doesn't like.

HTTP/1.1 200 OK
Server: GitHub-Babel/3.0
Content-Type: application/x-git-upload-pack-result
Content-Security-Policy: default-src 'none'; sandbox
expires: Fri, 01 Jan 1980 00:00:00 GMT
pragma: no-cache
Cache-Control: no-cache, max-age=0, must-revalidate
Vary: Accept-Encoding
Transfer-Encoding: chunked
Date: Mon, 17 Jun 2024 13:18:10 GMT
X-Frame-Options: DENY
X-GitHub-Request-Id: DCAF:2A8543:11A9C6A3:120CCA64:66703792

It looks like the problem is that it doesn't stop in time - after all, there is no refspec match, so it can't "want" anything, yet here we are.

It's notable that playground doesn't change how tags are handled, but even with .with_fetch_tags(gix::remote::fetch::Tags::None) there is change in behaviour.

Finally, it turned out that --depth 1 triggers the issue.

gix fetch --depth 1 -r https://github.com/dtolnay/rust-toolchain refs/tags/stable

Preparing a fix now.

Byron added a commit that referenced this issue Jun 17, 2024
Previously, when a shallow operation was specified, it was possible
to skip past the no-change test and attempt to negotiate even though
there was nothing to want.

This is now 'fixed' by simply aborting early if there is no refspec
mapping at all.

Further, it aborts as early as possible with a nicer error message,
after all, there is no use in having no mapping.
@Byron Byron self-assigned this Jun 17, 2024
@Byron Byron linked a pull request Jun 17, 2024 that will close this issue
9 tasks
@Byron
Copy link
Member

Byron commented Jun 17, 2024

Once the related PR is merged, it will print the following (as error):

Error: None of the refspec(s) refs/tags/stable matched any of the 1 refs on the remote

Byron added a commit that referenced this issue Jun 17, 2024
Previously, when a shallow operation was specified, it was possible
to skip past the no-change test and attempt to negotiate even though
there was nothing to want.

This is now 'fixed' by simply aborting early if there is no refspec
mapping at all.

Further, it aborts as early as possible with a nicer error message,
after all, there is no use in having no mapping.

Note that it's explicitly implemented such that obtaining such an empty
refmap is fine, but trying to receive it is not. That way, the user can
obtain information about the server without anything being an error.
Byron added a commit that referenced this issue Jun 19, 2024
Previously, when a shallow operation was specified, it was possible
to skip past the no-change test and attempt to negotiate even though
there was nothing to want.

This is now 'fixed' by simply aborting early if there is no refspec
mapping at all.

Further, it aborts as early as possible with a nicer error message,
after all, there is no use in having no mapping.

Note that it's explicitly implemented such that obtaining such an empty
refmap is fine, but trying to receive it is not. That way, the user can
obtain information about the server without anything being an error.
@udoprog
Copy link
Author

udoprog commented Jun 20, 2024

Updated and tested the reproduction, and this issue seems like it's been fixed. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledged an issue is accepted as shortcoming to be fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants