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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 10, 2025, 19:45 UTC
Message-ID: <xmqqbjme8rs4.fsf@gitster.g>
In-Reply-To: <48d92664-41af-bb59-1844-7bb57f21924f@mailbox.tu-dresden.de>

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

Show 50 quoted lines

> 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

Show 7 quoted lines

> 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.

Show 20 quoted lines

> -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.

Show 24 quoted lines

> +
> +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