Re: [PATCH 4/4] archive: ignore blob objects when checking reachability

[Michael Haggerty <mhagger@alum.mit.edu>]

On 06/06/2013 12:40 AM, Jeff King wrote:
> We cannot create an archive from a blob object, so we would
> not expect anyone to provide one to us. And if they do, we
> will fail anyway just after the reachability check.  We can
> therefore optimize our reachability check to ignore blobs
> completely, and not even create a "struct blob" for them.
> 
> Depending on the repository size and the exact place we find
> the reachable object in the traversal, this can save 20-25%,
> a we can avoid many lookups in the object hash.
> 
> The downside of this is that a blob provided to a remote
> archive process will fail with "no such object" rather than
> "object is not a tree" (we could organize the code to retain
> the old message, but since we no longer know whether the
> blob is reachable or not, we would potentially be leaking
> information about the existence of unreachable objects).

Could we change the error message to "no such tree object" to be
non-committal about the reason for the failure?


For a moment I thought that one could get correct error messages while
retaining the speed gain in the usual case by doing a quick object
lookup, and then

    check type of object
    if object is missing:
        die(there is no such object)
    else if object is a blob:
        do reachability test including blobs
        if object is not reachable:
            die(there is no such object)
        else
            die(object is not a tree)
    else
        do reachability test excluding blobs
        etc

However, even this would leak information about the existence of
nonreachable objects to a client measuring time time for the response
because the death due to non-reachability would take longer than death
due to missing object.  So, if one would insist on correct error
messages and no information leakage, one could just skip the first
"object is missing" optimization (it should be pretty rare anyway!) like so:

    check type of object
    if object is missing or object is a blob:
        /* Force the same delay in either case: */
        do reachability test including blobs
        if object is missing or object is not reachable:
            die(there is no such object)
        else
            die(object is not a tree)
    else
        do reachability test excluding blobs
        etc

I'm not suggesting that the extra effort is worth it; I just wanted to
mention the possibility.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/