From: Thomas Uhle <thomas.uhle@mailbox.tu-dresden.de>
Date: Fri, 10 Oct 2025 21:03:47 GMT
Subject: Re: [PATCH] contrib/credential: Amend and harmonize Makefiles
Message-ID: <c7cd0568-8161-205f-7f3e-ce63808dec8e@mailbox.tu-dresden.de>
In-Reply-To: <xmqqbjme8rs4.fsf@gitster.g>

On Fri, 10 Oct 2025, Junio C Hamano wrote:

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

I understand your point.  I guess it has not been done like this in the 
other three Makefiles because a variable ($MAIN here) was used instead 
of the executable file name itself and this variable is yet defined down 
below.


>> -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)?

I am only guessing.  git-credential-libsecret was renamed from 
git-credential-gnome-keyring somewhere in the past when the Gnome 
developers decided to dump libgnome-keyring in favour of libsecret.  So 
it could have been convenient to change only one line in the Makefile.


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

I was just using $(MAIN) as a variable name because this is the variable 
used in the other Makefile git-credential-libsecret.  Would 
$(GIT_CREDENTIAL_HELPER) be a better variable name?


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

My initial reason was just to add an install target rule because it is 
obviously missing.  And I wanted it to do in a similar way like the other 
two Makefiles (for git-subtree and git-contacts).  So I then ended up to 
reorder the lines.


> Thanks.

Thank you for the review.


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