Message ID | cover-00.17-00000000000-20210711T162803Z-avarab@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | refs API: get rid of errno setting entirely | expand |
On Sun, Jul 11, 2021 at 6:30 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > After starting a review of v5 of Han-Wen's where I was coming up with > squashes-on-top I continued with it and saw if I could get rid of > errno setting entirely in refs. thanks for taking this further! Is there a place where I can pull this code (applying patches is bothersome). It adds resolve_refs_unsafe_with_errno() to the public API in refs.h, and I think it has an ugly signature, but I suppose it's better than having the output be implicit through the global errno variable. I read over your patches, and they seem OK to me, modulo small comments I posted.
On Tue, Jul 13 2021, Han-Wen Nienhuys wrote: > On Sun, Jul 11, 2021 at 6:30 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> >> After starting a review of v5 of Han-Wen's where I was coming up with >> squashes-on-top I continued with it and saw if I could get rid of >> errno setting entirely in refs. > > thanks for taking this further! Is there a place where I can pull this > code (applying patches is bothersome). I've got it pushed to avar-review/pr-git-1012/hanwen/einval-sideband-v5 at https://github.com/avar/git.git : https://github.com/avar/git/tree/avar-review/pr-git-1012%2Fhanwen%2Feinval-sideband-v5 > It adds resolve_refs_unsafe_with_errno() to the public API in refs.h, > and I think it has an ugly signature, but I suppose it's better than > having the output be implicit through the global errno variable. Yeah it is a bit ugly, perhaps we should just end up using the old name once there's no callers of the old one? > I read over your patches, and they seem OK to me, modulo small > comments I posted. Thanks, don't have time to look it over in detail now, will reply later.
On Tue, Jul 13 2021, Ævar Arnfjörð Bjarmason wrote: > On Tue, Jul 13 2021, Han-Wen Nienhuys wrote: > >> On Sun, Jul 11, 2021 at 6:30 PM Ævar Arnfjörð Bjarmason >> <avarab@gmail.com> wrote: >>> >>> After starting a review of v5 of Han-Wen's where I was coming up with >>> squashes-on-top I continued with it and saw if I could get rid of >>> errno setting entirely in refs. >> >> thanks for taking this further! Is there a place where I can pull this >> code (applying patches is bothersome). > > I've got it pushed to avar-review/pr-git-1012/hanwen/einval-sideband-v5 > at https://github.com/avar/git.git : > https://github.com/avar/git/tree/avar-review/pr-git-1012%2Fhanwen%2Feinval-sideband-v5 > >> It adds resolve_refs_unsafe_with_errno() to the public API in refs.h, >> and I think it has an ugly signature, but I suppose it's better than >> having the output be implicit through the global errno variable. > > Yeah it is a bit ugly, perhaps we should just end up using the old name > once there's no callers of the old one? I looked at doing this, I don't think it's worth it. For my additional patches on top we need to somehow migrate all callers incrementally, so we could s/refs_resolve_ref_unsafe_with_errno/refs_resolve_ref_unsafe/g at the end, but it just seems like a lot more churn, and for the patches to be self-contained I'd need to re-indent all the argument lists again. It's a relatively obscure & scary API. I think it's OK that it's got a bit of a longer/more verbose name as a side-effect of the migration. And in any case with reftable I suspect that we'll end up refactoring further down the line sooner than later, i.e. to pass a "struct strbuf *err" or something, so getting it 100% right here seems premature.
On Wed, Jul 14, 2021 at 10:40 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > >> It adds resolve_refs_unsafe_with_errno() to the public API in refs.h, > >> and I think it has an ugly signature, but I suppose it's better than > >> having the output be implicit through the global errno variable. > > > > Yeah it is a bit ugly, perhaps we should just end up using the old name > > once there's no callers of the old one? > > I looked at doing this, I don't think it's worth it. .. > And in any case with reftable I suspect that we'll end up refactoring > further down the line sooner than later, i.e. to pass a "struct strbuf > *err" or something, so getting it 100% right here seems premature. SGTM.