On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote: > > Looking deeper, we do not need these trees and blobs at all. The problem > > is really just a tag that peels to an object that is not otherwise a ref > > tip, regardless of its type. > > Thanks for feedback and for coming up with the fix. Sure, I'm ok with > moving the test into your patch. However, even if a test becomes > different - narrowing down root of _current_ problem, I suggest to also > keep explicitly testing tag-to-blob and tag-to-tree (and if we really > also want tag-to-commit and tag-to-tag) behaviour. Reason is: if we skip > those now, they can potentially break in the future. Yeah, I have no problem testing these cases separately. There's no bug with them now, but it is a slightly uncommon case. My suggestion would be to submit a patch that goes on top of mine that covers these cases. > I would also suggest to fix upload-pack, as it is just not consistent to > reject sending objects that were advertised, and so can strike again > some way in the future. After all git.git's fetch-pack is not the only > git client that should be possible to interact with git.git's > upload-pack on remote side, right? No, it's not the only client. At the same time, I am on the fence over whether upload-pack's behavior is wrong or not. It depends what you take a peeled advertisement line to mean. Does it mean: this object has been advertised and clients should be able to fetch it? Or does it mean: by the way, you may be interested to know the peeled value of this tag in case you want to do tag-following? So far I think it has only meant the latter. I could see an argument for the former, but any client depending on that would never have worked, AFAICT. We could _make_ it work, but how would a client know which server version it's talking to (and therefore whether it is safe to make the request?). I think you'd have to add a capability to negotiate. > I'm not sure, but I would say that `fetch-pack --all` from an empty > repository should not fail and should just give empty output as fetch > does. Yeah, that seems reasonable to me. The die() that catches this dates back to 2005-era, and we later taught the "fetch" porcelain to handle this. I don't _think_ anybody would be upset that the plumbing learned to treat this as a noop. It's probably a one-liner change in fetch_pack() to return early instead of dying. > For the reference all the cases presented here are real - they appear in > our repositories on lab.nexedi.com for which I maintain the backup, and > I've noticed them in the process of switching git-backup from using > fetch to fetch-pack here: > > https://lab.nexedi.com/kirr/git-backup/blob/0ab7bbb6/git-backup.go#L436 I applaud you using the porcelain for your scripts, but I suspect that fetch-pack by itself is not at all well-used or well-tested these days (certainly this --all bug has been around for almost 6 years and is not very hard to trigger in practice). If an extra connection isn't a problem, you might be better off with "git ls-remote", and then picking through the results for refs of interest, and then "git fetch-pack" to actually get the pack. That's how git-fetch worked when it was a shell script (e.g., see c3a200120d, the last shell version). It may also be sane to just use "git fetch", which I'd say is _fairly_ safe to script. Of course I have no problem if you want to fix all of the corner cases in fetch-pack. Just giving you fair warning. :) -Peff
