On Sat, Dec 10, 2022 at 09:16:42AM +0900, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes:
>
> > + fd = open_loose_object(r, oid, &path);
> > + if (fd < 0) {
> > + if (errno != ENOENT)
> > + error_errno(_("unable to open loose object %s"), path);
> > + return -1;
> > + }
>
> I know there was a discussion in the previous round, but is this use
> of path truly safe? Currently it may happen to be as long as there
> is at least one element on the odb list, but when thinking things
> through with future-proofing point of view, I do not think assuming
> that path is always computable is a healthy thing to do in the
> longer term.
>
> Our "struct object_id" may be extended in the future and allow us to
> express "invalid" object name, in which case the error return we get
> may not even be about "loose object file not openable" but "there
> will never be a loose object file for such an invalid object name",
> in which case there won't be any path returned from the function.
Actually, I think it is much worse than that. The code as written above
is already buggy (which is my fault, as I suggested it).
In open_loose_object() we'll continue to iterate and pick out the "most
interesting errno". But we'll throw away the path that gave us that
errno. So we might well say:
unable to open loose object /some/alternate/12/34abcd: permission denied
when the actual problem is in /main/objdir/12/34abcd.
It's fixable, but with some pain in handling the allocations. I think it
would be sufficient to just say:
error_errno(_("unable to open loose object %s"), oid_to_hex(oid));
here. And possibly put a comment above open_loose_object() that "path"
is only guaranteed to point to something sensible when a non-negative
value is returned.
-Peff
