Re: [PATCH] contrib/credential: Amend and harmonize Makefiles
- From
Thomas Uhle <thomas.uhle@mailbox.tu-dresden.de>
- Date
- Oct 10, 2025, 21:03 UTC
- 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:
Show 64 quoted lines
> 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.
Show 23 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)?
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.
Show 5 quoted lines
> 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.
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