Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all

[Jeff King <peff@peff.net>]

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