On Tue, Jul 06 2021, Han-Wen Nienhuys wrote: > On Tue, Jul 6, 2021 at 2:48 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: >> >> >> On Mon, Jul 05 2021, Han-Wen Nienhuys via GitGitGadget wrote: >> >> > v5 >> >> v3? > > I erroneously closed out the github PR I used for the first 2 versions > of this change series. (https://github.com/git/git/pull/1011) > >> > * address Ævar's comment; punt on clearing errno. >> > [...] >> > >> > Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx> >> > Reviewed-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx> >> > + Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> >> >> FWIW per Documentation/SubmittingPatches: >> >> . `Reviewed-by:`, unlike the other tags, can only be offered by the >> reviewer and means that she is completely satisfied that the patch >> is ready for application. It is usually offered only after a >> detailed review. >> >> It's not that I'm hard to please, but I can honestly say that I don't >> quite understand some parts of what you're gonig for, so that trailer is >> probably premature :) > > oh, ok. Removed them. > > Does that mean you'd have to repost my patchseries just to add the > reviewed-by header? It's fine to keep it. I just noted this (with a ":)") since I Reviewed-By is understood by Junio (I believe) to mean something closer to "this person has fully understood this code & endorses it". So far I was only confident to say I was on right-hand-side of that "&", give-or-take suggested changes :) With the below suggestions for a v4 you can keep my Reviewed-By if you'd like. >> To rephrase my comment on the v2 to hopefully better get at the >> point/question I had. >> >> It wasn't that I don't get why you wouldn't save/restore errno in >> general. >> >> It's that the pattern of doing so seems backwards to me. I.e. surely the >> goal here should be to one function at a time, and from the bottom-up, >> figure out where we rely on "errno" and convert that to a >> "failure_errno". >> >> Instead not even files_read_raw_ref() resets "errno = 0" at the end, so >> the errno /there/ can propagate upwards, and in this v3 we're not clearing it at all. > > I would really want errno to be an explicit output for more functions > in refs.h, but I don't have the time to see that through, and it's a > distraction from the reftable work, so I have changed the objective of > the patch series. > > I'm limiting myself to explicitly propagating errno values that are > used beyond error reporting. The EINVAL value for > parse_loose_ref_contents is a borderline case: it has an extra "if ()" > body in lock_raw_ref, but it's for an error message. > > This means that we should avoid clearing errno, because we'd otherwise > have to track down all the places where it's being put into > strerror(). The one case you found earlier is still there, and by not > clearing errno, I can keep this series smaller by not also having to > rework verify_lock/read_refs_full. I really think your v2 (or v4?, whatever) that set "errno = 0" plus eyeballing that grep command as we've done is justification enough to keep the "errno = 0". I just had the comment to the effect of "uh, oh, maybe we've missed more of these" on v2, but I think since then with your grep we can be fairly sure we haven't. I.e. the codepaths involved return non-zero anyway, so any breakage would be limited to ignoring that and directly checking errno, or using errno to format error messages. If there is something like that still it'll be fairly far up the stack, I think it's unplausible that there's anything left. We can do it in a follow-up series if you'd like, or not do it before reftable. I just think it's cleaner to be assured that we're past this particular hump, and are guaranteed not to gain future callers that rely on the errno in some subtle way (without the explicit pass-in variable, that is). > In general, it's frustrating to observe that the files backend is a > complex beast that nobody understands, but in order to replace it with > something more principled, I have to spend lots of time cleaning it > up. Yeah, it sucks. Or more specifically, that our "abstract" API isn't very "abstract", but basically a 1=1 mapping to the files backend. >> Having dug a bit further, it seems what you're doing, whether it's >> intentional or not, is relying on the parse_loose_ref_contents() setting >> EINVAL, but you clobber your *failure_errno with it whether it returned >> -1 or not. > > I've added a failure_errno argument parse_loose_ref_contents now. Great, it's not clear if you picked up the full diff-on-top but I think consistently using *failure_errno in files_read_raw_ref() instead of "errno" and the "*failure_errno = errno" at the end also makes the code much more readable (and allows for the removal of the "saved_errno" we don't want). By using "errno" itself for the body of the function it needs to be really carefully read to assure oneself that one of the functions it calls doesn't make a syscall, and even if we're assured of that now if one of them has a new syscall added in the future it might be clobbered at a distance.
