Hi Junio, On Sat, 28 Sep 2019, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx> > 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. Okay, I will make that change. Ciao, Dscho
