On Tue, May 9, 2017 at 9:33 PM, Jeff King <peff@xxxxxxxx> wrote: > On Tue, May 09, 2017 at 09:22:11PM -0700, Shawn Pearce wrote: > >> > Hmm. That makes sense generally, as the request should succeed. But it >> > seems like we're creating a client that will sometimes succeed and >> > sometimes fail, and the reasoning will be somewhat opaque to the user. >> > I have a feeling I'm missing some context on when you'd expect this to >> > kick in. >> >> Specifically, someone I know was looking at building an application >> that is passed only a SHA-1 for the tip of a ref on a popular hosting >> site[1]. They wanted to run `git fetch URL SHA1`, but this failed >> because the site doesn't have upload.allowtipsha1inwant enabled. >> However the SHA1 was clearly in the output of git ls-remote. > > OK. So this is basically a case where we expect that the user knows what > they're doing. > >> For various reasons they expected this to work, because it works >> against other sites that do have upload.allowtipsha1inwant enabled. >> And I personally just expected it to work because the fetch client >> accepts SHA-1s, and the wire protocol uses "want SHA1" not "want ref", >> and the SHA-1 passed on the command line was currently in the >> advertisement when the connection opened, so its certainly something >> the server is currently willing to serve. > > Right, makes sense. I wondered if GitHub should be turning on > allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide > some internal refs[1], and they're things that users wouldn't want to > fetch. The problem for your case really is just on the client side, and > this patch fixes it. > > Some of this context might be useful in the commit message. > > -Peff > > [1] The reachability checks from upload-pack don't actually do much on > GitHub, because you can generally access the objects via the API or > the web site anyway. So I'm not really opposed to turning on > allowTipSHA1InWant if it would be useful for users, but after > Jonathan's patch I don't see how it would be. Right, I agree. That is why we proposed this patch to the client, rather than a support request with GitHub to change server configuration. :)