From: Jonathan Tan Date: Mon, 12 Dec 2022 20:38:11 GMT Subject: Re: [PATCH v4 3/4] object-file: emit corruption errors when detected Message-ID: <20221212203811.77228-1-jonathantanmy@google.com> In-Reply-To: Junio C Hamano writes: > Jonathan Tan 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. Ah, good point. I think what I can do is to document the function to only return a path if a path was involved in the error somehow, and make anything that uses "path" in the caller check for NULL. > Other than that, the series looks quite clearly written. Nicely > done. > > Thanks. Thanks for taking a look.