This series is on top of https://lore.kernel.org/git/pull.1272.git.1656516334.gitgitgadget@xxxxxxxxx/; and shows that we can factor our the allocation macros used in cache.h and git-compat-util.h instead of defining replacements for them and alloc_nr() in xdiff/*. The journy towards doing so is slightly longer, but I think worth doing, since... On Fri, Jul 08 2022, Phillip Wood wrote: > On 07/07/2022 12:17, Ævar Arnfjörð Bjarmason wrote: >> I don't think it's more readable to carry code in-tree that's >> unreachable except when combined with code out-of-tree. I.e. this series >> leaves us with the equivalent of: >> ptr = xmalloc(...); >> if (!ptr) >> /* unreachable in git.git ... */ >> I don't think it's more readable to have code that rather trivial >> analysis will show goes against the "__attribute__((noreturn))" we're >> placing on our die() function. > > We're already in this situation. The code in xdiff is written to > handle allocation failures and we use an allocation function that dies > instead. This patch series does nothing to alter that situation. >[...] >> That just seems inviting a segfault or undefined/untested behavior >> (whether in the sense of "undefined by C" or "untested by git.git's >> codebase logic"). Everything around xmalloc() now assumes "never returns >> NULL", and you want to: >> * Make it return NULL when combined with out-of-tree-code > > No I do not want to alter the behavior of xmalloc() at all, that is > why this series does not alter the behavior of xmalloc() > [...] > I think there is an argument that we should change our xdiff wrapper > to use malloc() rather than xmalloc() so we're able to test the error > handling. That then begs the question as to how we actually get the > allocation functions to fail when they're being tested. I also think > that is an orthogonal change that could happen with or without this > patch series. I think part of what I was claiming upthread I was just confused about. I.e. yes we do use xdl_malloc() defined as xmalloc() already, what *is* true (and I didn't make this clear) was that your proposed series cements that further in place. But as 6/7 here notes 36c83197249 (xdiff: use xmalloc/xrealloc, 2019-04-11) left us with that state of affairs for expediency, but as we're really close to just "properly lib-ifying" xdiff I think we should just do that. At the time of 36c83197249 we had ~20 calls to xdl_malloc(), after your series we were at ~1/2 of that (including a callers that 36c83197249 explicitly punted on). This larger series is something we can do later, but I'm submitting it as a non-RFC in case there's consensus to pick it up on top. The sum of the two would be smaller if they were squashed together, but I haven't done that here. The 1/7 is an amendmend I suggested to 47be28e51e6 (Merge branch 'pw/xdiff-alloc-fail', 2022-03-09), since it was modifying some of the same lines of code... Ævar Arnfjörð Bjarmason (7): xdiff: simplify freeing patterns around xdl_free_env() git-shared-util.h: move "shared" allocation utilities here git-shared-util.h: add G*() versions of *ALLOC_*() xdiff: use G[C]ALLOC_ARRAY(), not XDL_CALLOC_ARRAY() xdiff: use GALLOC_GROW(), not XDL_ALLOC_GROW() xdiff: remove xdl_malloc() wrapper, use malloc(), not xmalloc() xdiff: remove xdl_free(), use free() instead cache.h | 75 ----------------------------- git-compat-util.h | 28 ++--------- git-shared-util.h | 115 +++++++++++++++++++++++++++++++++++++++++++++ xdiff/xdiff.h | 5 -- xdiff/xdiffi.c | 47 ++++++++---------- xdiff/xhistogram.c | 15 +++--- xdiff/xmacros.h | 23 --------- xdiff/xmerge.c | 57 ++++++++++++---------- xdiff/xpatience.c | 14 +++--- xdiff/xprepare.c | 60 +++++++++++++---------- xdiff/xutils.c | 33 ++++--------- xdiff/xutils.h | 2 - 12 files changed, 234 insertions(+), 240 deletions(-) create mode 100644 git-shared-util.h -- 2.37.0.913.g189dca38629
