Re: [PATCH] compat: add memrchr()

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 15, 2010, 22:27 UTC
Message-ID: <7vy69zay7b.fsf@alter.siamese.dyndns.org>
In-Reply-To: <AANLkTi=YN41n-BnVNo3HsnxNxQNBX=Ev-upmM0N49uOZ@mail.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 5 quoted lines

> Maybe it should be a NO_* flag since that's what we use when we expect
> sanity, e.g. we have NO_REGEXP=, NO_CURL and NO_GETTEXT.
>
> Then you just need NO_MEMRCHR=UnfortunatelyYes entries in the big
> if/else block for those platforms that don't have it.

Hmmm, perhaps our naming convention have deteriorated over time.

I think one of the oldest one is NO_CURL, which started out as a way to say "I do not have to go over http and dumb http transport is of no use to me. I _might_ have curl library installed on this system, but it does not matter. No thanks, please do not build git with curl support". For some people, it also meant "Unfortunately I do not have a working curl library here, so I'll live with a git without http support".

On the other hand, NO_REGEX means "regrettably the platform implementation is not good enough for our codebase, and we do need to use a replacement implementation from compat/".

The difference is that it is not an option to build a less capable git that lacks features that depend on REGEX. But somehow we ended up using the same NO_ prefix, conflating the two. In that sense NO_GETTEXT is named correctly, I think, as we fall back on implementation (i.e. less capable git) that does not do i18n/l10n.

> But memrchr() is a GNU extension so it should probably be a HAVE_*. I
> don't know.

Yes, if we want to express that we positively rely on the existence of an extension, HAVE_* does sound more correct. It also is how the world with autoconf/configure works.

We might want to clean up the names of variables at some point, but from a quick glance things do not look very good.

Most NO_FOO are misnamed and should be !HAVE_FOO, but because our codebase tend to use more common features, switching from the way we currently do, i.e. assuming we have FOO and defining NO_FOO for oddball platforms, to the other way around of defining HAVE_FOO for platforms where we do have FOO and omit it for the ones that lack, may make the Makefile more cluttered.

But as I already said, switching to HAVE_FOO has one definite advantage; it is more conventional and would mesh a lot better with the way autoconf/configure would do things.

Here is a quick break-down of the current set I found from the 'master' branch.

* You decline to build, or are unable to build, git with...

  NO_CURL		transport based on libcurl e.g. HTTP.
  NO_ICONV		charset re-encoding support.
  NO_IPV6		IPV6
  NO_NSEC		nanosecond resolution file timestamps
  NO_POSIX_ONLY_PROGRAMS
  NO_PYTHON		anything that depends on Python
  NO_SYMLINK_HEAD	support for original layout where .git/HEAD was a symlink
  NO_SVN_TESTS		test that spends too much time on git-svn

* We work around the lack of the platform feature FOO without loss of
  features from git (i.e. should be !HAVE_FOO):

  NO_C99_FORMAT		SZ_FMT and friends
  NO_D_INO_IN_DIRENT	(struct dirent)d.d_ino
  NO_D_TYPE_IN_DIRENT	(struct dirent)d.d_type
  NO_HSTRERROR
  NO_INET_NTOP
  NO_INET_PTON
  NO_LIBGEN_H
  NO_MEMMEM
  NO_MKDTEMP
  NO_MKSTEMPS
  NO_MMAP
  HAVE_PATHS_H		#include <paths.h>
  NO_PERL_MAKEMAKER
  NO_PREAD
  NO_REGEX
  NO_R_TO_GCC_LINKER
  NO_SETENV
  NO_SOCKADDR_STORAGE
  NO_ST_BLOCKS_IN_STRUCT_STAT
  NO_STRCASESTR
  NO_STRLCPY
  NO_STRTOK_R
  NO_STRTOULL
  NO_STRTOUMAX
  NO_SYS_SELECT_H
  NO_TRUSTABLE_FILEMODE
  NO_UINTMAX_T
  NO_UNSETENV

Unfortunately the world is not so black-and-white.  There is another class

* Depending on platform features, we use different implementation with
  almost no observable difference in behaviour (perhaps except for
  performance)

  NO_PTHREADS
	Use of threads for better machine utilization

  NO_FAST_WORKING_DIRECTORY
	Reading from object database is faster than opening a file in the
	working directory and reading from it on this platform, so we do
	the former when we know the file in the working directory is clean.
        Perhaps !HAVE_FAST_WORKING_DIRECTORY.

And yet another that is completely unrelated:

  NO_SUBDIR