{"thread":{"id":"64295","subject":"[PATCH] contrib/credential: Amend and harmonize Makefiles","startedAt":"2025-10-10T17:30:34Z","lastAt":"2025-10-20T18:20:36Z","messageCount":8,"participants":["Thomas Uhle","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"528528","messageId":"48d92664-41af-bb59-1844-7bb57f21924f@mailbox.tu-dresden.de","threadId":"64295","inReplyTo":null,"subject":"[PATCH] contrib/credential: Amend and harmonize Makefiles","fromName":"Thomas Uhle","fromEmail":"thomas.uhle@mailbox.tu-dresden.de","sentAt":"2025-10-10T17:30:22Z","receivedAt":"2025-10-10T17:30:34Z","isPatch":true,"sender":{"key":"thomas.uhle@mailbox.tu-dresden.de","avatar":"https://avatars.githubusercontent.com/u/6583219?v=4"},"body":"Update these Makefiles to be in line with other Makefiles from contrib\nsuch as for contacts or subtree by making the following changes:\n* Make the default settings after including config.mak.autogen and\n   config.mak.\n* Add the missing $(CPPFLAGS) to the compiler command as well as the\n   missing $(CFLAGS) to the linker command.\n* Use a pattern rule for compilation instead of a dedicated rule for\n   each compile unit.\n* Add an install target rule.\n* Strip @ from $(RM) to let the clean target rule be verbose.\n* Define .PHONY for all special targets (all, clean, install).\n\nSigned-off-by: Thomas Uhle <thomas.uhle@mailbox.tu-dresden.de>\n---\n  contrib/credential/libsecret/Makefile   | 30 ++++++++++++++-------\n  contrib/credential/osxkeychain/Makefile | 36 ++++++++++++++++++-------\n  2 files changed, 46 insertions(+), 20 deletions(-)\n\ndiff --git a/contrib/credential/libsecret/Makefile b/contrib/credential/libsecret/Makefile\nindex 97ce9c9..8ee6cce 100644\n--- a/contrib/credential/libsecret/Makefile\n+++ b/contrib/credential/libsecret/Makefile\n@@ -1,17 +1,21 @@\n  # The default target of this Makefile is...\n  all::\n\n-MAIN:=git-credential-libsecret\n-all:: $(MAIN)\n-\n-CC = gcc\n-RM = rm -f\n-CFLAGS = -g -O2 -Wall\n-PKG_CONFIG = pkg-config\n-\n  -include ../../../config.mak.autogen\n  -include ../../../config.mak\n\n+prefix ?= /usr/local\n+gitexecdir ?= $(prefix)/libexec/git-core\n+\n+CC ?= gcc\n+CFLAGS ?= -g -O2 -Wall\n+PKG_CONFIG ?= pkg-config\n+INSTALL ?= install\n+RM ?= rm -f\n+\n+MAIN:=git-credential-libsecret\n+all:: $(MAIN)\n+\n  INCS:=$(shell $(PKG_CONFIG) --cflags libsecret-1 glib-2.0)\n  LIBS:=$(shell $(PKG_CONFIG) --libs libsecret-1 glib-2.0)\n\n@@ -22,7 +26,13 @@ OBJS:=$(SRCS:.c=.o)\n  \t$(CC) $(CFLAGS) $(CPPFLAGS) $(INCS) -o $@ -c $<\n\n  $(MAIN): $(OBJS)\n-\t$(CC) -o $@ $(LDFLAGS) $^ $(LIBS)\n+\t$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) $(LIBS)\n+\n+install: $(MAIN)\n+\t$(INSTALL) -d -m 755 $(DESTDIR)$(gitexecdir)\n+\t$(INSTALL) -m 755 $< $(DESTDIR)$(gitexecdir)\n\n  clean:\n-\t@$(RM) $(MAIN) $(OBJS)\n+\t$(RM) $(MAIN) $(OBJS)\n+\n+.PHONY: all install clean\ndiff --git a/contrib/credential/osxkeychain/Makefile b/contrib/credential/osxkeychain/Makefile\nindex 0948297..b1d7c29 100644\n--- a/contrib/credential/osxkeychain/Makefile\n+++ b/contrib/credential/osxkeychain/Makefile\n@@ -1,19 +1,35 @@\n  # The default target of this Makefile is...\n-all:: git-credential-osxkeychain\n-\n-CC = gcc\n-RM = rm -f\n-CFLAGS = -g -O2 -Wall\n+all::\n\n  -include ../../../config.mak.autogen\n  -include ../../../config.mak\n\n-git-credential-osxkeychain: git-credential-osxkeychain.o\n-\t$(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) \\\n+prefix ?= /usr/local\n+gitexecdir ?= $(prefix)/libexec/git-core\n+\n+CC ?= gcc\n+CFLAGS ?= -g -O2 -Wall\n+INSTALL ?= install\n+RM ?= rm -f\n+\n+MAIN:=git-credential-osxkeychain\n+all:: $(MAIN)\n+\n+SRCS:=$(MAIN).c\n+OBJS:=$(SRCS:.c=.o)\n+\n+%.o: %.c\n+\t$(CC) $(CFLAGS) $(CPPFLAGS) -o $@ -c $<\n+\n+$(MAIN): $(OBJS)\n+\t$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) \\\n  \t\t-framework Security -framework CoreFoundation\n\n-git-credential-osxkeychain.o: git-credential-osxkeychain.c\n-\t$(CC) -c $(CFLAGS) $<\n+install: $(MAIN)\n+\t$(INSTALL) -d -m 755 $(DESTDIR)$(gitexecdir)\n+\t$(INSTALL) -m 755 $< $(DESTDIR)$(gitexecdir)\n\n  clean:\n-\t$(RM) git-credential-osxkeychain git-credential-osxkeychain.o\n+\t$(RM) $(MAIN) $(OBJS)\n+\n+.PHONY: all install clean\n\nbase-commit: 60f3f52f17cceefa5299709b189ce6fe2d181e7b\n-- \n2.47.3\n"},{"id":"528532","messageId":"xmqqbjme8rs4.fsf@gitster.g","threadId":"64295","inReplyTo":"48d92664-41af-bb59-1844-7bb57f21924f@mailbox.tu-dresden.de","subject":"Re: [PATCH] contrib/credential: Amend and harmonize Makefiles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-10T19:45:31Z","receivedAt":"2025-10-10T19:45:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Uhle <thomas.uhle@mailbox.tu-dresden.de> writes:\n\n> diff --git a/contrib/credential/libsecret/Makefile b/contrib/credential/libsecret/Makefile\n> index 97ce9c9..8ee6cce 100644\n> --- a/contrib/credential/libsecret/Makefile\n> +++ b/contrib/credential/libsecret/Makefile\n> @@ -1,17 +1,21 @@\n>   # The default target of this Makefile is...\n>   all::\n>\n> -MAIN:=git-credential-libsecret\n> -all:: $(MAIN)\n> -\n> -CC = gcc\n> -RM = rm -f\n> -CFLAGS = -g -O2 -Wall\n> -PKG_CONFIG = pkg-config\n> -\n>   -include ../../../config.mak.autogen\n>   -include ../../../config.mak\n>\n> +prefix ?= /usr/local\n> +gitexecdir ?= $(prefix)/libexec/git-core\n> +\n> +CC ?= gcc\n> +CFLAGS ?= -g -O2 -Wall\n> +PKG_CONFIG ?= pkg-config\n> +INSTALL ?= install\n> +RM ?= rm -f\n> +\n> +MAIN:=git-credential-libsecret\n> +all:: $(MAIN)\n> +\n>   INCS:=$(shell $(PKG_CONFIG) --cflags libsecret-1 glib-2.0)\n>   LIBS:=$(shell $(PKG_CONFIG) --libs libsecret-1 glib-2.0)\n>\n> @@ -22,7 +26,13 @@ OBJS:=$(SRCS:.c=.o)\n>   \t$(CC) $(CFLAGS) $(CPPFLAGS) $(INCS) -o $@ -c $<\n>\n>   $(MAIN): $(OBJS)\n> -\t$(CC) -o $@ $(LDFLAGS) $^ $(LIBS)\n> +\t$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) $(LIBS)\n> +\n> +install: $(MAIN)\n> +\t$(INSTALL) -d -m 755 $(DESTDIR)$(gitexecdir)\n> +\t$(INSTALL) -m 755 $< $(DESTDIR)$(gitexecdir)\n>\n>   clean:\n> -\t@$(RM) $(MAIN) $(OBJS)\n> +\t$(RM) $(MAIN) $(OBJS)\n> +\n> +.PHONY: all install clean\n\n\n> diff --git a/contrib/credential/osxkeychain/Makefile b/contrib/credential/osxkeychain/Makefile\n> index 0948297..b1d7c29 100644\n> --- a/contrib/credential/osxkeychain/Makefile\n> +++ b/contrib/credential/osxkeychain/Makefile\n> @@ -1,19 +1,35 @@\n>   # The default target of this Makefile is...\n> -all:: git-credential-osxkeychain\n\nHaving the primary target name on this line very early in the file\nhas documentation value.\n\n> -CC = gcc\n> -RM = rm -f\n> -CFLAGS = -g -O2 -Wall\n> +all::\n>\n>   -include ../../../config.mak.autogen\n>   -include ../../../config.mak\n>\n> -git-credential-osxkeychain: git-credential-osxkeychain.o\n> -\t$(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) \\\n> +prefix ?= /usr/local\n> +gitexecdir ?= $(prefix)/libexec/git-core\n> +\n> +CC ?= gcc\n> +CFLAGS ?= -g -O2 -Wall\n> +INSTALL ?= install\n> +RM ?= rm -f\n> +\n> +MAIN:=git-credential-osxkeychain\n> +all:: $(MAIN)\n\nWhat's the point of an extra $(MAIN) definition (not just here but\nin the other Makefile as well)?  It may be slightly convenient to\nwrite while the thing is simple and stays one-source-one-binary, but\nprograms including Makefiles are more often read than written, so we\nshould optimize them for readers.  I personally think this extra\nindirection is hurting readability more than helping.\n\nOther than that, yes, it is great to make these three or four\nMakefiles look similar to allow readers compare and spot\ndifferences.\n\nThanks.\n\n> +\n> +SRCS:=$(MAIN).c\n> +OBJS:=$(SRCS:.c=.o)\n> +\n> +%.o: %.c\n> +\t$(CC) $(CFLAGS) $(CPPFLAGS) -o $@ -c $<\n> +\n> +$(MAIN): $(OBJS)\n> +\t$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) \\\n>   \t\t-framework Security -framework CoreFoundation\n>\n> -git-credential-osxkeychain.o: git-credential-osxkeychain.c\n> -\t$(CC) -c $(CFLAGS) $<\n> +install: $(MAIN)\n> +\t$(INSTALL) -d -m 755 $(DESTDIR)$(gitexecdir)\n> +\t$(INSTALL) -m 755 $< $(DESTDIR)$(gitexecdir)\n>\n>   clean:\n> -\t$(RM) git-credential-osxkeychain git-credential-osxkeychain.o\n> +\t$(RM) $(MAIN) $(OBJS)\n> +\n> +.PHONY: all install clean\n>\n> base-commit: 60f3f52f17cceefa5299709b189ce6fe2d181e7b\n"},{"id":"528537","messageId":"c7cd0568-8161-205f-7f3e-ce63808dec8e@mailbox.tu-dresden.de","threadId":"64295","inReplyTo":"xmqqbjme8rs4.fsf@gitster.g","subject":"Re: [PATCH] contrib/credential: Amend and harmonize Makefiles","fromName":"Thomas Uhle","fromEmail":"thomas.uhle@mailbox.tu-dresden.de","sentAt":"2025-10-10T21:03:47Z","receivedAt":"2025-10-10T21:03:57Z","isPatch":true,"sender":{"key":"thomas.uhle@mailbox.tu-dresden.de","avatar":"https://avatars.githubusercontent.com/u/6583219?v=4"},"body":"On Fri, 10 Oct 2025, Junio C Hamano wrote:\n\n> Thomas Uhle <thomas.uhle@mailbox.tu-dresden.de> writes:\n>\n>> diff --git a/contrib/credential/libsecret/Makefile b/contrib/credential/libsecret/Makefile\n>> index 97ce9c9..8ee6cce 100644\n>> --- a/contrib/credential/libsecret/Makefile\n>> +++ b/contrib/credential/libsecret/Makefile\n>> @@ -1,17 +1,21 @@\n>>   # The default target of this Makefile is...\n>>   all::\n>>\n>> -MAIN:=git-credential-libsecret\n>> -all:: $(MAIN)\n>> -\n>> -CC = gcc\n>> -RM = rm -f\n>> -CFLAGS = -g -O2 -Wall\n>> -PKG_CONFIG = pkg-config\n>> -\n>>   -include ../../../config.mak.autogen\n>>   -include ../../../config.mak\n>>\n>> +prefix ?= /usr/local\n>> +gitexecdir ?= $(prefix)/libexec/git-core\n>> +\n>> +CC ?= gcc\n>> +CFLAGS ?= -g -O2 -Wall\n>> +PKG_CONFIG ?= pkg-config\n>> +INSTALL ?= install\n>> +RM ?= rm -f\n>> +\n>> +MAIN:=git-credential-libsecret\n>> +all:: $(MAIN)\n>> +\n>>   INCS:=$(shell $(PKG_CONFIG) --cflags libsecret-1 glib-2.0)\n>>   LIBS:=$(shell $(PKG_CONFIG) --libs libsecret-1 glib-2.0)\n>>\n>> @@ -22,7 +26,13 @@ OBJS:=$(SRCS:.c=.o)\n>>   \t$(CC) $(CFLAGS) $(CPPFLAGS) $(INCS) -o $@ -c $<\n>>\n>>   $(MAIN): $(OBJS)\n>> -\t$(CC) -o $@ $(LDFLAGS) $^ $(LIBS)\n>> +\t$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) $(LIBS)\n>> +\n>> +install: $(MAIN)\n>> +\t$(INSTALL) -d -m 755 $(DESTDIR)$(gitexecdir)\n>> +\t$(INSTALL) -m 755 $< $(DESTDIR)$(gitexecdir)\n>>\n>>   clean:\n>> -\t@$(RM) $(MAIN) $(OBJS)\n>> +\t$(RM) $(MAIN) $(OBJS)\n>> +\n>> +.PHONY: all install clean\n>\n>\n>> diff --git a/contrib/credential/osxkeychain/Makefile b/contrib/credential/osxkeychain/Makefile\n>> index 0948297..b1d7c29 100644\n>> --- a/contrib/credential/osxkeychain/Makefile\n>> +++ b/contrib/credential/osxkeychain/Makefile\n>> @@ -1,19 +1,35 @@\n>>   # The default target of this Makefile is...\n>> -all:: git-credential-osxkeychain\n>\n> Having the primary target name on this line very early in the file\n> has documentation value.\n\nI understand your point.  I guess it has not been done like this in the \nother three Makefiles because a variable ($MAIN here) was used instead \nof the executable file name itself and this variable is yet defined down \nbelow.\n\n\n>> -CC = gcc\n>> -RM = rm -f\n>> -CFLAGS = -g -O2 -Wall\n>> +all::\n>>\n>>   -include ../../../config.mak.autogen\n>>   -include ../../../config.mak\n>>\n>> -git-credential-osxkeychain: git-credential-osxkeychain.o\n>> -\t$(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) \\\n>> +prefix ?= /usr/local\n>> +gitexecdir ?= $(prefix)/libexec/git-core\n>> +\n>> +CC ?= gcc\n>> +CFLAGS ?= -g -O2 -Wall\n>> +INSTALL ?= install\n>> +RM ?= rm -f\n>> +\n>> +MAIN:=git-credential-osxkeychain\n>> +all:: $(MAIN)\n>\n> What's the point of an extra $(MAIN) definition (not just here but\n> in the other Makefile as well)?\n\nI am only guessing.  git-credential-libsecret was renamed from \ngit-credential-gnome-keyring somewhere in the past when the Gnome \ndevelopers decided to dump libgnome-keyring in favour of libsecret.  So \nit could have been convenient to change only one line in the Makefile.\n\n\n> It may be slightly convenient to write while the thing is simple and\n> stays one-source-one-binary, but programs including Makefiles are more\n> often read than written, so we should optimize them for readers.  I\n> personally think this extra indirection is hurting readability more\n> than helping.\n\nI was just using $(MAIN) as a variable name because this is the variable \nused in the other Makefile git-credential-libsecret.  Would \n$(GIT_CREDENTIAL_HELPER) be a better variable name?\n\n\n> Other than that, yes, it is great to make these three or four\n> Makefiles look similar to allow readers compare and spot\n> differences.\n\nMy initial reason was just to add an install target rule because it is \nobviously missing.  And I wanted it to do in a similar way like the other \ntwo Makefiles (for git-subtree and git-contacts).  So I then ended up to \nreorder the lines.\n\n\n> Thanks.\n\nThank you for the review.\n\n\n>> +\n>> +SRCS:=$(MAIN).c\n>> +OBJS:=$(SRCS:.c=.o)\n>> +\n>> +%.o: %.c\n>> +\t$(CC) $(CFLAGS) $(CPPFLAGS) -o $@ -c $<\n>> +\n>> +$(MAIN): $(OBJS)\n>> +\t$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) \\\n>>   \t\t-framework Security -framework CoreFoundation\n>>\n>> -git-credential-osxkeychain.o: git-credential-osxkeychain.c\n>> -\t$(CC) -c $(CFLAGS) $<\n>> +install: $(MAIN)\n>> +\t$(INSTALL) -d -m 755 $(DESTDIR)$(gitexecdir)\n>> +\t$(INSTALL) -m 755 $< $(DESTDIR)$(gitexecdir)\n>>\n>>   clean:\n>> -\t$(RM) git-credential-osxkeychain git-credential-osxkeychain.o\n>> +\t$(RM) $(MAIN) $(OBJS)\n>> +\n>> +.PHONY: all install clean\n>>\n>> base-commit: 60f3f52f17cceefa5299709b189ce6fe2d181e7b\n"},{"id":"528539","messageId":"xmqqo6qe78lf.fsf@gitster.g","threadId":"64295","inReplyTo":"c7cd0568-8161-205f-7f3e-ce63808dec8e@mailbox.tu-dresden.de","subject":"Re: [PATCH] contrib/credential: Amend and harmonize Makefiles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-10T21:25:16Z","receivedAt":"2025-10-10T21:25:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Uhle <thomas.uhle@mailbox.tu-dresden.de> writes:\n\n>> Content-Type: text/plain; format=flowed; charset=\"US-ASCII\"\n\nPlease make sure your MUA does not corrupt whitespaces by sending\nyour e-mails with \"format=flowed\"\n\n$ git am ./+tu-credential-install\nwarning: Patch sent with format=flowed; space at the end of lines might be lost.\nApplying: contrib/credential: Amend and harmonize Makefiles\n\n"},{"id":"528571","messageId":"98592a42-71de-d86e-a727-32115615a82d@mailbox.tu-dresden.de","threadId":"64295","inReplyTo":"xmqqo6qe78lf.fsf@gitster.g","subject":"Re: [PATCH] contrib/credential: Amend and harmonize Makefiles","fromName":"Thomas Uhle","fromEmail":"thomas.uhle@mailbox.tu-dresden.de","sentAt":"2025-10-11T12:45:50Z","receivedAt":"2025-10-11T12:46:10Z","isPatch":true,"sender":{"key":"thomas.uhle@mailbox.tu-dresden.de","avatar":"https://avatars.githubusercontent.com/u/6583219?v=4"},"body":"On Fri, 10 Oct 2025, Junio C Hamano wrote:\n\n> Thomas Uhle <thomas.uhle@mailbox.tu-dresden.de> writes:\n>\n> >> Content-Type: text/plain; format=flowed; charset=\"US-ASCII\"\n>\n> Please make sure your MUA does not corrupt whitespaces by sending\n> your e-mails with \"format=flowed\"\n\nShall I simply resend the patch unchanged without \"format=flowed\" or has \nit to be a v2 patch then?\n\nShould I also rename $(MAIN) to $(GIT_CREDENTIAL_HELPER)?\n\nBest regards,\n\nThomas Uhle\n"},{"id":"528577","messageId":"xmqqikgl5nj3.fsf@gitster.g","threadId":"64295","inReplyTo":"98592a42-71de-d86e-a727-32115615a82d@mailbox.tu-dresden.de","subject":"Re: [PATCH] contrib/credential: Amend and harmonize Makefiles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-11T17:57:52Z","receivedAt":"2025-10-11T17:57:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Uhle <thomas.uhle@mailbox.tu-dresden.de> writes:\n\n> On Fri, 10 Oct 2025, Junio C Hamano wrote:\n>\n>> Thomas Uhle <thomas.uhle@mailbox.tu-dresden.de> writes:\n>>\n>> >> Content-Type: text/plain; format=flowed; charset=\"US-ASCII\"\n>>\n>> Please make sure your MUA does not corrupt whitespaces by sending\n>> your e-mails with \"format=flowed\"\n>\n> Shall I simply resend the patch unchanged without \"format=flowed\" or has \n> it to be a v2 patch then?\n\nThat was more to remind you before you actually need to send a\nsecond version (or another topic).  Of course, sending an email to\nyourself as practice to make sure it won't come as flowed text would\nbe a good idea, but straight resend is probably not needed.  Please\nfetch from my 'seen' branch from any of the public mirrors, and\ncheck what is queued as ac6152f0 (contrib/credential: Amend and\nharmonize Makefiles, 2025-10-10) is what you expected me to have\nwithout your mailer corrupting the patch contents.\n\n> Should I also rename $(MAIN) to $(GIT_CREDENTIAL_HELPER)?\n\nI do not think such a change would add any value.  \n\nIf the original did not use such an intermediate macro, adding to\nuse it may or may not have added value for \"not having to repeat\",\nbut it meant that now you have to repeat MAIN over and over, need to\nstill make sure you do not mistype it as MIAN, burdening the readers\nto hold in their head what $(MAIN) exactly referred to while reading\nthe file.  Makefile language does not offer warnings when you refer\nto an undefined macros, so using $(GIT_CREDENTIAL_HELPER) that is\nmore prone to mistyping than $(MAIN), while it makes it slightly\neasier to readers to follow, would not protect you from typos.  Not\nusing a macro at all and saying \"git-credential-helper\" when you\nmean it would have the same effect.\n\nSo it smells that viable choices are only three:\n\n * if the original did not use $(MAIN), leave them as-is and spell\n   the values (like \"git-credential-osxkeychain\") out.\n\n * if the original did use $(MAIN), leave them as-is, without rename\n   it to a longer and more typo-prone $(GIT_CREDENTIAL_HELPER).  Or\n\n * if the original did use $(MAIN), spell the values out instead.\n\nI prefer to do \"clean-up\" patches and \"functional\" patches\nseparately, and introduction of the install target is the latter, so\nperhaps leave all the changes to Makefile macro trick out of this\npatch and concentrate only on the new \"install\" target?  And then do\n\"clean-up\" using Makefile macro if you want, with merit of such a\nchange defended separately.\n\nThanks.\n"},{"id":"528578","messageId":"a13ed806-a04f-baae-ffa2-f1d12a0b3b0b@mailbox.tu-dresden.de","threadId":"64295","inReplyTo":"xmqqikgl5nj3.fsf@gitster.g","subject":"Re: [PATCH] contrib/credential: Amend and harmonize Makefiles","fromName":"Thomas Uhle","fromEmail":"thomas.uhle@mailbox.tu-dresden.de","sentAt":"2025-10-11T19:18:05Z","receivedAt":"2025-10-11T19:18:14Z","isPatch":true,"sender":{"key":"thomas.uhle@mailbox.tu-dresden.de","avatar":"https://avatars.githubusercontent.com/u/6583219?v=4"},"body":"On Sat, 11 Oct 2025, Junio C Hamano wrote:\n\n> [...]  Please\n> fetch from my 'seen' branch from any of the public mirrors, and\n> check what is queued as ac6152f0 (contrib/credential: Amend and\n> harmonize Makefiles, 2025-10-10) is what you expected me to have\n> without your mailer corrupting the patch contents.\n\nYes, it is correct.\n\n\n> [...]  So it smells that viable choices are only three:\n>\n> * if the original did not use $(MAIN), leave them as-is and spell\n>   the values (like \"git-credential-osxkeychain\") out.\n>\n> * if the original did use $(MAIN), leave them as-is, without rename\n>   it to a longer and more typo-prone $(GIT_CREDENTIAL_HELPER).  Or\n>\n> * if the original did use $(MAIN), spell the values out instead.\n\nSo the combination of the first and last option seems best to not keep \nthese differences within the same context (contrib/credential).  Do you \nagree?\n\n\n> I prefer to do \"clean-up\" patches and \"functional\" patches\n> separately, and introduction of the install target is the latter, so\n> perhaps leave all the changes to Makefile macro trick out of this\n> patch and concentrate only on the new \"install\" target?  And then do\n> \"clean-up\" using Makefile macro if you want, with merit of such a\n> change defended separately.\n\nDoesn't it make sense to clean up first and then add the install target \nrule in another patch if you prefer to split this patch?  That way I \ncould keep the first line of the commit message intact and the name of \nthe branch you have chosen would still match.  Moreover, it seems to me \nthat we are not that far from agreeing on a \"clean-up\" version.\n\n\n> Thanks.\n\nThanks for your comprehensive feedback.\n\nBest regards,\n\nThomas Uhle\n"},{"id":"529194","messageId":"0a61b0b3-365b-c198-6afd-f26fcd5a9c20@mailbox.tu-dresden.de","threadId":"64295","inReplyTo":"48d92664-41af-bb59-1844-7bb57f21924f@mailbox.tu-dresden.de","subject":"[PATCH v2] contrib/credential: Amend and harmonize Makefiles","fromName":"Thomas Uhle","fromEmail":"thomas.uhle@mailbox.tu-dresden.de","sentAt":"2025-10-20T18:20:22Z","receivedAt":"2025-10-20T18:20:36Z","isPatch":true,"sender":{"key":"thomas.uhle@mailbox.tu-dresden.de","avatar":"https://avatars.githubusercontent.com/u/6583219?v=4"},"body":"Update these Makefiles to be in line with other Makefiles from contrib\nsuch as for contacts or subtree by making the following changes:\n* Make the default settings after including config.mak.autogen and\n  config.mak.\n* Add the missing $(CPPFLAGS) to the compiler command as well as the\n  missing $(CFLAGS) to the linker command.\n* Use a pattern rule for compilation instead of a dedicated rule for\n  each compile unit.\n* Get rid of $(MAIN), $(SRCS) and $(OBJS) and simply use their values\n  such as git-credential-libsecret and git-credential-libsecret.o.\n* Strip @ from $(RM) to let the clean target rule be verbose.\n* Define .PHONY for all special targets (all, clean).\n\nSigned-off-by: Thomas Uhle <thomas.uhle@mailbox.tu-dresden.de>\n---\nChanges in v2:\n* Revert the changes in contrib/credentials/osxkeychain/Makefile that\n  have introduced $(MAIN), $(SRCS) and $(OBJS) as placeholders for\n  git-credential-osxkeychain, git-credential-osxkeychain.c and\n  git-credential-osxkeychain.o similar to\n  contrib/credential/libsecret/Makefile.  Instead replace $(MAIN),\n  $(SRCS) and $(OBJS) in contrib/credential/libsecret/Makefile.\n* Remove install target rule again to send this change as a separate\n  patch later as requested.\n* Adapt commit message.\n* Link to v1: https://lore.kernel.org/git/48d92664-41af-bb59-1844-7bb57f21924f@mailbox.tu-dresden.de/\n\n contrib/credential/libsecret/Makefile   | 29 ++++++++++++-------------\n contrib/credential/osxkeychain/Makefile | 21 +++++++++++-------\n 2 files changed, 27 insertions(+), 23 deletions(-)\n\ndiff --git a/contrib/credential/libsecret/Makefile b/contrib/credential/libsecret/Makefile\nindex 97ce9c9..7cacc57 100644\n--- a/contrib/credential/libsecret/Makefile\n+++ b/contrib/credential/libsecret/Makefile\n@@ -1,28 +1,27 @@\n # The default target of this Makefile is...\n-all::\n-\n-MAIN:=git-credential-libsecret\n-all:: $(MAIN)\n-\n-CC = gcc\n-RM = rm -f\n-CFLAGS = -g -O2 -Wall\n-PKG_CONFIG = pkg-config\n+all:: git-credential-libsecret\n\n -include ../../../config.mak.autogen\n -include ../../../config.mak\n\n+prefix ?= /usr/local\n+gitexecdir ?= $(prefix)/libexec/git-core\n+\n+CC ?= gcc\n+CFLAGS ?= -g -O2 -Wall\n+PKG_CONFIG ?= pkg-config\n+RM ?= rm -f\n+\n INCS:=$(shell $(PKG_CONFIG) --cflags libsecret-1 glib-2.0)\n LIBS:=$(shell $(PKG_CONFIG) --libs libsecret-1 glib-2.0)\n\n-SRCS:=$(MAIN).c\n-OBJS:=$(SRCS:.c=.o)\n-\n %.o: %.c\n \t$(CC) $(CFLAGS) $(CPPFLAGS) $(INCS) -o $@ -c $<\n\n-$(MAIN): $(OBJS)\n-\t$(CC) -o $@ $(LDFLAGS) $^ $(LIBS)\n+git-credential-libsecret: git-credential-libsecret.o\n+\t$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) $(LIBS)\n\n clean:\n-\t@$(RM) $(MAIN) $(OBJS)\n+\t$(RM) git-credential-libsecret git-credential-libsecret.o\n+\n+.PHONY: all clean\ndiff --git a/contrib/credential/osxkeychain/Makefile b/contrib/credential/osxkeychain/Makefile\nindex 0948297..c7d9121 100644\n--- a/contrib/credential/osxkeychain/Makefile\n+++ b/contrib/credential/osxkeychain/Makefile\n@@ -1,19 +1,24 @@\n # The default target of this Makefile is...\n all:: git-credential-osxkeychain\n\n-CC = gcc\n-RM = rm -f\n-CFLAGS = -g -O2 -Wall\n-\n -include ../../../config.mak.autogen\n -include ../../../config.mak\n\n+prefix ?= /usr/local\n+gitexecdir ?= $(prefix)/libexec/git-core\n+\n+CC ?= gcc\n+CFLAGS ?= -g -O2 -Wall\n+RM ?= rm -f\n+\n+%.o: %.c\n+\t$(CC) $(CFLAGS) $(CPPFLAGS) -o $@ -c $<\n+\n git-credential-osxkeychain: git-credential-osxkeychain.o\n-\t$(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) \\\n+\t$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) \\\n \t\t-framework Security -framework CoreFoundation\n\n-git-credential-osxkeychain.o: git-credential-osxkeychain.c\n-\t$(CC) -c $(CFLAGS) $<\n-\n clean:\n \t$(RM) git-credential-osxkeychain git-credential-osxkeychain.o\n+\n+.PHONY: all clean\n\nbase-commit: 60f3f52f17cceefa5299709b189ce6fe2d181e7b\n-- \n2.47.3\n"}]}