"Johannes Schindelin via GitGitGadget" <gitgitgad...@gmail.com> writes:
> --- > Makefile | 2 +- > compat/mingw.c | 5 ----- > git-compat-util.h | 4 +++- > compat/qsort.c => qsort.c | 2 +- > 4 files changed, 5 insertions(+), 8 deletions(-) > rename compat/qsort.c => qsort.c (97%) Quite pleasing. > diff --git a/compat/mingw.c b/compat/mingw.c > index 738f0a826a..77d4ef4d19 100644 > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -1229,11 +1229,6 @@ static int wenvcmp(const void *a, const void *b) > return _wcsnicmp(p, q, p_len); > } > > -/* We need a stable sort to convert the environment between UTF-16 <-> UTF-8 > */ > -#ifndef INTERNAL_QSORT > -#include "qsort.c" > -#endif > - Especially these ;-) > diff --git a/compat/qsort.c b/qsort.c > similarity index 97% > rename from compat/qsort.c > rename to qsort.c > index 7d071afb70..08f80eea09 100644 > --- a/compat/qsort.c > +++ b/qsort.c > @@ -1,4 +1,4 @@ > -#include "../git-compat-util.h" > +#include "git-compat-util.h" I however do not think this goes far enough. With a bit more effort would make the intention of the API more obvious. What we are saying now is that (1) some platforms do not even have qsort() (2) some codepaths do care the stability of sorting the former used to be the reason why we called our implementation git_qsort() and aliased qsort() to use git_qsort(). But now (2) is in the picture, we would probably want to make it more clear that our own implementation is not about having a sort function that behaves like qsort() and We want a lot more out of it (namely, stability). It probably is time to rename git_qsort() to git_stable_qsort() or something like that. Macros QSORT() and QSORT_S() are about having a convenient and less error-prone thin wrapper that retains an interface similar to qsort(3) and qsort_s(3) that developers are familiar with, so they can and should stay the same name, and it is perfectly fine if the former called qsort() that in turn calls git_stable_qsort() on some platforms. And that way, if/when the calling site of QSORT() (not the calling site of STABLE_QSORT()) needs a more performant but unstable sort implementation, we can safely give the most sensible name for it, which is git_qsort(). Other than that, the two patches look good to me. Thanks for working on this topic.
