On Fri, Apr 23, 2021 at 2:57 PM Jeff King <peff@xxxxxxxx> wrote: > > On Fri, Apr 23, 2021 at 10:24:06AM +0000, Han-Wen Nienhuys via GitGitGadget wrote: > > > Subject: refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn > > The subject says "read_raw_ref_fn", but the patch is touching > refs_resolve_ref_unsafe(). The former is an abstract type, and I didn't > dig to see the relationships, but I'll focus on the code change in the > patch. Well spotted. I reverted this part (I did glance over existing callers, and couldn't find anyone inspecting errno) > > A grep for EINVAL */*c reveals that no code inspects EINVAL after reading > > references. > > I don't think that's sufficient, for two reasons: > > - in general we try to be careful about forks and topics in flight, > which might end up with semantic conflicts. So we don't necessarily > assume that we can see all code, and prefer if any subtle changes > like this at least result in a compile failure (e.g., changing > function name or signature). In practice, this is balanced with how > likely such code is, how bad the breakage would be, what we're > gaining, etc. would you say this is warranted here? refs.h doesn't mention the word errno, so this behavior isn't documented at all. I also looked over the current callers of read_raw_ref, and outside of refs/*.c none seem to inspect errno. > - just because they are not looking for EINVAL specifically doesn't > mean they are not looking at errno at all (e.g., after calling > refs_resolve_ref_unsafe(), lock_ref_oid_basic() does so). So we have > to set errno to _something_ after the error. After your patch, we > don't set it at all for these error returns, and so we'll be left > with whatever junk was in errno from a previous unrelated syscall, > which could be very misleading. Since we have to set it to > something, EINVAL seems like a reasonable value. The function has several exit paths that don't set errno at all, so the result is kind of random anyway, but I can't see the code I don't have. I've updated the series, with some real progress to stamping out errno. Hope this pleases you better. -- Han-Wen Nienhuys - Google Munich I work 80%. Don't expect answers from me on Fridays. -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
