On Wed, Dec 07 2022, Jonathan Tan wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> Yeah, I think that's even better, although... > > [snip] > >> > It's also kind of weird that map_loose_object_1() is a noop on a >> > negative descriptor. That technically makes this correct, but I think it >> > would be much less surprising to always take a valid descriptor, and >> > this code should do: >> > >> > if (fd) >> > return -1; >> > return map_loose_object_1(r, path, fd, size); >> > >> > If we are going to make map_loose_object_1() less confusing (and I think >> > that is worth doing), let's go all the way. >> >> ...maybe we should go further in the other direction. I.e. with my >> earlier suggestion we're left with the mess that the "fd" ownership >> isn't clear. > > With Peff's suggestion I think we can make the function always close > the fd. If we find it not to be clear, we can rename the function > to ..._close_fd() or something like that. > > Thanks to both of you for your suggestions. I think that was my suggestion. Peff's on top of that was to never have it *open* the fd, but I'd left one caller doing that. I.e. that the ownership would still be passed to it, but at least it would always be passed, and wouldn't be the mixed affair that my initial suggestion left it at. I'll leave it to you to pick through this. I have a mild preference for my latest suggestion as the ownership of all the variables seems cleanest in that iteration. I.e. we don't need to xstrdup(), and the "fd" is always contained within map_loose_object_1(). We still have the "sometimes a path, sometimes I make a path from an oid" semantics though, but that seems unavoidable.
