Re: [PATCH] contrib/credential: Amend and harmonize Makefiles

[Junio C Hamano <gitster@pobox.com>]

Thomas Uhle <thomas.uhle@mailbox.tu-dresden.de> writes:

> diff --git a/contrib/credential/libsecret/Makefile b/contrib/credential/libsecret/Makefile
> index 97ce9c9..8ee6cce 100644
> --- a/contrib/credential/libsecret/Makefile
> +++ b/contrib/credential/libsecret/Makefile
> @@ -1,17 +1,21 @@
>   # The default target of this Makefile is...
>   all::
>
> -MAIN:=git-credential-libsecret
> -all:: $(MAIN)
> -
> -CC = gcc
> -RM = rm -f
> -CFLAGS = -g -O2 -Wall
> -PKG_CONFIG = pkg-config
> -
>   -include ../../../config.mak.autogen
>   -include ../../../config.mak
>
> +prefix ?= /usr/local
> +gitexecdir ?= $(prefix)/libexec/git-core
> +
> +CC ?= gcc
> +CFLAGS ?= -g -O2 -Wall
> +PKG_CONFIG ?= pkg-config
> +INSTALL ?= install
> +RM ?= rm -f
> +
> +MAIN:=git-credential-libsecret
> +all:: $(MAIN)
> +
>   INCS:=$(shell $(PKG_CONFIG) --cflags libsecret-1 glib-2.0)
>   LIBS:=$(shell $(PKG_CONFIG) --libs libsecret-1 glib-2.0)
>
> @@ -22,7 +26,13 @@ OBJS:=$(SRCS:.c=.o)
>   	$(CC) $(CFLAGS) $(CPPFLAGS) $(INCS) -o $@ -c $<
>
>   $(MAIN): $(OBJS)
> -	$(CC) -o $@ $(LDFLAGS) $^ $(LIBS)
> +	$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) $(LIBS)
> +
> +install: $(MAIN)
> +	$(INSTALL) -d -m 755 $(DESTDIR)$(gitexecdir)
> +	$(INSTALL) -m 755 $< $(DESTDIR)$(gitexecdir)
>
>   clean:
> -	@$(RM) $(MAIN) $(OBJS)
> +	$(RM) $(MAIN) $(OBJS)
> +
> +.PHONY: all install clean


> diff --git a/contrib/credential/osxkeychain/Makefile b/contrib/credential/osxkeychain/Makefile
> index 0948297..b1d7c29 100644
> --- a/contrib/credential/osxkeychain/Makefile
> +++ b/contrib/credential/osxkeychain/Makefile
> @@ -1,19 +1,35 @@
>   # The default target of this Makefile is...
> -all:: git-credential-osxkeychain

Having the primary target name on this line very early in the file
has documentation value.

> -CC = gcc
> -RM = rm -f
> -CFLAGS = -g -O2 -Wall
> +all::
>
>   -include ../../../config.mak.autogen
>   -include ../../../config.mak
>
> -git-credential-osxkeychain: git-credential-osxkeychain.o
> -	$(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) \
> +prefix ?= /usr/local
> +gitexecdir ?= $(prefix)/libexec/git-core
> +
> +CC ?= gcc
> +CFLAGS ?= -g -O2 -Wall
> +INSTALL ?= install
> +RM ?= rm -f
> +
> +MAIN:=git-credential-osxkeychain
> +all:: $(MAIN)

What's the point of an extra $(MAIN) definition (not just here but
in the other Makefile as well)?  It may be slightly convenient to
write while the thing is simple and stays one-source-one-binary, but
programs including Makefiles are more often read than written, so we
should optimize them for readers.  I personally think this extra
indirection is hurting readability more than helping.

Other than that, yes, it is great to make these three or four
Makefiles look similar to allow readers compare and spot
differences.

Thanks.

> +
> +SRCS:=$(MAIN).c
> +OBJS:=$(SRCS:.c=.o)
> +
> +%.o: %.c
> +	$(CC) $(CFLAGS) $(CPPFLAGS) -o $@ -c $<
> +
> +$(MAIN): $(OBJS)
> +	$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) \
>   		-framework Security -framework CoreFoundation
>
> -git-credential-osxkeychain.o: git-credential-osxkeychain.c
> -	$(CC) -c $(CFLAGS) $<
> +install: $(MAIN)
> +	$(INSTALL) -d -m 755 $(DESTDIR)$(gitexecdir)
> +	$(INSTALL) -m 755 $< $(DESTDIR)$(gitexecdir)
>
>   clean:
> -	$(RM) git-credential-osxkeychain git-credential-osxkeychain.o
> +	$(RM) $(MAIN) $(OBJS)
> +
> +.PHONY: all install clean
>
> base-commit: 60f3f52f17cceefa5299709b189ce6fe2d181e7b