Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

Jeff King writes: > So I dunno. That is far from exhaustive, but it does seem like most > cases should assume the presence of the file. You are right. I should have considered that "we got a random object-name looking thing and we do not care if it does not exist" is a very

Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

On Thu, Nov 23, 2017 at 11:35:21AM +0900, Junio C Hamano wrote: > Not limiting us to the caller in the "fetch" codepath, I think the > expectation by callers of lookup_commit_reference_gently() in the > ideal world would be: > > - It has an object name, and wants to use it as point in the

Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

Jeff King writes: > So there are four cases we care about for this call in fetch: > > 1. We fed a real sha1 and got a commit (or peeled to one). > > 2. We fed a real sha1 which resolved to a non-commit, and we got NULL. > > 3. We fed a real sha1 and the object was missing or

Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

On Wed, Nov 22, 2017 at 10:42:30AM +0900, Junio C Hamano wrote: > Jeff King writes: > > > I'm not sure what the right behavior is, but I'm pretty sure that's not > > it. Probably one of: > > > > - skip updating the ref when we see the breakage > > > > - ditto, but terminate

Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

On Wed, Nov 22, 2017 at 10:49:25AM +0900, Junio C Hamano wrote: > WRT existing codepaths that pass 0{40} and refuses to notice a > potential repository corruption (from getting a NULL for a non null > object name), I think we would need a sweep of the codebase and fix > them in the longer term.

Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

Jeff King writes: > Here's a re-roll of patch 5 that behaves this way (the patch should be > unsurprising, but I've updated the commit message to match). > > I did notice one other thing: the function looks up replace objects, so > we have both the original and the replaced sha1

Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

Jeff King writes: > I'm not sure what the right behavior is, but I'm pretty sure that's not > it. Probably one of: > > - skip updating the ref when we see the breakage > > - ditto, but terminate the whole operation, since we might be deleting > other refs and in a broken

Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

On Tue, Nov 21, 2017 at 02:20:19PM +0900, Junio C Hamano wrote: > After queuing this series to an earlier part of 'pu' and resolving a > conflict with jh/fsck-promisors topic, I ended up with a code that > rejects 0{40} a lot earlier, before we try to see if a pack entry > for 0{40} exists, even

Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

On Tue, Nov 21, 2017 at 11:37:28AM +0900, Junio C Hamano wrote: > Jeff King writes: > > > In an ideal world, we'd simply fix all of the callers to > > notice the null sha1 and avoid passing it to us. But a > > simple experiment to catch this with a BUG() shows that > > there are

Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

Jeff King writes: > This is the minimal fix that addresses the performance issues. > I'd actually have no problem at all declaring that looking up a null > sha1 is insane, and having the object-lookup routines simply return "no > such object" without even doing the loose/pack

Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

Jeff King writes: > In an ideal world, we'd simply fix all of the callers to > notice the null sha1 and avoid passing it to us. But a > simple experiment to catch this with a BUG() shows that > there are a large number of code paths. Well, we can view this (or the alternative you

Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

On Mon, Nov 20, 2017 at 12:47:53PM -0800, Stefan Beller wrote: > > This is the minimal fix that addresses the performance issues. > > I'd actually have no problem at all declaring that looking up a null > > sha1 is insane, and having the object-lookup routines simply return "no > > such object"

Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

On Mon, Nov 20, 2017 at 12:35 PM, Jeff King wrote: > In theory nobody should ever ask the low-level object code > for a null sha1. It's used as a sentinel for "no such > object" in lots of places, so leaking through to this level > is a sign that the higher-level code is not being

[PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

In theory nobody should ever ask the low-level object code for a null sha1. It's used as a sentinel for "no such object" in lots of places, so leaking through to this level is a sign that the higher-level code is not being careful about its error-checking. In practice, though, quite a few code