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

From: Jeff King <peff@peff.net>
Date: Jun 12, 2018, 09:48 UTC
Message-ID: <20180612094849.GB26123@sigill.intra.peff.net>
In-Reply-To: <20180611094255.GA15563@deco.navytux.spb.ru>

On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote:

Show 10 quoted lines

> > 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.

Show 5 quoted lines

> 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.

Show 6 quoted lines

> 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